fix(replay): abort the recordings load when the playlist unmounts - #75341
fix(replay): abort the recordings load when the playlist unmounts#75341posthog[bot] wants to merge 1 commit into
Conversation
The loadSessionRecordings loader read values.filters after awaiting the API call but before its breakpoint. When the playlist unmounted mid-request the store path was already gone, so the read threw and was reported as an exception. Moving breakpoint() ahead of the read discards the superseded response instead. Generated-By: PostHog Code Task-Id: 93b8671f-a0de-4bca-84c6-c6d40b113d3f
|
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✅ Bundle size — no changeUncompressed size of every built Total: 65.28 MiB · no change No file changed by more than 1000 B. Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report ✅ Eager graph — within budgetHow 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
🟢 Largest files eagerly shipped from
|
| 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.2 KiB | ../node_modules/.pnpm/posthog-js@1.408.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 |
| 235.5 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 226.5 KiB | ../node_modules/.pnpm/posthog-js@1.408.1/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.0 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.18 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.18 MiB · 17 files | no change | ████░░░░░░ 38.2% of 5.72 MiB |
| Deferred (lazy) | 2.07 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.4 KiB | dist/toolbar/toolbar-app-SHOYI5KB.css |
| 546.1 KiB | dist/toolbar/chunk-chunk-FT5FJR5I.js |
| 484.4 KiB | dist/toolbar/chunk-chunk-F4F6H6OV.js |
| 133.6 KiB | dist/toolbar/chunk-chunk-KOQJRGYO.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.0 KiB | dist/toolbar/toolbar-app-WNIJ5AXD.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-RK7DD72P.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-5CXCGT6F.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 — 🔺 +583 B (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1368.72 MiB · 🔺 +583 B (+0.0%)
Problem
Opening the Recordings tab on a person profile and then clicking another tab (or navigating away) throws:
loadSessionRecordingsreadsvalues.filtersright after awaitingapi.recordings.list(...), but itsbreakpoint()sits one line further down. Kea bumps every listener breakpoint counter inbeforeUnmount, so abreakpoint()placed before that read would abort the loader cleanly. As written, the loader instead resumes on a logic whose store path has already been removed, the selector throws, and kea-loaders'onFailurereports it viacaptureException.The same read is behind the highest-volume variants of this error too, not just the person page. Stack traces for the
--,scene---with-searchandwebAnalytics-keys all resolve to the same frame, and the$current_urlon those events is consistently a page the user had already navigated to (a dashboard,/home,/surveys) - the tell that the request outlived its playlist.Changes
Move
breakpoint()ahead of thevalues.filtersread in theloadSessionRecordingsloader. A superseded or orphaned response is now dropped instead of writing back into (or reading out of) a logic that is gone.reportRecordingsListFetchedalso stops firing for loads nobody is watching.How did you test this code?
Added one kea logic test: mount the playlist, hold the API response open, unmount, then resolve. It asserts nothing gets reported through
captureException.Verified it discriminates - with the fix reverted the new test fails with the exact production error string, and it passes with the fix in place. Full file green (84 tests). No existing test covered unmount-during-load, so this is the regression it guards.
I did not manually exercise the person profile in a browser, and I could not review a session replay of the affected flow: the recent occurrences all sit in other projects. So the exception is confirmed fixed, while the blank tab and "recording not found" rendering reported alongside it are not independently reproduced here.
Note
There is a second, separate defect in this logic that this PR does not touch.
actionToUrl'sbuildURLtargetsrouter.values.location.pathnameand spreadsrouter.values.searchParamswholesale. Because the playlist logic is still mounted while a cross-page navigation is being dispatched, it writes its ownfilters(and sometimessessionRecordingId) onto the page you just navigated to. You can see the result in the wild: exception events on this issue carry$current_urlvalues like/home?filters=<replay filters>. That would explain a person profile picking up a stalesessionRecordingIdand showing a not-found screen. Fixing it properly means giving the URL sync an owning scene rather than "wherever the router happens to be", which is a bigger change than this one and worth its own PR.Automatic notifications
🤖 Agent context
Autonomy: Fully autonomous
Written by Claude (Claude Code) from an inbox report. The report's own hypothesis was that the URL-sync handlers outlive their mount, so I started by pulling stack traces off the relevant Error tracking issues rather than reading the code cold. Every variant, across every logic key, resolved to one frame: the post-await
values.filtersread in the loader. So the URL sync is a real but different bug, called out in the note above instead of being folded in here.I checked kea's and kea-router's sources to confirm the semantics the fix leans on:
beforeUnmountincrements breakpoint counters (sobreakpoint()does abort after unmount), and kea-loaders swallows breakpoint errors rather than routing them toonFailure. I also considered gating the URL sync on the pathname the logic mounted on, and dropped it: several existing tests mount before pushing a URL, and I could not rule out a scene that legitimately keeps the playlist mounted across a pathname change. Shipping that blind alongside a confirmed one-line fix seemed like the wrong trade.Skills invoked:
/writing-tests.Created with PostHog Desktop from this inbox report.