fix(platform): improve chat streaming stability and UX#599
Conversation
Handle 'aborted' stream status as terminal in loading state and message processing to prevent stuck loading indicators. Move abortController initialization earlier to ensure availability in error paths. Add early return for cancelled generation in agent action. Improve chat input with proper send button, attach tooltip, and accessibility. Fix sidebar rename to fall back to "Untitled" on empty input and improve click target area.
Greptile SummaryThis PR addresses chat streaming stability issues and improves UX with better accessibility and error handling. Key Changes:
All changes are well-structured, maintain consistency across the codebase, and follow existing patterns. Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| services/platform/app/features/chat/components/chat-history-sidebar.tsx | Removed empty title validation with toast, now falls back to "Untitled"; improved click target area with absolute positioning and proper z-index layering |
| services/platform/app/features/chat/components/chat-input.tsx | Added send Button component with ArrowUp icon and aria-label; wrapped attach button in Tooltip; improved accessibility |
| services/platform/app/features/chat/hooks/use-chat-loading-state.ts | Added aborted as terminal status to prevent stuck loading indicators after stream cancellation |
| services/platform/app/features/chat/hooks/use-message-processing.ts | Added aborted status handling to remove streaming keys and prevent stuck streaming state |
| services/platform/convex/lib/agent_chat/internal_actions.ts | Added early return for cancelled generation (finishReason === 'cancelled') to skip validation |
| services/platform/convex/lib/agent_response/generate_response.ts | Moved abortController initialization before try block to ensure availability across all error paths |
| services/platform/messages/en.json | Added "send": "Send message" translation key for chat input send button aria-label |
Last reviewed commit: f7b953d
📝 WalkthroughWalkthroughThis pull request introduces multiple enhancements to the chat feature spanning UI interaction, component structure, and abort/cancel handling. Changes include: refactoring the chat history sidebar's rename interaction to use two-phase click detection (single-click navigate, double-click rename) with updated DOM structure; updating the chat input component with new UI primitives (Tooltip, Button) and adding a send button with ArrowUp icon; treating 'aborted' as a terminal status in message processing and loading-state hooks; adding early return handling for cancelled generation results; consolidating AbortController usage in response generation; and adding a new translation label for send messaging. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
![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)
services/platform/app/features/chat/components/chat-history-sidebar.tsx (1)
58-58:⚠️ Potential issue | 🟡 MinorClear pending click timeout on unmount to avoid delayed stale navigation.
clickTimeoutRefis set in Line 237 but never cleaned up on unmount. The delayed callback can still fire after teardown.💡 Proposed fix
useEffect(() => { const onKeyDown = (e: KeyboardEvent) => { const isMod = isMac ? e.metaKey : e.ctrlKey; @@ window.addEventListener('keydown', onKeyDown); return () => window.removeEventListener('keydown', onKeyDown); }, [isMac, onSearchOpen, onNewChat]); + + useEffect(() => { + return () => { + if (clickTimeoutRef.current) { + clearTimeout(clickTimeoutRef.current); + clickTimeoutRef.current = null; + } + }; + }, []);Also applies to: 231-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform/app/features/chat/components/chat-history-sidebar.tsx` at line 58, The clickTimeoutRef used in chat-history-sidebar.tsx (set via setTimeout in the click handler that schedules navigation) is never cleared on unmount; add a cleanup to the component to clear and nullify clickTimeoutRef to prevent delayed stale navigation: in the component body create or update a useEffect with a return cleanup function that checks clickTimeoutRef.current, calls clearTimeout on it if present, and then sets clickTimeoutRef.current to null so the delayed callback cannot run after unmount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/platform/app/features/chat/hooks/use-chat-loading-state.ts`:
- Around line 61-67: The current return logic in use-chat-loading-state.ts can
keep loading true when a tool-turn was cancelled mid-run because it still
requires !isUnfinishedToolTurn(lastMessage) even for the 'aborted' status;
update the condition around lastMessage.status and isUnfinishedToolTurn so that
'aborted' is treated as a terminal state regardless of isUnfinishedToolTurn,
i.e., consider status === 'aborted' as sufficient to stop loading, while keeping
the existing requirement that status === 'success' || status === 'failed' also
require !isUnfinishedToolTurn(lastMessage); adjust the boolean expression that
references lastMessage, status, and isUnfinishedToolTurn accordingly.
---
Outside diff comments:
In `@services/platform/app/features/chat/components/chat-history-sidebar.tsx`:
- Line 58: The clickTimeoutRef used in chat-history-sidebar.tsx (set via
setTimeout in the click handler that schedules navigation) is never cleared on
unmount; add a cleanup to the component to clear and nullify clickTimeoutRef to
prevent delayed stale navigation: in the component body create or update a
useEffect with a return cleanup function that checks clickTimeoutRef.current,
calls clearTimeout on it if present, and then sets clickTimeoutRef.current to
null so the delayed callback cannot run after unmount.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
services/platform/app/features/chat/components/chat-history-sidebar.tsxservices/platform/app/features/chat/components/chat-input.tsxservices/platform/app/features/chat/hooks/use-chat-loading-state.tsservices/platform/app/features/chat/hooks/use-message-processing.tsservices/platform/convex/lib/agent_chat/internal_actions.tsservices/platform/convex/lib/agent_response/generate_response.tsservices/platform/messages/en.json
…rn state When cancellation happens mid-tool-call, the last message part is a tool part, causing isUnfinishedToolTurn to return true. Previously this kept isLoading stuck at true even though the stream was aborted. Now aborted status short-circuits to not-loading, bypassing the tool turn check.
Post-merge review: streaming stability changesGood intent — fixing stuck loading spinners on abort is important. The UI improvements (send button, tooltip, sidebar hit target) are solid. However, the core abort-handling logic is built on incorrect assumptions about the Convex Agent SDK, meaning the key streaming fixes are dead code and the original bug remains. Core issue:
|
| File | Line | Code | Why it's dead |
|---|---|---|---|
use-chat-loading-state.ts |
64 | if (status === 'aborted') return false |
UIMessage status is never 'aborted', always 'failed' |
use-message-processing.ts |
207 | m.status === 'aborted' |
Same — 'failed' on line 206 already covers it |
internal_actions.ts |
277 | if (result.finishReason === 'cancelled') |
'cancelled' is not a valid FinishReason in AI SDK or Convex Agent ("stop" | "length" | "content-filter" | "tool-calls" | "error" | "other" | "unknown"). When abortController.abort() fires, the AI SDK throws an AbortError — it doesn't return a result. |
The const status: string | undefined type widening on line 61 masks this — it bypasses TypeScript's type checking to allow comparing against a value that isn't in the union. The test's status: 'aborted' as UIMessage['status'] type cast is also a signal: the type system is correctly saying this value doesn't exist.
The real bug (unfixed)
The actual bug is: when status === 'failed' AND isUnfinishedToolTurn() returns true, the loading state gets stuck forever.
The old logic:
return !(
lastMessage.role === 'assistant' &&
!isUnfinishedToolTurn(lastMessage) && // false when tool turn incomplete
(lastMessage.status === 'success' || lastMessage.status === 'failed')
);
// → !(true && false && true) → !(false) → true → loading foreverA failed/aborted generation mid-tool-call has unfinished tool parts, so isUnfinishedToolTurn returns true, and the whole condition never resolves. But a failed message will never complete its tool turn — no more steps will come.
Recommended fix
Make 'failed' unconditionally terminal, bypassing the tool-turn check:
if (lastMessage.role !== 'assistant') return true;
if (lastMessage.status === 'failed') return false; // always terminal, even mid-tool-call
return !(!isUnfinishedToolTurn(lastMessage) && lastMessage.status === 'success');Then remove the dead 'aborted' checks in use-message-processing.ts and the 'cancelled' check in internal_actions.ts. Update tests to cover status: 'failed' + unfinished tool parts (the actual bug scenario) instead of the fictional 'aborted' status.
What's correct in this PR
generate_response.ts: MovingabortControlleroutsidetryis a real, necessary fix — ensures it's accessible in error/cleanup pathschat-input.tsx: Send button with proper disable logic and a11y is goodchat-history-sidebar.tsx: Full-surface click target withabsolute inset-0is a nice UX improvementmessages/en.json: Translation addition is correct
Minor UI notes
- Sidebar input
min-h-[1.5rem](24px) vs display spanmin-h-[20px](20px) — slight visual jump when entering/exiting edit mode - Rename empty fallback to "Untitled" is fine UX-wise, but
history.toast.titleEmptymay now be dead i18n
2 participants
Summary
abortedstream status as terminal inuseChatLoadingStateanduseMessageProcessingto prevent stuck loading indicators after stream cancellationabortControllerinitialization earlier ingenerate_response.tsto ensure availability across all error pathsfinishReason === 'cancelled') in agent action to skip validationButtoncomponent,Tooltipfor attach, andaria-labelaccessibilityaria-labelTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements