Repository navigation
feat(insights): show annotations on SQL charts - #90013
Conversation
sampennington
added
the
skip-inkeep-docs
|
😎 Merged successfully - details. |
🤖 CI report
|
| File | Size | Δ vs base |
|---|---|---|
posthog-app/_parent/products/engineering_analytics/frontend/scenes/EngineeringAnalyticsScene.js |
71.7 KiB | 🔺 +9.9 KiB (+16.1%) |
render-query/src/render-query/render-query.js |
21.77 MiB | 🔺 +5.1 KiB (+0.0%) |
posthog-app/src/scenes/models/NodeDetailScene.js |
15.6 KiB | 🟢 -1.5 KiB (-8.5%) |
exporter/src/exporter/scenes/ExporterNotebookScene.js |
3.40 MiB | 🟢 -1011 B (-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.37 MiB · 22 files | 🔺 +41 B (+0.0%) | ███░░░░░░░ 30.3% of 4.51 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
8.82 MiB · 3,247 files | 🔺 +3.1 KiB (+0.0%) | █████████░ 90.8% of 9.71 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 |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 307.0 KiB | ../node_modules/.pnpm/posthog-js@1.422.0_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 263.4 KiB | ../node_modules/.pnpm/posthog-js@1.422.0_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.js |
| 252.5 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 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 |
| 104.7 KiB | src/lib/api.ts |
| 95.2 KiB | ../packages/quill/packages/quill/dist/index.js |
| 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
✅ Toolbar bundle — eager 2.26 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.26 MiB · 18 files | 🔺 +217 B (+0.0%) | ████░░░░░░ 39.5% of 5.72 MiB |
| Deferred (lazy) | 2.11 MiB · 45 files | 🔺 +22 B (+0.0%) | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 749.3 KiB | dist/toolbar/toolbar-app-J2MBJBF4.css |
| 588.4 KiB | dist/toolbar/chunk-chunk-K5GKF7GY.js |
| 484.7 KiB | dist/toolbar/chunk-chunk-OX4ECFTJ.js |
| 133.8 KiB | dist/toolbar/chunk-chunk-QBW74TBI.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-FDH2IBXT.js |
| 71.3 KiB | dist/toolbar/toolbar-app-SDJT2P3V.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-TSAL54PB.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-4IRI3WTW.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-KKY2CY6J.js |
| 6.8 KiB | dist/toolbar/chunk-chunk-DV7IWQNF.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +346.5 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1435.20 MiB · 🔺 +346.5 KiB (+0.0%)
Prompt To Fix All With AI### Issue 1
frontend/src/queries/nodes/DataVisualization/Components/DisplayTab.tsx:134-144
**Categorical axes break annotations**
When a SQL line, area, bar, stacked-bar, or mixed chart uses a categorical X-axis, this toggle passes the raw labels into date parsing, causing enabled annotations to be filtered out or assigned invalid badge positions. Restrict the option to date-based axes or make the overlay handle categorical labels.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "chore: update OpenAPI generated types" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Add an opt-in display setting for annotations and reuse the existing chart overlay across SQL chart renderers. Generated-By: PostHog Desktop Task-Id: f73f18d6-cca3-4d13-95b4-adcc43116049
Avoid importing the annotation layer through its own directory barrel. Generated-By: PostHog Desktop Task-Id: f73f18d6-cca3-4d13-95b4-adcc43116049
1224f36 to
9d65b6f
Compare
🦔 Hogbox preview · ✅ ready▶ Open the preview
commit |
sampennington
left a comment
sampennington
left a comment
There was a problem hiding this comment.
🤖 sp-ship · 🚫 Blockers
The annotations path needs a date-axis gate to avoid invalid categorical labels and unsupported displays. The new test also needs to prove the mocked annotation reached the overlay.
Only expose and render SQL chart annotations when the X axis is date-based. Strengthen annotation coverage and add a categorical-axis regression case. Generated-By: PostHog Desktop Task-Id: 9ead4f5f-399c-4aa1-9f94-38717266b3da
sampennington
added
the
stamphog
PostHog
thmsobrmlr
left a comment
thmsobrmlr
left a comment
There was a problem hiding this comment.
Didn't check for edge cases, but code looks good ✅
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additive, backward-compatible feature (optional showAnnotations flag, default off) with a date-axis gate in both the UI and render path plus tests covering it; the automated reviewer's categorical-axis concern is already addressed in the current diff, and there's a current-head teammate approval alongside moderate author familiarity.
- Author wrote 52% of the modified lines and has 225 merged PRs in these paths (familiarity MODERATE).
- thmsobrmlr reviewed the current head.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 90L, 14F substantive, 213L/21F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (213L, 21F, cross-cutting, feat) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 47ef047 · reviewed head 47ef047 |
The SQL editor's output pane has its own SqlChart call site, which never received the annotation props, so the Show annotations toggle had no visible effect until the insight was saved and viewed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A new stamphog review started for this PR — the fresh verdict replaces this approval.
There was a problem hiding this comment.
Additive UI feature (annotation overlay on SQL charts) with a backward-compatible optional schema field; the categorical-axis concern raised by both the author's own review and Greptile is now handled via an isDateXAxis gate in all three call sites, and tests cover the toggle default, persistence, and annotation rendering. Cross-team file touches are minor (1 file each in two other teams) and not in risky territory (no auth/billing/migration/CI/dependency changes).
- Author wrote 38% of the modified lines and has 229 merged PRs in these paths (familiarity MODERATE).
- 👍 on the PR from greptile-apps[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 120L, 15F substantive, 243L/22F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (243L, 22F, cross-cutting, feat) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ bd32a62 · reviewed head bd32a62 |
Problem
People can view annotations on trends insights, but SQL charts do not offer the same context.
Changes
Before
After
How did you test this code?
typescript:checkdid not finish within the 20-minute timeout.👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Automatic notifications
Docs update
No docs change. This adds SQL chart support to the existing annotations workflow.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Pi used Bash, Playwright, and PostHog signed commits. It invoked
/writing-ui-components,/writing-user-facing-copy,/writing-tests,/writing-pr-descriptions,/reviewing-before-pr, and/writing-simplified-technical-english.The Greptile CLI was unavailable. Pi reviewed the committed diff and replaced a self-barrel import with direct imports.