Improve time chart hover for dense charts#2722
Conversation
Charts with more than 10 series now collapse the hover tooltip to just the series nearest the cursor instead of listing every series, which is unreadable and never the one being pointed at. The emphasized line is redrawn by a dedicated overlay drawn on top, so it can no longer hide behind another line that shares its values, and the rest dim via a single CSS class instead of re-rendering every line on each cursor move. Synced charts highlight the same series by name: hovering a line on one surfaces it on the others. Extracts the tooltip row selection into a pure selectTooltipRows helper with unit coverage for the single, all, and synced-follower cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 0662097 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 |
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Review process: Full human review — logic, architecture, edge cases. Stats
|
Greptile SummaryThis PR improves dense time-chart hover behavior.
Confidence Score: 5/5The PR appears safe to merge. No blocking failures remain in the fixes related to the previous review threads. Important Files Changed
Reviews (3): Last reviewed commit: "Scope the cross-chart hovered-series sto..." | Re-trigger Greptile |
E2E Test Results✅ All tests passed • 242 passed • 1 skipped • 1053s
Tests ran across 4 shards in parallel. |
Deep Review✅ No critical issues found. No P0/P1: every traceable failure mode in this diff is a cosmetic hover-highlight glitch or a type/test/maintainability risk — none cause data loss, a happy-path crash, or a security exposure. The dense-tooltip helpers ( 🟡 P2 -- recommended
🔵 P3 nitpicks (7)
Reviewers (7): correctness, testing, maintainability, kieran-typescript, julik-frontend-races, performance, adversarial. Testing gaps:
|
…series Stacked bars do not capture the per-point Y positions the single-series tooltip uses to pick the series under the cursor, so a dense stacked bar fell back to the top series by value instead of the hovered segment; they now keep the full tooltip. The density threshold also counts the visible series (after legend selection and the draw cap) rather than the full series list, so filtering a dense chart down to a few series restores the full comparison. Moves the mode decision into a pure resolveTooltipMode helper next to the chart, with unit coverage for the stacked-bar and visible-count cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Both addressed in 99961da.
The decision moved into a pure |
The hovered-series store was a single module-level value that every mounted chart subscribed to, so hovering one chart re-rendered every chart on the page. Key it by sync group instead: synced charts (same recharts syncId) share a group, and an unsynced chart uses its own id, so a hover only re-renders the charts it actually affects. A group's entries are dropped when its last subscriber unmounts, so a page churning through many charts does not leak. Also adds empty-payload coverage for selectTooltipRows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Deep-review findings addressed:
Leaving the P3s as follow-ups since they are minor: the SCSS class-name coupling (no compile-time link is possible across the CSS-module boundary), the distance-cap policy living in two spots, the |
Summary
Time charts today render a hover tooltip that lists every series at the hovered time. On a dense chart (a group-by whose field has dozens of values) that list is taller than the chart itself and never the series you are actually pointing at.
This reworks the hover for dense charts:
The tooltip row selection is pulled into a small pure
selectTooltipRowshelper so the single, all, and synced-follower cases are unit tested without rendering Recharts in jsdom, matching the existing helper tests in this file.This is the first in a short series improving dense time chart interaction; it stands on its own. It keeps the existing line shape (monotone) unchanged, so existing charts look identical and the only visible change is the hover. It lands at tier 3 because the single-series tooltip, the emphasis overlay, and the cross-chart sync run through the same tooltip render and shared hovered-series store; splitting them would ship a half-working dense tooltip.
Screenshots
Dense chart (
count()grouped byServiceName, a field with ~150 values) with the single-series hover tooltip, captured at a 1440x900 viewport in light and dark:Test plan
yarn nx run @hyperdx/app:ci:lint(eslint under the 740 warning cap, tsc, stylelint): passes with 0 errors.yarn workspace @hyperdx/app jest HDXMultiSeriesTimeChart DBTimeChart: 40 tests pass, including 9 newselectTooltipRowscases covering all-series high-to-low sorting, the sort-a-copy contract, the single-series nearest pick with its two fallbacks, and the synced-follower name match plus its no-match null.count()grouped byServiceName(~150 series) against a local ClickHouse. Verified the single-series tooltip (light and dark above), the emphasized line drawing on top of overlapping lines, and that a chart without the hovered series shows only the shared cursor.[ui-states: allow] The hover applies to a populated multi-series chart; the empty, loading, and error states are unrelated to this change and unchanged.