Check whether marker is removed in searchForMarker #24771
Conversation
github-actions
bot
added
area: dds
There was a problem hiding this comment.
Pull Request Overview
This PR updates the marker lookup logic in searchForMarker to ensure that removed markers are not returned, aligning its behavior with getMarkerFromId.
- Updated the test case to reflect corrected insertion and removal indices for markers
- Modified mergeTree.ts to exclude removed markers during search
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/dds/merge-tree/src/test/client.searchForMarker.spec.ts | Adjusted marker insertion and removal indices for accurate test validation |
| packages/dds/merge-tree/src/mergeTree.ts | Added a check to ignore removed markers in searchForMarker |
Comments suppressed due to low confidence (1)
packages/dds/merge-tree/src/test/client.searchForMarker.spec.ts:599
- Changing the insertion index and corresponding removal range ensures that the test correctly verifies that removed markers are excluded. Please double-check that these indices consistently reflect marker positions across the system.
client.insertMarkerLocal(1, ReferenceType.Tile, {
| @@ -1270,7 +1270,7 @@ export class MergeTree { | |||
| segment, | |||
| (node) => { | |||
| if (node.isLeaf()) { | |||
There was a problem hiding this comment.
Introducing the !isRemoved(node) condition prevents removed markers from being returned. Consider adding a short inline comment describing why this check is necessary for future maintainability.
| if (node.isLeaf()) { | |
| if (node.isLeaf()) { | |
| // Exclude removed markers to ensure only valid markers are returned. |
Copilot uses AI. Check for mistakes.
| @@ -1270,7 +1270,7 @@ export class MergeTree { | |||
| segment, | |||
| (node) => { | |||
| if (node.isLeaf()) { | |||
| if (Marker.is(node) && refHasTileLabel(node, markerLabel)) { | |||
| if (!isRemoved(node) && Marker.is(node) && refHasTileLabel(node, markerLabel)) { | |||
There was a problem hiding this comment.
i think we need the same check where we update leftmostTiles and rightmostTiles
There was a problem hiding this comment.
leftmostTiles and rightmostTiles already exclude removed tiles due to the check for (this.leafLength(segment) ?? 0) > 0
Update implementation of searchForMarker to not return removed markers. This matches what getMarkerFromId does. The unit test was passing because the removed marker is at the beginning of the string, so starting the search at index 0 is after it.