Repository navigation
fix(windows): Prevent systray menu crash on rapid clicks (#4151) - #4812
Conversation
Add guard against concurrent TrackPopupMenuEx calls using atomic.Bool to prevent race condition when user clicks systray icon repeatedly. Change TrackPopupMenuEx failure from fatal to debug log with early return so the application gracefully handles failed menu display attempts instead of crashing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. WalkthroughAdded an atomic concurrency guard to prevent simultaneous TrackPopupMenuEx calls on Windows, converted a fatal TrackPopupMenuEx error to a non-fatal debug log with early return, and added monitor-aware menu positioning (bottom-left preference with right-edge adjustment). Changes limited to systray/menu display logic and changelog. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
Deploying wails with
|
| Latest commit: |
efc9ec5
|
| Status: | ⚡️ Build in progress... |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v3/pkg/application/popupmenu_windows.go (1)
221-225: Consider extracting magic numbers as named constants.The hardcoded
200(assumed menu width) and10(padding) work but are opaque. Named constants would improve readability.+const ( + estimatedMenuWidth = 200 // pixels; used for right-edge detection + menuEdgePadding = 10 // pixels; spacing from screen edge +) + // Check if we're close to the right edge of the screen // If so, right-align the menu with some padding -if x > int(monitorInfo.RcWork.Right)-200 { // Assuming 200px as a reasonable menu width +if x > int(monitorInfo.RcWork.Right)-estimatedMenuWidth { menuFlags = uint32(w32.TPM_RIGHTALIGN | w32.TPM_BOTTOMALIGN) // Add a small padding (10px) from the right edge - x = int(monitorInfo.RcWork.Right) - 10 + x = int(monitorInfo.RcWork.Right) - menuEdgePadding }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v3/UNRELEASED_CHANGELOG.md(1 hunks)v3/pkg/application/popupmenu_windows.go(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-01-24T22:41:18.566Z
Learnt from: leaanthony
Repo: wailsapp/wails PR: 4031
File: v3/pkg/application/menu.go:199-202
Timestamp: 2025-01-24T22:41:18.566Z
Learning: In the Wails menu system (v3/pkg/application/menu.go), shared state between menus is intentionally designed and desirable. Methods like `Append()` and `Prepend()` should maintain shared references to menu items rather than creating deep copies.
Applied to files:
v3/pkg/application/popupmenu_windows.go
📚 Learning: 2025-02-04T23:59:43.956Z
Learnt from: fbbdev
Repo: wailsapp/wails PR: 4045
File: v3/internal/generator/render/info.go:28-68
Timestamp: 2025-02-04T23:59:43.956Z
Learning: In the Wails v3 project, internal functions are designed to panic on nil parameters as they represent contract violations, rather than adding defensive nil checks.
Applied to files:
v3/pkg/application/popupmenu_windows.go
📚 Learning: 2024-10-08T22:11:37.054Z
Learnt from: leaanthony
Repo: wailsapp/wails PR: 3763
File: v3/examples/window/main.go:472-475
Timestamp: 2024-10-08T22:11:37.054Z
Learning: In `v3/examples/window/main.go`, `time.Sleep` is used within a goroutine and does not block the UI thread.
Applied to files:
v3/pkg/application/popupmenu_windows.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
v3/pkg/application/popupmenu_windows.go (3)
54-54: Concurrency guard field is well-placed.Using
atomic.Boolas an instance field correctly scopes the guard to eachWin32Menu, allowing different menus to display independently while preventing re-entrant calls on the same menu.
195-200: Solid concurrency guard implementation.The
CompareAndSwap+defer Store(false)pattern correctly prevents concurrentTrackPopupMenuExcalls. The defer ensures the flag is reset on all exit paths including early returns and panics.
227-232: Good error handling change.Converting the fatal crash to a debug log with early return is the right approach for transient failures during rapid clicks. The guard should prevent most failures, but this provides a safe fallback if
TrackPopupMenuExfails for other reasons (e.g., menu already dismissed).v3/UNRELEASED_CHANGELOG.md (1)
28-28: Verify the author attribution.The changelog attributes this fix to
@anthropics-ai, but the PR was opened by@leaanthony. Should this be attributed to the PR author instead?
|
…) (wailsapp#4812) Add guard against concurrent TrackPopupMenuEx calls using atomic.Bool to prevent race condition when user clicks systray icon repeatedly. Change TrackPopupMenuEx failure from fatal to debug log with early return so the application gracefully handles failed menu display attempts instead of crashing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Vibe Kanban <noreply@vibekanban.com> Co-authored-by: Claude <noreply@anthropic.com>
Summary
isShowing atomic.Boolfield to prevent race condition in popup menu displayFixes #4151
Test plan
v3/examples/systray-menuexample on Windows🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.