Add stop control to cancel active chat generation#1305
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds the ability to cancel active chat generation by introducing a stop button in the chat composer that appears while AI output is streaming. The implementation properly wires the cancel action through the application layers, from UI to the underlying SSE request, ensuring clean state management when generation is stopped.
Changes:
- Added AbortController-based stream cancellation to the SSE API client
- Extended chat state management to properly clean up UI state when stopping generation, including ending reasoning-in-progress
- Implemented stop button UI in chat input that replaces the submit button during streaming
- Added Node 25 compatibility fix for test storage APIs
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/vitest.setup.ts | Adds storage shim for localStorage/sessionStorage to fix Node 25 compatibility in tests |
| frontend/src/api/state/apiAppClient.ts | Implements AbortController to enable cancellable SSE streams via Observable teardown |
| frontend/src/pages/chat/state/zustand/chatStore.ts | Updates cancelActiveStream to unsubscribe, clear streaming state, and end reasoning-in-progress |
| frontend/src/pages/chat/state/chat.ts | Exposes stopGeneration function that wraps cancelActiveStream |
| frontend/src/pages/chat/conversation/ConversationPage.tsx | Passes stopGeneration and isStreaming props to ChatInput |
| frontend/src/pages/chat/conversation/ChatInput.tsx | Adds stop button UI with IconX that appears during streaming, replaces submit functionality |
| frontend/src/pages/chat/conversation/ChatInput.ui-unit.spec.tsx | Adds unit test verifying stop generation is called when submit button clicked during streaming |
Comments suppressed due to low confidence (1)
frontend/src/pages/chat/conversation/ChatInput.tsx:124
- The event.preventDefault() call has been moved to the start of the function, but this changes behavior. Previously, preventDefault was only called when the form was actually submitted (after validation). Now it's called unconditionally, even when validation fails. This means the default form submission behavior is prevented even when the submission is blocked by validation checks. While this likely doesn't cause issues in practice since the early return prevents further execution, it represents an unnecessary change in logic flow. Consider moving event.preventDefault() back after the validation check, or add a comment explaining why it needs to be at the start.
const doSubmit = useEventCallback((event: React.FormEvent) => {
event.preventDefault();
if (isDisabled || isStreaming || !input || input.length === 0 || upload.status === 'pending') {
return;
}
doSetText(input, chatFiles);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
related to #684 |
a32d859 to
4c3e76b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
frontend/src/pages/chat/conversation/ChatInput.tsx:121
- The doSubmit function checks
upload.status === 'pending'to prevent submission during file uploads, but the button's disabled state on line 318 checksuploadMutations.some((m) => m.status === 'pending'). These should use the same logic for consistency. Consider usinguploadMutations.some((m) => m.status === 'pending')in both places, or alternatively checkingupload.status === 'pending'in both places.
if (isDisabled || isStreaming || !input || input.length === 0 || upload.status === 'pending') {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1f594d5 to
121afdb
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const doSubmit = useEventCallback((event: React.FormEvent) => { | ||
| if (isDisabled || !input || input.length === 0 || upload.status === 'pending') { | ||
| event.preventDefault(); | ||
| if (isDisabled || isStreaming || !input || input.length === 0 || isUploadPending) { | ||
| return; |
There was a problem hiding this comment.
When isStreaming is true, doSubmit returns early, so pressing Enter in the textarea cannot trigger stopGeneration (only clicking the icon button can). This makes the stop control inaccessible to keyboard-only users; consider handling Enter (or Escape) to call stopGeneration when streaming (and it exists), or otherwise provide an equivalent keyboard interaction.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
95cc104 to
4dde196
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| onerror(err) { | ||
| try { | ||
| subscriber.error(err); | ||
| } finally { | ||
| if (abortController.signal.aborted) { | ||
| subscriber.complete(); | ||
| return; | ||
| } | ||
|
|
||
| subscriber.error(err); | ||
| throw err; | ||
| }, | ||
| onclose() { | ||
| subscriber.complete(); | ||
| }, | ||
| }).catch((err) => { | ||
| if (abortController.signal.aborted) { | ||
| subscriber.complete(); | ||
| return; | ||
| } | ||
| subscriber.error(err); | ||
| subscriber.complete(); | ||
| }); |
There was a problem hiding this comment.
streamPrompt: onerror calls subscriber.error(err) and then throws, which will reject the fetchEventSource(...) promise and flow into the .catch(...) handler. That makes the error-handling path harder to reason about (and can result in a second subscriber.error(...) call unless guarded). Consider emitting the error in only one place (either throw and let .catch call subscriber.error, or don’t call subscriber.error in .catch if onerror already did, or guard with a flag / subscriber.closed).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| aria-label={isStreaming ? texts.accessibility.stopGenerating : texts.common.send} | ||
| title={isStreaming ? texts.chat.stopGenerating : texts.chat.sendMessage} |
| onerror(err) { | ||
| try { | ||
| subscriber.error(err); | ||
| } finally { | ||
| if (abortController.signal.aborted) { | ||
| subscriber.complete(); | ||
| return; | ||
| } | ||
|
|
||
| subscriber.error(err); | ||
| throw err; | ||
| }, | ||
| onclose() { | ||
| subscriber.complete(); | ||
| }, | ||
| }).catch((err) => { | ||
| if (abortController.signal.aborted) { | ||
| subscriber.complete(); | ||
| return; | ||
| } | ||
| subscriber.error(err); | ||
| subscriber.complete(); | ||
| }); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| next: (event) => sendEvent('message', event), | ||
| error: (err: Error) => sendEvent('error', err), | ||
| complete: () => response.end(), | ||
| error: (err: Error) => { | ||
| sendEvent('error', err); | ||
| cleanup(false); | ||
| }, |
| @ApiNoContentResponse() | ||
| cancelMessage(@Req() req: Request, @Param('id', ParseIntPipe) id: number) { | ||
| this.activeChatStreams.cancel(id, req.user.id); | ||
| } |
|
|
||
| const sendMessage = (input: string, files?: FileDto[], editMessageId?: number) => { | ||
| // Only cancel existing stream for this specific chat if we're starting a new message | ||
| void api.stream.cancelPrompt(chatId); |
| await expect(element).toBeVisible(); | ||
| await element.click(); | ||
| await page.waitForLoadState('networkidle'); | ||
| await page.getByRole('option', { name: new RegExp(configuration.name) }).click(); |
…ex/stop-generation-button
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| }, [chatId]); | ||
|
|
||
| const listOfChatsStore = useListOfChatsStore(); | ||
| const stopGeneration = () => { |
| const sendMessage = (input: string, files?: FileDto[], editMessageId?: number) => { | ||
| // Only cancel existing stream for this specific chat if we're starting a new message | ||
| void api.stream.cancelPrompt(chatId); | ||
| chatStore.cancelActiveStream(chatId); | ||
|
|
| @ApiNoContentResponse() | ||
| cancelMessage(@Req() req: Request, @Param('id', ParseIntPipe) id: number) { | ||
| this.activeChatStreams.cancel(id, req.user.id); | ||
| } |
Summary
Validation