TUI polish and fixes

[anandh8x]

Summary

• persist /model selections to the active provider in user config
• add config writer coverage for updating provider models
• add TUI coverage for model switch persistence across restarts

Tests

• GOCACHE=/tmp/zero-go-cache go test ./internal/config ./internal/tui
• git diff --check

Summary by CodeRabbit

• New Features

  • Model selections made via the model command are now automatically saved to your user configuration and persist across sessions.

• Bug Fixes

  • Improved alternate-screen transcript rendering with a fixed/pinned title bar.
  • Updated mouse targeting and transcript line selection/overlay positioning so interactions align with the on-screen layout while scrolling.

• Tests

  • Added/extended tests covering model persistence (including whitespace/error cases) and validating pinned-title behavior and mouse-based interaction accuracy.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: be9bd8c9-7dd4-4115-bc77-098e01377c1e

📥 Commits

Reviewing files that changed from the base of the PR and between 74e68bb and a149f0d.

📒 Files selected for processing (8)

• internal/tui/command_center.go
• internal/tui/model.go
• internal/tui/model_test.go
• internal/tui/mouse.go
• internal/tui/mouse_test.go
• internal/tui/scroll_test.go
• internal/tui/startup.go
• internal/tui/transcript_selection.go

🚧 Files skipped from review as they are similar to previous changes (7)

• internal/tui/startup.go
• internal/tui/scroll_test.go
• internal/tui/model_test.go
• internal/tui/mouse.go
• internal/tui/transcript_selection.go
• internal/tui/mouse_test.go
• internal/tui/command_center.go

---

Walkthrough

Adds SetProviderModel to the config writer to persist model selections to the user config file. Integrates this into the TUI's handleModelCommand via a new persistSelectedModel helper, with the model-switch response now reporting save success/failure. Concurrently refactors TUI transcript rendering to support a pinned title bar in alt-screen mode using a new frame-layout system that distributes terminal height across header, body, and footer. Updates mouse coordinate calculations and transcript selection to use this shared layout model. Tests verify persistence, coordinate calculations, and title bar pinning during scroll.

Changes

Config persistence for model selection

Layer / File(s)	Summary
SetProviderModel config writer and tests internal/config/writer.go, internal/config/writer_test.go	SetProviderModel trims/validates inputs, does case-insensitive provider lookup, updates Model in place, and writes back to disk. Two tests cover the whitespace-trimming happy path (credential preservation, no ActiveProvider change) and the error path (unknown provider, no file rewrite).
TUI persistSelectedModel wiring and test internal/tui/command_center.go, internal/tui/model_test.go	persistSelectedModel validates userConfigPath and profile fields before calling config.SetProviderModel. Wired into handleModelCommand after provider rebuild; the returned message now conditionally appends saved: user config or saved: no (...). TUI test verifies in-memory model update, config persistence, and transcript content.

Pinned title bar refactoring

Layer / File(s)	Summary
Frame layout foundation internal/tui/model.go	Introduces transcriptFrameLayout struct and scrollableTranscriptFrame(header, footer) helper that compute header/body/footer line distribution within terminal height. Refactors scrollableTranscriptView to accept explicit header, compute body-only scroll window, clamp chatScrollOffset within body region, overlay viewport onto body, and assemble final frame. Updates chatScrollMetrics to derive body height from frame layout.
Title bar predicates and alt-screen integration internal/tui/model.go	Adds titleBarInTranscriptBody() predicate (title bar is inline only before first header print) and pinnedTitleBar() helper (pinned only in alt-screen with positive height). Alt-screen transcriptView now passes pinned title bar as a separate header to scrollableTranscriptView. Updates comments to clarify title bar management.
Mouse coordinate calculations internal/tui/mouse.go	mouseOverComposer obtains footerLines from scrollableTranscriptFrame and computes clippedPrefix from frame geometry. Recalculates footerTop using frame header line count and body height. overlayMouseTop switches to scrollableTranscriptFrame for overlay centering instead of prior transcript/startup/available-height branching.
Transcript selection and viewport internal/tui/transcript_selection.go	transcriptLineAtMouse now interprets mouse Y relative to viewport start and header offset, rejecting out-of-bounds clicks. transcriptViewportStart refactored to return (start, available, headerLinesLen) via scrollableTranscriptFrame, enabling selection to account for pinned headers. transcriptBody switches from headerPrinted to titleBarInTranscriptBody() predicate.
Test updates and assertions internal/tui/mouse_test.go, internal/tui/scroll_test.go, internal/tui/startup.go	Adds firstTranscriptTextMouseY helper to compute correct on-screen Y, replacing hardcoded constants across all transcript selection, reasoning toggle, and streaming tests. Updates scroll test to set gitBranch and assert title bar remains pinned before and after scroll. Updates startup empty-state overlay to use titleBarInTranscriptBody() predicate.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• Gitlawb/zero#66: Both PRs modify handleModelCommand in internal/tui/command_center.go for model switching logic and response handling.
• Gitlawb/zero#63: Both PRs extend the TUI handleModelCommand path; main PR adds persistence to user config via config.SetProviderModel, while the retrieved PR introduced the initial in-session model-switching implementation.

