Skip to content

Interval Collections: Refactor rebase to be based on local references #24867

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

Merged
merged 7 commits into from
Jun 17, 2025

Conversation

anthony-murphy
Copy link
Contributor

This pull request refactors the IntervalCollection class in packages/dds/sequence/src/intervalCollection.ts to improve type safety, streamline interval rebasing logic, and enhance the handling of local metadata. It also updates the IntervalCollectionMapInterfaces and adjusts test utilities accordingly.

Refactoring for Type Safety and Code Simplification:

  • Refactored hasEndpointChanges function to use a type predicate, ensuring the serialized parameter is of type ISerializedInterval when endpoints are defined.
  • Renamed and restructured rebasePositionWithSegmentSlide to rebaseReferenceWithSegmentSlide, simplifying its logic and improving readability by using LocalReferencePosition instead of raw positions.
  • Updated computeRebasedPositions method to return a Pick<ISerializedInterval, "start" | "end"> type for better specificity and clarity.

Enhancements to Interval Rebasing Logic:

  • Modified rebaseLocalInterval method to accept SerializedIntervalDelta directly instead of relying on original from metadata, improving flexibility and clarity.
  • Improved rebasing logic in computeRebasedPositions by leveraging LocalReferencePosition for calculating rebased start and end positions.

Updates to Metadata Handling:

  • Replaced original with interval in metadata interfaces (IntervalAddLocalMetadata and IntervalChangeLocalMetadata) for better alignment with interval operations.
  • Updated metadata rebased types to use Pick<ISerializedInterval, "start" | "end">, enhancing type specificity.

Test Utility Adjustments:

  • Added bounds adjustments for start and end in test operations (removeRange and annotateRange) to ensure valid interval positions during fuzz testing.

@Copilot Copilot AI review requested due to automatic review settings June 17, 2025 20:35
@anthony-murphy anthony-murphy requested a review from a team as a code owner June 17, 2025 20:35
@github-actions github-actions bot added base: main PRs targeted against main branch area: dds Issues related to distributed data structures area: dds: sharedstring labels Jun 17, 2025
Copy link
Contributor

@Copilot 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 refactors the IntervalCollection class to improve type safety, simplify interval rebasing logic, and update metadata handling.

  • Refactored methods (hasEndpointChanges, rebaseReferenceWithSegmentSlide, computeRebasedPositions, rebaseLocalInterval) to streamline the interval rebasing process.
  • Updated metadata interfaces by replacing "original" with a new "interval" field and refining the "rebased" types for better specificity.
  • Adjusted test utility functions to properly modify interval positions during fuzz testing.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
packages/dds/sequence/src/test/fuzz/fuzzUtils.ts Adjusts start/end positions in test operations for fuzz testing.
packages/dds/sequence/src/intervalCollectionMapInterfaces.ts Updates metadata interfaces by renaming fields and refining types.
packages/dds/sequence/src/intervalCollection.ts Refactors interval rebasing methods and updates API design for interval metadata handling.
Comments suppressed due to low confidence (2)

packages/dds/sequence/src/intervalCollection.ts:1467

  • The parameter 'original' is used only to check for endpoint changes and is not otherwise utilized. Consider renaming it or clarifying its role to reduce potential confusion with localOpMetadata.interval.
	): SerializedIntervalDelta | undefined {

packages/dds/sequence/src/intervalCollection.ts:1509

  • Assigning the result of changeInterval to localOpMetadata.interval overwrites the original interval metadata. It may be clearer to use a separate variable or make the update more explicit to avoid unintended side effects.
			localOpMetadata.interval = this.localCollection?.changeInterval(

@anthony-murphy anthony-murphy changed the title Interval Collections: Refactor rebase to be based on positions Interval Collections: Refactor rebase to be based on local references Jun 17, 2025
@anthony-murphy anthony-murphy merged commit d1bda35 into microsoft:main Jun 17, 2025
35 checks passed
@anthony-murphy anthony-murphy deleted the ic/rebase-refs-not-pos branch June 17, 2025 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds: sharedstring area: dds Issues related to distributed data structures base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants