fix(toolbar): present the connection and database switchers from the command, not the toolbar item - #2374
Merged
datlechin merged 5 commits intoAug 21, 2026
Conversation
…command, not the toolbar item Claude-Session: https://claude.ai/code/session_013MEaba8K1HQcyDNeq5wEFk
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 failsSwitcherEscapeUITestson CI, both cases, on rerun as well as first run.Root cause
isConnectionSwitcherShownis presentation state onMainContentCoordinator, and three view-agnostic routes set it: the toolbar action, the main-menu item, and its key equivalent. Its only consumer was.popover(isPresented:)onConnectionToolbarButton, mounted as the hosted SwiftUI content of theconnectionGrouptoolbar 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 itsmenuFormRepresentation. 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:
No button labelled
Connectionanywhere in the tree.This is not only about narrow windows.
MainWindowToolbarsetsallowsUserCustomization = truewithautosavesConfiguration = trueand implements notoolbarImmovableItemIdentifiers(_:), 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.
ToolbarSwitcherPresenterresolves 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:)throwsNSInvalidArgumentException"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:toggleToolbarShownonly flipsisVisibleand 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.
grepfinds zero references toisConnectionSwitcherShownorisDatabaseSwitcherShownacross the app and both test targets.Two approaches ruled out by evidence, not preference
visibilityPrioritycannot 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
connectionGroupfromisNavigationalmakes it worse. Measured at a 1364pt window:connectionGroupis still visible at x=703 whilerefreshSaveGroup,quickSwitcher,newTabandpreviewSQLare 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 clearedpresentedScopeSwitcher, 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
SwitcherEscapeUITestsis 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-reviewpass caught nine real defects in the first draft, including one that would have shipped the new path broken:QuickSwitcherPanelViewsupplies it. Without.quickSwitcherSurface(cornerRadius:)the switcher would have floated as unbacked text with its corners cut off, on precisely the path this PR adds..semitransientignores 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.NSPopover.didCloseNotification, so a closed popover kept its hosting controller, SwiftUI tree and loaded container list alive until the next presentation.@Stateand lost the typed filter.isPresentingalready existed and had no callers; it is now the guard, matchingshowQuickSwitcher().QuickSwitcherPanelControllerwhen the coordinator already owns one, giving a window two panels centred on the same point, neither able to dismiss the other.openConnectionSwitcherdid not clearpresentedScopeSwitcher, breaking the one-chooser-per-window invariant its own comment asserts.invalidate(), which window close goes through instead ofrepoint, did not dismiss. The panel is an independent floatingNSPanelwith no parent relationship to the window, so it would have outlived it.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. NowwithExtendedLifetime.Known trade-off
Both switchers anchor to
connectionGroup. That is the only item AppKit draws a frame for;connectionanddatabaseare subitem identifiers that exist to populate the overflow menu and carry no frame. SoCmd+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:134builds 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.