fix(defineNetwork): prevent event forwarding manually#2740
Merged
Conversation
📝 WalkthroughWalkthroughCentralizes per-frame listener cleanup in a Disposable, replaces AbortController/session cancellation with enable-scoped session.active gating for event forwarding, updates configure()'s invariant message to require DISABLED, and makes Disposable.dispose() aggregate and report subscription errors. ChangesState Management Refactor
Sequence Diagram(s)sequenceDiagram
participant Frame as frame.events
participant Network as defineNetwork.events
participant Session as enable session
Frame->>Network: emit('*', event)
Network->>Session: check session.active
alt session.active === true
Network->>Network: events.emit(event)
else session.active !== true
Network->>Network: suppress forwarding
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/experimental/define-network.ts (1)
175-187: ⚡ Quick winScope event forwarding to the current enable cycle.
This guard only suppresses forwarding while the network is disabled. If a frame listener survives into a later
enable()call, it becomes active again because it closes over the shared mutablereadyState. The oldAbortControllerbehavior invalidated those listeners permanently for that session, so a generation/token check here would preserve that isolation without crossing request contexts.Suggested direction
let enableGeneration = 0 enable() { // ... const currentGeneration = ++enableGeneration source.on('frame', async ({ frame }) => { frame.events.on('*', (event) => { if ( readyState !== NetworkReadyState.ENABLED || enableGeneration !== currentGeneration ) { return } events.emit(event) }) // ... }) }🤖 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/core/experimental/define-network.ts` around lines 175 - 187, The frame event handler currently only checks readyState and can be reactivated across subsequent enable() calls because it closes over the shared mutable readyState; introduce a generation/token check (e.g., an enableGeneration counter incremented inside enable() and captured as currentGeneration for that enable cycle) and additionally verify enableGeneration === currentGeneration (alongside readyState === NetworkReadyState.ENABLED) before forwarding via events.emit in the frame.events.on('*'...) callback so listeners from prior enable() cycles are inert; update enable() to increment the generation and capture currentGeneration for each installed listener.
🤖 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/core/experimental/define-network.ts`:
- Around line 175-187: The frame event handler currently only checks readyState
and can be reactivated across subsequent enable() calls because it closes over
the shared mutable readyState; introduce a generation/token check (e.g., an
enableGeneration counter incremented inside enable() and captured as
currentGeneration for that enable cycle) and additionally verify
enableGeneration === currentGeneration (alongside readyState ===
NetworkReadyState.ENABLED) before forwarding via events.emit in the
frame.events.on('*'...) callback so listeners from prior enable() cycles are
inert; update enable() to increment the generation and capture currentGeneration
for each installed listener.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 393ba05a-e744-4c40-a86f-9e94fe62d23a
📒 Files selected for processing (1)
src/core/experimental/define-network.ts
kettanaito
force-pushed
the
fix/manual-event-forwarding-prevention
branch
from
6fc97e6 to
b79c2e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/core/utils/internal/Disposable.ts`:
- Around line 15-19: The catch block only pushes instances of Error into the
errors array, so non-Error throws are ignored; update the catch in
src/core/utils/internal/Disposable.ts to convert any non-Error throw into an
Error before pushing (e.g., if (error instanceof Error) errors.push(error); else
errors.push(new Error(String(error)) ) ), and optionally preserve the original
value as the cause/property when supported so the original thrown value is still
accessible; target the same catch block that populates the errors array.
🪄 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: ddcd77f7-68cd-4a80-9fef-197a9eeeca70
📒 Files selected for processing (2)
src/core/experimental/define-network.tssrc/core/utils/internal/Disposable.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/experimental/define-network.ts
commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/experimental/define-network.ts (1)
173-175: ⚡ Quick winAdd a public
add()method toDisposableto expose the subscription registration API.
disposable['subscriptions']at line 173 couplesdefineNetworkto a protected implementation detail. A public method likeadd(subscription: DisposableSubscription): voidonDisposablewould make this lifecycle contract explicit and eliminate the need for bracket notation access.Suggested refactor
// src/core/utils/internal/Disposable.ts export class Disposable { protected subscriptions: Array<DisposableSubscription> = [] + + public add(subscription: DisposableSubscription): void { + this.subscriptions.push(subscription) + } public dispose() { // ... } } // src/core/experimental/define-network.ts - disposable['subscriptions'].push(() => { + disposable.add(() => { session.active = false })🤖 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/core/experimental/define-network.ts` around lines 173 - 175, The code in defineNetwork accesses Disposable's internal array via disposable['subscriptions'].push(...), coupling to a protected implementation detail; add a public method add(subscription: DisposableSubscription): void to the Disposable class that internally pushes the subscription onto its subscriptions array, then replace the bracket access in defineNetwork (where disposable['subscriptions'].push(() => { session.active = false })) with a call to disposable.add(() => { session.active = false }); this makes the lifecycle contract explicit and avoids direct access to the internal subscriptions storage.
🤖 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/core/experimental/define-network.ts`:
- Around line 187-200: The per-frame listener registered via
frame.events.on('*', ...) is never disposed and accumulates across
enable/disable cycles; modify the code that registers the listener (inside
defineNetwork/session setup) to capture the returned AbortController from
frame.events.on(...) into a variable (e.g., perFrameAbort) and ensure that when
the session/disable() path runs you call perFrameAbort.abort(); alternatively,
mirror the removal pattern used elsewhere (e.g., removeAllListeners in
interceptor-source) by removing that frame listener during disable — update the
registration site (frame.events.on) and the corresponding dispose/disable logic
to abort/remove the listener so events no longer accumulate while keeping the
existing session.active guard.
---
Nitpick comments:
In `@src/core/experimental/define-network.ts`:
- Around line 173-175: The code in defineNetwork accesses Disposable's internal
array via disposable['subscriptions'].push(...), coupling to a protected
implementation detail; add a public method add(subscription:
DisposableSubscription): void to the Disposable class that internally pushes the
subscription onto its subscriptions array, then replace the bracket access in
defineNetwork (where disposable['subscriptions'].push(() => { session.active =
false })) with a call to disposable.add(() => { session.active = false }); this
makes the lifecycle contract explicit and avoids direct access to the internal
subscriptions storage.
🪄 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: 4bb0b048-1d48-4424-b1c4-7dc4d2ded2ea
📒 Files selected for processing (1)
src/core/experimental/define-network.ts
Member
Author
Released: v2.14.6 🎉This has been released in v2.14.6. Get these changes by running the following command: Predictable release automation by Release. |
1 task
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.
Changes
defineNetworkno longer introduces anAbortControllerto nuke all the*listeners attached to individual network frames. Instead, it adds a manual check onreadyStateto prevent the forwarding of frame events after the network has been disabled.Motivation
While working on
@msw/cloudflare, I've encountered the following issue:It was caused by MSW reading
listenersController.signal.abortedindefineNetwork. The error originates from the guard workerd has against using abort controllers created in different request contexts:History
Previously, the abort controller was introduced to prevent listener leaks during enable->disable->enable chains. Now, every frame calls
this.events.removeAllListeners()by itself upon finish.Frames don't survive enable->disable->enable chain so there's no leak. Sources do, but sources remove all their listeners on dispose already.