Skip to content

fix(structure): confirm a Structure save once, at the execution gate or the rebuild review, and stay quiet after Cancel - #3154

Open
datlechin wants to merge 1 commit into
mainfrom
fix/structure-single-confirmation
Open

datlechin wants to merge 1 commit into
mainfrom
fix/structure-single-confirmation

Conversation

@datlechin

Copy link
Copy Markdown
Member

Follows #3147 and updates the StructureTypelessColumnUITests it added, which clicked both dialogs.

Root cause

Two components decided on their own whether to confirm a Structure save, and neither knew the other had already asked.

  • ALTER saves. StructureEditingSession+Apply showed a "Destructive Changes" alert for any change with requiresDataMigration: drop column, type change, nullable to NOT NULL, primary key change, adding a CHECK, rewriting a CHECK. Then executeSchemaChanges sent the statements to the execution gate, which confirms again whenever the text is destructive (any ALTER containing DROP, DROP INDEX) or the Safe Mode level confirms writes (Alert and above). A dropped column was asked twice at every level, Silent included. Every data-loss change was asked twice at Alert and above, and at Safe Mode that was alert, sheet, then Touch ID.
  • The editor alert existed only because the gate never read the statements' own flag. SchemaStatement.isDestructive was set by the generator and ignored by executeSchemaChanges, so the gate could not see that ALTER COLUMN .. TYPE or MODIFY COLUMN .. NOT NULL can lose or refuse rows.
  • Rebuilds. The rebuild review ("Apply and Rebuild", "Rebuild Table") ran StructureRebuildPlanRunner with a plain .interactiveUser request, so the gate stacked a second critical sheet over the review at every level. The comment in applyStagedChanges said the review was meant to be the one confirmation.
  • Cancel. A Cancel at the gate came back as DatabaseError.queryFailed("Operation cancelled by user"), shown as an "Error Applying Changes" sheet and reported as a failed operation.

Fix

  • The editor's alert is deleted. The gate's sheet, which shows the statements verbatim and names the connection, is the one confirmation for an ALTER save.
  • SchemaStatementGenerator marks every statement of a requiresDataMigration change as destructive. executeSchemaChanges builds its request through DatabaseManager.schemaChangeAuthorizationRequest, and schemaOperationKind(for:combinedSQL:databaseType:) returns .destructiveQuery for a flagged statement. The signature matches feat/mongodb-structure-field-edits, so that branch can drop its copy, and SchemaOperationKindTests.swift is byte-identical to that branch's.
  • A reviewed rebuild passes isConfirmationPreCleared: true through StructureRebuildPlanRunner.authorizationRequest. The gate skips its confirmation but still asks for Touch ID, still refuses at Read-Only and still writes the audit record. A metadata-only reorder, which has no review, passes false. A runnable review is titled with the operation, carries the "Runs on '…'" line, shows the script uncut and warns "This may permanently modify or delete data and cannot be undone." ahead of the plan's caveats, like the gate's own sheet for a destructive statement. A preview has no such warning, because it runs nothing.
  • OperationDecision.denied carries an OperationDenialCause (.policy by default, .cancelledByUser from the confirmation Cancel), and ExecutionGateError gained .cancelledByUser. The compiler flagged every existing binding site. StructureApplyFailure maps an error to what the save does: a Cancel returns .refused with the edits staged and shows nothing, a refusal shows its reason without a failed-operation report, and a server error behaves as before. The column reorder paths use the same mapping.
  • The gate can be injected into StructureEditingSession, executeSchemaChanges and the rebuild runner, so unit tests run the real DefaultExecutionGate with prompts that count and never reach a windowless alert.

What each Safe Mode level asks now

Level Save that can lose data (drop column, type change, NOT NULL, add or rewrite CHECK, primary key change, drop index or constraint) Any other ALTER save Table rebuild (SQLite foreign key change, column move) Column move with no rebuild
Silent One sheet Nothing The review only Nothing
Alert, Alert (Full) One sheet One sheet The review only One sheet
Safe Mode, Safe Mode (Full) One sheet, then Touch ID One sheet, then Touch ID The review, then Touch ID One sheet, then Touch ID
Read-Only Refused before anything runs Refused Refused Refused

Before this change a dropped column asked twice at every level, a rebuild asked twice at every level, and Cancel was answered with an error sheet.

Verified

  • StructureSaveConfirmationTests.alterSavesAskOnce runs 8 saves at all 6 levels through the real gate: the confirmation count, whether the sheet is marked destructive, the Touch ID count, and the Read-Only refusal with no prompt. It also covers the rebuild request with and without pre-clear, the review accessors and the failure mapping. StructureEditingSessionTests.cancelAtTheGateKeepsTheEdits: a Cancel returns .refused, the edits stay staged, nothing runs and nothing is shown. ExecutionGateTests checks the denial cause for a Cancel, Read-Only and a declined Touch ID.
  • 15 unit suites: 241 executed, 241 passed.
  • UI, on real SQLite files: StructureTypelessColumnUITests, the new StructureSaveConfirmationUITests (drop at Alert asks once, Cancel stays quiet and keeps the drop staged, foreign key removal and Move Column Down both run from their review with no second sheet), plus StructurePreviewSQLUITests, StructureForeignKeyEditUITests and StructureColumnMoveUITests: 9 cases, all passed.
  • Mutation check: with the generator mark reverted and the rebuild pre-clear set to false, 2 of 11 matrix tests failed and the foreign key UI test failed after 43 s.
  • Codex review, round 1: a P2 that the runnable rebuild review, now the only prompt, had lost the gate's data-loss warning, and a P3 that the docs said an index, key or check removal did not ask at Silent. Both fixed. runnableRebuildReviewWarnsOfDataLoss covers the warning with and without caveats, and previewShowsTheRebuildReadOnly now expects the review, but not the preview, to carry it.
  • Rebased onto main at c21dc512e: 6 unit suites, 114/114. UI: StructureSaveConfirmationUITests, StructureTypelessColumnUITests, StructureForeignKeyEditUITests, StructureColumnMoveUITests and StructurePreviewSQLUITests, 18/18.
  • Lint on all 26 changed Swift files: 0 violations. verify.sh docs: pass.

Deliberately not fixed here

  • The gate's sheet says "(1 statement)" for a multi-statement save, because AlertOperationConfirming passes the joined SQL as one statement for every caller. This predates the change and will be fixed separately.
  • Which window the gate's sheet lands on when a non-key window with a staged drop is closed. The gate already chose windows this way at Alert and above. Not measured here.
  • A Touch ID Cancel still shows "Authentication required…". OperationAuthenticating returns a Bool that cannot tell a Cancel from a Mac with no passcode.
  • For a data-loss save at Silent, the completion time now includes the time spent on the sheet, as it already did at Alert and above.
  • Trigger drop and ClickHouse partition drop or detach have the same double prompt, but they are not saves.

Touch ID levels cannot run in XCUITest, so the stub-prompt matrix covers them.

…or the rebuild review, and stay quiet after Cancel
@mintlify

mintlify Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated
TablePro 🟢 Ready View Preview Sep 26, 2026, 5:37 PM

💡 Tip: Enable Automations to automatically generate PRs for you.

This branch was successfully deployed

1 active deployment
staging - docs — 2349d3e7 Deployed Sep 26, 2026 by mintlify[bot]
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