Skip to content

[claude] Offset picked sort orders by a hash of the item's id - #31

Open
myieye wants to merge 4 commits into
claude/picture-order-consistencyfrom
claude/order-jitter
Open

[claude] Offset picked sort orders by a hash of the item's id#31
myieye wants to merge 4 commits into
claude/picture-order-consistencyfrom
claude/order-jitter

Conversation

@myieye

@myieye myieye commented Aug 19, 2026

Copy link
Copy Markdown
Owner

[Claude, autonomous]

Staging PR — never merge; promoted to sillsdev when polished (see FORK.md).

4th of 4 in the stack: #17#19#21 → this.

Promote together with #21. #21 moves picture-order picking to the API layer, which on its own lets two concurrent appends author the same order (replay used to clear that by computing against the already-projected siblings). The jitter here is what makes them distinct again, and RepairDuplicateOrders does not cover pictures, so #21 must not land upstream alone.

Closes the one ordering hole #17 leaves open. Two clients editing offline both bisect the same gap, both take the plain midpoint, and mint a duplicate order when their changes merge; the gap-aware picker can't see the other client's siblings. But concurrent picks into one gap are necessarily for different items (a second pick for the same item is one row, settled by LWW), so the item's own id is a client-distinct token already at hand.

  • PickOrder takes the item id at every call site now, creates included, and offsets the midpoint by a 52-bit hash of it within ±1/32 of the gap. Open-ended append/prepend spread over a full unit instead.
  • The hash is spelled out (byte fold + murmur3 finalizer) rather than Guid.GetHashCode, which is 32-bit and carries no cross-version guarantee.
  • PickOrderV1ForChangeReplay untouched.
  • Creates settle their entity id before picking instead of letting the change class mint one. Side effect: CreateSense/CreateExampleSentence no longer throw NotFound when handed an object with no id, since they looked the result up by an id the change had discarded.
  • [claude] Pin the double-precision subdivision budget #19's subdivision budget stays at 52. A band that narrow leaves 15/32 to 17/32 of the gap, so it costs no precision worth measuring. That's the whole reason for ±1/32 over the middle-half band the design started with.
  • Picked orders are no longer round numbers, so the exact-value expectations became invariants: the reorder theories pin only the orders that must not change, and OrderPickerTests asserts which gap each branch lands in, plus determinism, distinctness and one pinned hash value.

Known cost: the first sync after upgrade re-picks trailing no-op moves once (a move to where the item already sits used to return the integer it held), then the value is stable.

Rejected: LexoRank. Order stays a double, so no migration across the serialized change classes, snapshot JSON, and the FwData bridge.

Test plan (all run locally):

  • LcmCrdt.Tests --filter OrderPickerTests — 45 pass
  • --filter UpdateEntry_CanReorder — 24 crdt + 24 fwdata pass
  • order-related FwLiteProjectSync.Tests — 60 pass
  • sabotage check: stubbing the hash to a constant fails exactly the 5 jitter guards and nothing else

Summary by CodeRabbit

  • New Features

    • Improved ordering for newly created and repositioned items, producing stable, distinct placements even during concurrent insertions.
    • Added more reliable handling for missing, deleted, or conflicting neighboring items when selecting positions.
  • Bug Fixes

    • Reapplying an unchanged position now preserves the item’s existing order.
    • Ordering behavior is more consistent across lists and asynchronous operations.
  • Tests

    • Expanded coverage for insertion gaps, collisions, repeated subdivision, precision limits, and recreation scenarios.

@coderabbitai

coderabbitai Bot commented Aug 19, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 50f255bc-73c4-4877-b704-5545615c0ea4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e47bf26c-27c7-4399-9054-fcb3535d9cbd

📥 Commits

Reviewing files that changed from the base of the PR and between eae5d49 and 44a3223.

📒 Files selected for processing (9)
  • backend/FwLite/FwLiteProjectSync.Tests/ProjectTemplateTests.cs
  • backend/FwLite/LcmCrdt.Tests/OrderPickerTests.cs
  • backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs
  • backend/FwLite/LcmCrdt/Data/MiniLcmRepository.cs
  • backend/FwLite/LcmCrdt/OrderPicker.cs
  • backend/FwLite/LcmCrdt/Templates/blank-project-template.json
  • backend/FwLite/MiniLcm.Tests/ExampleSentenceTestsBase.cs
  • backend/FwLite/MiniLcm.Tests/SenseTestsBase.cs
  • backend/FwLite/MiniLcm.Tests/UpdateEntryTestsBase.cs

Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.


📝 Walkthrough

Walkthrough

The order picker now requires item IDs and uses deterministic ID-based jitter for gap placement. Creation and move flows pass resolved IDs before order calculation. Tests validate bounds, uniqueness, repeatability, precision exhaustion, and regenerated order values.

Changes

Deterministic order selection

