-
Notifications
You must be signed in to change notification settings - Fork 561
Add label for transaction commits for undo/redo grouping. #25938
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
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
noencke
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.
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
CommitMetadataAlphathat extendsCommitMetadataand has alabel: unknownproperty. - Update both
TreeBranch.commitAppliedandTreeBranch.changedto useCommitMetadataAlpha. - 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; |
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.
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; |
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 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.
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 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
CommitMetadataAlphainterface extendingCommitMetadatawith an optionallabelfield - Updates event signatures to use
CommitMetadataAlphainstead ofCommitMetadata - 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, |
Copilot
AI
Dec 8, 2025
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 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.
| label: this.transactionLabel, | |
| label: undefined, |
Description
This PR exposes a user provided "label" object that can be accessed through
SchematizingSimpleTreeView.currentTransactionLabel, which can be accessed from the event callback incommitApplied.