Skip to content

Rewrite tab/window subsystem: TabSession source of truth, fix rapid Cmd+Number animation queue#968

Merged
datlechin merged 2 commits intomainfrom
refactor/tab-subsystem
May 3, 2026
Merged

Rewrite tab/window subsystem: TabSession source of truth, fix rapid Cmd+Number animation queue#968
datlechin merged 2 commits intomainfrom
refactor/tab-subsystem

Conversation

@datlechin
Copy link
Copy Markdown
Member

Summary

  • Rewrite the editor tab/window subsystem so per-tab state lives in a single TabSession (@Observable @MainActor class) instead of being scattered across 13 stores. The MainContentCoordinator god-object shrinks substantially; query execution is extracted into a focused QueryExecutor service; lifecycle work moves to Apple-documented hooks.
  • Fix the rapid-Cmd+Number tail-of-switching bug: selectTab(number:) now uses NSWindowTabGroup.selectedWindow wrapped in NSAnimationContext.runAnimationGroup(duration: 0) so AppKit doesn't queue a CAAnimation per press. windowDidBecomeKey is now the lightweight Apple-documented contract — lazy-load runs through SwiftUI's .task(id:) view-appearance lifecycle instead.
  • Net diff: +2794 / -1609 across 71 files (production net is much smaller — most insertions are tests + the architecture doc).

What landed (per phase)

The refactor was done as a strangler-fig migration tracked across 5 logical phases. See docs/architecture/tab-subsystem-rewrite.md for the full design doc with Apple-source citations and the migration plan.

Phase Delivered Removed
PR1 TabSession.swift (foundation type), design doc, tests
PR2 TabSessionRegistry (UUID→TabSession bridge), loadEpoch field on QueryTab + TabSession, TableRowsStore becomes facade
PR3 QueryExecutor.swift, QuerySqlParser.swift, 22 tests 432-line query path inside MainContentCoordinator decomposed
PR4 selectTab(number:) uses NSWindowTabGroup.selectedWindow + NSAnimationContext; lazyLoadCurrentTabIfNeeded on coordinator; .task(id: TabLoadKey) on MainEditorContentView; deferred eviction sort onWindowBecameKey/onTeardown/onWindowWillClose stored closures, lastResignKeyDate field, 200ms isMenuBounce guard
PR5a row-access API on TabSessionRegistry TableRowsStore.swift (75 lines)
PR5b ColumnVisibilityPersistence.swift, MainContentCoordinator+ColumnVisibility.swift rewritten Models/UI/ColumnVisibilityManager.swift (83 lines)
PR5c MainContentCoordinator+FilterState.swift (per-tab helpers), TabFilterState now single-source-of-truth on QueryTab.filterState mirrored to TabSession FilterStateManager class (350 lines) — kept FilterLogicMode enum + TabFilterState ext
PR5d MainContentCoordinator+MongoDB.swift (empty)
PR5e CHANGELOG audit, design doc completion notes

Cmd+Number lag bug

The user-visible "switching continues after key release" symptom is fixed in PR4 by the NSAnimationContext.runAnimationGroup(duration: 0) wrapping (Apple-documented anti-animation idiom — not a custom debouncer/coalescer). Per-switch AppKit overhead remains because TablePro intentionally uses one NSWindow per tab (.tabbingMode = .preferred) for native integration; this is an accepted platform trade-off documented in docs/architecture/tab-subsystem-rewrite.md D2.

State migration: before → after

State Before After
Filters FilterStateManager (3 copies: live, snapshot, UserDefaults) QueryTab.filterState mirrored to TabSession.filterState; FilterStatePersistence enum for UserDefaults
Hidden columns ColumnVisibilityManager global, swap on tab switch QueryTab.columnLayout.hiddenColumns mirrored to TabSession.columnLayout; ColumnVisibilityPersistence enum for UserDefaults
Row data TableRowsStore[id] keyed by tab UUID TabSession.tableRows, accessed via TabSessionRegistry
Load-epoch (didn't exist) TabSession.loadEpoch (bumped on eviction; drives .task(id:) re-fire)
Stored closures onWindowBecameKey, onTeardown, onWindowWillClose gone — direct strong refs + syncSidebarToSelectedTab method

Out of scope (future work)

  • ConnectionToolbarState per-tab semantics — currently per-connection, migrating to per-tab would change UX (toolbar state would differ between tabs in same window). UX decision needed first.
  • Splitting MainContentCoordinator into a separate TabGroupCoordinator per the original design doc target — coordinator's responsibilities are now scoped properly via the per-domain helpers; deeper split deferred.

Test plan

  • Build cleanly in Xcode (no compile errors).
  • Run the test suite — existing tests pass; new tests cover TabSession round-trip, TabSessionRegistry, QueryExecutor SQL parsing/parameter handling, ColumnVisibilityPersistence, FilterStatePersistence, lazyLoadCurrentTabIfNeeded guard contract.
  • Manual: open a connection, open multiple tables (multiple native window tabs).
  • Manual: rapid Cmd+Number burst — confirm no tail of animations after key release.
  • Manual: switch tabs (Cmd+Shift+]), confirm filter state preserved per-tab.
  • Manual: hide columns on one tab, switch, confirm hidden state preserved per-tab and persisted across sessions.
  • Manual: close a window with unsaved changes — confirm aggregated save invariant preserved (sibling windows not clobbered).

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f709bbe696

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines 132 to 133
guard !activeTabIds.contains(tab.id),
tab.execution.lastExecutedAt != nil,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Exclude selected tab from deferred eviction candidates

handleTabChange now defers eviction with Task { ... }, so rapid tab switches can queue eviction jobs that run after selection has changed; this candidate guard only checks the captured activeTabIds and does not exclude the current selectedTabId, so a stale task can evict the tab the user is actively viewing and clear its rows unexpectedly. Add a guard against tab.id == tabManager.selectedTabId inside the eviction filter (or cancel/coalesce prior eviction tasks) to prevent active-tab data loss during fast navigation.

Useful? React with 👍 / 👎.

}
.task(id: TabLoadKey(
tabId: tabManager.selectedTabId,
loadEpoch: tabManager.selectedTab?.loadEpoch ?? 0
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Drive TabLoadKey from the epoch that eviction actually updates

The lazy-load task key uses tabManager.selectedTab?.loadEpoch, but eviction increments TabSession.loadEpoch in TabSessionRegistry and never propagates that change back to QueryTab.loadEpoch; as a result, the epoch portion of this task ID never changes after eviction, so .task(id:) will not re-fire when rows are evicted without a tab-ID change. This breaks the intended eviction-triggered reload path and can leave an evicted tab blank until a manual refresh.

Useful? React with 👍 / 👎.

…dupe stale-isExecuting clear, document invariants
@datlechin datlechin merged commit 1d1d6aa into main May 3, 2026
2 checks passed
@datlechin datlechin deleted the refactor/tab-subsystem branch May 3, 2026 07:34
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