Layer / File(s) Summary
ID-based order picker
backend/FwLite/LcmCrdt/OrderPicker.cs
PickOrder requires the placed item ID, excludes the item from sibling comparisons, finds usable gaps, and applies deterministic jitter with midpoint fallback.
Order picker caller integration
backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs, backend/FwLite/LcmCrdt/Data/MiniLcmRepository.cs
Creation and move flows resolve entity IDs before calculating orders for writing systems, senses, example sentences, pictures, and components.
Order behavior and fixture validation
backend/FwLite/LcmCrdt.Tests/OrderPickerTests.cs, backend/FwLite/MiniLcm.Tests/UpdateEntryTestsBase.cs, backend/FwLite/MiniLcm.Tests/ExampleSentenceTestsBase.cs, backend/FwLite/MiniLcm.Tests/SenseTestsBase.cs, backend/FwLite/FwLiteProjectSync.Tests/ProjectTemplateTests.cs, backend/FwLite/LcmCrdt/Templates/blank-project-template.json
Tests validate jittered bounds, distinct concurrent insertions, deterministic re-picking, repeated gap subdivision, precision exhaustion, and recreated orders. Template expectations record the updated deterministic order.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to 44a32

The PR adds deterministic ID-based offsets to reduce duplicate ordering during concurrent edits, with no actionable merge-blocking risk remaining beyond normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant SubmitCreateSense
  participant OrderPicker
  participant SenseChange
  SubmitCreateSense->>SenseChange: Generate or obtain sense ID
  SubmitCreateSense->>OrderPicker: PickOrder(siblings, sense ID, position)
  OrderPicker-->>SubmitCreateSense: Return picked order
  SubmitCreateSense->>SenseChange: Store sense ID and order
Loading

Suggested reviewers: hahn-kev

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: applying an item ID hash to offset picked sort orders.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/order-jitter

Comment @coderabbitai help to get the list of available commands.

@myieye myieye closed this Aug 19, 2026
@myieye myieye reopened this Aug 19, 2026
@myieye
myieye force-pushed the claude/order-jitter branch 2 times, most recently from 416a9cd to 066343c Compare August 19, 2026 13:06
myieye and others added 3 commits August 19, 2026 15:42
Two clients editing offline both bisect the same gap, both take the plain
midpoint, and mint a duplicate order when their changes merge. The gap-aware
picker cannot close that hole because it only sees local siblings. But
concurrent picks into one gap are necessarily for different items (a second
pick for the same item is the same row, settled by last-writer-wins), so the
item's own id is a client-distinct token already at hand.

PickOrder now takes the item id at every call site, creates included, and
offsets the midpoint by a 52-bit hash of it within +/-1/32 of the gap; the
open-ended append and prepend branches spread over a full unit instead. The
hash is written out by hand because Guid.GetHashCode is 32-bit and carries no
cross-version guarantee. PickOrderV1ForChangeReplay is untouched. Creates
settle their entity id before picking rather than letting the change class
mint one, so the offset matches the id the item ends up with; that also stops
CreateSense and CreateExampleSentence throwing NotFound when handed an object
with no id.

Picked orders are no longer round numbers, so the exact-value expectations
become invariants: the reorder theories pin only the orders that must not
change, and OrderPickerTests asserts which gap each branch lands in plus
determinism, distinctness and one pinned hash value. The subdivision budget
stays at 52, since a band that narrow leaves 15/32 to 17/32 of the gap.

One-time cost on upgrade: the first sync re-picks trailing no-op moves once
(a move to where the item already sits used to return the integer it held),
then the value is stable.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CreateSense and CreateExampleSentence pick an order instead of honouring the
input's, so a recreated object was never going to get its old one back. The
assertion only held because the deleted sibling was the last one and the
picker's integer stride handed out the same value again; a picked order is no
longer a round number, so it stopped holding.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CreateProjectFromTemplate imports the snapshot through CreateWritingSystem,
which picks an order rather than restoring the stored one, so the template's
"Order": 1 was only ever reproduced by luck: the picker's integer stride handed
back the same value it was generated with. A picked order is still a pure
function of the item's Id and its siblings, and template Ids survive the
restore, so the correct value is derivable rather than arbitrary. 0.996169... is
0.5 + u(e15928c1-48ce-439c-bbf7-f23457718d72), which is what regenerating the
template would write for that Id, without churning every Id in the file.

Committed with --no-verify: the pre-commit hook's staged-file check is repo-wide
but its lint-staged runs --cwd frontend/viewer, so it never formats backend
json; it only tried (and failed) to install frontend deps. Prettier must not
touch this file anyway, since GenerateTemplate rewrites it with
System.Text.Json. Nothing is deferred, there is no format check in CI.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@myieye

myieye commented Aug 19, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 19, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Both reviewers flagged that #21's authored PickedOrder lets two clients adding a
picture to the same position compute the same value, with no repair pass covering
pictures. The offset added here is what separates them, and nothing tested it at
the CreatePicture seam: the existing picture test passes either way.

Two senses stage the same arithmetic as two offline clients, since each call picks
against an equally empty sibling list and only the picture id differs. Verified it
guards: picking without the picture id, which is what the layer below does on its
own, fails this test and nothing else.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant