refactor(event-stream)!: drop autoclose option#1495
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe ChangesSSE API and lifecycle
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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/utils/internal/event-stream.ts (1)
28-43: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd regression coverage for never-sent stream disposal.
This constructor adds a new cleanup path for streams that are created but never sent. Add a test that disposes the event without calling
send(), verifiesonClosedruns, and confirms subsequent pushes are ignored.🤖 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/utils/internal/event-stream.ts` around lines 28 - 43, The constructor’s new event-disposal cleanup path lacks regression coverage for never-sent streams. Add a test that creates the event stream, registers an onClosed callback, disposes the event without calling send(), verifies the callback runs, and confirms pushes made afterward are ignored.
🤖 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/utils/internal/event-stream.ts`:
- Around line 28-43: The constructor’s new event-disposal cleanup path lacks
regression coverage for never-sent streams. Add a test that creates the event
stream, registers an onClosed callback, disposes the event without calling
send(), verifies the callback runs, and confirms pushes made afterward are
ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 08d72d64-69c4-4b26-add9-b6cd0fbbf771
📒 Files selected for processing (4)
docs/2.utils/2.response.mdsrc/index.tssrc/utils/event-stream.tssrc/utils/internal/event-stream.ts
The `autoclose` option existed in v1 to opt out of Node.js event listeners. The v2 rewrite and the `onDispose` hook replaced that system, leaving the option vestigial: - `autoclose: false` did not keep streams writable after disconnect (the transport still fails), so it had no practical effect. - A stream created with `autoclose: false` but never `send()`-ed would never fire its `onClosed` callbacks — a silent leak with no user-recoverable workaround. - The `false` path was untested. Remove the option before v2 stable. Auto-close via `onDispose` is now always on. `EventStreamOptions` and the `createEventStream(event, opts?)` signature are kept (now empty) for future config and signature compat. Closes #1489 Co-Authored-By: Claude Opus 4.8 <[email protected]>
pi0x
force-pushed
the
refactor/drop-eventstream-autoclose
branch
from
July 22, 2026 09:57
06523b0 to
71dd047
Compare
pi0
approved these changes
Jul 22, 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
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.
Closes #1489
What
Removes the
autocloseoption fromEventStream. TheEventStreamOptionsinterface and thecreateEventStream(event, opts?)signature are kept (options object is now empty) for future configuration and signature compatibility.Why
autoclose(defaulttrue) existed in v1 to opt out of Node.js event listeners. The v2 rewrite plus theonDisposehook replaced that whole mechanism, leaving the option vestigial:autoclose: falsedid not keep the stream writable after client disconnect; the transport layer fails regardless.autoclose: falsebut neversend()-ed never fired itsonClosedcallbacks, with no user-recoverable workaround.falsepath.Auto-close via
onDisposeis now always on, which was already the effective default.Changes
src/utils/event-stream.ts— drop theautoclosefield fromEventStreamOptions(interface kept, now empty);createEventStream(event, opts?)signature unchanged.src/utils/internal/event-stream.ts— drop theautoclosebranches; always registeronDisposecleanup and mark disposed on writer close. Constructor still accepts an (unused) options arg.src/index.ts—EventStreamOptionsexport retained.Notes
Breaking only for callers passing
autoclose(the SSE API is@experimentaland this lands before v2 stable). The public signature and exported types are otherwise unchanged. Existing SSE tests (including client-disconnect auto-close) pass unchanged.Summary by CodeRabbit