Fix syncing components referencing moved senses

[myieye]

Resolves: #2234

Apparently senses can sometimes/somehow move to a different entry.
That breaks an assumption I had made that senseId could be used to uniquely and consistently identify a complex-form-component. So, this PR does 3 things:

1. Creates a sync test that stumbles over an exception
2. Refactors IOrderableCollectionDiffApi to accept an ID type param, rather than only supporting Guids
3. Refactors all users of that interface, including the one causing the exception (so it now uses a tuple of all relevant Guids like we were already doing for Complex-Forms)

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR refactors the orderable collection diffing system to support arbitrary non-nullable ID types beyond Guid, replacing IOrderableCollectionDiffApi<T> with a generic IOrderableCollectionDiffApi<T, TId>. The changes update DiffCollection.cs and its dependent sync implementations to thread the ID type through position tracking and diff generation, while correcting the argument order for complex form component synchronization.

Changes

Cohort / File(s)	Summary
Core Diffing Refactor backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs	Generalizes orderable collection diffing from Guid to arbitrary non-nullable ID types; updates IOrderableCollectionDiffApi<T> to IOrderableCollectionDiffApi<T, TId>, changes BetweenPosition to generic BetweenPosition<T>, and refactors DiffOrderable with new type constraints. Requires careful review of stable ID tracking and position-diff generation logic.
Sync Implementation Updates backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs, backend/FwLite/MiniLcm/SyncHelpers/ExampleSentenceSync.cs, backend/FwLite/MiniLcm/SyncHelpers/WritingSystemSync.cs	Adapt sync APIs to implement the new generic IOrderableCollectionDiffApi<T, TId> interface, update Add/Move methods to accept BetweenPosition<T>, and provide GetId implementations. EntrySync also corrects complex form component diff argument order and refactors ComponentsDiffApi to use tuple-based keys.
Test Infrastructure Updates backend/FwLite/MiniLcm.Tests/TestOrderableDiffApi.cs, backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs	Update test helper methods to use strongly-typed BetweenPosition<TestOrderable> and adapt TestOrderableDiffApi to implement the new generic interface with explicit GetId method.
Regression Test backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs	Adds focused synchronization regression test validating that complex form component ComponentSenseId remains constant while ComponentEntryId updates when sense moves between entries.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~55 minutes

Possibly related PRs

• Move and handle writing systems effectively #1890: Modifies the same MiniLcm diffing and writing-system sync code with generic BetweenPosition/DiffCollection changes and orderable diffing APIs.
• Disambiguate complex-forms that reference same entry #1564: Directly related—modifies EntrySync's complex-form components diffing with tuple-based ID changes and ComplexFormsDiffApi updates.
• Add comprehensive tests for ComplexFormComponent creation #2204: Related through ComplexFormComponent creation/synchronization code and component ID/entry assignment validation tests.

Suggested labels

💻 FW Lite

Suggested reviewers

• hahn-kev
• rmunn

Poem

🐰 A refactoring tale, generically told,
Where IDs broke free from Guid's stronghold,
Through DiffCollections, position and place,
Complex forms now sync at a gentler pace,
Tests confirm senses stay, entries transform—
Order is restored, diffing finds form! 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% 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	The description is well-related to the changeset, explaining the problem (senses moving entries), the three-part solution (test, refactor interface, refactor consumers), and referencing the linked issue.
Linked Issues check	✅ Passed	The PR fully addresses issue #2234 by adding a regression test reproducing the sync failure, refactoring IOrderableCollectionDiffApi to accept generic ID types, and updating all consumers to use composite tuple keys instead of single IDs.
Out of Scope Changes check	✅ Passed	All changes are focused on fixing the syncing issue and refactoring related infrastructure; no unrelated or out-of-scope modifications are present.
Title check	✅ Passed	The title 'Fix syncing components referencing moved senses' directly describes the main change: addressing a sync failure when complex form components reference senses that have moved between entries.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix-syncing-moved-senses

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

UI unit Tests

  1 files  ±0   59 suites  ±0   32s ⏱️ -1s
176 tests ±0  176 ✅ ±0  0 💤 ±0  0 ❌ ±0 
245 runs  ±0  245 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit b4ceac2. ± Comparison against base commit e02e5ff.

[argos-ci]

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build	Status	Details	Updated (UTC)
default (Inspect)	✅ No changes detected	-	Apr 28, 2026, 2:14 PM

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@backend/FwLite/MiniLcm/SyncHelpers/WritingSystemSync.cs`:
- Around line 78-82: The Remove(WritingSystem value) method in WritingSystemSync
currently returns Task.FromResult(0) which silently ignores deletion; change it
to fail fast by throwing an appropriate exception (e.g., throw new
NotSupportedException("Deleting writing systems is not supported during sync.")
wrapped in a Task) so callers see the limitation; update the
Remove(WritingSystem value) implementation to return a failed Task (or use
Task.FromException) with a clear message and remove the misleading TODO/comment
if desired.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5130c46b-d1fe-47c5-bcd5-49cdd63eccb3

📥 Commits

Reviewing files that changed from the base of the PR and between e02e5ff and b4ceac2.

📒 Files selected for processing (7)

• backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs
• backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs
• backend/FwLite/MiniLcm.Tests/TestOrderableDiffApi.cs
• backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs
• backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs
• backend/FwLite/MiniLcm/SyncHelpers/ExampleSentenceSync.cs
• backend/FwLite/MiniLcm/SyncHelpers/WritingSystemSync.cs

[rmunn]

I just checked in FieldWorks Language Explorer. If an entry has more than one sense then each of its senses has a submenu "Move Sense to a New Entry". This creates a new entry with the same lexeme form and citation form (in fact, it copies all the entry-level fields), making the new entry a homograph of the old one and assigning homograph numbers immediately.

You cannot move a sense to a different existing entry, though: it has to be "create a new entry for this sense". The intended purpose appears to be "Wait, I have 'bank' as a verb, to tilt an airplane, and 'bank' as a noun, a place to store money, in the same dictionary entry. Those aren't really the same word, they're two different words that happen to be spelled the same. I should separate those meanings."

I had not realized that the sense's GUID would remain the same when that happens, but it does make sense now that I think about it, because other objects (like the grammatical categories and example sentences) depend on that sense (and reference it by GUID), so if the sense's GUID changed then all those other objects would need to be updated as well, which would potentially balloon in complexity.

[rmunn]

This also means that the design we had for Language Forge, where senses did not have a truly independent ID of their own but only hung off their parent entry, was subtly wrong. And the MiniLcm API will probably need to change the GetSense(Guid entryId, Guid senseId) to just be GetSense(Guid senseId) as well.

[rmunn]

LGTM. Good work figuring that out.