Suggested reviewers

• Vasanthdev2004

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title "TUI polish and fixes" is vague and generic, using non-descriptive terms that don't clearly convey the main changes (model persistence, pinned title bar refactor, test coverage).	Consider a more specific title that highlights the primary change, such as "Persist model selections and refactor pinned title bar rendering" or "Add model persistence and improve TUI transcript layout consistency."

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/tui-polish-fixes

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

internal/tui/startup.go (1)

58-63: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Align empty-overlay render centering with frame-based mouse hit geometry.

overlayMouseTop now uses scrollableTranscriptFrame(...), but this function still centers with a fixed normalizedStartupHeight(...)-5 heuristic. When footer height changes, rendered overlay Y and click-hit Y can drift, causing row mis-selection.

Proposed fix

 func (m model) emptyStateWithOverlay(width int, overlay string) string {
@@
-	available := normalizedStartupHeight(m.height) - 5
-	if m.titleBarInTranscriptBody() {
-		available -= 2
-	}
+	available := normalizedStartupHeight(m.height) - 5
+	if m.altScreen && m.height > 0 {
+		frame := m.scrollableTranscriptFrame(m.pinnedTitleBar(width), m.footerView(width))
+		available = frame.bodyHeight
+	}
+	if m.titleBarInTranscriptBody() {
+		available -= 2
+	}
 	gap := maxInt(0, (available-len(lines))/2)
 	return strings.Repeat("\n", gap) + strings.Join(lines, "\n") + strings.Repeat("\n", gap)
 }

