feat(sparkline): render simple sparklines via quill charts behind a flag - #70231
Conversation
Records the flag-gated quill Sparkline path shipped in #70231, the decisions made (dropped maximumIndicator/tooltipRowCutoff on the quill path, bar tooltip hit-testing difference), test-infra gotchas, and the next step (medium wave). Generated-By: PostHog Code Task-Id: 8021af43-c731-4cf0-b38b-36c61f0a7c28
🤖 CI report
|
| File | Size | Δ vs base |
|---|---|---|
render-query/src/render-query/render-query.js |
24.19 MiB | 🔺 +1.2 KiB (+0.0%) |
Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report
✅ Eager graph — within budget
How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.21 MiB · 22 files | no change | ███░░░░░░░ 28.2% of 4.29 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
8.10 MiB · 2,974 files | 🔺 +34 B (+0.0%) | █████████░ 87.5% of 9.25 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
Largest files eagerly shipped from src/index.tsx
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 668 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 278.2 KiB | ../node_modules/.pnpm/posthog-js@1.399.2/node_modules/posthog-js/dist/rrweb.js |
| 266.9 KiB | ../node_modules/.pnpm/@posthog+icons@0.37.4_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 234.9 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 221.5 KiB | ../node_modules/.pnpm/posthog-js@1.399.2/node_modules/posthog-js/dist/module.js |
| 164.0 KiB | src/queries/validators.js |
| 154.2 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 105.9 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
⚠️ Dist folder size — 🔺 +24.8 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1283.95 MiB · 🔺 +24.8 KiB (+0.0%)
The flag-gated path in #70231 now renders through an extended quill Sparkline component (bar type, series prop, opt-in tooltip) instead of hand-configured compact BarChart/LineChart. Generated-By: PostHog Code Task-Id: 8021af43-c731-4cf0-b38b-36c61f0a7c28
377b856 to
e1bcb86
Compare
🦔 Hogbox preview · ❌ build failedThe preview didn't come up for commit |
|
Reviews (1): Last reviewed commit: "fix(sparkline): give quill stories a def..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
sampennington
left a comment
sampennington
left a comment
There was a problem hiding this comment.
🤖 sp-ship · 🚫 Blockers
One parity gap would block a clean flag flip: the needsLegacyFeatures gate doesn't account for maximumIndicator/tooltipRowCutoff, so the quill path silently drops them (inline). It's flag-gated, so there's no production impact until rollout. Otherwise the split is clean — normalizeSparklineData and sparklineClassName are honest OnceAndOnlyOnce extractions and the seam test is strong.
Non-blocking nits not posted inline: two nested ternaries carried into normalizeSparklineData/sparklineClassName (the convention disallows them — worth flattening while the extraction is touching them), an unnamed CSS-color regex and the Entry ${i} placeholder label, and the 5-positional-arg normalizeSparklineData signature (a data clump that could take a props slice).
|
✅ Visual changes approved by @sampennington — baseline updated in 6 new. New |
Route the app Sparkline through the quill charts Sparkline when the `quill-sparkline` flag is on, falling back to the legacy Chart.js path for consumers that still need features the quill path doesn't cover yet (selection, highlighted ranges, incomplete bars, reference lines, custom scales). Storybook renders the quill path directly via an exported `QuillSparkline`, since the flag dispatch is unusable there. Built on top of the quill charts Sparkline bar/series/tooltip support. Generated-By: PostHog Code Task-Id: 8021af43-c731-4cf0-b38b-36c61f0a7c28
The quill Sparkline paints onto an absolutely-positioned canvas that adds no
intrinsic size, so a `w-full` wrapper collapsed to a zero-width, unsnapshottable
`#storybook-root` under the Storybook test runner ("element is not visible"),
failing flake verification. Give the wrapper a definite width and height so the
root has real dimensions.
Generated-By: PostHog Code
Task-Id: 8021af43-c731-4cf0-b38b-36c61f0a7c28
521e49f to
422b78e
Compare
6 updated Run: 7bd00f35-8e83-4dd6-8e9a-2f194d5d8246 Co-authored-by: sampennington <56024559+sampennington@users.noreply.github.com>
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
sampennington
left a comment
sampennington
left a comment
There was a problem hiding this comment.
🤖 sp-ship · 🚫 Blockers
One thing to decide before merge: the needsLegacyFeatures gate doesn't account for maximumIndicator (default true), so default-relying consumers silently lose the Y-axis max readout when the flag routes them to the axis-less quill sparkline — either gate on it or confirm the drop is intended for this wave. Otherwise this is a clean, well-tested flag seam: the normalizeSparklineData / sparklineClassName / resolveSparklineColor extractions are faithful shared helpers with no wrong-abstraction branch, the memoization deps are correct, and the tests are behavior-focused and deterministic. Remaining notes are non-blocking — a couple more silently-dropped props (tooltipRowCutoff / hoverColor), chartLabels re-implementing a fallback quill already provides, a zero-height-without-a-height-class edge, and a few coverage gaps.
Revert #70223. The HighlightedRange overlay and the per-bar `bars[i].hatch` fill were added ahead of migrating the Logs/Tracing sparkline consumers, but that migration is being reframed onto TimeSeriesBarChart and has not started, so nothing renders either capability today. Removing the unused code now; it will be rebuilt against real requirements during the population-B migration. #70231 does not depend on this, and the pre-existing hatch machinery (stroke.partial dashes, bars.track) is untouched. Generated-By: PostHog Code Task-Id: 2b83bd23-a83a-4c91-982e-b1e0e9c356ee
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Frontend-only change gated behind a default-off feature flag, no production impact until rollout; thoroughly reviewed (bot and author self-review) with all inline concerns resolved and the one deferred design decision (maximumIndicator gap) explicitly acknowledged as a follow-up, not a regression. Outside risky territory (no auth/billing/migration/API/dependency/CI surface), so independent assurance isn't required and the change is easily reversible via the flag.
- Author wrote 0% of the modified lines and has 403 merged PRs in these paths (familiarity MODERATE).
- 👍 on the PR from greptile-apps[bot], hex-security-app[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 242L, 4F substantive, 363L/5F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (363L, 5F, single-area, feat) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 72f9c2a · reviewed head f1a5c40 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Revert #70223. The HighlightedRange overlay and the per-bar `bars[i].hatch` fill were added ahead of migrating the Logs/Tracing sparkline consumers, but that migration is being reframed onto TimeSeriesBarChart and has not started, so nothing renders either capability today. Removing the unused code now; it will be rebuilt against real requirements during the population-B migration. #70231 does not depend on this, and the pre-existing hatch machinery (stroke.partial dashes, bars.track) is untouched. Generated-By: PostHog Code Task-Id: 2b83bd23-a83a-4c91-982e-b1e0e9c356ee
1 participant
Problem
We want to start rendering PostHog's app
Sparklinethrough the new@posthog/quill-chartscharts stack, but only for the simple cases the quill path covers so far — without regressing consumers that rely on Chart.js-only features.Changes
quill-sparklinefeature flag, the appSparklinerenders via the quill chartsSparkline.QuillSparkline, because the flag dispatch can't be exercised under Storybook (its implicit action args inject anonSelectionChangespy that the dispatch reads as a legacy-only feature).Stacked on #70236 (the quill charts
Sparklinebar/series/tooltip support this builds on) — review and merge that one first.How did you test this code?
Sparkline.test.tsxcovers the flag dispatch and the legacy-feature fallbacks. Ran the appSparklineJest suite locally. Reproduced and fixed the Storybook flake by renderingQuillSparklinedirectly in the quill stories.Automatic notifications
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Originally a single PR covering both the quill charts changes and the app-side flag work; split at the author's request into the base #70236 (quill library) and this PR (app-side, stacked on top) and rebased on latest master. The Storybook flake was root-caused to Storybook auto-injecting an
onSelectionChangeaction arg that flipped the dispatch onto the legacy path; the fix rendersQuillSparklinedirectly in the quill stories.Created with PostHog Code