codeant-ai-for-open-source[bot] commented on code in PR #41174:
URL: https://github.com/apache/superset/pull/41174#discussion_r3483264826
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts:
##########
@@ -472,6 +472,7 @@ export function transformFormulaAnnotation(
return {
name,
id: name,
+ z: 10,
Review Comment:
**Suggestion:** Raising annotation series to a high `z` while interval/event
annotations remain interactive can cause those overlays to sit above data and
capture hover/click interactions first, which degrades data-point
tooltip/cross-filter interactions near annotations. Keep visual layering high
for labels but avoid making overlays the top interactive target. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Timeseries tooltips prefer annotations over data points.
- ⚠️ Cross-filter clicks near annotations hit overlays instead.
- ⚠️ Data exploration near annotated ranges becomes less precise.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render a Mixed Timeseries chart with interval/event/timeseries
annotations enabled; the
ECharts options for this visualization are built in
`plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:370-47`,
where each
visible annotation layer calls `transformFormulaAnnotation`,
`transformIntervalAnnotation`, `transformEventAnnotation`, or
`transformTimeseriesAnnotation` from `Timeseries/transformers.ts`.
2. For interval annotations, `transformIntervalAnnotation` constructs a
synthetic `line`
series with `markArea` and sets `z: 10` on the series (see
`Timeseries/transformers.ts:64-79` and `series.push({ ..., z: 10, markArea:
{ silent:
false, ... } })` at lines 16–33 in the 549–668 snippet), while
`markArea.silent` is
explicitly `false`, making the mark area interactive.
3. For event annotations, `transformEventAnnotation` similarly pushes a
`line` series with
`markLine` and `z: 10` (see `Timeseries/transformers.ts:39-88` and
`series.push({ id:
\`Event - ${name}\`, type: 'line', animation: false, z: 10, markLine: {
silent: false, ...
} })` at lines 112–120 in the 549–668 snippet), again with `silent: false`
so the mark
lines participate in hover and click handling.
4. When a user hovers or clicks near an annotated region on a timeseries or
mixed-timeseries chart, ECharts receives both the underlying data series
shapes (default
`z` around 0) and the overlaid markArea/markLine shapes (series `z: 10`);
because the
annotation series sit above the data due to higher `z` and `silent: false`,
pointer
interactions (tooltip hover, axis trigger, and click-based data
mask/cross-filter) are
resolved against the annotation overlays first, preventing underlying data
points from
being the primary interactive target near annotations.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f5595b774ad8452caa7ce457512b2a20&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f5595b774ad8452caa7ce457512b2a20&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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:** 475:475
**Comment:**
*Possible Bug: Raising annotation series to a high `z` while
interval/event annotations remain interactive can cause those overlays to sit
above data and capture hover/click interactions first, which degrades
data-point tooltip/cross-filter interactions near annotations. Keep visual
layering high for labels but avoid making overlays the top interactive target.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41174&comment_hash=e343f3be13fddf5c4f7739c8f055ce5c17f117eee861c642ddc3f7b3e03214c5&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41174&comment_hash=e343f3be13fddf5c4f7739c8f055ce5c17f117eee861c642ddc3f7b3e03214c5&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/tooltip.ts:
##########
@@ -107,18 +111,39 @@ export function getDefaultTooltip(refs: Refs) {
}
}
- // Position tooltip above cursor, or below if no space
- yPos = mouseY - TOOLTIP_POINTER_MARGIN - effectiveTooltipHeight;
+ // Mirror horizontal logic: position tooltip below cursor when in top
half of chart,
+ // above cursor when in bottom half. This prevents the tooltip from
covering annotation
+ // labels that appear at the top of the chart (markArea/markLine labels).
+ const chartHeight = divRect?.height || viewportHeight;
+ const cursorYInChart = canvasMousePos[1];
+ const isInTopHalfOfChart = cursorYInChart < chartHeight / 2;
- // The tooltip is overflowing past the top edge of the window
- if (yPos <= 0) {
- // Attempt to place the tooltip to the bottom of the mouse position
+ if (isInTopHalfOfChart) {
yPos = mouseY + TOOLTIP_POINTER_MARGIN;
- // The tooltip is overflowing past the bottom edge of the window
- if (yPos + effectiveTooltipHeight >= viewportHeight)
- // Place the tooltip a fixed distance from the top edge of the window
- yPos = TOOLTIP_OVERFLOW_MARGIN;
+ if (yPos + effectiveTooltipHeight >= viewportHeight) {
+ yPos = mouseY - TOOLTIP_POINTER_MARGIN - effectiveTooltipHeight;
+
+ if (yPos <= 0) {
+ yPos = TOOLTIP_OVERFLOW_MARGIN;
+ }
+ }
+ } else {
+ yPos = mouseY - TOOLTIP_POINTER_MARGIN - effectiveTooltipHeight;
+
+ if (yPos <= 0) {
+ yPos = mouseY + TOOLTIP_POINTER_MARGIN;
+
+ if (yPos + effectiveTooltipHeight >= viewportHeight) {
+ yPos = TOOLTIP_OVERFLOW_MARGIN;
+ }
+ }
+ }
+
+ // Clamp tooltip away from the top of the chart to avoid covering
annotation labels
+ // (markLine/markArea labels rendered at insideEndTop are within the
first ~40px)
+ if (divRect) {
+ yPos = Math.max(yPos, divRect.y + TOOLTIP_TOP_CLEARANCE);
Review Comment:
**Suggestion:** The top-clearance clamp is applied after the viewport
overflow checks, so it can push the tooltip back below the viewport on charts
positioned low on the page. Re-clamp `y` after applying top clearance (or clamp
once with both min/max bounds) so the final position still respects bottom
overflow limits. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Tooltips on low-positioned charts can extend off-screen.
- ⚠️ Users may be unable to read full tooltip content.
- ⚠️ Shared tooltip helper misapplies vertical overflow constraints.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Any ECharts-based visualization that uses the shared helper
`getDefaultTooltip(refs)`
from `plugins/plugin-chart-echarts/src/utils/tooltip.ts:34-152` (e.g.
`Timeseries/transformProps.ts:1010-1049`,
`Heatmap/transformProps.ts:362-368`,
`Bubble/transformProps.ts:246-24`) is rendered low in the viewport, so its
container
`divRect.y` is close to `document.documentElement.clientHeight`.
2. When the user hovers a data point near the bottom of the chart,
`getDefaultTooltip`'s
`position` callback computes `yPos` with overflow-aware logic (lines 121–141
in
`tooltip.ts`): it first positions the tooltip above or below the cursor
depending on
`isInTopHalfOfChart`, and if `yPos + effectiveTooltipHeight >=
viewportHeight`, it flips
the tooltip (from below to above or vice versa) and clamps `yPos` to
`TOOLTIP_OVERFLOW_MARGIN` if it would go off-screen.
3. After these bottom-overflow checks, the code applies a top-clearance
clamp at
`tooltip.ts:145-147`:
- `if (divRect) { yPos = Math.max(yPos, divRect.y +
TOOLTIP_TOP_CLEARANCE); }`
For charts whose container top (`divRect.y`) is already far down the
page, `divRect.y +
TOOLTIP_TOP_CLEARANCE` can be greater than the overflow-safe `yPos`
previously
computed, so `yPos` is increased.
4. Because there is no subsequent re-check of `yPos +
effectiveTooltipHeight` against
`viewportHeight` after this clamp, the final `yPos` can violate the earlier
overflow
constraints (e.g. `yPos` pushed to `490` with `effectiveTooltipHeight = 200`
and
`viewportHeight = 600` gives `490 + 200 = 690`), resulting in the tooltip
extending below
the viewport on low-positioned charts that rely on `getDefaultTooltip`,
leaving part of
the tooltip off-screen despite the earlier overflow handling.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a65fe2dbb6b1425996d25d05202a7b3d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a65fe2dbb6b1425996d25d05202a7b3d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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/utils/tooltip.ts
**Line:** 145:146
**Comment:**
*Logic Error: The top-clearance clamp is applied after the viewport
overflow checks, so it can push the tooltip back below the viewport on charts
positioned low on the page. Re-clamp `y` after applying top clearance (or clamp
once with both min/max bounds) so the final position still respects bottom
overflow limits.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41174&comment_hash=0ceccea1059af904047afe1241e5d3240ee88de0468e48b3f0920a6c7c51b831&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41174&comment_hash=0ceccea1059af904047afe1241e5d3240ee88de0468e48b3f0920a6c7c51b831&reaction=dislike'>👎</a>
--
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]
