fix(insights): split aggregated bar bands per series#60488
Merged
Conversation
Contributor
|
Size Change: +5.42 kB (+0.01%) Total Size: 80.7 MB 📦 View Changed
ℹ️ View Unchanged
|
sampennington
changed the title
sampennington
force-pushed
the
sam/horizontal-bar-hover-color
branch
from
06a52d7 to
445e13a
Compare
sampennington
changed the title
sampennington
force-pushed
the
sam/horizontal-bar-hover-color
branch
from
445e13a to
9da7914
Compare
Contributor
|
👋 Visual changes detected for this PR. Review and approve in PostHog Visual Review If these changes are unexpected, they may be caused by a flaky test or a broken snapshot on master. Don't approve — rerun the job or wait for a fix. |
sampennington
changed the title
`buildTrendsBarAggregatedSeries` used the display label as the
d3.scaleBand domain key. Multiple trends series of the same event
(e.g. four \"\$pageview\" series with no breakdown) all collapsed onto
one band slot, so the sparse-stacked layout drew every series at the
same position and only the largest bar was visible. Hover
iter-by-band then picked highlights from non-visible series,
producing the \"purple bar turns blue\" effect.
Suffix the band key with the series identifier (action.order, or the
top-level `order` field formula-mode rows carry) so:
- Different series with the same label get their own band (the four
\$pageview case shows four rows).
- Breakdown rows of one series share the same id and keep collapsing
onto one band (formula + breakdown still renders as one row per
formula, with the existing overlap-on-larger-bar visual intact).
Display labels are returned separately and wired through the
horizontal axis tick formatter, with repeats suppressed so the shared
band doesn't render the same text on top of itself.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Horizontal bar charts with many bands previously crushed every row into a 5-pixel strip — tick labels and bars both became unreadable. Mirror the legacy ActionsHorizontalBar's `beforeFit` height behaviour: enforce a default 24 px per row, grow the chart's wrapper minHeight to `uniqueBands * minBandSize + chart margins`, and let the outer scene scroll vertically when content overflows. Expose `BarChartConfig.minBandSize` so consumers can override or opt out (pass `0`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
drawHover filtered candidate bars with `barContainsPointOnBandAxis`, which only checks the categorical axis. Stacked segments share that slot, so every series in the hovered row passed the check and all got darkened. The last-drawn (typically biggest) bar then painted its darkened color over every smaller segment — hovering a small green segment made the whole row read as the blue series. For stacked/percent, require full-rect containment so only the segment under the cursor is highlighted. Grouped keeps the band-axis check so cursor-above-bar still narrows correctly — covered by the existing `grouped layout still narrows when the cursor is above every bar` test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sparse-stacked aggregated bars overlap at the value-axis baseline: static paints largest-first, so each bar's visible slice is the region between the next-smaller bar's far edge and its own. The old band-axis-only hit-test darkened every overlapping bar, and the largest re-painted the whole row on hover. - Add `findVisibleStackedSegment` that walks every dataIndex sharing the hovered band and returns the last-painted (smallest) bar whose rect contains the cursor. - `drawHover` clips the highlight rect to that segment's slice so z-order doesn't shift on hover. - `BarTooltip` uses the same helper to put the visible segment at `seriesData[0]` and re-reads its value at its own dataIndex (the sparse cell at `ctx.dataIndex` would have been 0). - Tests cover small/mid/big visible slices with both series and value assertions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The aggregated TrendsBarChart was filtering tooltip rows by `data[ctx.dataIndex] !== 0` — for sparse-stacked overlap that picks whichever series owns the hovered dataIndex, which is rarely the segment the cursor is actually over. - Take `seriesData.slice(0, 1)` instead. BarTooltip already put the visible segment at index 0 (with its value re-read at its own dataIndex). - Add an integration test asserting the tooltip on a breakdown surfaces that breakdown's label and value, not 0 or the wrong row. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sampennington
force-pushed
the
sam/horizontal-bar-hover-color
branch
from
5971cb8 to
d6d93da
Compare
sampennington
commented
May 29, 2026
Contributor
Author
sampennington
left a comment
sampennington
left a comment
There was a problem hiding this comment.
🤖 Automated review
Verdict: Needs changes
Main blocker: the click path doesn't yet have the tooltip's fix applied. On formula+breakdown shapes the tooltip will say row X but the persons modal opens row Y. Same bug shape as the tooltip-value fix — same mirror needed in resolveClickedStackedSegment and the two trends consumers. After that lands, ship it: the rest is small.
Non-anchored findings
Critical: click routing in sparse-stacked overlap (3 callsites, all on unchanged lines)
frontend/src/lib/hog-charts/charts/BarChart/BarChart.tsx:594resolveClickedStackedSegment(hog-charts) — still callsresolveBarsAtCursorwithdataIndex: clickData.dataIndex(band index). For shared bands the visible segment lives at a different dataIndex. The rewrite toclickData.seriesis correct as far as it goes, butclickData.dataIndexis left pointing at the band. Apply the samefindVisibleStackedSegmentwalk and rewrite bothclickData.dataIndexandclickData.value.products/product_analytics/frontend/insights/trends/TrendsBarChart/TrendsBarChart.tsx:259onPointClick(code-reviewer, hog-charts) —handleTrendsBarAggregatedChartClick(clickData.dataIndex, …)dispatches the persons modal for whichever row owns the band index, not the visible segment. UseclickData.seriesIndex(the visible series's row index) instead, mirroring the tooltip-value fix.products/product_analytics/frontend/insights/trends/TrendsBarChart/TrendsBarChart.tsx:279tooltiponRowClick(code-reviewer, hog-charts) — same shape,datum.dataIndextraces back tocontext.dataIndex(band), not the visible segment.
Should Fix
- Missing regression test for the click path on shared bands (code-reviewer, hog-charts) —
TrendsBarChart.test.tsxcovers tooltip text only. Add apersonsModal.expectOpened(...)test for clicking the visible segment on a formula+breakdown band. - Missing tests for
BarChartConfig.minBandSize(hog-charts) — public API + DOM-shape change has zero coverage. At minimum:minBandSize: 0opts out; horizontal default of 24 grows wrapper; vertical never wraps. TooltipContext.seriesData[].valuesemantics drift fromctx.dataIndexfor stacked layouts (hog-charts) —BarTooltip.tsxnow re-reads value atvisibleDataIndex≠ctx.dataIndex. Consumers reading both (the trends adapter) see an undocumented contract mismatch. Either addentry.dataIndexper entry, or updatectx.dataIndexitself.
Performance / Suggestions
findVisibleStackedSegmentruns twice per mousemove (drawHover + BarTooltip). Same inputs, same answer. Consolidate via_privateonChartScalesor a hover-context cache. (efficiency, hog-charts)- Extract
clipBarToSliceandfindNextSmallerStackedExtenthelpers intoutils/bars-under-cursor.ts. (reuse, quality) - Hoist the static portion of the wrapper style to Tailwind (
flex flex-col flex-1), keep onlyminHeightinline. (react) - Extract
buildDedupedTickFormatterout ofTrendsBarChart.aggregatedConfig's memo. (quality) narrowSeriesByCursorhas 7 positional args — convert to options object. (quality)
Nits — comment cleanups (personal)
BarChart.test.tsx:21// Value scale [0, 100] (already nice).— narrates the next line.TrendsBarChart.test.tsx:409-410regression-history comment ("used to read at ctx.dataIndex…") — belongs in commit message.TrendsBarChart.test.tsx:420("Spike has aggregated_value 11 — largest…") — restates the assertion.
Positive
- Clean separation of
displayLabels(humans) and synthetic__seriesIdband keys (d3) — right seam. - New parameterized transform tests cleanly enumerate the band-keying rules.
xTickFormatterskip-repeats lives in the adapter, not the library — keeps hog-charts core formatter-agnostic.findVisibleStackedSegmentcomposesiterBarsAtCursorinstead of reimplementing it.- New
TrendsBarChart.test.tsxintegration test pins the original0-value regression with a real query path.
Reviewers: code-reviewer, react, personal, reuse, quality, efficiency, hog-charts. Inline comments below are tagged with the reviewer that raised them and the level of concern.
| ) | ||
|
|
||
| if (wrapperMinHeight === undefined) { | ||
| return chart |
Contributor
Author
There was a problem hiding this comment.
🤖 [hog-charts] · Should Fix
Returning bare chart vs <div>{chart}</div> based on wrapperMinHeight means toggling axisOrientation (or transitioning to/from no labels) changes the DOM tree shape, which causes React to unmount the inner Chart. That blows away the canvas, the scales memo, any pinned tooltip, and the hover-fade ref. Cheap fix: always wrap; pass minHeight: undefined when not needed.
Contributor
Author
There was a problem hiding this comment.
Fixed in a772a0247a8 — wrapper is now unconditional with flex flex-col flex-1 Tailwind classes, only minHeight is inline.
| // the wrapper grows and the outer scene scrolls. Margin matches DEFAULT_MARGINS + axis | ||
| // title floor in useChartMargins. | ||
| const HORIZONTAL_MIN_BAND_SIZE_DEFAULT = 24 | ||
| const HORIZONTAL_CHART_MARGIN_PX = 64 |
Contributor
Author
There was a problem hiding this comment.
🤖 [react+quality+hog-charts] · Should Fix
HORIZONTAL_MIN_BAND_SIZE_DEFAULT and HORIZONTAL_CHART_MARGIN_PX are module-level constants in spirit (SCREAMING_CASE, no closure refs) but live inside BarChartInner so they're re-bound every render. Hoist above BarChartInner alongside DEFAULT_BAR_SHADOW.
Also: the comment couples HORIZONTAL_CHART_MARGIN_PX = 64 to DEFAULT_MARGINS + axis-title floor in useChartMargins. That coupling isn't enforced — import / derive from the source rather than mirror the magic number.
Contributor
Author
There was a problem hiding this comment.
Fixed in a772a0247a8 — both hoisted to module scope. Exported X_AXIS_TITLE_MARGIN from useChartMargins and derived HORIZONTAL_CHART_MARGIN_PX = DEFAULT_MARGINS.top + DEFAULT_MARGINS.bottom + X_AXIS_TITLE_MARGIN so the value tracks.
| nextSmallerExtent = ext | ||
| } | ||
| } | ||
| } |
Contributor
Author
There was a problem hiding this comment.
🤖 [efficiency+reuse+hog-charts] · Performance
This nested loop re-walks the same labels × series set findVisibleStackedSegment just walked above, recomputing the BarRects it discarded. Cost: 2 · matching-dataIndices · |series| computeBarAtIndex calls per mousemove. For the typical aggregated bar shape it's small, but worth folding.
Have findVisibleStackedSegment return either all candidate extents or the second-largest in the same pass. drawHover then consumes one walk.
Contributor
Author
There was a problem hiding this comment.
Fixed in a772a0247a8 — findVisibleStackedSegment now returns nextSmallerExtent from the same pass it walks for the visible bar. drawHover consumes it directly; the second walk is gone.
| if (entry.series.key !== visibleKey) { | ||
| return entry | ||
| } | ||
| const raw = entry.series.data[visibleDataIndex!] |
Contributor
Author
There was a problem hiding this comment.
🤖 [quality] · Suggestion
Non-null assertion on visibleDataIndex! inside the .map closure, even though the outer if (line 110) already narrowed it. Hoist into a local const di = visibleDataIndex before the .map — TS narrows naturally and the ! disappears.
Contributor
Author
There was a problem hiding this comment.
Fixed in a772a0247a8 — hoisted to const di = visibleDataIndex before the .map.
| const raw = entry.series.data[ctx.dataIndex] | ||
| return typeof raw === 'number' && raw !== 0 | ||
| }), | ||
| seriesData: ctx.seriesData.slice(0, 1), |
Contributor
Author
There was a problem hiding this comment.
🤖 [hog-charts+code-reviewer] · Suggestion
seriesData.slice(0, 1) relies entirely on BarTooltip having put the cursor-visible segment at [0]. If a future refactor changes the ordering semantics, this silently selects the wrong row. Defensive option: filter on whatever key BarTooltip flags as visible, or pull the slice up into BarTooltip itself so the product layer stops second-guessing the invariant.
Contributor
Author
There was a problem hiding this comment.
Leaving the slice(0, 1) for now — BarTooltip is shared with other consumers that explicitly want all stacked entries (the stacked layout carries every visible series in the tooltip test locks that contract), so the slice has to live at the product layer. The new TrendsBarChart integration test (tooltip on a breakdown shows that breakdown row with its own value) plus the unit-level tooltip.seriesData[0].series.key assertions in BarChart.test.tsx lock the visible-segment-at-[0] invariant from both ends, so a future regression would surface. Open to a follow-up if we want a tagged isVisible field on SeriesDatum to make the contract explicit.
| // different series get distinct bands (breakdowns of one series still share a band). | ||
| const labels = visible.map((r, i) => { | ||
| const seriesId = r.action?.order ?? r.order ?? 0 | ||
| return `${displayLabels[i]}__${seriesId}` |
Contributor
Author
There was a problem hiding this comment.
🤖 [quality+hog-charts] · Suggestion
Two small things on this line:
-
The
__separator is a magic string. Any consumer that ever needs to parse a band key has to know it; if a realdisplayLabels[i]ever ends with__<n>matching another row's seriesId, two bands collide. Probability is low, but extract a namedBAND_KEY_SEPconstant or use a non-displayable delimiter. -
r.action?.order ?? r.order ?? 0falls back to0— which collides with a legitimateorder: 0(the first formula, the first action). For rows missing both fields the fallback collapses them onto each other. Either fall back to?? i(per-row unique) or comment why0is safe.
Contributor
Author
There was a problem hiding this comment.
Partial fix in a772a0247a8:
-
Separator: extracted
BAND_KEY_SEP = '\\u001f'(Unit Separator, U+001F). Never appears in user-facing labels. -
?? 0fallback: left as-is. Display labels already differ when bothaction.orderandorderare missing across rows, so the seriesId fallback collapsing to 0 is fine — the band key (displayLabel + sep + seriesId) stays unique. The only collision case is two rows with the same displayLabel and both missing series ids, which isn't a real shape we see from the trends data layer.?? iwould change the semantics for the formula+breakdown case where multiple rows legitimately shareorder: 0.
- Always wrap BarChart so axisOrientation toggle doesn't remount Chart - Hoist HORIZONTAL_MIN_BAND_SIZE_DEFAULT/HORIZONTAL_CHART_MARGIN_PX to module scope and derive the margin from DEFAULT_MARGINS + X_AXIS_TITLE_MARGIN - Fold the next-smaller-extent walk into findVisibleStackedSegment so drawHover walks the band once instead of twice - Drop the visibleDataIndex non-null assertion in BarTooltip - Extract BAND_KEY_SEP (U+001F) to name the synthetic-band-key separator - Critical: route clicks on shared bands through findVisibleStackedSegment and rewrite clickData.dataIndex + ctx.dataIndex so the persons modal / onPointClick / tooltip-row click open the segment the cursor is over, not whichever row owned the band index - Tests cover sparse-stacked click dispatch for small/mid/big visible slices Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Addressed review feedback in Critical click-routing fix. Should Fix items from the body.
Not done in this pass:
The 98 tests in |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Stacked-bar tooltips keyed only off band-axis containment, so a cursor in the row but beyond the longest bar still surfaced a tooltip. Resolve the visible segment under the cursor and bail when there is none. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Spec-driven .map fixtures for the same-label band-split tests — same assertions, fewer lines. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Focused frontend bug fix for tooltip/click routing in sparse-stacked horizontal bar charts. The older-commit review concern about the click path not being fixed is resolved in the current diff — resolveClickedStackedSegment now uses findVisibleStackedSegment with correct dataIndex rewriting. All other review issues are outdated with resolution replies, and the one unresolved current-head suggestion has a well-reasoned author response backed by tests. No backend, data model, API, security, or CI changes.
2 tasks
2 tasks
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
Two related bugs in the aggregated horizontal bar chart (
TrendsBarChart/ActionsBarValue, hog-charts path):\$pageviewrows with no breakdown) all surfacelabel="\$pageview". The band-key was the display label, so d3.scaleBand collapsed every series onto one band slot. The sparse-stacked layout drew them all at the same position; only the last-painted (typically largest) bar was visible.The intended overlap behavior for formula+breakdown stays correct (one band per formula, breakdown bars overlap by descending size inside it) — the bug only surfaces when different series happen to share a display label.
Changes
buildTrendsBarAggregatedSeriesnow returnsdisplayLabels(human strings, compare-suffix preserved) andlabels(synthetic band keys${displayLabel}__${seriesId}). Series id isaction.orderfor normal rows or the top-levelorderfor formula rows.TrendsBarChartreadsdisplayLabelsand feeds anxTickFormatterthat maps band keys back to human strings, returningnullfor repeats so band-shared rows don't draw the same tick text on top of itself.How did you test this code?
I'm an agent — needs a visual re-check on the affected setups.
pnpm exec jest products/product_analytics/frontend/insights/trends/TrendsBarChart/trendsBarChartTransforms.test.ts products/product_analytics/frontend/insights/trends/TrendsBarChart/TrendsBarChart.test.tsx— 52 pass. Includes two new transform tests that lock the rule in both directions:🤖 Agent context
Authored by Claude Opus 4.7 via Claude Code. Earlier branch attempts on this PR were the wrong shape:
ActionsHorizontalBar— covered a different code path the reporter wasn't using.BarChart.drawHoverto full-rect containment — treated the hover symptom but the bars themselves were still mis-grouped.This version distinguishes "same label because same series" (group) from "same label because different series" (split) using the series id the trends data layer already carries.