fix(session-recorder): replace deprecated unload listener with visibilitychange/pagehide - #306
fix(session-recorder): replace deprecated unload listener with visibilitychange/pagehide#306in-jun wants to merge 1 commit into
Conversation
…litychange/pagehide The BatchLogProcessor constructor registered a window 'unload' listener to flush pending logs. Chrome blocks 'unload' via Permissions Policy (logging a violation warning on every page load), so the end-of-session flush is silently dropped, and the mere presence of the listener makes the page ineligible for the back/forward cache. Flush on visibilitychange (hidden) and pagehide instead, and send the export with fetch keepalive (when under the 64 KiB keepalive body limit) so the request survives page termination. This mirrors the upstream Splunk fix (signalfx/splunk-otel-js-web#1066) and the pagehide/ visibilitychange listeners already used elsewhere in this repo. Fixes hyperdxio#219
🦋 Changeset detectedLatest commit: eab9842 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Greptile SummaryThe PR replaces the session recorder’s deprecated
Confidence Score: 4/5The PR appears safe to merge, with only the non-blocking test-directory placement needing cleanup. The lifecycle handlers atomically remove queued logs before exporting, and empty follow-up flushes are ignored; the only accepted concern is that the new test was added outside the repository’s prescribed test directories. Files Needing Attention: packages/session-recorder/test/BatchLogProcessor.test.ts
|
| Filename | Overview |
|---|---|
| packages/session-recorder/src/BatchLogProcessor.ts | Replaces the unload listener with hidden-state visibilitychange and pagehide flush handlers; queue transfer remains synchronous and prevents duplicate exports. |
| packages/session-recorder/src/OTLPLogExporter.ts | Adds a payload-size and visibility-gated keepalive option to termination-time fetch requests. |
| packages/session-recorder/test/BatchLogProcessor.test.ts | Covers listener registration and both flush triggers, but the new test violates the repository’s required test-directory convention. |
| .changeset/session-recorder-remove-unload.md | Adds patch releases for the session recorder and browser packages with an accurate description of the lifecycle change. |
Sequence Diagram
sequenceDiagram
participant Browser
participant Processor as BatchLogProcessor
participant Exporter as OTLPLogExporter
participant Collector
Browser->>Processor: visibilitychange(hidden) or pagehide
Processor->>Processor: Splice queued logs
Processor->>Exporter: export(logs)
Exporter->>Exporter: Encode and gzip
Exporter->>Collector: fetch POST (keepalive when eligible)
Reviews (1): Last reviewed commit: "fix(session-recorder): replace deprecate..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -0,0 +1,74 @@ | |||
| import { BatchLogProcessor, convert } from '../src/BatchLogProcessor'; | |||
There was a problem hiding this comment.
This new test is under packages/session-recorder/test/, while the repository convention requires tests in src/__tests__/ or __tests__/; leaving it here perpetuates an inconsistent layout and makes future tests harder to discover.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Kept it next to the existing test/index.test.ts — this package's tests live in test/ (as do otel-web's), so moving only the new file would split the layout within the package. Happy to move both in a follow-up if the maintainers prefer consolidating on src/__tests__/.
There was a problem hiding this comment.
That's a fair point — test/index.test.ts already exists, so placing the new file alongside it is the consistent choice within this package. My comment was incorrect about the convention here. Apologies for the noise.
Would you like me to remember that packages/session-recorder (and otel-web) use test/ as the test directory, not src/__tests__/?
1 participant
Summary
unloadlistener in the session recorder'sBatchLogProcessorwithvisibilitychange(hidden) +pagehide— the same flush strategyHyperDXBatchSpanProcessoralready uses for spanskeepalive(when under the 64 KiB body limit) so the final request survives page termination@hyperdx/otel-web-session-recorderand@hyperdx/browserFixes #219. Same fix as upstream Splunk's signalfx/splunk-otel-js-web#1066. The
beforeunloadlistener mentioned in the issue is left as is — it isn't deprecated, doesn't trigger the violation, and upstream keeps it.Testing
yarn ci:build/ci:lint/ci:unitpass locally; new Jest tests cover listener registration and flush behavior--enable-features=DeprecateUnload, OTLP collector + ClickHouse): the Permissions Policy violation is gone, events recorded just before leaving the page now arrive (previously dropped), and the page becomes bfcache-restorable (pageshow.persisted === true)