From 878e169985c7066afca8c2cb6a1eb5c9c89d16ed Mon Sep 17 00:00:00 2001 From: Eugene Date: Sat, 21 Mar 2026 14:16:57 +0200 Subject: [PATCH] fix: eliminate data races and reduce lock scope --- app/state.go | 29 +++++++++++++++---- main.go | 30 +++++++++++++++++-- ui/scroll_selection.go | 28 +++++------------- ui/tab_commands.go | 28 +++++++++--------- ui/tab_settings.go | 65 ++++++++++++++++++++++++++++-------------- ui/tab_statistics.go | 9 +----- ui/tab_treeview.go | 6 ++-- ui/workers.go | 2 +- 8 files changed, 118 insertions(+), 79 deletions(-) diff --git a/app/state.go b/app/state.go index 7678b21..c194d7a 100644 --- a/app/state.go +++ b/app/state.go @@ -120,6 +120,21 @@ type TabState struct { SettingsTab widget.Clickable } +// HotkeyResult is sent by the ProcessHotkeyUpdate goroutine back to the UI thread. +type HotkeyResult struct { + Error string // Empty on success + Success bool + PresetID int // Config.HotkeyPreset value to apply +} + +// AutostartResult is sent by the ProcessAutostartUpdate goroutine back to the UI thread. +type AutostartResult struct { + Error string // Empty on success + Success bool + NewEnabled bool // New autostart enabled state + AutoStart bool // Config.AutoStart value to apply +} + // HotkeyState holds hotkey configuration state (UI-THREAD-ONLY). type HotkeyState struct { // NeedsUpdate is set during frame processing and consumed after ev.Frame. @@ -133,6 +148,7 @@ type HotkeyState struct { Presets []hotkey.Preset // Preset definitions SelectedPresetID int // Currently selected preset (0, 1, or 2) PresetClickables []widget.Clickable // Clickables for preset buttons + ResultChan chan HotkeyResult // Buffered(1): goroutine sends result, UI thread drains } type State struct { @@ -181,12 +197,13 @@ type State struct { // Settings view state (UI-THREAD-ONLY) SettingsList widget.List - AutoStartEnabled bool // Whether autostart is currently on - AutoStartClick widget.Clickable // Toggle button - AutoStartError string // Error message after toggle attempt - AutoStartSuccess bool // Show success message after toggle - AutoStartNeedsUpdate bool // Deferred toggle flag (like Hotkeys.NeedsUpdate) - AutoStartUpdating bool // Guard: set on UI thread before goroutine launch, cleared by goroutine before Invalidate + AutoStartEnabled bool // Whether autostart is currently on + AutoStartClick widget.Clickable // Toggle button + AutoStartError string // Error message after toggle attempt + AutoStartSuccess bool // Show success message after toggle + AutoStartNeedsUpdate bool // Deferred toggle flag (like Hotkeys.NeedsUpdate) + AutoStartUpdating bool // Guard: set on UI thread before goroutine launch, cleared on result drain + AutoStartResultChan chan AutostartResult // Buffered(1): goroutine sends result, UI thread drains // Background-to-UI invalidation flag. // Background goroutines set this (via MarkDirty) alongside Window.Invalidate(). diff --git a/main.go b/main.go index acff7e3..861fde6 100644 --- a/main.go +++ b/main.go @@ -605,13 +605,15 @@ func run(p *runParams) *savedUIState { Presets: p.hotkeyPresets, PresetClickables: make([]widget.Clickable, hotkey.PresetCount), SelectedPresetID: cfg.HotkeyPreset, + ResultChan: make(chan appstate.HotkeyResult, 1), }, ShellFilter: nil, // nil = show all shells ShellBadges: make(map[string]*widget.Clickable), // Initialize badge widgets map - SettingsList: widget.List{List: layout.List{Axis: layout.Vertical}}, - AutoStartEnabled: syncAutoStartState(cfg), + SettingsList: widget.List{List: layout.List{Axis: layout.Vertical}}, + AutoStartEnabled: syncAutoStartState(cfg), + AutoStartResultChan: make(chan appstate.AutostartResult, 1), StoreShutdown: make(chan struct{}), // Unbuffered: signal store/polling goroutine to stop } @@ -816,6 +818,30 @@ func run(p *runParams) *savedUIState { lastFrameMetric = ev.Metric + // Drain settings result channels before rendering so results + // are visible in the current frame. Both channels are buffered(1); + // goroutines send exactly one result per invocation. + select { + case r := <-appState.Hotkeys.ResultChan: + appState.Hotkeys.Error = r.Error + + appState.Hotkeys.Success = r.Success + if r.Success { + appState.Config.HotkeyPreset = r.PresetID + } + default: + } + + select { + case r := <-appState.AutoStartResultChan: + appState.AutoStartError = r.Error + appState.AutoStartSuccess = r.Success + appState.AutoStartEnabled = r.NewEnabled + appState.Config.AutoStart = r.AutoStart + appState.AutoStartUpdating = false + default: + } + gtx := app.NewContext(&ops, ev) ui.RenderFrame(gtx, appState, theme) ev.Frame(gtx.Ops) diff --git a/ui/scroll_selection.go b/ui/scroll_selection.go index 5b0261b..8d03dbf 100644 --- a/ui/scroll_selection.go +++ b/ui/scroll_selection.go @@ -187,12 +187,9 @@ func findCommandMatch(searchList []*model.CommandEntry, nodePath string, mt matc bestMatchLen := 0 for i, cmd := range searchList { - var matched bool - if mt == matchFuzzy { // Fuzzy match: command contains node path - matched = strings.Contains(cmd.Command, nodePath) - if matched { + if strings.Contains(cmd.Command, nodePath) { foundIndex = i break // Take first fuzzy match } @@ -256,21 +253,15 @@ func syncTreeToCommandSelection(app *appstate.State) { return } - app.StoreMu.RUnlock() - - app.StoreMu.RLock() - - // Always search in DisplayCommands so the found index is valid for the - // currently displayed list. This avoids index mismatches when a source - // filter or search query has reduced DisplayCommands relative to - // LoadedCommands, and prevents overwriting the user's active filter. + // Snapshot DisplayCommands under lock, then release before linear scan. + // Copy-on-write discipline ensures the snapshot remains valid. + // Using one snapshot for both exact and fuzzy scans guarantees consistent indices. searchList := app.Commands.DisplayCommands + app.StoreMu.RUnlock() // Find the best matching command (prefer exact match or longest prefix) foundIndex := findCommandMatch(searchList, nodePath, matchExact) - app.StoreMu.RUnlock() - if foundIndex >= 0 { app.Commands.SelectedIndex = foundIndex // Break out of ScrollToEnd pinning and place target at top of viewport @@ -283,13 +274,8 @@ func syncTreeToCommandSelection(app *appstate.State) { return } - // No exact/prefix match - try fuzzy matching - - app.StoreMu.RLock() - - fuzzyFoundIndex := findCommandMatch(app.Commands.DisplayCommands, nodePath, matchFuzzy) - - app.StoreMu.RUnlock() + // No exact/prefix match - try fuzzy matching (same snapshot for consistency) + fuzzyFoundIndex := findCommandMatch(searchList, nodePath, matchFuzzy) if fuzzyFoundIndex >= 0 { app.Commands.SelectedIndex = fuzzyFoundIndex diff --git a/ui/tab_commands.go b/ui/tab_commands.go index 4f3d919..16b532d 100644 --- a/ui/tab_commands.go +++ b/ui/tab_commands.go @@ -39,7 +39,7 @@ func renderCommandsTab(gtx C, app *appstate.State, theme *material.Theme) D { } // renderCommandItem renders a single command entry with source indicator -func renderCommandItem(gtx C, app *appstate.State, theme *material.Theme, cmd *model.CommandEntry, index int, isSelected bool) D { +func renderCommandItem(gtx C, app *appstate.State, theme *material.Theme, cmd *model.CommandEntry, index int, isSelected bool, metadataCache map[*model.CommandEntry]string) D { // Outer spacing between rows return layout.Inset{ Top: unit.Dp(1), @@ -82,11 +82,11 @@ func renderCommandItem(gtx C, app *appstate.State, theme *material.Theme, cmd *m // Metadata (frequency and shell name) layout.Rigid(func(gtx C) D { // Check cache first for zero allocations - metadata, exists := app.Commands.MetadataCache[cmd] + metadata, exists := metadataCache[cmd] if !exists { - // Build and cache on first render + // Build and cache on first render (UI-thread-only writes to snapshot) metadata = tree.BuildCommandMetadata(cmd.Frequency, cmd.Shell.String()) - app.Commands.MetadataCache[cmd] = metadata + metadataCache[cmd] = metadata } label := material.Caption(theme, metadata) @@ -140,6 +140,8 @@ func renderCommandList(gtx C, app *appstate.State, theme *material.Theme) D { app.StoreMu.RLock() commands := app.Commands.DisplayCommands loadError := app.LoadError + metadataCache := app.Commands.MetadataCache + currentQuery := app.CurrentQuery app.StoreMu.RUnlock() // Initialize or resize height cache if needed @@ -212,8 +214,8 @@ func renderCommandList(gtx C, app *appstate.State, theme *material.Theme) D { // Handle empty state if len(commands) == 0 { message := "No commands found" - if app.CurrentQuery != "" { - message = fmt.Sprintf("No matches for '%s'", app.CurrentQuery) + if currentQuery != "" { + message = fmt.Sprintf("No matches for '%s'", currentQuery) } return layout.Center.Layout(gtx, func(gtx C) D { @@ -247,17 +249,13 @@ func renderCommandList(gtx C, app *appstate.State, theme *material.Theme) D { app.Window.Invalidate() } case pointer.Press: - app.StoreMu.RLock() - var cmdToCopy string - if index >= 0 && index < len(app.Commands.DisplayCommands) { - cmdToCopy = app.Commands.DisplayCommands[index].Command - } else if len(app.Commands.DisplayCommands) == 1 { - cmdToCopy = app.Commands.DisplayCommands[0].Command + if index >= 0 && index < len(commands) { + cmdToCopy = commands[index].Command + } else if len(commands) == 1 { + cmdToCopy = commands[0].Command } - app.StoreMu.RUnlock() - if cmdToCopy != "" { copyTextAndMinimize(gtx, app, cmdToCopy) } @@ -267,7 +265,7 @@ func renderCommandList(gtx C, app *appstate.State, theme *material.Theme) D { isSelected := app.Commands.SelectedIndex == index - dims := renderCommandItem(gtx, app, theme, cmd, index, isSelected) + dims := renderCommandItem(gtx, app, theme, cmd, index, isSelected, metadataCache) // Cache the rendered height for smart scrolling if index < len(app.Commands.ItemHeights) { diff --git a/ui/tab_settings.go b/ui/tab_settings.go index ea93e70..5c680a3 100644 --- a/ui/tab_settings.go +++ b/ui/tab_settings.go @@ -345,7 +345,8 @@ func renderAutostartMessages(gtx C, app *appstate.State, theme *material.Theme) } // ProcessAutostartUpdate performs the actual autostart toggle and config save. -// Must be called after ev.Frame in a goroutine to avoid blocking the UI thread. +// Runs as a goroutine after ev.Frame to avoid blocking the UI thread. +// Sends result on app.AutoStartResultChan; the event loop applies it on the UI thread. func ProcessAutostartUpdate(app *appstate.State) { newState := !app.AutoStartEnabled @@ -357,27 +358,39 @@ func ProcessAutostartUpdate(app *appstate.State) { } if err != nil { - app.AutoStartError = fmt.Sprintf(AutoStartFailure, err) log.Printf("Autostart toggle failed: %v", err) - app.AutoStartUpdating = false - app.Window.Invalidate() + app.AutoStartResultChan <- appstate.AutostartResult{ + Error: fmt.Sprintf(AutoStartFailure, err), + NewEnabled: app.AutoStartEnabled, // unchanged + AutoStart: app.Config.AutoStart, // unchanged + } + + app.MarkDirty() return } - app.AutoStartEnabled = newState + // Save config to disk (file I/O is fine from goroutine) app.Config.AutoStart = newState if err := app.Config.SaveConfig(app.ConfigPath); err != nil { - app.AutoStartError = fmt.Sprintf(AutoStartFailureCfg, err) log.Printf("Config save failed after autostart toggle: %v", err) + + app.AutoStartResultChan <- appstate.AutostartResult{ + Error: fmt.Sprintf(AutoStartFailureCfg, err), + NewEnabled: newState, + AutoStart: newState, + } } else { - app.AutoStartSuccess = true + app.AutoStartResultChan <- appstate.AutostartResult{ + Success: true, + NewEnabled: newState, + AutoStart: newState, + } } - app.AutoStartUpdating = false - app.Window.Invalidate() + app.MarkDirty() } // saveHotkeyPreset schedules a hotkey update for after frame submission. @@ -392,43 +405,51 @@ func saveHotkeyPreset(app *appstate.State) { } // ProcessHotkeyUpdate performs the actual hotkey registration and config save. -// Must be called after ev.Frame to avoid a dispatch_sync deadlock on macOS. +// Runs as a goroutine after ev.Frame to avoid a dispatch_sync deadlock on macOS. +// Sends result on app.Hotkeys.ResultChan; the event loop applies it on the UI thread. func ProcessHotkeyUpdate(app *appstate.State) { - if app.Hotkeys.SelectedPresetID < 0 || app.Hotkeys.SelectedPresetID >= len(app.Hotkeys.Presets) { - app.Hotkeys.Error = "Invalid preset selection" - app.Window.Invalidate() + presetID := app.Hotkeys.SelectedPresetID + + if presetID < 0 || presetID >= len(app.Hotkeys.Presets) { + app.Hotkeys.ResultChan <- appstate.HotkeyResult{Error: "Invalid preset selection"} + + app.MarkDirty() return } - preset := app.Hotkeys.Presets[app.Hotkeys.SelectedPresetID] + preset := app.Hotkeys.Presets[presetID] mods, key, err := hotkey.ConvertStrings(preset.Modifiers, preset.Key) if err != nil { - app.Hotkeys.Error = err.Error() - app.Window.Invalidate() + app.Hotkeys.ResultChan <- appstate.HotkeyResult{Error: err.Error()} + + app.MarkDirty() return } if err := app.Hotkeys.Manager.UpdateHotkey(mods, key, preset.Modifiers, preset.Key); err != nil { - app.Hotkeys.Error = fmt.Sprintf(HotKeyCardFailure, err) + app.Hotkeys.ResultChan <- appstate.HotkeyResult{Error: fmt.Sprintf(HotKeyCardFailure, err)} + log.Printf("Hotkey registration failed: %v", err) - app.Window.Invalidate() + app.MarkDirty() return } - app.Config.HotkeyPreset = app.Hotkeys.SelectedPresetID + // Save config to disk (file I/O is fine from goroutine) + app.Config.HotkeyPreset = presetID if err := app.Config.SaveConfig(app.ConfigPath); err != nil { - app.Hotkeys.Error = fmt.Sprintf(HotKeyCardFailureDueToCfg, err) + app.Hotkeys.ResultChan <- appstate.HotkeyResult{Error: fmt.Sprintf(HotKeyCardFailureDueToCfg, err)} + log.Printf("Config save failed: %v", err) } else { - app.Hotkeys.Success = true + app.Hotkeys.ResultChan <- appstate.HotkeyResult{Success: true, PresetID: presetID} log.Printf("Hotkey updated: %v + %v", mods, key) } - app.Window.Invalidate() + app.MarkDirty() } diff --git a/ui/tab_statistics.go b/ui/tab_statistics.go index d7ccb43..d73b7fb 100644 --- a/ui/tab_statistics.go +++ b/ui/tab_statistics.go @@ -252,14 +252,7 @@ func handleStatisticsScrolling(gtx C, app *appstate.State) { } // Initialize or resize height cache if needed - if len(app.Stats.ItemHeights) != totalItems { - if cap(app.Stats.ItemHeights) >= totalItems { - app.Stats.ItemHeights = app.Stats.ItemHeights[:totalItems] - clear(app.Stats.ItemHeights) - } else { - app.Stats.ItemHeights = make([]int, totalItems) - } - } + resizeHeightCache(&app.Stats.ItemHeights, totalItems) // Sum cached heights of items before the selected one to compute the pixel offset. offset := 0 diff --git a/ui/tab_treeview.go b/ui/tab_treeview.go index b2ab335..c9905d8 100644 --- a/ui/tab_treeview.go +++ b/ui/tab_treeview.go @@ -484,21 +484,19 @@ func handleTreeNodeClick(gtx C, app *appstate.State, node *model.TreeDisplayNode // navigateTreeWithSearch performs tree navigation using a custom search function // searchFn takes (nodes, selectedIndex) and returns search result func navigateTreeWithSearch(app *appstate.State, searchFn func(nodes []*model.TreeDisplayNode, selectedIndex int) treeNavigationResult) { - // Snapshot with generation counter + // Snapshot with generation counter (copy-on-write: nodes slice is immutable after snapshot) app.StoreMu.RLock() selectedTreeNode := app.Tree.SelectedNode nodes := app.Tree.Nodes generation := app.Tree.NodesGeneration + app.StoreMu.RUnlock() if selectedTreeNode < 0 || selectedTreeNode >= len(nodes) { - app.StoreMu.RUnlock() return } result := searchFn(nodes, selectedTreeNode) - app.StoreMu.RUnlock() - if result.foundIndex >= 0 { app.StoreMu.Lock() diff --git a/ui/workers.go b/ui/workers.go index e103856..94871e0 100644 --- a/ui/workers.go +++ b/ui/workers.go @@ -174,7 +174,7 @@ func updateHistory(state *appstate.State) { state.LoadError = nil // Clear metadata cache when store (and its CommandEntry objects) is replaced - clear(state.Commands.MetadataCache) + state.Commands.MetadataCache = make(map[*model.CommandEntry]string) // Initialize/reload commands initializeCommandsLocked(state)