refactor(picker): tighten sort toggle, drop dead wrapper, sharpen tests

Replace the non-atomic pickerSort ^ 1 toggle with a named CompareAndSwap
loop that can't corrupt the mode or lose a concurrent double-press.
Delete the now-callerless refreshPicker wrapper in favor of calling
refreshPickerOpts directly. Fix SelectedIndex's doc comment to match its
actual -1-only-when-nil-or-empty behavior, and make the
SelectNearestRow "negative clamps to first selectable" test case start
from a non-zero selection so it can't pass against a no-op.

commit: 5720a2c6f0d3465733b621103b770f4a885c207e
author: brice · 2mo ago · 2026-07-29 14:42 UTC
parent: ae3ded7
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

5 files · +44 -15

M internal/usecase/daemon/move_picker.go +0 -4


@@ -146,10 +146,6 @@ type pickerRefreshOptions struct {
146	146		nearestRow int
147	147		}
148	148
149		-	func (d Daemon) refreshPicker(ac attachedClient) {
150		-	d.refreshPickerOpts(ac, pickerRefreshOptions{nearestRow: -1})
151		-	}
152		-
153	149		func (d Daemon) refreshPickerOpts(ac attachedClient, opts pickerRefreshOptions) {
154	150		sess := ac.currentSession()
155	151		if sess == nil {


@@ -146,10 +146,6 @@ type pickerRefreshOptions struct {
146		nearestRow int	146	nearestRow int
147		}	147	}
148			148
149	-	func (d Daemon) refreshPicker(ac attachedClient) {
150	-	d.refreshPickerOpts(ac, pickerRefreshOptions{nearestRow: -1})
151	-	}
152	-
153		func (d Daemon) refreshPickerOpts(ac attachedClient, opts pickerRefreshOptions) {	149	func (d Daemon) refreshPickerOpts(ac attachedClient, opts pickerRefreshOptions) {
154		sess := ac.currentSession()	150	sess := ac.currentSession()
155		if sess == nil {	151	if sess == nil {

M internal/usecase/daemon/picker.go +17 -1


@@ -32,6 +32,22 @@ func pickerTitle(mode pickerSortMode) string {
32	32		return " Sessions · recent "
33	33		}
34	34
	35	+	// togglePickerSort flips the daemon's picker sort mode between the two known
	36	+	// modes with a CompareAndSwap loop, so a concurrent double-press can't lose
	37	+	// an update the way a bare Load-then-Store XOR can.
	38	+	func (d *Daemon) togglePickerSort() {
	39	+	for {
	40	+	cur := pickerSortMode(d.pickerSort.Load())
	41	+	next := pickerSortRecent
	42	+	if cur == pickerSortRecent {
	43	+	next = pickerSortGrouped
	44	+	}
	45	+	if d.pickerSort.CompareAndSwap(uint32(cur), uint32(next)) {
	46	+	return
	47	+	}
	48	+	}
	49	+	}
	50	+
35	51		// enterPicker preserves the existing navigation entry point. Navigation always
36	52		// publishes its model, including an empty one; only move entry can fail for a
37	53		// missing destination.

@@ -235,7 +251,7 @@ func (d Daemon) handlePickerInput(ac attachedClient, data []byte, effects ...*
235	251		rt.pickerMu.Unlock()
236	252
237	253		if result.action == 's' {
238		-	d.pickerSort.Store(d.pickerSort.Load() ^ 1)
	254	+	d.togglePickerSort()
239	255		d.refreshPickerOpts(ac, pickerRefreshOptions{preserveSelection: true, nearestRow: -1})
240	256		d.invalidateRender(sess, ac, true, "picker.go")
241	257		return


@@ -32,6 +32,22 @@ func pickerTitle(mode pickerSortMode) string {
32		return " Sessions · recent "	32		return " Sessions · recent "
33		}	33		}
34			34
			35	+	// togglePickerSort flips the daemon's picker sort mode between the two known
			36	+	// modes with a CompareAndSwap loop, so a concurrent double-press can't lose
			37	+	// an update the way a bare Load-then-Store XOR can.
			38	+	func (d *Daemon) togglePickerSort() {
			39	+	for {
			40	+	cur := pickerSortMode(d.pickerSort.Load())
			41	+	next := pickerSortRecent
			42	+	if cur == pickerSortRecent {
			43	+	next = pickerSortGrouped
			44	+	}
			45	+	if d.pickerSort.CompareAndSwap(uint32(cur), uint32(next)) {
			46	+	return
			47	+	}
			48	+	}
			49	+	}
			50	+
35		// enterPicker preserves the existing navigation entry point. Navigation always	51		// enterPicker preserves the existing navigation entry point. Navigation always
36		// publishes its model, including an empty one; only move entry can fail for a	52		// publishes its model, including an empty one; only move entry can fail for a
37		// missing destination.	53		// missing destination.

@@ -235,7 +251,7 @@ func (d Daemon) handlePickerInput(ac attachedClient, data []byte, effects ...*
235		rt.pickerMu.Unlock()	251		rt.pickerMu.Unlock()
236			252
237		if result.action == 's' {	253		if result.action == 's' {
238	-	d.pickerSort.Store(d.pickerSort.Load() ^ 1)	254	+	d.togglePickerSort()
239		d.refreshPickerOpts(ac, pickerRefreshOptions{preserveSelection: true, nearestRow: -1})	255		d.refreshPickerOpts(ac, pickerRefreshOptions{preserveSelection: true, nearestRow: -1})
240		d.invalidateRender(sess, ac, true, "picker.go")	256		d.invalidateRender(sess, ac, true, "picker.go")
241		return	257		return

M internal/usecase/daemon/picker_test.go +2 -2


@@ -520,7 +520,7 @@ func TestPickerMoveRefreshPreservesSelectedDestination(t *testing.T) {
520	520		destination.mu.Lock()
521	521		destination.tabs[0], destination.tabs[1] = destination.tabs[1], destination.tabs[0]
522	522		destination.mu.Unlock()
523		-	d.refreshPicker(ac)
	523	+	d.refreshPickerOpts(ac, pickerRefreshOptions{nearestRow: -1})
524	524
525	525		ac.overlays.pickerMu.Lock()
526	526		after, ok := ac.overlays.picker.Selected()

@@ -569,7 +569,7 @@ func TestPickerMoveRefreshPreservesSelectedDestination(t *testing.T) {
569	569		require.True(t, ok)
570	570		require.Equal(t, domain.SessionID("selected"), before.Session)
571	571
572		-	d.refreshPicker(ac)
	572	+	d.refreshPickerOpts(ac, pickerRefreshOptions{nearestRow: -1})
573	573
574	574		ac.overlays.pickerMu.Lock()
575	575		after, ok := ac.overlays.picker.Selected()


@@ -520,7 +520,7 @@ func TestPickerMoveRefreshPreservesSelectedDestination(t *testing.T) {
520		destination.mu.Lock()	520		destination.mu.Lock()
521		destination.tabs[0], destination.tabs[1] = destination.tabs[1], destination.tabs[0]	521		destination.tabs[0], destination.tabs[1] = destination.tabs[1], destination.tabs[0]
522		destination.mu.Unlock()	522		destination.mu.Unlock()
523	-	d.refreshPicker(ac)	523	+	d.refreshPickerOpts(ac, pickerRefreshOptions{nearestRow: -1})
524			524
525		ac.overlays.pickerMu.Lock()	525		ac.overlays.pickerMu.Lock()
526		after, ok := ac.overlays.picker.Selected()	526		after, ok := ac.overlays.picker.Selected()

@@ -569,7 +569,7 @@ func TestPickerMoveRefreshPreservesSelectedDestination(t *testing.T) {
569		require.True(t, ok)	569		require.True(t, ok)
570		require.Equal(t, domain.SessionID("selected"), before.Session)	570		require.Equal(t, domain.SessionID("selected"), before.Session)
571			571
572	-	d.refreshPicker(ac)	572	+	d.refreshPickerOpts(ac, pickerRefreshOptions{nearestRow: -1})
573			573
574		ac.overlays.pickerMu.Lock()	574		ac.overlays.pickerMu.Lock()
575		after, ok := ac.overlays.picker.Selected()	575		after, ok := ac.overlays.picker.Selected()

M internal/usecase/picker/model.go +4 -2


@@ -327,8 +327,10 @@ func (m *Model) Selected() (Target, bool) {
327	327		}, true
328	328		}
329	329
330		-	// SelectedIndex reports the raw selected row index, -1 when nothing is
331		-	// selected. Row indices are only meaningful against this exact model.
	330	+	// SelectedIndex reports the raw selected row index. It is -1 only when the
	331	+	// model is nil or has no rows at all; otherwise it is a real row index even
	332	+	// when that row is not selectable (see Selected). Row indices are only
	333	+	// meaningful against this exact model.
332	334		func (m *Model) SelectedIndex() int {
333	335		if m == nil {
334	336		return -1

 	}, true
 }
-// SelectedIndex reports the raw selected row index, -1 when nothing is
+// SelectedIndex reports the raw selected row index. It is -1 only when the
-// selected. Row indices are only meaningful against this exact model.
+// model is nil or has no rows at all; otherwise it is a real row index even
+// when that row is not selectable (see Selected). Row indices are only
+// meaningful against this exact model.
 func (m *Model) SelectedIndex() int {
 	if m == nil {
 		return -1

M internal/usecase/picker/model_test.go +21 -6


@@ -416,17 +416,32 @@ func TestSelectNearestRow(t *testing.T) {
416	416		tests := []struct {
417	417		name string
418	418		idx int
	419	+	cfg SelectionConfig
419	420		want domain.SessionID
420	421		}{
421		-	{name: "exact selectable row", idx: 3, want: "b"},
422		-	{name: "header row snaps to its tab", idx: 2, want: "b"},
423		-	{name: "past end clamps to last selectable", idx: 40, want: "c"},
424		-	{name: "negative clamps to first selectable", idx: -2, want: "a"},
425		-	{name: "last row is a tab and stays put", idx: 5, want: "c"},
	422	+	{name: "exact selectable row", idx: 3, cfg: SelectionConfig{Mode: SelectNavigationTab}, want: "b"},
	423	+	{name: "header row snaps to its tab", idx: 2, cfg: SelectionConfig{Mode: SelectNavigationTab}, want: "b"},
	424	+	{name: "past end clamps to last selectable", idx: 40, cfg: SelectionConfig{Mode: SelectNavigationTab}, want: "c"},
	425	+	{
	426	+	// Starts selection on "c" (via Current) so a no-op SelectNearestRow
	427	+	// would leave the selection on "c" and this case would fail; the
	428	+	// plain SelectNavigationTab config used above starts on "a" already,
	429	+	// which can't tell a real clamp-to-first from a no-op.
	430	+	name: "negative clamps to first selectable",
	431	+	idx: -2,
	432	+	cfg: SelectionConfig{Mode: SelectNavigationTab, Current: SourceFilter{Session: "c", TabID: "tc"}},
	433	+	want: "a",
	434	+	},
	435	+	{name: "last row is a tab and stays put", idx: 5, cfg: SelectionConfig{Mode: SelectNavigationTab}, want: "c"},
426	436		}
427	437		for _, tc := range tests {
428	438		t.Run(tc.name, func(t *testing.T) {
429		-	m := New(sessions, SelectionConfig{Mode: SelectNavigationTab})
	439	+	m := New(sessions, tc.cfg)
	440	+	if tc.name == "negative clamps to first selectable" {
	441	+	pre, ok := m.Selected()
	442	+	require.True(t, ok)
	443	+	require.Equal(t, domain.SessionID("c"), pre.Session, "guard: starting selection must not already be the expected result")
	444	+	}
430	445		m.SelectNearestRow(tc.idx)
431	446		target, ok := m.Selected()
432	447		require.True(t, ok)

 	tests := []struct {
 		name string
 		idx  int
+		cfg  SelectionConfig
 		want domain.SessionID
 	}{
-		{name: "exact selectable row", idx: 3, want: "b"},
+		{name: "exact selectable row", idx: 3, cfg: SelectionConfig{Mode: SelectNavigationTab}, want: "b"},
-		{name: "header row snaps to its tab", idx: 2, want: "b"},
+		{name: "header row snaps to its tab", idx: 2, cfg: SelectionConfig{Mode: SelectNavigationTab}, want: "b"},
-		{name: "past end clamps to last selectable", idx: 40, want: "c"},
+		{name: "past end clamps to last selectable", idx: 40, cfg: SelectionConfig{Mode: SelectNavigationTab}, want: "c"},
-		{name: "negative clamps to first selectable", idx: -2, want: "a"},
+		{
-		{name: "last row is a tab and stays put", idx: 5, want: "c"},
+			// Starts selection on "c" (via Current) so a no-op SelectNearestRow
+			// would leave the selection on "c" and this case would fail; the
+			// plain SelectNavigationTab config used above starts on "a" already,
+			// which can't tell a real clamp-to-first from a no-op.
+			name: "negative clamps to first selectable",
+			idx:  -2,
+			cfg:  SelectionConfig{Mode: SelectNavigationTab, Current: SourceFilter{Session: "c", TabID: "tc"}},
+			want: "a",
+		},
+		{name: "last row is a tab and stays put", idx: 5, cfg: SelectionConfig{Mode: SelectNavigationTab}, want: "c"},
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			m := New(sessions, SelectionConfig{Mode: SelectNavigationTab})
+			m := New(sessions, tc.cfg)
+			if tc.name == "negative clamps to first selectable" {
+				pre, ok := m.Selected()
+				require.True(t, ok)
+				require.Equal(t, domain.SessionID("c"), pre.Session, "guard: starting selection must not already be the expected result")
+			}
 			m.SelectNearestRow(tc.idx)
 			target, ok := m.Selected()
 			require.True(t, ok)