fix: use pagehide instead of beforeunload for tracking closing pages

[kettanaito]

• Addresses My app hooks in beforeunload with a confirmation message and then MSW stops working #1105

Root cause

beforeunload gives no means to know if upstream listener has prevented it.

Changes

• Uses pagehide event instead of beforeunload. This will respect the developer's decision to prevent page closure and will not send the CLIENT_CLOSED message to the worker in that case.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: e4b072f3-d26e-4238-b9a8-43bac166694e

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The pull request changes the client shutdown signal from the beforeunload event to pagehide, with conditional suppression when event.persisted is true, and reorganizes the service worker's CLIENT_CLOSED message handling to occur earlier in the handler. A type annotation in tests is updated for consistency.

Changes

Cohort / File(s)	Summary
Client shutdown signaling src/browser/sources/service-worker-source.ts	Event listener switched from beforeunload to pagehide with condition to suppress shutdown signal when page is persisted (restored from back/forward cache).
Service worker message handling src/mockServiceWorker.js	CLIENT_CLOSED handling logic moved from within the switch statement to an early conditional check before client resolution, streamlining the message dispatch flow.
Type annotation update test/browser/msw-api/unregister.test.ts	Import changed from SetupWorkerApi to SetupWorker for window.msw.worker type annotation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• #2715: Modifies service worker shutdown/cleanup flow and CLIENT_CLOSED handling, directly aligned with this PR's messaging and client lifecycle management changes.

Suggested labels

release candidate

Poem

🐰 From beforeunload's tangled dance,
We leap to pagehide's clearer chance,
When persisted flags ring true and clear,
The worker stays—no need to fear!
Shutdown signals find their place,
A service workers' safer space. 🚀

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: switching from beforeunload to pagehide event for tracking page closures.
Linked Issues check	✅ Passed	The code changes directly address issue #1105 by replacing beforeunload with pagehide and conditionally suppressing shutdown signaling when page persists.
Out of Scope Changes check	✅ Passed	All changes are directly related to the linked issue: fixing the event handling mechanism and updating related test type annotations accordingly.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check	✅ Passed	The pull request description clearly relates to the changeset, explaining the root cause and changes made to address issue #1105 regarding pagehide vs beforeunload events.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/beforeunload-cancellation

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/msw@2720

commit: c683679

[coderabbitai]

🧹 Nitpick comments (1)

src/mockServiceWorker.js (1)

30-45: Avoid calling self.clients.matchAll() on every message.

allClients is only used inside the CLIENT_CLOSED branch, but it now runs for every inbound message — including KEEPALIVE_REQUEST every 5s and every INTEGRITY_CHECK_REQUEST/MOCK_ACTIVATE. Move the matchAll call inside the branch to restore prior behavior.

♻️ Proposed fix

-  const allClients = await self.clients.matchAll({
-    type: 'window',
-  })
-
   if (event.data === 'CLIENT_CLOSED') {
     activeClientIds.delete(clientId)
 
+    const allClients = await self.clients.matchAll({
+      type: 'window',
+    })
+
     const remainingClients = allClients.filter((client) => {
       return client.id !== clientId
     })
 
     // Unregister itself when there are no more clients
     if (remainingClients.length === 0) {
       self.registration.unregister()
     }
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/mockServiceWorker.js` around lines 30 - 45, Move the expensive
self.clients.matchAll() call into the CLIENT_CLOSED branch so it only runs when
handling 'CLIENT_CLOSED'; currently const allClients is computed for every
message. Inside the if (event.data === 'CLIENT_CLOSED') block, call const
allClients = await self.clients.matchAll({ type: 'window' }) and then compute
remainingClients by filtering out clientId, delete from activeClientIds, and
call self.registration.unregister() if remainingClients.length === 0; remove the
top-level allClients declaration so
KEEPALIVE_REQUEST/INTEGRITY_CHECK_REQUEST/MOCK_ACTIVATE paths no longer invoke
matchAll.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/mockServiceWorker.js`:
- Around line 30-45: Move the expensive self.clients.matchAll() call into the
CLIENT_CLOSED branch so it only runs when handling 'CLIENT_CLOSED'; currently
const allClients is computed for every message. Inside the if (event.data ===
'CLIENT_CLOSED') block, call const allClients = await self.clients.matchAll({
type: 'window' }) and then compute remainingClients by filtering out clientId,
delete from activeClientIds, and call self.registration.unregister() if
remainingClients.length === 0; remove the top-level allClients declaration so
KEEPALIVE_REQUEST/INTEGRITY_CHECK_REQUEST/MOCK_ACTIVATE paths no longer invoke
matchAll.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 8502e0e3-58c7-4811-8ec1-bc18d29611ad

📥 Commits

Reviewing files that changed from the base of the PR and between 61dd37d and eb0e58f.

📒 Files selected for processing (3)

• src/browser/sources/service-worker-source.ts
• src/mockServiceWorker.js
• test/browser/msw-api/unregister.test.ts