TUI shell: ask_user modal, pickers, autocomplete, zeroline skin - #126
Conversation
… zeroline skin Ports the interactive TUI shell from the reskin branch onto the agent-runtime branch (stacked: needs agent.AskUserRequest/AskUserResponse/AskUserQuestion + Options.OnAskUser/ContextWindow from that PR). Built as a per-file 3-way merge of internal/tui (preserving main's #113 session-wiring drift), then renamed zenline->zeroline everywhere (imports, identifiers, skin string, and zenline_view.go -> zeroline_view.go) since the rendering package merged as internal/zeroline. - ask_user: interactive questionnaire modal wired to agent.OnAskUser (answer channel), with transcript rows + session payloads; degrades gracefully when no interactive surface. - compaction: model.go sizes agent ContextWindow from the active model's registry context window (modelContextWindow); unknown models -> 0 (compaction off). - pickers (model/theme/effort/mode) + slash-command autocomplete overlay; zeroline skin rendering (boot/home/chat) behind Options.Skin. - checkpoint recording: before each mutating tool the run batches an EventSessionCheckpoint (in order) and flushes it at end-of-run and on cancel. This required exposing sessions.SnapshotForCheckpoint (the snapshot-only entry point) with an explicit orphan-blob safe-usage contract — most callers should use the atomic CaptureToolCheckpoint, but the TUI needs the event batched IN ORDER with the run's other session events. build/vet/-race/full-suite + GOOS=windows build green; 115 tui tests pass; no new deps. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Thanks for the TUI shell work. I ran go test ./..., go run ./cmd/zero-release build, and git diff --check at 952d161; all passed locally. I found one blocker in the newly wired /rewind path, so I am requesting changes before this stacks cleanly.
The PR is also stacked on agent-runtime, so it should be merged after that base branch is accepted/merged.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds interactive ask-user questionnaires, slash-command autocomplete, reusable pickers (model/mode/effort/theme), a "zeroline" UI skin and rendering, exported Store.SnapshotForCheckpoint with an orphan-vulnerability contract, ordered mutation checkpointing before tool execution, and cancellation-safe flushing of session events; includes extensive TUI and unit tests. ChangesInteractive TUI with Questionnaire, Autocomplete, and Pickers
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/tui/zeroline_view.go (1)
189-224: 💤 Low valueConsider applying De Morgan's law for clarity.
Line 209's negated conjunction could be simplified to an equivalent disjunction:
r.id == "" || !resultIDs[r.id], which reads more naturally.♻️ Optional simplification
- Running: !(r.id != "" && resultIDs[r.id]), + Running: r.id == "" || !resultIDs[r.id],🤖 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 `@internal/tui/zeroline_view.go` around lines 189 - 224, In zerolineRows(), simplify the Running expression by applying De Morgan: replace the negated conjunction used when building the row for rowToolCall (currently using !(r.id != "" && resultIDs[r.id])) with the equivalent disjunction r.id == "" || !resultIDs[r.id]; update the Running field in the zeroline.Row construction for the rowToolCall case to use that clearer expression (refer to function zerolineRows, the loop variable r, and the resultIDs map).internal/tui/ask_user_test.go (1)
124-127: ⚡ Quick winRemove the empty if block.
The linter (SA9003) flags this empty branch. The comment explains the behavior but doesn't assert anything. Remove the if statement and keep the comment standalone, or remove both if the behavior is sufficiently documented elsewhere.
♻️ Suggested cleanup
- if next.pending { - // cancelRun is the normal Esc path; here we only cancel the prompt, the - // run continues with the degraded answers. - } + // cancelRun is the normal Esc path; here we only cancel the prompt, the + // run continues with the degraded answers (pending stays true).🤖 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 `@internal/tui/ask_user_test.go` around lines 124 - 127, Remove the empty conditional that checks next.pending in internal/tui/ask_user_test.go: delete the "if next.pending { ... }" block flagged by SA9003 and either leave the explanatory comment as a standalone comment above the surrounding code or remove the comment entirely; ensure any mention of cancelRun remains in nearby comments/tests if still relevant so behavior is still documented.
🤖 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.
Nitpick comments:
In `@internal/tui/ask_user_test.go`:
- Around line 124-127: Remove the empty conditional that checks next.pending in
internal/tui/ask_user_test.go: delete the "if next.pending { ... }" block
flagged by SA9003 and either leave the explanatory comment as a standalone
comment above the surrounding code or remove the comment entirely; ensure any
mention of cancelRun remains in nearby comments/tests if still relevant so
behavior is still documented.
In `@internal/tui/zeroline_view.go`:
- Around line 189-224: In zerolineRows(), simplify the Running expression by
applying De Morgan: replace the negated conjunction used when building the row
for rowToolCall (currently using !(r.id != "" && resultIDs[r.id])) with the
equivalent disjunction r.id == "" || !resultIDs[r.id]; update the Running field
in the zeroline.Row construction for the rowToolCall case to use that clearer
expression (refer to function zerolineRows, the loop variable r, and the
resultIDs map).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ab6d5627-bd1c-45ee-a8df-59bc79ea16d7
📒 Files selected for processing (22)
internal/sessions/checkpoint.gointernal/tui/ask_user_test.gointernal/tui/autocomplete.gointernal/tui/autocomplete_test.gointernal/tui/command_center.gointernal/tui/commands.gointernal/tui/model.gointernal/tui/model_catalog.gointernal/tui/model_test.gointernal/tui/options.gointernal/tui/picker.gointernal/tui/picker_test.gointernal/tui/rendering.gointernal/tui/run.gointernal/tui/session.gointernal/tui/session_controls.gointernal/tui/session_controls_test.gointernal/tui/session_test.gointernal/tui/transcript.gointernal/tui/view.gointernal/tui/zeroline_view.gointernal/tui/zeroline_view_test.go
…r rewind handleRewindCommand applied the rewind to the store but returned m unchanged, so m.sessionEvents, m.activeSession, and the transcript still held the dropped events. The next prompt's sessionPrompt re-sent m.sessionEvents as ContextEvents, so a user could /rewind and still have the rewound-away tool calls/messages included in the next turn. After a successful ApplyRewind, reload the session metadata (store.Get) + events (ReadEvents) and rebuild the transcript from the truncated log — so the dropped events neither reach the agent nor stay on screen. Regression: TestRewindRefreshesInMemorySessionState (resume → rewind latest → assert sessionEvents shrink, transcript + next prompt no longer contain the post-checkpoint content). build/vet/-race/full-suite + GOOS=windows build green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@Vasanthdev2004 fixed the P1 — after |
|
✅ Action performedReview finished.
|
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-reviewed current head 066ee5c.
The previous /rewind blocker is fixed: after ApplyRewind, the TUI reloads session metadata/events and rebuilds the transcript, and the new regression test verifies rewound-away content is not re-sent in the next prompt.
Validation passed locally:
go test ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smokegit diff --check origin/agent-runtime...origin/pr-126
CI/CodeRabbit are green. Approved.
2 participants
TUI shell — ask_user modal, pickers, autocomplete, zeroline skin
Ports the interactive TUI shell from the reskin branch. Stacked on #125 (agent runtime) — it needs
agent.AskUserRequest/AskUserResponse/AskUserQuestion+Options.OnAskUser/ContextWindowfrom that PR, so this targets theagent-runtimebranch; I'll retarget it tomainonce #125 lands.Built as a per-file 3-way merge of
internal/tui(preserving main's #113 session-wiring drift —SessionID/Model/ReasoningEffort/Cwd), then a full zenline→zeroline rename (imports, identifiers, theSkinstring, andzenline_view.go→zeroline_view.go) since the rendering package merged earlier asinternal/zeroline.What's in it
agent.OnAskUservia an answer channel; transcript rows + session payloads; degrades gracefully with no interactive surface.modelContextWindow()sets the agent'sContextWindowfrom the active model's registry entry; unknown/custom models → 0 (compaction off).Options.Skin.EventSessionCheckpointin order and flushes it at end-of-run and on cancel.One cross-module change (called out)
The checkpoint flow needs the event batched in order with the run's other session events (recording it out-of-band reorders it, and breaks multi-call ordering). That requires the snapshot-only entry point, so I exposed
sessions.SnapshotForCheckpointwith an explicit orphan-blob safe-usage contract: most callers should use the atomicCaptureToolCheckpoint; this one is for the batch-in-order pattern where the caller (the TUI) guarantees prompt recording incl. on cancel.Testing
115 tui tests pass.
go build ./...,go vet,go test ./...,go test -race ./internal/tui/ ./internal/sessions/,GOOS=windows go build ./...all green. No new deps.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
/modeand/rewindcommands enhanced;/rewindreports restored/deleted/skipped filesBug Fixes