Skip to content

ensure headwords are set on complex form components#2379

Merged
hahn-kev merged 3 commits into
developfrom
show-affix-on-components
Jun 30, 2026
Merged

ensure headwords are set on complex form components#2379
hahn-kev merged 3 commits into
developfrom
show-affix-on-components

Conversation

@hahn-kev

Copy link
Copy Markdown
Collaborator

now when looking at complex forms and components the headword will be properly formatted based on the morph type
image

@hahn-kev hahn-kev requested a review from myieye June 25, 2026 10:00
@github-actions github-actions Bot added the 💻 FW Lite issues related to the fw lite application, not miniLcm or crdt related label Jun 25, 2026
@argos-ci

argos-ci Bot commented Jun 25, 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 - Jun 29, 2026, 12:59 PM

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown

Review Change Stack

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.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 9dae34c7-27e5-449b-9305-ac90326ac4da

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
📝 Walkthrough

Walkthrough

The PR adds SQL query helpers and LinqToDB mappings for morph types, complex-form component relationships, and headword fields, updates bridge headword generation to use default token behavior, and adds tests covering the new query and component headword paths.

Changes

Component query and headword mapping

Layer / File(s) Summary
SQLite query helpers
backend/FwLite/LcmCrdt/Data/EntryQueryHelpers.cs
Adds SQLite-mapped concat_ws and nullif, SQL translation stubs, and the revised headword-with-tokens expression.
Mapping schema wiring
backend/FwLite/LcmCrdt/LcmCrdtKernel.cs
Registers Entry morph-type mapping and ComplexFormComponent relationship and headword mappings in ConfigureDbOptions.
Bridge headword defaults
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs
Updates component headword generation in ToEntryReference and ToSenseReference to use default LexEntryHeadwordOrUnknown() behavior.
Query helper tests
backend/FwLite/LcmCrdt.Tests/Data/QueryHelperTests.cs
Adds fixture-backed query tests for morph type, headword, component entry, and component headword helpers.
Complex-form headword regression
backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs
Makes appleId accessible to derived tests and adds a regression test that asserts component headwords preserve morph tokens.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • myieye
  • rmunn

Poem

A bunny hopped through SQL snow,
With -s in whiskers soft aglow.
I mapped the bits, both near and far,
And headwords blinked like a tiny star.
🐰✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.41% 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
Title check ✅ Passed The title matches the main change: setting complex form component headwords.
Description check ✅ Passed The description is on-topic and describes the complex form headword formatting change.
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.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch show-affix-on-components

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

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (1)

734-736: 🗄️ Data Integrity & Integration | 🔵 Trivial

Align Component and Complex Form Headwords with CRDT Tokenization

The modification to ComponentHeadword and ComplexFormHeadword now correctly emits morph tokens (e.g. -s), aligning this LCM bridge mapping with the canonical CRDT QueryHeadwordWithTokens behavior. This resolves the drift between the projected data and the source of truth.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs` around lines 734
- 736, Update the headword mapping in FwDataMiniLcmApi so ComponentHeadword and
ComplexFormHeadword use the same tokenized CRDT-style headword representation as
QueryHeadwordWithTokens instead of LexEntryHeadwordOrUnknown; adjust the
projection around the component/complexEntry mapping to emit morph tokens like
suffix markers consistently with the canonical source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/FwLite/LcmCrdt/Data/EntryQueryHelpers.cs`:
- Around line 85-91: The headword SQL expression in HeadwordWithTokensExpression
currently returns null for empty citation/lexeme values, which diverges from the
in-memory HeadwordWithTokens contract that returns string.Empty. Update the
final result handling in HeadwordWithTokensExpression so the
Expression<Func<Entry, WritingSystemId, string?, string?, string?>> always
coalesces a null outcome to an empty string, especially after the ConcatWs/Trim
path, and keep the behavior aligned with the nullable ComplexFormHeadword and
ComponentHeadword projections.

---

Nitpick comments:
In `@backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs`:
- Around line 734-736: Update the headword mapping in FwDataMiniLcmApi so
ComponentHeadword and ComplexFormHeadword use the same tokenized CRDT-style
headword representation as QueryHeadwordWithTokens instead of
LexEntryHeadwordOrUnknown; adjust the projection around the
component/complexEntry mapping to emit morph tokens like suffix markers
consistently with the canonical source of truth.
🪄 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: b8611bdc-7292-479c-8e25-84cf1d047ef5

📥 Commits

Reviewing files that changed from the base of the PR and between 5ae0554 and 3e8df1b.

📒 Files selected for processing (5)
  • backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs
  • backend/FwLite/LcmCrdt.Tests/Data/QueryHelperTests.cs
  • backend/FwLite/LcmCrdt/Data/EntryQueryHelpers.cs
  • backend/FwLite/LcmCrdt/LcmCrdtKernel.cs
  • backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs

Comment thread backend/FwLite/LcmCrdt/Data/EntryQueryHelpers.cs
@myieye

myieye commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Your changes changed some verified files. I accepted and commited the changes.

  • one change demonstrates your cool fix
  • another change demonstrates that we used to fallback to other writing-systems if the default was empty. Probably not important.

@myieye

myieye commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

The perfectionist in me really wishes that the headword props were multi-strings.
Currently I think we're actually super close to being able to treat any vernacular as the default vernacular.
These properties being only strings are perhaps the only big missing puzzle piece.

@hahn-kev

Copy link
Copy Markdown
Collaborator Author

Why would we want to treat any vernacular as the default? is that because of custom views?

@myieye

myieye commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Why would we want to treat any vernacular as the default? is that because of custom views?

Right. If a user has a custom-view that excludes (or reorders) the project's default vernacular WS.

That could happen for reasons such as:

  • They are more familiar with a different script
  • They are more familiar with a specific dialect

But, I realize now that that's unrelated to this PR. This PR is obviously a clear improvement. Expanding the Headword to a multi-string (and maybe including homograph numbers and sense numbers too, which FieldWorks also displays 🤦) are separate issues - they just touch the same code.

…random sync

CanSyncRandomEntries can't assert the now-derived ComplexFormHeadword/
ComponentHeadword: they're recomputed from the referenced entry's default
vernacular writing system (with morph tokens), which won't match the
randomly-generated Entry.Headword() inputs (those pick a writing system by code
and apply no tokens). Exclude them there unconditionally and instead add
ComplexFormComponentHeadwords_UpdateWhenReferencedEntriesChange, which positively
shows, for both CRDT and FwData, that the headwords update live when a referenced
entry changes, including morph tokens (suffix "s" -> "-s", edited to "-es").

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

@myieye myieye left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed a test and added another.

@hahn-kev hahn-kev merged commit 8dd70ba into develop Jun 30, 2026
21 checks passed
@hahn-kev hahn-kev deleted the show-affix-on-components branch June 30, 2026 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

💻 FW Lite issues related to the fw lite application, not miniLcm or crdt related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants