Skip to content

fix(charts): flip tall tooltips below the cursor when needed for space#117264

Merged
JoshuaKGoldberg merged 3 commits into
masterfrom
logs/fix-misplaced-chart-tooltips-group-by
Jun 11, 2026
Merged

fix(charts): flip tall tooltips below the cursor when needed for space#117264
JoshuaKGoldberg merged 3 commits into
masterfrom
logs/fix-misplaced-chart-tooltips-group-by

Conversation

@JoshuaKGoldberg

@JoshuaKGoldberg JoshuaKGoldberg commented Jun 9, 2026

Copy link
Copy Markdown
Member

Chart tooltip tops previously were a based on a max of either the existing position (- an offset) vs. the CHART_TOOLTIP_VIEWPORT_OFFSET. That works as long as there's room above the chart/cursor for all of it. But as seen in LOGS-851, if there isn't, then the tooltip weirdly hangs below where you'd expect it to.

Now, when a tooltip is too tall to fit above the cursor, it renders below the cursor instead, flipping the arrow up via the existing arrow-top class.

Before After
image image

Closes LOGS-851.

@linear-code

linear-code Bot commented Jun 9, 2026

Copy link
Copy Markdown

LOGS-851

@github-actions github-actions Bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jun 9, 2026
@JoshuaKGoldberg JoshuaKGoldberg force-pushed the logs/fix-misplaced-chart-tooltips-group-by branch from cbbed6a to 7bec210 Compare June 9, 2026 22:54
…o top

Chart tooltips position above the cursor, then clamp the top edge to the
viewport. For a single series the tooltip is short and always fits above, so
the clamp never bites. A group-by chart produces many series, making the
tooltip taller than the space above the cursor, so the clamp pinned it to the
top of the window and detached it from the chart.

When the tooltip is too tall to fit above the cursor, render it below the
cursor instead, flipping the arrow up via the existing `arrow-top` class.

Fixes LOGS-851
Co-Authored-By: Claude <noreply@anthropic.com>
@JoshuaKGoldberg JoshuaKGoldberg changed the title fix(charts): Flip tall tooltips below the cursor instead of pinning to top fix(charts): flip tall tooltips below the cursor when needed for space Jun 10, 2026
@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review June 10, 2026 14:48
@JoshuaKGoldberg JoshuaKGoldberg requested a review from a team as a code owner June 10, 2026 14:48
@JoshuaKGoldberg JoshuaKGoldberg requested a review from a team June 10, 2026 14:48

@scttcper scttcper left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not totally sure the tests are worth it

@JoshuaKGoldberg

Copy link
Copy Markdown
Member Author

👍 I can remove them, I am not passionate about keeping tricky JSDom mock testing.

Comment on lines +437 to +439
topPos = Number(pos[1]) + 20;
arrowOnTop = true;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The chart tooltip positioning logic lacks a check for the bottom viewport boundary, allowing tall tooltips to render off-screen when triggered near the bottom of a chart.
Severity: MEDIUM

Suggested Fix

Add a boundary check for the bottom of the viewport. After calculating topPos, check if topPos + tipHeight + chartBoundingRect.top exceeds window.innerHeight. If it does, adjust topPos to ensure the tooltip's bottom edge is clamped to the viewport boundary, preventing it from rendering off-screen.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: static/app/components/charts/components/tooltip.tsx#L437-L439

Potential issue: The tooltip positioning logic in the `position` function was updated to
prevent the tooltip from being clipped by the top of the viewport by flipping it below
the cursor. However, this new logic at `topPos = Number(pos[1]) + 20` does not include a
corresponding check for the bottom edge of the viewport. When a chart generates a tall
tooltip (e.g., from many series) and the user's cursor is near the bottom of the chart,
the tooltip can be positioned partially or entirely off-screen, making its content
inaccessible.

Did we get this right? 👍 / 👎 to inform future reviews.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 74ae800. Configure here.

// avoid tooltip from being cut off by the top edge of the window
CHART_TOOLTIP_VIEWPORT_OFFSET - chartBoundingRect.top
),
top: topPos,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flipped tooltip still clips viewport

Medium Severity

When the above-cursor placement would clip the viewport top, the tooltip moves to pos[1] + 20 with arrow-top, but there is no check that the new top clears CHART_TOOLTIP_VIEWPORT_OFFSET. If the chart’s bounding rect extends above the viewport, the below placement can remain clipped while the removed Math.max clamp used to keep it on screen.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 74ae800. Configure here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user has scrolled all the way down, they can scroll back up.

@JoshuaKGoldberg JoshuaKGoldberg merged commit b0cbabe into master Jun 11, 2026
77 checks passed
@JoshuaKGoldberg JoshuaKGoldberg deleted the logs/fix-misplaced-chart-tooltips-group-by branch June 11, 2026 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants