Darling Viewer: the Recommendations and FinOps pickers keep their servers when the server list reloads

[erikdarlingdata]

What changes

In the Darling Viewer, the Recommendations and FinOps tabs each have a server picker. When the server list reloaded, both pickers moved to the sidebar's server, and the user's choice was lost. The list reloads after a local add, edit or remove, and after the server-list sync sees a server added or removed outside this window (#4928). Pinning or unpinning a favorite had the same effect.

Now each picker keeps its server while that server exists. A picker whose server was removed takes the sidebar's server, as on the initial load.

Cause

• LoadServersAsync (MainWindow.xaml.cs) set the Recommendations picker to the first server (SelectedIndex = 0).
• Then it restored the sidebar selection without the suppression guard. That ran ServerList_SelectionChanged, and its call to SyncAggregateServerSelectors moved both pickers to the sidebar's server.
• FinOpsTab.SetServers already kept its own server by id, but the sidebar restore then moved it.
• ReapplyFavoritesToServerList, the favorites re-sort, restored the sidebar in the same way. Pinning and unpinning both run it.
• LoadServersAsync read the sidebar's and the Recommendations picker's servers before it awaited the server list. So a change made while the list loaded was put back.

Fix

• ViewerServerSetSync.PickerSelectionAfterReload picks the server that a picker shows after a rebuild. It keeps the server that the picker showed, while that server exists. Otherwise it takes the sidebar's server, and otherwise the first server.
• The Recommendations picker and FinOpsTab.SetServers both use it. The FinOps tab also re-reads the server list when it opens, and that call now passes the sidebar's server too.
• RestoreSidebarSelection sets the sidebar's selection under _suppressSidebarSelection. Then it starts one load of the visible tab, as the selection handler did. The reload and the favorites re-sort both use it.
• LoadServersAsync reads both servers after it awaits the server list, so a change made while the list loads is kept. FinOpsTab.SetServers already read its server after its own await.
• The initial load keeps no picker's server, so both pickers start on the sidebar's server, as before.
• A click on a server in the sidebar still moves both pickers to that server.

Every list that the reload rebuilds

• The sidebar (ServerList, single-select): it already restored its server by id. It now restores under the guard.
• The Recommendations picker (single-select): fixed.
• The FinOps picker (single-select): fixed, at both of its rebuilds.
• The Alert History and Job History server filters (single-select): each tab rebuilds its filter from its own data, not from this reload. Both already restore the selected server by id (AlertsHistoryTab.xaml.cs, JobHistoryTab.xaml.cs). No change.
• Multi-select: the Alert History and Job History grids select rows, not servers. The reload rebuilds no multi-select server list.
• The collector schedule editor and the database state editor get their server list when their window opens. The reload does not rebuild them.

Lite parity

Checked: Lite keeps the selection. Lite/MainWindow.xaml.cs RefreshServerList calls RefreshServerList on the FinOps and Recommendations tabs, and each one restores the selected server by id. The Lite sidebar never moves those pickers (Lite/MainWindow.SidebarTree.cs). No Lite change.

Tests

How the tests ran RED: commit 3cce5dd adds the tests and the seam. There, the seam returns what a rebuild did before this fix: the sidebar's server. At that commit, 4 of the 7 new tests in ViewerServerSetSyncTests failed:

• Reload_RebuildsTheRecommendationsPicker_KeepingItsServerById: Assert.DoesNotMatch() Failure: Match found
• Reload_AndTheFavoritesResort_RestoreTheSidebar_WithoutMovingThePickers: Assert.Matches() Failure: Pattern not found in value
• FinOpsPicker_KeepsItsServer_AndFallsBackToTheSidebarsServer_AtBothOfItsRebuilds: Assert.Matches() Failure: Pattern not found in value
• Picker_KeepsItsServer_WhileItExists_WhereverTheSidebarIs: Assert.Equal() Failure: Values differ

What this RED proves: the three source tests fail on dev's own shell source, which commit 3cce5dd leaves unchanged. The fourth test fails against the seam's stub, so it proves only the seam's contract. The UIA check below proves the behavior. Commit 23ea3da is the fix.

Round 2 adds two source tests:

• Reload_ReadsTheServersItKeeps_AfterItsAwait: commit 9ee6d1e adds it. It fails on the shell source of 23ea3da with "LoadServersAsync must read the sidebar's server after its await and before it rebinds the sidebar". Commit 7dd584a is the fix.
• EverySidebarSelectionWrite_RunsUnderTheSuppressionGuard: it scans every viewer source file. Each write to ServerList.SelectedItem, SelectedIndex or SelectedValue must come after _suppressSidebarSelection = true, with no reset between them. It passes on this branch. With one unguarded ServerList.SelectedItem = ... added to the server search handler, it fails with "a sidebar selection write outside the guard: MainWindow.xaml.cs:998".

Guards, which pass at both commits: Picker_WhoseServerWasRemoved_FollowsTheSidebar_AsOnAFreshLoad, FreshLoad_StartsThePicker_OnTheSidebarsServer and NothingToKeepOrFollow_TakesTheFirstServer_AndAnEmptyListSelectsNothing. The existing ViewerFinOpsPickerRefreshPinTests pass with no change.

UIA check, with the Viewer built from this branch, on a local store with three SQL Server targets:

1. The sidebar was on server A. Both pickers were set to server B.
2. Server C was removed outside the Viewer. After the sync reload ("Servers: 2"), both pickers still showed B. On dev, the same steps left both pickers on A.
3. Server C was added back with the CLI (--add-server). After the reload ("Servers: 3"), both pickers still showed B.
4. Server B was removed outside the Viewer. Both pickers took the sidebar's server, A.

Only the source test covers the favorites path, because a favorite toggle writes the user's viewer settings file.

Full suite at 7dd584a: Darling.Tests 19879 total, 0 failed, 1320 skipped (they need a live PostgreSQL store), 1 not run. This PR changes no Lite or shared code.

CHANGELOG

No entry in this PR; the release owner adds it under [Unreleased].

Proposed entry:

In the Darling Viewer, the Recommendations and FinOps pickers keep their servers when the server list reloads or a favorite changes.

[claude]

Verdict: LGTM

Claims

• VERIFIED — pre-fix LoadServersAsync set the Recommendations picker with RecommendationsServerSelector.SelectedIndex = 0 — confirmed in the diff's removed lines (old MainWindow.xaml.cs), replaced by PickerSelectionAfterReload.
• VERIFIED — the old sidebar restore ran unguarded and triggered ServerList_SelectionChanged → SyncAggregateServerSelectors, moving both pickers — confirmed by reading ServerList_SelectionChanged (MainWindow.xaml.cs:860-880) and SyncAggregateServerSelectors (MainWindow.xaml.cs:890-904), and the diff removing the unguarded ServerList.SelectedItem = _fleet.ResolveSelection(...) line.
• VERIFIED — FinOpsTab.SetServers already matched by id before this PR — confirmed in the diff's old body (servers.FirstOrDefault(s => s.ServerId == pid)).
• VERIFIED — ReapplyFavoritesToServerList restored the sidebar the same unguarded way — confirmed in MainWindow.ServerManagement.cs diff, now routed through RestoreSidebarSelection.
• VERIFIED — PickerSelectionAfterReload's keep → sidebar → first-server fallback chain — ViewerServerSetSync.cs:117-119, exercised by 4 real (non-source-pin) unit tests in ViewerServerSetSyncTests.cs.
• VERIFIED — Recommendations and FinOpsTab.SetServers both call the new seam — MainWindow.xaml.cs:1081, FinOpsTab.xaml.cs:85.
• VERIFIED — the FinOps tab's re-read on tab activation now also passes the sidebar's server — MainWindow.xaml.cs:1219-1221 (LoadVisibleTabAsync).
• VERIFIED — RestoreSidebarSelection sets the sidebar under _suppressSidebarSelection and starts one RefreshVisibleAsync — MainWindow.xaml.cs:945-961; both the reload and the favorites re-sort call it (MainWindow.xaml.cs:1100, MainWindow.ServerManagement.cs:86).
• VERIFIED — the initial load passes no previous picker server, so both pickers start on the sidebar's server — preserveSelection ? previousRecoServer : null and keepSelection: preserveSelection at MainWindow.xaml.cs:1082,1089.
• VERIFIED — a sidebar click still moves both pickers — ServerList_SelectionChanged is unchanged and still calls SyncAggregateServerSelectors for the non-suppressed path.
• VERIFIED — Lite parity: Lite's FinOps/Recommendations tabs already restore selection by id in their own RefreshServerList (Lite/Controls/FinOpsTab.xaml.cs:101-137, RecommendationsTab.xaml.cs's analogous method), and Lite's sidebar has no SyncAggregateServerSelectors-equivalent that drives those pickers — confirmed by grep across Lite/MainWindow.xaml.cs and the two controls. No Lite change needed.
• UNVERIFIED — the specific RED/GREEN test results at commits 3cce5dd88/23ea3da59, and the full-suite count (19877 total, 0 failed) — these commit SHAs aren't present in this shallow checkout and no test run can be executed from a read-only tree.
• UNVERIFIED — the manual UIA walkthrough (sidebar/picker behavior across add/remove/favorite on a live 3-server store) — requires a running Viewer and a live SQL Server store, not checkable here.

Riskiest lines

1. Darling/PerformanceMonitor.Darling.Viewer/ViewerServerSetSync.cs:117-119 — the keep→sidebar→first fallback chain both pickers now share — pinned by Picker_KeepsItsServer_WhileItExists_WhereverTheSidebarIs, Picker_WhoseServerWasRemoved_FollowsTheSidebar_AsOnAFreshLoad, FreshLoad_StartsThePicker_OnTheSidebarsServer, NothingToKeepOrFollow_TakesTheFirstServer_AndAnEmptyListSelectsNothing (real unit tests against the static method, not source pins).
2. Darling/PerformanceMonitor.Darling.Viewer/MainWindow.xaml.cs:945-961 (RestoreSidebarSelection) — if the _suppressSidebarSelection guard ever wrapped less than the SelectedItem assignment, every reload/favorites re-sort would silently reintroduce the exact bug this PR fixes (both pickers hijacked back to the sidebar server) — pinned by Reload_AndTheFavoritesResort_RestoreTheSidebar_WithoutMovingThePickers, but that test (like the whole Darling.Tests WPF surface, per ViewerFinOpsPickerRefreshPinTests.cs:17 — "WPF cannot run here") is a source-text pin, not an executed control; true runtime behavior rests on the PR's manual UIA check.
3. Darling/PerformanceMonitor.Darling.Viewer/FinOpsTab.xaml.cs:93 — the filter/drill-reset condition (now.ServerId != previousId) that must fire on a real switch and stay silent on a kept selection (#2306 regression surface) — pinned by FinOpsPicker_KeepsItsServer_AndFallsBackToTheSidebarsServer_AtBothOfItsRebuilds and ViewerFinOpsPickerRefreshPinTests, same source-text-pin caveat as above.

Traps near this diff

• "Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply. Lite's FinOpsTab.xaml.cs:101-137 and RecommendationsTab.xaml.cs already restore their own picker's server by id inside their own RefreshServerList, and Lite's sidebar (Lite/MainWindow.xaml.cs) has no code path analogous to SyncAggregateServerSelectors that reaches into those pickers — there is no Lite bug for this PR to fix in parallel.
• No SQL, collector/delta, analysis/scorer, alerting, storage/MCP-payload, or CI/workflow trap lines are near this diff (pure WPF selection-state plumbing in the Darling Viewer) — none apply.

Divergence (blind read vs PR body)

None — the body's account of the cause, the fix, and the "every list that reload rebuilds" survey all match what the diff and surrounding code show.

Reviewer checklist

• Darling.Tests has no WPF execution (confirmed by ViewerFinOpsPickerRefreshPinTests.cs:17's own comment); the real behavior change here is only verified by the manual UIA walkthrough in the PR body — worth a human re-running those same 4 steps (sidebar on A, both pickers on B, remove/re-add/remove-B outside the Viewer) against a live build before release.