From 732aac9d9c52f038ab6ed26b3baf880c5735e562 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Thu, 17 Sep 2026 18:25:48 +0700 Subject: [PATCH 1/4] fix(connection-form): pass each pane view model's changes on to the form's observers --- .../ConnectionFormCoordinator.swift | 34 ++++++++ .../ConnectionFormSplitViewController.swift | 8 +- .../ConnectionFormChildObservationTests.swift | 80 +++++++++++++++++++ 3 files changed, 116 insertions(+), 6 deletions(-) create mode 100644 TableProTests/ViewModels/ConnectionFormChildObservationTests.swift diff --git a/TablePro/Views/ConnectionForm/ConnectionFormCoordinator.swift b/TablePro/Views/ConnectionForm/ConnectionFormCoordinator.swift index 561a84ff9..89cca05a3 100644 --- a/TablePro/Views/ConnectionForm/ConnectionFormCoordinator.swift +++ b/TablePro/Views/ConnectionForm/ConnectionFormCoordinator.swift @@ -61,6 +61,8 @@ final class ConnectionFormCoordinator: ObservableObject { @Published private var temporaryTestIds: Set = [] + private var childChangeForwarding: AnyCancellable? + let services: AppServices var storage: ConnectionStorage { services.connectionStorage } @Published var dismissAction: (() -> Void)? @@ -137,6 +139,38 @@ final class ConnectionFormCoordinator: ObservableObject { customization.coordinator = ref advanced.coordinator = ref aiRules.coordinator = ref + + childChangeForwarding = forwardChildChanges() + } + + /// Every pane observes this coordinator and reads its values through a child, as in + /// `coordinator.network.type`. `@Published` on a child only fires when the reference is + /// replaced, never when a value inside it changes, so without this a pane stayed as it was + /// drawn: picking Sentinel left the Redis mode picker on Standalone with the host field still + /// showing. The send is synchronous because SwiftUI needs `objectWillChange` before the value + /// lands, and `switchToLatest` moves the subscription to a replacement child. + private func forwardChildChanges() -> AnyCancellable { + Publishers.MergeMany([ + Self.changes(of: $network), + Self.changes(of: $auth), + Self.changes(of: $ssh), + Self.changes(of: $remoteFile), + Self.changes(of: $cloudflareTunnel), + Self.changes(of: $cloudSQLProxy), + Self.changes(of: $socksProxy), + Self.changes(of: $tunnelCommand), + Self.changes(of: $ssl), + Self.changes(of: $customization), + Self.changes(of: $advanced), + Self.changes(of: $aiRules), + ]) + .sink { [weak self] in self?.objectWillChange.send() } + } + + private static func changes( + of child: Published.Publisher + ) -> AnyPublisher where Child.ObjectWillChangePublisher == ObservableObjectPublisher { + child.map(\.objectWillChange).switchToLatest().eraseToAnyPublisher() } /// Performs the one-time side-effecting setup: applying initial type diff --git a/TablePro/Views/ConnectionForm/ConnectionFormSplitViewController.swift b/TablePro/Views/ConnectionForm/ConnectionFormSplitViewController.swift index 336d40331..8e472554f 100644 --- a/TablePro/Views/ConnectionForm/ConnectionFormSplitViewController.swift +++ b/TablePro/Views/ConnectionForm/ConnectionFormSplitViewController.swift @@ -63,8 +63,8 @@ internal final class ConnectionFormSplitViewController: NSSplitViewController { /// The window title follows the connection's type, which the Change… button can now alter. /// /// `NSWindow(contentViewController:)` binds the window's title to this controller's, so nothing - /// writes `window.title` directly. `withObservationTracking` fires once per change, so the - /// closure re-arms itself. + /// writes `window.title` directly. The coordinator passes on its children's changes, so its own + /// publisher covers a type change in `network`. private func trackTitle() { title = windowTitle titleObservations = [ @@ -72,10 +72,6 @@ internal final class ConnectionFormSplitViewController: NSSplitViewController { guard let self else { return } self.title = self.windowTitle }, - coordinator.network.onMainActorChange { [weak self] in - guard let self else { return } - self.title = self.windowTitle - }, ] } diff --git a/TableProTests/ViewModels/ConnectionFormChildObservationTests.swift b/TableProTests/ViewModels/ConnectionFormChildObservationTests.swift new file mode 100644 index 000000000..de3d22ea8 --- /dev/null +++ b/TableProTests/ViewModels/ConnectionFormChildObservationTests.swift @@ -0,0 +1,80 @@ +// +// ConnectionFormChildObservationTests.swift +// TableProTests +// +// Every connection form pane observes the coordinator and reads its values through a child view +// model. A change inside a child that never reached the coordinator's publisher left the pane as +// it was drawn, which is how picking Sentinel kept the Redis mode picker on Standalone. +// + +import Combine +import Foundation +import Testing + +@testable import TablePro + +@MainActor +@Suite("Connection form child observation") +struct ConnectionFormChildObservationTests { + private final class ChangeCounter { + private(set) var sends = 0 + private var subscription: AnyCancellable? + + init(_ coordinator: ConnectionFormCoordinator) { + subscription = coordinator.objectWillChange.sink { [weak self] in self?.sends += 1 } + } + } + + private func children(of coordinator: ConnectionFormCoordinator) -> [(String, ObservableObjectPublisher)] { + [ + ("network", coordinator.network.objectWillChange), + ("auth", coordinator.auth.objectWillChange), + ("ssh", coordinator.ssh.objectWillChange), + ("remoteFile", coordinator.remoteFile.objectWillChange), + ("cloudflareTunnel", coordinator.cloudflareTunnel.objectWillChange), + ("cloudSQLProxy", coordinator.cloudSQLProxy.objectWillChange), + ("socksProxy", coordinator.socksProxy.objectWillChange), + ("tunnelCommand", coordinator.tunnelCommand.objectWillChange), + ("ssl", coordinator.ssl.objectWillChange), + ("customization", coordinator.customization.objectWillChange), + ("advanced", coordinator.advanced.objectWillChange), + ("aiRules", coordinator.aiRules.objectWillChange), + ] + } + + @Test("a value changed inside a child reaches the coordinator's observers") + func childValueChangeReachesCoordinator() { + let coordinator = ConnectionFormCoordinator(connectionId: nil) + let counter = ChangeCounter(coordinator) + + coordinator.network.additionalFieldValues["redisMode"] = "sentinel" + + #expect(counter.sends == 1) + } + + @Test("every child forwards its changes") + func everyChildForwards() { + let coordinator = ConnectionFormCoordinator(connectionId: nil) + + for (name, publisher) in children(of: coordinator) { + let counter = ChangeCounter(coordinator) + publisher.send() + #expect(counter.sends == 1, "\(name) did not reach the coordinator") + } + } + + @Test("a replaced child forwards, and the one it replaced goes quiet") + func replacedChildForwards() { + let coordinator = ConnectionFormCoordinator(connectionId: nil) + let replaced = coordinator.network + let replacement = NetworkPaneViewModel() + coordinator.network = replacement + let counter = ChangeCounter(coordinator) + + replaced.type = .postgresql + #expect(counter.sends == 0) + + replacement.type = .postgresql + #expect(counter.sends == 1) + } +} From d88293267867ca8090b67efeb1dc07a582d0fa94 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Thu, 17 Sep 2026 18:25:48 +0700 Subject: [PATCH 2/4] fix(tabs): observe tab execution for the editor tab strip's busy indicator --- ...plitViewController+TabStripAccessory.swift | 2 +- TablePro/Views/Main/EditorTabStrip.swift | 13 ++-- .../Views/Main/TabExecutionObservation.swift | 34 ++++++++++ .../Main/EditorTabStripChromeTests.swift | 2 +- .../Main/TabExecutionObservationTests.swift | 66 +++++++++++++++++++ 5 files changed, 106 insertions(+), 11 deletions(-) create mode 100644 TablePro/Views/Main/TabExecutionObservation.swift create mode 100644 TableProTests/Views/Main/TabExecutionObservationTests.swift diff --git a/TablePro/Core/Services/Infrastructure/MainSplitViewController+TabStripAccessory.swift b/TablePro/Core/Services/Infrastructure/MainSplitViewController+TabStripAccessory.swift index 90aebf860..d570b255e 100644 --- a/TablePro/Core/Services/Infrastructure/MainSplitViewController+TabStripAccessory.swift +++ b/TablePro/Core/Services/Infrastructure/MainSplitViewController+TabStripAccessory.swift @@ -98,7 +98,7 @@ internal extension MainSplitViewController { containerTarget: workspace.connection.flatMap { PluginManager.shared.containerSwitchTarget(for: $0.type) }, - executionOwner: sessionState.coordinator, + execution: TabExecutionObservation(owner: sessionState.coordinator), onNewTab: { [weak workspace] in workspace?.sessionState?.coordinator.commandActions?.newTab() } diff --git a/TablePro/Views/Main/EditorTabStrip.swift b/TablePro/Views/Main/EditorTabStrip.swift index 1fec6b1fb..a0b85275f 100644 --- a/TablePro/Views/Main/EditorTabStrip.swift +++ b/TablePro/Views/Main/EditorTabStrip.swift @@ -39,16 +39,11 @@ internal struct EditorTabStrip: View { /// shares a title with. Resolved by the window, because a view has no business asking the /// plugin registry what kind of container a connection has. internal let containerTarget: ContainerSwitchTarget? - /// Which tabs are running something. Read from the coordinator rather than pushed in, because - /// `tabExecution` is a stored property of an `@Observable`, so a claim opening or settling - /// invalidates this strip the same way it invalidates the result pane. A tab that is not the + /// Which tabs are running something. Observed rather than pushed in, so a claim opening or + /// settling redraws this strip the same way it redraws the result pane. A tab that is not the /// selected one has no status bar on screen, and its progress used to show as the window-wide /// spinner in the centre of the toolbar. - /// - /// Weak for the reason `MainWindowToolbar.coordinator` is: the coordinator leaves - /// `activeCoordinators` only on deinit, so a strong reference held by a pane that outlives the - /// workspace would keep a torn-down connection voting in every aggregate that walks it. - internal weak var executionOwner: MainContentCoordinator? + @ObservedObject internal var execution: TabExecutionObservation internal let onNewTab: () -> Void /// Left unset by the app, which reads the two accessibility settings instead. A test sets it, /// because glass does not rasterise. @@ -182,7 +177,7 @@ internal struct EditorTabStrip: View { ), position: index + 1, count: tabs.count, - isBusy: executionOwner?.tabExecution.isBusy(tab.id) ?? false, + isBusy: execution.isBusy(tab.id), commands: interaction.commands ) .opacity(opacity(of: tab)) diff --git a/TablePro/Views/Main/TabExecutionObservation.swift b/TablePro/Views/Main/TabExecutionObservation.swift new file mode 100644 index 000000000..3b2d98564 --- /dev/null +++ b/TablePro/Views/Main/TabExecutionObservation.swift @@ -0,0 +1,34 @@ +// +// TabExecutionObservation.swift +// TablePro +// + +import Combine +import Foundation + +/// Which tabs are running something, for a view that must not keep the coordinator alive. +/// +/// The coordinator leaves `activeCoordinators` only on deinit, so a strong reference held by a pane +/// that outlives the workspace keeps a torn-down connection voting in every aggregate that walks it. +/// A weak property cannot be `@ObservedObject`, and a view that reads `tabExecution` through one +/// never hears a claim open or settle: a tab that is not the selected one kept its spinner, or never +/// showed one, until something unrelated redrew the strip. +/// +/// This relays `tabExecution` alone rather than the whole coordinator, which also publishes the +/// cursor on every keystroke. +@MainActor +internal final class TabExecutionObservation: ObservableObject { + private weak var owner: MainContentCoordinator? + private var subscription: AnyCancellable? + + internal init(owner: MainContentCoordinator?) { + self.owner = owner + subscription = owner?.$tabExecution + .dropFirst() + .sink { [weak self] _ in self?.objectWillChange.send() } + } + + internal func isBusy(_ tabId: UUID) -> Bool { + owner?.tabExecution.isBusy(tabId) ?? false + } +} diff --git a/TableProTests/Views/Main/EditorTabStripChromeTests.swift b/TableProTests/Views/Main/EditorTabStripChromeTests.swift index 4279c7c8b..0fa0dde19 100644 --- a/TableProTests/Views/Main/EditorTabStripChromeTests.swift +++ b/TableProTests/Views/Main/EditorTabStripChromeTests.swift @@ -99,7 +99,7 @@ struct EditorTabStripChromeTests { tabManager: manager, interaction: interaction, containerTarget: nil, - executionOwner: nil, + execution: TabExecutionObservation(owner: nil), onNewTab: {}, surfaceStyle: .solid ) diff --git a/TableProTests/Views/Main/TabExecutionObservationTests.swift b/TableProTests/Views/Main/TabExecutionObservationTests.swift new file mode 100644 index 000000000..f5a4ae085 --- /dev/null +++ b/TableProTests/Views/Main/TabExecutionObservationTests.swift @@ -0,0 +1,66 @@ +// +// TabExecutionObservationTests.swift +// TableProTests +// +// The editor tab strip holds the coordinator weakly and reads which tabs are busy through this +// relay. Read through the weak reference directly, a claim opening or settling redrew nothing, so +// a background tab's spinner followed whatever else happened to redraw the strip. +// + +import Combine +import Foundation +import Testing + +@testable import TablePro + +@MainActor +@Suite("Tab execution observation") +struct TabExecutionObservationTests { + private final class ChangeCounter { + private(set) var sends = 0 + private var subscription: AnyCancellable? + + init(_ observation: TabExecutionObservation) { + subscription = observation.objectWillChange.sink { [weak self] in self?.sends += 1 } + } + } + + private func makeCoordinator() -> MainContentCoordinator { + MainContentCoordinator( + connection: TestFixtures.makeConnection(), + tabManager: QueryTabManager(), + changeManager: DataChangeManager(), + toolbarState: ConnectionToolbarState() + ) + } + + @Test("a claim opening and settling each reach the observer") + func claimLifecycleReachesObserver() { + let coordinator = makeCoordinator() + let observation = TabExecutionObservation(owner: coordinator) + let counter = ChangeCounter(observation) + let tabId = UUID() + + let claim = coordinator.tabExecution.claim(tabId) + #expect(counter.sends == 1) + #expect(observation.isBusy(tabId)) + + _ = coordinator.tabExecution.settle(claim) + #expect(counter.sends == 2) + #expect(!observation.isBusy(tabId)) + } + + @Test("subscribing does not report a change") + func subscribingIsSilent() { + let coordinator = makeCoordinator() + let observation = TabExecutionObservation(owner: coordinator) + let counter = ChangeCounter(observation) + + #expect(counter.sends == 0) + } + + @Test("no owner means no tab is busy") + func withoutOwnerNothingIsBusy() { + #expect(!TabExecutionObservation(owner: nil).isBusy(UUID())) + } +} From a778031875f89b251e41ff0b60c8577ccce72636 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Thu, 17 Sep 2026 18:25:49 +0700 Subject: [PATCH 3/4] fix(coordinator): react to sidebar selection and inspector view mode on the objects that publish them --- TablePro/Extensions/View+OnValueChange.swift | 24 +++++ TablePro/Views/Main/MainContentView.swift | 4 +- .../Extensions/ObservedValueChangeTests.swift | 102 ++++++++++++++++++ 3 files changed, 128 insertions(+), 2 deletions(-) create mode 100644 TableProTests/Extensions/ObservedValueChangeTests.swift diff --git a/TablePro/Extensions/View+OnValueChange.swift b/TablePro/Extensions/View+OnValueChange.swift index 0af9fcad6..e39d257b8 100644 --- a/TablePro/Extensions/View+OnValueChange.swift +++ b/TablePro/Extensions/View+OnValueChange.swift @@ -14,6 +14,30 @@ internal extension View { ) -> some View { modifier(PairedValueChangeModifier(value: value, action: action)) } + + /// The same, for a value that belongs to an object this view does not observe. + /// + /// A body that reads `parent.child.value` never hears the child change: `@Published` on the + /// child publishes on the child alone, and observing the parent covers only the parent's own + /// properties. The modifier observes the object instead of the view, so the object's other + /// changes redraw nothing but the modifier. + func onValueChange( + of keyPath: KeyPath, + in object: Object, + _ action: @escaping (Value, Value) -> Void + ) -> some View { + modifier(ObservedValueChangeModifier(object: object, keyPath: keyPath, action: action)) + } +} + +private struct ObservedValueChangeModifier: ViewModifier { + @ObservedObject var object: Object + let keyPath: KeyPath + let action: (Value, Value) -> Void + + func body(content: Content) -> some View { + content.onValueChange(of: object[keyPath: keyPath], action) + } } private struct PairedValueChangeModifier: ViewModifier { diff --git a/TablePro/Views/Main/MainContentView.swift b/TablePro/Views/Main/MainContentView.swift index 3815a572f..36be9ba6a 100644 --- a/TablePro/Views/Main/MainContentView.swift +++ b/TablePro/Views/Main/MainContentView.swift @@ -340,7 +340,7 @@ struct MainContentView: View { /// fields rendering changes the row under it without moving anything `InspectorTrigger` /// watches. Rebuilding on the switch is enough: the two renderings are never on screen /// together, so the stale snapshot is only ever reached by switching to it. - .onChange(of: trailingPaneState.inspector.viewMode) { _ in + .onValueChange(of: \.viewMode, in: trailingPaneState.inspector) { _, _ in updateInspectorContext() } /// A value window detached from a field goes on writing while the JSON rendering is the @@ -415,7 +415,7 @@ struct MainContentView: View { handleConnectionStatusChange() } - .onValueChange(of: coordinator.windowSidebarState.selectedTables) { oldTables, newTables in + .onValueChange(of: \.selectedTables, in: coordinator.windowSidebarState) { oldTables, newTables in guard !coordinator.isTearingDown else { Self.lifecycleLogger.debug("[switch] windowSidebarState.selectedTables SKIPPED (tearingDown) windowId=\(windowId, privacy: .public)") return diff --git a/TableProTests/Extensions/ObservedValueChangeTests.swift b/TableProTests/Extensions/ObservedValueChangeTests.swift new file mode 100644 index 000000000..f5134bc42 --- /dev/null +++ b/TableProTests/Extensions/ObservedValueChangeTests.swift @@ -0,0 +1,102 @@ +// +// ObservedValueChangeTests.swift +// TableProTests +// +// A view that observes a parent and reacts to `parent.child.value` never hears the child change, +// because the child's `@Published` values publish on the child alone. The main window reacts to +// the sidebar selection and the inspector's view mode that way, each held by a child object. +// + +import AppKit +import Combine +import SwiftUI +import Testing + +@testable import TablePro + +@MainActor +@Suite("Observed value change") +struct ObservedValueChangeTests { + private final class Child: ObservableObject { + @Published var value = 0 + } + + private final class Parent: ObservableObject { + @Published var title = "" + let child = Child() + } + + private final class Changes { + var pairs: [[Int]] = [] + } + + private struct ObservedProbe: View { + @ObservedObject var parent: Parent + let changes: Changes + + var body: some View { + Color.clear.onValueChange(of: \.value, in: parent.child) { old, new in + changes.pairs.append([old, new]) + } + } + } + + private struct ReadThroughProbe: View { + @ObservedObject var parent: Parent + let changes: Changes + + var body: some View { + Color.clear.onValueChange(of: parent.child.value) { old, new in + changes.pairs.append([old, new]) + } + } + } + + private func host(_ view: some View) -> NSWindow { + let window = NSWindow( + contentRect: NSRect(x: 0, y: 0, width: 40, height: 40), + styleMask: [.titled], + backing: .buffered, + defer: false + ) + window.isReleasedWhenClosed = false + window.contentView = NSHostingView(rootView: view) + settle(window) + return window + } + + private func settle(_ window: NSWindow) { + for _ in 0 ..< 10 { + window.contentView?.layoutSubtreeIfNeeded() + RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.02)) + } + } + + @Test("a change inside the child reaches the action with both values") + func childChangeReachesAction() { + let parent = Parent() + let changes = Changes() + let window = host(ObservedProbe(parent: parent, changes: changes)) + defer { window.orderOut(nil) } + + parent.child.value = 1 + settle(window) + parent.child.value = 2 + settle(window) + + #expect(changes.pairs == [[0, 1], [1, 2]]) + } + + @Test("reading the value through the parent misses the change") + func readingThroughParentMissesTheChange() { + let parent = Parent() + let changes = Changes() + let window = host(ReadThroughProbe(parent: parent, changes: changes)) + defer { window.orderOut(nil) } + + parent.child.value = 1 + settle(window) + + #expect(changes.pairs.isEmpty) + } +} From d06037f05665a929c72169ed31ded36db37cd25d Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Thu, 17 Sep 2026 18:25:49 +0700 Subject: [PATCH 4/4] test(datagrid): check a jumped column by its header frame and confirm the wide query was typed --- TableProUITests/ColumnJumpUITests.swift | 28 +++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/TableProUITests/ColumnJumpUITests.swift b/TableProUITests/ColumnJumpUITests.swift index c9bddbd33..686355b05 100644 --- a/TableProUITests/ColumnJumpUITests.swift +++ b/TableProUITests/ColumnJumpUITests.swift @@ -18,7 +18,10 @@ final class ColumnJumpUITests: UITestCase { let grid = runWideQuery(in: app) let lastColumnHeader = grid.buttons["Column: col_60"] XCTAssertTrue(lastColumnHeader.waitToExist(timeout: 10), "The result must expose its last column's header") - XCTAssertFalse(lastColumnHeader.isHittable, "Sixty columns must push the last one past the viewport") + XCTAssertFalse( + isInView(lastColumnHeader, of: grid), + "Sixty columns must push the last one past the viewport" + ) app.typeKey("j", modifierFlags: [.command, .shift]) let panel = switcherPanel(in: app) @@ -36,7 +39,7 @@ final class ColumnJumpUITests: UITestCase { app.typeKey(.return, modifierFlags: []) XCTAssertTrue(searchField.waitForNonExistence(timeout: 5), "Return must close the panel") XCTAssertTrue( - waitForPredicate(timeout: 10) { lastColumnHeader.isHittable }, + waitForPredicate(timeout: 10) { isInView(lastColumnHeader, of: grid) }, "The jump must scroll the column into view" ) @@ -72,15 +75,28 @@ final class ColumnJumpUITests: UITestCase { // MARK: - Helpers + /// Whether the header's centre lies inside the grid's frame, which is what scrolling a column + /// into view changes. + /// + /// Not `isHittable`: a data grid header never reports it, even when published, enabled, correctly + /// placed and unobstructed, so a wait on it times out after a jump that did scroll. Measured after + /// the jump, `col_60` sat at x 1795 to 1880 inside a grid spanning 959 to 1881 and still read as + /// not hittable. + private func isInView(_ header: XCUIElement, of grid: XCUIElement) -> Bool { + guard header.exists, grid.exists else { return false } + let frame = header.frame + return grid.frame.contains(CGPoint(x: frame.midX, y: frame.midY)) + } + + /// Through `typeQuery`, because sixty columns is a long run of keystrokes to send straight after + /// a click: the first ones can land before the editor takes them, and a query that lost its + /// `SELECT` produces no header to find and no result for Command Shift J to act on. private func runWideQuery(in app: XCUIApplication) -> XCUIElement { app.typeKey("t", modifierFlags: .command) - let editor = editorTextView(in: app) - XCTAssertTrue(editor.waitToExist(timeout: 10)) - editor.click() let columns = (1...Self.columnCount) .map { String(format: "%d AS col_%02d", $0, $0) } .joined(separator: ", ") - app.typeText("SELECT \(columns) FROM Track LIMIT 3;") + typeQuery("SELECT \(columns) FROM Track LIMIT 3;", in: app) app.typeKey(.return, modifierFlags: .command) let grid = app.windows.firstMatch.tables.matching(identifier: "data-grid").firstMatch