diff --git a/Makefile b/Makefile index ba4f376..abd6039 100644 --- a/Makefile +++ b/Makefile @@ -2,7 +2,7 @@ APP_NAME = reviewGOOSE BUNDLE_NAME = reviewGOOSE VERSION = 1.0.0 BUNDLE_VERSION = 1 -BUNDLE_ID = dev.codegroove.r2r +BUNDLE_ID = dev.codegroove.reviewGOOSE # Version information for builds # Try VERSION file first (for release tarballs), then fall back to git diff --git a/cmd/reviewGOOSE/loginitem_darwin.go b/cmd/reviewGOOSE/loginitem_darwin.go index 6d98073..57d7cde 100644 --- a/cmd/reviewGOOSE/loginitem_darwin.go +++ b/cmd/reviewGOOSE/loginitem_darwin.go @@ -11,179 +11,217 @@ import ( "os/exec" "path/filepath" "strings" + "sync" + "sync/atomic" + "time" "github.com/energye/systray" ) -// validateAndEscapePathForAppleScript validates and escapes a path for safe use in AppleScript. -// Returns empty string if path contains invalid characters. -func validateAndEscapePathForAppleScript(path string) string { - // Validate path contains only safe characters - for _, r := range path { +var ( + legacyCleanupOnce sync.Once + loginItemMu sync.Mutex // prevents concurrent toggle operations + loginItemCached atomic.Bool // cached state to avoid repeated osascript calls + loginItemChecked atomic.Bool // whether we've done the initial check +) + +const osascriptTimeout = 10 * time.Second + +// escapeForAppleScript validates and escapes a string for safe use in AppleScript. +func escapeForAppleScript(s string) string { + for _, r := range s { if (r < 'a' || r > 'z') && (r < 'A' || r > 'Z') && (r < '0' || r > '9') && r != ' ' && r != '.' && r != '/' && r != '-' && r != '_' { - slog.Error("Path contains invalid character for AppleScript", "char", string(r), "path", path) + slog.Error("invalid character for AppleScript", "char", string(r), "input", s) return "" } } - // Escape backslashes first then quotes - path = strings.ReplaceAll(path, `\`, `\\`) - path = strings.ReplaceAll(path, `"`, `\"`) - return path + return strings.ReplaceAll(strings.ReplaceAll(s, `\`, `\\`), `"`, `\"`) } -// isLoginItem checks if the app is set to start at login. -func isLoginItem(ctx context.Context) bool { - appPath, err := appPath() +// bundlePath returns the .app bundle path, or an error if not running from one. +func bundlePath() (string, error) { + p, err := os.Executable() if err != nil { - slog.Error("Failed to get app path", "error", err) - return false + return "", fmt.Errorf("executable: %w", err) + } + p, err = filepath.EvalSymlinks(p) + if err != nil { + return "", fmt.Errorf("symlinks: %w", err) + } + if i := strings.Index(p, ".app/Contents/MacOS/"); i != -1 { + return p[:i+4], nil } + return "", errors.New("not an app bundle") +} - // Use osascript to check login items - escapedPath := validateAndEscapePathForAppleScript(appPath) - if escapedPath == "" { - slog.Error("Invalid app path for AppleScript", "path", appPath) +// queryLoginItemEnabled queries System Events for login item status (slow, use sparingly). +func queryLoginItemEnabled(ctx context.Context) bool { + bp, err := bundlePath() + if err != nil { + return false + } + ep := escapeForAppleScript(bp) + if ep == "" { return false } - // We use %s here because the string is already validated and escaped - //nolint:gocritic // already escaped + + ctx, cancel := context.WithTimeout(ctx, osascriptTimeout) + defer cancel() + + //nolint:gocritic // ep is already escaped script := fmt.Sprintf( - `tell application "System Events" to get the name of every login item where path is "%s"`, - escapedPath) - slog.Debug("Executing command", "command", "osascript", "script", script) - cmd := exec.CommandContext(ctx, "osascript", "-e", script) - output, err := cmd.CombinedOutput() + `tell application "System Events" to get the name of every login item where path is "%s"`, ep) + out, err := exec.CommandContext(ctx, "osascript", "-e", script).CombinedOutput() if err != nil { - slog.Error("Failed to check login items", "error", err) + if ctx.Err() == context.DeadlineExceeded { + slog.Warn("timeout checking login item") + } else { + slog.Debug("failed to check login item", "error", err) + } return false } + return strings.TrimSpace(string(out)) != "" +} - result := strings.TrimSpace(string(output)) - return result != "" +// loginItemEnabled returns cached login item state (fast, safe to call frequently). +func loginItemEnabled() bool { + return loginItemCached.Load() } // setLoginItem adds or removes the app from login items. func setLoginItem(ctx context.Context, enable bool) error { - appPath, err := appPath() + bp, err := bundlePath() if err != nil { - return fmt.Errorf("get app path: %w", err) + return err } + ctx, cancel := context.WithTimeout(ctx, osascriptTimeout) + defer cancel() + + var script string if enable { - // Add to login items - escapedPath := validateAndEscapePathForAppleScript(appPath) - if escapedPath == "" { - return fmt.Errorf("invalid app path for AppleScript: %s", appPath) + ep := escapeForAppleScript(bp) + if ep == "" { + return fmt.Errorf("invalid path: %s", bp) } - // We use %s here because the string is already validated and escaped - //nolint:gocritic // already escaped - script := fmt.Sprintf( - `tell application "System Events" to make login item at end with properties {path:"%s", hidden:false}`, - escapedPath) - slog.Debug("Executing command", "command", "osascript", "script", script) - cmd := exec.CommandContext(ctx, "osascript", "-e", script) - if output, err := cmd.CombinedOutput(); err != nil { - return fmt.Errorf("add login item: %w (output: %s)", err, string(output)) - } - slog.Info("Added to login items", "path", appPath) + //nolint:gocritic // ep is already escaped + script = fmt.Sprintf( + `tell application "System Events" to make login item at end with properties {path:"%s", hidden:false}`, ep) } else { - // Remove from login items - appName := filepath.Base(appPath) - appName = strings.TrimSuffix(appName, ".app") - escapedName := validateAndEscapePathForAppleScript(appName) - if escapedName == "" { - return fmt.Errorf("invalid app name for AppleScript: %s", appName) - } - // We use %s here because the string is already validated and escaped - script := fmt.Sprintf(`tell application "System Events" to delete login item "%s"`, escapedName) //nolint:gocritic // already escaped - slog.Debug("Executing command", "command", "osascript", "script", script) - cmd := exec.CommandContext(ctx, "osascript", "-e", script) - if output, err := cmd.CombinedOutput(); err != nil { - // Ignore error if item doesn't exist - if !strings.Contains(string(output), "Can't get login item") { - return fmt.Errorf("remove login item: %w (output: %s)", err, string(output)) - } + name := strings.TrimSuffix(filepath.Base(bp), ".app") + en := escapeForAppleScript(name) + if en == "" { + return fmt.Errorf("invalid name: %s", name) } - slog.Info("Removed from login items", "app", appName) - } - - return nil -} - -// appPath returns the path to the application bundle. -func appPath() (string, error) { - // Get the executable path - execPath, err := os.Executable() - if err != nil { - return "", fmt.Errorf("get executable: %w", err) + //nolint:gocritic // en is already escaped + script = fmt.Sprintf(`tell application "System Events" to delete login item "%s"`, en) } - // Resolve any symlinks - execPath, err = filepath.EvalSymlinks(execPath) + slog.Debug("executing login item command", "enable", enable) + out, err := exec.CommandContext(ctx, "osascript", "-e", script).CombinedOutput() if err != nil { - return "", fmt.Errorf("eval symlinks: %w", err) - } - - // Check if we're running from an app bundle - // App bundles have the structure: /path/to/App.app/Contents/MacOS/executable - if strings.Contains(execPath, ".app/Contents/MacOS/") { - // Extract the .app path - parts := strings.Split(execPath, ".app/Contents/MacOS/") - if len(parts) >= 2 { - return parts[0] + ".app", nil + s := string(out) + if !enable && strings.Contains(s, "Can't get login item") { + return nil + } + if ctx.Err() == context.DeadlineExceeded { + return errors.New("timed out") } + return fmt.Errorf("%w (output: %s)", err, s) } - - // Not running from an app bundle, return empty string to indicate this - return "", errors.New("not running from app bundle") + slog.Info("login item updated", "enabled", enable, "path", bp) + return nil } // addLoginItemUI adds the login item menu option (macOS only). func addLoginItemUI(ctx context.Context, app *App) { - // Check if we're running from an app bundle - execPath, err := os.Executable() - if err != nil { - slog.Debug("Hiding 'Start at Login' menu item - could not get executable path") + if _, err := bundlePath(); err != nil { + slog.Debug("hiding Start at Login menu item", "error", err) return } - // Resolve any symlinks - execPath, err = filepath.EvalSymlinks(execPath) - if err != nil { - slog.Debug("Hiding 'Start at Login' menu item - could not resolve symlinks") - return - } + // Remove legacy login items (once per session, async to not block menu). + // Uses background context since this is fire-and-forget cleanup. + legacyCleanupOnce.Do(func() { + go func() { + for _, name := range []string{"Ready to Review", "Review Goose"} { + en := escapeForAppleScript(name) + if en == "" { + continue + } + // Use background context - this cleanup should complete even if app is shutting down. + cleanupCtx, cancel := context.WithTimeout(context.Background(), osascriptTimeout) + script := fmt.Sprintf(`tell application "System Events" to delete login item %q`, en) + out, err := exec.CommandContext(cleanupCtx, "osascript", "-e", script).CombinedOutput() + cancel() + if err == nil { + slog.Info("removed legacy login item", "name", name) + } else if !strings.Contains(string(out), "Can't get login item") { + slog.Debug("could not remove legacy login item", "name", name, "error", err) + } + } + }() + }) - // App bundles have the structure: /path/to/App.app/Contents/MacOS/executable - if !strings.Contains(execPath, ".app/Contents/MacOS/") { - slog.Debug("Hiding 'Start at Login' menu item - not running from app bundle") - return + // Check state asynchronously on first menu build, use cached value for display. + if !loginItemChecked.Load() { + go func() { + loginItemCached.Store(queryLoginItemEnabled(ctx)) + loginItemChecked.Store(true) + slog.Debug("login item state refreshed", "enabled", loginItemCached.Load()) + }() } - // Add text checkmark for consistency with other menu items - var loginText string - if isLoginItem(ctx) { - loginText = "✓ Start at Login" - } else { - loginText = "Start at Login" + // Use cached state for menu display (fast, non-blocking). + text := "Start at Login" + if loginItemEnabled() { + text = "✓ Start at Login" } - loginItem := systray.AddMenuItem(loginText, "Automatically start when you log in") - - loginItem.Click(func() { - isEnabled := isLoginItem(ctx) - newState := !isEnabled + item := systray.AddMenuItem(text, "Automatically start when you log in") - if err := setLoginItem(ctx, newState); err != nil { - slog.Error("Failed to set login item", "error", err) + item.Click(func() { + // Prevent concurrent toggle operations. + if !loginItemMu.TryLock() { + slog.Debug("[LOGIN_ITEM] toggle already in progress") return } + defer loginItemMu.Unlock() + + cur := loginItemEnabled() + next := !cur + slog.Debug("[LOGIN_ITEM] toggling", "from", cur, "to", next) - // Update UI state - slog.Info("[SETTINGS] Start at Login toggled", "enabled", newState) + // Optimistically update cache before the slow osascript call. + loginItemCached.Store(next) + + if err := setLoginItem(ctx, next); err != nil { + slog.Error("[LOGIN_ITEM] failed to set", "error", err, "enable", next) + loginItemCached.Store(cur) // revert on failure + go showLoginItemError(ctx, next, err) + return + } - // Rebuild menu to update checkmark + slog.Info("[SETTINGS] Start at Login toggled", "enabled", next) app.rebuildMenu(ctx) }) } + +func showLoginItemError(ctx context.Context, enable bool, err error) { + ctx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() + + action, verb := "enable", "adding" + if !enable { + action, verb = "disable", "removing" + } + msg := fmt.Sprintf("Could not %s 'Start at Login'.\n\nError: %v\n\n"+ + "Try %s reviewGOOSE manually in System Settings > General > Login Items.", + action, err, verb) + script := fmt.Sprintf( + `display dialog %q with title "reviewGOOSE" buttons {"OK"} default button "OK" with icon caution`, msg) + if out, err := exec.CommandContext(ctx, "osascript", "-e", script).CombinedOutput(); err != nil { + slog.Debug("failed to show error dialog", "error", err, "output", string(out)) + } +}