From c38834d0db882b62f1f229ef7f18f12272f4fc68 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Tue, 6 Oct 2026 12:40:25 +0300 Subject: [PATCH] fix(macos): disambiguate reassign picker and make ServersView routing testable Delete-reassign picker uses pickerTitle against the full profile list. Extract ServersRouteReducer, drop the duplicate onReceive observers on serverListView (handlers fired twice), add reducer + once-only tests. ViewInspector rejected (no new dependencies): tests cover the routing logic, not the SwiftUI binding. Refs #1449, closes #1392 --- .../MCPProxy/Views/ProfileEditorModel.swift | 7 ++ .../MCPProxy/Views/ProfileSheets.swift | 6 +- .../MCPProxy/MCPProxy/Views/ServersView.swift | 77 +++++++++---------- .../ProfileEditorModelTests.swift | 7 ++ .../ServersViewRoutingTests.swift | 40 ++++++++++ 5 files changed, 95 insertions(+), 42 deletions(-) diff --git a/native/macos/MCPProxy/MCPProxy/Views/ProfileEditorModel.swift b/native/macos/MCPProxy/MCPProxy/Views/ProfileEditorModel.swift index f33c68130..14a3b577b 100644 --- a/native/macos/MCPProxy/MCPProxy/Views/ProfileEditorModel.swift +++ b/native/macos/MCPProxy/MCPProxy/Views/ProfileEditorModel.swift @@ -412,6 +412,13 @@ final class ProfileEditorModel: ObservableObject { all.filter { $0.name != name } } + /// Picker entries (slug, label) for the delete sheet's reassign target. + /// The clash check runs against the FULL list so a target sharing a title + /// with the profile being deleted is still disambiguated. + static func reassignPickerLabels(excluding name: String, in all: [ProfileView]) -> [(name: String, label: String)] { + reassignTargets(excluding: name, in: all).map { ($0.name, $0.pickerTitle(in: all)) } + } + /// `DELETE /profiles/{name}?reassign_to=&force=`. @discardableResult func delete(reassignTo: String?, force: Bool) async -> Bool { diff --git a/native/macos/MCPProxy/MCPProxy/Views/ProfileSheets.swift b/native/macos/MCPProxy/MCPProxy/Views/ProfileSheets.swift index b62af54e7..81caa9075 100644 --- a/native/macos/MCPProxy/MCPProxy/Views/ProfileSheets.swift +++ b/native/macos/MCPProxy/MCPProxy/Views/ProfileSheets.swift @@ -111,6 +111,10 @@ struct DeleteProfileSheet: View { ProfileEditorModel.reassignTargets(excluding: profile.name, in: appState.profiles) } + private var pickerLabels: [(name: String, label: String)] { + ProfileEditorModel.reassignPickerLabels(excluding: profile.name, in: appState.profiles) + } + var body: some View { let impact = impact VStack(alignment: .leading, spacing: 12) { @@ -124,7 +128,7 @@ struct DeleteProfileSheet: View { if impact.requiresTarget { Picker("Move them to", selection: $target) { Text("Choose a profile…").tag("") - ForEach(targets) { Text($0.displayTitle).tag($0.name) } + ForEach(pickerLabels, id: \.name) { Text($0.label).tag($0.name) } } .accessibilityIdentifier("profile-delete-target") if targets.isEmpty { diff --git a/native/macos/MCPProxy/MCPProxy/Views/ServersView.swift b/native/macos/MCPProxy/MCPProxy/Views/ServersView.swift index 2971d0f42..539cc2a65 100644 --- a/native/macos/MCPProxy/MCPProxy/Views/ServersView.swift +++ b/native/macos/MCPProxy/MCPProxy/Views/ServersView.swift @@ -79,34 +79,14 @@ struct ServersView: View { // which branch is currently shown, so a later notification can always // switch straight to a different server's detail. .onReceive(NotificationCenter.default.publisher(for: .showAddServer)) { notification in - if let tab = notification.object as? AddServerTab { - addServerInitialTab = tab - } else { - addServerInitialTab = .catalog - } + addServerInitialTab = ServersRouteReducer.addServerTab(for: notification.object) showAddServer = true } .onReceive(NotificationCenter.default.publisher(for: .showServerDetail)) { notification in - let serverName: String - let tab: ServerDetailTab - let focusField: TrayConfigFocusField? - if let target = notification.object as? ServerDetailTarget { - serverName = target.serverName - tab = target.tab - focusField = target.focusField - } else if let name = notification.object as? String { - serverName = name - tab = .tools - focusField = nil - } else { - return - } - // Find the server by name in the current list or appState - if let server = servers.first(where: { $0.name == serverName }) - ?? appState.servers.first(where: { $0.name == serverName }) { - selectedServerInitialTab = tab - selectedServerInitialFocusField = focusField - selectedServer = server + if let route = ServersRouteReducer.detailRoute(for: notification.object, servers: servers, fallback: appState.servers) { + selectedServerInitialTab = route.tab + selectedServerInitialFocusField = route.focusField + selectedServer = route.server } } } @@ -285,22 +265,6 @@ struct ServersView: View { triggerLoad() } .onChange(of: profileFilter) { _ in triggerLoad() } - .onReceive(NotificationCenter.default.publisher(for: .showAddServer)) { notification in - if let tab = notification.object as? AddServerTab { - addServerInitialTab = tab - } else { - addServerInitialTab = .catalog - } - showAddServer = true - } - .onReceive(NotificationCenter.default.publisher(for: .showServerDetail)) { notification in - guard let serverName = notification.object as? String else { return } - // Find the server by name in the current list or appState - if let server = servers.first(where: { $0.name == serverName }) - ?? appState.servers.first(where: { $0.name == serverName }) { - selectedServer = server - } - } } /// Open the Add Server sheet on the Catalog tab for a pending toolbar @@ -1215,3 +1179,34 @@ struct ServerTableView: NSViewRepresentable { } } } + +/// Pure routing logic for the `.showAddServer` / `.showServerDetail` +/// notifications, extracted so it is unit-testable without a SwiftUI harness. +enum ServersRouteReducer { + struct DetailRoute: Equatable { + let server: ServerStatus + let tab: ServerDetailTab + let focusField: TrayConfigFocusField? + } + + static func addServerTab(for payload: Any?) -> AddServerTab { + (payload as? AddServerTab) ?? .catalog + } + + /// nil when the payload is unrecognised or the server is not (yet) in either + /// list; the notification is then dropped, as before. + static func detailRoute(for payload: Any?, servers: [ServerStatus], fallback: [ServerStatus]) -> DetailRoute? { + let name: String + var tab: ServerDetailTab = .tools + var focus: TrayConfigFocusField? + if let target = payload as? ServerDetailTarget { + name = target.serverName; tab = target.tab; focus = target.focusField + } else if let n = payload as? String { + name = n + } else { + return nil + } + guard let server = servers.first(where: { $0.name == name }) ?? fallback.first(where: { $0.name == name }) else { return nil } + return DetailRoute(server: server, tab: tab, focusField: focus) + } +} diff --git a/native/macos/MCPProxy/MCPProxyTests/ProfileEditorModelTests.swift b/native/macos/MCPProxy/MCPProxyTests/ProfileEditorModelTests.swift index 960cfc4fa..0a69b8171 100644 --- a/native/macos/MCPProxy/MCPProxyTests/ProfileEditorModelTests.swift +++ b/native/macos/MCPProxy/MCPProxyTests/ProfileEditorModelTests.swift @@ -438,6 +438,13 @@ final class ProfileEditorModelTests: XCTestCase { XCTAssertFalse(targets.contains { $0.name.isEmpty }, "there is no All servers target") } + func testReassignPickerLabelsDisambiguateSharedTitlesAgainstTheFullList() { + let all = [ProfileView(name: "work", title: "Team"), ProfileView(name: "work2", title: "Team"), ProfileView(name: "solo", title: "Solo")] + let labels = ProfileEditorModel.reassignPickerLabels(excluding: "work", in: all) + XCTAssertEqual(labels.map(\.name), ["work2", "solo"]) + XCTAssertEqual(labels.map(\.label), ["Team (work2)", "Solo"], "clash with the profile being deleted must still be slugged") + } + func testARenameMovesTheModelToTheNewName() async { let source = StubSource() let renamed = try! JSONDecoder().decode(ProfileRenameResponse.self, from: Data( diff --git a/native/macos/MCPProxy/MCPProxyTests/ServersViewRoutingTests.swift b/native/macos/MCPProxy/MCPProxyTests/ServersViewRoutingTests.swift index e6f9176c3..df168d70d 100644 --- a/native/macos/MCPProxy/MCPProxyTests/ServersViewRoutingTests.swift +++ b/native/macos/MCPProxy/MCPProxyTests/ServersViewRoutingTests.swift @@ -100,6 +100,46 @@ final class ServersViewRoutingTests: XCTestCase { "or a prior .showServerDetail notification's tab (e.g. .config) leaks into the next manual open") } + /// The observers must be attached exactly once (in `body`): a second copy on + /// `serverListView` fires the handler twice whenever the list is mounted. + func testNotificationObserversAreAttachedExactlyOnce() throws { + let source = try serversViewSource() + for name in [".showServerDetail", ".showAddServer"] { + let needle = ".onReceive(NotificationCenter.default.publisher(for: \(name)))" + XCTAssertEqual(source.components(separatedBy: needle).count - 1, 1, "\(name) observer count") + } + } + + private func server(_ name: String) throws -> ServerStatus { + try JSONDecoder().decode(ServerStatus.self, from: Data(#"{"id":"\#(name)","name":"\#(name)","protocol":"http","enabled":true,"connected":true,"quarantined":false,"tool_count":1}"#.utf8)) + } + + func testReducerSelectsAnExistingServerFromAStringPayload() throws { + let a = try server("a") + let r = ServersRouteReducer.detailRoute(for: "a", servers: [a], fallback: []) + XCTAssertEqual(r?.server.name, "a") + XCTAssertEqual(r?.tab, .tools) + XCTAssertNil(r?.focusField) + } + + func testReducerCarriesTabAndFallsBackToAppStateList() throws { + let b = try server("b") + let r = ServersRouteReducer.detailRoute(for: ServerDetailTarget(serverName: "b", tab: .config), servers: [], fallback: [b]) + XCTAssertEqual(r?.server.name, "b") + XCTAssertEqual(r?.tab, .config) + } + + func testReducerDropsAnUnknownServerAndBadPayload() throws { + let a = try server("a") + XCTAssertNil(ServersRouteReducer.detailRoute(for: "zzz", servers: [a], fallback: [a])) + XCTAssertNil(ServersRouteReducer.detailRoute(for: 42, servers: [a], fallback: [a])) + } + + func testReducerAddServerTabDefaultsToCatalog() { + XCTAssertEqual(ServersRouteReducer.addServerTab(for: nil), .catalog) + XCTAssertEqual(ServersRouteReducer.addServerTab(for: AddServerTab.allCases.last!), AddServerTab.allCases.last!) + } + // MARK: - Helpers /// Isolates `body`'s own text — from `var body: some View {` up to the