feat: optimize page leave events for bfcache by saikumarrs · Pull Request #2458 · rudderlabs/rudder-sdk-js

saikumarrs · 2025-09-01T18:36:27Z

PR Description

As beforeunload event registration impacts bfcache optimisation on modern browsers, so restricting it to only IE 11.

Linear task (optional)

https://linear.app/rudderstack/issue/SDK-3739/spike-investigate-if-we-can-get-away-with-beforeunload-event-handler

Cross Browser Tests

Please confirm you have tested for the following browsers:

Chrome
Firefox
IE11

Sanity Suite

All sanity suite test cases pass locally

Security

The code changed/added as part of this pull request won't create any security issues with how the software is being used.

Summary by CodeRabbit

Bug Fixes
- Improved reliability of page leave/unload handling across browsers; IE11-specific listener behavior prevents duplicate or missed callbacks.
Refactor
- Moved IE11 detection into a shared utility so capability checks are consistent across packages.
Tests
- Expanded IE11 and page lifecycle tests covering beforeunload/pagehide, visibility changes, and multiple-event scenarios.
Chores
- Updated bundle size limit to reflect current build size.

coderabbitai · 2025-09-01T18:36:35Z

📝 Walkthrough

Walkthrough

Adds an isIE11 detector to analytics-js-common, uses it to conditionally register beforeunload only on IE11, updates tests to simulate IE11 and switch a test to pagehide, removes the local isIE11 export from the capabilities manager detection module, and increments a size-limit value.

Changes

Cohort / File(s)	Summary
IE11 detection introduction `packages/analytics-js-common/src/utilities/detect.ts`	Adds and exports `isIE11(): boolean` that checks `globalThis.navigator.userAgent` for the IE11 Trident pattern.
Page leave logic adjustment `packages/analytics-js-common/src/utilities/page.ts`	Imports `isIE11` and registers the `beforeunload` listener only when `isIE11()` is true; other listeners unchanged; public API unchanged.
Tests — detection and page leave `packages/analytics-js-common/__tests__/utilities/detect.test.ts`, `packages/analytics-js-common/__tests__/utilities/page.test.ts`	Adds parameterized tests for `isIE11`; updates page.leave tests to simulate IE11 UA, restore original UA after each test, and add IE11-specific beforeunload/visibility/blur scenarios.
Capabilities manager import refactor `packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts`	Switches `isIE11` import to `@rudderstack/analytics-js-common/utilities/detect`; usage unchanged.
Removal of local IE11 detection export `packages/analytics-js/src/components/capabilitiesManager/detection/browser.ts`, `packages/analytics-js/src/components/capabilitiesManager/detection/index.ts`	Removes local `isIE11` implementation and its export; re-export list updated to exclude `isIE11`.
Test alignment with new page leave flow `packages/analytics-js/__tests__/app/RudderAnalytics.test.ts`	Replaces simulated `beforeunload` on `window` with `pagehide` dispatched on `document`.
Build config size limit tweak `packages/analytics-js/.size-limit.mjs`	Increases “Core (Content Script) - Modern - NPM (CJS)” size limit from 41 KiB to 41.5 KiB (data value change only).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant App
  participant PageUtils as analytics-js-common/utilities/page
  participant Detect as analytics-js-common/utilities/detect

  Note over App,PageUtils: onPageLeave(callback) initialization
  App->>PageUtils: onPageLeave(callback)
  PageUtils->>Detect: isIE11()
  alt IE11
    Note right of PageUtils: register beforeunload listener
    PageUtils->>PageUtils: addEventListener('beforeunload', ...)
  else Non-IE11
    Note right of PageUtils: skip beforeunload
  end
  Note over PageUtils: register blur, focus, pagehide, visibilitychange listeners

  rect rgba(230,245,255,0.6)
  Note over App,PageUtils: Page exit signals
  App-->>PageUtils: pagehide / visibilitychange / blur
  PageUtils-->>App: invoke callback (true/false depending on path)
  end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

feat: track time spent on a page #1876 — Modifies onPageLeave behavior in analytics-js-common/src/utilities/page.ts and related tests; closely related to the page-leave handling changes.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

MCP integration is disabled by default for public repositories
Jira integration is disabled

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 88bc148 and 49076a2.

📒 Files selected for processing (2)

