fix(ui): delete what the cursor is on, and make a seeded removal stick - #193
Merged
Merged
Conversation
`d` on the Hosts page resolved the row it was on through `selected_host()`, which answers with the host's `detail_worker` when the cursor is on a host header — whichever single roster entry happened to have probed the machine. So pressing it on a host of three agents removed one of them and left the host standing, with no way to tell from the screen which one went. The cursor already distinguishes the two: `HostsRow.agent` is `None` on a host header. Asking it first is the fix. A host row now removes the host — undeclaring every agent this machine declared on it and dropping its roster entries — and refuses on the primary local host, which is declared by [host] rather than by a removable entry and would be back at the next launch anyway. Agent rows are unchanged. Removing a host is several registry ops for one keypress, so they travel as one `Cmd::WorkerOps` and report one status rather than racing N. The rows are drawn as the tree they describe: branch glyphs, and id, harness and workspace in columns measured across the whole tree — not per host, which would restart the alignment under every header, and not per scroll window, which would slide the text sideways under a cursor that only moved down. Paths truncate from the left, because the tail is what says which checkout it is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The wizard asked which kind first, and "local" meant a harness plus a directory on this device — which is what an agent is now. Two flows wrote the same thing and disagreed about what to call it: one produced a host row, the other an agent under a host row, from the same two answers. Adding capacity to this device is declaring an agent, and that is done where the agents are (n on a local host row in the Hosts tab), so the page is remote-only and says so. What that retires, in order: the kind picker and its two-step state; the harness list and the detected-provider cache the page consulted every frame; the directory prompt; `add_local_host`; and `Cmd::StartLocalHost`, whose only producer it was. Hosts declared in [[hosts]] are unaffected — `local_host::start_all` starts those at launch, and always did. The one capability actually removed is adding a local host *mid-session*, which is the thing being replaced. `LocalHostSpawner` outlived its purpose by one reader: a workflow's `agent` step resolves a custom-harness name against it. Every other field existed for the spawn it no longer does, so it is now `LocalHostHarnesses` holding the options it reads them from — a type that spawns nothing should not be called a spawner. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two rail fixes. The tree began immediately under the green `+ New agent` button, so the button and the first agent read as one block — a list whose first entry happened to be coloured. A dim `── agents ──` now sits between them, matching the lane list's `── functions ──` so the rail's two headings read as the same kind of thing. It is emitted only when there is a tree to head, and the cursor skips it like a host header. And an idle declared agent was drawn wholly DIM — name, harness and directory alike — which left the row with nothing to read it by. The name now carries the row in cyan and only the qualifier recedes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An install that has never declared anything advertises a *seeded* fleet — one agent per coding-agent CLI found on PATH (`specs_for` falls back to `seed_for` when the declaration list is empty). Every row on a fresh machine is one of those, and none of them has a declaration behind it. So `d` could not remove one. `undeclare_selected_agent` guards on `agent.declared`, which is false for a seed, and returned without writing anything; all that was left was dropping the roster entry, which the seed recomputes from PATH at the next start. The agent came back, which is what "I deleted it and it is still there" was. The first removal is now what makes the list real: the host's surviving agents are written down as declarations and the removed one is not among them. From then on the fleet is what the operator declared rather than what happened to be installed, and the next removal is an ordinary undeclare. Declarations on other hosts are carried through untouched, and a row this machine may not edit is never written down — it belongs to the config that declares it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
sanil-23 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe TUI replaces stateful local-host spawning with harness-only wiring and batch worker operations. Host removal now persists seeded-agent changes. Add Host supports remote hosts only. Agent rail rows and host-list rendering receive new headings, styling, alignment, and truncation behavior. ChangesHost management and TUI routing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant remove_host_row
participant remove_selected_host
participant Config
participant WorkerOps
User->>remove_host_row: Delete selected host
remove_host_row->>remove_selected_host: Remove host and editable agents
remove_selected_host->>Config: Persist seeded-agent declarations
remove_selected_host-->>WorkerOps: Return worker removal operations
WorkerOps-->>User: Report removal status
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 6
🧹 Nitpick comments (2)
src/tui/src/ui/app/rail/mod.rs (1)
260-265: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMove behavior-heavy code out of directory
mod.rsfiles.These directory modules contain rail assembly and rendering behavior. Keep each
mod.rslimited to module documentation and wiring. Move the implementation into named child modules.
src/tui/src/ui/app/rail/mod.rs#L260-L265: moveflattenand its rail-tree assembly helpers into a dedicated child module.src/tui/src/ui/app/render/agents/rail/mod.rs#L346-L351: move row rendering dispatch into a dedicated child module.src/tui/src/ui/app/render/agents/rail/mod.rs#L399-L422: keep declared-agent row styling with that rendering implementation.As per coding guidelines, “Keep
mod.rsfocused on module documentation,mod/pub usewiring, and glue that fits no more specific submodule.”🤖 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 `@src/tui/src/ui/app/rail/mod.rs` around lines 260 - 265, Move the rail assembly logic in `Rail::flatten` and its helper code out of `src/tui/src/ui/app/rail/mod.rs` into a dedicated child module, leaving `mod.rs` as wiring/documentation only. Do the same for the row rendering dispatch in `src/tui/src/ui/app/render/agents/rail/mod.rs`, extracting the behavior from the render entrypoint while keeping the declared-agent row styling with that rendering implementation. Preserve the existing `flatten`, row-dispatch, and styling behavior by re-exporting or wiring the new child modules from each `mod.rs` rather than keeping the implementation inline.Source: Coding guidelines
src/tui/src/ui/app/hosts/tests.rs (1)
654-669: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the surviving declaration by id.
!declared.is_empty()passes for any non-empty list. Name the sibling instead, so the test fails if adoption writes the wrong agent or writes more than the survivors.♻️ Proposed test tightening
- assert!( - !declared.is_empty(), - "and its siblings now are, so the list stops being seeded: {declared:?}" - ); + let survivor = if target == "medulla-claude" { + "medulla-codex" + } else { + "medulla-claude" + }; + assert_eq!( + declared, + vec![survivor.to_string()], + "the sibling is declared and nothing else is: {declared:?}" + );🤖 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 `@src/tui/src/ui/app/hosts/tests.rs` around lines 654 - 669, Update the assertions in the host adoption test around the declared agent list to verify the specific surviving sibling agent ID rather than only checking that declared is non-empty. Preserve the existing assertion that target is absent, and assert the expected survivor by ID so incorrect or extra declarations fail the test.
🤖 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 `@src/tui/src/event_loop/types.rs`:
- Around line 109-112: Update the doc comment for the local_hosts field to
describe LocalHostHarnesses as containing the primary host’s options for
workflow agent steps to resolve a declared custom harness; remove references to
starting hosts, bus bindings, and session managers. Renaming local_hosts to
local_host_harnesses is optional and should include its known app_loop and
run_cmd references if performed.
In `@src/tui/src/hub_relay/tests.rs`:
- Around line 356-360: Update the comments in
a_host_added_after_launch_is_not_remembered_as_a_remote_peer, including the “The
spawner binds a second host” comment, to describe a second host being created
during the session without attributing that action to LocalHostHarnesses. Keep
the test and its save-time filtering scenario unchanged.
In `@src/tui/src/ui/app/hosts/edit.rs`:
- Around line 225-243: Update remove_selected_host so non-primary host removal
adopts seeded-agent survivors into the declaration list, following
adopt_seeded_agents and the single-agent removal path. Continue passing all
applicable agent IDs through undeclare_agent_id, including seeded agents. Update
the status reporting to detect undeclared < declared.len() and clearly indicate
the undeclare shortfall instead of reporting removal as complete.
- Around line 263-282: Update adopt_seeded_agents so survivors reuse their
existing AgentDeclaration when one already exists, preserving custom name and
strategy fields; only call declaration_for for agents without a matching
declaration. Keep filtering out the removed agent and non-editable rows, and
retain declarations belonging to other hosts.
In `@src/tui/src/ui/app/keys/routing/add_host.rs`:
- Around line 29-31: Update the add_host_key routing test in routing.rs to
reflect that the first c key now copies REMOTE_JOIN_COMMAND immediately. Assert
one copy after c, remove the obsolete Down/selection steps, and retain coverage
for the resulting handled routing behavior.
In `@src/tui/src/ui/app/rail/tests.rs`:
- Around line 417-443: Extend the test coverage around the rail-row behavior by
adding a fixture with no agents in every HostGroup and asserting that
`RailRow::AgentsHeader` is absent from `app.rail_rows()`. Keep the existing
agent-present assertions in
`the_agents_heading_sits_under_the_create_action_and_only_over_a_tree`
unchanged.
---
Nitpick comments:
In `@src/tui/src/ui/app/hosts/tests.rs`:
- Around line 654-669: Update the assertions in the host adoption test around
the declared agent list to verify the specific surviving sibling agent ID rather
than only checking that declared is non-empty. Preserve the existing assertion
that target is absent, and assert the expected survivor by ID so incorrect or
extra declarations fail the test.
In `@src/tui/src/ui/app/rail/mod.rs`:
- Around line 260-265: Move the rail assembly logic in `Rail::flatten` and its
helper code out of `src/tui/src/ui/app/rail/mod.rs` into a dedicated child
module, leaving `mod.rs` as wiring/documentation only. Do the same for the row
rendering dispatch in `src/tui/src/ui/app/render/agents/rail/mod.rs`, extracting
the behavior from the render entrypoint while keeping the declared-agent row
styling with that rendering implementation. Preserve the existing `flatten`,
row-dispatch, and styling behavior by re-exporting or wiring the new child
modules from each `mod.rs` rather than keeping the implementation inline.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a4a768d-7b33-4094-8ec5-939992667b08
📒 Files selected for processing (20)
src/tui/src/app_loop.rssrc/tui/src/event_loop/cmd_dispatch/mod.rssrc/tui/src/event_loop/cmd_dispatch/workflows.rssrc/tui/src/event_loop/types.rssrc/tui/src/hub_relay/tests.rssrc/tui/src/local_host/mod.rssrc/tui/src/ui/app/commands/dispatch.rssrc/tui/src/ui/app/hosts/edit.rssrc/tui/src/ui/app/hosts/tests.rssrc/tui/src/ui/app/keys/routing/add_host.rssrc/tui/src/ui/app/keys/routing/mod.rssrc/tui/src/ui/app/rail/mod.rssrc/tui/src/ui/app/rail/tests.rssrc/tui/src/ui/app/rail/types.rssrc/tui/src/ui/app/render/agents/rail/mod.rssrc/tui/src/ui/app/render/routing/add_host.rssrc/tui/src/ui/app/render/routing/hosts/list.rssrc/tui/src/ui/app/state.rssrc/tui/src/ui/app/types.rssrc/tui/src/ui/app/workspaces.rs
💤 Files with no reviewable changes (3)
- src/tui/src/ui/app/workspaces.rs
- src/tui/src/ui/app/commands/dispatch.rs
- src/tui/src/ui/app/state.rs
…finds Review follow-ups, and the CI failure behind them. The Add Host tests still drove the retired kind picker — Down to pick Remote, then two confirms. With one step the second 'a' was typed into the address prompt instead, so an empty submit was no longer empty. Three tests, two of which failed and one of which passed for that reason. `adopt_seeded_agents` rebuilt every survivor through `declaration_for`, which goes via `AgentDeclaration::new` and hardcodes `name: None` and the checkout strategy. A host holds declared and seeded rows at once, so removing one seed silently reset the operator's name (and any non-default strategy) on every declared sibling. An existing declaration is now carried through as it is, and only a row with none is built from the projection. `remove_selected_host` skipped seeded rows, so removing a host this device runs dropped roster entries that the seed recomputes from PATH at the next launch — the same non-removal this branch fixes for a single agent. A running host cannot be emptied for that exact reason: with no declarations left it seeds again. So it is refused, and the message points at its agents, which are removable one by one. The guard asks the *running* hosts rather than `host.kind`, because a row is also Local when it exists only because agents are declared against its id — that one has no config entry to come back from and is removable. The status compares what was undeclared against what was attempted: a declaration that would not write is an agent that returns at the next launch, and reporting it as removed is how the operator finds out later instead of now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
sanil-23 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Six more sites still drove the retired kind picker. The two that failed CI were in `feature_paste` and `feature_workers`; the rest passed while asserting a flow that no longer exists, which is worse. Two tests went with the behaviour they covered: the local-host retry (that whole save path is gone with the wizard's local kind) and the confirmed-kind arrowing regression (there is no kind to confirm). Three assertions were rewritten rather than deleted — the page's opening line changed with the page, `c` now always has an install line to copy instead of copying nothing on the local branch, and the roles test no longer pins which agent the cursor lands on: removing an undeclared row writes the survivors down, and a newly declared agent sorts above the roster-only rows, so the order after that keypress is legitimately different. What that test is about — the arrows leaving the role list — is asserted instead. Verified with the whole integration suite this time, not a truncated view of it: 36 targets pass. The remaining failures are the known macOS `launch_root` trio and the two `daemon::providers` watchdog tests that pass in isolation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
sanil-23 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
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 `@src/tui/tests/feature_workers/roles.rs`:
- Around line 132-141: Strengthen the assertion after
app.on_event(key(KeyCode::Down)) in the role-navigation test so it verifies tree
focus rather than merely finding an “Agent · ” label in the rendered buffer. Use
the unique tree-focus marker produced by render, or inspect the app’s focus
state directly, and ensure the assertion fails when focus remains on the role
toggles.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60d381d9-a6e4-4424-9616-ad9ad2f35d73
📒 Files selected for processing (4)
src/tui/tests/feature_paste/composer.rssrc/tui/tests/feature_workers/list.rssrc/tui/tests/feature_workers/roles.rssrc/tui/tests/feature_workers/routing.rs
Two review follow-ups, both about tests proving less than they read as proving.
The roles test asserted `out.contains("Agent · ")` after the removal, which only
says an agent label is somewhere in the terminal buffer — an agent preview is
drawn whether the arrows are on the tree or on the toggles, so it would have
passed with the focus still on the role list, which is the one thing the test
exists to catch. It now asks the focus directly, through a seam in the same shape
as `hosts_cursor_on_host` beside it.
And the heading test covered only the branch where a heading appears. The other
half is the one worth having: a hosting device with nothing declared and no
traffic offers the create action and no heading over it. Built on the empty
runtime rather than `hosting_app`, whose demo lanes are agents in their own
right — the first attempt asserted the empty case against a fixture that has a
tree, and failed for that reason rather than finding a bug.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
sanil-23 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Collaborator
Author
|
@coderabbitai review |
|
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
1 participant
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.
Four fixes to the Hosts page and the Agents rail, found by using them.
dremoved the wrong thingremove_host_rownever asked which kind of row the cursor was on. On a hostheader
selected_host_agent()isNone, so the undeclare no-opped and it fellthrough to
selected_host()— which for a host row resolves tohost.detail_worker, whichever single entry happened to have probed the machine.Pressing
don a host of three agents removed one of them and left the hoststanding, with nothing on screen saying which.
The cursor already distinguishes them (
HostsRow.agentisNoneon a header).Asking first is the fix. A host row now removes the host — undeclaring every
agent this machine declared on it and dropping its roster entries — and refuses
on the primary local host, which is declared by
[host]rather than by aremovable entry. Agent rows are unchanged.
Removing a host is several registry ops for one keypress, so they travel as one
new
Cmd::WorkerOpsand report one status instead of racing N.Removing a seeded agent was not a removal
An install that has never declared anything advertises a seeded fleet — one
agent per coding-agent CLI on
PATH(specs_forfalls back toseed_forwhenthe declaration list is empty). On a fresh machine every row is one of those
and none has a declaration behind it, so
undeclare_selected_agent— whichguards on
agent.declared— wrote nothing. Only the roster entry went, and theseed recomputed it from
PATHat the next start.The first removal now makes the list real: the host's survivors are written down
as declarations with the removed one absent, so the fleet becomes what the
operator declared rather than what happened to be installed. Later removals are
ordinary undeclares. Declarations on other hosts pass through untouched, and a
row this machine may not edit is never written down.
Add Host adds a machine, and only a machine
The wizard asked which kind first, and "local" meant a harness plus a directory
on this device — which is what an agent is now. Two flows wrote the same thing
from the same two answers and disagreed about what to call it. Adding capacity
here is declaring an agent (
non a local host row), so the page is remote-only.That retires the kind picker, the harness list, the per-frame provider-detection
cache, the directory prompt,
add_local_host, andCmd::StartLocalHost— whoseonly producer it was. Hosts declared in
[[hosts]]are unaffected:local_host::start_allstarts those at launch and always did. The one capabilityremoved is adding a local host mid-session, which is the thing being replaced.
LocalHostSpawneroutlived its purpose by one reader — a workflow'sagentstepresolves custom-harness names against it — so it is now
LocalHostHarnessesholding the options it reads them from.
Reading the tree
Host rows draw as the tree they describe:
├─/└─branches with id, harness andworkspace in columns measured across the whole tree — not per host, which restarts
the alignment under every header, and not per scroll window, which slides the text
sideways under a cursor that only moved down. Paths truncate from the left,
because the tail says which checkout it is.
On the Agents rail, the tree began immediately under the green
+ New agentbutton, so the two read as one block. A dim
── agents ──now separates them,matching the lane list's
── functions ──; it appears only when there is a treeto head and the cursor skips it. And an idle declared agent was drawn wholly
DIM— name, harness and directory alike — leaving nothing to read the row by;the name now carries it and only the qualifier recedes.
Validation
cargo fmt --all --checkcargo clippy --locked --all-targets -- -D warningscargo test --locked -p medulla-tui --lib ui::app— 394 passSix new tests: the two
dbranches, the declared-agent-by-host-row case, therefusal on this device, the seeded-removal persistence, and the heading's
placement.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes