fix(connections): keep the connections strip and its commands when a connection is down - #2551
Merged
Merged
Conversation
…connection is down
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This was referenced Aug 27, 2026
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.
A window whose selected connection is not connected hides the connections strip and disables every menu route to the other connections it hosts. Those connections stay open, with unsaved editor state in them, and the only ways left are Cmd+W, File > Close Connection, or another window's strip.
Found while investigating #2545.
Why the strip goes
ConnectionWindowPaneResolver.hidesChromeis right that an object browser with no session in it is an empty column, andhideWindowChromespells that as collapsing the sidebar split item. The connections strip lives in that same item, because AppKit grants full-height sidebar layout to exactly one leading sidebar andNavigationSidebarViewController's own comment records that two items was tried and rejected. So a rule about the object browser takes the window's connection switcher with it.The strip is not per connection. It lists every connection the window hosts, and switching through it needs no session and no coordinator.
What else was dead
Three separate things, each with the same shape: a window-level capability read off the selected connection.
MainContentCommandActions.showPreviousWorkspace,showNextWorkspace,toggleWorkspaceRail,canToggleWorkspaceRailandisWorkspaceRailEnabledall bounced straight back tocoordinator?.splitViewController, so the round trip through the coordinator added nothing but a nil failure mode.menuValidationContextreturned an all-false context whenevercommandActionswas nil, which disabled Show Next Connection and Show Previous Connection along with it, and the actions were no-ops anyway.TabRouter.openConnectionfronts the window and then returns atguard activeSessions[id]?.driver == nil. For a connection that is already up, that is every time, so Manage Connections re-fronted a window still showing a different connection and stopped there.The fix
ConnectionWindowPaneResolver.sidebarChromeMode(for:hasRail:)is a new pure function returningrevealed,railOnlyorhidden.hidesChromekeeps its answer and its reasoning for the object browser and the inspector.In
railOnlythe sidebar is narrowed rather than collapsed:minimumThicknessandmaximumThicknessboth go torailAllowance, inside theautosaveName = nilspan that already exists, because a clamp writes its width into the autosave record exactly as a collapse writes the collapsed flag.canCollapsegoes tofalsefor the span, since a divider double-click sees no menu validation.applySidebarMinimumThicknessis inert while clamped, because seven other call sites reachrecomputeWindowMinSizeand any of them writing the minimum back would leave a minimum above the maximum. The rail's own width is a setting, soonLayoutChangereapplies the clamp.ChromePaneLayoutis captured on the way out ofrevealedand never again, so arailOnlytohiddenstep cannot overwrite the user's width with the clamp.isSidebarCollapsednow answers "the object browser is off screen", which is what its five readers were asking. That keeps the toolbar's segment unlit and the menu title correct, andsetSidebarTab,focusSidebarSearchandpresentDatabaseFilterare refused while narrowed.The sidebar segment is disabled when there is no object browser to toggle. The View menu's switching commands act on the registry directly.
TabRouterselects the native tab, then the hosted connection.SidebarContainerViewController's search-field insets drop to.defaultHigh: they are a margin, and at width 0 a required pair is unsatisfiable.Measured
AppKit is not documented on this, so it was probed with
swiftcagainst the macOS 14 target (Xcode 27.0 / 27A5237l):minimumThickness == maximumThickness == 52and later releasing returns the item to its user width, 420, with or without an autosave name.autosaveName = nilspan.setPositionduring the clamped span discards the width: 420 becomes 280. Nothing here calls it.behavior = 1,allowsFullHeightLayout = true, itsNSGlassEffectViewandsafeAreaTop = 66, so sidebar material and full-height layout survive the clamp.Not done
Switch Connection in the toolbar stays disabled without a coordinator. Its popover is presented by the coordinator's
ToolbarSwitcherPresenter, so enabling it would give a live-looking button that does nothing. The connections strip and the View menu are the routes that survive, and both work now.Rail visibility still comes from the app-wide
WorkspaceRailStore.entries.count. Narrowing it to this window's own workspace count is a different quantity, not a smaller one: two windows each hosting one connection would lose their strips. That is a real question and it is left alone here.Tests
ConnectionWindowChromeTestsis new and builds a realMainSplitViewControllerin anNSWindowwith two workspaces, the harness #2545 established.ConnectionWindowPaneResolverTestsgains the pure cases.Covered: the strip survives an unavailable selected connection; a lone connection still hides its sidebar outright; the strip arriving and leaving while the connection is down moves the mode both ways; a narrowed sidebar refuses
setSidebarTaband refuses user collapse; the clamp tracks the rail allowance; reveal restores the user's sidebar across both hidden modes; and the View menu's switching validates with no coordinator.Making
sidebarChromeModeignorehasRailfails 5 of them. 110 cases pass across the chrome, resolver, three toolbar-validation, pane-synchronization, workspace-registry, phase-machine, detail-width and split-pane suites. macOS Debug build passes; SwiftLint reports 0 violations on the 11 changed files.No
TableProUITestscoverage: no suite opens two connections into one window today, and nothing drives a connection down deterministically. That gap is why the new suite drives the controller directly instead.Review
A Codex review of the branch found five issues, all fixed here: an existing toolbar-validation test broken by the new context field; Switch Connection enabled without a working action, which is why it stays disabled above;
makeKeyAndOrderFrontnot selecting a background native tab;canCollapseleft true inrailOnly; and the clamp not tracking a rail row-size change.Before / After
No screenshots. The state needs a window hosting two connections with the selected one down, which is what the new suite constructs; a still frame of a narrowed sidebar does not show that the strip is reachable, which is the whole claim.