Add composable labels for transactions#26405
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a composable LabelTree data structure to track labels from nested transactions, providing a hierarchical representation of transaction labels instead of the previous flat, outermost-only label.
Changes:
- Added a new
LabelTree<T>class that represents an immutable tree of labels with Set-like operations (has,values,forEach,size) - Extended
LocalChangeMetadataandRemoteChangeMetadatainterfaces with alabelsfield containing the composed label tree - Modified transaction lifecycle in
TreeCheckoutandSchematizingSimpleTreeViewto automatically compose label trees from nested transaction labels usingpushLabelFrameandpopLabelFramemethods
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/dds/tree/src/util/labelTree.ts | New immutable tree data structure for composing labels with Set-like operations |
| packages/dds/tree/src/util/index.ts | Export the new LabelTree class |
| packages/dds/tree/src/test/util/labelTree.spec.ts | Comprehensive test suite for LabelTree functionality |
| packages/dds/tree/src/test/shared-tree/schematizingTreeView.spec.ts | Integration tests for label tree composition in nested transactions |
| packages/dds/tree/src/shared-tree/treeCheckout.ts | Implementation of label frame stack and label tree composition logic |
| packages/dds/tree/src/shared-tree/schematizingTreeView.ts | Integration of label frame push/pop with transaction lifecycle |
| packages/dds/tree/src/index.ts | Public API export of LabelTree |
| packages/dds/tree/src/core/rebase/types.ts | Extended metadata interfaces with labels field |
| packages/dds/tree/api-report/tree.alpha.api.md | API surface documentation for new LabelTree and labels metadata |
| assert.equal(receivedLabels.children.length, 1); | ||
| assert.equal(receivedLabels.children[0].label, "inner"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
There is no test coverage for transaction labels when a transaction is aborted (rollback). Consider adding a test case to verify that labels are properly discarded when a labeled transaction is rolled back, ensuring that aborted transactions don't pollute the label tree.
| // Outermost level — set the composed tree for the commit metadata. | ||
| this.transactionLabels = node; | ||
| } else { | ||
| parent.children.push(node); |
There was a problem hiding this comment.
This code mutates a children array that will become part of a LabelTree, but LabelTree is documented as immutable and has a readonly children field. Mutating this array after passing it to LabelTree's constructor violates the immutability contract. Consider creating a new array when constructing each LabelTree node to ensure true immutability.
| } | ||
| } else if (parent !== undefined) { | ||
| // No label at this level — promote children to parent. | ||
| parent.children.push(...frame.children); |
There was a problem hiding this comment.
This code mutates a children array that will become part of a LabelTree, but LabelTree is documented as immutable and has a readonly children field. Mutating this array after passing it to LabelTree's constructor violates the immutability contract. Consider creating a new array when constructing each LabelTree node to ensure true immutability.
| * a node, with inner transaction labels becoming children of outer ones. | ||
| * | ||
| * @remarks | ||
| * This is only defined when the outermost transaction has a label. |
There was a problem hiding this comment.
The documentation states "This is only defined when the outermost transaction has a label," but this is inaccurate. Based on the test case "inner labels are surfaced with undefined root when outer transaction has no label" and the implementation in popLabelFrame (lines 518-521 of treeCheckout.ts), labels can also be defined when the outermost transaction has no label but inner transactions do have labels (in which case a LabelTree with undefined root is created). The documentation should be updated to reflect this behavior.
| * This is only defined when the outermost transaction has a label. | |
| * This is defined whenever at least one transaction in the nested stack has a label. | |
| * If the outermost transaction has no label but inner transactions do, a {@link LabelTree} | |
| * with an undefined root is created. |
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
2 participants
Description
This PR adds a composable
labelstype that keeps track of all labels in nested transactions.