fix(webui): clarify no-progress run failures - #7422
Conversation
italic-jinxin
added
size: M
|
🚅 Deployed to the ironclaw-pr-7422 environment in ironclaw-ci-preview
|
|
@ironloopai review |
🧭 IronLoop Run · ReviewThis comment updates in place as the Run moves through its stages. 🟩 Final result · Completed
Manual command by italic-jinxin · attempt 1 of 3 · completed in 14m 44s IronLoop completed the review and posted it to GitHub. 🔗 Result |
|
@claude review |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe WebUI removes unfinished assistant streaming phases after run failures, ignores late projection frames, and displays localized actionable copy for ChangesRun failure UI
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RunStatus
participant useChatEvents
participant FailureFormatter
participant ChatHistory
RunStatus->>useChatEvents: failed or recovery_required status
useChatEvents->>ChatHistory: remove active streaming draft
useChatEvents->>FailureFormatter: format no_progress_detected failure
FailureFormatter-->>useChatEvents: localized actionable message
useChatEvents->>ChatHistory: append failure message
RunStatus->>useChatEvents: late projection frame
useChatEvents->>ChatHistory: ignore projection text
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
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. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@crates/product/ironclaw_webui/frontend/src/pages/chat/lib/useChatEvents.test.ts`:
- Around line 2683-2763: Add a parallel regression test for useChatEvents
handling status "recovery_required", reusing the existing
failureMessageForRunStatus harness and the same initial assistant draft,
terminal status update, message finalization, failure error content, and
delayed-text replay assertions as the no-progress failure test. Ensure the
recovery_required case also leaves only the finalized assistant message and
error message, and late projection updates cannot restore the unfinished draft.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 27081050-0383-437b-a055-c762014cb8a1
📒 Files selected for processing (17)
crates/product/ironclaw_webui/CONTRACT.mdcrates/product/ironclaw_webui/frontend/src/i18n/ar.tscrates/product/ironclaw_webui/frontend/src/i18n/de.tscrates/product/ironclaw_webui/frontend/src/i18n/en.tscrates/product/ironclaw_webui/frontend/src/i18n/es.tscrates/product/ironclaw_webui/frontend/src/i18n/fr.tscrates/product/ironclaw_webui/frontend/src/i18n/hi.tscrates/product/ironclaw_webui/frontend/src/i18n/ja.tscrates/product/ironclaw_webui/frontend/src/i18n/ko.tscrates/product/ironclaw_webui/frontend/src/i18n/pt-BR.tscrates/product/ironclaw_webui/frontend/src/i18n/uk.tscrates/product/ironclaw_webui/frontend/src/i18n/zh-CN.tscrates/product/ironclaw_webui/frontend/src/lib/i18n.test.tscrates/product/ironclaw_webui/frontend/src/pages/chat/lib/failureMessages.test.tscrates/product/ironclaw_webui/frontend/src/pages/chat/lib/failureMessages.tscrates/product/ironclaw_webui/frontend/src/pages/chat/lib/useChatEvents.test.tscrates/product/ironclaw_webui/frontend/src/pages/chat/lib/useChatEvents.ts
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 IronLoop review
🟢 No actionable findings
No actionable issues found.
Validation
- ✅ WebUI frontend tests — 137 files / 1,197 tests passed.
- ✅ WebUI handler-contract tests — 5 tests passed.
- ✅ Patch whitespace check — No whitespace errors reported.
Review details
- Run:
4a9f2cfc-1b04-4163-8c10-7efc511b47e4 - Workflow: Review
- Attempts: 1
🧭 IronLoop Run · ReviewThis comment updates in place as the Run moves through its stages. ⬛ Final result · Stopped
Automatic trigger · attempt 1 of 3 · stopped after 4m 26s IronLoop stopped because the pull request target branch or head changed while this Run was active. |
hanakannzashi
left a comment
hanakannzashi
left a comment
There was a problem hiding this comment.
P1: withoutStreamingAssistantPhaseForRun treats isStreaming !== false as streaming. That also matches durable timeline assistant records: messagesFromTimeline leaves isStreaming undefined, while non-final assistant rows (including tool_result) have isFinalReply === false. A failed run will therefore remove completed durable rows until the async history reload succeeds (and permanently if it fails), contrary to the stated goal of preserving earlier completed phases. Please restrict this to actual live phases (isStreaming === true, or the existing isLiveAssistantMessage predicate) and add a timeline-sourced regression case.
02840a1 to
ff6a208
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/product/ironclaw_webui/frontend/src/pages/chat/lib/useChatEvents.test.ts (1)
2695-2781: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the same-batch terminal-text path.
This helper sends the terminal
run_statusin a later projection. The later replay is then blocked by the existingerr-run-1; it does not exercise the newbatchRunStatusByRunIdbranch incrates/product/ironclaw_webui/frontend/src/pages/chat/lib/useChatEvents.tsLine [631].Add a case with
run_status.status === terminalStatusand multiple text items in one snapshot. Assert that the completed phase remains and only the active phase is removed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/product/ironclaw_webui/frontend/src/pages/chat/lib/useChatEvents.test.ts` around lines 2695 - 2781, Extend assertNoProgressFailureClearsDraft with a same-projection case containing the terminal run_status and multiple text items, so it exercises batchRunStatusByRunId in useChatEvents. Assert that the completed assistant phase remains while only the active unfinished phase is removed, and retain the existing assertions for the later replay path.crates/product/ironclaw_webui/frontend/src/pages/chat/lib/useChatEvents.ts (1)
631-647: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftTerminal-batch handling and regression coverage share one gap. The implementation treats every text item in a terminal batch as late, so it can drop completed phases. The current regression does not exercise that batch shape.
crates/product/ironclaw_webui/frontend/src/pages/chat/lib/useChatEvents.ts#L631-L647: Process terminal-batch text before failure cleanup, or distinguish an already-observed failure from a terminal status first seen in the current batch.crates/product/ironclaw_webui/frontend/src/pages/chat/lib/useChatEvents.test.ts#L2695-L2781: Add one snapshot containing the terminal status and multiple text phases; assert that the completed phase remains and the active phase is removed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/product/ironclaw_webui/frontend/src/pages/chat/lib/useChatEvents.ts` around lines 631 - 647, The terminal-batch handling in useChatEvents.ts (lines 631-647) must process text phases before failure cleanup, or distinguish failures already observed before the current batch from terminal statuses first seen in it, so completed phases are retained while only the active failed phase is removed. In useChatEvents.test.ts (lines 2695-2781), add snapshot coverage with a terminal status and multiple text phases, asserting the completed phase remains and the active phase is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@crates/product/ironclaw_webui/frontend/src/pages/chat/lib/useChatEvents.test.ts`:
- Around line 2695-2781: Extend assertNoProgressFailureClearsDraft with a
same-projection case containing the terminal run_status and multiple text items,
so it exercises batchRunStatusByRunId in useChatEvents. Assert that the
completed assistant phase remains while only the active unfinished phase is
removed, and retain the existing assertions for the later replay path.
In `@crates/product/ironclaw_webui/frontend/src/pages/chat/lib/useChatEvents.ts`:
- Around line 631-647: The terminal-batch handling in useChatEvents.ts (lines
631-647) must process text phases before failure cleanup, or distinguish
failures already observed before the current batch from terminal statuses first
seen in it, so completed phases are retained while only the active failed phase
is removed. In useChatEvents.test.ts (lines 2695-2781), add snapshot coverage
with a terminal status and multiple text phases, asserting the completed phase
remains and the active phase is removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 05893a26-eaa8-4714-aaef-c6470120ba74
📒 Files selected for processing (2)
crates/product/ironclaw_webui/frontend/src/pages/chat/lib/useChatEvents.test.tscrates/product/ironclaw_webui/frontend/src/pages/chat/lib/useChatEvents.ts
Thanks for catching this — you’re right.
I updated the cleanup condition to require Fixed in |
* fix(webui): clarify no-progress run failures * test(webui): cover recovery-required run failures * fix(webui): preserve durable phases on run failure
* fix(webui): clarify no-progress run failures * test(webui): cover recovery-required run failures * fix(webui): preserve durable phases on run failure
* fix(webui): clarify no-progress run failures * test(webui): cover recovery-required run failures * fix(webui): preserve durable phases on run failure
This branch was successfully deployed
4 participants
Summary
no_progress_detectedfailures when the backend frame has no failure summary.Linked Issue
Closes #7351
Validation
TZ=UTC corepack pnpm testcorepack pnpm lint:conventionscorepack pnpm typecheckcargo test -p ironclaw_webui --all-featurescargo clippy -p ironclaw_webui --all-features --all-targets -- -D warningscargo fmt --all -- --checkcargo test -p ironclaw_integration_tests --test reborn_integration_terminal_warninggit diff --checkSecurity Impact
No security boundary or sensitive-data handling changes.
Database Impact
No schema, migration, or persistence changes.
Blast Radius
Limited to WebChat rendering for failed or recovery-required runs and localized no-progress failure copy.
E2E Assessment
No new E2E scenario is needed. The changed behavior is local to the SPA event reducer and is covered through its wire-shaped caller seam; the backend no-progress path already has integration coverage.
Rollback Plan
Revert commit
d5ab64ba2to restore the previous failed-run rendering behavior.Risk: low