MergeTree: Fix segment-offset typing

[anthony-murphy]

This pull request refactors several methods and classes in the merge-tree package to improve type safety and handle undefined values more gracefully. It also updates related test files to align with the new changes. The key changes focus on improving type definitions, ensuring safe navigation, and simplifying code logic.

The primary change is moving from a segment-offset type of the form:
{segment: ISegment | undefined, offset: number | undefined}
to:
{segment: ISegment , offset: number } | undefined

Refactoring for Improved Type Safety

• Updated the return type of getContainingSegment in packages/dds/merge-tree/src/mergeTree.ts to use stricter type definitions (segment and offset are now required when defined). This ensures better type safety throughout the codebase. [1] [2]

• Modified getSlideToSegoff in packages/dds/merge-tree/src/mergeTree.ts to handle undefined values explicitly and use stricter type definitions. Added checks to validate segment and offset before returning. [1] [2]

Graceful Handling of Undefined Values

• Updated methods such as getPropertiesAtPosition and getRangeExtentsOfPosition in packages/dds/merge-tree/src/client.ts to use safe navigation (?.) for handling potential undefined values in segment. [1] [2]

• Refactored tests in various files (client.applyMsg.spec.ts, client.localReference.spec.ts, etc.) to use safe navigation (?.) and added assertions to handle undefined values gracefully. [1] [2] [3]

Simplification of Code Logic

• Simplified destructuring logic in getSlideToSegoff and getContainingSegment methods by using default values and safe navigation operators. This reduces unnecessary checks and improves readability. [1] [2]

• Updated test cases in beastTest.spec.ts and other test files to align with the new type definitions and safe navigation changes, ensuring consistency across the codebase. [1] [2]

Test Updates for Consistency

• Refactored assertions and test logic in client.applyMsg.spec.ts and client.localReference.spec.ts to account for stricter type definitions and safe navigation changes. This ensures tests validate the new behavior correctly. [1] [2]

• Updated test cases in client.getPosition.spec.ts and client.annotateMarker.spec.ts to include assertions for undefined handling and stricter type checks. [1] [2]

[Copilot]

Pull Request Overview

This PR tightens type safety for segment-offset handling by updating getSlideToSegoff and getContainingSegment to return undefined when appropriate and refactoring all call sites to handle these optional returns.

• Changed getSlideToSegoff and getContainingSegment signatures and internal logic to return { segment, offset } | undefined
• Updated usage in mergeTree.ts, client.ts, and helper revertibles.ts to handle possible undefined cases
• Refactored numerous tests to use optional chaining and non-null assertions for the new signatures

Reviewed Changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
packages/dds/merge-tree/src/mergeTree.ts	Updated getSlideToSegoff & getContainingSegment return types and logic
packages/dds/merge-tree/src/client.ts	Adjusted calls to getSlideToSegoff & optional getContainingSegment
packages/dds/merge-tree/src/revertibles.ts	Handled optional return from getContainingSegment
packages/dds/merge-tree/src/test/wordUnitTests.spec.ts	Replaced ! on segoff with non-null assertion
packages/dds/merge-tree/src/test/tracking.spec.ts	Added optional chaining for segmentInfo?.segment calls
packages/dds/merge-tree/src/test/testClient.ts	Refactored fallback logic and undefined checks for segoff
packages/dds/merge-tree/src/test/sortedSegmentSet.spec.ts	Updated destructuring to handle undefined segment and offset
packages/dds/merge-tree/src/test/snapshot.utils.ts	Added ?? {} fallback before destructuring segment
packages/dds/merge-tree/src/test/resetPendingSegmentsToOp.spec.ts	Added ?? {} fallback for optional segment
packages/dds/merge-tree/src/test/obliterate.spec.ts	Replaced .segment? with startSeg? to handle missing segOff
packages/dds/merge-tree/src/test/mergeTree.markRangeRemoved.spec.ts	Added ?? {} fallback and assertion for segment
packages/dds/merge-tree/src/test/mergeTree.annotate.spec.ts	Updated all segmentInfo.segment to segmentInfo?.segment
packages/dds/merge-tree/src/test/client.rollback.spec.ts	Added assert(segInfo) before using segInfo.segment
packages/dds/merge-tree/src/test/client.localReferenceFarm.spec.ts	Added assert(seg) before using seg.segment
packages/dds/merge-tree/src/test/client.localReference.spec.ts	Extended return type and added assert(segInfo) for segment
packages/dds/merge-tree/src/test/client.getPosition.spec.ts	Added assert(segOff) and removed non-null !
packages/dds/merge-tree/src/test/client.attributionFarm.spec.ts	Used ?? {} fallback for destructuring candidate segment and offset
packages/dds/merge-tree/src/test/client.applyMsg.spec.ts	Applied optional chaining on segmentInfo?.segment and test assertions
packages/dds/merge-tree/src/test/client.annotateMarker.spec.ts	Changed to markerInfo?.segment before casting to Marker
packages/dds/merge-tree/src/test/beastTest.spec.ts	Replaced non-null ! with optional chaining for performance fetch logic

Comments suppressed due to low confidence (2)

packages/dds/merge-tree/src/test/client.annotateMarker.spec.ts:37

• You should assert that markerInfo is defined (e.g., assert(markerInfo)) before accessing markerInfo.segment to avoid passing undefined to client.annotateMarker.

const marker = markerInfo?.segment as Marker;

packages/dds/merge-tree/src/test/beastTest.spec.ts:1505

• Using optional chaining here allows curSeg to be undefined, but the subsequent textSeg !== null check will pass for undefined, leading to unexpected behavior. Consider asserting curSegOff or using a non-null assertion (curSegOff!.segment) or adding an explicit if (curSeg == null) return; before processing.

const curSeg = curSegOff?.segment;

[anthony-murphy]

the code here was very brittle and dependent on a quirk of getSlideToSegoff that would sometimes return a malformed result with only an {offset: 0}, which then would have getSlideToSegment return the same malformed result and then in createPositionReferenceFromSegoff if would become a detachedReferencePosition

the new check is much more semantically correct, and allowed the clean up of the quicks in the prior implementation .

[anthony-murphy]

this is the code that would sometimes return just an offset with no segment

[Abe27342]

[anthony-murphy]

another place where we would previously keep just the offset but reset the segment