-
Notifications
You must be signed in to change notification settings - Fork 561
refactor(tree): support more general revision replacement #26106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the revision replacement system to support more general scenarios by introducing a new RevisionReplacer interface. The refactoring addresses two key limitations: handling inputs where the same localId appears in different revisions, and maintaining consistent ChangeAtomId mappings across multiple modular changesets. These improvements are necessary to support merging and reverting within open transactions.
Key Changes
- Introduced a new
RevisionReplacerinterface andDefaultRevisionReplacerimplementation that intelligently handles local ID conflicts during revision replacement - Updated the
changeRevisionmethod signature across all change rebasers to accept aRevisionReplacerparameter instead of separate old/new revision parameters - Removed the
replaceAtomRevisionsutility function in favor of the new replacer pattern
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/dds/tree/src/core/rebase/changeRebaser.ts | Defines new RevisionReplacer interface and updates ChangeRebaser.changeRevision signature |
| packages/dds/tree/src/core/rebase/types.ts | Removes deprecated replaceAtomRevisions utility function |
| packages/dds/tree/src/feature-libraries/modular-schema/defaultRevisionReplacer.ts | Implements DefaultRevisionReplacer with conflict detection and resolution logic |
| packages/dds/tree/src/feature-libraries/modular-schema/modularChangeFamily.ts | Updates changeRevision to use new replacer pattern across all changeset types |
| packages/dds/tree/src/feature-libraries/sequence-field/replaceRevisions.ts | Refactors to use RevisionReplacer interface for updating marks and IDs |
| packages/dds/tree/src/feature-libraries/optional-field/optionalField.ts | Adapts replaceRevisions to work with new replacer pattern |
| packages/dds/tree/src/feature-libraries/modular-schema/genericFieldKind.ts | Updates generic field kind to use replacer for atom ID updates |
| packages/dds/tree/src/shared-tree/sharedTreeChangeFamily.ts | Simplifies changeRevision implementation using the new replacer |
| packages/dds/tree/src/test/shared-tree/sharedTreeChangeFamily.spec.ts | Adds comprehensive tests for local ID collision handling and consistency |
| packages/dds/tree/src/test/feature-libraries/modular-schema/defaultRevisionReplacer.spec.ts | New test file with full coverage of replacer functionality |
| Multiple test utility files | Updates test utilities to create and use DefaultRevisionReplacer instances |
Description
This PR addresses two limitations of the current revision replacement capabilities:
localIdvalue is used in the context of different revisions.ChangeAtomIdinput value to the sameChangeAtomIdoutput value across multiple modular changesets.See the new tests in src/test/shared-tree/sharedTreeChangeFamily.spec.ts for more details on the relevant scenarios.
Motivation
Both limitations need to be addressed in order for merging and reverting to be supported within open transactions.
Breaking Changes
None