Add label for transaction commits for undo/redo grouping. #25938

daesunp · 2025-11-27T22:08:15Z

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.

noencke

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:

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

noencke · 2025-12-02T20:09:32Z

packages/dds/tree/src/shared-tree/schematizingTreeView.ts

+	 * 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;


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.

I know we discussed about making this static yesterday, but unfortunately ran into a circular dependency issue when calling TreeCheckout.runWithTransactionLabel in schematizingTreeView.ts.

I left both as non-static members for now, but would making transactionLabel static, and runWithTransactionLabel non-static be better?

Sorry - in our latest conversation - I meant that you could make the property static, but not the method, for a slight performance benefit. The method is "already static" in that sense because it lives on the prototype, not the instances.

noencke · 2025-12-02T20:25:00Z

packages/dds/tree/src/shared-tree/schematizingTreeView.ts

+	 * 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;


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

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

Copilot · 2025-12-08T17:52:51Z

packages/dds/tree/src/shared-tree/treeCheckout.ts

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


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,

github-actions · 2025-12-23T19:22:43Z

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] 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"


> [email protected] serve
> docusaurus serve --no-open

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

> [email protected] check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  245432 links
    1786 destination URLs
    2025 URLs ignored
       0 warnings
       0 errors

noencke · 2025-12-23T19:24:10Z

packages/dds/tree/src/shared-tree/schematizingTreeView.ts

-		return value !== undefined
-			? { success: true, value: value as TSuccessValue }
-			: { success: true };
+		return this.checkout.runWithTransactionLabel(() => executeTransaction(), params?.label);


I would slightly prefer that you just wrap directly with the function rather than binding the lamda to a variable. Since it's only used once. Up to you though.

runWithTransactionLabel(() => { // ... });

rather than

const executeTransaction = () => { // ... }; runWithTransactionLabel() => executeTransaction);

noencke · 2025-12-23T19:26:35Z

packages/dds/tree/src/shared-tree/treeCheckout.ts

+	): TResult {
+		// If a transaction label is already set, nesting is occurring, so we should not override it
+		if (this.transactionLabel !== undefined) {
+			return fn(label);


We want to do fn(this.transactionLabel) here, not fn(label).

noencke · 2025-12-23T19:27:59Z

packages/dds/tree/src/core/rebase/types.ts

 		| {
 				readonly isLocal: false;
 				readonly getChange?: undefined;
+				label?: unknown;


We don't want a label here, right? Since there are never labels for remote changes. Let's get some more mileage out of this discriminated union. You can either remove the property (users will have to check isLocal before they can check the label), or you can make it label?: never. The latter is probably more user friendly because of not having to check for isLocal.

noencke · 2025-12-23T19:30:27Z

packages/dds/tree/src/simple-tree/api/transactionTypes.ts

+	 * An optional user-defined label for this transaction.
+	 *
+	 * This label is associated with the commit produced by this transaction, and is surfaced through the
+	 * `label` property of {@link ChangeMetadata} in the `commitApplied` event.


Don't forget the changed event, too.

noencke · 2025-12-23T19:31:35Z

packages/dds/tree/src/simple-tree/api/tree.ts

 	 * - Constraints will apply to the outermost transaction. Constraints are applied per commit and there will be one commit generated
 	 * for the outermost transaction which includes all inner transactions.
 	 * - Undo will undo the outermost transaction and all inner transactions.
+	 * - If a label is provided in the params, it will be used for the outermost transaction.


This makes it sound like providing a label to an inner transaction might possibly override an existing label for the outer transaction. What we want to say is something like "only the label for the outermost transaction does anything, we ignore all labels for inner/nested transactions".

noencke · 2025-12-24T04:56:27Z

Oh and don't forget to add a changeset for this one! The changeset should have a nice example of how to use the new functionality with undo/redo.

Add label for transaction commits

b56b3de

daesunp requested a review from a team as a code owner November 27, 2025 22:08

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

noencke reviewed Dec 2, 2025

View reviewed changes

PR review

43c539f

Copilot AI review requested due to automatic review settings December 8, 2025 17:48

Copilot started reviewing on behalf of daesunp December 8, 2025 17:48 View session

Copilot AI reviewed Dec 8, 2025

View reviewed changes

daesunp added 3 commits December 17, 2025 16:26

Merge branch 'main' into undo-redo-correlation-id

4e4d77f

fix after merge

9b860f3

PR review

cf68d66

noencke reviewed Dec 23, 2025

View reviewed changes

Add label for transaction commits for undo/redo grouping. #25938

Are you sure you want to change the base?

Add label for transaction commits for undo/redo grouping. #25938

Uh oh!

Conversation

daesunp commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

noencke left a comment

Choose a reason for hiding this comment

Uh oh!

noencke Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

daesunp Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

noencke Dec 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noencke Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 23, 2025

linkcheck output

Uh oh!

noencke Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

noencke Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

noencke Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

noencke Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

noencke Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noencke commented Dec 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

daesunp commented Nov 27, 2025 •

edited

Loading

noencke Dec 24, 2025 •

edited

Loading

noencke Dec 23, 2025 •

edited

Loading