Repository navigation
[v3 alpha] windows tray minor refactor - #4653
Conversation
WalkthroughImplements ownership-aware Windows system tray icon lifecycle, adds NOTIFYICON/NIF/NIN constants, replaces a direct reshow with a show() that surfaces errors, adds UTF‑16-safe tooltip truncation, makes tray-add failures non-fatal, improves cleanup and theme handling, and adds a systray-clock example. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant STray as WindowsSystemTray
participant Win as WinAPI
participant Theme as ThemeService
Note over App,STray: App init -> show()
App->>STray: systray.show()
STray->>Win: ShellNotifyIcon(NIM_ADD, data)
alt ShellNotifyIcon success
Win-->>STray: OK
STray->>Theme: register theme listener
STray->>STray: setTooltip(truncateUTF16(text), NIF_SHOWTIP)
else ShellNotifyIcon error
Win-->>STray: error
STray->>App: log warning (non-fatal)
end
Note over Theme,STray: On theme change
Theme->>STray: ThemeChanged
STray->>STray: resolve icons (light/dark, ownership)
STray->>Win: UpdateIcon (ownership-aware)
alt previous icon owned & unused
STray->>Win: DestroyIcon(oldHICON)
else shared/external
STray-->>Win: keep handle
end
Note over App,STray: Shutdown
App->>STray: Destroy()
STray->>Theme: unregister listener
STray->>Win: ShellNotifyIcon(NIM_DELETE)
STray->>Win: release owned icon handles
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Files/areas needing extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
Also, can move TaskbarCreated in windows system tray implementation |
|
I really like these changes! Thanks for taking the time to do this. Are you wanting to do more before making it ready for review? |
|
Thank you! I will clean up code and make this PR ready for review. |
|
Ready for review |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
v3/pkg/application/application_windows.go (1)
266-271: Bug: debounce uses 1000ns, not 1stime.Sleep(1000) sleeps ~1µs. Use time.Second.
- // 1 second debounce - time.Sleep(1000) + // 1 second debounce + time.Sleep(time.Second)v3/pkg/application/systemtray_windows.go (1)
313-315: Avoid panicking on NIM_MODIFY failureCrashing on ShellNotifyIcon modify defeats the PR’s resiliency goal. Warn and return; TaskbarCreated re-add will recover.
- if !w32.ShellNotifyIcon(w32.NIM_MODIFY, &nid) { - panic(syscall.GetLastError()) - } + if !w32.ShellNotifyIcon(w32.NIM_MODIFY, &nid) { + globalApplication.warning("ShellNotifyIcon NIM_MODIFY failed: %v", syscall.GetLastError()) + return + }
🧹 Nitpick comments (5)
v3/pkg/application/application_windows.go (2)
351-355: Don't hold map lock while calling systray.show()Copy references under RLock, then unlock before invoking Windows APIs to avoid lock contention and potential deadlocks.
-func (m *windowsApp) reshowSystrays() { - m.systrayMapLock.Lock() - defer m.systrayMapLock.Unlock() - for _, systray := range m.systrayMap { - if _, err := systray.show(); err != nil { - globalApplication.warning("failed to re-add system tray icon: %v", err) - } - } -} +func (m *windowsApp) reshowSystrays() { + m.systrayMapLock.RLock() + trays := make([]*windowsSystemTray, 0, len(m.systrayMap)) + for _, t := range m.systrayMap { + trays = append(trays, t) + } + m.systrayMapLock.RUnlock() + for _, systray := range trays { + if _, err := systray.show(); err != nil { + globalApplication.warning("failed to re-add system tray icon: %v", err) + } + } +}
128-132: name() returns empty and contains commented CEither implement a Windows-friendly fallback (e.g., from version resource or os.Executable) or remove the commented block.
v3/pkg/application/systemtray_windows.go (3)
223-229: Initialize icon before first NIM_ADDs.show() runs before resolving icons; HIcon may be 0, causing a brief empty icon/add-then-modify. Resolve base icon(s) and set s.currentIcon first (or set to class icon) to avoid flicker and reduce failures.
Also applies to: 230-242, 276-286
344-347: Don’t panic on invalid icon data in setIconSurface error via log and bail; don’t crash the app.
- newIcon, err := w32.CreateSmallHIconFromImage(icon) - if err != nil { - panic(err.Error()) - } + newIcon, err := w32.CreateSmallHIconFromImage(icon) + if err != nil { + globalApplication.warning("setIcon: invalid icon: %v", err) + return + }
373-376: Mirror non-panicking behavior in setDarkModeIconSame rationale as setIcon.
- newIcon, err := w32.CreateSmallHIconFromImage(icon) - if err != nil { - panic(err.Error()) - } + newIcon, err := w32.CreateSmallHIconFromImage(icon) + if err != nil { + globalApplication.warning("setDarkModeIcon: invalid icon: %v", err) + return + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
v3/pkg/application/application_windows.go(3 hunks)v3/pkg/application/systemtray_windows.go(12 hunks)v3/pkg/w32/constants.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
v3/pkg/application/systemtray_windows.go (8)
v3/pkg/w32/typedef.go (3)
HWND(218-218)HICON(205-205)NOTIFYICONDATA(670-686)v3/pkg/w32/user32.go (2)
DestroyIcon(1259-1264)DestroyWindow(373-376)v3/pkg/w32/icon.go (1)
CreateSmallHIconFromImage(53-72)v3/pkg/icons/icons.go (2)
SystrayLight(9-9)SystrayDark(15-15)v3/pkg/w32/constants.go (7)
NIF_TIP(2618-2618)NIF_SHOWTIP(2623-2623)NIM_DELETE(2613-2613)NIF_ICON(2617-2617)NIF_MESSAGE(2616-2616)NIM_ADD(2611-2611)NIM_SETVERSION(2614-2614)v3/pkg/w32/window.go (1)
StringToUTF16(194-197)v3/pkg/w32/shell32.go (2)
ShellNotifyIcon(231-234)NOTIFYICON_VERSION(83-83)v3/pkg/w32/kernel32.go (1)
GetLastError(209-212)
⏰ 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). (6)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
🔇 Additional comments (5)
v3/pkg/w32/constants.go (1)
2616-2623: NIF_ additions look correct*Values match Win32 (0x20, 0x40, 0x80). No issues.
v3/pkg/application/systemtray_windows.go (4)
40-52: Icon ownership guard is solidreleaseIcon prevents destroying shared handles and respects keep-list. Good safety improvement.
467-472: Tooltip truncation fix is correctUTF‑16 unit-aware truncation to 127 plus NIF_SHOWTIP is right. LGTM.
Also applies to: 568-587
513-527: Cleanup path looks safeIcons are released only if owned and not referenced; fields zeroed; window destroyed. LGTM.
540-566: show(): robust add/version sequenceGood: return errors, set NOTIFYICON_VERSION, then apply tooltip. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
v3/pkg/application/systemtray_windows.go (2)
219-221: Inconsistent error handling: panic on window creation failure.While the PR improves error handling for
ShellNotifyIconfailures (line 225-228), a panic remains here ifCreateWindowExfails. This is inconsistent with the robustness improvements elsewhere.Consider replacing the panic with graceful error handling:
if s.hwnd == 0 { - panic(syscall.GetLastError()) + globalApplication.error("failed to create systray window: %v", syscall.GetLastError()) + return }
314-316: Panic on ShellNotifyIcon failure remains.The PR improves error handling in
show()and initial tray registration, but a panic remains here ifNIM_MODIFYfails. This is inconsistent with the robustness improvements.Consider replacing the panic with error logging:
if !w32.ShellNotifyIcon(w32.NIM_MODIFY, &nid) { - panic(syscall.GetLastError()) + globalApplication.warning("failed to update systray icon: %v", syscall.GetLastError()) + return }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/pkg/application/systemtray_windows.go(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
v3/pkg/application/systemtray_windows.go (7)
v3/pkg/w32/typedef.go (3)
HWND(218-218)HICON(205-205)NOTIFYICONDATA(670-686)v3/pkg/w32/user32.go (2)
DestroyIcon(1259-1264)DestroyWindow(373-376)v3/pkg/w32/icon.go (1)
CreateSmallHIconFromImage(53-72)v3/pkg/w32/constants.go (10)
NIF_TIP(2618-2618)NIF_SHOWTIP(2623-2623)NIM_DELETE(2613-2613)NIF_STATE(2619-2619)NIS_HIDDEN(2625-2625)NIM_MODIFY(2612-2612)NIF_ICON(2617-2617)NIF_MESSAGE(2616-2616)NIM_ADD(2611-2611)NIM_SETVERSION(2614-2614)v3/pkg/w32/window.go (1)
StringToUTF16(194-197)v3/pkg/w32/shell32.go (2)
ShellNotifyIcon(231-234)NOTIFYICON_VERSION(83-83)v3/pkg/w32/kernel32.go (1)
GetLastError(209-212)
⏰ 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). (4)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (10)
v3/pkg/application/systemtray_windows.go (10)
28-38: LGTM! Ownership tracking fields are well-structured.The addition of ownership flags (
lightModeIconOwned,darkModeIconOwned,currentIconOwned) alongside icon handles enables proper lifecycle management and prevents destroying shared handles.
40-52: LGTM! Safe icon destruction logic.The
releaseIconhelper correctly implements ownership-aware destruction by checking the ownership flag and ensuring no other references exist before callingDestroyIcon. The documentation accurately references the Windows API requirement.
318-332: Ownership tracking logic is correct.The implementation correctly determines whether
currentIconis owned by checking if it referenceslightModeIconordarkModeIconwith their respective ownership flags. The old icon is safely destroyed only when owned and not referenced elsewhere.
359-368: Icon synchronization logic is correct.The implementation correctly synchronizes
darkModeIconwhen it was unset or shared the previous light icon, and properly tracks that onlylightModeIconowns the new shared handle. ThereleaseIconcalls correctly pass the keep list to avoid destroying handles still in use.
387-400: Light mode synchronization is symmetric and correct.The logic mirrors
setIconbut for dark mode, correctly synchronizinglightModeIconwhen it was unset or shared the previous dark icon, and properly releasing old handles.
465-483: Improved tooltip handling with NIF_SHOWTIP and proper UTF-16 truncation.The addition of
NIF_SHOWTIPand the use oftruncateUTF16withmaxUnits=127(leaving room for the null terminator in the 128-unit array) correctly addresses the CJK text truncation issue mentioned in the PR objectives.
585-604: Correct UTF-16 code unit counting for tooltip truncation.The implementation properly counts UTF-16 code units by checking if each rune exceeds U+FFFF (requiring a surrogate pair). Truncation at byte position
iis correct sincerangeover strings yields byte offsets.
496-531: Comprehensive ownership-aware cleanup.The destruction sequence correctly cancels the theme listener, unregisters the tray, destroys the menu, deletes the notification icon, releases owned icon handles with proper reference tracking, resets all state, and finally destroys the window.
533-559: Correct show/hide implementation using state flags.Both methods properly use
NIM_MODIFYwithNIF_STATEandDwStateMaskto control visibility. The guard against uninitializedhwndand debug logging on failure are appropriate.
561-583: Key robustness improvement: error handling instead of panics.This internal
show()helper correctly implements the PR's main objective by returning errors instead of panicking whenShellNotifyIconfails during initial registration. This allows graceful handling of cases where the Windows shell is not yet available.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v3/pkg/application/systemtray_windows.go (1)
314-331: Do not panic on NIM_MODIFY failure; log and revert state.Panicking contradicts the PR’s non-fatal intent. Revert state if modify fails so ownership and handles stay consistent.
- s.currentIcon = newIcon - nid := s.newNotifyIconData() + s.currentIcon = newIcon + nid := s.newNotifyIconData() nid.UFlags = w32.NIF_ICON if s.currentIcon != 0 { nid.HIcon = s.currentIcon } - if !w32.ShellNotifyIcon(w32.NIM_MODIFY, &nid) { - panic(syscall.GetLastError()) - } + if !w32.ShellNotifyIcon(w32.NIM_MODIFY, &nid) { + // revert state on failure + s.currentIcon = oldIcon + s.currentIconOwned = oldIconOwned + globalApplication.warning("ShellNotifyIcon NIM_MODIFY failed: %v", syscall.GetLastError()) + return + }
♻️ Duplicate comments (3)
v3/pkg/application/systemtray_windows.go (3)
231-257: Replace lo.Must panics with explicit error handling; remove lo dependency.These calls will panic on icon decode failure, undermining the PR’s robustness goal. Handle errors, log, and fall back instead. Also enables dropping the lo import.
- switch { - case s.parent.icon != nil: - s.lightModeIcon = lo.Must(w32.CreateSmallHIconFromImage(s.parent.icon)) - s.lightModeIconOwned = true - case defaultIcon != 0: - s.lightModeIcon = defaultIcon - s.lightModeIconOwned = false - default: - s.lightModeIcon = lo.Must(w32.CreateSmallHIconFromImage(icons.SystrayLight)) - s.lightModeIconOwned = true - } + switch { + case s.parent.icon != nil: + if icon, err := w32.CreateSmallHIconFromImage(s.parent.icon); err != nil { + globalApplication.warning("failed to create light icon: %v", err) + } else { + s.lightModeIcon = icon + s.lightModeIconOwned = true + } + case defaultIcon != 0: + s.lightModeIcon = defaultIcon + s.lightModeIconOwned = false + default: + if icon, err := w32.CreateSmallHIconFromImage(icons.SystrayLight); err != nil { + globalApplication.warning("failed to create built-in light icon: %v", err) + } else { + s.lightModeIcon = icon + s.lightModeIconOwned = true + } + } @@ - switch { - case s.parent.darkModeIcon != nil: - s.darkModeIcon = lo.Must(w32.CreateSmallHIconFromImage(s.parent.darkModeIcon)) - s.darkModeIconOwned = true - case s.parent.icon != nil: - s.darkModeIcon = s.lightModeIcon - s.darkModeIconOwned = false - case defaultIcon != 0: - s.darkModeIcon = defaultIcon - s.darkModeIconOwned = false - default: - s.darkModeIcon = lo.Must(w32.CreateSmallHIconFromImage(icons.SystrayDark)) - s.darkModeIconOwned = true - } + switch { + case s.parent.darkModeIcon != nil: + if icon, err := w32.CreateSmallHIconFromImage(s.parent.darkModeIcon); err != nil { + globalApplication.warning("failed to create dark icon: %v", err) + } else { + s.darkModeIcon = icon + s.darkModeIconOwned = true + } + case s.parent.icon != nil: + s.darkModeIcon = s.lightModeIcon + s.darkModeIconOwned = false + case defaultIcon != 0: + s.darkModeIcon = defaultIcon + s.darkModeIconOwned = false + default: + if icon, err := w32.CreateSmallHIconFromImage(icons.SystrayDark); err != nil { + globalApplication.warning("failed to create built-in dark icon: %v", err) + } else { + s.darkModeIcon = icon + s.darkModeIconOwned = true + } + }And remove the unused dependency once Must calls are gone:
-import ( - "errors" - "fmt" - "syscall" - "unsafe" - - "github.com/wailsapp/wails/v3/pkg/icons" - - "github.com/samber/lo" +import ( + "errors" + "fmt" + "syscall" + "unsafe" + + "github.com/wailsapp/wails/v3/pkg/icons"
342-366: Replace panic on icon decode with warning + early return in setIcon.- newIcon, err := w32.CreateSmallHIconFromImage(icon) - if err != nil { - panic(err.Error()) - } + newIcon, err := w32.CreateSmallHIconFromImage(icon) + if err != nil { + globalApplication.warning("failed to create icon: %v", err) + return + }
372-401: Replace panic on icon decode with warning + early return in setDarkModeIcon.- newIcon, err := w32.CreateSmallHIconFromImage(icon) - if err != nil { - panic(err.Error()) - } + newIcon, err := w32.CreateSmallHIconFromImage(icon) + if err != nil { + globalApplication.warning("failed to create dark mode icon: %v", err) + return + }
🧹 Nitpick comments (3)
v3/pkg/application/systemtray_windows.go (3)
558-575: Preselect current icon before first NIM_ADD to avoid blank initial add.s.currentIcon is 0 on first add; set it based on theme first so the initial NIM_ADD shows the icon without relying on a subsequent modify.
func (s *windowsSystemTray) show() (w32.NOTIFYICONDATA, error) { nid := s.newNotifyIconData() nid.UFlags = w32.NIF_ICON | w32.NIF_MESSAGE - nid.HIcon = s.currentIcon + // If currentIcon is unset (first show), pick one based on current theme + if s.currentIcon == 0 { + if w32.IsCurrentlyDarkMode() { + s.currentIcon = s.darkModeIcon + s.currentIconOwned = s.darkModeIconOwned + } else { + s.currentIcon = s.lightModeIcon + s.currentIconOwned = s.lightModeIconOwned + } + } + nid.HIcon = s.currentIcon
510-525: Destroy each unique owned handle once; avoid order/keep complexity.Deduplicate handles and destroy once per unique HICON where any reference is owned. Simpler and safer.
- lightIcon := s.lightModeIcon - darkIcon := s.darkModeIcon - currentIcon := s.currentIcon - - s.releaseIcon(lightIcon, s.lightModeIconOwned) - s.releaseIcon(darkIcon, s.darkModeIconOwned, lightIcon) - s.releaseIcon(currentIcon, s.currentIconOwned, lightIcon, darkIcon) + lightIcon := s.lightModeIcon + darkIcon := s.darkModeIcon + currentIcon := s.currentIcon + + unique := map[w32.HICON]bool{} + if lightIcon != 0 { + unique[lightIcon] = unique[lightIcon] || s.lightModeIconOwned + } + if darkIcon != 0 { + unique[darkIcon] = unique[darkIcon] || s.darkModeIconOwned + } + if currentIcon != 0 { + unique[currentIcon] = unique[currentIcon] || s.currentIconOwned + } + for h, owned := range unique { + s.releaseIcon(h, owned) + }
40-52: Log failed DestroyIcon attempts for diagnosis.Optional: log when DestroyIcon returns false to aid leak/handle debugging.
- w32.DestroyIcon(handle) + if !w32.DestroyIcon(handle) { + globalApplication.debug("DestroyIcon failed (handle=%v)", handle) + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/pkg/application/systemtray_windows.go(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
v3/pkg/application/systemtray_windows.go (6)
v3/pkg/w32/constants.go (14)
WM_USER(578-578)NIN_POPUPOPEN(2632-2632)NIN_POPUPCLOSE(2633-2633)NIF_TIP(2623-2623)NIF_SHOWTIP(2628-2628)NIM_DELETE(2618-2618)NIF_STATE(2624-2624)NIS_HIDDEN(2630-2630)NIM_MODIFY(2617-2617)NIF_ICON(2622-2622)NIF_MESSAGE(2621-2621)NIM_ADD(2616-2616)NOTIFYICON_VERSION_4(2612-2612)NIM_SETVERSION(2619-2619)v3/pkg/w32/typedef.go (3)
HWND(218-218)HICON(205-205)NOTIFYICONDATA(670-686)v3/pkg/w32/user32.go (2)
DestroyIcon(1259-1264)DestroyWindow(373-376)v3/pkg/w32/icon.go (1)
CreateSmallHIconFromImage(53-72)v3/pkg/w32/window.go (1)
StringToUTF16(194-197)v3/pkg/w32/shell32.go (1)
ShellNotifyIcon(230-233)
⏰ 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). (4)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (4)
v3/pkg/application/systemtray_windows.go (4)
109-112: Good: early return when taskbar bounds unavailable.
465-470: Nice: UTF‑16 aware tooltip truncation + NIF_SHOWTIP.This fixes CJK truncation and ensures tooltips display as expected.
430-437: NIN_POPUPOPEN/CLOSE wiring looks correct.
259-262: Verification confirmed: TaskbarCreated re-add path is properly implemented.The downstream handling exists and is correctly wired:
- TaskbarCreated registration:
v3/pkg/application/application_windows.go:26registers the message- Case handler:
v3/pkg/application/application_windows.go:260–270handleswmTaskbarCreated- Re-add logic:
reshowSystrays()(lines 347–355) iterates systray objects and callsshow()on each (line 351)The mechanism is thread-safe with a restartingTaskbar flag and 1-second debounce. The comment in systemtray_windows.go:260 accurately reflects this implementation.
|
Thanks 🙏 Please could you add an entry to the changelog located at |
|
This PR would be a good opportunity to remove the panics as per the rabbit's recommendation. What do you think? |
|
Working on; |
|
Removed almost all panics (without one creating window). but the current tray action interfaces do not return errors, which leads to silent failures (without logging). it seems good idea to revisit tray interface before v3 exits alpha. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v3/pkg/application/systemtray_windows.go (1)
345-362: Remove panic in updateIcon; log and rollback state instead.Panicking here defeats the robustness goal. Roll back to the previous icon on failure and return.
if !w32.ShellNotifyIcon(w32.NIM_MODIFY, &nid) { - panic(syscall.GetLastError()) + err := syscall.GetLastError() + globalApplication.warning("ShellNotifyIcon NIM_MODIFY (icon) failed: %v", err) + // Roll back state so we don't leak/lose the previous handle. + s.currentIcon = oldIcon + s.currentIconOwned = oldIconOwned + return }
🧹 Nitpick comments (1)
v3/pkg/application/systemtray_windows.go (1)
38-50: Capture DestroyIcon failures for diagnostics.Currently silent on failure; add a debug log to aid handle‑lifecycle troubleshooting.
- w32.DestroyIcon(handle) + if !w32.DestroyIcon(handle) { + globalApplication.debug("DestroyIcon failed for handle=%v: %v", handle, syscall.GetLastError()) + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v3/examples/systray-clock/main.go(1 hunks)v3/pkg/application/systemtray_windows.go(14 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-09-30T06:13:46.595Z
Learnt from: leaanthony
PR: wailsapp/wails#3763
File: v3/examples/window/main.go:472-475
Timestamp: 2024-09-30T06:13:46.595Z
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/examples/systray-clock/main.go
🧬 Code graph analysis (2)
v3/examples/systray-clock/main.go (5)
v3/pkg/application/application.go (1)
AlphaAssets(33-35)v3/pkg/application/application_options.go (4)
MacOptions(176-182)ActivationPolicy(164-164)ActivationPolicyAccessory(171-171)WindowsOptions(187-208)v3/pkg/application/systemtray.go (1)
SystemTray(44-66)v3/pkg/icons/icons.go (1)
SystrayMacTemplate(6-6)v3/pkg/application/context.go (1)
Context(3-6)
v3/pkg/application/systemtray_windows.go (8)
v3/pkg/w32/constants.go (14)
WM_USER(578-578)NIN_POPUPOPEN(2632-2632)NIN_POPUPCLOSE(2633-2633)NIF_TIP(2623-2623)NIF_SHOWTIP(2628-2628)NIM_DELETE(2618-2618)NIF_STATE(2624-2624)NIS_HIDDEN(2630-2630)NIM_MODIFY(2617-2617)NIF_ICON(2622-2622)NIF_MESSAGE(2621-2621)NIM_ADD(2616-2616)NOTIFYICON_VERSION_4(2612-2612)NIM_SETVERSION(2619-2619)v3/pkg/application/systemtray.go (1)
SystemTray(44-66)v3/pkg/w32/typedef.go (3)
HWND(218-218)HICON(205-205)NOTIFYICONDATA(670-686)v3/pkg/w32/user32.go (2)
DestroyIcon(1259-1264)DestroyWindow(373-376)v3/pkg/w32/icon.go (1)
CreateSmallHIconFromImage(53-72)v3/pkg/icons/icons.go (2)
SystrayLight(9-9)SystrayDark(15-15)v3/pkg/w32/window.go (1)
StringToUTF16(194-197)v3/pkg/w32/shell32.go (1)
ShellNotifyIcon(230-233)
⏰ 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: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
🔇 Additional comments (5)
v3/pkg/application/systemtray_windows.go (3)
496-513: UTF‑16‑safe tooltip truncation looks good.Correct use of NIF_TIP|NIF_SHOWTIP and surrogate‑aware truncation.
563-589: Show/Hide state updates look good.Appropriate use of NIF_STATE/NIS_HIDDEN with guarded hwnd.
591-615: No action required—TaskbarCreated re-add path confirmed.The verification shows the design is correct:
- TaskbarCreated message is registered and handled in application_windows.go (line 260)
- The code comment at systemtray_windows.go:291 confirms the intent: "Initial systray add can fail when the shell is not available. This is handled downstream via TaskbarCreated message."
- The
show()function (lines 591-615) returns errors gracefully without panics, as intendedv3/examples/systray-clock/main.go (2)
32-55: Goroutine ticker for tooltip updates is appropriate.Non-blocking and cancels on app context; clean.
27-31: macOS template icon usage is correct.Conditional SetTemplateIcon on darwin is the right approach.
github-actions
Bot
added
the
Documentation
|
added changelog |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v3/UNRELEASED_CHANGELOG.md (1)
20-20: Changelog entries are well-written and comprehensively capture the PR changes.All six new entries follow the "Keep a Changelog" format guidelines: they use present tense, include clear descriptions, reference the PR number, and explain user-visible impact. The entries align well with the AI-generated summary and PR objectives covering icon ownership, tooltip handling, Show/Hide lifecycle, and cleanup improvements.
One optional refinement: Line 25 packs three technical implementation details (icon reuse,
NOTIFYICON_VERSION_4,NIF_SHOWTIP) into a single entry. Consider whether simplifying the user-facing benefit would improve readability for non-Windows developers. For example: "Improved tray registration to be more efficient and resilient, so tooltips recover after Explorer restarts." Current phrasing is accurate and acceptable; this is a stylistic suggestion only.Also applies to: 24-25, 29-31
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/UNRELEASED_CHANGELOG.md(1 hunks)
⏰ 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). (4)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
- GitHub Check: semgrep-cloud-platform/scan
|
Amazing. Thank you! I'll create a separate PR to remove all panics and update any APIs as a result. |
|
|
Thanks @DeltaLaboratory 🙏 |
* remove systray add retry loop * correct setTooltip truncation * track Windows tray icon ownership to avoid destroying shared handles * fix incorrect warning call * stop leaking Windows tray theme listener after destroy * fix default app icon loading * fix incorrect truncation * harden system tray flow * implement windows tray show/hide * improved readability * updateIcon call path * improve error handling * systray clock example * added changelog --------- Co-authored-by: Lea Anthony <lea.anthony@gmail.com>
Description
Fixes #3693 (which is already fixed but for reference)
Type of change
Please select the option that is relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration using
wails doctor.If you checked Linux, please specify the distro and version.
Test Configuration
Please paste the output of
wails doctor. If you are unable to run this command, please describe your environment in as much detail as possible.Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Stability