Skip to content

refactor(connection-form): one transport picker and four tabs in place of eleven panes - #2645

Open
datlechin wants to merge 6 commits into
mainfrom
refactor/connection-form-ia
Open

refactor(connection-form): one transport picker and four tabs in place of eleven panes#2645
datlechin wants to merge 6 commits into
mainfrom
refactor/connection-form-ia

Conversation

@datlechin

Copy link
Copy Markdown
Member

Why

The connection form modelled one exclusive choice as five independent booleans, and modelled one object's facets as a navigation sidebar. Both are category errors, and together they produced a form with up to eleven destinations for a task whose common case is six fields.

The exclusivity was more than an IA problem. From TablePro/Models/Connection/ConnectionTunnelKind.swift:76:

var activeTunnelKind: ConnectionTunnelKind? {
    enabledTunnelKinds.count == 1 ? enabledTunnelKinds.first : nil
}

With two transports enabled, activeTunnelKind is nil, DatabaseManager.activeTunnelManager returns nil, and the connection opens directly to the database host with neither transport applied. isFormValid never checked for it, so Save stayed enabled. The form's only defence was an orange banner apologising for a state the model's own doc comment already calls impossible ("exactly one transport per connection"). For a database reachable only through a bastion that is a failed connect; for one reachable both ways it is an unintended direct connection that bypasses the proxy the user chose.

Five of PostgreSQL's ten sidebar rows were those mutually-exclusive transports, and each was a full-height pane containing one Toggle.

What changed

Transport is one value. ConnectionFormCoordinator.transport is a ConnectionTunnelKind?, the same type DatabaseConnection.activeTunnelKind returns, so the form now edits exactly what the connect path reads. Selecting one disables the rest, which makes the broken state unrepresentable. TunnelExclusivityBanner, ConnectionFormCoordinator+TunnelExclusivity.swift and the "Cannot use X and Y at the same time" rules in five view models are gone. normalizeTransport() collapses a connection stored by an older build onto its first transport on load, which is a repair rather than a change: the connect path was already ignoring both.

Eleven panes become four tabs: General, Network, Options, Appearance. The set is fixed except Network, which is absent only for a driver that connects directly and cannot negotiate TLS, so the form no longer changes shape between database types.

The commit actions moved to a bottom action bar, which is where macOS puts them for a window that edits one object, and that is where the validation message now lives. Every pane computed validationIssues: [String] and every one of those strings was thrown away by a warning triangle; the reason Save is dimmed now sits next to Save, prefixed with the tab that owns it when that is a different tab.

Smaller repairs in the same subsystem:

  • NetworkPaneViewModel.setType had no caller, so the database type was fixed once the form opened. Change… on the General tab now retypes the connection.
  • RemoteFilePaneView wrapped ConnectionSSHTunnelView in a second Form, so its grouped rows were inset twice. Both are sections of one form now.
  • usePgpass is a password-only controller and was being lifted above Username along with the auth-method dropdowns. splitCredentialControllers() now returns username controllers and password controllers separately, so each sits directly above what it controls.
  • Tab inserted a tab character in Startup Commands and Pre-Connect Script instead of moving focus. All three multi-line fields use one editor with movesFocusOnTab.
  • SSLPaneViewModel.validationIssues fired on drivers that render no SSL section, which could dim Save over a certificate field the form does not show.
  • GeneralPaneView was the only pane missing .scrollContentBackground(.hidden).

The window title, the accessibility identifiers the existing UI tests use, ConnectionFormEdits, and every buildTunnelMode() are unchanged, so persistence and ConnectionFormEditsCoverageTests are untouched.

A note on the tab control

TabView + .tabItem { Text } was the first implementation, following CompareApplySheetView. On macOS 26 the SwiftUI tab bar was redesigned and it renders as a collapsed ~30pt stub. Measured on macOS 27, screenshot below. The tab bar is an NSSegmentedControl through a .segmented Picker instead, which is what AppKit puts above the content of a single-object editor and is stable across versions. It publishes as a radio group, so VoiceOver and XCUITest both read it.

Before / After

Before: General. Ten sidebar destinations, a red triangle with no text anywhere explaining it, and a dimmed Save.
Before: SSH Tunnel. A whole 600pt pane whose entire content is one off toggle, in a pane already titled "SSH Tunnel". Four more panes were identical.
After: General. Four segments, the type visible and changeable, and "Connection name is required" beside the dimmed Save.
After: Network. The five panes replaced by one Connect via picker with a line saying what the choice does, SSL below it, and the cross-tab validation message reading "General: Connection name is required".
After: Network with SSH Tunnel. The same pane that used to hold one toggle, now holding the transport and its whole configuration.

Screenshots are attached in a comment. They are 1x captures on a 1920x1080 display, so they are half the pixel density of the docs images taken on a 2x display.

Verification

Step Result
verify.sh generate PASS
verify.sh build PASS
verify.sh test (8 suites) PASS, 67 cases
verify.sh uitest ConnectionFormTransportUITests PASS, 4 cases
verify.sh lint TablePro TableProTests TableProUITests 0 violations
docs/scripts/check-writing-style.sh PASS
docs/scripts/check-docs-against-source.py PASS

ConnectionFormTransportTests is new and covers the exclusivity round-trip, the legacy collapse, that switching away keeps the previous transport's fields, that a transport the type no longer offers falls back to direct, and that every issue blocking Save is claimed by a visible tab. ConnectionFormTunnelExclusivityTests is deleted: every case in it asserted the behaviour this PR removes.

ConnectionFormTransportUITests drives the real form for the picker swap and the validation message.

Review

