feat(min): add min dash session manager TUI - #1112
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds shared ChangesDash session management
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
b408dcf to
38fafdf
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
crates/minimal-client/src/file_upload.rs (1)
1258-1321: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test can pass without exercising the branch it names. It races a 10 ms
sleepagainstadd_file's read loop and acceptsOk(())as a pass, so on a fast machine it asserts nothing about shrink detection. The comment block at Lines 1270-1287 also narrates three abandoned approaches rather than describing the test.A deterministic alternative: make the source large enough that the read loop cannot complete in one
fill_buf, or drop the race entirely and unit-test the bounded-copy invariant by declaring a size larger than the file (e.g. write the file,set_lenit smaller beforeadd_file's stat, and assertwritten < declaredsurfaces). Trimming the comment to a one-liner about the race would help either way.🤖 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/minimal-client/src/file_upload.rs` around lines 1258 - 1321, Replace the timing-dependent test add_file_errors_when_source_shrinks_during_read with a deterministic shrink-detection test that guarantees the declared source size exceeds the bytes available to add_file, then assert the operation returns an error containing “shrank during upload.” Remove the accepted Ok(()) branch and the abandoned-approach narrative, retaining only a concise comment describing the invariant under test.crates/minimal-client/src/attach.rs (1)
174-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the un-pinned fallback branch. The tests cover the pinned path twice but never assert the
known_hosts-absent behaviour, which is the security-relevant half ofhost_key_opts.♻️ Suggested addition
/// No recorded host key yet (first boot): the check is waived rather /// than failing the attach, and nothing is written to a real known_hosts. #[test] fn host_key_opts_waive_when_no_known_hosts() { let tmp = tempfile::tempdir().unwrap(); let [strict, hosts_file] = host_key_opts(&tmp.path().join(paths::KNOWN_HOSTS_FILE)); assert_eq!(strict, "StrictHostKeyChecking=no"); assert_eq!(hosts_file, "UserKnownHostsFile=/dev/null"); }🤖 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/minimal-client/src/attach.rs` around lines 174 - 186, Add a test alongside host_key_opts_pin_to_an_adjacent_known_hosts for the missing-known_hosts fallback, using a temporary path that does not exist. Assert host_key_opts returns StrictHostKeyChecking=no and UserKnownHostsFile=/dev/null.crates/minimald/src/rpc.rs (1)
555-559: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale "Preview tab" wording — layout is now a vertical stack, not tabs.
Per the PR's own change ("Changed the detail pane from tabs to a vertical stack") and the matching docs/render.rs wording ("stacks ... Info, networking Policy, and ... Preview"), this doc comment's "Preview tab" is outdated terminology.
📝 Proposed fix
-/// `GetSessionScreen` (`min dash` Preview tab): a read-only snapshot of the +/// `GetSessionScreen` (`min dash`'s Preview section): a read-only snapshot of the /// session's terminal screen. Unlike attach, this mints nothing and resizes /// nothing — a session with no live host answers with an error the TUI /// renders as "session not active".🤖 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/minimald/src/rpc.rs` around lines 555 - 559, Update the doc comment for serve_get_session_screen to replace the stale “Preview tab” terminology with wording that describes the Preview section in the detail pane’s vertical stack; leave the rest of the session-screen behavior description unchanged.docs/reference/cli-min.md (1)
57-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a language to the fenced code block (MD040).
📝 Proposed fix
-``` +```text min dash</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/reference/cli-min.mdaround lines 57 - 59, Update the fenced code block
containing “min dash” to specify the text language identifier, using the
existing documentation formatting conventions.</details> <!-- cr-comment:v1:935adc8c3eef58061b55e30b --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>crates/minimal-tui/src/app.rs (1)</summary><blockquote> `645-655`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_ **Reorder the empty-provider guard so submit doesn't silently no-op with a stale index.** `provider` is resolved before the `providers.is_empty()` check; the guard works today but reads inverted. Checking emptiness first is clearer and drops the redundant `.filter(|p| *p < model.providers.len())`. <details> <summary>🤖 Prompt for AI Agents</summary>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/minimal-tui/src/app.rsaround lines 645 - 655, In the
CreateField::Network Enter handler, move the model.providers.is_empty() guard
before resolving provider so empty providers return the existing status message
immediately. Then simplify provider resolution to use the focused provider with
unwrap_or(0), removing the redundant length filter while preserving behavior for
non-empty providers.</details> <!-- cr-comment:v1:1fca00a71cac8faf36bb85a3 --> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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/minimal-tui/src/app.rs:
- Around line 481-548: Restrict bare-key action matches in the key handling
match to KeyModifiers::NONE: update the d, r, n, q, j, and k bindings, while
preserving the explicit Ctrl-C binding and any intentionally
modifier-independent keys such as slash or Enter. This prevents modified key
combinations from triggering destroy, rename, create, quit, or navigation
actions.- Around line 800-807: Update the Effect::Attach handling and attach_and_resume
flow so the blocking SSH status wait runs via an async process or spawn_blocking
instead of occupying the Tokio worker, while preserving terminal suspend/resume
behavior. Keep the existing EventStream instance alive; do not drop and recreate
it around the attach operation, and continue enqueueing Msg::Tick after
completion.In
@crates/minimal-tui/src/render.rs:
- Around line 547-564: Update wire_color to validate that the six-character
color payload consists only of ASCII hexadecimal digits before slicing it into
byte pairs and parsing RGB values. Preserve the existing indexed-color handling
and return Color::Reset for invalid or non-ASCII inputs.- Around line 241-246: Update shorten_home so the home-directory replacement
only applies when path equals home or the remaining suffix begins with the
platform path separator; otherwise return the original path unchanged. Preserve
the existing "~" formatting for valid home paths.In
@crates/minimal-tui/src/rpc.rs:
- Around line 72-126: Bound each oneshot RPC awaited by refresh, fetch_detail,
and fetch_screen with tokio::time::timeout using a timeout near the refresh
interval, and propagate timeout failures with clear context so exec_effect can
display them in the status line. Preserve the existing successful response
handling and per-RPC error contexts while ensuring no UI-loop await can remain
unbounded.In
@crates/minimal/src/lib.rs:
- Around line 864-875: Update cmd_dash so daemon detection does not perform two
identical provider-agnostic is_daemon_running calls. Prefer probing both
provider socket paths directly via minimal_client::resolve_socket_path, matching
minimal_tui::rpc::discover, and use those results to avoid calling ensure_daemon
when either daemon is already running; if retaining the helper, collapse this to
one call and document its provider-agnostic behavior.In
@docs/reference/cli-min.md:
- Around line 68-76: Clarify the
dkey description in the CLI documentation to
state that it destroys the session and causes an in-flight create/upload flow to
fail, rather than claiming it cancels the task. Do not describe cancellation
unless explicit cancellation plumbing is added.
Nitpick comments:
In@crates/minimal-client/src/attach.rs:
- Around line 174-186: Add a test alongside
host_key_opts_pin_to_an_adjacent_known_hosts for the missing-known_hosts
fallback, using a temporary path that does not exist. Assert host_key_opts
returns StrictHostKeyChecking=no and UserKnownHostsFile=/dev/null.In
@crates/minimal-client/src/file_upload.rs:
- Around line 1258-1321: Replace the timing-dependent test
add_file_errors_when_source_shrinks_during_read with a deterministic
shrink-detection test that guarantees the declared source size exceeds the bytes
available to add_file, then assert the operation returns an error containing
“shrank during upload.” Remove the accepted Ok(()) branch and the
abandoned-approach narrative, retaining only a concise comment describing the
invariant under test.In
@crates/minimal-tui/src/app.rs:
- Around line 645-655: In the CreateField::Network Enter handler, move the
model.providers.is_empty() guard before resolving provider so empty providers
return the existing status message immediately. Then simplify provider
resolution to use the focused provider with unwrap_or(0), removing the redundant
length filter while preserving behavior for non-empty providers.In
@crates/minimald/src/rpc.rs:
- Around line 555-559: Update the doc comment for serve_get_session_screen to
replace the stale “Preview tab” terminology with wording that describes the
Preview section in the detail pane’s vertical stack; leave the rest of the
session-screen behavior description unchanged.In
@docs/reference/cli-min.md:
- Around line 57-59: Update the fenced code block containing “min dash” to
specify the text language identifier, using the existing documentation
formatting conventions.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `feaccc3e-6f7d-49f9-bdb9-4b898ad7e60c` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 957c07619a3d265bdf4dff93ca27a76b75f75928 and b408dcf959487e9ac0938c37ce4adf1774fdf466. </details> <details> <summary>⛔ Files ignored due to path filters (7)</summary> * `Cargo.lock` is excluded by `!**/*.lock` * `crates/minimal-tui/tests/snapshots/snapshots__detail_pane_with_policy.snap` is excluded by `!**/*.snap` * `crates/minimal-tui/tests/snapshots/snapshots__empty_list.snap` is excluded by `!**/*.snap` * `crates/minimal-tui/tests/snapshots/snapshots__filtered.snap` is excluded by `!**/*.snap` * `crates/minimal-tui/tests/snapshots/snapshots__preview_section_with_screen_snapshot.snap` is excluded by `!**/*.snap` * `crates/minimal-tui/tests/snapshots/snapshots__single_provider.snap` is excluded by `!**/*.snap` * `crates/minimal-tui/tests/snapshots/snapshots__two_providers.snap` is excluded by `!**/*.snap` </details> <details> <summary>📒 Files selected for processing (25)</summary> * `AGENTS.md` * `Cargo.toml` * `crates/minimal-client/Cargo.toml` * `crates/minimal-client/src/attach.rs` * `crates/minimal-client/src/file_upload.rs` * `crates/minimal-client/src/lib.rs` * `crates/minimal-tui/Cargo.toml` * `crates/minimal-tui/src/app.rs` * `crates/minimal-tui/src/event.rs` * `crates/minimal-tui/src/filter.rs` * `crates/minimal-tui/src/lib.rs` * `crates/minimal-tui/src/render.rs` * `crates/minimal-tui/src/rpc.rs` * `crates/minimal-tui/src/state.rs` * `crates/minimal-tui/tests/snapshots.rs` * `crates/minimal/Cargo.toml` * `crates/minimal/src/lib.rs` * `crates/minimal/src/main.rs` * `crates/minimald-rpc/src/lib.rs` * `crates/minimald/src/rpc.rs` * `crates/minimald/src/session.rs` * `crates/minimald/src/session_host.rs` * `crates/minimald/src/sessions.rs` * `docs/reference/cli-min.md` * `docs/specs/07-spec-min-dash-tui/07-spec-min-dash-tui.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| // Attach suspends the TUI around a blocking ssh child; it | ||
| // needs the terminal guard, so it can't live in exec_effect. | ||
| Effect::Attach(key) => { | ||
| if let Some(p) = providers.get(key.provider) { | ||
| attach_and_resume(&mut terminal, &p.sock, key, &mut model); | ||
| inbox.push_back(Msg::Tick); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
crossterm EventStream background reader thread consumes stdin when not polled
💡 Result:
Crossterm's EventStream does not have a background thread that eagerly and continuously consumes stdin [1]. Instead, it employs a mechanism where a dedicated reader thread is only triggered to poll the input when the EventStream itself is being polled by the application [2][1][3]. When you poll an EventStream (e.g., via poll_next), it first checks for pending events using a non-blocking poll [2][3]. If no events are available, it spawns or signals a background task to block on the input, which then notifies the executor to wake the stream when data arrives [2][3]. Consequently, if the EventStream is not being polled, it does not actively read from stdin, meaning it should not "steal" input from other processes in that state [1]. However, developers sometimes observe input issues when using EventStream alongside other libraries that also read from stdin (such as image-rendering crates) [1]. This is typically due to the fact that crossterm maintains a global lock on the terminal's input reader [4]. If multiple components attempt to interact with the terminal input concurrently, or if the EventStream is not properly dropped or paused when relinquishing control, input sequences can be intercepted [4][1]. To safely hand off control of stdin to another process, it is often recommended to drop the EventStream entirely, as simply stopping the poll loop may not be sufficient if the underlying reader thread is already blocked waiting for input [4].
Citations:
- 1: How to temporarily lock EventStream polling crossterm-rs/crossterm#1039
- 2: https://github.com/crossterm-rs/crossterm/blob/master/src/event/stream.rs
- 3: https://github.com/crossterm-rs/crossterm/blob/6af9116b/src/event/stream.rs
- 4: https://github.com/openai/codex/blob/31519549/codex-rs/tui/src/tui/event_stream.rs
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target file outline ---'
ast-grep outline crates/minimal-tui/src/app.rs --view compact
printf '%s\n' '--- relevant implementation ---'
sed -n '720,835p' crates/minimal-tui/src/app.rs
sed -n '910,980p' crates/minimal-tui/src/app.rs
printf '%s\n' '--- runtime and call-site context ---'
rg -n -C 4 'Effect::Attach|Effect::Create|EventStream|attach_and_resume|Runtime|tokio::main|spawn' crates/minimal-tui/src crates -g '*.rs'
printf '%s\n' '--- standards ---'
if test -f docs/rust-coding-standards.md; then sed -n '1,240p' docs/rust-coding-standards.md; fiRepository: gominimal/minimal
Length of output: 334
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target file outline ---'
ast-grep outline crates/minimal-tui/src/app.rs --view signatures
printf '%s\n' '--- relevant implementation ---'
sed -n '720,835p' crates/minimal-tui/src/app.rs
sed -n '910,980p' crates/minimal-tui/src/app.rs
printf '%s\n' '--- runtime and call-site context ---'
rg -n -C 4 'Effect::Attach|Effect::Create|EventStream|attach_and_resume|Runtime|tokio::main|spawn' crates/minimal-tui/src crates -g '*.rs'
printf '%s\n' '--- standards ---'
if test -f docs/rust-coding-standards.md; then sed -n '1,240p' docs/rust-coding-standards.md; fiRepository: gominimal/minimal
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- app event loop and effects ---'
sed -n '735,825p' crates/minimal-tui/src/app.rs
printf '%s\n' '--- attach implementation ---'
sed -n '925,970p' crates/minimal-tui/src/app.rs
printf '%s\n' '--- app imports and runtime-related definitions ---'
sed -n '1,90p' crates/minimal-tui/src/app.rs
rg -n -C 5 'run_app|App::|EventStream|Runtime|block_on|spawn|Effect::Attach|Effect::Create|attach_and_resume' crates/minimal-tui/src -g '*.rs'
printf '%s\n' '--- package metadata ---'
sed -n '1,180p' crates/minimal-tui/Cargo.tomlRepository: gominimal/minimal
Length of output: 16973
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- TUI entrypoint and runtime configuration ---'
rg -n -C 8 'minimal_tui|minimal-tui|tokio::main|fn run|run\(' crates/minimal/src crates/minimal-tui/src -g '*.rs'
printf '%s\n' '--- crossterm version and source availability ---'
rg -n -A 8 -B 3 '^name = "crossterm"$|^name = "tokio"$' Cargo.lock
fd -i 'stream.rs' . | head -20
printf '%s\n' '--- terminal suspend/resume implementation ---'
sed -n '970,1045p' crates/minimal-tui/src/app.rs
printf '%s\n' '--- create task body ---'
sed -n '808,900p' crates/minimal-tui/src/app.rsRepository: gominimal/minimal
Length of output: 26467
🌐 Web query:
crossterm 0.29.0 src event stream.rs EventStream Drop poll_next reader thread
💡 Result:
In crossterm 0.29.0, EventStream utilizes a specialized architecture to bridge synchronous terminal event reading with asynchronous streams [1][2][3]. It relies on a dedicated background thread to block on terminal input, ensuring that the asynchronous executor is not blocked while waiting for events [1][4][5]. The mechanism centers on two key wakers used to coordinate activity [1][3]: 1. Stream Waker: This waker is provided by the asynchronous runtime (e.g., tokio or async-std). When the EventStream detects no events are currently available during a call to poll_next, it spawns or delegates to a background thread to wait for input [1][4]. Once an event arrives, this waker notifies the executor that the task can resume and poll_next can be called again [1][3]. 2. Internal Waker: This waker is used to manage the lifecycle of the background reader thread [3]. The Drop implementation for EventStream is critical for resource management [1][2]. When an EventStream is dropped, it performs the following steps to ensure a clean shutdown [1][4]: - It sets a flag (stream_wake_task_should_shutdown) to true to signal the background thread that it should terminate [1][4]. - It triggers the internal waker (poll_internal_waker.wake) to force the blocking poll operation to exit immediately [1][4][3]. This ensures that the thread dedicated to reading terminal input does not persist as a zombie or cause blocking issues after the EventStream has been dropped [1][4].
Citations:
- 1: https://github.com/crossterm-rs/crossterm/blob/6af9116b/src/event/stream.rs
- 2: https://docs.rs/crossterm/0.29.0/i686-pc-windows-msvc/crossterm/event/struct.EventStream.html
- 3: https://deepwiki.com/crossterm-rs/crossterm/6.3-event-system-architecture
- 4: https://github.com/crossterm-rs/crossterm/blob/master/src/event/stream.rs
- 5: Consider using
spawnandchannelin specific async runtime. crossterm-rs/crossterm#608
🏁 Script executed:
#!/bin/bash
set -e
url='https://raw.githubusercontent.com/crossterm-rs/crossterm/v0.29.0/src/event/stream.rs'
curl -fsSL "$url" | sed -n '1,280p'Repository: gominimal/minimal
Length of output: 204
🏁 Script executed:
#!/bin/bash
set -e
for ref in master v0.29.0 crossterm-0.29.0; do
url="https://raw.githubusercontent.com/crossterm-rs/crossterm/$ref/src/event/stream.rs"
if body=$(curl -fsSL "$url" 2>/dev/null); then
printf '%s\n' "--- $ref ---"
printf '%s\n' "$body" | sed -n '1,300p'
break
fi
doneRepository: gominimal/minimal
Length of output: 5049
🏁 Script executed:
#!/bin/bash
set -e
base='https://raw.githubusercontent.com/crossterm-rs/crossterm/master/src/event'
for file in internal.rs sys/unix.rs sys/windows.rs; do
printf '%s\n' "--- $file ---"
curl -fsSL "$base/$file" 2>/dev/null | rg -n -C 5 'lock_event_reader|fn poll|fn read|struct EventReader|Waker|impl Drop'
doneRepository: gominimal/minimal
Length of output: 2102
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- event module files ---'
curl -fsSL 'https://api.github.com/repos/crossterm-rs/crossterm/contents/src/event' |
jq -r '.[].path'
printf '%s\n' '--- internal reader implementation ---'
curl -fsSL 'https://raw.githubusercontent.com/crossterm-rs/crossterm/master/src/event/internal.rs' |
sed -n '1,260p'Repository: gominimal/minimal
Length of output: 2845
🏁 Script executed:
#!/bin/bash
set -e
for file in src/event/read.rs src/event/timeout.rs; do
printf '%s\n' "--- $file ---"
curl -fsSL "https://raw.githubusercontent.com/crossterm-rs/crossterm/master/$file" |
rg -n -C 8 'struct InternalEventReader|impl InternalEventReader|fn poll|fn read|fn try_read|VecDeque|stdin|read_buf'
doneRepository: gominimal/minimal
Length of output: 11072
🏁 Script executed:
#!/bin/bash
set -e
curl -fsSL 'https://raw.githubusercontent.com/crossterm-rs/crossterm/master/src/event/read.rs' |
sed -n '37,145p'Repository: gominimal/minimal
Length of output: 3716
Move the blocking attach wait off the Tokio worker.
cmd.status() blocks the runtime worker for the entire SSH session; on a single-worker runtime, an in-flight Effect::Create cannot progress. Use an async process or spawn_blocking while preserving terminal suspend/resume. EventStream only reads through polling and does not continuously consume stdin while idle, so dropping/recreating it is unnecessary.
🤖 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/minimal-tui/src/app.rs` around lines 800 - 807, Update the
Effect::Attach handling and attach_and_resume flow so the blocking SSH status
wait runs via an async process or spawn_blocking instead of occupying the Tokio
worker, while preserving terminal suspend/resume behavior. Keep the existing
EventStream instance alive; do not drop and recreate it around the attach
operation, and continue enqueueing Msg::Tick after completion.
| pub async fn cmd_dash(global: &GlobalArgs) -> Result<(), anyhow::Error> { | ||
| let dir = global.minimal_dir.as_deref(); | ||
| let host_up = autospawn::is_daemon_running(false, dir).unwrap_or(false); | ||
| let vm_up = autospawn::is_daemon_running(true, dir).unwrap_or(false); | ||
| if !host_up && !vm_up { | ||
| ensure_daemon(global)?; | ||
| } | ||
| minimal_tui::run(minimal_tui::DashOptions { | ||
| minimal_dir: global.minimal_dir.clone(), | ||
| }) | ||
| .await | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The two-provider probe is effectively one probe. autospawn::is_daemon_running ignores its use_minvmd argument (crates/minimal/src/autospawn.rs:273-276: let _ = use_minvmd; is_minvmd_running(minimal_dir)), so host_up and vm_up always hold the same value and the native-minimald socket is never checked. On Linux with only minimald running, both come back false and cmd_dash autospawns needlessly.
Either collapse to a single call with a comment about the helper's provider-agnostic behaviour, or probe the sockets directly (minimal_client::resolve_socket_path for both kinds) the way minimal_tui::rpc::discover does.
♻️ Single-probe form (if the helper stays provider-agnostic)
- let host_up = autospawn::is_daemon_running(false, dir).unwrap_or(false);
- let vm_up = autospawn::is_daemon_running(true, dir).unwrap_or(false);
- if !host_up && !vm_up {
+ // `is_daemon_running` ignores its provider flag today, so one call
+ // answers for both backends.
+ if !autospawn::is_daemon_running(global.use_minvmd(), dir).unwrap_or(false) {
ensure_daemon(global)?;
}🤖 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/minimal/src/lib.rs` around lines 864 - 875, Update cmd_dash so daemon
detection does not perform two identical provider-agnostic is_daemon_running
calls. Prefer probing both provider socket paths directly via
minimal_client::resolve_socket_path, matching minimal_tui::rpc::discover, and
use those results to avoid calling ensure_daemon when either daemon is already
running; if retaining the helper, collapse this to one call and document its
provider-agnostic behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/specs/07-spec-min-dash-tui/07-spec-min-dash-tui.md (1)
511-524: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winProof artifact 2 references a non-existent crate path.
grep -q 'Dash' crates/minimal2/src/main.rswon't find the file — this PR's actual CLI crate iscrates/minimal(seecrates/minimal/src/main.rs,crates/minimal/src/lib.rs). The doc usesminimal2consistently elsewhere too (lines 33, 42, 103-105, 119, 300-304), so this is a repo-wide stale reference, but now that this spec is markedshipped(line 5), its literal verification command should actually work.📝 Proposed fix
-`grep -q 'Dash' crates/minimal2/src/main.rs` — the subcommand exists. +`grep -q 'Dash' crates/minimal/src/main.rs` — the subcommand exists.🤖 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 `@docs/specs/07-spec-min-dash-tui/07-spec-min-dash-tui.md` around lines 511 - 524, Update the shipped specification’s stale CLI crate references from minimal2 to minimal, including the Proof artifact 2 grep command and the other occurrences identified in the document, while preserving the existing verification targets and wording.
🧹 Nitpick comments (3)
crates/minimal-client/src/file_upload.rs (1)
1258-1321: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim the abandoned-approach narration in this test. Lines 1266-1287 walk through three rejected designs before describing the one actually implemented, which makes the test harder to read than the code it covers. Keeping just the final paragraph (pre-populate, truncate from a background task, tolerate either race outcome) says everything a reader needs.
🤖 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/minimal-client/src/file_upload.rs` around lines 1258 - 1321, Simplify add_file_errors_when_source_shrinks_during_read by removing the abandoned synchronization and interposition commentary before the implemented background-truncation approach. Retain only the concise explanation that the file is pre-populated, truncated after a short delay, and either race outcome is accepted while shrink errors must be detected.crates/minimald-rpc/src/lib.rs (1)
215-245: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePer-cell JSON is a heavy wire format for a 2s preview poll.
A default 24×80 screen serializes ~1920 objects with two
Option<String>colors each — tens of KB of JSON per tick, per focused session. Consider a compacter shape later (e.g. runs of identical style, or a style table plus indices) if preview cost shows up.🤖 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/minimald-rpc/src/lib.rs` around lines 215 - 245, Reduce the preview wire payload represented by ScreenCell, ScreenRow, and ScreenSnapshot instead of serializing a separate object for every terminal cell. Prefer a compact representation such as style runs or a shared style table with cell indices, while preserving the existing screen dimensions, cursor positions, character data, and color/style semantics.crates/minimal/src/main.rs (1)
49-118: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider
tracing-appenderfor the dash log writer.Every
tracingevent whilemin dashis running performs a synchronous blocking file write on whatever thread emits it — since the TUI drives a tight redraw/event loop, this can introduce jank on slow or network filesystems. There's also no rotation/size cap ondash.log, so it grows unbounded across sessions.
tracing-appender'snon_blockingwriter (optionally combined withrolling::RollingFileAppender) solves both: writes go through a background thread, and rotation/size limits are built in, eliminating the hand-rolledDashLogwrapper.Based on learnings, avoid calling
std::fsdirectly / performing blocking I/O on paths that run inside async execution;tracing-appender::non_blockingis the idiomatic fix here.♻️ Sketch using tracing-appender
- let file = { - let path = base.join("dash.log"); - let _ = std::fs::create_dir_all(&base); - std::fs::OpenOptions::new() - .create(true) - .append(true) - .open(path) - .map(std::sync::Arc::new) - .ok() - }; - let log = move || -> Box<dyn std::io::Write + Send> { - match &file { - Some(f) => Box::new(DashLog(f.clone())), - None => Box::new(std::io::sink()), - } - }; - registry - .with(fmt::layer().with_writer(log).with_ansi(false)) - .init(); + let _ = std::fs::create_dir_all(&base); + let appender = tracing_appender::rolling::never(&base, "dash.log"); + let (non_blocking, guard) = tracing_appender::non_blocking(appender); + // `guard` must be kept alive for the process lifetime — leak it here + // since dash owns the process until it exits. + std::mem::forget(guard); + registry + .with(fmt::layer().with_writer(non_blocking).with_ansi(false)) + .init();🤖 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/minimal/src/main.rs` around lines 49 - 118, Update the min dash logging setup in the `Command::Dash` branch to use `tracing-appender`’s non-blocking writer, preferably with a rolling or size-limited appender for `dash.log`. Preserve `--minimal-dir` handling and the sink fallback when the log directory or appender cannot be initialized, and remove the synchronous `DashLog` wrapper and direct per-event file writes.Source: Learnings
🤖 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/minimal-tui/src/app.rs`:
- Around line 383-404: Update the refresh flow around exec_effect's
Effect::Refresh and the Msg::Refreshed handler so provider identity remains
aligned with the runtime providers when individual rpc::refresh calls fail.
Preserve one stable identity per runtime provider, such as carrying the provider
index with optional refresh data, and rebuild model.providers without shifting
surviving entries; ensure SessionKey::provider continues resolving to the same
runtime connection for Attach, Create, and subsequent exec_effect operations.
In `@docs/reference/cli-min.md`:
- Line 57: Update the fenced code block in the CLI reference documentation to
include an appropriate language identifier, such as console or text, while
preserving its existing contents.
---
Outside diff comments:
In `@docs/specs/07-spec-min-dash-tui/07-spec-min-dash-tui.md`:
- Around line 511-524: Update the shipped specification’s stale CLI crate
references from minimal2 to minimal, including the Proof artifact 2 grep command
and the other occurrences identified in the document, while preserving the
existing verification targets and wording.
---
Nitpick comments:
In `@crates/minimal-client/src/file_upload.rs`:
- Around line 1258-1321: Simplify
add_file_errors_when_source_shrinks_during_read by removing the abandoned
synchronization and interposition commentary before the implemented
background-truncation approach. Retain only the concise explanation that the
file is pre-populated, truncated after a short delay, and either race outcome is
accepted while shrink errors must be detected.
In `@crates/minimal/src/main.rs`:
- Around line 49-118: Update the min dash logging setup in the `Command::Dash`
branch to use `tracing-appender`’s non-blocking writer, preferably with a
rolling or size-limited appender for `dash.log`. Preserve `--minimal-dir`
handling and the sink fallback when the log directory or appender cannot be
initialized, and remove the synchronous `DashLog` wrapper and direct per-event
file writes.
In `@crates/minimald-rpc/src/lib.rs`:
- Around line 215-245: Reduce the preview wire payload represented by
ScreenCell, ScreenRow, and ScreenSnapshot instead of serializing a separate
object for every terminal cell. Prefer a compact representation such as style
runs or a shared style table with cell indices, while preserving the existing
screen dimensions, cursor positions, character data, and color/style semantics.
🪄 Autofix (Beta)
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: 0414b650-5c2e-4436-b537-8fe7fc9b3ed1
⛔ Files ignored due to path filters (7)
Cargo.lockis excluded by!**/*.lockcrates/minimal-tui/tests/snapshots/snapshots__detail_pane_with_policy.snapis excluded by!**/*.snapcrates/minimal-tui/tests/snapshots/snapshots__empty_list.snapis excluded by!**/*.snapcrates/minimal-tui/tests/snapshots/snapshots__filtered.snapis excluded by!**/*.snapcrates/minimal-tui/tests/snapshots/snapshots__preview_section_with_screen_snapshot.snapis excluded by!**/*.snapcrates/minimal-tui/tests/snapshots/snapshots__single_provider.snapis excluded by!**/*.snapcrates/minimal-tui/tests/snapshots/snapshots__two_providers.snapis excluded by!**/*.snap
📒 Files selected for processing (25)
AGENTS.mdCargo.tomlcrates/minimal-client/Cargo.tomlcrates/minimal-client/src/attach.rscrates/minimal-client/src/file_upload.rscrates/minimal-client/src/lib.rscrates/minimal-tui/Cargo.tomlcrates/minimal-tui/src/app.rscrates/minimal-tui/src/event.rscrates/minimal-tui/src/filter.rscrates/minimal-tui/src/lib.rscrates/minimal-tui/src/render.rscrates/minimal-tui/src/rpc.rscrates/minimal-tui/src/state.rscrates/minimal-tui/tests/snapshots.rscrates/minimal/Cargo.tomlcrates/minimal/src/lib.rscrates/minimal/src/main.rscrates/minimald-rpc/src/lib.rscrates/minimald/src/rpc.rscrates/minimald/src/session.rscrates/minimald/src/session_host.rscrates/minimald/src/sessions.rsdocs/reference/cli-min.mddocs/specs/07-spec-min-dash-tui/07-spec-min-dash-tui.md
Harden the dash TUI against the issues CodeRabbit flagged: - SessionKey::provider is now a label string, not a list index, so a partial refresh failure can't re-index keys onto the wrong daemon. - All oneshot RPCs are bounded by a timeout so a wedged daemon fails the call instead of freezing the UI loop. - Providers that drop mid-run stay in the sidebar (reachable=false) and are re-discovered periodically so they can rejoin. - The sidebar scrolls to keep the cursor visible. - wire_color no longer panics on a 6-byte non-ASCII color payload. - Bare-key bindings require KeyModifiers::NONE so chords like Ctrl-D don't trigger the destroy prompt. - shorten_home is path-boundary aware so /home/user2 isn't shortened against a /home/u home. - Doc fixes: code fence language in cli-min.md, stale minimal2 crate references in the shipped spec.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/specs/07-spec-min-dash-tui/07-spec-min-dash-tui.md (1)
33-38: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the stale attachment non-goal.
The specification now documents in-place SSH attachment, but
N1still says attachment is unsupported and future work. UpdateN1so the specification has one attachment contract.🤖 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 `@docs/specs/07-spec-min-dash-tui/07-spec-min-dash-tui.md` around lines 33 - 38, Update the N1 non-goal in the specification to remove the statement that SSH attachment is unsupported or deferred, aligning it with the documented in-place SSH attachment behavior. Preserve the remaining N1 scope and avoid introducing a second attachment contract.
🧹 Nitpick comments (3)
crates/minimal-tui/src/render.rs (1)
734-743: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard the test against an unset home directory.
dirs::home_dir().unwrap()panics when the environment provides no home directory. The panic message then hides the real cause. Return early instead, so the test skips rather than fails in a minimal container.♻️ Sketch
- let home = dirs::home_dir().unwrap(); - let home = home.to_str().unwrap(); + let Some(home) = dirs::home_dir() else { + return; + }; + let Some(home) = home.to_str() else { + return; + };🤖 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/minimal-tui/src/render.rs` around lines 734 - 743, Update the shorten_home_only_shortens_at_a_path_boundary test to handle dirs::home_dir() returning None by exiting the test early, while preserving the existing assertions when a home directory is available.crates/minimal-tui/src/rpc.rs (2)
269-276: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMove the blocking filesystem walk off the async task.
resolve_upload_rootcallsmfile::File::from_dir_recursive, which reads files while walking up ancestor directories.is_vcs_rootalso stats paths. Both run directly inside this async block. A slow or wedged filesystem (a stalled network mount) blocks the runtime worker. Run both intokio::task::spawn_blocking.Based on learnings: in this Rust repo, avoid blocking filesystem work in async contexts; run filesystem traversal in a blocking thread via
tokio::task::spawn_blocking.♻️ Sketch
- let upload_root = resolve_upload_root(project_path.as_utf8_path())?; - if !minimal_client::file_upload::is_vcs_root(upload_root.as_std_path()) { + let dir = project_path.as_utf8_path().to_path_buf(); + let upload_root = tokio::task::spawn_blocking(move || resolve_upload_root(&dir)) + .await + .context("resolving the upload root")??; + let root = upload_root.clone(); + let is_repo = + tokio::task::spawn_blocking(move || minimal_client::file_upload::is_vcs_root(root.as_std_path())) + .await + .context("checking the repository root")?; + if !is_repo {🤖 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/minimal-tui/src/rpc.rs` around lines 269 - 276, Move the filesystem operations in the async flow around resolve_upload_root and is_vcs_root into tokio::task::spawn_blocking closures, await their JoinHandles, and propagate both task and operation errors while preserving the existing repository-root validation and bail message.Source: Learnings
322-341: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider hosting one
resolve_upload_rootin the shared client crate.This function and its four tests duplicate the CLI's
resolve_upload_root(crates/minimal/src/lib.rs, near lines 1358 and 2773-2822). Both must stay identical, because a divergence changes which directory tree gets uploaded.minimal_client::file_uploadalready holdsis_vcs_rootand is a dependency of both crates, so it is a natural home. Defer this if the layering makes the move awkward.🤖 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/minimal-tui/src/rpc.rs` around lines 322 - 341, Move the shared `resolve_upload_root` implementation and its four tests from the CLI and minimal-TUI locations into the common client crate, colocated with `minimal_client::file_upload` and its `is_vcs_root` helper. Update both callers to use the shared function, remove the duplicated implementations and tests, and preserve identical success, fallback, and error-propagation behavior.
🤖 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/minimal-client/src/lib.rs`:
- Around line 307-309: Update handle_channel and oneshot_rpc so mutating RPC
timeouts cannot commit after the client gives up: close the underlying
connection before retrying, ensuring the daemon handler is cancelled. Apply this
to CreateSession, ConfigureLoadout, and FinalizeSession while preserving retries
only after teardown has completed.
In `@crates/minimal-tui/src/app.rs`:
- Around line 696-719: Preserve the create form when provider validation fails
in the Enter/CreateField::Network branch: do not leave model.action cleared on
the no-provider and unreachable-provider early-return paths. Restore or retain
the existing action, including the typed name and path, before each return while
still updating model.status with the relevant error.
In `@docs/specs/07-spec-min-dash-tui/07-spec-min-dash-tui.md`:
- Around line 40-42: Update the architecture paragraph to reference the
extracted shared minimal-client transport instead of the outdated client.rs
path, while preserving the existing RPC transport description.
- Line 534: Update the proof command in the specification to inspect
crates/minimal/src/lib.rs, where Command::Dash is declared and dispatched, or
replace it with an equivalent compile/test check; do not continue checking
main.rs for this CLI declaration.
---
Outside diff comments:
In `@docs/specs/07-spec-min-dash-tui/07-spec-min-dash-tui.md`:
- Around line 33-38: Update the N1 non-goal in the specification to remove the
statement that SSH attachment is unsupported or deferred, aligning it with the
documented in-place SSH attachment behavior. Preserve the remaining N1 scope and
avoid introducing a second attachment contract.
---
Nitpick comments:
In `@crates/minimal-tui/src/render.rs`:
- Around line 734-743: Update the shorten_home_only_shortens_at_a_path_boundary
test to handle dirs::home_dir() returning None by exiting the test early, while
preserving the existing assertions when a home directory is available.
In `@crates/minimal-tui/src/rpc.rs`:
- Around line 269-276: Move the filesystem operations in the async flow around
resolve_upload_root and is_vcs_root into tokio::task::spawn_blocking closures,
await their JoinHandles, and propagate both task and operation errors while
preserving the existing repository-root validation and bail message.
- Around line 322-341: Move the shared `resolve_upload_root` implementation and
its four tests from the CLI and minimal-TUI locations into the common client
crate, colocated with `minimal_client::file_upload` and its `is_vcs_root`
helper. Update both callers to use the shared function, remove the duplicated
implementations and tests, and preserve identical success, fallback, and
error-propagation behavior.
🪄 Autofix (Beta)
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: fd1e1778-895b-46d4-a1be-6303b147dae8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
crates/minimal-client/src/lib.rscrates/minimal-tui/Cargo.tomlcrates/minimal-tui/src/app.rscrates/minimal-tui/src/render.rscrates/minimal-tui/src/rpc.rscrates/minimal-tui/tests/snapshots.rscrates/minimal/src/lib.rscrates/minimald/src/session_host.rsdocs/reference/cli-min.mddocs/specs/07-spec-min-dash-tui/07-spec-min-dash-tui.md
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/minimal-tui/Cargo.toml
- crates/minimald/src/session_host.rs
- crates/minimal/src/lib.rs
Harden the dash TUI against the issues CodeRabbit flagged: - SessionKey::provider is now a label string, not a list index, so a partial refresh failure can't re-index keys onto the wrong daemon. - All oneshot RPCs are bounded by a timeout so a wedged daemon fails the call instead of freezing the UI loop. - Providers that drop mid-run stay in the sidebar (reachable=false) and are re-discovered periodically so they can rejoin. - The sidebar scrolls to keep the cursor visible. - wire_color no longer panics on a 6-byte non-ASCII color payload. - Bare-key bindings require KeyModifiers::NONE so chords like Ctrl-D don't trigger the destroy prompt. - shorten_home is path-boundary aware so /home/user2 isn't shortened against a /home/u home. - Doc fixes: code fence language in cli-min.md, stale minimal2 crate references in the shipped spec.
Pick up the review comments the first feedback commit missed: update stale "Preview tab" wording, fix the spec's transport reference and proof-command path, reword the N1 non-goal now that in-place attach ships, keep the create form open on provider-validation errors, guard the shorten_home test against an unset home directory, cover the host_key_opts fallback branch, and move the blocking upload-root walk into spawn_blocking.
7ec3033 to
0037575
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/specs/07-spec-min-dash-tui/07-spec-min-dash-tui.md (1)
151-152: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the refresh cadence with the detailed requirements.
This overview says each refresh tick re-fetches the focused session's detail and screen. R4.1 specifies one
GetSessionRecordandGetSessionPolicycall per focus change, while R5.4 specifiesGetSessionScreenon each tick. State these two cadences separately.🤖 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 `@docs/specs/07-spec-min-dash-tui/07-spec-min-dash-tui.md` around lines 151 - 152, Update the refresh behavior description near Msg::Tick to separate the cadences: fetch ListSessions and the focused session’s screen on each 1–2s tick, while fetching the focused session’s record and policy once per focus change. Align the wording with the R4.1 and R5.4 requirements.
🧹 Nitpick comments (1)
crates/minimal-client/src/file_upload.rs (1)
1342-1405: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim the abandoned-approach comments and make the shrink test assert something.
Lines 1354-1371 record several approaches that were considered and dropped. They describe code that does not exist, so a later reader must reconstruct which approach the test actually uses.
The test is also non-deterministic. If the truncate task lands after
add_filefinishes its read, theOk(())arm asserts nothing and the shrink-detection branch at lines 445-452 is never exercised. The test then passes without covering the behavior it names.Replace the exploratory comments with one sentence describing the race the test drives. To make the assertion unconditional, add a second archive-level test that drives the same branch deterministically — for example, hold the source open, truncate it, and then call
add_fileon a path whose stat size and readable size differ.🤖 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/minimal-client/src/file_upload.rs` around lines 1342 - 1405, Trim the exploratory comments in add_file_errors_when_source_shrinks_during_read to one sentence describing the intended truncation race. Remove the no-op Ok(()) assertion path and add a separate deterministic archive-level test that creates a stat/read-size mismatch, then calls TarZstArchive::add_file and unconditionally asserts the error contains “shrank during upload”.
🤖 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/minimal-tui/src/rpc.rs`:
- Around line 87-88: Update the socket metadata checks in both discover and
connect_missing so they do not call Path::exists() directly on the async UI
path. Perform the existence check through a bounded tokio::task::spawn_blocking
task or equivalent bounded asynchronous filesystem operation, ensuring
CONNECT_TIMEOUT covers any filesystem delay and preserving the existing continue
behavior for missing sockets.
In `@docs/specs/07-spec-min-dash-tui/07-spec-min-dash-tui.md`:
- Line 537: Update the pinning proof near the Cargo.toml dependency check so it
verifies an exact ratatui version constraint or validates the resolved version
in Cargo.lock; alternatively, revise the statement to claim only that the
dependency is declared.
- Around line 62-63: Update the F1 section in the specification to match the
implemented in-place attach flow: suspend the TUI, attach via SSH, and resume
after detachment. Remove outdated future-work language and any claim that
CommandExt::exec() replaces the process, leaving a single consistent attach
contract.
- Line 46: Update the fenced code block at the affected terminal layout in
07-spec-min-dash-tui.md to specify the text language tag, changing the opening
fence to use text while preserving the block contents.
---
Outside diff comments:
In `@docs/specs/07-spec-min-dash-tui/07-spec-min-dash-tui.md`:
- Around line 151-152: Update the refresh behavior description near Msg::Tick to
separate the cadences: fetch ListSessions and the focused session’s screen on
each 1–2s tick, while fetching the focused session’s record and policy once per
focus change. Align the wording with the R4.1 and R5.4 requirements.
---
Nitpick comments:
In `@crates/minimal-client/src/file_upload.rs`:
- Around line 1342-1405: Trim the exploratory comments in
add_file_errors_when_source_shrinks_during_read to one sentence describing the
intended truncation race. Remove the no-op Ok(()) assertion path and add a
separate deterministic archive-level test that creates a stat/read-size
mismatch, then calls TarZstArchive::add_file and unconditionally asserts the
error contains “shrank during upload”.
🪄 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: 0be238c7-d26e-402e-b44e-9a459b3e72e2
⛔ Files ignored due to path filters (7)
Cargo.lockis excluded by!**/*.lockcrates/minimal-tui/tests/snapshots/snapshots__detail_pane_with_policy.snapis excluded by!**/*.snapcrates/minimal-tui/tests/snapshots/snapshots__empty_list.snapis excluded by!**/*.snapcrates/minimal-tui/tests/snapshots/snapshots__filtered.snapis excluded by!**/*.snapcrates/minimal-tui/tests/snapshots/snapshots__preview_section_with_screen_snapshot.snapis excluded by!**/*.snapcrates/minimal-tui/tests/snapshots/snapshots__single_provider.snapis excluded by!**/*.snapcrates/minimal-tui/tests/snapshots/snapshots__two_providers.snapis excluded by!**/*.snap
📒 Files selected for processing (25)
AGENTS.mdCargo.tomlcrates/minimal-client/Cargo.tomlcrates/minimal-client/src/attach.rscrates/minimal-client/src/file_upload.rscrates/minimal-client/src/lib.rscrates/minimal-tui/Cargo.tomlcrates/minimal-tui/src/app.rscrates/minimal-tui/src/event.rscrates/minimal-tui/src/filter.rscrates/minimal-tui/src/lib.rscrates/minimal-tui/src/render.rscrates/minimal-tui/src/rpc.rscrates/minimal-tui/src/state.rscrates/minimal-tui/tests/snapshots.rscrates/minimal/Cargo.tomlcrates/minimal/src/lib.rscrates/minimal/src/main.rscrates/minimald-rpc/src/lib.rscrates/minimald/src/rpc.rscrates/minimald/src/session.rscrates/minimald/src/session_host.rscrates/minimald/src/sessions.rsdocs/reference/cli-min.mddocs/specs/07-spec-min-dash-tui/07-spec-min-dash-tui.md
🚧 Files skipped from review as they are similar to previous changes (19)
- crates/minimal-tui/src/event.rs
- crates/minimal/Cargo.toml
- crates/minimal-tui/src/lib.rs
- crates/minimal-tui/Cargo.toml
- AGENTS.md
- crates/minimald/src/session.rs
- crates/minimal-tui/tests/snapshots.rs
- crates/minimald-rpc/src/lib.rs
- crates/minimal/src/main.rs
- crates/minimal-tui/src/filter.rs
- crates/minimal-tui/src/state.rs
- crates/minimald/src/sessions.rs
- docs/reference/cli-min.md
- crates/minimal-client/Cargo.toml
- crates/minimal-client/src/attach.rs
- crates/minimal-tui/src/render.rs
- crates/minimald/src/rpc.rs
- crates/minimald/src/session_host.rs
- crates/minimal-tui/src/app.rs
| if !sock.exists() { | ||
| continue; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- rpc.rs relevant sections ---'
sed -n '1,135p' crates/minimal-tui/src/rpc.rs
printf '%s\n' '--- app.rs connect_missing call sites ---'
rg -n -C 8 'connect_missing|CONNECT_TIMEOUT' crates/minimal-tui/src
printf '%s\n' '--- async filesystem and timeout dependencies ---'
rg -n -C 3 'Path::exists|metadata\(|spawn_blocking|tokio::time::timeout|oneshot_rpc' crates/minimal-tui/src crates/minimal-tui/Cargo.tomlRepository: gominimal/minimal
Length of output: 14363
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
p = Path("crates/minimal-tui/src/rpc.rs")
lines = p.read_text().splitlines()
for start, end in [(70, 122), (220, 315)]:
print(f"--- {p}:{start}-{end} ---")
for n in range(start, min(end, len(lines)) + 1):
print(f"{n:4}: {lines[n-1]}")
PY
printf '%s\n' '--- repository test recipes ---'
if [ -f justfile ]; then rg -n -C 3 '^(ci|e2e|test-vm|test-cross):' justfile; fi
printf '%s\n' '--- Rust standards ---'
if [ -f docs/rust-coding-standards.md ]; then sed -n '1,220p' docs/rust-coding-standards.md; fiRepository: gominimal/minimal
Length of output: 14133
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- async function declarations in rpc.rs ---'
rg -n -C 4 'async fn|fn connect_missing|fn activate' crates/minimal-tui/src/rpc.rs
printf '%s\n' '--- socket path construction and caller flow ---'
rg -n -C 10 'activate\(|connect_missing\(|sock' crates/minimal-tui/src/app.rs crates/minimal-tui/src/rpc.rs
printf '%s\n' '--- exact tracked-file status ---'
git status --shortRepository: gominimal/minimal
Length of output: 23552
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
for name in ("crates/minimal-tui/src/rpc.rs", "crates/minimal-tui/src/app.rs"):
p = Path(name)
print(f"--- {name} ---")
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if any(x in line for x in ("connect_missing", "exec_effect", "Path::exists", "CONNECT_TIMEOUT", "spawn_blocking")):
lo, hi = max(1, i-6), min(len(lines), i+10)
for n in range(lo, hi+1):
print(f"{n:4}: {lines[n-1]}")
print()
PYRepository: gominimal/minimal
Length of output: 8805
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- TUI runtime and startup discovery ---'
rg -n -C 8 'discover\(|Runtime|Builder|current_thread|multi_thread|run\(|tokio::main|#[[]tokio' crates/minimal-tui/src crates/minimal-tui/Cargo.toml
printf '%s\n' '--- refresh scheduling and redraw loop ---'
rg -n -C 8 'Effect::Refresh|Msg::Refresh|REDISCOVERY_INTERVAL|draw\(|tick|interval' crates/minimal-tui/src/app.rsRepository: gominimal/minimal
Length of output: 428
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- TUI runtime and startup discovery ---'
rg -n -F -C 8 'discover(' crates/minimal-tui/src crates/minimal-tui/Cargo.toml
rg -n -C 8 'Runtime|Builder|current_thread|multi_thread|tokio::main' crates/minimal-tui/src crates/minimal-tui/Cargo.toml || true
printf '%s\n' '--- refresh scheduling and redraw loop ---'
rg -n -C 8 'Effect::Refresh|Msg::Refresh|REDISCOVERY_INTERVAL|draw(|tick|interval' crates/minimal-tui/src/app.rsRepository: gominimal/minimal
Length of output: 2923
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- runtime entry points ---'
rg -n -F -C 6 '#[tokio::main]' crates/minimal-tui/src crates/minimal-tui/Cargo.toml || true
rg -n -C 6 'Runtime|Builder|current_thread|multi_thread' crates/minimal-tui/src crates/minimal-tui/Cargo.toml || true
printf '%s\n' '--- refresh scheduling and redraw loop ---'
for needle in 'Effect::Refresh' 'Msg::Refresh' 'REDISCOVERY_INTERVAL' 'draw(' 'tick' 'interval'; do
echo "--- $needle ---"
rg -n -F -C 5 "$needle" crates/minimal-tui/src/app.rs || true
doneRepository: gominimal/minimal
Length of output: 12840
Move socket metadata checks off the async UI path.
Path::exists() runs before CONNECT_TIMEOUT in both discover and connect_missing. A stalled filesystem can block the UI loop before the connection timeout starts. Use a bounded tokio::task::spawn_blocking task or an equivalent bounded asynchronous filesystem operation.
🤖 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/minimal-tui/src/rpc.rs` around lines 87 - 88, Update the socket
metadata checks in both discover and connect_missing so they do not call
Path::exists() directly on the async UI path. Perform the existence check
through a bounded tokio::task::spawn_blocking task or equivalent bounded
asynchronous filesystem operation, ensuring CONNECT_TIMEOUT covers any
filesystem delay and preserving the existing continue behavior for missing
sockets.
Source: Learnings
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum GetSessionScreenRequest { | ||
| Id(SessionId), |
There was a problem hiding this comment.
Prefer to just have one predicate (session id) to lookup the screen buffer, unless lookup screen by name is needed for some user-facing feature
| | AbortSession::NAME | ||
| | GetSessionPolicy::NAME | ||
| | SessionDelta::NAME | ||
| | minimald_rpc::GetSessionScreen::NAME |
There was a problem hiding this comment.
Drop minimald_rpc and add GetSessionScreen to our big ol chonky import at the top
| AbortSession::NAME => serve!(serve_abort_session(s, channel)), | ||
| GetSessionPolicy::NAME => serve!(serve_get_session_policy(s, channel)), | ||
| SessionDelta::NAME => serve!(serve_session_delta(s, channel)), | ||
| minimald_rpc::GetSessionScreen::NAME => serve!(serve_get_session_screen(s, channel)), |
Clients can now read a session's visible terminal grid (structured cells with colors and attributes) without attaching, so consumers like a dashboard can preview a live session without triggering a PTY resize or opening an I/O relay. The lookup goes through the session manager's running map and never starts a stopped actor; a session with no live host answers with a soft "session is not active" error.
min dash opens an interactive dashboard (new minimal-tui crate, ratatui + crossterm, Elm-style model/update/view) listing sessions across the host and VM providers with fuzzy filtering, an Info/Policy/Preview detail pane, and cursor memory in dash-state.json. Preview shows a read-only snapshot of a session's live terminal via GetSessionScreen; enter attaches in place (suspend → ssh → resume on ctrl-w detach); d/r/n destroy, rename, and create sessions, with create running the full activate flow (upload, compose, finalize) on a background task so created sessions come up Active and survive daemon restarts. The SSH client transport moves from the minimal crate into a shared minimal-client crate (including the attach command builder and a progress-free workspace upload) so the CLI and TUI drive the daemon identically. Dash logs to dash.log instead of the terminal. Ships docs/specs/07-spec-min-dash-tui.
Info, Policy, and the live Preview now render as one vertical stack in the detail pane, so the focused session's screen is always visible without tab switching. The Preview refreshes on every tick regardless of tab focus.
Session rows summed to one column over the pane's inner width, so the terminal truncated the last cell — exactly where the activity indicator (spinner / ○ waiting / ● bell) sat. Rows are now sized to the column, and render-level tests pin the spinner and bell glyphs in place.
Review follow-ups: the Policy section's height and content now come from one policy_lines() so they can't drift, Rename and Create share one line_edit() helper, sidebar math counts display width (via unicode-width) so CJK/emoji session names align, an empty filter returns None instead of a sentinel match, and the screen preview refreshes on the 2s tick rather than firing a GetSessionScreen RPC per j/k keystroke.
The dash logger's MakeWriter closure ran create_dir_all plus a file open on every write; the file is now opened once at startup and shared through an Arc-backed writer.
The spec still described a tabbed detail pane and Tab/Shift+Tab cycling; the implementation stacks Info, Policy, and Preview vertically. Also notes that the Preview section refreshes on the tick only.
Harden the dash TUI against the issues CodeRabbit flagged: - SessionKey::provider is now a label string, not a list index, so a partial refresh failure can't re-index keys onto the wrong daemon. - All oneshot RPCs are bounded by a timeout so a wedged daemon fails the call instead of freezing the UI loop. - Providers that drop mid-run stay in the sidebar (reachable=false) and are re-discovered periodically so they can rejoin. - The sidebar scrolls to keep the cursor visible. - wire_color no longer panics on a 6-byte non-ASCII color payload. - Bare-key bindings require KeyModifiers::NONE so chords like Ctrl-D don't trigger the destroy prompt. - shorten_home is path-boundary aware so /home/user2 isn't shortened against a /home/u home. - Doc fixes: code fence language in cli-min.md, stale minimal2 crate references in the shipped spec.
Pick up the review comments the first feedback commit missed: update stale "Preview tab" wording, fix the spec's transport reference and proof-command path, reword the N1 non-goal now that in-place attach ships, keep the create form open on provider-validation errors, guard the shorten_home test against an unset home directory, cover the host_key_opts fallback branch, and move the blocking upload-root walk into spawn_blocking.
0037575 to
54ccbd7
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/minimal/src/lib.rs (1)
1187-1193: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftMove filesystem reads off the async task.
cmd_dashsynchronously reads config, policy, and loadout files before it starts the TUI. A stalled filesystem blocks the async runtime worker. Run this composition intokio::task::spawn_blockingand await its result beforeminimal_tui::run.Based on learnings: avoid blocking filesystem work in async contexts.
🤖 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/minimal/src/lib.rs` around lines 1187 - 1193, Update cmd_dash so the config, user-policy, and loadout composition block is executed inside tokio::task::spawn_blocking, then await and propagate both the blocking task and composition errors before calling minimal_tui::run. Keep the existing loadouts::compose_user_contribution inputs and behavior unchanged.Source: Learnings
🤖 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/minimal-tui/src/rpc.rs`:
- Around line 323-327: Update the cleanup path around the flow error handling to
use a bounded reconnect client for AbortSession rather than discarding the
existing client result. Check both transport and abort response failures, and
when cleanup fails return an error that includes the session ID and cleanup
error; otherwise preserve returning the original flow error.
---
Nitpick comments:
In `@crates/minimal/src/lib.rs`:
- Around line 1187-1193: Update cmd_dash so the config, user-policy, and loadout
composition block is executed inside tokio::task::spawn_blocking, then await and
propagate both the blocking task and composition errors before calling
minimal_tui::run. Keep the existing loadouts::compose_user_contribution inputs
and behavior unchanged.
🪄 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: dff7f129-732b-41b3-9660-79adb34b2dcf
⛔ Files ignored due to path filters (7)
Cargo.lockis excluded by!**/*.lockcrates/minimal-tui/tests/snapshots/snapshots__detail_pane_with_policy.snapis excluded by!**/*.snapcrates/minimal-tui/tests/snapshots/snapshots__empty_list.snapis excluded by!**/*.snapcrates/minimal-tui/tests/snapshots/snapshots__filtered.snapis excluded by!**/*.snapcrates/minimal-tui/tests/snapshots/snapshots__preview_section_with_screen_snapshot.snapis excluded by!**/*.snapcrates/minimal-tui/tests/snapshots/snapshots__single_provider.snapis excluded by!**/*.snapcrates/minimal-tui/tests/snapshots/snapshots__two_providers.snapis excluded by!**/*.snap
📒 Files selected for processing (25)
AGENTS.mdCargo.tomlcrates/minimal-client/Cargo.tomlcrates/minimal-client/src/attach.rscrates/minimal-client/src/file_upload.rscrates/minimal-client/src/lib.rscrates/minimal-tui/Cargo.tomlcrates/minimal-tui/src/app.rscrates/minimal-tui/src/event.rscrates/minimal-tui/src/filter.rscrates/minimal-tui/src/lib.rscrates/minimal-tui/src/render.rscrates/minimal-tui/src/rpc.rscrates/minimal-tui/src/state.rscrates/minimal-tui/tests/snapshots.rscrates/minimal/Cargo.tomlcrates/minimal/src/lib.rscrates/minimal/src/main.rscrates/minimald-rpc/src/lib.rscrates/minimald/src/rpc.rscrates/minimald/src/session.rscrates/minimald/src/session_host.rscrates/minimald/src/sessions.rsdocs/reference/cli-min.mddocs/specs/07-spec-min-dash-tui/07-spec-min-dash-tui.md
🚧 Files skipped from review as they are similar to previous changes (22)
- crates/minimal-client/Cargo.toml
- crates/minimal-tui/src/event.rs
- crates/minimal-tui/src/lib.rs
- crates/minimald/src/sessions.rs
- AGENTS.md
- crates/minimal-tui/tests/snapshots.rs
- crates/minimald-rpc/src/lib.rs
- crates/minimal-tui/Cargo.toml
- crates/minimald/src/rpc.rs
- docs/reference/cli-min.md
- crates/minimal-tui/src/render.rs
- Cargo.toml
- crates/minimal/Cargo.toml
- crates/minimald/src/session_host.rs
- crates/minimal-tui/src/state.rs
- crates/minimal/src/main.rs
- crates/minimald/src/session.rs
- crates/minimal-client/src/lib.rs
- crates/minimal-tui/src/filter.rs
- crates/minimal-tui/src/app.rs
- crates/minimal-client/src/attach.rs
- crates/minimal-client/src/file_upload.rs
| if let Err(e) = flow { | ||
| let _ = client | ||
| .oneshot_rpc::<minimald_rpc::AbortSession>(minimald_rpc::AbortSessionRequest { id }) | ||
| .await; | ||
| return Err(e); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not discard AbortSession failures.
If upload or a later RPC fails after CreateSession, this code ignores an abort transport failure and returns only the original error. The pending record can remain on the daemon and retain its session name.
Reconnect with a bounded client for cleanup, check the abort response, and include the session ID and cleanup error in the returned failure if cleanup does not succeed.
🤖 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/minimal-tui/src/rpc.rs` around lines 323 - 327, Update the cleanup
path around the flow error handling to use a bounded reconnect client for
AbortSession rather than discarding the existing client result. Check both
transport and abort response failures, and when cleanup fails return an error
that includes the session ID and cleanup error; otherwise preserve returning the
original flow error.
Drop the Id/Name enum from GetSessionScreenRequest per reviewer feedback — the Name variant was test-only with no user-facing need. Move GetSessionScreen into the shared import block in minimald's rpc.rs instead of qualifying it inline.
bd4d4b7 to
5547fd5
Compare
Summary
Adds
min dash, a full-screen session manager TUI, plus the daemon support it needs:minimald, VMminvmd), grouped and collapsible, with fuzzy filtering against name, id, and project path.GetSessionScreenRPC gives read-only screen snapshots without attaching (no PTY resize, no I/O relay).minimal-clientcrate so the CLI and the TUI use one transport.min dashlogs to<state>/dash.logsince the TUI owns the terminal.Changes
crates/minimald-rpcGetSessionScreenwire types (snapshot, rows, cells, colors)crates/minimaldGetScreenmessage on theHostactor, RPC handler, session lookupcrates/minimal-clientcrates/minimal-tuicrates/minimaldashsubcommand, file logging while the TUI runsdocs/cli-minreference for the subcommandThe TUI core is a pure
update(model, msg) -> Vec<Effect>; side effects run in the tokio driver. This keeps the state transitions unit-testable and the rendering snapshot-testable with insta.Verification
just cigates green: rustfmt, clippy (-D warnings), cargo-deny, doctests.mctx env::tests::env_channel_add_session) fails identically onmain(it needs a guest rootfs artifact).minimal-tuicovered by unit tests for the update loop and insta snapshot tests for rendering.Summary by CodeRabbit
min dashfull-screen session manager for browsing, filtering, creating, renaming, attaching to, and destroying sessions.Note
Add
min dashinteractive session manager TUIminimal-client(shared SSH transport, attach helpers, file upload) andminimal-tui(ratatui/crossterm dashboard).min dashcommand launches a two-pane TUI showing sessions grouped by provider, with Info/Policy/Preview detail panels, fuzzy filtering, scrolling, and modal actions for create/rename/destroy.GetSessionScreenRPC and supporting wire types (ScreenSnapshot,ScreenCell,ScreenRow) so the TUI can display a live terminal preview without attaching.min dashare written todash.logunder the state directory instead of stdout/stderr to avoid corrupting the terminal UI.Macroscope summarized 5547fd5.