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/click_test.go b/cmd/goose/click_test.go new file mode 100644 index 0000000..d6bfd8c --- /dev/null +++ b/cmd/goose/click_test.go @@ -0,0 +1,100 @@ +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 + systrayInterface: &MockSystray{}, // Use mock systray to avoid panics + } + + // 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 + systrayInterface: &MockSystray{}, // Use mock systray to avoid panics + } + + // 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) + } +} diff --git a/cmd/goose/deadlock_test.go b/cmd/goose/deadlock_test.go new file mode 100644 index 0000000..58f29ee --- /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 range concurrentOps { + 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 range concurrentOps { + go func() { + defer wg.Done() + + // This simulates generating menu titles + _ = app.generateMenuTitles() + }() + } + + // Simulate concurrent PR updates (write lock operations) + for i := range concurrentOps { + 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 range clickCount { + // 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/filtering_test.go b/cmd/goose/filtering_test.go index c1066ba..df0b8b0 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() @@ -57,7 +58,8 @@ 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 } 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/github.go b/cmd/goose/github.go index d16fb99..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 } @@ -223,6 +246,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..6072306 100644 --- a/cmd/goose/main.go +++ b/cmd/goose/main.go @@ -11,6 +11,7 @@ import ( "log" "os" "path/filepath" + "slices" "strings" "sync" "time" @@ -63,34 +64,34 @@ type PR struct { // App holds the application state. type App struct { + lastSearchAttempt time.Time lastSuccessfulFetch time.Time 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 - 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) { @@ -212,6 +213,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 @@ -251,6 +253,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 +343,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,11 +473,30 @@ 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. @@ -486,6 +534,12 @@ 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 + // generateMenuTitles acquires its own read lock, so we can't hold a lock here + menuTitles := app.generateMenuTitles() + app.mu.Lock() + app.lastMenuTitles = menuTitles + app.mu.Unlock() // Menu initialization complete } return @@ -528,6 +582,12 @@ 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 + // generateMenuTitles acquires its own read lock, so we can't hold a lock here + menuTitles := app.generateMenuTitles() + app.mu.Lock() + app.lastMenuTitles = menuTitles + app.mu.Unlock() // Menu initialization complete } else { app.updateMenu(ctx) diff --git a/cmd/goose/main_test.go b/cmd/goose/main_test.go index e2a4037..ded3cee 100644 --- a/cmd/goose/main_test.go +++ b/cmd/goose/main_test.go @@ -11,11 +11,15 @@ 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()) } func TestIsStale(t *testing.T) { + // Capture time.Now() once to avoid race conditions + now := time.Now() tests := []struct { time time.Time name string @@ -23,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 }, } @@ -41,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.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) } }) @@ -63,6 +71,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 +183,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 { @@ -299,6 +309,7 @@ func TestSoundPlaybackDuringTransitions(t *testing.T) { enableAudioCues: true, initialLoadComplete: true, // Set to true to allow sound playback menuInitialized: true, + systrayInterface: &MockSystray{}, // Use mock systray to avoid Windows-specific panics } tests := []struct { @@ -443,6 +454,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 @@ -482,7 +494,8 @@ 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 } // Track whether we're in grace period for verification @@ -607,6 +620,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 @@ -636,7 +650,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) } } @@ -666,6 +680,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 @@ -690,7 +705,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/menu_change_detection_test.go b/cmd/goose/menu_change_detection_test.go new file mode 100644 index 0000000..f87780b --- /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) + } +} diff --git a/cmd/goose/menu_item_interface.go b/cmd/goose/menu_item_interface.go new file mode 100644 index 0000000..d94cf05 --- /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 { + clickHandler func() + title string + tooltip string + subItems []MenuItem + disabled bool + checked bool +} + +// 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 +} 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..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. @@ -120,8 +118,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 +130,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/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. diff --git a/cmd/goose/systray_interface.go b/cmd/goose/systray_interface.go new file mode 100644 index 0000000..4b0af61 --- /dev/null +++ b/cmd/goose/systray_interface.go @@ -0,0 +1,78 @@ +package main + +import ( + "github.com/energye/systray" +) + +// SystrayInterface abstracts systray operations for testing. +type SystrayInterface interface { + ResetMenu() + AddMenuItem(title, tooltip string) MenuItem + AddSeparator() + SetTitle(title string) + SetOnClick(fn func(menu systray.IMenu)) + Quit() +} + +// RealSystray implements SystrayInterface using the actual systray library. +type RealSystray struct{} + +func (*RealSystray) ResetMenu() { + systray.ResetMenu() +} + +func (*RealSystray) AddMenuItem(title, tooltip string) MenuItem { + item := systray.AddMenuItem(title, tooltip) + return &RealMenuItem{MenuItem: item} +} + +func (*RealSystray) AddSeparator() { + systray.AddSeparator() +} + +func (*RealSystray) SetTitle(title string) { + systray.SetTitle(title) +} + +func (*RealSystray) SetOnClick(fn func(menu systray.IMenu)) { + systray.SetOnClick(fn) +} + +func (*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) MenuItem { + m.menuItems = append(m.menuItems, title) + // Return a MockMenuItem that won't panic when methods are called + return &MockMenuItem{ + title: title, + tooltip: tooltip, + } +} + +func (m *MockSystray) AddSeparator() { + m.menuItems = append(m.menuItems, "---") +} + +func (m *MockSystray) SetTitle(title string) { + m.title = title +} + +func (*MockSystray) SetOnClick(_ func(menu systray.IMenu)) { + // No-op for testing +} + +func (*MockSystray) Quit() { + // No-op for testing +} diff --git a/cmd/goose/ui.go b/cmd/goose/ui.go index 9bcbd56..42ac5d0 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 @@ -214,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 { @@ -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 @@ -278,50 +281,157 @@ 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", + 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 + } + + 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", + "Hide Stale Incoming PRs", + "Honks enabled", + "Auto-open in Browser", + "Hidden Organizations", + "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.PRState(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 // Clear all existing menu items - systray.ResetMenu() + app.systrayInterface.ResetMenu() // 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 @@ -332,14 +442,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() @@ -347,7 +457,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 @@ -358,7 +468,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 { @@ -379,11 +489,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() @@ -448,7 +558,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() } @@ -478,7 +588,7 @@ func (app *App) addStaticMenuItems(ctx context.Context) { // Audio cues // Add 'Audio cues' option - audioItem := systray.AddMenuItem("Audio cues", "Play sounds for notifications") + audioItem := app.systrayInterface.AddMenuItem("Honks enabled", "Play sounds for notifications") app.mu.RLock() if app.enableAudioCues { audioItem.Check() @@ -504,7 +614,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() @@ -534,9 +644,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() }) }