Codex could not read this diff: the workspace is out of credits until Sep 7 (You've hit your usage limit, read from the job log on two attempts, since a --wait run exits 0 either way). Skill(code-review) reviewed it instead and raised 15 findings. Fourteen were real and are fixed in this branch:

  • validationIssues summed only visible tabs, so an issue on a hidden Network tab was dropped from isFormValid and Save was enabled over a half-configured transport that buildEdits() still persisted. normalizeTransport() now also runs after the URL-import path, which is what could leave a transport enabled that no tab offered.
  • The transport getter chose .ssh vs .remoteFile by database type rather than by what was written, so a type offering both would let the picker select one and read back the other. availableTransports now offers exactly one of the pair.
  • Retyping a MySQL port forward to SQLite silently reclassified it as a read-only file copy. A type change now returns the connection to Direct and keeps the fields, since which transports exist and what they mean both change with the type.
  • remoteFilePath and sshForwardUnixSocketPath could survive into a transport whose form renders no control for them, while still being written on save. The setter clears each when it stops being reachable.
  • changeType made retyping reachable for the first time, and saveConnection deleted secure plugin fields for the new type only, leaving the old type's Keychain entries under the same connection id. It now mirrors what ownedAdditionalFieldIDs() already did for plain fields.
  • The action bar recomputed the whole validation set four to five times per body evaluation, on every keystroke in the Name field. It walks once.
  • visibleTabs was hand-assembled; it is now allCases filtered by a per-tab rule, so the compiler forces the answer for a case added later.
  • The Appearance tab claimed customization.validationIssues while Options renders the Safe Mode picker that view model owns, which would have sent the user to a tab with no such control.
  • Text(String) renders verbatim, so the backticks in the transport summaries printed as characters; the summaries are plain prose now.
  • Two .sheet modifiers on one view resolve to a single presenter on macOS, which could have left isChoosingType stuck true and Change… dead.
  • directSummary pointed a file-based driver at a Host and Port it does not have.
  • The CHANGELOG had a blank line splitting ### Fixed into two lists, and a Fixed entry for a pane this same unreleased change deletes.

One finding is refuted by measurement: .navigationTitle was said to have no consumer after the NavigationSplitView was removed. The running build's window reads "New PostgreSQL Connection", so it still propagates.

The review also caught that XCTSkipUnless in the UI test's tab helper would turn a broken tab bar green. Removing it exposed a genuine failure: app.searchFields.firstMatch was matching the welcome window's sidebar-filter rather than the chooser sheet's search field, so the driver name went into the connection filter and the chooser list was never filtered. The helper is scoped to the sheet now. TableProUITests/RedisConnectionModeUITests.swift:69 has the same latent mismatch and passes only because "Redis" is realised in the unfiltered list.

Docs

docs/connections/connection-form.mdx is rewritten around the four tabs and the transport picker, and its screenshot pair is re-captured. Fourteen other pages instructed readers to turn on toggles that no longer exist, named panes that are now sections, or gave a location for a control that moved; all are updated. quickstart.mdx in particular told the reader to click Test Connection in a Status row and Save & Connect in the toolbar, and neither is where it was.

Screenshots still to re-capture. 22 image pairs under docs/images/ show the old form. This PR re-captures connection-form-fields; the other 21 pairs are stale and are not done here, because they span 20-odd database types in two appearances and several need plugins that are registry-only:

cloud-sql-proxy-pane, cloudflare-d1-connection-form, cloudflare-tunnel-pane, cockroachdb-connection-form, connection-customization, connection-ssl-settings, connection-test, dameng-connection-form, duckdb-connection-form, dynamodb-connection-form, mongodb-connection-form, mssql-connection-form, oracle-connection-form, pglite-connection-form, postgresql-connection-form, snowflake-connection-form, socks-proxy-pane, sqlite-connection-form, ssh-tunnel-config, teradata-connection-form, trino-connection-form, tunnel-command-pane.

https://claude.ai/code/session_01EkZFosuu6fvVgdVmNaUFt9

@mintlify

mintlify Bot commented Sep 5, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated
TablePro 🟢 Ready View Preview Sep 5, 2026, 7:22 AM

💡 Tip: Enable Automations to automatically generate PRs for you.

@datlechin

Copy link
Copy Markdown
Member Author

Screenshots (1x, 1920x1080 display). Attached from the local capture run; paths for reference:

  • Before, General: before-general-crop.png
  • Before, SSH Tunnel: before-ssh.png
  • After, General: after-general.png
  • After, Network: after-network.png
  • After, Network with SSH Tunnel: after-network-ssh.png

gh cannot upload images, so these need dragging into the PR from the capture directory.

@datlechin

Copy link
Copy Markdown
Member Author

Updated since the first review round.

A regression this branch had introduced is fixed. ConnectionFormView carried two .sheet(isPresented: $coordinator.isChoosingType) presenters: I added one while addressing the review's duplicate-presenter finding and left the original in place. macOS resolves two sheet modifiers on one view to a single presenter, so the loser would keep its binding true with nothing on screen and Change… would go dead. Removed.

Twenty-two docs screenshots re-captured, replacing 11 of the 22 stale pairs listed above: postgresql-connection-form, sqlite-connection-form, cockroachdb-connection-form, pglite-connection-form, connection-customization, connection-ssl-settings, ssh-tunnel-config, socks-proxy-pane, cloudflare-tunnel-pane, cloud-sql-proxy-pane, tunnel-command-pane. Light and dark, 900x720, pairs verified equal.

Still stale, and why. Ten pairs need drivers that are registry-only, and a sandboxed run cannot load them: oracle, mongodb, mssql, dameng, duckdb, dynamodb, snowflake, teradata, trino, cloudflare-d1 connection forms. Seeding their built bundles into the sandbox's plugin directory did not get them loaded. Also stale: connection-test, which needs a connection that actually succeeds. All eleven show a per-driver General tab, so the change they miss is the same one postgresql-connection-form now shows.

A note on the capture, because it repeats a lesson from this branch's UI test. The first batch reported 22 successes and every non-General shot was silently the General tab: System Events matches a control by AXTitle, which SwiftUI does not publish for a segmented Picker, so click radio button "Network" was a no-op. The same shape as the XCTSkipUnless that made the transport UI test pass without touching the picker. The script now selects by index and asserts the tab and transport actually changed before capturing.

Five collateral defects found while investigating this refactor are now built in #2647, stacked on this branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant