Skip to content
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

Add Clone to SharedTree Revertible #23044

Merged
merged 57 commits into from
Nov 20, 2024
Merged

Conversation

jikim-msft
Copy link
Contributor

@jikim-msft jikim-msft commented Nov 9, 2024

Description

13864

This PR adds forkable revertible feature to the Revertible object of SharedTree.

  • Removed DisposableRevertible and replaced by RevertibleAlpha.
  • Added clone() method to the new interface.
  • Uses TreeBranch (which is subset of TreeCheckout) to access data necessary for revert operation.

@github-actions github-actions bot added base: main PRs targeted against main branch area: dds Issues related to distributed data structures area: dds: tree labels Nov 9, 2024
@jikim-msft jikim-msft changed the title Revertible alpha Add clone to SharedTree Revertible Nov 9, 2024
@jikim-msft jikim-msft changed the title Add clone to SharedTree Revertible Add Clone to SharedTree Revertible Nov 9, 2024
@jikim-msft jikim-msft marked this pull request as ready for review November 9, 2024 02:36
@jikim-msft jikim-msft requested a review from a team as a code owner November 9, 2024 02:36
Copy link
Collaborator

@msfluid-bot msfluid-bot left a comment

Choose a reason for hiding this comment

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

Code Coverage Summary

↓ packages.dds.tree.src.shared-tree:
Line Coverage Change: -0.02%    Branch Coverage Change: -0.17%
Metric NameBaseline coveragePR coverageCoverage Diff
Branch Coverage 91.76% 91.59% ↓ -0.17%
Line Coverage 97.30% 97.28% ↓ -0.02%
↑ packages.dds.tree.src.simple-tree.api:
Line Coverage Change: 0.02%    Branch Coverage Change: No change
Metric NameBaseline coveragePR coverageCoverage Diff
Branch Coverage 89.77% 89.77% → No change
Line Coverage 82.34% 82.36% ↑ 0.02%

Baseline commit: d3bf90c
Baseline build: 308876
Happy Coding!!

Code coverage comparison check passed!!

@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Nov 9, 2024

@fluid-example/bundle-size-tests: +919 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 467.24 KB 467.27 KB +35 Bytes
azureClient.js 564.01 KB 564.06 KB +49 Bytes
connectionState.js 724 Bytes 724 Bytes No change
containerRuntime.js 263.43 KB 263.45 KB +14 Bytes
fluidFramework.js 428.51 KB 428.85 KB +351 Bytes
loader.js 134.18 KB 134.19 KB +14 Bytes
map.js 42.71 KB 42.71 KB +7 Bytes
matrix.js 150.15 KB 150.16 KB +7 Bytes
odspClient.js 529.85 KB 529.89 KB +49 Bytes
odspDriver.js 97.88 KB 97.9 KB +21 Bytes
odspPrefetchSnapshot.js 42.81 KB 42.83 KB +14 Bytes
sharedString.js 166.23 KB 166.24 KB +7 Bytes
sharedTree.js 418.97 KB 419.3 KB +344 Bytes
Total Size 3.38 MB 3.38 MB +919 Bytes

Baseline commit: 224f59a

Generated by 🚫 dangerJS against d6afed1

packages/dds/tree/src/core/revertible.ts Outdated Show resolved Hide resolved
packages/dds/tree/src/shared-tree/treeCheckout.ts Outdated Show resolved Hide resolved
packages/dds/tree/src/shared-tree/treeCheckout.ts Outdated Show resolved Hide resolved

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 6 changed files in this pull request and generated no suggestions.

Files not reviewed (1)
  • packages/dds/tree/src/core/index.ts: Evaluated as low risk
Comments skipped due to low confidence (2)

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

  • [nitpick] The method name 'clone' might be ambiguous. Consider renaming it to 'createClone' or 'duplicate' for better clarity.
clone: (forkedBranch?: TreeBranch) => {

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

  • [nitpick] The error message 'Unable to dispose a revertible that has already been disposed.' could be more descriptive. Consider providing more context about why this error might occur.
throw new UsageError("Unable to dispose a revertible that has already been disposed.");

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

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.

Implementation looks correct. You'll need to a changeset and get docs review before checking in.

@jikim-msft jikim-msft requested a review from a team as a code owner November 20, 2024 02:47
@jikim-msft jikim-msft enabled auto-merge (squash) November 20, 2024 02:47
.changeset/dull-words-work.md Show resolved Hide resolved
packages/dds/tree/src/core/revertible.ts Outdated Show resolved Hide resolved
packages/dds/tree/src/test/shared-tree/undo.spec.ts Outdated Show resolved Hide resolved
Copy link
Contributor

🔗 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:
  170005 links
    1595 destination URLs
    1825 URLs ignored
       0 warnings
       0 errors


Copy link
Contributor

@jzaffiro jzaffiro left a comment

Choose a reason for hiding this comment

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

Approved for docs pending one minor comment

@jikim-msft jikim-msft merged commit 5abfa01 into microsoft:main Nov 20, 2024
33 checks passed
@jikim-msft jikim-msft deleted the revertible-alpha branch November 21, 2024 00:26
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 changeset-present public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants