fix(plugins): pin the row import sheet to one database and keep new table column edits across a re-read - #3191
Open
datlechin wants to merge 4 commits into
Conversation
…able column edits across a re-read
…read proposes the same value or misses the field
…pping' into fix/row-import-sheet-scope-and-new-table-edits # Conflicts: # CHANGELOG.md
…pping' into fix/row-import-sheet-scope-and-new-table-edits # Conflicts: # CHANGELOG.md
This branch has not been deployed
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.
Stacked on #3183 (
feat/import-remembered-column-mapping). Found while investigating #3172 (#3183).Two defects in the row import sheet.
1. The sheet wrote to whichever database the connection was browsing at each step
Root cause
The sheet asked
DatabaseManager.browseScope(for:)for its target at three different moments: when it listed the tables (withBrowseMetadataDriver), whenever the view rebuilt the identity of a column read (the scope inSourceRead), and when Import was pressed, which then created the new table, cleared it on a retry, ran the import and saved the mapping. The browse database is per session, not per window, so another window on the same connection, or an MCP client, can move it while the sheet is open, even though the sheet is modal to its own window.Two consequences:
A.ordersimported intoB.orders.createdTables) was keyed by bare table name. After a failed new-table import createdtinA, a retry after the browse database moved toBfoundtin the dictionary, plannedreuseAfterClearing, and ranDELETE FROM tonB's ownt.Fix
The sheet takes one
DatabaseScopewhen it opens (seeded ininitthroughState(initialValue:), so it is read once per sheet identity) and uses it for the catalog read, the column read,CREATE TABLE, the retry'sDELETE FROM, the import and the remembered mapping. The catalog read now goes throughwithMetadataDriver(scope:). If the connection had no session when the sheet opened, Try Again on the table list takes the scope once a session exists; after that it never changes. The intent the file's comments describe is kept: the scope is still the browse scope, taken at the moment the user opened the sheet.Execution stays correct when the browse database has moved on:
executionRoute(for:)sends the pinned scope to the session driver with aUSEon engines that switch in place, to a pooled connection on engines that reconnect to switch, and refuses on an engine that can do neither, rather than writing to the other database.ImportServicerecords the import in query history and in the completion report under the scope's database. It readbrowseDatabaseName(for:)when the import ended, so a switch during the import labeled it with the other database; this affects the SQL import dialog too, which already passed the scope it started with.NewTableImportPlannerbecomes a small value type that records what the sheet created keyed byTableScope(connection, database, schema, table), so a table the sheet made in one database can never vouch for a same-named table in another.TableScope(table:in:)builds a table scope from aDatabaseScope.2. A parsing option change threw away every new-table column edit
Root cause
Changing a parsing option (delimiter, header row, trim, empty as NULL, NULL text) reads the file again, and
loadNewColumnsrebuiltnewColumnsfrom scratch, so every rename, type, primary key, nullability, default and exclusion the user had set was lost.Fix
NewTableDraftholds the new table's columns outside the view. Each column keeps what the latest read proposes (name = field name, type = inferred type, included, not a key, nullable, no default) apart from what the user set,NewTableColumnEdits, which holds one optional per setting. Writing a column's settings records each setting the write changed, and a read never touches that record: a setting the user wrote stays theirs whatever a later read proposes, including a read that proposes the same value, and a setting nobody wrote follows each read. That is the rule the sheet already applies to its table name throughproposedTableName. A type written back with only a case difference is not a change, because the type menu offers the dialect's own spelling of the type on show.The draft also sets aside the edits of any field a read does not produce. A wrong delimiter or the header row turned off yields other fields for one read, and putting the option back brings the fields back with their edits.
The CREATE definition, the field-to-column mapping, the column name checks and Create all columns moved onto the draft with it, which takes the sheet from 1,124 to 1,078 lines.
Review follow-up
An independent review of the first version found two holes, both from deciding whether the user had edited a setting by comparing it with the latest proposal:
codeholds42: pick INTEGER, turn Trim on (proposes INTEGER), turn Trim off (proposes TEXT), and the column became TEXT.;picked by mistake on a comma-separated file, then,again, dropped every rename, key and exclusion.Recording edits as their own state fixes both. One behaviour changes on purpose: a setting changed and then set back to what the sheet proposed now stays the user's choice, where the first version let it follow the next read. Nothing on screen tells the two apart, so the last value the user picked is the one kept.
Tests
NewTableDraftTests(new, 15 cases): edits kept across a re-read, untouched settings following the new inference, a chosen type surviving an inference change, a chosen type surviving a read that proposes the same type and then one that proposes another, edits coming back with a field one read missed, a case-only type change and a write of the settings on show recording no edit, a setting set back to the proposal staying the user's, edits surviving several reads, fields added, dropped and reordered, the CREATE definition and mapping, column name problems, include-all. The two review cases fail against the first version of the draft.NewTableImportPlannerTests(updated to the new API, 3 new cases): a table created in one database or schema does not make a same-named table in another the sheet's own, and created names are listed per scope.ImportServiceHistoryTests(new, 1 case): an import handedarchiveon a connection whose browse database isshopis recorded underarchive. It registers a stand-in import format and fails at the route, which reaches the history path without a driver.RowImportNewTableEditsUITests(new): opens a CSV whoseGenreIdholds9001, setsTitleas the primary key, turns on Trim leading and trailing spaces, and checks thatGenreId's type follows the read toINTEGERwhileTitlestays the key. Not run locally: the machine was in use, so CI is its first run.The scope pin itself lives in the view and has no unit test; the part of it that decides what may be cleared (the planner keyed by
TableScope) does. A UI test for the scope drift would need a second window to switch database while the sheet is open, which is not deterministic.Verification
Through
.claude/skills/fix-issue/scripts/verify.shin the worktree:generate: PASS.build(TablePro, Debug): PASS.test ImportServiceHistoryTests NewTableDraftTests NewTableImportPlannerTests RowImportMappingTests TableScopeTests TableScopeDecodeTests TransferAlertWindowOwnershipTests ImportColumnMappingStoreTests CoordinatorRowImportTests: PASS, 65 executed, 65 passed, 0 failed.buildPASS,test NewTableDraftTestsPASS, 15 executed, 15 passed, 0 failed (NewTableDraftTestsis the only suite that uses the changed types),lint0 violations, docs checks andlocalization.py verifypass.linton every changed Swift file: 0 violations. The step itself reports FAIL only for a stale/Applications/Xcode-beta.apppath in.claude/skills/fix-issue/references/verification.md, which this branch does not touch.docs/scripts/check-writing-style.shanddocs/scripts/check-docs-against-source.py: pass.scripts/localization.py verify: ok. No new user-facing strings.Codex reviewed the branch against
feat/import-remembered-column-mappingtwice. The first pass found one issue, the history and completion report naming the browse database, which is fixed here. The second pass, on the final commit, found none. After the review follow-up, Codex reviewed the uncommitted change and flagged only narrative comments, which were trimmed; a third pass over the whole branch against its base found nothing.Not in this PR
selectedTargetTableas the target for a new-table import too, so Save Report on a new-table import that skipped rows names the wrong table, or none.