fix: useStream can now handle multiple streams per component lifetime - #20
Conversation
Fixes #19 Root causes addressed: 1. streamStarted ref was set but never cleared, blocking all future streams - Replaced with activeStreamRef that tracks the current streamId - Acts as both a Strict Mode guard AND allows detecting new streams 2. streamBody and streamEnded were never reset between streams - Now reset both states when a new streamId is detected 3. No AbortController caused stale stream reads to leak - Added AbortController to cleanup in-flight requests when: - Component unmounts - streamId changes to a new value - Errors from aborted fetches are now silently ignored 4. TextDecoder was allocated per chunk (minor perf issue) - Now allocated once per stream and reused The fix enables the real-time streaming UX for multiple consecutive messages in the same component session, rather than falling back to database persistence after the first stream completes. Co-authored-by: Ian Macartney <ianmacartney@users.noreply.github.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactored the Changes
Sequence DiagramsequenceDiagram
participant Component as Component
participant Effect as useEffect Hook
participant Controller as AbortController
participant Fetch as Fetch API
participant Stream as ReadableStream Reader
participant State as State Management
Component->>Effect: streamId changes
Effect->>Effect: Check activeStreamRef vs streamId
alt New streamId detected
Effect->>State: Reset streamBody & streamEnded
Effect->>Controller: Create new AbortController
Effect->>Fetch: POST request with signal
Fetch-->>Stream: response.body stream
loop Read chunks
Stream->>Stream: await reader.read()
alt Chunk received
Stream->>State: Append text to streamBody
else No more chunks
Stream->>State: Set streamEnded = true
Stream->>Stream: Break loop
end
end
else Matching streamId
Effect->>Effect: Skip (Strict Mode guard)
end
Component->>Effect: Unmount/New effect
Effect->>Controller: controller.abort()
Controller-->>Fetch: Cancel in-flight request
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
✨ 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 |
commit: |
|
Thanks, I confirm the fix is working |
Summary
Fixes #19
The
useStreamhook was unable to drive more than one stream per component lifetime. After the first stream completed, subsequent calls with a newstreamIdsilently skipped the HTTP POST and fell back to the database query, losing the real-time streaming UX.Root Causes Addressed
1.
streamStartedref was set but never clearedThe original code used a
streamStartedref intended as a React Strict Mode guard, but it permanently blocked all future streams after the first one completed.Fix: Replaced with
activeStreamRefthat tracks the currentstreamId. This serves as both:2.
streamBodyandstreamEndedwere never reset between streamsWhen
streamIdchanged, the state from the previous stream would persist, potentially causing text concatenation issues.Fix: Both states are now reset when a new
streamIdis detected.3. No
AbortControllercaused stale stream reads to leakIf the component re-rendered with a new
streamIdwhile a previous fetch was mid-read, the oldreader.read()loop would continue callingsetStreamBody, mixing text across streams.Fix: Added
AbortControllerto cleanup in-flight requests when:streamIdchanges to a new valueErrors from aborted fetches are silently ignored to avoid false error states.
4. Minor performance improvement
TextDecoderwas being allocated per chunk inside the while loop.Fix: Now allocated once per stream and reused.
Testing
npm run typecheck)npm run lint)npm test)Slack Thread
Summary by CodeRabbit