Skip to content

fix(toolbar): present the connection and database switchers from the command, not the toolbar item - #2374

Merged
datlechin merged 5 commits into
mainfrom
fix/switcher-presentation-independent-of-toolbar
Aug 21, 2026
Merged

fix(toolbar): present the connection and database switchers from the command, not the toolbar item#2374
datlechin merged 5 commits into
mainfrom
fix/switcher-presentation-independent-of-toolbar

Conversation

@datlechin

Copy link
Copy Markdown
Member

Switch Connection (Ctrl+Cmd+C) and Open Database (Cmd+K) do nothing at all on a narrow window. No popover, no error, no feedback. This is what fails SwitcherEscapeUITests on CI, both cases, on rerun as well as first run.

Root cause

isConnectionSwitcherShown is presentation state on MainContentCoordinator, and three view-agnostic routes set it: the toolbar action, the main-menu item, and its key equivalent. Its only consumer was .popover(isPresented:) on ConnectionToolbarButton, mounted as the hosted SwiftUI content of the connectionGroup toolbar item.

NSToolbarItem's header states the contract: when the window is too small an item "will be clipped but remain accessible from a 'clipped items' menu containing the menu item returned here", meaning its menuFormRepresentation. The item's view is never what survives clipping. So the command set a flag nothing was listening to.

From the element tree attached to the CI failure, at 1024x674:

Toolbar, {{0.0, 31.0}, {1024.0, 52.0}}
  Group ... RadioButton 'List' / 'favorite'
  Group ... Button 'Back' / 'Forward'
  Group ... 'Status'
  Button 'Inspector'
  PopUpButton, {{923.0, 38.0}, {38.0, 38.0}}, label: 'more toolbar items'

No button labelled Connection anywhere in the tree.

This is not only about narrow windows. MainWindowToolbar sets allowsUserCustomization = true with autosavesConfiguration = true and implements no toolbarImmovableItemIdentifiers(_:), so anyone can drag the item out in Customize Toolbar and it stays out. The HIG says why that matters: "Make every toolbar item available as a command in the menu bar. Because people can customize the toolbar or hide it, it can't be the only place that presents a command."

Fix

The command owns the surface. ToolbarSwitcherPresenter resolves the toolbar item at presentation time and picks between two surfaces:

Anchored. NSPopover.show(relativeTo: NSToolbarItem), API_AVAILABLE(macos(14.0)), our deployment target. Its header: "When the item is in the overflow menu, the popover will be presented from another appropriate affordance in the window." The overflow case is AppKit's job, not ours, which is the whole reason to use this call rather than hand-rolling an anchor.

Unanchored. The same content in QuickSwitcherPanelController, the panel Open Quickly already uses, for when the item is not in the toolbar at all.

show(relativeTo:) throws NSInvalidArgumentException "because the item is not in a toolbar, or because the toolbar is not in a window", and Swift cannot catch that, so resolution is a precondition rather than error handling. A hidden toolbar counts as no anchor: toggleToolbarShown only flips isVisible and leaves the items in place, so the documented throw condition is not met and the behaviour there is undocumented. Choosing the branch we can reason about avoids the uncatchable-exception class entirely.

Both flags are gone. grep finds zero references to isConnectionSwitcherShown or isDatabaseSwitcherShown across the app and both test targets.

Two approaches ruled out by evidence, not preference

visibilityPriority cannot do it. The header offers only "To suggest that an item always remain visible". A suggestion is not a guarantee, and the item still overflows eventually.

Demoting connectionGroup from isNavigational makes it worse. Measured at a 1364pt window: connectionGroup is still visible at x=703 while refreshSaveGroup, quickSwitcher, newTab and previewSQL are already in the overflow menu, with the chevron at x=1294. Ordinary items overflow sooner than navigational ones here, so removing the flag would clip it earlier.

Scope

Open Database has the identical defect one line below, at MainWindowToolbar+Buttons.swift:50, independently verified. openDatabaseSwitcher() additionally cleared presentedScopeSwitcher, disabling the one alternate route it had. Fixing Connection alone would leave the same class latent, so both ship together.

Deliberately untouched: the scope chip in the centred status item keeps its SwiftUI popover. It has no menu command and no shortcut, so nothing can fire it while its own view is off screen. It needed only a dismiss closure now that the shared content takes one.

Verification

build                                    PASS
test  ToolbarSwitcherAnchorTests         PASS  5 of 5
uitest SwitcherEscapeUITests             PASS  2 of 2, unmodified
uitest SwitcherWithoutToolbarAnchorUITests PASS 2 of 2
lint  7 paths                            PASS  0 violations

SwitcherEscapeUITests is the CI reproduction and passes without being edited, per the rule that a test defines expected behaviour.

What is not verified locally, and why. The exact CI state, an overflowed item on a 1024pt screen, cannot be reproduced on a developer machine: recomputeWindowMinSize() puts the window's minimum width near 1364pt with a table tab open, and a pinned 1024pt frame comes back as 1364x674, height honoured and width clamped. The overflow branch rests on AppKit's documented contract plus this PR's own CI run. What is covered locally is the branch we own: anchored, and unanchored via a hidden toolbar.

Review findings folded in

A code-review pass caught nine real defects in the first draft, including one that would have shipped the new path broken:

  • The panel paints no background of its own. It is borderless, clear and corner-masked at 26pt, and the surface material belongs to the content the way QuickSwitcherPanelView supplies it. Without .quickSwitcherSurface(cornerRadius:) the switcher would have floated as unbacked text with its corners cut off, on precisely the path this PR adds.
  • .semitransient ignores interaction outside its own window, so the chooser would linger over a window it no longer belonged to. Now .transient, matching what the SwiftUI popover did.
  • Nothing observed NSPopover.didCloseNotification, so a closed popover kept its hosting controller, SwiftUI tree and loaded container list alive until the next presentation.
  • No toggle guard, so a second press rebuilt the surface with empty @State and lost the typed filter. isPresenting already existed and had no callers; it is now the guard, matching showQuickSwitcher().
  • The presenter built its own QuickSwitcherPanelController when the coordinator already owns one, giving a window two panels centred on the same point, neither able to dismiss the other.
  • openConnectionSwitcher did not clear presentedScopeSwitcher, breaking the one-chooser-per-window invariant its own comment asserts.
  • invalidate(), which window close goes through instead of repoint, did not dismiss. The panel is an independent floating NSPanel with no parent relationship to the window, so it would have outlived it.
  • The unit tests used defer { _ = delegate } to keep a weakly-held toolbar delegate alive. That is a discard the optimizer may elide, and a deallocated delegate leaves a toolbar with no items, so the tests would have passed while testing nothing. Now withExtendedLifetime.
  • The docs sentence I wrote was wrong: a clipped item still resolves and still gets a popover, so the panel is only for a removed button or a hidden toolbar.

Known trade-off

Both switchers anchor to connectionGroup. That is the only item AppKit draws a frame for; connection and database are subitem identifiers that exist to populate the overflow menu and carry no frame. So Cmd+K's arrow points at the group rather than at the Database button specifically. Anchoring precisely again would mean reaching back into the hosted view, which is the coupling this removes.

Collateral, reported not fixed

ViewMenuBuilder.swift:134 builds the toolbar toggle with a fixed "Show Toolbar" title, so it reads that way even while the toolbar is showing. AppKit's standard item renames itself to match the state. Cosmetic, independent of this fix, and it is what made the first version of the new UI test fail.

@mintlify

mintlify Bot commented Aug 21, 2026

Copy link
Copy Markdown

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

Project Status Preview Updated (UTC)
TablePro 🟢 Ready View Preview Aug 21, 2026, 9:00 PM

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

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

…lbar

Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
…tion-independent-of-toolbar

# Conflicts:
#	docs/connections/index.mdx
…thub.com:TableProApp/TablePro into fix/switcher-presentation-independent-of-toolbar

# Conflicts:
#	docs/connections/index.mdx
@datlechin
datlechin merged commit 159be66 into main Aug 21, 2026
9 checks passed
@datlechin
datlechin deleted the fix/switcher-presentation-independent-of-toolbar branch August 21, 2026 21:11
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