feat(ui): resolve terminal capabilities lazily and per-client (#101) - #102
Conversation
- Replace the package-init probes in internal/ui/style (isDarkBg, termColorProfile) with a mutex-guarded, lazily-resolved capability holder so the blocking OSC background query no longer fires at import time against arbitrary process fds - Add SetTerminalCapabilities for frontends to supply the terminal they actually serve, and ResolveTerminalCapabilities to probe eagerly - Defer DefaultTheme derivation until first use; rebuild a derived theme on a capability change while preserving an explicitly set theme - Resolve capabilities in cmd/root.go before the TUI takes over stdin to keep the query off the render path - Re-export the new entry points from internal/ui and cover the override, derived-theme rebuild, and explicit-theme preservation with tests First step of decoupling session/terminal state from the TUI layer.
|
Connected to Huly®: KIT-103 |
📝 WalkthroughWalkthroughTerminal capability detection now resolves lazily or synchronously before interactive TUI startup. Styling APIs are re-exported, derived themes and caches respond to capability changes, and tests cover terminal switching, theme preservation, and logo colors. ChangesTerminal capability and theme handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/root.go (1)
1763-1768: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse Bubble Tea’s background query instead of probing
os.Stdin/os.Stdoutdirectly.
ResolveTerminalCapabilities()runslipgloss.HasDarkBackground(os.Stdin, os.Stdout), which is the standalone/background-independent path. For a Bubble Tea application, request the client terminal background viatea.RequestBackgroundColorinInit, handletea.BackgroundColorMsginUpdate, then apply that value instyle.SetTerminalCapabilities. This keeps background detection in lockstep with the TUI and avoids using local server fds, which would matter for Wish/SSH sessions.🤖 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 `@cmd/root.go` around lines 1763 - 1768, Replace the pre-TUI ResolveTerminalCapabilities call with Bubble Tea’s request flow: have the model return tea.RequestBackgroundColor from Init, handle tea.BackgroundColorMsg in Update, and pass the received color to style.SetTerminalCapabilities. Remove the direct os.Stdin/os.Stdout probing path used by ResolveTerminalCapabilities while preserving configured theme values.
🤖 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/ui/style/enhanced.go`:
- Around line 67-90: Synchronize all shared theme state to preserve the
goroutine-safety guarantee: protect currentTheme, themeExplicitlySet,
themeGeneration, and the typography/style/syntax cache fields consistently
across SetTerminalCapabilities, GetTheme, SetTheme, invalidateThemeCaches,
GetCachedStyles, and SyntaxStyle. Use the same mutex or a dedicated theme mutex
for every read and write, including cache invalidation, while preserving
explicit-theme behavior.
---
Nitpick comments:
In `@cmd/root.go`:
- Around line 1763-1768: Replace the pre-TUI ResolveTerminalCapabilities call
with Bubble Tea’s request flow: have the model return tea.RequestBackgroundColor
from Init, handle tea.BackgroundColorMsg in Update, and pass the received color
to style.SetTerminalCapabilities. Remove the direct os.Stdin/os.Stdout probing
path used by ResolveTerminalCapabilities while preserving configured theme
values.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 511528f4-5ded-40ac-a424-c9a278277d8d
📒 Files selected for processing (6)
cmd/root.gointernal/ui/exports.gointernal/ui/style/enhanced.gointernal/ui/style/logo_test.gointernal/ui/style/terminal_caps_test.gointernal/ui/style/themes.go
) Address CodeRabbit review on PR #102. - enhanced.go: SetTerminalCapabilities claimed "safe to call from any goroutine" but mutated currentTheme/themeExplicitlySet and rebuilt the theme-derived caches (generation counter, typography/style/syntax) with no lock, while the render loop reads them unsynchronized. Narrow the contract to match SetTheme (call on the UI goroutine); the capability swap itself stays mutex-guarded. Documented the shared UI-goroutine discipline on currentTheme/themeExplicitlySet (🔴 critical, valid). - Skipped: cmd/root.go nitpick to wire detection through tea.RequestBackgroundColor. Out of scope for this step — the PR preserves the pre-existing os.Stdin/os.Stdout probe for behaviour parity, and SetTerminalCapabilities is the seam a future Init/Update-based (Wish/SSH-aware) wiring will use (🔵 trivial).
|
Addressed the CodeRabbit review in 36262fe:
|
1 participant
Description
Replaces the process-global, import-time terminal probes in
internal/ui/stylewith lazily-resolved, per-client-overridable capabilities. Previously the package ran a blocking OSC background-color query and a color-profile lookup at package-init time, against whateveros.Stdin/os.Stdoutthe process happened to hold — which cannot represent more than one terminal and fires as an import side effect for any consumer of the package (including headless frontends).Terminal background and color profile are now held behind a mutex-guarded cache resolved lazily on first use. A frontend can call
SetTerminalCapabilities(darkBackground, colorProfile)to supply the terminal it actually serves, orResolveTerminalCapabilities()to probe eagerly. The interactive path calls the latter incmd/root.gojust before the TUI takes over stdin, preserving the old "detect before render" timing and keeping the synchronous OSC query off the render path.DefaultTheme()callsAdaptiveColor, so theme derivation is now deferred until first use as well; a lazily-derived theme is rebuilt when capabilities change, while a theme set explicitly viaSetThemeis preserved untouched.This is the first, independently-shippable step of #101 (decoupling session/terminal state from the TUI layer). It is a pure refactor with no intended user-visible change. It does not close the issue on its own — the remaining steps (typed event union,
TreeManagervalue methods, session file I/O intointernal/app) land as separate PRs, and the final one will carryFixes #101.Refs #101
Type of Change
Checklist
go vet,golangci-lint run,gofmt)go test -race ./internal/ui/...)/theme, theme switch) in tmuxAdditional Information
Files changed:
internal/ui/style/enhanced.go— replacedisDarkBg/termColorProfilepackage vars with a lazily-resolved capability holder; addedSetTerminalCapabilitiesandResolveTerminalCapabilities; madecurrentThemelazy and rebuildable on capability change.internal/ui/style/themes.go— swapped the twoisDarkBgreads forisDarkBackground().internal/ui/exports.go— re-exported the two new entry points frominternal/ui.cmd/root.go— resolve capabilities beforetea.NewProgram.internal/ui/style/logo_test.go— migrated three tests off the removedisDarkBgvar ontoSetTerminalCapabilities.internal/ui/style/terminal_caps_test.go(new) — covers override, derived-theme rebuild, and explicit-theme preservation.Backward compatibility: No public API removed. The TUI resolves capabilities against the same fds at an equivalent (pre-TUI) point, so interactive behaviour is unchanged. The new seam is what future headless frontends will call instead of relying on the import-time probe.
Summary by CodeRabbit