fix(frontend): keep non-string API error bodies out of React children - #75875
fix(frontend): keep non-string API error bodies out of React children#75875posthog[bot] wants to merge 2 commits into
Conversation
ApiError copied `data.detail` (and the sibling DRF fields) straight off the response body even though they are declared `string | null`. When a backend returns a non-string detail, that object travels into a JSX child slot or a toast message and React throws #31, taking the whole page down to the ErrorBoundary. Guard the assignment at the source so the declared type is honest, stop `parseErrorMessage` and `extractValidationError` laundering non-strings onward, and add fallbacks at the bare render sites. Generated-By: PostHog Code Task-Id: 7cf2cdd3-c2f3-483c-b439-064263f83c50
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🤖 CI report
|
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.25 MiB · 22 files | no change | ███░░░░░░░ 27.7% of 4.51 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
8.12 MiB · 3,023 files | 🔺 +60 B (+0.0%) | ████████░░ 83.6% 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 |
|---|---|
| 285.3 KiB | ../node_modules/.pnpm/posthog-js@1.409.2/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 |
| 235.5 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 231.4 KiB | ../node_modules/.pnpm/posthog-js@1.409.2/node_modules/posthog-js/dist/module.js |
| 154.3 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.2 KiB | src/lib/api.ts |
| 94.7 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.19 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.19 MiB · 17 files | 🔺 +3 B (+0.0%) | ████░░░░░░ 38.3% of 5.72 MiB |
| Deferred (lazy) | 2.08 MiB · 33 files | no change | 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 |
|---|---|
| 717.8 KiB | dist/toolbar/toolbar-app-2QFKEBT7.css |
| 551.1 KiB | dist/toolbar/chunk-chunk-47XJDVYW.js |
| 484.6 KiB | dist/toolbar/chunk-chunk-QCKKTYHY.js |
| 133.6 KiB | dist/toolbar/chunk-chunk-3MD6VT5F.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.0 KiB | dist/toolbar/toolbar-app-EQOD2D5T.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-BSDSQHOD.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-AJ7PFJB6.js |
| 12.2 KiB | dist/toolbar/chunk-chunk-PIK3PADE.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +7.3 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1371.00 MiB · 🔺 +7.3 KiB (+0.0%)
The existing case asserted that a non-string error_message passed straight through to `detail`, which is the crash this PR fixes. Generated-By: PostHog Code Task-Id: 7cf2cdd3-c2f3-483c-b439-064263f83c50
Problem
object with keys {type, code, detail, attr}- the shape of our DRF error body. An API error response is landing in a React child slot that expects a string.ApiErrorcopiesdata.detailoff the response body with notypeofcheck, even though the field is declaredstring | null. Every consumer downstream trusts that declaration.error.detailrender and toast sites can produce this the moment a backend returns a non-string detail - including the startup program application submit, which toastserror.detaildirectly.Changes
lib/api-error.tsstring | nulldataqueries/query.tsparseErrorMessageerrorMessage || '', handing back the object itself''instead of passing the value throughInsightViz/utils.tsextractValidationError(detail || data.error_message)?.replace(...)-TypeErroron a non-stringnullunless it's a stringMetricErrorStatedetail: string | object | anydetail?: string | nullEmptyStates,LegacyErrorChecklist,InstallationProgressViewerror.detail/titlebareThe source guard is the actual fix. The render-site guards are defense in depth for the paths that don't go through
ApiError.Note
Anything that wanted the nested field-error shape should read
error.data, which is untouched. Nothing in the tree does today - I checked for indexing orObject.keysonApiError.detailand found none.How did you test this code?
frontend/src/lib/api-error.test.ts: parameterized cases proving a non-stringdetail(nested field errors, a list, a number) is dropped tonullwhile the raw value stays ondata, plus one case that valid string fields still survive. That is the regression - if thetypeofguard is removed, an object reaches a React child again and the page dies. No existing test coveredApiErrorfield normalization.src/lib/api-error.test.tsand the existingsrc/scenes/insights/EmptyStates/InsightErrorStates.test.tsx- 9 passed.pnpm --filter=@posthog/frontend typescript:check: no errors in any touched file. (There are pre-existing unrelated@posthog/quill-*module-resolution errors in this sandbox because the quill packages aren't built.)pnpm --filter=@posthog/frontend fix.detailfrom the backend, which the unit test exercises directly against the class that produced it.Automatic notifications
Docs update
No user-facing behavior or documented workflow changes.
🤖 Agent context
Autonomy: Fully autonomous
Written by Claude (Claude Code) from a PostHog inbox report. Skills invoked:
/writing-tests.I checked for competing work first. The two nearest open PRs, #70436 and #71624, only rework error-boundary plumbing and leave the unguarded values intact, so they are complementary rather than overlapping. #75858 touches
EmptyStates.tsxfor an unrelated cache-miss change - a trivial conflict at worst.Decisions: I fixed this at the source rather than patching each render site, because the site list is long and grows. I considered tightening
InsightErrorStateProps.titletostring, but callers legitimately pass JSX, so I guarded the render withisValidElementinstead. I skipped a component-render test for the object-title path since the source fix already prevents it and a render test costs more than it catches.Created with PostHog Desktop from this inbox report.