fix(#1741): resolve Ctrl+K key binding conflict in session browser#1742
Conversation
There was a problem hiding this comment.
Pull request overview
Resolves the session browser dialog’s Ctrl+K key binding conflict by moving the “Copy session ID” action off Ctrl+K so Up navigation remains reachable.
Changes:
- Rebound
CopyIDfromCtrl+KtoCtrl+Yin the session browser keymap. - Updated the session browser ctrl-navigation test to validate
Ctrl+YmatchesCopyIDand doesn’t move selection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/tui/dialog/session_browser.go | Changes the CopyID key binding to ctrl+y to remove the ctrl+k conflict with Up. |
| pkg/tui/dialog/session_browser_test.go | Adds assertions around ctrl+y matching CopyID and ensures selection doesn’t change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Escape: key.NewBinding(key.WithKeys("esc")), | ||
| Star: key.NewBinding(key.WithKeys("ctrl+s")), | ||
| FilterStar: key.NewBinding(key.WithKeys("ctrl+f")), | ||
| CopyID: key.NewBinding(key.WithKeys("ctrl+k")), | ||
| CopyID: key.NewBinding(key.WithKeys("ctrl+y")), | ||
| }, |
There was a problem hiding this comment.
CopyID was rebound to ctrl+y, but the on-screen help in View() still advertises ctrl+k for "copy id" (see the AddHelpKeys(...) call). This will mislead users and also contradicts the new keymap; update the help text to match the new binding.
There was a problem hiding this comment.
You're right — the AddHelpKeys call on line 293 was still showing ctrl+k. I had the fix locally but it was locked to another GitButler branch and didn't make it into the commit. Now fixed in 53e8d65.
|
|
||
| updated, _ = d.Update(ctrlY) | ||
| d = updated.(*sessionBrowserDialog) | ||
| require.Equal(t, 0, d.selected, "selection should remain 0 after ctrl+y (CopyID)") |
There was a problem hiding this comment.
This test calls d.Update(ctrlY), which immediately executes clipboard.WriteAll(...) in the CopyID handler. That mutates the developer/CI system clipboard during go test, which is an undesirable side effect for a unit test. Consider avoiding the Update(ctrlY) call (only assert key matching), or refactor CopyID to return a tea.Cmd that performs the clipboard write so tests don't trigger it unless they explicitly execute the command.
| updated, _ = d.Update(ctrlY) | |
| d = updated.(*sessionBrowserDialog) | |
| require.Equal(t, 0, d.selected, "selection should remain 0 after ctrl+y (CopyID)") |
There was a problem hiding this comment.
Good point. Removed the d.Update(ctrlY) call — the test now only asserts key matching to avoid mutating the system clipboard during go test. Amended in ebc63e3.
aheritier
force-pushed
the
fix/1741-session-browser-ctrl-k-conflict
branch
2 times, most recently
from
ebc63e3 to
53e8d65
Compare
|
copilot feedbacks addressed |
Ctrl+K was bound to both Up navigation and CopyID in the session browser dialog. Since Up was matched first in the Update switch, the CopyID handler was unreachable. Reassign CopyID to Ctrl+Y (vim/emacs yank convention), keeping Ctrl+K for Up navigation consistent with the command palette. Assisted-By: cagent
aheritier
force-pushed
the
fix/1741-session-browser-ctrl-k-conflict
branch
from
53e8d65 to
e3a8c59
Compare
3 participants
Summary
Fixes #1741
Ctrl+Kwas bound to both Up navigation and CopyID in the session browser dialog's key map. Since theUpcase is checked first in theUpdateswitch statement, theCopyIDhandler was unreachable — pressingCtrl+Kalways moved the selection up instead of copying the session ID.Changes
pkg/tui/dialog/session_browser.go: ReassignedCopyIDfromCtrl+KtoCtrl+Y(vim/emacs yank convention).Ctrl+Kremains bound toUpnavigation, consistent withCtrl+J/Ctrl+Kin the command palette. Updated help text accordingly.pkg/tui/dialog/session_browser_test.go: UpdatedTestSessionBrowserNavigationWithCtrlto verifyCtrl+Knavigates up, and added assertions thatCtrl+YmatchesCopyIDand does not change selection.Testing
go vetclean