fix(tree): Constrain column removal on rows like we constrain row removal on columns#27226
fix(tree): Constrain column removal on rows like we constrain row removal on columns#27226Josmithr wants to merge 1 commit intomicrosoft:mainfrom
Conversation
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (48 lines, 2 files), I've queued these reviewers:
How this works
|
There was a problem hiding this comment.
Pull request overview
This PR tightens Table.removeColumns() undo behavior so that reverting a column removal is dropped if doing so would reintroduce cells for rows that have since been removed (mirroring the existing constraint pattern in removeRows()).
Changes:
- Add
preconditionsOnRevertconstraints toremoveColumns()to require the relevant rows still exist when the column-removal is undone. - Add a unit test capturing the cross-client scenario where a row removal would otherwise orphan cells when a column removal is undone.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/dds/tree/src/tableSchema.ts | Adds row-existence constraints on revert for removeColumns() transactions to prevent orphaned cells after undo. |
| packages/dds/tree/src/test/tableSchema.spec.ts | Adds a regression test for dropping undo of removeColumns() when a row is concurrently removed. |
| // Relevant invariant: each cell corresponds to an existing row and column | ||
| // Adding a constraint on rows here to prevent cells being orphaned. The relevant scenario is: | ||
| // Client A removes columns | ||
| // Client B (either concurrently or not, so long as B's edit is sequenced after A's edit) removes a row, | ||
| // Client A reverts the removal of the columns | ||
| // TODO: Replace with "no detach on revert" constraint on the row array when available. | ||
| const rowConstraints: TransactionConstraintAlpha[] = this.table.rows.map((row) => ({ | ||
| type: "nodeInDocument", | ||
| node: row as RowValueType, | ||
| })); |
See new unit test for the scenario being captured.