🤖 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/startup.go` around lines 58 - 63, The startup overlay centering
uses a fixed heuristic (normalizedStartupHeight(m.height) - 5) to calculate
available height, but this differs from the frame-based geometry used by
overlayMouseTop which uses scrollableTranscriptFrame(...). When footer height
changes, this causes the rendered overlay Y position and click-hit Y position to
drift, resulting in row mis-selection. Replace the hardcoded available height
calculation with a frame-based approach that derives the height from the same
scrollableTranscriptFrame(...) calculation used in overlayMouseTop, ensuring the
centering logic is consistent with the actual frame geometry used for mouse hit
detection.

🤖 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.

Outside diff comments:
In `@internal/tui/startup.go`:
- Around line 58-63: The startup overlay centering uses a fixed heuristic
(normalizedStartupHeight(m.height) - 5) to calculate available height, but this
differs from the frame-based geometry used by overlayMouseTop which uses
scrollableTranscriptFrame(...). When footer height changes, this causes the
rendered overlay Y position and click-hit Y position to drift, resulting in row
mis-selection. Replace the hardcoded available height calculation with a
frame-based approach that derives the height from the same
scrollableTranscriptFrame(...) calculation used in overlayMouseTop, ensuring the
centering logic is consistent with the actual frame geometry used for mouse hit
detection.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 72e366cc-9ddd-41a5-a471-99dac630eff7

📥 Commits

Reviewing files that changed from the base of the PR and between 0fdf31b and 74e68bb.

📒 Files selected for processing (6)

• internal/tui/model.go
• internal/tui/mouse.go
• internal/tui/mouse_test.go
• internal/tui/scroll_test.go
• internal/tui/startup.go
• internal/tui/transcript_selection.go

[github-actions]

Zero automated PR review

Verdict: No blockers found

Blockers

• None found.

Validation

• [pass] Diff hygiene: git diff --check
• [pass] Tests: go test ./...
• [pass] Build: go run ./cmd/zero-release build
• [pass] Smoke build: go run ./cmd/zero-release smoke

Scope

Head: a149f0d081df
Changed files (10): internal/config/writer.go, internal/config/writer_test.go, internal/tui/command_center.go, internal/tui/model.go, internal/tui/model_test.go, internal/tui/mouse.go, internal/tui/mouse_test.go, internal/tui/scroll_test.go, internal/tui/startup.go, internal/tui/transcript_selection.go

This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality.

[gnanam1990]

Review — TUI polish and fixes

Verdict: LGTM with a couple of minor nits. Focused and well-tested.

Strengths

• The pinned-title-bar refactor is correct where it matters. Render (scrollableTranscriptView) and all hit-testing paths (transcriptViewportStart, mouseOverComposer, overlayMouseTop) now derive geometry from one shared scrollableTranscriptFrame(pinnedTitleBar, footerView). Because render and mouse math use the same header/body/footer split, clicks map to the right line — that's the real fix, and the top offset is threaded consistently (bodyY := start + msg.Y - top). Frame clamping degrades sanely on tiny terminals (drops the header before starving the body).
• titleBarInTranscriptBody() (!altScreen && !headerPrinted) cleanly separates the inline-scrollback title from the pinned alt-screen title; startup.go/transcript_selection.go updated so it isn't double-counted.
• SetProviderModel mirrors the existing SetActiveProvider writer; persistSelectedModel no-ops gracefully and surfaces saved: user config / saved: no (err). Good coverage (update + credential preserved + unrelated provider untouched + unknown-provider-no-rewrite + the end-to-end /model persistence test).

Minor nits (non-blocking)

1. SetProviderModel has no permission-preservation test, unlike SetActiveProvider (TestSetActiveProviderTightensExistingConfigFilePermissions) — worth adding for consistency.
2. The "provider not in user config → saved: no (...)" branch is graceful but untested.
3. Read-modify-write without an atomicity guarantee — fine since it matches the other writers, but if writeConfigFile isn't temp+rename that's a pre-existing concern across all of them.

Heads-up: merge collision

This touches mouse.go / model.go / transcript_selection.go, which #217 also modifies and #222 rewrites for Bubble Tea v2. Expect conflicts — see the note on #222 re: merge order.

[gnanam1990]

Approving — clean, well-tested, and CI is green across all platforms. The viewport-math consolidation through a single scrollableTranscriptFrame is a nice improvement beyond the surface feature, and routing both /model <x> and the model picker through handleModelCommand means a selection persists either way.

A few non-blocking nits for a follow-up (or now — your call):

• Display vs. persisted value: the status line reports target.modelID while persistSelectedModel writes nextProfile.Model. They're equal in the covered path, so this isn't a known bug — but persisting target.modelID (or asserting the two match) removes any latent risk of saving a different value than was shown.
• Project-only configs: persistence targets only userConfigPath, so with just a project config the selection silently doesn't persist and no saved: line appears. Consider surfacing something like saved: no (no user config) so it isn't silently dropped.
• Test coverage: the happy path is well covered; the persistErr branch (saved: no (...)) and the empty-userConfigPath no-op aren't. A small test for each would lock in the behavior.
• Readability: the short-terminal clamping in scrollableTranscriptFrame (drop the header first, then re-clamp the footer) is correct and the priority is sensible — a one-line comment on the precedence (composer wins, header sacrificed) would help future readers.

Optional: a quick manual alt-screen smoke — scroll (title stays pinned), click a reasoning toggle, drag-select transcript text, open the model picker — since the overlay/mouse coordinate edges are the kind of thing that passes unit tests but can still land a row off live.

Heads-up on merge ordering (not a blocker here): #224 also touches model.go / command_center.go, so whichever of #220/#224 lands second will need a rebase.

LGTM 👍