From 1e3e44251eafa2722ee8c79b1cb1a4dccbdc998c Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Tue, 5 Aug 2025 20:36:36 -0400 Subject: [PATCH 1/2] finally fix the excessive menu rebuilds --- cache.go | 8 ++-- github.go | 133 ++++++++++++++++++++++++++++++++++++++---------------- main.go | 74 ++++++++++++++---------------- 3 files changed, 131 insertions(+), 84 deletions(-) diff --git a/cache.go b/cache.go index 88b5390..b452286 100644 --- a/cache.go +++ b/cache.go @@ -24,7 +24,7 @@ type cacheEntry struct { } // turnData fetches Turn API data with caching. -func (app *App) turnData(ctx context.Context, url string, updatedAt time.Time) (*turn.CheckResponse, error) { +func (app *App) turnData(ctx context.Context, url string, updatedAt time.Time) (*turn.CheckResponse, bool, error) { // Create cache key from URL and updated timestamp key := fmt.Sprintf("%s-%s", url, updatedAt.Format(time.RFC3339)) hash := sha256.Sum256([]byte(key)) @@ -43,7 +43,7 @@ func (app *App) turnData(ctx context.Context, url string, updatedAt time.Time) ( } } else if time.Since(entry.CachedAt) < cacheTTL && entry.UpdatedAt.Equal(updatedAt) { // Check if cache is still valid (2 hour TTL) - return entry.Data, nil + return entry.Data, true, nil } } } @@ -80,7 +80,7 @@ func (app *App) turnData(ctx context.Context, url string, updatedAt time.Time) ( ) if err != nil { log.Printf("Turn API error after %d retries (will use PR without metadata): %v", maxRetries, err) - return nil, err + return nil, false, err } // Save to cache (don't fail if caching fails) - skip if --no-cache is set @@ -102,7 +102,7 @@ func (app *App) turnData(ctx context.Context, url string, updatedAt time.Time) ( } } - return data, nil + return data, false, nil } // cleanupOldCache removes cache files older than 5 days. diff --git a/github.go b/github.go index 6c924fa..d647fbf 100644 --- a/github.go +++ b/github.go @@ -230,11 +230,11 @@ 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)) - for _, pr := range incoming { - log.Printf("[GITHUB] Incoming PR: %s", pr.URL) + for i := range incoming { + log.Printf("[GITHUB] Incoming PR: %s", incoming[i].URL) } - for _, pr := range outgoing { - log.Printf("[GITHUB] Outgoing PR: %s", pr.URL) + for i := range outgoing { + log.Printf("[GITHUB] Outgoing PR: %s", outgoing[i].URL) } // Fetch Turn API data @@ -255,41 +255,63 @@ func (app *App) fetchPRsInternal(ctx context.Context, waitForTurn bool) (incomin } // updatePRData updates PR data with Turn API results. -func (app *App) updatePRData(url string, needsReview bool, isOwner bool, actionReason string) *PR { +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 { - app.outgoing[i].NeedsReview = needsReview - app.outgoing[i].IsBlocked = needsReview - app.outgoing[i].ActionReason = actionReason - return &app.outgoing[i] + 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 { - app.incoming[i].NeedsReview = needsReview - app.incoming[i].ActionReason = actionReason - return &app.incoming[i] + 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 + 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() type prResult struct { - err error - turnData *turn.CheckResponse - url string - isOwner bool + err error + turnData *turn.CheckResponse + url string + isOwner bool + wasFromCache bool } // Create a channel for results @@ -312,13 +334,14 @@ func (app *App) fetchTurnDataSync(ctx context.Context, issues []*github.Issue, u updatedAt := issue.GetUpdatedAt().Time // Call turnData - it now has proper exponential backoff with jitter - turnData, err := app.turnData(ctx, url, updatedAt) + turnData, wasFromCache, err := app.turnData(ctx, url, updatedAt) results <- prResult{ - url: issue.GetHTMLURL(), - turnData: turnData, - err: err, - isOwner: issue.GetUser().GetLogin() == user, + url: issue.GetHTMLURL(), + turnData: turnData, + err: err, + isOwner: issue.GetUser().GetLogin() == user, + wasFromCache: wasFromCache, } }(issue) } @@ -381,10 +404,11 @@ func (app *App) fetchTurnDataAsync(ctx context.Context, issues []*github.Issue, turnStart := time.Now() type prResult struct { - err error - turnData *turn.CheckResponse - url string - isOwner bool + err error + turnData *turn.CheckResponse + url string + isOwner bool + wasFromCache bool } // Create a channel for results @@ -407,13 +431,14 @@ func (app *App) fetchTurnDataAsync(ctx context.Context, issues []*github.Issue, updatedAt := issue.GetUpdatedAt().Time // Call turnData - it now has proper exponential backoff with jitter - turnData, err := app.turnData(ctx, url, updatedAt) + turnData, wasFromCache, err := app.turnData(ctx, url, updatedAt) results <- prResult{ - url: issue.GetHTMLURL(), - turnData: turnData, - err: err, - isOwner: issue.GetUser().GetLogin() == user, + url: issue.GetHTMLURL(), + turnData: turnData, + err: err, + isOwner: issue.GetUser().GetLogin() == user, + wasFromCache: wasFromCache, } }(issue) } @@ -442,7 +467,8 @@ func (app *App) fetchTurnDataAsync(ctx context.Context, issues []*github.Issue, needsReview = true actionReason = action.Reason log.Printf("[TURN] UnblockAction for %s: Reason=%q, Kind=%q", result.url, action.Reason, action.Kind) - } else { + } else if !result.wasFromCache { + // Only log "no action" for fresh API results, not cached ones log.Printf("[TURN] No UnblockAction found for user %s on %s", user, result.url) } @@ -452,6 +478,7 @@ func (app *App) fetchTurnDataAsync(ctx context.Context, issues []*github.Issue, NeedsReview: needsReview, IsOwner: result.isOwner, ActionReason: actionReason, + WasFromCache: result.wasFromCache, } app.mu.Lock() @@ -459,7 +486,14 @@ func (app *App) fetchTurnDataAsync(ctx context.Context, issues []*github.Issue, app.mu.Unlock() updatesApplied++ - log.Printf("[TURN] Turn data received for %s (needsReview=%v, actionReason=%q) - buffered", result.url, needsReview, actionReason) + // Reduce verbosity - only log if not from cache or if blocked + if !result.wasFromCache || needsReview { + cacheStatus := "fresh" + if result.wasFromCache { + cacheStatus = "cached" + } + log.Printf("[TURN] %s data for %s (needsReview=%v, actionReason=%q)", cacheStatus, result.url, needsReview, actionReason) + } } else if result.err != nil { turnFailures++ } @@ -475,15 +509,36 @@ func (app *App) fetchTurnDataAsync(ctx context.Context, issues []*github.Issue, app.loadingTurnData = false app.mu.Unlock() - log.Printf("[TURN] Applying %d buffered Turn results", len(pendingResults)) + // Check if any results came from fresh API calls (not cache) + var cacheHits, freshResults int for _, result := range pendingResults { - app.updatePRData(result.URL, result.NeedsReview, result.IsOwner, result.ActionReason) + if result.WasFromCache { + cacheHits++ + } else { + freshResults++ + } + } + + 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++ + } } // Update tray title and menu with final Turn data if menu is already initialized app.setTrayTitle() if app.menuInitialized { - log.Print("[TURN] Turn data loaded, checking if menu needs update") - app.updateMenuIfChanged(ctx) + // Only trigger menu update if PR data actually changed + if actualChanges > 0 { + log.Printf("[TURN] Turn data applied - %d PRs actually changed, checking if menu needs update", actualChanges) + app.updateMenuIfChanged(ctx) + } else { + log.Print("[TURN] Turn data applied - no PR changes detected (cached data unchanged), skipping menu update") + } } } diff --git a/main.go b/main.go index 1bcd6d9..c156933 100644 --- a/main.go +++ b/main.go @@ -51,14 +51,15 @@ const ( // PR represents a pull request with metadata. type PR struct { - UpdatedAt time.Time - Title string - URL string - Repository string - ActionReason string // Action reason from Turn API when blocked - Number int - IsBlocked bool - NeedsReview bool + UpdatedAt time.Time + TurnDataAppliedAt time.Time + Title string + URL string + Repository string + ActionReason string + Number int + IsBlocked bool + NeedsReview bool } // MenuState represents the current state of menu items for comparison. @@ -85,6 +86,7 @@ type TurnResult struct { ActionReason string NeedsReview bool IsOwner bool + WasFromCache bool // Track if this result came from cache } // App holds the application state. @@ -427,15 +429,15 @@ func (app *App) buildCurrentMenuState() *MenuState { now := time.Now() staleThreshold := now.Add(-stalePRThreshold) - for _, pr := range app.incoming { - if !app.hideStaleIncoming || pr.UpdatedAt.After(staleThreshold) { - filteredIncoming = append(filteredIncoming, pr) + for i := range app.incoming { + if !app.hideStaleIncoming || app.incoming[i].UpdatedAt.After(staleThreshold) { + filteredIncoming = append(filteredIncoming, app.incoming[i]) } } - for _, pr := range app.outgoing { - if !app.hideStaleIncoming || pr.UpdatedAt.After(staleThreshold) { - filteredOutgoing = append(filteredOutgoing, pr) + for i := range app.outgoing { + if !app.hideStaleIncoming || app.outgoing[i].UpdatedAt.After(staleThreshold) { + filteredOutgoing = append(filteredOutgoing, app.outgoing[i]) } } @@ -445,28 +447,28 @@ func (app *App) buildCurrentMenuState() *MenuState { // Build menu item states incomingItems := make([]MenuItemState, len(incomingSorted)) - for i, pr := range incomingSorted { + for i := range incomingSorted { incomingItems[i] = MenuItemState{ - URL: pr.URL, - Title: pr.Title, - Repository: pr.Repository, - Number: pr.Number, - NeedsReview: pr.NeedsReview, + URL: incomingSorted[i].URL, + Title: incomingSorted[i].Title, + Repository: incomingSorted[i].Repository, + Number: incomingSorted[i].Number, + NeedsReview: incomingSorted[i].NeedsReview, IsBlocked: false, // incoming PRs don't use IsBlocked - ActionReason: pr.ActionReason, + ActionReason: incomingSorted[i].ActionReason, } } outgoingItems := make([]MenuItemState, len(outgoingSorted)) - for i, pr := range outgoingSorted { + for i := range outgoingSorted { outgoingItems[i] = MenuItemState{ - URL: pr.URL, - Title: pr.Title, - Repository: pr.Repository, - Number: pr.Number, - NeedsReview: pr.NeedsReview, - IsBlocked: pr.IsBlocked, - ActionReason: pr.ActionReason, + URL: outgoingSorted[i].URL, + Title: outgoingSorted[i].Title, + Repository: outgoingSorted[i].Repository, + Number: outgoingSorted[i].Number, + NeedsReview: outgoingSorted[i].NeedsReview, + IsBlocked: outgoingSorted[i].IsBlocked, + ActionReason: outgoingSorted[i].ActionReason, } } @@ -482,18 +484,8 @@ func (app *App) updateMenuIfChanged(ctx context.Context) { app.mu.RLock() // Skip menu updates while Turn data is still loading to avoid excessive rebuilds if app.loadingTurnData { - // But still save the current state for future comparisons - currentMenuState := app.buildCurrentMenuState() - if app.lastMenuState == nil { - log.Print("[MENU] Skipping menu update - Turn data still loading, but saving state for future comparison") - app.mu.RUnlock() - app.mu.Lock() - app.lastMenuState = currentMenuState - app.mu.Unlock() - } else { - app.mu.RUnlock() - log.Print("[MENU] Skipping menu update - Turn data still loading") - } + app.mu.RUnlock() + log.Print("[MENU] Skipping menu update - Turn data still loading") return } log.Print("[MENU] *** updateMenuIfChanged called - calculating diff ***") From 4a1ff356cccb73d3451ea97eaa2f655dd6a178e8 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Tue, 5 Aug 2025 20:59:11 -0400 Subject: [PATCH 2/2] fix lint error --- loginitem_darwin.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/loginitem_darwin.go b/loginitem_darwin.go index c088e02..3fe377d 100644 --- a/loginitem_darwin.go +++ b/loginitem_darwin.go @@ -36,7 +36,7 @@ func validateAndEscapePathForAppleScript(path string) string { // isLoginItem checks if the app is set to start at login. func isLoginItem(ctx context.Context) bool { - appPath, err := getAppPath() + appPath, err := appPath() if err != nil { log.Printf("Failed to get app path: %v", err) return false @@ -67,7 +67,7 @@ func isLoginItem(ctx context.Context) bool { // setLoginItem adds or removes the app from login items. func setLoginItem(ctx context.Context, enable bool) error { - appPath, err := getAppPath() + appPath, err := appPath() if err != nil { return fmt.Errorf("get app path: %w", err) } @@ -131,8 +131,8 @@ func isRunningFromAppBundle() bool { return strings.Contains(execPath, ".app/Contents/MacOS/") } -// getAppPath returns the path to the application bundle. -func getAppPath() (string, error) { +// appPath returns the path to the application bundle. +func appPath() (string, error) { // Get the executable path execPath, err := os.Executable() if err != nil {