packages/analytics-js-common/__tests__/utilities/page.test.ts (6 hunks)
packages/analytics-js-common/src/utilities/detect.ts (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

packages/analytics-js-common/src/utilities/detect.ts
packages/analytics-js-common/tests/utilities/page.test.ts

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

GitHub Check: Bundle size checks
GitHub Check: Code quality checks
GitHub Check: Unit Tests and Lint

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

Visit our Status Page to check the current availability of CodeRabbit.
Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

coderabbitai

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

packages/analytics-js-common/src/utilities/page.ts (1)
51-55: Also listen for pagehide on window to match the spec

pagehide is a Window event in specs; some engines may not deliver it to document. Safe to register on window in addition to document.
-  document.addEventListener('pagehide', () => {
+  // Listen on both Window and Document for broader compatibility
+  (globalThis as typeof window).addEventListener('pagehide', () => {
+    isAccessible = document.visibilityState === 'hidden';
+    handleOnLeave();
+  });
+  document.addEventListener('pagehide', () => {
     isAccessible = document.visibilityState === 'hidden';
     handleOnLeave();
   });

🧹 Nitpick comments (4)

packages/analytics-js/__tests__/app/RudderAnalytics.test.ts (1)
804-806: Good switch to pagehide; consider exercising bfcache via PageTransitionEvent

Dispatching pagehide on document aligns with the listener site. Optionally, emit a PageTransitionEvent (when available) to validate bfcache scenarios (persisted true/false).
-      const event = new Event('pagehide');
-      document.dispatchEvent(event);
+      const event =
+        typeof (globalThis as any).PageTransitionEvent === 'function'
+          ? new (globalThis as any).PageTransitionEvent('pagehide', { persisted: false })
+          : new Event('pagehide');
+      document.dispatchEvent(event);
packages/analytics-js-common/src/utilities/page.ts (1)
39-45: Blur-based fallback can overfire on simple tab switches

Blur fires on tab switches too; consider narrowing to iOS Safari only to avoid false positives.
-  (globalThis as typeof window).addEventListener('blur', () => {
+  const isIosSafari =
+    /iP(hone|ad|od)/.test(globalThis.navigator?.platform || '') &&
+    /Safari/.test(globalThis.navigator?.userAgent || '') &&
+    !/Chrome|CriOS|FxiOS/.test(globalThis.navigator?.userAgent || '');
+  (globalThis as typeof window).addEventListener('blur', () => {
     isAccessible = true;
     handleOnLeave();
-  });
+  }, { passive: true });
packages/analytics-js/src/components/capabilitiesManager/detection/index.ts (1)
2-2: isIE11 re-export removed — verify no remaining imports rely on this path

Search for imports from './detection' pulling isIE11 and update them to @rudderstack/analytics-js-common/utilities/detect to avoid breakages.

Optionally, keep a transitional re-export to ease migration:
 export { isBrowser, isNode, hasCrypto, hasUAClientHints, hasBeacon } from './browser';
+export { isIE11 } from '@rudderstack/analytics-js-common/utilities/detect';
packages/analytics-js-common/__tests__/utilities/page.test.ts (1)
49-51: Prefer spying on navigator.userAgent over direct assignment

userAgent may be non-writable; spying is safer and avoids global mutation. Consider a small helper to DRY the pattern.

Example helper:
const withUserAgent = (ua: string, fn: () => void) => {
  const spy = jest.spyOnProperty(globalThis.navigator, 'userAgent', 'get').mockReturnValue(ua as any);
  try { fn(); } finally { spy.mockRestore(); }
};
Usage:
withUserAgent('Mozilla/5.0 (Windows NT 10.0; Trident/7.0; rv:11.0) like Gecko', () => {
  const evCallback = jest.fn();
  onPageLeave(evCallback);
  dispatchWindowEvent('beforeunload');
  expect(evCallback).toHaveBeenCalledTimes(1);
  expect(evCallback).toHaveBeenCalledWith(false);
});
Also applies to: 106-109, 118-121, 181-184, 201-204

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

MCP integration is disabled by default for public repositories
Jira integration is disabled

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 7ca8a6e and 88bc148.

📒 Files selected for processing (9)

packages/analytics-js-common/__tests__/utilities/detect.test.ts (2 hunks)
packages/analytics-js-common/__tests__/utilities/page.test.ts (6 hunks)
packages/analytics-js-common/src/utilities/detect.ts (1 hunks)
packages/analytics-js-common/src/utilities/page.ts (2 hunks)
packages/analytics-js/.size-limit.mjs (1 hunks)
packages/analytics-js/__tests__/app/RudderAnalytics.test.ts (1 hunks)
packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts (1 hunks)
packages/analytics-js/src/components/capabilitiesManager/detection/browser.ts (1 hunks)
packages/analytics-js/src/components/capabilitiesManager/detection/index.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (7)

**/*.{js,jsx,ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/002-javascript-typescript.mdc)

Code must work in browsers AND service workers (no Node.js APIs)

Files:

packages/analytics-js/__tests__/app/RudderAnalytics.test.ts
packages/analytics-js-common/src/utilities/detect.ts
packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts
packages/analytics-js/src/components/capabilitiesManager/detection/index.ts
packages/analytics-js-common/__tests__/utilities/detect.test.ts
packages/analytics-js/src/components/capabilitiesManager/detection/browser.ts
packages/analytics-js-common/src/utilities/page.ts
packages/analytics-js-common/__tests__/utilities/page.test.ts

packages/*/__tests__/**