fix(v3/macos): apply setMenuItemChecked state synchronously#5165
fix(v3/macos): apply setMenuItemChecked state synchronously#5165RALIST wants to merge 3 commits intowailsapp:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
WalkthroughUpdates macOS menu item mutators to apply changes synchronously on the Cocoa main thread (short‑circuiting when already on main thread and using Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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.
🧹 Nitpick comments (1)
v3/pkg/application/menuitem_darwin.go (1)
47-93: Optional: sibling setters retain the samedispatch_asyncpattern.
setMenuItemLabel,setMenuItemDisabled,setMenuItemHidden, andsetMenuItemTooltipstill enqueue to the main queue viadispatch_async, so in principle they can exhibit the same "update applied after menu render on rapid reopen" behavior as the bug fixed here. The PR description explicitly scopes this change tosetMenuItemCheckedbecause only the checked-state race was user-reported, which is fine. Consider a follow-up to apply the sameisMainThread+dispatch_syncpattern to these setters (or factor out a small helper) if similar staleness reports emerge for label/disabled/hidden/tooltip updates on context menus.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/pkg/application/menuitem_darwin.go` around lines 47 - 93, The four setters (setMenuItemLabel, setMenuItemDisabled, setMenuItemHidden, setMenuItemTooltip) currently use dispatch_async to update UI and can suffer the same stale-update race as setMenuItemChecked; change them to perform the update immediately if already on the main thread, otherwise use dispatch_sync to dispatch to the main queue so updates apply before the menu is rendered. Implement this by adding the isMainThread check (or a small helper like performOnMainThreadSync) and applying the existing body (casting to MenuItem, setting title/enabled/hidden/toolTip and freeing label/tooltip) directly when on main, or via dispatch_sync when not.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@v3/pkg/application/menuitem_darwin.go`:
- Around line 47-93: The four setters (setMenuItemLabel, setMenuItemDisabled,
setMenuItemHidden, setMenuItemTooltip) currently use dispatch_async to update UI
and can suffer the same stale-update race as setMenuItemChecked; change them to
perform the update immediately if already on the main thread, otherwise use
dispatch_sync to dispatch to the main queue so updates apply before the menu is
rendered. Implement this by adding the isMainThread check (or a small helper
like performOnMainThreadSync) and applying the existing body (casting to
MenuItem, setting title/enabled/hidden/toolTip and freeing label/tooltip)
directly when on main, or via dispatch_sync when not.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a504dd10-ff2c-4cc3-aaa9-3957f9b76bf5
📒 Files selected for processing (2)
v3/UNRELEASED_CHANGELOG.mdv3/pkg/application/menuitem_darwin.go
leaanthony
left a comment
leaanthony
left a comment
There was a problem hiding this comment.
Looks good! I'm ok with extending this to the other menu item types too!
setMenuItemChecked() wrapped the NSMenuItem.state assignment in dispatch_async(dispatch_get_main_queue(), ...). When invoked from the main thread (e.g. handleClick reacting to a user toggle), the block runs on the next runloop turn, which can land *after* the menu has already been rendered if the user reopens the menu quickly. The result is a stale checkmark on the next open. Apply the state directly when called on the main thread, otherwise fall back to dispatch_sync so the assignment lands before we return. This matches the maintainer's suggested fix in the issue and avoids the deadlock of dispatch_sync-from-main. Fixes wailsapp#5002
…tem setters Extend the isMainThread/dispatch_sync pattern from setMenuItemChecked to the other NSMenuItem mutators that shared the same dispatch_async race: - setMenuItemLabel - setMenuItemDisabled - setMenuItemHidden - setMenuItemTooltip Each had the same failure mode as setMenuItemChecked: a rapid menu reopen could render the previous title/enabled/hidden/tooltip state because the dispatch_async block had not yet run. Apply the change directly on the main thread and fall back to dispatch_sync off-thread so the mutation lands before the caller returns. Per maintainer approval on wailsapp#5165.
RALIST
force-pushed
the
fix/v3-menuitem-checked-race
branch
from
e1e73ca to
784eeac
Compare
|
✅ Triaged by Wails PR Reviewer This PR has been reviewed and accepted. A test sub-issue has been created for macOS. Head Ref OID: This comment serves as a signature that this PR has been triaged. Future runs will skip this PR based on the headRefOid. |
|
Tested on macOS — verdict: ✓ pass
Notes: The PR extends the |
2 participants
Description
setMenuItemChecked()(inv3/pkg/application/menuitem_darwin.go) wraps theNSMenuItem.stateassignment insidedispatch_async(dispatch_get_main_queue(), ...).When the function is invoked from the main thread — which is the common path,
since
MenuItem.handleClickruns on the menu's action thread — the assignmentis enqueued for the next runloop turn. If the user reopens the menu before
that turn fires, the menu renders with the previous checked state.
This makes context menus with checkbox-style items (e.g. Bold/Italic toggles)
visibly out of sync until the next idle tick. Reproduction steps and full
analysis are in the issue.
This PR applies the maintainer's suggested fix from the issue thread:
NSMenuItem.statedirectly(avoids
dispatch_sync-from-main deadlock).dispatch_syncfrom background threads, so the state isapplied before we return.
The same
dispatch_asyncpattern still exists in the sibling setters(
setMenuItemLabel,setMenuItemDisabled,setMenuItemHidden,setMenuItemTooltip). They have the same theoretical race, but the issue andthe maintainer comment scope to
setMenuItemChecked, so this PR is keptsurgical. Happy to extend the fix in a follow-up if desired.
Fixes #5002
Type of change
How Has This Been Tested?
Built
./pkg/application/...on macOS.Ran the menuitem unit tests:
go test -run "TestMenuItem|MenuItem" ./pkg/application/— all pass.Manually verified in a downstream app (Writer Studio) that uses checkbox-style
context menu items: after toggling, reopening the menu now shows the new
state immediately, without the previous Go-side workaround.
Windows
macOS
Linux
Test Configuration
macOS 26.3.1 (darwin/arm64), Go toolchain matching repo
go.mod.Checklist:
v3/UNRELEASED_CHANGELOG.mdwith details of this PRSummary by CodeRabbit