Builds the ratatui TUI: EngineHandle bridging the async engine to the render loop, chat viewport rendering with a markdown renderer, input handling, a permission modal wired to the real oneshot ask path, and a session picker. Adds TestBackend snapshot tests covering empty session, tool cards (running/completed/error), permission modal, session picker, and message rendering.
darman
requested review from claude-reviewer 2026-07-11 14:41:57 +02:00
Good structure: EngineHandle cleanly separates the non-blocking multi-turn engine from the render loop, the run-slot reservation has a nice comment about the insert-after-spawn race, the terminal guard restores on both drop and panic, and the TestBackend snapshot tests are a great choice for a TUI. Main issue is scroll behavior; a couple of smaller things.
Chat viewport never follows the tail (medium)
render_chat (render.rs:1483) applies .scroll((state.scroll_offset, 0)), but scroll_offset is only ever changed by manual scroll_up/scroll_down and reset to 0 on session load/new. Nothing advances it toward the bottom as content arrives. Consequences:
Once a transcript exceeds the chat height, streaming assistant output renders below the fold and is invisible until the user manually scrolls down.
Resuming a long session (set_session sets scroll_offset = 0) shows the top of the history, not the latest turn.
scroll_down (state.rs:2413) is saturating_add with no upper clamp, so the user can scroll into blank space past the end.
Typical fix: track the total rendered line count vs. viewport height, and when the user is "pinned to bottom" auto-set scroll_offset to max(0, total_lines - viewport) each frame; clamp manual scrolling to that max.
Persistent DB filename uses an unstable hash (low-medium)
db_path (harness-app/src/lib.rs:65) derives the sqlite filename from std::collections::hash_map::DefaultHasher. DefaultHasher's algorithm/output is explicitly not guaranteed stable across Rust releases, so a toolchain upgrade can change the hash → change the filename → silently orphan the user's existing session history under a new DB. Prefer a stable hash (fixed FNV/seahash/blake3) or just the sanitized path.
Minor
render(... &requests[0], area) (render.rs:1432) indexes directly. It's safe by the current invariant (Permission modal is cleared to None when the last request resolves), but requests.first() with a guard would be more robust against a future refactor.
Loading a session that has an in-flight run via the picker leaves state.running = false (spinner/abort hint stale), since running is only driven by RunStarted/RunFinished for the live session — set_session doesn't consult engine.is_running.
PartView::Reasoning re-renders markdown every frame (no cached_lines), unlike Text/Tool. Minor perf.
Enter always submits, so there's no way to insert a literal newline into a multi-line prompt (Up/Down navigate existing lines but nothing creates them). Consider Shift+Enter or Alt+Enter for newline.
— automated review (Claude)
## Review: M2 — TUI
Good structure: `EngineHandle` cleanly separates the non-blocking multi-turn engine from the render loop, the run-slot reservation has a nice comment about the insert-after-spawn race, the terminal guard restores on both drop and panic, and the TestBackend snapshot tests are a great choice for a TUI. Main issue is scroll behavior; a couple of smaller things.
### Chat viewport never follows the tail (medium)
`render_chat` (`render.rs:1483`) applies `.scroll((state.scroll_offset, 0))`, but `scroll_offset` is only ever changed by manual `scroll_up`/`scroll_down` and reset to `0` on session load/new. Nothing advances it toward the bottom as content arrives. Consequences:
- Once a transcript exceeds the chat height, streaming assistant output renders *below the fold* and is invisible until the user manually scrolls down.
- Resuming a long session (`set_session` sets `scroll_offset = 0`) shows the top of the history, not the latest turn.
- `scroll_down` (`state.rs:2413`) is `saturating_add` with no upper clamp, so the user can scroll into blank space past the end.
Typical fix: track the total rendered line count vs. viewport height, and when the user is "pinned to bottom" auto-set `scroll_offset` to `max(0, total_lines - viewport)` each frame; clamp manual scrolling to that max.
### Persistent DB filename uses an unstable hash (low-medium)
`db_path` (`harness-app/src/lib.rs:65`) derives the sqlite filename from `std::collections::hash_map::DefaultHasher`. `DefaultHasher`'s algorithm/output is explicitly not guaranteed stable across Rust releases, so a toolchain upgrade can change the hash → change the filename → silently orphan the user's existing session history under a new DB. Prefer a stable hash (fixed FNV/seahash/blake3) or just the sanitized path.
### Minor
- `render(... &requests[0], area)` (`render.rs:1432`) indexes directly. It's safe by the current invariant (Permission modal is cleared to `None` when the last request resolves), but `requests.first()` with a guard would be more robust against a future refactor.
- Loading a session that has an in-flight run via the picker leaves `state.running = false` (spinner/abort hint stale), since `running` is only driven by `RunStarted`/`RunFinished` for the live session — `set_session` doesn't consult `engine.is_running`.
- `PartView::Reasoning` re-renders markdown every frame (no `cached_lines`), unlike Text/Tool. Minor perf.
- `Enter` always submits, so there's no way to insert a literal newline into a multi-line prompt (Up/Down navigate existing lines but nothing creates them). Consider Shift+Enter or Alt+Enter for newline.
— automated review (Claude)
You are not authorized to merge this pull request.
This pull request can be merged automatically.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
claude-reviewer
EngineHandlebridging the async engine to the render loopTestBackendsnapshot tests (empty session, tool cards, permission modal, session picker, messages)Review: M2 — TUI
Good structure:
EngineHandlecleanly separates the non-blocking multi-turn engine from the render loop, the run-slot reservation has a nice comment about the insert-after-spawn race, the terminal guard restores on both drop and panic, and the TestBackend snapshot tests are a great choice for a TUI. Main issue is scroll behavior; a couple of smaller things.Chat viewport never follows the tail (medium)
render_chat(render.rs:1483) applies.scroll((state.scroll_offset, 0)), butscroll_offsetis only ever changed by manualscroll_up/scroll_downand reset to0on session load/new. Nothing advances it toward the bottom as content arrives. Consequences:set_sessionsetsscroll_offset = 0) shows the top of the history, not the latest turn.scroll_down(state.rs:2413) issaturating_addwith no upper clamp, so the user can scroll into blank space past the end.Typical fix: track the total rendered line count vs. viewport height, and when the user is "pinned to bottom" auto-set
scroll_offsettomax(0, total_lines - viewport)each frame; clamp manual scrolling to that max.Persistent DB filename uses an unstable hash (low-medium)
db_path(harness-app/src/lib.rs:65) derives the sqlite filename fromstd::collections::hash_map::DefaultHasher.DefaultHasher's algorithm/output is explicitly not guaranteed stable across Rust releases, so a toolchain upgrade can change the hash → change the filename → silently orphan the user's existing session history under a new DB. Prefer a stable hash (fixed FNV/seahash/blake3) or just the sanitized path.Minor
render(... &requests[0], area)(render.rs:1432) indexes directly. It's safe by the current invariant (Permission modal is cleared toNonewhen the last request resolves), butrequests.first()with a guard would be more robust against a future refactor.state.running = false(spinner/abort hint stale), sincerunningis only driven byRunStarted/RunFinishedfor the live session —set_sessiondoesn't consultengine.is_running.PartView::Reasoningre-renders markdown every frame (nocached_lines), unlike Text/Tool. Minor perf.Enteralways submits, so there's no way to insert a literal newline into a multi-line prompt (Up/Down navigate existing lines but nothing creates them). Consider Shift+Enter or Alt+Enter for newline.— automated review (Claude)
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.