fix(insights): hog-charts hover regressions from #60187#60255
Merged
Conversation
- ValueLabels: subscribe to layout + hover separately so labels don't re-render on every mousemove, and skip the lift when multiple labels share a dataIndex (grouped bars) since hoverIndex can't disambiguate which bar the cursor is on. - BarChart: use full band width for the tooltip anchor extent. The sub-band width combined with band-center position.x was anchoring the tooltip at the wrong bar in grouped layout. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…band useChartDraw was only watching hoverIndex, so entering a bar from empty space inside the same band (hoverIndex unchanged) skipped the overlay redraw. BarChart resolves bar-vs-empty hits per-frame in drawHover, so it needs the position change to retrigger. The fade timer still only resets on hoverIndex change, and drawHover is O(series) not O(points), so the per-mousemove cost is bounded. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
band.bandwidth() includes the group scale's outer padding, so the right-edge anchor sat in empty space beside the rightmost bar. d3's group scale is symmetric within the band, so 2 * rightmostBar.right - band.bandwidth() gives the content extent (first-bar-left to last-bar- right) centered on the band center. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sampennington
changed the title
Contributor
|
🎭 Playwright didn't run on this PR — your changes touch code that could affect E2E behavior, but Playwright is opt-in via label now to keep CI cost down. Add the Most PRs don't need this. Real regressions still get caught on master and fix-forward. |
Contributor
|
Size Change: 0 B Total Size: 80.2 MB ℹ️ View Unchanged
|
sampennington
commented
May 27, 2026
Contributor
Author
sampennington
left a comment
sampennington
left a comment
There was a problem hiding this comment.
🤖 Automated review
Verdict: Needs changes
One real regression risk: the liftableIndices predicate in ValueLabels is too coarse — it correctly suppresses the lift for grouped bars (the case you tested) but also suppresses it for stacked bars and multi-series line charts, where multiple labels share a dataIndex at the same x and used to lift together as one column. Tighten the predicate to bucket by (dataIndex, x).
Other findings are perf notes (per-pixel hover effect cycle is acceptable but worth a follow-up for LineChart) and small clarity / reuse cleanups. No tests pin any of the three fixes — worth adding parameterized cases for the ValueLabels lift across layouts.
Non-anchored findings
- Stale doc comment (
frontend/src/lib/hog-charts/core/chart-context.ts:58, not in this diff): the docstring foruseChartLayoutstill lists "value labels" as an example of overlays that don't needhoverIndex— butValueLabelsnow reads it fromuseChartHover. Update the example list. - No tests pin any of the three fixes (hog-charts): regression fixes need failing-then-passing assertions. Suggested cases: tooltip extent in grouped mode (N=1 and N≥2 visible series), hover redraw within the same band after cursor moves from empty space onto a bar, and a parameterized
ValueLabelslift test across layouts (single bar lifts, stacked lifts, multi-series line lifts, grouped does NOT lift).
Reviewers: hog-charts, react, personal, reuse, quality, efficiency. Inline comments above are tagged with the reviewer that raised them and the level of concern.
| <> | ||
| {visible.map((c) => { | ||
| const isHovered = c.dataIndex === hoverIndex | ||
| const isHovered = c.dataIndex === hoverIndex && liftableIndices.has(c.dataIndex) |
Contributor
Author
There was a problem hiding this comment.
🤖 [hog-charts] · Critical
Over-broad — this gates the lift on count === 1 per dataIndex, but the comment's intent ("can't tell which bar in a grouped layout") is really about distinct x positions. Stacked bars and multi-series line charts also produce multiple candidates per dataIndex, but they share the same x (band center) and used to lift together as one visual column pre-PR. After this change, those layouts silently lose the lift entirely.
Tighten the predicate to bucket by (dataIndex, roundedX):
const liftableIndices = useMemo(() => {
const xsByIndex = new Map<number, Set<number>>()
for (const c of visible) {
const xs = xsByIndex.get(c.dataIndex) ?? new Set<number>()
xs.add(Math.round(c.x))
xsByIndex.set(c.dataIndex, xs)
}
const set = new Set<number>()
for (const [dIdx, xs] of xsByIndex) {
if (xs.size === 1) set.add(dIdx)
}
return set
}, [visible])Keeps the grouped-bar fix and restores stacked / multi-line lift behavior.
Contributor
Author
There was a problem hiding this comment.
Fixed in 4cd35f4abd6 — switched to (dataIndex, roundedX) bucketing per your snippet. Stacked + multi-series line will lift together again since they share x; grouped still doesn't lift.
| // Don't lift when multiple labels share a dataIndex (grouped bars) — we can't tell | ||
| // which bar the cursor is on from hoverIndex alone, so lifting all of them is worse | ||
| // than lifting none. | ||
| const liftableIndices = useMemo(() => { |
Contributor
Author
There was a problem hiding this comment.
🤖 [react+quality+reuse] · Suggestion
Two-pass Map → filter → Set could be a single pass — start with a candidate Set, demote on second sighting. Saves the temporary Map allocation on every recompute. Inputs are already small (post collision-avoidance), so this is minor — but if you take the bucket-by-x fix from the Critical comment above, that one needs the Map<number, Set<number>> anyway, so this collapses naturally.
Contributor
Author
There was a problem hiding this comment.
Folded into the critical fix above — the Map<number, Set<number>> is needed for the x-bucketing anyway, so the two-pass shape stays but the redundant Map→filter→Set chain is gone.
| }: ValueLabelsProps): React.ReactElement | null { | ||
| const { series, scales, labels, theme, resolvePositionValue, axis, hoverIndex } = useChart() | ||
| const { series, scales, labels, theme, resolvePositionValue, axis } = useChartLayout() | ||
| const { hoverIndex } = useChartHover() |
Contributor
Author
There was a problem hiding this comment.
🤖 [hog-charts+react] · Praise
The useChart() → useChartLayout() + useChartHover() split here is exactly the right call. Pre-change ValueLabels re-rendered on every mousemove (because useChart merges hover into layout); post-change it only re-renders when hoverIndex actually changes. Re-render isolation established by the prior PR is preserved and strengthened.
Contributor
Author
There was a problem hiding this comment.
Thanks — agreed, would be a good sweep to do across the other overlays (Crosshair, ReferenceLine, etc.) too.
| // series/labels/theme/drawHover are read via refs — see top of hook. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [overlayCtx, dimensions, scales, hoverIndex, hoverAnimationMs]) | ||
| }, [overlayCtx, dimensions, scales, hoverIndex, hoverPosition, hoverAnimationMs]) |
Contributor
Author
There was a problem hiding this comment.
🤖 [hog-charts+efficiency] · Performance
This effect now re-runs on every mousemove. The per-frame cost is bounded (drawHover is O(series), the cancelAnimationFrame/requestAnimationFrame collapses bursts, and the fade timer is properly gated on hoverIndex change so it doesn't restart) — so this is acceptable.
Two things worth knowing:
- LineChart pays for redraws that don't change pixels — its
drawHoveronly readshoverIndex, nothoverPosition, so per-pixel redraws repaint identical output. Cheap but wasteful. Follow-up: let chart types opt out of position-driven redraw (e.g. via a flag returned fromcreateScales). - The justification for re-arming the RAF every mouse pixel — "
drawHoveris O(series), not O(points)" — is true forBarChart. It's worth dropping a one-liner nearsetHover(or at the top ofuseChartDraw) noting that the cadence is now per-pixel so future readers don't bury expensive work indrawHover.
Contributor
Author
There was a problem hiding this comment.
Acknowledged — keeping the per-pixel re-arm for now; the existing comment block at L156-160 already calls out the cost contract (drawHover must stay O(series)) so I'll leave the inline note where it is. LineChart redraw-without-changes is a real wart but punting to a follow-up — the right fix is probably a per-chart-type flag returned from createScales like you suggested.
| } | ||
| } | ||
| // series/labels/theme/hoverPosition/drawHover are read via refs — see top of hook. | ||
| // hoverPosition is in the dep array (not the ref) so the overlay redraws when the |
Contributor
Author
There was a problem hiding this comment.
🤖 [quality] · Nit
Five-line comment for one dep-array entry — most of it could move to the commit message / PR description. Suggested one-liner that keeps the load-bearing bit:
// hoverPosition in deps: redraw on intra-band cursor moves; fade timer guarded by hoverIndex check above.Fine to keep as-is if you'd rather lean on the comment for readers who land here cold.
Contributor
Author
There was a problem hiding this comment.
Keeping the longer comment — per the project's CLAUDE.md the bar to keep a multi-line comment is "a genuinely tricky invariant" and the dep-array decision here qualifies: a future reader landing cold needs both "why position is in deps" and "why the fade doesn't restart on every mousemove" to safely modify this block. The one-liner loses the fade-timer reasoning.
| const domain = groupScale.domain() | ||
| if (domain.length > 0) { | ||
| const lastOffset = groupScale(domain[domain.length - 1]) ?? 0 | ||
| const contentExtent = 2 * (lastOffset + groupScale.bandwidth()) - d3Scales.band.bandwidth() |
Contributor
Author
There was a problem hiding this comment.
🤖 [hog-charts] · Performance
This identity (2 * rightmostBarRight − band.bandwidth() = content extent) holds only because core/scales.ts:352 calls .padding(groupPadding) (sets inner == outer). Anyone who later splits this into paddingInner / paddingOuter on the group scale will silently break tooltip positioning here.
A padding-asymmetry-safe formulation:
const firstOffset = groupScale(domain[0]) ?? 0
const lastOffset = groupScale(domain[domain.length - 1]) ?? 0
const contentExtent = lastOffset + groupScale.bandwidth() - firstOffsetUnder asymmetric padding the band-center anchor and the content center diverge anyway, so the single-extent tooltip model is already conceptually fragile there — but the formula above at least matches "width of rendered content" by construction. Alternative: leave the math as-is and drop a one-line comment in scales.ts:352 noting that tooltip math assumes symmetric padding.
Contributor
Author
There was a problem hiding this comment.
Fixed in 4cd35f4abd6 — switched to lastOffset + bandwidth - firstOffset per your snippet. Doesn't assume symmetric padding any more, and the formula reads as "width of rendered content" directly.
| // overshoots the rightmost bar's right edge — anchoring the tooltip in | ||
| // empty space. d3.scaleBand is symmetric, so 2 * rightmostBarRight - | ||
| // band.bandwidth() gives the content extent centered on the band center. | ||
| extent: () => { |
Contributor
Author
There was a problem hiding this comment.
🤖 [reuse+quality] · Suggestion
This inline extent block duplicates the "grouped-mode geometry on top of d3.scaleBand" logic already present in bandCenter (L45) / groupedBarCenter (L53) and in core/bar-layout.ts. Extract a sibling helper (e.g. groupedContentExtent(d3Scales) next to bandCenter/groupedBarCenter, or getBandContentExtent(d3Scales, isHorizontal, barLayout) in core/bar-layout.ts) so the three call sites that reach into d3Scales.group.domain() / .bandwidth() share one implementation.
At the call site this would collapse to extent: () => getBandContentExtent(d3Scales, isHorizontal, barLayout).
Contributor
Author
There was a problem hiding this comment.
Disagree on extracting for now. bandCenter, groupedBarCenter, and extent all read the same scales but solve three different geometric problems (band center, per-series center, content extent), and they have different signatures and call sites. A shared helper would either be a thin wrapper around one expression or a kitchen-sink barLayoutMath(scales, op) thing. Happy to revisit if a fourth call site lands.
| ? undefined | ||
| : ((barLayout === 'grouped' ? d3Scales.group?.bandwidth() : undefined) ?? | ||
| d3Scales.band.bandwidth()), | ||
| // Width of the rendered bar content within the band. In grouped mode |
Contributor
Author
There was a problem hiding this comment.
🤖 [quality] · Nit
The 2 * factor and the symmetry derivation are explained in the comment but invisible in the code. A named intermediate makes the symmetry self-documenting and shortens the comment to one line:
const bandCenter = d3Scales.band.bandwidth() / 2
const rightmostBarRight = lastOffset + groupScale.bandwidth()
const contentExtent = 2 * (rightmostBarRight - bandCenter)
Contributor
Author
There was a problem hiding this comment.
Indirectly fixed in 4cd35f4abd6 — the new formula uses firstOffset / lastOffset / contentExtent as named intermediates, and the 2 * factor is gone (the asymmetry-safe formula doesn't need it).
- ValueLabels: bucket liftableIndices by (dataIndex, x) so stacked bars and multi-series line labels still lift together — the prior count-based predicate disabled them too. - BarChart.extent: use last-bar-right minus first-bar-left so the formula doesn't silently rely on the group scale being symmetric. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Reviews (1): Last reviewed commit: "chore(insights): address review on hog-c..." | Re-trigger Greptile |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pure frontend chart rendering fix — all substantive review concerns (liftable-index predicate, padding-asymmetry formula) were addressed in the current head. Remaining inline comments are nits or suggestions the author declined with clear reasoning.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Follow-up to #60187. That PR added a band-edge tooltip anchor and a value-label hover lift, but introduced three hover regressions:
dataIndexonly, which is shared across series in the same band).hoverIndexdoesn't change in that move, and the overlay's effect only watchedhoverIndex— so the bar hit-test indrawHovernever re-ran.Changes
ValueLabels— readhoverIndexfromuseChartHover()(notuseChart()) so labels don't re-render on every mousemove, and only lift when exactly one label sits at the hovereddataIndex. Grouped layouts now lift nothing (cleanest until we plumb a series-key through the hover context).useChartDraw—hoverPositionback in the hover-effect dependency array. The fade timer only resets onhoverIndexchange, so re-running the effect per mousemove is cheap (drawHoveris O(series), not O(points)).BarChart.extent— compute2 * rightmostBarRight - band.bandwidth()in grouped mode.d3.scaleBandis symmetric within the band, so this gives the content extent (first-bar-left to last-bar-right) centered on the band center. Falls back toband.bandwidth()for stacked/percent (where the bar fills the band).How did you test this code?
I'm an agent. Automated tests:
BarChart.test.tsx(37 tests) andValueLabels.test.tsx(21 tests) — all passing.canvas-renderer.test.ts(56 tests) — all passing.FunnelHistogramChart,FunnelStepsBarChart,FunnelLineChart) — all passing.Manually verified by the human reviewer against a live funnel-steps chart: the lift no longer fires on the wrong bars, hover-from-canvas-empty-space onto a bar now shows the highlight, and the tooltip hugs the rendered bar instead of floating in the padding gap.
Publish to changelog?
🤖 Agent context
Authored by Claude Code (Opus 4.7) on top of the original PR #60187 (also agent-authored). Issues surfaced by the human reviewer testing in the browser. Each commit addresses one root cause separately so the bisect / revert surface stays small.
Considered (and rejected) plumbing
hoverSeriesKeythroughChartHoverContextso the lift could disambiguate within a grouped band. Decided against it for this PR — the hit-test lives insidedrawHover(a canvas-frame step, not a React event), and surfacing the result back into context would require a new state path. "Don't lift anything in grouped mode" is a simpler stop-gap and matches what the bar highlighter already does (it narrows to the cursor-targeted series, but value labels can't follow without that plumbing).