feat(abort): persist partial streamed response to history on Stop Generation#824
feat(abort): persist partial streamed response to history on Stop Generation#824netrezver wants to merge 2 commits into
Conversation
When the user clicks Stop Generation, accumulate the partial text streamed so far and write a synthetic assistant entry to the SDK's JSONL history file before interrupting. On the next turn Claude sees what it had already generated and can continue without losing context. Changes: - server/claude-sdk.js: track cwd per session; add encodeProjectDir and appendInterruptedAssistantEntry helpers; pass partialResponse to abort - server/modules/websocket/services/chat-websocket.service.ts: thread partialResponse from the abort-session WS message through to the SDK - src/components/chat/hooks/useChatComposerState.ts: capture streamed text via accumulatedStreamRef; send it with abort-session; optimistically clear loading state on abort
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR preserves partial assistant output on abort: backend stores per-session writer/cwd, adds helpers to append an interrupted assistant entry into the Claude project JSONL, and the frontend/WebSocket pass captured partial text into the abort call. ChangesAbort with Partial Response Persistence
Sequence DiagramsequenceDiagram
participant UI as Frontend UI
participant WS as WebSocket Handler
participant SDK as Claude SDK
participant JSONL as Project JSONL Storage
UI->>UI: User clicks abort, ref contains partial
UI->>WS: sendMessage({type: 'abort-session', partialResponse: '...'})
WS->>SDK: abortClaudeSDKSession(sessionId, partialResponse)
alt partialResponse is non-empty
SDK->>JSONL: appendInterruptedAssistantEntry(sessionId, cwd, partial)
JSONL->>JSONL: read last entry, extract context
JSONL->>JSONL: append synthetic assistant entry with partial + marker
end
SDK->>SDK: session.instance.interrupt()
SDK-->>WS: success
WS-->>UI: abort complete
Possibly related PRs
Suggested reviewers
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 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 (2)
server/claude-sdk.js (1)
782-816: ⚖️ Poor tradeoffRace condition: file may be modified between read and append.
Between reading the JSONL file (line 794) and appending the new entry (line 857), the SDK's subprocess may have written additional entries. This could result in:
- The
parentUuidpointing to an outdated entry- The idempotency check passing even though the SDK already wrote something
This is a time-of-check to time-of-use (TOCTOU) issue. However, since
interrupt()is called after this function completes, and the SDK subprocess should be blocked waiting for the main process, the practical risk is low.Consider using file locking or atomic read-modify-write if this becomes an issue in practice. For now, adding a comment documenting the assumption would help future maintainers.
🤖 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 `@server/claude-sdk.js` around lines 782 - 816, The appendInterruptedAssistantEntry function reads the session JSONL (jsonlPath) then later appends a new entry using the previously read parentUuid, which creates a TOCTOU race if the SDK subprocess can write between read and append; add a clear inline comment inside appendInterruptedAssistantEntry (near the read/parse block and before the append logic) documenting the assumption that the SDK subprocess is blocked and that reads are safe, and also note mitigation options (file locking or atomic read-modify-write using tempfile+rename) with references to jsonlPath, parentUuid, and the idempotency check so future maintainers can apply a proper lock if needed.src/components/chat/hooks/useChatComposerState.ts (1)
960-976: ⚖️ Poor tradeoffOptimistic UI clearing may leave stale state if abort fails.
The UI state is cleared immediately (lines 961-963) before the abort message is sent. If the WebSocket send fails or the server-side abort fails, the UI will show a "not loading" state while the backend session may still be active.
However, the
completemessage from the server (sent at line 176-185 in chat-websocket.service.ts) will arrive regardless of abort success/failure and could be used to reconcile state. The current approach prioritizes responsiveness.This is acceptable for now, but consider adding error handling or state reconciliation if users report UI inconsistencies after failed aborts.
🤖 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/components/chat/hooks/useChatComposerState.ts` around lines 960 - 976, The optimistic UI clears (setIsLoading, setCanAbortSession, setClaudeStatus) before sending the abort message; modify the abort handler in the useChatComposerState hook to attempt sendMessage(...) inside a try/catch (or check sendMessage's returned promise/result) and, on send failure, restore the previous UI state (revert setIsLoading/setCanAbortSession/setClaudeStatus to their prior values) and/or set an explicit error flag; additionally, ensure the global websocket "complete" server event handler (the server-side complete message) is used to reconcile final state by updating these same setters when that message arrives so UI and backend remain consistent.
🤖 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 `@server/claude-sdk.js`:
- Around line 782-816: The appendInterruptedAssistantEntry function reads the
session JSONL (jsonlPath) then later appends a new entry using the previously
read parentUuid, which creates a TOCTOU race if the SDK subprocess can write
between read and append; add a clear inline comment inside
appendInterruptedAssistantEntry (near the read/parse block and before the append
logic) documenting the assumption that the SDK subprocess is blocked and that
reads are safe, and also note mitigation options (file locking or atomic
read-modify-write using tempfile+rename) with references to jsonlPath,
parentUuid, and the idempotency check so future maintainers can apply a proper
lock if needed.
In `@src/components/chat/hooks/useChatComposerState.ts`:
- Around line 960-976: The optimistic UI clears (setIsLoading,
setCanAbortSession, setClaudeStatus) before sending the abort message; modify
the abort handler in the useChatComposerState hook to attempt sendMessage(...)
inside a try/catch (or check sendMessage's returned promise/result) and, on send
failure, restore the previous UI state (revert
setIsLoading/setCanAbortSession/setClaudeStatus to their prior values) and/or
set an explicit error flag; additionally, ensure the global websocket "complete"
server event handler (the server-side complete message) is used to reconcile
final state by updating these same setters when that message arrives so UI and
backend remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f70119ac-0d57-40b1-be97-3fa6bd660b4f
📒 Files selected for processing (3)
server/claude-sdk.jsserver/modules/websocket/services/chat-websocket.service.tssrc/components/chat/hooks/useChatComposerState.ts
netrezver
left a comment
netrezver
left a comment
There was a problem hiding this comment.
Thanks for the review!
TOCTOU race (server/claude-sdk.js appendInterruptedAssistantEntry):
Good catch on the theoretical race. In practice the invariant holds: interrupt() is called only after this function returns, so the SDK subprocess is not writing during our read→append window. Added an inline comment documenting that assumption and noting the mitigation path (atomic tempfile+rename or file lock) for future maintainers. See the follow-up commit.
Optimistic UI clearing (useChatComposerState.ts):
Intentional — the user clicked Stop, so clearing the spinner immediately feels right regardless of whether the server confirms. If the abort fails the server's complete event will arrive anyway and the session state will reconcile naturally. Adding try/catch with state rollback for a fire-and-forget abort message would add complexity without user-visible benefit.
2 participants
Problem
When the user clicks Stop Generation mid-response, Claude's entire partial reply is silently discarded. On the next message Claude has no memory of what it had already generated and starts from scratch, causing context loss and repetition.
Root Cause
abortClaudeSDKSessioncallssession.instance.interrupt()without recording anything. The streamed text was visible on screen but never written to the SDK's JSONL history file at~/.claude/projects/<encoded-cwd>/<sessionId>.jsonl.Fix
cwdper session —addSessionnow stores the working directory so the abort path can locate the correct history file.encodeProjectDir(cwd)— mirrors the SDK's own directory naming logic (/→-).appendInterruptedAssistantEntry(sessionId, cwd, partialText)— writes a syntheticassistantentry with the partial text and[generation interrupted]suffix to the JSONL file before callinginterrupt(). Idempotent: if the last entry already contains[generation interrupted]it skips the write.useChatComposerStatereads accumulated streamed text viaaccumulatedStreamRefand sends it in theabort-sessionWebSocket message. Also optimistically clearsisLoading/claudeStatuson abort so the UI doesn't stay stuck in a processing state.partialResponsefrom the message through toabortClaudeSDKSession.How to Reproduce
[generation interrupted]) and can continue or acknowledge it naturally.Files Changed
server/claude-sdk.jscwdin sessions; addencodeProjectDir,appendInterruptedAssistantEntry; updateabortClaudeSDKSessionserver/modules/websocket/services/chat-websocket.service.tspartialResponsefrom WS message to SDK abortsrc/components/chat/hooks/useChatComposerState.tsSummary by CodeRabbit
New Features
Improvements