Skip to content

Commit f8941db

Browse files
tstrombergtstromberg
committed
avoid panics
1 parent b41f870 commit f8941db

File tree

8 files changed

+148
-99
lines changed

8 files changed

+148
-99
lines changed

‎cmd/goose/click_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ func TestMenuClickRateLimit(t *testing.T) {
1818
hiddenOrgs: make(map[string]bool),
1919
seenOrgs: make(map[string]bool),
2020
lastSearchAttempt: time.Now().Add(-15 * time.Second), // 15 seconds ago
21-
systrayInterface: &MockSystray{}, // Use mock systray to avoid panics
21+
systrayInterface: &MockSystray{}, // Use mock systray to avoid panics
2222
}
2323

2424
// Simulate the click handler logic (without the actual UI interaction)
@@ -70,7 +70,7 @@ func TestScheduledUpdateRateLimit(t *testing.T) {
7070
hiddenOrgs: make(map[string]bool),
7171
seenOrgs: make(map[string]bool),
7272
lastSearchAttempt: time.Now().Add(-5 * time.Second), // 5 seconds ago
73-
systrayInterface: &MockSystray{}, // Use mock systray to avoid panics
73+
systrayInterface: &MockSystray{}, // Use mock systray to avoid panics
7474
}
7575

7676
// Simulate the scheduled update logic
@@ -97,4 +97,4 @@ func TestScheduledUpdateRateLimit(t *testing.T) {
9797
if !shouldUpdate2 {
9898
t.Errorf("Scheduled update after 12 seconds should proceed, last search was %v ago", timeSince2)
9999
}
100-
}
100+
}

‎cmd/goose/filtering_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ func TestCountPRsWithHiddenOrgs(t *testing.T) {
2121
"org2": true, // Hide org2
2222
},
2323
hideStaleIncoming: false,
24-
systrayInterface: &MockSystray{}, // Use mock systray to avoid panics
24+
systrayInterface: &MockSystray{}, // Use mock systray to avoid panics
2525
}
2626

2727
counts := app.countPRs()
@@ -58,7 +58,7 @@ func TestCountPRsWithStalePRs(t *testing.T) {
5858
{Repository: "org1/repo5", IsBlocked: true, UpdatedAt: recentTime},
5959
},
6060
hiddenOrgs: map[string]bool{},
61-
hideStaleIncoming: true, // Hide stale PRs
61+
hideStaleIncoming: true, // Hide stale PRs
6262
systrayInterface: &MockSystray{}, // Use mock systray to avoid panics
6363
}
6464

@@ -101,7 +101,7 @@ func TestCountPRsWithBothFilters(t *testing.T) {
101101
"org2": true,
102102
},
103103
hideStaleIncoming: true,
104-
systrayInterface: &MockSystray{}, // Use mock systray to avoid panics
104+
systrayInterface: &MockSystray{}, // Use mock systray to avoid panics
105105
}
106106

107107
counts := app.countPRs()

‎cmd/goose/github.go‎

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -138,16 +138,39 @@ func (*App) token(ctx context.Context) (string, error) {
138138
}
139139

140140
log.Printf("Executing command: %s auth token", ghPath)
141-
cmd := exec.CommandContext(ctx, ghPath, "auth", "token")
142-
output, err := cmd.CombinedOutput()
143-
if err != nil {
144-
log.Printf("gh command failed: %v", err)
145-
return "", fmt.Errorf("exec 'gh auth token': %w", err)
146-
}
147-
token := strings.TrimSpace(string(output))
148-
if err := validateGitHubToken(token); err != nil {
149-
return "", fmt.Errorf("invalid token from gh CLI: %w", err)
141+
142+
// Use retry logic for gh CLI command as it may fail temporarily
143+
var token string
144+
retryErr := retry.Do(func() error {
145+
// Create timeout context for gh CLI call
146+
cmdCtx, cancel := context.WithTimeout(ctx, 10*time.Second)
147+
defer cancel()
148+
149+
cmd := exec.CommandContext(cmdCtx, ghPath, "auth", "token")
150+
output, cmdErr := cmd.CombinedOutput()
151+
if cmdErr != nil {
152+
log.Printf("gh command failed (will retry): %v", cmdErr)
153+
return fmt.Errorf("exec 'gh auth token': %w", cmdErr)
154+
}
155+
156+
token = strings.TrimSpace(string(output))
157+
if validateErr := validateGitHubToken(token); validateErr != nil {
158+
// Don't retry on invalid token - it won't get better
159+
return retry.Unrecoverable(fmt.Errorf("invalid token from gh CLI: %w", validateErr))
160+
}
161+
return nil
162+
},
163+
retry.Attempts(3), // Fewer attempts for local command
164+
retry.Delay(time.Second),
165+
retry.OnRetry(func(n uint, err error) {
166+
log.Printf("[GH CLI] Retry %d/3: %v", n+1, err)
167+
}),
168+
retry.Context(ctx),
169+
)
170+
if retryErr != nil {
171+
return "", retryErr
150172
}
173+
151174
log.Println("Successfully obtained GitHub token from gh CLI")
152175
return token, nil
153176
}
@@ -227,7 +250,7 @@ func (app *App) fetchPRsInternal(ctx context.Context) (incoming []PR, outgoing [
227250
app.mu.Lock()
228251
app.lastSearchAttempt = time.Now()
229252
app.mu.Unlock()
230-
253+
231254
// Check if we have a client
232255
if app.client == nil {
233256
return nil, nil, fmt.Errorf("no GitHub client available: %s", app.authError)

‎cmd/goose/main.go‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -256,12 +256,12 @@ func (app *App) onReady(ctx context.Context) {
256256
// Set up click handlers first (needed for both success and error states)
257257
systray.SetOnClick(func(menu systray.IMenu) {
258258
log.Println("Icon clicked")
259-
259+
260260
// Check if we can perform a forced refresh (rate limited to every 10 seconds)
261261
app.mu.RLock()
262262
timeSinceLastSearch := time.Since(app.lastSearchAttempt)
263263
app.mu.RUnlock()
264-
264+
265265
if timeSinceLastSearch >= minUpdateInterval {
266266
log.Printf("[CLICK] Forcing search refresh (last search %v ago)", timeSinceLastSearch)
267267
go func() {
@@ -271,7 +271,7 @@ func (app *App) onReady(ctx context.Context) {
271271
remainingTime := minUpdateInterval - timeSinceLastSearch
272272
log.Printf("[CLICK] Rate limited - search performed %v ago, %v remaining", timeSinceLastSearch, remainingTime)
273273
}
274-
274+
275275
if menu != nil {
276276
if err := menu.ShowMenu(); err != nil {
277277
log.Printf("Failed to show menu: %v", err)
@@ -350,13 +350,13 @@ func (app *App) updateLoop(ctx context.Context) {
350350
app.mu.RLock()
351351
timeSinceLastSearch := time.Since(app.lastSearchAttempt)
352352
app.mu.RUnlock()
353-
353+
354354
if timeSinceLastSearch >= minUpdateInterval {
355355
log.Println("Running scheduled PR update")
356356
app.updatePRs(ctx)
357357
} else {
358358
remainingTime := minUpdateInterval - timeSinceLastSearch
359-
log.Printf("Skipping scheduled update - recent search %v ago, %v remaining until next allowed",
359+
log.Printf("Skipping scheduled update - recent search %v ago, %v remaining until next allowed",
360360
timeSinceLastSearch, remainingTime)
361361
}
362362
case <-ctx.Done():
@@ -480,29 +480,28 @@ func (app *App) updatePRs(ctx context.Context) {
480480
func (app *App) updateMenu(ctx context.Context) {
481481
// Generate current menu titles
482482
currentTitles := app.generateMenuTitles()
483-
483+
484484
// Compare with last titles to see if rebuild is needed
485485
app.mu.RLock()
486486
lastTitles := app.lastMenuTitles
487487
app.mu.RUnlock()
488-
488+
489489
// Check if titles have changed
490490
if slices.Equal(currentTitles, lastTitles) {
491491
log.Printf("[MENU] No changes detected, skipping rebuild (%d items unchanged)", len(currentTitles))
492492
return
493493
}
494-
494+
495495
// Titles have changed, rebuild menu
496496
log.Printf("[MENU] Changes detected, rebuilding menu (%d→%d items)", len(lastTitles), len(currentTitles))
497497
app.rebuildMenu(ctx)
498-
498+
499499
// Store new titles
500500
app.mu.Lock()
501501
app.lastMenuTitles = currentTitles
502502
app.mu.Unlock()
503503
}
504504

505-
506505
// updatePRsWithWait fetches PRs and waits for Turn data before building initial menu.
507506
func (app *App) updatePRsWithWait(ctx context.Context) {
508507
incoming, outgoing, err := app.fetchPRsInternal(ctx)
@@ -539,8 +538,10 @@ func (app *App) updatePRsWithWait(ctx context.Context) {
539538
app.rebuildMenu(ctx)
540539
app.menuInitialized = true
541540
// Store initial menu titles to prevent unnecessary rebuild on first update
541+
// generateMenuTitles acquires its own read lock, so we can't hold a lock here
542+
menuTitles := app.generateMenuTitles()
542543
app.mu.Lock()
543-
app.lastMenuTitles = app.generateMenuTitles()
544+
app.lastMenuTitles = menuTitles
544545
app.mu.Unlock()
545546
// Menu initialization complete
546547
}
@@ -585,8 +586,10 @@ func (app *App) updatePRsWithWait(ctx context.Context) {
585586
app.rebuildMenu(ctx)
586587
app.menuInitialized = true
587588
// Store initial menu titles to prevent unnecessary rebuild on first update
589+
// generateMenuTitles acquires its own read lock, so we can't hold a lock here
590+
menuTitles := app.generateMenuTitles()
588591
app.mu.Lock()
589-
app.lastMenuTitles = app.generateMenuTitles()
592+
app.lastMenuTitles = menuTitles
590593
app.mu.Unlock()
591594
// Menu initialization complete
592595
} else {

‎cmd/goose/main_test.go‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,9 @@ import (
1111

1212
func TestMain(m *testing.M) {
1313
// Set test mode to prevent actual sound playback during tests
14-
_ = os.Setenv("GOOSE_TEST_MODE", "1")
14+
if err := os.Setenv("GOOSE_TEST_MODE", "1"); err != nil {
15+
panic(err)
16+
}
1517
os.Exit(m.Run())
1618
}
1719

@@ -486,7 +488,7 @@ func TestGracePeriodPreventsNotifications(t *testing.T) {
486488
enableAudioCues: true,
487489
initialLoadComplete: true,
488490
menuInitialized: true,
489-
startTime: time.Now(), // Just started
491+
startTime: time.Now(), // Just started
490492
systrayInterface: &MockSystray{}, // Use mock systray to avoid panics
491493
}
492494

@@ -672,7 +674,7 @@ func TestNewlyBlockedPRAfterGracePeriod(t *testing.T) {
672674
initialLoadComplete: true, // Already past initial load
673675
menuInitialized: true,
674676
startTime: time.Now().Add(-35 * time.Second), // Started 35 seconds ago
675-
systrayInterface: &MockSystray{}, // Use mock systray to avoid panics
677+
systrayInterface: &MockSystray{}, // Use mock systray to avoid panics
676678
}
677679

678680
// Start with no blocked PRs

‎cmd/goose/menu_change_detection_test.go‎

Lines changed: 27 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,13 @@ import (
1212
func TestMenuChangeDetection(t *testing.T) {
1313
// Create app with test data
1414
app := &App{
15-
mu: sync.RWMutex{},
16-
stateManager: NewPRStateManager(time.Now()),
17-
hiddenOrgs: make(map[string]bool),
18-
seenOrgs: make(map[string]bool),
19-
blockedPRTimes: make(map[string]time.Time),
20-
browserRateLimiter: NewBrowserRateLimiter(30*time.Second, 5, defaultMaxBrowserOpensDay),
21-
systrayInterface: &MockSystray{},
15+
mu: sync.RWMutex{},
16+
stateManager: NewPRStateManager(time.Now()),
17+
hiddenOrgs: make(map[string]bool),
18+
seenOrgs: make(map[string]bool),
19+
blockedPRTimes: make(map[string]time.Time),
20+
browserRateLimiter: NewBrowserRateLimiter(30*time.Second, 5, defaultMaxBrowserOpensDay),
21+
systrayInterface: &MockSystray{},
2222
incoming: []PR{
2323
{Repository: "org1/repo1", Number: 1, Title: "Fix bug", URL: "https://github.com/org1/repo1/pull/1", NeedsReview: true, UpdatedAt: time.Now()},
2424
{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) {
105105
func TestFirstRunMenuRebuildBug(t *testing.T) {
106106
// Create app simulating initial state
107107
app := &App{
108-
mu: sync.RWMutex{},
109-
stateManager: NewPRStateManager(time.Now()),
110-
hiddenOrgs: make(map[string]bool),
111-
seenOrgs: make(map[string]bool),
112-
blockedPRTimes: make(map[string]time.Time),
113-
browserRateLimiter: NewBrowserRateLimiter(30*time.Second, 5, defaultMaxBrowserOpensDay),
114-
menuInitialized: false,
115-
systrayInterface: &MockSystray{},
116-
lastMenuTitles: nil, // This is nil on first run - the bug condition
108+
mu: sync.RWMutex{},
109+
stateManager: NewPRStateManager(time.Now()),
110+
hiddenOrgs: make(map[string]bool),
111+
seenOrgs: make(map[string]bool),
112+
blockedPRTimes: make(map[string]time.Time),
113+
browserRateLimiter: NewBrowserRateLimiter(30*time.Second, 5, defaultMaxBrowserOpensDay),
114+
menuInitialized: false,
115+
systrayInterface: &MockSystray{},
116+
lastMenuTitles: nil, // This is nil on first run - the bug condition
117117
incoming: []PR{
118118
{Repository: "test/repo", Number: 1, Title: "Test PR", URL: "https://github.com/test/repo/pull/1"},
119119
},
@@ -148,7 +148,7 @@ func TestFirstRunMenuRebuildBug(t *testing.T) {
148148

149149
// Test 2: Current and stored titles should be equal (no changes)
150150
if !slices.Equal(currentTitles, storedTitles) {
151-
t.Errorf("BUG: Titles marked as different when they're the same:\nCurrent: %v\nStored: %v",
151+
t.Errorf("BUG: Titles marked as different when they're the same:\nCurrent: %v\nStored: %v",
152152
currentTitles, storedTitles)
153153
}
154154

@@ -164,13 +164,13 @@ func TestFirstRunMenuRebuildBug(t *testing.T) {
164164
// TestHiddenOrgChangesMenu tests that hiding/showing orgs updates menu titles
165165
func TestHiddenOrgChangesMenu(t *testing.T) {
166166
app := &App{
167-
mu: sync.RWMutex{},
168-
stateManager: NewPRStateManager(time.Now()),
169-
hiddenOrgs: make(map[string]bool),
170-
seenOrgs: make(map[string]bool),
171-
blockedPRTimes: make(map[string]time.Time),
172-
browserRateLimiter: NewBrowserRateLimiter(30*time.Second, 5, defaultMaxBrowserOpensDay),
173-
systrayInterface: &MockSystray{},
167+
mu: sync.RWMutex{},
168+
stateManager: NewPRStateManager(time.Now()),
169+
hiddenOrgs: make(map[string]bool),
170+
seenOrgs: make(map[string]bool),
171+
blockedPRTimes: make(map[string]time.Time),
172+
browserRateLimiter: NewBrowserRateLimiter(30*time.Second, 5, defaultMaxBrowserOpensDay),
173+
systrayInterface: &MockSystray{},
174174
incoming: []PR{
175175
{Repository: "org1/repo1", Number: 1, Title: "PR 1", URL: "https://github.com/org1/repo1/pull/1"},
176176
{Repository: "org2/repo2", Number: 2, Title: "PR 2", URL: "https://github.com/org2/repo2/pull/2"},
@@ -186,15 +186,15 @@ func TestHiddenOrgChangesMenu(t *testing.T) {
186186

187187
// Generate new titles - should have fewer items
188188
newTitles := app.generateMenuTitles()
189-
189+
190190
// Titles should be different
191191
if slices.Equal(initialTitles, newTitles) {
192192
t.Error("Hiding an org didn't change menu titles")
193193
}
194194

195195
// Should have fewer items (org1/repo1 should be hidden)
196196
if len(newTitles) >= initialCount {
197-
t.Errorf("Menu should have fewer items after hiding org: got %d, started with %d",
197+
t.Errorf("Menu should have fewer items after hiding org: got %d, started with %d",
198198
len(newTitles), initialCount)
199199
}
200-
}
200+
}

0 commit comments

Comments
 (0)