Repository navigation
Conversation
Member
Author
Contributor
|
Size Change: -466 kB (-0.73%) Total Size: 63.7 MB 📦 View Changed
ℹ️ View Unchanged
|
rodrigoi
force-pushed
the
experiments/breakdown-attribution-metric-row-group
branch
from
June 5, 2026 07:48
f342f6c to
cb4b028
Compare
2 tasks
rodrigoi
force-pushed
the
experiments/breakdown-attribution-metric-row-group
branch
from
June 9, 2026 07:19
99037fb to
63649fa
Compare
This comment was marked as resolved.
This comment was marked as resolved.
rodrigoi
force-pushed
the
experiments/breakdown-attribution-metric-row-group
branch
from
June 16, 2026 22:54
63649fa to
4285ee0
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
rodrigoi
added
waiting
rodrigoi
force-pushed
the
experiments/breakdown-attribution-metric-row-group
branch
from
July 16, 2026 01:45
4285ee0 to
374ecbe
Compare
Contributor
🤖 CI report
|
| File | Size | Δ vs base |
|---|---|---|
render-query/src/render-query/render-query.js |
21.82 MiB | 🔺 +5.0 KiB (+0.0%) |
posthog-app/src/scenes/experiments/Experiment.js |
256.3 KiB | 🔺 +3.5 KiB (+1.4%) |
Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report
✅ Eager graph — within budget
How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.28 MiB · 22 files | no change | ███░░░░░░░ 28.4% of 4.51 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
8.54 MiB · 3,144 files | 🔺 +590 B (+0.0%) | █████████░ 87.9% of 9.71 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
Largest files eagerly shipped from src/index.tsx
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 304.7 KiB | ../node_modules/.pnpm/posthog-js@1.417.1/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 243.9 KiB | ../node_modules/.pnpm/posthog-js@1.417.1/node_modules/posthog-js/dist/module.js |
| 238.5 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 154.2 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 103.9 KiB | src/lib/api.ts |
| 95.0 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.23 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.23 MiB · 17 files | 🔺 +72 B (+0.0%) | ████░░░░░░ 38.9% of 5.72 MiB |
| Deferred (lazy) | 2.08 MiB · 33 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 741.7 KiB | dist/toolbar/toolbar-app-WKC4VBYK.css |
| 565.6 KiB | dist/toolbar/chunk-chunk-3WQDNPNI.js |
| 484.6 KiB | dist/toolbar/chunk-chunk-UISDLV4D.js |
| 133.6 KiB | dist/toolbar/chunk-chunk-KGYL6IWX.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.2 KiB | dist/toolbar/toolbar-app-YPMSMIMY.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-3HDUBWIR.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-AYYGAPFX.js |
| 12.2 KiB | dist/toolbar/chunk-chunk-PIK3PADE.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +120.1 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1442.59 MiB · 🔺 +120.1 KiB (+0.0%)
rodrigoi
force-pushed
the
experiments/breakdown-attribution-metric-row-group
branch
2 times, most recently
from
July 16, 2026 20:48
860939e to
af9db15
Compare
rodrigoi
marked this pull request as ready for review
July 16, 2026 20:48
Contributor
🦔 Hogbox preview · ✅ ready▶ Open the preview
commit |
Contributor
|
Reviews (1): Last reviewed commit: "fix(experiments): removing em-dashes fro..." | Re-trigger Greptile |
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the |
rodrigoi
added
waiting
rodrigoi
force-pushed
the
experiments/breakdown-attribution-metric-row-group
branch
from
August 14, 2026 21:21
440324d to
0adb791
Compare
Contributor
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 1 should fix, 1 consider. Published 2 findings (view the review). |
…lues on the experiment logic
…unordered funnels
rodrigoi
force-pushed
the
experiments/breakdown-attribution-metric-row-group
branch
from
August 15, 2026 04:21
0adb791 to
7044d86
Compare
Contributor
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Comment on lines
+284
to
+293
| <div | ||
| // Prevent the attribution dropdown (and chip close) from toggling the collapse. | ||
| onClick={(e) => e.stopPropagation()} | ||
| > | ||
| <BreakdownChipsRow | ||
| metric={metric} | ||
| onRemoveBreakdown={onRemoveBreakdown} | ||
| onAttributionChange={onAttributionChange} | ||
| onBreakdownLimitChange={onBreakdownLimitChange} | ||
| /> |
Contributor
There was a problem hiding this comment.
The collapse header contains nested buttons
Why we think it's a valid issue
- Checked: how
LemonCollapserenders a panel header, whatLemonSelectrenders, and whether the nesting is new or pre-existing in this component. - Found: the premise holds.
LemonCollapsePanelwrapsheaderChildrenin aLemonButton—frontend/src/lib/lemon-ui/LemonCollapse/LemonCollapse.tsx:151-166for the expandable case,:167-174for the disabled case.LemonSelectrenders its trigger as aLemonButton(frontend/src/lib/lemon-ui/LemonSelect/LemonSelect.tsx:157), which emits a real<button>. SoMetricBreakdownAttributionandMetricBreakdownLimitboth land inside the header<button>atMetricRowGroup.tsx:284-293. - Found: React's dev-mode
validateDOMNestingtreats<button>inside<button>as invalid and logs a warning on every render of an expanded metric row. - Found: LemonUI already supplies the sibling-control API the suggestion asks for.
LemonCollapseacceptsheaderasLemonButtonProps(LemonCollapse.tsx:15and:140-144), andsideActionrenders the extra control as a sibling of the toggle inside a wrapper div, not a descendant (frontend/src/lib/lemon-ui/LemonButton/LemonButton.tsx:312-338).frontend/src/scenes/hog-functions/mapping/HogFunctionMappings.tsx:311-336uses that pattern. So the fix is available without new component work. - Found: the nesting is not introduced by this PR. On master the same header already rendered
BreakdownTagwithonClose, which emits a nestedLemonButtonclose control (frontend/src/scenes/insights/filters/BreakdownFilter/BreakdownTag.tsx:183-195). The PR extends an existing defect rather than creating one. - Impact: confirmed invalid markup and muddied accessible-name computation for the header button, plus React dev warnings. There is no functional break for mouse or keyboard users: the wrapper
onClickstops the click before it reaches the toggle handler, and the select's popover renders in a portal outside the button. - Priority: lowered to
consider. The defect is real and deterministic, but it predates the PR in the same header, it breaks no user-visible behavior, and the two added controls widen an existing condition rather than opening a new failure mode.
Issue description
LemonCollapse renders its header as a LemonButton. Both new selectors also render LemonButton controls. This creates invalid nested button markup and unclear screen-reader semantics. Stopping click propagation does not fix the HTML structure.
Suggested fix
Render the selectors outside the collapse toggle button. Alternatively, update the header API to render the toggle and its actions as sibling controls.
Prompt to fix with AI (copy-paste)
## Context
@frontend/src/scenes/experiments/MetricsView/new/MetricRowGroup.tsx#L284-293
<issue_description>
LemonCollapse renders its header as a LemonButton. Both new selectors also render LemonButton controls. This creates invalid nested button markup and unclear screen-reader semantics. Stopping click propagation does not fix the HTML structure.
</issue_description>
<issue_validation>
- **Checked:** how `LemonCollapse` renders a panel header, what `LemonSelect` renders, and whether the nesting is new or pre-existing in this component.
- **Found:** the premise holds. `LemonCollapsePanel` wraps `headerChildren` in a `LemonButton` — `frontend/src/lib/lemon-ui/LemonCollapse/LemonCollapse.tsx:151-166` for the expandable case, `:167-174` for the disabled case. `LemonSelect` renders its trigger as a `LemonButton` (`frontend/src/lib/lemon-ui/LemonSelect/LemonSelect.tsx:157`), which emits a real `<button>`. So `MetricBreakdownAttribution` and `MetricBreakdownLimit` both land inside the header `<button>` at `MetricRowGroup.tsx:284-293`.
- **Found:** React's dev-mode `validateDOMNesting` treats `<button>` inside `<button>` as invalid and logs a warning on every render of an expanded metric row.
- **Found:** LemonUI already supplies the sibling-control API the suggestion asks for. `LemonCollapse` accepts `header` as `LemonButtonProps` (`LemonCollapse.tsx:15` and `:140-144`), and `sideAction` renders the extra control as a sibling of the toggle inside a wrapper div, not a descendant (`frontend/src/lib/lemon-ui/LemonButton/LemonButton.tsx:312-338`). `frontend/src/scenes/hog-functions/mapping/HogFunctionMappings.tsx:311-336` uses that pattern. So the fix is available without new component work.
- **Found:** the nesting is not introduced by this PR. On master the same header already rendered `BreakdownTag` with `onClose`, which emits a nested `LemonButton` close control (`frontend/src/scenes/insights/filters/BreakdownFilter/BreakdownTag.tsx:183-195`). The PR extends an existing defect rather than creating one.
- **Impact:** confirmed invalid markup and muddied accessible-name computation for the header button, plus React dev warnings. There is no functional break for mouse or keyboard users: the wrapper `onClick` stops the click before it reaches the toggle handler, and the select's popover renders in a portal outside the button.
- **Priority:** lowered to `consider`. The defect is real and deterministic, but it predates the PR in the same header, it breaks no user-visible behavior, and the two added controls widen an existing condition rather than opening a new failure mode.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Render the selectors outside the collapse toggle button. Alternatively, update the header API to render the toggle and its actions as sibling controls.
</potential_solution>
Contributor
There was a problem hiding this comment.
Confirmed: the collapse header is always rendered inside a LemonButton (LemonCollapse has no non-button header variant), so the attribution/limit LemonSelect triggers and the chip close buttons do end up as nested <button>s. Stopping click propagation avoids the toggle firing but doesn't change the HTML nesting or the accessible-name computation, as you note.
I'm escalating rather than fixing this unattended, for two reasons. First, this isn't new to the PR — the header already nested a button on master (the breakdown chip's close control), so this change widens an existing condition rather than opening a new one, and there's no functional break for mouse or keyboard users. Second, the two clean fixes are both bigger than a mechanical change: moving the controls out of the collapse header is a layout decision whose result needs to be eyeballed, and the sideAction route doesn't fit here — sideAction renders its contents inside a second LemonButton, so a whole chips-plus-two-selects row would just re-nest the same buttons. A proper sibling-controls fix means either restructuring this header's layout or extending the shared LemonCollapse header API, which reaches beyond this PR's scope.
Decision for a human: whether to invest in restructuring the breakdown row so the interactive controls render as siblings of the collapse toggle (and if so, header-layout change here vs. a LemonCollapse API addition), or to accept the pre-existing nesting as-is.
The updateMetricAttribution and updateMetricBreakdownLimit listeners fired a recalculation immediately after dispatching updateExperiment without awaiting it. The updateExperiment loader defers its PATCH into a microtask, so the recalculation request left the browser before the save, and a failed save (e.g. a version conflict) still kicked off a recalculation that recomputed with the old attribution or limit. Await asyncActions.updateExperiment before triggering the recalculation, matching the existing pattern in changeExperimentStartDate, changeExperimentEndDate, and updateExperimentSettings. On a failed save the recalculation no longer runs. Generated-By: PostHog Desktop Task-Id: 4c96df3c-fa46-4fb2-908a-a95cd926252e
This branch was successfully deployed
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
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
Experiment metric breakdowns currently attribute the breakdown value from the exposure event. That diverges from insights funnels, where the breakdown is read off the funnel events with a configurable attribution mode, and it produces null buckets whenever the exposure event doesn't carry the property.
We're moving experiment breakdowns to metric-event attribution. This PR is the frontend for it; the backend lands in the rest of the stack (#61796, #62291, #62292, #62293).
Changes
This PR adds the frontend controls for metric-event breakdowns: a funnel attribution dropdown and a breakdown limit, both on the results table's breakdown row.
Feature gated
Everything renders behind the
experiment-metric-event-breakdownsflag. The attribution dropdown shows for funnel metrics only; the limit applies to all metric types.frontend/src/lib/constants.tsxposthog/constants.pyAttribution control
MetricBreakdownAttributionis aLemonSelectoffering first touchpoint, last touchpoint, all steps, and a specific step ("Any step" replaces the step list for unordered funnels). The "Specific step" choice is encoded asstep/<index>so a single select carries both the attribution type and the step. The selection is stored on the metric asbreakdownAttributionTypeandbreakdownAttributionValue, matching how funnel insights model attribution.frontend/src/scenes/experiments/MetricsView/shared/MetricBreakdownAttribution.tsxBreakdown limit control
MetricBreakdownLimitcaps the number of breakdown values shown (5 to 25, plus "Show all", which maps to the backend max of 1000). Values beyond the limit are grouped under "Other" by the backend. If a metric carries a non-preset limit, the dropdown surfaces it instead of falling back to a placeholder.frontend/src/scenes/experiments/MetricsView/shared/MetricBreakdownLimit.tsxMetric row group
The breakdown chips row is extracted into
BreakdownChipsRowand rendered in both the collapsible section header and the breakdown error state. Clicks on the controls stop propagation so they don't toggle the collapse.frontend/src/scenes/experiments/MetricsView/new/MetricRowGroup.tsxfrontend/src/scenes/experiments/MetricsView/new/MetricsTable.tsxExperiment logic
updateMetricAttributionandupdateMetricBreakdownLimitupdate the inline metric, persist viaupdateExperiment(withupdate_feature_flag_params: false), and refetch the affected primary or secondary results.frontend/src/scenes/experiments/experimentLogic.tsxfrontend/src/scenes/experiments/experimentLogic.test.tsSchema
ExperimentFunnelMetricgainsbreakdownAttributionTypeandbreakdownAttributionValue. The limit reuses the existingbreakdownFilter.breakdown_limit.frontend/src/queries/schema/schema-general.tsfrontend/src/queries/schema.json(generated)frontend/src/queries/validators.js(generated)posthog/schema.py(generated)products/dashboards/frontend/generated/api.schemas.ts(generated)products/early_access_features/frontend/generated/api.schemas.ts(generated)services/mcp/src/api/generated.ts(generated)How did you test this code?
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Automatic notifications
Docs update
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Model: Opus 4.7 (implementation), Fable 5 (restack and PR description)
Manually refactored: yes
Skills used:
Relevant decisions:
breakdownAttributionTypeandbreakdownAttributionValueonExperimentFunnelMetric), not onbreakdownFilter, since attribution is a property of how the funnel reads the value, not of the breakdown definition.LemonSelectencodes "Specific step" asstep/<index>, avoiding a second control for the step picker.updateExperimentand triggers a results refetch, so the new attribution takes effect immediately.