fix(tui): bypass toggleSidebar and toggleMouse global shortcuts when composer is non-empty - #576
Conversation
…composer is non-empty Allows standard readline/Emacs cursor movement keybindings (Ctrl+B and Ctrl+E) to work inside the prompt composer when typing a prompt. This prevents these keys from being globally intercepted to toggle the sidebar or mouse mode when the composer is populated.
|
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)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughCtrl+B and Ctrl+E now use a binding-aware empty-composer gate before toggling sidebar and mouse state. Tests cover empty and non-empty composer cases, plus existing remapped and explicit-default chord behavior. ChangesCtrl+B/Ctrl+E Composer Gating
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 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}
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found a couple of blockers that need to be addressed before this is ready.
Findings
-
[P1] Fix the failing diff hygiene check
internal/tui/keybinding_help_test.go:194
The current PR is failing theZero Reviewdiff hygiene step because this added blank line contains trailing whitespace (git diff --check origin/main...HEADreportstrailing whitespace). Please remove the stray whitespace so the review/CI gate can pass before the code change is considered ready. -
[P2] Keep remapped toggle shortcuts working while composing
internal/tui/model.go:1172
The newm.composerValue() == ""guard is applied to the wholem.keyMatch(...)result, so it disables user-configuredtoggleMouseandtoggleSidebarbindings whenever the composer has text, not just the built-inCtrl+E/Ctrl+Bchords that conflict with readline navigation. For example, a user who remapstoggleMouseto a non-readline chord can no longer use that shortcut while drafting a prompt because this case no longer fires and the key falls through to the composer path. Please scope the bypass to the conflicting default chords, or otherwise preserve configured toggle bindings that do not need to fall through as composer navigation. -
[P2] Link the approved parent issue for this community PR
CONTRIBUTING.md:60
The contribution policy requires community PRs to link an existing issue that already has theissue-approvedlabel, but this PR body does not link any parent issue. Please add the approved issue reference, or get a maintainer to confirm an explicit exception, so the PR satisfies the repository's issue-first requirement.
… chords Addresses jatmn's review: - Removed a trailing-whitespace-only blank line in keybinding_help_test.go that was failing the diff-hygiene CI check. - The composerValue() == "" guard on toggleMouse/toggleSidebar applied to the whole keyMatch(...) result, so a user who remaps either binding to a chord that doesn't conflict with readline navigation (Ctrl+E/Ctrl+B) lost that shortcut entirely while typing. The guard now only applies when the match came through the default chord; a remapped binding fires regardless of composer state. Added TestRemappedToggleBindingsIgnoreComposerGuard covering both bindings.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found issues that still need to be addressed.
Findings
- [P2] Keep explicit default bindings on the composer bypass path
internal/tui/model.go:1172
The new guard now preserves remapped shortcuts, but it treats any non-zero configured binding as safe to fire while the composer has text. A user can still explicitly put the documented defaults in config, for exampletoggleMouse: "ctrl+e"ortoggleSidebar: "ctrl+b";resolveKeyBindingskeeps those as non-zero bindings, so the!m.keyBindings.toggleMouse.isZero()/!m.keyBindings.toggleSidebar.isZero()side of the condition is true and Ctrl+E/Ctrl+B once again toggle mouse/sidebar instead of reaching the composer readline handlers. Please distinguish conflicting default chords from genuinely non-conflicting remaps, and add coverage for explicitctrl+e/ctrl+bconfig values while composing.
Addresses jatmn's follow-up review: the prior fix checked !b.isZero() to decide whether a binding is a genuine, non-conflicting remap, but a user can explicitly configure toggleMouse: "ctrl+e" or toggleSidebar: "ctrl+b" (the same chord as the built-in default). parseBinding gives that a non-zero parsedBinding, so isZero() alone wrongly treated it as safe to bypass the composer guard, letting Ctrl+E/Ctrl+B hijack keystrokes mid-sentence again. requiresEmptyComposer now compares the resolved binding against the canonical default chord directly (parsedBinding is a plain comparable struct), so both "unset" and "explicitly re-affirmed to the same chord" require an empty composer; only a binding that resolves to a genuinely different chord bypasses it. Added TestExplicitDefaultChordConfigStillRequiresEmptyComposer.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tui/model.go (1)
1172-1172: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: dedupe the composer-gate expression.
The
!requiresEmptyComposer(binding, defaultChord) || m.composerValue() == ""pattern is repeated verbatim for the mouse and sidebar cases. A tiny helper would remove the duplication and make eachcaseline easier to scan.♻️ Optional helper extraction
+// canFireComposerGatedToggle reports whether a toggle bound to b (with +// hardcoded conflicting default) may fire given the current composer text. +func canFireComposerGatedToggle(b parsedBinding, conflicting parsedBinding, composerEmpty bool) bool { + return !requiresEmptyComposer(b, conflicting) || composerEmpty +}- case m.keyMatch(m.keyBindings.toggleMouse, msg, func(tea.KeyMsg) bool { return keyCtrl(msg, 'e') }) && (!requiresEmptyComposer(m.keyBindings.toggleMouse, defaultToggleMouseChord) || m.composerValue() == ""): + case m.keyMatch(m.keyBindings.toggleMouse, msg, func(tea.KeyMsg) bool { return keyCtrl(msg, 'e') }) && canFireComposerGatedToggle(m.keyBindings.toggleMouse, defaultToggleMouseChord, m.composerValue() == ""):Also applies to: 1377-1377
🤖 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` at line 1172, The composer-gate check is duplicated in the toggleMouse and sidebar key handling cases, making the switch harder to scan. Extract the repeated `!requiresEmptyComposer(... ) || m.composerValue() == ""` logic into a small helper near `m.keyMatch`/`requiresEmptyComposer`, then use that helper in both cases so the `model.go` switch branches stay concise and consistent.
🤖 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/model.go`:
- Line 1172: The composer-gate check is duplicated in the toggleMouse and
sidebar key handling cases, making the switch harder to scan. Extract the
repeated `!requiresEmptyComposer(... ) || m.composerValue() == ""` logic into a
small helper near `m.keyMatch`/`requiresEmptyComposer`, then use that helper in
both cases so the `model.go` switch branches stay concise and consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1038f305-0828-44d5-aaaa-b62ed171130e
📒 Files selected for processing (3)
internal/tui/keybinding_help_test.gointernal/tui/keybindings.gointernal/tui/model.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/tui/keybinding_help_test.go
CodeRabbit nitpick: the toggleMouse/toggleSidebar dispatch cases both repeated !requiresEmptyComposer(...) || m.composerValue() == "" inline. Extracted canFireComposerGatedToggle so both cases share one expression.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the previously discussed paths and do not see any remaining actionable issues from my side.
@Vasanthdev2004 LGTM
…composer is non-empty (Gitlawb#576) * fix(tui): bypass toggleSidebar and toggleMouse global shortcuts when composer is non-empty Allows standard readline/Emacs cursor movement keybindings (Ctrl+B and Ctrl+E) to work inside the prompt composer when typing a prompt. This prevents these keys from being globally intercepted to toggle the sidebar or mouse mode when the composer is populated. * fix(tui): remove trailing whitespace, scope composer guard to default chords Addresses jatmn's review: - Removed a trailing-whitespace-only blank line in keybinding_help_test.go that was failing the diff-hygiene CI check. - The composerValue() == "" guard on toggleMouse/toggleSidebar applied to the whole keyMatch(...) result, so a user who remaps either binding to a chord that doesn't conflict with readline navigation (Ctrl+E/Ctrl+B) lost that shortcut entirely while typing. The guard now only applies when the match came through the default chord; a remapped binding fires regardless of composer state. Added TestRemappedToggleBindingsIgnoreComposerGuard covering both bindings. * fix(tui): treat an explicit default-chord config the same as unset Addresses jatmn's follow-up review: the prior fix checked !b.isZero() to decide whether a binding is a genuine, non-conflicting remap, but a user can explicitly configure toggleMouse: "ctrl+e" or toggleSidebar: "ctrl+b" (the same chord as the built-in default). parseBinding gives that a non-zero parsedBinding, so isZero() alone wrongly treated it as safe to bypass the composer guard, letting Ctrl+E/Ctrl+B hijack keystrokes mid-sentence again. requiresEmptyComposer now compares the resolved binding against the canonical default chord directly (parsedBinding is a plain comparable struct), so both "unset" and "explicitly re-affirmed to the same chord" require an empty composer; only a binding that resolves to a genuinely different chord bypasses it. Added TestExplicitDefaultChordConfigStillRequiresEmptyComposer. * refactor(tui): dedupe the composer-gated toggle check CodeRabbit nitpick: the toggleMouse/toggleSidebar dispatch cases both repeated !requiresEmptyComposer(...) || m.composerValue() == "" inline. Extracted canFireComposerGatedToggle so both cases share one expression.
3 participants
Summary
Prevents global interception of readline/Emacs cursor movement keybindings (
Ctrl+BandCtrl+E) when typing inside the input composer.Previously,
Ctrl+B(which toggles the sidebar) andCtrl+E(which toggles mouse mode) were globally intercepted at the model level on keypress. This broke standard Emacs-style readline navigation within the text composer (moving the cursor backward withCtrl+Band moving it to the end of the line withCtrl+E).This fix modifies the key match condition for these global shortcuts so they are bypassed when the input composer contains text. If the composer has text,
Ctrl+BandCtrl+Efall through to the text editor and behave as cursor movement keys. If the composer is empty, they continue to toggle the sidebar and mouse modes as expected.Changes
internal/tui/model.go&& m.composerValue() == ""totoggleSidebar(Ctrl+B) andtoggleMouse(Ctrl+E) global shortcut switch cases.internal/tui/keybinding_help_test.goTestCtrlBCtrlECursorNavigationBypassto verify both empty-composer toggling and non-empty-composer cursor navigation bypass.Test plan
go test -v ./internal/tui/...— okSummary by CodeRabbit
Ctrl+BandCtrl+Enow toggle the sidebar and mouse capture only when the composer is empty and the shortcuts match the default toggle bindings; while typing, they no longer interrupt input.Ctrl+B/Ctrl+Ebehavior for both empty and non-empty composer states, without affecting existing toggle-binding expectations.