fix(v3/darwin): systray highlight and attached window focus stealing#4963
fix(v3/darwin): systray highlight and attached window focus stealing#4963leaanthony wants to merge 2 commits intov3-alphafrom
Conversation
…4910) Fix two macOS systray issues: - Set button highlight before showing popup menu so the icon visually responds to clicks - Show attached windows with orderFrontRegardless instead of activateIgnoringOtherApps to prevent stealing focus from other apps Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
WalkthroughChanges implement platform-specific system tray click handling and window management for macOS. A new focus-respecting window toggle method replaces default click behavior on macOS, while cross-platform fallback logic is added for other operating systems. Native helpers expose highlight state control and focus-less window display. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Fixes macOS system tray UX issues by improving status item highlight behavior and preventing attached windows from stealing focus.
Changes:
- Highlight the NSStatusItem button before showing the status item menu on macOS
- Add native helpers for setting highlight state and showing a window without activating the app
- Move macOS attached-window toggle behavior into
processClick(and avoid setting default click handler for darwin)
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| v3/pkg/application/systemtray_darwin.m | Highlights status item before menu popup; adds new native helpers for highlight and non-activating window show |
| v3/pkg/application/systemtray_darwin.h | Exposes new native helper function declarations to cgo |
| v3/pkg/application/systemtray_darwin.go | Adds macOS-specific attached window toggling that avoids app activation |
| v3/pkg/application/systemtray.go | Skips setting default click handler on macOS so darwin processClick can handle window toggling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| NSStatusItem *statusItem = (NSStatusItem *)nsStatusItem; | ||
| [statusItem.button highlight:YES]; | ||
| [statusItem popUpStatusItemMenu:(NSMenu *)nsMenu]; | ||
| // Post a mouse up event so the statusitem defocuses |
There was a problem hiding this comment.
highlight:YES is set before popUpStatusItemMenu: but never explicitly cleared. Since popUpStatusItemMenu: returns after menu tracking finishes, consider calling [statusItem.button highlight:NO]; immediately after it returns (or in a finally-style pattern) to avoid the icon remaining highlighted in cases where the synthetic mouse-up doesn’t clear the highlight state.
| int statusBarHeight(); | ||
| void systemTrayPositionWindow(void* nsStatusItem, void* nsWindow, int offset); No newline at end of file | ||
| void systemTrayPositionWindow(void* nsStatusItem, void* nsWindow, int offset); | ||
| void systemTraySetHighlight(void* nsStatusItem, bool highlighted); |
There was a problem hiding this comment.
bool requires <stdbool.h> in C/Objective-C headers (otherwise it may not compile under cgo/clang). Add #include <stdbool.h> to this header (or change the parameter type to _Bool / BOOL consistently with the rest of the native API).
| NSWindow *window = (NSWindow *)nsWindow; | ||
| [window orderFrontRegardless]; |
There was a problem hiding this comment.
AppKit APIs must be called on the main thread. showMenu already dispatches to the main queue, but systemTrayShowWindowWithoutActivation doesn’t. To avoid thread-safety issues/crashes, wrap the orderFrontRegardless call in dispatch_async(dispatch_get_main_queue(), ^{ ... }); (or otherwise guarantee main-thread execution at this boundary).
| NSWindow *window = (NSWindow *)nsWindow; | |
| [window orderFrontRegardless]; | |
| dispatch_async(dispatch_get_main_queue(), ^{ | |
| NSWindow *window = (NSWindow *)nsWindow; | |
| [window orderFrontRegardless]; | |
| }); |
| _ = s.parent.PositionWindow(w.Window, w.Offset) | ||
| nativeWindow := w.Window.NativeWindow() | ||
| if nativeWindow != nil { | ||
| C.systemTrayShowWindowWithoutActivation(nativeWindow) |
There was a problem hiding this comment.
If NativeWindow() returns nil, the window never gets shown in the else branch. Consider adding a fallback path (e.g., call the cross-platform Show()/equivalent) so clicking the tray icon still makes the attached window visible even when NativeWindow() is temporarily unavailable.
| C.systemTrayShowWindowWithoutActivation(nativeWindow) | |
| C.systemTrayShowWindowWithoutActivation(nativeWindow) | |
| } else { | |
| // Fallback: ensure the window is still shown if the native window is unavailable. | |
| w.Window.Show() |
| // Handle attached window toggle without stealing focus | ||
| if s.parent.attachedWindow.Window != nil { | ||
| s.parent.defaultClickHandler() | ||
| s.toggleAttachedWindow() | ||
| } |
There was a problem hiding this comment.
This attached-window toggle block appears redundant/unreachable in the leftButtonDown flow because earlier in the same case you already check attachedWindow.Window != nil, call toggleAttachedWindow(), and return. Consider removing this duplicate block or restructuring so there’s a single, clearly reachable toggle path.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@v3/pkg/application/systemtray_darwin.go`:
- Around line 258-278: The function macosSystemTray.toggleAttachedWindow
currently copies s.parent.attachedWindow into a local struct (w), so mutations
to w.initialClick and w.hasBeenShown are lost; change the function to operate on
a pointer to the parent's WindowAttachConfig (e.g. wp :=
&s.parent.attachedWindow) and use wp throughout: check wp.Window for nil, call
wp.initialClick.Do(...), set wp.hasBeenShown, pass wp.Window and wp.Offset into
s.parent.PositionWindow, and use wp.Window.NativeWindow() before calling
C.systemTrayShowWindowWithoutActivation so all state changes affect the original
struct rather than a temporary copy.
In `@v3/pkg/application/systemtray_darwin.m`:
- Around line 164-167: The function systemTraySetHighlight is unused and should
be removed or documented: either delete its implementation here and remove its
declaration from the header (and any exported symbol registrations) to avoid
dead code, or keep it but add a clear TODO comment above systemTraySetHighlight
explaining its intended future use, why it remains declared in the header, and
mark it static/internal if it should not be exported; locate the implementation
referencing NSStatusItem and statusItem.button highlight and the corresponding
declaration in the header to apply the change.
🧹 Nitpick comments (3)
v3/pkg/application/systemtray.go (2)
109-115: Consider a build-tag-based approach instead ofruntime.GOOS.This file is shared across all platforms (no build tags). The project convention (per other platform-specific files) is to use compile-time build constraints rather than runtime OS checks. A cleaner alternative would be to extract the darwin-specific default into a platform-specific file (e.g., a
systraySmartDefaults_darwin.go/systraySmartDefaults_other.gopair or a small hook function), keepingapplySmartDefaultsfree ofruntime.GOOSchecks.Note that line 131 (
ToggleWindow) and line 155 (defaultClickHandler) also haveruntime.GOOS == "windows"checks—those would benefit from the same treatment in a follow-up.Based on learnings: "Reviewers should ensure there are no runtime OS checks in system.go and that platform-specific behavior is controlled via build tags. If runtime switches exist, remove them in favor of compile-time platform constraints to reduce overhead and improve correctness."
144-166: Remove the unuseddefaultClickHandlermethod.The function is unreachable dead code. On darwin,
processClickroutes attached window toggle directly totoggleAttachedWindow(). On non-darwin platforms,applySmartDefaultssetsclickHandler = s.ToggleWindowfor attached windows, and Windows provides a fallback empty callback.defaultClickHandleris never called in production code—only in test benchmarks.v3/pkg/application/systemtray_darwin.m (1)
169-172: Ensure this is only called from the main thread.
orderFrontRegardlessis an AppKit call that must execute on the main thread. Currently it's called fromtoggleAttachedWindow→processClick→systrayClickCallback, which originates from a UI event on the main thread, so this should be safe. However, unlikeshowMenu(which wraps indispatch_async), this function has no main-thread guard—if it's ever called from a background context in the future, it would silently break.Consider wrapping with
dispatch_async(dispatch_get_main_queue(), ...)for defensive safety, or at minimum add a comment noting the main-thread requirement.
| func (s *macosSystemTray) toggleAttachedWindow() { | ||
| w := s.parent.attachedWindow | ||
| if w.Window == nil { | ||
| return | ||
| } | ||
|
|
||
| w.initialClick.Do(func() { | ||
| w.hasBeenShown = w.Window.IsVisible() | ||
| }) | ||
|
|
||
| if w.Window.IsVisible() { | ||
| w.Window.Hide() | ||
| } else { | ||
| w.hasBeenShown = true | ||
| _ = s.parent.PositionWindow(w.Window, w.Offset) | ||
| nativeWindow := w.Window.NativeWindow() | ||
| if nativeWindow != nil { | ||
| C.systemTrayShowWindowWithoutActivation(nativeWindow) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: w is a value copy of attachedWindow—mutations and sync.Once.Do are lost.
WindowAttachConfig is a struct (not a pointer), so w := s.parent.attachedWindow creates a shallow copy. This means:
sync.Oncecopy:w.initialClick.Do(...)fires on the copy; the original'sinitialClickremains in the "not done" state, so it will fire again if anything callsToggleWindowordefaultClickHandleron the original.hasBeenShownmutations lost: Lines 265 and 271 sethasBeenShownon the copy, which is discarded at the end of the function. The original struct retains its old value.
Use a pointer to the parent's struct instead:
🐛 Fix: use a pointer instead of a copy
func (s *macosSystemTray) toggleAttachedWindow() {
- w := s.parent.attachedWindow
+ w := &s.parent.attachedWindow
if w.Window == nil {
return
}📝 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.
| func (s *macosSystemTray) toggleAttachedWindow() { | |
| w := s.parent.attachedWindow | |
| if w.Window == nil { | |
| return | |
| } | |
| w.initialClick.Do(func() { | |
| w.hasBeenShown = w.Window.IsVisible() | |
| }) | |
| if w.Window.IsVisible() { | |
| w.Window.Hide() | |
| } else { | |
| w.hasBeenShown = true | |
| _ = s.parent.PositionWindow(w.Window, w.Offset) | |
| nativeWindow := w.Window.NativeWindow() | |
| if nativeWindow != nil { | |
| C.systemTrayShowWindowWithoutActivation(nativeWindow) | |
| } | |
| } | |
| } | |
| func (s *macosSystemTray) toggleAttachedWindow() { | |
| w := &s.parent.attachedWindow | |
| if w.Window == nil { | |
| return | |
| } | |
| w.initialClick.Do(func() { | |
| w.hasBeenShown = w.Window.IsVisible() | |
| }) | |
| if w.Window.IsVisible() { | |
| w.Window.Hide() | |
| } else { | |
| w.hasBeenShown = true | |
| _ = s.parent.PositionWindow(w.Window, w.Offset) | |
| nativeWindow := w.Window.NativeWindow() | |
| if nativeWindow != nil { | |
| C.systemTrayShowWindowWithoutActivation(nativeWindow) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@v3/pkg/application/systemtray_darwin.go` around lines 258 - 278, The function
macosSystemTray.toggleAttachedWindow currently copies s.parent.attachedWindow
into a local struct (w), so mutations to w.initialClick and w.hasBeenShown are
lost; change the function to operate on a pointer to the parent's
WindowAttachConfig (e.g. wp := &s.parent.attachedWindow) and use wp throughout:
check wp.Window for nil, call wp.initialClick.Do(...), set wp.hasBeenShown, pass
wp.Window and wp.Offset into s.parent.PositionWindow, and use
wp.Window.NativeWindow() before calling C.systemTrayShowWindowWithoutActivation
so all state changes affect the original struct rather than a temporary copy.
| void systemTraySetHighlight(void* nsStatusItem, bool highlighted) { | ||
| NSStatusItem *statusItem = (NSStatusItem *)nsStatusItem; | ||
| [statusItem.button highlight:highlighted]; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if systemTraySetHighlight is called from Go code
rg -nP 'systemTraySetHighlight' --type=goRepository: wailsapp/wails
Length of output: 40
🏁 Script executed:
# Also check the header file to understand the declaration
fd -e h | xargs rg -l 'systemTraySetHighlight'Repository: wailsapp/wails
Length of output: 96
🏁 Script executed:
# Check all occurrences across the entire codebase
rg -n 'systemTraySetHighlight'Repository: wailsapp/wails
Length of output: 277
🏁 Script executed:
# Search in Objective-C/C files for calls to systemTraySetHighlight
rg -n 'systemTraySetHighlight' --type-list | grep -E '\.(m|c)$'
rg -n 'systemTraySetHighlight' v3/pkg/application/Repository: wailsapp/wails
Length of output: 684
Remove unused function systemTraySetHighlight.
This function is declared in the header and implemented here, but has no callers in the codebase. Either remove it or add a comment explaining its intended future use.
🤖 Prompt for AI Agents
In `@v3/pkg/application/systemtray_darwin.m` around lines 164 - 167, The function
systemTraySetHighlight is unused and should be removed or documented: either
delete its implementation here and remove its declaration from the header (and
any exported symbol registrations) to avoid dead code, or keep it but add a
clear TODO comment above systemTraySetHighlight explaining its intended future
use, why it remains declared in the header, and mark it static/internal if it
should not be exported; locate the implementation referencing NSStatusItem and
statusItem.button highlight and the corresponding declaration in the header to
apply the change.
Deploying wails with
|
| Latest commit: |
dadc2bd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://69e22458.wails.pages.dev |
| Branch Preview URL: | https://v3-bugfix-systray-highlight.wails.pages.dev |
|
Summary
Fixes #4910
[statusItem.button highlight:YES]beforepopUpStatusItemMenu:so the icon visually responds to clicks, matching native macOS behaviororderFrontRegardlessinstead ofmakeKeyAndOrderFront+activateIgnoringOtherApps:YES, preventing the app from stealing focus from other applications when the systray icon is clickedapplySmartDefaultsno longer sets the default click handler —processClickhandles attached window toggling directly via a newtoggleAttachedWindow()methodTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit