feat(standard-server): send initial comment in event stream to flush response headers immediately #1204
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @unnoq, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a feature to optimize Server-Sent Events (SSE) streams by sending an immediate, empty comment. This ensures that HTTP response headers are flushed as soon as the stream starts, which can significantly improve the perceived responsiveness and connection establishment time for clients consuming the event stream. The change is reflected in both the core implementation and updated test cases. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds optional initial SSE comment emission at stream start and emits a leading keep-alive prefix before event data; introduces two ToEventStream options ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Stream as ReadableStream
participant Producer
rect rgb(250,250,245)
Note over Stream,Producer: Stream initialization — optional initial comment
Client->>Stream: reader.read()
Stream->>Producer: start(controller)
alt initial comment enabled
Producer->>Stream: enqueue(": <initial>\n\n")
Stream-->>Client: ": <initial>\n\n"
else initial comment disabled
Producer->>Stream: (no initial comment)
end
end
rect rgb(245,250,255)
Note over Stream,Producer: Keep-alive (periodic) and event frames
loop periodic
alt keep-alive enabled
Producer->>Stream: enqueue(": \n\n")
Stream-->>Client: ": \n\n"
end
Producer->>Stream: enqueue("event: ...\ndata: ...\n\n")
Stream-->>Client: "event/data frame"
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a performance optimization for server-sent event (SSE) streams by sending an initial empty comment immediately when the stream starts. This causes response headers to flush immediately, allowing clients to establish connections earlier without waiting for the first actual event from the iterator.
- Added initial empty comment (
': \n\n') in thestart()method of the ReadableStream - Updated all test cases to expect and consume the initial comment before subsequent events
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/standard-server-fetch/src/event-iterator.ts | Added logic to send an initial empty comment in the stream's start() method to flush response headers immediately |
| packages/standard-server-fetch/src/event-iterator.test.ts | Updated all tests to expect the initial comment as the first chunk in event streams, ensuring test assertions align with the new behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request introduces a useful feature to immediately flush response headers for event streams by sending an initial comment. The implementation in toEventStream is straightforward and correct. The accompanying test updates are thorough, but I've identified a few minor inconsistencies in the testing style that could be improved for better maintainability and robustness. I've left specific suggestions in the comments below.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/standard-server-fetch/src/event-iterator.test.ts (1)
321-321: Consider adding assertions for consistency.These lines correctly consume the initial empty-comment frame before testing cancellation scenarios. However, for consistency with other tests and explicit documentation of the behavior, consider asserting the value:
- await reader.read() + expect((await reader.read())).toEqual({ done: false, value: ': \n\n' })This makes it clearer that we're intentionally consuming the initial frame and maintains consistency with other test cases.
Also applies to: 351-351, 382-382
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/standard-server-fetch/src/event-iterator.test.ts(8 hunks)packages/standard-server-fetch/src/event-iterator.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/standard-server-fetch/src/event-iterator.ts (2)
packages/shared/src/otel.ts (1)
startSpan(44-47)packages/standard-server/src/event-iterator/encoder.ts (1)
encodeEventMessage(52-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: publish-commit
- GitHub Check: test
🔇 Additional comments (2)
packages/standard-server-fetch/src/event-iterator.ts (1)
154-164: LGTM! Excellent improvement for connection establishment.The implementation correctly sends an initial SSE comment event to flush response headers immediately. This is a well-established pattern in server-sent events that ensures clients can establish the connection without waiting for the first data event.
The use of
encodeEventMessage({ comments: [''] })produces the valid SSE format: \n\n(comment line + blank line terminator), which complies with the SSE specification.packages/standard-server-fetch/src/event-iterator.test.ts (1)
225-225: LGTM! Test assertions correctly verify the initial frame.The tests properly assert that the initial empty-comment frame (
: \n\n) is sent before any data events across various scenarios (with/without return, error cases, keep-alive). This ensures the implementation behavior is well-tested.Also applies to: 247-247, 269-269, 291-291, 415-415
More templates
@orpc/ai-sdk
@orpc/arktype
@orpc/client
@orpc/contract
@orpc/experimental-durable-iterator
@orpc/hey-api
@orpc/interop
@orpc/json-schema
@orpc/nest
@orpc/openapi
@orpc/openapi-client
@orpc/otel
@orpc/experimental-pino
@orpc/experimental-publisher
@orpc/experimental-ratelimit
@orpc/react
@orpc/react-query
@orpc/experimental-react-swr
@orpc/server
@orpc/shared
@orpc/solid-query
@orpc/standard-server
@orpc/standard-server-aws-lambda
@orpc/standard-server-fastify
@orpc/standard-server-fetch
@orpc/standard-server-node
@orpc/standard-server-peer
@orpc/svelte-query
@orpc/tanstack-query
@orpc/trpc
@orpc/valibot
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…response headers immediately
Adds an initial empty comment (': \n\n') in the `start()` method of `toEventStream`
to immediately flush response headers to the client. This ensures the connection is
established without waiting for others events
f1b55ce to
dafdae4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/standard-server-node/src/event-iterator.test.ts (1)
35-35: Consider adding test coverage for disabled keep-alive behavior.While this test correctly validates the default behavior (initial keep-alive chunk
: \n\n), consider adding a companion test case that explicitly disables the keep-alive to ensure both code paths are covered. Other test files in this PR pass explicit options likeeventIteratorKeepAliveEnabled: false.For example, add a test case like:
it('toEventStream without keep-alive', async () => { async function* gen() { yield 1 yield 2 } const reader = Readable.toWeb(toEventStream(gen(), { eventIteratorKeepAliveEnabled: false })) .pipeThrough(new TextDecoderStream()) .getReader() // Should NOT have initial keep-alive chunk expect((await reader.read())).toEqual({ done: false, value: 'event: message\ndata: 1\n\n' }) expect((await reader.read())).toEqual({ done: false, value: 'event: message\ndata: 2\n\n' }) expect((await reader.read())).toEqual({ done: true, value: undefined }) })apps/content/docs/adapters/http.md (1)
250-298: Well-documented feature addition!The new Event Iterator Configuration section clearly documents the initial comment and keep-alive options. The structure with separate subsections, option tables, and usage examples is easy to follow.
Minor suggestion: Consider expanding the warning at lines 278-279 to clarify when link-side event iterator options would be useful (e.g., "only when your client is sending an event stream to the server, which is uncommon in most applications").
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/content/docs/adapters/http.md(1 hunks)apps/content/docs/client/rpc-link.md(0 hunks)apps/content/docs/openapi/client/openapi-link.md(0 hunks)apps/content/docs/openapi/openapi-handler.md(0 hunks)apps/content/docs/rpc-handler.md(0 hunks)packages/standard-server-aws-lambda/src/response.test.ts(3 hunks)packages/standard-server-fastify/src/response.test.ts(1 hunks)packages/standard-server-fetch/src/body.test.ts(1 hunks)packages/standard-server-fetch/src/event-iterator.test.ts(9 hunks)packages/standard-server-fetch/src/event-iterator.ts(2 hunks)packages/standard-server-fetch/src/response.test.ts(1 hunks)packages/standard-server-node/src/body.test.ts(1 hunks)packages/standard-server-node/src/event-iterator.test.ts(1 hunks)packages/standard-server-node/src/response.test.ts(3 hunks)
💤 Files with no reviewable changes (4)
- apps/content/docs/rpc-handler.md
- apps/content/docs/client/rpc-link.md
- apps/content/docs/openapi/openapi-handler.md
- apps/content/docs/openapi/client/openapi-link.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/standard-server-fetch/src/event-iterator.ts
- packages/standard-server-node/src/body.test.ts
- packages/standard-server-fetch/src/event-iterator.test.ts
- packages/standard-server-fetch/src/response.test.ts
- packages/standard-server-fastify/src/response.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: publish-commit
🔇 Additional comments (7)
packages/standard-server-aws-lambda/src/response.test.ts (3)
96-139: LGTM! Initial comment chunk properly tested.The test correctly verifies that an initial keep-alive comment (
Buffer.from(': \n\n')) is emitted before the event data chunks, ensuring headers are flushed immediately when streaming begins.
141-195: LGTM! Stream cancellation with initial comment tested.The test properly verifies that the initial comment chunk is sent before event data even in the error/cancellation scenario, and that cleanup occurs correctly when the stream is destroyed.
197-251: LGTM! Stream cancellation without error properly tested.The test correctly handles the initial comment emission and stream cancellation without error, verifying proper cleanup behavior.
packages/standard-server-node/src/response.test.ts (3)
120-161: LGTM! Async generator streaming test properly updated.The test correctly verifies that the response text begins with the initial comment (
': \n\n') followed by the event data. The full-text assertion approach is appropriate for the node adapter's response handling.
163-219: LGTM! Stream destruction with error properly tested.The test correctly verifies initial comment emission and proper cleanup when the stream is destroyed with an error.
221-277: LGTM! Stream destruction without error properly tested.The test correctly verifies initial comment emission and proper cleanup when the stream is destroyed gracefully.
packages/standard-server-fetch/src/body.test.ts (1)
280-302: Verify test configuration for initial comment behavior.The test sets
eventIteratorKeepAliveEnabled: false(line 285) but still expects an initial': \n\n'chunk (line 299). This appears to be testing that the initial comment is emitted even when periodic keep-alive is disabled, which aligns with the feature design where initial comments and keep-alive are separate options.However, the test doesn't explicitly set
eventIteratorInitialCommentEnabled. If the default istrue(as documented), this test is implicitly relying on that default.Consider explicitly setting
eventIteratorInitialCommentEnabled: truein the options to make the test's intent clearer:- const options = { eventIteratorKeepAliveEnabled: false } + const options = { eventIteratorKeepAliveEnabled: false, eventIteratorInitialCommentEnabled: true }Alternatively, if this test is specifically meant to verify the default behavior, add a comment explaining that.
Additionally, verify that there's test coverage for when
eventIteratorInitialCommentEnabled: falseto ensure the initial comment can be disabled:
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/standard-server-fetch/src/event-iterator.test.ts (1)
247-255: Inconsistent assertion patterns across tests.Lines 247-251 check both
doneandvalueproperties, while lines 270-272 and 292-295 only check.value. For improved robustness and consistency, consider standardizing on checking both properties throughout the test suite, especially for the assertions following the initial comment check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/content/docs/adapters/http.md(1 hunks)packages/standard-server-fetch/src/event-iterator.test.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/content/docs/adapters/http.md
🧰 Additional context used
🧬 Code graph analysis (1)
packages/standard-server-fetch/src/event-iterator.test.ts (1)
packages/standard-server-fetch/src/event-iterator.ts (2)
toEventStream(156-262)start(171-179)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: publish-commit
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (4)
packages/standard-server-fetch/src/event-iterator.test.ts (4)
221-234: LGTM! Initial comment assertion added correctly.The test properly accounts for the new initial comment behavior, checking for
': \n\n'as the first read value before subsequent messages.
317-322: LGTM! Extra read correctly accounts for initial comment.The additional
await reader.read()at line 321 properly consumes the initial comment before canceling, maintaining the test's intent to cancel after reading actual data.
396-432: Comprehensive keep-alive test coverage.The test correctly validates keep-alive behavior with appropriate timing expectations. The logic is sound: with a 40ms interval and 100ms generator delay, two keep-alive pings should be sent before each message.
468-511: Excellent test coverage for initial comment feature.The tests properly validate both enabled and disabled states of the initial comment feature, with clear timing expectations to verify immediate delivery when enabled (< 10ms) versus waiting for the first actual message when disabled (>= 50ms).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/standard-server-fetch/src/event-iterator.test.ts (4)
269-273: Assertion style inconsistency remains unaddressed.As previously noted, line 269 checks both
doneandvalueproperties while lines 270-272 only check.value. This inconsistency affects test robustness. Consider standardizing to always check both properties.
291-295: Assertion style inconsistency remains unaddressed.Similar to the previous test, line 291 checks both
doneandvaluewhile lines 292-295 only check.value. Standardizing the assertion pattern would improve test consistency.
416-439: Timing assertions remain potentially flaky (unresolved from previous review).The tight timing windows (e.g.,
< 50ms,< 100ms,< 130ms) throughout this test could still fail in resource-constrained CI environments. As previously suggested, consider:
- Widening upper bounds (e.g.,
< 150msinstead of< 130ms)- Using
vi.waitForwith timing predicates instead of single point-in-time checks
464-471: Tight timing windows in disabled test may also be flaky.Similar to the enabled test, lines 466-467 and 470-471 use tight timing assertions (
< 110ms,< 220ms) that could fail under CI load. Consider relaxing these bounds for better reliability.
🧹 Nitpick comments (2)
packages/standard-server-fetch/src/event-iterator.test.ts (2)
478-501: Initial comment test logic is sound, but consider relaxing timing bounds.The test correctly verifies that:
- Initial comment is sent immediately on stream start (line 496-497)
- Custom comment text is used (line 487)
- Keep-alive is properly isolated (line 488)
However, lines 499-500 use a tight upper bound (
< 60ms) that could be relaxed to< 80msfor better CI reliability.
503-522: Disabled test correctly verifies absence of initial comment.The test properly confirms that when
eventIteratorInitialCommentEnabled: false, no initial comment is sent and the first read directly receives the message event. Consider widening line 521's upper bound to< 80msfor CI robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/standard-server-fetch/src/event-iterator.test.ts(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/standard-server-fetch/src/event-iterator.test.ts (2)
packages/standard-server-fetch/src/event-iterator.ts (2)
toEventStream(156-262)start(171-179)packages/standard-server-node/src/event-iterator.ts (1)
toEventStream(22-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: publish-commit
- GitHub Check: test
🔇 Additional comments (3)
packages/standard-server-fetch/src/event-iterator.test.ts (3)
221-230: LGTM! Tests correctly updated for initial comment default behavior.The removal of the explicit empty options object is cleaner, and the new assertion on line 225 correctly verifies that the initial comment (
: \n\n) is sent as the first value when using default options.
317-322: LGTM! Correctly adapted for initial comment.The additional
reader.read()call on line 321 is necessary to consume the initial comment before testing cancellation behavior. This ensures the test logic remains correct with the new default behavior.
530-540: LGTM! Enhanced roundtrip test with keep-alive interaction.The increased delay (50ms on line 532) and addition of
eventIteratorKeepAliveInterval: 10(line 535) improves test coverage by exercising the interaction between keep-alive messages and actual events in the roundtrip scenario.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/content/docs/adapters/http.md (1)
273-279: Minor clarity suggestion: Clarify the initial comment's primary use case.The warning at lines 277–279 correctly notes that link options apply to client-to-server streaming. However, since the initial comment feature is primarily designed for server-to-client event streaming (to flush response headers), it might be helpful to add a brief note clarifying that
eventIteratorInitialCommentEnabledandeventIteratorInitialCommenton links are typically not needed in common patterns.Consider updating the warning to explicitly mention that initial comment options on links are rarely needed:
::: warning -Link options apply when streaming from **client to server**, not server to client (as with handlers). In most cases, you don't need to configure these on the link. +Link options apply when streaming from **client to server**, not server to client (as with handlers). In most cases, you don't need to configure these on the link. Note: `eventIteratorInitialComment*` options are typically only relevant for handlers, not links. :::
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/content/docs/adapters/http.md(1 hunks)packages/standard-server-fetch/src/event-iterator.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/standard-server-fetch/src/event-iterator.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: publish-commit
- GitHub Check: test
- GitHub Check: lint
🔇 Additional comments (1)
apps/content/docs/adapters/http.md (1)
250-289: Documentation defaults are accurate and consistent with implementation.Verification confirms all documented Event Iterator Options defaults match the actual implementation in
packages/standard-server-fetch/src/event-iterator.ts:
eventIteratorInitialCommentEnabled:true✓eventIteratorInitialComment:''✓eventIteratorKeepAliveEnabled:true✓eventIteratorKeepAliveInterval:5000✓eventIteratorKeepAliveComment:''✓The documentation is well-structured, accurate, and complete.
Summary by CodeRabbit