diff --git a/client/ui/CLAUDE.md b/client/ui/CLAUDE.md index 12c00a9e3..26a864eb2 100644 --- a/client/ui/CLAUDE.md +++ b/client/ui/CLAUDE.md @@ -9,6 +9,7 @@ This is the Wails v3 desktop UI for NetBird. Go services live in `services/`; th ### Go (top-level package `main`) - `main.go` — app entry. Builds the shared gRPC `Conn`, constructs services, registers them with Wails, creates the main webview window, then starts (in order) the Linux SNI watcher → tray → `peers.Watch` → `app.Run`. CLI flags: `--daemon-addr`, `--log-file` (repeatable; first user-provided value drops the seeded `console` default), `--log-level` (`trace|debug|info|warn|error`, default `info`). - `tray.go` — `Tray` struct + menu. Subscribes to `EventStatus`, `EventSystem`, `EventUpdateAvailable`, `EventUpdateProgress`. Owns per-status icon/dot, Profiles submenu, Connect/Disconnect swap, About → Update, session-expired toast. +- **Tray menu updates go through `relayoutMenu` (whole-tree rebuild), never in-place submenu mutation.** Any dynamic menu change — Profiles submenu (`tray_profiles.go loadProfiles` → caches rows under `profilesMu`, then `fillProfileSubmenu`), Exit Node submenu (`tray_exitnodes.go refreshExitNodes` → `fillExitNodeSubmenu`), daemon-version row (`tray_status.go`), and the About → Update row (`tray_update.go applyState` → `onMenuChange` callback) — rebuilds the entire menu via `Tray.relayoutMenu` (`buildMenu()` + repaint cached state + single `t.tray.SetMenu`). Serialised by `menuMu`. **Why:** on KDE/Plasma the StatusNotifierItem host caches a submenu's layout the first time it's opened (`GetLayout` for that submenu id) and never re-fetches it on a `LayoutUpdated(parent=0)` signal — so the old `submenu.Clear()`+`Add()` left both the visible rows AND the click→id mapping frozen on the first snapshot. Because `Clear()`+`Add()` allocates fresh monotonic item ids each time (Wails `menuitem.go`), clicks then sent ids the rebuilt `itemMap` no longer knew, and silently no-op'd ("Manage Profiles" stopped responding after the first switch). `buildMenu()` allocates a brand-new submenu container id each relayout, which Plasma treats as unseen and re-queries on next open — fixing both the stale paint and the dead clicks. Confirmed via `dbus-monitor`: a re-opened submenu issued no `GetLayout` until its container id changed. The whole-tree `SetMenu` also subsumes the older darwin detached-NSMenu workaround. `fill*Submenu` helpers are pure UI (read caches, no daemon fetch, no `SetMenu`) so `relayoutMenu` never recurses back into the fetchers. - `tray_linux.go` — `init()` sets `WEBKIT_DISABLE_DMABUF_RENDERER=1` (blank-white window on VMs / minimal WMs) and `WEBKIT_DISABLE_COMPOSITING_MODE=1` (Intel/Mesa SIGSEGV in `g_application_run` via unimplemented DRM-format-modifier paths — DMABUF-disable alone doesn't cover the GL compositor). Both are skipped if the user already set the var. Also `WEBKIT_DISABLE_SANDBOX_THIS_IS_DANGEROUS=1` when unprivileged userns are blocked. - `tray_watcher_linux.go`, `xembed_host_linux.go`, `xembed_tray_linux.{c,h}` — in-process SNI watcher + XEmbed bridge for minimal WMs. See `LINUX-TRAY.md`. - `signal_unix.go` / `signal_windows.go` — `listenForShowSignal`. Unix uses SIGUSR1; Windows uses a named event `Global\NetBirdQuickActionsTriggerEvent`. Mirrors the legacy Fyne UI's external-trigger contract so the installer / CLI keep working. diff --git a/client/ui/tray.go b/client/ui/tray.go index c54376454..2febf5226 100644 --- a/client/ui/tray.go +++ b/client/ui/tray.go @@ -112,7 +112,7 @@ type Tray struct { // connected, the last status string, the daemon version, the // routed-networks revision, and the post-connect login-trigger flag. // These are all written by applyStatus and read by the menu painters - // (applyIcon, reapplyMenuState, refreshExitNodes' connected sample, + // (applyIcon, relayoutMenu, refreshExitNodes' connected sample, // etc.). One mutex covers them because they change together on every // Status push. statusMu sync.Mutex @@ -170,8 +170,24 @@ type Tray struct { // callers. profileLoadMu sync.Mutex + // profilesMu guards the cached profile rows that relayoutMenu repaints + // into a freshly built Profiles submenu. loadProfiles fetches and stores + // them here; fillProfileSubmenu reads them. Kept separate from the live + // submenu so a relayout (which throws the old submenu away) always has a + // source of truth to repaint from without re-hitting the daemon. + profilesMu sync.Mutex + profiles []services.Profile + profilesUser string + + // menuMu serialises relayoutMenu — the full buildMenu + SetMenu cycle. + // loadProfiles (under profileLoadMu) and refreshExitNodes (under + // exitNodesRebuildMu) both drive a relayout from independent mutexes, and + // applyLanguage drives one from the Localizer goroutine; without this guard + // two relayouts could interleave their t.menu swap and SetMenu push. + menuMu sync.Mutex + // exitNodesMu guards the t.exitNodes row cache so reading the cached - // rows in reapplyMenuState (and tearing a copy off the slice for + // rows in relayoutMenu (and tearing a copy off the slice for // Repaint) doesn't contend with status-push readers of statusMu. exitNodesMu sync.Mutex // exitNodes are the rows currently painted into the Exit Node @@ -196,7 +212,7 @@ func NewTray(app *application.App, window *application.WebviewWindow, svc TraySe // the right locale — no English flash followed by a re-paint. loc: svc.Localizer, } - t.updater = newTrayUpdater(app, window, svc.Update, svc.Notifier, t.loc, func() { t.applyIcon() }) + t.updater = newTrayUpdater(app, window, svc.Update, svc.Notifier, t.loc, func() { t.applyIcon() }, func() { t.relayoutMenu() }) t.tray = app.SystemTray.New() // Seed panel-theme detection (Linux only) before the first paint so the // initial icon already matches the panel's light/dark scheme; repaints @@ -309,17 +325,36 @@ func (t *Tray) applyLanguage() { if runtime.GOOS == "linux" { t.tray.SetLabel(t.loc.T("tray.tooltip")) } - t.menu = t.buildMenu() - t.tray.SetMenu(t.menu) - t.reapplyMenuState() + t.relayoutMenu() } -// reapplyMenuState walks cached state and re-applies the visibility, -// enablement and label mutations that applyStatus would have performed -// since the last menu rebuild. Required after buildMenu because that -// constructor returns items in their default (disconnected) shape. The -// update menu item is re-applied by trayUpdater.applyLanguage. -func (t *Tray) reapplyMenuState() { +// relayoutMenu rebuilds the ENTIRE tray menu from scratch (buildMenu), repaints +// the cached status/session/profile/exit-node state into the fresh items, and +// pushes the whole tree with a single SetMenu. It is the only Linux path that +// reliably propagates submenu changes. +// +// Why a full rebuild rather than mutating the existing submenu in place: on +// KDE/Plasma the StatusNotifierItem host caches a submenu's layout the first +// time it is opened (GetLayout for that submenu id) and never re-fetches it on +// a LayoutUpdated(parent=0) signal — so Clear()+Add() into the same submenu +// container left the visible menu (and, worse, the click→id mapping) frozen on +// the first snapshot: clicks sent the stale ids, which the freshly-rebuilt +// itemMap no longer knew, so they silently no-op'd. buildMenu allocates a brand +// new submenu container id every time, which Plasma treats as an unseen menu +// and re-queries on next open — both the labels and the click ids stay live. +// (Confirmed via dbus-monitor: a re-opened submenu issued no GetLayout until +// its container id changed.) The darwin detached-NSMenu workaround that the old +// per-submenu SetMenu addressed is also covered, since this rebuilds the whole +// tree against the cached top-level pointer. +// +// Pulls profile/exit-node rows from their caches (profilesMu / exitNodes) so it +// never re-hits the daemon and never recurses back into loadProfiles. +func (t *Tray) relayoutMenu() { + t.menuMu.Lock() + defer t.menuMu.Unlock() + + t.menu = t.buildMenu() + t.statusMu.Lock() connected := t.connected lastStatus := t.lastStatus @@ -374,15 +409,19 @@ func (t *Tray) reapplyMenuState() { if t.updater != nil { t.updater.applyLanguage() } - // buildMenu just recreated an empty Exit Node submenu, so repaint the - // cached rows unconditionally (a refreshExitNodes would skip the rebuild - // when the list is unchanged). Hold exitNodesRebuildMu so this rebuild - // can't race a status-push-driven refreshExitNodes mutating the same - // submenu. - t.exitNodesRebuildMu.Lock() - t.rebuildExitNodes(exitNodeEntries) - t.exitNodesRebuildMu.Unlock() - go t.loadProfiles() + // buildMenu just recreated empty Profiles + Exit Node submenus, so repaint + // both from their caches before the single SetMenu below. fillExitNodeSubmenu + // uses the entries snapshotted above; fillProfileSubmenu reads profilesMu. + // Neither re-fetches, so relayoutMenu never recurses back into + // loadProfiles/refreshExitNodes. (We must NOT re-take exitNodesRebuildMu + // here — refreshExitNodes already holds it when it calls relayoutMenu.) + t.fillExitNodeSubmenu(exitNodeEntries) + t.fillProfileSubmenu() + + // Single push of the whole tree. On Linux this emits one LayoutUpdated with + // fresh submenu container ids; on darwin it rebuilds the NSMenu against the + // cached top-level pointer. + t.tray.SetMenu(t.menu) } func (t *Tray) buildMenu() *application.Menu { @@ -462,7 +501,7 @@ func (t *Tray) buildMenu() *application.Menu { // exitNodeSubmenu hosts one row per peer advertising a default // route (0.0.0.0/0 or ::/0). Populated asynchronously by - // rebuildExitNodes on every Status push that changes the set; + // refreshExitNodes (via relayoutMenu) on every Status push that changes the set; // the parent row stays disabled until at least one candidate is // known. We grab the parent MenuItem via FindByLabel (same // pattern as the Profiles submenu) so applyStatus can flip its diff --git a/client/ui/tray_exitnodes.go b/client/ui/tray_exitnodes.go index 2b9454118..53a434653 100644 --- a/client/ui/tray_exitnodes.go +++ b/client/ui/tray_exitnodes.go @@ -23,19 +23,17 @@ type exitNodeEntry struct { Selected bool } -// rebuildExitNodes paints one clickable row per exit-node candidate into the -// Exit Node submenu. Each row carries the network's NetID and its selected -// state from ListNetworks; clicking toggles it via toggleExitNode. The active -// node is marked with a "✓ " prefix using a plain Add rather than AddCheckbox -// for the same reason as loadProfiles — Wails auto-toggles a checkbox's state -// on click before the OnClick handler runs, so the deselect/select round-trip -// would briefly show two checked rows. Rebuilds via Clear + Add so the row set -// stays in sync; SetMenu on the root menu is required because Wails v3 alpha -// menu Update() builds a detached NSMenu on darwin that never replaces the -// empty submenu attached at initial setup (same workaround as loadProfiles). -// Callers must hold exitNodesRebuildMu so concurrent rebuilds can't race the -// submenu's item slice. -func (t *Tray) rebuildExitNodes(nodes []exitNodeEntry) { +// fillExitNodeSubmenu paints one clickable row per exit-node candidate into +// the (freshly built) Exit Node submenu. Each row carries the network's NetID +// and its selected state from ListNetworks; clicking toggles it via +// toggleExitNode. The active node is marked with a "✓ " prefix using a plain +// Add rather than AddCheckbox for the same reason as fillProfileSubmenu — +// Wails auto-toggles a checkbox's state on click before the OnClick handler +// runs, so the deselect/select round-trip would briefly show two checked rows. +// Pure UI: it never calls SetMenu — relayoutMenu owns the single SetMenu that +// pushes the whole tree. Callers must hold exitNodesRebuildMu so concurrent +// rebuilds can't race the submenu's item slice. +func (t *Tray) fillExitNodeSubmenu(nodes []exitNodeEntry) { if t.exitNodeSubmenu == nil { return } @@ -51,9 +49,6 @@ func (t *Tray) rebuildExitNodes(nodes []exitNodeEntry) { t.toggleExitNode(id, selected) }) } - if t.menu != nil { - t.tray.SetMenu(t.menu) - } } // refreshExitNodes re-fetches the routed-network list from the daemon and @@ -93,14 +88,11 @@ func (t *Tray) refreshExitNodes() { t.exitNodes = nodes t.exitNodesMu.Unlock() - // Set enablement before rebuildExitNodes' SetMenu so the rebuild reads the - // updated state at NSMenuItem construction time (Wails v3 alpha reads - // item.disabled at build time, not lazily). - if t.exitNodeItem != nil { - t.exitNodeItem.SetEnabled(connected && len(nodes) > 0) - } + // relayoutMenu rebuilds the whole tree (allocating a fresh exitNodeItem) and + // repaints the parent's enablement from the cached entries we just stored, + // so there is no need to poke the old exitNodeItem here. if changed { - t.rebuildExitNodes(nodes) + t.relayoutMenu() } } diff --git a/client/ui/tray_profiles.go b/client/ui/tray_profiles.go index eb7b748f7..a80cd77f5 100644 --- a/client/ui/tray_profiles.go +++ b/client/ui/tray_profiles.go @@ -43,14 +43,16 @@ func (t *Tray) loadConfig() { t.profileMu.Unlock() } -// loadProfiles refreshes the Profiles submenu from the daemon. Each -// entry is a checkbox showing the active profile and switches on click. -// Called on ApplicationStarted, after a successful switchProfile, and -// from applyStatus whenever the daemon's status text changes — the -// last case catches profile flips driven by another channel (CLI -// "netbird profile select", autoconnect picking the persisted profile -// after the UI's first ListProfiles, etc.) since the daemon does not -// emit a dedicated active-profile event. +// loadProfiles fetches the profile list from the daemon, caches it under +// profilesMu, and drives a full tray relayout (relayoutMenu) so the Profiles +// submenu repaints. Called on ApplicationStarted, after a successful +// switchProfile, and from applyStatus whenever the daemon's status text +// changes — the last case catches profile flips driven by another channel +// (CLI "netbird profile select", autoconnect picking the persisted profile +// after the UI's first ListProfiles, etc.) since the daemon does not emit a +// dedicated active-profile event. The relayout (rather than a Clear()+Add() +// into the live submenu) is what makes KDE/Plasma actually repaint and keep +// the click→id mapping live — see relayoutMenu's doc comment. func (t *Tray) loadProfiles() { if t.profileSubmenu == nil { return @@ -69,6 +71,29 @@ func (t *Tray) loadProfiles() { log.Debugf("list profiles: %v", err) return } + + t.profilesMu.Lock() + t.profiles = profiles + t.profilesUser = username + t.profilesMu.Unlock() + + t.relayoutMenu() +} + +// fillProfileSubmenu paints the cached profile rows into the (freshly built) +// Profiles submenu and updates the parent label + email row. Pure UI: it +// never fetches and never calls SetMenu — relayoutMenu owns the single +// SetMenu that pushes the whole tree. Reads the rows captured by loadProfiles +// under profilesMu. +func (t *Tray) fillProfileSubmenu() { + if t.profileSubmenu == nil { + return + } + t.profilesMu.Lock() + profiles := append([]services.Profile(nil), t.profiles...) + username := t.profilesUser + t.profilesMu.Unlock() + sort.Slice(profiles, func(i, j int) bool { return profiles[i].Name < profiles[j].Name }) t.profileSubmenu.Clear() @@ -102,7 +127,7 @@ func (t *Tray) loadProfiles() { t.profileSubmenu.Add(t.loc.T("tray.menu.manageProfiles")).OnClick(func(*application.Context) { t.svc.WindowManager.OpenSettings("profiles") }) - log.Infof("tray loadProfiles: received %d profile(s) for user %q, active=%q", len(profiles), username, activeName) + log.Infof("tray fillProfileSubmenu: %d profile(s) for user %q, active=%q", len(profiles), username, activeName) if t.profileSubmenuItem != nil && activeName != "" { t.profileSubmenuItem.SetLabel(activeName) } @@ -114,17 +139,6 @@ func (t *Tray) loadProfiles() { t.profileEmailItem.SetHidden(true) } } - // Wails v3 alpha's submenu.Update() builds a fresh, detached NSMenu on - // darwin that never replaces the empty NSMenu attached to the parent - // menu item at initial setup — so the visible Profiles menu stays - // frozen on the snapshot taken when the tray was registered. Re-running - // SetMenu on the top-level rebuilds the entire NSMenu tree against the - // cached pointer and is the only path that propagates submenu changes. - if t.menu != nil { - t.tray.SetMenu(t.menu) - } else { - t.profileSubmenu.Update() - } } // switchProfile cancels any in-flight profile switch, then starts a new one. diff --git a/client/ui/tray_status.go b/client/ui/tray_status.go index bc0deb954..4157bae22 100644 --- a/client/ui/tray_status.go +++ b/client/ui/tray_status.go @@ -72,7 +72,11 @@ func (t *Tray) applyStatus(st services.Status) { go t.refreshExitNodes() } if daemonVersionChanged && t.daemonVersionItem != nil { - t.daemonVersionItem.SetLabel(t.loc.T("tray.menu.daemonVersion", "version", st.DaemonVersion)) + // The version row lives in the About submenu, which KDE/Plasma caches on + // first open and never re-fetches on a plain SetLabel (see relayoutMenu's + // doc comment). Drive a full relayout so the new version actually paints. + // relayoutMenu repaints the label from the cached lastDaemonVersion. + t.relayoutMenu() } if sessionExpiredEnter { t.handleSessionExpired() @@ -155,12 +159,11 @@ func (t *Tray) refreshMenuItemsForStatus(st services.Status, connected bool) { // Refresh the Profiles submenu on every status-text transition: the // daemon does not emit an active-profile event, so the startup race // (UI loads profiles before autoconnect picks the persisted profile) - // and a CLI "profile select && up" both surface here. Fired AFTER - // all SetHidden/SetEnabled writes on the static menu items above so - // loadProfiles' SetMenu rebuild (which clearMenu+processMenu the - // entire NSMenu and re-assigns item.impl) cannot race those - // writes — the Wails 3 alpha menu API is not goroutine-safe and - // reads item.disabled/item.hidden at NSMenuItem construction time. + // and a CLI "profile select && up" both surface here. loadProfiles + // fetches the rows and drives a full relayoutMenu (serialised by menuMu), + // so it cannot race the SetHidden/SetEnabled writes on the static items + // above — the Wails 3 alpha menu API is not goroutine-safe and reads + // item.disabled/item.hidden at NSMenuItem construction time. go t.loadProfiles() } diff --git a/client/ui/tray_update.go b/client/ui/tray_update.go index 0028f0565..8fcf90da2 100644 --- a/client/ui/tray_update.go +++ b/client/ui/tray_update.go @@ -32,6 +32,13 @@ type trayUpdater struct { // transitions, so the tray can repaint its icon (the small badge // overlay differs between has-update / no-update). onIconChange func() + // onMenuChange drives a full tray relayout (Tray.relayoutMenu) after an + // event-driven update-state change. The update row lives in the About + // submenu, which KDE/Plasma caches on first open and never re-fetches on a + // plain SetLabel/SetHidden — so a newly-available update would never paint + // there. relayoutMenu rebuilds the whole tree (fresh submenu ids) and + // re-attaches this item from the cached state via attach → refreshMenuItem. + onMenuChange func() mu sync.Mutex item *application.MenuItem @@ -40,7 +47,7 @@ type trayUpdater struct { progressWindowOpen bool // last installing value we acted on } -func newTrayUpdater(app *application.App, window *application.WebviewWindow, update *services.Update, notifier *notifications.NotificationService, loc *Localizer, onIconChange func()) *trayUpdater { +func newTrayUpdater(app *application.App, window *application.WebviewWindow, update *services.Update, notifier *notifications.NotificationService, loc *Localizer, onIconChange func(), onMenuChange func()) *trayUpdater { u := &trayUpdater{ app: app, window: window, @@ -48,6 +55,7 @@ func newTrayUpdater(app *application.App, window *application.WebviewWindow, upd notifier: notifier, loc: loc, onIconChange: onIconChange, + onMenuChange: onMenuChange, } app.Event.On(updater.EventStateChanged, u.onStateEvent) // Seed from the cached state so we don't miss an event that fired @@ -142,7 +150,16 @@ func (u *trayUpdater) applyState(st updater.State) { } u.mu.Unlock() - u.refreshMenuItem(st) + // Drive a full relayout rather than mutating u.item in place: on KDE the + // About submenu is layout-cached, so a direct SetLabel/SetHidden here would + // not paint the newly-available update. relayoutMenu re-attaches the item + // from u.state, which re-runs refreshMenuItem. Fall back to the in-place + // refresh if no relayout hook was wired (defensive — always set today). + if u.onMenuChange != nil { + u.onMenuChange() + } else { + u.refreshMenuItem(st) + } if prev.Available != st.Available && u.onIconChange != nil { u.onIconChange() }