Conversation
…Manager Phase B of the data grid clean-architecture refactor. Replaces six loose collections (changes, changeIndex, deletedRowIndices, insertedRowIndices, modifiedCells, insertedRowData, changedRowIndices) with a single PendingChanges value type that owns the cross-collection invariants. DataChangeManager shrinks from ~960 LOC to ~190 LOC: it keeps undo/redo registration, plugin SQL generation, and observation, while delegating all state mutation to pending.recordCellChange / recordRowDeletion / etc. Also renamed TabPendingChanges -> TabChangeSnapshot since it's a snapshot DTO for tab persistence, distinct from the live PendingChanges tracker. PendingChanges has 18 unit tests covering record / undo / replay / snapshot round-trip / clear / consume.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 734733c11c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+242
to
+246
| if let updateChange = pending.change(forRow: rowIndex, type: .update) { | ||
| if updateChange.cellChanges.contains(where: { $0.columnIndex == columnIndex }) { | ||
| pending.revertUpdateCell( | ||
| rowIndex: rowIndex, columnIndex: columnIndex, | ||
| columnName: columnName, previousValue: previousValue |
There was a problem hiding this comment.
applyCellEditUndo mutates pending but never records rowIndex in the changed-row set, so consumeChangedRowIndices() can return empty after undo/redo. In DataGridView.reloadAndSyncSelection, version-only updates reload rows only from that set and skip reload when hasChanges is still true, so undoing one cell while other pending edits exist leaves the visible row stale until a later full reload.
Useful? React with 👍 / 👎.
Comment on lines
+190
to
+194
| let cellChange = CellChange( | ||
| rowIndex: rowIndex, | ||
| columnIndex: columnIndex, | ||
| columnName: columnName, | ||
| oldValue: nil, |
There was a problem hiding this comment.
reapplyCellChange hard-codes oldValue: nil when rebuilding an update after an undo/redo transition. For a normal edit (A -> B), undo removes the update; redo recreates it with oldValue=nil; then the next undo can no longer collapse back to clean state because it compares against nil instead of A, leaving a phantom pending update and incorrect SQL diff tracking.
Useful? React with 👍 / 👎.
- Restore old behavior in applyCellEditUndo: when an update change exists for the row but has no cellChange for the column, leave the update unchanged. The new code was creating a stale cellChange entry. - Guard recordRowDeletion / reapplyRowDeletion / recordRowInsertion against double-recording. Two calls for the same row no longer corrupt the changeIndex. - Add MARK groupings: cancel pending vs. replay (NSUndoManager-driven). - Two new tests covering double-deletion / double-insertion idempotency.
Phase B regression: undoRowDeletion / undoRowInsertion / undoBatchRowInsertion and the replay helpers (reapplyRowDeletion, reapplyCellChange, revertUpdateCell, updateInsertedCellDirectly, reinsertRow, reinsertBatch) lost the changedRowIndices.insert that the original DataChangeManager called at the end of every undo path. Without it, consumeChangedRowIndices returned empty after undo. The data grid's reloadAndSyncSelection then fell through to the !hasChanges fallback, doing a full reloadData over all rows. With 1000 rows this was 33ms per undo. With larger datasets it gets worse. Trace before this fix (cellEdit undo to clean state): applyDataUndo END mutate=0.08ms callback=0.06ms total=0.19ms reloadAndSync VERSION_CHANGED no changes -> full reload reloadAndSync total=33.2ms updateNSView reloadVersion=8 elapsed=33.5ms After: changedRows is non-empty, partial reload of just the affected row fires. Six new regression tests cover each replay method.
…ollapses
User-found bug: edit cell -> undo -> redo -> undo. Value reverts visually but
yellow modified-bg persists.
Root cause: reapplyCellChange (called when redo applies an edit and no pending
change exists for that row) was creating a CellChange with oldValue=nil. When
the next undo went through revertUpdateCell, it compared previousValue (the
original DB value) to the cellChange's oldValue (nil) and they didn't match,
so it took the "update inline" branch instead of the "collapse" branch.
modifiedCells stayed populated.
Fix: reapplyCellChange now takes an `originalDBValue` parameter and stores it
as the cellChange's oldValue. The DataChangeManager call site passes the
action's `newValue` (which is the original DB value in the redo direction).
This matches the OLD recordCellChangeForRedo semantics.
Trace through the failing scenario after this fix:
edit A->B : cellChange(old=A, new=B), modified[0]={1}
undo : remove cellChange, modified[0]=removed
redo : reapplyCellChange creates cellChange(old=A, new=B), modified[0]={1} (was nil before)
undo (second time) : revertUpdateCell sees old=A, prevValue=A -> match -> COLLAPSE
modified[0]=removed (was stuck before)
Three new tests:
- reapplyCellPreservesOriginalDBValue
- editUndoRedoUndoCollapses (the exact user-reported scenario)
- updated reapplyCellChange signature in existing tests
- undoRowInsertion and undoBatchRowInsertion now bump reloadVersion so direct callers refresh the grid (matched the pattern in undoRowDeletion / recordRowInsertion). - Correct DataChangeManagerExtendedTests.recordDeletionForAlreadyDeletedRow assertion: PendingChanges guards duplicate deletions, so changes.count stays at 1 (matches PendingChangesTests.doubleDeletionIsIdempotent). - Drop two explanatory comments from applyCellEditUndo per the no-comments rule. - CHANGELOG entry for the Phase B PendingChanges extraction. Reviewer also flagged shiftRowIndicesDown / undoBatchRowInsertion as missing deletedRowIndices and modifiedCells shifts. Investigated: RowOperationsManager always inserts at the tail (addNewRow, duplicate, paste all use resultRows.count after append), and recordRowInsertion does not call shiftRowIndicesUp. The down-shift is symmetric with the record path, so adding the deletedRowIndices shift would diverge from recordRowInsertion. Skipping pending verification of the in-the-middle insertion scenario reviewer assumed.
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
1 participant
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.
Summary
Phase B of the data grid clean-architecture refactor. Consolidates six loose collections in
DataChangeManagerinto a singlePendingChangesvalue type that owns cross-collection invariants.Why
DataChangeManagerwas a ~960-line god class managing six separate collections that all describe the same conceptual thing ("the user's pending edits"):changes: [RowChange]changeIndex: [RowChangeKey: Int](lookup cache)deletedRowIndices: Set<Int>insertedRowIndices: Set<Int>modifiedCells: [Int: Set<Int>]insertedRowData: [Int: [String?]]changedRowIndices: Set<Int>Mutation paths had to manually keep all six in sync. The class also handled undo/redo registration, plugin SQL generation, and observation. Past bugs (cross-tab desync, modifiedCells out-of-sync with changes) traced back to one path forgetting to update one collection.
What
PendingChangesvalue type (Core/ChangeTracking/PendingChanges.swift, 513 LOC)private(set)recordCellChange,recordRowDeletion,recordRowInsertion,undoRowDeletion,undoRowInsertion,undoBatchRowInsertion,reapplyRowDeletion,reapplyCellChange,reinsertRow,reinsertBatch,revertUpdateCell)isRowDeleted,isRowInserted,isCellModified,modifiedColumns(forRow:),change(forRow:type:))clear,consumeChangedRowIndices,snapshot,restore)DataChangeManagershrinks from ~960 → ~190 LOC@Observableintegration,lastUndoResult,onUndoAppliedpending: PendingChangesapplyDataUndosplit into 5 focused per-action helpers (applyCellEditUndo,applyRowInsertionUndo,applyRowDeletionUndo,applyBatchRowDeletionUndo,applyBatchRowInsertionUndo)TabPendingChanges→TabChangeSnapshot(it's a serialization DTO, distinct from the livePendingChangestracker)Tests
18 new unit tests for
PendingChangescovering:Existing
DataChangeManagerExtendedTestsupdated: helper now wires upundoManagerProvider(was already broken since #924 due to the Phase 3 architecture change but not noticed because the helper was missing).Files
Core/ChangeTracking/PendingChanges.swift(new, 513 LOC)Core/ChangeTracking/DataChangeManager.swift(962 → ~190 LOC)Core/ChangeTracking/DataChangeModels.swift(comment update)Models/Query/QueryTabState.swift(renameTabPendingChanges→TabChangeSnapshot)Models/Query/QueryTab.swift,QueryTabManager.swift(rename usage)Views/Main/Extensions/MainContentCoordinator+Discard.swift,+SaveChanges.swift(rename usage)TableProTests/Core/ChangeTracking/PendingChangesTests.swift(new)TableProTests/Core/ChangeTracking/DataChangeManagerExtendedTests.swift(helper fix)TableProTests/Core/ChangeTracking/DataChangeManagerTests.swift(helper fix)TableProTests/Core/ChangeTracking/DataChangeModelsTests.swift(rename usage)Test plan
TabChangeSnapshot