fix(tui): stop the composer cursor blinking while typing or unfocused#672
fix(tui): stop the composer cursor blinking while typing or unfocused#672euxaristia wants to merge 3 commits into
Conversation
The composer's cursor blinked on a fixed timer unconditionally, including mid-keystroke, which made typing feel janky since the cursor's job is to mark the current position. Gate the existing blink tick on two states instead: force solid while a keystroke landed within the last 500ms (reusing the existing lastCharTime tracker), and hide it entirely while the terminal window is unfocused (new terminalFocused field, driven by tea.FocusMsg/ BlurMsg). Idle and focused keeps the prior blink behavior unchanged. Also decouples view.ReportFocus from the notifier (it was previously gated on m.notifier != nil, which is effectively always true in production but left cursor-focus behavior fragile if that ever changes) so focus reporting is requested unconditionally.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe TUI now tracks terminal focus and applies focus- and typing-aware composer cursor visibility rules. Focus reporting is always enabled, pasted or typed input shows the caret immediately, command hints follow cursor state, and tests cover focused, unfocused, typing, idle, and refocus behavior. ChangesComposer cursor focus behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Terminal
participant View
participant BubbleTea
participant TUIModel
participant Composer
Terminal->>View: focus or blur terminal
View->>BubbleTea: report focus event
BubbleTea->>TUIModel: deliver FocusMsg or BlurMsg
TUIModel->>TUIModel: update terminalFocused and cursor visibility
BubbleTea->>TUIModel: deliver keypress, paste, or blink message
TUIModel->>Composer: render visible, hidden, or toggled caret
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/model.go (1)
1221-1223: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate cursor visibility immediately on state transitions.
While the background blink timer correctly evaluates typing and focus states on each tick, relying solely on it to update cursor visibility introduces a noticeable UI delay. If the user types a key, focuses, or blurs while the cursor is in its hidden blink phase, the cursor will remain invisible for up to 500ms until the next tick fires. Update the visibility directly in the message handlers so the UI responds instantly.
internal/tui/model.go#L1221-L1223: Addm.composerCursorVisible = trueto force the cursor visible immediately on keypress.internal/tui/model.go#L1890-L1901: Addm.composerCursorVisible = truetotea.FocusMsgandm.composerCursorVisible = falsetotea.BlurMsgto instantly sync the cursor with terminal focus.🐛 Proposed fixes
m.lastKeyTime = now if !keyIs(msg, tea.KeyEnter) { m.lastCharTime = now } + m.composerCursorVisible = truecase tea.FocusMsg: m.terminalFocused = true + m.composerCursorVisible = true if m.notifier != nil { m.notifier.SetFocused(true) } return m, nil case tea.BlurMsg: m.terminalFocused = false + m.composerCursorVisible = false if m.notifier != nil { m.notifier.SetFocused(false) } return m, nil🤖 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 1221 - 1223, Update internal/tui/model.go at lines 1221-1223 to set m.composerCursorVisible = true on keypress alongside the existing m.lastCharTime update. In the tea.FocusMsg and tea.BlurMsg handlers at lines 1890-1901, set composerCursorVisible to true and false respectively so visibility changes immediately with terminal focus.
🤖 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/model.go`:
- Around line 1221-1223: Update internal/tui/model.go at lines 1221-1223 to set
m.composerCursorVisible = true on keypress alongside the existing m.lastCharTime
update. In the tea.FocusMsg and tea.BlurMsg handlers at lines 1890-1901, set
composerCursorVisible to true and false respectively so visibility changes
immediately with terminal focus.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1112d40c-81ce-4467-972d-7a8341d16a6f
📒 Files selected for processing (2)
internal/tui/model.gointernal/tui/model_test.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Apply the new caret state at the input and focus transitions
internal/tui/model.go:1221
A keypress only recordslastCharTime, while focus and blur only updateterminalFocused;composerCursorVisibleis changed solely by the next 530 mscomposerBlinkMsg. If an idle tick has hidden the caret, the next character is rendered without a caret until a later tick; a blur likewise leaves a visible caret, and a refocus can leave it hidden, for up to a tick. This contradicts the solid-while-typing and hidden-while-unfocused behavior, and the added tests miss it because they explicitly send a blink message after every transition. Update visibility synchronously for the relevant key/focus/blur messages and cover the state immediately after each message. -
[P2] Route required-command hints through the caret visibility state
internal/tui/model.go:3778
When a slash command is awaiting a required argument,composerLinetakes thecommandArgumentHintComposerLinebranch, which always callscomposerCursorand never readsm.composerCursorVisible. Consequently that composer caret remains visible while the terminal is unfocused and never observes the new typing/idle blink rules. Pass the visibility state into this renderer (and add an argument-hint case) so this alternate composer path follows the same focus contract.
…nt line - A keypress, focus, or blur now updates composerCursorVisible in its own message handler instead of waiting for the next 530ms composerBlinkMsg tick, which could render a typed character caret-less, leave a visible caret in an unfocused terminal, or keep it hidden after a refocus. - commandArgumentHintComposerLine takes the caret visibility state instead of painting its cursor cell unconditionally, so the required-argument hint path follows the same focus and blink contract as the main composer renderer. - New tests assert the state immediately after each message, with no blink tick in between, and cover the argument-hint render in both states.
|
Pushed ee2e157 for both findings, plus CodeRabbit's outside-diff comment, which described the same defect as the first finding.
go build, go vet, and the tui test suite pass locally, except TestAltScreenTranscriptScrollKeepsFooterFixed, which fails on this branch without these changes and passes on current main, so it is a stale-branch artifact rather than a regression from this change. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P2] Apply the caret state when pasting into the composer
internal/tui/clipboard.go:58
tea.PasteMsgand right-clickclipboardReadMsgboth reachroutePaste, which updates the editable composer without refreshinglastCharTimeorcomposerCursorVisible. If the idle blink has just hidden the caret, pasting text leaves that newly populated composer caret-less until the next 530 ms tick; because the timestamp is stale, that tick toggles instead of entering the solid-while-typing state. This is the same immediate-input transition fixed fortea.KeyPressMsg, so refresh the caret/timing state for the composer-paste branch and cover a paste immediately after a hidden blink.
A paste is the same immediate-input transition as a keypress: routePaste now refreshes lastCharTime and sets the caret visible for both the bracketed tea.PasteMsg and right-click clipboardReadMsg paths, so a paste right after the blink phase hid the caret renders solid immediately instead of waiting for a tick that would toggle off a stale idle state.
|
Pushed a fix for the paste finding: routePaste now refreshes lastCharTime and sets the caret visible before any early return, covering both the bracketed tea.PasteMsg path and the right-click clipboardReadMsg path, so a paste immediately after a hidden blink renders a solid caret and the next tick holds solid instead of toggling off the stale idle timestamp. TestComposerCursorShowsImmediatelyOnPaste covers the hidden-blink-then-paste sequence with no blink tick in between. |
There was a problem hiding this comment.
Approve. The three-state blink handler is clean unfocused hides, active typing stays solid via the 500ms idle threshold, and idle+focused toggles as before and syncing the caret immediately on keypress/paste/focus/blur closes the race where a tick could leave it in the wrong state for up to a full blink period. One thing worth a glance: ReportFocus is now always on (CSI ?1004h) regardless of notifier config, so terminals that previously never received focus events will now start sending them; it's a standard, widely-ignored DEC mode and the true default handles terminals that never emit events, but it does broaden the surface. New tests cover every branch and pass; the only failing test (TestHandleAddDirCommand) reproduces on clean origin/main and is unrelated.
3 participants
Summary
internal/tui/model.go,composerBlinkMsghandler) on two states instead of an unconditional toggle:lastCharTimetracker): cursor stays solid.terminalFocusedfield, driven bytea.FocusMsg/tea.BlurMsg): cursor hides entirely, matching the convention in Vim/iTerm2/most editors that a hidden caret is the unambiguous "not active" signal.view.ReportFocusfrom the notifier (m.notifier != nil->true). It was previously gated on notifier existence, which is effectively always true in production today, but left cursor-focus behavior fragile if that ever changes; focus reporting is a rendering concern independent of notification config.Linked issue
None. Direct user report about the blinking feeling janky while typing.
Test plan
go build ./...go vet ./...go test ./...(all green except the same pre-existing, unrelatedinternal/contextreportfailure that also fails on unmodifiedmain)gofmt -lcleaninternal/tui/model_test.go: cursor stays solid while typing, hidden while unfocused, resumes blinking after refocus + idle, unchanged toggle behavior when idle+focusedtmux, diffing rawcapture-pane -eoutput over time to confirm the actual rendered cursor styling toggles while idle, stays solid across consecutive keystrokes sent at 200ms intervals, disappears immediately after injecting a terminal blur escape sequence (CSI O) and stays hidden across a blink interval, then resumes toggling after injecting a focus escape sequence (CSI I) and waiting past the idle thresholdSummary by CodeRabbit
Bug Fixes
Tests