From 1416a2e1606df98e7ca41a15f202072829894498 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Papp?= Date: Fri, 29 May 2026 14:45:58 +0200 Subject: [PATCH] ui: reduce cognitive complexity in tray/feed/xembed status handlers Extract helpers to bring three methods under the 20 cognitive-complexity limit without changing behavior: - DaemonFeed.statusStreamLoop: split out handleStatusRecvErr and emitStatus - Tray.applyStatus: split out consumePendingConnectLogin and refreshMenuItemsForStatus - xembedHost.flattenMenu: split out menuItemFromLayout plus propString / propBool / propInt32 dbusmenu property accessors --- client/ui/services/daemon_feed.go | 41 +++++--- client/ui/tray_status.go | 156 ++++++++++++++++-------------- client/ui/xembed_host_linux.go | 128 +++++++++++++----------- 3 files changed, 184 insertions(+), 141 deletions(-) diff --git a/client/ui/services/daemon_feed.go b/client/ui/services/daemon_feed.go index 5d7d7103b..c330e17bd 100644 --- a/client/ui/services/daemon_feed.go +++ b/client/ui/services/daemon_feed.go @@ -349,22 +349,10 @@ func (s *DaemonFeed) statusStreamLoop(ctx context.Context) { for { resp, err := stream.Recv() if err != nil { - if ctx.Err() != nil { - return ctx.Err() - } - if isDaemonUnreachable(err) { - emitUnavailable() - } - return fmt.Errorf("status stream recv: %w", err) + return s.handleStatusRecvErr(ctx, err, emitUnavailable) } unavailable = false - st := statusFromProto(resp) - log.Infof("backend event: status status=%q peers=%d", st.Status, len(st.Peers)) - if s.shouldSuppress(st) { - log.Debugf("suppressing status=%q during profile switch", st.Status) - continue - } - s.emitter.Emit(EventStatusSnapshot, st) + s.emitStatus(statusFromProto(resp)) } } @@ -373,6 +361,31 @@ func (s *DaemonFeed) statusStreamLoop(ctx context.Context) { } } +// handleStatusRecvErr maps a SubscribeStatus stream.Recv error into the +// backoff loop's return value: ctx cancellation stops the loop, an +// unreachable socket flips the synthetic-unavailable signal, everything +// else is a retryable wrapped error. +func (s *DaemonFeed) handleStatusRecvErr(ctx context.Context, err error, emitUnavailable func()) error { + if ctx.Err() != nil { + return ctx.Err() + } + if isDaemonUnreachable(err) { + emitUnavailable() + } + return fmt.Errorf("status stream recv: %w", err) +} + +// emitStatus pushes a fresh snapshot to the frontend, dropping the transient +// stale-Connected / Idle pushes that occur mid profile switch. +func (s *DaemonFeed) emitStatus(st Status) { + log.Infof("backend event: status status=%q peers=%d", st.Status, len(st.Peers)) + if s.shouldSuppress(st) { + log.Debugf("suppressing status=%q during profile switch", st.Status) + return + } + s.emitter.Emit(EventStatusSnapshot, st) +} + // toastStreamLoop subscribes to the daemon's SubscribeEvents RPC and // re-emits every SystemEvent on the Wails event bus. The downstream // consumers turn these into OS notifications, populate the Recent diff --git a/client/ui/tray_status.go b/client/ui/tray_status.go index 6a509bf1a..294dc61a4 100644 --- a/client/ui/tray_status.go +++ b/client/ui/tray_status.go @@ -41,26 +41,7 @@ func (t *Tray) applyStatus(st services.Status) { sessionExpiredEnter := strings.EqualFold(st.Status, services.StatusSessionExpired) && !strings.EqualFold(t.lastStatus, services.StatusSessionExpired) - // Consume the SSO auto-handoff flag armed by handleConnect. Trigger - // the browser-login flow on a Connect → NeedsLogin transition so the - // user doesn't need to click Connect a second time. Clear it on any - // other terminal state — including Connecting bursts that resolve to - // Connected / Idle / LoginFailed / DaemonUnavailable — so a stale - // flag can't fire weeks later when the daemon happens to flip. - triggerLogin := false - if t.pendingConnectLogin { - switch { - case strings.EqualFold(st.Status, services.StatusNeedsLogin): - triggerLogin = true - t.pendingConnectLogin = false - case strings.EqualFold(st.Status, services.StatusConnected), - strings.EqualFold(st.Status, services.StatusIdle), - strings.EqualFold(st.Status, services.StatusLoginFailed), - strings.EqualFold(st.Status, services.StatusSessionExpired), - strings.EqualFold(st.Status, services.StatusDaemonUnavailable): - t.pendingConnectLogin = false - } - } + triggerLogin := t.consumePendingConnectLogin(st.Status) daemonVersionChanged := st.DaemonVersion != "" && st.DaemonVersion != t.lastDaemonVersion t.connected = connected @@ -79,57 +60,7 @@ func (t *Tray) applyStatus(st services.Status) { if iconChanged { t.applyIcon() - 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 below on this same transition. Settings just needs the - // daemon socket reachable. - if t.settingsItem != nil { - t.settingsItem.SetEnabled(!daemonUnavailable) - } - if t.profileSubmenuItem != nil { - t.profileSubmenuItem.SetEnabled(!daemonUnavailable) - } - // 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. - go t.loadProfiles() + t.refreshMenuItemsForStatus(st, connected) } // Re-fetch the selectable exit-node list whenever the daemon's routed- // networks revision bumps (a route candidate added/removed, or a selection @@ -150,6 +81,89 @@ func (t *Tray) applyStatus(st services.Status) { t.applySessionExpiry(st.SessionExpiresAt, connected) } +// consumePendingConnectLogin acts on the SSO auto-handoff flag armed by +// handleConnect. It returns true (and clears the flag) when the daemon +// reached NeedsLogin, signalling the browser-login flow should start so the +// user doesn't need to click Connect a second time. The flag is also cleared +// on any other terminal state — including Connecting bursts that resolve to +// Connected / Idle / LoginFailed / DaemonUnavailable — so a stale flag can't +// fire weeks later when the daemon happens to flip. Must be called with +// statusMu held. +func (t *Tray) consumePendingConnectLogin(status string) bool { + if !t.pendingConnectLogin { + return false + } + switch { + case strings.EqualFold(status, services.StatusNeedsLogin): + t.pendingConnectLogin = false + return true + case strings.EqualFold(status, services.StatusConnected), + strings.EqualFold(status, services.StatusIdle), + strings.EqualFold(status, services.StatusLoginFailed), + strings.EqualFold(status, services.StatusSessionExpired), + strings.EqualFold(status, services.StatusDaemonUnavailable): + t.pendingConnectLogin = false + } + 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) + } + if t.profileSubmenuItem != nil { + t.profileSubmenuItem.SetEnabled(!daemonUnavailable) + } + // 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. + 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 diff --git a/client/ui/xembed_host_linux.go b/client/ui/xembed_host_linux.go index 3c94848b7..f90e7de75 100644 --- a/client/ui/xembed_host_linux.go +++ b/client/ui/xembed_host_linux.go @@ -51,8 +51,6 @@ type dbusMenuLayout struct { Children []dbus.Variant } - - // xembedHost manages one XEmbed tray icon for an SNI item. type xembedHost struct { conn *dbus.Conn @@ -314,66 +312,51 @@ func (h *xembedHost) flattenMenu(layout dbusMenuLayout) []menuItemInfo { if err := dbus.Store([]interface{}{childVar.Value()}, &child); err != nil { continue } - - mi := menuItemInfo{ - id: child.ID, - enabled: true, + if mi, ok := h.menuItemFromLayout(child); ok { + items = append(items, mi) } - - if v, ok := child.Properties["type"]; ok { - if s, ok := v.Value().(string); ok && s == "separator" { - mi.isSeparator = true - items = append(items, mi) - continue - } - } - - if v, ok := child.Properties["label"]; ok { - if s, ok := v.Value().(string); ok { - mi.label = s - } - } - - if v, ok := child.Properties["enabled"]; ok { - if b, ok := v.Value().(bool); ok { - mi.enabled = b - } - } - - if v, ok := child.Properties["visible"]; ok { - if b, ok := v.Value().(bool); ok && !b { - continue // skip hidden items - } - } - - if v, ok := child.Properties["toggle-type"]; ok { - if s, ok := v.Value().(string); ok && s == "checkmark" { - mi.isCheck = true - } - } - - if v, ok := child.Properties["toggle-state"]; ok { - if n, ok := v.Value().(int32); ok && n == 1 { - mi.checked = true - } - } - - // Recurse into nested submenus. The dbusmenu spec marks a folder - // item with children-display=="submenu"; the children are already - // in child.Children because GetLayout was called with - // recursionDepth=-1 (all levels). - if v, ok := child.Properties["children-display"]; ok { - if s, ok := v.Value().(string); ok && s == "submenu" { - mi.children = h.flattenMenu(child) - } - } - - items = append(items, mi) } return items } +// menuItemFromLayout decodes one dbusmenu child node into a menuItemInfo, +// recursing into submenus. The bool return is false when the item is hidden +// (visible=false) and should be dropped from the parent's list. +func (h *xembedHost) menuItemFromLayout(child dbusMenuLayout) (menuItemInfo, bool) { + mi := menuItemInfo{id: child.ID, enabled: true} + + if propString(child.Properties, "type") == "separator" { + mi.isSeparator = true + return mi, true + } + + if vis, ok := propBool(child.Properties, "visible"); ok && !vis { + return menuItemInfo{}, false + } + + mi.label = propString(child.Properties, "label") + if en, ok := propBool(child.Properties, "enabled"); ok { + mi.enabled = en + } + if propString(child.Properties, "toggle-type") == "checkmark" { + mi.isCheck = true + } + if n, ok := propInt32(child.Properties, "toggle-state"); ok && n == 1 { + mi.checked = true + } + + // Recurse into nested submenus. The dbusmenu spec marks a folder + // item with children-display=="submenu"; the children are already + // in child.Children because GetLayout was called with + // recursionDepth=-1 (all levels). + if propString(child.Properties, "children-display") == "submenu" { + mi.children = h.flattenMenu(child) + } + + return mi, true +} + func (h *xembedHost) sendMenuEvent(id int32) { menuPath := dbus.ObjectPath("/StatusNotifierMenu") menuObj := h.conn.Object(h.busName, menuPath) @@ -442,3 +425,36 @@ func boolToInt(b bool) C.int { } return 0 } + +// propString returns the string value of a dbusmenu property, or "" when the +// property is absent or not a string. +func propString(props map[string]dbus.Variant, key string) string { + if v, ok := props[key]; ok { + if s, ok := v.Value().(string); ok { + return s + } + } + return "" +} + +// propBool returns the bool value of a dbusmenu property; ok is false when the +// property is absent or not a bool. +func propBool(props map[string]dbus.Variant, key string) (value, ok bool) { + if v, present := props[key]; present { + if b, isBool := v.Value().(bool); isBool { + return b, true + } + } + return false, false +} + +// propInt32 returns the int32 value of a dbusmenu property; ok is false when +// the property is absent or not an int32. +func propInt32(props map[string]dbus.Variant, key string) (value int32, ok bool) { + if v, present := props[key]; present { + if n, isInt := v.Value().(int32); isInt { + return n, true + } + } + return 0, false +}