Skip to content

Commit ca07d65

Browse files
tstrombergtstromberg
authored
Merge pull request #116 from tstromberg/main
improve login item toggling
2 parents d4d9aad + aa7ea43 commit ca07d65

File tree

2 files changed

+158
-120
lines changed

2 files changed

+158
-120
lines changed

‎Makefile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ APP_NAME = reviewGOOSE
22
BUNDLE_NAME = reviewGOOSE
33
VERSION = 1.0.0
44
BUNDLE_VERSION = 1
5-
BUNDLE_ID = dev.codegroove.r2r
5+
BUNDLE_ID = dev.codegroove.reviewGOOSE
66

77
# Version information for builds
88
# Try VERSION file first (for release tarballs), then fall back to git

‎cmd/reviewGOOSE/loginitem_darwin.go‎

Lines changed: 157 additions & 119 deletions
Original file line numberDiff line numberDiff line change
@@ -11,179 +11,217 @@ import (
1111
"os/exec"
1212
"path/filepath"
1313
"strings"
14+
"sync"
15+
"sync/atomic"
16+
"time"
1417

1518
"github.com/energye/systray"
1619
)
1720

18-
// validateAndEscapePathForAppleScript validates and escapes a path for safe use in AppleScript.
19-
// Returns empty string if path contains invalid characters.
20-
func validateAndEscapePathForAppleScript(path string) string {
21-
// Validate path contains only safe characters
22-
for _, r := range path {
21+
var (
22+
legacyCleanupOnce sync.Once
23+
loginItemMu sync.Mutex // prevents concurrent toggle operations
24+
loginItemCached atomic.Bool // cached state to avoid repeated osascript calls
25+
loginItemChecked atomic.Bool // whether we've done the initial check
26+
)
27+
28+
const osascriptTimeout = 10 * time.Second
29+
30+
// escapeForAppleScript validates and escapes a string for safe use in AppleScript.
31+
func escapeForAppleScript(s string) string {
32+
for _, r := range s {
2333
if (r < 'a' || r > 'z') && (r < 'A' || r > 'Z') &&
2434
(r < '0' || r > '9') && r != ' ' && r != '.' &&
2535
r != '/' && r != '-' && r != '_' {
26-
slog.Error("Path contains invalid character for AppleScript", "char", string(r), "path", path)
36+
slog.Error("invalid character for AppleScript", "char", string(r), "input", s)
2737
return ""
2838
}
2939
}
30-
// Escape backslashes first then quotes
31-
path = strings.ReplaceAll(path, `\`, `\\`)
32-
path = strings.ReplaceAll(path, `"`, `\"`)
33-
return path
40+
return strings.ReplaceAll(strings.ReplaceAll(s, `\`, `\\`), `"`, `\"`)
3441
}
3542

36-
// isLoginItem checks if the app is set to start at login.
37-
func isLoginItem(ctx context.Context) bool {
38-
appPath, err := appPath()
43+
// bundlePath returns the .app bundle path, or an error if not running from one.
44+
func bundlePath() (string, error) {
45+
p, err := os.Executable()
3946
if err != nil {
40-
slog.Error("Failed to get app path", "error", err)
41-
return false
47+
return "", fmt.Errorf("executable: %w", err)
48+
}
49+
p, err = filepath.EvalSymlinks(p)
50+
if err != nil {
51+
return "", fmt.Errorf("symlinks: %w", err)
52+
}
53+
if i := strings.Index(p, ".app/Contents/MacOS/"); i != -1 {
54+
return p[:i+4], nil
4255
}
56+
return "", errors.New("not an app bundle")
57+
}
4358

44-
// Use osascript to check login items
45-
escapedPath := validateAndEscapePathForAppleScript(appPath)
46-
if escapedPath == "" {
47-
slog.Error("Invalid app path for AppleScript", "path", appPath)
59+
// queryLoginItemEnabled queries System Events for login item status (slow, use sparingly).
60+
func queryLoginItemEnabled(ctx context.Context) bool {
61+
bp, err := bundlePath()
62+
if err != nil {
63+
return false
64+
}
65+
ep := escapeForAppleScript(bp)
66+
if ep == "" {
4867
return false
4968
}
50-
// We use %s here because the string is already validated and escaped
51-
//nolint:gocritic // already escaped
69+
70+
ctx, cancel := context.WithTimeout(ctx, osascriptTimeout)
71+
defer cancel()
72+
73+
//nolint:gocritic // ep is already escaped
5274
script := fmt.Sprintf(
53-
`tell application "System Events" to get the name of every login item where path is "%s"`,
54-
escapedPath)
55-
slog.Debug("Executing command", "command", "osascript", "script", script)
56-
cmd := exec.CommandContext(ctx, "osascript", "-e", script)
57-
output, err := cmd.CombinedOutput()
75+
`tell application "System Events" to get the name of every login item where path is "%s"`, ep)
76+
out, err := exec.CommandContext(ctx, "osascript", "-e", script).CombinedOutput()
5877
if err != nil {
59-
slog.Error("Failed to check login items", "error", err)
78+
if ctx.Err() == context.DeadlineExceeded {
79+
slog.Warn("timeout checking login item")
80+
} else {
81+
slog.Debug("failed to check login item", "error", err)
82+
}
6083
return false
6184
}
85+
return strings.TrimSpace(string(out)) != ""
86+
}
6287

63-
result := strings.TrimSpace(string(output))
64-
return result != ""
88+
// loginItemEnabled returns cached login item state (fast, safe to call frequently).
89+
func loginItemEnabled() bool {
90+
return loginItemCached.Load()
6591
}
6692

6793
// setLoginItem adds or removes the app from login items.
6894
func setLoginItem(ctx context.Context, enable bool) error {
69-
appPath, err := appPath()
95+
bp, err := bundlePath()
7096
if err != nil {
71-
return fmt.Errorf("get app path: %w", err)
97+
return err
7298
}
7399

100+
ctx, cancel := context.WithTimeout(ctx, osascriptTimeout)
101+
defer cancel()
102+
103+
var script string
74104
if enable {
75-
// Add to login items
76-
escapedPath := validateAndEscapePathForAppleScript(appPath)
77-
if escapedPath == "" {
78-
return fmt.Errorf("invalid app path for AppleScript: %s", appPath)
105+
ep := escapeForAppleScript(bp)
106+
if ep == "" {
107+
return fmt.Errorf("invalid path: %s", bp)
79108
}
80-
// We use %s here because the string is already validated and escaped
81-
//nolint:gocritic // already escaped
82-
script := fmt.Sprintf(
83-
`tell application "System Events" to make login item at end with properties {path:"%s", hidden:false}`,
84-
escapedPath)
85-
slog.Debug("Executing command", "command", "osascript", "script", script)
86-
cmd := exec.CommandContext(ctx, "osascript", "-e", script)
87-
if output, err := cmd.CombinedOutput(); err != nil {
88-
return fmt.Errorf("add login item: %w (output: %s)", err, string(output))
89-
}
90-
slog.Info("Added to login items", "path", appPath)
109+
//nolint:gocritic // ep is already escaped
110+
script = fmt.Sprintf(
111+
`tell application "System Events" to make login item at end with properties {path:"%s", hidden:false}`, ep)
91112
} else {
92-
// Remove from login items
93-
appName := filepath.Base(appPath)
94-
appName = strings.TrimSuffix(appName, ".app")
95-
escapedName := validateAndEscapePathForAppleScript(appName)
96-
if escapedName == "" {
97-
return fmt.Errorf("invalid app name for AppleScript: %s", appName)
98-
}
99-
// We use %s here because the string is already validated and escaped
100-
script := fmt.Sprintf(`tell application "System Events" to delete login item "%s"`, escapedName) //nolint:gocritic // already escaped
101-
slog.Debug("Executing command", "command", "osascript", "script", script)
102-
cmd := exec.CommandContext(ctx, "osascript", "-e", script)
103-
if output, err := cmd.CombinedOutput(); err != nil {
104-
// Ignore error if item doesn't exist
105-
if !strings.Contains(string(output), "Can't get login item") {
106-
return fmt.Errorf("remove login item: %w (output: %s)", err, string(output))
107-
}
113+
name := strings.TrimSuffix(filepath.Base(bp), ".app")
114+
en := escapeForAppleScript(name)
115+
if en == "" {
116+
return fmt.Errorf("invalid name: %s", name)
108117
}
109-
slog.Info("Removed from login items", "app", appName)
110-
}
111-
112-
return nil
113-
}
114-
115-
// appPath returns the path to the application bundle.
116-
func appPath() (string, error) {
117-
// Get the executable path
118-
execPath, err := os.Executable()
119-
if err != nil {
120-
return "", fmt.Errorf("get executable: %w", err)
118+
//nolint:gocritic // en is already escaped
119+
script = fmt.Sprintf(`tell application "System Events" to delete login item "%s"`, en)
121120
}
122121

123-
// Resolve any symlinks
124-
execPath, err = filepath.EvalSymlinks(execPath)
122+
slog.Debug("executing login item command", "enable", enable)
123+
out, err := exec.CommandContext(ctx, "osascript", "-e", script).CombinedOutput()
125124
if err != nil {
126-
return "", fmt.Errorf("eval symlinks: %w", err)
127-
}
128-
129-
// Check if we're running from an app bundle
130-
// App bundles have the structure: /path/to/App.app/Contents/MacOS/executable
131-
if strings.Contains(execPath, ".app/Contents/MacOS/") {
132-
// Extract the .app path
133-
parts := strings.Split(execPath, ".app/Contents/MacOS/")
134-
if len(parts) >= 2 {
135-
return parts[0] + ".app", nil
125+
s := string(out)
126+
if !enable && strings.Contains(s, "Can't get login item") {
127+
return nil
128+
}
129+
if ctx.Err() == context.DeadlineExceeded {
130+
return errors.New("timed out")
136131
}
132+
return fmt.Errorf("%w (output: %s)", err, s)
137133
}
138-
139-
// Not running from an app bundle, return empty string to indicate this
140-
return "", errors.New("not running from app bundle")
134+
slog.Info("login item updated", "enabled", enable, "path", bp)
135+
return nil
141136
}
142137

143138
// addLoginItemUI adds the login item menu option (macOS only).
144139
func addLoginItemUI(ctx context.Context, app *App) {
145-
// Check if we're running from an app bundle
146-
execPath, err := os.Executable()
147-
if err != nil {
148-
slog.Debug("Hiding 'Start at Login' menu item - could not get executable path")
140+
if _, err := bundlePath(); err != nil {
141+
slog.Debug("hiding Start at Login menu item", "error", err)
149142
return
150143
}
151144

152-
// Resolve any symlinks
153-
execPath, err = filepath.EvalSymlinks(execPath)
154-
if err != nil {
155-
slog.Debug("Hiding 'Start at Login' menu item - could not resolve symlinks")
156-
return
157-
}
145+
// Remove legacy login items (once per session, async to not block menu).
146+
// Uses background context since this is fire-and-forget cleanup.
147+
legacyCleanupOnce.Do(func() {
148+
go func() {
149+
for _, name := range []string{"Ready to Review", "Review Goose"} {
150+
en := escapeForAppleScript(name)
151+
if en == "" {
152+
continue
153+
}
154+
// Use background context - this cleanup should complete even if app is shutting down.
155+
cleanupCtx, cancel := context.WithTimeout(context.Background(), osascriptTimeout)
156+
script := fmt.Sprintf(`tell application "System Events" to delete login item %q`, en)
157+
out, err := exec.CommandContext(cleanupCtx, "osascript", "-e", script).CombinedOutput()
158+
cancel()
159+
if err == nil {
160+
slog.Info("removed legacy login item", "name", name)
161+
} else if !strings.Contains(string(out), "Can't get login item") {
162+
slog.Debug("could not remove legacy login item", "name", name, "error", err)
163+
}
164+
}
165+
}()
166+
})
158167

159-
// App bundles have the structure: /path/to/App.app/Contents/MacOS/executable
160-
if !strings.Contains(execPath, ".app/Contents/MacOS/") {
161-
slog.Debug("Hiding 'Start at Login' menu item - not running from app bundle")
162-
return
168+
// Check state asynchronously on first menu build, use cached value for display.
169+
if !loginItemChecked.Load() {
170+
go func() {
171+
loginItemCached.Store(queryLoginItemEnabled(ctx))
172+
loginItemChecked.Store(true)
173+
slog.Debug("login item state refreshed", "enabled", loginItemCached.Load())
174+
}()
163175
}
164176

165-
// Add text checkmark for consistency with other menu items
166-
var loginText string
167-
if isLoginItem(ctx) {
168-
loginText = "✓ Start at Login"
169-
} else {
170-
loginText = "Start at Login"
177+
// Use cached state for menu display (fast, non-blocking).
178+
text := "Start at Login"
179+
if loginItemEnabled() {
180+
text = "✓ Start at Login"
171181
}
172-
loginItem := systray.AddMenuItem(loginText, "Automatically start when you log in")
173-
174-
loginItem.Click(func() {
175-
isEnabled := isLoginItem(ctx)
176-
newState := !isEnabled
182+
item := systray.AddMenuItem(text, "Automatically start when you log in")
177183

178-
if err := setLoginItem(ctx, newState); err != nil {
179-
slog.Error("Failed to set login item", "error", err)
184+
item.Click(func() {
185+
// Prevent concurrent toggle operations.
186+
if !loginItemMu.TryLock() {
187+
slog.Debug("[LOGIN_ITEM] toggle already in progress")
180188
return
181189
}
190+
defer loginItemMu.Unlock()
191+
192+
cur := loginItemEnabled()
193+
next := !cur
194+
slog.Debug("[LOGIN_ITEM] toggling", "from", cur, "to", next)
182195

183-
// Update UI state
184-
slog.Info("[SETTINGS] Start at Login toggled", "enabled", newState)
196+
// Optimistically update cache before the slow osascript call.
197+
loginItemCached.Store(next)
198+
199+
if err := setLoginItem(ctx, next); err != nil {
200+
slog.Error("[LOGIN_ITEM] failed to set", "error", err, "enable", next)
201+
loginItemCached.Store(cur) // revert on failure
202+
go showLoginItemError(ctx, next, err)
203+
return
204+
}
185205

186-
// Rebuild menu to update checkmark
206+
slog.Info("[SETTINGS] Start at Login toggled", "enabled", next)
187207
app.rebuildMenu(ctx)
188208
})
189209
}
210+
211+
func showLoginItemError(ctx context.Context, enable bool, err error) {
212+
ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
213+
defer cancel()
214+
215+
action, verb := "enable", "adding"
216+
if !enable {
217+
action, verb = "disable", "removing"
218+
}
219+
msg := fmt.Sprintf("Could not %s 'Start at Login'.\n\nError: %v\n\n"+
220+
"Try %s reviewGOOSE manually in System Settings > General > Login Items.",
221+
action, err, verb)
222+
script := fmt.Sprintf(
223+
`display dialog %q with title "reviewGOOSE" buttons {"OK"} default button "OK" with icon caution`, msg)
224+
if out, err := exec.CommandContext(ctx, "osascript", "-e", script).CombinedOutput(); err != nil {
225+
slog.Debug("failed to show error dialog", "error", err, "output", string(out))
226+
}
227+
}

0 commit comments

Comments
 (0)