From 64b435c6714a98aa3644dfe623b73416adb90dc7 Mon Sep 17 00:00:00 2001 From: Ayush Jhanwar Date: Wed, 9 Sep 2026 18:08:10 +0530 Subject: [PATCH] fix(funnel): stop the breakdown limit from dropping lower funnel steps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With a breakdown and more distinct values than `limit`, every step of every breakdown row reports an identical count at 100%, so a funnel looks like it converts perfectly end to end. toSeries bails out of the whole reduce once the accumulator holds `limit` keys. The funnel query is ordered by `level DESC`, so the first rows in are the deepest level: the limit is reached while only max-level rows have been seen, and every remaining row is discarded — including the lower-level rows of the series already accepted. Each series is left holding a single row, and fillFunnel accumulates that one count into every step, which also makes totalSessions equal to it and every percent 100%. The limit is meant to cap how many series are returned, so only reject keys that are new. Observed on a report with 185 distinct breakdown values against a limit of 50: a path reported 128/128/128/128/128 and now reports 323/256/151/150/128, matching the underlying ClickHouse rows. Reports with fewer breakdown values than their limit were never affected. Adds funnel.service.test.ts; two of its four cases fail without this change. --- .../db/src/services/funnel.service.test.ts | 84 +++++++++++++++++++ packages/db/src/services/funnel.service.ts | 14 +++- 2 files changed, 94 insertions(+), 4 deletions(-) create mode 100644 packages/db/src/services/funnel.service.test.ts diff --git a/packages/db/src/services/funnel.service.test.ts b/packages/db/src/services/funnel.service.test.ts new file mode 100644 index 000000000..fef772db6 --- /dev/null +++ b/packages/db/src/services/funnel.service.test.ts @@ -0,0 +1,84 @@ +import { describe, expect, it } from 'vitest'; +import { FunnelService } from './funnel.service'; + +// toSeries is pure — it only reshapes rows — so the client is never touched. +const service = new FunnelService({} as any); + +const BREAKDOWNS = [{ name: 'path' }]; + +/** + * The funnel query is ordered by `level DESC`, so every level-5 row arrives + * before any level-4 row, and so on. Rows are shaped as the query returns them. + */ +const rowsOrderedByLevelDesc = ( + paths: string[], + perLevel: Record, +) => + [5, 4, 3, 2, 1].flatMap((level) => + paths.map((path) => ({ + level, + count: perLevel[level]!, + b_0: path, + })), + ); + +describe('FunnelService.toSeries', () => { + it('keeps every level of a series once the limit is reached', () => { + // 3 paths but a limit of 2: the third is dropped, the first two must still + // carry all five of their levels. + const rows = rowsOrderedByLevelDesc( + ['/a', '/b', '/c'], + { 5: 128, 4: 22, 3: 1, 2: 105, 1: 67 }, + ); + + const series = service.toSeries(rows, BREAKDOWNS, 2); + + expect(series).toHaveLength(2); + for (const s of series) { + expect(s.map((row) => row.level).sort()).toEqual([1, 2, 3, 4, 5]); + } + }); + + it('caps the number of series at the limit', () => { + const rows = rowsOrderedByLevelDesc( + ['/a', '/b', '/c', '/d'], + { 5: 1, 4: 1, 3: 1, 2: 1, 1: 1 }, + ); + + expect(service.toSeries(rows, BREAKDOWNS, 2)).toHaveLength(2); + expect(service.toSeries(rows, BREAKDOWNS, 4)).toHaveLength(4); + expect(service.toSeries(rows, BREAKDOWNS, undefined)).toHaveLength(4); + }); + + it('does not flatten a funnel into an all-100% series when limited', () => { + // Regression: dropping the lower-level rows left each series holding only + // its deepest level, which fillFunnel then accumulated into every step — + // rendering an identical count at 100% for all steps. + const rows = rowsOrderedByLevelDesc( + ['/a', '/b'], + { 5: 128, 4: 22, 3: 1, 2: 105, 1: 67 }, + ); + + const [first] = service.toSeries(rows, BREAKDOWNS, 1); + + expect(first).toBeDefined(); + const counts = first! + .sort((a, b) => a.level - b.level) + .map((row) => row.count); + // 67, 105, 1, 22, 128 — distinct per level, not five copies of 128. + expect(counts).toEqual([67, 105, 1, 22, 128]); + expect(new Set(counts).size).toBeGreaterThan(1); + }); + + it('returns a single series when there are no breakdowns', () => { + const rows = [ + { level: 2, count: 5 }, + { level: 1, count: 9 }, + ]; + + const series = service.toSeries(rows, [], 1); + + expect(series).toHaveLength(1); + expect(series[0]).toHaveLength(2); + }); +}); diff --git a/packages/db/src/services/funnel.service.ts b/packages/db/src/services/funnel.service.ts index 381ca6a6e..69efbf9b4 100644 --- a/packages/db/src/services/funnel.service.ts +++ b/packages/db/src/services/funnel.service.ts @@ -203,14 +203,20 @@ export class FunnelService { // 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) => normalizeBreakdownValue(f[`b_${index}`])) .join('|'); if (!acc[key]) { + // The limit caps how many breakdown series we return, so it must only + // reject NEW keys. Bailing out of the whole reduce here would drop the + // remaining rows of series already accepted: the query is ordered by + // level DESC, so those rows are the lower funnel steps, and losing them + // leaves each series holding only its deepest level. fillFunnel then + // accumulates that single row into every step, so every step reports an + // identical count at 100%. + if (limit && Object.keys(acc).length >= limit) { + return acc; + } acc[key] = []; } acc[key]!.push({