Set PartOfSpeech object in snapshots

[myieye]

Resolves #2090

• Start setting PartOfSpeech
• Write failing test (there already was a test 🥳 that I wrote on almost exactly the same day as the comment was written)
• Fix the test (Unify behavior of adding and updating projected entities harmony#58)
• Reference Harmony main (Waiting for Unify behavior of adding and updating projected entities harmony#58 to actually get merged)

[coderabbitai]

Important

Review skipped

Auto incremental 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.

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

📝 Walkthrough

Walkthrough

Two CRDT change handlers are updated to properly resolve and assign part-of-speech objects to Sense entities, ensuring part-of-speech values are captured in snapshots alongside IDs, addressing the issue where part-of-speech appeared as "none" in snapshots.

Changes

Cohort / File(s)	Summary
Part of Speech Resolution in Sense Creation backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs	Pre-fetches PartOfSpeechId snapshots and derives PartOfSpeech objects, then sets both PartOfSpeech and PartOfSpeechId in returned Sense. Replaces previous approach that only set PartOfSpeechId via DeletedAsNull.
Part of Speech Clearing in Sense Updates backend/FwLite/LcmCrdt/Changes/SetPartOfSpeechChange.cs	Directly assigns PartOfSpeech objects to entities: null when PartOfSpeechId becomes null, resolved object when valid, or clears both fields when deleted. Removes previous comment and now actively assigns PartOfSpeech.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Snapshot resolution and null-safety logic across two interconnected change handlers
• Cross-file consistency of PartOfSpeech object/ID synchronization needs verification
• Integration with snapshot pre-fetching and deletion handling warrants careful validation

Possibly related PRs

• PR #1919: Updates EF mapping to make PartOfSpeech FK optional with SetNull on delete behavior, complementing these CRDT-level changes
• PR #1816: Adds SetSensePartOfSpeech and modifies JSON-patch to change handling for senses, directly related to how PartOfSpeech is populated in Sense objects
• PR #2051: Modifies how Sense PartOfSpeech is assigned/cleared with null-safe resolution, sharing the same core intent

Suggested reviewers

• rmunn

Poem

🐰 Snapshots now sparkle with speech parts so clear,
No more "none" whispers throughout the year,
Where objects were missing, now objects reside,
Semantic domains now have company beside! ✨

Pre-merge checks and finishing touches

❌ 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%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The code changes directly address issue #2090 by modifying CreateSenseChange and SetPartOfSpeechChange to set the PartOfSpeech object in snapshots, fulfilling the core objective.
Out of Scope Changes check	✅ Passed	All changes are focused on setting PartOfSpeech objects in snapshots as required by issue #2090, with no extraneous modifications detected.
Title check	✅ Passed	The title accurately reflects the main change: both files now set the PartOfSpeech object in snapshots (CreateSenseChange sets it when creating, SetPartOfSpeechChange sets it when updating).
Description check	✅ Passed	The pull request description clearly references issue #2090 and outlines concrete changes related to setting PartOfSpeech in snapshots, which aligns with the changeset modifications to CreateSenseChange and SetPartOfSpeechChange.

---

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.}

[argos-ci]

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

Build	Status	Details	Updated (UTC)
default (Inspect)	✅ No changes detected	-	Jan 14, 2026, 3:54 PM

[github-actions]

UI unit Tests

  1 files  ±0   45 suites  ±0   17s ⏱️ ±0s
111 tests ±0  111 ✅ ±0  0 💤 ±0  0 ❌ ±0 
160 runs  ±0  160 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 03f8ae6. ± Comparison against base commit d6363e7.

♻️ This comment has been updated with latest results.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

backend/FwLite/LcmCrdt/Changes/SetPartOfSpeechChange.cs (1)

13-28: Syncing PartOfSpeech with PartOfSpeechId in updates looks solid

The null / invalid / deleted branches correctly clear both PartOfSpeechId and PartOfSpeech, and the valid branch sets both from the resolved entity. This should ensure snapshots see a real PartOfSpeech object instead of “none” while avoiding stale or deleted references. If more call sites start resolving POS from an ID, you might later want a small helper to centralize this pattern, but it’s not necessary for this PR.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c0957ed and 6ada411.

📒 Files selected for processing (2)

• backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs (1 hunks)
• backend/FwLite/LcmCrdt/Changes/SetPartOfSpeechChange.cs (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

backend/FwLite/LcmCrdt/Changes/SetPartOfSpeechChange.cs (3)

backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (1)

• PartOfSpeech (631-640)

backend/LfClassicData/LfClassicMiniLcmApi.cs (1)

• PartOfSpeech (371-384)

backend/FwLite/MiniLcm/Models/Sense.cs (1)

• PartOfSpeech (58-63)

backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs (2)

backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (3)

• PartOfSpeech (631-640)
• Sense (762-777)
• MultiString (795-817)

backend/FwLite/MiniLcm/Models/PartOfSpeech.cs (2)

• PartOfSpeech (3-31)
• PartOfSpeech (21-30)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

• GitHub Check: Build FwHeadless / publish-fw-headless
• GitHub Check: Build FW Lite and run tests
• GitHub Check: frontend-component-unit-tests
• GitHub Check: frontend
• GitHub Check: Analyze (csharp)

🔇 Additional comments (1)

backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs (1)

38-51: Snapshot-based POS resolution on sense creation is consistent with the goal

Prefetching the PartOfSpeech snapshot and only embedding it when EntityIsDeleted == false, then setting PartOfSpeechId = partOfSpeech?.Id, aligns creation semantics with your update path and should make POS available in snapshots from the outset. Please just double‑check that GetSnapshot + EntityIsDeleted covers all the same “treat as null” cases that DeletedAsNull handled previously (e.g., missing or already-deleted POS), so we don’t change behavior in subtle edge cases.

[github-actions]

C# Unit Tests

146 tests  ±0   146 ✅ ±0   20s ⏱️ ±0s
 22 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 03f8ae6. ± Comparison against base commit d6363e7.

♻️ This comment has been updated with latest results.

[imnasnainaec]

Looks good! Just one test to possibly add.