refactor(coordinator): give a window a registry of connection workspaces - #2097
Open
datlechin wants to merge 5 commits into
Open
refactor(coordinator): give a window a registry of connection workspaces#2097datlechin wants to merge 5 commits into
datlechin wants to merge 5 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
Why
A user opened two connections and got two separate windows, each showing the full workspace rail listing every open connection, and asked whether they could be merged into one window.
That is not a stray bug. Since 0.64.0 every connection gets its own window, and
WindowManager.tabbingIdentifier(for:)keys the native tab group on the connection id, so two connections can never share a window. Every window still renders the rail listing all of them, so the app names all your connections in N places and makes you window-switch between them. No mature client does both: tools with a persistent connection sidebar have one window, tools with a window per connection put the connection list behind an on-demand switcher.The tell that this was already known:
TabWindowController.frameAutosaveNameis one shared slot for every connection, so the windows stack and a rail click "reads as the window changing content rather than a different window being raised". That illusion survives one interaction. Every window writes that key on resize and move, only new windows read it back, andrecomputeWindowMinSize()resizes windows on its own.This is the first of three PRs moving the app to a single-window model. It is scaffolding, with no user-visible change.
What this changes
The connection was bound into the object graph at the wrong layer.
MainSplitViewControllerfixed its connection atinitand held the session, phase, and panel state as scalars, because a window served exactly one connection for its whole life.ConnectionWorkspaceholds everything a window needs to present one connection: phase, attempt token, session, session state, right-panel state.ConnectionWorkspaceRegistryholds the set of connections a window hosts and which one it shows, with ordering, neighbour selection on removal, and wrap-around cycling.MainSplitViewControllerreads those fields through the registry instead of storing them. Workspace construction moved out ofinitintoadoptWorkspace(payload:autoConnect:), so a window can call it more than once.WindowManager.connectionIdsRetainingRestoreIntent()collects per workspace rather than per window.The registry holds one entry throughout this PR, so behaviour is identical to before.
Two decisions worth reviewing
A late connect attempt discards itself.
finishAttemptnow takes the connection id and returns early when the registry no longer holds that workspace. The missing entry is the generation check, so a connect that outlives a connection the user closed cannot write a phase back and resurrect it.transition(to:for:)repaints only when the target workspace is the one on screen.Each workspace owns its undo stack.
NSWindow.undoManagerwas the right source while a window meant one connection. One window shared between connections would let Cmd+Z in one roll back an edit made in another, soConnectionWorkspacecarries its ownUndoManager.DataChangeManagerstill routes through the window for now and moves over in PR 2, when a window can actually hold more than one connection.Testing
ConnectionWorkspaceRegistryTests, 10 cases: insertion order and selection, idempotent insert, phase and attempt-token isolation between workspaces, distinct undo managers, neighbour selection on removal, clearing selection when the last workspace goes, ignoring selection of an unhosted connection, a removed workspace being unreachable, and cycling in both directions.No UI automation, because this PR has no user-visible behaviour to drive. The UI automation that would have caught the original report lands in PR 3, which is where two connections start sharing a window.
Build succeeds and
swiftlint lint --strictis clean.Not in this PR
The
TabWindowControllerandMainSplitViewControllerrenames the design called for are left out. They touch around 40 files with no behaviour change and would bury this diff. Worth doing as separate cleanup.Next
NSTabViewControllerwithtabStyle = .unspecified,newTab()appending to the existing tab manager, and the four window-reconciliation files deleted.