From 83761638f2d9900b3110f0acd8cf4d61ccb86b3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carl-Gerhard=20Lindesva=CC=88rd?= Date: Mon, 2 Mar 2026 15:28:28 +0100 Subject: [PATCH] fix: improve how previous state is shown for funnels --- apps/start/src/components/delta-chip.tsx | 10 +- .../common/previous-diff-indicator.tsx | 2 +- .../report-chart/funnel/breakdown-list.tsx | 34 +- .../components/report-chart/funnel/chart.tsx | 458 ++++++++++++------ packages/db/src/services/funnel.service.ts | 21 +- 5 files changed, 369 insertions(+), 156 deletions(-) diff --git a/apps/start/src/components/delta-chip.tsx b/apps/start/src/components/delta-chip.tsx index 09e321e3..0d05b677 100644 --- a/apps/start/src/components/delta-chip.tsx +++ b/apps/start/src/components/delta-chip.tsx @@ -3,7 +3,7 @@ import { type VariantProps, cva } from 'class-variance-authority'; import { ArrowDownIcon, ArrowUpIcon } from 'lucide-react'; const deltaChipVariants = cva( - 'flex items-center gap-1 rounded-full px-2 py-1 text-sm font-semibold', + 'flex items-center justify-center gap-1 rounded-full font-semibold', { variants: { variant: { @@ -12,9 +12,10 @@ const deltaChipVariants = cva( default: 'bg-muted text-muted-foreground', }, size: { - sm: 'text-xs', - md: 'text-sm', - lg: 'text-base', + xs: 'px-1.5 py-0 leading-none text-[10px]', + sm: 'px-2 py-1 text-xs', + md: 'px-2 py-1 text-sm', + lg: 'px-2 py-1 text-base', }, }, defaultVariants: { @@ -30,6 +31,7 @@ type DeltaChipProps = VariantProps & { }; const iconVariants: Record, number> = { + xs: 8, sm: 12, md: 16, lg: 20, diff --git a/apps/start/src/components/report-chart/common/previous-diff-indicator.tsx b/apps/start/src/components/report-chart/common/previous-diff-indicator.tsx index 669af54b..18a71d1d 100644 --- a/apps/start/src/components/report-chart/common/previous-diff-indicator.tsx +++ b/apps/start/src/components/report-chart/common/previous-diff-indicator.tsx @@ -97,7 +97,7 @@ interface PreviousDiffIndicatorPureProps { diff?: number | null | undefined; state?: string | null | undefined; inverted?: boolean; - size?: 'sm' | 'lg' | 'md'; + size?: 'xs' | 'sm' | 'lg' | 'md'; className?: string; showPrevious?: boolean; } diff --git a/apps/start/src/components/report-chart/funnel/breakdown-list.tsx b/apps/start/src/components/report-chart/funnel/breakdown-list.tsx index 76de4b8a..791a1442 100644 --- a/apps/start/src/components/report-chart/funnel/breakdown-list.tsx +++ b/apps/start/src/components/report-chart/funnel/breakdown-list.tsx @@ -3,8 +3,10 @@ import { useNumber } from '@/hooks/use-numer-formatter'; import type { RouterOutputs } from '@/trpc/client'; import { cn } from '@/utils/cn'; import { getChartColor } from '@/utils/theme'; +import { getPreviousMetric } from '@openpanel/common'; import { ChevronDown, ChevronRight } from 'lucide-react'; import { useState } from 'react'; +import { PreviousDiffIndicatorPure } from '../common/previous-diff-indicator'; import { Tables } from './chart'; interface BreakdownListProps { @@ -48,10 +50,9 @@ export function BreakdownList({ }); }; - // Get the color index for a breakdown based on its position in the - // visible series list (so colors match the chart bars) - const getVisibleIndex = (id: string) => { - return visibleSeriesIds.indexOf(id); + // Get the stable color index for a breakdown (position in full list, matches chart) + const getStableColorIndex = (id: string) => { + return allBreakdowns.findIndex((b) => b.id === id); }; if (allBreakdowns.length === 0) { @@ -81,14 +82,12 @@ export function BreakdownList({ {allBreakdowns.map((item, index) => { const isExpanded = expandedIds.has(item.id); const isVisible = visibleSeriesIds.includes(item.id); - const visibleIndex = getVisibleIndex(item.id); + const stableColorIndex = getStableColorIndex(item.id); const previousItem = previousData[index] ?? null; const hasBreakdownName = item.breakdowns && item.breakdowns.length > 0; const color = - isVisible && visibleIndex !== -1 - ? getChartColor(visibleIndex) - : undefined; + stableColorIndex >= 0 ? getChartColor(stableColorIndex) : undefined; return (
@@ -107,7 +106,7 @@ export function BreakdownList({ className="shrink-0" style={{ borderColor: color, - backgroundColor: isVisible ? color : 'transparent', + backgroundColor: isVisible && color ? color : 'transparent', }} /> )} @@ -141,6 +140,14 @@ export function BreakdownList({ '%', )}
+ {previousItem && ( + + )}
@@ -149,6 +156,14 @@ export function BreakdownList({
{number.format(item.lastStep.count)}
+ {previousItem && ( + + )}
@@ -160,6 +175,7 @@ export function BreakdownList({ current: item, previous: previousItem, }} + noTopBorderRadius /> )} diff --git a/apps/start/src/components/report-chart/funnel/chart.tsx b/apps/start/src/components/report-chart/funnel/chart.tsx index 0a17a641..de518829 100644 --- a/apps/start/src/components/report-chart/funnel/chart.tsx +++ b/apps/start/src/components/report-chart/funnel/chart.tsx @@ -1,20 +1,6 @@ -import { ColorSquare } from '@/components/color-square'; -import { Button } from '@/components/ui/button'; -import { pushModal } from '@/modals'; -import type { RouterOutputs } from '@/trpc/client'; -import { cn } from '@/utils/cn'; -import { ChevronRightIcon, InfoIcon, UsersIcon } from 'lucide-react'; - -import { alphabetIds } from '@openpanel/constants'; - -import { createChartTooltip } from '@/components/charts/chart-tooltip'; -import { BarShapeBlue, BarShapeProps } from '@/components/charts/common-bar'; -import { Tooltiper } from '@/components/ui/tooltip'; -import { WidgetTable } from '@/components/widget-table'; -import { useNumber } from '@/hooks/use-numer-formatter'; -import type { IVisibleFunnelBreakdowns } from '@/hooks/use-visible-funnel-breakdowns'; -import { getChartColor, getChartTranslucentColor } from '@/utils/theme'; import { getPreviousMetric } from '@openpanel/common'; +import { alphabetIds } from '@openpanel/constants'; +import { ChevronRightIcon, InfoIcon, UsersIcon } from 'lucide-react'; import { useCallback } from 'react'; import { Bar, @@ -31,12 +17,24 @@ import { PreviousDiffIndicatorPure } from '../common/previous-diff-indicator'; import { SerieIcon } from '../common/serie-icon'; import { SerieName } from '../common/serie-name'; import { useReportChartContext } from '../context'; +import { createChartTooltip } from '@/components/charts/chart-tooltip'; +import { BarShapeProps } from '@/components/charts/common-bar'; +import { ColorSquare } from '@/components/color-square'; +import { Button } from '@/components/ui/button'; +import { Tooltiper } from '@/components/ui/tooltip'; +import { WidgetTable } from '@/components/widget-table'; +import { useNumber } from '@/hooks/use-numer-formatter'; +import { pushModal } from '@/modals'; +import type { RouterOutputs } from '@/trpc/client'; +import { cn } from '@/utils/cn'; +import { getChartColor, getChartTranslucentColor } from '@/utils/theme'; type Props = { data: { current: RouterOutputs['chart']['funnel']['current'][number]; previous: RouterOutputs['chart']['funnel']['current'][number] | null; }; + noTopBorderRadius?: boolean; }; export const Metric = ({ @@ -50,20 +48,16 @@ export const Metric = ({ enhancer?: React.ReactNode; className?: string; }) => ( -
-
{label}
-
+
+
{label}
+
{value}
{enhancer &&
{enhancer}
}
); -export function Summary({ - data, -}: { - data: RouterOutputs['chart']['funnel']; -}) { +export function Summary({ data }: { data: RouterOutputs['chart']['funnel'] }) { const number = useNumber(); const highestConversion = data.current .slice(0) @@ -81,10 +75,10 @@ export function Summary({ } /> - + {number.formatWithUnit( highestConversion.lastStep.percent / 100, - '%', + '%' )}
@@ -95,7 +89,7 @@ export function Summary({ label="Most conversions" value={} /> - + {number.format(highestCount.lastStep.count)}
@@ -107,7 +101,10 @@ export function Summary({ function ChartName({ breakdowns, className, -}: { breakdowns: string[]; className?: string }) { +}: { + breakdowns: string[]; + className?: string; +}) { return (
{breakdowns.map((name, index) => { @@ -127,6 +124,7 @@ export function Tables({ current: { steps, mostDropoffsStep, lastStep, breakdowns }, previous: previousData, }, + noTopBorderRadius, }: Props) { const number = useNumber(); const hasHeader = breakdowns.length > 0; @@ -145,11 +143,11 @@ export function Tables({ } = useReportChartContext(); const funnelOptions = options?.type === 'funnel' ? options : undefined; - const funnelWindow = funnelOptions?.funnelWindow; - const funnelGroup = funnelOptions?.funnelGroup; const handleInspectStep = (step: (typeof steps)[0], stepIndex: number) => { - if (!projectId || !step.event.id) return; + if (!(projectId && step.event.id)) { + return; + } // For funnels, we need to pass the step index so the modal can query // users who completed at least that step in the funnel sequence @@ -172,48 +170,56 @@ export function Tables({ }); }; return ( -
+
{hasHeader && } -
-
+
+
) } + label="Conversion" + value={number.formatWithUnit(lastStep?.percent / 100, '%')} /> ) } + label="Completed" + value={number.format(lastStep?.count)} /> {!!mostDropoffsStep && ( @@ -223,44 +229,26 @@ export function Tables({ conversion rate will likely increase. } + tooltipClassName="max-w-xs" > } + label="Most dropoffs after" + value={mostDropoffsStep?.event?.displayName} /> )}
item.event.id!} - className={'text-sm @container'} + className={'@container text-sm'} columnClassName="px-2 group/row items-center" - eachRow={(item, index) => { - return ( -
-
-
- ); - }} columns={[ { name: 'Event', render: (item, index) => ( -
+
{alphabetIds[index]} @@ -295,17 +283,17 @@ export function Tables({ name: '', render: (item) => ( @@ -314,6 +302,27 @@ export function Tables({ width: '48px', }, ]} + data={steps} + eachRow={(item, index) => { + return ( +
+
+
+ ); + }} + keyExtractor={(item) => item.event.id!} />
@@ -363,9 +372,11 @@ const useRechartData = ({ ...visibleBreakdowns.reduce((acc, visibleItem, visibleIdx) => { // Find the original index for this visible breakdown const originalIndex = current.findIndex( - (item) => item.id === visibleItem.id, + (item) => item.id === visibleItem.id ); - if (originalIndex === -1) return acc; + if (originalIndex === -1) { + return acc; + } const diff = previous?.[originalIndex]; return { @@ -391,6 +402,47 @@ const useRechartData = ({ ); }; +const StripedBarShape = (props: any) => { + const { x, y, width, height, fill, stroke, value } = props; + const patternId = `prev-stripes-${(fill || '').replace(/[^a-z0-9]/gi, '')}`; + return ( + + + + + + + + + {value > 0 && ( + + )} + + ); +}; + export function Chart({ data, visibleBreakdowns, @@ -403,96 +455,162 @@ export function Chart({ const yAxisProps = useYAxisProps(); const hasBreakdowns = data.current.length > 1; const hasVisibleBreakdowns = visibleBreakdowns.length > 1; + const hasPrevious = + data.previous !== null && + data.previous !== undefined && + data.previous.length > 0; + const showPreviousBars = hasPrevious && !hasBreakdowns; const CustomLegend = useCallback(() => { - if (!hasVisibleBreakdowns) return null; + if (!hasVisibleBreakdowns) { + return null; + } return ( -
- {visibleBreakdowns.map((breakdown, idx) => ( -
- - 0 - ? breakdown.breakdowns - : ['Funnel'] - } - className="font-semibold" - /> -
- ))} +
+ {visibleBreakdowns.map((breakdown, idx) => { + const stableIndex = data.current.findIndex((b) => b.id === breakdown.id); + const colorIndex = stableIndex >= 0 ? stableIndex : idx; + return ( +
+ + 0 + ? breakdown.breakdowns + : ['Funnel'] + } + /> +
+ ); + })}
); }, [visibleBreakdowns, hasVisibleBreakdowns]); + const PreviousLegend = useCallback(() => { + if (!showPreviousBars) { + return null; + } + return ( +
+
+
+ Current +
+
+ + + + + + + + + + Previous +
+
+ ); + }, [showPreviousBars]); + return ( b.id))} > -
+
data.current[0].steps.find((step) => step.event.id === id) ?.event.displayName ?? '' } + tickMargin={4} + tickSize={0} + type={'category'} /> - {hasBreakdowns ? ( - visibleBreakdowns.map((item, breakdownIndex) => ( - } - > - {rechartData.map((item, stepIndex) => ( - - ))} - - )) - ) : ( - } - > + {hasBreakdowns && + visibleBreakdowns.map((item, breakdownIndex) => { + const stableIndex = data.current.findIndex( + (b) => b.id === item.id, + ); + const colorIndex = + stableIndex >= 0 ? stableIndex : breakdownIndex; + return ( + } + > + {rechartData.map((row, stepIndex) => ( + + ))} + + ); + })} + {!hasBreakdowns && ( + }> {rechartData.map((item, index) => ( + ))} + + )} + {showPreviousBars && ( + }> + {rechartData.map((item, index) => ( + ))} )} {hasVisibleBreakdowns && } />} + {showPreviousBars && } />} @@ -506,27 +624,85 @@ const { Tooltip, TooltipProvider } = createChartTooltip< { data: RouterOutputs['chart']['funnel']['current']; visibleBreakdownIds: Set; + hasPrevious: boolean; + hasBreakdowns: boolean; } >(({ data: dataArray, context, ...props }) => { - const data = dataArray[0]!; + const data = dataArray[0]; const number = useNumber(); + if (!data) { + return null; + } const variants = Object.keys(data).filter((key) => - key.startsWith('step:data:'), + key.startsWith('step:data:') ) as `step:data:${number}`[]; const index = context.data[0].steps.findIndex( - (step) => step.event.id === (data as any).id, + (step) => step.event.id === (data as any).id ); // Filter variants to only show visible breakdowns // The variant object contains the full breakdown item, so we can check its ID directly const visibleVariants = variants.filter((key) => { const variant = data[key]; - if (!variant) return false; + if (!variant) { + return false; + } // The variant is the breakdown item itself (with step added), so it has an id property return context.visibleBreakdownIds.has(variant.id); }); + if (!context.hasBreakdowns && context.hasPrevious) { + const currentVariant = data['step:data:0']; + const previousVariant = data['prev_step:data:0']; + + if (!currentVariant?.step) { + return null; + } + + const metric = getPreviousMetric( + currentVariant.step.percent, + previousVariant?.step.percent + ); + + return ( + <> +
{data.name}
+
+
+ Current + + {number.format(currentVariant.step.count)} ( + {number.formatWithUnit(currentVariant.step.percent / 100, '%')}) + +
+ {previousVariant?.step && ( +
+ Previous + + {number.format(previousVariant.step.count)} ( + {number.formatWithUnit(previousVariant.step.percent / 100, '%')} + ) + +
+ )} + {metric && metric.diff != null && ( +
+ + {metric.state === 'positive' + ? 'Improvement' + : metric.state === 'negative' + ? 'Decline' + : 'No change'} + + +
+ )} +
+ + ); + } + return ( <>
@@ -538,30 +714,33 @@ const { Tooltip, TooltipProvider } = createChartTooltip< if (!variant?.step) { return null; } - // Find the original breakdown index for color + // Find the original breakdown index for color (matches chart bar order) const originalBreakdownIndex = context.data.findIndex( - (b) => b.id === variant.id, + (b) => b.id === variant.id ); + let colorIndex = index; + if (visibleVariants.length > 1) { + colorIndex = + originalBreakdownIndex >= 0 ? originalBreakdownIndex : visibleIndex; + } return (
1 ? visibleIndex : index, - ), + background: getChartColor(colorIndex), }} /> -
+
-
-
+
+
{number.formatWithUnit(variant.step.percent / 100, '%')} - + ({number.format(variant.step.count)})
@@ -569,8 +748,9 @@ const { Tooltip, TooltipProvider } = createChartTooltip<
diff --git a/packages/db/src/services/funnel.service.ts b/packages/db/src/services/funnel.service.ts index 944065bc..da6de5ec 100644 --- a/packages/db/src/services/funnel.service.ts +++ b/packages/db/src/services/funnel.service.ts @@ -12,6 +12,17 @@ import { } from './chart.service'; import { onlyReportEvents } from './reports.service'; +/** Display label for null/empty breakdown values (e.g. property not set). */ +export const EMPTY_BREAKDOWN_LABEL = 'Not set'; + +function normalizeBreakdownValue(value: unknown): string { + if (value == null || value === '') { + return EMPTY_BREAKDOWN_LABEL; + } + const s = String(value).trim(); + return s === '' ? EMPTY_BREAKDOWN_LABEL : s; +} + export class FunnelService { constructor(private client: typeof ch) {} @@ -144,20 +155,24 @@ export class FunnelService { ]; } - // Group by breakdown values + // Group by breakdown values (normalize empty/null to "Not set") const series = funnel.reduce( (acc, f) => { if (limit && Object.keys(acc).length >= limit) { return acc; } - const key = breakdowns.map((b, index) => f[`b_${index}`]).join('|'); + const key = breakdowns + .map((b, index) => normalizeBreakdownValue(f[`b_${index}`])) + .join('|'); if (!acc[key]) { acc[key] = []; } acc[key]!.push({ id: key, - breakdowns: breakdowns.map((b, index) => f[`b_${index}`]), + breakdowns: breakdowns.map((b, index) => + normalizeBreakdownValue(f[`b_${index}`]), + ), level: f.level, count: f.count, });