Add variant support to FwLite (model, CRDT, FwData bridge, sync)#2408
Closed
myieye wants to merge 10 commits into
Closed
Add variant support to FwLite (model, CRDT, FwData bridge, sync)#2408myieye wants to merge 10 commits into
myieye wants to merge 10 commits into
Conversation
📝 WalkthroughWalkthroughThis PR introduces variant relationships across the FieldWorks Lite stack: new ChangesVariant relationship feature
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
💻 FW Lite
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
One Variant link = one LexEntryRef (RefType=Variant) with per-link Types, HideMinorEntry and Comment (LCM Summary), per VARIANTS.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Conformance suites for both IMiniLcmApi implementations, change/usage/serialization tests, verified-file regens, AutoFaker support. LCM enforces acyclicity over the combined complex-form + variant graph, so AddVariantChange mirrors that check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Regenerated via ProjectTemplateTests.GenerateTemplate; new CRDT projects get the 7 standard FLEx variant types (well-known guids), matching FieldWorks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Variant lists sort by composite key on both sides (deterministic, culture-free); BulkCreateEntries emits variant links; regenerated Sena3 live db + snapshots — sena-3's real variants (custom Pronunciation Variant type) now round-trip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A sense-targeted variant link is identified by its full composite key; the FwData bridge matched by sense guid alone, so a moved sense made the sync drop the link (new regression test covers both implementations). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
myieye
force-pushed
the
feat/variants-backend
branch
from
4722098 to
a5a336f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
frontend/viewer/src/project/demo/demo-entry-data.ts (2)
348-352: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
UNSPECIFIED_VARIANT_TYPE_IDinstead of re-hardcoding the guid.
variant-types.tsalready exportsUNSPECIFIED_VARIANT_TYPE_ID = '3942addb-99fd-43e9-ab7d-99025ceb0d4e'. This array re-declares the same literal, so if the well-known guid ever changes it must be updated in two places.♻️ Proposed fix
+import {UNSPECIFIED_VARIANT_TYPE_ID} from '$project/data/variant-types'; + export const variantTypes: IVariantType[] = [ - {id: '3942addb-99fd-43e9-ab7d-99025ceb0d4e', name: {en: 'Unspecified Variant'}}, + {id: UNSPECIFIED_VARIANT_TYPE_ID, name: {en: 'Unspecified Variant'}}, {id: '024b62c9-93b3-41a0-ab19-587a0030219a', name: {en: 'Dialectal Variant'}}, {id: '0c4663b3-4d9a-47af-b9a1-c8565d8112ed', name: {en: 'Spelling Variant'}}, ];🤖 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 `@frontend/viewer/src/project/demo/demo-entry-data.ts` around lines 348 - 352, The `variantTypes` demo data is hardcoding the same UUID used by `UNSPECIFIED_VARIANT_TYPE_ID`, so replace the literal in `demo-entry-data.ts` with the exported constant from `variant-types.ts`. Update the `variantTypes` array to reference `UNSPECIFIED_VARIANT_TYPE_ID` for the “Unspecified Variant” entry and keep the other variant entries unchanged.
354-368: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded array indices for seeding the demo variant pair are fragile.
entries[2]/entries[3]assume a fixed entries ordering. A later reorder or insertion earlier in the array would silently link the wrong entries as variant/main. Since this is demo-only seed data the impact is limited, but consider selecting entries by a stable id instead of index.🤖 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 `@frontend/viewer/src/project/demo/demo-entry-data.ts` around lines 354 - 368, The demo variant seeding in demo-entry-data is relying on fragile fixed array positions for entries[2] and entries[3]. Update the logic around demoVariantEntry and demoMainEntry to select the intended entries by a stable identifier or other explicit property instead of index, then keep the existing demoVariant wiring to variantOf and variants using those resolved entries.frontend/viewer/src/project/data/variant-types.ts (1)
1-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
UNSPECIFIED_VARIANT_TYPE_IDduplicated as a literal elsewhere.The same GUID (
'3942addb-99fd-43e9-ab7d-99025ceb0d4e') is hardcoded again indemo-entry-data.ts'svariantTypesarray instead of referencing this constant. Consider importingUNSPECIFIED_VARIANT_TYPE_IDthere to avoid drift if the well-known guid ever needs updating.🤖 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 `@frontend/viewer/src/project/data/variant-types.ts` around lines 1 - 11, The well-known variant GUID is duplicated as a hardcoded string instead of reusing the shared constant. Update the `variantTypes` array in `demo-entry-data.ts` to import and reference `UNSPECIFIED_VARIANT_TYPE_ID` from `variant-types.ts` so the `useVariantTypes`/`UNSPECIFIED_VARIANT_TYPE_ID` source of truth stays consistent and avoids drift.backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/EntryFakerHelper.cs (1)
31-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent "false" handling vs. sibling flags.
The new
createVariants: falsebranch explicitly clearsentry.VariantOf/entry.Variants, but the sibling flags (createComponents,createComplexForms,createComplexFormTypes,createPublications) have no equivalent clearing when disabled — they leave AutoFaker-generated, uncommitted references in the entry. Worth confirming whether the other flags should behave the same way for consistency, or whether variants specifically need this due to the new cycle/reference validators added in this PR.🤖 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/MiniLcm.Tests/AutoFakerHelpers/EntryFakerHelper.cs` around lines 31 - 47, The false-branch handling in EntryFakerHelper is inconsistent: CreateVariantLinkEntry clears VariantOf/Variants when createVariants is disabled, but the sibling flags in the same helper do not clear their AutoFaker-generated collections. Review the CreateComplexFormComponentEntry, CreateComplexFormTypes, and CreatePublications paths in EntryFakerHelper and make their disabled behavior match the intended contract—either clear the corresponding entry collections too, or keep the variant-only clearing if it is required for the new validators and document that distinction.
🤖 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/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs`:
- Around line 1479-1481: Add a validation in AddVariant before resolving target
so that when variant.MainSenseId is present it confirms the sense belongs to
variant.MainEntryId; otherwise reject the request before any mutation. Use the
existing AddVariant flow in FwDataMiniLcmApi and the
SenseRepository/EntriesRepository lookup path to compare the sense’s owning
entry against variant.MainEntryId, then only create the variant link if they
match so CreateVariant and UpdateVariant keep working with the same composite
key.
In `@backend/FwLite/LcmCrdt/Changes/Entries/AddVariantChange.cs`:
- Around line 45-54: The shouldBeDeleted check in AddVariantChange.NewEntity
does not treat a missing mainSense the same way as missing or deleted entry
records, so a dangling MainSenseId can keep the variant alive. Update the
condition around MainSenseId/MainSenseId.Value to consider mainSense being null
as deletion-equivalent, consistent with the variantEntry and mainEntry checks,
so the variant is marked deleted whenever the referenced sense is absent or
deleted.
In `@backend/FwLite/LcmCrdt/Migrations/20260703134525_AddVariants.cs`:
- Around line 95-107: Update the unique index definitions in AddVariants so
soft-deleted rows are excluded from uniqueness checks. In
20260703134525_AddVariants.cs, adjust both CreateIndex calls for
IX_Variants_VariantEntryId_MainEntryId and
IX_Variants_VariantEntryId_MainEntryId_MainSenseId to include DeletedAt IS NULL
in their filter alongside the existing MainSenseId condition. This keeps
AddVariantChange from failing when it writes a duplicate Variant with DeletedAt
set.
In `@backend/FwLite/MiniLcm/Validators/EntryValidator.cs`:
- Around line 25-31: The EntryValidator rules for e.Variants currently rely on
VariantValidator, which still permits an empty endpoint and lets VariantEntryId
be left blank. Add an explicit non-empty validation for the variant side in
EntryValidator alongside the existing NotBeVariantSelfReference and
HaveCorrectMainEntryReference checks, using the e.Variants RuleForEach chain so
incomplete variant links are rejected before SetValidator(new
VariantValidator()) runs.
In `@backend/FwLite/MiniLcm/Validators/MiniLcmApiValidationWrapper.cs`:
- Around line 132-155: The submit write paths are bypassing validation because
MiniLcmApiValidationWrapper only overrides the before/after update methods and
not the patch-based submit members. Add explicit overrides for
SubmitUpdateVariant and SubmitUpdateVariantType in MiniLcmApiValidationWrapper
so they validate the incoming update payload through VariantValidator-related
checks before forwarding to _api, and make sure the submit variants do not rely
on the default IMiniLcmWriteApi implementation path that skips this wrapper.
---
Nitpick comments:
In `@backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/EntryFakerHelper.cs`:
- Around line 31-47: The false-branch handling in EntryFakerHelper is
inconsistent: CreateVariantLinkEntry clears VariantOf/Variants when
createVariants is disabled, but the sibling flags in the same helper do not
clear their AutoFaker-generated collections. Review the
CreateComplexFormComponentEntry, CreateComplexFormTypes, and CreatePublications
paths in EntryFakerHelper and make their disabled behavior match the intended
contract—either clear the corresponding entry collections too, or keep the
variant-only clearing if it is required for the new validators and document that
distinction.
In `@frontend/viewer/src/project/data/variant-types.ts`:
- Around line 1-11: The well-known variant GUID is duplicated as a hardcoded
string instead of reusing the shared constant. Update the `variantTypes` array
in `demo-entry-data.ts` to import and reference `UNSPECIFIED_VARIANT_TYPE_ID`
from `variant-types.ts` so the `useVariantTypes`/`UNSPECIFIED_VARIANT_TYPE_ID`
source of truth stays consistent and avoids drift.
In `@frontend/viewer/src/project/demo/demo-entry-data.ts`:
- Around line 348-352: The `variantTypes` demo data is hardcoding the same UUID
used by `UNSPECIFIED_VARIANT_TYPE_ID`, so replace the literal in
`demo-entry-data.ts` with the exported constant from `variant-types.ts`. Update
the `variantTypes` array to reference `UNSPECIFIED_VARIANT_TYPE_ID` for the
“Unspecified Variant” entry and keep the other variant entries unchanged.
- Around line 354-368: The demo variant seeding in demo-entry-data is relying on
fragile fixed array positions for entries[2] and entries[3]. Update the logic
around demoVariantEntry and demoMainEntry to select the intended entries by a
stable identifier or other explicit property instead of index, then keep the
existing demoVariant wiring to variantOf and variants using those resolved
entries.
🪄 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: 61e6aa41-388a-410b-aa71-150855376533
📒 Files selected for processing (87)
backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/VariantTests.csbackend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.csbackend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateVariantProxy.csbackend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateVariantTypeProxy.csbackend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.csbackend/FwLite/FwLiteProjectSync.Tests/Import/ResumableTests.csbackend/FwLite/FwLiteProjectSync.Tests/Snapshots/sena-3_snapshot.2026-06-18.verified.txtbackend/FwLite/FwLiteProjectSync.Tests/SyncTests.csbackend/FwLite/FwLiteProjectSync.Tests/UpdateDiffTests.csbackend/FwLite/FwLiteProjectSync.Tests/VariantSyncTests.csbackend/FwLite/FwLiteProjectSync.Tests/sena-3-live.verified.sqlitebackend/FwLite/FwLiteProjectSync.Tests/sena-3-live_snapshot.verified.txtbackend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.csbackend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.csbackend/FwLite/FwLiteProjectSync/Import/ResumableImportApi.csbackend/FwLite/FwLiteShared/Services/MiniLcmApiNotifyWrapper.csbackend/FwLite/FwLiteShared/Services/MiniLcmJsInvokable.csbackend/FwLite/FwLiteWeb/Hubs/MiniLcmApiHubBase.csbackend/FwLite/LcmCrdt.Tests/Changes/ChangeDeserializationRegressionData.latest.verified.txtbackend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.csbackend/FwLite/LcmCrdt.Tests/Changes/VariantChangeTests.csbackend/FwLite/LcmCrdt.Tests/Data/MigrationTests.csbackend/FwLite/LcmCrdt.Tests/Data/MigrationTests_FromScriptedDb.v1.ProjectSnapshot.verified.txtbackend/FwLite/LcmCrdt.Tests/Data/MigrationTests_FromScriptedDb.v1.Snapshots.verified.txtbackend/FwLite/LcmCrdt.Tests/Data/MigrationTests_FromScriptedDb.v2.ProjectSnapshot.verified.txtbackend/FwLite/LcmCrdt.Tests/Data/MigrationTests_FromScriptedDb.v2.Snapshots.verified.txtbackend/FwLite/LcmCrdt.Tests/Data/SnapshotDeserializationRegressionData.latest.verified.txtbackend/FwLite/LcmCrdt.Tests/Data/SnapshotDeserializationRegressionData.legacy.verified.txtbackend/FwLite/LcmCrdt.Tests/Data/VerifyRegeneratedSnapshotsAfterMigrationFromScriptedDb.v1.verified.jsonbackend/FwLite/LcmCrdt.Tests/Data/VerifyRegeneratedSnapshotsAfterMigrationFromScriptedDb.v2.verified.jsonbackend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyChangeModels.verified.txtbackend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txtbackend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyIObjectWithIdModels.verified.txtbackend/FwLite/LcmCrdt.Tests/MiniLcmTests/VariantTests.csbackend/FwLite/LcmCrdt.Tests/SnapshotAtCommitServiceTests.csbackend/FwLite/LcmCrdt/Changes/CreateVariantType.csbackend/FwLite/LcmCrdt/Changes/Entries/AddVariantChange.csbackend/FwLite/LcmCrdt/Changes/Entries/AddVariantTypeChange.csbackend/FwLite/LcmCrdt/Changes/Entries/RemoveVariantTypeChange.csbackend/FwLite/LcmCrdt/CrdtMiniLcmApi.csbackend/FwLite/LcmCrdt/Data/EntryQueryHelpers.csbackend/FwLite/LcmCrdt/Data/MiniLcmRepository.csbackend/FwLite/LcmCrdt/LcmCrdtDbContext.csbackend/FwLite/LcmCrdt/LcmCrdtKernel.csbackend/FwLite/LcmCrdt/Migrations/20260703134525_AddVariants.Designer.csbackend/FwLite/LcmCrdt/Migrations/20260703134525_AddVariants.csbackend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.csbackend/FwLite/LcmCrdt/QueryHelpers.csbackend/FwLite/LcmCrdt/Templates/blank-project-template.jsonbackend/FwLite/MiniLcm.Tests/AutoFakerHelpers/EntryFakerHelper.csbackend/FwLite/MiniLcm.Tests/FluentAssertGlobalConfig.csbackend/FwLite/MiniLcm.Tests/Validators/VariantTypeValidationTests.csbackend/FwLite/MiniLcm.Tests/Validators/VariantValidationTests.csbackend/FwLite/MiniLcm.Tests/VariantTestsBase.csbackend/FwLite/MiniLcm/CreateEntryOptions.csbackend/FwLite/MiniLcm/IMiniLcmReadApi.csbackend/FwLite/MiniLcm/IMiniLcmWriteApi.csbackend/FwLite/MiniLcm/Import/ProjectImporter.csbackend/FwLite/MiniLcm/MiniLcmApiExtensions.csbackend/FwLite/MiniLcm/Models/Entry.csbackend/FwLite/MiniLcm/Models/IObjectWithId.csbackend/FwLite/MiniLcm/Models/Variant.csbackend/FwLite/MiniLcm/Models/VariantType.csbackend/FwLite/MiniLcm/Normalization/MiniLcmApiWriteNormalizationWrapper.csbackend/FwLite/MiniLcm/ProjectSnapshot.csbackend/FwLite/MiniLcm/SyncHelpers/EntrySync.csbackend/FwLite/MiniLcm/SyncHelpers/VariantSync.csbackend/FwLite/MiniLcm/SyncHelpers/VariantTypeSync.csbackend/FwLite/MiniLcm/Validators/EntryValidator.csbackend/FwLite/MiniLcm/Validators/MiniLcmApiValidationWrapper.csbackend/FwLite/MiniLcm/Validators/MiniLcmValidators.csbackend/FwLite/MiniLcm/Validators/VariantTypeValidator.csbackend/FwLite/MiniLcm/Validators/VariantValidator.csbackend/FwLite/VARIANTS.mdbackend/LfClassicData/LfClassicMiniLcmApi.csfrontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMiniLcmJsInvokable.tsfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IEntry.tsfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IVariant.tsfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IVariantType.tsfrontend/viewer/src/lib/dotnet-types/index.tsfrontend/viewer/src/lib/utils.tsfrontend/viewer/src/project/data/index.tsfrontend/viewer/src/project/data/variant-types.tsfrontend/viewer/src/project/demo/demo-entry-data.tsfrontend/viewer/src/project/demo/in-memory-demo-api.tsfrontend/viewer/src/stories/editor/entity-primitives/entry-editor-primitive.stories.sveltefrontend/viewer/tests/entry-api-helper.ts
…riant entry id Both implementations now reject a MainSenseId that belongs to a different entry (fail before mutation); EntryValidator requires VariantEntryId on Variants items. Also corrects the stale cycle-guard decision text in VARIANTS.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
Closing per the iterative review workflow (not merged, not abandoned): this backend step is now CodeRabbit-handled, Devin-clean and CI-green at 55d4485. The next PR carries this step plus the viewer UI on top; the steps will be reopened/merged one by one when review completes. See backend/FwLite/VARIANTS.md for the feature log. |
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.
Adds variants end-to-end in the backend: one MiniLcm
Variantlink = one FieldWorksLexEntryRef(RefType=Variant), carrying per-linkTypes,HideMinorEntryandCommentso FLEx data round-trips losslessly. Entry gainsVariantOf/Variants;VariantTypeis a first-class synced object. Design, decisions and behavioral differences vs complex forms are documented inbackend/FwLite/VARIANTS.md.LexEntryRef.ValidateAddObjectInternal); chains are allowed, cycles/self-refs are not.GetVariantTypesreads the possibility list flattened so the Irregularly Inflected Form subtypes (Plural/Past) are assignable — the headline use case (forum thread).🤖 Generated with Claude Code