Skip to content

Improve time chart hover for dense charts#2722

Open
alex-fedotyev wants to merge 3 commits into
mainfrom
alex/timechart-dense-hover
Open

Improve time chart hover for dense charts#2722
alex-fedotyev wants to merge 3 commits into
mainfrom
alex/timechart-dense-hover

Conversation

@alex-fedotyev

Copy link
Copy Markdown
Contributor

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:

  • Charts with more than 10 series now show a single-series hover tooltip: only the series nearest the cursor. Below that threshold the full sorted list is kept, so small charts are unchanged.
  • The hovered series is emphasized by a dedicated line drawn on top of the others, and the rest dim via one CSS class. Drawing the emphasized line as a separate overlay fixes a case where an emphasized line stayed hidden behind another line that shared its values: Recharts paints graphical items in mount order and ignores a reorder of existing children, so a freshly mounted overlay is the reliable way to bring one line to the front. Dimming the rest with a single class avoids re-rendering every line on each cursor move.
  • Synced charts (they share a time cursor) now highlight the same series by name: hovering a line on one chart surfaces that series on the others, and a chart that does not have that series shows only the shared time cursor instead of an unrelated row.

The tooltip row selection is pulled into a small pure selectTooltipRows helper 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 by ServiceName, a field with ~150 values) with the single-series hover tooltip, captured at a 1440x900 viewport in light and dark:

Single-series hover tooltip, light theme

Single-series hover tooltip, dark theme

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 new selectTooltipRows cases 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.
  • knip: no new unused exports.
  • Manual: a dashboard tile of count() grouped by ServiceName (~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.

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>
@vercel

vercel Bot commented Jul 23, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment Jul 23, 2026 7:32pm
hyperdx-storybook Ready Ready Preview, Comment Jul 23, 2026 7:32pm

Request Review

@changeset-bot

changeset-bot Bot commented Jul 23, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 0662097

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@hyperdx/app Patch
@hyperdx/api Patch
@hyperdx/otel-collector Patch

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 github-actions Bot added the review/tier-3 Standard — full human review required label Jul 23, 2026
@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

🟡 Tier 3 — Standard

Introduces new logic, modifies core functionality, or touches areas with non-trivial risk.

Why this tier:

  • Diff size: 368 production lines changed (Tier 2 max: < 250)

Review process: Full human review — logic, architecture, edge cases.
SLA: First-pass feedback within 1 business day.

Stats
  • Production files changed: 2
  • Production lines changed: 368 (+ 179 in test files, excluded from tier calculation)
  • Branch: alex/timechart-dense-hover
  • Author: alex-fedotyev

To override this classification, remove the review/tier-3 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@greptile-apps

greptile-apps Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR improves dense time-chart hover behavior.

  • Uses the number of visible series to select between single-series and full-list tooltips.
  • Keeps stacked-bar tooltips in full-list mode.
  • Adds a foreground overlay and CSS dimming to emphasize the hovered series.
  • Synchronizes hovered series by display name across charts in the same sync group.
  • Adds unit coverage for tooltip row selection and mode resolution.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failures remain in the fixes related to the previous review threads.

Important Files Changed

Filename Overview
packages/app/src/HDXMultiSeriesTimeChart.tsx Implements visible-series-aware tooltip modes, synchronized hover state, nearest-series selection, and overlay-based emphasis; the two previously reported failures are addressed.
packages/app/src/tests/HDXMultiSeriesTimeChart.test.ts Adds focused unit coverage for stacked-bar mode, visible-series thresholds, sorted tooltip rows, nearest-series selection, and synchronized followers.
packages/app/styles/HDXLineChart.module.scss Adds container-scoped opacity rules that dim base curves while preserving the emphasis overlay.
.changeset/timechart-dense-hover.md Documents the user-visible dense-chart hover behavior and the stacked-bar and legend-filter handling.

Reviews (3): Last reviewed commit: "Scope the cross-chart hovered-series sto..." | Re-trigger Greptile

Comment thread packages/app/src/components/DBTimeChart.tsx Outdated
Comment thread packages/app/src/components/DBTimeChart.tsx Outdated
@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 242 passed • 1 skipped • 1053s

Status Count
✅ Passed 242
❌ Failed 0
⚠️ Flaky 1
⏭️ Skipped 1

Tests ran across 4 shards in parallel.

View full report →

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

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 (selectTooltipRows, resolveTooltipMode) and findNearestSeriesKey are well unit-tested. The findings below are recommended before merge.

🟡 P2 -- recommended

  • packages/app/src/HDXMultiSeriesTimeChart.tsx:1346 -- activePointYByKeyRef is only ever written, never pruned, so after a legend filter-down (or a connectNulls gap) onMouseMove can pick a no-longer-drawn series as nearest; the publish effect then resolves it against the full lineData and broadcasts a hidden series to synced followers, and the chart's emphasized line and the tooltip row disagree.
    • Fix: Constrain the nearest-series candidate list (and the publish lookup) to the currently-visible dataKeys, and/or prune activePointYByKeyRef when visibleLineData changes.
    • correctness, julik-frontend-races, adversarial
  • packages/app/src/HDXMultiSeriesTimeChart.tsx:906 -- the clear-effect cleanup writes undefined to the shared store unconditionally, so when the cursor crosses from one synced chart to another the leaving chart's teardown can clobber the value the entered chart just published, blanking other followers' tooltips until the next mouse move (or flickering on every handoff).
    • Fix: Make the cleanup compare-and-clear — only reset the group value if it still equals the name this chart last published (track it in a ref).
    • adversarial, julik-frontend-races
  • packages/app/src/HDXMultiSeriesTimeChart.tsx:86 -- the new shared cross-chart store (setSharedHoveredSeries/useSharedHoveredSeries, its dedup guard and zero-subscriber cleanup), the publish/clear effects, and the tooltip's overlay-dedup (typedPayload) carry the most non-trivial new logic in the diff and have no coverage; they are module-private so no test can reach them.
    • Fix: Export the store (and the dedup) or move it to a small dedicated module with a reset hook, and add unit tests for multi-subscriber notification, the set-same-value no-op, and last-unsubscribe cleanup.
    • testing, maintainability
  • packages/app/src/HDXMultiSeriesTimeChart.tsx:268 -- intersecting HDXLineChartTooltipProps with Record<string, any> collapses every declared prop (including lineDataMap, containerRef, coordinate) to any, so a typo or renamed prop compiles silently and the pointerY: number | undefined annotation at :326 performs no real narrowing.
    • Fix: Type the recharts-injected props explicitly (e.g. TooltipProps<number, string> or a small hand-written shape) and drop the open index signature so the app-owned props stay checked.
    • kieran-typescript
🔵 P3 nitpicks (7)
  • packages/app/src/HDXMultiSeriesTimeChart.tsx:329 -- in single-series (dense) mode the tooltip still runs the all-series findNearestSeriesKey scan over ~150 keys plus an array allocation, but the result is only read behind !nearestOnly, so it is dead work on the exact hot path this PR targets.
    • Fix: Compute the all-series nearest key only in the !nearestOnly branch (or reuse singleSeriesKey).
  • packages/app/src/HDXMultiSeriesTimeChart.tsx:1350 -- "nearest series" is derived in three call sites (mouse-move emphasis, tooltip bolding, tooltip single-row) with the single→Infinity / all→30px cap policy duplicated across them, so a future policy change must be mirrored in three places that can silently diverge.
    • Fix: Extract one helper that encapsulates the cap-by-mode decision and call it from all three sites.
    • maintainability
  • packages/app/src/HDXMultiSeriesTimeChart.tsx:1001 -- the emphasis feature depends on an exact match between the bare hdx-emphasis-overlay class string in the TSX and the :global(...) selector in HDXLineChart.module.scss:77, with no compile-time link, so a rename on either side silently dims the emphasized line.
    • Fix: Define the class name as an exported constant referenced from both the TSX and (via comment) the SCSS.
    • maintainability
  • packages/app/src/HDXMultiSeriesTimeChart.tsx:234 -- selectTooltipRows returns [] in the single-series-hovered empty branch while the documented "render nothing" contract is null; the caller only bails on null, so an empty array would render an empty box (currently unreachable behind the caller's payload.length guard).
    • Fix: Return null for every render-nothing case, or have the caller also bail when rows.length === 0.
    • kieran-typescript
  • packages/app/src/HDXMultiSeriesTimeChart.tsx:131 -- TooltipPayload.name/value are declared required but assigned from the untyped recharts payload with no validation, and TooltipItem's name ?? p.dataKey fallback is dead code under that type, so the type and the defensive code contradict each other.
    • Fix: Mark recharts-omittable fields optional (name?, color?) and narrow on ingest so the fallback is justified.
    • kieran-typescript
  • packages/app/src/HDXMultiSeriesTimeChart.tsx:1317 -- clearing the shared hovered series depends solely on recharts firing onMouseLeave; if it does not (fast pointer exit, a modal/drawer opening over the chart, window blur) isHovered stays true and synced followers keep emphasizing a series the user is no longer pointing at.
    • Fix: Add a belt-and-suspenders pointerleave/window-blur handler on the container that forces isHovered false (or clears the store directly).
    • julik-frontend-races
  • packages/app/src/HDXMultiSeriesTimeChart.tsx:239 -- cross-chart matching keys on display name only, so two charts sharing a syncId with colliding names for unrelated data (e.g. both have an error series) will highlight semantically-different series on a follower, implying a correlation that does not exist.
    • Fix: Key the shared value on a more stable identity than display name, or document that name-based matching can collide across heterogeneous charts in one sync group.
    • adversarial

Reviewers (7): correctness, testing, maintainability, kieran-typescript, julik-frontend-races, performance, adversarial.

Testing gaps:

  • The shared cross-chart store, the publish/clear effects, and the tooltip overlay-dedup have no tests (they are module-private); a render/hook test for the chart-to-chart handoff and a legend-filter-then-move sequence would lock in the two P2 correctness fixes.
  • findNearestSeriesKey, selectTooltipRows, and resolveTooltipMode (including the 10-vs-11 boundary and the StackedBar exemption) are well covered — no gap there.

…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>
@alex-fedotyev

Copy link
Copy Markdown
Contributor Author

Both addressed in 99961da.

  • Stacked bars: the single-series tooltip is now gated to line/area charts. Stacked bars keep the full tooltip, since they do not capture the per-point Y that the nearest-series pick needs and a single row would hide the rest of the stack.
  • Threshold: it now counts the visible series (after legend selection and the draw cap) instead of the full series list, so filtering a dense chart down to a few series brings the full comparison back.

The decision moved into a pure resolveTooltipMode(displayType, visibleCount) helper next to the chart, with unit tests for the stacked-bar and visible-count cases.

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>
@alex-fedotyev

Copy link
Copy Markdown
Contributor Author

Deep-review findings addressed:

  • Cross-chart re-render scope (P2): the hovered-series store is now keyed by sync group in 0662097, so a hover only re-renders charts in the same syncId group. An unsynced chart uses its own id and is fully isolated, and a group's entries are dropped when its last subscriber unmounts.
  • Threshold on total vs visible count (P2): fixed in 99961da. The mode is derived from the visible/drawn series count via a pure resolveTooltipMode, gated to line/area charts (stacked bars keep the full tooltip).
  • Test gaps: added empty-payload coverage for selectTooltipRows, plus resolveTooltipMode unit tests for the stacked-bar and visible-count cases.

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 activePointYByKey ref not being cleared on legend deselection, and the one-frame emphasis gap on a cross-chart handoff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-3 Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant