diff --git a/README.md b/README.md index 5b7acab..3143fa2 100644 --- a/README.md +++ b/README.md @@ -76,7 +76,7 @@ We don't yet persist fine-grained tokens to disk - PR's welcome! ## Pricing -- Review Goose is free forever for public repositories ❤️ +- Free forever for public repositories ❤️ - Private repo access will soon be a supporter-only feature to ensure the goose is fed. ($2.56/mo is our recommendation) ## Privacy diff --git a/cmd/goose/cache.go b/cmd/goose/cache.go index c8687b8..d502da9 100644 --- a/cmd/goose/cache.go +++ b/cmd/goose/cache.go @@ -76,10 +76,10 @@ func (app *App) turnData(ctx context.Context, url string, updatedAt time.Time) ( return nil }, retry.Attempts(maxRetries), - retry.DelayType(retry.BackOffDelay), + retry.DelayType(retry.CombineDelay(retry.BackOffDelay, retry.RandomDelay)), // Add jitter for better backoff distribution retry.MaxDelay(maxRetryDelay), retry.OnRetry(func(n uint, err error) { - log.Printf("Turn API retry %d/%d for %s: %v", n+1, maxRetries, url, err) + log.Printf("[TURN] API retry %d/%d for %s: %v", n+1, maxRetries, url, err) }), retry.Context(ctx), ) diff --git a/cmd/goose/github.go b/cmd/goose/github.go index 4caafb3..d16fb99 100644 --- a/cmd/goose/github.go +++ b/cmd/goose/github.go @@ -199,10 +199,10 @@ func (app *App) executeGitHubQuery(ctx context.Context, query string, opts *gith return nil }, retry.Attempts(maxRetries), - retry.DelayType(retry.BackOffDelay), + retry.DelayType(retry.CombineDelay(retry.BackOffDelay, retry.RandomDelay)), // Add jitter for better backoff distribution retry.MaxDelay(maxRetryDelay), retry.OnRetry(func(n uint, err error) { - log.Printf("GitHub Search.Issues retry %d/%d: %v", n+1, maxRetries, err) + log.Printf("[GITHUB] Search.Issues retry %d/%d: %v", n+1, maxRetries, err) }), retry.Context(ctx), ) @@ -221,10 +221,8 @@ type prResult struct { wasFromCache bool } -// fetchPRsInternal is the implementation for PR fetching. -// It returns GitHub data immediately and starts Turn API queries in the background (when waitForTurn=false), -// or waits for Turn data to complete (when waitForTurn=true). -func (app *App) fetchPRsInternal(ctx context.Context, waitForTurn bool) (incoming []PR, outgoing []PR, _ error) { +// fetchPRsInternal fetches PRs and Turn data synchronously for simplicity. +func (app *App) fetchPRsInternal(ctx context.Context) (incoming []PR, outgoing []PR, _ error) { // Check if we have a client if app.client == nil { return nil, nil, fmt.Errorf("no GitHub client available: %s", app.authError) @@ -362,71 +360,12 @@ func (app *App) fetchPRsInternal(ctx context.Context, waitForTurn bool) (incomin log.Printf("[GITHUB] Found %d incoming, %d outgoing PRs from GitHub", len(incoming), len(outgoing)) // Fetch Turn API data - if waitForTurn { - // Synchronous - wait for Turn data - // Fetch Turn API data synchronously before building menu - app.fetchTurnDataSync(ctx, allIssues, user, &incoming, &outgoing) - } else { - // Asynchronous - start in background - app.mu.Lock() - app.loadingTurnData = true - app.pendingTurnResults = make([]TurnResult, 0) // Reset buffer - app.mu.Unlock() - go app.fetchTurnDataAsync(ctx, allIssues, user) - } + // Always synchronous now for simplicity - Turn API calls are fast with caching + app.fetchTurnDataSync(ctx, allIssues, user, &incoming, &outgoing) return incoming, outgoing, nil } -// updatePRData updates PR data with Turn API results. -func (app *App) updatePRData(url string, needsReview bool, isOwner bool, actionReason string) (*PR, bool) { - app.mu.Lock() - defer app.mu.Unlock() - - if isOwner { - // Update outgoing PRs - for i := range app.outgoing { - if app.outgoing[i].URL != url { - continue - } - // Check if Turn data was already applied for this UpdatedAt - now := time.Now() - if app.outgoing[i].TurnDataAppliedAt.After(app.outgoing[i].UpdatedAt) { - // Turn data already applied for this PR version, no change - return &app.outgoing[i], false - } - changed := app.outgoing[i].NeedsReview != needsReview || - app.outgoing[i].IsBlocked != needsReview || - app.outgoing[i].ActionReason != actionReason - app.outgoing[i].NeedsReview = needsReview - app.outgoing[i].IsBlocked = needsReview - app.outgoing[i].ActionReason = actionReason - app.outgoing[i].TurnDataAppliedAt = now - return &app.outgoing[i], changed - } - } else { - // Update incoming PRs - for i := range app.incoming { - if app.incoming[i].URL != url { - continue - } - // Check if Turn data was already applied for this UpdatedAt - now := time.Now() - if app.incoming[i].TurnDataAppliedAt.After(app.incoming[i].UpdatedAt) { - // Turn data already applied for this PR version, no change - return &app.incoming[i], false - } - changed := app.incoming[i].NeedsReview != needsReview || - app.incoming[i].ActionReason != actionReason - app.incoming[i].NeedsReview = needsReview - app.incoming[i].ActionReason = actionReason - app.incoming[i].TurnDataAppliedAt = now - return &app.incoming[i], changed - } - } - return nil, false -} - // fetchTurnDataSync fetches Turn API data synchronously and updates PRs directly. func (app *App) fetchTurnDataSync(ctx context.Context, issues []*github.Issue, user string, incoming *[]PR, outgoing *[]PR) { turnStart := time.Now() @@ -523,143 +462,3 @@ func (app *App) fetchTurnDataSync(ctx context.Context, issues []*github.Issue, u log.Printf("[TURN] Turn API queries completed in %v (%d/%d succeeded)", time.Since(turnStart), turnSuccesses, turnSuccesses+turnFailures) } - -// fetchTurnDataAsync fetches Turn API data in the background and updates PRs as results arrive. -func (app *App) fetchTurnDataAsync(ctx context.Context, issues []*github.Issue, user string) { - turnStart := time.Now() - - // Create a channel for results - results := make(chan prResult, len(issues)) - - // Use a WaitGroup to track goroutines - var wg sync.WaitGroup - - // Create semaphore to limit concurrent Turn API calls - sem := make(chan struct{}, maxConcurrentTurnAPICalls) - - // Process PRs in parallel with concurrency limit - for _, issue := range issues { - if !issue.IsPullRequest() { - continue - } - - wg.Add(1) - go func(issue *github.Issue) { - defer wg.Done() - - // Acquire semaphore - sem <- struct{}{} - defer func() { <-sem }() - - url := issue.GetHTMLURL() - updatedAt := issue.GetUpdatedAt().Time - - // Call turnData - it now has proper exponential backoff with jitter - turnData, wasFromCache, err := app.turnData(ctx, url, updatedAt) - - results <- prResult{ - url: issue.GetHTMLURL(), - turnData: turnData, - err: err, - isOwner: issue.GetUser().GetLogin() == user, - wasFromCache: wasFromCache, - } - }(issue) - } - - // Close the results channel when all goroutines are done - go func() { - wg.Wait() - close(results) - }() - - // Collect results and update PRs incrementally - turnSuccesses := 0 - turnFailures := 0 - updatesApplied := 0 - - // Process results as they arrive and buffer them - - for result := range results { - if result.err == nil && result.turnData != nil && result.turnData.PRState.UnblockAction != nil { - turnSuccesses++ - - // Check if user needs to review and get action reason - needsReview := false - actionReason := "" - if action, exists := result.turnData.PRState.UnblockAction[user]; exists { - needsReview = true - actionReason = action.Reason - // Only log blocked PRs from fresh API calls - if !result.wasFromCache { - log.Printf("[TURN] UnblockAction for %s: Reason=%q, Kind=%q", result.url, action.Reason, action.Kind) - } - } - - // Buffer the Turn result instead of applying immediately - turnResult := TurnResult{ - URL: result.url, - NeedsReview: needsReview, - IsOwner: result.isOwner, - ActionReason: actionReason, - WasFromCache: result.wasFromCache, - } - - app.mu.Lock() - app.pendingTurnResults = append(app.pendingTurnResults, turnResult) - app.mu.Unlock() - - updatesApplied++ - // Only log fresh API calls (not cached) - if !result.wasFromCache { - log.Printf("[TURN] Fresh API data for %s (needsReview=%v)", result.url, needsReview) - } - } else if result.err != nil { - turnFailures++ - } - } - - log.Printf("[TURN] Turn API queries completed in %v (%d/%d succeeded, %d PRs updated)", - time.Since(turnStart), turnSuccesses, turnSuccesses+turnFailures, updatesApplied) - - // Apply all buffered Turn results at once - app.mu.Lock() - pendingResults := app.pendingTurnResults - app.pendingTurnResults = nil - app.loadingTurnData = false - app.mu.Unlock() - - // Check if any results came from fresh API calls (not cache) - var cacheHits, freshResults int - for _, result := range pendingResults { - if result.WasFromCache { - cacheHits++ - } else { - freshResults++ - } - } - - // Only log if we have fresh results - if freshResults > 0 { - log.Printf("[TURN] Applying %d buffered Turn results (%d from cache, %d fresh)", len(pendingResults), cacheHits, freshResults) - } - - // Track how many PRs actually changed - var actualChanges int - for _, result := range pendingResults { - _, changed := app.updatePRData(result.URL, result.NeedsReview, result.IsOwner, result.ActionReason) - if changed { - actualChanges++ - } - } - - // Only check for newly blocked PRs if there were actual changes - // checkForNewlyBlockedPRs will handle UI updates internally if needed - if actualChanges > 0 { - app.checkForNewlyBlockedPRs(ctx) - // UI updates are handled inside checkForNewlyBlockedPRs - } else { - // No changes, but still update tray title in case of initial load - app.setTrayTitle() - } -} diff --git a/cmd/goose/main.go b/cmd/goose/main.go index dbae8c4..261d0cf 100644 --- a/cmd/goose/main.go +++ b/cmd/goose/main.go @@ -37,13 +37,14 @@ const ( maxPRsToProcess = 200 minUpdateInterval = 10 * time.Second defaultUpdateInterval = 1 * time.Minute + blockedPRIconDuration = 1 * time.Minute maxRetryDelay = 2 * time.Minute maxRetries = 10 minorFailureThreshold = 3 majorFailureThreshold = 10 panicFailureIncrement = 10 turnAPITimeout = 10 * time.Second - maxConcurrentTurnAPICalls = 10 + maxConcurrentTurnAPICalls = 20 defaultMaxBrowserOpensDay = 20 ) @@ -61,15 +62,6 @@ type PR struct { NeedsReview bool } -// TurnResult represents a Turn API result to be applied later. -type TurnResult struct { - URL string - ActionReason string - NeedsReview bool - IsOwner bool - WasFromCache bool // Track if this result came from cache -} - // App holds the application state. type App struct { lastSuccessfulFetch time.Time @@ -83,14 +75,14 @@ type App struct { targetUser string cacheDir string authError string - pendingTurnResults []TurnResult - lastMenuTitles []string + // Removed pendingTurnResults - simplified to synchronous approach + // Removed lastMenuTitles and lastMenuRebuild - simplified to always rebuild incoming []PR outgoing []PR updateInterval time.Duration consecutiveFailures int mu sync.RWMutex - loadingTurnData bool + // Removed loadingTurnData - simplified to synchronous approach menuInitialized bool initialLoadComplete bool enableAudioCues bool @@ -120,10 +112,10 @@ func loadCurrentUser(ctx context.Context, app *App) { return nil }, retry.Attempts(maxRetries), - retry.DelayType(retry.BackOffDelay), + retry.DelayType(retry.CombineDelay(retry.BackOffDelay, retry.RandomDelay)), // Add jitter for better backoff distribution retry.MaxDelay(maxRetryDelay), retry.OnRetry(func(n uint, err error) { - log.Printf("GitHub Users.Get retry %d/%d: %v", n+1, maxRetries, err) + log.Printf("[GITHUB] Users.Get retry %d/%d: %v", n+1, maxRetries, err) }), retry.Context(ctx), ) @@ -216,7 +208,7 @@ func main() { targetUser: targetUser, noCache: noCache, updateInterval: updateInterval, - pendingTurnResults: make([]TurnResult, 0), + // Removed pendingTurnResults initialization - simplified to synchronous enableAudioCues: true, enableAutoBrowser: false, // Default to false for safety browserRateLimiter: NewBrowserRateLimiter(browserOpenDelay, maxBrowserOpensMinute, maxBrowserOpensDay), @@ -341,7 +333,7 @@ func (app *App) updateLoop(ctx context.Context) { } func (app *App) updatePRs(ctx context.Context) { - incoming, outgoing, err := app.fetchPRsInternal(ctx, false) + incoming, outgoing, err := app.fetchPRsInternal(ctx) if err != nil { log.Printf("Error fetching PRs: %v", err) app.mu.Lock() @@ -448,42 +440,16 @@ func (app *App) updatePRs(ctx context.Context) { app.updateMenu(ctx) } -// updateMenu rebuilds the menu only when content actually changes. +// updateMenu rebuilds the menu every time - simple and reliable. func (app *App) updateMenu(ctx context.Context) { - app.mu.RLock() - // Skip menu updates while Turn data is still loading - if app.loadingTurnData { - app.mu.RUnlock() - return - } - - // Build current menu titles for comparison - var currentTitles []string - for i := range app.incoming { - currentTitles = append(currentTitles, fmt.Sprintf("IN:%s #%d", app.incoming[i].Repository, app.incoming[i].Number)) - } - for i := range app.outgoing { - currentTitles = append(currentTitles, fmt.Sprintf("OUT:%s #%d", app.outgoing[i].Repository, app.outgoing[i].Number)) - } - - lastTitles := app.lastMenuTitles - app.mu.RUnlock() - - // Only rebuild if titles changed - if reflect.DeepEqual(lastTitles, currentTitles) { - return - } - - app.mu.Lock() - app.lastMenuTitles = currentTitles - app.mu.Unlock() - + // Always rebuild - it's just a small menu, performance is not an issue + log.Println("[MENU] Rebuilding menu") app.rebuildMenu(ctx) } // 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, true) + incoming, outgoing, err := app.fetchPRsInternal(ctx) if err != nil { log.Printf("Error fetching PRs: %v", err) app.mu.Lock() @@ -564,7 +530,12 @@ func (app *App) tryAutoOpenPR(ctx context.Context, pr PR, autoBrowserEnabled boo log.Printf("[BROWSER] Auto-opening newly blocked PR: %s #%d - %s", pr.Repository, pr.Number, pr.URL) // Use strict validation for auto-opened URLs - if err := openURLAutoStrict(ctx, pr.URL); err != nil { + // Validate against strict GitHub PR URL pattern for auto-opening + if err := validateGitHubPRURL(pr.URL); err != nil { + log.Printf("Auto-open strict validation failed for %s: %v", sanitizeForLog(pr.URL), err) + return + } + if err := openURL(ctx, pr.URL); err != nil { log.Printf("[BROWSER] Failed to auto-open PR %s: %v", pr.URL, err) } else { app.browserRateLimiter.RecordOpen(pr.URL) @@ -671,30 +642,44 @@ func (app *App) checkForNewlyBlockedPRs(ctx context.Context) { continue } - if incoming[i].NeedsReview { - currentBlocked[incoming[i].URL] = true - // Track when first blocked - if blockedTime, exists := blockedTimes[incoming[i].URL]; exists { - newBlockedTimes[incoming[i].URL] = blockedTime - incoming[i].FirstBlockedAt = blockedTime - } else if !previousBlocked[incoming[i].URL] { - // Newly blocked PR - newBlockedTimes[incoming[i].URL] = now - incoming[i].FirstBlockedAt = now - log.Printf("[BLOCKED] Setting FirstBlockedAt for incoming PR: %s #%d at %v", - incoming[i].Repository, incoming[i].Number, now) - - // Skip sound and auto-open for stale PRs when hideStaleIncoming is enabled - isStale := incoming[i].UpdatedAt.Before(staleThreshold) - if hideStaleIncoming && isStale { - log.Printf("[BLOCKED] New incoming PR blocked (stale, skipping): %s #%d - %s", - incoming[i].Repository, incoming[i].Number, incoming[i].Title) - } else { - log.Printf("[BLOCKED] New incoming PR blocked: %s #%d - %s", - incoming[i].Repository, incoming[i].Number, incoming[i].Title) - app.notifyWithSound(ctx, incoming[i], true, &playedHonk) - app.tryAutoOpenPR(ctx, incoming[i], autoBrowserEnabled, startTime) - } + if !incoming[i].NeedsReview { + continue + } + + pr := &incoming[i] + currentBlocked[pr.URL] = true + + if previousBlocked[pr.URL] { + // PR was blocked before and is still blocked - preserve timestamp + if blockedTime, exists := blockedTimes[pr.URL]; exists { + newBlockedTimes[pr.URL] = blockedTime + pr.FirstBlockedAt = blockedTime + log.Printf("[BLOCKED] Preserving FirstBlockedAt for still-blocked incoming PR: %s #%d (blocked since %v, %v ago)", + pr.Repository, pr.Number, blockedTime, time.Since(blockedTime)) + } else { + // Edge case: PR was marked as blocked but timestamp is missing + log.Printf("[BLOCKED] WARNING: Missing timestamp for previously blocked incoming PR: %s #%d - setting new timestamp", + pr.Repository, pr.Number) + newBlockedTimes[pr.URL] = now + pr.FirstBlockedAt = now + } + } else { + // PR is newly blocked (wasn't blocked in previous check) + newBlockedTimes[pr.URL] = now + pr.FirstBlockedAt = now + log.Printf("[BLOCKED] Setting FirstBlockedAt for incoming PR: %s #%d at %v", + pr.Repository, pr.Number, now) + + // Skip sound and auto-open for stale PRs when hideStaleIncoming is enabled + isStale := pr.UpdatedAt.Before(staleThreshold) + if hideStaleIncoming && isStale { + log.Printf("[BLOCKED] New incoming PR blocked (stale, skipping): %s #%d - %s", + pr.Repository, pr.Number, pr.Title) + } else { + log.Printf("[BLOCKED] New incoming PR blocked: %s #%d - %s", + pr.Repository, pr.Number, pr.Title) + app.notifyWithSound(ctx, *pr, true, &playedHonk) + app.tryAutoOpenPR(ctx, *pr, autoBrowserEnabled, startTime) } } } @@ -707,34 +692,48 @@ func (app *App) checkForNewlyBlockedPRs(ctx context.Context) { continue } - if outgoing[i].IsBlocked { - currentBlocked[outgoing[i].URL] = true - // Track when first blocked - if blockedTime, exists := blockedTimes[outgoing[i].URL]; exists { - newBlockedTimes[outgoing[i].URL] = blockedTime - outgoing[i].FirstBlockedAt = blockedTime - } else if !previousBlocked[outgoing[i].URL] { - // Newly blocked PR - newBlockedTimes[outgoing[i].URL] = now - outgoing[i].FirstBlockedAt = now - log.Printf("[BLOCKED] Setting FirstBlockedAt for outgoing PR: %s #%d at %v", - outgoing[i].Repository, outgoing[i].Number, now) - - // Skip sound and auto-open for stale PRs when hideStaleIncoming is enabled - isStale := outgoing[i].UpdatedAt.Before(staleThreshold) - if hideStaleIncoming && isStale { - log.Printf("[BLOCKED] New outgoing PR blocked (stale, skipping): %s #%d - %s", - outgoing[i].Repository, outgoing[i].Number, outgoing[i].Title) - } else { - // Add delay if we already played honk sound - if playedHonk && !playedJet { - time.Sleep(2 * time.Second) - } - log.Printf("[BLOCKED] New outgoing PR blocked: %s #%d - %s", - outgoing[i].Repository, outgoing[i].Number, outgoing[i].Title) - app.notifyWithSound(ctx, outgoing[i], false, &playedJet) - app.tryAutoOpenPR(ctx, outgoing[i], autoBrowserEnabled, startTime) + if !outgoing[i].IsBlocked { + continue + } + + pr := &outgoing[i] + currentBlocked[pr.URL] = true + + if previousBlocked[pr.URL] { + // PR was blocked before and is still blocked - preserve timestamp + if blockedTime, exists := blockedTimes[pr.URL]; exists { + newBlockedTimes[pr.URL] = blockedTime + pr.FirstBlockedAt = blockedTime + log.Printf("[BLOCKED] Preserving FirstBlockedAt for still-blocked outgoing PR: %s #%d (blocked since %v, %v ago)", + pr.Repository, pr.Number, blockedTime, time.Since(blockedTime)) + } else { + // Edge case: PR was marked as blocked but timestamp is missing + log.Printf("[BLOCKED] WARNING: Missing timestamp for previously blocked outgoing PR: %s #%d - setting new timestamp", + pr.Repository, pr.Number) + newBlockedTimes[pr.URL] = now + pr.FirstBlockedAt = now + } + } else { + // PR is newly blocked (wasn't blocked in previous check) + newBlockedTimes[pr.URL] = now + pr.FirstBlockedAt = now + log.Printf("[BLOCKED] Setting FirstBlockedAt for outgoing PR: %s #%d at %v", + pr.Repository, pr.Number, now) + + // Skip sound and auto-open for stale PRs when hideStaleIncoming is enabled + isStale := pr.UpdatedAt.Before(staleThreshold) + if hideStaleIncoming && isStale { + log.Printf("[BLOCKED] New outgoing PR blocked (stale, skipping): %s #%d - %s", + pr.Repository, pr.Number, pr.Title) + } else { + // Add delay if we already played honk sound + if playedHonk && !playedJet { + time.Sleep(2 * time.Second) } + log.Printf("[BLOCKED] New outgoing PR blocked: %s #%d - %s", + pr.Repository, pr.Number, pr.Title) + app.notifyWithSound(ctx, *pr, false, &playedJet) + app.tryAutoOpenPR(ctx, *pr, autoBrowserEnabled, startTime) } } } diff --git a/cmd/goose/ui.go b/cmd/goose/ui.go index a0cb117..cb69295 100644 --- a/cmd/goose/ui.go +++ b/cmd/goose/ui.go @@ -15,18 +15,6 @@ import ( "github.com/energye/systray" ) -// openURLAutoStrict safely opens a URL in the default browser with strict validation for auto-opening. -// This function is used for auto-opening PRs and enforces stricter URL patterns. -func openURLAutoStrict(ctx context.Context, rawURL string) error { - // Validate against strict GitHub PR URL pattern - if err := validateGitHubPRURL(rawURL); err != nil { - return fmt.Errorf("strict validation failed: %w", err) - } - - // Use the regular openURL after strict validation passes - return openURL(ctx, rawURL) -} - // openURL safely opens a URL in the default browser after validation. func openURL(ctx context.Context, rawURL string) error { // Parse and validate the URL @@ -172,29 +160,6 @@ func (app *App) setTrayTitle() { systray.SetTitle(title) } -// sortPRsBlockedFirst creates a sorted copy of PRs with blocked ones first. -// This maintains stable ordering within blocked and non-blocked groups. -func sortPRsBlockedFirst(prs []PR) []PR { - // Create a copy to avoid modifying the original slice - sorted := make([]PR, len(prs)) - copy(sorted, prs) - - // Stable sort: blocked PRs first, then by update time (newest first) - sort.SliceStable(sorted, func(i, j int) bool { - // First priority: blocked status - if sorted[i].NeedsReview != sorted[j].NeedsReview { - return sorted[i].NeedsReview // true (blocked) comes before false - } - if sorted[i].IsBlocked != sorted[j].IsBlocked { - return sorted[i].IsBlocked // true (blocked) comes before false - } - // Second priority: more recent PRs first - return sorted[i].UpdatedAt.After(sorted[j].UpdatedAt) - }) - - return sorted -} - // addPRSection adds a section of PRs to the menu. func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string, blockedCount int) { if len(prs) == 0 { @@ -207,8 +172,20 @@ func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string, header := systray.AddMenuItem(headerText, "") header.Disable() - // Sort PRs with blocked ones first - sortedPRs := sortPRsBlockedFirst(prs) + // Sort PRs with blocked ones first - inline for simplicity + sortedPRs := make([]PR, len(prs)) + copy(sortedPRs, prs) + sort.SliceStable(sortedPRs, func(i, j int) bool { + // First priority: blocked status + if sortedPRs[i].NeedsReview != sortedPRs[j].NeedsReview { + return sortedPRs[i].NeedsReview // true (blocked) comes before false + } + if sortedPRs[i].IsBlocked != sortedPRs[j].IsBlocked { + return sortedPRs[i].IsBlocked // true (blocked) comes before false + } + // Second priority: more recent PRs first + return sortedPRs[i].UpdatedAt.After(sortedPRs[j].UpdatedAt) + }) // Get hidden orgs with proper locking app.mu.RLock() @@ -220,40 +197,40 @@ func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string, app.mu.RUnlock() // Add PR items in sorted order - for i := range sortedPRs { + for prIndex := range sortedPRs { // Apply filters // Skip PRs from hidden orgs - org := extractOrgFromRepo(sortedPRs[i].Repository) + org := extractOrgFromRepo(sortedPRs[prIndex].Repository) if org != "" && hiddenOrgs[org] { continue } // Skip stale PRs if configured - if hideStale && sortedPRs[i].UpdatedAt.Before(time.Now().Add(-stalePRThreshold)) { + if hideStale && sortedPRs[prIndex].UpdatedAt.Before(time.Now().Add(-stalePRThreshold)) { continue } - title := fmt.Sprintf("%s #%d", sortedPRs[i].Repository, sortedPRs[i].Number) + title := fmt.Sprintf("%s #%d", sortedPRs[prIndex].Repository, sortedPRs[prIndex].Number) // Add bullet point or emoji for blocked PRs - if sortedPRs[i].NeedsReview || sortedPRs[i].IsBlocked { - // Show emoji for PRs blocked within the last hour - if !sortedPRs[i].FirstBlockedAt.IsZero() && time.Since(sortedPRs[i].FirstBlockedAt) < time.Hour { + if sortedPRs[prIndex].NeedsReview || sortedPRs[prIndex].IsBlocked { + // Show emoji for PRs blocked within the last 25 minutes + if !sortedPRs[prIndex].FirstBlockedAt.IsZero() && time.Since(sortedPRs[prIndex].FirstBlockedAt) < blockedPRIconDuration { // Use party popper for outgoing PRs, goose for incoming PRs if sectionTitle == "Outgoing" { title = fmt.Sprintf("🎉 %s", title) log.Printf("[MENU] Adding party popper to outgoing PR: %s (blocked %v ago)", - sortedPRs[i].URL, time.Since(sortedPRs[i].FirstBlockedAt)) + sortedPRs[prIndex].URL, time.Since(sortedPRs[prIndex].FirstBlockedAt)) } else { title = fmt.Sprintf("🪿 %s", title) log.Printf("[MENU] Adding goose to incoming PR: %s (blocked %v ago)", - sortedPRs[i].URL, time.Since(sortedPRs[i].FirstBlockedAt)) + sortedPRs[prIndex].URL, time.Since(sortedPRs[prIndex].FirstBlockedAt)) } } else { title = fmt.Sprintf("• %s", title) } } // Format age inline for tooltip - duration := time.Since(sortedPRs[i].UpdatedAt) + duration := time.Since(sortedPRs[prIndex].UpdatedAt) var age string switch { case duration < time.Hour: @@ -265,19 +242,19 @@ func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string, case duration < 365*24*time.Hour: age = fmt.Sprintf("%dmo", int(duration.Hours()/(24*30))) default: - age = sortedPRs[i].UpdatedAt.Format("2006") + age = sortedPRs[prIndex].UpdatedAt.Format("2006") } - tooltip := fmt.Sprintf("%s (%s)", sortedPRs[i].Title, age) + tooltip := fmt.Sprintf("%s (%s)", sortedPRs[prIndex].Title, age) // Add action reason for blocked PRs - if (sortedPRs[i].NeedsReview || sortedPRs[i].IsBlocked) && sortedPRs[i].ActionReason != "" { - tooltip = fmt.Sprintf("%s - %s", tooltip, sortedPRs[i].ActionReason) + if (sortedPRs[prIndex].NeedsReview || sortedPRs[prIndex].IsBlocked) && sortedPRs[prIndex].ActionReason != "" { + tooltip = fmt.Sprintf("%s - %s", tooltip, sortedPRs[prIndex].ActionReason) } // Create PR menu item item := systray.AddMenuItem(title, tooltip) // Capture URL to avoid loop variable capture bug - prURL := sortedPRs[i].URL + prURL := sortedPRs[prIndex].URL item.Click(func() { if err := openURL(ctx, prURL); err != nil { log.Printf("failed to open url: %v", err) @@ -442,8 +419,7 @@ func (app *App) addStaticMenuItems(ctx context.Context) { orgItem.Check() log.Printf("[SETTINGS] Hiding org: %s", orgName) } - // Clear menu titles to force rebuild - app.lastMenuTitles = nil + // Menu always rebuilds now - no need to clear titles app.mu.Unlock() // Save settings @@ -465,8 +441,7 @@ func (app *App) addStaticMenuItems(ctx context.Context) { app.mu.Lock() app.hideStaleIncoming = !app.hideStaleIncoming hideStale := app.hideStaleIncoming - // Clear menu titles to force rebuild - app.lastMenuTitles = nil + // Menu always rebuilds now - no need to clear titles app.mu.Unlock() if hideStale {