Open
Conversation
CRDT CRUD operations for MorphTypeData are not yet implemented, so this will fail to sync and throw a NotImplementedException if you actually try to sync.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request adds comprehensive MorphTypeData support to the CRDT system, including database migrations, change tracking, API implementations, sync logic, and test infrastructure. Key features include unique constraint enforcement on MorphType, immutability of MorphType post-creation, duplicate prevention, and nullable LeadingToken/TrailingToken properties. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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
the
💻 FW Lite
Contributor
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
In liblcm, the .Prefix and .Postfix properties can be (and often are) null, so the corresponding LeadingToken and TrailingToken properties in MorphTypeData need to be nullable as well. The UI will have to do ?? "" in appropriate places as it displays the leading/trailing tokens.
rmunn
changed the title
Created the change but forgot to include it in what the method returns.
Test was failing because mock didn't have new API registered so was returning null.
rmunn
force-pushed
the
feat/sync-morph-types
branch
from
8003c78 to
7563c10
Compare
This one used yet another snapshot file that I hadn't updated yet
Contributor
Author
|
Kind of bothered by the fact that I don't see FwDataApiDoesNotUpdateMorphType failing yet; that test is supposed to start failing once we start syncing morph type. The fact that it's not failing yet may mean I've failed to implement something. |
This time instead of an empty list of morph types, we'll use the actual morph types found in the .fwdata file, which is what the test will actually see.
Needed both the Snapshot *and* the sqlite3 database to be updated to expect the new MorphTypeData table.
Check in two places: the API, which is the most likely place to catch duplicates, but also in the Change object itself, in case two people somehow end up submitting the same change twice (e.g., one person was offline for a period of time).
Contributor
Author
|
Okay, tests are finally green, so I can finally push the "don't create duplicate MorphTypes" logic and make sure it doesn't break the tests. |
Now that the API prevents creating duplicate MorphTypes, we can add a database constraint to enforce that at the DB level.
Instead of passing all the object's properties as separate params to CreateMorphTypeDataChange, it's much simpler to just pass in the entire object, similarly to how it's done for Entry and other large objects.
Turns out we don't need ON CONFLICT IGNORE since the wrappers we have built around EF Core will handle it for us: any object that gets created in an already-deleted state (as we do for duplicate morph types) will never be sent to the DB in the first place. So it's safe to use EF Core's API for the migration after all, keeping it consistent with how the other migrations are written.
Editing typo that caused far too much head-scratching
Contributor
Author
|
@CodeRabbit fullreview |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/FwLite/MiniLcm/MiniLcmApiExtensions.cs (1)
20-20: SortAllMorphTypeDatabefore snapshotting.This new array currently inherits repository iteration order, so snapshot files can churn on order-only changes. The new
WithoutStrictOrderingFor(x => x.AllMorphTypeData)inbackend/FwLite/LcmCrdt.Tests/SnapshotAtCommitServiceTests.csis already compensating for that. Sorting here by an immutable key such asMorphTypekeeps snapshots deterministic.♻️ Proposed fix
- await api.GetAllMorphTypeData().ToArrayAsync(), + (await api.GetAllMorphTypeData().ToArrayAsync()) + .OrderBy(m => m.MorphType) + .ToArray(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/FwLite/MiniLcm/MiniLcmApiExtensions.cs` at line 20, The array returned by awaiting api.GetAllMorphTypeData() is not ordered and causes snapshot churn; modify the call in MiniLcmApiExtensions (the invocation of GetAllMorphTypeData().ToArrayAsync()) to sort the sequence by the immutable key MorphType before materializing (e.g., OrderBy(m => m.MorphType) or equivalent) so AllMorphTypeData is deterministic when snapshotting.backend/FwLite/MiniLcm/Models/MorphType.cs (1)
38-39: Potential sync churn between null and empty string.The
SimpleStringDiff.GetStringDiff()method (inMiniLcm/SyncHelpers/SimpleStringDiff.cs) uses direct equality comparison (before == after), which treatsnulland""as distinct values. If FwData stores empty strings for Prefix/Postfix while CRDT storesnull, each sync cycle may generate spurious patch operations.Per the retrieved learnings, the two-pass sync pattern prevents data loss — any null in CRDT gets reconciled from FwData in pass 1 before pass 2 runs. However, you may want to consider normalizing empty strings to null (or vice versa) in
GetStringDiffor during entity creation to avoid unnecessary operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/FwLite/MiniLcm/Models/MorphType.cs` around lines 38 - 39, The equality check in SimpleStringDiff.GetStringDiff treats null and "" as different, causing churn with MorphType's LeadingToken/TrailingToken; update GetStringDiff to normalize string values before comparison (e.g., map both null and empty string to a canonical value) or add normalization when creating/updating MorphType entities so LeadingToken and TrailingToken are consistently stored (null or ""), ensuring comparisons like in GetStringDiff use the normalized values to avoid spurious diffs.
🤖 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/LcmCrdt/CrdtMiniLcmApi.cs`:
- Around line 352-365: Filter out tombstoned rows by only querying live rows
(where DeletedAt == null) in both GetAllMorphTypeData and GetMorphTypeData:
replace usages of repo.AllMorphTypeData with a filtered query (e.g.,
repo.AllMorphTypeData.Where(x => x.DeletedAt == null) or call a repository
helper that returns only live rows) so GetAllMorphTypeData yields only
non-deleted MorphTypeData and GetMorphTypeData uses the same filtered source
when calling SingleOrDefaultAsync; this ensures DeleteMorphTypeData tombstones
won’t be returned by these APIs.
---
Nitpick comments:
In `@backend/FwLite/MiniLcm/MiniLcmApiExtensions.cs`:
- Line 20: The array returned by awaiting api.GetAllMorphTypeData() is not
ordered and causes snapshot churn; modify the call in MiniLcmApiExtensions (the
invocation of GetAllMorphTypeData().ToArrayAsync()) to sort the sequence by the
immutable key MorphType before materializing (e.g., OrderBy(m => m.MorphType) or
equivalent) so AllMorphTypeData is deterministic when snapshotting.
In `@backend/FwLite/MiniLcm/Models/MorphType.cs`:
- Around line 38-39: The equality check in SimpleStringDiff.GetStringDiff treats
null and "" as different, causing churn with MorphType's
LeadingToken/TrailingToken; update GetStringDiff to normalize string values
before comparison (e.g., map both null and empty string to a canonical value) or
add normalization when creating/updating MorphType entities so LeadingToken and
TrailingToken are consistently stored (null or ""), ensuring comparisons like in
GetStringDiff use the normalized values to avoid spurious diffs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e566f485-c8f8-4d47-8bff-4847158d06e7
📒 Files selected for processing (34)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.csbackend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateMorphTypeDataProxy.csbackend/FwLite/FwLiteProjectSync.Tests/Import/ResumableTests.csbackend/FwLite/FwLiteProjectSync.Tests/ProjectSnapshotSerializationTests.csbackend/FwLite/FwLiteProjectSync.Tests/Snapshots/sena-3_snapshot.2026-03-11.verified.txtbackend/FwLite/FwLiteProjectSync.Tests/SyncTests.csbackend/FwLite/FwLiteProjectSync.Tests/sena-3-live.verified.sqlitebackend/FwLite/FwLiteProjectSync.Tests/sena-3-live_snapshot.verified.txtbackend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.csbackend/FwLite/FwLiteProjectSync/Import/ResumableImportApi.csbackend/FwLite/FwLiteProjectSync/MiniLcmImport.csbackend/FwLite/LcmCrdt.Tests/Changes/ChangeDeserializationRegressionData.latest.verified.txtbackend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.csbackend/FwLite/LcmCrdt.Tests/Data/MigrationTests.csbackend/FwLite/LcmCrdt.Tests/Data/MigrationTests_FromScriptedDb.v1.ProjectSnapshot.verified.txtbackend/FwLite/LcmCrdt.Tests/Data/MigrationTests_FromScriptedDb.v2.ProjectSnapshot.verified.txtbackend/FwLite/LcmCrdt.Tests/Data/SnapshotDeserializationRegressionData.latest.verified.txtbackend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyChangeModels.verified.txtbackend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txtbackend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyIObjectWithIdModels.verified.txtbackend/FwLite/LcmCrdt.Tests/SnapshotAtCommitServiceTests.csbackend/FwLite/LcmCrdt/Changes/CreateMorphTypeDataChange.csbackend/FwLite/LcmCrdt/CrdtMiniLcmApi.csbackend/FwLite/LcmCrdt/Data/MiniLcmRepository.csbackend/FwLite/LcmCrdt/LcmCrdtDbContext.csbackend/FwLite/LcmCrdt/LcmCrdtKernel.csbackend/FwLite/LcmCrdt/Migrations/20260306045826_AddMorphTypeDataTable.Designer.csbackend/FwLite/LcmCrdt/Migrations/20260306045826_AddMorphTypeDataTable.csbackend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.csbackend/FwLite/MiniLcm/MiniLcmApiExtensions.csbackend/FwLite/MiniLcm/Models/IObjectWithId.csbackend/FwLite/MiniLcm/Models/MorphType.csbackend/FwLite/MiniLcm/ProjectSnapshot.csbackend/FwLite/MiniLcm/SyncHelpers/MorphTypeDataSync.cs
myieye
reviewed
Mar 12, 2026
Collaborator
myieye
left a comment
myieye
left a comment
There was a problem hiding this comment.
Just two small things I noticed before I do a deeper investigation.
Comment on lines
+29
to
+31
| public MultiString? Name { get; set; } | ||
| public MultiString? Abbreviation { get; set; } | ||
| public RichMultiString? Description { get; set; } |
Collaborator
There was a problem hiding this comment.
These are all non-nullable in MorphTypeData. So, I think it makes sense for them to be non-nullable here too.
Then you can remove the occurences of ?? [] in NewEntity below.
Suggested change
| public MultiString? Name { get; set; } | |
| public MultiString? Abbreviation { get; set; } | |
| public RichMultiString? Description { get; set; } | |
| public MultiString Name { get; set; } | |
| public MultiString Abbreviation { get; set; } | |
| public RichMultiString Description { get; set; } |
Contributor
Author
There was a problem hiding this comment.
Had to make them nullable or else the parameterless JSON constructor was erroring: the compiler was saying "Hey, you have a constructor that doesn't set all the values". I've run into this behavior before and I don't remember how to tell C# what I'm doing here. The easiest way to get the compiler to shut up was to make these all nullable. Is there a better way? What did I miss/forget?
Collaborator
There was a problem hiding this comment.
Oh, I see. I think you need to mark them as required? And I wonder if init would be better than set? But that's not so important. I'm not sure if we've been consistent at all.
Contributor
Author
There was a problem hiding this comment.
backend/FwLite/FwLiteProjectSync.Tests/Import/ResumableTests.cs
Outdated
Show resolved
Hide resolved
Now we get to iterate over the actual enum values, rather than their int equivalents which have to be converted. Which means we can also use the enum names in test strings and so on. Handy.
Next commit will rename the MorphType enum to MorphTypeKind, and the MorphTypeData class to plain MorphType. Also, the MorphType property of the renamed class will become Kind. No changes will be made to the property name in the Entry class, so its JSON will not need to be adjusted; the type of the Entry.MorphType property will be renamed to MorphTypeKind, but that name never shows up in JSON serialization so that's not a problem.
Also rename MorphTypeData (now MorphType)'s property of type MorphTypeKind. Formerly named MorphType, now named Kind. Also rename AllMorphTypeData to MorphTypes, plural, to regain consistency with rest of API. This mostly completes the great renaming.
This may reduce the number of test failures we get, but not to zero since we will likely have missed something in at least one file (and we definitely haven't gotten every verified file yet, as the sena-3 Sqlite file needs to run a DB migration and a sync before it will have its correct contents, so there's going to be at least one test failure on this commit).
Now that the great renaming is complete, the DB migration will have the new table name and column names, so it's time to bring it back.
github-actions
bot
added
the
📦 Lexbox
Contributor
Needs to match the C# name since we autogenerate the Typescript types from the C# types.
Missed these the first time around
Contributor
Author
|
I suspect I need to change the JSON-encoded Sena-3 data in demo-entry-data.ts, but that's going to be a bit of a pain and I don't want to do it right now. I'll do that on Monday. |
Will need to rename some new entries in the demo data from develop, which are causing the tests to fail because they have the old names.
Contributor
Author
|
Turns out the test failures were because of a use of the MorphType enum in |
This allows us to not mark them nullable, even though the parameterless constructor doesn't set them, pushing the burden of setting those properties off onto the calling code that creates the object.
The CreateMorphTypeChange main constructor sets all required properties, but C# needs a compiler hint to tell it so.
Contributor
Author
|
@myieye - Fixed the CreateMorphTypeChange type by using Did you hear from Jason about renaming LeadingToken and TrailingToken? I'm about to do that rename (once I verify that the tests have turned green again), then this should be ready for final approval. I tagged you for re-review already, but if you want to hold off until I push the LeadingToken and TrailingToken rename, that's fine. P.S. I plan to do the rename of LeadingToken and TrailingToken in a single commit, which I will get running locally and then push. That way it will be easy to revert if we decide later that we don't want to match the FwData names. But it seems likely that we'll want to match FwData's names internally, so I'll go ahead with the rename before hearing what Jason thinks¸ as the work isn't likely to be wasted. |
Matches the names from liblcm, which may help reduce confusion.
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
2 participants
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.
Fixes #1847.