Skip to content

fix(platform): make failed status unconditionally terminal and remove dead code#603

Merged
Israeltheminer merged 2 commits into
mainfrom
fix/chat-loading-state-failed-status
Feb 28, 2026
Merged

fix(platform): make failed status unconditionally terminal and remove dead code#603
Israeltheminer merged 2 commits into
mainfrom
fix/chat-loading-state-failed-status

Conversation

@Israeltheminer
Copy link
Copy Markdown
Collaborator

@Israeltheminer Israeltheminer commented Feb 28, 2026

Summary

Addresses review feedback from @larryro on #599. The Convex Agent SDK maps stream abort to failed (not aborted) at the UIMessage level via statusFromStreamStatus(), and finishReason never contains cancelled (valid values: stop, length, content-filter, tool-calls, error, other, unknown).

  • Fix stuck loading on mid-tool-call failure: failed is now unconditionally terminal — when generation fails during a tool turn, isLoading correctly resolves to false instead of staying stuck forever
  • Remove dead aborted status checks: SDK never surfaces aborted as a UIMessage status (maps to failed), so these checks in use-chat-loading-state.ts and use-message-processing.ts were unreachable
  • Remove dead finishReason === 'cancelled' check: Not a valid vFinishReason value in the SDK, so this early-return in internal_actions.ts was unreachable
  • Remove dead history.toast.titleEmpty i18n key: No longer referenced anywhere after sidebar rename UX changes
  • Fix min-h mismatch in sidebar: Edit input used min-h-[1.5rem] (24px) while display span used min-h-[20px] — aligned both to 1.5rem

Test plan

  • Updated use-chat-loading-state.test.ts — replaced tool-turn-aware tests with failed unconditionally terminal tests
  • Added test: failed mid-tool-call clears isPending
  • Lint passes (0 warnings, 0 errors)
  • Format passes
  • CI checks pass

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Fixed loading state indicator to properly clear when tool calls fail, ensuring accurate feedback during chat operations.
    • Improved handling of failed operations during message processing.
  • Style

    • Refined chat title area vertical spacing for better UI consistency.

… dead code

The SDK maps stream abort to 'failed' (not 'aborted') at the UIMessage
level, which means several checks were dead code. More critically, when
generation failed mid-tool-call, isUnfinishedToolTurn kept isLoading
stuck forever because 'failed' was only terminal when the last part
wasn't a tool part.

- Make 'failed' bypass tool-turn check (fixes stuck loading on failure)
- Remove dead 'aborted' status checks (SDK never surfaces this value)
- Remove dead finishReason === 'cancelled' check (not a valid value)
- Remove dead 'history.toast.titleEmpty' i18n key
- Fix min-h mismatch between edit input and display span in sidebar
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Feb 28, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f623b68 and ad2c63a.

📒 Files selected for processing (6)
  • services/platform/app/features/chat/components/chat-history-sidebar.tsx
  • services/platform/app/features/chat/hooks/__tests__/use-chat-loading-state.test.ts
  • services/platform/app/features/chat/hooks/use-chat-loading-state.ts
  • services/platform/app/features/chat/hooks/use-message-processing.ts
  • services/platform/convex/lib/agent_chat/internal_actions.ts
  • services/platform/messages/en.json
💤 Files with no reviewable changes (2)
  • services/platform/messages/en.json
  • services/platform/convex/lib/agent_chat/internal_actions.ts

📝 Walkthrough

Walkthrough

This pull request refactors the chat loading state logic and message processing across multiple files. The core changes involve simplifying the loading determination by removing the isUnfinishedToolTurn helper and treating 'failed' status as a terminal condition, alongside removing special handling for 'aborted' status in streaming state management. Related test expectations were updated to reflect the new terminal condition semantics. Additionally, an early return on 'cancelled' finishReason was removed from the agent chat action handler. A UI adjustment changes the chat title span min-height from 20px to 1.5rem, and the localization file removes the "titleEmpty" toast message entry.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main changes: making failed status unconditionally terminal and removing dead code related to aborted/cancelled status handling across multiple files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/chat-loading-state-failed-status

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Feb 28, 2026

Greptile Summary

This PR addresses review feedback from #599 by simplifying loading state management and removing dead code paths. The key insight is that the Convex Agent SDK maps stream abort to failed status (not aborted), which means the previous tool-turn-aware loading logic was unnecessarily complex.

Key changes:

  • Made failed status unconditionally terminal in use-chat-loading-state.ts, fixing a bug where failures during tool turns would leave isLoading stuck forever
  • Removed unreachable aborted status checks (SDK never surfaces this status at the UIMessage level)
  • Removed unreachable finishReason === 'cancelled' check (not a valid vFinishReason value in the SDK)
  • Fixed UI inconsistency: aligned min-height between display span and edit input in sidebar
  • Removed unused i18n key history.toast.titleEmpty

All changes are well-tested with comprehensive test coverage that verifies the new behavior, including a new test specifically for failed mid-tool-call clearing isPending.

Confidence Score: 5/5

  • This PR is safe to merge with no risk
  • Clean refactoring that simplifies code while fixing a real bug. All changes are well-tested with comprehensive test coverage. The dead code removal is verified (no remaining references to aborted status or titleEmpty key). The logic simplification is sound and makes the code more maintainable.
  • No files require special attention

Important Files Changed

Filename Overview
services/platform/app/features/chat/hooks/use-chat-loading-state.ts Simplified loading state logic by removing tool-turn awareness and making failed status unconditionally terminal
services/platform/app/features/chat/hooks/tests/use-chat-loading-state.test.ts Updated tests to remove tool-turn awareness checks and added test for failed mid-tool-call clearing isPending
services/platform/app/features/chat/hooks/use-message-processing.ts Removed aborted status check from streaming state cleanup logic
services/platform/convex/lib/agent_chat/internal_actions.ts Removed unreachable early return for finishReason === 'cancelled'

Last reviewed commit: ad2c63a

Copy link
Copy Markdown
Collaborator

@larryro larryro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Israeltheminer Israeltheminer merged commit f9492d8 into main Feb 28, 2026
9 checks passed
@Israeltheminer Israeltheminer deleted the fix/chat-loading-state-failed-status branch February 28, 2026 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants