Skip to content

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
feat/import-remembered-column-mappingfrom
fix/row-import-sheet-scope-and-new-table-edits
Open

datlechin wants to merge 4 commits into
feat/import-remembered-column-mappingfrom
fix/row-import-sheet-scope-and-new-table-edits

Conversation

@datlechin

@datlechin datlechin commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

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 in SourceRead), 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 mapping read from A.orders imported into B.orders.
  • The retry bookkeeping for a new table (createdTables) was keyed by bare table name. After a failed new-table import created t in A, a retry after the browse database moved to B found t in the dictionary, planned reuseAfterClearing, and ran DELETE FROM t on B's own t.

Fix

The sheet takes one DatabaseScope when it opens (seeded in init through State(initialValue:), so it is read once per sheet identity) and uses it for the catalog read, the column read, CREATE TABLE, the retry's DELETE FROM, the import and the remembered mapping. The catalog read now goes through withMetadataDriver(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 a USE on 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.

ImportService records the import in query history and in the completion report under the scope's database. It read browseDatabaseName(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.

NewTableImportPlanner becomes a small value type that records what the sheet created keyed by TableScope (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 a DatabaseScope.

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 loadNewColumns rebuilt newColumns from scratch, so every rename, type, primary key, nullability, default and exclusion the user had set was lost.

Fix

NewTableDraft holds 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 through proposedTableName. 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:

  • A type the user picked counted as untouched once a read happened to propose that same type, and the next read replaced it. On a CSV whose code holds 42: pick INTEGER, turn Trim on (proposes INTEGER), turn Trim off (proposes TEXT), and the column became TEXT.
  • Edits of a field missing from one read were gone for good. A ; 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 handed archive on a connection whose browse database is shop is recorded under archive. 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 whose GenreId holds 9001, sets Title as the primary key, turns on Trim leading and trailing spaces, and checks that GenreId's type follows the read to INTEGER while Title stays 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.sh in 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.
  • After the review follow-up: build PASS, test NewTableDraftTests PASS, 15 executed, 15 passed, 0 failed (NewTableDraftTests is the only suite that uses the changed types), lint 0 violations, docs checks and localization.py verify pass.
  • lint on every changed Swift file: 0 violations. The step itself reports FAIL only for a stale /Applications/Xcode-beta.app path in .claude/skills/fix-issue/references/verification.md, which this branch does not touch.
  • docs/scripts/check-writing-style.sh and docs/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-mapping twice. 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

  • The success alert passes the existing-table picker's selectedTargetTable as 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.

…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

No deployments
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