From 9fa6b4249f08363a5aeea393d475116050ea2657 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Tue, 5 Aug 2025 14:14:53 -0400 Subject: [PATCH] Improve rendering performance & reliability --- cache.go | 46 +++++++--- github.go | 235 +++++++++++++++++++++++++++++++++++++++++++++------ go.mod | 1 + go.sum | 2 + main.go | 145 ++++++++++++++++++++++++------- main_test.go | 2 +- ui.go | 233 ++++++++++++++++++++++++++++++++++++++++++++------ 7 files changed, 570 insertions(+), 94 deletions(-) diff --git a/cache.go b/cache.go index 6eda802..51834e2 100644 --- a/cache.go +++ b/cache.go @@ -29,14 +29,18 @@ func (app *App) turnData(ctx context.Context, url string, updatedAt time.Time) ( hash := sha256.Sum256([]byte(key)) cacheFile := filepath.Join(app.cacheDir, hex.EncodeToString(hash[:])[:16]+".json") - // Try to read from cache - if data, err := os.ReadFile(cacheFile); err == nil { + // Try to read from cache (gracefully handle all cache errors) + if data, readErr := os.ReadFile(cacheFile); readErr == nil { var entry cacheEntry - if err := json.Unmarshal(data, &entry); err == nil { - // Check if cache is still valid (2 hour TTL) - if time.Since(entry.CachedAt) < cacheTTL && entry.UpdatedAt.Equal(updatedAt) { - return entry.Data, nil + if unmarshalErr := json.Unmarshal(data, &entry); unmarshalErr != nil { + log.Printf("Failed to unmarshal cache data for %s: %v", url, unmarshalErr) + // Remove corrupted cache file + if removeErr := os.Remove(cacheFile); removeErr != nil { + log.Printf("Failed to remove corrupted cache file: %v", removeErr) } + } else if time.Since(entry.CachedAt) < cacheTTL && entry.UpdatedAt.Equal(updatedAt) { + // Check if cache is still valid (2 hour TTL) + return entry.Data, nil } } @@ -53,16 +57,20 @@ func (app *App) turnData(ctx context.Context, url string, updatedAt time.Time) ( return nil, err } - // Save to cache + // Save to cache (don't fail if caching fails) entry := cacheEntry{ Data: data, CachedAt: time.Now(), UpdatedAt: updatedAt, } - cacheData, err := json.Marshal(entry) - if err == nil { - if err := os.WriteFile(cacheFile, cacheData, 0o600); err != nil { - log.Printf("Failed to write cache for %s: %v", url, err) + if cacheData, marshalErr := json.Marshal(entry); marshalErr != nil { + log.Printf("Failed to marshal cache data for %s: %v", url, marshalErr) + } else { + // Ensure cache directory exists + if dirErr := os.MkdirAll(filepath.Dir(cacheFile), 0o700); dirErr != nil { + log.Printf("Failed to create cache directory: %v", dirErr) + } else if writeErr := os.WriteFile(cacheFile, cacheData, 0o600); writeErr != nil { + log.Printf("Failed to write cache file for %s: %v", url, writeErr) } } @@ -73,9 +81,11 @@ func (app *App) turnData(ctx context.Context, url string, updatedAt time.Time) ( func (app *App) cleanupOldCache() { entries, err := os.ReadDir(app.cacheDir) if err != nil { + log.Printf("Failed to read cache directory for cleanup: %v", err) return } + var cleanupCount, errorCount int for _, entry := range entries { if !strings.HasSuffix(entry.Name(), ".json") { continue @@ -83,14 +93,24 @@ func (app *App) cleanupOldCache() { info, err := entry.Info() if err != nil { + log.Printf("Failed to get file info for cache entry %s: %v", entry.Name(), err) + errorCount++ continue } // Remove cache files older than 5 days if time.Since(info.ModTime()) > cacheCleanupInterval { - if err := os.Remove(filepath.Join(app.cacheDir, entry.Name())); err != nil { - log.Printf("Failed to remove old cache: %v", err) + filePath := filepath.Join(app.cacheDir, entry.Name()) + if removeErr := os.Remove(filePath); removeErr != nil { + log.Printf("Failed to remove old cache file %s: %v", filePath, removeErr) + errorCount++ + } else { + cleanupCount++ } } } + + if cleanupCount > 0 || errorCount > 0 { + log.Printf("Cache cleanup completed: %d files removed, %d errors", cleanupCount, errorCount) + } } diff --git a/github.go b/github.go index 593fea6..a27a1ec 100644 --- a/github.go +++ b/github.go @@ -11,8 +11,10 @@ import ( "path/filepath" "runtime" "strings" + "sync" "time" + "github.com/codeGROOVE-dev/retry" "github.com/google/go-github/v57/github" "github.com/ready-to-review/turnclient/pkg/turn" "golang.org/x/oauth2" @@ -118,6 +120,7 @@ func (*App) githubToken(ctx context.Context) (string, error) { } // fetchPRs retrieves all PRs involving the current user. +// It returns GitHub data immediately and starts Turn API queries in the background. func (app *App) fetchPRs(ctx context.Context) (incoming []PR, outgoing []PR, err error) { // Use targetUser if specified, otherwise use authenticated user user := app.currentUser.GetLogin() @@ -138,13 +141,40 @@ func (app *App) fetchPRs(ctx context.Context) (incoming []PR, outgoing []PR, err log.Printf("Searching for PRs with query: %s", query) searchStart := time.Now() - // Just try once - GitHub API is reliable enough - result, resp, err := app.client.Search.Issues(ctx, query, opts) + // 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 { - // Check for rate limit - const httpStatusForbidden = 403 - if resp != nil && resp.StatusCode == httpStatusForbidden { - log.Print("GitHub API rate limited") + // 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) + } + 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 + log.Printf("GitHub API network error: %v", err) } return nil, nil, fmt.Errorf("search PRs: %w", err) } @@ -157,10 +187,7 @@ func (app *App) fetchPRs(ctx context.Context) (incoming []PR, outgoing []PR, err result.Issues = result.Issues[:maxPRsToProcess] } - // Process results - turnSuccesses := 0 - turnFailures := 0 - + // Process GitHub results immediately for _, issue := range result.Issues { if !issue.IsPullRequest() { continue @@ -175,28 +202,188 @@ func (app *App) fetchPRs(ctx context.Context) (incoming []PR, outgoing []PR, err UpdatedAt: issue.GetUpdatedAt().Time, } - // Get Turn API data with caching - turnData, err := app.turnData(ctx, issue.GetHTMLURL(), issue.GetUpdatedAt().Time) - if err == nil && turnData != nil && turnData.PRState.UnblockAction != nil { - turnSuccesses++ - if _, exists := turnData.PRState.UnblockAction[user]; exists { - pr.NeedsReview = true - } - } else if err != nil { - turnFailures++ - } - // Categorize as incoming or outgoing // When viewing another user's PRs, we're looking at it from their perspective if issue.GetUser().GetLogin() == user { - pr.IsBlocked = pr.NeedsReview outgoing = append(outgoing, pr) } else { incoming = append(incoming, pr) } } - log.Printf("Found %d incoming, %d outgoing PRs (Turn API: %d/%d succeeded)", - len(incoming), len(outgoing), turnSuccesses, turnSuccesses+turnFailures) + log.Printf("[GITHUB] Found %d incoming, %d outgoing PRs from GitHub", len(incoming), len(outgoing)) + for _, pr := range incoming { + log.Printf("[GITHUB] Incoming PR: %s", pr.URL) + } + for _, pr := range outgoing { + log.Printf("[GITHUB] Outgoing PR: %s", pr.URL) + } + + // Start Turn API queries in background + go app.fetchTurnDataAsync(ctx, result.Issues, user) + return incoming, outgoing, nil } + +// updatePRData updates PR data with Turn API results. +func (app *App) updatePRData(url string, needsReview bool, isOwner bool) *PR { + app.mu.Lock() + defer app.mu.Unlock() + + if isOwner { + // Update outgoing PRs + for i := range app.outgoing { + if app.outgoing[i].URL == url { + app.outgoing[i].NeedsReview = needsReview + app.outgoing[i].IsBlocked = needsReview + return &app.outgoing[i] + } + } + } else { + // Update incoming PRs + for i := range app.incoming { + if app.incoming[i].URL == url { + app.incoming[i].NeedsReview = needsReview + return &app.incoming[i] + } + } + } + return nil +} + +// 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() + + 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() + + // Retry logic for Turn API with exponential backoff and jitter + var turnData *turn.CheckResponse + var err error + + 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) + } + + 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 incrementally + turnSuccesses := 0 + turnFailures := 0 + updatesApplied := 0 + + // Batch updates to reduce menu rebuilds + updateBatch := 0 + const batchSize = 10 + lastUpdateTime := time.Now() + const minUpdateInterval = 500 * time.Millisecond + + for result := range results { + if result.err == nil && result.turnData != nil && result.turnData.PRState.UnblockAction != nil { + turnSuccesses++ + + // Check if user needs to review + needsReview := false + if _, exists := result.turnData.PRState.UnblockAction[user]; exists { + needsReview = true + } + + // Update the PR in our lists + pr := app.updatePRData(result.url, needsReview, result.isOwner) + + if pr != nil { + updatesApplied++ + updateBatch++ + log.Printf("[TURN] Turn data received for %s (needsReview=%v)", result.url, needsReview) + // Update the specific menu item immediately + app.updatePRMenuItem(*pr) + + // Periodically update section headers and 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() + app.setTrayTitle() + updateBatch = 0 + lastUpdateTime = time.Now() + } + } + } else if result.err != nil { + turnFailures++ + } + } + + // 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() +} diff --git a/go.mod b/go.mod index b281eb4..a2bf3ad 100644 --- a/go.mod +++ b/go.mod @@ -3,6 +3,7 @@ module ready-to-review go 1.23.4 require ( + github.com/codeGROOVE-dev/retry v1.2.0 github.com/energye/systray v1.0.2 github.com/gen2brain/beeep v0.11.1 github.com/google/go-github/v57 v57.0.0 diff --git a/go.sum b/go.sum index b5af1a5..b8d3375 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,7 @@ git.sr.ht/~jackmordaunt/go-toast v1.1.2 h1:/yrfI55LRt1M7H1vkaw+NaH1+L1CDxrqDltwm5euVuE= git.sr.ht/~jackmordaunt/go-toast v1.1.2/go.mod h1:jA4OqHKTQ4AFBdwrSnwnskUIIS3HYzlJSgdzCKqfavo= +github.com/codeGROOVE-dev/retry v1.2.0 h1:xYpYPX2PQZmdHwuiQAGGzsBm392xIMl4nfMEFApQnu8= +github.com/codeGROOVE-dev/retry v1.2.0/go.mod h1:8OgefgV1XP7lzX2PdKlCXILsYKuz6b4ZpHa/20iLi8E= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= diff --git a/main.go b/main.go index e8dbc82..c4c0763 100644 --- a/main.go +++ b/main.go @@ -11,6 +11,7 @@ import ( "log" "os" "path/filepath" + "strings" "sync" "time" @@ -54,20 +55,25 @@ 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 - targetUser string // User to query PRs for (overrides currentUser if set) previousBlockedPRs map[string]bool client *github.Client + sectionHeaders map[string]*systray.MenuItem + targetUser string cacheDir string - lastMenuHash string incoming []PR - menuItems []*systray.MenuItem outgoing []PR + menuItems []*systray.MenuItem + lastMenuHashInt uint64 consecutiveFailures int mu sync.RWMutex hideStaleIncoming bool initialLoadComplete bool + turnDataLoading bool + turnDataLoaded bool + menuInitialized bool } func main() { @@ -96,6 +102,8 @@ func main() { hideStaleIncoming: true, previousBlockedPRs: make(map[string]bool), targetUser: targetUser, + prMenuItems: make(map[string]*systray.MenuItem), + sectionHeaders: make(map[string]*systray.MenuItem), } log.Println("Initializing GitHub clients...") @@ -160,7 +168,7 @@ func (app *App) onReady(ctx context.Context) { // Create initial menu log.Println("Creating initial menu") - app.updateMenu(ctx) + app.initializeMenu(ctx) // Clean old cache on startup app.cleanupOldCache() @@ -174,8 +182,20 @@ func (app *App) updateLoop(ctx context.Context) { defer func() { if r := recover(); r != nil { log.Printf("PANIC in update loop: %v", r) - // Restart the update loop after a delay - time.Sleep(10 * time.Second) + + // Set error state in UI + systray.SetTitle("💥") + systray.SetTooltip("GitHub PR Monitor - Critical error, restarting...") + + // Update failure count + app.mu.Lock() + app.consecutiveFailures += 5 // Treat panic as multiple failures + 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) } }() @@ -204,27 +224,59 @@ func (app *App) updatePRs(ctx context.Context) { log.Printf("Error fetching PRs: %v", err) app.mu.Lock() app.consecutiveFailures++ + failureCount := app.consecutiveFailures app.mu.Unlock() - // Show different icon based on failure count - if app.consecutiveFailures > 3 { - systray.SetTitle("❌") // Complete failure - } else { - systray.SetTitle("⚠️") // Warning + // 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" } - // Include time since last success in tooltip + systray.SetTitle(title) + + // Include time since last success and user info timeSinceSuccess := "never" if !app.lastSuccessfulFetch.IsZero() { timeSinceSuccess = time.Since(app.lastSuccessfulFetch).Round(time.Minute).String() } - // Include user in error tooltip userInfo := "" if app.targetUser != "" { userInfo = fmt.Sprintf(" - @%s", app.targetUser) } - systray.SetTooltip(fmt.Sprintf("GitHub PR Monitor%s - Error: %v\nLast success: %s ago", userInfo, err, timeSinceSuccess)) + + // Provide actionable error message based on error type + var errorHint string + errMsg := err.Error() + switch { + case strings.Contains(errMsg, "rate limited"): + errorHint = "\nRate limited - wait before retrying" + case strings.Contains(errMsg, "authentication"): + errorHint = "\nCheck GitHub token with 'gh auth status'" + case strings.Contains(errMsg, "network"): + errorHint = "\nCheck internet connection" + default: + // No specific hint for this error type + } + + fullTooltip := fmt.Sprintf("%s%s\nLast success: %s ago%s", tooltip, userInfo, timeSinceSuccess, errorHint) + systray.SetTooltip(fullTooltip) return } @@ -286,18 +338,6 @@ func (app *App) updatePRs(ctx context.Context) { app.outgoing = outgoing app.mu.Unlock() - // Set title based on PR state - switch { - case incomingBlocked == 0 && outgoingBlocked == 0: - systray.SetTitle("😊") - case incomingBlocked > 0 && outgoingBlocked > 0: - systray.SetTitle(fmt.Sprintf("🕵️ %d / 🚀 %d", incomingBlocked, outgoingBlocked)) - case incomingBlocked > 0: - systray.SetTitle(fmt.Sprintf("🕵️ %d", incomingBlocked)) - default: - systray.SetTitle(fmt.Sprintf("🚀 %d", outgoingBlocked)) - } - app.updateMenuIfChanged(ctx) // Mark initial load as complete after first successful update @@ -310,20 +350,63 @@ func (app *App) updatePRs(ctx context.Context) { // isStale returns true if the PR hasn't been updated in over 90 days. func isStale(updatedAt time.Time) bool { - return time.Since(updatedAt) > stalePRThreshold + return updatedAt.Before(time.Now().Add(-stalePRThreshold)) } // updateMenuIfChanged only rebuilds the menu if the PR data has actually changed. func (app *App) updateMenuIfChanged(ctx context.Context) { app.mu.RLock() - // Simple hash: just count of PRs and hideStale setting - currentHash := fmt.Sprintf("%d-%d-%t", len(app.incoming), len(app.outgoing), app.hideStaleIncoming) + // Calculate hash including blocking status - use efficient integer hash + var incomingBlocked, outgoingBlocked int + for i := range app.incoming { + if app.incoming[i].NeedsReview { + incomingBlocked++ + } + } + for i := range app.outgoing { + if app.outgoing[i].IsBlocked { + outgoingBlocked++ + } + } + + // Build hash as integers to avoid string allocation + const ( + hashPartsCount = 6 + hideStaleIndex = 4 + turnLoadingIndex = 5 + bitsPerHashPart = 8 + ) + hashParts := [hashPartsCount]int{ + len(app.incoming), + len(app.outgoing), + 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 + for i, part := range hashParts { + // Safe conversion since we control the values + if part >= 0 { + currentHashInt ^= uint64(part) << (i * bitsPerHashPart) + } + } app.mu.RUnlock() - if currentHash == app.lastMenuHash { + if currentHashInt == app.lastMenuHashInt { + log.Printf("[MENU] Menu hash unchanged (%d), skipping update", currentHashInt) return } - app.lastMenuHash = currentHash + log.Printf("[MENU] Menu hash changed from %d to %d, updating menu", app.lastMenuHashInt, currentHashInt) + app.lastMenuHashInt = currentHashInt app.updateMenu(ctx) } diff --git a/main_test.go b/main_test.go index ee05a9e..3927ad7 100644 --- a/main_test.go +++ b/main_test.go @@ -7,8 +7,8 @@ import ( func TestIsStale(t *testing.T) { tests := []struct { - name string time time.Time + name string expected bool }{ { diff --git a/ui.go b/ui.go index e67368b..754d557 100644 --- a/ui.go +++ b/ui.go @@ -114,8 +114,12 @@ func (app *App) countPRs() (int, int, int, int) { var incomingCount, incomingBlocked, outgoingCount, outgoingBlocked int + // Pre-calculate stale threshold to avoid repeated time calculations + now := time.Now() + staleThreshold := now.Add(-stalePRThreshold) + for i := range app.incoming { - if !app.hideStaleIncoming || !isStale(app.incoming[i].UpdatedAt) { + if !app.hideStaleIncoming || app.incoming[i].UpdatedAt.After(staleThreshold) { incomingCount++ if app.incoming[i].NeedsReview { incomingBlocked++ @@ -124,7 +128,7 @@ func (app *App) countPRs() (int, int, int, int) { } for i := range app.outgoing { - if !app.hideStaleIncoming || !isStale(app.outgoing[i].UpdatedAt) { + if !app.hideStaleIncoming || app.outgoing[i].UpdatedAt.After(staleThreshold) { outgoingCount++ if app.outgoing[i].IsBlocked { outgoingBlocked++ @@ -134,7 +138,74 @@ 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. +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: + systray.SetTitle("😊") + case incomingBlocked > 0 && outgoingBlocked > 0: + systray.SetTitle(fmt.Sprintf("🕵️ %d / 🚀 %d", incomingBlocked, outgoingBlocked)) + case incomingBlocked > 0: + systray.SetTitle(fmt.Sprintf("🕵️ %d", incomingBlocked)) + default: + systray.SetTitle(fmt.Sprintf("🚀 %d", outgoingBlocked)) + } +} + +// 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) + } + log.Printf("[MENU] Updating PR menu item for %s: '%s' -> '%s'", pr.URL, oldTitle, title) + item.SetTitle(title) + } 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, _ bool) { title := fmt.Sprintf("%s #%d", pr.Repository, pr.Number) // Add bullet point for PRs where user is blocking @@ -142,8 +213,22 @@ func (app *App) addPRMenuItem(ctx context.Context, pr PR, _ bool) { title = fmt.Sprintf("• %s", title) } tooltip := fmt.Sprintf("%s (%s)", pr.Title, formatAge(pr.UpdatedAt)) + + // 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 and context properly to avoid loop variable capture bug item.Click(func(capturedCtx context.Context, url string) func() { @@ -162,11 +247,34 @@ func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string, } // Add header - header := systray.AddMenuItem(fmt.Sprintf("%s — %d blocked on you", sectionTitle, blockedCount), "") - header.Disable() - app.menuItems = append(app.menuItems, header) + var headerText string + app.mu.RLock() + loading := app.turnDataLoading + app.mu.RUnlock() + + if loading && !app.turnDataLoaded { + headerText = fmt.Sprintf("%s — blocked on you", sectionTitle) + } else { + headerText = fmt.Sprintf("%s — %d blocked on you", sectionTitle, blockedCount) + } - // Add PR items (already protected by mutex in caller) + // 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 + } + + // Add PR items (mutex already held) for i := range prs { // Apply filters if app.hideStaleIncoming && isStale(prs[i].UpdatedAt) { @@ -174,31 +282,107 @@ func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string, } app.addPRMenuItem(ctx, prs[i], isOutgoing) } + + app.mu.Unlock() +} + +// initializeMenu creates the initial menu structure with static items. +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) + + app.menuInitialized = true + log.Print("[MENU] Menu initialization complete") } -// updateMenu rebuilds the system tray menu. +// 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() - log.Printf("Updating menu with %d incoming and %d outgoing PRs", incomingLen, outgoingLen) + log.Printf("[MENU] Updating menu with %d incoming and %d outgoing PRs", incomingLen, outgoingLen) - // Store the current menu items to clean up later - oldMenuItems := app.menuItems - app.menuItems = nil + // 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) + } + } + app.mu.Unlock() // Calculate counts first incomingCount, incomingBlocked, outgoingCount, outgoingBlocked := app.countPRs() - // Show "No pull requests" if both lists are empty + // Handle "No pull requests" item + const noPRsKey = "__no_prs__" + app.mu.Lock() if incomingLen == 0 && outgoingLen == 0 { - noPRs := systray.AddMenuItem("No pull requests", "") - noPRs.Disable() - app.menuItems = append(app.menuItems, noPRs) - systray.AddSeparator() + 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 + } + } 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() + } } + app.mu.Unlock() // Incoming section if incomingCount > 0 { @@ -212,21 +396,17 @@ func (app *App) updateMenu(ctx context.Context) { app.addPRSection(ctx, app.outgoing, "Outgoing", outgoingBlocked, true) } - // Add static menu items - app.addStaticMenuItems(ctx) - - // Now hide old menu items after new ones are created - // This prevents the flicker by ensuring new items exist before old ones disappear - for _, item := range oldMenuItems { - item.Hide() - } + log.Print("[MENU] Menu update complete") } // addStaticMenuItems adds the static menu items (hide stale, dashboard, about, quit). func (app *App) addStaticMenuItems(ctx context.Context) { + log.Print("[MENU] Adding static menu items") + systray.AddSeparator() // 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 { @@ -241,13 +421,14 @@ func (app *App) addStaticMenuItems(ctx context.Context) { } log.Printf("Hide stale PRs: %v", app.hideStaleIncoming) // Force menu rebuild since hideStaleIncoming changed - app.lastMenuHash = "" + app.lastMenuHashInt = 0 app.updateMenu(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() { @@ -261,6 +442,7 @@ func (app *App) addStaticMenuItems(ctx context.Context) { 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() { @@ -276,6 +458,7 @@ func (app *App) addStaticMenuItems(ctx context.Context) { addLoginItemUI(ctx, app) // Quit + log.Print("[MENU] Adding 'Quit' menu item") quitItem := systray.AddMenuItem("Quit", "") app.menuItems = append(app.menuItems, quitItem) quitItem.Click(func() {