Skip to content

Commit 618fc4c

Browse files
authored
refactor(datagrid): extract PendingChanges value type from DataChangeManager (#928)
* refactor(datagrid): extract PendingChanges value type from DataChangeManager 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. * refactor(datagrid): address PendingChanges review feedback - 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. * fix(datagrid): undo replay paths now mark affected rows as changed 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. * fix(datagrid): preserve original DB value through redo so next undo collapses 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 * fix(datagrid): apply PendingChanges review fixes - 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.
1 parent 5486fdf commit 618fc4c

13 files changed

Lines changed: 1074 additions & 643 deletions

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
2222

2323
### Changed
2424

25+
- DataChangeManager extracted a PendingChanges value type that owns cross-collection invariants for cell edits, row insertions, and deletions. DataChangeManager kept undo/redo registration, plugin SQL generation, and the `@Observable` boundary, dropping from ~960 to ~190 lines. The serialization DTO `TabPendingChanges` is renamed to `TabChangeSnapshot` to distinguish it from the live tracker.
2526
- AnyChangeManager uses ChangeManaging protocol instead of closure-based type erasure, removing all runtime `[Any]` downcasts
2627
- Row selection state moved from MainContentView @State to GridSelectionState @Observable class, preventing full view tree invalidation on every row click
2728
- QueryTab decomposed into focused sub-types: TabExecutionState, TabTableContext, TabQueryContent, TabDisplayState

0 commit comments

Comments
 (0)