fix: address code review findings - #5
Merged
Merged
Conversation
… and testing - Add SSEProviderError class with typed error codes (TRANSPORT/NETWORK/PARSE) - Fix non-Error throws in parseEvent catch blocks to properly wrap values - Add re-entrancy guard and generation counter for rapid URL changes - Tighten SSEConfigWithSchema.schema type from any to unknown - Re-export formatSSEEvent/formatSSEData from server entry point - Add maxAttempts exhaustion docs and troubleshooting section to SPEC.md - Add latency simulation (setLatency/resetLatency) to mockSSE testing utility - Add @ts-expect-error regression guards for ParsedEvent type enforcement - Add explanatory comment for listener memoization design decision - 30 new tests covering error wrapping, race conditions, and latency simulation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds re-entrancy and generation guards to SSEProvider to ignore stale callbacks, introduces SSEProviderError and SSEErrorCode for structured error classification, extends MockSSEControls with per-instance latency and async send APIs, adds tests for races/parse errors/latency, and expands docs with maxAttempts exhaustion and troubleshooting guidance. Changes
Sequence DiagramsequenceDiagram
participant Client
participant SSEProvider
participant Guard as CreatingGuard
participant Transport
participant Callbacks
Client->>SSEProvider: request connect / rapid URL changes (A → B → C)
SSEProvider->>Guard: check creatingConnectionRef
alt not creating
Guard-->>SSEProvider: mark creating, bump generation (gen N)
SSEProvider->>Transport: create transport (captures gen N)
else creating
Guard-->>SSEProvider: bail (no new create)
end
Note over Transport,SSEProvider: Transport events may arrive after new generation
Transport->>SSEProvider: onopen / onmessage / onerror (gen M)
SSEProvider->>SSEProvider: isActiveConnection? (gen M == current gen?)
alt active
SSEProvider->>Callbacks: invoke handlers (onConnect/onMessage/onError)
else superseded
SSEProvider-->>Transport: ignore event
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
docs/SPEC.md (2)
1054-1057: Add language specifier to fenced code block.Same issue — this SSE wire format example should have a language identifier.
📝 Proposed fix
-``` +```text data: {"type":"order:updated","payload":{...}}\n\n</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/SPEC.mdaround lines 1054 - 1057, Update the fenced code block showing
the SSE wire format example in SPEC.md to include a language specifier (use
"text") so the block starts with ```text; locate the block containing data:
{"type":"order:updated","payload":{...}}\n\n and add the language identifier to
the opening fence to ensure proper syntax highlighting and consistency with
other examples.</details> --- `1024-1029`: **Add language specifier to fenced code block.** The fenced code block starting at line 1024 is missing a language identifier, which helps with syntax highlighting and accessibility. <details> <summary>📝 Proposed fix</summary> ```diff -``` +```text # Verify with curl curl -v -N -H "Accept: text/event-stream" http://localhost:3000/api/events # Look for: < HTTP/1.1 200 OK # < Content-Type: text/event-stream ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/SPEC.mdaround lines 1024 - 1029, The fenced code block containing the
curl example lacks a language specifier; update the triple-backtick fence that
wraps the curl snippet (the block starting with "# Verify with curl") to include
a language identifier such as "text" (e.g., ```text) so syntax highlighting and
accessibility are improved for the curl example.</details> </blockquote></details> <details> <summary>src/server/index.ts (1)</summary><blockquote> `2-6`: **Redundant import can be removed.** Line 2 imports `formatSSEEvent` which is then re-exported on line 6. Since the re-export makes it available in this module's scope, the separate import is unnecessary. <details> <summary>🧹 Proposed cleanup</summary> ```diff // Server-side utilities for reactiveSWR -import { formatSSEEvent } from '../sseParser' import type { SSEAdapter } from './adapters/types.ts' // SSE formatting helpers — server-side only export { formatSSEData, formatSSEEvent } from '../sseParser' ``` Then update usages of `formatSSEEvent` within this file to reference it directly (the re-export makes it available in the module scope). </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/server/index.ts` around lines 2 - 6, Remove the redundant named import of formatSSEEvent from the top-level import list and rely on the existing re-export (export { formatSSEData, formatSSEEvent } from '../sseParser') so usages in this module reference formatSSEEvent directly; specifically, delete the import clause that brings in formatSSEEvent from '../sseParser' while keeping any other imports (e.g., type SSEAdapter) intact and verify no other code relies on the removed import statement. ``` </details> </blockquote></details> <details> <summary>src/SSEProvider.tsx (1)</summary><blockquote> `542-560`: **Add generation check to `onmessage` handler for consistency.** The `onopen` and `onerror` handlers both guard against stale connections using `isActiveConnection()`, but `onmessage` does not. If a superseded connection receives a message before cleanup completes, it will still dispatch to `processEvent`. If strict ordering is required during rapid URL changes, apply the same guard here. This may be intentional (allowing in-flight messages to be processed), but the inconsistency should be clarified. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/SSEProvider.tsx` around lines 542 - 560, The onmessage handler can process messages from a stale connection; add the same generation guard used in onopen/onerror by calling isActiveConnection() at the top of the connection.onmessage callback and returning early if it’s false, so only the active connection calls processEvent; ensure you still perform the try/catch and call configRef.current.onEventError with SSEProviderError when parsing fails, referencing connection.onmessage, isActiveConnection, processEvent, configRef, and SSEProviderError to locate the changes. ``` </details> </blockquote></details> <details> <summary>src/__tests__/connection.test.tsx (1)</summary><blockquote> `819-845`: **Test name doesn't match implementation.** The test title claims to verify behavior "when URL changes during createConnection," but it only renders once with a single URL. Due to SSR (`renderToString`) limitations acknowledged in the suite comment, this test verifies the simpler invariant that a single render creates exactly one connection. Consider renaming to clarify what the test actually validates, e.g., `"should create exactly one EventSource on initial render"`. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/__tests__/connection.test.tsx` around lines 819 - 845, Rename the test's description to match what it actually verifies: change the it(...) title that currently reads "should create only one EventSource when URL changes during createConnection" to something like "should create exactly one EventSource on initial render" (or equivalent). Update the test title in the test case containing the SSEProvider render and createdUrls assertions so the name reflects a single render/SSR behavior instead of a URL-change scenario. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@src/__tests__/testing-utils-latency.test.ts:
- Around line 127-149: The test uses optional chaining when creating reader
(response.body?.getReader()) so reader may be undefined but later calls
reader.read(); update the test to assert or guard that reader is defined before
using it (e.g., check response.body or assert reader is truthy) so calls to
reader.read() and reader.cancel() are safe; locate this in the 'sendRaw delay'
test where mockSSE, response, and reader are used and add a simple
assertion/guard immediately after creating reader to prevent a null reference.In
@src/testing/index.ts:
- Around line 15-22: The API for the testing mock changed: sendEvent, sendRaw,
and sendSSE now return Promise (previously void), which can trigger
@typescript-eslint/no-floating-promisesin existing tests; update the PR
description or project migration guide to call out this breaking change and
recommend fixes (await the promises, use void operator, or adjust tests to
handle the returned Promise), and mention that MockEventSource consumers may
need to update any synchronous assumptions tied to latencyMs behavior so
consumers (tests using sendEvent/sendRaw/sendSSE) are aware to await or
explicitly ignore the returned Promise.
Nitpick comments:
In@docs/SPEC.md:
- Around line 1054-1057: Update the fenced code block showing the SSE wire
format example in SPEC.md to include a language specifier (use "text") so the
block starts with ```text; locate the block containing data:
{"type":"order:updated","payload":{...}}\n\n and add the language identifier to
the opening fence to ensure proper syntax highlighting and consistency with
other examples.- Around line 1024-1029: The fenced code block containing the curl example lacks
a language specifier; update the triple-backtick fence that wraps the curl
snippet (the block starting with "# Verify with curl") to include a language
identifier such as "text" (e.g., ```text) so syntax highlighting and
accessibility are improved for the curl example.In
@src/__tests__/connection.test.tsx:
- Around line 819-845: Rename the test's description to match what it actually
verifies: change the it(...) title that currently reads "should create only one
EventSource when URL changes during createConnection" to something like "should
create exactly one EventSource on initial render" (or equivalent). Update the
test title in the test case containing the SSEProvider render and createdUrls
assertions so the name reflects a single render/SSR behavior instead of a
URL-change scenario.In
@src/server/index.ts:
- Around line 2-6: Remove the redundant named import of formatSSEEvent from the
top-level import list and rely on the existing re-export (export {
formatSSEData, formatSSEEvent } from '../sseParser') so usages in this module
reference formatSSEEvent directly; specifically, delete the import clause that
brings in formatSSEEvent from '../sseParser' while keeping any other imports
(e.g., type SSEAdapter) intact and verify no other code relies on the removed
import statement.In
@src/SSEProvider.tsx:
- Around line 542-560: The onmessage handler can process messages from a stale
connection; add the same generation guard used in onopen/onerror by calling
isActiveConnection() at the top of the connection.onmessage callback and
returning early if it’s false, so only the active connection calls processEvent;
ensure you still perform the try/catch and call configRef.current.onEventError
with SSEProviderError when parsing fails, referencing connection.onmessage,
isActiveConnection, processEvent, configRef, and SSEProviderError to locate the
changes.</details> --- <details> <summary>ℹ️ Review info</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 8a1a8ade25b68b4448bb0b0d2dfbd1bb35ecc351 and fd8426e1a9f5f379f86daa598937a5739094f209. </details> <details> <summary>📒 Files selected for processing (10)</summary> * `docs/SPEC.md` * `src/SSEProvider.tsx` * `src/__tests__/connection.test.tsx` * `src/__tests__/errorHandling.test.tsx` * `src/__tests__/testing-utils-latency.test.ts` * `src/__tests__/types.test.ts` * `src/index.ts` * `src/server/index.ts` * `src/testing/index.ts` * `src/types.ts` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+15
to
22
| sendEvent: (event: SSEEventData) => Promise<void> | ||
| sendRaw: (text: string) => Promise<void> | ||
| sendSSE: (data: unknown) => Promise<void> | ||
| setLatency: (ms: number) => void | ||
| resetLatency: () => void | ||
| close: () => void | ||
| getConnection: () => MockEventSource | undefined | ||
| } |
There was a problem hiding this comment.
Breaking API change: methods now return Promise.
The sendEvent, sendRaw, and sendSSE methods now return Promise<void> instead of void. This is a breaking change for existing test code that doesn't await these calls.
Existing tests that relied on synchronous dispatch will still work (the promise resolves immediately when latencyMs <= 0), but TypeScript may flag unused promises if @typescript-eslint/no-floating-promises is enabled.
Consider adding a note in the PR description or migration guide about this change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/testing/index.ts` around lines 15 - 22, The API for the testing mock
changed: sendEvent, sendRaw, and sendSSE now return Promise<void> (previously
void), which can trigger `@typescript-eslint/no-floating-promises` in existing
tests; update the PR description or project migration guide to call out this
breaking change and recommend fixes (await the promises, use void operator, or
adjust tests to handle the returned Promise), and mention that MockEventSource
consumers may need to update any synchronous assumptions tied to latencyMs
behavior so consumers (tests using sendEvent/sendRaw/sendSSE) are aware to await
or explicitly ignore the returned Promise.
Owner
Author
There was a problem hiding this comment.
won't fix — this library is pre-1.0 (v0.1.0) so this isn't a breaking change for external consumers. Existing sync calls still work because the promise resolves immediately when latencyMs=0. The return type change is intentional to support await in tests that use setLatency().
There was a problem hiding this comment.
@queso, that makes sense! Pre-1.0 semver allows for more flexibility, and your point about backward compatibility (immediate resolution when latencyMs=0) is valid. The intentional design to support await for latency-enabled tests is clear. Thanks for the clarification!
queso
commented
Mar 1, 2026
queso
left a comment
queso
left a comment
Owner
Author
There was a problem hiding this comment.
Addressing CodeRabbit nitpicks:
- SPEC.md code block language specifiers — fixing shortly
- Redundant
formatSSEEventimport inserver/index.ts— fixing shortly onmessagemissingisActiveConnection()guard — fixing shortly, good catch for consistency- Test name mismatch in
connection.test.tsx— fixing shortly
queso
commented
Mar 1, 2026
queso
left a comment
queso
left a comment
Owner
Author
There was a problem hiding this comment.
Correction on the redundant import nitpick for src/server/index.ts: the import { formatSSEEvent } on line 2 is not redundant. It's used internally on lines 292, 327, and 350. In ES modules, export { X } from 'Y' is a re-export that does NOT make X available in the current module scope. Both the import and the re-export are necessary.
- Add null guard for reader in sendRaw/sendSSE latency tests - Add isActiveConnection() guard to onmessage and named event handlers - Add language specifiers to SPEC.md fenced code blocks - Rename misleading test to match actual behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/SSEProvider.tsx (1)
435-438:⚠️ Potential issue | 🟠 MajorNormalize transport errors before invoking
onEventError.
onEventErrorcurrently receives raw throw values, while status storesSSEProviderError. This creates inconsistent error contracts and can leak non-Errorvalues to consumers.💡 Proposed fix
} catch (error) { // Release the guard before returning on error creatingConnectionRef.current = false + const normalizedCause = + error instanceof Error ? error : new Error(String(error)) + const providerError = new SSEProviderError( + normalizedCause.message, + 'TRANSPORT', + { cause: normalizedCause }, + ) + configRef.current.onEventError?.( { type: 'transport_error', payload: null }, - error, + providerError, ) // Install a no-op closed transport to prevent re-entry on next render eventSourceRef.current = { @@ updateStatus({ connected: false, connecting: false, - error: new SSEProviderError( - error instanceof Error ? error.message : String(error), - 'TRANSPORT', - error instanceof Error ? { cause: error } : undefined, - ), + error: providerError, }) return }Also applies to: 453-457
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SSEProvider.tsx` around lines 435 - 438, The onEventError callback is being called with raw thrown values (e.g., the local variable error) which can be non-Error types; normalize these into SSEProviderError instances before invoking configRef.current.onEventError. Update both call sites that pass the raw error (the one calling configRef.current.onEventError({ type: 'transport_error', payload: null }, error) and the similar call around lines 453-457) to transform the thrown value into a consistent SSEProviderError (wrap non-Error values, preserve message/stack when present) or call a helper like toSSEProviderError(error) so consumers always receive SSEProviderError objects.
🤖 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/SSEProvider.tsx`:
- Around line 558-562: The parse error handlers inside SSEProvider should
preserve provenance for non-Error throws: in the catch blocks that construct new
SSEProviderError (currently passing { cause: error } only when error instanceof
Error), normalize non-Error thrown values into an Error-like wrapper and pass it
as the cause so the original value is retained; update both occurrences around
the parse logic (the SSEProviderError construction at the parse catch sites,
including the one shown and the one at lines ~613-617) to always provide a cause
object (e.g., wrap non-Error values into an Error or { original: value } and
pass that as the cause) so downstream logging/observability receives the
original thrown value.
---
Outside diff comments:
In `@src/SSEProvider.tsx`:
- Around line 435-438: The onEventError callback is being called with raw thrown
values (e.g., the local variable error) which can be non-Error types; normalize
these into SSEProviderError instances before invoking
configRef.current.onEventError. Update both call sites that pass the raw error
(the one calling configRef.current.onEventError({ type: 'transport_error',
payload: null }, error) and the similar call around lines 453-457) to transform
the thrown value into a consistent SSEProviderError (wrap non-Error values,
preserve message/stack when present) or call a helper like
toSSEProviderError(error) so consumers always receive SSEProviderError objects.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/SPEC.mdsrc/SSEProvider.tsxsrc/__tests__/connection.test.tsxsrc/__tests__/testing-utils-latency.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/SPEC.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tests/connection.test.tsx
- src/tests/testing-utils-latency.test.ts
ErrorOptions.cause accepts unknown, so always pass { cause: error }
instead of conditionally dropping it for non-Error values. Improves
debugging observability for thrown strings, objects, null, etc.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use the same SSEProviderError instance for both onEventError callback and updateStatus, ensuring consistent error contracts for consumers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CI already runs `bun run build` as a separate step. Testing build output inside a unit test is fragile and redundant — it was the sole cause of CI failures on main. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/SSEProvider.tsx (1)
399-423:⚠️ Potential issue | 🟠 MajorClear stale reconnect timers before opening a new connection.
Line 532 schedules reconnects, but Line 399 starts a new connection without cancelling any existing timer. A stale timeout can still fire later, call
createConnection(), and unexpectedly replace a healthy active connection.💡 Proposed fix
const createConnection = useCallback(() => { // Re-entrancy guard: bail out if a connection is already being created if (creatingConnectionRef.current) { return } creatingConnectionRef.current = true + + // Cancel any previously scheduled reconnect to avoid stale reconnect races + if (reconnectTimeoutRef.current) { + clearTimeout(reconnectTimeoutRef.current) + reconnectTimeoutRef.current = null + } // Increment generation so closures from any previous connection know they // are stale. Capture the current generation for this connection's callbacks. connectionGenerationRef.current += 1Also applies to: 532-539
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SSEProvider.tsx` around lines 399 - 423, createConnection currently starts a new connection without cancelling any previously scheduled reconnect timer, so stale timeouts can later call createConnection and replace a healthy connection; fix by checking and clearing the reconnect timer at the start of createConnection (use reconnectTimeoutRef.current and clearTimeout) and set reconnectTimeoutRef.current = null when you clear/close the old EventSource (and when scheduling new reconnects) so stale timers cannot fire; update createConnection, the block that closes oldConnection (eventSourceRef), and wherever you assign reconnectTimeoutRef to ensure the timeout is cleared/reset consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/SSEProvider.tsx`:
- Around line 399-423: createConnection currently starts a new connection
without cancelling any previously scheduled reconnect timer, so stale timeouts
can later call createConnection and replace a healthy connection; fix by
checking and clearing the reconnect timer at the start of createConnection (use
reconnectTimeoutRef.current and clearTimeout) and set
reconnectTimeoutRef.current = null when you clear/close the old EventSource (and
when scheduling new reconnects) so stale timers cannot fire; update
createConnection, the block that closes oldConnection (eventSourceRef), and
wherever you assign reconnectTimeoutRef to ensure the timeout is cleared/reset
consistently.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/SSEProvider.tsxsrc/__tests__/build-pipeline.test.ts
💤 Files with no reviewable changes (1)
- src/tests/build-pipeline.test.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
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.
Summary
Comprehensive code review identified 10 issues across error handling, type safety, race conditions, documentation, and testing utilities. All addressed in this PR:
TRANSPORT/NETWORK/PARSEcodes, replacing genericnew Error()calls. Backwards-compatible (instanceof Errorstill works). Original errors preserved viacause.parseEventcatch blocks to properly wrap strings, objects,null, andundefinedinErrorinstances instead of passing raw values through.creatingConnectionRef) and monotonic generation counter (connectionGenerationRef) to prevent overlappingcreateConnection()calls and out-of-orderonConnect/onDisconnectcallbacks.SSEConfigWithSchema.schemafromRecord<string, any>toRecord<string, unknown>. Added@ts-expect-errorregression guards forParsedEvent.type: stringenforcement.formatSSEEvent/formatSSEDatafromsrc/server/index.ts(they were never in the client bundle, but had no public server API path).maxAttempts Exhaustionsection andTroubleshootingguide to SPEC.md.setLatency(ms)/resetLatency()tomockSSEfor simulating network delays.Test plan
SSEProviderError extends Error, all existing assertions still work🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests