fix(tabs): close the review findings on moving a tab to its own window - #2574
Merged
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.
Closes the review findings #2572 shipped with. That PR merged as a draft with three P1s and five lower ones listed in its body; this closes eleven of them and says which two are left.
The P1s
A moved table tab arrived uninitialized. Its rows transferred and the grid drew, but the destination coordinator was built with no payload and
selectedTabIdis set before anything observes the manager, so the switch that normally prepares an incoming tab never ran:toolbarState.isTableTabstayed false andchangeManagerkept empty table, column and primary-key metadata. Find and Filter were disabled and a later edit could not name the row it was saving. The move now runshandleTabChange(from: nil, to:), which is that same preparation with no outgoing tab to put away.A container's tabs were classified by the wrong window.
WorkspaceCloseActiongathered victims from every window hosting the connection but put all of them to the coordinator that answered first. A tab's live grid and structure edits exist only in its own coordinator, so a victim in the other window read as clean and was closed without a prompt. Victims are now partitioned by owner and each group resolved and saved through the coordinator that has it.Closing a detached window discarded work. It went through
super.performClose, aroundcloseTabAwaiting, so live edits went with no Save or Cancel and the tab never reached Recently Closed Tabs, which CLAUDE.md forbids. It now closes the tab through the normal primitive and then closes the window throughcloseWindowAwaiting, which is what asks about an inspector edit: that work is connection scoped, so the tab close deliberately does not see it, and a rawclose()tore downRightPanelStatewith it.Three more that came out of fixing those:
MainSplitViewController.hostsDetachedTabseparates them.The rest
isBusy, notisExecuting. Fetch All registers unclaimed work and an exact row count lives in the tab's pagination, so both read as idle: the tab arrived withisLoadingMoreorisCountingExactraised and the completion still owned by the window it left, so it never came down.reportedStatus. An installed driver is a handle, not a live connection (CLAUDE.md); it survives a reconnect and the health monitor giving up, so the command was moving tabs into windows that could not use them.WindowHostSelectionprefers the frontmost owner when a connection is hosted by several windows. The candidate list comes from a dictionary, so "the first window that has it" is arbitrary, and a Create Table opened from the detached window could append to and focus the other one. Covered by three new cases.canMoveToNewWindowis withheld while a connection is split. Both it andmoveToNewWindowresolve the host by connection id alone, so with two hosts the command was offered in one rail and acted on the other's workspace.enrichedForPersistence, so the raw value dropped the user's position.Still open
ConnectionSessionscoped, sohasUnsavedWork(among:)reports it in both owners' iterations; Don't Save does not clear it, so the second window asks again and Cancel there aborts a close the user already answered for. Needs connection-scoped work treated once with per-coordinator edits still asked separately.///rationale in the changed files to be removed, readingCLAUDE.md's no-comment rule literally. I have kept them: every file this touches is already written that way, and the rule as practised bans comments that restate what the code does, not the recorded reason a shape is the way it is. Worth a maintainer's call rather than mine.Verified
buildPASS.testPASS, 74 cases:WindowHostSelectionTestswith its three new cases,EditorTabDetachPolicyTests, everyEditorTab*suite, andTabDiskStateDecodingTests.swiftlint lint --strictclean over the worktree.No CHANGELOG entry: the feature these fix is itself unreleased, and CLAUDE.md says to fold such fixes into the entry that already describes it rather than adding a Fixed line.
TableProUITestsstill does not run on this machine. Every case fails at launch with "The sample database never finished opening", at cleanmainas well as here, soEditorTabDetachUITestshas still never executed. Nothing in this change has been exercised end to end.https://claude.ai/code/session_01L7uaHbJBPV1LaWL5QXzxyp