From 62b52ea3f29faa52ab21c58ff54fdebe7178f00d Mon Sep 17 00:00:00 2001 From: Evan Lowry Date: Sun, 3 May 2026 17:01:51 +0000 Subject: [PATCH] client/systray: fix recommended exit node not showing as selected When an exit node was set before launching systray, the recommended row in exit nodes rendered as not selected even when the active exit node was at the same location. This looks to be two different things: - suggestExitNode takes its own suggestion into account, and not the users active exit node. When a mullvad city is reached via the picker rather than the recommended row, the suggester's pick and prefs.ExitNodeID end up as distinct peers in the same city, resulting in an ID-only equality check missing the match. - Toggle state was constructed and mutated via .Check(), which for newly created elements may be cached (such as when launching systray, with an already active node). Fixes #19626 Signed-off-by: Evan Lowry --- client/systray/systray.go | 36 +++++++--- client/systray/systray_test.go | 120 +++++++++++++++++++++++++++++++++ 2 files changed, 148 insertions(+), 8 deletions(-) create mode 100644 client/systray/systray_test.go diff --git a/client/systray/systray.go b/client/systray/systray.go index 65c1bec20..d0287e647 100644 --- a/client/systray/systray.go +++ b/client/systray/systray.go @@ -621,11 +621,9 @@ func (menu *Menu) rebuildExitNodeMenu(ctx context.Context) { title += strings.Split(sugg.Name, ".")[0] } menu.exitNodes.AddSeparator() - rm := menu.exitNodes.AddSubMenuItemCheckbox(title, "", false) + active := recommendedIsActive(status, sugg.ID, sugg.Location.CountryCode(), sugg.Location.City()) + rm := menu.exitNodes.AddSubMenuItemCheckbox(title, "", active) setExitNodeOnClick(rm, sugg.ID) - if status.ExitNodeStatus != nil && sugg.ID == status.ExitNodeStatus.ID { - rm.Check() - } } } @@ -647,13 +645,11 @@ func (menu *Menu) rebuildExitNodeMenu(ctx context.Context) { if !ps.Online { name += " (offline)" } - sm := menu.exitNodes.AddSubMenuItemCheckbox(name, "", false) + active := status.ExitNodeStatus != nil && ps.ID == status.ExitNodeStatus.ID + sm := menu.exitNodes.AddSubMenuItemCheckbox(name, "", active) if !ps.Online { sm.Disable() } - if status.ExitNodeStatus != nil && ps.ID == status.ExitNodeStatus.ID { - sm.Check() - } setExitNodeOnClick(sm, ps.ID) } } @@ -743,6 +739,30 @@ func (mc *mvCountry) sortedCities() []*mvCity { return cities } +// recommendedIsActive reports whether the suggested exit node corresponds to +// the currently active exit node in status. +func recommendedIsActive(status *ipnstate.Status, suggID tailcfg.StableNodeID, suggCountry, suggCity string) bool { + if status == nil || status.ExitNodeStatus == nil || status.ExitNodeStatus.ID.IsZero() { + return false + } + if suggID == status.ExitNodeStatus.ID { + return true + } + if suggCountry == "" || suggCity == "" { + return false + } + for _, p := range status.Peer { + if p.ID != status.ExitNodeStatus.ID { + continue + } + if loc := p.Location; loc != nil && loc.CountryCode == suggCountry && loc.City == suggCity { + return true + } + return false + } + 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 { diff --git a/client/systray/systray_test.go b/client/systray/systray_test.go new file mode 100644 index 000000000..6b8ce8b95 --- /dev/null +++ b/client/systray/systray_test.go @@ -0,0 +1,120 @@ +// Copyright (c) Tailscale Inc & contributors +// SPDX-License-Identifier: BSD-3-Clause + +//go:build cgo || !darwin + +package systray + +import ( + "testing" + + "tailscale.com/ipn/ipnstate" + "tailscale.com/tailcfg" + "tailscale.com/types/key" +) + +func TestRecommendedIsActive(t *testing.T) { + t.Parallel() + + const ( + activeID = tailcfg.StableNodeID("active") + suggID = tailcfg.StableNodeID("suggestion") + ) + usNYC := &tailcfg.Location{CountryCode: "US", City: "New York"} + usCHI := &tailcfg.Location{CountryCode: "US", City: "Chicago"} + seSTO := &tailcfg.Location{CountryCode: "SE", City: "Stockholm"} + + statusWith := func(activePeer *ipnstate.PeerStatus) *ipnstate.Status { + s := &ipnstate.Status{ + ExitNodeStatus: &ipnstate.ExitNodeStatus{ID: activeID}, + } + if activePeer != nil { + s.Peer = map[key.NodePublic]*ipnstate.PeerStatus{{}: activePeer} + } + return s + } + + tests := []struct { + name string + status *ipnstate.Status + suggID tailcfg.StableNodeID + suggCountry string + suggCity string + isActive bool + }{ + { + name: "nil_status", + status: nil, + suggID: suggID, + }, + { + name: "no_exit_node", + status: &ipnstate.Status{}, + suggID: suggID, + }, + { + name: "exit_node_id_is_zero", + status: &ipnstate.Status{ExitNodeStatus: &ipnstate.ExitNodeStatus{}}, + suggID: suggID, + }, + { + name: "exact_id_match_short-circuits", + status: statusWith(&ipnstate.PeerStatus{ID: activeID, Location: usCHI}), + suggID: activeID, + suggCountry: "US", + suggCity: "New York", + isActive: true, + }, + { + name: "id_mismatch_but_same_city", + status: statusWith(&ipnstate.PeerStatus{ID: activeID, Location: usNYC}), + suggID: suggID, + suggCountry: "US", + suggCity: "New York", + isActive: true, + }, + { + name: "different_city", + status: statusWith(&ipnstate.PeerStatus{ID: activeID, Location: usCHI}), + suggID: suggID, + suggCountry: "US", + suggCity: "New York", + }, + { + name: "different_country", + status: statusWith(&ipnstate.PeerStatus{ID: activeID, Location: seSTO}), + suggID: suggID, + suggCountry: "US", + suggCity: "New York", + }, + { + name: "id_mismatch_suggestion_has_no_location", + status: statusWith(&ipnstate.PeerStatus{ID: activeID, Location: usNYC}), + suggID: suggID, + }, + { + name: "id_mismatch_active_peer_has_no_location", + status: statusWith(&ipnstate.PeerStatus{ID: activeID}), + suggID: suggID, + suggCountry: "US", + suggCity: "New York", + }, + { + name: "active_peer_not_in_status", + status: statusWith(nil), + suggID: suggID, + suggCountry: "US", + suggCity: "New York", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + isExitNodeActive := recommendedIsActive(tt.status, tt.suggID, tt.suggCountry, tt.suggCity) + if isExitNodeActive != tt.isActive { + t.Errorf("recommendedIsActive; got %v, want %v", isExitNodeActive, tt.isActive) + } + }) + } +}