Polish main TUI surface#168
Conversation
Simplify the empty chat surface, replace starter prompt chips with a cleaner brand state, move model and permission state into the composer frame, quiet the footer status, and improve composer word-editing keybindings. Tested: GOCACHE=/tmp/zero-go-cache go test ./internal/tui Tested: GOCACHE=/tmp/zero-go-cache go test ./... Tested: git diff --check
|
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:
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 (18)
🚧 Files skipped from review as they are similar to previous changes (12)
WalkthroughAdds composer word navigation/mention-aware edits, dual command/file suggestion palettes with cached file indexing and ranked file completions, centered bordered suggestion/picker UI, model-picker loading/favorites persistence, provider-wizard gating, mouse-driven overlay hit-testing and transcript selection, and many aligned tests. ChangesComposer UI and interaction overhaul
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tui/view.go (1)
123-146: 💤 Low valueUpdate outdated comment.
The comment at line 124 mentions "surface name," but the code only adds provider and usage segments. Update the comment to match the actual behavior.
📝 Proposed fix
-// statusLine renders the bottom readout as ` │ `-separated groups: provider -// on the left, then a flexible gap, then tokens/cost and the surface name. -// Groups drop with the width tier: narrow keeps provider+tokens only, tiny -// shows the provider only. +// statusLine renders the bottom readout as ` │ `-separated groups: provider +// on the left, then a flexible gap, then tokens/cost. +// Groups drop with the width tier: narrow keeps provider+tokens only, tiny +// shows the provider only.🤖 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/view.go` around lines 123 - 146, The top comment for the statusLine function is outdated: update the comment to accurately describe that the bottom readout renders provider on the left and usage/tokens on the right (no "surface name"), and clarify the width-tier behavior (tiny shows provider only, narrow shows provider+tokens/usage). Edit the comment above func statusLine(width int) to remove "surface name" and adjust the description of groups and tier behavior to match the implementation using left, rightGroups, and usageStatusSegment().
🤖 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/view.go`:
- Around line 123-146: The top comment for the statusLine function is outdated:
update the comment to accurately describe that the bottom readout renders
provider on the left and usage/tokens on the right (no "surface name"), and
clarify the width-tier behavior (tiny shows provider only, narrow shows
provider+tokens/usage). Edit the comment above func statusLine(width int) to
remove "surface name" and adjust the description of groups and tier behavior to
match the implementation using left, rightGroups, and usageStatusSegment().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e83c5750-dbc7-4b5b-a861-81a4f7fecaac
📒 Files selected for processing (10)
internal/tui/composer.gointernal/tui/composer_test.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/rendering_lime_test.gointernal/tui/startup.gointernal/tui/startup_test.gointernal/tui/theme.gointernal/tui/view.gointernal/tui/width_tiers_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/view.go (1)
122-126:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRender the divider metadata when it exactly fits.
When
width == metaWidth+4, the"╰ "+meta+" ╯"variant fits exactly, but the<=check still falls back to a blank rule. That drops the model/mode label one column earlier than necessary.💡 Proposed fix
- if width <= metaWidth+4 { + if width < metaWidth+4 { return zeroTheme.lineStrong.Render("╰" + strings.Repeat("─", width-2) + "╯") }🤖 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/view.go` around lines 122 - 126, The divider logic currently treats width == metaWidth+4 as too small and renders a blank rule; change the guard in view.go from "if width <= metaWidth+4" to "if width < metaWidth+4" so the exact-fit case renders the "╰ "+meta+" ╯" variant; update the branch using width, metaWidth, meta and zeroTheme.lineStrong.Render accordingly so the meta label appears when width equals metaWidth+4.
🤖 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 `@internal/tui/autocomplete.go`:
- Around line 267-277: The Esc branch removes a trailing @ token by mutating
m.input via removeTrailingAtToken(...) but doesn't update the composer state, so
subsequent word-edit keys (which call applyComposerKey()) operate on stale
composerState; after adjusting m.input (inside the if m.suggestionsAreFiles
block) call resetComposerFromInput() to sync composerState (and ensure cursor
positioning via m.input.CursorEnd() remains correct), leaving the rest of the
cleanup (clearComposer(), suggestions niling, flags resetting) unchanged.
---
Outside diff comments:
In `@internal/tui/view.go`:
- Around line 122-126: The divider logic currently treats width == metaWidth+4
as too small and renders a blank rule; change the guard in view.go from "if
width <= metaWidth+4" to "if width < metaWidth+4" so the exact-fit case renders
the "╰ "+meta+" ╯" variant; update the branch using width, metaWidth, meta and
zeroTheme.lineStrong.Render accordingly so the meta label appears when width
equals metaWidth+4.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7436017d-5cf3-416d-98d7-db5b487291d1
📒 Files selected for processing (7)
internal/tui/autocomplete.gointernal/tui/autocomplete_test.gointernal/tui/model.gointernal/tui/selectable_list.gointernal/tui/selectable_list_test.gointernal/tui/startup.gointernal/tui/view.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/tui/startup.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/autocomplete_test.go (1)
277-286:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis scrolled-window assertion is the CI failure and it’s too specific.
The second half of this test hard-codes
❯ clear/providervisibility aftersuggestionPaletteMaxVisible + 1Down events, but CI is already showing that the renderer now chooses a different valid window on every platform. That makes the PR red even when selection movement itself works.Assert that the selected suggestion is visible and that the legacy “more” row stays absent, instead of pinning the viewport to one command name.
Suggested fix
for range suggestionPaletteMaxVisible + 1 { updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyDown}) m = updated.(model) } plain = plainRender(t, m.View()) if strings.Contains(plain, "more") { t.Fatalf("scrolled palette should not render a more-count row, got %q", plain) } - if !strings.Contains(plain, "│ ❯ clear") || strings.Contains(plain, "│ ❯ provider") || strings.Contains(plain, "│ provider") { - t.Fatalf("scrolled palette should move the visible command window, got %q", plain) - } + selected := strings.TrimPrefix(m.suggestions[m.suggestionIdx].Name, "/") + if !strings.Contains(plain, "│ ❯ "+selected) { + t.Fatalf("scrolled palette should keep the selected command visible, got %q", plain) + }🤖 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/autocomplete_test.go` around lines 277 - 286, The test currently hard-codes which items must be visible after suggestionPaletteMaxVisible+1 Down events; change it to only assert that the legacy "more" row is absent and that the currently-selected suggestion is visible. After the loop that calls m.Update(tea.KeyMsg{Type: tea.KeyDown}) suggestionPaletteMaxVisible+1 times, render with plain := plainRender(t, m.View()) and replace the strict checks for "│ ❯ clear" / "provider" with two assertions: 1) strings.Contains(plain, "more") is false (keep the existing "more" check), and 2) the rendered output contains the selected suggestion text or selection marker by deriving the selection from the model (use the model's public accessors or fields used in this test—e.g., the current suggestion via m.Suggestions()/m.Cursor() or the presence of the selection marker "❯" in plain) so the test only verifies visibility of the selected item rather than a specific window layout.Source: Pipeline failures
🤖 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 `@internal/tui/autocomplete.go`:
- Around line 512-516: The Esc branch that handles file suggestions (when
m.suggestionsAreFiles) currently calls removeTrailingAtToken(m.input.Value())
and moves the cursor to the end, which removes the wrong `@-mention` when the
cursor is inside an earlier path query; change this to use the active path query
range returned by extractPathQuery(value, m.input.Position()) (the same data
recomputeSuggestions() uses): remove the substring covering that pathQuery range
from the input value, set the input value to the new string with that range
removed, and restore the cursor to the start index of that removed range
(instead of CursorEnd()), then call m.resetComposerFromInput() so the composer
reflects the updated cursor/contents. Ensure you reference
recomputeSuggestions(), m.suggestionsAreFiles, extractPathQuery,
removeTrailingAtToken (remove its use here), input.SetValue, input.CursorEnd
(replace with cursor set to removed range start), and resetComposerFromInput in
your changes.
---
Outside diff comments:
In `@internal/tui/autocomplete_test.go`:
- Around line 277-286: The test currently hard-codes which items must be visible
after suggestionPaletteMaxVisible+1 Down events; change it to only assert that
the legacy "more" row is absent and that the currently-selected suggestion is
visible. After the loop that calls m.Update(tea.KeyMsg{Type: tea.KeyDown})
suggestionPaletteMaxVisible+1 times, render with plain := plainRender(t,
m.View()) and replace the strict checks for "│ ❯ clear" / "provider" with two
assertions: 1) strings.Contains(plain, "more") is false (keep the existing
"more" check), and 2) the rendered output contains the selected suggestion text
or selection marker by deriving the selection from the model (use the model's
public accessors or fields used in this test—e.g., the current suggestion via
m.Suggestions()/m.Cursor() or the presence of the selection marker "❯" in plain)
so the test only verifies visibility of the selected item rather than a specific
window layout.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b36b850-d190-49d5-bcfa-af127672c68d
📒 Files selected for processing (7)
internal/tui/autocomplete.gointernal/tui/autocomplete_test.gointernal/tui/composer.gointernal/tui/composer_test.gointernal/tui/model.gointernal/tui/view.gointernal/tui/width_tiers_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/tui/width_tiers_test.go
- internal/tui/view.go
- internal/tui/model.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/model.go`:
- Around line 858-859: The footer-anchored palette can overflow on short
terminals; modify the logic around suggestionInFooter (and the block using
suggestionOverlay/footer) to guard against footer height exceeding the terminal:
compute footerLines := len(viewLines(footer)) (or use
scrollableTranscriptView.View() line count) and only set suggestionInFooter =
true when suggestionOverlay != "" AND !(m.transcriptEmpty() && !m.pending) AND
footerLines < m.height (or footerLines <= m.height-1 if you need room for the
palette); otherwise fall back to body anchoring or clamp the footer height
(reduce footerLines and render a shortened footer) so the palette/composer stack
never pushes past m.height. Ensure you update both the suggestionInFooter
assignment and the subsequent branch that renders the footer/palette (lines
~907-912) to use this guarded value.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e1a042fd-4305-40b5-9656-0284597883c8
📒 Files selected for processing (2)
internal/tui/autocomplete_test.gointernal/tui/model.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/tui/model.go (1)
943-963:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClamp the footer stack before slicing the alt-screen viewport.
Lines 946-949 still force
available = 1whenlen(footerLines) >= m.height, and Line 963 then appends the full footer anyway. That makesView()return more thanm.heightlines on short alt-screen terminals, so the composer/palette stack still spills past the screen instead of fitting inside it.🤖 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/model.go` around lines 943 - 963, In scrollableTranscriptView, prevent the footer from overflowing the alt-screen by capping footerLines before computing the viewport: clamp footerLines to at most m.height-1 entries (e.g. take the last min(len(footerLines), m.height-1) lines), then set available = m.height - len(footerLines) (and ensure available >= 1), compute start/end/maxOffset using that available, and finally append the already-clamped footerLines; this keeps the total lines returned ≤ m.height and preserves existing logic in viewLines, overlayViewportLines, and the chatScrollOffset calculations.
🤖 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.
Duplicate comments:
In `@internal/tui/model.go`:
- Around line 943-963: In scrollableTranscriptView, prevent the footer from
overflowing the alt-screen by capping footerLines before computing the viewport:
clamp footerLines to at most m.height-1 entries (e.g. take the last
min(len(footerLines), m.height-1) lines), then set available = m.height -
len(footerLines) (and ensure available >= 1), compute start/end/maxOffset using
that available, and finally append the already-clamped footerLines; this keeps
the total lines returned ≤ m.height and preserves existing logic in viewLines,
overlayViewportLines, and the chatScrollOffset calculations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 59fd6206-380d-4402-bac5-2d61873bb64f
📒 Files selected for processing (2)
internal/tui/autocomplete_test.gointernal/tui/model.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tui/commands_test.go (1)
94-110: ⚡ Quick winAdd direct parser edge-case tests for optional-depth behavior.
You’re testing command-level outcomes, but not the parser contract itself (
commandUsageRequiredPlaceholder). Add a small table-driven test for mixed optional/required patterns to harden regressions.Proposed test addition
+func TestCommandUsageRequiredPlaceholder(t *testing.T) { + cases := []struct { + usage string + want string + }{ + {usage: "/spec <task>", want: "task"}, + {usage: "/rewind [latest|<sequence>]", want: ""}, + {usage: "/x [opt] <required>", want: "required"}, + {usage: "/x [a [b <c>]] <d>", want: "d"}, + {usage: "/x", want: ""}, + } + for _, tc := range cases { + if got := commandUsageRequiredPlaceholder(tc.usage); got != tc.want { + t.Fatalf("commandUsageRequiredPlaceholder(%q) = %q, want %q", tc.usage, got, tc.want) + } + } +}🤖 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/commands_test.go` around lines 94 - 110, Add a small table-driven unit test exercising the parser function commandUsageRequiredPlaceholder (not just commandRequiredInputHint) to cover mixed optional/required patterns and edge cases: create cases with usages like "cmd [opt] <req>", "cmd <req> [opt]", nested or multiple optionals like "cmd [a] [b] <req>", and fully-optional patterns to assert the returned placeholder matches expected (e.g., "<req>" vs "" or "[opt]"). Put the test alongside existing tests in the same test file, iterate cases, call commandUsageRequiredPlaceholder for each pattern, and use t.Fatalf/t.Errorf when the result differs from the expected value so regressions in optional-depth parsing are caught.
🤖 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/commands_test.go`:
- Around line 94-110: Add a small table-driven unit test exercising the parser
function commandUsageRequiredPlaceholder (not just commandRequiredInputHint) to
cover mixed optional/required patterns and edge cases: create cases with usages
like "cmd [opt] <req>", "cmd <req> [opt]", nested or multiple optionals like
"cmd [a] [b] <req>", and fully-optional patterns to assert the returned
placeholder matches expected (e.g., "<req>" vs "" or "[opt]"). Put the test
alongside existing tests in the same test file, iterate cases, call
commandUsageRequiredPlaceholder for each pattern, and use t.Fatalf/t.Errorf when
the result differs from the expected value so regressions in optional-depth
parsing are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c2ea047-bed5-48fe-be8c-1680ee243484
📒 Files selected for processing (7)
internal/tui/autocomplete.gointernal/tui/autocomplete_test.gointernal/tui/commands.gointernal/tui/commands_test.gointernal/tui/composer.gointernal/tui/model.gointernal/tui/rendering_lime_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/tui/rendering_lime_test.go
- internal/tui/autocomplete.go
- internal/tui/autocomplete_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tui/view.go (1)
657-683: 💤 Low valueWidth calculation iterates all picker items on every render.
The
modelPickerOverlayWidthfunction loops through allpicker.itemsto determine the widest label. For typical model catalogs this is negligible, but if the item count grows large, consider caching the computed width when items change rather than recalculating per-frame.🤖 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/view.go` around lines 657 - 683, modelPickerOverlayWidth currently iterates picker.items and calls modelPickerItemDetail on every render which is wasteful for large catalogs; modify commandPicker to cache the computed widest label (e.g., add fields like cachedOverlayWidth and a change marker such as itemsVersion or lastItemsHash) and update that cache only when the items slice or item properties change (update the marker whenever items are mutated or replaced); then have modelPickerOverlayWidth read the cached width from the picker when available instead of recomputing, falling back to the existing loop only if the cache is missing or stale.
🤖 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/view.go`:
- Around line 657-683: modelPickerOverlayWidth currently iterates picker.items
and calls modelPickerItemDetail on every render which is wasteful for large
catalogs; modify commandPicker to cache the computed widest label (e.g., add
fields like cachedOverlayWidth and a change marker such as itemsVersion or
lastItemsHash) and update that cache only when the items slice or item
properties change (update the marker whenever items are mutated or replaced);
then have modelPickerOverlayWidth read the cached width from the picker when
available instead of recomputing, falling back to the existing loop only if the
cache is missing or stale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: adc73514-d8e4-46d6-98da-2ee33048adba
📒 Files selected for processing (16)
internal/cli/app.gointernal/cli/app_test.gointernal/config/resolver.gointernal/config/resolver_test.gointernal/config/types.gointernal/config/writer.gointernal/config/writer_test.gointernal/tui/model.gointernal/tui/onboarding.gointernal/tui/options.gointernal/tui/picker.gointernal/tui/picker_test.gointernal/tui/provider_wizard.gointernal/tui/provider_wizard_test.gointernal/tui/rendering_lime_test.gointernal/tui/view.go
💤 Files with no reviewable changes (2)
- internal/tui/onboarding.go
- internal/tui/provider_wizard.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/tui/provider_wizard_test.go
- internal/tui/model.go
|
@coderabbitai re review |
|
✅ Action performedFull review finished. |
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.
Requesting changes for one test reliability issue.
Finding:
internal/tui/provider_wizard_test.goTestProviderWizardSupportsLeftAndGuardedRightNavigationassumes the selected provider has no credential already available in the process environment. On a normal developer machine with a provider key set (for exampleOPENAI_API_KEY/OLLAMA_API_KEY),credentialReadyForRight()returns true, so Right advances to the model step and the test fails:
right from empty credential step = 2, want credential.
This should be made hermetic. Please clear the selected provider's AuthEnvVars with t.Setenv(env, "") before the empty-credential assertion, or use a helper that clears all catalog auth env vars for this test. Keep TestProviderWizardRightAllowsExistingCredentialEnv as the explicit positive coverage for env-backed credentials.
Validation I ran:
go test ./internal/tui ./internal/config ./internal/clifails in my normal environment because of the test above.- With provider API env vars cleared:
go test ./internal/tuipasses. - With provider API env vars cleared:
go test ./...passes. git diff --check origin/main...HEADpasses.go run ./cmd/zero-release buildpasses.go run ./cmd/zero-release smokepasses.
The implementation otherwise looks sound from my pass; this is a bounded fix.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Reviewed the current state (post un-draft, +4074/−485). Solid and genuinely well-tested work — flagging a few things for a team decision rather than blocking, since none are correctness bugs.
Scope — this is now 4+ features in one ~4,000-line PR
It grew from "polish empty state/composer" to also include two-step mouse selection, OSC-52 clipboard, provider-wizard/picker mouse hit-testing, and favorite-models persistence. That's hard to review atomically, bisect, or revert. Strong suggestion: split the mouse-selection + clipboard work into its own PR on top of the polish. At minimum it must rebase on #170, which also rewrote renderPermissionRow in rendering.go.
Mouse capture disables native terminal text selection
wantsMouseCapture enables EnableMouseCellMotion across the whole chat surface (chatWantsMouseCapture = !setup.visible). Once mouse is captured, the terminal's own drag-to-copy stops working — users who rely on native selection (or copying into another app) lose it, replaced by the PR's own selection. This is the defining UX tradeoff of the feature and should be a deliberate, documented decision (and worth noting the Shift-drag escape hatch most terminals honor). Several users will read this as "I can't select text in my terminal anymore."
Hit-testing duplicates layout math (maintainability trap)
overlayMouseHit/overlayMouseTop re-derive each overlay's on-screen position by replicating the render path's centering logic, and each select*AtMouse hand-counts row offsets to skip headers/separators (hit.y - 3, rowStart := baseRow + 2, +1 for error/status lines, group-header increments). The renderer computes those positions one way; the hit-tester re-counts them another, linked only by magic numbers. Any layout tweak (and this PR tweaks layouts) will silently map clicks to the wrong row. mouse_test.go helps, but these offsets should derive from the same constants the renderer uses rather than being re-counted by hand.
OSC-52 write bypasses the Bubble Tea render loop
os.Stdout.WriteString(ansi.SetSystemClipboard(text)) writes raw escapes to stdout directly from an update handler, outside tea's managed output — in alt-screen that can race/corrupt the frame; the idiomatic path is a tea.Cmd/tea.Printf. Also OSC-52 is a silent no-op on terminals that don't support it (and has a payload cap) — no fallback or user feedback when a copy doesn't land.
Still open from the earlier pass
fileIndexTTL = 30 * time.Secondis unchanged — a file created mid-session is invisible to@-completion for up to 30s. Shorten it or invalidate the cwd entry on write-tool activity.- Worth re-confirming the
dismissSuggestionsmid-prompt token removal and ANSI-overlay wide-rune (CJK/emoji) correctness — the overlay code now also backs mouse hit-testing, so wide-rune alignment matters more.
Verdict
Strong, well-tested feature work; nothing here is a data-correctness bug. But it's large and tightly coupled — I'd want a rebase on #170, an explicit team decision on the mouse-capture/native-selection tradeoff, and ideally the mouse/clipboard work split out before merge. Happy to dig into any specific spot.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Requesting changes (see my detailed comment above for the full list). Nothing here is a correctness bug — the work is strong and well-tested — but two items should gate the merge:
- Rebase on #170 first. #170 rewrote
renderPermissionRowinrendering.go; this PR touches the same file and is later in the merge order, so it needs to rebase or it'll conflict/break. - Decide the mouse-capture tradeoff explicitly. Enabling mouse capture across the chat removes the terminal's native drag-to-copy; that's a product decision (with a documented Shift-drag note), not a silent default.
Everything else in the comment (splitting the mouse/clipboard work out, the hit-test layout duplication, the OSC-52 stdout write, the 30s file-index TTL) is non-blocking polish. Happy to re-approve once the rebase lands and the capture tradeoff is signed off.
|
Re-review after #170 merged: correcting my earlier review.
So the only thing I'd still want before approving is an explicit call on the mouse-capture/native-selection tradeoff (a line in the PR description or a code comment documenting it, ideally with the Shift-drag note). The rest — splitting mouse/clipboard out, the hit-test layout-offset duplication, the OSC-52 stdout write, the 30s file-index TTL — remains non-blocking polish. Clear that one tradeoff and I'm happy to approve. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approved on rereview. The provider-wizard test is now hermetic in 6e088c3; it passes in my normal environment with API key env vars present.
Validation:
- current PR head:
go test ./internal/tui ./internal/config ./internal/cli - clean merge onto latest
origin/main:git diff --check - clean merge onto latest
origin/main:go test ./internal/tui ./internal/config ./internal/cli - clean merge onto latest
origin/main:go test ./... - clean merge onto latest
origin/main:go run ./cmd/zero-release build - clean merge onto latest
origin/main:go run ./cmd/zero-release smoke
GitHub checks are also green. My previous blocker is resolved; no blockers from my side.
…polish # Conflicts: # internal/tui/scroll_test.go
|
@gnanam1990 addressed the remaining mouse-capture point in the PR description. The explicit product call is now documented there: app-managed mouse selection/copy intentionally captures mouse input while the TUI is active, and native terminal selection remains available through the terminal escape path such as Shift-drag where supported. Also merged latest |
|
@coderabbitai review The requested changes have been addressed in the latest branch updates:
Please re-review when ready. |
|
✅ Action performedReview finished.
|
3 participants
Summary:
Tests:
More UI polish can continue in follow-up PRs.
Summary by CodeRabbit