fix(datagrid): repair the six defects the data rewind investigation found in the save path

[datlechin]

Stacked on #2440. Review that one first; this branch is based on it, not on main.

These are the six defects the Data Rewind investigation turned up in the grid save path and deliberately left out of that PR. Four were verified there; the other two are verified here and both are real.

1 and 2. PendingChanges renumbered a different subset of its state in each of three places

PendingChanges keys six things by row index: changes[].rowIndex, changeIndex, insertedRowIndices, deletedRowIndices, insertedRowData and modifiedCells. Three functions renumbered them, and each handled a different subset:

	shiftRowIndicesUp	shiftRowIndicesDown	undoBatchRowInsertion
changes[].rowIndex	yes	yes	yes
insertedRowIndices	yes	yes	yes
deletedRowIndices	yes	no	no
insertedRowData	yes	yes	no
modifiedCells	yes	yes	no

The insertedRowData gap is the live one. Paste three rows, delete the first, press Save: the survivors' values are still filed under their old indices, so generateInsertSQL finds nothing at the index it is given, falls back to cellChanges (empty for a pasted row) and returns nil. Nothing guards INSERT the way UPDATE and DELETE are guarded, so one row is written with another row's values, the third is dropped, and Save reports success.

The same function also rebuilt the returned values from cellChanges, which holds only the columns the user typed into, compacted. Add a row, type "Bob" into the third column, delete it, Cmd+Z: Bob comes back in the first column and the DEFAULT markers are gone.

Fixed by collapsing the three renumbering paths into one reindex(_:) that touches all six, so a collection cannot be added without being wired in, and by reading the values from insertedRowData, which is what savedInsertedValues already treats as the source of truth.

The deletedRowIndices and modifiedCells arms are not reachable today, because a new row always lands at the end of the grid and nothing pending is ever below one. They are covered anyway, and the code says why: the alternative is three paths covering three subsets again.

3. Undo wrote through a display row as if it were a storage index

PendingChanges is keyed by display position for cell edits and existing-row deletes, and by storage index for inserted rows, which is what lets an inserted row be physically removed. applyUndoResult's .cellEdit arm passed its display row straight into TableRows.edit, which indexes storage. The forward write converts two lines earlier; the undo did not. With a per-column value filter narrowing the grid the two diverge and undo reverts whichever row sits at that storage offset.

Fixed by resolving through DisplayRowMapping in that arm, the same way deleteFilteredRows already does. The row arms are left alone: their index is storage-native from the start, so converting them would break what works.

I reported lines 199 and 223 as having the same defect. They do not; they consume storage-native indices. Correcting that here.

4. Statements were grouped by verb, so a delete could not free a value for an insert

generateAttributedStatements bucketed updates and deletes and flushed them after the walk, emitting every INSERT, then every UPDATE, then every DELETE. On a table with UNIQUE(email), deleting the row holding a@b.com and adding a new row with that email is a valid end state that came out as a constraint violation.

Rewritten as one ordered pass. Deletes still batch, but only across a consecutive run, so "select 500 rows, press Delete" is still one statement while a delete separated from another by an insert stays on its own side of it.

Ordering could not come from the array: removeChangeAt swap-removes with the last element, so cancelling any earlier change relocates a later one into its slot. RowChange now carries a monotonic sequence, stamped in appendChange, and the generator walks in that order. Without it the fix would have held only until the first undo.

5. A partial commit was reported as a total failure

Verified here. DataWriteExecutor collected a result per statement and then dropped them all on the way out. On the thirteen drivers with no transactions, the save's catch marked every statement failed in history including the ones that had committed, kept every row pending, and told the user it failed. Pressing Save again re-ran the committed INSERTs and duplicated the rows.

DataWritePartialCommitError now carries the committed results out, thrown when there was no transaction to roll back or when the rollback itself failed and the committed extent is unknown. The save records the committed statements as the successes they were, records only the one that failed, and shows Save Incomplete naming how many of how many landed and warning that saving again writes them twice. It does not reduce the change set: a DataWriteStep carries no row identity to do that safely, and claiming a precision the data does not support is how this went wrong the first time.

6. "Ignore foreign key checks" did nothing on three engines

Verified here. SQLite, libSQL and Cloudflare D1 disable foreign keys with PRAGMA foreign_keys = OFF, which SQLite documents as a no-op inside a transaction, and the executor opened the transaction before step 0. SQLite is supportsForeignKeyDisable: true and supportsTransactions: true, so the option was reachable and inert. The re-enable never ran on the success path at all.

DataWritePlan gained a prologue beside its epilogue. The toggles are no longer steps: the prologue runs before BEGIN, the epilogue after COMMIT and after ROLLBACK alike. MySQL's SET FOREIGN_KEY_CHECKS is session-scoped, so outside the transaction is correct there too, which is why this is a placement fix rather than a new per-engine capability. displayStatements composes prologue, steps and epilogue, so Preview SQL and the authorization gate still show the toggles the user is approving, and the executor reports the statements it ran around the transaction so they still reach query history.

Also

CellChange.rowIndex is gone. It was set at 14 construction sites and read nowhere, and no renumbering path ever touched it, so a contributor wiring new logic to it would have found it silently stale.

Verification

Step	Result
build	PASS
test, 26 suites	PASS, 322 of 322
swiftlint --strict, app, plugins and tests	0 violations

New tests: PendingChangesReindexTests and PendingChangesSequenceTests, SQLStatementGeneratorOrderingTests (five cases including that order survives the swap-remove, and that an interrupted delete run splits), UndoRowIndexTests, and six more in DataWriteExecutorTests for the toggle placement, the rollback path and the partial-commit error. DataWriteExecutorTests now has one order-recording driver for every case in the file rather than two ad hoc doubles.

Run swiftlint from inside the worktree rather than through verify.sh lint with relative paths, which resolves them against the main checkout and reported clean while three violations stood.

Refs #2107

https://claude.ai/code/session_01KsqHrFwJxUW6eWozYjT8JZ

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.