Add stop button to interrupt agent response#41
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to stop message generation in the chat interface. It updates the ChatArea and ChatInput components to support an onStop callback and dynamically toggles the input button between 'send' and 'stop' icons based on the streaming state. A review comment points out that disabling the textarea during streaming prevents the 'Enter' key from triggering the stop action; using readOnly instead of disabled is suggested to ensure keyboard events are still captured.
- Implement 'stop' button in `ChatInput` that appears during streaming - Disable textarea while streaming to prevent concurrent messages - Map 'Enter' key to stop generation during streaming - Connect `stopGeneration` from `useChat` hook to the UI Co-authored-by: MrOrz <108608+MrOrz@users.noreply.github.com>
fae9893 to
d778052
Compare
17f424d to
d778052
Compare
…tion Replace runChat server function with a /api/run-sse proxy route so that AbortController.abort() propagates directly through fetch's reader.read() instead of being swallowed by serverFnFetcher's internal IIFE error handler. Server-side request.signal is forwarded to ADK so the upstream connection also terminates on client abort. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Enter key should only submit, not trigger onStop during streaming. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@claude review |
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, comment @claude review on this pull request to trigger a review.
|
Claude finished @MrOrz's task in 3m 23s —— View job Code Review Complete ✓
Overall AssessmentStatus: Approved with minor suggestions ✅ This is a well-implemented feature that successfully adds stop button functionality to interrupt agent responses. The core architectural decision to bypass TanStack's Key Strengths
Issues & Suggestions1. Missing Optional Chaining SafetyFile: onClick={isStreaming ? onStop : handleSubmit}While onClick={isStreaming ? onStop ?? (() => {}) : handleSubmit}Or better yet, guard the button state: onClick={isStreaming && onStop ? onStop : !isStreaming ? handleSubmit : undefined}2. Accessibility - Missing ARIA LabelsFile: The button only has a visual icon with no accessible label. Screen reader users won't know what the button does or when its function changes. Suggestion: <button
onClick={isStreaming ? onStop : handleSubmit}
disabled={(!isStreaming && !value.trim()) || disabled}
? '停止生成' : '發送訊息'}
className="absolute right-2 bottom-2 p-1.5 bg-primary text-black rounded-lg hover:bg-primary-hover transition-colors flex items-center justify-center disabled:opacity-40 disabled:cursor-not-allowed"
>
<span className="material-symbols-outlined text-sm" aria-hidden="true">
{isStreaming ? 'stop' : 'send'}
</span>
</button>3. Error Message Could Be More SpecificFile: throw new Error(`ADK request failed: HTTP ${response.status}`)Consider including throw new Error(`ADK request failed: HTTP ${response.status} ${response.statusText}`)4. Potential Edge Case - Reader Already ConsumedFile: const reader = response.body!.getReader()The non-null assertion assumes if (!response.body) {
throw new Error('ADK response missing body')
}
const reader = response.body.getReader()Code Quality NotesPositive Patterns
Fixed IssuesThe Enter key handler concern from the initial review has been properly addressed in commit Security & PerformanceSecurity: ✅ No issues found
Performance: ✅ Improved
Testing RecommendationsConsider testing these edge cases:
SummaryThis PR successfully implements the stop button feature with a well-reasoned architectural approach. The abort signal propagation issue with TanStack's RPC layer is correctly identified and solved. The suggestions above are minor improvements for robustness and accessibility, but the core implementation is solid. Recommendation: Approve and merge (address accessibility issue if possible) |
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Added a stop button to the chat interface to allow users to interrupt agent responses.
Changes:
isStreamingandonStopprops. The send button now switches to a stop button whenisStreamingis true. The textarea is disabled during streaming.onStopprop and passes it along withisStreamingtoChatInput.stopGenerationfrom theuseChathook and passes it toChatArea.runChatserver function (createServerFn) with a plain/api/run-sseproxy route. TanStack Start'sserverFnFetcherswallowsAbortErrorinside its internal IIFE, so callingAbortController.abort()on the client never unblocked thefor-awaitloop. A plain API route lets the client usefetch()directly, wherereader.read()throwsAbortErrorimmediately.request.signalis also forwarded server-side so the upstream ADK connection terminates on abort.This fulfills the requirement from the 20260428 meeting notes.
Fixes #36
stop-session.mp4
(Speed 1.5x)
PR created automatically by Jules for task 1373008896763086594 started by @MrOrz