fix: make setControllerPhase synchronous, dispatch modal timeouts via GTK main thread

The async glib.IdleAdd in setControllerPhase caused a race: after Ctrl+P
entered pane mode, the phase switch to capture was deferred to the next
idle cycle, so the follow-up 's' key still reached the WebView in bubble
phase. Fix by making setControllerPhase synchronous and instead dispatching
the timer's ExitMode call to the GTK thread via an injectable scheduler.

Also DRY-refactors modal.go: extracts shared enterMode() helper,
startTimeoutLocked() for glib dispatch, and SetMainThreadScheduler()
for testability (tests use direct execution, app wires glib.IdleAdd).

commit: ff5eb4fa041996fab7d4faa9c010a3217ee12520
author: brice · 7mo ago · 2026-03-13 06:50 UTC
parent: 9c71032
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

2 files · +64 -116

M internal/ui/input/keyboard.go +19 -9


@@ -134,19 +134,18 @@ func (h *KeyboardHandler) SetOnModeChange(fn func(from, to Mode)) {
134	134		}
135	135
136	136		// setControllerPhase changes the propagation phase of the key controller.
137		-	// Uses glib.IdleAdd because this may be called from timer goroutines
138		-	// (modal timeout) which are not on the GTK main thread.
	137	+	// Capture phase during modal modes so plain keys reach the handler before WebView.
	138	+	// Bubble phase in normal mode for WebKit IM/dead-key support.
	139	+	//
	140	+	// This is called synchronously from onModeChange so the phase switch takes
	141	+	// effect before the next key event. The caller must ensure GTK thread safety;
	142	+	// modal timeouts dispatch to the GTK main thread via glib.IdleAdd before
	143	+	// calling ExitMode, so the onModeChange callback always runs on the GTK thread.
139	144		func (h *KeyboardHandler) setControllerPhase(phase gtk.PropagationPhase) {
140	145		if h.controller == nil {
141	146		return
142	147		}
143		-	cb := glib.SourceFunc(func(_ uintptr) bool {
144		-	if h.controller != nil {
145		-	h.controller.SetPropagationPhase(phase)
146		-	}
147		-	return false
148		-	})
149		-	glib.IdleAdd(&cb, 0)
	148	+	h.controller.SetPropagationPhase(phase)
150	149		}
151	150
152	151		// SetRouteKey sets the callback that determines how each key event should be routed.

