fix: implement proper ServiceWorkerSource singleton pattern#2715
Merged
kettanaito merged 9 commits intomainfrom Apr 16, 2026
Merged
fix: implement proper ServiceWorkerSource singleton pattern#2715kettanaito merged 9 commits intomainfrom
ServiceWorkerSource singleton pattern#2715kettanaito merged 9 commits intomainfrom
Conversation
4 tasks
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a ServiceWorkerSource async singleton factory and a terminate lifecycle method, switches WorkerChannel to a lazy Changes
Sequence DiagramsequenceDiagram
participant Client
participant SWS as ServiceWorkerSource
participant WC as WorkerChannel
participant SW as ServiceWorker
participant Browser as navigator.serviceWorker
Client->>SWS: setupWorker().start(options)
SWS->>SWS: ServiceWorkerSource.from(options) (singleton, shouldInvalidateWorker)
SWS->>WC: new WorkerChannel({ getWorker: () => this.workerPromise })
WC->>Browser: register message listener (AbortController.signal)
SWS->>SWS: `#startWorker`() -> create/resolve workerPromise
Client->>SWS: enable()
Client->>WC: postMessage(request)
WC->>WC: `#getWorker`() -> await workerPromise
WC->>SW: postMessage(request)
SW-->>Client: intercepted response
Client->>SWS: disable()
SWS->>WC: postMessage(CLIENT_CLOSED)
Client->>SWS: terminate()
SWS->>Browser: unregister service worker, clear timers, clear singleton
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/browser/sources/service-worker-source.ts (1)
193-201: Remove thebeforeunloadlistener or prevent its accumulation.Each call to
#startWorker()adds a newbeforeunloadlistener without removing prior ones. After multiple stop/start cycles (triggered bydisable()resettingworkerPromise), multiple listeners accumulate. While the operations performed are idempotent, accumulating listeners is poor practice and wastes memory. Either remove the listener ondisable(), use anAbortSignalto clean up automatically, or ensure the listener is only added once.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/browser/sources/service-worker-source.ts` around lines 193 - 201, The beforeunload listener added in `#startWorker`() accumulates across start/stop cycles; modify the implementation so the listener is cleaned up on disable() (or switched to an AbortSignal tied to the worker lifecycle) — e.g., capture the listener function when adding it in `#startWorker`(), store it on the instance (or register it with an AbortSignal), and then removeEventListener('beforeunload', listener) (or abort the signal) inside disable() where workerPromise is reset and `#keepAliveInterval` is cleared; ensure the code references the same listener and also avoids adding the listener if it already exists to prevent duplicates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/browser/utils/workerChannel.ts`:
- Around line 134-137: Typo in the invariant message inside WorkerChannel:
update the string passed to invariant (the call referencing this.#worker) to
replace "in with MSW" with "with MSW" so the message reads "...the channel is
already open. This is likely an issue with MSW. Please report it on GitHub:
https://github.com/mswjs/msw/issues"; keep the rest of the invariant call and
surrounding logic unchanged.
---
Nitpick comments:
In `@src/browser/sources/service-worker-source.ts`:
- Around line 193-201: The beforeunload listener added in `#startWorker`()
accumulates across start/stop cycles; modify the implementation so the listener
is cleaned up on disable() (or switched to an AbortSignal tied to the worker
lifecycle) — e.g., capture the listener function when adding it in
`#startWorker`(), store it on the instance (or register it with an AbortSignal),
and then removeEventListener('beforeunload', listener) (or abort the signal)
inside disable() where workerPromise is reset and `#keepAliveInterval` is cleared;
ensure the code references the same listener and also avoids adding the listener
if it already exists to prevent duplicates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 73de015e-3038-4466-a383-db9e00f29991
📒 Files selected for processing (4)
src/browser/sources/service-worker-source.tssrc/browser/utils/workerChannel.tstest/browser/msw-api/setup-worker/start/start-after-stop.mocks.tstest/browser/msw-api/setup-worker/start/start-after-stop.test.ts
kettanaito
changed the title
ServiceWorkerSource singleton pattern
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/browser/utils/workerChannel.ts (1)
162-180: Enforce terminal state afterterminate()to match documented contract.
terminate()says the channel is unusable afterward, butpostMessage()can still send. Add a terminated flag/invariant to prevent accidental use-after-terminate.♻️ Proposed contract guard
export class WorkerChannel extends Emitter<WorkerChannelEventMap> { `#getWorker`: WorkerChannelOptions['getWorker'] `#controller`: AbortController + `#terminated` = false @@ public postMessage(type: OutgoingWorkerEvents): void { + invariant( + !this.#terminated, + 'Failed to post message on a WorkerChannel: the channel has been terminated.', + ) + invariant( SUPPORTS_SERVICE_WORKER, 'Failed to post message on a WorkerChannel: the Service Worker API is unavailable in this environment. This is likely an issue with MSW. Please report it on GitHub: https://github.com/mswjs/msw/issues', ) @@ public terminate(): void { + this.#terminated = true this.#controller.abort() this.removeAllListeners() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/browser/utils/workerChannel.ts` around lines 162 - 180, Add a terminal-state guard so postMessage cannot be called after terminate(): introduce a private boolean (e.g., this.#terminated) set to true in terminate() immediately after this.#controller.abort() and before removeAllListeners(), and update postMessage() to invariant(!this.#terminated, 'WorkerChannel has been terminated') before checking SUPPORTS_SERVICE_WORKER and calling this.#getWorker(). This prevents use-after-terminate in postMessage(), and ensures the documented contract for terminate() is enforced; keep the existing SUPPORTS_SERVICE_WORKER check and this.#getWorker() usage otherwise.
🤖 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/browser/utils/workerChannel.ts`:
- Around line 162-180: Add a terminal-state guard so postMessage cannot be
called after terminate(): introduce a private boolean (e.g., this.#terminated)
set to true in terminate() immediately after this.#controller.abort() and before
removeAllListeners(), and update postMessage() to invariant(!this.#terminated,
'WorkerChannel has been terminated') before checking SUPPORTS_SERVICE_WORKER and
calling this.#getWorker(). This prevents use-after-terminate in postMessage(),
and ensures the documented contract for terminate() is enforced; keep the
existing SUPPORTS_SERVICE_WORKER check and this.#getWorker() usage otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e4c1e98f-135b-423d-8cb1-f3e8c556572e
📒 Files selected for processing (5)
src/browser/setup-worker.tssrc/browser/sources/service-worker-source.tssrc/browser/utils/workerChannel.tstest/browser/msw-api/setup-worker/start/start-after-stop.test.tstest/browser/msw-api/setup-worker/stop.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/browser/msw-api/setup-worker/start/start-after-stop.test.ts
- src/browser/sources/service-worker-source.ts
commit: |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/browser/sources/service-worker-source.ts`:
- Around line 195-217: The terminate() method currently doesn't remove the
beforeunload listener installed by ServiceWorkerSource, so store the handler or
use an AbortController when you add the 'beforeunload' listener (e.g., save to
this.#unloadHandler or this.#unloadAbortController) and then remove/abort it
inside terminate(): if you chose a handler call
window.removeEventListener('beforeunload', this.#unloadHandler) (and clear the
field), or if you used an AbortController call
this.#unloadAbortController.abort() (and set it to undefined) to ensure the old
instance can't post CLIENT_CLOSED after teardown.
- Around line 98-118: The enable() path must be serialized so concurrent calls
share the same in-flight startup promise: introduce a private field (e.g.,
this.#startingPromise) and, at the start of enable(), if `#startingPromise` exists
return it; otherwise set `#startingPromise` to the async activation flow that
calls this.#startWorker(), clears this.#stoppedAt, removes listeners, awaits
start, and resolves to the ServiceWorkerRegistration; ensure `#startingPromise` is
cleared (set to undefined) on both fulfillment and rejection so future enable()
calls can retry; keep existing workerPromise and the early-return branch that
checks workerPromise.state and `#stoppedAt` intact.
In `@src/browser/utils/should-invalidate-worker.ts`:
- Around line 7-12: The current comparison uses
JSON.stringify(prevOptions.serviceWorker.options) !==
JSON.stringify(nextOptions.serviceWorker.options), which is order-sensitive;
replace that with an order-insensitive deep equality check so equivalent
RegistrationOptions don't trigger invalidation. Update the
should-invalidate-worker logic to use a stable/deep-equal comparison (e.g.,
import/use a utility like isEqual from lodash or implement a small deepEqual
that compares keys/values recursively or stringify with sorted keys) to compare
prevOptions.serviceWorker.options and nextOptions.serviceWorker.options, leaving
the prevOptions.findWorker and serviceWorker.url checks unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b6b5e8fe-e493-45ab-af5a-ff9e86f5d4ce
📒 Files selected for processing (3)
src/browser/sources/service-worker-source.tssrc/browser/utils/should-invalidate-worker.test.tssrc/browser/utils/should-invalidate-worker.ts
kettanaito
changed the title
ServiceWorkerSource singleton patternServiceWorkerSource singleton pattern
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/browser/sources/service-worker-source.ts`:
- Around line 120-121: The enable() flow currently creates a new AbortController
in this.#listenerController before each call to `#startWorker`(), causing multiple
beforeunload handlers to accumulate; instead reuse a single controller per
ServiceWorkerSource instance (create it once on construction) or explicitly call
this.#listenerController.abort() before replacing it so prior beforeunload
handlers are removed; update enable(), `#startWorker`(), and terminate() so the
beforeunload listener is registered only once (or previous controller aborted)
and the keepalive timer / service worker registration persist across
enable()/disable() cycles (refer to methods: enable(), `#startWorker`(),
terminate(), and the private field `#listenerController`).
- Around line 61-63: The current reuse path in ServiceWorkerSource.from() calls
shouldInvalidateWorker(ServiceWorkerSource.#current.#options, options) but
ignores changes to the quiet flag, causing stale logging/validateWorkerScope()
behavior; update shouldInvalidateWorker to include comparison of the quiet
property (or alternatively, when reusing the singleton in from(), refresh
ServiceWorkerSource.#current.#options with the new options) so that changes to
quiet take effect; reference shouldInvalidateWorker, ServiceWorkerSource.from(),
ServiceWorkerSource.#current, and the instance field `#options` to locate where to
add the comparison or the options refresh.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d28b531b-41a2-4095-b463-04a7d03fcb3e
📒 Files selected for processing (1)
src/browser/sources/service-worker-source.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/browser/sources/service-worker-source.ts (1)
61-66:⚠️ Potential issue | 🟡 Minor
quietoption changes are silently ignored on reuse.When
shouldInvalidateWorkerreturnsfalse(i.e., onlyquietdiffers), the existing singleton is returned without updating#options. Subsequentenable()/disable()calls will use the stalequietvalue for logging decisions.Either include
quietin invalidation criteria, or refresh#optionson the reuse path:🔧 Option A: Refresh options on reuse
} else if ( shouldInvalidateWorker(ServiceWorkerSource.#current.#options, options) ) { await ServiceWorkerSource.#current.terminate() ServiceWorkerSource.#current = new ServiceWorkerSource(options) + } else { + // Refresh options that don't require invalidation (e.g., quiet) + ServiceWorkerSource.#current.#options = options }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/browser/sources/service-worker-source.ts` around lines 61 - 66, The existing singleton ServiceWorkerSource.#current keeps stale `#options` when shouldInvalidateWorker(...) returns false (i.e., only quiet changed), so subsequent enable()/disable() use the old quiet; to fix, when reusing the singleton (the else branch where shouldInvalidateWorker is false) update the instance's options to the new options (e.g., assign ServiceWorkerSource.#current.#options = options or call an instance method like ServiceWorkerSource.#current.updateOptions(options)), or alternatively add quiet to shouldInvalidateWorker's invalidation criteria; ensure enable()/disable() read the refreshed `#options` from the reused ServiceWorkerSource.#current.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/browser/sources/service-worker-source.ts`:
- Around line 61-66: The existing singleton ServiceWorkerSource.#current keeps
stale `#options` when shouldInvalidateWorker(...) returns false (i.e., only quiet
changed), so subsequent enable()/disable() use the old quiet; to fix, when
reusing the singleton (the else branch where shouldInvalidateWorker is false)
update the instance's options to the new options (e.g., assign
ServiceWorkerSource.#current.#options = options or call an instance method like
ServiceWorkerSource.#current.updateOptions(options)), or alternatively add quiet
to shouldInvalidateWorker's invalidation criteria; ensure enable()/disable()
read the refreshed `#options` from the reused ServiceWorkerSource.#current.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1a1ebd1e-1a81-4967-95c6-bdc9b29e1c9d
📒 Files selected for processing (1)
src/browser/sources/service-worker-source.ts
Member
Author
Released: v2.13.4 🎉This has been released in v2.13.4. Get these changes by running the following command: Predictable release automation by Release. |
This was referenced Apr 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
setupWorkeris called more than once #2585Root cause
The
workerpromise passed to theWorkerChannelin the constructor was stale after the worker source has been disabled. Reassigning#workerPromisedid not provide a new value to the worker channel so it kept communicating with an outdated service worker reference.