diff --git a/client/ui/CLAUDE.md b/client/ui/CLAUDE.md index db2e2bdb2..27a1ce0e3 100644 --- a/client/ui/CLAUDE.md +++ b/client/ui/CLAUDE.md @@ -9,7 +9,8 @@ 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; default is **empty** so "no flag" is distinguishable from explicit `--log-file console` — when empty `parseFlagsAndInitLog` falls back to `console` for `InitLog` and returns `userSetLogFile=false`), `--log-level` (`trace|debug|info|warn|error`, default `info`). See "GUI debug logging" below for what `userSetLogFile` gates. - `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 menu updates go through `relayoutMenu` (whole-tree rebuild), never in-place submenu mutation.** Any dynamic menu change — status-text transitions (`tray_status.go applyStatus`, which also folds in daemon-version and session-deadline changes into one aggregated relayout per push), Profiles submenu (`tray_profiles.go loadProfiles` → caches rows under `profilesMu`, then `fillProfileSubmenu`), Exit Node submenu (`tray_exitnodes.go refreshExitNodes` → `fillExitNodeSubmenu`), 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 concurrency model (menuMu domain).** Wails dispatches `Event.On` listeners and menu `OnClick` callbacks on **fresh goroutines** (so `applyStatus` runs are concurrent with each other and with relayouts), and Wails `MenuItem` setters are not goroutine-safe. Hence `t.menu` and every `*Item`/`*Submenu` field on `Tray` are `menuMu`-owned: `buildMenu` reassigns them all on each relayout, and repaints happen from the caches inside `relayoutMenu` (caches are committed *before* the menu is touched, which makes a write on an orphaned pre-relayout item self-healing). Two sanctioned out-of-lock accesses: the Connect/Disconnect `OnClick` closures capture their own item, and `refreshSessionExpiresLabel` (30s ticker) snapshots `sessionExpiresItem` under `menuMu`. `relayoutMenu` is the **only post-startup `tray.SetMenu` call site** — never push a menu pointer snapshotted outside `menuMu` (it can reinstall a stale tree); `applyStatusIndicator` is `SetBitmap`-only, the relayout's trailing `SetMenu` is what repaints the macOS dot. Tray code never runs on the OS main thread, so taking `menuMu` from any goroutine can't deadlock the setters' `dispatch_sync`/`InvokeSync`. - `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 5b34329b7..426cd5a4b 100644 --- a/client/ui/tray.go +++ b/client/ui/tray.go @@ -89,13 +89,15 @@ type Tray struct { // language switch. loc *Localizer + // menu and the *Item/*Submenu fields below are reassigned by buildMenu + // on every relayout — touch them only with menuMu held. Exceptions: + // the Connect/Disconnect OnClick closures capture their own item, and + // refreshSessionExpiresLabel snapshots its item under menuMu. menu *application.Menu statusItem *application.MenuItem // sessionExpiresItem displays the SSO session deadline as a humanised - // remaining-time label ("Session: 47m"). Hidden when no deadline is - // tracked (non-SSO peer or login-expiration disabled on the account). - // Refreshed by applyStatus on every Status push and by a 1-minute - // ticker between pushes so the countdown moves naturally. + // remaining-time label ("Session: 47m"). Painted by relayoutMenu from + // the sessionMu cache; a 30s ticker keeps the countdown moving. sessionExpiresItem *application.MenuItem upItem *application.MenuItem downItem *application.MenuItem @@ -180,11 +182,10 @@ type Tray struct { 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 serialises relayoutMenu (buildMenu + SetMenu) and guards the + // menu/item-pointer fields above. relayoutMenu is the only post-startup + // SetMenu call site — a menu snapshot pushed outside the lock could + // reinstall a stale tree. menuMu sync.Mutex // exitNodesMu guards the t.exitNodes row cache so reading the cached @@ -411,10 +412,13 @@ func (t *Tray) relayoutMenu() { } } if t.upItem != nil { + // Connect stays visible in the NeedsLogin states too — Up drives + // the SSO re-auth flow; hidden only when it would be a no-op. t.upItem.SetHidden(connected || connecting || daemonUnavailable) t.upItem.SetEnabled(!connected && !connecting && !daemonUnavailable) } if t.downItem != nil { + // Disconnect doubles as the abort path while still Connecting. t.downItem.SetHidden(!connected && !connecting) t.downItem.SetEnabled(connected || connecting) } @@ -468,11 +472,16 @@ func (t *Tray) buildMenu() *application.Menu { menu.AddSeparator() // Only the action that applies to the current state is visible: Connect - // when disconnected, Disconnect when connected. applyStatus swaps them on - // each daemon status change. - t.upItem = menu.Add(t.loc.T("tray.menu.connect")).OnClick(func(*application.Context) { t.handleConnect() }) - t.downItem = menu.Add(t.loc.T("tray.menu.disconnect")).OnClick(func(*application.Context) { t.handleDisconnect() }) - t.downItem.SetHidden(true) + // when disconnected, Disconnect when connected. The OnClick closures + // capture the local item — t.upItem/t.downItem are menuMu-guarded and + // must not be read from the click goroutine. + upItem := menu.Add(t.loc.T("tray.menu.connect")) + upItem.OnClick(func(*application.Context) { t.handleConnect(upItem) }) + t.upItem = upItem + downItem := menu.Add(t.loc.T("tray.menu.disconnect")) + downItem.OnClick(func(*application.Context) { t.handleDisconnect(downItem) }) + downItem.SetHidden(true) + t.downItem = downItem menu.AddSeparator() @@ -584,7 +593,9 @@ func (t *Tray) buildMenu() *application.Menu { return menu } -func (t *Tray) handleConnect() { +// handleConnect receives the clicked item from the buildMenu closure — +// t.upItem is menuMu-guarded and must not be read here. +func (t *Tray) handleConnect(upItem *application.MenuItem) { // NeedsLogin/SessionExpired/LoginFailed mean the daemon won't honor a // plain Up RPC ("up already in progress: current status NeedsLogin") — // it needs the Login → WaitSSOLogin → Up sequence instead. Emit @@ -601,7 +612,7 @@ func (t *Tray) handleConnect() { t.app.Event.Emit(services.EventTriggerLogin) return } - t.upItem.SetEnabled(false) + upItem.SetEnabled(false) // Arm the SSO auto-handoff: Up() is async and the daemon may flip to // NeedsLogin once it detects an SSO peer with no cached token. The // flag is consumed by applyStatus on that transition, which then @@ -619,7 +630,7 @@ func (t *Tray) handleConnect() { t.statusMu.Lock() t.pendingConnectLogin = false t.statusMu.Unlock() - t.upItem.SetEnabled(true) + upItem.SetEnabled(true) } }() } @@ -630,8 +641,9 @@ func (t *Tray) handleConnect() { // no-op. Also clears Peers' optimistic-Connecting guard so the daemon's // Idle push (and any subsequent updates) paint through immediately // instead of being swallowed by the profile-switch suppression filter. -func (t *Tray) handleDisconnect() { - t.downItem.SetEnabled(false) +// Receives the clicked item from the buildMenu closure (see handleConnect). +func (t *Tray) handleDisconnect(downItem *application.MenuItem) { + downItem.SetEnabled(false) t.profileMu.Lock() if t.switchCancel != nil { t.switchCancel() @@ -643,7 +655,7 @@ func (t *Tray) handleDisconnect() { if err := t.svc.Connection.Down(context.Background()); err != nil { log.Errorf("disconnect: %v", err) t.notifyError(t.loc.T("notify.error.disconnect")) - t.downItem.SetEnabled(true) + downItem.SetEnabled(true) } }() } diff --git a/client/ui/tray_profiles.go b/client/ui/tray_profiles.go index e130695a6..ddaaa39b7 100644 --- a/client/ui/tray_profiles.go +++ b/client/ui/tray_profiles.go @@ -54,9 +54,6 @@ func (t *Tray) loadConfig() { // 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 - } t.profileLoadMu.Lock() defer t.profileLoadMu.Unlock() ctx := context.Background() diff --git a/client/ui/tray_session.go b/client/ui/tray_session.go index aa6643f73..671cea5af 100644 --- a/client/ui/tray_session.go +++ b/client/ui/tray_session.go @@ -50,11 +50,10 @@ func (t *Tray) handleSessionExpired() { } } -// applySessionExpiry refreshes the "Session: 47m" tray row from the latest -// SSO deadline carried on the Status snapshot. Hidden when no deadline is -// tracked or the tunnel is down; otherwise renders the remaining time via -// formatSessionRemaining. -func (t *Tray) applySessionExpiry(deadline *time.Time, connected bool) { +// applySessionExpiry refreshes the cached SSO deadline and reports whether +// it changed. Cache-only — the tray row is painted by relayoutMenu; +// applyStatus drives a relayout when this returns true. +func (t *Tray) applySessionExpiry(deadline *time.Time, connected bool) bool { var d time.Time if connected && deadline != nil { d = *deadline @@ -76,17 +75,7 @@ func (t *Tray) applySessionExpiry(deadline *time.Time, connected bool) { deadline.Format(time.RFC3339), time.Until(*deadline), connected) } } - - if t.sessionExpiresItem == nil { - return - } - if d.IsZero() { - t.sessionExpiresItem.SetHidden(true) - return - } - remaining := t.formatSessionRemaining(time.Until(d)) - t.sessionExpiresItem.SetLabel(t.loc.T("tray.session.expiresIn", "remaining", remaining)) - t.sessionExpiresItem.SetHidden(false) + return changed } // runSessionExpiryTicker keeps the "Expires in …" countdown row fresh by @@ -99,10 +88,15 @@ func (t *Tray) runSessionExpiryTicker() { } } -// refreshSessionExpiresLabel recomputes the "Session expires in …" tray -// row label from the cached SSO deadline. +// refreshSessionExpiresLabel recomputes the countdown row label from the +// cached SSO deadline. The item is snapshotted under menuMu (buildMenu +// reassigns it on every relayout); no full relayout here — a 30s-cadence +// rebuild could disturb an open menu. func (t *Tray) refreshSessionExpiresLabel() { - if t.sessionExpiresItem == nil { + t.menuMu.Lock() + item := t.sessionExpiresItem + t.menuMu.Unlock() + if item == nil { return } t.sessionMu.Lock() @@ -112,7 +106,7 @@ func (t *Tray) refreshSessionExpiresLabel() { return } remaining := t.formatSessionRemaining(time.Until(deadline)) - t.sessionExpiresItem.SetLabel(t.loc.T("tray.session.expiresIn", "remaining", remaining)) + item.SetLabel(t.loc.T("tray.session.expiresIn", "remaining", remaining)) } // formatSessionRemaining renders the time-to-deadline as a localised diff --git a/client/ui/tray_status.go b/client/ui/tray_status.go index f45eb7f7b..40d76cf51 100644 --- a/client/ui/tray_status.go +++ b/client/ui/tray_status.go @@ -58,9 +58,19 @@ func (t *Tray) applyStatus(st services.Status) { t.app.Event.Emit(services.EventTriggerLogin) } + // Cache-only; the row is painted by the relayout below. + sessionChanged := t.applySessionExpiry(st.SessionExpiresAt, connected) + if iconChanged { t.applyIcon() - t.refreshMenuItemsForStatus(st, connected) + } + // All repainting goes through relayoutMenu (menuMu-serialised, paints + // from the caches committed above): applyStatus runs concurrently with + // itself and with relayouts (Wails dispatches listeners on fresh + // goroutines), so in-place item mutation here would race the buildMenu + // pointer swap. + if iconChanged || daemonVersionChanged || sessionChanged { + t.relayoutMenu() } // Re-fetch the selectable exit-node list whenever the daemon's routed- // networks revision bumps (a route candidate added/removed, or a selection @@ -71,18 +81,14 @@ func (t *Tray) applyStatus(st services.Status) { if iconChanged || revisionChanged { go t.refreshExitNodes() } - if daemonVersionChanged && t.daemonVersionItem != nil { - // 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() + // The daemon emits no active-profile event, so profile flips driven + // elsewhere (CLI, autoconnect) surface via status transitions. + if iconChanged { + go t.loadProfiles() } if sessionExpiredEnter { t.handleSessionExpired() } - - t.applySessionExpiry(st.SessionExpiresAt, connected) } // consumePendingConnectLogin acts on the SSO auto-handoff flag armed by @@ -111,84 +117,14 @@ func (t *Tray) consumePendingConnectLogin(status string) bool { return false } -// refreshMenuItemsForStatus updates the status row, Connect/Disconnect -// enablement, Settings/Profiles gating, and Profiles submenu on a status-text -// transition (called from applyStatus only when iconChanged). -func (t *Tray) refreshMenuItemsForStatus(st services.Status, connected bool) { - daemonUnavailable := strings.EqualFold(st.Status, services.StatusDaemonUnavailable) - connecting := strings.EqualFold(st.Status, services.StatusConnecting) - if t.statusItem != nil { - // Label-only: row is informational (no OnClick). Enablement - // is platform-dependent via statusRowEnabled — Windows - // keeps it enabled so the Win32 disabled-state mask does - // not desaturate the coloured dot; macOS/Linux disable it. - // Swap the displayed text so the user sees a familiar - // phrase instead of the raw daemon enum. - t.statusItem.SetLabel(t.loc.StatusLabel(st.Status)) - t.statusItem.SetEnabled(statusRowEnabled()) - t.applyStatusIndicator(st.Status) - } - if t.upItem != nil { - // Connect stays visible/clickable in NeedsLogin/SessionExpired/ - // LoginFailed too — the daemon's Up RPC kicks off the SSO flow - // when re-auth is required, mirroring the legacy Fyne client - // where the same button drove the initial and the re-login - // paths. Hidden only when the action would be a no-op (tunnel - // up, daemon mid-connect — Disconnect takes the slot) or - // would fail with no useful side effect (daemon unreachable). - t.upItem.SetHidden(connected || connecting || daemonUnavailable) - t.upItem.SetEnabled(!connected && !connecting && !daemonUnavailable) - } - if t.downItem != nil { - // Disconnect is the abort path while the daemon is still - // retrying the management dial — without it the user has no - // way to stop the loop short of killing the daemon. - t.downItem.SetHidden(!connected && !connecting) - t.downItem.SetEnabled(connected || connecting) - } - // Exit Node parent-item enablement (greyed unless the tunnel is up - // AND at least one candidate exists) is owned by refreshExitNodes, - // triggered by applyStatus on this same transition. Settings just needs - // the daemon socket reachable. - if t.settingsItem != nil { - t.settingsItem.SetEnabled(!daemonUnavailable) - } - disableProfiles, _ := t.featuresDisabled() - if t.profileSubmenuItem != nil { - t.profileSubmenuItem.SetEnabled(!daemonUnavailable && !disableProfiles) - } - // 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. 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() -} - -// applyStatusIndicator sets the small coloured dot shown on the status -// menu entry. The dot mirrors the tray icon's state through a fixed -// palette: green for Connected, yellow for Connecting, blue for the -// login states, red for hard errors, grey for the idle/disconnected -// pair and a darker grey when the daemon socket is unreachable. -// -// Wails v3 alpha's setMenuItemBitmap calls NSMenuItem.setImage from -// whichever thread invoked SetBitmap — unlike setMenuItemLabel/Disabled/ -// Hidden/Checked which dispatch_sync onto the main queue. The off-thread -// AppKit call leaves the visible dot stale until the next time the menu -// is reopened (close+reopen workaround). Rebuilding via tray.SetMenu -// reruns processMenu inside InvokeSync, so the bitmap is applied to a -// fresh NSMenuItem on the main thread and macOS picks it up. +// applyStatusIndicator sets the coloured status dot. Called only from +// relayoutMenu (menuMu held): on macOS the bitmap repaints via the +// relayout's trailing SetMenu — no SetMenu here, the tree is half-built. func (t *Tray) applyStatusIndicator(status string) { if t.statusItem == nil { return } t.statusItem.SetBitmap(statusIndicatorBitmap(status)) - if t.menu != nil { - t.tray.SetMenu(t.menu) - } } func statusIndicatorBitmap(status string) []byte {