codeant-ai-for-open-source[bot] commented on code in PR #36896:
URL: https://github.com/apache/superset/pull/36896#discussion_r2660458340
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts:
##########
@@ -454,40 +457,40 @@ export function transformIntervalAnnotation(
| MarkArea1DDataItemOption
| MarkArea2DDataItemOption
)[] = [
- [
- {
- name: label,
- ...(isHorizontal ? { yAxis: time } : { xAxis: time }),
- },
- isHorizontal ? { yAxis: intervalEnd } : { xAxis: intervalEnd },
- ],
- ];
+ [
+ {
+ name: label,
+ ...(isHorizontal ? { yAxis: time } : { xAxis: time }),
+ },
+ isHorizontal ? { yAxis: intervalEnd } : { xAxis: intervalEnd },
+ ],
+ ];
const intervalLabel: SeriesLabelOption = showLabel
? {
- show: true,
- color: theme.colorTextLabel,
+ show: true,
+ color: theme.colorTextLabel,
+ position: 'insideTop',
+ verticalAlign: 'top',
+ fontWeight: 'bold',
+ // @ts-ignore
+ emphasis: {
position: 'insideTop',
verticalAlign: 'top',
- fontWeight: 'bold',
- // @ts-ignore
- emphasis: {
- position: 'insideTop',
- verticalAlign: 'top',
- backgroundColor: theme.colorPrimaryBgHover,
- },
- }
+ backgroundColor: theme.colorPrimaryBgHover,
+ },
+ }
: {
- show: false,
- color: theme.colorTextLabel,
- // @ts-ignore
- emphasis: {
- fontWeight: 'bold',
- show: true,
- position: 'insideTop',
- verticalAlign: 'top',
- backgroundColor: theme.colorPrimaryBgHover,
- },
- };
+ show: false,
+ color: theme.colorTextLabel,
+ // @ts-ignore
+ emphasis: {
+ fontWeight: 'bold',
+ show: true,
Review Comment:
**Suggestion:** The `intervalLabel` fallback (when `showLabel` is false)
sets `emphasis.show: true`, which can make hidden labels appear in emphasized
states unexpectedly; remove the `show: true` from emphasis so labels remain
hidden while still configuring emphasis styling. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Keeping `emphasis.show: true` while `show: false` is contradictory — it can
cause labels that should be hidden to appear on emphasis/hover. Removing the
`show: true` is a safe, sensible behavior change that preserves "hidden by
default" semantics while keeping emphasis styling.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts
**Line:** 488:488
**Comment:**
*Possible Bug: The `intervalLabel` fallback (when `showLabel` is false)
sets `emphasis.show: true`, which can make hidden labels appear in emphasized
states unexpectedly; remove the `show: true` from emphasis so labels remain
hidden while still configuring emphasis styling.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -565,7 +575,7 @@ export default function transformProps(
convertInteger(xAxisTitleMargin),
);
- const { setDataMask = () => {}, onContextMenu } = hooks;
+ const { setDataMask = () => { }, onContextMenu } = hooks;
Review Comment:
**Suggestion:** Destructuring `hooks` directly can throw if `hooks` is
undefined; the current code does `const { setDataMask = () => { },
onContextMenu } = hooks;` which will raise a TypeError when `hooks` is
null/undefined. Default to an empty object when destructuring to avoid runtime
exceptions. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
const { setDataMask = () => {}, onContextMenu } = hooks || {};
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Destructuring from an undefined `hooks` will throw at runtime. In this PR
`hooks` comes from chartProps and may be absent in some call sites, so guarding
with `hooks || {}` is a safe, local hardening change that prevents a TypeError.
The improved code is syntactically correct and fixes a real potential runtime
crash.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
**Line:** 578:578
**Comment:**
*Possible Bug: Destructuring `hooks` directly can throw if `hooks` is
undefined; the current code does `const { setDataMask = () => { },
onContextMenu } = hooks;` which will raise a TypeError when `hooks` is
null/undefined. Default to an empty object when destructuring to avoid runtime
exceptions.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
