-
Notifications
You must be signed in to change notification settings - Fork 560
Add serializable SharedTree change #25992
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
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 introduces an alpha API for capturing and applying serialized tree changes, enabling AI sandboxing scenarios with the tree-agent package. Users can now capture an opaque serialized version of tree changes via the getChange() method in the "changed" event, and apply it to different branches that share the same head commit.
Key changes:
- Introduces
ChangeMetadatatype that extendsCommitMetadatawith agetChange()method for local changes - Adds
applyChange()method toTreeBranchAlphainterface for applying serialized changes - Updates event signatures to use
ChangeMetadatainstead ofCommitMetadata
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/dds/tree/src/core/rebase/types.ts | Defines new ChangeMetadata type with conditional getChange() method based on isLocal flag |
| packages/dds/tree/src/core/rebase/index.ts | Exports the new ChangeMetadata type |
| packages/dds/tree/src/core/index.ts | Re-exports ChangeMetadata and ChangeEncodingContext from core module |
| packages/dds/tree/src/index.ts | Exports ChangeMetadata type in public API surface |
| packages/dds/tree/src/simple-tree/api/tree.ts | Adds applyChange() method to TreeBranchAlpha interface and updates TreeBranchEvents signature |
| packages/dds/tree/src/shared-tree/treeCheckout.ts | Implements serialization/deserialization logic with validation and branch state checking |
| packages/dds/tree/src/shared-tree/schematizingTreeView.ts | Delegates applyChange() to underlying checkout |
| packages/dds/tree/src/shared-tree-core/index.ts | Exports additional message codec types |
| packages/dds/tree/src/test/simple-tree/api/tree.spec.ts | Adds comprehensive tests for applying changes across branches and error scenarios |
| packages/dds/tree/api-report/tree.alpha.api.md | Updates API report with new alpha types and methods |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| * Apply a serialized change to this branch. | ||
| * @param change - the change to apply. | ||
| * Changes are acquired via `getChange` in a branch's {@link TreeBranchEvents.changed | "changed"} event. | ||
| * @remarks Changes may only be applied to a SharedTree with the same IdCompressor session ID and the same branch state from which they were generated. |
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.
Is "the same IdCompressor session ID" really enough? It seems that the implementation relies on the ID compressor instance being shared by the two branches, which seems like a stricter requirement.
I'm also curious how much value this new API is adding if the restriction is so tight. How will this be used in a way that normal branching and merging couldn't be used?
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.
The purpose of this api is to allow changes to be passed over a sandbox boundary, where normal branching and merging wouldn't be possible. The requirement for same id compressor session id because the approach we're planning is to essentially recreate an idcompressor in the sandbox for allocating ids and burning some ids on the original to allow for this.
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.
Is "the same IdCompressor session ID" really enough though?
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.
I think it should be, but we can also make sure with Noah when he's back. The session id will be shared between the id compressor instances and I don't think there is any other information that needs to be kept consistent between the original and sandboxed views when it comes to changes. If a change is passed in that has the same idCompressor session id but is an invalid change, the application itself should fail. If the id is the same and the change is valid, there's no reason we can't apply it, is there?
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.
The potential reason we wouldn't be able to apply is if the ID compressor instance on the view where the change needs to be applied needs to first be notified so that in can adopt the new IDs generated by the other view.
If sharing the session ID is enough then it would be good to update the test so that it uses different compressor ID instances with the same session ID. I'm not sure how to do that, be presumably the Boards sandbox code will be doing that.
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.
Ah I see, this may not be sufficient then. That's why we're planning to burn ids in advance but there's nothing about this that actually checks that the appropriate ids have been burned and I'm not sure exactly what will happen in that case. I would expect that change application to just fail but it probably wouldn't fail with a super useful message. I'll look into this and probably add a note but I don't think the pr needs to be blocked on this, as long as we do error in some way.
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.
It seems bad to have the API state a weaker requirement than what is really needed, so I think we should update the docs here to say "same ID Compressor instance".
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.
I think it makes sense to update the docs but "same instance" isn't entirely accurate since we're going to be generating from a different instance in most cases. How would you feel about "same ID Compressor state"?
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.
I don't know enough about the ID compressor to know what will or won't work, but if you're going to be generating from a different instance then you're going to be doing something that isn't covered by the current test in this PR. So, either the current test needs to be updated so that the views use different ID compressor instances, or the requirements need be tightened to say that the same ID compressor instance must be used. Based on what you're saying, it sounds like the test needs to be updated.
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.
I'll update the comment for now and leave a todo. I don't think updating the test is currently possible because the scenario I'm describing is what I'm going to be implementing next. I'll update the test and revert the comment along with that work.
yann-achard-MS
left a comment
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.
Test needs updating
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
This introduces an alpha API that allows a user to capture a serialized version of tree changes whenever the view is changed. That (opaque) object can then be applied to a different tree branch later, as long as that branch has the same head commit as the originating branch. This API is necessary to support AI sandboxing scenarios using the tree-agent package.