From a76858e64681aabdc4b57cf98c3cba0a278ed641 Mon Sep 17 00:00:00 2001 From: Evan Lowry Date: Sun, 26 Apr 2026 05:16:37 +0000 Subject: [PATCH] client/systray: refresh in place; drop full rebuild on every event Observed on a minimalist Wayland setup w/ ashell: every IPN-bus event called systray.ResetMenu() and re-added the full menu. The resulting LayoutUpdated signal storm let libdbusmenu hosts race into half-built menus and cache empty submenus. The menu is now built once and only rebuilt when its structural shape changes. Three further bugs surfaced while I was A/B testing these changes, that I included fixes for: - The initial icon is set before systray.Run so the StatusNotifierItem exports a non-empty image; this crashed ashell (added resilience upstream at MalpenZibo/ashell#693) zero-size pixmap. - Mullvad VPN and multi-city country rows are now plain submenu items rather than checkboxes-with-children, which some hosts treated as selectable leaves -- clicking the parent dismissed the menu instead of expanding it (#17879) - Exit-node selection is driven off prefs.ExitNodeID rather than status.ExitNodeStatus.ID, so a configured-but-unreachable exit node renders as "exit node offline" instead of as a plain connected tray. Fixes #15528 Fixes #17879 Signed-off-by: Evan Lowry --- client/systray/logo.go | 2 +- client/systray/systray.go | 464 ++++++++++++++++++++++++++------- client/systray/systray_test.go | 418 +++++++++++++++++++++++++++++ 3 files changed, 793 insertions(+), 91 deletions(-) create mode 100644 client/systray/systray_test.go diff --git a/client/systray/logo.go b/client/systray/logo.go index a0f8bf7d0..d713b34af 100644 --- a/client/systray/logo.go +++ b/client/systray/logo.go @@ -264,7 +264,7 @@ func (logo tsLogo) renderWithBorder(borderUnits int) *bytes.Buffer { } // setAppIcon renders logo and sets it as the systray icon. -func setAppIcon(icon tsLogo) { +func setAppIcon(icon *tsLogo) { if icon.dots == loading.dots { startLoadingAnimation() } else { diff --git a/client/systray/systray.go b/client/systray/systray.go index 65c1bec20..c48f66a5f 100644 --- a/client/systray/systray.go +++ b/client/systray/systray.go @@ -56,6 +56,10 @@ func (menu *Menu) Run(client *local.Client) { menu.lc = client menu.updateState() + // Set the initial icon before systray.Run so the StatusNotifierItem + // exports a non-empty image on startup. + setAppIcon(&disconnected) + // exit cleanly on SIGINT and SIGTERM go func() { interrupt := make(chan os.Signal, 1) @@ -78,6 +82,7 @@ type Menu struct { lc *local.Client status *ipnstate.Status + prefs *ipn.Prefs curProfile ipn.LoginProfile allProfiles []ipn.LoginProfile @@ -98,9 +103,29 @@ type Menu struct { rebuildMenu *systray.MenuItem quit *systray.MenuItem - rebuildCh chan struct{} // triggers a menu rebuild - accountsCh chan ipn.ProfileID - exitNodeCh chan tailcfg.StableNodeID // ID of selected exit node + // lastShape is the menuShape from the most recent build. + lastShape menuShape + + // Cached last-applied values used to short-circuit redundant updates. + connectTitle string + selfTitle string + disconnectVisible bool + lastTooltip string + lastIcon *tsLogo + + // Per-row tracking, populated by buildMenu and reset on each rebuild. + accountItems map[ipn.ProfileID]*systray.MenuItem + noExitNodeItem *systray.MenuItem + recommendedExitItem *systray.MenuItem + recommendedExitNodeID tailcfg.StableNodeID + tailnetExitItems map[tailcfg.StableNodeID]*systray.MenuItem + mullvadCountryItems map[string]*systray.MenuItem // CC -> item (single-city only) + mullvadCityItems map[string]*systray.MenuItem // "/" -> item + + rebuildCh chan struct{} // triggers a menu refresh (build if shape changed) + forceBuildCh chan struct{} // forces a full buildMenu (escape hatch) + accountsCh chan ipn.ProfileID + exitNodeCh chan tailcfg.StableNodeID // ID of selected exit node eventCancel context.CancelFunc // cancel eventLoop @@ -114,6 +139,7 @@ func (menu *Menu) init() { } menu.rebuildCh = make(chan struct{}, 1) + menu.forceBuildCh = make(chan struct{}, 1) menu.accountsCh = make(chan ipn.ProfileID) menu.exitNodeCh = make(chan tailcfg.StableNodeID) @@ -170,13 +196,13 @@ tailscale systray See https://tailscale.com/kb/1597/linux-systray for more information.`) } - setAppIcon(disconnected) + setAppIcon(&disconnected) // set initial title, which is used by the systray package as the ID of the StatusNotifierItem. // This value will get overwritten later as the client status changes. systray.SetTitle("tailscale") - menu.rebuild() + menu.buildMenu() menu.mu.Lock() if menu.readonly { @@ -203,6 +229,13 @@ func (menu *Menu) updateState() { if err != nil { log.Print(err) } + menu.prefs, err = menu.lc.GetPrefs(menu.bgCtx) + if err != nil { + if local.IsAccessDeniedError(err) { + menu.readonly = true + } + log.Print(err) + } menu.curProfile, menu.allProfiles, err = menu.lc.ProfileStatus(menu.bgCtx) if err != nil { if local.IsAccessDeniedError(err) { @@ -212,22 +245,276 @@ func (menu *Menu) updateState() { } } -// rebuild the systray menu based on the current Tailscale state. -// -// We currently rebuild the entire menu because it is not easy to update the existing menu. -// You cannot iterate over the items in a menu, nor can you remove some items like separators. -// So for now we rebuild the whole thing, and can optimize this later if needed. -func (menu *Menu) rebuild() { +// activeExitNodeID returns the user's selected exit node, preferring +// prefs over status so a configured-but-unreachable exit node still +// reports as selected. +func activeExitNodeID(prefs *ipn.Prefs, status *ipnstate.Status) tailcfg.StableNodeID { + if prefs != nil && !prefs.ExitNodeID.IsZero() { + return prefs.ExitNodeID + } + if status != nil && status.ExitNodeStatus != nil { + return status.ExitNodeStatus.ID + } + return "" +} + +// appearance returns the systray icon and tooltip for the given state. +func appearance(status *ipnstate.Status, prefs *ipn.Prefs) (*tsLogo, string) { + if status == nil { + return &disconnected, "Disconnected" + } + switch status.BackendState { + case ipn.Running.String(): + if !activeExitNodeID(prefs, status).IsZero() { + if status.ExitNodeStatus != nil && status.ExitNodeStatus.Online { + return &exitNodeOnline, "Using exit node" + } + return &exitNodeOffline, "Exit node offline" + } + return &connected, fmt.Sprintf("Connected to %s", status.CurrentTailnet.Name) + case ipn.Starting.String(): + return &loading, "Connecting" + } + return &disconnected, "Disconnected" +} + +// setEnabled and setChecked skip the underlying call when the item is +// already in the desired state, to avoid redundant menu update signals. +func setEnabled(item *systray.MenuItem, enabled bool) { + if item == nil || item.Disabled() == !enabled { + return + } + + if enabled { + item.Enable() + } else { + item.Disable() + } +} + +func setChecked(item *systray.MenuItem, checked bool) { + if item == nil || item.Checked() == checked { + return + } + + if checked { + item.Check() + } else { + item.Uncheck() + } +} + +// setTitleIfChanged updates item's title only when it differs from the +// cached value. +func setTitleIfChanged(cache *string, item *systray.MenuItem, title string) { + if item == nil || *cache == title { + return + } + + *cache = title + item.SetTitle(title) +} + +// setVisibleIfChanged toggles item visibility only when it differs from +// the cached value. +func setVisibleIfChanged(cache *bool, item *systray.MenuItem, visible bool) { + if item == nil || *cache == visible { + return + } + + *cache = visible + if visible { + item.Show() + } else { + item.Hide() + } +} + +// setTooltipIfChanged forwards to setTooltip only when the text differs +// from the last call. +func (menu *Menu) setTooltipIfChanged(text string) { + if menu.lastTooltip == text { + return + } + menu.lastTooltip = text + setTooltip(text) +} + +// setAppIconIfChanged forwards to setAppIcon only when icon differs from +// the last call. +func (menu *Menu) setAppIconIfChanged(icon *tsLogo) { + if menu.lastIcon == icon { + return + } + menu.lastIcon = icon + setAppIcon(icon) +} + +// menuShape captures everything that, if changed, requires a full rebuild of +// the menu rather than an in-place property refresh. +type menuShape struct { + readonly bool + backendKnown bool + curProfileID ipn.ProfileID + profileIDs string // sorted, comma-joined ProfileIDs + tailnetExitIDs string // sorted, comma-joined StableNodeIDs of tailnet exit nodes + mullvadEnabled bool + mullvadKey string // sorted, comma-joined "/" pairs +} + +// computeShape derives a menuShape from the given inputs. +func computeShape(status *ipnstate.Status, curProfile ipn.LoginProfile, allProfiles []ipn.LoginProfile, readonly bool) menuShape { + s := menuShape{ + readonly: readonly, + backendKnown: status != nil, + curProfileID: curProfile.ID, + } + + ids := make([]string, 0, len(allProfiles)) + for _, p := range allProfiles { + ids = append(ids, string(p.ID)) + } + slices.Sort(ids) + s.profileIDs = strings.Join(ids, ",") + + if status == nil { + return s + } + + var tailnetIDs []string + var mullvadPairs []string + mullvadEligible := status.Self != nil && status.Self.CapMap.Contains("mullvad") + for _, ps := range status.Peer { + if !ps.ExitNodeOption { + continue + } + if ps.Location == nil { + tailnetIDs = append(tailnetIDs, string(ps.ID)) + continue + } + if mullvadEligible { + mullvadPairs = append(mullvadPairs, ps.Location.CountryCode+"/"+ps.Location.CityCode) + } + } + slices.Sort(tailnetIDs) + s.tailnetExitIDs = strings.Join(tailnetIDs, ",") + s.mullvadEnabled = mullvadEligible && len(mullvadPairs) > 0 + if s.mullvadEnabled { + slices.Sort(mullvadPairs) + mullvadPairs = slices.Compact(mullvadPairs) + s.mullvadKey = strings.Join(mullvadPairs, ",") + } + return s +} + +// refreshMenu reflects the current Tailscale state into the menu. If the +// structural shape of the inputs hasn't changed, it mutates existing items +// in place; otherwise it falls back to a full buildMenu. +func (menu *Menu) refreshMenu() { menu.mu.Lock() defer menu.mu.Unlock() menu.init() + shape := computeShape(menu.status, menu.curProfile, menu.allProfiles, menu.readonly) + if menu.connect == nil || shape != menu.lastShape { + menu.buildMenuLocked() + return + } + + icon, tooltip := appearance(menu.status, menu.prefs) + menu.setTooltipIfChanged(tooltip) + menu.setAppIconIfChanged(icon) + + running := menu.status != nil && menu.status.BackendState == ipn.Running.String() + if running { + setTitleIfChanged(&menu.connectTitle, menu.connect, "Connected") + setEnabled(menu.connect, false) + setVisibleIfChanged(&menu.disconnectVisible, menu.disconnect, true) + setEnabled(menu.disconnect, !menu.readonly) + } else { + setTitleIfChanged(&menu.connectTitle, menu.connect, "Connect") + setEnabled(menu.connect, !menu.readonly) + setVisibleIfChanged(&menu.disconnectVisible, menu.disconnect, false) + } + setEnabled(menu.more, running) + + if menu.status != nil && menu.status.Self != nil && len(menu.status.Self.TailscaleIPs) > 0 { + setTitleIfChanged(&menu.selfTitle, menu.self, fmt.Sprintf("This Device: %s (%s)", + menu.status.Self.HostName, menu.status.Self.TailscaleIPs[0])) + setEnabled(menu.self, true) + } else { + setTitleIfChanged(&menu.selfTitle, menu.self, "This Device: not connected") + setEnabled(menu.self, false) + } + + for id, item := range menu.accountItems { + setChecked(item, id == menu.curProfile.ID) + } + + sel := activeExitNodeID(menu.prefs, menu.status) + setChecked(menu.noExitNodeItem, sel == "") + setChecked(menu.recommendedExitItem, sel != "" && sel == menu.recommendedExitNodeID) + + var mvCountry, mvCity string + if menu.status != nil { + for _, ps := range menu.status.Peer { + if !ps.ExitNodeOption { + continue + } + if ps.Location == nil { + if item, ok := menu.tailnetExitItems[ps.ID]; ok { + setChecked(item, ps.ID == sel) + setEnabled(item, ps.Online) + } + continue + } + if sel != "" && ps.ID == sel { + mvCountry = ps.Location.CountryCode + mvCity = mvCountry + "/" + ps.Location.CityCode + } + } + } + for cc, item := range menu.mullvadCountryItems { + setChecked(item, cc == mvCountry) + } + for k, item := range menu.mullvadCityItems { + setChecked(item, k == mvCity) + } +} + +// buildMenu rebuilds the menu from scratch. Prefer refreshMenu when only +// properties have changed. +func (menu *Menu) buildMenu() { + menu.mu.Lock() + defer menu.mu.Unlock() + menu.buildMenuLocked() +} + +// buildMenuLocked is buildMenu with menu.mu already held. +func (menu *Menu) buildMenuLocked() { + menu.init() + if menu.eventCancel != nil { menu.eventCancel() } ctx := context.Background() ctx, menu.eventCancel = context.WithCancel(ctx) + // Reset trackers and caches; ResetMenu invalidates the previous + // build's item pointers. + menu.accountItems = map[ipn.ProfileID]*systray.MenuItem{} + menu.tailnetExitItems = map[tailcfg.StableNodeID]*systray.MenuItem{} + menu.mullvadCountryItems = map[string]*systray.MenuItem{} + menu.mullvadCityItems = map[string]*systray.MenuItem{} + menu.noExitNodeItem = nil + menu.recommendedExitItem = nil + menu.recommendedExitNodeID = "" + menu.connectTitle = "" + menu.selfTitle = "" + menu.disconnectVisible = false + menu.lastTooltip = "" + menu.lastIcon = nil + systray.ResetMenu() if menu.readonly { @@ -247,41 +534,17 @@ func (menu *Menu) rebuild() { // delay to prevent race setting icon on first start time.Sleep(newMenuDelay) - // Set systray menu icon and title. - // Also adjust connect/disconnect menu items if needed. - var backendState string - if menu.status != nil { - backendState = menu.status.BackendState - } - switch backendState { - case ipn.Running.String(): - if menu.status.ExitNodeStatus != nil && !menu.status.ExitNodeStatus.ID.IsZero() { - if menu.status.ExitNodeStatus.Online { - setTooltip("Using exit node") - setAppIcon(exitNodeOnline) - } else { - setTooltip("Exit node offline") - setAppIcon(exitNodeOffline) - } - } else { - setTooltip(fmt.Sprintf("Connected to %s", menu.status.CurrentTailnet.Name)) - setAppIcon(connected) - } - menu.connect.SetTitle("Connected") - menu.connect.Disable() - menu.disconnect.Show() - menu.disconnect.Enable() - case ipn.Starting.String(): - setTooltip("Connecting") - setAppIcon(loading) - default: - setTooltip("Disconnected") - setAppIcon(disconnected) - } + icon, tooltip := appearance(menu.status, menu.prefs) + menu.setTooltipIfChanged(tooltip) + menu.setAppIconIfChanged(icon) - if menu.readonly { + if menu.status != nil && menu.status.BackendState == ipn.Running.String() { + setTitleIfChanged(&menu.connectTitle, menu.connect, "Connected") + menu.connect.Disable() + setVisibleIfChanged(&menu.disconnectVisible, menu.disconnect, true) + setEnabled(menu.disconnect, !menu.readonly) + } else if menu.readonly { menu.connect.Disable() - menu.disconnect.Disable() } account := "Account" @@ -298,8 +561,9 @@ func (menu *Menu) rebuild() { if profile.ID == menu.curProfile.ID { item = accounts.AddSubMenuItemCheckbox(title, "", true) } else { - item = accounts.AddSubMenuItem(title, "") + item = accounts.AddSubMenuItemCheckbox(title, "", false) } + menu.accountItems[profile.ID] = item setRemoteIcon(item, profile.UserProfile.ProfilePicURL) onClick(ctx, item, func(ctx context.Context) { select { @@ -337,9 +601,12 @@ func (menu *Menu) rebuild() { // but is at least more discoverable than having users switch profiles or exit nodes. menu.rebuildMenu = systray.AddMenuItem("Rebuild menu", "Fix missing menu items") onClick(ctx, menu.rebuildMenu, func(ctx context.Context) { + // Force a full rebuild; refresh would be a no-op when the + // structural shape is unchanged. select { case <-ctx.Done(): - case menu.rebuildCh <- struct{}{}: + case menu.forceBuildCh <- struct{}{}: + default: } }) menu.rebuildMenu.Enable() @@ -347,6 +614,8 @@ func (menu *Menu) rebuild() { menu.quit = systray.AddMenuItem("Quit", "Quit the app") menu.quit.Enable() + menu.lastShape = computeShape(menu.status, menu.curProfile, menu.allProfiles, menu.readonly) + go menu.eventLoop(ctx) } @@ -417,9 +686,9 @@ func setTooltip(text string) { } } -// eventLoop is the main event loop for handling click events on menu items -// and responding to Tailscale state changes. -// This method does not return until ctx.Done is closed. +// eventLoop dispatches menu clicks and bus-driven refreshes until ctx is +// canceled. Daemon API calls use menu.bgCtx so user actions aren't lost +// when ctx is canceled by a concurrent rebuild. func (menu *Menu) eventLoop(ctx context.Context) { for { select { @@ -427,9 +696,12 @@ func (menu *Menu) eventLoop(ctx context.Context) { return case <-menu.rebuildCh: menu.updateState() - menu.rebuild() + menu.refreshMenu() + case <-menu.forceBuildCh: + menu.updateState() + menu.buildMenu() case <-menu.connect.ClickedCh: - _, err := menu.lc.EditPrefs(ctx, &ipn.MaskedPrefs{ + _, err := menu.lc.EditPrefs(menu.bgCtx, &ipn.MaskedPrefs{ Prefs: ipn.Prefs{ WantRunning: true, }, @@ -440,7 +712,7 @@ func (menu *Menu) eventLoop(ctx context.Context) { } case <-menu.disconnect.ClickedCh: - _, err := menu.lc.EditPrefs(ctx, &ipn.MaskedPrefs{ + _, err := menu.lc.EditPrefs(menu.bgCtx, &ipn.MaskedPrefs{ Prefs: ipn.Prefs{ WantRunning: false, }, @@ -454,14 +726,14 @@ func (menu *Menu) eventLoop(ctx context.Context) { menu.copyTailscaleIP(menu.status.Self) case id := <-menu.accountsCh: - if err := menu.lc.SwitchProfile(ctx, id); err != nil { + if err := menu.lc.SwitchProfile(menu.bgCtx, id); err != nil { log.Printf("error switching to profile ID %v: %v", id, err) } case exitNode := <-menu.exitNodeCh: if exitNode.IsZero() { log.Print("disable exit node") - if err := menu.lc.SetUseExitNode(ctx, false); err != nil { + if err := menu.lc.SetUseExitNode(menu.bgCtx, false); err != nil { log.Printf("error disabling exit node: %v", err) } } else { @@ -472,7 +744,7 @@ func (menu *Menu) eventLoop(ctx context.Context) { }, ExitNodeIDSet: true, } - if _, err := menu.lc.EditPrefs(ctx, mp); err != nil { + if _, err := menu.lc.EditPrefs(menu.bgCtx, mp); err != nil { log.Printf("error setting exit node: %v", err) } } @@ -549,7 +821,12 @@ func (menu *Menu) watchIPNBusInner() error { rebuild = true } if rebuild { - menu.rebuildCh <- struct{}{} + // Refreshes are level-triggered; a queued request already + // covers any further events. + select { + case menu.rebuildCh <- struct{}{}: + default: + } } } } @@ -606,7 +883,10 @@ func (menu *Menu) rebuildExitNodeMenu(ctx context.Context) { }) } - noExitNodeMenu := menu.exitNodes.AddSubMenuItemCheckbox("None", "", status.ExitNodeStatus == nil) + sel := activeExitNodeID(menu.prefs, status) + + noExitNodeMenu := menu.exitNodes.AddSubMenuItemCheckbox("None", "", sel == "") + menu.noExitNodeItem = noExitNodeMenu setExitNodeOnClick(noExitNodeMenu, "") // Show recommended exit node if available. @@ -621,11 +901,10 @@ func (menu *Menu) rebuildExitNodeMenu(ctx context.Context) { title += strings.Split(sugg.Name, ".")[0] } menu.exitNodes.AddSeparator() - rm := menu.exitNodes.AddSubMenuItemCheckbox(title, "", false) + rm := menu.exitNodes.AddSubMenuItemCheckbox(title, "", sel != "" && sugg.ID == sel) + menu.recommendedExitItem = rm + menu.recommendedExitNodeID = sugg.ID setExitNodeOnClick(rm, sugg.ID) - if status.ExitNodeStatus != nil && sugg.ID == status.ExitNodeStatus.ID { - rm.Check() - } } } @@ -647,13 +926,11 @@ func (menu *Menu) rebuildExitNodeMenu(ctx context.Context) { if !ps.Online { name += " (offline)" } - sm := menu.exitNodes.AddSubMenuItemCheckbox(name, "", false) + sm := menu.exitNodes.AddSubMenuItemCheckbox(name, "", ps.ID == sel) + menu.tailnetExitItems[ps.ID] = sm if !ps.Online { sm.Disable() } - if status.ExitNodeStatus != nil && ps.ID == status.ExitNodeStatus.ID { - sm.Check() - } setExitNodeOnClick(sm, ps.ID) } } @@ -666,46 +943,33 @@ func (menu *Menu) rebuildExitNodeMenu(ctx context.Context) { if len(mullvadExitNodes.countries) > 0 { menu.exitNodes.AddSeparator() menu.exitNodes.AddSubMenuItem("Location-based Exit Nodes", "").Disable() - mullvadMenu := menu.exitNodes.AddSubMenuItemCheckbox("Mullvad VPN", "", false) + // Use a plain submenu (no checkbox) for the parent. Some hosts + // treat a checkbox-with-children as a selectable leaf and dismiss + // the menu instead of expanding it. + mullvadMenu := menu.exitNodes.AddSubMenuItem("Mullvad VPN", "") for _, country := range mullvadExitNodes.sortedCountries() { - flag := countryFlag(country.code) - countryMenu := mullvadMenu.AddSubMenuItemCheckbox(flag+" "+country.name, "", false) + title := countryFlag(country.code) + " " + country.name - // single-city country, no submenu if len(country.cities) == 1 || hideMullvadCities { + countryMenu := mullvadMenu.AddSubMenuItemCheckbox(title, "", country.hasPeer(sel)) + menu.mullvadCountryItems[country.code] = countryMenu setExitNodeOnClick(countryMenu, country.best.ID) - if status.ExitNodeStatus != nil { - for _, city := range country.cities { - for _, ps := range city.peers { - if status.ExitNodeStatus.ID == ps.ID { - mullvadMenu.Check() - countryMenu.Check() - } - } - } - } continue } - // multi-city country, build submenu with "best available" option and cities. + // Multi-city country: plain submenu parent so the host expands + // it on click. Cities remain checkboxes. + countryMenu := mullvadMenu.AddSubMenuItem(title, "") time.Sleep(newMenuDelay) bm := countryMenu.AddSubMenuItemCheckbox("Best Available", "", false) setExitNodeOnClick(bm, country.best.ID) countryMenu.AddSeparator() for _, city := range country.sortedCities() { - cityMenu := countryMenu.AddSubMenuItemCheckbox(city.name, "", false) + cityMenu := countryMenu.AddSubMenuItemCheckbox(city.name, "", city.hasPeer(sel)) + menu.mullvadCityItems[country.code+"/"+city.best.Location.CityCode] = cityMenu setExitNodeOnClick(cityMenu, city.best.ID) - if status.ExitNodeStatus != nil { - for _, ps := range city.peers { - if status.ExitNodeStatus.ID == ps.ID { - mullvadMenu.Check() - countryMenu.Check() - cityMenu.Check() - } - } - } } } } @@ -743,6 +1007,16 @@ func (mc *mvCountry) sortedCities() []*mvCity { return cities } +// hasPeer reports whether any city in mc contains a peer with the given ID. +func (mc *mvCountry) hasPeer(id tailcfg.StableNodeID) bool { + for _, city := range mc.cities { + if city.hasPeer(id) { + return true + } + } + return false +} + // countryFlag takes a 2-character ASCII string and returns the corresponding emoji flag. // It returns the empty string on error. func countryFlag(code string) string { @@ -767,6 +1041,16 @@ type mvCity struct { peers []*ipnstate.PeerStatus } +// hasPeer reports whether mc contains a peer with the given ID. +func (mc *mvCity) hasPeer(id tailcfg.StableNodeID) bool { + for _, ps := range mc.peers { + if ps.ID == id { + return true + } + } + return false +} + func newMullvadPeers(status *ipnstate.Status) mullvadPeers { countries := make(map[string]*mvCountry) for _, ps := range status.Peer { diff --git a/client/systray/systray_test.go b/client/systray/systray_test.go new file mode 100644 index 000000000..16662caaf --- /dev/null +++ b/client/systray/systray_test.go @@ -0,0 +1,418 @@ +// Copyright (c) Tailscale Inc & contributors +// SPDX-License-Identifier: BSD-3-Clause + +//go:build cgo || !darwin + +package systray + +import ( + "slices" + "testing" + + "fyne.io/systray" + "tailscale.com/ipn" + "tailscale.com/ipn/ipnstate" + "tailscale.com/tailcfg" + "tailscale.com/types/key" +) + +// mkExitPeer creates a Mullvad-shaped exit-node peer at the given location. +func mkExitPeer(id, country, countryCode, city, cityCode string, priority int) *ipnstate.PeerStatus { + return &ipnstate.PeerStatus{ + ID: tailcfg.StableNodeID(id), + ExitNodeOption: true, + Location: &tailcfg.Location{ + Country: country, + CountryCode: countryCode, + City: city, + CityCode: cityCode, + Priority: priority, + }, + } +} + +// statusWith returns a Status whose Peer map contains the given peers, each +// keyed by a freshly generated NodePublic so the map can hold them all. +func statusWith(peers ...*ipnstate.PeerStatus) *ipnstate.Status { + st := &ipnstate.Status{Peer: map[key.NodePublic]*ipnstate.PeerStatus{}} + for _, p := range peers { + st.Peer[key.NewNode().Public()] = p + } + return st +} + +func TestNewMullvadPeers(t *testing.T) { + st := statusWith( + mkExitPeer("us-nyc-1", "United States", "US", "New York", "NYC", 50), + mkExitPeer("us-la-1", "United States", "US", "Los Angeles", "LAX", 40), + mkExitPeer("us-la-2", "United States", "US", "Los Angeles", "LAX", 90), + mkExitPeer("jp-tyo-1", "Japan", "JP", "Tokyo", "TYO", 60), + // A non-Mullvad exit-node-eligible peer (Location nil) — must be + // excluded from mullvad grouping. + &ipnstate.PeerStatus{ID: "tailnet-exit", ExitNodeOption: true}, + // A regular non-exit peer — also excluded. + &ipnstate.PeerStatus{ID: "regular"}, + ) + + got := newMullvadPeers(st) + + if len(got.countries) != 2 { + t.Fatalf("got %d countries, want 2 (US, JP)", len(got.countries)) + } + + us, ok := got.countries["US"] + if !ok { + t.Fatal("missing US country") + } + if us.best == nil || us.best.ID != "us-la-2" { + t.Errorf("US best = %v, want us-la-2 (highest priority)", us.best) + } + if la := us.cities["LAX"]; la == nil || la.best.ID != "us-la-2" || len(la.peers) != 2 { + t.Errorf("LAX = %v, want best=us-la-2 with 2 peers", la) + } + if jp := got.countries["JP"]; jp == nil || jp.best.ID != "jp-tyo-1" { + t.Errorf("JP best = %v, want jp-tyo-1", jp) + } +} + +func TestSortedCountriesAndCities(t *testing.T) { + st := statusWith( + mkExitPeer("uk", "United Kingdom", "GB", "London", "LON", 1), + mkExitPeer("us-1", "United States", "US", "Boston", "BOS", 1), + mkExitPeer("us-2", "United States", "US", "Atlanta", "ATL", 1), + mkExitPeer("at", "Austria", "AT", "Vienna", "VIE", 1), + ) + mp := newMullvadPeers(st) + + gotCountries := []string{} + for _, c := range mp.sortedCountries() { + gotCountries = append(gotCountries, c.name) + } + wantCountries := []string{"Austria", "United Kingdom", "United States"} + if !slices.Equal(gotCountries, wantCountries) { + t.Errorf("sortedCountries = %v, want %v", gotCountries, wantCountries) + } + + gotCities := []string{} + for _, c := range mp.countries["US"].sortedCities() { + gotCities = append(gotCities, c.name) + } + wantCities := []string{"Atlanta", "Boston"} + if !slices.Equal(gotCities, wantCities) { + t.Errorf("US sortedCities = %v, want %v", gotCities, wantCities) + } +} + +func TestActiveExitNodeID(t *testing.T) { + const id tailcfg.StableNodeID = "exit-node-1" + + tests := []struct { + name string + prefs *ipn.Prefs + status *ipnstate.Status + want tailcfg.StableNodeID + }{ + { + name: "neither set", + want: "", + }, + { + name: "prefs set, peer in netmap", + prefs: &ipn.Prefs{ExitNodeID: id}, + status: &ipnstate.Status{ + ExitNodeStatus: &ipnstate.ExitNodeStatus{ID: id, Online: true}, + }, + want: id, + }, + { + name: "prefs set, peer missing from netmap", + prefs: &ipn.Prefs{ExitNodeID: id}, + status: &ipnstate.Status{}, + want: id, + }, + { + name: "prefs nil, status fallback", + status: &ipnstate.Status{ExitNodeStatus: &ipnstate.ExitNodeStatus{ID: id}}, + want: id, + }, + { + name: "prefs zero, status fallback", + prefs: &ipn.Prefs{}, + status: &ipnstate.Status{ExitNodeStatus: &ipnstate.ExitNodeStatus{ID: id}}, + want: id, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := activeExitNodeID(tt.prefs, tt.status); got != tt.want { + t.Errorf("activeExitNodeID = %q, want %q", got, tt.want) + } + }) + } +} + +func TestAppearance(t *testing.T) { + const id tailcfg.StableNodeID = "exit-node-1" + tailnet := &ipnstate.TailnetStatus{Name: "tailnet"} + + tests := []struct { + name string + prefs *ipn.Prefs + status *ipnstate.Status + wantIcon *tsLogo + wantTooltip string + }{ + { + name: "nil status", + wantIcon: &disconnected, + wantTooltip: "Disconnected", + }, + { + name: "starting", + status: &ipnstate.Status{BackendState: ipn.Starting.String()}, + wantIcon: &loading, + wantTooltip: "Connecting", + }, + { + name: "running, no exit node", + status: &ipnstate.Status{ + BackendState: ipn.Running.String(), + CurrentTailnet: tailnet, + }, + wantIcon: &connected, + wantTooltip: "Connected to tailnet", + }, + { + name: "running, exit node online", + prefs: &ipn.Prefs{ExitNodeID: id}, + status: &ipnstate.Status{ + BackendState: ipn.Running.String(), + CurrentTailnet: tailnet, + ExitNodeStatus: &ipnstate.ExitNodeStatus{ID: id, Online: true}, + }, + wantIcon: &exitNodeOnline, + wantTooltip: "Using exit node", + }, + { + name: "running, exit node in netmap but offline", + prefs: &ipn.Prefs{ExitNodeID: id}, + status: &ipnstate.Status{ + BackendState: ipn.Running.String(), + CurrentTailnet: tailnet, + ExitNodeStatus: &ipnstate.ExitNodeStatus{ID: id, Online: false}, + }, + wantIcon: &exitNodeOffline, + wantTooltip: "Exit node offline", + }, + { + name: "running, exit node configured but missing from netmap", + prefs: &ipn.Prefs{ExitNodeID: id}, + status: &ipnstate.Status{ + BackendState: ipn.Running.String(), + CurrentTailnet: tailnet, + }, + wantIcon: &exitNodeOffline, + wantTooltip: "Exit node offline", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotIcon, gotTooltip := appearance(tt.status, tt.prefs) + if gotIcon != tt.wantIcon { + t.Errorf("icon = %p, want %p", gotIcon, tt.wantIcon) + } + if gotTooltip != tt.wantTooltip { + t.Errorf("tooltip = %q, want %q", gotTooltip, tt.wantTooltip) + } + }) + } +} + +// TestShortCircuitCaches verifies the cache field is the only signal of +// whether the underlying fyne mutator runs: fyne exposes no title getter +// and refresh() is a no-op without a dbus connection, so the cache value +// is the only externally observable contract. +func TestShortCircuitCaches(t *testing.T) { + item := systray.AddMenuItem("probe", "") + + t.Run("setTitleIfChanged", func(t *testing.T) { + var cache string + setTitleIfChanged(&cache, item, "alpha") + if cache != "alpha" { + t.Fatalf("after first set, cache = %q, want alpha", cache) + } + + setTitleIfChanged(&cache, item, "alpha") + if cache != "alpha" { + t.Fatalf("no-op mutated cache: %q", cache) + } + + setTitleIfChanged(&cache, item, "beta") + if cache != "beta" { + t.Fatalf("transition cache = %q, want beta", cache) + } + + setTitleIfChanged(&cache, nil, "gamma") + if cache != "beta" { + t.Fatalf("nil item mutated cache: %q", cache) + } + }) + + t.Run("setVisibleIfChanged", func(t *testing.T) { + var cache bool + setVisibleIfChanged(&cache, item, true) + if !cache { + t.Fatal("after Show, cache should be true") + } + + setVisibleIfChanged(&cache, item, true) + if !cache { + t.Fatal("no-op flipped cache") + } + + setVisibleIfChanged(&cache, item, false) + if cache { + t.Fatal("after Hide, cache should be false") + } + + setVisibleIfChanged(&cache, nil, true) + if cache { + t.Fatal("nil item mutated cache") + } + }) + + t.Run("Menu.setTooltipIfChanged", func(t *testing.T) { + m := &Menu{} + m.setTooltipIfChanged("hello") + if m.lastTooltip != "hello" { + t.Fatalf("lastTooltip = %q, want hello", m.lastTooltip) + } + + m.setTooltipIfChanged("hello") + if m.lastTooltip != "hello" { + t.Fatalf("no-op mutated lastTooltip: %q", m.lastTooltip) + } + + m.setTooltipIfChanged("world") + if m.lastTooltip != "world" { + t.Fatalf("lastTooltip = %q, want world", m.lastTooltip) + } + }) + + t.Run("Menu.setAppIconIfChanged", func(t *testing.T) { + m := &Menu{} + m.setAppIconIfChanged(&disconnected) + if m.lastIcon != &disconnected { + t.Fatal("lastIcon should be &disconnected") + } + + m.setAppIconIfChanged(&disconnected) + if m.lastIcon != &disconnected { + t.Fatal("no-op mutated lastIcon") + } + + m.setAppIconIfChanged(&connected) + if m.lastIcon != &connected { + t.Fatal("lastIcon should be &connected after transition") + } + }) +} + +// statusWithSelf returns a Status whose Self is non-nil, with the mullvad +// capability either set or unset. +func statusWithSelf(mullvad bool) *ipnstate.Status { + st := &ipnstate.Status{ + Peer: map[key.NodePublic]*ipnstate.PeerStatus{}, + Self: &ipnstate.PeerStatus{}, + } + if mullvad { + st.Self.CapMap = tailcfg.NodeCapMap{"mullvad": nil} + } + return st +} + +func TestComputeShape(t *testing.T) { + pa := ipn.LoginProfile{ID: "A"} + pb := ipn.LoginProfile{ID: "B"} + pc := ipn.LoginProfile{ID: "C"} + + t.Run("readonly differs", func(t *testing.T) { + s1 := computeShape(nil, ipn.LoginProfile{}, nil, false) + s2 := computeShape(nil, ipn.LoginProfile{}, nil, true) + + if s1 == s2 { + t.Error("readonly change did not produce different shape") + } + }) + + t.Run("profile add changes shape", func(t *testing.T) { + s1 := computeShape(nil, pa, []ipn.LoginProfile{pa, pb}, false) + s2 := computeShape(nil, pa, []ipn.LoginProfile{pa, pb, pc}, false) + + if s1 == s2 { + t.Error("profile add did not change shape") + } + }) + + t.Run("profile order does not change shape", func(t *testing.T) { + s1 := computeShape(nil, pa, []ipn.LoginProfile{pa, pb, pc}, false) + s2 := computeShape(nil, pa, []ipn.LoginProfile{pc, pa, pb}, false) + + if s1 != s2 { + t.Errorf("profile reordering changed shape: %+v vs %+v", s1, s2) + } + }) + + t.Run("current profile change moves shape", func(t *testing.T) { + s1 := computeShape(nil, pa, []ipn.LoginProfile{pa, pb}, false) + s2 := computeShape(nil, pb, []ipn.LoginProfile{pa, pb}, false) + + if s1 == s2 { + t.Error("current profile change did not move shape") + } + }) + + t.Run("tailnet exit node add changes shape", func(t *testing.T) { + st1 := statusWith(&ipnstate.PeerStatus{ID: "p1", ExitNodeOption: true}) + st2 := statusWith( + &ipnstate.PeerStatus{ID: "p1", ExitNodeOption: true}, + &ipnstate.PeerStatus{ID: "p2", ExitNodeOption: true}, + ) + s1 := computeShape(st1, ipn.LoginProfile{}, nil, false) + s2 := computeShape(st2, ipn.LoginProfile{}, nil, false) + + if s1 == s2 { + t.Error("tailnet exit node add did not change shape") + } + }) + + t.Run("non-exit peer does not change shape", func(t *testing.T) { + st1 := statusWithSelf(false) + st2 := statusWithSelf(false) + st2.Peer[key.NewNode().Public()] = &ipnstate.PeerStatus{ID: "p1"} + s1 := computeShape(st1, ipn.LoginProfile{}, nil, false) + s2 := computeShape(st2, ipn.LoginProfile{}, nil, false) + + if s1 != s2 { + t.Errorf("non-exit peer changed shape: %+v vs %+v", s1, s2) + } + }) + + t.Run("mullvad gated by capability", func(t *testing.T) { + mvPeer := mkExitPeer("mv-1", "United States", "US", "New York", "NYC", 1) + without := statusWithSelf(false) + without.Peer[key.NewNode().Public()] = mvPeer + + if s := computeShape(without, ipn.LoginProfile{}, nil, false); s.mullvadEnabled { + t.Error("mullvad enabled without capability") + } + + with := statusWithSelf(true) + with.Peer[key.NewNode().Public()] = mvPeer + s := computeShape(with, ipn.LoginProfile{}, nil, false) + + if !s.mullvadEnabled || s.mullvadKey == "" { + t.Errorf("mullvad not enabled with capability: %+v", s) + } + }) +}