Skip to content

Split sense API out of CrdtMiniLcmApi - #2733

Merged
hahn-kev merged 1 commit into
claude/crdt-mini-lcm-picture-apifrom
claude/crdt-mini-lcm-sense-api
Oct 7, 2026
Merged

hahn-kev merged 1 commit into
claude/crdt-mini-lcm-picture-apifrom
claude/crdt-mini-lcm-sense-api

Conversation

@hahn-kev-bot

Copy link
Copy Markdown
Collaborator

AI summary

Stacked on #2732. Moves the sense methods out of CrdtMiniLcmApi into a new scoped CrdtSenseApi:

  • GetSense ×2, SubmitCreateSense, CreateSense, SubmitUpdateSense, UpdateSense ×2, MoveSense, SubmitMoveSense, DeleteSense

The sense link methods follow the precedent set by AddPublication and AddComplexFormType, which live in the service for the type being linked to:

  • AddSemanticDomainToSense and RemoveSemanticDomainFromSense move to CrdtSemanticDomainsApi
  • SetSensePartOfSpeech moves to CrdtPartsOfSpeechApi

CreateSenseChanges and VerifySenseBelongsToEntry are now internal static on CrdtSenseApi, because CreateEntry and the example sentence methods (which move in later PRs) still use them.

The method bodies are moved unchanged, with one exception. CreateSense used to check that the part of speech exists by calling the facade's GetPartOfSpeech. It now runs an AnyAsync query against the repo directly, so CrdtSenseApi doesn't depend on CrdtPartsOfSpeechApi. The check and the exception it throws are the same.

Test plan

  • LcmCrdt.Tests: SenseTests, PartOfSpeechTests, SemanticDomainTests, CreateEntryTests, ExampleSentenceTests, UpdateEntryTests, BasicApiTests, ConfigRegistrationTests (100 passed)
  • FwLiteProjectSync.Tests: targeted sense move/reorder/create, semantic domain and part-of-speech sync and import tests (25 passed)

🤖 Generated with Claude Code

Move the sense methods into CrdtSenseApi. Following how entry links went
to their reference-type services, AddSemanticDomainToSense and
RemoveSemanticDomainFromSense move to CrdtSemanticDomainsApi and
SetSensePartOfSpeech to CrdtPartsOfSpeechApi.

CreateSenseChanges and VerifySenseBelongsToEntry become internal statics
on CrdtSenseApi, since CreateEntry and the example sentence methods still
use them from the facade.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: fcee5099-cc81-4e36-ae7c-d0fb18fd032e
📥 Commits

Reviewing files that changed from the base of the PR and between dbf5072 and 5d89a73.

📒 Files selected for processing (5)
  • backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs
  • backend/FwLite/LcmCrdt/LcmCrdtKernel.cs
  • backend/FwLite/LcmCrdt/MiniLcmImp/CrdtPartsOfSpeechApi.cs
  • backend/FwLite/LcmCrdt/MiniLcmImp/CrdtSemanticDomainsApi.cs
  • backend/FwLite/LcmCrdt/MiniLcmImp/CrdtSenseApi.cs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

The change adds CrdtSenseApi for sense retrieval and lifecycle operations, then routes sense operations in CrdtMiniLcmApi through it. The client core registers the new API. The change also adds methods to set a sense’s part of speech and add or remove semantic-domain associations.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Refactor

Suggested reviewers: hahn-kev

Merge Risk: ⚪ Minimal · up to 5d89a

No concrete issue remains that would block merging after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 2.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 40 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: moving the sense API out of CrdtMiniLcmApi.
Description check ✅ Passed The description explains the API moves, the repository-check change, and the reported test results. It is directly related to the changeset.
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.
  • Fix all pre-merge checks with AI
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

A rabbit checks each sense in line
Then sorts its sentences by design
A part of speech gets set in place
Domains join or leave with grace
Changes travel, clear and bright
The burrow rests in order tonight

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

@github-actions github-actions Bot added the 🔩 FW Lite Core Shared MiniLcm, CRDT and sync libraries that ship in both FW Lite and FwHeadless label Oct 7, 2026
@hahn-kev
hahn-kev added this pull request to stack #2735 October 7, 2026 06:53
@argos-ci

argos-ci Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

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

Build Status Details Updated (UTC)
default (Inspect) ✅ No changes detected - Oct 7, 2026, 7:03 AM
e2e (Inspect) ✅ No changes detected - Oct 7, 2026, 7:10 AM

@hahn-kev hahn-kev added the self-reviewed 👁️ I reviewed this myself and with AI and decided it was safe to merge without a second set of eyes label Oct 7, 2026
@hahn-kev
hahn-kev merged commit 6406b0e into develop Oct 7, 2026
27 checks passed
@hahn-kev
hahn-kev deleted the claude/crdt-mini-lcm-sense-api branch October 7, 2026 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🔩 FW Lite Core Shared MiniLcm, CRDT and sync libraries that ship in both FW Lite and FwHeadless self-reviewed 👁️ I reviewed this myself and with AI and decided it was safe to merge without a second set of eyes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants