Skip to content

fix(tabs): close the review findings on moving a tab to its own window - #2574

Merged
datlechin merged 2 commits into
mainfrom
fix/editor-tab-detach-followups
Aug 29, 2026
Merged

fix(tabs): close the review findings on moving a tab to its own window#2574
datlechin merged 2 commits into
mainfrom
fix/editor-tab-detach-followups

Conversation

@datlechin

Copy link
Copy Markdown
Member

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 selectedTabId is set before anything observes the manager, so the switch that normally prepares an incoming tab never ran: toolbarState.isTableTab stayed false and changeManager kept 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 runs handleTabChange(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. WorkspaceCloseAction gathered 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, around closeTabAwaiting, 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 through closeWindowAwaiting, which is what asks about an inspector edit: that work is connection scoped, so the tab close deliberately does not see it, and a raw close() tore down RightPanelState with it.

Three more that came out of fixing those:

  • The window is closed only if another host still exists after the await. A save sheet can stand for as long as the user likes, and the window the tab came from can close underneath it; closing then took the connection's last window and disconnected the session.
  • The tear-off window is marked, not inferred. With exactly two tabs open, detaching one leaves both windows holding one workspace and one tab with the connection hosted twice, so a test on that state alone was true of the source too and Cmd+W there closed the connection's own window. MainSplitViewController.hostsDetachedTab separates them.
  • Every window revealed for a prompt goes back. A dirty split connection brings more than one forward, and only one was restored, so an abandoned close left the others switched to a connection nobody chose.

The rest

  • The busy gate reads isBusy, not isExecuting. Fetch All registers unclaimed work and an exact row count lives in the tab's pagination, so both read as idle: the tab arrived with isLoadingMore or isCountingExact raised and the completion still owned by the window it left, so it never came down.
  • The connected gate reads 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.
  • WindowHostSelection prefers 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.
  • canMoveToNewWindow is withheld while a connection is split. Both it and moveToNewWindow resolve the host by connection id alone, so with two hosts the command was offered in one rail and acted on the other's workspace.
  • A moved query tab keeps its caret and selection. They live on the source coordinator and reach the tab only through enrichedForPersistence, so the raw value dropped the user's position.

Still open

  • [P2] A shared pending operation prompts twice. A staged truncate or delete is ConnectionSession scoped, so hasUnsavedWork(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.
  • [P3] Codex asked for the /// rationale in the changed files to be removed, reading CLAUDE.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

  • build PASS.
  • test PASS, 74 cases: WindowHostSelectionTests with its three new cases, EditorTabDetachPolicyTests, every EditorTab* suite, and TabDiskStateDecodingTests.
  • swiftlint lint --strict clean over the worktree.
  • Both docs scripts clean.
  • Reviewed by Codex over four rounds across this change and feat(tabs): move an editor tab into a window of its own #2572: 15 findings (8 P1), then 8 (3 P1), then 3 (1 P1), then 4 (1 P1). Everything except the two above is closed.

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.

TableProUITests still does not run on this machine. Every case fails at launch with "The sample database never finished opening", at clean main as well as here, so EditorTabDetachUITests has still never executed. Nothing in this change has been exercised end to end.

https://claude.ai/code/session_01L7uaHbJBPV1LaWL5QXzxyp

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@datlechin
datlechin merged commit 4716cd8 into main Aug 29, 2026
9 of 13 checks passed
@datlechin
datlechin deleted the fix/editor-tab-detach-followups branch August 29, 2026 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant