diff --git a/cmd/goose/icons.go b/cmd/goose/icons.go new file mode 100644 index 0000000..eb0f7eb --- /dev/null +++ b/cmd/goose/icons.go @@ -0,0 +1,81 @@ +package main + +import ( + _ "embed" + "log/slog" + "os" + "path/filepath" +) + +// Embed icon files at compile time for better distribution +// +//go:embed icons/goose.png +var iconGoose []byte + +//go:embed icons/popper.png +var iconPopper []byte + +//go:embed icons/smiling-face.png +var iconSmiling []byte + +//go:embed icons/lock.png +var iconLock []byte + +//go:embed icons/warning.png +var iconWarning []byte + +// IconType represents different icon states +type IconType int + +const ( + IconSmiling IconType = iota // No blocked PRs + IconGoose // Incoming PRs blocked + IconPopper // Outgoing PRs blocked + IconBoth // Both incoming and outgoing blocked + IconWarning // General error/warning + IconLock // Authentication error +) + +// getIcon returns the icon bytes for the given type +func getIcon(iconType IconType) []byte { + switch iconType { + case IconGoose: + return iconGoose + case IconPopper: + return iconPopper + case IconSmiling: + return iconSmiling + case IconWarning: + return iconWarning + case IconLock: + return iconLock + case IconBoth: + // For both, we'll use the goose icon as primary + return iconGoose + default: + return iconSmiling + } +} + +// loadIconFromFile loads an icon from the filesystem (fallback if embed fails) +func loadIconFromFile(filename string) []byte { + iconPath := filepath.Join("icons", filename) + data, err := os.ReadFile(iconPath) + if err != nil { + slog.Warn("Failed to load icon file", "path", iconPath, "error", err) + return nil + } + return data +} + +// setTrayIcon updates the system tray icon based on PR counts +func (app *App) setTrayIcon(iconType IconType) { + iconBytes := getIcon(iconType) + if iconBytes == nil || len(iconBytes) == 0 { + slog.Warn("Icon bytes are empty, skipping icon update", "type", iconType) + return + } + + app.systrayInterface.SetIcon(iconBytes) + slog.Debug("[TRAY] Setting icon", "type", iconType) +} \ No newline at end of file diff --git a/cmd/goose/icons/goose.png b/cmd/goose/icons/goose.png new file mode 100644 index 0000000..0a569f3 Binary files /dev/null and b/cmd/goose/icons/goose.png differ diff --git a/cmd/goose/icons/lock.png b/cmd/goose/icons/lock.png new file mode 100644 index 0000000..72d1e3f Binary files /dev/null and b/cmd/goose/icons/lock.png differ diff --git a/cmd/goose/icons/popper.png b/cmd/goose/icons/popper.png new file mode 100644 index 0000000..fab5eeb Binary files /dev/null and b/cmd/goose/icons/popper.png differ diff --git a/cmd/goose/icons/smiling-face.png b/cmd/goose/icons/smiling-face.png new file mode 100644 index 0000000..ffa3a4c Binary files /dev/null and b/cmd/goose/icons/smiling-face.png differ diff --git a/cmd/goose/icons/warning.png b/cmd/goose/icons/warning.png new file mode 100644 index 0000000..4b9c95b Binary files /dev/null and b/cmd/goose/icons/warning.png differ diff --git a/cmd/goose/loginitem_darwin.go b/cmd/goose/loginitem_darwin.go index 39d0052..5775c92 100644 --- a/cmd/goose/loginitem_darwin.go +++ b/cmd/goose/loginitem_darwin.go @@ -142,7 +142,7 @@ func appPath() (string, error) { } // addLoginItemUI adds the login item menu option (macOS only). -func addLoginItemUI(ctx context.Context, _ *App) { +func addLoginItemUI(ctx context.Context, app *App) { // Check if we're running from an app bundle execPath, err := os.Executable() if err != nil { @@ -163,12 +163,14 @@ func addLoginItemUI(ctx context.Context, _ *App) { return } - loginItem := systray.AddMenuItem("Start at Login", "Automatically start when you log in") - - // Set initial state + // Add text checkmark for consistency with other menu items + var loginText string if isLoginItem(ctx) { - loginItem.Check() + loginText = "✓ Start at Login" + } else { + loginText = "Start at Login" } + loginItem := systray.AddMenuItem(loginText, "Automatically start when you log in") loginItem.Click(func() { isEnabled := isLoginItem(ctx) @@ -176,20 +178,13 @@ func addLoginItemUI(ctx context.Context, _ *App) { if err := setLoginItem(ctx, newState); err != nil { slog.Error("Failed to set login item", "error", err) - // Revert the UI state on error - if isEnabled { - loginItem.Check() - } else { - loginItem.Uncheck() - } return } // Update UI state - if newState { - loginItem.Check() - } else { - loginItem.Uncheck() - } + slog.Info("[SETTINGS] Start at Login toggled", "enabled", newState) + + // Rebuild menu to update checkmark + app.rebuildMenu(ctx) }) } diff --git a/cmd/goose/main.go b/cmd/goose/main.go index ebff0ff..2ac9dbd 100644 --- a/cmd/goose/main.go +++ b/cmd/goose/main.go @@ -11,6 +11,7 @@ import ( "log/slog" "os" "path/filepath" + "runtime" "slices" "strings" "sync" @@ -90,6 +91,7 @@ type App struct { updateInterval time.Duration consecutiveFailures int mu sync.RWMutex + menuMutex sync.Mutex // Mutex to prevent concurrent menu rebuilds enableAutoBrowser bool hideStaleIncoming bool hasPerformedInitialDiscovery bool // Track if we've done the first poll to distinguish from real state changes @@ -157,7 +159,7 @@ func main() { Level: logLevel, } slog.SetDefault(slog.New(slog.NewTextHandler(os.Stderr, opts))) - slog.Info("Starting GitHub PR Monitor", "version", version, "commit", commit, "date", date) + slog.Info("Starting Goose", "version", version, "commit", commit, "date", date) slog.Info("Configuration", "update_interval", updateInterval, "max_retries", maxRetries, "max_delay", maxRetryDelay) slog.Info("Browser auto-open configuration", "startup_delay", browserOpenDelay, "max_per_minute", maxBrowserOpensMinute, "max_per_day", maxBrowserOpensDay) @@ -286,23 +288,111 @@ func main() { func (app *App) onReady(ctx context.Context) { slog.Info("System tray ready") + // On Linux, immediately build a minimal menu to ensure it's visible + if runtime.GOOS == "linux" { + slog.Info("[LINUX] Building initial minimal menu") + app.systrayInterface.ResetMenu() + placeholderItem := app.systrayInterface.AddMenuItem("Loading...", "Goose is starting up") + if placeholderItem != nil { + placeholderItem.Disable() + } + app.systrayInterface.AddSeparator() + quitItem := app.systrayInterface.AddMenuItem("Quit", "Quit Goose") + if quitItem != nil { + quitItem.Click(func() { + slog.Info("Quit clicked") + systray.Quit() + }) + } + } + // Set up click handlers first (needed for both success and error states) systray.SetOnClick(func(menu systray.IMenu) { slog.Debug("Icon clicked") - // Check if we can perform a forced refresh (rate limited to every 10 seconds) + // Check if we're in auth error state and should retry app.mu.RLock() - timeSinceLastSearch := time.Since(app.lastSearchAttempt) + hasAuthError := app.authError != "" app.mu.RUnlock() - if timeSinceLastSearch >= minUpdateInterval { - slog.Info("[CLICK] Forcing search refresh", "lastSearchAgo", timeSinceLastSearch) + if hasAuthError { + slog.Info("[CLICK] Auth error detected, attempting to re-authenticate") go func() { - app.updatePRs(ctx) + // Try to reinitialize clients which will attempt to get token via gh auth token + if err := app.initClients(ctx); err != nil { + slog.Warn("[CLICK] Re-authentication failed", "error", err) + app.mu.Lock() + app.authError = err.Error() + app.mu.Unlock() + } else { + // Success! Clear auth error and reload user + slog.Info("[CLICK] Re-authentication successful") + app.mu.Lock() + app.authError = "" + app.mu.Unlock() + + // Load current user + if app.client != nil { + var user *github.User + err := retry.Do(func() error { + var retryErr error + user, _, retryErr = app.client.Users.Get(ctx, "") + if retryErr != nil { + slog.Warn("GitHub Users.Get failed (will retry)", "error", retryErr) + return retryErr + } + return nil + }, + retry.Attempts(maxRetries), + retry.DelayType(retry.CombineDelay(retry.BackOffDelay, retry.RandomDelay)), + retry.MaxDelay(maxRetryDelay), + retry.OnRetry(func(n uint, err error) { + slog.Debug("[RETRY] Retrying GitHub API call", "attempt", n, "error", err) + }), + ) + if err == nil && user != nil { + if app.targetUser == "" { + app.targetUser = user.GetLogin() + slog.Info("Set target user to current user", "user", app.targetUser) + } + } + } + + // Update tooltip + tooltip := "Goose - Loading PRs..." + if app.targetUser != "" { + tooltip = fmt.Sprintf("Goose - Loading PRs... (@%s)", app.targetUser) + } + systray.SetTooltip(tooltip) + + // Rebuild menu to remove error state + app.rebuildMenu(ctx) + + // Start update loop if not already running + if !app.menuInitialized { + app.menuInitialized = true + go app.updateLoop(ctx) + } else { + // Just do a single update to refresh data + go app.updatePRs(ctx) + } + } }() } else { - remainingTime := minUpdateInterval - timeSinceLastSearch - slog.Debug("[CLICK] Rate limited", "lastSearchAgo", timeSinceLastSearch, "remaining", remainingTime) + // Normal operation - check if we can perform a forced refresh + app.mu.RLock() + timeSinceLastSearch := time.Since(app.lastSearchAttempt) + app.mu.RUnlock() + + if timeSinceLastSearch >= minUpdateInterval { + slog.Info("[CLICK] Forcing search refresh", "lastSearchAgo", timeSinceLastSearch) + go func() { + app.updatePRs(ctx) + }() + } else { + remainingTime := minUpdateInterval - timeSinceLastSearch + slog.Debug("[CLICK] Rate limited", "lastSearchAgo", timeSinceLastSearch, "remaining", remainingTime) + } } if menu != nil { @@ -323,8 +413,9 @@ func (app *App) onReady(ctx context.Context) { // Check if we have an auth error if app.authError != "" { - systray.SetTitle("⚠️") - systray.SetTooltip("GitHub PR Monitor - Authentication Error") + systray.SetTitle("") + app.setTrayIcon(IconLock) + systray.SetTooltip("Goose - Authentication Error") // Create initial error menu app.rebuildMenu(ctx) // Clean old cache on startup @@ -332,12 +423,13 @@ func (app *App) onReady(ctx context.Context) { return } - systray.SetTitle("Loading PRs...") + systray.SetTitle("") + app.setTrayIcon(IconSmiling) // Start with smiling icon while loading // Set tooltip based on whether we're using a custom user - tooltip := "GitHub PR Monitor" + tooltip := "Goose - Loading PRs..." if app.targetUser != "" { - tooltip = fmt.Sprintf("GitHub PR Monitor - @%s", app.targetUser) + tooltip = fmt.Sprintf("Goose - Loading PRs... (@%s)", app.targetUser) } systray.SetTooltip(tooltip) @@ -355,8 +447,9 @@ func (app *App) updateLoop(ctx context.Context) { slog.Error("PANIC in update loop", "panic", r) // Set error state in UI - systray.SetTitle("💥") - systray.SetTooltip("GitHub PR Monitor - Critical error") + systray.SetTitle("") + app.setTrayIcon(IconWarning) + systray.SetTooltip("Goose - Critical error") // Update failure count app.mu.Lock() @@ -424,23 +517,19 @@ func (app *App) updatePRs(ctx context.Context) { app.mu.Unlock() // Progressive degradation based on failure count - var title, tooltip string + var tooltip string + var iconType IconType 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" + iconType = IconWarning + tooltip = fmt.Sprintf("Goose - %d consecutive failures", failureCount) default: - title = "💀" - tooltip = "GitHub PR Monitor - Service degraded, check authentication" + iconType = IconWarning + tooltip = "Goose - Connection failures, check network/auth" } - systray.SetTitle(title) + systray.SetTitle("") + app.setTrayIcon(iconType) // Include time since last success and user info timeSinceSuccess := "never" @@ -602,23 +691,19 @@ func (app *App) updatePRsWithWait(ctx context.Context) { app.mu.Unlock() // Progressive degradation based on failure count - var title, tooltip string + var tooltip string + var iconType IconType 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" + iconType = IconWarning + tooltip = fmt.Sprintf("Goose - %d consecutive failures", failureCount) default: - title = "💀" - tooltip = "GitHub PR Monitor - Service degraded, check authentication" + iconType = IconWarning + tooltip = "Goose - Connection failures, check network/auth" } - systray.SetTitle(title) + systray.SetTitle("") + app.setTrayIcon(iconType) systray.SetTooltip(tooltip) // Create or update menu to show error state @@ -740,7 +825,7 @@ func (app *App) tryAutoOpenPR(ctx context.Context, pr PR, autoBrowserEnabled boo slog.Warn("Auto-open strict validation failed", "url", sanitizeForLog(pr.URL), "error", err) return } - if err := openURL(ctx, pr.URL, pr.ActionKind); err != nil { + if err := openURL(ctx, pr.URL); err != nil { slog.Error("[BROWSER] Failed to auto-open PR", "url", pr.URL, "error", err) } else { app.browserRateLimiter.RecordOpen(pr.URL) diff --git a/cmd/goose/main_test.go b/cmd/goose/main_test.go index 04d332d..8e0df14 100644 --- a/cmd/goose/main_test.go +++ b/cmd/goose/main_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "os" + "runtime" "sync" "testing" "time" @@ -196,8 +197,15 @@ func TestTrayIconRestoredAfterNetworkRecovery(t *testing.T) { } app.setTrayTitle() initialTitle := mock.title - if initialTitle != "🪿 1" { - t.Errorf("Expected initial tray title '🪿 1', got %q", initialTitle) + + // Expected title varies by platform + expectedTitle := "" // Most platforms: icon only, no text + if runtime.GOOS == "darwin" { + expectedTitle = "1" // macOS: show count with icon + } + + if initialTitle != expectedTitle { + t.Errorf("Expected initial tray title %q, got %q", expectedTitle, initialTitle) } // Simulate network failure - updatePRs would set warning icon and return early @@ -213,8 +221,8 @@ func TestTrayIconRestoredAfterNetworkRecovery(t *testing.T) { // With our fix, setTrayTitle() is now called after successful fetch app.setTrayTitle() recoveredTitle := mock.title - if recoveredTitle != "🪿 1" { - t.Errorf("Expected tray title to be restored to '🪿 1' after recovery, got %q", recoveredTitle) + if recoveredTitle != expectedTitle { + t.Errorf("Expected tray title to be restored to %q after recovery, got %q", expectedTitle, recoveredTitle) } } @@ -242,7 +250,7 @@ func TestTrayTitleUpdates(t *testing.T) { name: "no PRs", incoming: []PR{}, outgoing: []PR{}, - expectedTitle: "😊", + expectedTitle: "", // No count shown when no blocked PRs }, { name: "only incoming blocked", @@ -251,7 +259,7 @@ func TestTrayTitleUpdates(t *testing.T) { {Repository: "test/repo", Number: 2, NeedsReview: true, UpdatedAt: time.Now()}, }, outgoing: []PR{}, - expectedTitle: "🪿 2", + expectedTitle: "2", // macOS format: just the count }, { name: "only outgoing blocked", @@ -261,7 +269,7 @@ func TestTrayTitleUpdates(t *testing.T) { {Repository: "test/repo", Number: 4, IsBlocked: true, UpdatedAt: time.Now()}, {Repository: "test/repo", Number: 5, IsBlocked: true, UpdatedAt: time.Now()}, }, - expectedTitle: "🎉 3", + expectedTitle: "3", // macOS format: just the count }, { name: "both incoming and outgoing blocked", @@ -271,7 +279,7 @@ func TestTrayTitleUpdates(t *testing.T) { outgoing: []PR{ {Repository: "test/repo", Number: 2, IsBlocked: true, UpdatedAt: time.Now()}, }, - expectedTitle: "🪿 1 🎉 1", + expectedTitle: "1 / 1", // macOS format: "incoming / outgoing" }, { name: "mixed blocked and unblocked", @@ -283,7 +291,7 @@ func TestTrayTitleUpdates(t *testing.T) { {Repository: "test/repo", Number: 3, IsBlocked: false, UpdatedAt: time.Now()}, {Repository: "test/repo", Number: 4, IsBlocked: true, UpdatedAt: time.Now()}, }, - expectedTitle: "🪿 1 🎉 1", + expectedTitle: "1 / 1", // macOS format: "incoming / outgoing" }, { name: "hidden org filters out blocked PRs", @@ -293,7 +301,7 @@ func TestTrayTitleUpdates(t *testing.T) { }, outgoing: []PR{}, hiddenOrgs: map[string]bool{"hidden-org": true}, - expectedTitle: "🪿 1", + expectedTitle: "1", // macOS format: just the count }, { name: "stale PRs filtered when hideStaleIncoming is true", @@ -303,7 +311,7 @@ func TestTrayTitleUpdates(t *testing.T) { }, outgoing: []PR{}, hideStaleIncoming: true, - expectedTitle: "🪿 1", + expectedTitle: "1", // macOS format: just the count }, } @@ -314,22 +322,19 @@ func TestTrayTitleUpdates(t *testing.T) { app.hiddenOrgs = tt.hiddenOrgs app.hideStaleIncoming = tt.hideStaleIncoming - // Get the title that would be set - counts := app.countPRs() - var title string - switch { - case counts.IncomingBlocked == 0 && counts.OutgoingBlocked == 0: - title = "😊" - case counts.IncomingBlocked > 0 && counts.OutgoingBlocked > 0: - title = fmt.Sprintf("🪿 %d 🎉 %d", counts.IncomingBlocked, counts.OutgoingBlocked) - case counts.IncomingBlocked > 0: - title = fmt.Sprintf("🪿 %d", counts.IncomingBlocked) - default: - title = fmt.Sprintf("🎉 %d", counts.OutgoingBlocked) + // Call setTrayTitle to get the actual title + app.setTrayTitle() + actualTitle := app.systrayInterface.(*MockSystray).title + + // Adjust expected title based on platform + expectedTitle := tt.expectedTitle + if runtime.GOOS != "darwin" { + // Non-macOS platforms show icon only (no text) + expectedTitle = "" } - if title != tt.expectedTitle { - t.Errorf("Expected tray title %q, got %q", tt.expectedTitle, title) + if actualTitle != expectedTitle { + t.Errorf("Expected tray title %q, got %q", expectedTitle, actualTitle) } }) } @@ -447,15 +452,19 @@ func TestSoundPlaybackDuringTransitions(t *testing.T) { // Actual sound playback is verified through integration testing. // Set initial state + app.mu.Lock() app.incoming = tt.initialIncoming app.outgoing = tt.initialOutgoing + app.mu.Unlock() // Run first check to establish baseline app.checkForNewlyBlockedPRs(ctx) // Update to new state + app.mu.Lock() app.incoming = tt.updatedIncoming app.outgoing = tt.updatedOutgoing + app.mu.Unlock() // Run check again to detect transitions app.checkForNewlyBlockedPRs(ctx) @@ -465,6 +474,7 @@ func TestSoundPlaybackDuringTransitions(t *testing.T) { if len(tt.expectedSounds) > 0 { // Check that blocked PRs are tracked in previousBlockedPRs blocked := 0 + app.mu.RLock() for _, pr := range app.incoming { if pr.NeedsReview && app.previousBlockedPRs[pr.URL] { blocked++ @@ -475,6 +485,7 @@ func TestSoundPlaybackDuringTransitions(t *testing.T) { blocked++ } } + app.mu.RUnlock() if blocked == 0 { t.Errorf("%s: expected blocked PRs to be tracked in previousBlockedPRs", tt.description) } diff --git a/cmd/goose/notifications.go b/cmd/goose/notifications.go index a1b3870..caa2c7a 100644 --- a/cmd/goose/notifications.go +++ b/cmd/goose/notifications.go @@ -56,39 +56,44 @@ func (app *App) processNotifications(ctx context.Context) { slog.Info("[NOTIFY] PRs need notifications", "count", len(toNotify)) - // Send notifications for each PR - playedHonk := false - playedRocket := false - - for i := range toNotify { - pr := toNotify[i] - isIncoming := false - // Check if it's in the incoming list - for j := range incoming { - if incoming[j].URL == pr.URL { - isIncoming = true - break + // Process notifications in a goroutine to avoid blocking the UI thread + go func() { + // Send notifications for each PR + playedHonk := false + playedRocket := false + + for i := range toNotify { + pr := toNotify[i] + isIncoming := false + // Check if it's in the incoming list + for j := range incoming { + if incoming[j].URL == pr.URL { + isIncoming = true + break + } } - } - // Send notification - if isIncoming { - app.sendPRNotification(ctx, pr, "PR Blocked on You 🪿", "honk", &playedHonk) - } else { - // Add delay between different sound types - if playedHonk && !playedRocket { - time.Sleep(2 * time.Second) + // Send notification + if isIncoming { + app.sendPRNotification(ctx, pr, "PR Blocked on You 🪿", "honk", &playedHonk) + } else { + // Add delay between different sound types in goroutine to avoid blocking + if playedHonk && !playedRocket { + time.Sleep(2 * time.Second) + } + app.sendPRNotification(ctx, pr, "Your PR is Blocked 🚀", "rocket", &playedRocket) } - app.sendPRNotification(ctx, pr, "Your PR is Blocked 🚀", "rocket", &playedRocket) - } - // Auto-open if enabled - if app.enableAutoBrowser && time.Since(app.startTime) > startupGracePeriod { - app.tryAutoOpenPR(ctx, pr, app.enableAutoBrowser, app.startTime) + // Auto-open if enabled + if app.enableAutoBrowser && time.Since(app.startTime) > startupGracePeriod { + app.tryAutoOpenPR(ctx, pr, app.enableAutoBrowser, app.startTime) + } } - } - // Update menu if we sent notifications + }() + + // Update menu immediately after sending notifications + // This needs to happen in the main thread to show the party popper emoji if len(toNotify) > 0 { slog.Info("[FLOW] Updating menu after sending notifications", "notified_count", len(toNotify)) app.updateMenu(ctx) @@ -100,12 +105,14 @@ func (app *App) processNotifications(ctx context.Context) { func (app *App) sendPRNotification(ctx context.Context, pr PR, title string, soundType string, playedSound *bool) { message := fmt.Sprintf("%s #%d: %s", pr.Repository, pr.Number, pr.Title) - // Send desktop notification - if err := beeep.Notify(title, message, ""); err != nil { - slog.Error("[NOTIFY] Failed to send notification", "url", pr.URL, "error", err) - } + // Send desktop notification in a goroutine to avoid blocking + go func() { + if err := beeep.Notify(title, message, ""); err != nil { + slog.Error("[NOTIFY] Failed to send notification", "url", pr.URL, "error", err) + } + }() - // Play sound (only once per type per cycle) + // Play sound (only once per type per cycle) - already async in playSound if !*playedSound { slog.Debug("[NOTIFY] Playing sound for PR", "soundType", soundType, "repo", pr.Repository, "number", pr.Number) app.playSound(ctx, soundType) diff --git a/cmd/goose/systray_interface.go b/cmd/goose/systray_interface.go index 4b0af61..edc1223 100644 --- a/cmd/goose/systray_interface.go +++ b/cmd/goose/systray_interface.go @@ -1,6 +1,9 @@ package main import ( + "log/slog" + "sync" + "github.com/energye/systray" ) @@ -10,6 +13,7 @@ type SystrayInterface interface { AddMenuItem(title, tooltip string) MenuItem AddSeparator() SetTitle(title string) + SetIcon(iconBytes []byte) SetOnClick(fn func(menu systray.IMenu)) Quit() } @@ -18,10 +22,12 @@ type SystrayInterface interface { type RealSystray struct{} func (*RealSystray) ResetMenu() { + slog.Debug("[SYSTRAY] ResetMenu called") systray.ResetMenu() } func (*RealSystray) AddMenuItem(title, tooltip string) MenuItem { + slog.Debug("[SYSTRAY] AddMenuItem called", "title", title) item := systray.AddMenuItem(title, tooltip) return &RealMenuItem{MenuItem: item} } @@ -31,9 +37,14 @@ func (*RealSystray) AddSeparator() { } func (*RealSystray) SetTitle(title string) { + slog.Info("[SYSTRAY] SetTitle called", "title", title, "len", len(title)) systray.SetTitle(title) } +func (*RealSystray) SetIcon(iconBytes []byte) { + systray.SetIcon(iconBytes) +} + func (*RealSystray) SetOnClick(fn func(menu systray.IMenu)) { systray.SetOnClick(fn) } @@ -44,15 +55,20 @@ func (*RealSystray) Quit() { // MockSystray implements SystrayInterface for testing. type MockSystray struct { + mu sync.Mutex title string menuItems []string } func (m *MockSystray) ResetMenu() { + m.mu.Lock() + defer m.mu.Unlock() m.menuItems = nil } func (m *MockSystray) AddMenuItem(title, tooltip string) MenuItem { + m.mu.Lock() + defer m.mu.Unlock() m.menuItems = append(m.menuItems, title) // Return a MockMenuItem that won't panic when methods are called return &MockMenuItem{ @@ -62,13 +78,21 @@ func (m *MockSystray) AddMenuItem(title, tooltip string) MenuItem { } func (m *MockSystray) AddSeparator() { + m.mu.Lock() + defer m.mu.Unlock() m.menuItems = append(m.menuItems, "---") } func (m *MockSystray) SetTitle(title string) { + m.mu.Lock() + defer m.mu.Unlock() m.title = title } +func (*MockSystray) SetIcon(iconBytes []byte) { + // No-op for testing +} + func (*MockSystray) SetOnClick(_ func(menu systray.IMenu)) { // No-op for testing } diff --git a/cmd/goose/ui.go b/cmd/goose/ui.go index 4a1e5f3..909a015 100644 --- a/cmd/goose/ui.go +++ b/cmd/goose/ui.go @@ -20,8 +20,7 @@ import ( var _ *systray.MenuItem = nil // openURL safely opens a URL in the default browser after validation. -// action parameter is optional and specifies the expected action type for tracking. -func openURL(ctx context.Context, rawURL string, action string) error { +func openURL(ctx context.Context, rawURL string) error { // Parse and validate the URL u, err := url.Parse(rawURL) if err != nil { @@ -46,15 +45,10 @@ func openURL(ctx context.Context, rawURL string, action string) error { return errors.New("URLs with user info are not allowed") } - // Add goose parameter to track source and action for GitHub and dash URLs + // Add goose=1 parameter to track source for GitHub and dash URLs if u.Host == "github.com" || u.Host == "www.github.com" || u.Host == "dash.ready-to-review.dev" { q := u.Query() - // Use action if provided, otherwise default to "1" for backward compatibility - if action != "" { - q.Set("goose", action) - } else { - q.Set("goose", "1") - } + q.Set("goose", "1") u.RawQuery = q.Encode() rawURL = u.String() } @@ -192,31 +186,58 @@ func (app *App) countPRs() PRCounts { } } -// setTrayTitle updates the system tray title based on PR counts. +// setTrayTitle updates the system tray title and icon based on PR counts. func (app *App) setTrayTitle() { counts := app.countPRs() - // Set title based on PR state + // Set title and icon based on PR state var title string - switch { - case counts.IncomingBlocked == 0 && counts.OutgoingBlocked == 0: - title = "😊" - case counts.IncomingBlocked > 0 && counts.OutgoingBlocked > 0: - title = fmt.Sprintf("🪿 %d 🎉 %d", counts.IncomingBlocked, counts.OutgoingBlocked) - case counts.IncomingBlocked > 0: - title = fmt.Sprintf("🪿 %d", counts.IncomingBlocked) - default: - title = fmt.Sprintf("🎉 %d", counts.OutgoingBlocked) + var iconType IconType + + // On macOS, show counts with the icon + // On all other platforms (Linux, Windows, FreeBSD, etc), just show the icon + if runtime.GOOS == "darwin" { + // macOS: show counts alongside icon + switch { + case counts.IncomingBlocked == 0 && counts.OutgoingBlocked == 0: + title = "" + iconType = IconSmiling + case counts.IncomingBlocked > 0 && counts.OutgoingBlocked > 0: + title = fmt.Sprintf("%d / %d", counts.IncomingBlocked, counts.OutgoingBlocked) + iconType = IconBoth + case counts.IncomingBlocked > 0: + title = fmt.Sprintf("%d", counts.IncomingBlocked) + iconType = IconGoose + default: + title = fmt.Sprintf("%d", counts.OutgoingBlocked) + iconType = IconPopper + } + } else { + // All other platforms: icon only, no text + title = "" + switch { + case counts.IncomingBlocked == 0 && counts.OutgoingBlocked == 0: + iconType = IconSmiling + case counts.IncomingBlocked > 0 && counts.OutgoingBlocked > 0: + iconType = IconBoth + case counts.IncomingBlocked > 0: + iconType = IconGoose + default: + iconType = IconPopper + } } // Log title change with detailed counts - slog.Debug("[TRAY] Setting title", + slog.Info("[TRAY] Setting title and icon", + "os", runtime.GOOS, "title", title, + "icon", iconType, "incoming_total", counts.IncomingTotal, "incoming_blocked", counts.IncomingBlocked, "outgoing_total", counts.OutgoingTotal, "outgoing_blocked", counts.OutgoingBlocked) app.systrayInterface.SetTitle(title) + app.setTrayIcon(iconType) } // addPRSection adds a section of PRs to the menu. @@ -372,11 +393,10 @@ func (app *App) addPRSection(ctx context.Context, prs []PR, sectionTitle string, "blocked", sortedPRs[prIndex].NeedsReview || sortedPRs[prIndex].IsBlocked) item := app.systrayInterface.AddMenuItem(title, tooltip) - // Capture URL and action to avoid loop variable capture bug + // Capture URL to avoid loop variable capture bug prURL := sortedPRs[prIndex].URL - prAction := sortedPRs[prIndex].ActionKind item.Click(func() { - if err := openURL(ctx, prURL, prAction); err != nil { + if err := openURL(ctx, prURL); err != nil { slog.Error("failed to open url", "error", err) } }) @@ -532,10 +552,22 @@ func (app *App) generatePRSectionTitles(prs []PR, sectionTitle string, hiddenOrg // rebuildMenu completely rebuilds the menu from scratch. func (app *App) rebuildMenu(ctx context.Context) { + // Prevent concurrent menu rebuilds + app.menuMutex.Lock() + defer app.menuMutex.Unlock() + // Rebuild entire menu + slog.Info("[MENU] Starting rebuildMenu", "os", runtime.GOOS) // Clear all existing menu items app.systrayInterface.ResetMenu() + slog.Info("[MENU] Called ResetMenu") + + // On Linux, add a small delay to ensure DBus properly processes the reset + // This helps prevent menu item duplication + if runtime.GOOS == "linux" { + time.Sleep(50 * time.Millisecond) + } // Check for errors (auth or connection failures) app.mu.RLock() @@ -555,7 +587,7 @@ func (app *App) rebuildMenu(ctx context.Context) { // Add error details errorMsg := app.systrayInterface.AddMenuItem(authError, "Click to see setup instructions") errorMsg.Click(func() { - if err := openURL(ctx, "https://cli.github.com/manual/gh_auth_login", ""); err != nil { + if err := openURL(ctx, "https://cli.github.com/manual/gh_auth_login"); err != nil { slog.Error("failed to open setup instructions", "error", err) } }) @@ -665,7 +697,7 @@ func (app *App) rebuildMenu(ctx context.Context) { // Add Web Dashboard link dashboardItem := app.systrayInterface.AddMenuItem("Web Dashboard", "") dashboardItem.Click(func() { - if err := openURL(ctx, "https://dash.ready-to-review.dev/", ""); err != nil { + if err := openURL(ctx, "https://dash.ready-to-review.dev/"); err != nil { slog.Error("failed to open dashboard", "error", err) } }) @@ -753,60 +785,56 @@ func (app *App) addStaticMenuItems(ctx context.Context) { // Add checkbox items for each org for _, org := range orgs { orgName := org // Capture for closure - orgItem := hideOrgsMenu.AddSubMenuItem(orgName, "") - - // Check if org is currently hidden + // Add text checkmark for all platforms + var orgText string if hiddenOrgs[orgName] { - orgItem.Check() + orgText = "✓ " + orgName + } else { + orgText = orgName } + orgItem := hideOrgsMenu.AddSubMenuItem(orgText, "") orgItem.Click(func() { app.mu.Lock() if app.hiddenOrgs[orgName] { delete(app.hiddenOrgs, orgName) - orgItem.Uncheck() slog.Info("[SETTINGS] Unhiding org", "org", orgName) } else { app.hiddenOrgs[orgName] = true - orgItem.Check() slog.Info("[SETTINGS] Hiding org", "org", orgName) } - // Menu always rebuilds now - no need to clear titles app.mu.Unlock() // Save settings app.saveSettings() - // Rebuild menu to reflect changes + // Rebuild menu to update checkmarks app.rebuildMenu(ctx) }) } } // Hide stale PRs - // Add 'Hide stale PRs' option - hideStaleItem := app.systrayInterface.AddMenuItem("Hide stale PRs (>90 days)", "") + // Add 'Hide stale PRs' option with text checkmark for all platforms + var hideStaleText string if app.hideStaleIncoming { - hideStaleItem.Check() + hideStaleText = "✓ Hide stale PRs (>90 days)" + } else { + hideStaleText = "Hide stale PRs (>90 days)" } + hideStaleItem := app.systrayInterface.AddMenuItem(hideStaleText, "") hideStaleItem.Click(func() { app.mu.Lock() app.hideStaleIncoming = !app.hideStaleIncoming hideStale := app.hideStaleIncoming - // Menu always rebuilds now - no need to clear titles app.mu.Unlock() - if hideStale { - hideStaleItem.Check() - } else { - hideStaleItem.Uncheck() - } - // Save settings to disk app.saveSettings() - // Toggle hide stale PRs setting - // Force menu rebuild since hideStaleIncoming changed + slog.Info("[SETTINGS] Hide stale PRs toggled", "enabled", hideStale) + + // Rebuild menu to update checkmarks app.rebuildMenu(ctx) }) @@ -814,39 +842,42 @@ func (app *App) addStaticMenuItems(ctx context.Context) { addLoginItemUI(ctx, app) // Audio cues - // Add 'Audio cues' option - audioItem := app.systrayInterface.AddMenuItem("Honks enabled", "Play sounds for notifications") + // Add 'Audio cues' option with text checkmark for all platforms app.mu.RLock() + var audioText string if app.enableAudioCues { - audioItem.Check() + audioText = "✓ Honks enabled" + } else { + audioText = "Honks enabled" } app.mu.RUnlock() + audioItem := app.systrayInterface.AddMenuItem(audioText, "Play sounds for notifications") audioItem.Click(func() { app.mu.Lock() app.enableAudioCues = !app.enableAudioCues enabled := app.enableAudioCues app.mu.Unlock() - if enabled { - audioItem.Check() - slog.Info("[SETTINGS] Audio cues enabled") - } else { - audioItem.Uncheck() - slog.Info("[SETTINGS] Audio cues disabled") - } + slog.Info("[SETTINGS] Audio cues toggled", "enabled", enabled) // Save settings to disk app.saveSettings() + + // Rebuild menu to update checkmarks + app.rebuildMenu(ctx) }) // Auto-open blocked PRs in browser - // Add 'Auto-open PRs' option - autoOpenItem := app.systrayInterface.AddMenuItem("Auto-open incoming PRs", "Automatically open newly blocked PRs in browser (rate limited)") + // Add 'Auto-open PRs' option with text checkmark for all platforms app.mu.RLock() + var autoText string if app.enableAutoBrowser { - autoOpenItem.Check() + autoText = "✓ Auto-open incoming PRs" + } else { + autoText = "Auto-open incoming PRs" } app.mu.RUnlock() + autoOpenItem := app.systrayInterface.AddMenuItem(autoText, "Automatically open newly blocked PRs in browser (rate limited)") autoOpenItem.Click(func() { app.mu.Lock() app.enableAutoBrowser = !app.enableAutoBrowser @@ -857,16 +888,13 @@ func (app *App) addStaticMenuItems(ctx context.Context) { } app.mu.Unlock() - if enabled { - autoOpenItem.Check() - slog.Info("[SETTINGS] Auto-open blocked PRs enabled") - } else { - autoOpenItem.Uncheck() - slog.Info("[SETTINGS] Auto-open blocked PRs disabled") - } + slog.Info("[SETTINGS] Auto-open blocked PRs toggled", "enabled", enabled) // Save settings to disk app.saveSettings() + + // Rebuild menu to update checkmarks + app.rebuildMenu(ctx) }) // Quit