Open
Conversation
Implement suspension mechanism that keeps the port listener alive when a client misses ping-pong,
removes it from subscription/heartbeat state, and reactivates on next message from the resumed tab.
fix(shared-worker): `squashedChanges` iterates wrong array
Fix inner loop in `squashedChanges` to iterate `sortedChanges` instead of `requestAddChange` so that
add+remove pair elimination actually executes for same-request entries within aggregation window.
fix(shared-worker): orphaned `SubscriptionState` on empty changes
Guard `handleDelayedAggregation` against creating subscription state when changes queue is empty, and
add self-invalidation in `processChanges` when post-squash changes are empty with no active clients.
fix(shared-worker): `BFCache` interaction with `pagehide` listener
Remove `{ once: true }` from `pagehide` listener so the handler fires correctly when the page truly
navigates away after a prior BFCache entry consumed the one-shot listener.
fix(shared-worker): version comparison and token handling
Replace independent semver component comparison with hierarchical check, fix `accessTokensMap` being
overwritten on each new token, and serialize `onTokenChange` with a promise chain for ordering.
fix(tests): shared worker test reliability
Replace fixed-delay sequential subscription with channel-list verification on status events, and fix
`makeSendable` overrides to always return a valid tuple after calling `done()`.
parfeon
added
priority: medium
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Event Types & Contracts src/transport/subscription-worker/components/custom-events/client-event.ts, src/transport/subscription-worker/components/custom-events/client-manager-event.ts, src/transport/subscription-worker/subscription-worker-types.ts |
New PubNubClientEvent.Reactivate, PubNubClientsManagerEvent.Reactivated, and SharedWorkerClientSuspended event types; new event classes PubNubClientReactivateEvent and PubNubClientManagerReactivateEvent for event detail and forwarding. |
Core Client Suspension State src/transport/subscription-worker/components/pubnub-client.ts |
PubNubClient gains _suspended tracking, isSuspended getter, suspended setter, cancelActiveRequests() method, and handleReactivation() handler that clears suspension and routes triggering messages to appropriate handlers. |
Clients Manager Suspension/Reactivation src/transport/subscription-worker/components/pubnub-clients-manager.ts |
New hasClient() method; new suspendClient() method replaces immediate unregistration on timeout; new reactivateClient() method restores client state and re-subscribes to events; timeout handling now calls suspendClient instead of unregisterClient; listener wiring uses abort signals and one-time reactivation listener. |
Listener Reattachment src/transport/subscription-worker/components/heartbeat-requests-manager.ts, src/transport/subscription-worker/components/subscribe-requests-manager.ts |
Refactored listeners into attachClientListeners(client) helpers; both managers now reattach listeners on ClientsManagerEvent.Reactivated; handleDelayedAggregation squashes changes before early exit. |
Subscription State Fixes src/transport/subscription-worker/components/subscription-state.ts |
squashedChanges iteration fixed to cover full sortedChanges array; processChanges now explicitly dispatches invalidate event when changes list is empty; request cleanup index check changed to >= 0 to include first element. |
Worker Middleware Integration src/transport/subscription-worker/subscription-worker-middleware.ts |
New tokenChangeChain promise serializes token-change operations; middleware now handles shared-worker-client-suspended event by rejecting pending callbacks with retryable timeout error; token caching refactored to assign directly to map. |
Browser Integration src/web/index.ts |
Simplified pagehide listener setup for SubscriptionWorkerMiddleware termination. |
Integration Tests test/integration/shared-worker/shared-worker.test.ts |
Rewrote subscription verification logic; improved auth-token error handling; added comprehensive suspension/reactivation tests simulating frozen-tab scenarios with message delivery verification. |
Sequence Diagram(s)
sequenceDiagram
participant Client as PubNubClient
participant Manager as ClientsManager
participant Heartbeat as HeartbeatMgr
participant Subscribe as SubscribeMgr
participant Middleware as Middleware
Note over Client,Middleware: Client lifecycle: registration
Manager->>Client: create & register
Manager->>Heartbeat: ClientsManagerEvent.Registered
Heartbeat->>Heartbeat: attachClientListeners(client)
Manager->>Subscribe: ClientsManagerEvent.Registered
Subscribe->>Subscribe: attachClientListeners(client)
Note over Client,Middleware: Missed ping/pong: suspension
Manager->>Manager: timeout detected
Manager->>Client: suspendClient()
Client->>Client: _suspended = true
Client->>Client: cancelActiveRequests()
Manager->>Manager: dispatch UnregisterEvent
Note over Client,Middleware: Incoming message: reactivation
Client->>Client: message received while suspended
Client->>Client: handleReactivation()
Client->>Client: _suspended = false
Client->>Client: dispatch ReactivateEvent
Manager->>Client: reactivateClient()
Manager->>Heartbeat: ClientsManagerEvent.Reactivated
Heartbeat->>Heartbeat: attachClientListeners(client)
Manager->>Subscribe: ClientsManagerEvent.Reactivated
Subscribe->>Subscribe: attachClientListeners(client)
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title 'Client suspension and reactivation' accurately summarizes the main feature added in this pull request—implementing a suspension/reactivation mechanism for clients in the shared worker. |
| Description check | ✅ Passed | The description clearly relates to the changeset, detailing the suspension/reactivation mechanism and multiple bug fixes across the shared worker implementation. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
fix/shared-worker-subscription-reliability
Tip
💬 Introducing Slack Agent: The best way for teams to turn conversations into code.
Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
- Generate code and open pull requests
- Plan features and break down work
- Investigate incidents and troubleshoot customer tickets together
- Automate recurring tasks and respond to alerts with triggers
- Summarize progress and report instantly
Built for teams:
- Shared memory across your entire org—no repeating context
- Per-thread sandboxes to safely plan and execute work
- Governance built-in—scoped access, auditability, and budget controls
One agent for your entire SDLC. Right inside Slack.
Comment @coderabbitai help to get the list of available commands and usage tips.
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 (2)
src/transport/subscription-worker/subscription-worker-middleware.ts (1)
440-445:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope suspended events to the target client.
At Line 444,
shared-worker-client-suspendedbypassesclientIdentifierfiltering. Then Line 488 rejects all pending callbacks in this instance. A suspend event for another client can incorrectly fail this client’s in-flight requests.🔧 Proposed fix
if ( data.type !== 'shared-worker-ping' && data.type !== 'shared-worker-connected' && data.type !== 'shared-worker-console-log' && data.type !== 'shared-worker-console-dir' && - data.type !== 'shared-worker-client-suspended' && data.clientIdentifier !== this.configuration.clientIdentifier ) return;Also applies to: 481-498
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/transport/subscription-worker/subscription-worker-middleware.ts` around lines 440 - 445, The 'shared-worker-client-suspended' message is not being filtered by clientIdentifier, causing suspend events from other clients to reject this instance's in-flight requests; update the message handling so the condition that checks data.clientIdentifier !== this.configuration.clientIdentifier also applies to data.type === 'shared-worker-client-suspended', and modify the logic that rejects pending callbacks (the method that currently rejects all pending callbacks when handling 'shared-worker-client-suspended') to only reject callbacks associated with the suspended client's identifier (use the same clientIdentifier field on the pending callback entries) instead of rejecting every outstanding callback.src/transport/subscription-worker/components/pubnub-clients-manager.ts (1)
211-214:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPotential out-of-bounds splice when client not found in array.
If
clientIdxis-1(client not in array),splice(-1, 1)removes the last element, which would corrupt the client list.Proposed fix
const clientsBySubscribeKey = this.clientBySubscribeKey[client.subKey]; if (clientsBySubscribeKey) { const clientIdx = clientsBySubscribeKey.indexOf(client); - clientsBySubscribeKey.splice(clientIdx, 1); + if (clientIdx >= 0) clientsBySubscribeKey.splice(clientIdx, 1);Note:
suspendClientat line 155 already has this guard, butunregisterClientdoes not.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/transport/subscription-worker/components/pubnub-clients-manager.ts` around lines 211 - 214, In unregisterClient (pubnub-clients-manager.ts) avoid calling splice with -1 by checking that the client exists before removing: locate the code that accesses this.clientBySubscribeKey[client.subKey] and computing clientIdx = clientsBySubscribeKey.indexOf(client) and add a guard like clientIdx !== -1 (or use a filter to remove the exact reference) before calling clientsBySubscribeKey.splice; mirror the same presence check used in suspendClient to prevent accidentally removing the last array element when the client isn't found.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/transport/subscription-worker/components/heartbeat-requests-manager.ts`:
- Around line 183-186: The attachClientListeners method currently overwrites
this.clientAbortControllers[client.identifier] without aborting any existing
controller, which allows duplicate heartbeat/presence handlers; update
attachClientListeners to check this.clientAbortControllers[client.identifier]
and if an existing AbortController exists, call abort() (and optionally remove
it) before creating and storing the new AbortController for the PubNubClient,
and ensure any listener cleanup tied to the AbortSignal runs so duplicate
handlers are not left registered.
In `@src/transport/subscription-worker/components/subscribe-requests-manager.ts`:
- Around line 372-375: The attachClientListeners function currently overwrites
this.clientAbortControllers[client.identifier] with a new AbortController
without aborting any prior controller, which allows duplicate handlers to
remain; before creating and assigning a new AbortController in
attachClientListeners, check for an existing controller at
this.clientAbortControllers[client.identifier], call .abort() on it (and
optionally delete it) to cancel / clean up previous listeners/work, then create
and assign the new AbortController so re-attaching won’t leave duplicate
handlers enqueued.
In `@src/transport/subscription-worker/subscription-worker-middleware.ts`:
- Around line 232-248: The tokenChangeChain currently becomes permanently
rejected if parsedAccessToken or scheduleEventPost rejects; wrap the promise
sequence attached to tokenChangeChain so that errors are caught and handled
(e.g., log via the worker logger) and the chain returns a resolved value to
allow future updates to run; specifically, update the code that chains
parsedAccessToken(...) -> then(...) -> scheduleEventPost(updateEvent) on
tokenChangeChain to append a .catch(err => { /* log with context about
updateEvent/clientIdentifier */ }) that swallows or converts the error into a
resolved outcome so tokenChangeChain remains recoverable while still recording
the failure.
In `@test/integration/shared-worker/shared-worker.test.ts`:
- Line 1637: The test uses it.only which will cause all other tests to be
skipped; remove the .only from the it.only call (the test named "should receive
timeout presence events when browser tabs are closed") so it becomes a normal
it(...) test, ensuring the full suite runs in CI and eliminating this debugging
artifact.
---
Outside diff comments:
In `@src/transport/subscription-worker/components/pubnub-clients-manager.ts`:
- Around line 211-214: In unregisterClient (pubnub-clients-manager.ts) avoid
calling splice with -1 by checking that the client exists before removing:
locate the code that accesses this.clientBySubscribeKey[client.subKey] and
computing clientIdx = clientsBySubscribeKey.indexOf(client) and add a guard like
clientIdx !== -1 (or use a filter to remove the exact reference) before calling
clientsBySubscribeKey.splice; mirror the same presence check used in
suspendClient to prevent accidentally removing the last array element when the
client isn't found.
In `@src/transport/subscription-worker/subscription-worker-middleware.ts`:
- Around line 440-445: The 'shared-worker-client-suspended' message is not being
filtered by clientIdentifier, causing suspend events from other clients to
reject this instance's in-flight requests; update the message handling so the
condition that checks data.clientIdentifier !==
this.configuration.clientIdentifier also applies to data.type ===
'shared-worker-client-suspended', and modify the logic that rejects pending
callbacks (the method that currently rejects all pending callbacks when handling
'shared-worker-client-suspended') to only reject callbacks associated with the
suspended client's identifier (use the same clientIdentifier field on the
pending callback entries) instead of rejecting every outstanding callback.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af3072b3-e398-4591-a9aa-2e0658bb5f06
⛔ Files ignored due to path filters (4)
dist/web/pubnub.jsis excluded by!**/dist/**dist/web/pubnub.min.jsis excluded by!**/dist/**,!**/*.min.jsdist/web/pubnub.worker.jsis excluded by!**/dist/**dist/web/pubnub.worker.min.jsis excluded by!**/dist/**,!**/*.min.js
📒 Files selected for processing (12)
lib/types/index.d.tssrc/transport/subscription-worker/components/custom-events/client-event.tssrc/transport/subscription-worker/components/custom-events/client-manager-event.tssrc/transport/subscription-worker/components/heartbeat-requests-manager.tssrc/transport/subscription-worker/components/pubnub-client.tssrc/transport/subscription-worker/components/pubnub-clients-manager.tssrc/transport/subscription-worker/components/subscribe-requests-manager.tssrc/transport/subscription-worker/components/subscription-state.tssrc/transport/subscription-worker/subscription-worker-middleware.tssrc/transport/subscription-worker/subscription-worker-types.tssrc/web/index.tstest/integration/shared-worker/shared-worker.test.ts
Add defensive guards for splice index, abort controller reuse, token chain recovery, and remove `it.only` from tests.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/transport/subscription-worker/components/pubnub-clients-manager.ts (1)
178-194: 💤 Low valueConsider guarding against reactivating an already-active client.
reactivateClientdoesn't check if the client is already registered inthis.clients. If called for a non-suspended client (due to race conditions or programming error), it would overwrite the existing entry without aborting the previousAbortController, potentially leaking listeners.🛡️ Suggested defensive guard
private reactivateClient(client: PubNubClient) { + // Abort any existing controller to prevent duplicate listeners if called erroneously. + const existing = this.clients[client.identifier]; + if (existing?.abortController) existing.abortController.abort(); + this.clients[client.identifier] = { client, abortController: new AbortController() };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/transport/subscription-worker/components/pubnub-clients-manager.ts` around lines 178 - 194, The reactivateClient method can overwrite an existing this.clients[client.identifier] and leak the previous AbortController/listeners; update reactivateClient to first check if this.clients[client.identifier] already exists and either (a) if already active, return early (no-op) or (b) if you intend to replace, call abort() on the existing entry's abortController and clean up subscriptions before overwriting; ensure you also avoid duplicating entries in this.clientBySubscribeKey[client.subKey] when reactivating and only call subscribeOnClientEvents and dispatchEvent after safely replacing or confirming activation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/transport/subscription-worker/components/pubnub-clients-manager.ts`:
- Around line 178-194: The reactivateClient method can overwrite an existing
this.clients[client.identifier] and leak the previous AbortController/listeners;
update reactivateClient to first check if this.clients[client.identifier]
already exists and either (a) if already active, return early (no-op) or (b) if
you intend to replace, call abort() on the existing entry's abortController and
clean up subscriptions before overwriting; ensure you also avoid duplicating
entries in this.clientBySubscribeKey[client.subKey] when reactivating and only
call subscribeOnClientEvents and dispatchEvent after safely replacing or
confirming activation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62cc01f8-2c2e-46c4-9d68-59986c60ec51
⛔ Files ignored due to path filters (4)
dist/web/pubnub.jsis excluded by!**/dist/**dist/web/pubnub.min.jsis excluded by!**/dist/**,!**/*.min.jsdist/web/pubnub.worker.jsis excluded by!**/dist/**dist/web/pubnub.worker.min.jsis excluded by!**/dist/**,!**/*.min.js
📒 Files selected for processing (5)
src/transport/subscription-worker/components/heartbeat-requests-manager.tssrc/transport/subscription-worker/components/pubnub-clients-manager.tssrc/transport/subscription-worker/components/subscribe-requests-manager.tssrc/transport/subscription-worker/subscription-worker-middleware.tstest/integration/shared-worker/shared-worker.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/transport/subscription-worker/subscription-worker-middleware.ts
- src/transport/subscription-worker/components/subscribe-requests-manager.ts
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
2 participants
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.
feat(shared-worker): client suspension and reactivation
Implement suspension mechanism that keeps the port listener alive when a client misses ping-pong, removes it from subscription/heartbeat state, and reactivates on next message from the resumed tab.
fix(shared-worker):
squashedChangesiterates wrong arrayFix inner loop in
squashedChangesto iteratesortedChangesinstead ofrequestAddChangeso that add+remove pair elimination actually executes for same-request entries within aggregation window.fix(shared-worker): orphaned
SubscriptionStateon empty changesGuard
handleDelayedAggregationagainst creating subscription state when changes queue is empty, and add self-invalidation inprocessChangeswhen post-squash changes are empty with no active clients.fix(shared-worker):
BFCacheinteraction withpagehidelistenerRemove
{ once: true }frompagehidelistener so the handler fires correctly when the page truly navigates away after a prior BFCache entry consumed the one-shot listener.fix(shared-worker): version comparison and token handling
Replace independent semver component comparison with hierarchical check, fix
accessTokensMapbeing overwritten on each new token, and serializeonTokenChangewith a promise chain for ordering.fix(tests): shared worker test reliability
Replace fixed-delay sequential subscription with channel-list verification on status events, and fix
makeSendableoverrides to always return a valid tuple after callingdone().