fix(permissions): ignore all keys briefly after dialog opens (#3055)

Kieran Klukas created

Change summary

internal/ui/dialog/dialog.go           |  72 +++++++++++++-
internal/ui/dialog/overlay_test.go     | 131 ++++++++++++++++++++++++++++
internal/ui/dialog/permissions_test.go |  98 ++++++++++++++++++++
internal/ui/model/permission_test.go   |   8 
internal/ui/model/ui.go                |   4 
5 files changed, 299 insertions(+), 14 deletions(-)

Detailed changes

internal/ui/dialog/dialog.go 🔗

@@ -1,6 +1,8 @@
 package dialog
 
 import (
+	"time"
+
 	"charm.land/bubbles/v2/key"
 	tea "charm.land/bubbletea/v2"
 	"charm.land/lipgloss/v2"
@@ -47,9 +49,26 @@ type LoadingDialog interface {
 	StopLoading()
 }
 
+// Grace period constants for dialogs that open asynchronously and may
+// receive in-flight keystrokes from a previously focused component.
+const (
+	// graceQuietPeriod is how long input must be quiet before the dialog
+	// arms. Each absorbed keystroke resets this timer.
+	graceQuietPeriod = 200 * time.Millisecond
+	// graceMaxDelay is the absolute ceiling: the dialog always arms after
+	// this duration regardless of input activity. Prevents auto-repeat
+	// from keeping the dialog disarmed indefinitely.
+	graceMaxDelay = 1500 * time.Millisecond
+)
+
 // Overlay manages multiple dialogs as an overlay.
 type Overlay struct {
 	dialogs []Dialog
+
+	// Grace period state for the front dialog. Only active when the
+	// dialog was opened via OpenDialogWithGrace.
+	graceOpenedAt    time.Time
+	graceLastInputAt time.Time
 }
 
 // NewOverlay creates a new [Overlay] instance.
@@ -77,6 +96,36 @@ func (d *Overlay) ContainsDialog(dialogID string) bool {
 // OpenDialog opens a new dialog to the stack.
 func (d *Overlay) OpenDialog(dialog Dialog) {
 	d.dialogs = append(d.dialogs, dialog)
+	d.graceOpenedAt = time.Time{}
+	d.graceLastInputAt = time.Time{}
+}
+
+// OpenDialogWithGrace opens a dialog with an input grace period. All
+// keystrokes are absorbed until either the input has been quiet for
+// graceQuietPeriod or graceMaxDelay has elapsed since opening, whichever
+// comes first. Use this for dialogs that open asynchronously (e.g.
+// permission prompts) where in-flight keystrokes from a previously
+// focused component could act on the dialog before the user sees it.
+func (d *Overlay) OpenDialogWithGrace(dialog Dialog) {
+	now := time.Now()
+	d.dialogs = append(d.dialogs, dialog)
+	d.graceOpenedAt = now
+	d.graceLastInputAt = now
+}
+
+// inGracePeriod reports whether the front dialog is still within its
+// input grace period. Returns false if no grace period is active.
+func (d *Overlay) inGracePeriod() bool {
+	if d.graceOpenedAt.IsZero() {
+		return false
+	}
+	if time.Since(d.graceOpenedAt) >= graceMaxDelay {
+		return false
+	}
+	if time.Since(d.graceLastInputAt) >= graceQuietPeriod {
+		return false
+	}
+	return true
 }
 
 // CloseDialog closes the dialog with the specified ID from the stack.
@@ -97,6 +146,15 @@ func (d *Overlay) CloseFrontDialog() {
 	d.removeDialog(len(d.dialogs) - 1)
 }
 
+func (d *Overlay) removeDialog(idx int) {
+	d.dialogs = append(d.dialogs[:idx], d.dialogs[idx+1:]...)
+	// Clear grace state when the front dialog changes.
+	if idx == len(d.dialogs) {
+		d.graceOpenedAt = time.Time{}
+		d.graceLastInputAt = time.Time{}
+	}
+}
+
 // Dialog returns the dialog with the specified ID, or nil if not found.
 func (d *Overlay) Dialog(dialogID string) Dialog {
 	for _, dialog := range d.dialogs {
@@ -133,6 +191,12 @@ func (d *Overlay) Update(msg tea.Msg) tea.Msg {
 		return nil
 	}
 
+	// Absorb keystrokes during the grace period for async dialogs.
+	if _, ok := msg.(tea.KeyPressMsg); ok && d.inGracePeriod() {
+		d.graceLastInputAt = time.Now()
+		return nil
+	}
+
 	idx := len(d.dialogs) - 1 // active dialog is the last one
 	dialog := d.dialogs[idx]
 	if dialog == nil {
@@ -203,11 +267,3 @@ func (d *Overlay) Draw(scr uv.Screen, area uv.Rectangle) *tea.Cursor {
 	}
 	return cur
 }
-
-// removeDialog removes a dialog from the stack.
-func (d *Overlay) removeDialog(idx int) {
-	if idx < 0 || idx >= len(d.dialogs) {
-		return
-	}
-	d.dialogs = append(d.dialogs[:idx], d.dialogs[idx+1:]...)
-}

internal/ui/dialog/overlay_test.go 🔗

@@ -0,0 +1,131 @@
+package dialog
+
+import (
+	"testing"
+	"time"
+
+	tea "charm.land/bubbletea/v2"
+	uv "github.com/charmbracelet/ultraviolet"
+	"github.com/stretchr/testify/require"
+)
+
+// stubDialog is a minimal Dialog for testing Overlay behavior.
+type stubDialog struct {
+	id       string
+	received []tea.Msg
+}
+
+func (s *stubDialog) ID() string { return s.id }
+func (s *stubDialog) HandleMsg(msg tea.Msg) Action {
+	s.received = append(s.received, msg)
+	return nil
+}
+func (s *stubDialog) Draw(_ uv.Screen, _ uv.Rectangle) *tea.Cursor { return nil }
+
+func keyMsg(r rune) tea.KeyPressMsg {
+	return tea.KeyPressMsg{Code: r, Text: string(r)}
+}
+
+// TestOverlay_GracePeriodSwallowsKeys verifies that all keystrokes
+// arriving within the grace period after OpenDialogWithGrace are absorbed
+// and never forwarded to the dialog.
+func TestOverlay_GracePeriodSwallowsKeys(t *testing.T) {
+	t.Parallel()
+
+	d := &stubDialog{id: "test"}
+	o := NewOverlay()
+	o.OpenDialogWithGrace(d)
+
+	for _, r := range []rune{'a', 's', 'd', 'x', 'z'} {
+		o.Update(keyMsg(r))
+	}
+	require.Empty(t, d.received, "no keys should reach the dialog during grace period")
+}
+
+// TestOverlay_GracePeriodArmsAfterQuiet verifies that once input has been
+// quiet for graceQuietPeriod, subsequent keys are forwarded normally.
+func TestOverlay_GracePeriodArmsAfterQuiet(t *testing.T) {
+	t.Parallel()
+
+	d := &stubDialog{id: "test"}
+	o := NewOverlay()
+	o.OpenDialogWithGrace(d)
+
+	// Backdate so both deadlines have elapsed.
+	o.graceOpenedAt = time.Now().Add(-graceMaxDelay - time.Millisecond)
+	o.graceLastInputAt = time.Now().Add(-graceQuietPeriod - time.Millisecond)
+
+	o.Update(keyMsg('a'))
+	require.Len(t, d.received, 1, "key after grace period should reach the dialog")
+}
+
+// TestOverlay_GracePeriodBurstExtendsQuietWindow verifies that a sustained
+// burst of keystrokes keeps resetting the quiet timer, but the fixed
+// ceiling (graceMaxDelay) eventually arms the dialog regardless.
+func TestOverlay_GracePeriodBurstExtendsQuietWindow(t *testing.T) {
+	t.Parallel()
+
+	d := &stubDialog{id: "test"}
+	o := NewOverlay()
+	o.OpenDialogWithGrace(d)
+
+	// Simulate keys arriving every 40ms (within graceQuietPeriod).
+	// Each one resets the quiet timer, but we stay under graceMaxDelay.
+	for range 5 {
+		o.graceLastInputAt = time.Now().Add(-40 * time.Millisecond)
+		o.Update(keyMsg('a'))
+	}
+	require.Empty(t, d.received, "burst within max delay should be absorbed")
+
+	// Now exceed the max delay ceiling. Even though lastInputAt is recent,
+	// the fixed deadline forces arming.
+	o.graceOpenedAt = time.Now().Add(-graceMaxDelay - time.Millisecond)
+	o.graceLastInputAt = time.Now() // just typed, but max delay wins
+	o.Update(keyMsg('b'))
+	require.Len(t, d.received, 1, "key after max delay should reach dialog even during burst")
+}
+
+// TestOverlay_OpenDialogWithoutGraceHasNoGuard verifies that dialogs
+// opened via OpenDialog (without grace) receive keys immediately.
+func TestOverlay_OpenDialogWithoutGraceHasNoGuard(t *testing.T) {
+	t.Parallel()
+
+	d := &stubDialog{id: "test"}
+	o := NewOverlay()
+	o.OpenDialog(d)
+
+	o.Update(keyMsg('a'))
+	require.Len(t, d.received, 1, "dialog without grace should receive keys immediately")
+}
+
+// TestOverlay_GraceClearedOnClose verifies that closing the front dialog
+// clears grace state so a subsequently opened dialog without grace is
+// not affected.
+func TestOverlay_GraceClearedOnClose(t *testing.T) {
+	t.Parallel()
+
+	d1 := &stubDialog{id: "first"}
+	d2 := &stubDialog{id: "second"}
+	o := NewOverlay()
+	o.OpenDialogWithGrace(d1)
+
+	// Close the grace dialog and open a normal one.
+	o.CloseFrontDialog()
+	o.OpenDialog(d2)
+
+	o.Update(keyMsg('a'))
+	require.Len(t, d2.received, 1, "new dialog after grace close should receive keys immediately")
+}
+
+// TestOverlay_NonKeyMessagesPassDuringGrace verifies that non-keypress
+// messages (e.g. mouse, tick) are forwarded even during the grace period.
+func TestOverlay_NonKeyMessagesPassDuringGrace(t *testing.T) {
+	t.Parallel()
+
+	d := &stubDialog{id: "test"}
+	o := NewOverlay()
+	o.OpenDialogWithGrace(d)
+
+	o.Update(tea.MouseWheelMsg{})
+	require.Len(t, d.received, 1, "non-key messages should pass through during grace")
+}

internal/ui/dialog/permissions_test.go 🔗

@@ -0,0 +1,98 @@
+package dialog
+
+import (
+	"testing"
+
+	tea "charm.land/bubbletea/v2"
+	"github.com/charmbracelet/crush/internal/permission"
+	"github.com/charmbracelet/crush/internal/ui/common"
+	"github.com/charmbracelet/crush/internal/ui/styles"
+	"github.com/stretchr/testify/require"
+)
+
+func newTestPermissions(t *testing.T) *Permissions {
+	t.Helper()
+	s := styles.CharmtonePantera()
+	com := &common.Common{Styles: &s}
+	perm := permission.PermissionRequest{
+		ID:         "perm-test",
+		ToolCallID: "tool-call-test",
+		ToolName:   "bash",
+	}
+	return NewPermissions(com, perm)
+}
+
+// TestPermissions_ActionKeysResolve verifies that action keys produce the
+// correct permission response.
+func TestPermissions_ActionKeysResolve(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		key    tea.KeyPressMsg
+		action PermissionAction
+	}{
+		{keyMsg('a'), PermissionAllow},
+		{keyMsg('A'), PermissionAllow},
+		{keyMsg('d'), PermissionDeny},
+		{keyMsg('D'), PermissionDeny},
+		{keyMsg('s'), PermissionAllowForSession},
+		{keyMsg('S'), PermissionAllowForSession},
+	}
+
+	for _, tc := range tests {
+		p := newTestPermissions(t)
+		action := p.HandleMsg(tc.key)
+		resp, ok := action.(ActionPermissionResponse)
+		require.Truef(t, ok, "key %q should produce ActionPermissionResponse", tc.key.Text)
+		require.Equal(t, tc.action, resp.Action)
+	}
+}
+
+// TestPermissions_NavigationCyclesOptions verifies that tab and arrow keys
+// cycle through the three permission options.
+func TestPermissions_NavigationCyclesOptions(t *testing.T) {
+	t.Parallel()
+
+	p := newTestPermissions(t)
+	require.Equal(t, 0, p.selectedOption)
+
+	// Tab cycles forward.
+	p.HandleMsg(tea.KeyPressMsg{Code: tea.KeyTab})
+	require.Equal(t, 1, p.selectedOption)
+
+	p.HandleMsg(tea.KeyPressMsg{Code: tea.KeyTab})
+	require.Equal(t, 2, p.selectedOption)
+
+	// Wrap around.
+	p.HandleMsg(tea.KeyPressMsg{Code: tea.KeyTab})
+	require.Equal(t, 0, p.selectedOption)
+
+	// Left cycles backward.
+	p.HandleMsg(keyMsg('h'))
+	require.Equal(t, 2, p.selectedOption)
+}
+
+// TestPermissions_EnterConfirmsSelection verifies that enter confirms the
+// currently selected option.
+func TestPermissions_EnterConfirmsSelection(t *testing.T) {
+	t.Parallel()
+
+	p := newTestPermissions(t)
+	p.selectedOption = 1 // Allow for session.
+
+	action := p.HandleMsg(tea.KeyPressMsg{Code: tea.KeyEnter})
+	resp, ok := action.(ActionPermissionResponse)
+	require.True(t, ok)
+	require.Equal(t, PermissionAllowForSession, resp.Action)
+}
+
+// TestPermissions_EscapeDenies verifies that escape denies the request.
+func TestPermissions_EscapeDenies(t *testing.T) {
+	t.Parallel()
+
+	p := newTestPermissions(t)
+	action := p.HandleMsg(tea.KeyPressMsg{Code: tea.KeyEscape})
+	resp, ok := action.(ActionPermissionResponse)
+	require.True(t, ok)
+	require.Equal(t, PermissionDeny, resp.Action)
+}

internal/ui/model/permission_test.go 🔗

@@ -25,7 +25,7 @@ func TestHandlePermissionNotification_RemoteGrantClosesDialog(t *testing.T) {
 		ToolCallID: "tool-call-X",
 		ToolName:   "bash",
 	}
-	u.dialog.OpenDialog(dialog.NewPermissions(u.com, perm))
+	u.dialog.OpenDialogWithGrace(dialog.NewPermissions(u.com, perm))
 	require.True(t, u.dialog.ContainsDialog(dialog.PermissionsID))
 
 	u.handlePermissionNotification(permission.PermissionNotification{
@@ -45,7 +45,7 @@ func TestHandlePermissionNotification_RemoteDenyClosesDialog(t *testing.T) {
 		ID:         "perm-2",
 		ToolCallID: "tool-call-Y",
 	}
-	u.dialog.OpenDialog(dialog.NewPermissions(u.com, perm))
+	u.dialog.OpenDialogWithGrace(dialog.NewPermissions(u.com, perm))
 
 	u.handlePermissionNotification(permission.PermissionNotification{
 		ToolCallID: "tool-call-Y",
@@ -64,7 +64,7 @@ func TestHandlePermissionNotification_InitialPendingDoesNotClose(t *testing.T) {
 		ID:         "perm-3",
 		ToolCallID: "tool-call-Z",
 	}
-	u.dialog.OpenDialog(dialog.NewPermissions(u.com, perm))
+	u.dialog.OpenDialogWithGrace(dialog.NewPermissions(u.com, perm))
 
 	// The initial notification published by permission.Request is
 	// neither granted nor denied; it must not dismiss the dialog.
@@ -84,7 +84,7 @@ func TestHandlePermissionNotification_DifferentToolCallIDDoesNotClose(t *testing
 		ID:         "perm-4",
 		ToolCallID: "tool-call-A",
 	}
-	u.dialog.OpenDialog(dialog.NewPermissions(u.com, perm))
+	u.dialog.OpenDialogWithGrace(dialog.NewPermissions(u.com, perm))
 
 	u.handlePermissionNotification(permission.PermissionNotification{
 		ToolCallID: "tool-call-B",

internal/ui/model/ui.go 🔗

@@ -1808,7 +1808,7 @@ func (m *UI) openAuthenticationDialog(provider catwalk.Provider, model config.Se
 		return nil
 	}
 
-	m.dialog.OpenDialog(dlg)
+	m.dialog.OpenDialogWithGrace(dlg)
 	return cmd
 }
 
@@ -3519,7 +3519,7 @@ func (m *UI) openPermissionsDialog(perm permission.PermissionRequest) tea.Cmd {
 	}
 
 	permDialog := dialog.NewPermissions(m.com, perm, opts...)
-	m.dialog.OpenDialog(permDialog)
+	m.dialog.OpenDialogWithGrace(permDialog)
 	return nil
 }