fix(structure): give each tab its own structure editor and let Save reach it - #2296
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
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.
A structure tab's staged ALTERs, and the ability to apply them, now belong to the tab rather than to whichever structure view happens to be on screen.
The two halves
A tab's structure editor was keyed on the table, not the tab.
structureContentgaveTableStructureViewthe identity"<database>.<schema>.<table>". Only the selected tab renders, so two tabs on one table occupied the same slot with the same explicit id, and SwiftUI updated the view in place instead of re-creating it.TableStructureView.initseeded_gridDelegateand_wrappedChangeManagerthroughState(wrappedValue:), which runs only at the first creation of an identity, andStructureGridDelegateholds its manager in alet. So the grid went on writing into the first tab'sStructureChangeManagerwhile the computedstructureChangeManagerresolved to the second's. Save applied a different set of changes than the one on screen, the second tab's Add and Remove buttons did nothing,onAppearnever re-ran so the inspector kept the first tab's rows, and closing the second tab never warned about edits it appeared to hold while closing the first discarded them.Applying those edits required a mounted structure view. This is the worse half, and it is not visible from the identity bug.
hasUnsavedWorkreadsstructureSessions[tab.id], so the unsaved-changes prompt can be raised by a tab that is not showing its structure at all. The Save it offered dispatched throughcoordinator.structureActions, aweakslot only a mounted structure view ever fills, and then returnedtrueunconditionally. Three ways to answer Save and lose the work:Cmd+W, SaveresultsViewMode, so it was skipped; every later branch was false; returnedtrueand closedCmd+W, SaveselectedTab != .ddlshort-circuited the handler; returnedtrueand closedThe DDL, Parts and Triggers sub-tabs are read-only everywhere else in the file, so that guard was suppressing the save and nothing else.
The fix
applyStagedChangesmoves ontoStructureEditingSession, the one object that exists per tab whether or not a view does. It returns aStructureSaveOutcomewhoseallowsCloseis false whenever the edits are still staged afterwards, so a save Safe Mode refused, one cancelled at the data-loss prompt, or one the server rejected leaves the tab open instead of closing over it.saveSelectedTabWorkasks the session instead of the view mode, and the batch close walks its victims.structureContentkeys on.id(tab.id), matching the five sibling builders that cache a view model undertab.id. Thesession.identity == identitybranch stays: flipping it is what forces a real remount when a tab is retargeted to another table.gridDelegate,wrappedChangeManager, and the editor's own place (selectedTab,searchText,sortState, its sort descriptor and the per-sub-tab column layouts) move onto the session too. That removes the class of bug rather than this instance of it: with them on the session, nothing is seeded from an input that can change under a stable identity. It also means each tab keeps its own sub-tab, filter and sort, including across a trip through the Data view, where all five used to be discarded.Two more discards came out of self-review, both on paths the first draft missed:
Also fixed, found while investigating
onTabRetargetedreleases the per-tab caches keyed on that tab id. A retarget keeps the id and changes what it means, so the structure session survived and went on raising an unsaved-changes prompt naming the previous table.selectedTabHoldsProtectedContentconsults staged structure edits. Single-clicking another table reused a preview tab in place with no prompt of any kind, even one holding a renamed column. This gates FK navigation through the same property.DataRefreshRequest.reaches(tabScope:)matches on connection, database and schema rather than table, so a mounted structure view on the same database answered it by asking whether to throw away the edits the user had just asked to save. Suppressed for the duration of the walk by adefer-scoped flag rather than a latch.TableStructureView.onDisappearandCreateTableView.onDisappearguard their clears by identity, the way the neighbouringinspectorRowSourceline in each already did. SwiftUI does not order the outgoing view'sonDisappearbefore the incoming view'sonAppear, so the unguarded clear could nil the wiring the incoming tab had just installed and leave its Save, Refresh, Preview SQL, undo and footer buttons dead until the tab was left and returned to.CreateTableViewis already on.id(tab.id), so it had this today.Verification
buildPASStest81 of 82 across the affected suites, with the wrapper reporting no unexplained failures. The one failure isCommandActionsDispatchTests/insertQueryFromAI_appendsToExisting, which is quarantined and unrelated. An earlier combined run also trippedMainContentCoordinatorRefreshTests/singleRequestRefreshHasNoTrailing, which passes 13 of 13 when its suite runs alone; it sleeps 400ms and asserts coalescer state, so it is load-sensitive rather than a regression here.lint TablePro TableProTestsreports three violations, all pre-existing onmainand all fixed by style: satisfy the swiftlint strict gate in the operation reporting code #2293. Nothing in any file this PR touches.New coverage in
StructureEditingSessionTests: two tabs on one table stage into their own managers, a session's grid delegate writes into that session's manager, each session keeps its own place in the editor, the outcome decides whether the close may proceed, and an end-to-end apply through a mock driver with no view mounted.OpenTableTabTestsgains the reuse gate and the retarget release.StructureTabIdentityUITestscovers the flow that regressed: open one table in two tabs and confirm the second does not inherit the first's sub-tab. It compiles and CI runs-only-testing:TableProUITests, but it could not be executed on this machine: the runner fails to initialize with "Authentication canceled. System authentication is running." Recording that rather than claiming it ran.Self-review also caught two things worth naming because they were mine, not pre-existing. Moving
gridDelegateonto the session created a reference cycle through the closures the view installs on it, which would have leaked the coordinator and its driver references;releaseViewWiring()breaks it at every site a session is dropped, includingteardown(). And a successful apply clearedsession.hasLoadedso an unmounted tab would refetch, but the mounted path never set it back, so the next remount re-ranloadInitialDataand re-baselined the change manager, discarding anything staged after the save.refreshAfterApplyrestores it.One thing left alone:
StructureSessionBaseDriverin the new test file duplicatesSchemaRoutingBaseDriverinDatabaseManagerSchemaChangeRoutingTests. Extracting a shared stub is test-only cleanup and did not belong in this diff.Two paths are not unit-tested and cannot easily be:
AlertHelper.presentfalls back torunModal()when no window resolves, so exercising the refused and failed outcomes throughapplyStagedChangeswould block the suite. They are covered throughStructureSaveOutcome.allowsCloseand the successful end-to-end path instead.