Repository navigation
fix(app): keep the heatmap tooltip from overflowing its tile - #3618
Conversation
The hover tooltip was positioned inside the chart container and clamped assuming a 90px height. With the drag hint it is taller, so it spilled past the tile, which clipped it and added scrollbars. Render it into the body with fixed positioning, like the line chart and annotation tooltips.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 2ffe44f The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
the
review/tier-2
🔵 Tier 2 — Low RiskSmall, isolated change with no API route or data model modifications. Why this tier:
Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns. Stats
|
Duplicate code analysisNo new duplicate code introduced by this PR. jscpd compared this branch against |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
PR Review
1 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 1 minor 1 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
Deep Review✅ No critical issues found. This is a focused, well-tested presentational change: the heatmap cell tooltip is portaled into 🟡 P2 — recommended
🔵 P3 nitpicks (2)
Reviewers (10): correctness, testing, maintainability, project-standards, kieran-typescript, julik-frontend-races, adversarial, previous-comments, agent-native, learnings-researcher. Testing gaps:
No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
Keep the heatmap container in state through a callback ref so the tooltip reads a DOM node at render, not a ref.
The tooltip is fixed to the viewport, so scrolling under a still cursor left it behind while the chart moved. Hide it on any scroll; the next mouse move shows it again at the right spot. Also cover the bottom clamp and right-half flip in tests.
E2E Test Results✅ All tests passed • 463 passed • 1 skipped • 1065s
Tests ran across 4 shards in parallel. |
| // The tooltip is fixed to the viewport, so a scroll under a still cursor | ||
| // would leave it behind; hide it until the next mouse move. | ||
| const hasHighlight = highlightedPoint != null; | ||
| useEffect(() => { |
There was a problem hiding this comment.
🔵 minor — Dismiss-on-scroll effect repeats the pinned tooltip's scroll listener in DBTimeChart
packages/app/src/components/DBTimeChart.tsx:246-262 already registers a capture-phase, passive window scroll listener to close a fixed-position chart tooltip when anything scrolls. This effect is a second copy of that logic. Move it into a shared hook next to useChartTooltipZIndex in components/charts/ChartTooltip.tsx, for example useDismissOnScroll(active, onDismiss, ignoreRef?), and use it in both places so the two body-portaled tooltips can't drift apart.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Leaving this out of this PR: sharing the listener means editing DBTimeChart.tsx and ChartTooltip.tsx, which this fix doesn't otherwise touch. Worth a follow-up.
Replaces the fixed 150px height clamp, which lifted the shorter tooltip (no drag hint) well above the cursor near the bottom of the window.
The tooltip now renders into the body, so the page object can't find it inside the chart container anymore. Look it up by test id instead.
| // The tooltip is fixed to the viewport, so a scroll under a still cursor | ||
| // would leave it behind; hide it until the next mouse move. | ||
| const hasHighlight = highlightedPoint != null; | ||
| useEffect(() => { |
There was a problem hiding this comment.
🔵 minor — Copies the scroll-dismiss listener that already exists for the pinned time-chart tooltip
PinnedTooltip in packages/app/src/components/DBTimeChart.tsx:246-262 already does the same thing: it adds a capture-phase, passive window scroll listener while a fixed, body-portaled chart tooltip is open, and dismisses the tooltip on scroll. Move that into one shared hook next to useChartTooltipZIndex/toViewportPoint in components/charts/ChartTooltip.tsx, e.g. useDismissOnScroll(open, onDismiss, ignoreRef?), and call it from both places. That way the rule for fixed tooltips is defined once and the two copies can't drift apart.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
Summary
Hovering the heatmap on a short tile (for example "Query Latency" on the ClickHouse dashboard) pushed the tooltip past the bottom of the tile. The tile cut the tooltip off and grew scrollbars, which shifted the panel while you moused around. The tooltip now renders above the page, the same way the line chart and release annotation tooltips do, so it is never clipped and the tile layout stays put.
Screenshots or video
ClickHouse dashboard, "Query Latency" tile, hovering a cell in the lowest row. Before, the tile body overflows (196px of content in 185px) and the tooltip is cut off. Screenshots are being added by hand;
ghcan't upload images.How to reproduce
Starting point:
yarn app:dev:local, then "Connect to Demo Server".On
main:On this branch:
How to test on Vercel preview
Preview routes: /clickhouse
Steps:
References