Skip to content

Conversation

@daesunp
Copy link
Contributor

@daesunp daesunp commented Nov 27, 2025

Description

This PR exposes a user provided "label" object that can be accessed through SchematizingSimpleTreeView.currentTransactionLabel, which can be accessed from the event callback in commitApplied.

@daesunp daesunp requested a review from a team as a code owner November 27, 2025 22:08
@github-actions github-actions bot added area: dds Issues related to distributed data structures area: dds: tree area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct public api change Changes to a public API base: main PRs targeted against main branch labels Nov 27, 2025
@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
    2013 URLs ignored
       0 warnings
       0 errors


Copy link
Contributor

@noencke noencke left a comment

Choose a reason for hiding this comment

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

This is the first half of the feature - adding and setting the property. The second half is to pass the property to the changed and commitApplied event. I think that both halves are small enough changes that you should do them in the same PR. So next:

  1. Create a CommitMetadataAlpha that extends CommitMetadata and has a label: unknown property.
  2. Update both TreeBranch.commitApplied and TreeBranch.changed to use CommitMetadataAlpha.
  3. Update the tests to consume the label from the event commit metadata rather than directly from the static.

* This value is intended to be read within event handlers like `commitApplied`.
* This value is cleared after each transaction to prevent providing stale/incorrect labels.
*/
private static _currentTransactionLabel: unknown | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, there is no benefit to having a private property here and then making a getter and setter, because the getter and setter do no validation or transformation of the value.

I do think we would benefit from constraining the way that this field can be set. How about this:

private static transactionLabel?: unknown;

public static runWithTransactionLabel<T>(label: T, fn: (label: T) => void): void {
  this.transactionLabel = label;
  try {
    fn(label);
  } finally {
    this.transactionLabel = undefined;
  }
}

Then it's impossible for any code to ever accidentally set the label and forget to clear it, or forget to properly handle errors.

The passing of label to the fn and the resulting generic parameter is not strictly necessary but just a nice to have, IMO.

* This value is intended to be read within event handlers like `commitApplied`.
* This value is cleared after each transaction to prevent providing stale/incorrect labels.
*/
private static _currentTransactionLabel: unknown | undefined;
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 this property might work out even better if you put it on TreeCheckout - and it could be a member rather than be static. I know we discussed using a static because it's correct and it's easy - but if a non-static member is just as easy, then non-static is probably better style/convention.

Copilot AI review requested due to automatic review settings December 8, 2025 17:48
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 adds support for labeling transactions to enable undo/redo grouping. It introduces an optional label field in RunTransactionParams that can be accessed through CommitMetadataAlpha.label in the commitApplied event callback.

  • Adds CommitMetadataAlpha interface extending CommitMetadata with an optional label field
  • Updates event signatures to use CommitMetadataAlpha instead of CommitMetadata
  • Implements label storage and propagation through the transaction commit flow
  • Includes comprehensive test coverage for labeled/unlabeled transactions and undo/redo grouping

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/framework/fluid-framework/api-report/fluid-framework.alpha.api.md Adds label field to RunTransactionParams API surface
packages/dds/tree/api-report/tree.alpha.api.md Adds label field to RunTransactionParams API surface
packages/dds/tree/src/core/rebase/types.ts Defines CommitMetadataAlpha interface with optional label field
packages/dds/tree/src/core/rebase/index.ts Exports CommitMetadataAlpha interface
packages/dds/tree/src/core/index.ts Exports CommitMetadataAlpha interface
packages/dds/tree/src/simple-tree/api/transactionTypes.ts Adds label field to RunTransactionParams with documentation
packages/dds/tree/src/simple-tree/api/tree.ts Updates event signatures to use CommitMetadataAlpha
packages/dds/tree/src/shared-tree/treeCheckout.ts Implements label storage and propagation via transactionLabel field and runWithTransactionLabel method
packages/dds/tree/src/shared-tree/schematizingTreeView.ts Updates runTransaction to conditionally call runWithTransactionLabel when label is provided
packages/dds/tree/src/test/shared-tree/schematizingTreeView.spec.ts Adds comprehensive tests for transaction labels and undo/redo grouping

const metaData: CommitMetadataAlpha = {
isLocal: false,
kind: CommitKind.Default,
label: this.transactionLabel,
Copy link

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

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

The transactionLabel should not be included for remote changes. Remote changes come from other clients and should not have the local checkout's transaction label. The label should be undefined for remote changes.

Suggested change
label: this.transactionLabel,
label: undefined,

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: dds: tree area: dds Issues related to distributed data structures area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct base: main PRs targeted against main branch public api change Changes to a public API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants