Chat abort #1381
Chat abort #1381
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds stop/cancel support to chat generation via AbortController, exposes handleStop from useChatLogic and wires it to ChatInput as onStop, updates submit button behavior/icons for generating state, adjusts two locale placeholders and moves PO anchors, and adds speaker-annotation lines to two template Jinja assets. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ChatInput
participant ChatView
participant useChatLogic
participant Stream as streamText
participant AbortCtl as AbortController
User->>ChatInput: Click Submit
ChatInput->>ChatView: onSubmit / forward props
ChatView->>useChatLogic: handleSubmit()
useChatLogic->>AbortCtl: create controller
useChatLogic->>Stream: streamText({ signal })
note right of useChatLogic: isGenerating = true
alt User clicks Stop
User->>ChatInput: Click Stop
ChatInput->>ChatView: onStop()
ChatView->>useChatLogic: handleStop()
useChatLogic->>AbortCtl: abort()
Stream--x useChatLogic: aborted/error
useChatLogic->>useChatLogic: clear controller, reset flags
else Stream completes
Stream-->>useChatLogic: done
useChatLogic->>useChatLogic: clear controller, reset flags
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
| const queryClient = useQueryClient(); | ||
|
|
||
| // Reset generation state and abort ongoing streams when session changes | ||
| useEffect(() => { |
There was a problem hiding this comment.
useEffect lacks a cleanup function, so ongoing streams aren’t aborted on component unmount which can leak resources and trigger setState on an unmounted component
Prompt for AI agents
Address the following comment on apps/desktop/src/components/right-panel/hooks/useChatLogic.ts at line 65:
<comment>useEffect lacks a cleanup function, so ongoing streams aren’t aborted on component unmount which can leak resources and trigger setState on an unmounted component</comment>
<file context>
@@ -56,10 +56,25 @@ export function useChatLogic({
const [isGenerating, setIsGenerating] = useState(false);
const [isStreamingText, setIsStreamingText] = useState(false);
const isGeneratingRef = useRef(false);
+ const abortControllerRef = useRef<AbortController | null>(null);
const sessions = useSessions((state) => state.sessions);
const { getLicense } = useLicense();
const queryClient = useQueryClient();
+ // Reset generation state and abort ongoing streams when session changes
</file context>
| size="icon" | ||
| onClick={handleSubmit} | ||
| disabled={!inputValue.trim() || isGenerating} | ||
| onClick={isGenerating ? onStop : handleSubmit} |
There was a problem hiding this comment.
Ensure onClick never receives undefined so the Stop button always performs an action
Prompt for AI agents
Address the following comment on apps/desktop/src/components/right-panel/components/chat/chat-input.tsx at line 388:
<comment>Ensure onClick never receives undefined so the Stop button always performs an action</comment>
<file context>
@@ -383,10 +385,18 @@ export function ChatInput(
<Button
size="icon"
- onClick={handleSubmit}
- disabled={!inputValue.trim() || isGenerating}
+ onClick={isGenerating ? onStop : handleSubmit}
+ disabled={isGenerating ? false : (!inputValue.trim())}
>
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/desktop/src/components/right-panel/components/chat/chat-input.tsx (1)
14-25:onKeyDownprop is unused; remove it here and at call sites to satisfy “no unused” guideline.
onKeyDownis defined and passed from the parent but never attached to any element. This violates the repository guideline “No unused imports, variables, or functions.”Apply these diffs in this file:
interface ChatInputProps { inputValue: string; onChange: (e: React.ChangeEvent<HTMLTextAreaElement>) => void; onSubmit: (mentionedContent?: Array<{ id: string; type: string; label: string }>) => void; - onKeyDown: (e: React.KeyboardEvent<HTMLTextAreaElement>) => void; autoFocus?: boolean; entityId?: string; entityType?: BadgeType; onNoteBadgeClick?: () => void; isGenerating?: boolean; onStop?: () => void; } @@ { inputValue, onChange, onSubmit, - onKeyDown, autoFocus = false, entityId, entityType = "note", onNoteBadgeClick, isGenerating = false, onStop, }: ChatInputProps, ) { @@ - }, [editorRef.current?.editor, onKeyDown, handleSubmit, inputValue]); + }, [editorRef.current?.editor, handleSubmit, inputValue]);And in the parent (see review comments on chat-view.tsx) remove the prop at the call site and from
useChatLogicdestructuring/return.Also applies to: 27-40, 271-271
apps/desktop/src/components/right-panel/hooks/useChatLogic.ts (1)
312-315: MCP clients are only closed on successful finish — leak on error/abort.On cancellation or error,
onFinishwon’t fire, leaving MCP SSE clients open.Add a
finallythat always closes clients (and keeponFinishfor the happy path):try { const provider = await modelProvider(); @@ - const { fullStream } = streamText({ + const { fullStream } = streamText({ model, @@ onFinish: () => { for (const client of allMcpClients) { client.close(); } }, abortSignal: abortController.signal, }); @@ - } catch (error) { + } catch (error) { console.error(error); @@ - await dbCommands.upsertChatMessage({ + await dbCommands.upsertChatMessage({ id: aiMessageId, group_id: groupId, created_at: new Date().toISOString(), role: "Assistant", - content: finalErrorMesage, + content: finalErrorMessage, type: "text-delta", }); - } + } finally { + // Ensure cleanup even on abort/errors + for (const client of allMcpClients) { + try { client.close(); } catch {} + } + }Note: This patch also references the variable rename in the next comment.
apps/desktop/src/components/right-panel/views/chat-view.tsx (1)
60-68: Remove unused keydown plumbing between ChatView → ChatInput.
handleKeyDownfrom the hook is only passed through toChatInput, which does not use it. Drop it here and inuseChatLogic’s return to satisfy the “no unused” guideline.const { isGenerating, isStreamingText, handleSubmit, handleQuickAction, handleApplyMarkdown, - handleKeyDown, handleStop, } = useChatLogic({ @@ <ChatInput inputValue={inputValue} onChange={handleInputChange} onSubmit={(mentionedContent) => handleSubmit(mentionedContent)} - onKeyDown={handleKeyDown} autoFocus={true} entityId={activeEntity?.id} entityType={activeEntity?.type} onNoteBadgeClick={handleNoteBadgeClick} isGenerating={isGenerating} onStop={handleStop} />Additionally, remove
handleKeyDownfromuseChatLogic’s return (see hook review).Also applies to: 180-191
🧹 Nitpick comments (7)
crates/template/assets/enhance.system.jinja (1)
58-58: Align speaker assumption wording and avoid gendered phrasingCurrent copy both over-qualifies (“most likely”) and uses gendered “him/herself.” Recommend clearer, inclusive wording with a neutrality fallback to minimize misattribution risks when diarization is wrong.
Apply this minimal diff:
-- Meeting Transcript (txt): [Speaker 0] is most likely the user him/herself. +- Meeting Transcript (txt): [Speaker 0] usually corresponds to the user; if context contradicts this, do not assume—attribute neutrally.Optional: If you expect the “user speaker index” to vary, parametrize it for consistency across templates:
-- Meeting Transcript (txt): [Speaker 0] usually corresponds to the user; if context contradicts this, do not assume—attribute neutrally. +- Meeting Transcript (txt): [Speaker {{ config.transcript.user_speaker_index | default(0) }}] usually corresponds to the user; if context contradicts this, do not assume—attribute neutrally.apps/desktop/src/components/right-panel/components/chat/chat-input.tsx (2)
208-233: Avoid re-binding TipTap key handlers on every keystroke.The effect depends on
inputValue, so handlers are detached/attached on each change. You already compute emptiness from the editor state; you can dropinputValuefrom the logic and dependencies.Apply this diff to rely solely on the editor document state and stabilize the effect:
editor.setOptions({ editorProps: { ...editor.options.editorProps, handleKeyDown: (view, event) => { if (event.key === "Enter" && !event.shiftKey) { - const isEmpty = view.state.doc.textContent.trim() === ""; - if (isEmpty) { - return true; - } - if (inputValue.trim()) { - event.preventDefault(); - handleSubmit(); - return true; - } + const isEmpty = view.state.doc.textContent.trim() === ""; + if (!isEmpty) { + event.preventDefault(); + handleSubmit(); + return true; + } + return true; } return false; }, }, }); - }, [editorRef.current?.editor, inputValue, handleSubmit]); + }, [editorRef.current?.editor, handleSubmit]);
234-271: Second DOM keydown handler also re-binds on every input change; decouple frominputValue.This handler prevents formatting shortcuts and handles Enter. You can read the current editor text directly instead of closing over
inputValue, and remove it from the dependency array to avoid constant re-binding.Apply this diff:
useEffect(() => { const editor = editorRef.current?.editor; if (editor) { const handleKeyDown = (event: KeyboardEvent) => { if (event.metaKey || event.ctrlKey) { if (["b", "i", "u", "k"].includes(event.key.toLowerCase())) { event.preventDefault(); return; } } if (event.key === "Enter" && !event.shiftKey) { event.preventDefault(); - if (inputValue.trim()) { - handleSubmit(); - } + const hasContent = editor.state.doc.textContent.trim().length > 0; + if (hasContent) handleSubmit(); } }; @@ - } - }, [editorRef.current?.editor, onKeyDown, handleSubmit, inputValue]); + } + }, [editorRef.current?.editor, handleSubmit]);apps/desktop/src/components/right-panel/hooks/useChatLogic.ts (3)
185-186: Typo:enabledSevers→enabledServers.Minor naming fix for clarity.
- const enabledSevers = mcpServers.filter((server) => server.enabled); - for (const server of enabledSevers) { + const enabledServers = mcpServers.filter((server) => server.enabled); + for (const server of enabledServers) {
209-215: Shadowingtoolimport with loop variable — rename local for readability.The loop variable
toolshadows the importedtool()helper, which is confusing.- for (const [toolName, tool] of Object.entries(tools as Record<string, any>)) { - newMcpTools[toolName] = dynamicTool({ - description: tool.description, - inputSchema: tool.inputSchema || z.any(), - execute: tool.execute, - }); + for (const [toolName, srvTool] of Object.entries(tools as Record<string, any>)) { + newMcpTools[toolName] = dynamicTool({ + description: srvTool.description, + inputSchema: srvTool.inputSchema || z.any(), + execute: srvTool.execute, + }); }
551-556: MakehandleStopflip UI flags immediately for snappier UX.Currently the UI updates only when the stream throws and the catch/finally runs. You can set flags right away on user action to reduce perceived latency.
const handleStop = useCallback(() => { if (abortControllerRef.current) { abortControllerRef.current.abort(); abortControllerRef.current = null; + setIsGenerating(false); + setIsStreamingText(false); + isGeneratingRef.current = false; } }, []);apps/desktop/src/components/right-panel/views/chat-view.tsx (1)
92-101: Stopping before starting a new chat avoids orphaned streams.Starting a new chat while generating could leave the previous stream running briefly. Stop first if needed.
const handleNewChat = async () => { if (!sessionId || !userId) { return; } + if (isGenerating) { + handleStop(); + } + setCurrentChatGroupId(null); setMessages([]); setHasChatStarted(false); setInputValue(""); };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
apps/desktop/src/components/right-panel/components/chat/chat-input.tsx(4 hunks)apps/desktop/src/components/right-panel/hooks/useChatLogic.ts(6 hunks)apps/desktop/src/components/right-panel/views/chat-view.tsx(2 hunks)apps/desktop/src/locales/en/messages.po(19 hunks)apps/desktop/src/locales/ko/messages.po(19 hunks)crates/template/assets/enhance.system.jinja(1 hunks)crates/template/assets/enhance.user.jinja(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
apps/desktop/src/components/right-panel/hooks/useChatLogic.tsapps/desktop/src/components/right-panel/components/chat/chat-input.tsxapps/desktop/src/components/right-panel/views/chat-view.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: zizmor
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (2)
apps/desktop/src/components/right-panel/hooks/useChatLogic.ts (1)
64-76: Good: abort-and-reset on session change.Aborting any ongoing generation and resetting streaming flags when
sessionIdchanges is the right call to avoid cross-session bleed.apps/desktop/src/locales/en/messages.po (1)
267-271: LGTM: placeholder annotations updated to match new UI logic.The added placeholder hints for
{0}reflect the new state-driven labels (“Back/Save and close”, “Go/Resume”). No string changes; only context expanded.
| <Button | ||
| size="icon" | ||
| onClick={handleSubmit} | ||
| disabled={!inputValue.trim() || isGenerating} | ||
| onClick={isGenerating ? onStop : handleSubmit} | ||
| disabled={isGenerating ? false : (!inputValue.trim())} | ||
| > | ||
| <ArrowUpIcon className="h-4 w-4" /> | ||
| {isGenerating | ||
| ? ( | ||
| <Square | ||
| className="h-4 w-4" | ||
| fill="currentColor" | ||
| strokeWidth={0} | ||
| /> | ||
| ) | ||
| : <ArrowUpIcon className="h-4 w-4" />} | ||
| </Button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stop button can be enabled with no handler (click does nothing) — guard and add an a11y label.
If isGenerating is true and onStop is undefined, the button is enabled but has no onClick, resulting in a dead control. Also, the button lacks an accessible label.
Apply this diff to make behavior explicit and improve accessibility:
<Button
size="icon"
- onClick={isGenerating ? onStop : handleSubmit}
- disabled={isGenerating ? false : (!inputValue.trim())}
+ onClick={() => (isGenerating ? onStop?.() : handleSubmit())}
+ disabled={isGenerating ? !onStop : !inputValue.trim()}
+ aria-label={isGenerating ? "Stop generating" : "Send message"}
>
{isGenerating
? (
<Square
className="h-4 w-4"
fill="currentColor"
strokeWidth={0}
/>
)
: <ArrowUpIcon className="h-4 w-4" />}
</Button>🤖 Prompt for AI Agents
In apps/desktop/src/components/right-panel/components/chat/chat-input.tsx around
lines 386 to 400, the send/stop Button becomes enabled when isGenerating is true
but onStop can be undefined, creating a dead control and lacks an accessible
label; fix by only wiring onClick to onStop when onStop is defined (e.g.,
onClick={onStop ?? handleSubmit} or conditionally set onClick) and disable the
button when isGenerating && !onStop, and add an aria-label (and optional title)
that reflects the action ("Stop generation" when isGenerating, otherwise "Send
message").
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src/components/right-panel/hooks/useChatLogic.ts (1)
64-76: Add unmount cleanup to avoid leaking a live stream and stale setState.Effect resets on session changes, but there’s no cleanup on component unmount. Abort the in-flight stream in a cleanup function too. Also, prefer a “Why” comment per guidelines.
Apply this diff:
- // Reset generation state and abort ongoing streams when session changes + // Why: prevent leaking a live stream and stale updates when the session changes or the component unmounts useEffect(() => { // Abort any ongoing generation when session changes if (abortControllerRef.current) { abortControllerRef.current.abort(); abortControllerRef.current = null; } // Reset generation state for new session setIsGenerating(false); setIsStreamingText(false); isGeneratingRef.current = false; + return () => { + if (abortControllerRef.current) { + abortControllerRef.current.abort(); + abortControllerRef.current = null; + } + }; }, [sessionId]);
🧹 Nitpick comments (1)
apps/desktop/src/components/right-panel/hooks/useChatLogic.ts (1)
551-557: Optional: make stop feel instantaneous in the UI.Currently, the flags flip only when the stream throws/settles. Optionally set them immediately in handleStop for snappier UX; the catch will keep them in sync.
- const handleStop = useCallback(() => { - if (abortControllerRef.current) { - abortControllerRef.current.abort(); - abortControllerRef.current = null; - } - }, []); + const handleStop = useCallback(() => { + // Optimistically update UI + setIsGenerating(false); + setIsStreamingText(false); + isGeneratingRef.current = false; + if (abortControllerRef.current) { + abortControllerRef.current.abort(); + abortControllerRef.current = null; + } + }, []);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/desktop/src/components/right-panel/hooks/useChatLogic.ts(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
apps/desktop/src/components/right-panel/hooks/useChatLogic.ts
🧬 Code graph analysis (1)
apps/desktop/src/components/right-panel/hooks/useChatLogic.ts (3)
packages/utils/src/contexts/sessions.tsx (1)
useSessions(33-47)apps/desktop/src/hooks/use-license.ts (1)
useLicense(9-105)apps/desktop/src/components/right-panel/components/chat/types.ts (1)
Message(7-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (4)
apps/desktop/src/components/right-panel/hooks/useChatLogic.ts (4)
59-60: Good call introducing an AbortController ref.This is the right primitive for cooperative cancellation of the streaming generator.
110-121: License gate threshold reads correctly.Check is against the current count; with “4 messages allowed,” the 5th send is blocked (count 4). Dialog copy matches the policy.
If the policy shifts, adjust the numeric check and dialog copy together to avoid off-by-one UX confusion.
284-286: Abort is correctly wired through to the stream.Creating a fresh controller per stream and passing its signal into streamText is correct. No concerns.
Also applies to: 316-316
479-479: Controller is cleared on both success and error.This prevents stale aborted signals from affecting subsequent requests. LGTM.
Also applies to: 508-509
Summary by cubic
Adds chat abort so users can stop a streaming response mid-generation. The send button now becomes a stop button while generating, and generation state is safely reset on session changes.
New Features
Improvements