merge-tree: split reset arms + previousProps accessor backed by WeakMap#27414
merge-tree: split reset arms + previousProps accessor backed by WeakMap#27414anthony-murphy wants to merge 7 commits into
Conversation
…rs from resetPendingDeltaToOps
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (253 lines, 5 files), I've queued these reviewers:
How this works
|
| if (index === -1) { | ||
| return undefined; | ||
| } | ||
| return segmentGroup.previousProps[index]; |
There was a problem hiding this comment.
Deep Review: previousPropsForSegment adds three observable branches — previousProps === undefined → undefined, indexOf === -1 → undefined, and the happy path returning segmentGroup.previousProps[index] — but the PR touches only production code; there's no segmentGroupCollection.spec.ts hunk and no other test exercises the new method directly.
The silent-undefined-over-assert shape is a deliberate design choice (consistent with SegmentGroup being intentionally internal per PR #22696 / PR #23401), but right now only the docstring locks it in. A future drift toward an assert-on-miss variant would go uncaught until a downstream caller broke, and the upcoming squash PR is about to start depending on this contract.
A ~5-line direct test covering the three branches — previousProps === undefined → undefined, segment not in group → undefined, happy-path positional pairing returns segmentGroup.previousProps[index] for the correct index — closes the gap before the first migrated caller lands. Either in this PR or paired with the first caller migration in the squash PR.
anthony-murphy
changed the title
| */ | ||
| public previousPropsForSegment(segmentGroup: SegmentGroup): PropertySet | undefined { | ||
| return segmentGroup.previousProps?.get(this.segment); | ||
| } |
There was a problem hiding this comment.
Deep Review: The PR description states the accessor "encapsulates the … lookup" and that callers become a clean one-liner, but the two natural call sites that landed in the same PR continue to access previousProps directly — mergeTree.ts:2479 uses pendingSegmentGroup.previousProps!.get(segment) and client.ts:~1286 uses segmentGroup.previousProps.get(segment). grep previousPropsForSegment across packages/dds/merge-tree returns only this declaration; the encapsulation goal isn't realized in this commit.
Blast radius is narrow — index.ts does not re-export SegmentGroupCollection, so this isn't package-public surface — but the description-vs-diff inconsistency is real and PR #11961's review precedent was to keep copy-only previousProps plumbing private until a consumer exists.
Either (a) migrate mergeTree.ts:2479 and client.ts:~1286 to segment.segmentGroups.previousPropsForSegment(segmentGroup) so the encapsulation lands here, or (b) drop the accessor until the consumer lands in the squash PR.
| assert( | ||
| props !== undefined, | ||
| "Segment missing previousProps entry on annotate rollback", | ||
| ); |
There was a problem hiding this comment.
Deep Review: This new assert uses a raw string, but every other assert(...) in mergeTree.ts uses a numbered hex tag (0x036, 0x037, 0x043–0x04f, 0x751, 0x86c, 0xa40, 0xa6e, 0xb5c, 0xb5d, 0xb6e–0xb73, 0xbaf — 16+ examples). This very PR preserves 0x036, 0x037, 0xbaf, 0xb5c byte-identical in the extracted client.ts helpers, so the convention is intentional.
The repo ships policy-check:asserts (flub generate assertTags) that rewrites raw strings to hex tags, so this either gets rewritten before merge or trips the policy gate. Tags are how assertionShortCodesMap maps production failures back to messages without shipping the strings.
Run npm run policy-check:asserts before merge, or tag manually now:
| ); | |
| const props = pendingSegmentGroup.previousProps!.get(segment); | |
| assert( | |
| props !== undefined, | |
| 0xXXXX /* Segment missing previousProps entry on annotate rollback */, | |
| ); |
| if (sourceProps !== undefined) { | ||
| newPreviousProps.set(segment, sourceProps); | ||
| } | ||
| } |
There was a problem hiding this comment.
Deep Review: This hunk replaces previousProps: segmentGroup.previousProps?.slice(0) with a freshly constructed WeakMap carrying only the current segment's entry — the PR description identifies the old .slice(0) as a latent inconsistency (defensive copy of the props array, but the surrounding loop only ever wrote a single segment to the new group, so the array shape was already mispaired post-copy). The PR ships with no test changes.
Closest existing coverage (resetPendingSegmentsToOp.spec.ts:189-211, client.rollback.spec.ts:206-245 and :658-688) doesn't inspect per-segment previousProps entries. Prior bugs in this exact reset/rebase path were fuzz-discovered, not unit-discovered (#11946, #22069), so existing unit tests are an unreliable safety net for semantic regressions here.
Add a focused regression in resetPendingSegmentsToOp.spec.ts (or client.rollback.spec.ts) that constructs a pending annotate group with multiple segments, triggers resetPendingDeltaToOps reissue, and asserts each new per-segment SegmentGroup has a previousProps WeakMap containing exactly the entry for its own segment.
|
Deep Review: The missing-tests concern is resolved — this commit adds three direct cases for |
| const sourceProps = segmentGroup.previousProps.get(sourceSegment); | ||
| if (sourceProps !== undefined) { | ||
| segmentGroup.previousProps.set(this.segment, sourceProps); | ||
| } |
There was a problem hiding this comment.
Deep Review: enqueueOnCopy is the structural successor to PR #11961 (scarlettjlee, 2022-09-15), which fixed the exact split-after-annotate rollback crash: "when a segment is split after an annotation, there was no corresponding property set for the new segment, causing rollback to crash." The rewritten copy path — segmentGroup.previousProps.get(sourceSegment) → segmentGroup.previousProps.set(this.segment, sourceProps) — is the load-bearing replacement for the prior indexOf + parallel-array push logic.
The new .copyTo spec in test/segmentGroupCollection.spec.ts only enqueues groups with previousProps === undefined (segmentGroups.enqueue({ segments: [], localSeq: 1, refSeq: 0 })). Three new tests were added for the trivial previousPropsForSegment read accessor; zero for the nontrivial copy logic. Prior bugs on this path (#11946, #22069, #24253) were fuzz-discovered, not unit-discovered — existing unit tests aren't a reliable safety net for a silent regression on the split-then-rollback case.
Distinct from the reissue-path concern on client.ts:1293 — that thread covers resetPendingDeltaToOps; this is the split-via-copyTo path.
Add a .copyTo (or sibling) case here that constructs a source SegmentGroup with a populated previousProps WeakMap entry for the source segment, runs the enqueue-on-copy path, and asserts the destination's SegmentGroup.previousProps.get(destSegment) === sourceProps. Ideally also exercise MergeTree.rollback ANNOTATE end-to-end against the split case PR #11961 originally regressed on.
Deep ReviewReviewed commit Readiness: 6/10 — MAKING PROGRESS Structural-prep refactor (helper extraction + Path to Ready
Context for Reviewers
For human reviewer
Review history (4 prior reviews)
|
1 participant
Description
Bundles three merge-tree structural changes:
resetAnnotateOp/resetInsertOphelpers from the long switch inClient.resetPendingDeltaToOps(client.ts). Pure code motion: the ANNOTATE and INSERT arms become one-line calls to the new private helpers, with their original control flow, parameters, and assertions preserved byte-identical.SegmentGroupCollection.previousPropsForSegmentaccessor — encapsulates the "what's the previousProps entry for this collection's segment in the given SegmentGroup" lookup.SegmentGroup.previousProps: WeakMap<ISegmentLeaf, PropertySet>— replaces the array (paired by index withSegmentGroup.segments) with a WeakMap keyed by segment. Removes the fragile "i-thpreviousPropsentry pairs with i-thsegmentsentry" invariant that comments had to call out. ThepreviousPropsForSegmentaccessor becomes a cleanreturn segmentGroup.previousProps?.get(this.segment);one-liner.Sites converted for the WeakMap migration:
mergeTreeNodes.ts: type declaration.mergeTree.ts:addToPendingList:previousProps = new WeakMap().mergeTree.tswrite site:set(segment, propertySet)instead of array push.mergeTree.ts:2479(rollback ANNOTATE):previousProps!.get(segment)instead ofpreviousProps![i]. Theicounter is removed.client.tsreissue path: the original.slice(0)was a defensive copy of the props array, but the surrounding loop only ever wrote a single segment to the new group, so the array shape was already mispaired post-copy (latent inconsistency). Replaced with a newWeakMapcontaining only the entry for the current segment.segmentGroupCollection.ts:enqueueOnCopy: drops theindexOf(sourceSegment); maps source's WeakMap value onto destination segment in the destination map.Truthy-presence semantics (e.g.
op.type === ANNOTATE && !pendingSegmentGroup.previousProps) work unchanged — aWeakMapis truthy.Why
Client.resetPendingDeltaToOpsis ~360 lines with a 4-way switch and triple-nested ternaries in the ANNOTATE arm. Extracting the arms makes future per-arm changes localized to small focused helpers.The implicit "i-th entry pairs with i-th segment" invariant in
previousPropswas a known source of bugs and forced comment annotations. Keying on the segment directly viaWeakMapmakes the pairing inherent in the type. As a bonus, two arrayindexOfcalls insegmentGroupCollection.tsare now O(1).Notes
previousPropsis purely internal merge-tree bookkeeping (not on the wire, not public API). Pure structural prep — no behavior change, no squash logic, no tests touched, no api-report changes.