From c46b66d72e66a78f7ee8462f72d24f7ddd434114 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Wed, 27 Aug 2025 09:19:05 -0400 Subject: [PATCH 01/13] auto-refresh --- cmd/goose/click_test.go | 98 ++++++++++++++++++++++++++++++++++++ cmd/goose/github.go | 5 ++ cmd/goose/main.go | 60 ++++++++++++++++++++-- cmd/goose/ui.go | 107 +++++++++++++++++++++++++++++++++++++++- 4 files changed, 264 insertions(+), 6 deletions(-) create mode 100644 cmd/goose/click_test.go diff --git a/cmd/goose/click_test.go b/cmd/goose/click_test.go new file mode 100644 index 0000000..bf900a7 --- /dev/null +++ b/cmd/goose/click_test.go @@ -0,0 +1,98 @@ +package main + +import ( + "context" + "sync" + "testing" + "time" +) + +// TestMenuClickRateLimit tests that menu clicks are properly rate limited. +func TestMenuClickRateLimit(t *testing.T) { + ctx := context.Background() + + // Create app with initial state + app := &App{ + mu: sync.RWMutex{}, + stateManager: NewPRStateManager(time.Now().Add(-35 * time.Second)), + hiddenOrgs: make(map[string]bool), + seenOrgs: make(map[string]bool), + lastSearchAttempt: time.Now().Add(-15 * time.Second), // 15 seconds ago + } + + // Simulate the click handler logic (without the actual UI interaction) + testClick := func() (shouldRefresh bool, timeSinceLastSearch time.Duration) { + app.mu.RLock() + timeSince := time.Since(app.lastSearchAttempt) + app.mu.RUnlock() + + if timeSince >= minUpdateInterval { + // Would trigger refresh + app.mu.Lock() + app.lastSearchAttempt = time.Now() + app.mu.Unlock() + return true, timeSince + } + return false, timeSince + } + + // Test 1: First click should allow refresh (last search was 15s ago) + shouldRefresh, timeSince := testClick() + if !shouldRefresh { + t.Errorf("First click should allow refresh, last search was %v ago", timeSince) + } + + // Test 2: Immediate second click should be rate limited + shouldRefresh2, timeSince2 := testClick() + if shouldRefresh2 { + t.Errorf("Second click should be rate limited, last search was %v ago", timeSince2) + } + + // Test 3: After waiting 10+ seconds, should allow refresh again + app.mu.Lock() + app.lastSearchAttempt = time.Now().Add(-11 * time.Second) + app.mu.Unlock() + + shouldRefresh3, timeSince3 := testClick() + if !shouldRefresh3 { + t.Errorf("Click after 11 seconds should allow refresh, last search was %v ago", timeSince3) + } + + _ = ctx // Keep context for potential future use +} + +// TestScheduledUpdateRateLimit tests that scheduled updates respect rate limiting. +func TestScheduledUpdateRateLimit(t *testing.T) { + app := &App{ + mu: sync.RWMutex{}, + stateManager: NewPRStateManager(time.Now().Add(-35 * time.Second)), + hiddenOrgs: make(map[string]bool), + seenOrgs: make(map[string]bool), + lastSearchAttempt: time.Now().Add(-5 * time.Second), // 5 seconds ago + } + + // Simulate the scheduled update logic + testScheduledUpdate := func() (shouldUpdate bool, timeSinceLastSearch time.Duration) { + app.mu.RLock() + timeSince := time.Since(app.lastSearchAttempt) + app.mu.RUnlock() + + return timeSince >= minUpdateInterval, timeSince + } + + // Test 1: Scheduled update should be skipped (last search was only 5s ago) + shouldUpdate, timeSince := testScheduledUpdate() + if shouldUpdate { + t.Errorf("Scheduled update should be skipped, last search was %v ago", timeSince) + } + + // Test 2: After waiting 10+ seconds, scheduled update should proceed + app.mu.Lock() + app.lastSearchAttempt = time.Now().Add(-12 * time.Second) + app.mu.Unlock() + + shouldUpdate2, timeSince2 := testScheduledUpdate() + if !shouldUpdate2 { + t.Errorf("Scheduled update after 12 seconds should proceed, last search was %v ago", timeSince2) + } +} \ No newline at end of file diff --git a/cmd/goose/github.go b/cmd/goose/github.go index d16fb99..f6f1e44 100644 --- a/cmd/goose/github.go +++ b/cmd/goose/github.go @@ -223,6 +223,11 @@ type prResult struct { // fetchPRsInternal fetches PRs and Turn data synchronously for simplicity. func (app *App) fetchPRsInternal(ctx context.Context) (incoming []PR, outgoing []PR, _ error) { + // Update search attempt time for rate limiting + app.mu.Lock() + app.lastSearchAttempt = time.Now() + app.mu.Unlock() + // Check if we have a client if app.client == nil { return nil, nil, fmt.Errorf("no GitHub client available: %s", app.authError) diff --git a/cmd/goose/main.go b/cmd/goose/main.go index 28a64ef..e744f06 100644 --- a/cmd/goose/main.go +++ b/cmd/goose/main.go @@ -11,6 +11,7 @@ import ( "log" "os" "path/filepath" + "slices" "strings" "sync" "time" @@ -64,6 +65,8 @@ type PR struct { // App holds the application state. type App struct { lastSuccessfulFetch time.Time + lastSearchAttempt time.Time // For rate limiting forced refreshes + lastMenuTitles []string // For change detection to prevent unnecessary redraws startTime time.Time client *github.Client turnClient *turn.Client @@ -251,6 +254,22 @@ func (app *App) onReady(ctx context.Context) { // Set up click handlers first (needed for both success and error states) systray.SetOnClick(func(menu systray.IMenu) { log.Println("Icon clicked") + + // Check if we can perform a forced refresh (rate limited to every 10 seconds) + app.mu.RLock() + timeSinceLastSearch := time.Since(app.lastSearchAttempt) + app.mu.RUnlock() + + if timeSinceLastSearch >= minUpdateInterval { + log.Printf("[CLICK] Forcing search refresh (last search %v ago)", timeSinceLastSearch) + go func() { + app.updatePRs(ctx) + }() + } else { + remainingTime := minUpdateInterval - timeSinceLastSearch + log.Printf("[CLICK] Rate limited - search performed %v ago, %v remaining", timeSinceLastSearch, remainingTime) + } + if menu != nil { if err := menu.ShowMenu(); err != nil { log.Printf("Failed to show menu: %v", err) @@ -325,8 +344,19 @@ func (app *App) updateLoop(ctx context.Context) { for { select { case <-ticker.C: - log.Println("Running scheduled PR update") - app.updatePRs(ctx) + // Check if we should skip this scheduled update due to recent forced refresh + app.mu.RLock() + timeSinceLastSearch := time.Since(app.lastSearchAttempt) + app.mu.RUnlock() + + if timeSinceLastSearch >= minUpdateInterval { + log.Println("Running scheduled PR update") + app.updatePRs(ctx) + } else { + remainingTime := minUpdateInterval - timeSinceLastSearch + log.Printf("Skipping scheduled update - recent search %v ago, %v remaining until next allowed", + timeSinceLastSearch, remainingTime) + } case <-ctx.Done(): log.Println("Update loop stopping due to context cancellation") return @@ -444,13 +474,33 @@ func (app *App) updatePRs(ctx context.Context) { log.Print("[DEBUG] Completed PR state updates and notifications") } -// updateMenu rebuilds the menu every time - simple and reliable. +// updateMenu rebuilds the menu only if there are changes to improve UX. func (app *App) updateMenu(ctx context.Context) { - // Always rebuild - it's just a small menu, performance is not an issue - log.Println("[MENU] Rebuilding menu") + // Generate current menu titles + currentTitles := app.generateMenuTitles() + + // Compare with last titles to see if rebuild is needed + app.mu.RLock() + lastTitles := app.lastMenuTitles + app.mu.RUnlock() + + // Check if titles have changed + if slices.Equal(currentTitles, lastTitles) { + log.Printf("[MENU] No changes detected, skipping rebuild (%d items unchanged)", len(currentTitles)) + return + } + + // Titles have changed, rebuild menu + log.Printf("[MENU] Changes detected, rebuilding menu (%dā†’%d items)", len(lastTitles), len(currentTitles)) app.rebuildMenu(ctx) + + // Store new titles + app.mu.Lock() + app.lastMenuTitles = currentTitles + app.mu.Unlock() } + // updatePRsWithWait fetches PRs and waits for Turn data before building initial menu. func (app *App) updatePRsWithWait(ctx context.Context) { incoming, outgoing, err := app.fetchPRsInternal(ctx) diff --git a/cmd/goose/ui.go b/cmd/goose/ui.go index 9bcbd56..9e87be6 100644 --- a/cmd/goose/ui.go +++ b/cmd/goose/ui.go @@ -278,6 +278,111 @@ func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string, } } +// generateMenuTitles generates the list of menu item titles that would be shown +// without actually building the UI. Used for change detection. +func (app *App) generateMenuTitles() []string { + var titles []string + + // Check for auth error first + if app.authError != "" { + titles = append(titles, "āš ļø Authentication Error") + titles = append(titles, app.authError) + titles = append(titles, "To fix this issue:") + titles = append(titles, "1. Install GitHub CLI: brew install gh") + titles = append(titles, "2. Run: gh auth login") + titles = append(titles, "3. Or set GITHUB_TOKEN environment variable") + titles = append(titles, "Quit") + return titles + } + + app.mu.RLock() + incoming := make([]PR, len(app.incoming)) + copy(incoming, app.incoming) + outgoing := make([]PR, len(app.outgoing)) + copy(outgoing, app.outgoing) + hiddenOrgs := make(map[string]bool) + for org, hidden := range app.hiddenOrgs { + hiddenOrgs[org] = hidden + } + hideStale := app.hideStaleIncoming + app.mu.RUnlock() + + // Add common menu items + titles = append(titles, "Web Dashboard") + + // Generate PR section titles + if len(incoming) == 0 && len(outgoing) == 0 { + titles = append(titles, "No pull requests") + } else { + // Add incoming PR titles + if len(incoming) > 0 { + titles = append(titles, "šŸ“„ Incoming PRs") + titles = append(titles, app.generatePRSectionTitles(incoming, "Incoming", hiddenOrgs, hideStale)...) + } + + // Add outgoing PR titles + if len(outgoing) > 0 { + titles = append(titles, "šŸ“¤ Outgoing PRs") + titles = append(titles, app.generatePRSectionTitles(outgoing, "Outgoing", hiddenOrgs, hideStale)...) + } + } + + // Add settings menu items + titles = append(titles, "āš™ļø Settings") + titles = append(titles, "Hide Stale Incoming PRs") + titles = append(titles, "Honks enabled") + titles = append(titles, "Auto-open in Browser") + titles = append(titles, "Hidden Organizations") + titles = append(titles, "Quit") + + return titles +} + +// generatePRSectionTitles generates the titles for a specific PR section +func (app *App) generatePRSectionTitles(prs []PR, sectionTitle string, hiddenOrgs map[string]bool, hideStale bool) []string { + var titles []string + + // Sort PRs by UpdatedAt (most recent first) + sortedPRs := make([]PR, len(prs)) + copy(sortedPRs, prs) + sort.Slice(sortedPRs, func(i, j int) bool { + return sortedPRs[i].UpdatedAt.After(sortedPRs[j].UpdatedAt) + }) + + for prIndex := range sortedPRs { + // Apply filters (same logic as in addPRSection) + org := extractOrgFromRepo(sortedPRs[prIndex].Repository) + if org != "" && hiddenOrgs[org] { + continue + } + + if hideStale && sortedPRs[prIndex].UpdatedAt.Before(time.Now().Add(-stalePRThreshold)) { + continue + } + + title := fmt.Sprintf("%s #%d", sortedPRs[prIndex].Repository, sortedPRs[prIndex].Number) + + // Add bullet point or emoji for blocked PRs (same logic as in addPRSection) + if sortedPRs[prIndex].NeedsReview || sortedPRs[prIndex].IsBlocked { + prState, hasState := app.stateManager.GetPRState(sortedPRs[prIndex].URL) + + if hasState && !prState.FirstBlockedAt.IsZero() && time.Since(prState.FirstBlockedAt) < blockedPRIconDuration { + if sectionTitle == "Outgoing" { + title = fmt.Sprintf("šŸŽ‰ %s", title) + } else { + title = fmt.Sprintf("šŸŖæ %s", title) + } + } else { + title = fmt.Sprintf("ā€¢ %s", title) + } + } + + titles = append(titles, title) + } + + return titles +} + // rebuildMenu completely rebuilds the menu from scratch. func (app *App) rebuildMenu(ctx context.Context) { // Rebuild entire menu @@ -478,7 +583,7 @@ func (app *App) addStaticMenuItems(ctx context.Context) { // Audio cues // Add 'Audio cues' option - audioItem := systray.AddMenuItem("Audio cues", "Play sounds for notifications") + audioItem := systray.AddMenuItem("Honks enabled", "Play sounds for notifications") app.mu.RLock() if app.enableAudioCues { audioItem.Check() From 6bd1a21081e7762263353d8144124f2d6bfaeb02 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Wed, 27 Aug 2025 09:28:01 -0400 Subject: [PATCH 02/13] add placeholder for tests --- cmd/goose/main_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/goose/main_test.go b/cmd/goose/main_test.go index e2a4037..925bbeb 100644 --- a/cmd/goose/main_test.go +++ b/cmd/goose/main_test.go @@ -299,6 +299,7 @@ func TestSoundPlaybackDuringTransitions(t *testing.T) { enableAudioCues: true, initialLoadComplete: true, // Set to true to allow sound playback menuInitialized: true, + lastMenuTitles: []string{"placeholder"}, // Initialize to prevent menu rebuilds during testing } tests := []struct { From f80fc48c75f6df284173b02973fbbec60344e841 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Wed, 27 Aug 2025 09:51:27 -0400 Subject: [PATCH 03/13] mock the systray for tests --- cmd/goose/main.go | 2 ++ cmd/goose/main_test.go | 2 +- cmd/goose/ui.go | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/cmd/goose/main.go b/cmd/goose/main.go index e744f06..a6b9826 100644 --- a/cmd/goose/main.go +++ b/cmd/goose/main.go @@ -87,6 +87,7 @@ type App struct { enableAutoBrowser bool hideStaleIncoming bool noCache bool + systrayInterface SystrayInterface // For mocking systray operations in tests hiddenOrgs map[string]bool seenOrgs map[string]bool @@ -215,6 +216,7 @@ func main() { enableAutoBrowser: false, // Default to false for safety browserRateLimiter: NewBrowserRateLimiter(browserOpenDelay, maxBrowserOpensMinute, maxBrowserOpensDay), startTime: startTime, + systrayInterface: &RealSystray{}, // Use real systray implementation seenOrgs: make(map[string]bool), hiddenOrgs: make(map[string]bool), // Deprecated fields for test compatibility diff --git a/cmd/goose/main_test.go b/cmd/goose/main_test.go index 925bbeb..dee2fe6 100644 --- a/cmd/goose/main_test.go +++ b/cmd/goose/main_test.go @@ -299,7 +299,7 @@ func TestSoundPlaybackDuringTransitions(t *testing.T) { enableAudioCues: true, initialLoadComplete: true, // Set to true to allow sound playback menuInitialized: true, - lastMenuTitles: []string{"placeholder"}, // Initialize to prevent menu rebuilds during testing + systrayInterface: &MockSystray{}, // Use mock systray to avoid Windows-specific panics } tests := []struct { diff --git a/cmd/goose/ui.go b/cmd/goose/ui.go index 9e87be6..24f031d 100644 --- a/cmd/goose/ui.go +++ b/cmd/goose/ui.go @@ -388,7 +388,7 @@ func (app *App) rebuildMenu(ctx context.Context) { // Rebuild entire menu // Clear all existing menu items - systray.ResetMenu() + app.systrayInterface.ResetMenu() // Check for auth error first if app.authError != "" { From ca5c1b47a22ce5a4bb67e6c0a71a0b0276e5385a Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Wed, 27 Aug 2025 09:53:21 -0400 Subject: [PATCH 04/13] add systray_interface --- cmd/goose/systray_interface.go | 71 ++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100644 cmd/goose/systray_interface.go diff --git a/cmd/goose/systray_interface.go b/cmd/goose/systray_interface.go new file mode 100644 index 0000000..1ea6cb6 --- /dev/null +++ b/cmd/goose/systray_interface.go @@ -0,0 +1,71 @@ +package main + +import "github.com/energye/systray" + +// SystrayInterface abstracts systray operations for testing +type SystrayInterface interface { + ResetMenu() + AddMenuItem(title, tooltip string) *systray.MenuItem + AddSeparator() + SetTitle(title string) + SetOnClick(fn func(menu systray.IMenu)) + Quit() +} + +// RealSystray implements SystrayInterface using the actual systray library +type RealSystray struct{} + +func (r *RealSystray) ResetMenu() { + systray.ResetMenu() +} + +func (r *RealSystray) AddMenuItem(title, tooltip string) *systray.MenuItem { + return systray.AddMenuItem(title, tooltip) +} + +func (r *RealSystray) AddSeparator() { + systray.AddSeparator() +} + +func (r *RealSystray) SetTitle(title string) { + systray.SetTitle(title) +} + +func (r *RealSystray) SetOnClick(fn func(menu systray.IMenu)) { + systray.SetOnClick(fn) +} + +func (r *RealSystray) Quit() { + systray.Quit() +} + +// MockSystray implements SystrayInterface for testing +type MockSystray struct { + title string + menuItems []string +} + +func (m *MockSystray) ResetMenu() { + m.menuItems = nil +} + +func (m *MockSystray) AddMenuItem(title, tooltip string) *systray.MenuItem { + m.menuItems = append(m.menuItems, title) + return &systray.MenuItem{} // Return empty menu item for testing +} + +func (m *MockSystray) AddSeparator() { + m.menuItems = append(m.menuItems, "---") +} + +func (m *MockSystray) SetTitle(title string) { + m.title = title +} + +func (m *MockSystray) SetOnClick(fn func(menu systray.IMenu)) { + // No-op for testing +} + +func (m *MockSystray) Quit() { + // No-op for testing +} \ No newline at end of file From d2e77c4adaecd76478a2a2beae885eeed41482a3 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Wed, 27 Aug 2025 10:02:08 -0400 Subject: [PATCH 05/13] use a mock systray --- cmd/goose/click_test.go | 2 ++ cmd/goose/filtering_test.go | 3 ++ cmd/goose/main_test.go | 6 ++++ cmd/goose/ui.go | 55 +++++++++++++++++++------------------ 4 files changed, 40 insertions(+), 26 deletions(-) diff --git a/cmd/goose/click_test.go b/cmd/goose/click_test.go index bf900a7..322e995 100644 --- a/cmd/goose/click_test.go +++ b/cmd/goose/click_test.go @@ -18,6 +18,7 @@ func TestMenuClickRateLimit(t *testing.T) { hiddenOrgs: make(map[string]bool), seenOrgs: make(map[string]bool), lastSearchAttempt: time.Now().Add(-15 * time.Second), // 15 seconds ago + systrayInterface: &MockSystray{}, // Use mock systray to avoid panics } // Simulate the click handler logic (without the actual UI interaction) @@ -69,6 +70,7 @@ func TestScheduledUpdateRateLimit(t *testing.T) { hiddenOrgs: make(map[string]bool), seenOrgs: make(map[string]bool), lastSearchAttempt: time.Now().Add(-5 * time.Second), // 5 seconds ago + systrayInterface: &MockSystray{}, // Use mock systray to avoid panics } // Simulate the scheduled update logic diff --git a/cmd/goose/filtering_test.go b/cmd/goose/filtering_test.go index c1066ba..0c02ee5 100644 --- a/cmd/goose/filtering_test.go +++ b/cmd/goose/filtering_test.go @@ -21,6 +21,7 @@ func TestCountPRsWithHiddenOrgs(t *testing.T) { "org2": true, // Hide org2 }, hideStaleIncoming: false, + systrayInterface: &MockSystray{}, // Use mock systray to avoid panics } counts := app.countPRs() @@ -58,6 +59,7 @@ func TestCountPRsWithStalePRs(t *testing.T) { }, hiddenOrgs: map[string]bool{}, hideStaleIncoming: true, // Hide stale PRs + systrayInterface: &MockSystray{}, // Use mock systray to avoid panics } counts := app.countPRs() @@ -99,6 +101,7 @@ func TestCountPRsWithBothFilters(t *testing.T) { "org2": true, }, hideStaleIncoming: true, + systrayInterface: &MockSystray{}, // Use mock systray to avoid panics } counts := app.countPRs() diff --git a/cmd/goose/main_test.go b/cmd/goose/main_test.go index dee2fe6..f819239 100644 --- a/cmd/goose/main_test.go +++ b/cmd/goose/main_test.go @@ -63,6 +63,7 @@ func TestMenuItemTitleTransition(t *testing.T) { seenOrgs: make(map[string]bool), blockedPRTimes: make(map[string]time.Time), browserRateLimiter: NewBrowserRateLimiter(30*time.Second, 5, defaultMaxBrowserOpensDay), + systrayInterface: &MockSystray{}, // Use mock systray to avoid panics } // Test incoming PR that just became blocked @@ -174,6 +175,7 @@ func TestTrayTitleUpdates(t *testing.T) { hiddenOrgs: make(map[string]bool), seenOrgs: make(map[string]bool), browserRateLimiter: NewBrowserRateLimiter(30*time.Second, 5, defaultMaxBrowserOpensDay), + systrayInterface: &MockSystray{}, // Use mock systray to avoid panics } tests := []struct { @@ -444,6 +446,7 @@ func TestSoundDisabledNoPlayback(t *testing.T) { enableAudioCues: false, // Audio disabled initialLoadComplete: true, menuInitialized: true, + systrayInterface: &MockSystray{}, // Use mock systray to avoid panics } // Note: We verify behavior through state changes rather than direct sound capture @@ -484,6 +487,7 @@ func TestGracePeriodPreventsNotifications(t *testing.T) { initialLoadComplete: true, menuInitialized: true, startTime: time.Now(), // Just started + systrayInterface: &MockSystray{}, // Use mock systray to avoid panics } // Track whether we're in grace period for verification @@ -608,6 +612,7 @@ func TestNotificationScenarios(t *testing.T) { initialLoadComplete: tt.initialLoadComplete, menuInitialized: true, startTime: time.Now().Add(-tt.timeSinceStart), + systrayInterface: &MockSystray{}, // Use mock systray to avoid panics } // Set up previous state @@ -667,6 +672,7 @@ func TestNewlyBlockedPRAfterGracePeriod(t *testing.T) { initialLoadComplete: true, // Already past initial load menuInitialized: true, startTime: time.Now().Add(-35 * time.Second), // Started 35 seconds ago + systrayInterface: &MockSystray{}, // Use mock systray to avoid panics } // Start with no blocked PRs diff --git a/cmd/goose/ui.go b/cmd/goose/ui.go index 24f031d..bd6d307 100644 --- a/cmd/goose/ui.go +++ b/cmd/goose/ui.go @@ -12,9 +12,12 @@ import ( "sort" "time" - "github.com/energye/systray" + "github.com/energye/systray" // needed for MenuItem type ) +// Ensure systray package is used +var _ *systray.MenuItem = nil + // openURL safely opens a URL in the default browser after validation. func openURL(ctx context.Context, rawURL string) error { // Parse and validate the URL @@ -157,7 +160,7 @@ func (app *App) setTrayTitle() { // Log title change with detailed counts log.Printf("[TRAY] Setting title to '%s' (incoming_total=%d, incoming_blocked=%d, outgoing_total=%d, outgoing_blocked=%d)", title, counts.IncomingTotal, counts.IncomingBlocked, counts.OutgoingTotal, counts.OutgoingBlocked) - systray.SetTitle(title) + app.systrayInterface.SetTitle(title) } // addPRSection adds a section of PRs to the menu. @@ -169,7 +172,7 @@ func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string, // Add header headerText := fmt.Sprintf("%s ā€” %d blocked on you", sectionTitle, blockedCount) // Create section header - header := systray.AddMenuItem(headerText, "") + header := app.systrayInterface.AddMenuItem(headerText, "") header.Disable() // Sort PRs with blocked ones first - inline for simplicity @@ -266,7 +269,7 @@ func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string, } // Create PR menu item - item := systray.AddMenuItem(title, tooltip) + item := app.systrayInterface.AddMenuItem(title, tooltip) // Capture URL to avoid loop variable capture bug prURL := sortedPRs[prIndex].URL @@ -393,40 +396,40 @@ func (app *App) rebuildMenu(ctx context.Context) { // Check for auth error first if app.authError != "" { // Show authentication error message - errorTitle := systray.AddMenuItem("āš ļø Authentication Error", "") + errorTitle := app.systrayInterface.AddMenuItem("āš ļø Authentication Error", "") errorTitle.Disable() - systray.AddSeparator() + app.systrayInterface.AddSeparator() // Add error details - errorMsg := systray.AddMenuItem(app.authError, "Click to see setup instructions") + errorMsg := app.systrayInterface.AddMenuItem(app.authError, "Click to see setup instructions") errorMsg.Click(func() { if err := openURL(ctx, "https://cli.github.com/manual/gh_auth_login"); err != nil { log.Printf("failed to open setup instructions: %v", err) } }) - systray.AddSeparator() + app.systrayInterface.AddSeparator() // Add setup instructions - setupInstr := systray.AddMenuItem("To fix this issue:", "") + setupInstr := app.systrayInterface.AddMenuItem("To fix this issue:", "") setupInstr.Disable() - option1 := systray.AddMenuItem("1. Install GitHub CLI: brew install gh", "") + option1 := app.systrayInterface.AddMenuItem("1. Install GitHub CLI: brew install gh", "") option1.Disable() - option2 := systray.AddMenuItem("2. Run: gh auth login", "") + option2 := app.systrayInterface.AddMenuItem("2. Run: gh auth login", "") option2.Disable() - option3 := systray.AddMenuItem("3. Or set GITHUB_TOKEN environment variable", "") + option3 := app.systrayInterface.AddMenuItem("3. Or set GITHUB_TOKEN environment variable", "") option3.Disable() - systray.AddSeparator() + app.systrayInterface.AddSeparator() // Add quit option - quitItem := systray.AddMenuItem("Quit", "") + quitItem := app.systrayInterface.AddMenuItem("Quit", "") quitItem.Click(func() { - systray.Quit() + app.systrayInterface.Quit() }) return @@ -437,14 +440,14 @@ func (app *App) rebuildMenu(ctx context.Context) { // Dashboard at the top // Add Web Dashboard link - dashboardItem := systray.AddMenuItem("Web Dashboard", "") + dashboardItem := app.systrayInterface.AddMenuItem("Web Dashboard", "") dashboardItem.Click(func() { if err := openURL(ctx, "https://dash.ready-to-review.dev/"); err != nil { log.Printf("failed to open dashboard: %v", err) } }) - systray.AddSeparator() + app.systrayInterface.AddSeparator() // Get PR counts counts := app.countPRs() @@ -452,7 +455,7 @@ func (app *App) rebuildMenu(ctx context.Context) { // Handle "No pull requests" case if counts.IncomingTotal == 0 && counts.OutgoingTotal == 0 { // No PRs to display - noPRs := systray.AddMenuItem("No pull requests", "") + noPRs := app.systrayInterface.AddMenuItem("No pull requests", "") noPRs.Disable() } else { // Incoming section @@ -463,7 +466,7 @@ func (app *App) rebuildMenu(ctx context.Context) { app.addPRSection(ctx, incoming, "Incoming", counts.IncomingBlocked) } - systray.AddSeparator() + app.systrayInterface.AddSeparator() // Outgoing section if counts.OutgoingTotal > 0 { @@ -484,11 +487,11 @@ func (app *App) rebuildMenu(ctx context.Context) { func (app *App) addStaticMenuItems(ctx context.Context) { // Add static menu items - systray.AddSeparator() + app.systrayInterface.AddSeparator() // Hide orgs submenu // Add 'Hide orgs' submenu - hideOrgsMenu := systray.AddMenuItem("Hide orgs", "Select organizations to hide PRs from") + hideOrgsMenu := app.systrayInterface.AddMenuItem("Hide orgs", "Select organizations to hide PRs from") // Get combined list of seen orgs and hidden orgs app.mu.RLock() @@ -553,7 +556,7 @@ func (app *App) addStaticMenuItems(ctx context.Context) { // Hide stale PRs // Add 'Hide stale PRs' option - hideStaleItem := systray.AddMenuItem("Hide stale PRs (>90 days)", "") + hideStaleItem := app.systrayInterface.AddMenuItem("Hide stale PRs (>90 days)", "") if app.hideStaleIncoming { hideStaleItem.Check() } @@ -583,7 +586,7 @@ func (app *App) addStaticMenuItems(ctx context.Context) { // Audio cues // Add 'Audio cues' option - audioItem := systray.AddMenuItem("Honks enabled", "Play sounds for notifications") + audioItem := app.systrayInterface.AddMenuItem("Honks enabled", "Play sounds for notifications") app.mu.RLock() if app.enableAudioCues { audioItem.Check() @@ -609,7 +612,7 @@ func (app *App) addStaticMenuItems(ctx context.Context) { // Auto-open blocked PRs in browser // Add 'Auto-open PRs' option - autoOpenItem := systray.AddMenuItem("Auto-open incoming PRs", "Automatically open newly blocked PRs in browser (rate limited)") + autoOpenItem := app.systrayInterface.AddMenuItem("Auto-open incoming PRs", "Automatically open newly blocked PRs in browser (rate limited)") app.mu.RLock() if app.enableAutoBrowser { autoOpenItem.Check() @@ -639,9 +642,9 @@ func (app *App) addStaticMenuItems(ctx context.Context) { // Quit // Add 'Quit' option - quitItem := systray.AddMenuItem("Quit", "") + quitItem := app.systrayInterface.AddMenuItem("Quit", "") quitItem.Click(func() { log.Println("Quit requested by user") - systray.Quit() + app.systrayInterface.Quit() }) } From e7fa7c0742003808d9be7f809475e13a8930889a Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Wed, 27 Aug 2025 10:07:09 -0400 Subject: [PATCH 06/13] fix initial rebuild bug --- cmd/goose/main.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/cmd/goose/main.go b/cmd/goose/main.go index a6b9826..83ed859 100644 --- a/cmd/goose/main.go +++ b/cmd/goose/main.go @@ -538,6 +538,10 @@ func (app *App) updatePRsWithWait(ctx context.Context) { // Create initial menu despite error app.rebuildMenu(ctx) app.menuInitialized = true + // Store initial menu titles to prevent unnecessary rebuild on first update + app.mu.Lock() + app.lastMenuTitles = app.generateMenuTitles() + app.mu.Unlock() // Menu initialization complete } return @@ -580,6 +584,10 @@ func (app *App) updatePRsWithWait(ctx context.Context) { // Initialize menu structure app.rebuildMenu(ctx) app.menuInitialized = true + // Store initial menu titles to prevent unnecessary rebuild on first update + app.mu.Lock() + app.lastMenuTitles = app.generateMenuTitles() + app.mu.Unlock() // Menu initialization complete } else { app.updateMenu(ctx) From b41f870ad985d5aa46db4fbddcc5ff3592d77a08 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Wed, 27 Aug 2025 10:12:52 -0400 Subject: [PATCH 07/13] add menu chane detection test --- cmd/goose/menu_change_detection_test.go | 200 ++++++++++++++++++++++++ 1 file changed, 200 insertions(+) create mode 100644 cmd/goose/menu_change_detection_test.go diff --git a/cmd/goose/menu_change_detection_test.go b/cmd/goose/menu_change_detection_test.go new file mode 100644 index 0000000..23a69d6 --- /dev/null +++ b/cmd/goose/menu_change_detection_test.go @@ -0,0 +1,200 @@ +package main + +import ( + "slices" + "sync" + "testing" + "time" +) + +// TestMenuChangeDetection tests that the menu change detection logic works correctly +// and prevents unnecessary menu rebuilds when PR data hasn't changed. +func TestMenuChangeDetection(t *testing.T) { + // Create app with test data + app := &App{ + mu: sync.RWMutex{}, + stateManager: NewPRStateManager(time.Now()), + hiddenOrgs: make(map[string]bool), + seenOrgs: make(map[string]bool), + blockedPRTimes: make(map[string]time.Time), + browserRateLimiter: NewBrowserRateLimiter(30*time.Second, 5, defaultMaxBrowserOpensDay), + systrayInterface: &MockSystray{}, + incoming: []PR{ + {Repository: "org1/repo1", Number: 1, Title: "Fix bug", URL: "https://github.com/org1/repo1/pull/1", NeedsReview: true, UpdatedAt: time.Now()}, + {Repository: "org2/repo2", Number: 2, Title: "Add feature", URL: "https://github.com/org2/repo2/pull/2", NeedsReview: false, UpdatedAt: time.Now()}, + }, + outgoing: []PR{ + {Repository: "org3/repo3", Number: 3, Title: "Update docs", URL: "https://github.com/org3/repo3/pull/3", IsBlocked: true, UpdatedAt: time.Now()}, + }, + } + + t.Run("same_titles_should_be_equal", func(t *testing.T) { + // Generate titles twice with same data + titles1 := app.generateMenuTitles() + titles2 := app.generateMenuTitles() + + // They should be equal + if !slices.Equal(titles1, titles2) { + t.Errorf("Same PR data generated different titles:\nFirst: %v\nSecond: %v", titles1, titles2) + } + }) + + t.Run("different_pr_count_changes_titles", func(t *testing.T) { + // Generate initial titles + initialTitles := app.generateMenuTitles() + + // Add a new PR + app.incoming = append(app.incoming, PR{ + Repository: "org4/repo4", + Number: 4, + Title: "New PR", + URL: "https://github.com/org4/repo4/pull/4", + NeedsReview: true, + UpdatedAt: time.Now(), + }) + + // Generate new titles + newTitles := app.generateMenuTitles() + + // They should be different + if slices.Equal(initialTitles, newTitles) { + t.Error("Adding a PR didn't change the menu titles") + } + + // The new titles should have more items + if len(newTitles) <= len(initialTitles) { + t.Errorf("New titles should have more items: got %d, initial had %d", len(newTitles), len(initialTitles)) + } + }) + + t.Run("pr_repository_change_updates_menu", func(t *testing.T) { + // Generate initial titles + initialTitles := app.generateMenuTitles() + + // Change a PR repository (this would be unusual but tests the title generation) + app.incoming[0].Repository = "different-org/different-repo" + + // Generate new titles + newTitles := app.generateMenuTitles() + + // They should be different because menu shows "org/repo #number" + if slices.Equal(initialTitles, newTitles) { + t.Error("Changing a PR repository didn't change the menu titles") + } + }) + + t.Run("blocked_status_change_updates_menu", func(t *testing.T) { + // Generate initial titles + initialTitles := app.generateMenuTitles() + + // Change blocked status + app.incoming[1].NeedsReview = true // Make it blocked + + // Generate new titles + newTitles := app.generateMenuTitles() + + // They should be different because the title prefix changes for blocked PRs + if slices.Equal(initialTitles, newTitles) { + t.Error("Changing PR blocked status didn't change the menu titles") + } + }) +} + +// TestFirstRunMenuRebuildBug tests the specific bug where the first scheduled update +// after initial load would unnecessarily rebuild the menu. +func TestFirstRunMenuRebuildBug(t *testing.T) { + // Create app simulating initial state + app := &App{ + mu: sync.RWMutex{}, + stateManager: NewPRStateManager(time.Now()), + hiddenOrgs: make(map[string]bool), + seenOrgs: make(map[string]bool), + blockedPRTimes: make(map[string]time.Time), + browserRateLimiter: NewBrowserRateLimiter(30*time.Second, 5, defaultMaxBrowserOpensDay), + menuInitialized: false, + systrayInterface: &MockSystray{}, + lastMenuTitles: nil, // This is nil on first run - the bug condition + incoming: []PR{ + {Repository: "test/repo", Number: 1, Title: "Test PR", URL: "https://github.com/test/repo/pull/1"}, + }, + } + + // Simulate what happens during initial load + // OLD BEHAVIOR: lastMenuTitles would remain nil + // NEW BEHAVIOR: lastMenuTitles should be set after initial menu build + + // Generate initial titles (simulating what rebuildMenu would show) + initialTitles := app.generateMenuTitles() + + // This is the fix - store titles after initial build + app.mu.Lock() + app.lastMenuTitles = initialTitles + app.menuInitialized = true + app.mu.Unlock() + + // Now simulate first scheduled update with same data + // Generate current titles (should be same as initial) + currentTitles := app.generateMenuTitles() + + // Get stored titles + app.mu.RLock() + storedTitles := app.lastMenuTitles + app.mu.RUnlock() + + // Test 1: Stored titles should not be nil/empty + if len(storedTitles) == 0 { + t.Fatal("BUG: lastMenuTitles not set after initial menu build") + } + + // Test 2: Current and stored titles should be equal (no changes) + if !slices.Equal(currentTitles, storedTitles) { + t.Errorf("BUG: Titles marked as different when they're the same:\nCurrent: %v\nStored: %v", + currentTitles, storedTitles) + } + + // Test 3: Verify the comparison result that updateMenu would make + // In the bug, this would be comparing non-empty current titles with nil/empty stored titles + // and would return false (different), triggering unnecessary rebuild + shouldSkipRebuild := slices.Equal(currentTitles, storedTitles) + if !shouldSkipRebuild { + t.Error("BUG: Would rebuild menu even though PR data hasn't changed") + } +} + +// TestHiddenOrgChangesMenu tests that hiding/showing orgs updates menu titles +func TestHiddenOrgChangesMenu(t *testing.T) { + app := &App{ + mu: sync.RWMutex{}, + stateManager: NewPRStateManager(time.Now()), + hiddenOrgs: make(map[string]bool), + seenOrgs: make(map[string]bool), + blockedPRTimes: make(map[string]time.Time), + browserRateLimiter: NewBrowserRateLimiter(30*time.Second, 5, defaultMaxBrowserOpensDay), + systrayInterface: &MockSystray{}, + incoming: []PR{ + {Repository: "org1/repo1", Number: 1, Title: "PR 1", URL: "https://github.com/org1/repo1/pull/1"}, + {Repository: "org2/repo2", Number: 2, Title: "PR 2", URL: "https://github.com/org2/repo2/pull/2"}, + }, + } + + // Generate initial titles + initialTitles := app.generateMenuTitles() + initialCount := len(initialTitles) + + // Hide org1 + app.hiddenOrgs["org1"] = true + + // Generate new titles - should have fewer items + newTitles := app.generateMenuTitles() + + // Titles should be different + if slices.Equal(initialTitles, newTitles) { + t.Error("Hiding an org didn't change menu titles") + } + + // Should have fewer items (org1/repo1 should be hidden) + if len(newTitles) >= initialCount { + t.Errorf("Menu should have fewer items after hiding org: got %d, started with %d", + len(newTitles), initialCount) + } +} \ No newline at end of file From f8941dbbc97f5a217253e55a289bcd212dc7899a Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Wed, 27 Aug 2025 11:49:19 -0400 Subject: [PATCH 08/13] avoid panics --- cmd/goose/click_test.go | 6 +-- cmd/goose/filtering_test.go | 6 +-- cmd/goose/github.go | 43 +++++++++++++++----- cmd/goose/main.go | 27 +++++++------ cmd/goose/main_test.go | 8 ++-- cmd/goose/menu_change_detection_test.go | 54 ++++++++++++------------- cmd/goose/systray_interface.go | 49 +++++++++++++++------- cmd/goose/ui.go | 54 +++++++++++++------------ 8 files changed, 148 insertions(+), 99 deletions(-) diff --git a/cmd/goose/click_test.go b/cmd/goose/click_test.go index 322e995..d6bfd8c 100644 --- a/cmd/goose/click_test.go +++ b/cmd/goose/click_test.go @@ -18,7 +18,7 @@ func TestMenuClickRateLimit(t *testing.T) { hiddenOrgs: make(map[string]bool), seenOrgs: make(map[string]bool), lastSearchAttempt: time.Now().Add(-15 * time.Second), // 15 seconds ago - systrayInterface: &MockSystray{}, // Use mock systray to avoid panics + systrayInterface: &MockSystray{}, // Use mock systray to avoid panics } // Simulate the click handler logic (without the actual UI interaction) @@ -70,7 +70,7 @@ func TestScheduledUpdateRateLimit(t *testing.T) { hiddenOrgs: make(map[string]bool), seenOrgs: make(map[string]bool), lastSearchAttempt: time.Now().Add(-5 * time.Second), // 5 seconds ago - systrayInterface: &MockSystray{}, // Use mock systray to avoid panics + systrayInterface: &MockSystray{}, // Use mock systray to avoid panics } // Simulate the scheduled update logic @@ -97,4 +97,4 @@ func TestScheduledUpdateRateLimit(t *testing.T) { if !shouldUpdate2 { t.Errorf("Scheduled update after 12 seconds should proceed, last search was %v ago", timeSince2) } -} \ No newline at end of file +} diff --git a/cmd/goose/filtering_test.go b/cmd/goose/filtering_test.go index 0c02ee5..df0b8b0 100644 --- a/cmd/goose/filtering_test.go +++ b/cmd/goose/filtering_test.go @@ -21,7 +21,7 @@ func TestCountPRsWithHiddenOrgs(t *testing.T) { "org2": true, // Hide org2 }, hideStaleIncoming: false, - systrayInterface: &MockSystray{}, // Use mock systray to avoid panics + systrayInterface: &MockSystray{}, // Use mock systray to avoid panics } counts := app.countPRs() @@ -58,7 +58,7 @@ func TestCountPRsWithStalePRs(t *testing.T) { {Repository: "org1/repo5", IsBlocked: true, UpdatedAt: recentTime}, }, hiddenOrgs: map[string]bool{}, - hideStaleIncoming: true, // Hide stale PRs + hideStaleIncoming: true, // Hide stale PRs systrayInterface: &MockSystray{}, // Use mock systray to avoid panics } @@ -101,7 +101,7 @@ func TestCountPRsWithBothFilters(t *testing.T) { "org2": true, }, hideStaleIncoming: true, - systrayInterface: &MockSystray{}, // Use mock systray to avoid panics + systrayInterface: &MockSystray{}, // Use mock systray to avoid panics } counts := app.countPRs() diff --git a/cmd/goose/github.go b/cmd/goose/github.go index f6f1e44..8b84c7b 100644 --- a/cmd/goose/github.go +++ b/cmd/goose/github.go @@ -138,16 +138,39 @@ func (*App) token(ctx context.Context) (string, error) { } log.Printf("Executing command: %s auth token", ghPath) - cmd := exec.CommandContext(ctx, ghPath, "auth", "token") - output, err := cmd.CombinedOutput() - if err != nil { - log.Printf("gh command failed: %v", err) - return "", fmt.Errorf("exec 'gh auth token': %w", err) - } - token := strings.TrimSpace(string(output)) - if err := validateGitHubToken(token); err != nil { - return "", fmt.Errorf("invalid token from gh CLI: %w", err) + + // Use retry logic for gh CLI command as it may fail temporarily + var token string + retryErr := retry.Do(func() error { + // Create timeout context for gh CLI call + cmdCtx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + + cmd := exec.CommandContext(cmdCtx, ghPath, "auth", "token") + output, cmdErr := cmd.CombinedOutput() + if cmdErr != nil { + log.Printf("gh command failed (will retry): %v", cmdErr) + return fmt.Errorf("exec 'gh auth token': %w", cmdErr) + } + + token = strings.TrimSpace(string(output)) + if validateErr := validateGitHubToken(token); validateErr != nil { + // Don't retry on invalid token - it won't get better + return retry.Unrecoverable(fmt.Errorf("invalid token from gh CLI: %w", validateErr)) + } + return nil + }, + retry.Attempts(3), // Fewer attempts for local command + retry.Delay(time.Second), + retry.OnRetry(func(n uint, err error) { + log.Printf("[GH CLI] Retry %d/3: %v", n+1, err) + }), + retry.Context(ctx), + ) + if retryErr != nil { + return "", retryErr } + log.Println("Successfully obtained GitHub token from gh CLI") return token, nil } @@ -227,7 +250,7 @@ func (app *App) fetchPRsInternal(ctx context.Context) (incoming []PR, outgoing [ app.mu.Lock() app.lastSearchAttempt = time.Now() app.mu.Unlock() - + // Check if we have a client if app.client == nil { return nil, nil, fmt.Errorf("no GitHub client available: %s", app.authError) diff --git a/cmd/goose/main.go b/cmd/goose/main.go index 83ed859..4cb0deb 100644 --- a/cmd/goose/main.go +++ b/cmd/goose/main.go @@ -256,12 +256,12 @@ func (app *App) onReady(ctx context.Context) { // Set up click handlers first (needed for both success and error states) systray.SetOnClick(func(menu systray.IMenu) { log.Println("Icon clicked") - + // Check if we can perform a forced refresh (rate limited to every 10 seconds) app.mu.RLock() timeSinceLastSearch := time.Since(app.lastSearchAttempt) app.mu.RUnlock() - + if timeSinceLastSearch >= minUpdateInterval { log.Printf("[CLICK] Forcing search refresh (last search %v ago)", timeSinceLastSearch) go func() { @@ -271,7 +271,7 @@ func (app *App) onReady(ctx context.Context) { remainingTime := minUpdateInterval - timeSinceLastSearch log.Printf("[CLICK] Rate limited - search performed %v ago, %v remaining", timeSinceLastSearch, remainingTime) } - + if menu != nil { if err := menu.ShowMenu(); err != nil { log.Printf("Failed to show menu: %v", err) @@ -350,13 +350,13 @@ func (app *App) updateLoop(ctx context.Context) { app.mu.RLock() timeSinceLastSearch := time.Since(app.lastSearchAttempt) app.mu.RUnlock() - + if timeSinceLastSearch >= minUpdateInterval { log.Println("Running scheduled PR update") app.updatePRs(ctx) } else { remainingTime := minUpdateInterval - timeSinceLastSearch - log.Printf("Skipping scheduled update - recent search %v ago, %v remaining until next allowed", + log.Printf("Skipping scheduled update - recent search %v ago, %v remaining until next allowed", timeSinceLastSearch, remainingTime) } case <-ctx.Done(): @@ -480,29 +480,28 @@ func (app *App) updatePRs(ctx context.Context) { func (app *App) updateMenu(ctx context.Context) { // Generate current menu titles currentTitles := app.generateMenuTitles() - + // Compare with last titles to see if rebuild is needed app.mu.RLock() lastTitles := app.lastMenuTitles app.mu.RUnlock() - + // Check if titles have changed if slices.Equal(currentTitles, lastTitles) { log.Printf("[MENU] No changes detected, skipping rebuild (%d items unchanged)", len(currentTitles)) return } - + // Titles have changed, rebuild menu log.Printf("[MENU] Changes detected, rebuilding menu (%dā†’%d items)", len(lastTitles), len(currentTitles)) app.rebuildMenu(ctx) - + // Store new titles app.mu.Lock() app.lastMenuTitles = currentTitles app.mu.Unlock() } - // updatePRsWithWait fetches PRs and waits for Turn data before building initial menu. func (app *App) updatePRsWithWait(ctx context.Context) { incoming, outgoing, err := app.fetchPRsInternal(ctx) @@ -539,8 +538,10 @@ func (app *App) updatePRsWithWait(ctx context.Context) { app.rebuildMenu(ctx) app.menuInitialized = true // Store initial menu titles to prevent unnecessary rebuild on first update + // generateMenuTitles acquires its own read lock, so we can't hold a lock here + menuTitles := app.generateMenuTitles() app.mu.Lock() - app.lastMenuTitles = app.generateMenuTitles() + app.lastMenuTitles = menuTitles app.mu.Unlock() // Menu initialization complete } @@ -585,8 +586,10 @@ func (app *App) updatePRsWithWait(ctx context.Context) { app.rebuildMenu(ctx) app.menuInitialized = true // Store initial menu titles to prevent unnecessary rebuild on first update + // generateMenuTitles acquires its own read lock, so we can't hold a lock here + menuTitles := app.generateMenuTitles() app.mu.Lock() - app.lastMenuTitles = app.generateMenuTitles() + app.lastMenuTitles = menuTitles app.mu.Unlock() // Menu initialization complete } else { diff --git a/cmd/goose/main_test.go b/cmd/goose/main_test.go index f819239..24c6c98 100644 --- a/cmd/goose/main_test.go +++ b/cmd/goose/main_test.go @@ -11,7 +11,9 @@ import ( func TestMain(m *testing.M) { // Set test mode to prevent actual sound playback during tests - _ = os.Setenv("GOOSE_TEST_MODE", "1") + if err := os.Setenv("GOOSE_TEST_MODE", "1"); err != nil { + panic(err) + } os.Exit(m.Run()) } @@ -486,7 +488,7 @@ func TestGracePeriodPreventsNotifications(t *testing.T) { enableAudioCues: true, initialLoadComplete: true, menuInitialized: true, - startTime: time.Now(), // Just started + startTime: time.Now(), // Just started systrayInterface: &MockSystray{}, // Use mock systray to avoid panics } @@ -672,7 +674,7 @@ func TestNewlyBlockedPRAfterGracePeriod(t *testing.T) { initialLoadComplete: true, // Already past initial load menuInitialized: true, startTime: time.Now().Add(-35 * time.Second), // Started 35 seconds ago - systrayInterface: &MockSystray{}, // Use mock systray to avoid panics + systrayInterface: &MockSystray{}, // Use mock systray to avoid panics } // Start with no blocked PRs diff --git a/cmd/goose/menu_change_detection_test.go b/cmd/goose/menu_change_detection_test.go index 23a69d6..f87780b 100644 --- a/cmd/goose/menu_change_detection_test.go +++ b/cmd/goose/menu_change_detection_test.go @@ -12,13 +12,13 @@ import ( func TestMenuChangeDetection(t *testing.T) { // Create app with test data app := &App{ - mu: sync.RWMutex{}, - stateManager: NewPRStateManager(time.Now()), - hiddenOrgs: make(map[string]bool), - seenOrgs: make(map[string]bool), - blockedPRTimes: make(map[string]time.Time), - browserRateLimiter: NewBrowserRateLimiter(30*time.Second, 5, defaultMaxBrowserOpensDay), - systrayInterface: &MockSystray{}, + mu: sync.RWMutex{}, + stateManager: NewPRStateManager(time.Now()), + hiddenOrgs: make(map[string]bool), + seenOrgs: make(map[string]bool), + blockedPRTimes: make(map[string]time.Time), + browserRateLimiter: NewBrowserRateLimiter(30*time.Second, 5, defaultMaxBrowserOpensDay), + systrayInterface: &MockSystray{}, incoming: []PR{ {Repository: "org1/repo1", Number: 1, Title: "Fix bug", URL: "https://github.com/org1/repo1/pull/1", NeedsReview: true, UpdatedAt: time.Now()}, {Repository: "org2/repo2", Number: 2, Title: "Add feature", URL: "https://github.com/org2/repo2/pull/2", NeedsReview: false, UpdatedAt: time.Now()}, @@ -105,15 +105,15 @@ func TestMenuChangeDetection(t *testing.T) { func TestFirstRunMenuRebuildBug(t *testing.T) { // Create app simulating initial state app := &App{ - mu: sync.RWMutex{}, - stateManager: NewPRStateManager(time.Now()), - hiddenOrgs: make(map[string]bool), - seenOrgs: make(map[string]bool), - blockedPRTimes: make(map[string]time.Time), - browserRateLimiter: NewBrowserRateLimiter(30*time.Second, 5, defaultMaxBrowserOpensDay), - menuInitialized: false, - systrayInterface: &MockSystray{}, - lastMenuTitles: nil, // This is nil on first run - the bug condition + mu: sync.RWMutex{}, + stateManager: NewPRStateManager(time.Now()), + hiddenOrgs: make(map[string]bool), + seenOrgs: make(map[string]bool), + blockedPRTimes: make(map[string]time.Time), + browserRateLimiter: NewBrowserRateLimiter(30*time.Second, 5, defaultMaxBrowserOpensDay), + menuInitialized: false, + systrayInterface: &MockSystray{}, + lastMenuTitles: nil, // This is nil on first run - the bug condition incoming: []PR{ {Repository: "test/repo", Number: 1, Title: "Test PR", URL: "https://github.com/test/repo/pull/1"}, }, @@ -148,7 +148,7 @@ func TestFirstRunMenuRebuildBug(t *testing.T) { // Test 2: Current and stored titles should be equal (no changes) if !slices.Equal(currentTitles, storedTitles) { - t.Errorf("BUG: Titles marked as different when they're the same:\nCurrent: %v\nStored: %v", + t.Errorf("BUG: Titles marked as different when they're the same:\nCurrent: %v\nStored: %v", currentTitles, storedTitles) } @@ -164,13 +164,13 @@ func TestFirstRunMenuRebuildBug(t *testing.T) { // TestHiddenOrgChangesMenu tests that hiding/showing orgs updates menu titles func TestHiddenOrgChangesMenu(t *testing.T) { app := &App{ - mu: sync.RWMutex{}, - stateManager: NewPRStateManager(time.Now()), - hiddenOrgs: make(map[string]bool), - seenOrgs: make(map[string]bool), - blockedPRTimes: make(map[string]time.Time), - browserRateLimiter: NewBrowserRateLimiter(30*time.Second, 5, defaultMaxBrowserOpensDay), - systrayInterface: &MockSystray{}, + mu: sync.RWMutex{}, + stateManager: NewPRStateManager(time.Now()), + hiddenOrgs: make(map[string]bool), + seenOrgs: make(map[string]bool), + blockedPRTimes: make(map[string]time.Time), + browserRateLimiter: NewBrowserRateLimiter(30*time.Second, 5, defaultMaxBrowserOpensDay), + systrayInterface: &MockSystray{}, incoming: []PR{ {Repository: "org1/repo1", Number: 1, Title: "PR 1", URL: "https://github.com/org1/repo1/pull/1"}, {Repository: "org2/repo2", Number: 2, Title: "PR 2", URL: "https://github.com/org2/repo2/pull/2"}, @@ -186,7 +186,7 @@ func TestHiddenOrgChangesMenu(t *testing.T) { // Generate new titles - should have fewer items newTitles := app.generateMenuTitles() - + // Titles should be different if slices.Equal(initialTitles, newTitles) { t.Error("Hiding an org didn't change menu titles") @@ -194,7 +194,7 @@ func TestHiddenOrgChangesMenu(t *testing.T) { // Should have fewer items (org1/repo1 should be hidden) if len(newTitles) >= initialCount { - t.Errorf("Menu should have fewer items after hiding org: got %d, started with %d", + t.Errorf("Menu should have fewer items after hiding org: got %d, started with %d", len(newTitles), initialCount) } -} \ No newline at end of file +} diff --git a/cmd/goose/systray_interface.go b/cmd/goose/systray_interface.go index 1ea6cb6..ab18b2e 100644 --- a/cmd/goose/systray_interface.go +++ b/cmd/goose/systray_interface.go @@ -1,8 +1,12 @@ package main -import "github.com/energye/systray" +import ( + "reflect" -// SystrayInterface abstracts systray operations for testing + "github.com/energye/systray" +) + +// SystrayInterface abstracts systray operations for testing. type SystrayInterface interface { ResetMenu() AddMenuItem(title, tooltip string) *systray.MenuItem @@ -12,34 +16,34 @@ type SystrayInterface interface { Quit() } -// RealSystray implements SystrayInterface using the actual systray library +// RealSystray implements SystrayInterface using the actual systray library. type RealSystray struct{} -func (r *RealSystray) ResetMenu() { +func (*RealSystray) ResetMenu() { systray.ResetMenu() } -func (r *RealSystray) AddMenuItem(title, tooltip string) *systray.MenuItem { +func (*RealSystray) AddMenuItem(title, tooltip string) *systray.MenuItem { return systray.AddMenuItem(title, tooltip) } -func (r *RealSystray) AddSeparator() { +func (*RealSystray) AddSeparator() { systray.AddSeparator() } -func (r *RealSystray) SetTitle(title string) { +func (*RealSystray) SetTitle(title string) { systray.SetTitle(title) } -func (r *RealSystray) SetOnClick(fn func(menu systray.IMenu)) { +func (*RealSystray) SetOnClick(fn func(menu systray.IMenu)) { systray.SetOnClick(fn) } -func (r *RealSystray) Quit() { +func (*RealSystray) Quit() { systray.Quit() } -// MockSystray implements SystrayInterface for testing +// MockSystray implements SystrayInterface for testing. type MockSystray struct { title string menuItems []string @@ -49,9 +53,24 @@ func (m *MockSystray) ResetMenu() { m.menuItems = nil } -func (m *MockSystray) AddMenuItem(title, tooltip string) *systray.MenuItem { +func (m *MockSystray) AddMenuItem(title, _ string) *systray.MenuItem { m.menuItems = append(m.menuItems, title) - return &systray.MenuItem{} // Return empty menu item for testing + // Create a MenuItem with initialized internal maps using reflection + // This prevents nil map panics when methods are called + item := &systray.MenuItem{} + + // Use reflection to initialize internal maps if they exist + // This is a hack but necessary for testing + rv := reflect.ValueOf(item).Elem() + rt := rv.Type() + for i := 0; i < rt.NumField(); i++ { + field := rv.Field(i) + if field.Kind() == reflect.Map && field.IsNil() && field.CanSet() { + field.Set(reflect.MakeMap(field.Type())) + } + } + + return item } func (m *MockSystray) AddSeparator() { @@ -62,10 +81,10 @@ func (m *MockSystray) SetTitle(title string) { m.title = title } -func (m *MockSystray) SetOnClick(fn func(menu systray.IMenu)) { +func (*MockSystray) SetOnClick(_ func(menu systray.IMenu)) { // No-op for testing } -func (m *MockSystray) Quit() { +func (*MockSystray) Quit() { // No-op for testing -} \ No newline at end of file +} diff --git a/cmd/goose/ui.go b/cmd/goose/ui.go index bd6d307..54854c3 100644 --- a/cmd/goose/ui.go +++ b/cmd/goose/ui.go @@ -15,7 +15,7 @@ import ( "github.com/energye/systray" // needed for MenuItem type ) -// Ensure systray package is used +// Ensure systray package is used. var _ *systray.MenuItem = nil // openURL safely opens a URL in the default browser after validation. @@ -285,16 +285,17 @@ func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string, // without actually building the UI. Used for change detection. func (app *App) generateMenuTitles() []string { var titles []string - + // Check for auth error first if app.authError != "" { - titles = append(titles, "āš ļø Authentication Error") - titles = append(titles, app.authError) - titles = append(titles, "To fix this issue:") - titles = append(titles, "1. Install GitHub CLI: brew install gh") - titles = append(titles, "2. Run: gh auth login") - titles = append(titles, "3. Or set GITHUB_TOKEN environment variable") - titles = append(titles, "Quit") + titles = append(titles, + "āš ļø Authentication Error", + app.authError, + "To fix this issue:", + "1. Install GitHub CLI: brew install gh", + "2. Run: gh auth login", + "3. Or set GITHUB_TOKEN environment variable", + "Quit") return titles } @@ -322,8 +323,8 @@ func (app *App) generateMenuTitles() []string { titles = append(titles, "šŸ“„ Incoming PRs") titles = append(titles, app.generatePRSectionTitles(incoming, "Incoming", hiddenOrgs, hideStale)...) } - - // Add outgoing PR titles + + // Add outgoing PR titles if len(outgoing) > 0 { titles = append(titles, "šŸ“¤ Outgoing PRs") titles = append(titles, app.generatePRSectionTitles(outgoing, "Outgoing", hiddenOrgs, hideStale)...) @@ -331,44 +332,45 @@ func (app *App) generateMenuTitles() []string { } // Add settings menu items - titles = append(titles, "āš™ļø Settings") - titles = append(titles, "Hide Stale Incoming PRs") - titles = append(titles, "Honks enabled") - titles = append(titles, "Auto-open in Browser") - titles = append(titles, "Hidden Organizations") - titles = append(titles, "Quit") - + titles = append(titles, + "āš™ļø Settings", + "Hide Stale Incoming PRs", + "Honks enabled", + "Auto-open in Browser", + "Hidden Organizations", + "Quit") + return titles } -// generatePRSectionTitles generates the titles for a specific PR section +// generatePRSectionTitles generates the titles for a specific PR section. func (app *App) generatePRSectionTitles(prs []PR, sectionTitle string, hiddenOrgs map[string]bool, hideStale bool) []string { var titles []string - + // Sort PRs by UpdatedAt (most recent first) sortedPRs := make([]PR, len(prs)) copy(sortedPRs, prs) sort.Slice(sortedPRs, func(i, j int) bool { return sortedPRs[i].UpdatedAt.After(sortedPRs[j].UpdatedAt) }) - + for prIndex := range sortedPRs { // Apply filters (same logic as in addPRSection) org := extractOrgFromRepo(sortedPRs[prIndex].Repository) if org != "" && hiddenOrgs[org] { continue } - + if hideStale && sortedPRs[prIndex].UpdatedAt.Before(time.Now().Add(-stalePRThreshold)) { continue } title := fmt.Sprintf("%s #%d", sortedPRs[prIndex].Repository, sortedPRs[prIndex].Number) - + // Add bullet point or emoji for blocked PRs (same logic as in addPRSection) if sortedPRs[prIndex].NeedsReview || sortedPRs[prIndex].IsBlocked { prState, hasState := app.stateManager.GetPRState(sortedPRs[prIndex].URL) - + if hasState && !prState.FirstBlockedAt.IsZero() && time.Since(prState.FirstBlockedAt) < blockedPRIconDuration { if sectionTitle == "Outgoing" { title = fmt.Sprintf("šŸŽ‰ %s", title) @@ -379,10 +381,10 @@ func (app *App) generatePRSectionTitles(prs []PR, sectionTitle string, hiddenOrg title = fmt.Sprintf("ā€¢ %s", title) } } - + titles = append(titles, title) } - + return titles } From 4e4c5326b2166709943d3064e5ee444db3240477 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Wed, 27 Aug 2025 15:19:30 -0400 Subject: [PATCH 09/13] fix lint issues --- cmd/goose/main_test.go | 4 ++-- cmd/goose/notifications.go | 2 +- cmd/goose/pr_state.go | 8 ++++---- cmd/goose/pr_state_test.go | 4 ++-- cmd/goose/systray_interface.go | 30 +++++++++--------------------- cmd/goose/ui.go | 4 ++-- 6 files changed, 20 insertions(+), 32 deletions(-) diff --git a/cmd/goose/main_test.go b/cmd/goose/main_test.go index 24c6c98..e2ddd6a 100644 --- a/cmd/goose/main_test.go +++ b/cmd/goose/main_test.go @@ -644,7 +644,7 @@ func TestNotificationScenarios(t *testing.T) { t.Errorf("%s: Expected PR to be tracked as blocked", tt.description) } // Should have set FirstBlockedAt in state manager - if state, exists := app.stateManager.GetPRState("https://github.com/test/repo/pull/1"); !exists || state.FirstBlockedAt.IsZero() { + if state, exists := app.stateManager.PRState("https://github.com/test/repo/pull/1"); !exists || state.FirstBlockedAt.IsZero() { t.Errorf("%s: Expected FirstBlockedAt to be set in state manager", tt.description) } } @@ -699,7 +699,7 @@ func TestNewlyBlockedPRAfterGracePeriod(t *testing.T) { } // Verify FirstBlockedAt was set in state manager - if state, exists := app.stateManager.GetPRState("https://github.com/test/repo/pull/1"); !exists || state.FirstBlockedAt.IsZero() { + if state, exists := app.stateManager.PRState("https://github.com/test/repo/pull/1"); !exists || state.FirstBlockedAt.IsZero() { t.Error("Expected FirstBlockedAt to be set for newly blocked PR in state manager") } } diff --git a/cmd/goose/notifications.go b/cmd/goose/notifications.go index 1490554..b5a7e05 100644 --- a/cmd/goose/notifications.go +++ b/cmd/goose/notifications.go @@ -33,7 +33,7 @@ func (app *App) processNotifications(ctx context.Context) { app.mu.Lock() app.previousBlockedPRs = make(map[string]bool) app.blockedPRTimes = make(map[string]time.Time) - states := app.stateManager.GetBlockedPRs() + states := app.stateManager.BlockedPRs() for url, state := range states { app.previousBlockedPRs[url] = true app.blockedPRTimes[url] = state.FirstBlockedAt diff --git a/cmd/goose/pr_state.go b/cmd/goose/pr_state.go index dd67041..f8398fd 100644 --- a/cmd/goose/pr_state.go +++ b/cmd/goose/pr_state.go @@ -120,8 +120,8 @@ func (m *PRStateManager) UpdatePRs(incoming, outgoing []PR, hiddenOrgs map[strin return toNotify } -// GetBlockedPRs returns all currently blocked PRs with their states. -func (m *PRStateManager) GetBlockedPRs() map[string]*PRState { +// BlockedPRs returns all currently blocked PRs with their states. +func (m *PRStateManager) BlockedPRs() map[string]*PRState { m.mu.RLock() defer m.mu.RUnlock() @@ -132,8 +132,8 @@ func (m *PRStateManager) GetBlockedPRs() map[string]*PRState { return result } -// GetPRState returns the state for a specific PR. -func (m *PRStateManager) GetPRState(url string) (*PRState, bool) { +// PRState returns the state for a specific PR. +func (m *PRStateManager) PRState(url string) (*PRState, bool) { m.mu.RLock() defer m.mu.RUnlock() diff --git a/cmd/goose/pr_state_test.go b/cmd/goose/pr_state_test.go index c42fc67..9beb87f 100644 --- a/cmd/goose/pr_state_test.go +++ b/cmd/goose/pr_state_test.go @@ -36,7 +36,7 @@ func TestPRStateManager(t *testing.T) { } // Verify state was removed - if _, exists := mgr.GetPRState(pr1.URL); exists { + if _, exists := mgr.PRState(pr1.URL); exists { t.Error("Expected PR state to be removed when unblocked") } @@ -66,7 +66,7 @@ func TestPRStateManagerGracePeriod(t *testing.T) { } // Verify state is still tracked - if _, exists := mgr.GetPRState(pr1.URL); !exists { + if _, exists := mgr.PRState(pr1.URL); !exists { t.Error("Expected PR state to be tracked even during grace period") } diff --git a/cmd/goose/systray_interface.go b/cmd/goose/systray_interface.go index ab18b2e..4b0af61 100644 --- a/cmd/goose/systray_interface.go +++ b/cmd/goose/systray_interface.go @@ -1,15 +1,13 @@ package main import ( - "reflect" - "github.com/energye/systray" ) // SystrayInterface abstracts systray operations for testing. type SystrayInterface interface { ResetMenu() - AddMenuItem(title, tooltip string) *systray.MenuItem + AddMenuItem(title, tooltip string) MenuItem AddSeparator() SetTitle(title string) SetOnClick(fn func(menu systray.IMenu)) @@ -23,8 +21,9 @@ func (*RealSystray) ResetMenu() { systray.ResetMenu() } -func (*RealSystray) AddMenuItem(title, tooltip string) *systray.MenuItem { - return systray.AddMenuItem(title, tooltip) +func (*RealSystray) AddMenuItem(title, tooltip string) MenuItem { + item := systray.AddMenuItem(title, tooltip) + return &RealMenuItem{MenuItem: item} } func (*RealSystray) AddSeparator() { @@ -53,24 +52,13 @@ func (m *MockSystray) ResetMenu() { m.menuItems = nil } -func (m *MockSystray) AddMenuItem(title, _ string) *systray.MenuItem { +func (m *MockSystray) AddMenuItem(title, tooltip string) MenuItem { m.menuItems = append(m.menuItems, title) - // Create a MenuItem with initialized internal maps using reflection - // This prevents nil map panics when methods are called - item := &systray.MenuItem{} - - // Use reflection to initialize internal maps if they exist - // This is a hack but necessary for testing - rv := reflect.ValueOf(item).Elem() - rt := rv.Type() - for i := 0; i < rt.NumField(); i++ { - field := rv.Field(i) - if field.Kind() == reflect.Map && field.IsNil() && field.CanSet() { - field.Set(reflect.MakeMap(field.Type())) - } + // Return a MockMenuItem that won't panic when methods are called + return &MockMenuItem{ + title: title, + tooltip: tooltip, } - - return item } func (m *MockSystray) AddSeparator() { diff --git a/cmd/goose/ui.go b/cmd/goose/ui.go index 54854c3..42ac5d0 100644 --- a/cmd/goose/ui.go +++ b/cmd/goose/ui.go @@ -217,7 +217,7 @@ func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string, // Add bullet point or emoji for blocked PRs if sortedPRs[prIndex].NeedsReview || sortedPRs[prIndex].IsBlocked { // Get the blocked time from state manager - prState, hasState := app.stateManager.GetPRState(sortedPRs[prIndex].URL) + prState, hasState := app.stateManager.PRState(sortedPRs[prIndex].URL) // Show emoji for PRs blocked within the last 5 minutes if hasState && !prState.FirstBlockedAt.IsZero() && time.Since(prState.FirstBlockedAt) < blockedPRIconDuration { @@ -369,7 +369,7 @@ func (app *App) generatePRSectionTitles(prs []PR, sectionTitle string, hiddenOrg // Add bullet point or emoji for blocked PRs (same logic as in addPRSection) if sortedPRs[prIndex].NeedsReview || sortedPRs[prIndex].IsBlocked { - prState, hasState := app.stateManager.GetPRState(sortedPRs[prIndex].URL) + prState, hasState := app.stateManager.PRState(sortedPRs[prIndex].URL) if hasState && !prState.FirstBlockedAt.IsZero() && time.Since(prState.FirstBlockedAt) < blockedPRIconDuration { if sectionTitle == "Outgoing" { From f27a57cd55fbf7ca5c5e356cf369d1e08b8b5def Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Wed, 27 Aug 2025 15:20:29 -0400 Subject: [PATCH 10/13] add interfaces and tests --- cmd/goose/deadlock_test.go | 187 +++++++++++++++++++++++++++++++ cmd/goose/menu_item_interface.go | 123 ++++++++++++++++++++ 2 files changed, 310 insertions(+) create mode 100644 cmd/goose/deadlock_test.go create mode 100644 cmd/goose/menu_item_interface.go diff --git a/cmd/goose/deadlock_test.go b/cmd/goose/deadlock_test.go new file mode 100644 index 0000000..722b70d --- /dev/null +++ b/cmd/goose/deadlock_test.go @@ -0,0 +1,187 @@ +package main + +import ( + "sync" + "testing" + "time" +) + +// TestConcurrentMenuOperations tests that concurrent menu operations don't cause deadlocks +func TestConcurrentMenuOperations(t *testing.T) { + app := &App{ + mu: sync.RWMutex{}, + stateManager: NewPRStateManager(time.Now()), + hiddenOrgs: make(map[string]bool), + seenOrgs: make(map[string]bool), + blockedPRTimes: make(map[string]time.Time), + browserRateLimiter: NewBrowserRateLimiter(30*time.Second, 5, defaultMaxBrowserOpensDay), + systrayInterface: &MockSystray{}, + incoming: []PR{ + {Repository: "org1/repo1", Number: 1, Title: "Fix bug", URL: "https://github.com/org1/repo1/pull/1"}, + }, + outgoing: []PR{ + {Repository: "org2/repo2", Number: 2, Title: "Add feature", URL: "https://github.com/org2/repo2/pull/2"}, + }, + } + + // Use a WaitGroup to coordinate goroutines + var wg sync.WaitGroup + + // Use a channel to detect if we've deadlocked + done := make(chan bool, 1) + + // Number of concurrent operations to test + concurrentOps := 10 + + wg.Add(concurrentOps * 3) // 3 types of operations + + // Start a goroutine that will signal completion + go func() { + wg.Wait() + done <- true + }() + + // Simulate concurrent menu clicks (write lock operations) + for i := 0; i < concurrentOps; i++ { + go func() { + defer wg.Done() + + // This simulates the click handler storing menu titles + menuTitles := app.generateMenuTitles() + app.mu.Lock() + app.lastMenuTitles = menuTitles + app.mu.Unlock() + }() + } + + // Simulate concurrent menu generation (read lock operations) + for i := 0; i < concurrentOps; i++ { + go func() { + defer wg.Done() + + // This simulates generating menu titles + _ = app.generateMenuTitles() + }() + } + + // Simulate concurrent PR updates (write lock operations) + for i := 0; i < concurrentOps; i++ { + go func(iteration int) { + defer wg.Done() + + app.mu.Lock() + // Simulate updating PR data + if iteration%2 == 0 { + app.incoming = append(app.incoming, PR{ + Repository: "test/repo", + Number: iteration, + Title: "Test PR", + URL: "https://github.com/test/repo/pull/1", + }) + } + app.mu.Unlock() + }(i) + } + + // Wait for operations to complete or timeout + select { + case <-done: + // Success - all operations completed without deadlock + t.Log("All concurrent operations completed successfully") + case <-time.After(5 * time.Second): + t.Fatal("Deadlock detected: operations did not complete within 5 seconds") + } +} + +// TestMenuClickDeadlockScenario specifically tests the deadlock scenario that was fixed +func TestMenuClickDeadlockScenario(t *testing.T) { + app := &App{ + mu: sync.RWMutex{}, + stateManager: NewPRStateManager(time.Now()), + hiddenOrgs: make(map[string]bool), + seenOrgs: make(map[string]bool), + blockedPRTimes: make(map[string]time.Time), + browserRateLimiter: NewBrowserRateLimiter(30*time.Second, 5, defaultMaxBrowserOpensDay), + systrayInterface: &MockSystray{}, + incoming: []PR{ + {Repository: "org1/repo1", Number: 1, Title: "Test PR", URL: "https://github.com/org1/repo1/pull/1"}, + }, + } + + // This exact sequence previously caused a deadlock: + // 1. Click handler acquires write lock + // 2. Click handler calls generateMenuTitles while holding lock + // 3. generateMenuTitles tries to acquire read lock + // 4. Deadlock! + + // The fix ensures we don't hold the lock when calling generateMenuTitles + done := make(chan bool, 1) + + go func() { + // Simulate the fixed click handler behavior + menuTitles := app.generateMenuTitles() // Called WITHOUT holding lock + app.mu.Lock() + app.lastMenuTitles = menuTitles + app.mu.Unlock() + done <- true + }() + + select { + case <-done: + t.Log("Click handler completed without deadlock") + case <-time.After(1 * time.Second): + t.Fatal("Click handler deadlocked") + } +} + +// TestRapidMenuClicks tests that rapid menu clicks don't cause issues +func TestRapidMenuClicks(t *testing.T) { + app := &App{ + mu: sync.RWMutex{}, + stateManager: NewPRStateManager(time.Now()), + hiddenOrgs: make(map[string]bool), + seenOrgs: make(map[string]bool), + blockedPRTimes: make(map[string]time.Time), + browserRateLimiter: NewBrowserRateLimiter(30*time.Second, 5, defaultMaxBrowserOpensDay), + systrayInterface: &MockSystray{}, + lastSearchAttempt: time.Now().Add(-15 * time.Second), // Allow first click + incoming: []PR{ + {Repository: "org1/repo1", Number: 1, Title: "Test", URL: "https://github.com/org1/repo1/pull/1"}, + }, + } + + // Simulate 20 rapid clicks + clickCount := 20 + successfulClicks := 0 + + for i := 0; i < clickCount; i++ { + // Check if enough time has passed for rate limiting + app.mu.RLock() + timeSince := time.Since(app.lastSearchAttempt) + app.mu.RUnlock() + + if timeSince >= minUpdateInterval { + // This click would trigger a refresh + app.mu.Lock() + app.lastSearchAttempt = time.Now() + app.mu.Unlock() + successfulClicks++ + + // Also update menu titles as the real handler would + menuTitles := app.generateMenuTitles() + app.mu.Lock() + app.lastMenuTitles = menuTitles + app.mu.Unlock() + } + + // Small delay between clicks to simulate human clicking + time.Sleep(10 * time.Millisecond) + } + + // Due to rate limiting, we should only have 1-2 successful clicks + if successfulClicks > 3 { + t.Errorf("Rate limiting not working: %d clicks succeeded out of %d rapid clicks", successfulClicks, clickCount) + } + + t.Logf("Rate limiting working correctly: %d clicks succeeded out of %d rapid clicks", successfulClicks, clickCount) +} diff --git a/cmd/goose/menu_item_interface.go b/cmd/goose/menu_item_interface.go new file mode 100644 index 0000000..2a3e4af --- /dev/null +++ b/cmd/goose/menu_item_interface.go @@ -0,0 +1,123 @@ +package main + +import "github.com/energye/systray" + +// MenuItem is an interface for menu items that can be implemented by both +// real systray menu items and mock menu items for testing. +type MenuItem interface { + Disable() + Enable() + Check() + Uncheck() + SetTitle(string) + SetTooltip(string) + Click(func()) + AddSubMenuItem(title, tooltip string) MenuItem +} + +// RealMenuItem wraps a real systray.MenuItem to implement our MenuItem interface. +type RealMenuItem struct { + *systray.MenuItem +} + +// Ensure RealMenuItem implements MenuItem interface. +var _ MenuItem = (*RealMenuItem)(nil) + +// Disable disables the menu item. +func (r *RealMenuItem) Disable() { + r.MenuItem.Disable() +} + +// Enable enables the menu item. +func (r *RealMenuItem) Enable() { + r.MenuItem.Enable() +} + +// Check checks the menu item. +func (r *RealMenuItem) Check() { + r.MenuItem.Check() +} + +// Uncheck unchecks the menu item. +func (r *RealMenuItem) Uncheck() { + r.MenuItem.Uncheck() +} + +// SetTitle sets the menu item title. +func (r *RealMenuItem) SetTitle(title string) { + r.MenuItem.SetTitle(title) +} + +// SetTooltip sets the menu item tooltip. +func (r *RealMenuItem) SetTooltip(tooltip string) { + r.MenuItem.SetTooltip(tooltip) +} + +// Click sets the click handler. +func (r *RealMenuItem) Click(handler func()) { + r.MenuItem.Click(handler) +} + +// AddSubMenuItem adds a sub menu item and returns it wrapped in our interface. +func (r *RealMenuItem) AddSubMenuItem(title, tooltip string) MenuItem { + subItem := r.MenuItem.AddSubMenuItem(title, tooltip) + return &RealMenuItem{MenuItem: subItem} +} + +// MockMenuItem implements MenuItem for testing without calling systray functions. +type MockMenuItem struct { + title string + tooltip string + disabled bool + checked bool + clickHandler func() + subItems []MenuItem +} + +// Ensure MockMenuItem implements MenuItem interface. +var _ MenuItem = (*MockMenuItem)(nil) + +// Disable marks the item as disabled. +func (m *MockMenuItem) Disable() { + m.disabled = true +} + +// Enable marks the item as enabled. +func (m *MockMenuItem) Enable() { + m.disabled = false +} + +// Check marks the item as checked. +func (m *MockMenuItem) Check() { + m.checked = true +} + +// Uncheck marks the item as unchecked. +func (m *MockMenuItem) Uncheck() { + m.checked = false +} + +// SetTitle sets the title. +func (m *MockMenuItem) SetTitle(title string) { + m.title = title +} + +// SetTooltip sets the tooltip. +func (m *MockMenuItem) SetTooltip(tooltip string) { + m.tooltip = tooltip +} + +// Click sets the click handler. +func (m *MockMenuItem) Click(handler func()) { + m.clickHandler = handler +} + +// AddSubMenuItem adds a sub menu item (mock). +func (m *MockMenuItem) AddSubMenuItem(title, tooltip string) MenuItem { + subItem := &MockMenuItem{ + title: title, + tooltip: tooltip, + } + m.subItems = append(m.subItems, subItem) + return subItem +} From 80a0d1799b41ede4e104263e8fdc4f76b795c898 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Wed, 27 Aug 2025 15:30:06 -0400 Subject: [PATCH 11/13] fix TestIsStale on Windows --- cmd/goose/main_test.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/cmd/goose/main_test.go b/cmd/goose/main_test.go index e2ddd6a..0df6997 100644 --- a/cmd/goose/main_test.go +++ b/cmd/goose/main_test.go @@ -18,6 +18,8 @@ func TestMain(m *testing.M) { } func TestIsStale(t *testing.T) { + // Capture time.Now() once to avoid race conditions + now := time.Now() tests := []struct { time time.Time name string @@ -25,17 +27,17 @@ func TestIsStale(t *testing.T) { }{ { name: "recent PR", - time: time.Now().Add(-24 * time.Hour), + time: now.Add(-24 * time.Hour), expected: false, }, { name: "stale PR", - time: time.Now().Add(-91 * 24 * time.Hour), + time: now.Add(-91 * 24 * time.Hour), expected: true, }, { name: "exactly at threshold", - time: time.Now().Add(-90 * 24 * time.Hour), + time: now.Add(-90 * 24 * time.Hour), expected: true, // >= 90 days is stale }, } @@ -43,7 +45,11 @@ func TestIsStale(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // isStale was inlined - test the logic directly - if got := tt.time.Before(time.Now().Add(-stalePRThreshold)); got != tt.expected { + // Use the same 'now' for consistency + threshold := now.Add(-stalePRThreshold) + got := tt.time.Before(threshold) + if got != tt.expected { + t.Logf("Test time: %v, Threshold: %v, Before: %v", tt.time, threshold, got) t.Errorf("stale check = %v, want %v", got, tt.expected) } }) From b5b1c472702f79c8b143f6267171bf4002f1732c Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Wed, 27 Aug 2025 15:48:24 -0400 Subject: [PATCH 12/13] more TestIsStale fixing, run tests w/ -race --- Makefile | 5 ++++- cmd/goose/main_test.go | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 4de6e40..7a6bc52 100644 --- a/Makefile +++ b/Makefile @@ -10,7 +10,10 @@ GIT_COMMIT := $(shell git rev-parse --short HEAD 2>/dev/null || echo "unknown") BUILD_DATE := $(shell date -u +"%Y-%m-%dT%H:%M:%SZ") LDFLAGS := -X main.version=$(GIT_VERSION) -X main.commit=$(GIT_COMMIT) -X main.date=$(BUILD_DATE) -.PHONY: all build clean deps run app-bundle install install-darwin install-unix install-windows +.PHONY: all build clean deps run app-bundle install install-darwin install-unix install-windows test + +test: + go test -race ./... # Default target all: build diff --git a/cmd/goose/main_test.go b/cmd/goose/main_test.go index 0df6997..ded3cee 100644 --- a/cmd/goose/main_test.go +++ b/cmd/goose/main_test.go @@ -47,7 +47,7 @@ func TestIsStale(t *testing.T) { // isStale was inlined - test the logic directly // Use the same 'now' for consistency threshold := now.Add(-stalePRThreshold) - got := tt.time.Before(threshold) + got := !tt.time.After(threshold) // time <= threshold means stale (>= 90 days old) if got != tt.expected { t.Logf("Test time: %v, Threshold: %v, Before: %v", tt.time, threshold, got) t.Errorf("stale check = %v, want %v", got, tt.expected) From 27204b593d7268c17f769eae47fa980c10a15b66 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Wed, 27 Aug 2025 15:52:33 -0400 Subject: [PATCH 13/13] run make fix --- cmd/goose/deadlock_test.go | 8 ++++---- cmd/goose/main.go | 35 +++++++++++++++----------------- cmd/goose/menu_item_interface.go | 4 ++-- cmd/goose/pr_state.go | 10 ++++----- cmd/goose/settings.go | 2 +- 5 files changed, 27 insertions(+), 32 deletions(-) diff --git a/cmd/goose/deadlock_test.go b/cmd/goose/deadlock_test.go index 722b70d..58f29ee 100644 --- a/cmd/goose/deadlock_test.go +++ b/cmd/goose/deadlock_test.go @@ -42,7 +42,7 @@ func TestConcurrentMenuOperations(t *testing.T) { }() // Simulate concurrent menu clicks (write lock operations) - for i := 0; i < concurrentOps; i++ { + for range concurrentOps { go func() { defer wg.Done() @@ -55,7 +55,7 @@ func TestConcurrentMenuOperations(t *testing.T) { } // Simulate concurrent menu generation (read lock operations) - for i := 0; i < concurrentOps; i++ { + for range concurrentOps { go func() { defer wg.Done() @@ -65,7 +65,7 @@ func TestConcurrentMenuOperations(t *testing.T) { } // Simulate concurrent PR updates (write lock operations) - for i := 0; i < concurrentOps; i++ { + for i := range concurrentOps { go func(iteration int) { defer wg.Done() @@ -154,7 +154,7 @@ func TestRapidMenuClicks(t *testing.T) { clickCount := 20 successfulClicks := 0 - for i := 0; i < clickCount; i++ { + for range clickCount { // Check if enough time has passed for rate limiting app.mu.RLock() timeSince := time.Since(app.lastSearchAttempt) diff --git a/cmd/goose/main.go b/cmd/goose/main.go index 4cb0deb..6072306 100644 --- a/cmd/goose/main.go +++ b/cmd/goose/main.go @@ -64,37 +64,34 @@ type PR struct { // App holds the application state. type App struct { + lastSearchAttempt time.Time lastSuccessfulFetch time.Time - lastSearchAttempt time.Time // For rate limiting forced refreshes - lastMenuTitles []string // For change detection to prevent unnecessary redraws startTime time.Time + systrayInterface SystrayInterface + browserRateLimiter *BrowserRateLimiter + blockedPRTimes map[string]time.Time + currentUser *github.User + stateManager *PRStateManager client *github.Client + hiddenOrgs map[string]bool + seenOrgs map[string]bool turnClient *turn.Client - currentUser *github.User - stateManager *PRStateManager // NEW: Centralized state management - browserRateLimiter *BrowserRateLimiter - targetUser string - cacheDir string + previousBlockedPRs map[string]bool authError string - incoming []PR + cacheDir string + targetUser string + lastMenuTitles []string outgoing []PR + incoming []PR updateInterval time.Duration consecutiveFailures int mu sync.RWMutex - menuInitialized bool - initialLoadComplete bool - enableAudioCues bool enableAutoBrowser bool hideStaleIncoming bool noCache bool - systrayInterface SystrayInterface // For mocking systray operations in tests - hiddenOrgs map[string]bool - seenOrgs map[string]bool - - // Deprecated: These fields are kept for backward compatibility with tests - // The actual state is managed by stateManager - previousBlockedPRs map[string]bool // Deprecated: use stateManager - blockedPRTimes map[string]time.Time // Deprecated: use stateManager + enableAudioCues bool + initialLoadComplete bool + menuInitialized bool } func loadCurrentUser(ctx context.Context, app *App) { diff --git a/cmd/goose/menu_item_interface.go b/cmd/goose/menu_item_interface.go index 2a3e4af..d94cf05 100644 --- a/cmd/goose/menu_item_interface.go +++ b/cmd/goose/menu_item_interface.go @@ -66,12 +66,12 @@ func (r *RealMenuItem) AddSubMenuItem(title, tooltip string) MenuItem { // MockMenuItem implements MenuItem for testing without calling systray functions. type MockMenuItem struct { + clickHandler func() title string tooltip string + subItems []MenuItem disabled bool checked bool - clickHandler func() - subItems []MenuItem } // Ensure MockMenuItem implements MenuItem interface. diff --git a/cmd/goose/pr_state.go b/cmd/goose/pr_state.go index f8398fd..cc75dd3 100644 --- a/cmd/goose/pr_state.go +++ b/cmd/goose/pr_state.go @@ -9,20 +9,18 @@ import ( // PRState tracks the complete state of a PR including blocking history. type PRState struct { - PR PR FirstBlockedAt time.Time LastSeenBlocked time.Time - HasNotified bool // Have we already notified about this PR being blocked? + PR PR + HasNotified bool } // PRStateManager manages all PR states with proper synchronization. type PRStateManager struct { - mu sync.RWMutex - states map[string]*PRState // Key is PR URL - - // Configuration startTime time.Time + states map[string]*PRState gracePeriodSeconds int + mu sync.RWMutex } // NewPRStateManager creates a new PR state manager. diff --git a/cmd/goose/settings.go b/cmd/goose/settings.go index e21c336..8ba453c 100644 --- a/cmd/goose/settings.go +++ b/cmd/goose/settings.go @@ -10,10 +10,10 @@ import ( // Settings represents persistent user settings. type Settings struct { + HiddenOrgs map[string]bool `json:"hidden_orgs"` EnableAudioCues bool `json:"enable_audio_cues"` HideStale bool `json:"hide_stale"` EnableAutoBrowser bool `json:"enable_auto_browser"` - HiddenOrgs map[string]bool `json:"hidden_orgs"` } // settingsDir returns the configuration directory for settings.