fix(coordinator): load a table tab's initial query exactly once#1456
Merged
Conversation
9d5d1d4 to
821c463
Compare
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.
Symptom
Opening a table runs the same initial query up to four times. Logs from a Chinook sample SQLite open show four
[executeUserQuery] SELECT * FROMTrackLIMIT 1000 OFFSET 0lines, of which three returnrows=0(interrupted) and one returnsrows=1000. The grid ends up correct becausequeryGenerationdiscards the stale results, but four SQLite queries actually ran, three were cancelled mid-flight viasqlite3_interrupt, and on slower remote databases the redundant attempts add real latency and flicker.Root cause
The initial auto-load was not idempotent, for two reasons:
initializeAndRestoreTabs(MainContentView+Setup.swift:70and:160) calledcoordinator.executeTableTabQueryDirectly()directly, skipping the freshness guards inlazyLoadCurrentTabIfNeeded.lazyLoadCurrentTabIfNeededcould not tell "abandoned mid-flight" from "still running." Any second trigger cleared the flag and re-fired, cancelling the running query.Apple's docs give no exactly-once guarantee for
onAppear/.task(only "before the first frame"). The correct fix is to make the load idempotent in the coordinator, not to chase lifecycle re-fires.Approach
Apple's WWDC21 "Protect mutable state with Swift actors" pattern: store the in-flight
Taskand have concurrent callers coalesce onto it instead of starting a new one. TablePro already uses this pattern inSQLSchemaProvider.loadTask. This PR extends it to table-tab loads.Changes
MainContentCoordinator: newtableLoadTasks: [UUID: Task<Void, Never>], keyed by tab id.teardown()cancels and clears the map.MainContentCoordinator+WindowLifecycle.lazyLoadCurrentTabIfNeeded: rewritten as the single funnel. Pure freshness gate (canAutoLoadTableTabhelper) → map-based coalescing (tableLoadTasks[tab.id] == nilelse return) → abandoned-flag recovery (clearAbandonedExecutingFlagIfNeededhelper, usescurrentQueryTask == nilas the discriminator) → connection check → spawn wrapping Task. The wrapping Task awaitscurrentQueryTaskso the map entry lives until the query observably completes;deferclears it.MainContentView+Setup.swift: the twoinitializeAndRestoreTabsauto-load call sites now go throughlazyLoadCurrentTabIfNeeded(). Combined with the existing.task(id:)andconnectionStatusChangedpaths, all four view trigger paths now funnel through one coordinator method.canAutoLoadTableTab,clearAbandonedExecutingFlagIfNeeded) replace it, matching the project's no-comments rule.The three other callers of
executeTableTabQueryDirectly(runQuery:748, Redis post-switchNavigation:479, and the new wrapping Task inside lazyLoad) are intentional re-runs and stay direct.Why a refactor, not a patch
The earlier targeted patch in this branch (
currentQueryTaskas the in-flight signal) closed the symptom but kept the auto-load orchestration spread across the view layer (initializeAndRestoreTabs's direct executor calls, the lazyLoad heuristic, the abandoned-flag clear). The full refactor moves auto-load ownership onto the coordinator with the WWDC21 coalescing pattern, eliminates the heuristic in favour of an unambiguous signal (presence in the per-tab map), and makes the call shape uniform across all four lifecycle trigger paths.Tests
skipsWhenLoadTaskRegistered(replacesskipsWhenAlreadyExecuting): seedstableLoadTasks[tabId]with a sleeping Task, calls lazyLoad, asserts no new entry andneedsLazyLoad == false(returned early before the connection check). Directly exercises the map-based coalescing.recoversAbandonedExecutingFlag:isExecuting=true,currentQueryTask=nil, empty map. lazyLoad clears the flag and falls through to the disconnected deferral. Proves abandoned-load recovery still works.Checks
swiftlint lint --stricton changed files: clean.execution.errorMessageandlastExecutedAtare NOT persisted byQueryTab.toPersistedTab(), so the extra guardslazyLoadadds for restored tabs are no-ops in practice (no regression for tabs that previously errored).