Skip to content

Commit 1150dd0

Browse files
tstrombergtstromberg
committed
lint cleanup
1 parent 23426bf commit 1150dd0

File tree

5 files changed

+121
-93
lines changed

5 files changed

+121
-93
lines changed

‎cache.go‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"strings"
1414
"time"
1515

16+
"github.com/codeGROOVE-dev/retry"
1617
"github.com/ready-to-review/turnclient/pkg/turn"
1718
)
1819

@@ -54,13 +55,31 @@ func (app *App) turnData(ctx context.Context, url string, updatedAt time.Time) (
5455
log.Printf("Cache miss for %s, fetching from Turn API", url)
5556
}
5657

57-
// Just try once with timeout - if Turn API fails, it's not critical
58-
turnCtx, cancel := context.WithTimeout(ctx, 10*time.Second)
59-
defer cancel()
58+
// Use exponential backoff with jitter for Turn API calls
59+
var data *turn.CheckResponse
60+
err := retry.Do(func() error {
61+
// Create timeout context for Turn API call
62+
turnCtx, cancel := context.WithTimeout(ctx, 10*time.Second)
63+
defer cancel()
6064

61-
data, err := app.turnClient.Check(turnCtx, url, app.currentUser.GetLogin(), updatedAt)
65+
var retryErr error
66+
data, retryErr = app.turnClient.Check(turnCtx, url, app.currentUser.GetLogin(), updatedAt)
67+
if retryErr != nil {
68+
log.Printf("Turn API error (will retry): %v", retryErr)
69+
return retryErr
70+
}
71+
return nil
72+
},
73+
retry.Attempts(maxRetries),
74+
retry.DelayType(retry.BackOffDelay),
75+
retry.MaxDelay(maxRetryDelay),
76+
retry.OnRetry(func(n uint, err error) {
77+
log.Printf("Turn API retry %d/%d for %s: %v", n+1, maxRetries, url, err)
78+
}),
79+
retry.Context(ctx),
80+
)
6281
if err != nil {
63-
log.Printf("Turn API error (will use PR without metadata): %v", err)
82+
log.Printf("Turn API error after %d retries (will use PR without metadata): %v", maxRetries, err)
6483
return nil, err
6584
}
6685

‎github.go‎

Lines changed: 55 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ func (*App) githubToken(ctx context.Context) (string, error) {
9797
}
9898

9999
if ghPath == "" {
100-
return "", errors.New("gh cli not found in trusted locations, please install from https://cli.github.com")
100+
return "", errors.New("gh cli not found in trusted locations")
101101
}
102102

103103
log.Printf("Executing command: %s auth token", ghPath)
@@ -141,42 +141,59 @@ func (app *App) fetchPRs(ctx context.Context) (incoming []PR, outgoing []PR, err
141141
log.Printf("Searching for PRs with query: %s", query)
142142
searchStart := time.Now()
143143

144-
// Create timeout context for GitHub API call
145-
githubCtx, cancel := context.WithTimeout(ctx, 30*time.Second)
146-
defer cancel()
147-
148-
result, resp, err := app.client.Search.Issues(githubCtx, query, opts)
149-
if err != nil {
150-
// Enhanced error handling with specific cases
151-
if resp != nil {
152-
const (
153-
httpStatusUnauthorized = 401
154-
httpStatusForbidden = 403
155-
httpStatusUnprocessable = 422
156-
)
157-
switch resp.StatusCode {
158-
case httpStatusForbidden:
159-
if resp.Header.Get("X-Ratelimit-Remaining") == "0" {
160-
resetTime := resp.Header.Get("X-Ratelimit-Reset")
161-
log.Printf("GitHub API rate limited, reset at: %s", resetTime)
162-
return nil, nil, fmt.Errorf("github API rate limited, try again later: %w", err)
144+
var result *github.IssuesSearchResult
145+
var resp *github.Response
146+
err = retry.Do(func() error {
147+
// Create timeout context for GitHub API call
148+
githubCtx, cancel := context.WithTimeout(ctx, 30*time.Second)
149+
defer cancel()
150+
151+
var retryErr error
152+
result, resp, retryErr = app.client.Search.Issues(githubCtx, query, opts)
153+
if retryErr != nil {
154+
// Enhanced error handling with specific cases
155+
if resp != nil {
156+
const (
157+
httpStatusUnauthorized = 401
158+
httpStatusForbidden = 403
159+
httpStatusUnprocessable = 422
160+
)
161+
switch resp.StatusCode {
162+
case httpStatusForbidden:
163+
if resp.Header.Get("X-Ratelimit-Remaining") == "0" {
164+
resetTime := resp.Header.Get("X-Ratelimit-Reset")
165+
log.Printf("GitHub API rate limited, reset at: %s (will retry)", resetTime)
166+
return retryErr // Retry on rate limit
167+
}
168+
log.Print("GitHub API access forbidden (check token permissions)")
169+
return retry.Unrecoverable(fmt.Errorf("github API access forbidden: %w", retryErr))
170+
case httpStatusUnauthorized:
171+
log.Print("GitHub API authentication failed (check token)")
172+
return retry.Unrecoverable(fmt.Errorf("github API authentication failed: %w", retryErr))
173+
case httpStatusUnprocessable:
174+
log.Printf("GitHub API query invalid: %s", query)
175+
return retry.Unrecoverable(fmt.Errorf("github API query invalid: %w", retryErr))
176+
default:
177+
log.Printf("GitHub API error (status %d): %v (will retry)", resp.StatusCode, retryErr)
163178
}
164-
log.Print("GitHub API access forbidden (check token permissions)")
165-
return nil, nil, fmt.Errorf("github API access forbidden: %w", err)
166-
case httpStatusUnauthorized:
167-
log.Print("GitHub API authentication failed (check token)")
168-
return nil, nil, fmt.Errorf("github API authentication failed: %w", err)
169-
case httpStatusUnprocessable:
170-
log.Printf("GitHub API query invalid: %s", query)
171-
return nil, nil, fmt.Errorf("github API query invalid: %w", err)
172-
default:
173-
log.Printf("GitHub API error (status %d): %v", resp.StatusCode, err)
179+
} else {
180+
// Likely network error - retry these
181+
log.Printf("GitHub API network error: %v (will retry)", retryErr)
174182
}
175-
} else {
176-
// Likely network error
177-
log.Printf("GitHub API network error: %v", err)
183+
return retryErr
178184
}
179-
return nil, nil, fmt.Errorf("search PRs: %w", err)
185+
return nil
186+
},
187+
retry.Attempts(maxRetries),
188+
retry.DelayType(retry.BackOffDelay),
189+
retry.MaxDelay(maxRetryDelay),
190+
retry.OnRetry(func(n uint, err error) {
191+
log.Printf("GitHub Search.Issues retry %d/%d: %v", n+1, maxRetries, err)
192+
}),
193+
retry.Context(ctx),
194+
)
195+
if err != nil {
196+
return nil, nil, fmt.Errorf("search PRs after %d retries: %w", maxRetries, err)
180197
}
181198

182199
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,
288305
go func(issue *github.Issue) {
289306
defer wg.Done()
290307

291-
// Retry logic for Turn API with exponential backoff and jitter
292-
var turnData *turn.CheckResponse
293-
var err error
308+
url := issue.GetHTMLURL()
309+
updatedAt := issue.GetUpdatedAt().Time
294310

295-
turnData, err = retry.DoWithData(
296-
func() (*turn.CheckResponse, error) {
297-
data, apiErr := app.turnData(ctx, issue.GetHTMLURL(), issue.GetUpdatedAt().Time)
298-
if apiErr != nil {
299-
log.Printf("Turn API attempt failed for %s: %v", issue.GetHTMLURL(), apiErr)
300-
}
301-
return data, apiErr
302-
},
303-
retry.Context(ctx),
304-
retry.Attempts(5), // 5 attempts max
305-
retry.Delay(500*time.Millisecond), // Start with 500ms
306-
retry.MaxDelay(30*time.Second), // Cap at 30 seconds
307-
retry.DelayType(retry.FullJitterBackoffDelay), // Exponential backoff with jitter
308-
retry.OnRetry(func(attempt uint, err error) {
309-
log.Printf("Turn API retry attempt %d for %s: %v", attempt, issue.GetHTMLURL(), err)
310-
}),
311-
)
312-
if err != nil {
313-
log.Printf("Turn API failed after all retries for %s: %v", issue.GetHTMLURL(), err)
314-
}
311+
// Call turnData - it now has proper exponential backoff with jitter
312+
turnData, err := app.turnData(ctx, url, updatedAt)
315313

316314
results <- prResult{
317315
url: issue.GetHTMLURL(),
@@ -340,22 +338,9 @@ func (app *App) fetchTurnDataAsync(ctx context.Context, issues []*github.Issue,
340338
const minUpdateInterval = 500 * time.Millisecond
341339

342340
for result := range results {
343-
// Debug logging for PR #1203 - check all responses
344-
if strings.Contains(result.url, "1203") {
345-
log.Printf("[TURN] DEBUG PR #1203: result.err=%v, turnData=%v", result.err, result.turnData != nil)
346-
if result.turnData != nil {
347-
log.Printf("[TURN] DEBUG PR #1203: PRState.UnblockAction=%v", result.turnData.PRState.UnblockAction != nil)
348-
}
349-
}
350-
351341
if result.err == nil && result.turnData != nil && result.turnData.PRState.UnblockAction != nil {
352342
turnSuccesses++
353343

354-
// Debug logging for PR #1203
355-
if strings.Contains(result.url, "1203") {
356-
log.Printf("[TURN] DEBUG PR #1203: UnblockAction keys: %+v", result.turnData.PRState.UnblockAction)
357-
}
358-
359344
// Check if user needs to review and get action reason
360345
needsReview := false
361346
actionReason := ""

‎main.go‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
"sync"
1616
"time"
1717

18+
"github.com/codeGROOVE-dev/retry"
1819
"github.com/energye/systray"
1920
"github.com/gen2brain/beeep"
2021
"github.com/google/go-github/v57/github"
@@ -39,6 +40,10 @@ const (
3940

4041
// Update intervals.
4142
updateInterval = 5 * time.Minute // Reduced frequency to avoid rate limits
43+
44+
// Retry settings for external API calls - exponential backoff with jitter up to 2 minutes.
45+
maxRetryDelay = 2 * time.Minute
46+
maxRetries = 10 // Should reach 2 minutes with exponential backoff
4247
)
4348

4449
// PR represents a pull request with metadata.
@@ -88,6 +93,7 @@ func main() {
8893

8994
log.SetFlags(log.LstdFlags | log.Lshortfile)
9095
log.Printf("Starting GitHub PR Monitor (version=%s, commit=%s, date=%s)", version, commit, date)
96+
log.Printf("Retry configuration: max_retries=%d, max_delay=%v", maxRetries, maxRetryDelay)
9197

9298
ctx := context.Background()
9399

@@ -118,9 +124,26 @@ func main() {
118124
}
119125

120126
log.Println("Loading current user...")
121-
user, _, err := app.client.Users.Get(ctx, "")
127+
var user *github.User
128+
err = retry.Do(func() error {
129+
var retryErr error
130+
user, _, retryErr = app.client.Users.Get(ctx, "")
131+
if retryErr != nil {
132+
log.Printf("GitHub Users.Get failed (will retry): %v", retryErr)
133+
return retryErr
134+
}
135+
return nil
136+
},
137+
retry.Attempts(maxRetries),
138+
retry.DelayType(retry.BackOffDelay),
139+
retry.MaxDelay(maxRetryDelay),
140+
retry.OnRetry(func(n uint, err error) {
141+
log.Printf("GitHub Users.Get retry %d/%d: %v", n+1, maxRetries, err)
142+
}),
143+
retry.Context(ctx),
144+
)
122145
if err != nil {
123-
log.Fatalf("Failed to load current user: %v", err)
146+
log.Fatalf("Failed to load current user after %d retries: %v", maxRetries, err)
124147
}
125148
app.currentUser = user
126149

‎sound.go‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,13 @@ package main
44
import (
55
"context"
66
_ "embed"
7+
"fmt"
78
"log"
89
"os"
910
"os/exec"
1011
"path/filepath"
1112
"runtime"
13+
"strings"
1214
"sync"
1315
"time"
1416
)
@@ -84,8 +86,10 @@ func (app *App) playSound(ctx context.Context, soundType string) {
8486
case "darwin":
8587
cmd = exec.CommandContext(soundCtx, "afplay", soundPath)
8688
case "windows":
87-
// Use PowerShell's SoundPlayer
88-
script := `(New-Object Media.SoundPlayer "` + soundPath + `").PlaySync()`
89+
// Use PowerShell's SoundPlayer with proper escaping
90+
//nolint:gocritic // Need literal quotes in PowerShell script
91+
script := fmt.Sprintf(`(New-Object Media.SoundPlayer "%s").PlaySync()`,
92+
strings.ReplaceAll(soundPath, `"`, `""`))
8993
cmd = exec.CommandContext(soundCtx, "powershell", "-WindowStyle", "Hidden", "-c", script)
9094
case "linux":
9195
// Try paplay first (PulseAudio), then aplay (ALSA)

‎ui.go‎

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,6 @@ func (app *App) updatePRMenuItem(pr PR) {
202202
tooltip := fmt.Sprintf("%s (%s)", pr.Title, formatAge(pr.UpdatedAt))
203203
if (pr.NeedsReview || pr.IsBlocked) && pr.ActionReason != "" {
204204
tooltip = fmt.Sprintf("%s - %s", tooltip, pr.ActionReason)
205-
log.Printf("[MENU] DEBUG: Updating tooltip for %s with actionReason: %q -> %q", pr.URL, pr.ActionReason, tooltip)
206205
}
207206

208207
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) {
215214

216215
// addPRMenuItem adds a menu item for a pull request.
217216
// NOTE: Caller must hold app.mu.Lock() when calling this function.
218-
func (app *App) addPRMenuItem(ctx context.Context, pr PR, _ bool) {
217+
func (app *App) addPRMenuItem(ctx context.Context, pr PR) {
219218
title := fmt.Sprintf("%s #%d", pr.Repository, pr.Number)
220219
// Add bullet point for PRs where user is blocking
221220
if pr.NeedsReview {
@@ -225,7 +224,6 @@ func (app *App) addPRMenuItem(ctx context.Context, pr PR, _ bool) {
225224
// Add action reason for blocked PRs
226225
if (pr.NeedsReview || pr.IsBlocked) && pr.ActionReason != "" {
227226
tooltip = fmt.Sprintf("%s - %s", tooltip, pr.ActionReason)
228-
log.Printf("[MENU] DEBUG: Setting tooltip for %s with actionReason: %q -> %q", pr.URL, pr.ActionReason, tooltip)
229227
}
230228

231229
// Check if menu item already exists
@@ -244,18 +242,17 @@ func (app *App) addPRMenuItem(ctx context.Context, pr PR, _ bool) {
244242
app.menuItems = append(app.menuItems, item)
245243
app.prMenuItems[pr.URL] = item
246244

247-
// Capture URL and context properly to avoid loop variable capture bug
248-
item.Click(func(capturedCtx context.Context, url string) func() {
249-
return func() {
250-
if err := openURL(capturedCtx, url); err != nil {
251-
log.Printf("failed to open url: %v", err)
252-
}
245+
// Capture URL to avoid loop variable capture bug
246+
prURL := pr.URL
247+
item.Click(func() {
248+
if err := openURL(ctx, prURL); err != nil {
249+
log.Printf("failed to open url: %v", err)
253250
}
254-
}(ctx, pr.URL))
251+
})
255252
}
256253

257254
// addPRSection adds a section of PRs to the menu.
258-
func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string, blockedCount int, isOutgoing bool) {
255+
func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string, blockedCount int) {
259256
if len(prs) == 0 {
260257
return
261258
}
@@ -294,7 +291,7 @@ func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string,
294291
if app.hideStaleIncoming && isStale(prs[i].UpdatedAt) {
295292
continue
296293
}
297-
app.addPRMenuItem(ctx, prs[i], isOutgoing)
294+
app.addPRMenuItem(ctx, prs[i])
298295
}
299296

300297
app.mu.Unlock()
@@ -400,14 +397,14 @@ func (app *App) updateMenu(ctx context.Context) {
400397

401398
// Incoming section
402399
if incomingCount > 0 {
403-
app.addPRSection(ctx, app.incoming, "Incoming", incomingBlocked, false)
400+
app.addPRSection(ctx, app.incoming, "Incoming", incomingBlocked)
404401
}
405402

406403
systray.AddSeparator()
407404

408405
// Outgoing section
409406
if outgoingCount > 0 {
410-
app.addPRSection(ctx, app.outgoing, "Outgoing", outgoingBlocked, true)
407+
app.addPRSection(ctx, app.outgoing, "Outgoing", outgoingBlocked)
411408
}
412409

413410
log.Print("[MENU] Menu update complete")

0 commit comments

Comments
 (0)