Skip to content

Commit e3b18e5

Browse files
authored
fix: prevent preset index panic, reduce hover invalidations (#13)
1 parent ada9aad commit e3b18e5

7 files changed

Lines changed: 214 additions & 26 deletions

File tree

‎ui/bar_search.go‎

Lines changed: 10 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -84,29 +84,21 @@ func handleSearchInput(app *appstate.State) {
8484
// Only clear selections if the search query actually changed
8585
// This prevents clearing NeedScrollToSel during tab switches when text hasn't changed
8686
if queryChanged {
87-
// Save current selections before clearing them (only if we have a selection)
88-
if app.Commands.SelectedIndex >= 0 {
89-
app.StoreMu.RLock()
87+
// Save current selections before clearing them (single lock for consistent snapshot)
88+
app.StoreMu.RLock()
9089

91-
if app.Commands.SelectedIndex < len(app.Commands.DisplayCommands) {
92-
app.Commands.LastSelectedIndex = app.Commands.SelectedIndex
93-
app.Commands.LastSelectedCmd = app.Commands.DisplayCommands[app.Commands.SelectedIndex].Command
94-
}
95-
96-
app.StoreMu.RUnlock()
90+
if app.Commands.SelectedIndex >= 0 && app.Commands.SelectedIndex < len(app.Commands.DisplayCommands) {
91+
app.Commands.LastSelectedIndex = app.Commands.SelectedIndex
92+
app.Commands.LastSelectedCmd = app.Commands.DisplayCommands[app.Commands.SelectedIndex].Command
9793
}
9894

99-
if app.Tree.SelectedNode >= 0 {
100-
app.StoreMu.RLock()
101-
102-
if app.Tree.SelectedNode < len(app.Tree.Nodes) {
103-
app.Tree.LastSelectedNode = app.Tree.SelectedNode
104-
app.Tree.LastSelectedPath = app.Tree.Nodes[app.Tree.SelectedNode].Path
105-
}
106-
107-
app.StoreMu.RUnlock()
95+
if app.Tree.SelectedNode >= 0 && app.Tree.SelectedNode < len(app.Tree.Nodes) {
96+
app.Tree.LastSelectedNode = app.Tree.SelectedNode
97+
app.Tree.LastSelectedPath = app.Tree.Nodes[app.Tree.SelectedNode].Path
10898
}
10999

100+
app.StoreMu.RUnlock()
101+
110102
// Reset selection when user types (return to search mode)
111103
app.Commands.SelectedIndex = -1 // UI-only state
112104
app.NeedScrollToSel = false

‎ui/keyboard.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -357,7 +357,7 @@ func findNextWordBoundary(runes []rune, pos int) int {
357357
// total is the number of items. pageSize is items to jump.
358358
// up=true moves toward 0; up=false moves toward total-1.
359359
// Returns current unchanged when total == 0.
360-
func pageJump(current, total, pageSize int, up bool) int { //nolint:unparam
360+
func pageJump(current, total, pageSize int, up bool) int {
361361
if total == 0 {
362362
return current
363363
}

‎ui/keyboard_test.go‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
package ui
2+
3+
import "testing"
4+
5+
func TestPageJump(t *testing.T) {
6+
tests := []struct {
7+
name string
8+
current, total, pageSize int
9+
up bool
10+
want int
11+
}{
12+
// No selection
13+
{"no selection page up", -1, 20, 10, true, 19},
14+
{"no selection page down", -1, 20, 10, false, 0},
15+
// Normal movement
16+
{"page down from middle", 5, 20, 10, false, 15},
17+
{"page up from middle", 15, 20, 10, true, 5},
18+
// Clamp at boundaries
19+
{"page up clamps at 0", 3, 20, 10, true, 0},
20+
{"page down clamps at last", 18, 20, 10, false, 19},
21+
// Empty list
22+
{"empty list page up", -1, 0, 10, true, -1},
23+
{"empty list page down", 0, 0, 10, false, 0},
24+
// Single item
25+
{"single item page up", -1, 1, 10, true, 0},
26+
{"single item page down", 0, 1, 10, false, 0},
27+
// Already at boundary
28+
{"at first page up", 0, 10, 5, true, 0},
29+
{"at last page down", 9, 10, 5, false, 9},
30+
}
31+
32+
for _, tt := range tests {
33+
t.Run(tt.name, func(t *testing.T) {
34+
got := pageJump(tt.current, tt.total, tt.pageSize, tt.up)
35+
if got != tt.want {
36+
t.Errorf("pageJump(%d, %d, %d, %v) = %d, want %d",
37+
tt.current, tt.total, tt.pageSize, tt.up, got, tt.want)
38+
}
39+
})
40+
}
41+
}

‎ui/scroll_selection.go‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -172,17 +172,24 @@ func syncCommandToTreeSelection(app *appstate.State) {
172172

173173
// findCommandMatch searches for a command that matches the given node path
174174
// Returns the index of the best match, or -1 if not found
175-
// matchType: "exact" for exact/prefix matching, "fuzzy" for contains matching
175+
// matchType: matchExact for exact/prefix matching, matchFuzzy for contains matching
176176
//
177177
// NOTE: Linear scan is acceptable for current data sizes
178-
func findCommandMatch(searchList []*model.CommandEntry, nodePath, matchType string) int {
178+
type matchType int
179+
180+
const (
181+
matchExact matchType = iota
182+
matchFuzzy
183+
)
184+
185+
func findCommandMatch(searchList []*model.CommandEntry, nodePath string, mt matchType) int {
179186
foundIndex := -1
180187
bestMatchLen := 0
181188

182189
for i, cmd := range searchList {
183190
var matched bool
184191

185-
if matchType == "fuzzy" {
192+
if mt == matchFuzzy {
186193
// Fuzzy match: command contains node path
187194
matched = strings.Contains(cmd.Command, nodePath)
188195
if matched {
@@ -260,7 +267,7 @@ func syncTreeToCommandSelection(app *appstate.State) {
260267
searchList := app.Commands.DisplayCommands
261268

262269
// Find the best matching command (prefer exact match or longest prefix)
263-
foundIndex := findCommandMatch(searchList, nodePath, "exact")
270+
foundIndex := findCommandMatch(searchList, nodePath, matchExact)
264271

265272
app.StoreMu.RUnlock()
266273

@@ -280,7 +287,7 @@ func syncTreeToCommandSelection(app *appstate.State) {
280287

281288
app.StoreMu.RLock()
282289

283-
fuzzyFoundIndex := findCommandMatch(app.Commands.DisplayCommands, nodePath, "fuzzy")
290+
fuzzyFoundIndex := findCommandMatch(app.Commands.DisplayCommands, nodePath, matchFuzzy)
284291

285292
app.StoreMu.RUnlock()
286293

‎ui/scroll_test.go‎

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
package ui
2+
3+
import (
4+
"image"
5+
"testing"
6+
7+
"gioui.org/layout"
8+
"gioui.org/op"
9+
"gioui.org/unit"
10+
)
11+
12+
// makeGtx creates a minimal layout.Context with the given viewport height.
13+
// Uses 1:1 dp-to-px ratio for test simplicity.
14+
func makeGtx(viewportHeight int) C {
15+
var ops op.Ops
16+
17+
return layout.Context{
18+
Ops: &ops,
19+
Constraints: layout.Constraints{
20+
Max: image.Pt(800, viewportHeight),
21+
},
22+
Metric: unit.Metric{PxPerDp: 1, PxPerSp: 1},
23+
}
24+
}
25+
26+
func TestCalculateSmartScrollPositionVariable_AlreadyVisible(t *testing.T) {
27+
gtx := makeGtx(300)
28+
heights := []int{50, 50, 50, 50, 50, 50}
29+
30+
// Item 2 is visible when first=0 (items 0-4 fit in 300px with 20px margin)
31+
newFirst, shouldScroll := calculateSmartScrollPositionVariable(gtx, 0, 2, heights)
32+
if shouldScroll {
33+
t.Errorf("Expected no scroll needed, got newFirst=%d", newFirst)
34+
}
35+
}
36+
37+
func TestCalculateSmartScrollPositionVariable_AboveViewport(t *testing.T) {
38+
gtx := makeGtx(300)
39+
heights := []int{50, 50, 50, 50, 50, 50, 50, 50, 50, 50}
40+
41+
// currentFirst=5, selected=2 (above viewport)
42+
newFirst, shouldScroll := calculateSmartScrollPositionVariable(gtx, 5, 2, heights)
43+
if !shouldScroll {
44+
t.Fatal("Expected scroll needed")
45+
}
46+
47+
if newFirst != 2 {
48+
t.Errorf("Expected newFirst=2, got %d", newFirst)
49+
}
50+
}
51+
52+
func TestCalculateSmartScrollPositionVariable_BelowViewport(t *testing.T) {
53+
gtx := makeGtx(200)
54+
heights := []int{50, 50, 50, 50, 50, 50, 50, 50, 50, 50}
55+
56+
// currentFirst=0, selected=8 (below viewport - only ~3 items visible in 200-20=180px)
57+
newFirst, shouldScroll := calculateSmartScrollPositionVariable(gtx, 0, 8, heights)
58+
if !shouldScroll {
59+
t.Fatal("Expected scroll needed")
60+
}
61+
// Should scroll so item 8 is near the bottom
62+
if newFirst > 8 || newFirst < 5 {
63+
t.Errorf("Expected newFirst between 5 and 8, got %d", newFirst)
64+
}
65+
}
66+
67+
func TestCalculateSmartScrollPositionVariable_EmptyHeights(t *testing.T) {
68+
gtx := makeGtx(300)
69+
70+
newFirst, shouldScroll := calculateSmartScrollPositionVariable(gtx, 0, 0, []int{})
71+
if shouldScroll {
72+
t.Errorf("Expected no scroll for empty heights, got newFirst=%d", newFirst)
73+
}
74+
}
75+
76+
func TestCalculateSmartScrollPositionVariable_NegativeSelected(t *testing.T) {
77+
gtx := makeGtx(300)
78+
heights := []int{50, 50, 50}
79+
80+
newFirst, shouldScroll := calculateSmartScrollPositionVariable(gtx, 0, -1, heights)
81+
if shouldScroll {
82+
t.Errorf("Expected no scroll for negative index, got newFirst=%d", newFirst)
83+
}
84+
}
85+
86+
func TestCalculateSmartScrollPositionVariable_LargeItemExceedsViewport(t *testing.T) {
87+
gtx := makeGtx(100)
88+
heights := []int{50, 50, 200, 50} // item 2 is larger than viewport
89+
90+
newFirst, shouldScroll := calculateSmartScrollPositionVariable(gtx, 0, 2, heights)
91+
if !shouldScroll {
92+
t.Fatal("Expected scroll needed for large item")
93+
}
94+
// When item exceeds viewport, it should be placed at the top
95+
if newFirst != 2 {
96+
t.Errorf("Expected newFirst=2 for large item, got %d", newFirst)
97+
}
98+
}
99+
100+
func TestCalculateSmartScrollPositionVariable_ZeroViewport(t *testing.T) {
101+
gtx := makeGtx(0)
102+
heights := []int{50, 50, 50}
103+
104+
_, shouldScroll := calculateSmartScrollPositionVariable(gtx, 0, 1, heights)
105+
if shouldScroll {
106+
t.Error("Expected no scroll for zero viewport")
107+
}
108+
}
109+
110+
func TestCalculateSmartScrollPositionVariable_UnmeasuredItems(t *testing.T) {
111+
gtx := makeGtx(300)
112+
// Some items unmeasured (0 height) — should use fallback
113+
heights := []int{50, 0, 0, 50, 0, 50, 50, 50, 50, 50}
114+
115+
// Should not panic and should produce valid result
116+
newFirst, _ := calculateSmartScrollPositionVariable(gtx, 0, 8, heights)
117+
if newFirst < 0 || newFirst > 8 {
118+
t.Errorf("Expected valid newFirst, got %d", newFirst)
119+
}
120+
}
121+
122+
func TestEstimateFallbackHeight_AllMeasured(t *testing.T) {
123+
gtx := makeGtx(300)
124+
heights := []int{40, 60, 50}
125+
got := estimateFallbackHeight(gtx, heights)
126+
127+
want := 50 // (40+60+50)/3
128+
if got != want {
129+
t.Errorf("estimateFallbackHeight = %d, want %d", got, want)
130+
}
131+
}
132+
133+
func TestEstimateFallbackHeight_NoneMeasured(t *testing.T) {
134+
gtx := makeGtx(300)
135+
heights := []int{0, 0, 0}
136+
got := estimateFallbackHeight(gtx, heights)
137+
// Should use TreeRowHeight + TreeRowInsetHeight via gtx.Dp
138+
if got <= 0 {
139+
t.Errorf("estimateFallbackHeight = %d, want > 0", got)
140+
}
141+
}

‎ui/tab_settings.go‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ func renderHotkeyMessages(gtx C, app *appstate.State, theme *material.Theme) D {
204204
})
205205
}
206206

207-
if app.Hotkeys.Success {
207+
if app.Hotkeys.Success && app.Hotkeys.SelectedPresetID >= 0 && app.Hotkeys.SelectedPresetID < len(app.Hotkeys.Presets) {
208208
return layout.Inset{Bottom: SpacingMedium}.Layout(gtx, func(gtx C) D {
209209
selectedPreset := app.Hotkeys.Presets[app.Hotkeys.SelectedPresetID]
210210
message := HotKeyCardSuccess + selectedPreset.DisplayName
@@ -394,6 +394,13 @@ func saveHotkeyPreset(app *appstate.State) {
394394
// ProcessHotkeyUpdate performs the actual hotkey registration and config save.
395395
// Must be called after ev.Frame to avoid a dispatch_sync deadlock on macOS.
396396
func ProcessHotkeyUpdate(app *appstate.State) {
397+
if app.Hotkeys.SelectedPresetID < 0 || app.Hotkeys.SelectedPresetID >= len(app.Hotkeys.Presets) {
398+
app.Hotkeys.Error = "Invalid preset selection"
399+
app.Window.Invalidate()
400+
401+
return
402+
}
403+
397404
preset := app.Hotkeys.Presets[app.Hotkeys.SelectedPresetID]
398405

399406
mods, key, err := hotkey.ConvertStrings(preset.Modifiers, preset.Key)

‎ui/tab_treeview.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -434,7 +434,7 @@ func handleTreeNodePointerEvents(gtx C, app *appstate.State, node *model.TreeDis
434434
if app.Tree.SuppressHover {
435435
app.Tree.SuppressHover = false
436436
needsInvalidate = true
437-
} else {
437+
} else if app.Tree.HoveredNode != index {
438438
app.Tree.HoveredNode = index
439439
needsInvalidate = true
440440
}

0 commit comments

Comments
 (0)