Skip to content

Conversation

@noencke
Copy link
Contributor

@noencke noencke commented Dec 5, 2025

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.

Copilot AI review requested due to automatic review settings December 5, 2025 20:08
@noencke noencke requested a review from a team as a code owner December 5, 2025 20:08
Copy link
Contributor

Copilot AI left a 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 ChangeMetadata type that extends CommitMetadata with a getChange() method for local changes
  • Adds applyChange() method to TreeBranchAlpha interface for applying serialized changes
  • Updates event signatures to use ChangeMetadata instead of CommitMetadata

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

* 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.
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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".

Copy link
Contributor

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"?

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

@yann-achard-MS yann-achard-MS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test needs updating

@github-actions
Copy link
Contributor

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> fluid-framework-docs-site@0.0.0 ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> fluid-framework-docs-site@0.0.0 serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> fluid-framework-docs-site@0.0.0 check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  243138 links
    1776 destination URLs
    2015 URLs ignored
       0 warnings
       0 errors


@jenn-le jenn-le merged commit 50b5ecd into microsoft:main Dec 10, 2025
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants