feat(funnels): add horizontal funnel step row components#60948
Merged
Conversation
Presentational building blocks for the horizontal funnel's flowing-row layout: GlyphColumn (the step-number rail), StepHeader (step title + metadata), and StepFooter (converted/dropped-off legend). Wired up by the flow-rows refactor that stacks on top of this. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 tasks
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
products/product_analytics/frontend/insights/funnels/FunnelBarHorizontalChart/StepFooter.tsx:15
Companion import needed when switching the prop type to `Noun`.
```suggestion
import { type Noun } from '~/models/groupsModel'
import type { FunnelsFilter } from '~/queries/schema/schema-general'
```
### Issue 2 of 2
products/product_analytics/frontend/insights/funnels/FunnelBarHorizontalChart/StepFooter.tsx:22
The inline shape `{ singular: string; plural: string }` duplicates the existing `Noun` interface from `~/models/groupsModel`. All six `funnelUtils` helpers this component calls (`formatConvertedCount`, `formatDroppedOffCount`, `getTooltipTitleForConverted`, etc.) already declare their parameter as `Noun`, so the type should be imported here too to keep the codebase DRY.
```suggestion
aggregationTargetLabel: Noun
```
Reviews (1): Last reviewed commit: "feat(funnels): add horizontal funnel ste..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } from 'scenes/funnels/funnelUtils' | ||
| import { ValueInspectorButton } from 'scenes/funnels/ValueInspectorButton' | ||
|
|
||
| import type { FunnelsFilter } from '~/queries/schema/schema-general' |
Contributor
There was a problem hiding this comment.
Noun.
Suggested change
| import type { FunnelsFilter } from '~/queries/schema/schema-general' | |
| import { type Noun } from '~/models/groupsModel' | |
| import type { FunnelsFilter } from '~/queries/schema/schema-general' |
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/product_analytics/frontend/insights/funnels/FunnelBarHorizontalChart/StepFooter.tsx
Line: 15
Comment:
Companion import needed when switching the prop type to `Noun`.
```suggestion
import { type Noun } from '~/models/groupsModel'
import type { FunnelsFilter } from '~/queries/schema/schema-general'
```
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
Author
There was a problem hiding this comment.
Fixed in 0144610 — imported Noun from ~/models/groupsModel.
| step: FunnelStepWithConversionMetrics | ||
| stepIndex: number | ||
| funnelsFilter: FunnelsFilter | null | undefined | ||
| aggregationTargetLabel: { singular: string; plural: string } |
Contributor
There was a problem hiding this comment.
{ singular: string; plural: string } duplicates the existing Noun interface from ~/models/groupsModel. All six funnelUtils helpers this component calls (formatConvertedCount, formatDroppedOffCount, getTooltipTitleForConverted, etc.) already declare their parameter as Noun, so the type should be imported here too to keep the codebase DRY.
Suggested change
| aggregationTargetLabel: { singular: string; plural: string } | |
| aggregationTargetLabel: Noun |
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/product_analytics/frontend/insights/funnels/FunnelBarHorizontalChart/StepFooter.tsx
Line: 22
Comment:
The inline shape `{ singular: string; plural: string }` duplicates the existing `Noun` interface from `~/models/groupsModel`. All six `funnelUtils` helpers this component calls (`formatConvertedCount`, `formatDroppedOffCount`, `getTooltipTitleForConverted`, etc.) already declare their parameter as `Noun`, so the type should be imported here too to keep the codebase DRY.
```suggestion
aggregationTargetLabel: Noun
```
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
Author
There was a problem hiding this comment.
Fixed in 0144610 — aggregationTargetLabel now uses the Noun type instead of the duplicated inline shape, matching the funnelUtils helpers.
Contributor
|
Size Change: 0 B Total Size: 80.9 MB ℹ️ View Unchanged
|
Replace the inline { singular; plural } shape with the existing Noun type
from groupsModel, matching the funnelUtils helpers this component calls.
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
|
✅ Visual changes approved by @sampennington — baseline updated in 13 changed, 4 new. |
17 updated Run: 86572e4f-46e1-427a-b436-119647e07e38 Co-authored-by: sampennington <56024559+sampennington@users.noreply.github.com>
github-actions
Bot
dismissed
their stale review
New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.
Contributor
|
🎭 Playwright report · View test results →
These issues are not necessarily caused by your changes. |
MattPua
pushed a commit
that referenced
this pull request
Jun 1, 2026
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
Base of a stack that refactors the horizontal funnel chart (#60843 stacks on this). Splitting the new presentational components out keeps the refactor PR focused on the rewiring rather than the component bodies.
Changes
Adds three pure presentational components for the horizontal funnel's flowing-row layout. They only depend on existing utilities (
SeriesGlyph,EntityFilterInfo,DuplicateStepIndicator,funnelUtils); they are not yet imported anywhere — the stacked refactor PR (#60843) wires them in.GlyphColumn— the left step-number rail (glyph + connector lines + optional-step decoration)StepHeader— step title, optional/duplicate indicators, avg conversion timeStepFooter— converted / dropped-off legend with person-modal buttonsHow did you test this code?
I'm an agent (Claude Code). These are leaf components extracted verbatim from the refactor branch; they compile against existing utilities. End-to-end behaviour is exercised by the stacked refactor PR (#60843) and its Storybook stories. No standalone tests added — they have no logic beyond layout.
Automatic notifications
🤖 Agent context
Authored with Claude Code (Opus 4.8). Extracted from the funnel flow-rows branch on request to shrink the refactor PR. Only these three components were separable —
SingleStepBarstays in #60843 because it imports the rewrittenfunnelBarHorizontalTransformsand the new hog-chartsvalueDomainconfig, which are coupled to the chart rewrite. The components are unused until #60843 wires them, so they'll show as dead until that PR merges.Requires human review — do not self-merge.