Skip to content

feat(experiments): add breakdown attribution to the metric row group #61788

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rodrigoi wants to merge 15 commits into from
Closed

feat(experiments): add breakdown attribution to the metric row group #61788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
17c08d1
feat(experiments): add breakdown attribution to the metric row group
rodrigoi Jun 5, 2026
7c1fbdc
chore: update OpenAPI generated types
tests-posthog[bot] Jun 5, 2026
0a87ce1
feat(experiments): adding breakdown limit property to the UI.
rodrigoi Jun 5, 2026
48dca52
fix(experiments): removing em-dashes from attribution tooltio
rodrigoi Jul 16, 2026
5ef9e4d
fix(experiments): using shared metrics metadata to store breakdown va…
rodrigoi Jul 16, 2026
c3d3c09
fix(experiments): using shared metrics metadata to store breakdown va…
rodrigoi Jul 16, 2026
c11841d
fix(experiments): update components to handble unordered funnels
rodrigoi Jul 16, 2026
4510b66
fix(experiments): sorting feature flags alphabetically
rodrigoi Jul 21, 2026
262e733
fix(experiments): import shared funnel step count limit constant
rodrigoi Aug 14, 2026
ab409da
fix(experiments): correct attribution tooltip copy for all steps and …
rodrigoi Aug 14, 2026
c2645c4
fix(experiments): clarify breakdown limit has no backend max clamp
rodrigoi Aug 14, 2026
b2eb39c
fix(experiments): only merge attribution metadata onto funnel metrics
rodrigoi Aug 14, 2026
0d621a2
fix(experiments): derive isPrimary from shared metric metadata in bre…
rodrigoi Aug 14, 2026
7044d86
chore(experiments): format metrics table attribution handler
rodrigoi Aug 14, 2026
7576a14
fix(experiments): await metric config save before recalculation
posthog[bot] Aug 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions frontend/src/lib/constants.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,7 @@ export const FEATURE_FLAGS = {
EVENT_MEDIA_PREVIEWS: 'event-media-previews', // owner: @alexlider
EXPERIMENT_BEHAVIOR_COMPARISON: 'experiment-behavior-comparison', // owner: @mp-hog #team-experiments
EXPERIMENT_FLAG_CLEANUP_PR: 'experiment-flag-cleanup-pr', // owner: @jurajmajerik #team-experiments
EXPERIMENT_METRIC_EVENT_BREAKDOWNS: 'experiment-metric-event-breakdowns', // owner: @rodrigoi #team-experiments
EXPERIMENT_RECORDINGS_TAB: 'experiment-recordings-tab', // owner: @mp-hog #team-experiments
EXPERIMENT_SESSION_REPLAYS_SKILL: 'experiment-session-replays-skill', // owner: @rodrigoi #team-experiments
EXPERIMENTS_DW_AA_TEST: 'experiments-dw-aa-test', // owner: @rodrigoi #team-experiments
Expand Down
18 changes: 18 additions & 0 deletions frontend/src/queries/schema.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions frontend/src/queries/schema/schema-general.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
AnyPersonScopeFilter,
AnyPropertyFilter,
BaseMathType,
BreakdownAttributionType,
BreakdownKeyType,
BreakdownType,
CalendarHeatmapMathType,
Expand Down Expand Up @@ -5166,6 +5167,10 @@ export type ExperimentFunnelMetric = ExperimentMetricBaseProperties & {
metric_type: ExperimentMetricType.FUNNEL
series: ExperimentFunnelMetricStep[]
funnel_order_type?: StepOrderValue
/** How to attribute the breakdown value across funnel steps. @default first_touch */
breakdownAttributionType?: BreakdownAttributionType
/** When breakdownAttributionType is `step`, the 0-indexed step to attribute from. @asType integer */
breakdownAttributionValue?: integer
}

export const isExperimentFunnelMetric = (metric: ExperimentMetric): metric is ExperimentFunnelMetric =>
Expand Down
545 changes: 318 additions & 227 deletions frontend/src/queries/validators.js
View file
Open in desktop

Large diffs are not rendered by default.

147 changes: 122 additions & 25 deletions frontend/src/scenes/experiments/MetricsView/new/MetricRowGroup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@ import { useActions, useValues } from 'kea'
import React, { useEffect, useRef, useState } from 'react'
import { createPortal } from 'react-dom'

import { IconTrending } from '@posthog/icons'
import { IconInfo, IconTrending } from '@posthog/icons'

import { FEATURE_FLAGS } from 'lib/constants'
import { useFeatureFlag } from 'lib/hooks/useFeatureFlag'
import { IconTrendingDown } from 'lib/lemon-ui/icons'
import { LemonCollapse } from 'lib/lemon-ui/LemonCollapse'
import { LemonSkeleton } from 'lib/lemon-ui/LemonSkeleton'
import { Tooltip } from 'lib/lemon-ui/Tooltip'
import { featureFlagLogic } from 'lib/logic/featureFlagLogic'
import { humanFriendlyLargeNumber } from 'lib/utils/numbers'
import { VariantTag } from 'scenes/experiments/ExperimentView/VariantTag'
Expand All @@ -19,11 +21,12 @@ import { formatBreakdownLabel } from 'scenes/insights/utils'

import {
Breakdown,
ExperimentFunnelMetric,
ExperimentMetric,
ExperimentStatsBaseValidated,
NewExperimentQueryResponse,
} from '~/queries/schema/schema-general'
import { NodeKind } from '~/queries/schema/schema-general'
import { isExperimentFunnelMetric, NodeKind } from '~/queries/schema/schema-general'
import { experimentLogic } from '~/scenes/experiments/experimentLogic'
import { experimentMetricsLogic } from '~/scenes/experiments/experimentMetricsLogic'
import { isLaunched } from '~/scenes/experiments/experimentsLogic'
Expand Down Expand Up @@ -53,8 +56,10 @@ import {
isSignificant,
isWinning,
} from '~/scenes/experiments/MetricsView/shared/utils'
import { Experiment, InsightType } from '~/types'
import { BreakdownAttributionType, Experiment, InsightType, StepOrderValue } from '~/types'

import { MetricBreakdownAttribution } from '../shared/MetricBreakdownAttribution'
import { MetricBreakdownLimit } from '../shared/MetricBreakdownLimit'
import { ChartCell } from './ChartCell'
import {
CELL_HEIGHT,
Expand All @@ -73,28 +78,108 @@ import { renderTooltipContent } from './MetricRowGroupTooltip'
import { TimeseriesModal } from './TimeseriesModal'
import { useAxisScale } from './useAxisScale'

/**
* Renders the metric's breakdown chips on the left, then (behind the
* metric-event-breakdowns flag) the funnel attribution dropdown and the
* breakdown limit. Attribution is funnel-only; the limit applies to all types.
*/
function BreakdownChipsRow({
metric,
onRemoveBreakdown,
onAttributionChange,
onBreakdownLimitChange,
}: {
metric: ExperimentMetric
onRemoveBreakdown: (index: number) => void
onAttributionChange: (attributionType: BreakdownAttributionType, attributionValue?: number) => void
onBreakdownLimitChange: (breakdownLimit: number) => void
}): JSX.Element {
const metricEventBreakdownsEnabled = useFeatureFlag('EXPERIMENT_METRIC_EVENT_BREAKDOWNS')
const showAttribution = metricEventBreakdownsEnabled && isExperimentFunnelMetric(metric)
const showLimit = metricEventBreakdownsEnabled

return (
<div className="flex items-center gap-3">
<div className="flex items-center gap-2">
{metric.breakdownFilter?.breakdowns?.map((breakdown, index) => (
<BreakdownTag
key={index}
breakdown={breakdown.property}
breakdownType={breakdown.type || 'event'}
onClose={() => onRemoveBreakdown(index)}
size="small"
/>
))}
</div>
{showAttribution && (
<div className="flex items-center gap-1">
<span className="text-muted">Attribution</span>
<Tooltip
title={
<>
Which funnel step the breakdown value is read from for each user:
<ul className="list-disc pl-4">
<li>
<strong>First touchpoint</strong>: value at the user's first step
</li>
<li>
<strong>Last touchpoint</strong>: value at the user's last step
</li>
<li>
<strong>All steps</strong>: behaves like first touchpoint
</li>
{(metric as ExperimentFunnelMetric).funnel_order_type ===
StepOrderValue.UNORDERED ? (
<li>
<strong>Any step</strong>: value at any matching step
</li>
) : (
<li>
<strong>Specific step</strong>: value at a step you choose
</li>
)}
</ul>
</>
}
>
<IconInfo className="text-secondary text-base shrink-0" />
</Tooltip>
<MetricBreakdownAttribution
metric={metric as ExperimentFunnelMetric}
onChange={onAttributionChange}
/>
</div>
)}
{showLimit && <MetricBreakdownLimit metric={metric} onChange={onBreakdownLimitChange} />}
</div>
)
}

interface BreakdownErrorStateProps {
metric: ExperimentMetric
isAlternatingRow: boolean
onRemoveBreakdown: (index: number) => void
onAttributionChange: (attributionType: BreakdownAttributionType, attributionValue?: number) => void
onBreakdownLimitChange: (breakdownLimit: number) => void
}

function BreakdownErrorState({ metric, isAlternatingRow, onRemoveBreakdown }: BreakdownErrorStateProps): JSX.Element {
function BreakdownErrorState({
metric,
isAlternatingRow,
onRemoveBreakdown,
onAttributionChange,
onBreakdownLimitChange,
}: BreakdownErrorStateProps): JSX.Element {
return (
<tr data-breakdown-row className="hover:bg-bg-hover group">
<td colSpan={7} className={`p-0 border-t border-b ${isAlternatingRow ? 'bg-bg-table' : 'bg-bg-light'}`}>
<div className="p-1">
<div className="flex items-center gap-2">
{metric.breakdownFilter?.breakdowns?.map((breakdown, index) => (
<BreakdownTag
key={index}
breakdown={breakdown.property}
breakdownType={breakdown.type || 'event'}
onClose={() => onRemoveBreakdown(index)}
size="small"
/>
))}
</div>
<BreakdownChipsRow
metric={metric}
onRemoveBreakdown={onRemoveBreakdown}
onAttributionChange={onAttributionChange}
onBreakdownLimitChange={onBreakdownLimitChange}
/>
</div>
</td>
</tr>
Expand All @@ -114,6 +199,8 @@ interface CollapsibleBreakdownSectionProps {
colors: ReturnType<typeof useChartColors>
scale: ReturnType<typeof useAxisScale>
onRemoveBreakdown: (index: number) => void
onAttributionChange: (attributionType: BreakdownAttributionType, attributionValue?: number) => void
onBreakdownLimitChange: (breakdownLimit: number) => void
onRetry: () => void
query?: Record<string, any>
handleTooltipMouseEnter: (e: React.MouseEvent, variantResult: ExperimentVariantResult) => void
Expand All @@ -133,6 +220,8 @@ function CollapsibleBreakdownSection({
colors,
scale,
onRemoveBreakdown,
onAttributionChange,
onBreakdownLimitChange,
onRetry,
query,
handleTooltipMouseEnter,
Expand Down Expand Up @@ -192,16 +281,16 @@ function CollapsibleBreakdownSection({
{
key: 'breakdowns',
header: (
<div className="flex items-center gap-2">
{metric.breakdownFilter?.breakdowns?.map((breakdown, index) => (
<BreakdownTag
key={index}
breakdown={breakdown.property}
breakdownType={breakdown.type || 'event'}
onClose={() => onRemoveBreakdown(index)}
size="small"
/>
))}
<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}
/>
Comment on lines +284 to +293

@posthog posthog Bot Aug 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The collapse header contains nested buttons

consider accessibility

Why we think it's a valid issue
  • 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 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>

@posthog posthog Bot Aug 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

</div>
),
// Render no content (non-expandable disabled header) when recalculating (stale
Expand Down Expand Up @@ -539,6 +628,8 @@ interface MetricRowGroupProps {
onDeleteMetric?: () => void
onBreakdownChange: (breakdown: Breakdown) => void
onRemoveBreakdown: (index: number) => void
onAttributionChange: (attributionType: BreakdownAttributionType, attributionValue?: number) => void
onBreakdownLimitChange: (breakdownLimit: number) => void
error?: any
isLoading?: boolean
exposuresLoading?: boolean
Expand All @@ -562,6 +653,8 @@ export function MetricRowGroup({
onDeleteMetric,
onBreakdownChange,
onRemoveBreakdown,
onAttributionChange,
onBreakdownLimitChange,
error,
isLoading,
exposuresLoading = false,
Expand Down Expand Up @@ -894,6 +987,8 @@ export function MetricRowGroup({
metric={metric}
isAlternatingRow={isAlternatingRow}
onRemoveBreakdown={onRemoveBreakdown}
onAttributionChange={onAttributionChange}
onBreakdownLimitChange={onBreakdownLimitChange}
/>
)}
</>
Expand Down Expand Up @@ -1211,6 +1306,8 @@ export function MetricRowGroup({
colors={colors}
scale={scale}
onRemoveBreakdown={onRemoveBreakdown}
onAttributionChange={onAttributionChange}
onBreakdownLimitChange={onBreakdownLimitChange}
onRetry={handleRetry}
query={debugQuery}
handleTooltipMouseEnter={handleTooltipMouseEnter}
Expand Down
16 changes: 16 additions & 0 deletions frontend/src/scenes/experiments/MetricsView/new/MetricsTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ export function MetricsTable({
updateExperimentMetrics,
updateMetricBreakdown,
removeMetricBreakdown,
updateMetricAttribution,
updateMetricBreakdownLimit,
removeMetric,
removeSharedMetricFromExperiment,
reorderMetrics,
Expand Down Expand Up @@ -278,6 +280,20 @@ export function MetricsTable({

removeMetricBreakdown(metric.uuid, index, breakdown)
}}
onAttributionChange={(attributionType, attributionValue) => {
if (!metric.uuid) {
return
}

updateMetricAttribution(metric.uuid, attributionType, attributionValue)
}}
onBreakdownLimitChange={(breakdownLimit) => {
if (!metric.uuid) {
return
}

updateMetricBreakdownLimit(metric.uuid, breakdownLimit)
}}
error={error}
isLoading={isLoading}
exposuresLoading={exposuresLoading}
Expand Down
Loading
Loading