@@ -205,6 +204,17 @@ func (h KeyboardHandler) AttachTo(window gtk.ApplicationWindow) {
205	204		// so app shortcuts still work.
206	205		h.controller.SetPropagationPhase(gtk.PhaseBubbleValue)
207	206
	207	+	// Wire GTK main thread scheduler for modal timeouts. Timer goroutines
	208	+	// must dispatch ExitMode to the GTK thread because onModeChange may
	209	+	// call setControllerPhase (a GTK operation).
	210	+	h.modal.SetMainThreadScheduler(func(fn func()) {
	211	+	cb := glib.SourceFunc(func(_ uintptr) bool {
	212	+	fn()
	213	+	return false
	214	+	})
	215	+	glib.IdleAdd(&cb, 0)
	216	+	})
	217	+
208	218		// Connect key pressed handler (retain callback to prevent GC).
209	219		// The callback receives: keyval (translated key), keycode (hardware key position), state (modifiers)
210	220		h.keyPressedCb = func(_ gtk.EventControllerKey, keyval uint, keycode uint, state gdk.ModifierType) bool {

 }
 // setControllerPhase changes the propagation phase of the key controller.
-// Uses glib.IdleAdd because this may be called from timer goroutines
+// Capture phase during modal modes so plain keys reach the handler before WebView.
-// (modal timeout) which are not on the GTK main thread.
+// Bubble phase in normal mode for WebKit IM/dead-key support.
+//
+// This is called synchronously from onModeChange so the phase switch takes
+// effect before the next key event. The caller must ensure GTK thread safety;
+// modal timeouts dispatch to the GTK main thread via glib.IdleAdd before
+// calling ExitMode, so the onModeChange callback always runs on the GTK thread.
 func (h *KeyboardHandler) setControllerPhase(phase gtk.PropagationPhase) {
 	if h.controller == nil {
 		return
 	}
-	cb := glib.SourceFunc(func(_ uintptr) bool {
+	h.controller.SetPropagationPhase(phase)
-		if h.controller != nil {
-			h.controller.SetPropagationPhase(phase)
-		}
-		return false
-	})
-	glib.IdleAdd(&cb, 0)
 }
 // SetRouteKey sets the callback that determines how each key event should be routed.
 	// so app shortcuts still work.
 	h.controller.SetPropagationPhase(gtk.PhaseBubbleValue)
+	// Wire GTK main thread scheduler for modal timeouts. Timer goroutines
+	// must dispatch ExitMode to the GTK thread because onModeChange may
+	// call setControllerPhase (a GTK operation).
+	h.modal.SetMainThreadScheduler(func(fn func()) {
+		cb := glib.SourceFunc(func(_ uintptr) bool {
+			fn()
+			return false
+		})
+		glib.IdleAdd(&cb, 0)
+	})
 	// Connect key pressed handler (retain callback to prevent GC).
 	// The callback receives: keyval (translated key), keycode (hardware key position), state (modifiers)
 	h.keyPressedCb = func(_ gtk.EventControllerKey, keyval uint, keycode uint, state gdk.ModifierType) bool {

M internal/ui/input/modal.go +45 -107


@@ -67,6 +67,13 @@ type ModalState struct {
67	67		// Callback for mode changes (called synchronously under lock).
68	68		onModeChange func(from, to Mode)
69	69
	70	+	// scheduleOnMainThread dispatches a function to the GTK main thread.
	71	+	// Timer callbacks use this so that onModeChange (which may make GTK
	72	+	// calls like switching controller phase) always runs on the GTK thread.
	73	+	// Defaults to direct execution (suitable for tests without a GTK loop).
	74	+	// The app sets this to a glib.IdleAdd wrapper via SetMainThreadScheduler.
	75	+	scheduleOnMainThread func(fn func())
	76	+
70	77		mu sync.RWMutex
71	78		}
72	79

@@ -75,10 +82,20 @@ func NewModalState(ctx context.Context) *ModalState {
75	82		log := logging.FromContext(ctx)
76	83		log.Debug().Msg("creating modal state")
77	84		return &ModalState{
78		-	mode: ModeNormal,
	85	+	mode: ModeNormal,
	86	+	scheduleOnMainThread: func(fn func()) { fn() },
79	87		}
80	88		}
81	89
	90	+	// SetMainThreadScheduler sets the function used to dispatch timer callbacks
	91	+	// to the GTK main thread. In the real app this should be a glib.IdleAdd
	92	+	// wrapper. In tests, the default (direct execution) is used.
	93	+	func (m *ModalState) SetMainThreadScheduler(fn func(func())) {
	94	+	m.mu.Lock()
	95	+	defer m.mu.Unlock()
	96	+	m.scheduleOnMainThread = fn
	97	+	}
	98	+
82	99		// Mode returns the current mode (thread-safe).
83	100		func (m *ModalState) Mode() Mode {
84	101		m.mu.RLock()

@@ -87,142 +104,54 @@ func (m *ModalState) Mode() Mode {
87	104		}
88	105
89	106		// EnterTabMode switches to tab mode with an optional timeout.
90		-	// If timeout is 0, the mode stays until explicitly exited.
91	107		func (m *ModalState) EnterTabMode(ctx context.Context, timeout time.Duration) {
92		-	log := logging.FromContext(ctx)
93		-	m.mu.Lock()
94		-	defer m.mu.Unlock()
95		-
96		-	if m.mode == ModeTab {
97		-	// Already in tab mode, just reset timeout
98		-	m.resetTimeoutLocked(ctx, timeout)
99		-	return
100		-	}
101		-
102		-	m.cancelTimerLocked()
103		-	oldMode := m.mode
104		-	m.mode = ModeTab
105		-	m.timeout = timeout
106		-
107		-	if timeout > 0 {
108		-	m.timer = time.AfterFunc(timeout, func() {
109		-	m.ExitMode(ctx)
110		-	})
111		-	}
112		-
113		-	log.Debug().
114		-	Str("from", oldMode.String()).
115		-	Str("to", m.mode.String()).
116		-	Dur("timeout", timeout).
117		-	Msg("entered tab mode")
118		-
119		-	if m.onModeChange != nil {
120		-	m.onModeChange(oldMode, m.mode)
121		-	}
	108	+	m.enterMode(ctx, ModeTab, timeout)
122	109		}
123	110
124	111		// EnterPaneMode switches to pane mode with an optional timeout.
125		-	// If timeout is 0, the mode stays until explicitly exited.
126	112		func (m *ModalState) EnterPaneMode(ctx context.Context, timeout time.Duration) {
127		-	log := logging.FromContext(ctx)
128		-	m.mu.Lock()
129		-	defer m.mu.Unlock()
130		-
131		-	if m.mode == ModePane {
132		-	// Already in pane mode, just reset timeout
133		-	m.resetTimeoutLocked(ctx, timeout)
134		-	return
135		-	}
136		-
137		-	m.cancelTimerLocked()
138		-	oldMode := m.mode
139		-	m.mode = ModePane
140		-	m.timeout = timeout
141		-
142		-	if timeout > 0 {
143		-	m.timer = time.AfterFunc(timeout, func() {
144		-	m.ExitMode(ctx)
145		-	})
146		-	}
147		-
148		-	log.Debug().
149		-	Str("from", oldMode.String()).
150		-	Str("to", m.mode.String()).
151		-	Dur("timeout", timeout).
152		-	Msg("entered pane mode")
153		-
154		-	if m.onModeChange != nil {
155		-	m.onModeChange(oldMode, m.mode)
156		-	}
	113	+	m.enterMode(ctx, ModePane, timeout)
157	114		}
158	115
159	116		// EnterSessionMode switches to session mode with an optional timeout.
160		-	// If timeout is 0, the mode stays until explicitly exited.
161	117		func (m *ModalState) EnterSessionMode(ctx context.Context, timeout time.Duration) {
162		-	log := logging.FromContext(ctx)
163		-	m.mu.Lock()
164		-	defer m.mu.Unlock()
165		-
166		-	if m.mode == ModeSession {
167		-	// Already in session mode, just reset timeout
168		-	m.resetTimeoutLocked(ctx, timeout)
169		-	return
170		-	}
171		-
172		-	m.cancelTimerLocked()
173		-	oldMode := m.mode
174		-	m.mode = ModeSession
175		-	m.timeout = timeout
176		-
177		-	if timeout > 0 {
178		-	m.timer = time.AfterFunc(timeout, func() {
179		-	m.ExitMode(ctx)
180		-	})
181		-	}
182		-
183		-	log.Debug().
184		-	Str("from", oldMode.String()).
185		-	Str("to", m.mode.String()).
186		-	Dur("timeout", timeout).
187		-	Msg("entered session mode")
188		-
189		-	if m.onModeChange != nil {
190		-	m.onModeChange(oldMode, m.mode)
191		-	}
	118	+	m.enterMode(ctx, ModeSession, timeout)
192	119		}
193	120
194	121		// EnterResizeMode switches to resize mode with an optional timeout.
195		-	// If timeout is 0, the mode stays until explicitly exited.
196	122		func (m *ModalState) EnterResizeMode(ctx context.Context, timeout time.Duration) {
	123	+	m.enterMode(ctx, ModeResize, timeout)
	124	+	}
	125	+
	126	+	// enterMode is the shared implementation for all mode-enter methods.
	127	+	// If already in the target mode, it resets the timeout instead.
	128	+	func (m *ModalState) enterMode(ctx context.Context, target Mode, timeout time.Duration) {
197	129		log := logging.FromContext(ctx)
198	130		m.mu.Lock()
199	131		defer m.mu.Unlock()
200	132
201		-	if m.mode == ModeResize {
202		-	// Already in resize mode, just reset timeout
	133	+	if m.mode == target {
203	134		m.resetTimeoutLocked(ctx, timeout)
204	135		return
205	136		}
206	137
207	138		m.cancelTimerLocked()
208	139		oldMode := m.mode
209		-	m.mode = ModeResize
	140	+	m.mode = target
210	141		m.timeout = timeout
211	142
212	143		if timeout > 0 {
213		-	m.timer = time.AfterFunc(timeout, func() {
214		-	m.ExitMode(ctx)
215		-	})
	144	+	m.startTimeoutLocked(ctx, timeout)
216	145		}
217	146
218	147		log.Debug().
219	148		Str("from", oldMode.String()).
220		-	Str("to", m.mode.String()).
	149	+	Str("to", target.String()).
221	150		Dur("timeout", timeout).
222		-	Msg("entered resize mode")
	151	+	Msg("entered " + target.String() + " mode")
223	152
224	153		if m.onModeChange != nil {
225		-	m.onModeChange(oldMode, m.mode)
	154	+	m.onModeChange(oldMode, target)
226	155		}
227	156		}
228	157

@@ -281,14 +210,23 @@ func (m *ModalState) cancelTimerLocked() {
281	210		}
282	211		}
283	212
	213	+	// startTimeoutLocked starts a new timeout timer that exits the mode via the
	214	+	// main thread scheduler. Timer goroutines must not call ExitMode directly
	215	+	// because onModeChange may make GTK calls (e.g., switching controller phase).
	216	+	// Must be called with m.mu held.
	217	+	func (m *ModalState) startTimeoutLocked(ctx context.Context, timeout time.Duration) {
	218	+	schedule := m.scheduleOnMainThread
	219	+	m.timer = time.AfterFunc(timeout, func() {
	220	+	schedule(func() { m.ExitMode(ctx) })
	221	+	})
	222	+	}
	223	+
284	224		// resetTimeoutLocked resets the timeout timer.
285	225		// Must be called with m.mu held.
286	226		func (m *ModalState) resetTimeoutLocked(ctx context.Context, timeout time.Duration) {
287	227		m.cancelTimerLocked()
288	228		m.timeout = timeout
289	229		if timeout > 0 {
290		-	m.timer = time.AfterFunc(timeout, func() {
291		-	m.ExitMode(ctx)
292		-	})
	230	+	m.startTimeoutLocked(ctx, timeout)
293	231		}
294	232		}

 	// Callback for mode changes (called synchronously under lock).
 	onModeChange func(from, to Mode)
+	// scheduleOnMainThread dispatches a function to the GTK main thread.
+	// Timer callbacks use this so that onModeChange (which may make GTK
+	// calls like switching controller phase) always runs on the GTK thread.
+	// Defaults to direct execution (suitable for tests without a GTK loop).
+	// The app sets this to a glib.IdleAdd wrapper via SetMainThreadScheduler.
+	scheduleOnMainThread func(fn func())
 	mu sync.RWMutex
 }
 	log := logging.FromContext(ctx)
 	log.Debug().Msg("creating modal state")
 	return &ModalState{
-		mode: ModeNormal,
+		mode:                 ModeNormal,
+		scheduleOnMainThread: func(fn func()) { fn() },
 	}
 }
+// SetMainThreadScheduler sets the function used to dispatch timer callbacks
+// to the GTK main thread. In the real app this should be a glib.IdleAdd
+// wrapper. In tests, the default (direct execution) is used.
+func (m *ModalState) SetMainThreadScheduler(fn func(func())) {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	m.scheduleOnMainThread = fn
+}
 // Mode returns the current mode (thread-safe).
 func (m *ModalState) Mode() Mode {
 	m.mu.RLock()
 }
 // EnterTabMode switches to tab mode with an optional timeout.
-// If timeout is 0, the mode stays until explicitly exited.
 func (m *ModalState) EnterTabMode(ctx context.Context, timeout time.Duration) {
-	log := logging.FromContext(ctx)
+	m.enterMode(ctx, ModeTab, timeout)
-	m.mu.Lock()
-	defer m.mu.Unlock()
-	if m.mode == ModeTab {
-		// Already in tab mode, just reset timeout
-		m.resetTimeoutLocked(ctx, timeout)
-		return
-	}
-	m.cancelTimerLocked()
-	oldMode := m.mode
-	m.mode = ModeTab
-	m.timeout = timeout
-	if timeout > 0 {
-		m.timer = time.AfterFunc(timeout, func() {
-			m.ExitMode(ctx)
-		})
-	}
-	log.Debug().
-		Str("from", oldMode.String()).
-		Str("to", m.mode.String()).
-		Dur("timeout", timeout).
-		Msg("entered tab mode")
-	if m.onModeChange != nil {
-		m.onModeChange(oldMode, m.mode)
-	}
 }
 // EnterPaneMode switches to pane mode with an optional timeout.
-// If timeout is 0, the mode stays until explicitly exited.
 func (m *ModalState) EnterPaneMode(ctx context.Context, timeout time.Duration) {
-	log := logging.FromContext(ctx)
+	m.enterMode(ctx, ModePane, timeout)
-	m.mu.Lock()
-	defer m.mu.Unlock()
-	if m.mode == ModePane {
-		// Already in pane mode, just reset timeout
-		m.resetTimeoutLocked(ctx, timeout)
-		return
-	}
-	m.cancelTimerLocked()
-	oldMode := m.mode
-	m.mode = ModePane
-	m.timeout = timeout
-	if timeout > 0 {
-		m.timer = time.AfterFunc(timeout, func() {
-			m.ExitMode(ctx)
-		})
-	}
-	log.Debug().
-		Str("from", oldMode.String()).
-		Str("to", m.mode.String()).
-		Dur("timeout", timeout).
-		Msg("entered pane mode")
-	if m.onModeChange != nil {
-		m.onModeChange(oldMode, m.mode)
-	}
 }
 // EnterSessionMode switches to session mode with an optional timeout.
-// If timeout is 0, the mode stays until explicitly exited.
 func (m *ModalState) EnterSessionMode(ctx context.Context, timeout time.Duration) {
-	log := logging.FromContext(ctx)
+	m.enterMode(ctx, ModeSession, timeout)
-	m.mu.Lock()
-	defer m.mu.Unlock()
-	if m.mode == ModeSession {
-		// Already in session mode, just reset timeout
-		m.resetTimeoutLocked(ctx, timeout)
-		return
-	}
-	m.cancelTimerLocked()
-	oldMode := m.mode
-	m.mode = ModeSession
-	m.timeout = timeout
-	if timeout > 0 {
-		m.timer = time.AfterFunc(timeout, func() {
-			m.ExitMode(ctx)
-		})
-	}
-	log.Debug().
-		Str("from", oldMode.String()).
-		Str("to", m.mode.String()).
-		Dur("timeout", timeout).
-		Msg("entered session mode")
-	if m.onModeChange != nil {
-		m.onModeChange(oldMode, m.mode)
-	}
 }
 // EnterResizeMode switches to resize mode with an optional timeout.
-// If timeout is 0, the mode stays until explicitly exited.
 func (m *ModalState) EnterResizeMode(ctx context.Context, timeout time.Duration) {
+	m.enterMode(ctx, ModeResize, timeout)
+}
+// enterMode is the shared implementation for all mode-enter methods.
+// If already in the target mode, it resets the timeout instead.
+func (m *ModalState) enterMode(ctx context.Context, target Mode, timeout time.Duration) {
 	log := logging.FromContext(ctx)
 	m.mu.Lock()
 	defer m.mu.Unlock()
-	if m.mode == ModeResize {
+	if m.mode == target {
-		// Already in resize mode, just reset timeout
 		m.resetTimeoutLocked(ctx, timeout)
 		return
 	}
 	m.cancelTimerLocked()
 	oldMode := m.mode
-	m.mode = ModeResize
+	m.mode = target
 	m.timeout = timeout
 	if timeout > 0 {
-		m.timer = time.AfterFunc(timeout, func() {
+		m.startTimeoutLocked(ctx, timeout)
-			m.ExitMode(ctx)
-		})
 	}
 	log.Debug().
 		Str("from", oldMode.String()).
-		Str("to", m.mode.String()).
+		Str("to", target.String()).
 		Dur("timeout", timeout).
-		Msg("entered resize mode")
+		Msg("entered " + target.String() + " mode")
 	if m.onModeChange != nil {
-		m.onModeChange(oldMode, m.mode)
+		m.onModeChange(oldMode, target)
 	}
 }
 	}
 }
+// startTimeoutLocked starts a new timeout timer that exits the mode via the
+// main thread scheduler. Timer goroutines must not call ExitMode directly
+// because onModeChange may make GTK calls (e.g., switching controller phase).
+// Must be called with m.mu held.
+func (m *ModalState) startTimeoutLocked(ctx context.Context, timeout time.Duration) {
+	schedule := m.scheduleOnMainThread
+	m.timer = time.AfterFunc(timeout, func() {
+		schedule(func() { m.ExitMode(ctx) })
+	})
+}
 // resetTimeoutLocked resets the timeout timer.
 // Must be called with m.mu held.
 func (m *ModalState) resetTimeoutLocked(ctx context.Context, timeout time.Duration) {
 	m.cancelTimerLocked()
 	m.timeout = timeout
 	if timeout > 0 {
-		m.timer = time.AfterFunc(timeout, func() {
+		m.startTimeoutLocked(ctx, timeout)
-			m.ExitMode(ctx)
-		})
 	}
 }