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 <evan@tailscale.com>

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 {

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)
+			}
+		})
+	}
+}