Repository navigation
tui: tool cells carry their own state - #5799
Merged
Merged
Conversation
A tool card's border said nothing about the tool. `wrap_card_rail`
inserted its `╭ │ ╰` with `Span::raw`, so a running card, a finished one
and a failed one were bordered identically and only the header glyph
carried the state. The parallel-read fan-out strip was worse: every dot
was painted flat `WHALE_INFO`, so `✕` for a failed read was the same ink
as `●` for a finished one.
Adopts the rule from OMP's `output-block.ts` (design/SHELL-DESIGN-20260901
§2.1): a block takes a state and its border colour follows it. Nothing
elsewhere has to narrate what the block already shows.
`Theme::tool_status_color` was already the single status→colour function,
so this retunes it onto the WHALE tokens and makes the two unstyled
surfaces consume it, rather than adding a mapping beside the one that
exists:
Running WHALE_ACTION #6AAEF2 (was ACCENT_TOOL_LIVE seafoam)
Success TEXT_MUTED #93A0B8 (was STATUS_SUCCESS green)
Hydrated TEXT_DIM #8A99B3 (was the running accent — a stalled
"retry required" read as in-flight)
Warning WHALE_HUMAN #F6C453 (was STATUS_WARNING coral)
Failed WHALE_ERROR #FF86B2
Light and solarized get the same roles in their own tokens (running takes
the action accent, success recedes into muted text). Grayscale already
had the shape and is untouched — four greys cannot separate five states,
which the cross-palette test states rather than pretends away.
`palette/adapt.rs` already maps every new dark token to its light
counterpart, so the light adaptation needed no change.
Also collapses the status derivation onto the cell, which is what "carries
its own state" has to mean literally: `ToolCell::status()`,
`frame.rs::tool_cell_is_running` and
`activity_detail.rs::tool_status_for_activity` were three copies of one
fold, and the exploring card's own fourth copy could not produce `Failed`
at all — a fan-out with a failed read painted itself `done` in the success
colour. One `ExploringCell::status()` now feeds all four call sites.
No goldens changed: the golden buffers are text-only and none of them
render a tool cell.
Evidence, run in tmp/wt-toolcells with the isolated build dir:
RUST_MIN_STACK=16777216 scripts/dev-test.sh tui tui::
Summary [27.320s] 3325 tests run: 3325 passed (1 leaky), 8506 skipped
RUST_MIN_STACK=16777216 scripts/dev-test.sh tui (whole lib)
Summary [157.134s] 11820 tests run: 11819 passed, 1 failed, 13 skipped
The one failure is `task_manager::tests::forced_idle_timeout_releases_
the_worker_for_later_tasks`, a timing test this change does not touch;
re-run alone it passes:
Summary [0.439s] 1 test run: 1 passed, 11832 skipped
`card_rail_carries_the_cell_status` fails without the fix — reverting
just the two `Span::styled` rail lines gives:
panicked at crates/tui/src/tui/history/tests.rs:2375:13:
Running left the card rail unstyled — the border must follow the state
cargo fmt --all -- --check: clean.
cargo clippy -p codewhale-tui --lib --all-targets
-A clippy::too_many_arguments -A clippy::uninlined_format_args
-A clippy::unnecessary_map_or: exit 0, zero warnings.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AdbuqwHAXSDcikPiS6L6Qw
The previous commit ran one mapping for the whole cell, which made a
settled card dim its glyph along with its border — and the accepted
mockup (tideline-mockups/tideline-01) does not do that. It draws a
finished `test` row with a green ✓ and a finished `read` row with the
blue magnifier that says "this was a read". Collapsing both to muted
grey threw away the identity while chasing the border rule.
So the rail and the glyph are now two mappings that agree wherever it
matters and part only where the card has settled:
RAIL — OMP's `output-block.ts` border rule (`Theme::tool_rail_color`),
consumed by `wrap_card_rail`:
Running tool_running_accent WHALE_ACTION #6AAEF2
Success tool_value_color TEXT_MUTED #93A0B8
Hydrated tool_label_color TEXT_DIM #8A99B3
Warning tool_warning_accent WHALE_HUMAN #F6C453
Failed tool_failed_accent WHALE_ERROR #FF86B2
GLYPH + state word — the mockup's reading (`Theme::tool_glyph_color`),
consumed by the card header and the exploring fan-out dots:
Running tool_running_accent WHALE_ACTION #6AAEF2
Success + Verify tool_success_accent WHALE_WORKING_GREEN #9BD66F
Success + other tool_running_accent WHALE_ACTION #6AAEF2
Hydrated tool_label_color TEXT_DIM #8A99B3
Warning tool_warning_accent WHALE_HUMAN #F6C453
Failed tool_failed_accent WHALE_ERROR #FF86B2
`WHALE_WORKING_GREEN` reaches the theme as `palette::STATUS_SUCCESS`,
which is built from `WHALE_WORKING_GREEN_RGB` — no new alias.
Running, warning and failure are the same ink in both tables by
construction, so the border and the glyph can never disagree about
trouble; only `Success` splits, which is exactly the case the mockup
cares about. `Hydrated` stays dim in both: it has not succeeded at
anything, so it never borrows the verified or family accent.
That means `tool_success_accent` goes back to the green it always held
in all four palettes (this reverts that half of the previous commit) and
the rail takes the `tool_value_color` field that already existed, rather
than a new field. Light and solarized follow the same roles in their own
tokens; grayscale is untouched.
Tests now assert the two tables separately:
deepseek_theme::tests::tool_rail_color_maps_each_status
deepseek_theme::tests::tool_glyph_color_maps_each_status_and_family
deepseek_theme::tests::rail_and_glyph_split_only_where_the_card_has_settled
tui::history::tests::card_rail_carries_the_cell_status (updated)
tui::history::tests::a_settled_verify_glyph_does_not_read_as_a_settled_read
Evidence:
RUST_MIN_STACK=16777216 scripts/dev-test.sh tui tui::history
Summary [0.367s] 77 tests run: 77 passed, 11759 skipped
RUST_MIN_STACK=16777216 scripts/dev-test.sh tui deepseek_theme
Summary [0.027s] 8 tests run: 8 passed, 11828 skipped
RUST_MIN_STACK=16777216 scripts/dev-test.sh tui tui::
Summary [40.938s] 3328 tests run: 3328 passed, 8508 skipped
cargo fmt --all -- --check: clean.
cargo clippy -p codewhale-tui --lib --all-targets
-A clippy::too_many_arguments -A clippy::uninlined_format_args
-A clippy::unnecessary_map_or: exit 0, zero warnings.
No goldens changed.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AdbuqwHAXSDcikPiS6L6Qw
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_4022e7cc-164a-4570-a398-02445061fcad) |
Contributor
|
Claude finished @Hmbown's task in 3m 14s —— View job Review: tui: tool cells carry their own state (#5799)Todo list
SummaryThis is a clean, well-scoped refactor. I read the full diff ( What checks out:
Minor, non-blocking observation:
Not independently verified: I could not execute |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The exploring fan-out summary text counts warnings in the “failed” bucket but always renders them as “N failed”, which produces incorrect UI messaging for warning-only runs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the TUI tool-card rendering so a tool cell’s own lifecycle state (running/success/hydrated/warning/failed) consistently drives both the transcript header glyph and the card rail/border styling, eliminating previously drifted/duplicated status folds and unstyled borders.
Changes:
- Centralizes tool status rollups (notably
ExploringCell::status) and reusesToolCell::status()/ToolCell::is_running()across UI surfaces. - Styles the card rail via a dedicated rail color mapping (
tool_rail_color) and separates it from glyph/state-word ink (tool_glyph_color) to match the intended visual semantics. - Adds/updates tests to lock in rail styling and glyph-vs-rail invariants across statuses.
File summaries
| File | Description |
|---|---|
| crates/tui/src/tui/ui/frame.rs | Removes the local running-status helper now replaced by ToolCell::is_running(). |
| crates/tui/src/tui/ui/activity_detail.rs | Switches activity status derivation to tool.status() to use the unified rollup. |
| crates/tui/src/tui/ui.rs | Uses tool.is_running() for stall-watchdog/running-tool detection. |
| crates/tui/src/tui/history.rs | Implements unified exploring status rollup; styles card rail with state; splits rail vs glyph/state-word theme mapping. |
| crates/tui/src/tui/history/tests.rs | Adds regression tests asserting rail is styled and matches glyph behavior across statuses/families. |
| crates/tui/src/deepseek_theme.rs | Introduces tool_rail_color vs tool_glyph_color mappings and updates theme tests accordingly. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
1255
to
1259
| let counts = format!( | ||
| "{done} done, {running} running{}", | ||
| " {done} done, {running} running{}", | ||
| if failed > 0 { | ||
| format!(", {failed} failed") | ||
| } else { |
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
The PR moves tool-card state into the rail, consolidates ExploringCell status rollup, and adds dark-theme tests for the new Theme helpers. Core logic is sound, but some key visual changes lack direct coverage.
Findings
- [WARNING] No rendering test locks per-entry exploring dot colours (
crates/tui/src/tui/history/tests.rs)
The diff changes exploring fan-out dots from one flat WHALE_INFO line to per-entry styled spans so a failed read is distinguishable from a done one. The new tests only assert ExploringCell::status() rollup and exec-cell rail/glyph colours; no test renders an ExploringCell with mixed statuses and checks the dot spans' foreground colours. A regression in that code path would pass the current suite. - [INFO] Glyph/status colour mappings are only asserted for dark theme (
crates/tui/src/deepseek_theme.rs)
Theme::tool_glyph_color has test coverage only through Theme::dark(). The PR changes light tokens (tool_running_accent -> LIGHT_ACTION) and claims solarized follows the same roles, but there is no per-palette assertion for the glyph split (verify success green vs read success action) or for tool_status_style following the glyph. every_palette_separates_running_failed_and_success only checks rail colours for running/failed/success. This leaves non-dark palettes unprotected against the same kind of token regression this PR is fixing.
Assessment
Well-structured refactor that centralizes previously duplicated status logic and aligns rail/glyph behaviour. The dark-theme tests are strong, but coverage should be extended to the rendered exploring dot strip and to light/solarized/grayscale glyph mappings before the requested visual guarantees are fully locked down.
Advisory review by Codewhale (codewhale review --pr 5799 --post, head 693f08c03ad8dc09ffa8651c5888df4c159cc40a). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
This was referenced Sep 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A running, failed or warned tool now reads as such in the transcript itself, the way oh-my-pi's output blocks do (
output-block.ts: the block's state drives its border). What was wrong here:wrap_card_railinserted the╭ │ ╰rail withSpan::raw, so the card border carried no state; the exploring fan-out painted every dot flatWHALE_INFO, so a failed read looked like a done one; and four copies of the status fold had drifted apart — the card header's copy could not even produceFailed, so a fan-out with a failed read rendereddonein the success colour.Now there is one fold (
ExploringCell::status) and two theme functions, both reusing the existingTheme::tool_status_colorplumbing:tool_rail_color): runningWHALE_ACTION, successTEXT_MUTED, hydratedTEXT_DIM, warningWHALE_HUMAN, failedWHALE_ERROR.tool_glyph_color, per family): running action-blue; settled verify rows✓inWHALE_WORKING_GREEN; settled read/find/run/patch rows keep their family accent (the blue magnifier in mockup tideline-01); warning/failed as the rail.Running, warning and failure use the same ink in both tables by construction, so border and glyph can never disagree about trouble; only success splits, which is the case the accepted mockup cares about. Light and solarized follow the same roles in their own tokens; grayscale is untouched (four greys cannot separate five states, and a test says so rather than papering over it).
Design rationale: codewhale-ops
design/SHELL-DESIGN-20260901.md§2.1 (OMP paragraph) and §2.5a.Testing
scripts/dev-test.sh tui tui::history→Summary 77 tests run: 77 passedscripts/dev-test.sh tui deepseek_theme→Summary 8 tests run: 8 passedscripts/dev-test.sh tui tui::→Summary 3328 tests run: 3328 passedwhole tui lib →
11820 tests run: 11819 passed, 1 failed— the one failure istask_manager::tests::forced_idle_timeout_releases_the_worker_for_later_tasks, a timing test this change does not touch; alone:1 test run: 1 passedThe rail regression test fails without the fix: reverting the two
Span::styledlines →panicked at history/tests.rs:2375: Running left the card rail unstyledNo goldens changed (text-only goldens; none renders a tool cell)
cargo fmt --all -- --checkcargo clippy -p codewhale-tuiwarning-free under the CI allow listtui suites above
Checklist
🤖 Generated with Claude Code
https://claude.ai/code/session_01AdbuqwHAXSDcikPiS6L6Qw
Note
Low Risk
TUI-only visual and status-display logic with broad test coverage; no auth, persistence, or API surface changes.
Overview
Tool transcript cards now encode lifecycle in the border and identity in the header glyph, aligned with oh-my-pi output blocks and the tideline mockup.
Theme replaces a single
tool_status_colorwithtool_rail_color(running/warning/failed stay loud; success and hydrated recede to muted/dim) andtool_glyph_color(status, family)(verify success stays green; other families keep the action accent when done). Dark/light running accents shift to action tokens; status text styling is family-aware.Rendering passes
ToolStatusintowrap_card_railso╭│╰rails are styled, not raw. Headers and exploring fan-out dots use per-entry glyph colors instead of one flat info color.Status rollup adds
ExploringCell::status()(running first, then failed/warning/hydrated) and routesToolCell::status, activity timeline, and headers through it—fixing fan-outs that could showdonewhen a child failed. Duplicate helperstool_cell_is_running/tool_status_for_activityare dropped in favor ofToolCell::is_running()/status().Tests lock rail distinctness, rail vs glyph split on success, verify vs read glyphs, and exploring precedence across palettes.
Reviewed by Cursor Bugbot for commit 693f08c. Bugbot is set up for automated code reviews on this repo. Configure here.