fix(tui): clear stale tool failures from sidebar (#1884)#2258
Conversation
Deduplicate failures per tool name, clear stale failures when the same tool succeeds, and cap visible failures at 2 so old failures don't crowd the sidebar after the task moves on.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the sidebar tool row rendering logic to deduplicate and limit visible failures, preventing stale failures from crowding the sidebar. The reviewer identified two critical issues related to the processing order (most recent first): first, older failures should be skipped if a more recent success has already been seen; second, the candidates.retain block incorrectly removes more recent failures when processing older successes and should be removed.
| if row.status == ToolStatus::Failed { | ||
| // Deduplicate failures for the same tool name: keep only the most | ||
| // recent failure per tool. Fixes #1884 — stale failures from | ||
| // tools that have since succeeded no longer crowd the sidebar. | ||
| let fail_key = row.name.trim().to_ascii_lowercase(); | ||
| if seen_failures.contains(&fail_key) { | ||
| continue; | ||
| } | ||
| seen_failures.push(fail_key); | ||
| row.summary = failure_summary_with_hint(&row.summary); | ||
| } |
There was a problem hiding this comment.
Because rows is processed in reverse chronological order (most recent first), the most recent executions are processed first. This means if a tool failed and then succeeded, the Success row is processed first, and the older Failure row is processed later.
To prevent the older Failure row from being added to candidates, we should check seen_tool_names_succeeded here and skip the failure if a more recent success has already been seen.
| if row.status == ToolStatus::Failed { | |
| // Deduplicate failures for the same tool name: keep only the most | |
| // recent failure per tool. Fixes #1884 — stale failures from | |
| // tools that have since succeeded no longer crowd the sidebar. | |
| let fail_key = row.name.trim().to_ascii_lowercase(); | |
| if seen_failures.contains(&fail_key) { | |
| continue; | |
| } | |
| seen_failures.push(fail_key); | |
| row.summary = failure_summary_with_hint(&row.summary); | |
| } | |
| if row.status == ToolStatus::Failed { | |
| // Deduplicate failures for the same tool name: keep only the most | |
| // recent failure per tool. Fixes #1884 — stale failures from | |
| // tools that have since succeeded no longer crowd the sidebar. | |
| let fail_key = row.name.trim().to_ascii_lowercase(); | |
| if seen_failures.contains(&fail_key) || seen_tool_names_succeeded.contains(&fail_key) { | |
| continue; | |
| } | |
| seen_failures.push(fail_key); | |
| row.summary = failure_summary_with_hint(&row.summary); | |
| } |
| // When a tool succeeds, remove any prior failure for the same | ||
| // tool name. Prevents stale `gh issue create (failed)` from | ||
| // lingering after a successful retry. (#1884) | ||
| if row.status == ToolStatus::Success { | ||
| let normalized = row.name.trim().to_ascii_lowercase(); | ||
| candidates.retain(|c| { | ||
| c.row.name.trim().to_ascii_lowercase() != normalized | ||
| || c.row.status != ToolStatus::Failed | ||
| }); | ||
| } |
There was a problem hiding this comment.
This candidates.retain block is incorrect and should be removed. Because rows is processed most recent first, an older success is processed after a more recent failure. When that older success is processed, this block will incorrectly remove the more recent failure from candidates.
By removing this block and instead checking seen_tool_names_succeeded in the ToolStatus::Failed block, we correctly skip older failures when a more recent success exists, while preserving more recent failures when an older success exists.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let fail_key = row.name.trim().to_ascii_lowercase(); | ||
| if seen_failures.contains(&fail_key) { | ||
| continue; | ||
| } | ||
| seen_failures.push(fail_key); |
There was a problem hiding this comment.
🔴 seen_tool_names_succeeded is populated but never read, so failure suppression doesn't work when rows arrive newest-first
The variable seen_tool_names_succeeded is populated at lines 1228-1231 but is never read anywhere in the function. It was clearly intended to be checked in the failure-handling block (around line 1168-1170) to skip failures for tools that have already succeeded.
This matters because recent_tool_rows (line 836) passes rows in newest-first order via .rev(). When a success (newer) is processed before its corresponding older failure, the candidates.retain() at line 1239 is a no-op — the failure candidate hasn't been added yet, so there's nothing to remove. The missing seen_tool_names_succeeded check was supposed to handle exactly this case: when the loop later encounters the older failure, it should see that the tool already succeeded and skip it.
Without this check, the PR's stated goal (#1884 — clearing stale failures after a successful retry) does not work for the recent_tool_rows code path, which is the primary path for showing completed tool history.
| let fail_key = row.name.trim().to_ascii_lowercase(); | |
| if seen_failures.contains(&fail_key) { | |
| continue; | |
| } | |
| seen_failures.push(fail_key); | |
| let fail_key = row.name.trim().to_ascii_lowercase(); | |
| if seen_failures.contains(&fail_key) | |
| || seen_tool_names_succeeded.contains(&fail_key) | |
| { | |
| continue; | |
| } | |
| seen_failures.push(fail_key); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| candidates.retain(|c| { | ||
| c.row.name.trim().to_ascii_lowercase() != normalized | ||
| || c.row.status != ToolStatus::Failed | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟡 visible_failure_count not decremented when candidates.retain removes failure candidates
When candidates.retain at line 1239 removes a failure candidate (because a later success for the same tool was encountered), visible_failure_count is not decremented. This inflates the count, causing subsequent legitimate failures to be incorrectly demoted to rank 3 even when fewer than MAX_VISIBLE_FAILURES rank-0 failures remain in candidates. This only affects the active_tool_rows path (oldest-first order) where retain can actually remove failures.
| candidates.retain(|c| { | |
| c.row.name.trim().to_ascii_lowercase() != normalized | |
| || c.row.status != ToolStatus::Failed | |
| }); | |
| } | |
| let normalized = row.name.trim().to_ascii_lowercase(); | |
| let before = candidates.len(); | |
| candidates.retain(|c| { | |
| c.row.name.trim().to_ascii_lowercase() != normalized | |
| || c.row.status != ToolStatus::Failed | |
| }); | |
| let removed = before - candidates.len(); | |
| visible_failure_count = visible_failure_count.saturating_sub(removed); |
Was this helpful? React with 👍 or 👎 to provide feedback.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Independent review: Scope is correct: "stale" is per-render-pass over Real concerns:
No UI race (single synchronous pass, no clear-then-re-add visible to renderer). No conflict with #2256 v0.8.48 — that PR touches 60+ files but not Net: ship-worthy fix for #1884; recommend follow-up to tighten the name-match heuristic before tools with shared names produce a regression. |
Problem
The sidebar's editorial_tool_rows function never deduplicated or capped failed tool entries, causing old failures like 'gh issue create (failed)' to linger as the top-ranked items even after the task moved on or the same tool succeeded in a later attempt.
Fix
Three targeted changes:
Verification
cargo check --workspace✅cargo test -p codewhale-tui✅ — 3,449 passed, 0 failedCloses #1884
Greptile Summary
This PR fixes stale tool-failure entries lingering in the TUI sidebar by introducing three targeted improvements to
editorial_tool_rows: per-tool failure deduplication, clearing of stale failures when a newer success is seen (correctly gated onOldestFirstorder), and a two-failure visibility cap that demotes excess entries to rank 3.ToolRowOrderenum is threaded through both callers (active_tool_rowsusesOldestFirst,recent_tool_rowsusesNewestFirst) to guard the retain-on-success logic against removing still-fresh failures in the history path.visible_failure_countis correctly decremented andseen_failuresis cleared when the retain removes candidates, reclaiming failure slots for subsequent failures.Confidence Score: 5/5
Safe to merge; the changes are scoped to sidebar display logic and carry no risk of data loss or crashes.
All three issues flagged in previous review rounds (retain direction, seen_failures cleanup, visible_failure_count decrement) are correctly addressed. The only remaining gap is cosmetic: in the OldestFirst path, repeated failures for the same tool keep the oldest error summary rather than the most recent one. This does not break any existing behaviour.
crates/tui/src/tui/sidebar.rs — specifically the seen_failures deduplication block around the OldestFirst path.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[editorial_tool_rows called] --> B{row.status == Failed?} B -- Yes --> C{NewestFirst AND tool already succeeded?} C -- Yes --> SKIP1[continue — stale failure dropped] C -- No --> D{seen_failures contains tool?} D -- Yes --> SKIP2[continue — duplicate failure dropped] D -- No --> E[push to seen_failures\napply failure_summary_with_hint] E --> F{visible_failure_count >= MAX 2?} F -- Yes --> G[rank = 3\ndemoted failure] F -- No --> H[visible_failure_count++\nrank = 0] B -- No --> I{row.status == Success AND OldestFirst?} I -- Yes --> J[candidates.retain: remove same-tool failures\ndecrement visible_failure_count\nclear seen_failures entry] I -- No --> K[rank = tool_row_rank] G --> PUSH[candidates.push] H --> PUSH J --> K K --> PUSH PUSH --> SORT[sort by rank, order\ntake limit] SORT --> OUT[final SidebarToolRow list]Reviews (2): Last reviewed commit: "fix(tui): make sidebar failure clearing ..." | Re-trigger Greptile