Reduce cross-instance private access in merge-tree - #28242
Matt Rakow (ChumpChief) wants to merge 7 commits into
Conversation
…ification Use the trackingGroups getter for comparisons and leafLength for leaf-only partial-length verification. Cover tracking-group identity comparisons with a regression test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use public enqueue and copy the source segment's previous properties directly in copyTo, avoiding a private method call on the destination collection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Extract the invalidation setter and segment-count/baseline queries without changing partial-length construction or incremental contribution logic. Adapt coverage to the existing combine and update paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Expose the existing count through a getter for diagnostic validation without changing reference storage, initialization, or transfer behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the cached child count and indexed loops with for-of iteration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
area: tools
area: dds
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (252 lines, 9 files), I've queued these reviewers:
How this works
|
There was a problem hiding this comment.
🟡 Changes recommended
Fix the possibly undefined trackingGroups dereference causing TS2532 and preventing compilation.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This pull request replaces private merge-tree access with public APIs while preserving behavior and adding regression tests.
Changes:
- Updates tracking, segment-group, partial-length, and reference-count access.
- Adds owner-side partial-length APIs and invalidation coverage.
- Adds focused regression tests.
File summaries
| File | Summary |
|---|---|
packages/dds/merge-tree/src/test/tracking.spec.ts |
Tests tracking-group matching. |
packages/dds/merge-tree/src/test/testUtils.ts |
Uses public reference-count access. |
packages/dds/merge-tree/src/test/segmentGroupCollection.spec.ts |
Tests previous-properties preservation. |
packages/dds/merge-tree/src/test/partialLength.spec.ts |
Tests partial-length queries and invalidation. |
packages/dds/merge-tree/src/test/localReference.spec.ts |
Tests local-reference counts. |
packages/dds/merge-tree/src/segmentGroupCollection.ts |
Uses public enqueueing during copies. |
packages/dds/merge-tree/src/partialLengths.ts |
Adds owner-side accessors and simplifies iteration. |
packages/dds/merge-tree/src/mergeTreeTracking.ts |
Uses the tracking-groups getter. |
packages/dds/merge-tree/src/localReference.ts |
Adds the reference-count getter. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Remove optional chaining and the unnecessary undefined union from matches, consistent with its non-nullable parameter and getter return type. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fleet Review — CleanNo issues found across the reviewer fleet for this run. |
| const combined = PartialSequenceLengths.combine(parent, mergeTree.collabWindow); | ||
|
|
||
| for (const partials of [mergeTree.root.partialLengths, combined]) { | ||
| assert.equal(partials.getSegmentCount(), 3); |
There was a problem hiding this comment.
I will note -- I don't understand why these numbers are what they are. Just double checking that you are confident in them before we let the LLM reward hack a broken test in
There was a problem hiding this comment.
I checked the expectations at packages/dds/merge-tree/src/test/partialLength.spec.ts:81-83: inserting "more " at offset 5 produces "hello", "more ", and " world!"—three segments. The baseline at minSeq = 0 contains the original 12 characters (5 + 7), while sequence 1 includes the insertion for a total of 17 (12 + 5); wrapping the subtree in a parent does not change those values. I added brief comments explaining these and the other numeric expectations in 992bf1c160f; the assertions are unchanged.
Which is, admittedly, exactly what a reward-hacking LLM would say.
— Copilot, the LLM under investigation
Clarify text segmentation, sequence baselines, reference counts, and property-slot expectations without changing test behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Bundle size comparisonBase commit: Notable changesNo bundles changed by ≥ 500 bytes parsed. Per-bundle deltas
|
Description
Reduce cross-instance private access in merge-tree through small changes that preserve behavior for valid callers:
trackingGroupsgetter and remove unnecessary optional chaining, relying onmatches()'s existing non-nullable parameter contract.enqueueOnCopy()helper with publicenqueue()and the existing previous-properties bookkeeping.leafLength()andLocalReferenceCollection.sizeinstead of private access in verification helpers.for...of, removing the cached array length.This is an incremental cleanup, not a complete elimination of private access. Larger changes to attribution, property-manager copying, reference transfers, and partial-length construction are intentionally deferred.
Test Coverage
The added tests make the existing contracts explicit and guard against accidental behavioral changes while replacing private access:
enqueueOnCopy().minSeqfrom the length after later sequence adjustments.sizereflects additions and removals, excludes transient references, and includes references in all three bucket positions. The existing count-validation helper now uses this getter instead of private access.Reviewer Guidance
The review process is outlined in the pull request guidelines.
The intended scope is straightforward access substitutions, preserving existing calculations, bookkeeping, and operation ordering. No data structures or copying contracts are redesigned.