feat(insights): add BoxPlot chart primitive to hog-charts library#59880
Merged
sampennington merged 13 commits intoMay 27, 2026
Conversation
Contributor
|
🎭 Playwright didn't run on this PR — your changes touch code that could affect E2E behavior, but Playwright is opt-in via label now to keep CI cost down. Add the Most PRs don't need this. Real regressions still get caught on master and fix-forward. |
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
frontend/src/lib/hog-charts/charts/BoxPlot/BoxPlot.tsx:298-305
Redundant full-series recompute on every hover event. `datum` is already in hand at this point, so `computeBoxRect` can be called directly instead of iterating every label via `computeSeriesBoxes` and then searching for the matching index with `.find`.
```suggestion
const box = computeBoxRect({
seriesKey: s.key,
label: drawLabels[hoverIndex],
dataIndex: hoverIndex,
datum,
scales: priv.scales,
grouped: priv.grouped,
})
```
### Issue 2 of 2
frontend/src/lib/hog-charts/charts/BoxPlot/BoxPlot.tsx:404-405
The JSDoc says the function "Falls back to `rgba(0,0,0,alpha)`" for unrecognised colors, but the actual code returns the original `color` string unchanged. The comment is misleading — the canvas will just try to use the raw color string as-is.
```suggestion
/** Best-effort hex/rgb-to-rgba conversion. Returns the original color string unchanged when
* the input isn't a recognised hex/rgb/rgba color — the canvas will attempt to use it as-is. */
```
Reviews (1): Last reviewed commit: "feat(insights): add BoxPlot chart primit..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
|
Size Change: 0 B Total Size: 80.2 MB ℹ️ View Unchanged
|
Contributor
|
✅ Visual changes approved by @sampennington — baseline updated in 4 changed. |
sampennington
commented
May 25, 2026
Contributor
Author
sampennington
left a comment
sampennington
left a comment
There was a problem hiding this comment.
🤖 Automated review
Verdict: Needs changes — small ones
Clean three-layer split (pure geometry / canvas primitives / React wrapper) that mirrors BarChart and PieChart. Two findings are worth fixing before ship: (1) whisker caps draw on the box outline when min == p25 or max == p75, producing a visible cross-bar in the no-whisker case; (2) the consumer-facing tooltip callback leaks the internal BoxPlotAdaptedMeta adapter shape — once shipped, that's awkward to refactor away. Everything else is quick polish.
Next steps
- Guard whisker-cap draw against the no-whisker case (and add a visual-regression test that samples the cap pixel).
- Replace
tooltip: (ctx: TooltipContext<BoxPlotAdaptedMeta<Meta>>) => …with aBoxPlotTooltipContextshape parallel toBoxPlotClickData— exposeBoxPlotSeries<Meta>andBoxPlotDatumdirectly. - Mop up: drop unused exports (
nearestBoxIndex,cursorInsideBoxRect), replace localhexToRgbawith the existingdimHex, decide on one channel betweenadaptedSerieswhisker-append andvalueRangeSeries, fixoriginalSeries's silentseries[0]fallback, memoize the inlineconfigobject.
Non-anchored findings
- JSDoc field-restating comments are pervasive across
BoxPlot.tsx,BoxPlotTooltip.tsx,computeBoxLayout.ts, andcanvas-renderer.ts(e.g.,/** Lower whisker endpoint. */onmin,/** Median — drawn as a line across the box. */onmedian, etc.). Per the project comment guidance, default to deleting these; keep only the few that carry non-obvious info (e.g., theuseChartMarginswhisker-append trick, thenull-return cases oncomputeBoxRect). - Narrating inline comments in
drawBox(// Whisker stem above the box,// Whisker caps at min and max,// Box rectangle (p25 → p75) — fill then outline,// Median line across the box,// Mean marker — filled circle outlined in the series color) — restate the next 2–4 lines; delete. - Test helper branching (
computeBoxLayout.test.tsandboxes-under-cursor.test.ts) — both files define amakeScaleshelper withif (d) { … }guards and aseriesSpec.length > 1 ? 'grouped' : 'stacked'default. Either extract to a shared non-test fixture file (where branching is fine) or passbarLayoutexplicitly at every call site. BoxPlotInneris ~300 lines — extractadaptedSeries/valueRangeSeries/datumsByKeyto auseBoxPlotAdapterhook or module-scope helpers, same shape PieChart uses withcomputePieLayout.ts.- Magic-number defaults duplicated 3× (
meanRadius=3,whiskerCapRatio=0.6,boxStrokeWidth=1.5) — defined inBoxPlot.tsxdefaults, again indrawBoxoptions defaults, again in JSDoc; pull to exported named constants neardrawBox. - Global
isFinite(coerces) used throughoutBoxPlot.tsxandcomputeBoxLayout.ts; the test files correctly useNumber.isFinite. Switch the production code to match —isFinite('1')returnstrue, which matters if a stray string slips intodata. - Stories pass
color: ''everywhere (BoxPlot.stories.tsx) — the empty-string sentinel relies onChart.tsx's falsy fallback totheme.colors[i].Series.coloris optional; recommend stories omit it entirely so they document the actual consumer ergonomics. - Library generality is clean — no kea, no PostHog imports, no
lib/utils, no module-level mutable state. Same external dependencies (d3, React) as existing chart types.
Reviewers ran: hog-charts, react, personal, reuse, quality, efficiency. Inline comments above are tagged with the reviewer that raised them and the level of concern. The generic code-reviewer was skipped — all changed paths are under frontend/src/lib/hog-charts/, so hog-charts-reviewer supersedes it.
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
posthog | 9aac728 | May 25 2026, 03:19 PM |
sampennington
commented
May 26, 2026
Contributor
Author
sampennington
left a comment
sampennington
left a comment
There was a problem hiding this comment.
🤖 Automated review
Verdict: Needs changes
Three blocking items: (1) core/canvas-renderer.ts imports a type from charts/BoxPlot/ — a core → charts layering inversion; (2) adaptedSeries smuggles whisker min/max past labels.length in Series.data, breaking the Series data[i] ↔ labels[i] invariant; (3) in grouped mode onBoxClick reports the first-visible series, not the box actually under the cursor (a real UX bug for the persons-modal wiring the follow-up needs). Plenty of strong primitives underneath — drawBoxes batches paths well, the three-layer test split is clean, error boundary + nativeTooltip mode are good additions — but the layering, click semantics, and Series-data hack should land before the wiring follow-up.
Next steps:
- Move
BoxRectintocore/canvas-renderer.ts(mirroringBarRect) and stop importing fromcharts/incore/. - Drive y-domain widening entirely through the existing
valueRangeSeries/stackedSerieschannel — don't push whisker extremes ontoSeries.data. - Fix
onBoxClick's primary series in grouped mode, or explicitly document the gap.
Non-anchored findings
Should Fix
computeBoxLayout.ts(top of file) —BoxPlotDatum extends Pick<SchemaBoxPlotDatum, ...>reaches into~/queries/schema/schema-generalfrom insidelib/hog-charts/. This is the only PostHog coupling in the library; even as a type-only Pick it crosses the "library is self-contained" line. Prefer defining a structural six-number type here and letting adapters map onto it.index.ts:25-37— wide public surface (10+ named exports including internal types likeBoxPlotAdaptedMetaand three geometry helpers). NeighbourPieChartexports only the component + config/props. Narrow before this surface ships, because deleting later is hard.boxes-under-cursor.test.ts(helper) — same branching patterns ascomputeBoxLayout.test.ts(ternary default param aroundbarLayout,if (d)filter,scales.group!('b') ?? 0nullish-coalesce as control flow in an assertion). Same fixes apply.
Suggestions
BoxPlot.tsx(band-axis math) andBarChart.tsx:43-58—bandCenter/groupedBarCenterare reimplemented inline here (in bothcreateScales.xanddrawStatic.xScale). Promote them ontoBarScaleSetor export fromcore/bar-layout.ts.BoxPlot.tsx:26-32—BoxPlotPrivatestashesscales,datumsByKey, andgrouped. The latter two are derived from theseriesprop which the draw callbacks already capture via closure. Stash onlyscales, matchingBarChartPrivate.BoxPlotTooltip.tsx— its outer chrome (px-3 py-2 rounded-lg shadow-lg text-[13px]+DEFAULT_TOOLTIP_BG/DEFAULT_TOOLTIP_COLOR+ the color-swatch span) is a third copy of the same JSX inoverlays/DefaultTooltip.tsxandcharts/PieChart/PieTooltip.tsx. Extract a<TooltipFrame>shared shell.BoxPlotTooltip.tsx(entry build) — unlikeBarTooltip, it doesn't narrowseriesDataby cursor in grouped mode, so hovering in the gap between sub-band boxes still shows every series's stat table. The existingseriesKeysAtCursorcan drive the same narrowing.computeBoxLayout.ts(computeBoxRect) — two consecutiveisFiniteblocks and two consecutive scale-lookup-null blocks could be one each.computeBoxLayout.ts(computeBoxRect) — silently swapsp25 > p75/min > maxviaMath.min/Math.maxand renders a misleading box. Consider returningnullinstead — a flipped six-number summary is almost certainly a data bug, not legitimate input.BoxPlot.tsx(BoxPlotConfig) — the three new optionsmeanRadius/whiskerCapRatio/boxStrokeWidthduplicate theDrawBoxOptionsdefaults (meanRadius,whiskerCapRatio,lineWidth) incanvas-renderer.ts. Consider a singleboxStyle?: Partial<DrawBoxOptions>instead.BoxPlot.tsx(groupedderivation) —series.filter(...).length > 1allocates a filtered array just to check length;series.some(...)+ a counter would be one pass. Tiny.BoxPlotTooltip.tsx(root) —<>{userTooltip(ctx)}</>wraps aReactNodein a Fragment to satisfy theReactElement | nullreturn type. Widen the return type toReactNodeinstead.- Tests don't pin single-series boxplots in the
'stacked'barLayoutbranch — worth one explicit case so thegrouped=false → barLayout='stacked'mapping is locked in.
Nits — unnecessary comments
- Several JSDoc lines on
BoxPlotClickDatafields (seriesIndex,dataIndex,label,datum,crossSeriesData) restate the field name in prose. - Per-field JSDocs on
BoxPlotSeries(key,label,data,meta,visibility) similarly restate types. Thecoloranddata-null docs encode real contracts — keep those. BoxPlot.tsx(onBoxClickJSDoc) references "the product layer wires this to the persons modal in the BoxPlot insight" — drops downstream wiring context that doesn't belong in the library.BoxPlotTooltip.tsx(ROWSconstant) — "Rows shown per box, in the same order the legacy BoxPlotChart used" — references migration context that won't age well.BoxPlot.tsx(aroundvalueRangeSeries/adaptedSeries) — the same "why we widen the y-domain" explanation appears twice; pick one site.canvas-renderer.ts(insidedrawBoxes) — the numbered step comments (// 1. Whisker stems,// 4. Median lines) mostly restate the next line; keep only the load-bearing ones (the cap-skip reason, the per-box mean justification).
Positive observations
_privatestash slot used correctly viaChartScales._private, matching BarChart's pattern — nouseReffor chart-private state.drawBoxesexplicitly batches stems / caps / box outlines / medians into sharedbeginPath+strokecalls (4+N instead of 5N).- Three-layer test split (
computeBoxLayout.test.tspure geometry,boxes-under-cursor.test.tspure hit-test,BoxPlot.test.tsxrendered component) is exactly the right shape. - Degenerate-input coverage (inverted p25 > p75, all-equal samples, non-finite stats, null entries) is thorough.
ChartErrorBoundarywired withonErrorforwarding.nativeTooltipopt-in inrenderHogChartis a clean accessor extension — lives inlib/hog-charts/testing/so every chart inherits it.- No new module-level mutable state, no kea, no runtime PostHog imports (modulo the schema type Pick noted above).
Reviewers: hog-charts-reviewer, react-reviewer, personal, reuse, quality, efficiency. Inline comments above are tagged with the reviewer that raised them and the level of concern. Generic code-reviewer was skipped because 100% of the diff sits under lib/hog-charts/ and hog-charts-reviewer supersedes it there.
| @@ -1,8 +1,11 @@ | |||
| import * as d3 from 'd3' | |||
|
|
|||
| import type { BoxRect } from '../charts/BoxPlot/computeBoxLayout' | |||
Contributor
Author
There was a problem hiding this comment.
🤖 [hog-charts] · Critical
core/canvas-renderer.ts lives in core/, but this imports BoxRect from charts/BoxPlot/computeBoxLayout (and re-exports it on line 7). The hog-charts architecture is one-way: charts/ depends on core/, never the reverse. BarRect follows that rule — its definition lives in core/canvas-renderer.ts next to drawBars. Move BoxRect here too (and have computeBoxLayout.ts re-export it for ergonomics), or move drawBox* out of core/ into charts/BoxPlot/. The first is more consistent with BarRect.
Contributor
Author
There was a problem hiding this comment.
Fixed in ee66989 — moved BoxRect next to BarRect in core/canvas-renderer.ts. computeBoxLayout.ts re-exports it so consumers that pull the geometry helpers don't need a second import.
Comment on lines
+125
to
+145
| const adaptedSeries = useMemo<Series<BoxPlotAdaptedMeta<Meta>>[]>( | ||
| () => | ||
| series.map((s) => { | ||
| const data: number[] = Array.from({ length: labels.length }, (_, i) => { | ||
| const datum = s.data[i] | ||
| return datum && isFinite(datum.median) ? datum.median : Number.NaN | ||
| }) | ||
| let seriesMin = Infinity | ||
| let seriesMax = -Infinity | ||
| for (const datum of s.data) { | ||
| if (!datum) { | ||
| continue | ||
| } | ||
| if (isFinite(datum.min) && datum.min < seriesMin) { | ||
| seriesMin = datum.min | ||
| } | ||
| if (isFinite(datum.max) && datum.max > seriesMax) { | ||
| seriesMax = datum.max | ||
| } | ||
| } | ||
| if (isFinite(seriesMin)) { |
Contributor
Author
There was a problem hiding this comment.
🤖 [hog-charts+reuse] · Should Fix
adaptedSeries.data carries labels.length median entries — and then pushes one or two synthetic min/max values past the end purely to coerce useChartMargins → seriesValueRange into sizing the y-tick column for the whisker range. This breaks the long-held invariant in this library that series.data[i] corresponds to labels[i], which is what every other consumer of Series assumes (e.g. createStackedData iterates labels.length).
The y-domain is already widened cleanly via valueRangeSeries + stackedSeries: ... in createBarScales. Extend the same channel to useChartMargins (an optional explicit value-range, or a parallel valueRangeSeries arg) — one concept, two call sites — and drop the data-array tail. At minimum, leave a comment that this is a workaround, not the API.
Contributor
Author
There was a problem hiding this comment.
Fixed in ee66989 — dropped the .push(seriesMin/Max) tail. Added a valueRangeSeries?: Series[] prop to ChartProps that forwards to a new valueRangeSeries override on useChartMargins, so the synthetic min/max series feeds both createBarScales (as stackedSeries) and the margin sizer through the same channel. adaptedSeries.data is now length-stable at labels.length.
| [tooltip, grouped] | ||
| ) | ||
|
|
||
| const onPointClick = useCallback( |
Contributor
Author
There was a problem hiding this comment.
🤖 [hog-charts] · Should Fix
In grouped mode the upstream buildPointClickData picks the primary series by visible-series index, not by which sub-band the cursor was actually over. So clicking series B's box in a grouped layout fires onBoxClick({ series: A, ... }). The test at BoxPlot.test.tsx:441 codifies this as expected — for bars that's defensible, but for box plots the user is unambiguously clicking one box, and onBoxClick is what wires the persons modal. Either reuse seriesKeysAtCursor here at click time to pick the under-cursor series (same as the hover highlight already does), or document on BoxPlotClickData.series that it's "the first visible series in this column" so the product layer disambiguates via crossSeriesData + cursor x.
Contributor
Author
There was a problem hiding this comment.
Documented in ee66989. Went with option (b): BoxPlotClickData.series now explicitly carries "first visible series at the column, not necessarily under-cursor", with a pointer to crossSeriesData + cursor x for the precise box. Matches BarChart's contract — the persons modal layer can disambiguate.
| boxStrokeWidth = 1.5, | ||
| } = config ?? {} | ||
|
|
||
| const grouped = useMemo(() => series.filter((s) => !s.visibility?.excluded).length > 1, [series]) |
Contributor
Author
There was a problem hiding this comment.
🤖 [react] · Performance
useMemo on a cheap boolean — series.filter(...).length > 1 is O(small) and produces a primitive. Memoizing a value type buys nothing downstream because boolean is compared by value, not reference. Replace with a plain const grouped = series.filter(...).length > 1 (or series.some(...) + counter for one pass).
Contributor
Author
There was a problem hiding this comment.
Fixed in ee66989 — const grouped = series.filter(...).length > 1.
Comment on lines
+395
to
+398
| const chartConfig = useMemo( | ||
| () => ({ ...config, axisOrientation: 'vertical' as const, xTickFormatter }), | ||
| [config, xTickFormatter] | ||
| ) |
Contributor
Author
There was a problem hiding this comment.
🤖 [react+quality] · Performance
Two issues here:
-
configis almost always passed inline by callers (config={{ showGrid: true }}in stories — likely the same in real usage), so it's a fresh object each render and thisuseMemonever hits. The downstream<Chart>then receives a newchartConfigref every render anyway. Either drop the memo, or destructureconfig's primitive fields into the deps to actually stabilize it. -
xTickFormatterwas already destructured out ofconfigabove and the spread...configcarries the same value. The explicitxTickFormatteroverride is a no-op.
Contributor
Author
There was a problem hiding this comment.
Fixed in ee66989 on both counts. Dropped the chartConfig memo entirely — config is constructed inline at the <Chart> call site ({ ...config, axisOrientation: 'vertical' }). And removed the xTickFormatter destructure: it's already carried in ...config, so the explicit override was a no-op.
Comment on lines
+419
to
+425
| /** Translucent variant of a series colour for box fills, mean markers, and the hover overlay. | ||
| * Uses `d3.color` so we cover hex (3/6 digit), `rgb(…)`, `rgba(…)`, named colors, and HSL — | ||
| * the previous hand-rolled hex parser only handled a subset of those. Falls back to the raw | ||
| * string when `d3.color` returns `null`, matching the BarChart pattern. */ | ||
| function dimColor(color: string, alpha: number): string { | ||
| return d3.color(color)?.copy({ opacity: alpha }).toString() ?? color | ||
| } |
Contributor
Author
There was a problem hiding this comment.
🤖 [reuse+quality] · Suggestion
dimColor is a generic color-with-alpha helper — BarChart.tsx does the same thing inline with d3.color(s.color).copy({ opacity: ... }).toString(). Promote to a small core/color-utils.ts (or onto canvas-renderer.ts) so BarChart can switch to it.
Also the four-line JSDoc above ("the previous hand-rolled hex parser only handled a subset") references prior implementation that no longer exists in the diff — trim to a single line like // Translucent series color (alpha applied) — handles all CSS color forms.
Contributor
Author
There was a problem hiding this comment.
Promoted in ee66989 — dimColor lives in core/color-utils.ts with a one-line JSDoc. Did not migrate BarChart's two inline call sites in this PR: one of them needs the rgba(0,0,0, alpha) fallback (the comment there is load-bearing — falling back to s.color would paint an opaque plot-height block) and the other uses .darker(0.6) rather than .copy({ opacity }). Both worth their own pass, but not a drop-in.
| } | ||
|
|
||
| /** Paint a single box-and-whisker. Takes a pre-laid-out {@link BoxRect}; no scale access. */ | ||
| export function drawBox(ctx: CanvasRenderingContext2D, box: BoxRect, options: DrawBoxOptions): void { |
Contributor
Author
There was a problem hiding this comment.
🤖 [reuse+quality] · Suggestion
drawBox (singular) is a one-line wrapper around drawBoxes(ctx, [box], options) and is never called — BoxPlot.tsx only imports drawBoxes and drawBoxHighlight. BarChart only exposes drawBars (no singular sibling). Delete it, or document why the plural-only sibling needs a singular counterpart for downstream consumers.
Contributor
Author
There was a problem hiding this comment.
Deleted in ee66989 — singular drawBox is gone; BoxPlot.tsx only used drawBoxes and drawBoxHighlight.
| labels: string[], | ||
| seriesSpec: { key: string; data: (BoxPlotDatum | null)[] }[], | ||
| dims = PIXEL_TEST_DIMENSIONS, | ||
| barLayout: 'grouped' | 'stacked' = seriesSpec.length > 1 ? 'grouped' : 'stacked' |
Contributor
Author
There was a problem hiding this comment.
🤖 [personal] · Should Fix
Ternary inside a test-helper default parameter: barLayout: 'grouped' | 'stacked' = seriesSpec.length > 1 ? 'grouped' : 'stacked'. Tests should be linear and deterministic — a conditional default means the helper does different things depending on input shape, and the test reader can't see which branch ran without reasoning about the caller.
Make barLayout a required parameter so each call site passes its layout explicitly, or split into makeGroupedScales / makeStackedScales.
Contributor
Author
There was a problem hiding this comment.
Fixed in ee66989 — barLayout is a required 3rd parameter on makeScales and every call site passes either 'stacked' or 'grouped' explicitly.
Comment on lines
+33
to
+39
| for (const d of s.data) { | ||
| if (d) { | ||
| mins.push(d.min) | ||
| maxs.push(d.max) | ||
| } | ||
| } | ||
| if (mins.length > 0) { |
Contributor
Author
There was a problem hiding this comment.
🤖 [personal] · Should Fix
Branching inside the test helper: if (d) { ... } filtering null datums, plus two if (mins.length > 0) / if (maxs.length > 0) guards before valueRange.push. Every test fixture in this file supplies non-null entries, so both length guards are unconditionally true and the if (d) only matters for one fixture.
Fix shape: compute data arrays without nulls up front in each test (or use s.data.flatMap(d => d ? [{...}] : []) once at the call site so the helper has no branches). Or delete the helper and inline the two cases that need it as separate parameterized fixtures.
Contributor
Author
There was a problem hiding this comment.
Fixed in ee66989 — makeScales now takes BoxPlotDatum[] (no nulls). The min/max collection is a branchless flatMap. The one test that exercises null datums (drops null and unrenderable indices) passes the nullable list to computeSeriesBoxes and a filtered, non-null copy to makeScales — the helper has no branches.
Comment on lines
+256
to
+264
| try { | ||
| const { chart } = renderHogChart( | ||
| <BoxPlot series={TWO_SERIES} labels={LABELS} theme={THEME} tooltip={tooltip} onError={onError} /> | ||
| ) | ||
| chart.hoverAtIndex(1) | ||
| await waitFor(() => expect(onError).toHaveBeenCalled()) | ||
| } finally { | ||
| consoleErrorSpy.mockRestore() | ||
| } |
Contributor
Author
There was a problem hiding this comment.
🤖 [personal] · Should Fix
try { … } finally { consoleErrorSpy.mockRestore() } in a test body is exactly the branching shape that should live in setup hooks. Hoist into the describe('error handling') block:
let consoleErrorSpy: jest.SpyInstance
beforeEach(() => { consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}) })
afterEach(() => { consoleErrorSpy.mockRestore() })The test body collapses to its actual assertions, and you can't forget the cleanup on a future test added to this describe.
Contributor
Author
There was a problem hiding this comment.
Fixed in ee66989 — hoisted consoleErrorSpy into beforeEach/afterEach on the error handling describe. Future tests added there inherit the cleanup automatically.
gesh
approved these changes
May 27, 2026
Member
gesh
left a comment
gesh
left a comment
There was a problem hiding this comment.
Looks good to me! 🙌
One more comment from the bot:
createBarScales clamps the low end to 0 (scales.ts:379), so boxes for data far from zero get squashed — the legacy Chart.js box plot auto-fits instead (BoxPlotChart.tsx:275). The call site is BoxPlot.tsx:186; BoxPlot likely needs its own tight-fit value scale rather than bar semantics.
| /** Stash slot — survives a render via `ChartScales._private` so drawStatic/drawHover | ||
| * can read the d3 scales and the per-series datums lookup without recomputing them. */ | ||
| interface BoxPlotPrivate { | ||
| __boxPlot: { |
Member
There was a problem hiding this comment.
Do we use the double underscore convention across the frontend
Contributor
Author
There was a problem hiding this comment.
afaik not widely, but it is used in elsewhere in this library already
Member
There was a problem hiding this comment.
Yeah, good, makes sense to stick to it
| scales, | ||
| grouped, | ||
| }: ComputeBoxRectOptions): BoxRect | null { | ||
| if (!isFinite(datum.min) || !isFinite(datum.max) || !isFinite(datum.p25) || !isFinite(datum.p75)) { |
Member
There was a problem hiding this comment.
From the bot:
Use Number.isFinite — the global coerces null→0 (passes), silently rendering a box at 0 instead of skipping it. Fix computeBoxLayout.ts:87,90,104,107 and BoxPlot.tsx:132,156,159.
Contributor
Author
There was a problem hiding this comment.
Fixed in a2e41c5 — swapped all seven sites (the four in computeBoxRect and the three in BoxPlot.tsx you flagged) to Number.isFinite. The global isFinite(null) evaluating true was exactly the silent-box-at-0 footgun you described. Tests still green (41/41 in the BoxPlot suite).
Adds a canvas-drawn box-and-whisker chart over a band x-axis. Each box shows the p25–p75 box, a median line, a mean marker, whiskers to min/max, hover-driven highlighting, and a per-x-position tooltip with the six canonical stats. Supports multiple series via grouped side-by-side boxes inside the same band, sharing the BarChart band/group scale infrastructure. Includes: - BoxPlotDatum / BoxPlotSeries types — per-point six-number summaries. - computeBoxRect / computeSeriesBoxes — pure geometry helpers. - drawBox / drawBoxHighlight — canvas drawing primitives. - boxes-under-cursor — band-axis hit-test mirroring BarChart's pattern. - BoxPlot React component — wraps the Chart base, computes scales from whisker min/max via the createBarScales stackedSeries hook, and exposes an onBoxClick callback (no persons-modal here — that lives at the product layer in a follow-up). - BoxPlotTooltip — default tooltip with Max → p75 → Median → Mean → p25 → Min rows per series. - Unit tests for geometry and hit-testing (degenerate cases, grouping, null data) and component tests for canvas mount, tooltip context, and click data. - Storybook stories: single series, multi-series grouped, no-grid, with-gaps, and empty. Library only — no Trends.tsx changes or feature flag wiring. Generated-By: PostHog Code Task-Id: 64e06e82-8970-4b3d-9ad5-0d8dde58147e
Applies oxfmt formatting (CI was failing on format check), plus two trusted-bot review suggestions from Greptile: - Replace the redundant `computeSeriesBoxes` + `.find` in drawHover with a direct `computeBoxRect` call — `datum` is already in hand on the hover path, no need to recompute every box. - Fix the misleading JSDoc on `hexToRgba`: the function actually returns the original color unchanged on unrecognised input, not the documented `rgba(0,0,0,alpha)` fallback. No behavior change. Generated-By: PostHog Code Task-Id: 64e06e82-8970-4b3d-9ad5-0d8dde58147e
`useChartMargins` derives the y-tick column width from `seriesValueRange(series)`. BoxPlot's adapted inner Series only carried medians on `data`, so when whiskers extend much further than medians (e.g. medians ≤60 but max ~280), the margin estimator sized the y-tick column for narrow labels like "0, 20, 40, 60" while the actual axis rendered "0, 50, …, 250, 300" — the wider labels got clipped. Append each series's whisker min and max to `data` past `labels.length`. `seriesValueRange` walks every entry so margin estimation sees the real range, while interaction (which indexes `[0, labels.length)`) and the draw path (which reads `meta.datums`) never touch the extras. Adds a regression test that builds a series with median-vs-whisker divergence and asserts the rendered y ticks reach the whisker scale. Generated-By: PostHog Code Task-Id: 64e06e82-8970-4b3d-9ad5-0d8dde58147e
- canvas-renderer: skip whisker caps when stems are skipped, and batch per-series path/stroke pairs from 5N to 4+N (whisker stems, caps, box outlines, medians each become one shared path; mean markers stay per-box due to fill+stroke pairing) - BoxPlot: use d3.color for translucent fills (drops bespoke hexToRgba which missed named colors / HSL); memoize chart config; build seriesByKey map for O(1) cross- series lookup and drop the silent series[0] fallback; hoist mousemove adapter out of the hot path so seriesKeysAtCursor doesn't reallocate per frame; drop the redundant whisker-min/max appended to adaptedSeries.data (valueRangeSeries already drives the y-domain); Omit axisOrientation from BoxPlotConfig since BoxPlot is vertical-only - BoxPlot tooltip API: expose BoxPlotAdaptedMeta and BoxPlotTooltipContext on the barrel so consumers don't have to redeclare the adapter shape - BoxPlot interaction: split seriesKeysAtCursor into band-only (cheap, hit-test path) and full-rect (highlight path) via computeBoxBand; drop unused nearestBoxIndex / cursorInsideBoxRect helpers and their tests - BoxPlotDatum: alias from queries/schema-general so the chart type stays in lock- step with the canonical schema type - BoxRect: single home in computeBoxLayout.ts; canvas-renderer re-exports for backwards compatibility - BoxPlotTooltip: stable React key from series.key (was index) - Tests: tighten row-extraction helpers in BoxPlotTooltip.test to non-null asserts; drop optional chaining in BoxPlot.test tooltip meta assertion Generated-By: PostHog Code Task-Id: 2acf2f65-f758-4264-ab6b-5c8a3e667816
Restore the whisker min/max append on adaptedSeries.data — useChartMargins reads seriesValueRange(series) independently of the d3 y-domain that valueRangeSeries drives, so both channels are needed. Without the append the y-tick column was sized for medians and "300" got clipped on the left. Generated-By: PostHog Code Task-Id: 2acf2f65-f758-4264-ab6b-5c8a3e667816
The empty boxplot snapshot just shows a y-axis with no data — not a meaningful regression-test target, and the default tick labels clip against the left edge in the snapshot. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
12 updated Run: a33927cf-cb13-4f7b-b623-6442e6cf7341 Co-authored-by: sampennington <56024559+sampennington@users.noreply.github.com>
Add `nativeTooltip` option to `renderHogChart` that skips the tooltip-prop
interception, letting the chart render its own internal tooltip component
(e.g. BoxPlotTooltip) so tests can assert on the real DOM output. The
default behavior — context capture via a DefaultTooltip fallback — is
unchanged for existing callers.
Migrate the BoxPlotTooltip row-ordering, header, grouped/single-series,
and user-tooltip pass-through tests into `BoxPlot.test.tsx` driven through
`renderHogChart(..., { nativeTooltip: true })` + `waitForHogChartTooltip()`.
This brings the BoxPlot tests in line with how Bar/Pie/TimeSeriesLine
exercise their tooltips, and the `grouped` decision is now driven by the
chart's own series-count logic rather than passed by hand.
Drop the null-datum-returns-null case: at the chart layer the hover only
fires on a band hit, so the empty-tooltip path isn't reachable through
the chart. The existing `tolerates null entries in series data` test
still covers crash-free rendering when datums are null.
Generated-By: PostHog Code
Task-Id: e84e756c-13e5-4591-98db-12fee0845eb1
Generated-By: PostHog Code Task-Id: e84e756c-13e5-4591-98db-12fee0845eb1
- Move BoxRect into core/canvas-renderer (charts/ shouldn't be a core/ dep) - Drop adaptedSeries.data tail hack; thread valueRangeSeries through Chart into useChartMargins so series.data[i] <-> labels[i] invariant holds - Drop hitTestSeries adapter (structural identity of series) - Drop chartConfig useMemo (defeated by inline config); xTickFormatter is already spread via ...config - grouped: useMemo on a bool -> plain const - Promote dimColor to core/color-utils - Delete unused singular drawBox wrapper - Clarify BoxPlotClickData.series grouped-mode semantics - Test helpers: make barLayout required, strip unreachable branches, hoist consoleErrorSpy into beforeEach/afterEach
The global `isFinite` coerces `null` → `0`, so a null field on a BoxPlotDatum slipped past the guard and silently rendered a box at value 0. Switching to `Number.isFinite` makes the guard strictly numeric — null/undefined fields are now skipped rather than rendered at 0. Touches the seven sites Gesh flagged: the four guards in `computeBoxRect` and the three in `BoxPlot.tsx` (the `adaptedSeries` median lookup and the `valueRangeSeries` min/max collectors). Generated-By: PostHog Code Task-Id: c0dd6782-d14c-44c3-bf81-5a67b128f749
sampennington
force-pushed
the
posthog-code/hog-charts-boxplot-primitive
branch
from
a2e41c5 to
4e071b9
Compare
4 updated Run: 9089df71-ebcc-406d-92d7-b122d6ef32a2 Co-authored-by: sampennington <56024559+sampennington@users.noreply.github.com>
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
2 participants
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
The hog-charts library is the new in-house chart toolkit gradually replacing Chart.js for trends visualizations. The legacy BoxPlot trend uses Chart.js via the
chartjs-chart-boxplotplugin. To migrate it, we need a native hog-charts BoxPlot primitive that follows the library's layering conventions (pure geometry + canvas drawing primitives + hit-testing + React component) and reuses the BarChart band/group scale infrastructure.This PR only adds the library primitive. Wiring it into the BoxPlot insight behind a feature flag is a separate follow-up —
Trends.tsx, feature flags, and persons-modal integration are not touched here.Changes
Adds a boxplot chart
How did you test this code?
Storybook snapshots. It's no used anywhere yet.
Publish to changelog?
do not publish to changelog
Docs update
No docs update needed — this is a library primitive that isn't user-visible until the BoxPlot insight migration follow-up.
🤖 Agent context
BarChart(closest template — band+group scales, grouped slot hit-testing) andPieChart(most recent new-chart-type onboarding — single-canvas, dedicated geometry module + tests). The hardest decision was how to reconcileSeries.data: number[]with a per-point six-number summary. I considered (1) building a non-Chart-base component, (2) overloadingseries.datawith flattened arrays, and (3) adaptingBoxPlotSeriestoSeries<{ datums }>internally with medians ondataand synthetic min/max series fed intocreateBarScales's existingstackedSeriesoption to drive the y-domain. Picked (3) — it reuses the entireChartinfrastructure (interaction, tooltip lifecycle, error boundary, axis labels, overlays) without forking, and the meta carries the original datums all the way to the tooltip / click callbacks.renderHogChartintercepts the component'stooltipprop to captureTooltipContext, so the component-level test asserts on the structured context (datums reachable throughseries.meta) and a separateBoxPlotTooltip.test.tsxrenders the tooltip directly with a hand-crafted context to verify the six-stat row ordering — same split RTL would suggest for any context-dependent render.onBoxClicksemantics: matches BarChart'sonPointClick—seriesis the primary (first-visible) series, with all visible series at the column incrossSeriesData. The product layer can narrow to the under-cursor series viacrossSeriesDataif needed.