fix(funnels): scope persons modal, recordings, and display to single breakdown value#53819
Open
sampennington wants to merge 5 commits intomasterfrom
Open
fix(funnels): scope persons modal, recordings, and display to single breakdown value#53819sampennington wants to merge 5 commits intomasterfrom
sampennington wants to merge 5 commits intomasterfrom
Conversation
… clicks When clicking a funnel breakdown segment to view session recordings, the breakdown property filter was not included in the recording filters, causing recordings from all breakdown values to appear instead of just the selected segment. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Size Change: +2.76 kB (0%) Total Size: 128 MB ℹ️ View Unchanged
|
Contributor
|
🎭 Playwright report · View test results →
These issues are not necessarily caused by your changes. |
…ntal funnel When a horizontal funnel has a breakdown filter that resolves to a single visible value, clicking the bar opened the persons modal unfiltered. The non-breakdown rendering branch was used for any single-entry nested_breakdown, which lost the breakdown context. Now treat single-entry steps as breakdowns when there's a real breakdown value, so clicks route through openPersonsModalForSeries. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…n clicks instead The previous commit changed isBreakdown so single-value breakdown steps used the breakdown rendering branch — but that branch computes percentages from breakdownSum/basisStep with an empty space, causing the first step to fill less than 100% when not all persons fall into the visible breakdown values. Restore the original isBreakdown logic so rendering matches master, and instead route clicks through a small openStep helper that picks openPersonsModalForSeries when getStepBreakdownSeries finds a single visible breakdown value. The helper function lives in funnelUtils with focused unit tests covering all branches. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…r unrepresentable breakdowns Address review feedback on the recordingFilters fix: - Extract a single buildFunnelBreakdownFilter helper used by both the session-IDs and fallback paths, removing ~50 lines of duplication. - Bail out for breakdowns that can't be expressed as a single Exact property filter: cohort lists, multi-key (array) breakdown properties, and array breakdown values. Previously these would have produced a malformed filter key like "1,2,3". - Drop the unnecessary `as FunnelsActorsQuery` casts now that the discriminated union narrows on `source.kind`. - Add tests for the fallback path and the three bailout cases. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…l display When a horizontal funnel has a breakdown filter that resolves to a single visible value, switch the bar width, converted count, dropped-off count, and percentage labels to use that series' own stats instead of the parent step's aggregate. This makes the display internally consistent with the tooltip (which was already per-breakdown) and with the persons modal counts returned by breakdown-scoped clicks. Also trim the verbose comments on buildFunnelBreakdownFilter and getStepBreakdownSeries to keep the signal density higher. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
sampennington
changed the title
Contributor
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+70
to
+81
| const filterType: PropertyFilterType = | ||
| breakdownType === 'person' | ||
| ? PropertyFilterType.Person | ||
| : breakdownType === 'group' | ||
| ? PropertyFilterType.Group | ||
| : PropertyFilterType.Event | ||
| return { | ||
| key: String(breakdown), | ||
| value: source.funnelStepBreakdown, | ||
| operator: PropertyOperator.Exact, | ||
| type: filterType, | ||
| } as UniversalFilterValue |
Contributor
There was a problem hiding this comment.
For group breakdowns, the resulting PropertyFilterType.Group filter is missing group_type_index. Without it, the backend cannot match the recording to the correct group type, so recordings may not be correctly scoped when the breakdown type is group.
breakdown_group_type_index is available on source.source.breakdownFilter and should be spread into the returned object when breakdownType === 'group'.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/scenes/trends/persons-modal/personsModalLogic.ts
Line: 70-81
Comment:
**Missing group type index for group breakdown filters**
For group breakdowns, the resulting `PropertyFilterType.Group` filter is missing `group_type_index`. Without it, the backend cannot match the recording to the correct group type, so recordings may not be correctly scoped when the breakdown type is `group`.
`breakdown_group_type_index` is available on `source.source.breakdownFilter` and should be spread into the returned object when `breakdownType === 'group'`.
How can I resolve this? If you propose a fix, please make it concise.
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
When a funnel has a breakdown (e.g. by country) and you filter it down to a single visible value, clicking into the persons modal or session recordings wasn't scoped to that breakdown value. Three related issues in the horizontal (top-to-bottom) layout:
recordingFiltersselector ignoredfunnelStepBreakdownonFunnelsActorsQuerysources.nested_breakdownas "not a breakdown", so clicks routed throughopenPersonsModalForStepwithout any breakdown context.Changes
personsModalLogic.ts— newbuildFunnelBreakdownFilterhelper used by both the session-IDs path and the fallback filter path; bails out for cohort, multi-key, and array-value breakdowns that can't be expressed as a single Exact property filter.funnelUtils.tsx— newgetStepBreakdownSerieshelper that returns the single visible breakdown series on a funnel step when the step is rendered without a per-breakdown bar.FunnelBarHorizontal.tsx— computesdisplayStep = stepBreakdownSeries ?? stepand uses it for the bar width, the converted/dropped-off counts, and the percentage labels. Clicks route throughopenPersonsModalForSerieswhen there's a single visible breakdown value so the persons modal is scoped to it.getStepBreakdownSeries, plus 5 new cases inpersonsModalLogic.test.tscovering the session-IDs path, the fallback path, and the three bailout scenarios.How did you test this code?
Screen.Recording.2026-04-09.at.11.17.32.mov
Publish to changelog?