Skip to content

Commit e420564

Browse files
sampenningtonclaude
andcommitted
refactor(trends): extract buildDerivedConfigs + computeDashedFromIndex
Move the per-feature derived-config construction out of the four `useMemo` blocks in TrendsLineChart.tsx and into a pure buildDerivedConfigs(results, opts) helper in trendsChartTransforms.ts. Component shrinks; the helper is testable without rendering. New shared helper computeDashedFromIndex(r, opts) keeps the in-progress dashed-tail boundary in one place — read by both buildMainTrendsSeries (for stroke.partial.fromIndex) and buildDerivedConfigs (for the trendline fitUpTo). Earlier they each computed it independently and could drift. buildMainTrendsSeries simplifies: it now returns Series<M> directly instead of a BuiltTrendsSeries<M> wrapper, and stops dimming compare-prev colours — the library's applyComparisonDimming pass picks that up via the new comparisonOf entries that buildDerivedConfigs emits. The wrapper shape only existed to feed the now-deleted buildDerivedTrendsSeries. TrendsLineChart polish: - Hoist TOOLTIP_CONFIG to module scope; the chartConfig memo no longer re-creates the tooltip object every render. - Replace O(n²) indexOf-per-call in TrendsAlertOverlays' getYAxisId callback with a precomputed Map lookup. Tests: - 25 new trendsChartTransforms.test.ts cases for buildDerivedConfigs (composition, MA-trendline gating, fitUpTo derivation, compare-prev mapping) and computeDashedFromIndex. - buildTrendsYAxisConfig log10/showGrid/format-passthrough tests. - schemaGoalLinesToConfigs null/empty/non-empty tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 0e024ca commit e420564

5 files changed

Lines changed: 435 additions & 143 deletions

File tree

frontend/src/scenes/trends/viz/trends-line-chart/TrendsLineChart.tsx

Lines changed: 45 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -4,16 +4,7 @@ import { useCallback, useMemo, type ErrorInfo } from 'react'
44

55
import { buildTheme } from 'lib/charts/utils/theme'
66
import { createXAxisTickCallback, DEFAULT_Y_AXIS_ID, TimeSeriesLineChart } from 'lib/hog-charts'
7-
import type {
8-
ConfidenceIntervalConfig,
9-
MovingAverageConfig,
10-
PointClickData,
11-
Series,
12-
TimeSeriesLineChartConfig,
13-
TooltipContext,
14-
TrendLineConfig,
15-
} from 'lib/hog-charts'
16-
import { ciRanges } from 'lib/statistics'
7+
import type { PointClickData, Series, TimeSeriesLineChartConfig, TooltipConfig, TooltipContext } from 'lib/hog-charts'
178
import { formatPercentStackAxisValue } from 'scenes/insights/aggregationAxisFormat'
189
import { insightLogic } from 'scenes/insights/insightLogic'
1910
import type { SeriesDatum } from 'scenes/insights/InsightTooltip/insightTooltipUtils'
@@ -33,7 +24,7 @@ import { AnnotationsLayer } from './AnnotationsLayer'
3324
import { schemaGoalLinesToConfigs } from './goalLinesAdapter'
3425
import { TrendsAlertOverlays } from './TrendsAlertOverlays'
3526
import { buildTrendsYAxisConfig } from './trendsAxisFormat'
36-
import { buildTrendsSeries } from './trendsChartTransforms'
27+
import { buildDerivedConfigs, buildTrendsSeries } from './trendsChartTransforms'
3728
import type { TrendsSeriesMeta } from './trendsSeriesMeta'
3829
import { TrendsTooltip } from './TrendsTooltip'
3930

@@ -42,6 +33,8 @@ interface TrendsLineChartProps {
4233
inSharedMode?: boolean
4334
}
4435

36+
const TOOLTIP_CONFIG: TooltipConfig = { pinnable: true, placement: 'top' }
37+
4538
const handleChartError = (error: Error, info: ErrorInfo): void => {
4639
posthog.captureException(error, {
4740
feature: 'trends-line-chart',
@@ -130,6 +123,9 @@ export function TrendsLineChart({ context, inSharedMode = false }: TrendsLineCha
130123
]
131124
)
132125

126+
// Built here (not via `config.xAxis.{timezone,interval,allDays}`) so the same instance
127+
// can be threaded into <AnnotationsLayer> below — its visible-tick computation has to
128+
// match what the chart actually draws.
133129
const xTickFormatter = useMemo(
134130
() =>
135131
createXAxisTickCallback({
@@ -156,97 +152,67 @@ export function TrendsLineChart({ context, inSharedMode = false }: TrendsLineCha
156152
[trendsFilter, isPercentStackView, baseCurrency]
157153
)
158154

159-
const confidenceIntervals: ConfidenceIntervalConfig[] | undefined = useMemo(() => {
160-
if (!showConfidenceIntervals || !indexedResults?.length) {
161-
return undefined
162-
}
163-
const ci = (confidenceLevel ?? 95) / 100
164-
return indexedResults.map((r) => {
165-
const [lower, upper] = ciRanges(r.data, ci)
166-
return { seriesKey: String(r.id), lower, upper }
167-
})
168-
}, [showConfidenceIntervals, confidenceLevel, indexedResults])
169-
170-
const movingAverage: MovingAverageConfig[] | undefined = useMemo(() => {
171-
if (!showMovingAverage || movingAverageIntervals === undefined || !indexedResults?.length) {
172-
return undefined
173-
}
174-
return indexedResults
175-
.filter((r) => r.data.length >= movingAverageIntervals)
176-
.map((r) => ({ seriesKey: String(r.id), window: movingAverageIntervals }))
177-
}, [showMovingAverage, movingAverageIntervals, indexedResults])
178-
179-
const trendLines: TrendLineConfig[] | undefined = useMemo(() => {
180-
if (!showTrendLines || !indexedResults?.length) {
181-
return undefined
182-
}
183-
const out: TrendLineConfig[] = []
184-
const includeMa = showMovingAverage && movingAverageIntervals !== undefined
185-
for (const r of indexedResults) {
186-
if (getTrendsHidden(r)) {
187-
continue
188-
}
189-
const isActiveSeries = !r.compare || r.compare_label !== 'previous'
190-
const isInProgress =
191-
!isStickiness && incompletenessOffsetFromEnd !== undefined && incompletenessOffsetFromEnd < 0
192-
const fitUpTo = isInProgress && isActiveSeries ? r.data.length + incompletenessOffsetFromEnd : undefined
193-
out.push({ seriesKey: String(r.id), kind: 'linear', fitUpTo })
194-
if (includeMa && r.data.length >= movingAverageIntervals) {
195-
out.push({
196-
seriesKey: `${r.id}-ma`,
197-
kind: 'linear',
198-
label: `${r.label ?? ''} (Moving avg)`,
199-
})
200-
}
201-
}
202-
return out.length ? out : undefined
203-
}, [
204-
showTrendLines,
205-
showMovingAverage,
206-
movingAverageIntervals,
207-
indexedResults,
208-
getTrendsHidden,
209-
isStickiness,
210-
incompletenessOffsetFromEnd,
211-
])
155+
const derivedConfigs = useMemo(
156+
() =>
157+
buildDerivedConfigs(indexedResults ?? [], {
158+
showConfidenceIntervals,
159+
confidenceLevel,
160+
showMovingAverage,
161+
movingAverageIntervals,
162+
showTrendLines,
163+
isStickiness,
164+
incompletenessOffsetFromEnd,
165+
getHidden: getTrendsHidden,
166+
}),
167+
[
168+
indexedResults,
169+
showConfidenceIntervals,
170+
confidenceLevel,
171+
showMovingAverage,
172+
movingAverageIntervals,
173+
showTrendLines,
174+
isStickiness,
175+
incompletenessOffsetFromEnd,
176+
getTrendsHidden,
177+
]
178+
)
212179

213-
// Compare-previous dimming flows through `buildMainTrendsSeries` for now; a
214-
// follow-up PR (the buildDerivedConfigs extraction) moves it to the library's
215-
// `comparisonOf` pass. Visual parity preserved either way.
216180
const chartConfig: TimeSeriesLineChartConfig = useMemo(
217181
() => ({
218-
xAxis: {
219-
tickFormatter: xTickFormatter,
220-
},
182+
xAxis: { tickFormatter: xTickFormatter },
221183
yAxis: yAxisConfig,
222184
valueLabels: showValuesOnSeries ? { formatter: valueLabelFormatter } : false,
223185
goalLines: goalLineConfigs,
224-
confidenceIntervals,
225-
movingAverage,
226-
trendLines,
186+
...derivedConfigs,
227187
percentStackView: isPercentStackView,
228188
showCrosshair: true,
229-
tooltip: { pinnable: true, placement: 'top' },
189+
tooltip: TOOLTIP_CONFIG,
230190
}),
231191
[
232192
xTickFormatter,
233193
yAxisConfig,
234194
showValuesOnSeries,
235195
valueLabelFormatter,
236196
goalLineConfigs,
237-
confidenceIntervals,
238-
movingAverage,
239-
trendLines,
197+
derivedConfigs,
240198
isPercentStackView,
241199
]
242200
)
243201

202+
// O(1) result→index lookup for the alert overlay's getYAxisId callback. indexOf-per-call
203+
// would be O(n²) over indexedResults.
204+
const indexByResult = useMemo(() => {
205+
const m = new Map<IndexedTrendResult, number>()
206+
;(indexedResults ?? []).forEach((r, i) => m.set(r, i))
207+
return m
208+
}, [indexedResults])
209+
244210
const getYAxisId = useCallback(
245211
(r: IndexedTrendResult) => {
246-
const idx = (indexedResults ?? []).indexOf(r)
212+
const idx = indexByResult.get(r) ?? 0
247213
return showMultipleYAxes && idx > 0 ? `y${idx}` : DEFAULT_Y_AXIS_ID
248214
},
249-
[indexedResults, showMultipleYAxes]
215+
[indexByResult, showMultipleYAxes]
250216
)
251217

252218
const canHandleClick = !!context?.onDataPointClick || !!hasPersonsModal

frontend/src/scenes/trends/viz/trends-line-chart/goalLinesAdapter.test.ts

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,11 @@ import type { Series } from 'lib/hog-charts'
33

44
import type { GoalLine as SchemaGoalLine } from '~/queries/schema/schema-general'
55

6-
import { alertThresholdsToReferenceLines, goalLinesToReferenceLines } from './goalLinesAdapter'
6+
import {
7+
alertThresholdsToReferenceLines,
8+
goalLinesToReferenceLines,
9+
schemaGoalLinesToConfigs,
10+
} from './goalLinesAdapter'
711

812
const makeSeries = (data: number[], overrides: Partial<Series> = {}): Series => ({
913
key: 'a',
@@ -120,4 +124,46 @@ describe('goalLinesAdapter', () => {
120124
expect(alertThresholdsToReferenceLines(lines).map((r) => r.label)).toEqual(['Crossed', 'Above'])
121125
})
122126
})
127+
128+
describe('schemaGoalLinesToConfigs', () => {
129+
it.each<[string, SchemaGoalLine[] | null | undefined]>([
130+
['null', null],
131+
['undefined', undefined],
132+
['empty array', []],
133+
])('returns undefined for %s input', (_, input) => {
134+
expect(schemaGoalLinesToConfigs(input)).toBeUndefined()
135+
})
136+
137+
it('produces one GoalLineConfig per schema goal line, mapping every field', () => {
138+
const lines: SchemaGoalLine[] = [
139+
{
140+
label: 'Target',
141+
value: 100,
142+
displayLabel: true,
143+
borderColor: '#ff0000',
144+
position: 'end',
145+
displayIfCrossed: true,
146+
},
147+
{ label: 'Floor', value: 10 },
148+
]
149+
expect(schemaGoalLinesToConfigs(lines)).toEqual([
150+
{
151+
label: 'Target',
152+
value: 100,
153+
displayLabel: true,
154+
color: '#ff0000',
155+
labelPosition: 'end',
156+
displayIfCrossed: true,
157+
},
158+
{
159+
label: 'Floor',
160+
value: 10,
161+
displayLabel: undefined,
162+
color: undefined,
163+
labelPosition: undefined,
164+
displayIfCrossed: undefined,
165+
},
166+
])
167+
})
168+
})
123169
})

frontend/src/scenes/trends/viz/trends-line-chart/trendsAxisFormat.test.ts

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { AggregationAxisFormat } from 'scenes/insights/aggregationAxisFormat'
33

44
import { CurrencyCode, TrendsFilter } from '~/queries/schema/schema-general'
55

6-
import { trendsFilterToYFormatterConfig } from './trendsAxisFormat'
6+
import { buildTrendsYAxisConfig, trendsFilterToYFormatterConfig } from './trendsAxisFormat'
77

88
const NBSP = ' '
99

@@ -75,3 +75,35 @@ describe('trends y-tick formatter end-to-end', () => {
7575
expect(fmt(50)).toBe('50%')
7676
})
7777
})
78+
79+
describe('buildTrendsYAxisConfig', () => {
80+
it.each([
81+
['log10', 'log'],
82+
['linear', 'linear'],
83+
['auto', 'linear'],
84+
[undefined, 'linear'],
85+
[null, 'linear'],
86+
] as const)('translates yAxisScaleType "%s" to scale "%s"', (input, expected) => {
87+
expect(buildTrendsYAxisConfig(null, false, undefined, { yAxisScaleType: input }).scale).toBe(expected)
88+
})
89+
90+
it('passes showGrid through from extras', () => {
91+
expect(buildTrendsYAxisConfig(null, false, undefined, { showGrid: true }).showGrid).toBe(true)
92+
expect(buildTrendsYAxisConfig(null, false, undefined, {}).showGrid).toBeUndefined()
93+
})
94+
95+
it('spreads the formatter config from trendsFilterToYFormatterConfig', () => {
96+
const trendsFilter: TrendsFilter = {
97+
aggregationAxisFormat: 'duration',
98+
aggregationAxisPrefix: '~',
99+
decimalPlaces: 2,
100+
}
101+
const cfg = buildTrendsYAxisConfig(trendsFilter, false, 'USD' as CurrencyCode)
102+
expect(cfg).toMatchObject({ format: 'duration', prefix: '~', decimalPlaces: 2, currency: 'USD' })
103+
})
104+
105+
it('forces percentage format when isPercentStackView is true', () => {
106+
const trendsFilter: TrendsFilter = { aggregationAxisFormat: 'currency', aggregationAxisPrefix: '$' }
107+
expect(buildTrendsYAxisConfig(trendsFilter, true, 'USD' as CurrencyCode).format).toBe('percentage')
108+
})
109+
})

0 commit comments

Comments
 (0)