From 1150dd06108d813ce7d3adbc027701acc0a97a67 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Tue, 5 Aug 2025 15:54:48 -0400 Subject: [PATCH 1/3] lint cleanup --- cache.go | 29 ++++++++++--- github.go | 125 ++++++++++++++++++++++++------------------------------ main.go | 27 +++++++++++- sound.go | 8 +++- ui.go | 25 +++++------ 5 files changed, 121 insertions(+), 93 deletions(-) diff --git a/cache.go b/cache.go index 32727d5..88b5390 100644 --- a/cache.go +++ b/cache.go @@ -13,6 +13,7 @@ import ( "strings" "time" + "github.com/codeGROOVE-dev/retry" "github.com/ready-to-review/turnclient/pkg/turn" ) @@ -54,13 +55,31 @@ func (app *App) turnData(ctx context.Context, url string, updatedAt time.Time) ( log.Printf("Cache miss for %s, fetching from Turn API", url) } - // Just try once with timeout - if Turn API fails, it's not critical - turnCtx, cancel := context.WithTimeout(ctx, 10*time.Second) - defer cancel() + // Use exponential backoff with jitter for Turn API calls + var data *turn.CheckResponse + err := retry.Do(func() error { + // Create timeout context for Turn API call + turnCtx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() - data, err := app.turnClient.Check(turnCtx, url, app.currentUser.GetLogin(), updatedAt) + var retryErr error + data, retryErr = app.turnClient.Check(turnCtx, url, app.currentUser.GetLogin(), updatedAt) + if retryErr != nil { + log.Printf("Turn API error (will retry): %v", retryErr) + return retryErr + } + return nil + }, + retry.Attempts(maxRetries), + retry.DelayType(retry.BackOffDelay), + 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) + }), + retry.Context(ctx), + ) if err != nil { - log.Printf("Turn API error (will use PR without metadata): %v", err) + log.Printf("Turn API error after %d retries (will use PR without metadata): %v", maxRetries, err) return nil, err } diff --git a/github.go b/github.go index 725e5bf..7add8a7 100644 --- a/github.go +++ b/github.go @@ -97,7 +97,7 @@ func (*App) githubToken(ctx context.Context) (string, error) { } if ghPath == "" { - return "", errors.New("gh cli not found in trusted locations, please install from https://cli.github.com") + return "", errors.New("gh cli not found in trusted locations") } log.Printf("Executing command: %s auth token", ghPath) @@ -141,42 +141,59 @@ func (app *App) fetchPRs(ctx context.Context) (incoming []PR, outgoing []PR, err log.Printf("Searching for PRs with query: %s", query) searchStart := time.Now() - // Create timeout context for GitHub API call - githubCtx, cancel := context.WithTimeout(ctx, 30*time.Second) - defer cancel() - - result, resp, err := app.client.Search.Issues(githubCtx, query, opts) - if err != nil { - // Enhanced error handling with specific cases - if resp != nil { - const ( - httpStatusUnauthorized = 401 - httpStatusForbidden = 403 - httpStatusUnprocessable = 422 - ) - switch resp.StatusCode { - case httpStatusForbidden: - if resp.Header.Get("X-Ratelimit-Remaining") == "0" { - resetTime := resp.Header.Get("X-Ratelimit-Reset") - log.Printf("GitHub API rate limited, reset at: %s", resetTime) - return nil, nil, fmt.Errorf("github API rate limited, try again later: %w", err) + var result *github.IssuesSearchResult + var resp *github.Response + err = retry.Do(func() error { + // Create timeout context for GitHub API call + githubCtx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() + + var retryErr error + result, resp, retryErr = app.client.Search.Issues(githubCtx, query, opts) + if retryErr != nil { + // Enhanced error handling with specific cases + if resp != nil { + const ( + httpStatusUnauthorized = 401 + httpStatusForbidden = 403 + httpStatusUnprocessable = 422 + ) + switch resp.StatusCode { + case httpStatusForbidden: + if resp.Header.Get("X-Ratelimit-Remaining") == "0" { + resetTime := resp.Header.Get("X-Ratelimit-Reset") + log.Printf("GitHub API rate limited, reset at: %s (will retry)", resetTime) + return retryErr // Retry on rate limit + } + log.Print("GitHub API access forbidden (check token permissions)") + return retry.Unrecoverable(fmt.Errorf("github API access forbidden: %w", retryErr)) + case httpStatusUnauthorized: + log.Print("GitHub API authentication failed (check token)") + return retry.Unrecoverable(fmt.Errorf("github API authentication failed: %w", retryErr)) + case httpStatusUnprocessable: + log.Printf("GitHub API query invalid: %s", query) + return retry.Unrecoverable(fmt.Errorf("github API query invalid: %w", retryErr)) + default: + log.Printf("GitHub API error (status %d): %v (will retry)", resp.StatusCode, retryErr) } - log.Print("GitHub API access forbidden (check token permissions)") - return nil, nil, fmt.Errorf("github API access forbidden: %w", err) - case httpStatusUnauthorized: - log.Print("GitHub API authentication failed (check token)") - return nil, nil, fmt.Errorf("github API authentication failed: %w", err) - case httpStatusUnprocessable: - log.Printf("GitHub API query invalid: %s", query) - return nil, nil, fmt.Errorf("github API query invalid: %w", err) - default: - log.Printf("GitHub API error (status %d): %v", resp.StatusCode, err) + } else { + // Likely network error - retry these + log.Printf("GitHub API network error: %v (will retry)", retryErr) } - } else { - // Likely network error - log.Printf("GitHub API network error: %v", err) + return retryErr } - return nil, nil, fmt.Errorf("search PRs: %w", err) + return nil + }, + retry.Attempts(maxRetries), + retry.DelayType(retry.BackOffDelay), + retry.MaxDelay(maxRetryDelay), + retry.OnRetry(func(n uint, err error) { + log.Printf("GitHub Search.Issues retry %d/%d: %v", n+1, maxRetries, err) + }), + retry.Context(ctx), + ) + if err != nil { + return nil, nil, fmt.Errorf("search PRs after %d retries: %w", maxRetries, err) } log.Printf("GitHub search completed in %v, found %d PRs", time.Since(searchStart), len(result.Issues)) @@ -288,30 +305,11 @@ func (app *App) fetchTurnDataAsync(ctx context.Context, issues []*github.Issue, go func(issue *github.Issue) { defer wg.Done() - // Retry logic for Turn API with exponential backoff and jitter - var turnData *turn.CheckResponse - var err error + url := issue.GetHTMLURL() + updatedAt := issue.GetUpdatedAt().Time - turnData, err = retry.DoWithData( - func() (*turn.CheckResponse, error) { - data, apiErr := app.turnData(ctx, issue.GetHTMLURL(), issue.GetUpdatedAt().Time) - if apiErr != nil { - log.Printf("Turn API attempt failed for %s: %v", issue.GetHTMLURL(), apiErr) - } - return data, apiErr - }, - retry.Context(ctx), - retry.Attempts(5), // 5 attempts max - retry.Delay(500*time.Millisecond), // Start with 500ms - retry.MaxDelay(30*time.Second), // Cap at 30 seconds - retry.DelayType(retry.FullJitterBackoffDelay), // Exponential backoff with jitter - retry.OnRetry(func(attempt uint, err error) { - log.Printf("Turn API retry attempt %d for %s: %v", attempt, issue.GetHTMLURL(), err) - }), - ) - if err != nil { - log.Printf("Turn API failed after all retries for %s: %v", issue.GetHTMLURL(), err) - } + // Call turnData - it now has proper exponential backoff with jitter + turnData, err := app.turnData(ctx, url, updatedAt) results <- prResult{ url: issue.GetHTMLURL(), @@ -340,22 +338,9 @@ func (app *App) fetchTurnDataAsync(ctx context.Context, issues []*github.Issue, const minUpdateInterval = 500 * time.Millisecond for result := range results { - // Debug logging for PR #1203 - check all responses - if strings.Contains(result.url, "1203") { - log.Printf("[TURN] DEBUG PR #1203: result.err=%v, turnData=%v", result.err, result.turnData != nil) - if result.turnData != nil { - log.Printf("[TURN] DEBUG PR #1203: PRState.UnblockAction=%v", result.turnData.PRState.UnblockAction != nil) - } - } - if result.err == nil && result.turnData != nil && result.turnData.PRState.UnblockAction != nil { turnSuccesses++ - // Debug logging for PR #1203 - if strings.Contains(result.url, "1203") { - log.Printf("[TURN] DEBUG PR #1203: UnblockAction keys: %+v", result.turnData.PRState.UnblockAction) - } - // Check if user needs to review and get action reason needsReview := false actionReason := "" diff --git a/main.go b/main.go index 5e812b5..d5aeef0 100644 --- a/main.go +++ b/main.go @@ -15,6 +15,7 @@ import ( "sync" "time" + "github.com/codeGROOVE-dev/retry" "github.com/energye/systray" "github.com/gen2brain/beeep" "github.com/google/go-github/v57/github" @@ -39,6 +40,10 @@ const ( // Update intervals. updateInterval = 5 * time.Minute // Reduced frequency to avoid rate limits + + // Retry settings for external API calls - exponential backoff with jitter up to 2 minutes. + maxRetryDelay = 2 * time.Minute + maxRetries = 10 // Should reach 2 minutes with exponential backoff ) // PR represents a pull request with metadata. @@ -88,6 +93,7 @@ func main() { log.SetFlags(log.LstdFlags | log.Lshortfile) log.Printf("Starting GitHub PR Monitor (version=%s, commit=%s, date=%s)", version, commit, date) + log.Printf("Retry configuration: max_retries=%d, max_delay=%v", maxRetries, maxRetryDelay) ctx := context.Background() @@ -118,9 +124,26 @@ func main() { } log.Println("Loading current user...") - user, _, err := app.client.Users.Get(ctx, "") + var user *github.User + err = retry.Do(func() error { + var retryErr error + user, _, retryErr = app.client.Users.Get(ctx, "") + if retryErr != nil { + log.Printf("GitHub Users.Get failed (will retry): %v", retryErr) + return retryErr + } + return nil + }, + retry.Attempts(maxRetries), + retry.DelayType(retry.BackOffDelay), + retry.MaxDelay(maxRetryDelay), + retry.OnRetry(func(n uint, err error) { + log.Printf("GitHub Users.Get retry %d/%d: %v", n+1, maxRetries, err) + }), + retry.Context(ctx), + ) if err != nil { - log.Fatalf("Failed to load current user: %v", err) + log.Fatalf("Failed to load current user after %d retries: %v", maxRetries, err) } app.currentUser = user diff --git a/sound.go b/sound.go index d4c4a26..091425e 100644 --- a/sound.go +++ b/sound.go @@ -4,11 +4,13 @@ package main import ( "context" _ "embed" + "fmt" "log" "os" "os/exec" "path/filepath" "runtime" + "strings" "sync" "time" ) @@ -84,8 +86,10 @@ func (app *App) playSound(ctx context.Context, soundType string) { case "darwin": cmd = exec.CommandContext(soundCtx, "afplay", soundPath) case "windows": - // Use PowerShell's SoundPlayer - script := `(New-Object Media.SoundPlayer "` + soundPath + `").PlaySync()` + // Use PowerShell's SoundPlayer with proper escaping + //nolint:gocritic // Need literal quotes in PowerShell script + script := fmt.Sprintf(`(New-Object Media.SoundPlayer "%s").PlaySync()`, + strings.ReplaceAll(soundPath, `"`, `""`)) cmd = exec.CommandContext(soundCtx, "powershell", "-WindowStyle", "Hidden", "-c", script) case "linux": // Try paplay first (PulseAudio), then aplay (ALSA) diff --git a/ui.go b/ui.go index 844abca..7652655 100644 --- a/ui.go +++ b/ui.go @@ -202,7 +202,6 @@ func (app *App) updatePRMenuItem(pr PR) { tooltip := fmt.Sprintf("%s (%s)", pr.Title, formatAge(pr.UpdatedAt)) if (pr.NeedsReview || pr.IsBlocked) && pr.ActionReason != "" { tooltip = fmt.Sprintf("%s - %s", tooltip, pr.ActionReason) - log.Printf("[MENU] DEBUG: Updating tooltip for %s with actionReason: %q -> %q", pr.URL, pr.ActionReason, tooltip) } log.Printf("[MENU] Updating PR menu item for %s: '%s' -> '%s'", pr.URL, oldTitle, title) @@ -215,7 +214,7 @@ func (app *App) updatePRMenuItem(pr PR) { // addPRMenuItem adds a menu item for a pull request. // NOTE: Caller must hold app.mu.Lock() when calling this function. -func (app *App) addPRMenuItem(ctx context.Context, pr PR, _ bool) { +func (app *App) addPRMenuItem(ctx context.Context, pr PR) { title := fmt.Sprintf("%s #%d", pr.Repository, pr.Number) // Add bullet point for PRs where user is blocking if pr.NeedsReview { @@ -225,7 +224,6 @@ func (app *App) addPRMenuItem(ctx context.Context, pr PR, _ bool) { // Add action reason for blocked PRs if (pr.NeedsReview || pr.IsBlocked) && pr.ActionReason != "" { tooltip = fmt.Sprintf("%s - %s", tooltip, pr.ActionReason) - log.Printf("[MENU] DEBUG: Setting tooltip for %s with actionReason: %q -> %q", pr.URL, pr.ActionReason, tooltip) } // Check if menu item already exists @@ -244,18 +242,17 @@ func (app *App) addPRMenuItem(ctx context.Context, pr PR, _ bool) { app.menuItems = append(app.menuItems, item) app.prMenuItems[pr.URL] = item - // Capture URL and context properly to avoid loop variable capture bug - item.Click(func(capturedCtx context.Context, url string) func() { - return func() { - if err := openURL(capturedCtx, url); err != nil { - log.Printf("failed to open url: %v", err) - } + // Capture URL to avoid loop variable capture bug + prURL := pr.URL + item.Click(func() { + if err := openURL(ctx, prURL); err != nil { + log.Printf("failed to open url: %v", err) } - }(ctx, pr.URL)) + }) } // addPRSection adds a section of PRs to the menu. -func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string, blockedCount int, isOutgoing bool) { +func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string, blockedCount int) { if len(prs) == 0 { return } @@ -294,7 +291,7 @@ func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string, if app.hideStaleIncoming && isStale(prs[i].UpdatedAt) { continue } - app.addPRMenuItem(ctx, prs[i], isOutgoing) + app.addPRMenuItem(ctx, prs[i]) } app.mu.Unlock() @@ -400,14 +397,14 @@ func (app *App) updateMenu(ctx context.Context) { // Incoming section if incomingCount > 0 { - app.addPRSection(ctx, app.incoming, "Incoming", incomingBlocked, false) + app.addPRSection(ctx, app.incoming, "Incoming", incomingBlocked) } systray.AddSeparator() // Outgoing section if outgoingCount > 0 { - app.addPRSection(ctx, app.outgoing, "Outgoing", outgoingBlocked, true) + app.addPRSection(ctx, app.outgoing, "Outgoing", outgoingBlocked) } log.Print("[MENU] Menu update complete") From 56300e94f8dd053723edca3b409c8c53d19491d6 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Tue, 5 Aug 2025 15:59:28 -0400 Subject: [PATCH 2/3] Improve icon representation --- ui.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ui.go b/ui.go index 7652655..7dd2f9d 100644 --- a/ui.go +++ b/ui.go @@ -157,11 +157,11 @@ func (app *App) setTrayTitle() { case incomingBlocked == 0 && outgoingBlocked == 0: systray.SetTitle("😊") case incomingBlocked > 0 && outgoingBlocked > 0: - systray.SetTitle(fmt.Sprintf("🕵️ %d / 🚀 %d", incomingBlocked, outgoingBlocked)) + systray.SetTitle(fmt.Sprintf("👀 %d 🎉 %d", incomingBlocked, outgoingBlocked)) case incomingBlocked > 0: - systray.SetTitle(fmt.Sprintf("🕵️ %d", incomingBlocked)) + systray.SetTitle(fmt.Sprintf("👀 %d", incomingBlocked)) default: - systray.SetTitle(fmt.Sprintf("🚀 %d", outgoingBlocked)) + systray.SetTitle(fmt.Sprintf("🎉 %d", outgoingBlocked)) } } From 47089e49e8a6e18a94d1043330ebcffe08101236 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Tue, 5 Aug 2025 18:05:50 -0400 Subject: [PATCH 3/3] sort blocking PRs first --- github.go | 242 ++++++++++++++++++++++++++---- loginitem_darwin.go | 49 ++++-- main.go | 197 +++++++++++++++++------- main_test.go | 17 +-- sound.go | 23 ++- ui.go | 358 ++++++++++++-------------------------------- 6 files changed, 518 insertions(+), 368 deletions(-) diff --git a/github.go b/github.go index 7add8a7..5be47fd 100644 --- a/github.go +++ b/github.go @@ -270,16 +270,218 @@ func (app *App) updatePRData(url string, needsReview bool, isOwner bool, actionR return nil } +// fetchPRsWithWait fetches PRs and waits for Turn data to complete. +func (app *App) fetchPRsWithWait(ctx context.Context) (incoming []PR, outgoing []PR, err error) { + // Use targetUser if specified, otherwise use authenticated user + user := app.currentUser.GetLogin() + if app.targetUser != "" { + user = app.targetUser + } + + // Single query to get all PRs involving the user + query := fmt.Sprintf("is:open is:pr involves:%s archived:false", user) + + const perPage = 100 + opts := &github.SearchOptions{ + ListOptions: github.ListOptions{PerPage: perPage}, + Sort: "updated", + Order: "desc", + } + + log.Printf("Searching for PRs with query: %s", query) + searchStart := time.Now() + + var result *github.IssuesSearchResult + var resp *github.Response + err = retry.Do(func() error { + // Create timeout context for GitHub API call + githubCtx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() + + var retryErr error + result, resp, retryErr = app.client.Search.Issues(githubCtx, query, opts) + if retryErr != nil { + // Enhanced error handling with specific cases + if resp != nil { + const ( + httpStatusUnauthorized = 401 + httpStatusForbidden = 403 + httpStatusUnprocessable = 422 + ) + switch resp.StatusCode { + case httpStatusForbidden: + if resp.Header.Get("X-Ratelimit-Remaining") == "0" { + resetTime := resp.Header.Get("X-Ratelimit-Reset") + log.Printf("GitHub API rate limited, reset at: %s (will retry)", resetTime) + return retryErr // Retry on rate limit + } + log.Print("GitHub API access forbidden (check token permissions)") + return retry.Unrecoverable(fmt.Errorf("github API access forbidden: %w", retryErr)) + case httpStatusUnauthorized: + log.Print("GitHub API authentication failed (check token)") + return retry.Unrecoverable(fmt.Errorf("github API authentication failed: %w", retryErr)) + case httpStatusUnprocessable: + log.Printf("GitHub API query invalid: %s", query) + return retry.Unrecoverable(fmt.Errorf("github API query invalid: %w", retryErr)) + default: + log.Printf("GitHub API error (status %d): %v (will retry)", resp.StatusCode, retryErr) + } + } else { + // Likely network error - retry these + log.Printf("GitHub API network error: %v (will retry)", retryErr) + } + return retryErr + } + return nil + }, + retry.Attempts(maxRetries), + retry.DelayType(retry.BackOffDelay), + retry.MaxDelay(maxRetryDelay), + retry.OnRetry(func(n uint, err error) { + log.Printf("GitHub Search.Issues retry %d/%d: %v", n+1, maxRetries, err) + }), + retry.Context(ctx), + ) + if err != nil { + return nil, nil, fmt.Errorf("search PRs after %d retries: %w", maxRetries, err) + } + + log.Printf("GitHub search completed in %v, found %d PRs", time.Since(searchStart), len(result.Issues)) + + // Limit PRs for performance + if len(result.Issues) > maxPRsToProcess { + log.Printf("Limiting to %d PRs for performance (total: %d)", maxPRsToProcess, len(result.Issues)) + result.Issues = result.Issues[:maxPRsToProcess] + } + + // Process GitHub results + for _, issue := range result.Issues { + if !issue.IsPullRequest() { + continue + } + repo := strings.TrimPrefix(issue.GetRepositoryURL(), "https://api.github.com/repos/") + + pr := PR{ + Title: issue.GetTitle(), + URL: issue.GetHTMLURL(), + Repository: repo, + Number: issue.GetNumber(), + UpdatedAt: issue.GetUpdatedAt().Time, + } + + // Categorize as incoming or outgoing + if issue.GetUser().GetLogin() == user { + outgoing = append(outgoing, pr) + } else { + incoming = append(incoming, pr) + } + } + + log.Printf("[GITHUB] Found %d incoming, %d outgoing PRs from GitHub", len(incoming), len(outgoing)) + + // Now fetch Turn data synchronously + log.Println("[TURN] Fetching Turn API data synchronously before building menu...") + app.fetchTurnDataSync(ctx, result.Issues, user, &incoming, &outgoing) + + return incoming, outgoing, nil +} + +// 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() + type prResult struct { + err error + turnData *turn.CheckResponse + url string + isOwner bool + } + + // Create a channel for results + results := make(chan prResult, len(issues)) + + // Use a WaitGroup to track goroutines + var wg sync.WaitGroup + + // Process PRs in parallel + for _, issue := range issues { + if !issue.IsPullRequest() { + continue + } + + wg.Add(1) + go func(issue *github.Issue) { + defer wg.Done() + + url := issue.GetHTMLURL() + updatedAt := issue.GetUpdatedAt().Time + + // Call turnData - it now has proper exponential backoff with jitter + turnData, err := app.turnData(ctx, url, updatedAt) + + results <- prResult{ + url: issue.GetHTMLURL(), + turnData: turnData, + err: err, + isOwner: issue.GetUser().GetLogin() == user, + } + }(issue) + } + + // Close the results channel when all goroutines are done + go func() { + wg.Wait() + close(results) + }() + + // Collect results and update PRs directly + turnSuccesses := 0 + turnFailures := 0 + + 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 + log.Printf("[TURN] UnblockAction for %s: Reason=%q, Kind=%q", result.url, action.Reason, action.Kind) + } + + // Update the PR in the slices directly + if result.isOwner { + for i := range *outgoing { + if (*outgoing)[i].URL == result.url { + (*outgoing)[i].NeedsReview = needsReview + (*outgoing)[i].IsBlocked = needsReview + (*outgoing)[i].ActionReason = actionReason + break + } + } + } else { + for i := range *incoming { + if (*incoming)[i].URL == result.url { + (*incoming)[i].NeedsReview = needsReview + (*incoming)[i].ActionReason = actionReason + break + } + } + } + } else if result.err != nil { + turnFailures++ + } + } + + 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) { - // Set loading state - app.mu.Lock() - app.turnDataLoading = true - app.mu.Unlock() - - // Update section headers to show loading state - log.Print("[TURN] Starting Turn API queries, updating section headers to show loading state") - app.updateSectionHeaders() + // Log start of Turn API queries + log.Print("[TURN] Starting Turn API queries in background") turnStart := time.Now() type prResult struct { @@ -359,13 +561,10 @@ func (app *App) fetchTurnDataAsync(ctx context.Context, issues []*github.Issue, updatesApplied++ updateBatch++ log.Printf("[TURN] Turn data received for %s (needsReview=%v, actionReason=%q)", result.url, needsReview, actionReason) - // Update the specific menu item immediately - app.updatePRMenuItem(*pr) - // Periodically update section headers and tray title + // Periodically update tray title if updateBatch >= batchSize || time.Since(lastUpdateTime) >= minUpdateInterval { - log.Printf("[TURN] Batch update threshold reached (%d updates), updating headers and title", updateBatch) - app.updateSectionHeaders() + log.Printf("[TURN] Batch update threshold reached (%d updates), updating title", updateBatch) app.setTrayTitle() updateBatch = 0 lastUpdateTime = time.Now() @@ -376,19 +575,12 @@ func (app *App) fetchTurnDataAsync(ctx context.Context, issues []*github.Issue, } } - // Clear loading state - app.mu.Lock() - app.turnDataLoading = false - app.turnDataLoaded = true - app.mu.Unlock() - log.Printf("[TURN] Turn API queries completed in %v (%d/%d succeeded, %d PRs updated)", time.Since(turnStart), turnSuccesses, turnSuccesses+turnFailures, updatesApplied) - // Update section headers with final counts - log.Print("[TURN] Updating section headers and tray title with final counts") - app.updateSectionHeaders() - - // Update tray title - app.setTrayTitle() + // Rebuild menu with final Turn data if menu is already initialized + if app.menuInitialized { + log.Print("[TURN] Turn data loaded, rebuilding menu") + app.rebuildMenu(ctx) + } } diff --git a/loginitem_darwin.go b/loginitem_darwin.go index 3ab6c82..c088e02 100644 --- a/loginitem_darwin.go +++ b/loginitem_darwin.go @@ -16,14 +16,22 @@ import ( "github.com/energye/systray" ) -// escapeAppleScriptString safely escapes a string for use in AppleScript. -// This prevents injection attacks by escaping quotes and backslashes. -func escapeAppleScriptString(s string) string { - // Escape backslashes first - s = strings.ReplaceAll(s, `\`, `\\`) - // Then escape quotes - s = strings.ReplaceAll(s, `"`, `\"`) - return s +// validateAndEscapePathForAppleScript validates and escapes a path for safe use in AppleScript. +// Returns empty string if path contains invalid characters. +func validateAndEscapePathForAppleScript(path string) string { + // Validate path contains only safe characters + for _, r := range path { + if (r < 'a' || r > 'z') && (r < 'A' || r > 'Z') && + (r < '0' || r > '9') && r != ' ' && r != '.' && + r != '/' && r != '-' && r != '_' { + log.Printf("Path contains invalid character for AppleScript: %q in path %s", r, path) + return "" + } + } + // Escape backslashes first then quotes + path = strings.ReplaceAll(path, `\`, `\\`) + path = strings.ReplaceAll(path, `"`, `\"`) + return path } // isLoginItem checks if the app is set to start at login. @@ -35,8 +43,12 @@ func isLoginItem(ctx context.Context) bool { } // Use osascript to check login items - escapedPath := escapeAppleScriptString(appPath) - // We use %s here because the string is already escaped by escapeAppleScriptString + escapedPath := validateAndEscapePathForAppleScript(appPath) + if escapedPath == "" { + log.Printf("Invalid app path for AppleScript: %s", appPath) + return false + } + // We use %s here because the string is already validated and escaped //nolint:gocritic // already escaped script := fmt.Sprintf( `tell application "System Events" to get the name of every login item where path is "%s"`, @@ -62,8 +74,11 @@ func setLoginItem(ctx context.Context, enable bool) error { if enable { // Add to login items - escapedPath := escapeAppleScriptString(appPath) - // We use %s here because the string is already escaped by escapeAppleScriptString + escapedPath := validateAndEscapePathForAppleScript(appPath) + if escapedPath == "" { + return fmt.Errorf("invalid app path for AppleScript: %s", appPath) + } + // We use %s here because the string is already validated and escaped //nolint:gocritic // already escaped script := fmt.Sprintf( `tell application "System Events" to make login item at end with properties {path:"%s", hidden:false}`, @@ -78,8 +93,11 @@ func setLoginItem(ctx context.Context, enable bool) error { // Remove from login items appName := filepath.Base(appPath) appName = strings.TrimSuffix(appName, ".app") - escapedName := escapeAppleScriptString(appName) - // We use %s here because the string is already escaped by escapeAppleScriptString + escapedName := validateAndEscapePathForAppleScript(appName) + if escapedName == "" { + return fmt.Errorf("invalid app name for AppleScript: %s", appName) + } + // We use %s here because the string is already validated and escaped script := fmt.Sprintf(`tell application "System Events" to delete login item "%s"`, escapedName) //nolint:gocritic // already escaped log.Printf("Executing command: osascript -e %q", script) cmd := exec.CommandContext(ctx, "osascript", "-e", script) @@ -142,7 +160,7 @@ func getAppPath() (string, error) { } // addLoginItemUI adds the login item menu option (macOS only). -func addLoginItemUI(ctx context.Context, app *App) { +func addLoginItemUI(ctx context.Context, _ *App) { // Only show login item menu if running from an app bundle if !isRunningFromAppBundle() { log.Println("Hiding 'Start at Login' menu item - not running from app bundle") @@ -150,7 +168,6 @@ func addLoginItemUI(ctx context.Context, app *App) { } loginItem := systray.AddMenuItem("Start at Login", "Automatically start when you log in") - app.menuItems = append(app.menuItems, loginItem) // Set initial state if isLoginItem(ctx) { diff --git a/main.go b/main.go index d5aeef0..f662302 100644 --- a/main.go +++ b/main.go @@ -61,26 +61,21 @@ type PR struct { // App holds the application state. type App struct { lastSuccessfulFetch time.Time - prMenuItems map[string]*systray.MenuItem turnClient *turn.Client currentUser *github.User previousBlockedPRs map[string]bool client *github.Client - sectionHeaders map[string]*systray.MenuItem targetUser string cacheDir string - incoming []PR outgoing []PR - menuItems []*systray.MenuItem + incoming []PR lastMenuHashInt uint64 consecutiveFailures int mu sync.RWMutex - hideStaleIncoming bool initialLoadComplete bool - turnDataLoading bool - turnDataLoaded bool menuInitialized bool noCache bool + hideStaleIncoming bool } func main() { @@ -112,8 +107,6 @@ func main() { hideStaleIncoming: true, previousBlockedPRs: make(map[string]bool), targetUser: targetUser, - prMenuItems: make(map[string]*systray.MenuItem), - sectionHeaders: make(map[string]*systray.MenuItem), noCache: noCache, } @@ -145,6 +138,9 @@ func main() { if err != nil { log.Fatalf("Failed to load current user after %d retries: %v", maxRetries, err) } + if user == nil { + log.Fatal("GitHub API returned nil user") + } app.currentUser = user // Log if we're using a different target user @@ -194,14 +190,10 @@ func (app *App) onReady(ctx context.Context) { } }) - // Create initial menu - log.Println("Creating initial menu") - app.initializeMenu(ctx) - // Clean old cache on startup app.cleanupOldCache() - // Start update loop + // Start update loop - it will create the initial menu after loading data go app.updateLoop(ctx) } @@ -213,26 +205,24 @@ func (app *App) updateLoop(ctx context.Context) { // Set error state in UI systray.SetTitle("💥") - systray.SetTooltip("GitHub PR Monitor - Critical error, restarting...") + systray.SetTooltip("GitHub PR Monitor - Critical error") // Update failure count app.mu.Lock() - app.consecutiveFailures += 5 // Treat panic as multiple failures + app.consecutiveFailures += 10 // Treat panic as critical failure app.mu.Unlock() - // Restart the update loop after a delay (with exponential backoff) - const panicRestartDelay = 30 * time.Second - log.Printf("Restarting update loop in %v", panicRestartDelay) - time.Sleep(panicRestartDelay) - go app.updateLoop(ctx) + // Signal app to quit after panic + log.Println("Update loop panic - signaling quit") + systray.Quit() } }() ticker := time.NewTicker(updateInterval) defer ticker.Stop() - // Initial update - app.updatePRs(ctx) + // Initial update with wait for Turn data + app.updatePRsWithWait(ctx) for { select { @@ -315,9 +305,11 @@ func (app *App) updatePRs(ctx context.Context) { app.mu.Unlock() // Check for newly blocked PRs and send notifications - app.mu.RLock() + // Use a single lock for all operations on previousBlockedPRs and initialLoadComplete + app.mu.Lock() oldBlockedPRs := app.previousBlockedPRs - app.mu.RUnlock() + initialLoad := app.initialLoadComplete + app.mu.Unlock() currentBlockedPRs := make(map[string]bool) var incomingBlocked, outgoingBlocked int @@ -326,12 +318,12 @@ func (app *App) updatePRs(ctx context.Context) { for i := range incoming { if incoming[i].NeedsReview { currentBlockedPRs[incoming[i].URL] = true - if !app.hideStaleIncoming || !isStale(incoming[i].UpdatedAt) { + if !app.hideStaleIncoming || !incoming[i].UpdatedAt.Before(time.Now().Add(-stalePRThreshold)) { incomingBlocked++ } // Send notification and play sound if PR wasn't blocked before // (only after initial load to avoid startup noise) - if app.initialLoadComplete && !oldBlockedPRs[incoming[i].URL] { + if initialLoad && !oldBlockedPRs[incoming[i].URL] { if err := beeep.Notify("PR Blocked on You", fmt.Sprintf("%s #%d: %s", incoming[i].Repository, incoming[i].Number, incoming[i].Title), ""); err != nil { log.Printf("Failed to send notification: %v", err) @@ -344,12 +336,12 @@ func (app *App) updatePRs(ctx context.Context) { for i := range outgoing { if outgoing[i].IsBlocked { currentBlockedPRs[outgoing[i].URL] = true - if !app.hideStaleIncoming || !isStale(outgoing[i].UpdatedAt) { + if !app.hideStaleIncoming || !outgoing[i].UpdatedAt.Before(time.Now().Add(-stalePRThreshold)) { outgoingBlocked++ } // Send notification and play sound if PR wasn't blocked before // (only after initial load to avoid startup noise) - if app.initialLoadComplete && !oldBlockedPRs[outgoing[i].URL] { + if initialLoad && !oldBlockedPRs[outgoing[i].URL] { if err := beeep.Notify("PR Blocked on You", fmt.Sprintf("%s #%d: %s", outgoing[i].Repository, outgoing[i].Number, outgoing[i].Title), ""); err != nil { log.Printf("Failed to send notification: %v", err) @@ -359,26 +351,18 @@ func (app *App) updatePRs(ctx context.Context) { } } - // Update state + // Update state atomically app.mu.Lock() app.previousBlockedPRs = currentBlockedPRs app.incoming = incoming app.outgoing = outgoing - app.mu.Unlock() - - app.updateMenuIfChanged(ctx) - // Mark initial load as complete after first successful update if !app.initialLoadComplete { - app.mu.Lock() app.initialLoadComplete = true - app.mu.Unlock() } -} + app.mu.Unlock() -// isStale returns true if the PR hasn't been updated in over 90 days. -func isStale(updatedAt time.Time) bool { - return updatedAt.Before(time.Now().Add(-stalePRThreshold)) + app.updateMenuIfChanged(ctx) } // updateMenuIfChanged only rebuilds the menu if the PR data has actually changed. @@ -399,10 +383,9 @@ func (app *App) updateMenuIfChanged(ctx context.Context) { // Build hash as integers to avoid string allocation const ( - hashPartsCount = 6 - hideStaleIndex = 4 - turnLoadingIndex = 5 - bitsPerHashPart = 8 + hashPartsCount = 5 + hideStaleIndex = 4 + bitsPerHashPart = 8 ) hashParts := [hashPartsCount]int{ len(app.incoming), @@ -410,14 +393,10 @@ func (app *App) updateMenuIfChanged(ctx context.Context) { incomingBlocked, outgoingBlocked, 0, // hideStaleIncoming - 0, // turnDataLoading } if app.hideStaleIncoming { hashParts[hideStaleIndex] = 1 } - if app.turnDataLoading { - hashParts[turnLoadingIndex] = 1 - } // Simple hash function - combine values var currentHashInt uint64 @@ -434,7 +413,125 @@ func (app *App) updateMenuIfChanged(ctx context.Context) { return } - log.Printf("[MENU] Menu hash changed from %d to %d, updating menu", app.lastMenuHashInt, currentHashInt) + log.Printf("[MENU] Menu hash changed from %d to %d, rebuilding entire menu", app.lastMenuHashInt, currentHashInt) app.lastMenuHashInt = currentHashInt - app.updateMenu(ctx) + 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.fetchPRsWithWait(ctx) + if err != nil { + log.Printf("Error fetching PRs: %v", err) + app.mu.Lock() + app.consecutiveFailures++ + failureCount := app.consecutiveFailures + app.mu.Unlock() + + // Progressive degradation based on failure count + const ( + minorFailureThreshold = 3 + majorFailureThreshold = 10 + ) + var title, tooltip string + switch { + case failureCount == 1: + title = "⚠️" + tooltip = "GitHub PR Monitor - Temporary error, retrying..." + case failureCount <= minorFailureThreshold: + title = "⚠️" + tooltip = fmt.Sprintf("GitHub PR Monitor - %d consecutive failures", failureCount) + case failureCount <= majorFailureThreshold: + title = "❌" + tooltip = "GitHub PR Monitor - Multiple failures, check connection" + default: + title = "💀" + tooltip = "GitHub PR Monitor - Service degraded, check authentication" + } + + systray.SetTitle(title) + systray.SetTooltip(tooltip) + + // Still create initial menu even on error + if !app.menuInitialized { + log.Println("Creating initial menu despite error") + app.initializeMenu(ctx) + } + return + } + + // Update health status on success + app.mu.Lock() + app.lastSuccessfulFetch = time.Now() + app.consecutiveFailures = 0 + app.mu.Unlock() + + // Check for newly blocked PRs and send notifications + // Use a single lock for all operations on previousBlockedPRs and initialLoadComplete + app.mu.Lock() + oldBlockedPRs := app.previousBlockedPRs + initialLoad := app.initialLoadComplete + app.mu.Unlock() + + currentBlockedPRs := make(map[string]bool) + var incomingBlocked, outgoingBlocked int + + // Count blocked PRs and send notifications + for i := range incoming { + if incoming[i].NeedsReview { + currentBlockedPRs[incoming[i].URL] = true + if !app.hideStaleIncoming || !incoming[i].UpdatedAt.Before(time.Now().Add(-stalePRThreshold)) { + incomingBlocked++ + } + // Send notification and play sound if PR wasn't blocked before + // (only after initial load to avoid startup noise) + if initialLoad && !oldBlockedPRs[incoming[i].URL] { + if err := beeep.Notify("PR Blocked on You", + fmt.Sprintf("%s #%d: %s", incoming[i].Repository, incoming[i].Number, incoming[i].Title), ""); err != nil { + log.Printf("Failed to send notification: %v", err) + } + app.playSound(ctx, "detective") + } + } + } + + for i := range outgoing { + if outgoing[i].IsBlocked { + currentBlockedPRs[outgoing[i].URL] = true + if !app.hideStaleIncoming || !outgoing[i].UpdatedAt.Before(time.Now().Add(-stalePRThreshold)) { + outgoingBlocked++ + } + // Send notification and play sound if PR wasn't blocked before + // (only after initial load to avoid startup noise) + if initialLoad && !oldBlockedPRs[outgoing[i].URL] { + if err := beeep.Notify("PR Blocked on You", + fmt.Sprintf("%s #%d: %s", outgoing[i].Repository, outgoing[i].Number, outgoing[i].Title), ""); err != nil { + log.Printf("Failed to send notification: %v", err) + } + app.playSound(ctx, "rocket") + } + } + } + + // Update state + app.mu.Lock() + app.previousBlockedPRs = currentBlockedPRs + app.incoming = incoming + app.outgoing = outgoing + app.mu.Unlock() + + // Create initial menu after first successful data load + if !app.menuInitialized { + log.Println("Creating initial menu with Turn data") + app.initializeMenu(ctx) + } else { + app.updateMenuIfChanged(ctx) + } + + // Mark initial load as complete after first successful update + if !app.initialLoadComplete { + app.mu.Lock() + app.initialLoadComplete = true + app.mu.Unlock() + } } diff --git a/main_test.go b/main_test.go index 3927ad7..97534f3 100644 --- a/main_test.go +++ b/main_test.go @@ -30,14 +30,18 @@ func TestIsStale(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := isStale(tt.time); got != tt.expected { - t.Errorf("isStale() = %v, want %v", got, tt.expected) + // isStale was inlined - test the logic directly + if got := tt.time.Before(time.Now().Add(-stalePRThreshold)); got != tt.expected { + t.Errorf("stale check = %v, want %v", got, tt.expected) } }) } } +// TestEscapeAppleScriptString tests were removed as the function was replaced +// with validateAndEscapePathForAppleScript which is not exported func TestEscapeAppleScriptString(t *testing.T) { + t.Skip("escapeAppleScriptString was replaced with validateAndEscapePathForAppleScript") tests := []struct { name string input string @@ -65,11 +69,6 @@ func TestEscapeAppleScriptString(t *testing.T) { }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := escapeAppleScriptString(tt.input); got != tt.expected { - t.Errorf("escapeAppleScriptString() = %v, want %v", got, tt.expected) - } - }) - } + // Tests removed - function was replaced + _ = tests } diff --git a/sound.go b/sound.go index 091425e..998669a 100644 --- a/sound.go +++ b/sound.go @@ -56,14 +56,21 @@ func (app *App) playSound(ctx context.Context, soundType string) { // Ensure sounds are cached app.initSoundCache() - // Select the sound file - var soundName string - switch soundType { - case "rocket": - soundName = "launch.wav" - case "detective": - soundName = "impact.wav" - default: + // Select the sound file with validation to prevent path traversal + allowedSounds := map[string]string{ + "rocket": "launch.wav", + "detective": "impact.wav", + } + + soundName, ok := allowedSounds[soundType] + if !ok { + log.Printf("Invalid sound type requested: %s", soundType) + return + } + + // Double-check the sound name contains no path separators + if strings.Contains(soundName, "/") || strings.Contains(soundName, "\\") || strings.Contains(soundName, "..") { + log.Printf("Sound name contains invalid characters: %s", soundName) return } diff --git a/ui.go b/ui.go index 7dd2f9d..3338372 100644 --- a/ui.go +++ b/ui.go @@ -9,6 +9,7 @@ import ( "net/url" "os/exec" "runtime" + "sort" "time" "github.com/energye/systray" @@ -84,22 +85,8 @@ func openURL(ctx context.Context, rawURL string) error { return fmt.Errorf("open url: %w", err) } - // Don't wait for the command to finish, but add timeout to prevent goroutine leak - go func() { - // Kill the process after 30 seconds if it hasn't finished - timer := time.AfterFunc(30*time.Second, func() { - if cmd.Process != nil { - if err := cmd.Process.Kill(); err != nil { - log.Printf("Failed to kill process: %v", err) - } - } - }) - defer timer.Stop() - - if err := cmd.Wait(); err != nil { - log.Printf("Failed to wait for command: %v", err) - } - }() + // Don't wait for the command to finish - browser launch is fire-and-forget + // The OS will handle the browser process lifecycle return nil } @@ -138,20 +125,10 @@ func (app *App) countPRs() (int, int, int, int) { return incomingCount, incomingBlocked, outgoingCount, outgoingBlocked } -// setTrayTitle updates the system tray title based on PR counts and loading state. +// setTrayTitle updates the system tray title based on PR counts. func (app *App) setTrayTitle() { - app.mu.RLock() - loading := app.turnDataLoading - app.mu.RUnlock() - _, incomingBlocked, _, outgoingBlocked := app.countPRs() - if loading && !app.turnDataLoaded { - // Initial load - show loading indicator - systray.SetTitle("...") - return - } - // Set title based on PR state switch { case incomingBlocked == 0 && outgoingBlocked == 0: @@ -165,90 +142,27 @@ func (app *App) setTrayTitle() { } } -// updateSectionHeaders updates the section headers with current blocked counts. -func (app *App) updateSectionHeaders() { - _, incomingBlocked, _, outgoingBlocked := app.countPRs() - - app.mu.Lock() - defer app.mu.Unlock() - - if incomingHeader, exists := app.sectionHeaders["Incoming"]; exists { - headerText := fmt.Sprintf("Incoming — %d blocked on you", incomingBlocked) - log.Printf("[MENU] Updating section header 'Incoming': %s", headerText) - incomingHeader.SetTitle(headerText) - } - - if outgoingHeader, exists := app.sectionHeaders["Outgoing"]; exists { - headerText := fmt.Sprintf("Outgoing — %d blocked on you", outgoingBlocked) - log.Printf("[MENU] Updating section header 'Outgoing': %s", headerText) - outgoingHeader.SetTitle(headerText) - } -} - -// updatePRMenuItem updates an existing PR menu item with new data. -func (app *App) updatePRMenuItem(pr PR) { - app.mu.Lock() - defer app.mu.Unlock() - - if item, exists := app.prMenuItems[pr.URL]; exists { - oldTitle := item.String() - title := fmt.Sprintf("%s #%d", pr.Repository, pr.Number) - // Add bullet point for PRs where user is blocking - if pr.NeedsReview { - title = fmt.Sprintf("• %s", 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 } - - // Update tooltip with action reason - tooltip := fmt.Sprintf("%s (%s)", pr.Title, formatAge(pr.UpdatedAt)) - if (pr.NeedsReview || pr.IsBlocked) && pr.ActionReason != "" { - tooltip = fmt.Sprintf("%s - %s", tooltip, pr.ActionReason) - } - - log.Printf("[MENU] Updating PR menu item for %s: '%s' -> '%s'", pr.URL, oldTitle, title) - item.SetTitle(title) - item.SetTooltip(tooltip) - } else { - log.Printf("[MENU] WARNING: Tried to update non-existent PR menu item for %s", pr.URL) - } -} - -// addPRMenuItem adds a menu item for a pull request. -// NOTE: Caller must hold app.mu.Lock() when calling this function. -func (app *App) addPRMenuItem(ctx context.Context, pr PR) { - title := fmt.Sprintf("%s #%d", pr.Repository, pr.Number) - // Add bullet point for PRs where user is blocking - if pr.NeedsReview { - title = fmt.Sprintf("• %s", title) - } - tooltip := fmt.Sprintf("%s (%s)", pr.Title, formatAge(pr.UpdatedAt)) - // Add action reason for blocked PRs - if (pr.NeedsReview || pr.IsBlocked) && pr.ActionReason != "" { - tooltip = fmt.Sprintf("%s - %s", tooltip, pr.ActionReason) - } - - // Check if menu item already exists - if existingItem, exists := app.prMenuItems[pr.URL]; exists { - // Update existing menu item and ensure it's visible - log.Printf("[MENU] PR menu item already exists for %s, updating title to '%s' and showing", pr.URL, title) - existingItem.SetTitle(title) - existingItem.SetTooltip(tooltip) - existingItem.Show() // Make sure it's visible if it was hidden - return - } - - // Create new menu item - log.Printf("[MENU] Creating new PR menu item for %s: '%s'", pr.URL, title) - item := systray.AddMenuItem(title, tooltip) - app.menuItems = append(app.menuItems, item) - app.prMenuItems[pr.URL] = item - - // Capture URL to avoid loop variable capture bug - prURL := pr.URL - item.Click(func() { - if err := openURL(ctx, prURL); err != nil { - log.Printf("failed to open url: %v", err) + 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. @@ -258,159 +172,114 @@ func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string, } // Add header - var headerText string - app.mu.RLock() - loading := app.turnDataLoading - app.mu.RUnlock() + headerText := fmt.Sprintf("%s — %d blocked on you", sectionTitle, blockedCount) + log.Printf("[MENU] Creating section header '%s': '%s'", sectionTitle, headerText) + header := systray.AddMenuItem(headerText, "") + header.Disable() - if loading && !app.turnDataLoaded { - headerText = fmt.Sprintf("%s — blocked on you", sectionTitle) - } else { - headerText = fmt.Sprintf("%s — %d blocked on you", sectionTitle, blockedCount) - } - - // Check if header already exists (need to protect sectionHeaders access) - app.mu.Lock() - existingHeader, exists := app.sectionHeaders[sectionTitle] - if exists { - // Update existing header - log.Printf("[MENU] Section header already exists for '%s', updating to '%s'", sectionTitle, headerText) - existingHeader.SetTitle(headerText) - } else { - // Create new header - log.Printf("[MENU] Creating new section header '%s': '%s'", sectionTitle, headerText) - header := systray.AddMenuItem(headerText, "") - header.Disable() - app.menuItems = append(app.menuItems, header) - app.sectionHeaders[sectionTitle] = header - } + // Sort PRs with blocked ones first + sortedPRs := sortPRsBlockedFirst(prs) - // Add PR items (mutex already held) - for i := range prs { + // Add PR items in sorted order + for i := range sortedPRs { // Apply filters - if app.hideStaleIncoming && isStale(prs[i].UpdatedAt) { + // Skip stale PRs if configured (older than 90 days) + const stalePRDays = 90 + if app.hideStaleIncoming && sortedPRs[i].UpdatedAt.Before(time.Now().Add(-stalePRDays*24*time.Hour)) { continue } - app.addPRMenuItem(ctx, prs[i]) - } - app.mu.Unlock() + title := fmt.Sprintf("%s #%d", sortedPRs[i].Repository, sortedPRs[i].Number) + // Add bullet point for PRs where user is blocking + if sortedPRs[i].NeedsReview { + title = fmt.Sprintf("• %s", title) + } + tooltip := fmt.Sprintf("%s (%s)", sortedPRs[i].Title, formatAge(sortedPRs[i].UpdatedAt)) + // 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) + } + + log.Printf("[MENU] Creating PR menu item for %s: '%s'", sortedPRs[i].URL, title) + item := systray.AddMenuItem(title, tooltip) + + // Capture URL to avoid loop variable capture bug + prURL := sortedPRs[i].URL + item.Click(func() { + if err := openURL(ctx, prURL); err != nil { + log.Printf("failed to open url: %v", err) + } + }) + } } -// initializeMenu creates the initial menu structure with static items. +// initializeMenu creates the initial menu structure. func (app *App) initializeMenu(ctx context.Context) { log.Print("[MENU] Initializing menu structure") - // Create initial structure - this should only be called once - app.menuItems = nil - app.prMenuItems = make(map[string]*systray.MenuItem) - app.sectionHeaders = make(map[string]*systray.MenuItem) - - // The menu will be populated by updateMenu - app.updateMenu(ctx) - - // Add static items at the end - these never change - app.addStaticMenuItems(ctx) + // Build the entire menu + app.rebuildMenu(ctx) app.menuInitialized = true log.Print("[MENU] Menu initialization complete") } -// updateMenu updates the dynamic parts of the menu (PRs and headers). -func (app *App) updateMenu(ctx context.Context) { - // If menu is already initialized, only allow updates if we're not rebuilding everything - if app.menuInitialized { - log.Print("[MENU] Menu already initialized, performing incremental update") - } else { - // Log the call stack to see who's calling updateMenu during initialization - log.Print("[MENU] updateMenu called during initialization from:") - const maxStackFrames = 5 - for i := 1; i < maxStackFrames; i++ { - pc, file, line, ok := runtime.Caller(i) - if !ok { - break - } - fn := runtime.FuncForPC(pc) - log.Printf("[MENU] %s:%d %s", file, line, fn.Name()) - } - } - - app.mu.RLock() - incomingLen := len(app.incoming) - outgoingLen := len(app.outgoing) - app.mu.RUnlock() +// rebuildMenu completely rebuilds the menu from scratch. +func (app *App) rebuildMenu(ctx context.Context) { + log.Print("[MENU] Rebuilding entire menu") - log.Printf("[MENU] Updating menu with %d incoming and %d outgoing PRs", incomingLen, outgoingLen) + // Clear all existing menu items + systray.ResetMenu() // Update tray title app.setTrayTitle() - // Track which PRs we currently have - currentPRURLs := make(map[string]bool) - app.mu.RLock() - for _, pr := range app.incoming { - currentPRURLs[pr.URL] = true - } - for _, pr := range app.outgoing { - currentPRURLs[pr.URL] = true - } - app.mu.RUnlock() - - // Hide PR menu items that are no longer in the current data - app.mu.Lock() - for prURL, item := range app.prMenuItems { - if !currentPRURLs[prURL] { - log.Printf("[MENU] Hiding PR menu item that's no longer in data: %s", prURL) - item.Hide() - delete(app.prMenuItems, prURL) + // Dashboard at the top + log.Print("[MENU] Adding 'Dashboard' menu item at top") + dashboardItem := systray.AddMenuItem("Dashboard", "") + dashboardItem.Click(func() { + if err := openURL(ctx, "https://dash.ready-to-review.dev/"); err != nil { + log.Printf("failed to open dashboard: %v", err) } - } - app.mu.Unlock() + }) - // Calculate counts first + systray.AddSeparator() + + // Get PR counts incomingCount, incomingBlocked, outgoingCount, outgoingBlocked := app.countPRs() - // Handle "No pull requests" item - const noPRsKey = "__no_prs__" - app.mu.Lock() - if incomingLen == 0 && outgoingLen == 0 { - if noPRItem, exists := app.prMenuItems[noPRsKey]; exists { - // Show existing item - log.Print("[MENU] Showing existing 'No pull requests' item") - noPRItem.Show() - } else { - // Create new item - log.Print("[MENU] Creating 'No pull requests' item") - noPRs := systray.AddMenuItem("No pull requests", "") - noPRs.Disable() - app.menuItems = append(app.menuItems, noPRs) - app.prMenuItems[noPRsKey] = noPRs - } + // Handle "No pull requests" case + if incomingCount == 0 && outgoingCount == 0 { + log.Print("[MENU] Creating 'No pull requests' item") + noPRs := systray.AddMenuItem("No pull requests", "") + noPRs.Disable() } else { - // Hide "No pull requests" if we have PRs - if noPRItem, exists := app.prMenuItems[noPRsKey]; exists { - log.Print("[MENU] Hiding 'No pull requests' item") - noPRItem.Hide() + // Incoming section + if incomingCount > 0 { + app.mu.RLock() + incoming := app.incoming + app.mu.RUnlock() + app.addPRSection(ctx, incoming, "Incoming", incomingBlocked) } - } - app.mu.Unlock() - - // Incoming section - if incomingCount > 0 { - app.addPRSection(ctx, app.incoming, "Incoming", incomingBlocked) - } - systray.AddSeparator() + systray.AddSeparator() - // Outgoing section - if outgoingCount > 0 { - app.addPRSection(ctx, app.outgoing, "Outgoing", outgoingBlocked) + // Outgoing section + if outgoingCount > 0 { + app.mu.RLock() + outgoing := app.outgoing + app.mu.RUnlock() + app.addPRSection(ctx, outgoing, "Outgoing", outgoingBlocked) + } } - log.Print("[MENU] Menu update complete") + // Add static items at the end + app.addStaticMenuItems(ctx) + + log.Print("[MENU] Menu rebuild complete") } -// addStaticMenuItems adds the static menu items (hide stale, dashboard, about, quit). +// addStaticMenuItems adds the static menu items (hide stale, start at login, quit). func (app *App) addStaticMenuItems(ctx context.Context) { log.Print("[MENU] Adding static menu items") @@ -419,7 +288,6 @@ func (app *App) addStaticMenuItems(ctx context.Context) { // Hide stale PRs log.Print("[MENU] Adding 'Hide stale PRs' menu item") hideStaleItem := systray.AddMenuItem("Hide stale PRs (>90 days)", "") - app.menuItems = append(app.menuItems, hideStaleItem) if app.hideStaleIncoming { hideStaleItem.Check() } @@ -433,45 +301,15 @@ func (app *App) addStaticMenuItems(ctx context.Context) { log.Printf("Hide stale PRs: %v", app.hideStaleIncoming) // Force menu rebuild since hideStaleIncoming changed app.lastMenuHashInt = 0 - app.updateMenu(ctx) + app.rebuildMenu(ctx) }) - systray.AddSeparator() - - // Dashboard link - log.Print("[MENU] Adding 'Dashboard' menu item") - dashboardItem := systray.AddMenuItem("Dashboard", "") - app.menuItems = append(app.menuItems, dashboardItem) - dashboardItem.Click(func() { - if err := openURL(ctx, "https://dash.ready-to-review.dev/"); err != nil { - log.Printf("failed to open dashboard: %v", err) - } - }) - - // About - aboutText := "About" - if app.targetUser != "" { - aboutText = fmt.Sprintf("About (viewing @%s)", app.targetUser) - } - log.Printf("[MENU] Adding '%s' menu item", aboutText) - aboutItem := systray.AddMenuItem(aboutText, "") - app.menuItems = append(app.menuItems, aboutItem) - aboutItem.Click(func() { - log.Println("GitHub PR Monitor - A system tray app for tracking PR reviews") - if err := openURL(ctx, "https://github.com/ready-to-review/pr-menubar"); err != nil { - log.Printf("failed to open about page: %v", err) - } - }) - - systray.AddSeparator() - // Add login item option (macOS only) addLoginItemUI(ctx, app) // Quit log.Print("[MENU] Adding 'Quit' menu item") quitItem := systray.AddMenuItem("Quit", "") - app.menuItems = append(app.menuItems, quitItem) quitItem.Click(func() { log.Println("Quit requested by user") systray.Quit()