fix(funnels): flow horizontal funnel rows to fit step text#60843
Merged
Conversation
Contributor
|
Size Change: 0 B Total Size: 80.9 MB ℹ️ View Unchanged
|
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
frontend/src/lib/hog-charts/core/types.ts:235-239
The JSDoc says `valueDomain` is "Ignored for `barLayout: 'percent'`", but `buildBarValueScale` checks `if (valueDomain)` before the `percent` branch, so `valueDomain` actually takes precedence over the percent domain rather than being ignored. This will mislead future callers who think they can safely pass both — they'll get the fixed domain, not the `[0, 1]` percent scale.
```suggestion
/** Fix the value-axis domain instead of deriving it from the data. Opt-in — when set, the
* value scale spans exactly this `[min, max]` with no `d3.nice()` rounding, so independent
* charts that share a logical scale (e.g. one single-bar chart per funnel step, all `0–100`)
* stay visually comparable. Takes precedence over `barLayout: 'percent'`'s built-in `[0, 1]`
* domain if both are supplied. */
valueDomain?: [number, number]
```
### Issue 2 of 2
frontend/src/lib/hog-charts/charts/BarChart/BarChart.tsx:272-284
`valueDomain` is an array (`[number, number]`), and `useMemo` uses `Object.is` equality, so a consumer who passes the tuple inline (e.g. `bars: { valueDomain: [0, 100] }` in JSX without memoizing the config object) will get a new reference on every render and cause the callback — and thus a full canvas redraw — to fire on every render. The current funnel consumer is safe because `CHART_CONFIG` is a module-level constant, but this is easy to misuse. Consider noting in the `valueDomain` JSDoc (or the `BarsConfig` comment) that the domain array must be a stable reference to avoid spurious redraws.
Reviews (1): Last reviewed commit: "fix(funnels): flow horizontal funnel row..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
272
to
+284
| _private: barChartPrivate, | ||
| } | ||
| }, | ||
| [yScaleType, barLayout, axisOrientation, stackedData, isHorizontal, divergingStack, maxBandRange, bandPadding] | ||
| [ | ||
| yScaleType, | ||
| barLayout, | ||
| axisOrientation, | ||
| stackedData, | ||
| isHorizontal, | ||
| divergingStack, | ||
| maxBandRange, | ||
| bandPadding, | ||
| valueDomain, |
Contributor
There was a problem hiding this comment.
valueDomain is an array ([number, number]), and useMemo uses Object.is equality, so a consumer who passes the tuple inline (e.g. bars: { valueDomain: [0, 100] } in JSX without memoizing the config object) will get a new reference on every render and cause the callback — and thus a full canvas redraw — to fire on every render. The current funnel consumer is safe because CHART_CONFIG is a module-level constant, but this is easy to misuse. Consider noting in the valueDomain JSDoc (or the BarsConfig comment) that the domain array must be a stable reference to avoid spurious redraws.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/lib/hog-charts/charts/BarChart/BarChart.tsx
Line: 272-284
Comment:
`valueDomain` is an array (`[number, number]`), and `useMemo` uses `Object.is` equality, so a consumer who passes the tuple inline (e.g. `bars: { valueDomain: [0, 100] }` in JSX without memoizing the config object) will get a new reference on every render and cause the callback — and thus a full canvas redraw — to fire on every render. The current funnel consumer is safe because `CHART_CONFIG` is a module-level constant, but this is easy to misuse. Consider noting in the `valueDomain` JSDoc (or the `BarsConfig` comment) that the domain array must be a stable reference to avoid spurious redraws.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
|
✅ Visual changes approved by @sampennington — baseline updated in 6 changed, 4 new. |
Contributor
|
🎭 Playwright report · View test results →
These issues are not necessarily caused by your changes. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Comment on lines
+381
to
386
| if (valueDomain) { | ||
| return d3.scaleLinear().domain(valueDomain).range(valueRange) | ||
| } | ||
| if (barLayout === 'percent') { | ||
| return d3.scaleLinear().domain([0, 1]).nice(tickCount).range(valueRange) | ||
| } |
Contributor
There was a problem hiding this comment.
The valueDomain parameter bypasses the barLayout === 'percent' check, contradicting the documented behavior.
According to the JSDoc in types.ts (line 239): "Ignored for barLayout: 'percent', which is already [0, 1]."
However, when both valueDomain and barLayout: 'percent' are set, the code returns early with the custom domain instead of using [0, 1].
Fix:
if (valueDomain && barLayout !== 'percent') {
return d3.scaleLinear().domain(valueDomain).range(valueRange)
}
if (barLayout === 'percent') {
return d3.scaleLinear().domain([0, 1]).nice(tickCount).range(valueRange)
}This ensures percent layouts always use [0, 1] domain regardless of valueDomain setting.
Suggested change
| if (valueDomain) { | |
| return d3.scaleLinear().domain(valueDomain).range(valueRange) | |
| } | |
| if (barLayout === 'percent') { | |
| return d3.scaleLinear().domain([0, 1]).nice(tickCount).range(valueRange) | |
| } | |
| if (valueDomain && barLayout !== 'percent') { | |
| return d3.scaleLinear().domain(valueDomain).range(valueRange) | |
| } | |
| if (barLayout === 'percent') { | |
| return d3.scaleLinear().domain([0, 1]).nice(tickCount).range(valueRange) | |
| } | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
sampennington
commented
Jun 1, 2026
Contributor
Author
There was a problem hiding this comment.
(Duplicate of the automated review just below — posted twice by mistake. See #60843 (review) for the full review with inline comments.)
sampennington
commented
Jun 1, 2026
Contributor
Author
sampennington
left a comment
sampennington
left a comment
There was a problem hiding this comment.
🤖 Automated review
Verdict: Ship it (with low-cost follow-ups)
Clean, well-decomposed refactor: the 211-line absolutely-positioned StepDecorations is replaced by flowing flex rows (GlyphColumn/StepHeader/StepFooter) + one canvas BarChart per step, and the new roundStackEnds pill-clip correctly rounds the stack's outer ends even for thin breakdown slivers. Canvas clip/shadow save-restore nesting is balanced, the per-band pill union is correct, and dep arrays are complete. Nothing blocks merge. The two worthwhile follow-ups: fix the valueDomain-vs-percent doc/code contradiction (1-liner) and add unit tests for the new library features.
Non-anchored findings
Should Fix
- Missing library tests for the new public features — none of
bar-scales.test.ts/canvas-renderer.test.ts/BarChart.test.tsxcovervalueDomain,roundStackEnds, orstackPillRects. Worth:buildBarValueScalewithvalueDomain(spans exactly[min,max], nonice());stackPillRectsper-dataIndexunion + zero-size filter + horizontal/vertical extension;clipToRoundedRectsempty-array no-op. (hog-charts) - Missing funnel transform tests for two edge cases in
funnelBarHorizontalTransforms.ts: a later step missing a breakdown variant still uses step-0's color/label (:78), andbuildFillerfloors at 0 when summed coverage exceeds 100% (:117). (code-reviewer)
Performance / Suggestion
SingleStepBaris notReact.memo'd, and the parent redefinesonSegmentClick/renderTooltipinline per step on every render (FunnelBarHorizontalChart.tsx:88-113). With one canvas chart per step now, any parent re-render (hover/modal/theme) re-renders all N charts. MemoizingSingleStepBar+ stabilizing the per-step handlers is the refactor's intended payoff. (react + code-reviewer + quality)- Hoist
stackPillRectsintobar-layout.tsnext tocomputeSeriesBars/computeBarTrackRect(pure geometry helpers already tested there) — makes it unit-testable instead of module-private toBarChart.tsx. (hog-charts + quality)
Suggestion (pre-existing)
StepHeader/StepFooter/GlyphColumnduplicate the funnel step title/legend/glyph-rail that already live inscenes/funnels/FunnelBarHorizontal/FunnelBarHorizontal.tsxandFunnelBarVertical/StepLegend.tsx. This triplication was inherited from the deletedStepDecorations.tsx, not introduced here — a future sharedFunnelStepConversionMetadatacomponent would consolidate all three. (reuse)
Nits
- Naming consistency:
highlightRadius(drawHover) vs the inlinestackPills.length > 0 ? 0 : barCornerRadius(drawStatic) express the same "square segments because the clip rounds the ends" intent — name both.LINE_CLASS→CONNECTOR_LINE_CLASSto distinguish from the optional-step decoration lines.BAR_CORNER_RADIUSis the one chart-config constant defined locally inSingleStepBar.tsxwhile the others moved to the transforms module. (quality) theme/fillerColormemos listisDarkModeOnas a dep without referencing it (proxy for "CSS vars changed") — pre-existing pattern, worth a one-line comment. (react)
Positive Observations
- Decomposition removes the fragile coupling between decoration layout and the chart's internal plot dimensions; rows now flow to fit wrapped text (the PR's goal). (code-reviewer)
buildBreakdownSegmentshoistsgetReferenceStepout of the inner breakdown loop — O(steps) instead of O(steps×breakdowns) calls. (efficiency)FunnelBarHorizontalTooltipnow takesstep/stepIndexas props instead of re-deriving fromcontext.dataIndex, removing an index-coupling footgun. (code-reviewer)- No branching logic in the test file; reference-step variations correctly use
it.each. (personal) - New
DefaultNarrow/BreakdownNarrowstories directly exercise the footer-wrapping behavior. (code-reviewer)
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.
| stackedSeries: Series[] | undefined, | ||
| valueDomain: [number, number] | undefined | ||
| ): D3YScale { | ||
| if (valueDomain) { |
Contributor
Author
There was a problem hiding this comment.
🤖 [hog-charts] · Should Fix
This returns the valueDomain scale before the barLayout === 'percent' branch, but the valueDomain docstring (types.ts:238) says it's "Ignored for barLayout: 'percent', which is already [0, 1]" — the code does the opposite. A caller setting both gets the custom domain, not [0,1]. Either move the percent check above this one, or fix the doc to say valueDomain overrides everything (incl. percent). The funnel uses 'stacked' so it isn't bitten today, but the JSDoc is a public-surface contract.
Same block also silently forces a linear scale and ignores yScaleType: 'log' / the diverging-stack bottom extension — worth a doc line on BarsConfig.valueDomain.
| renderTooltip, | ||
| onError, | ||
| }: SingleStepBarProps): JSX.Element { | ||
| const onPointClick = useMemo( |
Contributor
Author
There was a problem hiding this comment.
🤖 [react+code-reviewer] · Suggestion
This useMemo never hits its cache: its dep onSegmentClick is redefined inline on every parent render (FunnelBarHorizontalChart.tsx:88), so it recomputes each render and buys nothing while BarChart isn't memoized. Either drop the memo (plain ternary), or — for the real win — React.memo(SingleStepBar) + stabilize onSegmentClick/renderTooltip upstream so this memo becomes effective and sibling step charts stop re-rendering on every hover.
| } | ||
| // Match the resting bar's pill clip so the darker highlight rounds at the stack's outer | ||
| // ends instead of poking square corners past them. | ||
| const hoveredBandPills = roundStackEnds |
Contributor
Author
There was a problem hiding this comment.
🤖 [efficiency+quality] · Performance
When roundStackEnds is on, the hovered band's bars are iterated again here (a fresh iterBarsAtCursor pass + spread + .map) purely to build the pills, on every mousemove — items already iterated the same band above. Cost is O(series-in-band) so it's negligible for funnels, but the stacked path needs the full-stack span while the grouped/else path could reuse items' bars. Consider collecting bar into a local during the existing loop instead of re-iterating.
| ) | ||
|
|
||
| return ( | ||
| <div className="flex" key={step.order}> |
Contributor
Author
There was a problem hiding this comment.
🤖 [code-reviewer] · Suggestion
key={step.order} (carried over from the deleted StepDecorations) assumes order is unique across rendered steps. If two steps can ever share an order (worth confirming for unordered funnels), React reconciliation misbehaves. key={${step.order}-${stepIndex}} is a cheap safeguard.
sampennington
force-pushed
the
posthog-code/funnel-horizontal-flow-rows
branch
from
a65b059 to
3623c98
Compare
2 tasks
Contributor
Author
|
🔀 Split this PR in two. This one now contains just the horizontal funnel flow-rows refactor. The rounded pill bar-ends + cursor-following tooltip (and the |
sampennington
force-pushed
the
posthog-code/funnel-horizontal-flow-rows
branch
from
3623c98 to
6d24e67
Compare
2 tasks
Contributor
Author
|
📦 Now part of a 3-PR stack (this PR shrank from +484 to +308 by extracting the components):
|
Replace the absolutely-positioned StepDecorations overlay with flowing flex rows and one canvas BarChart per step, so each row grows to fit its wrapped step text instead of being crushed into a fixed-height band. Adds the hog-charts `bars.valueDomain` option so the independent per-step charts share a 0–100 scale. The now-unused StepDecorations file is left in place and removed in the stacked follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sampennington
force-pushed
the
posthog-code/funnel-horizontal-flow-rows
branch
from
6d24e67 to
149f70e
Compare
10 updated Run: d1ed45d8-8ccf-4dba-810e-3522ca3133f6 Co-authored-by: sampennington <56024559+sampennington@users.noreply.github.com>
Drop the no-op useMemo in SingleStepBar and shorten two over-long doc comments. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drop the redundant breakdown-narrow story (DefaultNarrow already proves the footer-wrap row growth), inline the single-use bar corner-radius const, and tighten two doc comments to one line. 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}
There was a problem hiding this comment.
An unresolved substantive comment on scales.ts — flagged by two bots and acknowledged by the author as "Should Fix" — is still present in the merged diff: valueDomain is checked before barLayout === 'percent', but the JSDoc explicitly says valueDomain is "Ignored for barLayout: 'percent'". The code does the opposite, introducing a misleading contract in a shared library.
buildBarValueScale checks valueDomain before the percent branch, so it takes precedence — the JSDoc claimed it was ignored for percent. Match the doc to the code. 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.
Clean frontend refactor with no showstoppers. The flagged valueDomain/percent JSDoc inconsistency was resolved in this very diff by updating the types.ts JSDoc to explicitly document override semantics. Logic changes are correct, tests cover the new per-step data shape, and the shared valueDomain constant ensures visual alignment across independent per-step charts.
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
The hog-charts horizontal ("top to bottom") funnel rendered every step as a band in one stacked canvas
BarChart, with the step header/footer text living in an absolutely-positioned HTML overlay (StepDecorations) placed against a uniform band scale — every row the same height. A row therefore can't grow to fit its own content, so when a footer wraps to two lines on a narrow chart it overflows its fixed slot and visually overlaps the next step's header.Changes
Restructured the funnel into a vertical flexbox column of per-step rows. Each row is just a bar plus some divs in normal document flow, so it grows to fit its own text and overlap becomes structurally impossible:
StepDecorations) and its JS position math.<BarChart>per step (pill rounding / track / hover preserved).GlyphColumn(numbered glyph + connecting lines),StepHeader(title, duplicate-step indicator, optional styling, avg time), andStepFooter(completed / dropped-off value-inspector buttons).Shared value scale
Every step's bar must share the same
0–100axis so a 28% bar and a 100% bar stay comparable. Independent per-step charts would otherwise each scale to their own max. Rather than rely on the filler summing to 100 (whichd3.nice()could round off and desync across charts), I added an opt-inBarChartConfig.bars.valueDomain?: [number, number]to hog-charts — a fixed linear domain with nonice()— and pass[0, 100]from the funnel. It's threadedBarChart → createBarScales → buildBarValueScaleand defaults to off, so existing charts are unaffected. The per-step filler segment is retained as the visible drop-off track and the drop-off click target.Preserved: color resolution, single-series and multi-breakdown cases, all persons-modal interactions (converted, drop-off, per-breakdown), the cursor tooltip, and the
inCardView/showPersonsModalinteractivity gating.How did you test this code?
I'm an agent (Claude Code, Opus 4.8). This sandbox had no installed
node_modules, so I could not run jest, Storybook, or a path-alias-awaretsclocally — these need to run in CI. I updatedfunnelBarHorizontalTransforms.test.tsto the new per-step output shape and added narrow-width Storybook stories (DefaultNarrow,BreakdownNarrow) so the wrapping fix is visible in the visual-regression suite. The intended visual checks (a single-line footer stays short, a wrapping footer grows only its own row, no cross-step overlap, no regression at normal width) still need a human to confirm against the rendered stories.Automatic notifications
🤖 Agent context
Authored with Claude Code (Opus 4.8). Key decisions:
valueDomainconfig over forcing the domain via a filler-that-sums-to-100, becaused3.nice()on a float-imperfect[0, ~100]could round the max up and desync the scale between independent per-step charts. Kept the filler purely as the drop-off track / click target.<div>bars — staying on the canvas library was the point of the prior rework. The header/footer/glyph moved to normal-flow divs since that's where the overlap problem lived.Follow-up to #60762. Agent-assisted; requires human review. Visual regression snapshots will change and need eyeballing.