git.iko.soy/crush
on main help sign in
fix(permissions): ignore all keys briefly after dialog opens (#3055)
7da9695Kieran Klukas
internal/ui/dialog/dialog.go+64−8
compare with main
unifiedsplit
@@ -1,6 +1,8 @@
1package dialog
1package dialog
2
2
3import (
3import (
4+ "time"
5+
4 "charm.land/bubbles/v2/key"
6 "charm.land/bubbles/v2/key"
5 tea "charm.land/bubbletea/v2"
7 tea "charm.land/bubbletea/v2"
6 "charm.land/lipgloss/v2"
8 "charm.land/lipgloss/v2"
@@ -47,9 +49,26 @@ type LoadingDialog interface {
47 StopLoading()
49 StopLoading()
48}
50}
49
51
52+// Grace period constants for dialogs that open asynchronously and may
53+// receive in-flight keystrokes from a previously focused component.
54+const (
55+ // graceQuietPeriod is how long input must be quiet before the dialog
56+ // arms. Each absorbed keystroke resets this timer.
57+ graceQuietPeriod = 200 * time.Millisecond
58+ // graceMaxDelay is the absolute ceiling: the dialog always arms after
59+ // this duration regardless of input activity. Prevents auto-repeat
60+ // from keeping the dialog disarmed indefinitely.
61+ graceMaxDelay = 1500 * time.Millisecond
62+)
63+
50// Overlay manages multiple dialogs as an overlay.
64// Overlay manages multiple dialogs as an overlay.
51type Overlay struct {
65type Overlay struct {
52 dialogs []Dialog
66 dialogs []Dialog
67+
68+ // Grace period state for the front dialog. Only active when the
69+ // dialog was opened via OpenDialogWithGrace.
70+ graceOpenedAt time.Time
71+ graceLastInputAt time.Time
53}
72}
54
73
55// NewOverlay creates a new [Overlay] instance.
74// NewOverlay creates a new [Overlay] instance.
@@ -77,6 +96,36 @@ func (d *Overlay) ContainsDialog(dialogID string) bool {
77// OpenDialog opens a new dialog to the stack.
96// OpenDialog opens a new dialog to the stack.
78func (d *Overlay) OpenDialog(dialog Dialog) {
97func (d *Overlay) OpenDialog(dialog Dialog) {
79 d.dialogs = append(d.dialogs, dialog)
98 d.dialogs = append(d.dialogs, dialog)
99+ d.graceOpenedAt = time.Time{}
100+ d.graceLastInputAt = time.Time{}
101+}
102+
103+// OpenDialogWithGrace opens a dialog with an input grace period. All
104+// keystrokes are absorbed until either the input has been quiet for
105+// graceQuietPeriod or graceMaxDelay has elapsed since opening, whichever
106+// comes first. Use this for dialogs that open asynchronously (e.g.
107+// permission prompts) where in-flight keystrokes from a previously
108+// focused component could act on the dialog before the user sees it.
109+func (d *Overlay) OpenDialogWithGrace(dialog Dialog) {
110+ now := time.Now()
111+ d.dialogs = append(d.dialogs, dialog)
112+ d.graceOpenedAt = now
113+ d.graceLastInputAt = now
114+}
115+
116+// inGracePeriod reports whether the front dialog is still within its
117+// input grace period. Returns false if no grace period is active.
118+func (d *Overlay) inGracePeriod() bool {
119+ if d.graceOpenedAt.IsZero() {
120+ return false
121+ }
122+ if time.Since(d.graceOpenedAt) >= graceMaxDelay {
123+ return false
124+ }
125+ if time.Since(d.graceLastInputAt) >= graceQuietPeriod {
126+ return false
127+ }
128+ return true
80}
129}
81
130
82// CloseDialog closes the dialog with the specified ID from the stack.
131// CloseDialog closes the dialog with the specified ID from the stack.
@@ -97,6 +146,15 @@ func (d *Overlay) CloseFrontDialog() {
97 d.removeDialog(len(d.dialogs) - 1)
146 d.removeDialog(len(d.dialogs) - 1)
98}
147}
99
148
149+func (d *Overlay) removeDialog(idx int) {
150+ d.dialogs = append(d.dialogs[:idx], d.dialogs[idx+1:]...)
151+ // Clear grace state when the front dialog changes.
152+ if idx == len(d.dialogs) {
153+ d.graceOpenedAt = time.Time{}
154+ d.graceLastInputAt = time.Time{}
155+ }
156+}
157+
100// Dialog returns the dialog with the specified ID, or nil if not found.
158// Dialog returns the dialog with the specified ID, or nil if not found.
101func (d *Overlay) Dialog(dialogID string) Dialog {
159func (d *Overlay) Dialog(dialogID string) Dialog {
102 for _, dialog := range d.dialogs {
160 for _, dialog := range d.dialogs {
@@ -133,6 +191,12 @@ func (d *Overlay) Update(msg tea.Msg) tea.Msg {
133 return nil
191 return nil
134 }
192 }
135
193
194+ // Absorb keystrokes during the grace period for async dialogs.
195+ if _, ok := msg.(tea.KeyPressMsg); ok && d.inGracePeriod() {
196+ d.graceLastInputAt = time.Now()
197+ return nil
198+ }
199+
136 idx := len(d.dialogs) - 1 // active dialog is the last one
200 idx := len(d.dialogs) - 1 // active dialog is the last one
137 dialog := d.dialogs[idx]
201 dialog := d.dialogs[idx]
138 if dialog == nil {
202 if dialog == nil {
@@ -203,11 +267,3 @@ func (d *Overlay) Draw(scr uv.Screen, area uv.Rectangle) *tea.Cursor {
203 }
267 }
204 return cur
268 return cur
205}
269}
206−
207−// removeDialog removes a dialog from the stack.
208−func (d *Overlay) removeDialog(idx int) {
209− if idx < 0 || idx >= len(d.dialogs) {
210− return
211− }
212− d.dialogs = append(d.dialogs[:idx], d.dialogs[idx+1:]...)
213−}
internal/ui/dialog/overlay_test.gonew file+131
@@ -0,0 +1,131 @@
1+package dialog
2+
3+import (
4+ "testing"
5+ "time"
6+
7+ tea "charm.land/bubbletea/v2"
8+ uv "github.com/charmbracelet/ultraviolet"
9+ "github.com/stretchr/testify/require"
10+)
11+
12+// stubDialog is a minimal Dialog for testing Overlay behavior.
13+type stubDialog struct {
14+ id string
15+ received []tea.Msg
16+}
17+
18+func (s *stubDialog) ID() string { return s.id }
19+func (s *stubDialog) HandleMsg(msg tea.Msg) Action {
20+ s.received = append(s.received, msg)
21+ return nil
22+}
23+func (s *stubDialog) Draw(_ uv.Screen, _ uv.Rectangle) *tea.Cursor { return nil }
24+
25+func keyMsg(r rune) tea.KeyPressMsg {
26+ return tea.KeyPressMsg{Code: r, Text: string(r)}
27+}
28+
29+// TestOverlay_GracePeriodSwallowsKeys verifies that all keystrokes
30+// arriving within the grace period after OpenDialogWithGrace are absorbed
31+// and never forwarded to the dialog.
32+func TestOverlay_GracePeriodSwallowsKeys(t *testing.T) {
33+ t.Parallel()
34+
35+ d := &stubDialog{id: "test"}
36+ o := NewOverlay()
37+ o.OpenDialogWithGrace(d)
38+
39+ for _, r := range []rune{'a', 's', 'd', 'x', 'z'} {
40+ o.Update(keyMsg(r))
41+ }
42+ require.Empty(t, d.received, "no keys should reach the dialog during grace period")
43+}
44+
45+// TestOverlay_GracePeriodArmsAfterQuiet verifies that once input has been
46+// quiet for graceQuietPeriod, subsequent keys are forwarded normally.
47+func TestOverlay_GracePeriodArmsAfterQuiet(t *testing.T) {
48+ t.Parallel()
49+
50+ d := &stubDialog{id: "test"}
51+ o := NewOverlay()
52+ o.OpenDialogWithGrace(d)
53+
54+ // Backdate so both deadlines have elapsed.
55+ o.graceOpenedAt = time.Now().Add(-graceMaxDelay - time.Millisecond)
56+ o.graceLastInputAt = time.Now().Add(-graceQuietPeriod - time.Millisecond)
57+
58+ o.Update(keyMsg('a'))
59+ require.Len(t, d.received, 1, "key after grace period should reach the dialog")
60+}
61+
62+// TestOverlay_GracePeriodBurstExtendsQuietWindow verifies that a sustained
63+// burst of keystrokes keeps resetting the quiet timer, but the fixed
64+// ceiling (graceMaxDelay) eventually arms the dialog regardless.
65+func TestOverlay_GracePeriodBurstExtendsQuietWindow(t *testing.T) {
66+ t.Parallel()
67+
68+ d := &stubDialog{id: "test"}
69+ o := NewOverlay()
70+ o.OpenDialogWithGrace(d)
71+
72+ // Simulate keys arriving every 40ms (within graceQuietPeriod).
73+ // Each one resets the quiet timer, but we stay under graceMaxDelay.
74+ for range 5 {
75+ o.graceLastInputAt = time.Now().Add(-40 * time.Millisecond)
76+ o.Update(keyMsg('a'))
77+ }
78+ require.Empty(t, d.received, "burst within max delay should be absorbed")
79+
80+ // Now exceed the max delay ceiling. Even though lastInputAt is recent,
81+ // the fixed deadline forces arming.
82+ o.graceOpenedAt = time.Now().Add(-graceMaxDelay - time.Millisecond)
83+ o.graceLastInputAt = time.Now() // just typed, but max delay wins
84+ o.Update(keyMsg('b'))
85+ require.Len(t, d.received, 1, "key after max delay should reach dialog even during burst")
86+}
87+
88+// TestOverlay_OpenDialogWithoutGraceHasNoGuard verifies that dialogs
89+// opened via OpenDialog (without grace) receive keys immediately.
90+func TestOverlay_OpenDialogWithoutGraceHasNoGuard(t *testing.T) {
91+ t.Parallel()
92+
93+ d := &stubDialog{id: "test"}
94+ o := NewOverlay()
95+ o.OpenDialog(d)
96+
97+ o.Update(keyMsg('a'))
98+ require.Len(t, d.received, 1, "dialog without grace should receive keys immediately")
99+}
100+
101+// TestOverlay_GraceClearedOnClose verifies that closing the front dialog
102+// clears grace state so a subsequently opened dialog without grace is
103+// not affected.
104+func TestOverlay_GraceClearedOnClose(t *testing.T) {
105+ t.Parallel()
106+
107+ d1 := &stubDialog{id: "first"}
108+ d2 := &stubDialog{id: "second"}
109+ o := NewOverlay()
110+ o.OpenDialogWithGrace(d1)
111+
112+ // Close the grace dialog and open a normal one.
113+ o.CloseFrontDialog()
114+ o.OpenDialog(d2)
115+
116+ o.Update(keyMsg('a'))
117+ require.Len(t, d2.received, 1, "new dialog after grace close should receive keys immediately")
118+}
119+
120+// TestOverlay_NonKeyMessagesPassDuringGrace verifies that non-keypress
121+// messages (e.g. mouse, tick) are forwarded even during the grace period.
122+func TestOverlay_NonKeyMessagesPassDuringGrace(t *testing.T) {
123+ t.Parallel()
124+
125+ d := &stubDialog{id: "test"}
126+ o := NewOverlay()
127+ o.OpenDialogWithGrace(d)
128+
129+ o.Update(tea.MouseWheelMsg{})
130+ require.Len(t, d.received, 1, "non-key messages should pass through during grace")
131+}
internal/ui/dialog/permissions_test.gonew file+98
@@ -0,0 +1,98 @@
1+package dialog
2+
3+import (
4+ "testing"
5+
6+ tea "charm.land/bubbletea/v2"
7+ "github.com/charmbracelet/crush/internal/permission"
8+ "github.com/charmbracelet/crush/internal/ui/common"
9+ "github.com/charmbracelet/crush/internal/ui/styles"
10+ "github.com/stretchr/testify/require"
11+)
12+
13+func newTestPermissions(t *testing.T) *Permissions {
14+ t.Helper()
15+ s := styles.CharmtonePantera()
16+ com := &common.Common{Styles: &s}
17+ perm := permission.PermissionRequest{
18+ ID: "perm-test",
19+ ToolCallID: "tool-call-test",
20+ ToolName: "bash",
21+ }
22+ return NewPermissions(com, perm)
23+}
24+
25+// TestPermissions_ActionKeysResolve verifies that action keys produce the
26+// correct permission response.
27+func TestPermissions_ActionKeysResolve(t *testing.T) {
28+ t.Parallel()
29+
30+ tests := []struct {
31+ key tea.KeyPressMsg
32+ action PermissionAction
33+ }{
34+ {keyMsg('a'), PermissionAllow},
35+ {keyMsg('A'), PermissionAllow},
36+ {keyMsg('d'), PermissionDeny},
37+ {keyMsg('D'), PermissionDeny},
38+ {keyMsg('s'), PermissionAllowForSession},
39+ {keyMsg('S'), PermissionAllowForSession},
40+ }
41+
42+ for _, tc := range tests {
43+ p := newTestPermissions(t)
44+ action := p.HandleMsg(tc.key)
45+ resp, ok := action.(ActionPermissionResponse)
46+ require.Truef(t, ok, "key %q should produce ActionPermissionResponse", tc.key.Text)
47+ require.Equal(t, tc.action, resp.Action)
48+ }
49+}
50+
51+// TestPermissions_NavigationCyclesOptions verifies that tab and arrow keys
52+// cycle through the three permission options.
53+func TestPermissions_NavigationCyclesOptions(t *testing.T) {
54+ t.Parallel()
55+
56+ p := newTestPermissions(t)
57+ require.Equal(t, 0, p.selectedOption)
58+
59+ // Tab cycles forward.
60+ p.HandleMsg(tea.KeyPressMsg{Code: tea.KeyTab})
61+ require.Equal(t, 1, p.selectedOption)
62+
63+ p.HandleMsg(tea.KeyPressMsg{Code: tea.KeyTab})
64+ require.Equal(t, 2, p.selectedOption)
65+
66+ // Wrap around.
67+ p.HandleMsg(tea.KeyPressMsg{Code: tea.KeyTab})
68+ require.Equal(t, 0, p.selectedOption)
69+
70+ // Left cycles backward.
71+ p.HandleMsg(keyMsg('h'))
72+ require.Equal(t, 2, p.selectedOption)
73+}
74+
75+// TestPermissions_EnterConfirmsSelection verifies that enter confirms the
76+// currently selected option.
77+func TestPermissions_EnterConfirmsSelection(t *testing.T) {
78+ t.Parallel()
79+
80+ p := newTestPermissions(t)
81+ p.selectedOption = 1 // Allow for session.
82+
83+ action := p.HandleMsg(tea.KeyPressMsg{Code: tea.KeyEnter})
84+ resp, ok := action.(ActionPermissionResponse)
85+ require.True(t, ok)
86+ require.Equal(t, PermissionAllowForSession, resp.Action)
87+}
88+
89+// TestPermissions_EscapeDenies verifies that escape denies the request.
90+func TestPermissions_EscapeDenies(t *testing.T) {
91+ t.Parallel()
92+
93+ p := newTestPermissions(t)
94+ action := p.HandleMsg(tea.KeyPressMsg{Code: tea.KeyEscape})
95+ resp, ok := action.(ActionPermissionResponse)
96+ require.True(t, ok)
97+ require.Equal(t, PermissionDeny, resp.Action)
98+}
internal/ui/model/permission_test.go+4−4
@@ -25,7 +25,7 @@ func TestHandlePermissionNotification_RemoteGrantClosesDialog(t *testing.T) {
25 ToolCallID: "tool-call-X",
25 ToolCallID: "tool-call-X",
26 ToolName: "bash",
26 ToolName: "bash",
27 }
27 }
28− u.dialog.OpenDialog(dialog.NewPermissions(u.com, perm))
28+ u.dialog.OpenDialogWithGrace(dialog.NewPermissions(u.com, perm))
29 require.True(t, u.dialog.ContainsDialog(dialog.PermissionsID))
29 require.True(t, u.dialog.ContainsDialog(dialog.PermissionsID))
30
30
31 u.handlePermissionNotification(permission.PermissionNotification{
31 u.handlePermissionNotification(permission.PermissionNotification{
@@ -45,7 +45,7 @@ func TestHandlePermissionNotification_RemoteDenyClosesDialog(t *testing.T) {
45 ID: "perm-2",
45 ID: "perm-2",
46 ToolCallID: "tool-call-Y",
46 ToolCallID: "tool-call-Y",
47 }
47 }
48− u.dialog.OpenDialog(dialog.NewPermissions(u.com, perm))
48+ u.dialog.OpenDialogWithGrace(dialog.NewPermissions(u.com, perm))
49
49
50 u.handlePermissionNotification(permission.PermissionNotification{
50 u.handlePermissionNotification(permission.PermissionNotification{
51 ToolCallID: "tool-call-Y",
51 ToolCallID: "tool-call-Y",
@@ -64,7 +64,7 @@ func TestHandlePermissionNotification_InitialPendingDoesNotClose(t *testing.T) {
64 ID: "perm-3",
64 ID: "perm-3",
65 ToolCallID: "tool-call-Z",
65 ToolCallID: "tool-call-Z",
66 }
66 }
67− u.dialog.OpenDialog(dialog.NewPermissions(u.com, perm))
67+ u.dialog.OpenDialogWithGrace(dialog.NewPermissions(u.com, perm))
68
68
69 // The initial notification published by permission.Request is
69 // The initial notification published by permission.Request is
70 // neither granted nor denied; it must not dismiss the dialog.
70 // neither granted nor denied; it must not dismiss the dialog.
@@ -84,7 +84,7 @@ func TestHandlePermissionNotification_DifferentToolCallIDDoesNotClose(t *testing
84 ID: "perm-4",
84 ID: "perm-4",
85 ToolCallID: "tool-call-A",
85 ToolCallID: "tool-call-A",
86 }
86 }
87− u.dialog.OpenDialog(dialog.NewPermissions(u.com, perm))
87+ u.dialog.OpenDialogWithGrace(dialog.NewPermissions(u.com, perm))
88
88
89 u.handlePermissionNotification(permission.PermissionNotification{
89 u.handlePermissionNotification(permission.PermissionNotification{
90 ToolCallID: "tool-call-B",
90 ToolCallID: "tool-call-B",
internal/ui/model/ui.go+2−2
@@ -1808,7 +1808,7 @@ func (m *UI) openAuthenticationDialog(provider catwalk.Provider, model config.Se
1808 return nil
1808 return nil
1809 }
1809 }
1810
1810
1811− m.dialog.OpenDialog(dlg)
1811+ m.dialog.OpenDialogWithGrace(dlg)
1812 return cmd
1812 return cmd
1813}
1813}
1814
1814
@@ -3519,7 +3519,7 @@ func (m *UI) openPermissionsDialog(perm permission.PermissionRequest) tea.Cmd {
3519 }
3519 }
3520
3520
3521 permDialog := dialog.NewPermissions(m.com, perm, opts...)
3521 permDialog := dialog.NewPermissions(m.com, perm, opts...)
3522− m.dialog.OpenDialog(permDialog)
3522+ m.dialog.OpenDialogWithGrace(permDialog)
3523 return nil
3523 return nil
3524}
3524}
3525
3525