Fix spurious CFC SetOrder during fwdata→crdt sync#2304
Merged
Conversation
The orderable diff in EntrySync hands BetweenPosition(null, null) to Add when no stable neighbours exist (e.g. a singleton component). During sync the same logical CFC is reached twice — once via SyncComplexForms on the component-side entry (no between) and once via SyncComplexFormComponents on the complex-form entry (between with both neighbours null). The second call found the just-created CFC and, because between was non-null, fell through to MoveComplexFormComponent → PickOrder → max+1 every time, emitting a spurious SetOrderChange that bumped Order from 1 → 2 (and further on repeated syncs). CreateComplexFormComponent now treats `(null, null)` as no positional intent — same boundary normalisation OrderPicker.PickOrder and FwDataMiniLcmApi.InsertComplexFormComponent already do. New test in ComplexFormComponentTestsBase exercises both API implementations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Names the predicate shared by OrderPicker.PickOrder (the create branch's "no positional intent" shortcut) and the new CreateComplexFormComponent guard. The CRDT site's previous inverted pattern read awkwardly; both callsites now use the same null-tolerant helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ches" This reverts commit 78233aa.
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds a guard condition in ChangesBetween-position singleton guard
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
💻 FW Lite
|
The latest updates on your projects. Learn more about Argos notifications ↗︎ Awaiting the start of a new Argos build… |
Contributor
Keep just the WHY: where (null, null) comes from and what skipping the move prevents. The full sync trace belongs in the commit history, not the source. Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
During sync the same logical CFC is reached twice — once via
SyncComplexFormson the component-side entry (nobetween), again viaSyncComplexFormComponentson the complex-form entry withBetweenPosition(null, null)for a singleton. The second call found the existing CFC and, becausebetweenwas non-null, fell through toMoveComplexFormComponent→PickOrder→max + 1, emitting a spuriousSetOrderChangethat bumped Order 1→2 (and further on repeat syncs).CreateComplexFormComponentnow treats(null, null)as "no positional intent", matching the normalisationOrderPicker.PickOrderandFwDataMiniLcmApi.InsertComplexFormComponent:1137already apply.Surfaced in a real DB: 243 of 365 FieldWorks-authored
SetOrderChangeevents were on CFCs that are the only sibling on their complex form.Test plan
CreateComplexFormComponent_BetweenWithNoAnchors_DoesNotReorderExistinginComplexFormComponentTestsBase(runs against both LcmCrdt and FwData implementations — FwData already honoured the contract).🤖 Generated with Claude Code