refactor: extract TrayIconCoordinator from App.xaml.cs - #818
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 25, 2026, 4:24 AM ET / 08:24 UTC. Summary Reproducibility: not applicable. this is a refactor PR rather than a bug report. The relevant behavioral check is source-verifiable: queued tray updates now check App-owned liveness before touching the TrayIcon. Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the narrow refactor after required validation finishes, keeping tray icon lifecycle ownership in App and the coordinator guarded by App's liveness state. Do we have a high-confidence way to reproduce the issue? Not applicable; this is a refactor PR rather than a bug report. The relevant behavioral check is source-verifiable: queued tray updates now check App-owned liveness before touching the TrayIcon. Is this the best way to solve the issue? Yes; the current approach is narrow and preserves the existing App lifecycle boundary while moving tray icon update and tooltip formatting into a small coordinator. The remaining requirement is validation completion, not a different code design. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 4e7982bafb86. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
@AlexAlves87 heads up, you're still targeting master and we moved to main because clawsweeper uses main. |
|
Thanks for keeping this small. One repo/process issue first: this PR is still targeting master, but main is the active branch and master was the mistaken target from the earlier Barbara PRs. Please retarget/rebase this onto main so the diff is only the TrayIconCoordinator extraction. I did a quick local rebase check. After skipping the already-ported master-only commits, the intended diff is still just the three tray-icon files, but there is one shutdown-race fix needed before we should merge it: when the tray icon is disposed during shutdown, also clear _trayIconCoordinator next to _trayIcon = null. Otherwise a queued UpdateTrayIcon() can call through a coordinator that still holds the disposed TrayIcon instance. Suggested fix in the tray icon shutdown step:
After that, rerun the required validation on main: ./build.ps1, Shared tests, and Tray tests. |
Moves tray icon update logic — icon path, tooltip build, tooltip apply, and UI-thread marshalling — into a dedicated TrayIconCoordinator. App.xaml.cs retains a one-line delegation. No behaviour change. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
A queued UpdateTrayIcon() dispatch could reach the coordinator after TrayIcon is disposed. Nulling the coordinator in the same shutdown step that disposes and nulls TrayIcon closes that window. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1b1370a to
2caddc1
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review |
A tray update marshalled off the UI thread can run after shutdown has disposed the tray icon. Clearing the coordinator reference alone does not stop an already-queued delegate, so the coordinator now checks a liveness callback before touching the icon, restoring the no-op behaviour the inlined code had via its null check. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
UpdateTrayIconandApplyTrayTooltipwere living inApp.xaml.cs. This moves them into a small dedicatedTrayIconCoordinator. Pure refactor, no behavior change.What changed
Services/TrayIconCoordinator.cs: ownsUpdateTrayIcon()andApplyTrayTooltip(string).App.xaml.cs:UpdateTrayIcon()now delegates to_trayIconCoordinator;ApplyTrayTooltipremoved; coordinator initialized inInitializeTrayIcon.Testing
./build.ps1dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restoredotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restoreAll passing.
Proof
Hovered over the tray icon after connecting and disconnecting the gateway to exercise the extracted coordinator through
UpdateTrayIcon. Tooltip rendered correctly in both states — connected and disconnected.Grabacion.de.pantalla.2026-06-25.013309.mp4
Notes
Intentionally small. No threading changes, no logic added or removed.
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com