Merge pull request #49 from codeGROOVE-dev/auto-refresh

Auto-refresh on menu click

Author: tstromberg

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

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)
+	}
+}

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)
+}

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()

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)