fix(chat): implement stop generation button functionality#1157
fix(chat): implement stop generation button functionality#1157fbricon merged 1 commit intokortex-hub:mainfrom
Conversation
fbricon
requested review from
benoitf and
bmahabirbu
and removed request for
a team
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughMain process now tracks active inference streams with AbortControllers and exposes an IPC to stop them; preload returns stream callback IDs synchronously and exposes a stop API; renderer starts streams without awaiting and forwards aborts to preload; tests add a stop-button helper and a smoke test for cancelling generation. (36 words) Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Renderer as Renderer Process
participant Preload as Preload/IPC Bridge
participant Main as Main Process
participant API as Inference API
User->>Renderer: Start text generation
Renderer->>Preload: inferenceStreamText(params, callbacks)
Preload->>Main: ipcInvoke('inference:streamText', ...)
Main->>Main: create AbortController, store in activeStreams(onDataId)
Main->>API: streamText(..., abortSignal)
API-->>Main: stream chunks
Main-->>Preload: inference:streamText-onData (chunks)
Preload-->>Renderer: onChunk callback
User->>Renderer: Click Stop
Renderer->>Preload: inferenceStopStream(onDataId)
Preload->>Main: ipcInvoke('inference:stopStream', onDataId)
Main->>Main: abort controller (activeStreams)
Main->>API: abort stream
API-->>Main: stream ended
Main-->>Preload: inference:streamText-onEnd
Preload-->>Renderer: onEnd callback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
bmahabirbu
left a comment
bmahabirbu
left a comment
There was a problem hiding this comment.
LGTM one quesiton I had and itll be all set
| this.activeStreams.delete(params.onDataId); | ||
| this.webContents.send('inference:streamText-onEnd', params.onDataId); |
There was a problem hiding this comment.
activeStreams.delete() in stopStream and the one in the finally block can both run so its doing a double delete? should be fine but just a question
There was a problem hiding this comment.
So I asked Claude, why not just rely on the code in the finally block?
The Real Issue: Event Loop Timing
The timing window is about async execution order in the event loop:
private stopStream(onDataId: number): void {
const stream = this.activeStreams.get(onDataId);
if (stream) {
stream.controller.abort();
// No delete here - relies on finally
}
}What actually happens:
- stopStream() called (synchronous)
- controller.abort() triggers abort signal (synchronous)
- stopStream() returns (synchronous)
- Event loop continues - other code can execute
- Eventually, the stream's reader.read() Promise rejects/resolves
- finally block runs
Between steps 3-6, the map still has the entry even though we've "stopped" it.
Realistic Problematic Scenario
// Renderer calls: stop stream
await window.inferenceStopStream(123);
// ^ This returns immediately after stopStream() executes
// Immediately after (same tick or next tick):
await window.deleteChat(chatId);
→ stopStreamsByChat(chatId) runs
→ iterates activeStreams
→ finds entry 123 (STILL THERE - finally hasn't run yet!)
→ aborts the already-aborted controller
→ deletes entry
// Later (when microtask queue processes):
// The finally block from the original stream runs
→ tries to delete (already deleted)
→ sends onEnd event
The issue is async ordering. The finally block runs "eventually" when the Promise settles, but synchronous code keeps executing.
Why Immediate Delete Matters
With delete in stopStream():
private stopStream(onDataId: number): void {
const stream = this.activeStreams.get(onDataId);
if (stream) {
stream.controller.abort();
this.activeStreams.delete(onDataId); // Synchronous cleanup
}
}Now:
window.inferenceStopStream(123);
// Map entry deleted IMMEDIATELY (synchronous)
window.deleteChat(chatId);
→ stopStreamsByChat(chatId)
→ iterates activeStreams
→ entry 123 is GONE
→ nothing to process
The Real Benefits
- Synchronous state consistency: The map is correct immediately after stopStream() returns, not "eventually"
- Prevents redundant operations: stopStreamsByChat() won't re-abort already-stopped streams
- Predictable state: Caller can trust the map state without waiting for Promises to settle
- finally as safety net: Catches cases we didn't explicitly stop (natural completion, errors)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
packages/main/src/chat/chat-manager.ts (1)
261-343:⚠️ Potential issue | 🔴 CriticalMake cancellation observable before the async setup starts.
activeStreams.set()is not reached until Line 307, after multiple awaits. If the renderer callsinference:stopStreamor deletes the chat whilestreamText()is still in setup, both stop paths miss the entry and the generation can still start; for a brand-new chat, the remaining setup can even recreate a chat the user just deleted. Thefinallyat Line 341 is also too narrow, because anything that throws beforegetReader()skips the cleanup and never emitsonEnd. Create/store the controller before the firstawait, and re-checksignal.abortedbefore mutating chat state or starting the stream.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/main/src/chat/chat-manager.ts` around lines 261 - 343, streamText creates and registers the AbortController too late and its final cleanup is too narrow, so cancellations during the async setup or errors before getReader() are missed; fix by creating the AbortController at the top of streamText (before any awaits), immediately storing it in activeStreams with params.onDataId, and then after each awaited step that may mutate chat state (e.g. before calling this.chatQueries.saveChat, before calling this.generateTitleInBackground, and before starting the stream from streamText(...) / model inference) re-check controller.signal.aborted and bail out if set; also broaden the try/finally so the activeStreams.delete(params.onDataId) and this.webContents.send('inference:streamText-onEnd', ...) run for any early throw or abort to guarantee cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/preload/src/index.ts`:
- Around line 1398-1415: The catch in the inferenceStreamText handler currently
only calls callback.onError and leaves the entry in onDataCallbacksStreamText,
preventing normal termination; update the catch block for the
ipcInvoke('inference:streamText') call so that after calling callback.onError it
also removes the callback from onDataCallbacksStreamText (using
onDataCallbacksStreamText.delete(id)) and invokes callback.onEnd() to finalize
the stream; ensure you reference onDataCallbacksStreamTextId and the returned id
variable as currently used so the correct map entry is cleaned up.
---
Outside diff comments:
In `@packages/main/src/chat/chat-manager.ts`:
- Around line 261-343: streamText creates and registers the AbortController too
late and its final cleanup is too narrow, so cancellations during the async
setup or errors before getReader() are missed; fix by creating the
AbortController at the top of streamText (before any awaits), immediately
storing it in activeStreams with params.onDataId, and then after each awaited
step that may mutate chat state (e.g. before calling this.chatQueries.saveChat,
before calling this.generateTitleInBackground, and before starting the stream
from streamText(...) / model inference) re-check controller.signal.aborted and
bail out if set; also broaden the try/finally so the
activeStreams.delete(params.onDataId) and
this.webContents.send('inference:streamText-onEnd', ...) run for any early throw
or abort to guarantee cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 96a0065e-fabe-4c51-82c1-f572e6e6dc9c
📒 Files selected for processing (5)
packages/main/src/chat/chat-manager.tspackages/preload/src/index.tspackages/renderer/src/lib/chat/components/ipc-chat-transport.tstests/playwright/src/model/pages/chat-page.tstests/playwright/src/specs/provider-specs/chat-smoke.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/playwright/src/model/pages/chat-page.ts
- tests/playwright/src/specs/provider-specs/chat-smoke.spec.ts
- packages/renderer/src/lib/chat/components/ipc-chat-transport.ts
fbricon
force-pushed
the
GH-535
branch
4 times, most recently
from
031c02b to
d6588e0
Compare
- Wire abort signal through IPC layer to cancel AI response streams - Make inferenceStreamText return onDataId synchronously for immediate abort listener registration - Create AbortController early (before awaits) to handle cancellations during async setup - Add abort checks after each async operation (saveChat, getInferenceComponents, saveMessages) - Store AbortController per stream with chatId tracking - Abort active streams when chat is deleted - Add E2E test [CHAT-17] for stop generation Fixes kortex-hub#535 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Fred Bricon <fbricon@gmail.com>
MarsKubeX
left a comment
MarsKubeX
left a comment
There was a problem hiding this comment.
LGTM. Tested in MacOS. Thanks for the fix.
3 participants
stop-message.mp4
Fixes #535
Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com
Signed-off-by: Fred Bricon fbricon@gmail.com