feat(tabs): move an editor tab into a window of its own - #2572
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
datlechin
marked this pull request as ready for review
August 29, 2026 11:52
|
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.
Draft. Do not merge yet. Two Codex rounds have taken this from 15 findings (8 P1) to 8 (3 P1), and the three that remain are data-loss or broken-state class. They are listed in full below.
TableProUITestsalso cannot run on this machine, so none of it has been exercised end to end.Follow-up to #2571, which gave the tab strip's pointer input a single AppKit owner and left
canTearOffansweringfalse. This makes it answer for real.Fixes the second half of #2438.
What a connection hosted by two windows needs
Detaching a tab means one connection is visible in two windows. That state existed before 0.65.0 and the machinery is still here, so most of this is teaching the parts that assumed one window to ask for all of them:
WindowManager.workspaces(for:)andcoordinators(for:)return every hosting window.workspace(for:)stays as the first of them, and its two callers are corrected.MainContentCoordinator.allTabs(for:)unions across hosting windows and dedupes by tab id. It previously answered from the first window found, so a new query was numbered after tabs it could not see.aggregatedTabs(for:)dedupes, because the saved set is the union and writing a partial list over a full one is how tabs that were never closed get erased.TabPersistenceCoordinatoris now one instance per connection, not one per coordinator.hasObservedTabsis the gate that stops a partial write, and a second instance starts with it closed: the detached window's saves would all have been withheld, and whichever instance the periodic save elected decided whether anything reached disk at all.clearForUserClosedAllTabsrefuses while any window still holds tabs.ConnectionCloseActiondetects and saves unsaved work in every window, not the first.WorkspaceCloseActiongathers a container's tabs from every window, closes each through the coordinator that owns it, and runs the closing lifecycle on every workspace.The move
WindowManager.openTabInNewWindow(connectionId:tabId:)resolves the workspace that actually holds the tab, builds aSessionStatecarrying just that tab, hands the tab's liveTabSessionto the new registry so the rows it already loaded survive, registers it as pending, and opens a standalone window with.openContent(.restoreOrDefaultwould fill the new window with the whole saved set). The tab leaves the source manager only once the window exists.EditorTabDetachPolicyis the gate: more than one tab, nothing unsaved, nothing in flight, connection up. The command sits on the AppKit contextual menu, the SwiftUI menu and the accessibility actions, so the keyboard and VoiceOver reach it too.A detached window shows no tab strip, because
showsTabStriphides at one tab the way Safari hides its tab bar. That is deliberate, and it is why the UI tests identify the detached window by title.Still open, and why this is a draft
closeFrontmostTabreturn false so AppKit closes the window rather than emptying it. Codex found that this routes aroundcloseTabAwaiting, discarding live edits with no Save/Cancel and skipping Recently Closed Tabs, and CLAUDE.md requires Cmd+W to stay on thecloseTab()route. I reverted it, so the current behaviour is the lesser bug: closing a detached window's last tab leaves an empty window, and closing again takes the connection down in both. Needs an async route through the normal close primitive.WorkspaceCloseActionstill classifies victims with one coordinator. Closing is routed to the owning coordinator now, butconfirm(victims, coordinator:)still asks the first one, so a foreign victim with live grid or DDL edits reads as clean and is closed without a prompt.payload: nilandselectedTabIdnever changes after mount, sotoolbarState.isTableTabstays false andchangeManagerkeeps empty table, column and primary-key metadata: Find and Filter are disabled and later edits cannot save correctly.isExecutingis false during Fetch All and exact row counts; the tab would arrive permanently busy. NeedsTabExecutionRegistry.isBusyplus pagination state.ConnectionSession.reportedStatusor liveness.host(for:)still returns the first owner, so a Create Table started in the detached window can land in the original one.canMoveToNewWindow/moveToNewWindoware ambiguous for a split connection.cursorPositionslive on the source coordinator).Verified
buildPASS.testPASS, 64 cases: the newEditorTabDetachPolicyTestsplus everyEditorTab*suite andTabDiskStateDecodingTests, which owns the persisted-tab decoding this change touches.swiftlint lint --strictclean over the whole worktree. It also fixes two violations that reachedmainwith refactor(tabs): give the editor tab strip's pointer input a single AppKit owner #2571: alet _ =discard and two ACL-above-parent declarations.TableProUITestsdid not run. Every case fails at launch with "The sample database never finished opening", on this branch and on a worktree at cleanmainalike, and the app launched by hand copies the sample then sits idle at 0% CPU with no window.EditorTabDetachUITestsis written and committed; it has never executed.Given three open P1s and no end-to-end run, my recommendation is that this stays a draft until the UI harness works and the three are closed.
https://claude.ai/code/session_01L7uaHbJBPV1LaWL5QXzxyp