Interval Collection: local rollback#24307
Conversation
…to ic/encapsulate-properties
…to ic/encapsulate-properties
…to mt/tie-split-rollback
…to mt/tie-split-rollback
…to ic/local-rollback
…to ic/local-rollback
github-actions
Bot
added
area: dds
…to ic/local-rollback
There was a problem hiding this comment.
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
packages/dds/sequence/src/intervals/sequenceInterval.ts:225
- [nitpick] Consider providing an explicit default value (e.g., false) for the 'rollback' parameter to ensure consistent behavior when it is not provided.
rollback?: boolean,
packages/dds/sequence/src/intervalCollection.ts:1414
- [nitpick] Double-check that the 'rollback' flag is consistently handled in collaboration scenarios to prevent unintended delta submissions during rollback operations.
if (this.isCollaborating && rollback !== true) {
…to ic/local-rollback
| switch (opName) { | ||
| case "add": { | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| const interval = this.getIntervalById(id)!; |
There was a problem hiding this comment.
Maybe assert that the interval exists instead of a non-null assertion?
There was a problem hiding this comment.
nice catch. there were actually some other problems on this code path, so improved delete overall.
| await assertPropertiesEqual(a, b); | ||
| const firstLabels = Array.from(a.getIntervalCollectionLabels()).sort(); | ||
| const otherLabels = Array.from(b.getIntervalCollectionLabels()).sort(); | ||
| assert.deepEqual( |
There was a problem hiding this comment.
What's the reason for removing this?
There was a problem hiding this comment.
collections are created lazily, there is not specific op. so when we see any op under a collection we just create it, so for rollback we might create a collection, but then not send any op, so the collection only exists on that one client, but it doesn't matter that it is exists, as if anyone ever adds something to it in an op, all clients will just agree it exists.
There was a problem hiding this comment.
in fact the call below, getIntervalCollection will create the interval collection locally too, so its not even just ops that create it.
| ...[ | ||
| 3, 4, 9, 10, 13, 21, 27, 35, 37, 38, 39, 40, 47, 48, 49, 63, 68, 71, 74, 87, 90, 92, | ||
| 96, 98, | ||
| ], // Can't rollback attach message, |
There was a problem hiding this comment.
Can you link the relevant work item here?
There was a problem hiding this comment.
this will be fixed by the staging mode feature
There was a problem hiding this comment.
…to ic/local-rollback
|
|
||
| this.localSeqToSerializedInterval.set(localSeq, serializedInterval); | ||
| } | ||
| this.addPendingChange(id, serializedInterval); |
There was a problem hiding this comment.
ah. was going to ask what's going on here but I see that our removePendingChange implementation is probably overly tolerant to inconsistent add/removes (specifically it's ok with calls to removePendingChange that had no corresponding add) which probably had some lurking bugs with more complex series of changes...
There was a problem hiding this comment.
yeah. the existing code is all over the place. we leak pending state, we leak local references. so far i've taken a light approach to fixing those things to reduce churn. as we build out rollback in the presence of remote changes i expect to improve/fix a bunch of this
This change adds rollback functionality to interval collections, and fixes a bug in sequence: