Conversation
Focus improvements: - raiseWindowByAXTitle now uses _AXUIElementGetWindow (AX SPI) + CGS APIs to switch to the correct Space before raising, replacing the Screen Recording dependency for single-window cross-Space cases. Screen Recording is still used when available (findSwitchAndActivate) as a faster first pass via CGWindowListCopyWindowInfo. - AXAllWindows used in raiseWindowByAXDocument so Ghostty windows on other Spaces are also reachable. - When the window cannot be found (e.g. Electron apps with multiple windows across Spaces where AXAllWindows is current-Space only), the app is activated so the user lands on its Space without a disruptive Space iteration. Accessibility onboarding: - promptAccessibilityOnce now opens Finder with ClaudeNotifier.app highlighted alongside Settings → Accessibility so users can drag the app directly into the list. macOS does not auto-add LSUIElement background apps in all cases. - Notification updated: "Drag the highlighted ClaudeNotifier.app into the Accessibility list, then enable the toggle." Other fixes: - Fallback timeout in send mode switches from OsascriptNotificationService to NSNotificationService (fixes Script Editor activating on click). - -persistent flag added to ClaudeNotifier for alert-style notifications; used by permission-prompt notifications so they stay visible. - GetTerminalNotifierPath falls back to executable-relative lookup when CLAUDE_PLUGIN_ROOT is unset. - focus-window usage string corrected (covers all terminals, not just VSCode). - Tests: assert persistent=false in existing cases; add testParsePersistent and testParsePersistentNotSet. - Docs: CLICK_TO_FOCUS.md permission section rewritten; troubleshooting.md adds entry for ClaudeNotifier.app missing from Accessibility list. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds Space-switching via AX SPI, AppleScript fallback for accessibility failures, persistent/alert-style notifications, expanded terminal-notifier discovery, Screen Recording Info.plist entry, logging around focus operations, and documentation updates for permissions and troubleshooting. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Notifier as Claude Notifier CLI/App
participant AX as macOS Accessibility (AX)
participant CGS as Core Graphics Server (Spaces)
participant AppleScript as AppleScript Engine
participant NS as Notification Service
User->>Notifier: Click notification / run focus-window
activate Notifier
Notifier->>AX: Query AXAllWindows / AXWindows for app window
alt AX permission granted
AX-->>Notifier: AXElement + AXPosition/AXSize
Notifier->>AX: _AXUIElementGetWindow -> CGWindowID
AX-->>Notifier: CGWindowID
Notifier->>CGS: Switch to window's Space (using CGWindowID)
CGS-->>Notifier: Space switched
Notifier->>AX: Raise window (AXPress)
AX-->>Notifier: Success
Notifier->>NS: (if configured) log success / return
else AX permission missing or lookup failed
AX-->>Notifier: Permission error / not found
Notifier->>AppleScript: activateViaAppleScript (last-resort)
AppleScript-->>Notifier: App activated (may not raise correct window)
Notifier->>User: Emit System Settings notification / prompt (copy path)
end
deactivate Notifier
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
swift-notifier/Tests/terminal-notifier-modernTests/ArgumentParserTests.swift (1)
25-39: Minor consistency gap:testParseMinimalArgsdoesn't assertconfig.persistent.Every other boolean flag (
timeSensitive,silent) has an explicitfalseassertion in this test.testParsePersistentNotSetindependently covers the same scenario, so there's no actual coverage gap — just an inconsistency.✅ Suggested addition for consistency
XCTAssertFalse(config.timeSensitive) XCTAssertFalse(config.silent) + XCTAssertFalse(config.persistent) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@swift-notifier/Tests/terminal-notifier-modernTests/ArgumentParserTests.swift` around lines 25 - 39, In testParseMinimalArgs add an explicit assertion for the persistent flag so the test mirrors the other boolean checks: update the testParseMinimalArgs function to include XCTAssertFalse(config.persistent) after the existing boolean assertions (alongside XCTAssertFalse(config.timeSensitive) and XCTAssertFalse(config.silent)) to ensure consistency in verifying default boolean values on the ArgumentParser.Config object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/notifier/ax_focus_darwin.go`:
- Around line 145-149: raiseWindowByAXDocument currently aborts if AXAllWindows
isn't present; update it to mirror raiseWindowByAXTitle's fallback: after
failing AXUIElementCopyAttributeValue(..., CFSTR("AXAllWindows"), &windowsRef)
or if windowsRef is NULL, attempt AXUIElementCopyAttributeValue(appEl,
CFSTR("AXWindows"), &windowsRef) before bailing; ensure you still
CFRelease(appEl) only after all attempts, treat windowsRef as the same CFTypeRef
array (check CFArrayCreate/CFArrayGetCount usage already in
raiseWindowByAXDocument), and CFRelease windowsRef when done to avoid leaks—use
the same error/NULL handling pattern and variable names (windowsRef, appEl) as
in raiseWindowByAXTitle.
In `@swift-notifier/Sources/terminal-notifier-modern/main.swift`:
- Around line 61-66: The timeout fallback currently runs on a background queue:
the DispatchQueue.global().asyncAfter block invokes
NSNotificationService().send(config:) which ultimately calls
NSUserNotificationCenter.default.deliver(...) off the main thread; change the
block so it dispatches back to the main queue before calling
NSNotificationService().send — e.g., inside the asyncAfter closure wrap the
NSNotificationService().send(...) call in DispatchQueue.main.async { ... } so
NSNotificationService (and NSUserNotificationCenter.default.deliver) execute on
the main thread and still call exit(ExitCode.success) in the completion handler.
In
`@swift-notifier/Sources/terminal-notifier-modern/Notification/NSNotificationService.swift`:
- Around line 28-33: The comment is wrong: display style is controlled by
NSUserNotificationAlertStyle in Info.plist, not hasActionButton; update the
codebase by either 1) changing the Info.plist NSUserNotificationAlertStyle value
from "banner" to "alert" so config.persistent takes effect (keep
NSNotificationService.swift's hasActionButton/actionButtonTitle logic:
hasActionButton = config.persistent and actionButtonTitle = "Open" in the if
config.persistent branch), or 2) remove the non-functional persistent branch
from NSNotificationService.swift (remove hasActionButton/config.persistent
conditional and actionButtonTitle assignment) and add a comment noting modern
macOS suppression (NSNotificationCenter/Sequoia) so the fallback remains
reference-only; locate references by the symbols config.persistent,
hasActionButton, actionButtonTitle, NSNotificationService.swift and the
Info.plist NSUserNotificationAlertStyle key to apply the chosen change.
---
Nitpick comments:
In
`@swift-notifier/Tests/terminal-notifier-modernTests/ArgumentParserTests.swift`:
- Around line 25-39: In testParseMinimalArgs add an explicit assertion for the
persistent flag so the test mirrors the other boolean checks: update the
testParseMinimalArgs function to include XCTAssertFalse(config.persistent) after
the existing boolean assertions (alongside XCTAssertFalse(config.timeSensitive)
and XCTAssertFalse(config.silent)) to ensure consistency in verifying default
boolean values on the ArgumentParser.Config object.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/claude-notifications/main.godocs/CLICK_TO_FOCUS.mddocs/troubleshooting.mdinternal/notifier/ax_focus_darwin.gointernal/notifier/notifier.gointernal/notifier/terminal_darwin.goswift-notifier/Resources/Info.plistswift-notifier/Sources/terminal-notifier-modern/CLI/ArgumentParser.swiftswift-notifier/Sources/terminal-notifier-modern/Notification/NSNotificationService.swiftswift-notifier/Sources/terminal-notifier-modern/main.swiftswift-notifier/Tests/terminal-notifier-modernTests/ArgumentParserTests.swift
Comment on lines
145
to
149
| // AXAllWindows returns windows across all Spaces; AXWindows is current-Space only. | ||
| CFTypeRef windowsRef = NULL; | ||
| if (AXUIElementCopyAttributeValue(appEl, CFSTR("AXWindows"), &windowsRef) != kAXErrorSuccess || !windowsRef) { | ||
| if (AXUIElementCopyAttributeValue(appEl, CFSTR("AXAllWindows"), &windowsRef) != kAXErrorSuccess || !windowsRef) { | ||
| CFRelease(appEl); | ||
| return 0; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and understand its context
find . -name "ax_focus_darwin.go" -type fRepository: 777genius/claude-notifications-go
Length of output: 115
🏁 Script executed:
# Read the file to understand the full context
cat -n internal/notifier/ax_focus_darwin.go | head -200Repository: 777genius/claude-notifications-go
Length of output: 9118
🏁 Script executed:
# Search for raiseWindowByAXTitle to verify if it has the fallback mentioned
rg -n "raiseWindowByAXTitle" internal/notifier/ax_focus_darwin.go -A 20Repository: 777genius/claude-notifications-go
Length of output: 2756
🏁 Script executed:
# Also search for AXWindows usage pattern to understand how fallback works elsewhere
rg -n "AXWindows" internal/notifier/ax_focus_darwin.goRepository: 777genius/claude-notifications-go
Length of output: 552
Add AXWindows fallback when AXAllWindows is unavailable.
raiseWindowByAXDocument hard-requires AXAllWindows without fallback. If an app (or older macOS) doesn't expose it, Ghostty focus will fail even on the current Space. raiseWindowByAXTitle already implements this fallback pattern (lines 214-224); apply the same approach here.
Proposed fix
- CFTypeRef windowsRef = NULL;
- if (AXUIElementCopyAttributeValue(appEl, CFSTR("AXAllWindows"), &windowsRef) != kAXErrorSuccess || !windowsRef) {
- CFRelease(appEl);
- return 0;
- }
+ CFTypeRef windowsRef = NULL;
+ AXError allErr = AXUIElementCopyAttributeValue(appEl, CFSTR("AXAllWindows"), &windowsRef);
+ if (allErr != kAXErrorSuccess || !windowsRef) {
+ allErr = AXUIElementCopyAttributeValue(appEl, CFSTR("AXWindows"), &windowsRef);
+ if (allErr != kAXErrorSuccess || !windowsRef) {
+ CFRelease(appEl);
+ return 0;
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // AXAllWindows returns windows across all Spaces; AXWindows is current-Space only. | |
| CFTypeRef windowsRef = NULL; | |
| if (AXUIElementCopyAttributeValue(appEl, CFSTR("AXWindows"), &windowsRef) != kAXErrorSuccess || !windowsRef) { | |
| if (AXUIElementCopyAttributeValue(appEl, CFSTR("AXAllWindows"), &windowsRef) != kAXErrorSuccess || !windowsRef) { | |
| CFRelease(appEl); | |
| return 0; | |
| // AXAllWindows returns windows across all Spaces; AXWindows is current-Space only. | |
| CFTypeRef windowsRef = NULL; | |
| AXError allErr = AXUIElementCopyAttributeValue(appEl, CFSTR("AXAllWindows"), &windowsRef); | |
| if (allErr != kAXErrorSuccess || !windowsRef) { | |
| allErr = AXUIElementCopyAttributeValue(appEl, CFSTR("AXWindows"), &windowsRef); | |
| if (allErr != kAXErrorSuccess || !windowsRef) { | |
| CFRelease(appEl); | |
| return 0; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/notifier/ax_focus_darwin.go` around lines 145 - 149,
raiseWindowByAXDocument currently aborts if AXAllWindows isn't present; update
it to mirror raiseWindowByAXTitle's fallback: after failing
AXUIElementCopyAttributeValue(..., CFSTR("AXAllWindows"), &windowsRef) or if
windowsRef is NULL, attempt AXUIElementCopyAttributeValue(appEl,
CFSTR("AXWindows"), &windowsRef) before bailing; ensure you still
CFRelease(appEl) only after all attempts, treat windowsRef as the same CFTypeRef
array (check CFArrayCreate/CFArrayGetCount usage already in
raiseWindowByAXDocument), and CFRelease windowsRef when done to avoid leaks—use
the same error/NULL handling pattern and variable names (windowsRef, appEl) as
in raiseWindowByAXTitle.
Comment on lines
61
to
66
| DispatchQueue.global().asyncAfter(deadline: .now() + 3.0) { | ||
| fputs("Warning: UNUserNotificationCenter timed out, using osascript fallback\n", stderr) | ||
| OsascriptNotificationService().send(config: config) { _ in | ||
| fputs("Warning: UNUserNotificationCenter timed out, using NSNotificationService fallback\n", stderr) | ||
| NSNotificationService().send(config: config) { _ in | ||
| exit(ExitCode.success) | ||
| } | ||
| } |
There was a problem hiding this comment.
NSUserNotificationCenter.default.deliver() called off the main thread — threading bug.
DispatchQueue.global().asyncAfter runs on a background queue, and NSNotificationService().send() calls NSUserNotificationCenter.default.deliver(notification) directly on that queue. NSUserNotificationCenter is a main-thread-only API; calling it from a background queue is undefined behaviour and can silently drop the notification or crash.
🔒 Proposed fix: dispatch into main before invoking NSNotificationService
DispatchQueue.global().asyncAfter(deadline: .now() + 3.0) {
fputs("Warning: UNUserNotificationCenter timed out, using NSNotificationService fallback\n", stderr)
- NSNotificationService().send(config: config) { _ in
- exit(ExitCode.success)
- }
+ DispatchQueue.main.async {
+ NSNotificationService().send(config: config) { _ in
+ exit(ExitCode.success)
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| DispatchQueue.global().asyncAfter(deadline: .now() + 3.0) { | |
| fputs("Warning: UNUserNotificationCenter timed out, using osascript fallback\n", stderr) | |
| OsascriptNotificationService().send(config: config) { _ in | |
| fputs("Warning: UNUserNotificationCenter timed out, using NSNotificationService fallback\n", stderr) | |
| NSNotificationService().send(config: config) { _ in | |
| exit(ExitCode.success) | |
| } | |
| } | |
| DispatchQueue.global().asyncAfter(deadline: .now() + 3.0) { | |
| fputs("Warning: UNUserNotificationCenter timed out, using NSNotificationService fallback\n", stderr) | |
| DispatchQueue.main.async { | |
| NSNotificationService().send(config: config) { _ in | |
| exit(ExitCode.success) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@swift-notifier/Sources/terminal-notifier-modern/main.swift` around lines 61 -
66, The timeout fallback currently runs on a background queue: the
DispatchQueue.global().asyncAfter block invokes
NSNotificationService().send(config:) which ultimately calls
NSUserNotificationCenter.default.deliver(...) off the main thread; change the
block so it dispatches back to the main queue before calling
NSNotificationService().send — e.g., inside the asyncAfter closure wrap the
NSNotificationService().send(...) call in DispatchQueue.main.async { ... } so
NSNotificationService (and NSUserNotificationCenter.default.deliver) execute on
the main thread and still call exit(ExitCode.success) in the completion handler.
Comment on lines
+28
to
+33
| // Persistent notifications use alert style (stays until dismissed). | ||
| // Non-persistent use banner style (auto-dismisses). | ||
| notification.hasActionButton = config.persistent | ||
| if config.persistent { | ||
| notification.actionButtonTitle = "Open" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Info.plist exists and contains NSUserNotificationAlertStyle
fd -e plist "Info.plist" | head -20Repository: 777genius/claude-notifications-go
Length of output: 112
🏁 Script executed:
#!/bin/bash
# Look at the NSNotificationService file to understand context
fd -type f -name "NSNotificationService.swift" | head -5Repository: 777genius/claude-notifications-go
Length of output: 247
🏁 Script executed:
#!/bin/bash
# Get the full content of NSNotificationService.swift to understand if it's active or reference code
fd -type f -name "NSNotificationService.swift" --exec wc -l {} \;Repository: 777genius/claude-notifications-go
Length of output: 247
🏁 Script executed:
#!/bin/bash
# Read the Info.plist file
cat swift-notifier/Resources/Info.plistRepository: 777genius/claude-notifications-go
Length of output: 1148
🏁 Script executed:
#!/bin/bash
# Find and read NSNotificationService.swift
find . -name "NSNotificationService.swift" -exec cat -n {} \;Repository: 777genius/claude-notifications-go
Length of output: 3052
hasActionButton = true does not produce alert-style (persistent) notifications — the comment is incorrect.
NSUserNotificationAlertStyle in Info.plist controls the notification display style. The app explicitly sets this to banner (auto-dismiss), not alert (stays until dismissed). This means all notifications will auto-dismiss regardless of hasActionButton, making the persistent code path non-functional. The hasActionButton property only controls whether the action button is shown; it has no effect on display style.
Additionally, per the codebase learnings, NSUserNotificationCenter is silently suppressed on macOS Sequoia, and this service is kept as reference code. The entire fallback path will not function on modern macOS regardless.
Change NSUserNotificationAlertStyle from banner to alert in Info.plist, or remove the non-functional persistent logic from this service.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@swift-notifier/Sources/terminal-notifier-modern/Notification/NSNotificationService.swift`
around lines 28 - 33, The comment is wrong: display style is controlled by
NSUserNotificationAlertStyle in Info.plist, not hasActionButton; update the
codebase by either 1) changing the Info.plist NSUserNotificationAlertStyle value
from "banner" to "alert" so config.persistent takes effect (keep
NSNotificationService.swift's hasActionButton/actionButtonTitle logic:
hasActionButton = config.persistent and actionButtonTitle = "Open" in the if
config.persistent branch), or 2) remove the non-functional persistent branch
from NSNotificationService.swift (remove hasActionButton/config.persistent
conditional and actionButtonTitle assignment) and add a comment noting modern
macOS suppression (NSNotificationCenter/Sequoia) so the fallback remains
reference-only; locate references by the symbols config.persistent,
hasActionButton, actionButtonTitle, NSNotificationService.swift and the
Info.plist NSUserNotificationAlertStyle key to apply the chosen change.
…oarding macOS does not allow background LSUIElement apps to bring Finder to the foreground (open -R, osascript, etc. all fail silently without Automation permission). Instead, copy the ClaudeNotifier.app path to the clipboard via pbcopy so the user can paste it into Finder's ⌘⇧G dialog and drag the app into the Accessibility / Screen Recording list. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
internal/notifier/ax_focus_darwin.go (1)
145-150:raiseWindowByAXDocumentstill lacksAXWindowsfallback.The new comment at line 145 documents the AXAllWindows vs AXWindows distinction, but the code still hard-fails when
AXAllWindowsis unavailable — unlikeraiseWindowByAXTitle(lines 214–224) which gracefully falls back. If Ghostty (or a future AXDocument-based app) doesn't exposeAXAllWindows, focus will silently fail on the current Space.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/notifier/ax_focus_darwin.go` around lines 145 - 150, raiseWindowByAXDocument currently aborts when AXAllWindows isn't available, causing silent failures on apps that only expose AXWindows; change it to mirror raiseWindowByAXTitle by first trying AXAllWindows via AXUIElementCopyAttributeValue(appEl, CFSTR("AXAllWindows"), &windowsRef) and if that fails or returns NULL, retry with CFSTR("AXWindows") before bailing. Ensure the same CFTypeRef windowsRef lifecycle and CFRelease(appEl) behavior is preserved and that any code handling the returned windows array (iteration, casting, CFRelease of windowsRef) matches the fallback path used in raiseWindowByAXTitle.
🧹 Nitpick comments (3)
docs/troubleshooting.md (1)
15-15: Ambiguous phrasing: "before seeing this"."before seeing this" is unclear — it could mean before seeing this document or before reading the notification content. Consider "If you dismissed the notification without completing the steps above" for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/troubleshooting.md` at line 15, Replace the ambiguous sentence "If you dismissed the notification before seeing this," with a clearer phrasing such as "If you dismissed the notification without completing the steps above, trigger it again by deleting the marker file and clicking any notification:" to remove ambiguity about whether "this" refers to the document or the notification content; update the line in docs/troubleshooting.md where that exact sentence appears.internal/notifier/ax_focus_darwin.go (2)
392-465:promptScreenRecordingOnceandpromptAccessibilityOnceshare ~90% identical logic.Both functions follow the same pattern: check marker file → write marker → derive
ClaudeNotifier.apppath → buildpbcopy + open Settingscommand → send notification. Only the marker filename, Settings URL fragment, and notification text differ. Consider extracting a shared helper to reduce duplication.Sketch of a shared helper
func promptPermissionOnce(markerName, settingsFragment, title, message string) { stableDir, err := config.GetStableConfigDir() if err != nil { return } markerPath := filepath.Join(stableDir, markerName) if _, err := os.Stat(markerPath); err == nil { return } _ = os.MkdirAll(stableDir, 0755) _ = os.WriteFile(markerPath, []byte("1"), 0644) executeCmd := fmt.Sprintf(`open "x-apple.systempreferences:com.apple.preference.security?%s"`, settingsFragment) if exe, err := os.Executable(); err == nil { appPath := filepath.Join(filepath.Dir(exe), "ClaudeNotifier.app") if _, err := os.Stat(appPath); err == nil { shellSafe := strings.ReplaceAll(appPath, "'", "'\\''") executeCmd = fmt.Sprintf( `printf %%s '%s' | pbcopy; open "x-apple.systempreferences:com.apple.preference.security?%s"`, shellSafe, settingsFragment, ) } } _ = SendQuickNotification(title, message, executeCmd) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/notifier/ax_focus_darwin.go` around lines 392 - 465, promptScreenRecordingOnce and promptAccessibilityOnce duplicate almost all logic; extract a shared helper (e.g., promptPermissionOnce) that accepts markerName, settingsFragment, title, and message, reusing config.GetStableConfigDir, the marker file create/check, building executeCmd (including deriving ClaudeNotifier.app via os.Executable and using pbcopy when appPath exists), and calling SendQuickNotification; then replace both promptScreenRecordingOnce and promptAccessibilityOnce with thin wrappers that call this helper using their respective marker filenames (".screen-recording-prompted" / ".accessibility-prompted"), settings fragments ("Privacy_ScreenCapture" / "Privacy_Accessibility"), and their notification title/messages.
374-389: All non-Ghostty paths now returnnil— callers lose visibility into partial failures.Both
result < 0(no Accessibility) andresult == 0(window not found) now returnnilinstead of an error. The rationale is sound —activateByPID/AppleScript already brought the app forward — but callers cannot distinguish full success from "app activated but wrong window visible." Consider returning a sentinel or logging at a higher level so partial focus failures are observable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/notifier/ax_focus_darwin.go` around lines 374 - 389, The switch currently swallows partial failures by returning nil for both result < 0 and result == 0; change it to return distinct sentinel errors so callers can observe partial-focus conditions. Define package-level errors (e.g., ErrNoAccessibility and ErrWindowNotFound) and return ErrNoAccessibility after calling activateViaAppleScript and promptAccessibilityOnce (for result < 0), and return ErrWindowNotFound for result == 0 (window not found/AXAllWindows on another Space); keep activateByPID/activateViaAppleScript/promptAccessibilityOnce behavior intact and update callers to handle these new errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/troubleshooting.md`:
- Line 33: The Screen Recording instruction currently tells users to press ⌘⇧G
in Finder after the notification lands them in "System Settings → Privacy &
Security → Screen Recording", but omits the explicit step to switch to Finder;
update the paragraph that mentions "On first use... copies the path to
`ClaudeNotifier.app` to your clipboard" to insert a clear step like "Switch to
Finder (click the desktop or use ⌘+Tab to select Finder)" before "In Finder
press ⌘⇧G, paste, then drag `ClaudeNotifier.app` into the list and enable the
toggle" so users know they must open Finder first.
- Line 13: The docs step misses that ⌘⇧G only works when Finder is active;
update the sentence that begins "Clicking the notification opens Settings..." to
instruct the user to switch to Finder first (e.g., "switch to Finder (click the
Desktop or press ⌘⇧Tab)"), then press ⌘⇧G, paste the path to
`ClaudeNotifier.app`, and drag it into the Accessibility list to enable the
toggle; ensure the wording around "⌘⇧G" explicitly says it must be used in
Finder.
In `@internal/notifier/ax_focus_darwin.go`:
- Around line 308-315: The activateViaAppleScript function currently
interpolates bundleID directly into an AppleScript string, which allows
malformed or quote-containing bundle IDs to break or inject into the script; fix
by validating/sanitizing bundleID before building the script (in
activateViaAppleScript): reject or return when bundleID contains characters
outside a safe whitelist (e.g. letters, digits, dots, hyphens, and underscores)
using a regex check, or alternatively escape any double-quote chars in bundleID
before interpolation, and log/handle the invalid case rather than executing
osascript with an unsafe string.
- Around line 416-419: The current use of fmt.Sprintf with %q to build
executeCmd is not shell-safe for paths containing shell metacharacters; change
construction of executeCmd to use single-quote shell quoting with proper
escaping of embedded single quotes (replace each ' with '\'' or equivalent) so
the appPath is passed to printf '%s' '...'; update the same pattern in the
promptAccessibilityOnce code path as well (look for executeCmd and the
promptAccessibilityOnce function) to ensure both screen-capture and
accessibility prompts use shell-safe single-quoted arguments.
---
Duplicate comments:
In `@internal/notifier/ax_focus_darwin.go`:
- Around line 145-150: raiseWindowByAXDocument currently aborts when
AXAllWindows isn't available, causing silent failures on apps that only expose
AXWindows; change it to mirror raiseWindowByAXTitle by first trying AXAllWindows
via AXUIElementCopyAttributeValue(appEl, CFSTR("AXAllWindows"), &windowsRef) and
if that fails or returns NULL, retry with CFSTR("AXWindows") before bailing.
Ensure the same CFTypeRef windowsRef lifecycle and CFRelease(appEl) behavior is
preserved and that any code handling the returned windows array (iteration,
casting, CFRelease of windowsRef) matches the fallback path used in
raiseWindowByAXTitle.
---
Nitpick comments:
In `@docs/troubleshooting.md`:
- Line 15: Replace the ambiguous sentence "If you dismissed the notification
before seeing this," with a clearer phrasing such as "If you dismissed the
notification without completing the steps above, trigger it again by deleting
the marker file and clicking any notification:" to remove ambiguity about
whether "this" refers to the document or the notification content; update the
line in docs/troubleshooting.md where that exact sentence appears.
In `@internal/notifier/ax_focus_darwin.go`:
- Around line 392-465: promptScreenRecordingOnce and promptAccessibilityOnce
duplicate almost all logic; extract a shared helper (e.g., promptPermissionOnce)
that accepts markerName, settingsFragment, title, and message, reusing
config.GetStableConfigDir, the marker file create/check, building executeCmd
(including deriving ClaudeNotifier.app via os.Executable and using pbcopy when
appPath exists), and calling SendQuickNotification; then replace both
promptScreenRecordingOnce and promptAccessibilityOnce with thin wrappers that
call this helper using their respective marker filenames
(".screen-recording-prompted" / ".accessibility-prompted"), settings fragments
("Privacy_ScreenCapture" / "Privacy_Accessibility"), and their notification
title/messages.
- Around line 374-389: The switch currently swallows partial failures by
returning nil for both result < 0 and result == 0; change it to return distinct
sentinel errors so callers can observe partial-focus conditions. Define
package-level errors (e.g., ErrNoAccessibility and ErrWindowNotFound) and return
ErrNoAccessibility after calling activateViaAppleScript and
promptAccessibilityOnce (for result < 0), and return ErrWindowNotFound for
result == 0 (window not found/AXAllWindows on another Space); keep
activateByPID/activateViaAppleScript/promptAccessibilityOnce behavior intact and
update callers to handle these new errors.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/CLICK_TO_FOCUS.mddocs/troubleshooting.mdinternal/notifier/ax_focus_darwin.go
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/CLICK_TO_FOCUS.md
|
|
||
| ### Fix | ||
|
|
||
| Clicking the notification opens Settings and copies the path to `ClaudeNotifier.app` to your clipboard. In Finder press **⌘⇧G**, paste, then drag `ClaudeNotifier.app` into the Accessibility list and enable the toggle. |
There was a problem hiding this comment.
Missing "open Finder" step before ⌘⇧G.
⌘⇧G ("Go to Folder") only works when Finder is the active application. Since clicking the notification brings the user directly to System Settings, they need to switch to Finder first before the shortcut works. The instruction as written will leave users confused if they try ⌘⇧G while System Settings is focused.
✏️ Suggested wording
-Clicking the notification opens Settings and copies the path to `ClaudeNotifier.app` to your clipboard. In Finder press **⌘⇧G**, paste, then drag `ClaudeNotifier.app` into the Accessibility list and enable the toggle.
+Clicking the notification opens **System Settings → Privacy & Security → Accessibility** and copies the path to `ClaudeNotifier.app` to your clipboard. Open **Finder**, press **⌘⇧G**, paste the path, and press **Go**. Then drag `ClaudeNotifier.app` into the Accessibility list and enable the toggle.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Clicking the notification opens Settings and copies the path to `ClaudeNotifier.app` to your clipboard. In Finder press **⌘⇧G**, paste, then drag `ClaudeNotifier.app` into the Accessibility list and enable the toggle. | |
| Clicking the notification opens **System Settings → Privacy & Security → Accessibility** and copies the path to `ClaudeNotifier.app` to your clipboard. Open **Finder**, press **⌘⇧G**, paste the path, and press **Go**. Then drag `ClaudeNotifier.app` into the Accessibility list and enable the toggle. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/troubleshooting.md` at line 13, The docs step misses that ⌘⇧G only works
when Finder is active; update the sentence that begins "Clicking the
notification opens Settings..." to instruct the user to switch to Finder first
(e.g., "switch to Finder (click the Desktop or press ⌘⇧Tab)"), then press ⌘⇧G,
paste the path to `ClaudeNotifier.app`, and drag it into the Accessibility list
to enable the toggle; ensure the wording around "⌘⇧G" explicitly says it must be
used in Finder.
| ### Fix | ||
|
|
||
| On first use the binary requests Screen Recording access automatically — a macOS dialog will appear. If you dismissed it: | ||
| On first use you will receive a notification. Clicking it opens **System Settings → Privacy & Security → Screen Recording** and copies the path to `ClaudeNotifier.app` to your clipboard. In Finder press **⌘⇧G**, paste, then drag `ClaudeNotifier.app` into the list and enable the toggle. |
There was a problem hiding this comment.
Same missing "open Finder" step in the Screen Recording fix.
Identical issue: the user lands in System Settings → Screen Recording after clicking the notification, so ⌘⇧G won't work until they explicitly switch to Finder.
✏️ Suggested wording
-On first use you will receive a notification. Clicking it opens **System Settings → Privacy & Security → Screen Recording** and copies the path to `ClaudeNotifier.app` to your clipboard. In Finder press **⌘⇧G**, paste, then drag `ClaudeNotifier.app` into the list and enable the toggle.
+On first use you will receive a notification. Clicking it opens **System Settings → Privacy & Security → Screen Recording** and copies the path to `ClaudeNotifier.app` to your clipboard. Open **Finder**, press **⌘⇧G**, paste the path, and press **Go**. Then drag `ClaudeNotifier.app` into the list and enable the toggle.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| On first use you will receive a notification. Clicking it opens **System Settings → Privacy & Security → Screen Recording** and copies the path to `ClaudeNotifier.app` to your clipboard. In Finder press **⌘⇧G**, paste, then drag `ClaudeNotifier.app` into the list and enable the toggle. | |
| On first use you will receive a notification. Clicking it opens **System Settings → Privacy & Security → Screen Recording** and copies the path to `ClaudeNotifier.app` to your clipboard. Open **Finder**, press **⌘⇧G**, paste the path, and press **Go**. Then drag `ClaudeNotifier.app` into the list and enable the toggle. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/troubleshooting.md` at line 33, The Screen Recording instruction
currently tells users to press ⌘⇧G in Finder after the notification lands them
in "System Settings → Privacy & Security → Screen Recording", but omits the
explicit step to switch to Finder; update the paragraph that mentions "On first
use... copies the path to `ClaudeNotifier.app` to your clipboard" to insert a
clear step like "Switch to Finder (click the desktop or use ⌘+Tab to select
Finder)" before "In Finder press ⌘⇧G, paste, then drag `ClaudeNotifier.app` into
the list and enable the toggle" so users know they must open Finder first.
Comment on lines
+308
to
+315
| // activateViaAppleScript sends a bare "activate" AppleScript to the app. | ||
| // Used as a last-resort when NSRunningApplication.activate may not work | ||
| // (e.g. no window-server context in certain subprocess scenarios). | ||
| // Does not enumerate or focus any specific window — just brings the app to front. | ||
| func activateViaAppleScript(bundleID string) { | ||
| script := fmt.Sprintf(`tell application id "%s" to activate`, bundleID) | ||
| _ = exec.Command("osascript", "-e", script).Run() | ||
| } |
There was a problem hiding this comment.
Minor injection risk: bundleID is interpolated unsanitized into AppleScript.
If bundleID ever contains a double-quote character, the generated tell application id "..." to activate string becomes syntactically broken or potentially injectable. In practice bundle IDs are controlled reverse-DNS strings, but a simple validation or escaping step would harden this.
Suggested defensive check
func activateViaAppleScript(bundleID string) {
+ // Bundle IDs should be reverse-DNS; reject anything suspicious.
+ for _, c := range bundleID {
+ if c == '"' || c == '\\' || c == '\'' {
+ return
+ }
+ }
script := fmt.Sprintf(`tell application id "%s" to activate`, bundleID)
_ = exec.Command("osascript", "-e", script).Run()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // activateViaAppleScript sends a bare "activate" AppleScript to the app. | |
| // Used as a last-resort when NSRunningApplication.activate may not work | |
| // (e.g. no window-server context in certain subprocess scenarios). | |
| // Does not enumerate or focus any specific window — just brings the app to front. | |
| func activateViaAppleScript(bundleID string) { | |
| script := fmt.Sprintf(`tell application id "%s" to activate`, bundleID) | |
| _ = exec.Command("osascript", "-e", script).Run() | |
| } | |
| // activateViaAppleScript sends a bare "activate" AppleScript to the app. | |
| // Used as a last-resort when NSRunningApplication.activate may not work | |
| // (e.g. no window-server context in certain subprocess scenarios). | |
| // Does not enumerate or focus any specific window — just brings the app to front. | |
| func activateViaAppleScript(bundleID string) { | |
| // Bundle IDs should be reverse-DNS; reject anything suspicious. | |
| for _, c := range bundleID { | |
| if c == '"' || c == '\\' || c == '\'' { | |
| return | |
| } | |
| } | |
| script := fmt.Sprintf(`tell application id "%s" to activate`, bundleID) | |
| _ = exec.Command("osascript", "-e", script).Run() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/notifier/ax_focus_darwin.go` around lines 308 - 315, The
activateViaAppleScript function currently interpolates bundleID directly into an
AppleScript string, which allows malformed or quote-containing bundle IDs to
break or inject into the script; fix by validating/sanitizing bundleID before
building the script (in activateViaAppleScript): reject or return when bundleID
contains characters outside a safe whitelist (e.g. letters, digits, dots,
hyphens, and underscores) using a regex check, or alternatively escape any
double-quote chars in bundleID before interpolation, and log/handle the invalid
case rather than executing osascript with an unsafe string.
Comment on lines
+416
to
+419
| executeCmd = fmt.Sprintf( | ||
| `printf %%s %q | pbcopy; open "x-apple.systempreferences:com.apple.preference.security?Privacy_ScreenCapture"`, | ||
| appPath, | ||
| ) |
There was a problem hiding this comment.
Go %q is not shell-safe quoting — fragile for paths with $, backticks, or !.
fmt.Sprintf("printf %%s %q | pbcopy; ...", appPath) uses Go's %q verb, which produces Go-escaped strings (e.g., "foo$bar" without escaping $). If appPath contains shell metacharacters ($, `, !), the shell will interpret them. Realistic macOS binary paths won't hit this, but a single-quoted format would be more robust.
Safer shell quoting
- executeCmd = fmt.Sprintf(
- `printf %%s %q | pbcopy; open "x-apple.systempreferences:com.apple.preference.security?Privacy_ScreenCapture"`,
- appPath,
- )
+ // Use single quotes for shell-safe quoting; escape embedded single quotes.
+ shellSafe := strings.ReplaceAll(appPath, "'", "'\\''")
+ executeCmd = fmt.Sprintf(
+ `printf %%s '%s' | pbcopy; open "x-apple.systempreferences:com.apple.preference.security?Privacy_ScreenCapture"`,
+ shellSafe,
+ )Apply the same pattern in promptAccessibilityOnce (line 455).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| executeCmd = fmt.Sprintf( | |
| `printf %%s %q | pbcopy; open "x-apple.systempreferences:com.apple.preference.security?Privacy_ScreenCapture"`, | |
| appPath, | |
| ) | |
| // Use single quotes for shell-safe quoting; escape embedded single quotes. | |
| shellSafe := strings.ReplaceAll(appPath, "'", "'\\''") | |
| executeCmd = fmt.Sprintf( | |
| `printf %%s '%s' | pbcopy; open "x-apple.systempreferences:com.apple.preference.security?Privacy_ScreenCapture"`, | |
| shellSafe, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/notifier/ax_focus_darwin.go` around lines 416 - 419, The current use
of fmt.Sprintf with %q to build executeCmd is not shell-safe for paths
containing shell metacharacters; change construction of executeCmd to use
single-quote shell quoting with proper escaping of embedded single quotes
(replace each ' with '\'' or equivalent) so the appPath is passed to printf '%s'
'...'; update the same pattern in the promptAccessibilityOnce code path as well
(look for executeCmd and the promptAccessibilityOnce function) to ensure both
screen-capture and accessibility prompts use shell-safe single-quoted arguments.
wagenet
marked this pull request as draft
February 24, 2026 21:54
Contributor
Author
|
I think my recent PR had some regressions. Working through fixes here. |
Owner
|
Thanks for the heads up and for jumping on the fixes! Really appreciate the thoroughness. Just ping me when you're ready for review. |
- Add ScreenCaptureKit-based window lookup (findWindowIDSCK) as fallback when CGWindowListCopyWindowInfo returns no titles (macOS 15+ regression) - Add SCK-based Space switching for Ghostty (findGhosttySpaceViaSCK) using sub-window/container geometry matching - Add AXWindows fallback to raiseWindowByAXDocument; Ghostty does not implement AXAllWindows (kAXErrorAttributeUnsupported) - Add Space switching inside raiseWindowByAXDocument via _AXUIElementGetWindow + switchToWindowSpace, matching raiseWindowByAXTitle behavior - Replace activateByPID with activateViaAppleScript for Ghostty (reliable in subprocess/terminal-notifier -execute context) - Remove focusVSCodeViaCLI which opened new VS Code windows instead of focusing existing ones - Add one-time Screen Recording permission prompt for cross-space focus - Add AX diagnostic helper (diagAXWindows) and cross-space regression test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
raiseWindowByAXTitlenow uses_AXUIElementGetWindow(AX SPI) + CGS APIs to switch to the correct Space for non-Electron apps that support cross-SpaceAXAllWindows. VS Code cross-Space focus still requires Screen Recording (used byfindSwitchAndActivateto identify the correct window viakCGWindowName).promptAccessibilityOnceandpromptScreenRecordingOncenow copy theClaudeNotifier.apppath to the clipboard viapbcopy, then open the relevant System Settings pane. macOS does not allowLSUIElementbackground apps to bring Finder to the foreground (open -R and osascript require Automation permission), so the clipboard + ⌘⇧G workflow is the reliable path for dragging the app into the privacy list.OsascriptNotificationServicetoNSNotificationService, fixing Script Editor activating on notification click.-persistentflag: new flag for alert-style (stays until dismissed) notifications; used for permission-prompt notifications so they are not missed.CLICK_TO_FOCUS.mdpermission section rewritten with clipboard + ⌘⇧G instructions and clarified Screen Recording requirement;troubleshooting.mdadds entry forClaudeNotifier.appmissing from Accessibility list and updates Screen Recording section.Test plan
~/.claude/claude-notifications-go/.accessibility-promptedand click a notification without Accessibility granted → notification fires, clicking it copies theClaudeNotifier.apppath to clipboard and opens Settings → AccessibilityClaudeNotifier.appinto the Accessibility list, enable toggle, click notification again → focus works.screen-recording-promptedand Screen Recordingswift testinswift-notifier/🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation