Conversation
📝 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 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
the
💻 FW Lite
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
rmunn
changed the title
rmunn
force-pushed
the
feat/sync-morph-types
branch
from
8003c78 to
7563c10
Compare
|
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. |
|
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/FwLite/LcmCrdt/LcmCrdtKernel.cs (1)
254-260:⚠️ Potential issue | 🟠 MajorUse a custom patch change for
MorphTypeDatathat preventsMorphTypefield updates.
JsonPatchChange<MorphTypeData>allows arbitrary patches to all fields, includingMorphType. However,CreateMorphTypeDataChangeenforces a unique constraint onMorphTypeonly at creation time. Updates via generic patch can bypass this, allowing one record to be patched to duplicate another'sMorphType, and can changeMorphTypeafter creation in violation of immutability. Create a dedicated change class for updates that rejects/MorphTypeoperations and only allows mutations toName,Abbreviation,Description,LeadingToken,TrailingToken, andSecondaryOrder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/FwLite/LcmCrdt/LcmCrdtKernel.cs` around lines 254 - 260, Replace the generic JsonPatchChange<MorphTypeData> with a custom patch-change implementation (e.g., MorphTypeDataPatchChange) and register that instead in config.ChangeTypeListBuilder; implement MorphTypeDataPatchChange to accept only JSON-Patch operations addressing Name, Abbreviation, Description, LeadingToken, TrailingToken, and SecondaryOrder, explicitly reject any operation whose path targets /MorphType (return validation/error), and reuse CreateMorphTypeDataChange for creation-only logic so updates cannot bypass the uniqueness/immutability constraint on MorphType.
🧹 Nitpick comments (2)
backend/FwLite/FwLiteProjectSync.Tests/Snapshots/sena-3_snapshot.2025-09-30.verified.txt (1)
124315-124315: Avoid leaving the gold-standard snapshot with zero morph-type coverage.With
AllMorphTypeDataempty here, the Sena3 snapshot still won’t catch regressions in MorphTypeData extraction/serialization/round-tripping. Since this PR’s core behavior is morph type sync, please consider backing it with at least one fixture or assertion that produces a non-emptyAllMorphTypeDataentry.Based on learnings: The Sena3SyncTests class uses a real FwData project and represents the gold standard for sync testing; when fixing a sync bug, reproduce it with a test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/FwLite/FwLiteProjectSync.Tests/Snapshots/sena-3_snapshot.2025-09-30.verified.txt` at line 124315, The snapshot currently has an empty "AllMorphTypeData" which will let regressions in morph-type extraction/serialization slip; update the Sena3SyncTests setup to produce and persist at least one MorphTypeData entry so the snapshot contains a non-empty AllMorphTypeData. Specifically, in the Sena3SyncTests test fixture or test data builder that seeds the real FwData project, create a representative MorphType (or reuse an existing one) and ensure the sync/export path populates AllMorphTypeData before the snapshot is written, then re-run the test to regenerate the gold snapshot so the file shows a non-empty AllMorphTypeData array.backend/FwLite/MiniLcm/MiniLcmApiExtensions.cs (1)
20-20: MakeAllMorphTypeDataordering deterministic at snapshot creation.Two test helpers in this PR now had to opt out of strict ordering for
AllMorphTypeData. Sorting here by the stable morph-type key would keepProjectSnapshotdeterministic instead of teaching every comparison to ignore order.🤖 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 call to GetAllMorphTypeData() should produce a deterministic ordering for ProjectSnapshot; update the snapshot creation path in MiniLcmApiExtensions by ordering the result of GetAllMorphTypeData() by the stable morph-type key (the property used as the morph-type identifier) before materializing the array (i.e., insert an OrderBy on the morph-type key prior to ToArrayAsync()); this ensures AllMorphTypeData is consistently ordered and removes the need for tests to ignore ordering.
🤖 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/FwLiteProjectSync.Tests/Import/ResumableTests.cs`:
- Around line 83-94: The test currently doesn't validate resumable morph-type
imports because the mock source data from GetAllMorphTypeData isn't asserted
after import and UnreliableApi isn't configured to inject failures for
morph-type writes; update the ResumableTests.cs test to (1) assert that the new
MorphTypeData appears in the destination ProjectSnapshot after running
CrdtFwdataProjectSyncService.Sync(), (2) route morph-type create calls
(CreateMorphTypeData) through UnreliableApi.MaybeThrowRandom so failures are
injected during writes, and (3) optionally run the sync in a dry-run via
DryRunMiniLcmApi to inspect expected changes while debugging the resume behavior
in CrdtFwdataProjectSyncService.Sync and ProjectSnapshot handling.
In
`@backend/FwLite/LcmCrdt.Tests/Changes/ChangeDeserializationRegressionData.latest.verified.txt`:
- Around line 826-827: The test corpus only contains non-null "LeadingToken" and
"TrailingToken" values, so add a new regression entry in
ChangeDeserializationRegressionData.latest.verified.txt that exercises the
nullable shape by including at least one object where "LeadingToken": null
and/or "TrailingToken": null (preserving other required fields), so
deserialization of the new nullable properties is covered; locate the JSON block
containing "LeadingToken" and "TrailingToken" and add an additional case with
the null token(s) and appropriate context strings to verify parsing.
In
`@backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyChangeModels.verified.txt`:
- Around line 43-46: The UpdateMorphTypeData endpoint currently accepts
JsonPatchChange<MorphTypeData> without field-level checks, allowing modification
of the immutable MorphType field; add validation in UpdateMorphTypeData(Guid id,
UpdateObjectInput<MorphTypeData> update) that inspects the incoming patch
operations and rejects any operation whose path targets "MorphType" (or
equivalent casing) before applying changes, or alternatively restrict the
generic JsonPatchChange<MorphTypeData> registration so it only permits the exact
set of patchable properties used by MorphTypeDataSync (i.e., exclude
"MorphType"); ensure you use the existing JsonPatchValidator logic as a base and
extend it to check for literal field paths (not only array/index ops) and return
a validation error when a forbidden path is present.
In `@backend/FwLite/LcmCrdt/Changes/CreateMorphTypeDataChange.cs`:
- Around line 21-32: The current CreateMorphTypeDataChange returns a new
MorphTypeData even when an existing row with the same MorphType exists, which
will violate the new unique index; change the behavior in
CreateMorphTypeDataChange so that when alreadyExists is true you do not insert a
new row but instead locate the existing MorphTypeData (query by MorphType) and
update/return that entity (e.g., set its DeletedAt to commit.DateTime or simply
return the existing instance) rather than constructing a new MorphTypeData with
the same MorphType; update the code paths that reference MorphTypeData creation
to use the located entity instead.
In `@backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs`:
- Around line 389-392: The UpdateMorphTypeData method currently accepts any
JsonPatchChange<MorphTypeData>, allowing clients to modify the immutable
MorphType/Kind; detect and reject any patch operations that target the MorphType
(e.g. op.Path equals "/MorphType" or "/Kind", case-insensitive) before calling
AddChange, and return/throw a BadRequest/InvalidOperation indicating MorphType
is immutable. Alternatively, replace the direct JsonPatchChange<MorphTypeData>
usage here with a dedicated change class that only applies allowed mutable
fields and use that class in AddChange; update callers to construct the
dedicated change when necessary. Ensure you reference UpdateMorphTypeData,
JsonPatchChange<MorphTypeData>, AddChange and GetMorphTypeData when making the
change.
- Around line 367-386: In CreateMorphTypeData, the final reload uses the
caller-supplied Id which can fail if a concurrent writer created the same
MorphType after the initial pre-check; change the final fetch to reload the
canonical row by MorphType (e.g. use SingleAsync or FirstAsync on
repo.AllMorphTypeData where c.MorphType == morphTypeData.MorphType) so the
method returns the surviving record after duplicate suppression instead of
assuming the original Id is present.
---
Outside diff comments:
In `@backend/FwLite/LcmCrdt/LcmCrdtKernel.cs`:
- Around line 254-260: Replace the generic JsonPatchChange<MorphTypeData> with a
custom patch-change implementation (e.g., MorphTypeDataPatchChange) and register
that instead in config.ChangeTypeListBuilder; implement MorphTypeDataPatchChange
to accept only JSON-Patch operations addressing Name, Abbreviation, Description,
LeadingToken, TrailingToken, and SecondaryOrder, explicitly reject any operation
whose path targets /MorphType (return validation/error), and reuse
CreateMorphTypeDataChange for creation-only logic so updates cannot bypass the
uniqueness/immutability constraint on MorphType.
---
Nitpick comments:
In
`@backend/FwLite/FwLiteProjectSync.Tests/Snapshots/sena-3_snapshot.2025-09-30.verified.txt`:
- Line 124315: The snapshot currently has an empty "AllMorphTypeData" which will
let regressions in morph-type extraction/serialization slip; update the
Sena3SyncTests setup to produce and persist at least one MorphTypeData entry so
the snapshot contains a non-empty AllMorphTypeData. Specifically, in the
Sena3SyncTests test fixture or test data builder that seeds the real FwData
project, create a representative MorphType (or reuse an existing one) and ensure
the sync/export path populates AllMorphTypeData before the snapshot is written,
then re-run the test to regenerate the gold snapshot so the file shows a
non-empty AllMorphTypeData array.
In `@backend/FwLite/MiniLcm/MiniLcmApiExtensions.cs`:
- Line 20: The call to GetAllMorphTypeData() should produce a deterministic
ordering for ProjectSnapshot; update the snapshot creation path in
MiniLcmApiExtensions by ordering the result of GetAllMorphTypeData() by the
stable morph-type key (the property used as the morph-type identifier) before
materializing the array (i.e., insert an OrderBy on the morph-type key prior to
ToArrayAsync()); this ensures AllMorphTypeData is consistently ordered and
removes the need for tests to ignore ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4371c2dd-bf66-4825-9e2a-ec8409562a53
📒 Files selected for processing (33)
backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateMorphTypeDataProxy.csbackend/FwLite/FwLiteProjectSync.Tests/Import/ResumableTests.csbackend/FwLite/FwLiteProjectSync.Tests/Snapshots/sena-3_snapshot.2025-09-30.verified.txtbackend/FwLite/FwLiteProjectSync.Tests/Snapshots/sena-3_snapshot.2025-09-31.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/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/20260310065457_UniqueIndexOnMorphType.Designer.csbackend/FwLite/LcmCrdt/Migrations/20260310065457_UniqueIndexOnMorphType.csbackend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.csbackend/FwLite/MiniLcm/MiniLcmApiExtensions.csbackend/FwLite/MiniLcm/Models/IObjectWithId.csbackend/FwLite/MiniLcm/Models/MorphType.csbackend/FwLite/MiniLcm/ProjectSnapshot.cs
backend/FwLite/FwLiteProjectSync.Tests/Import/ResumableTests.cs
Outdated
Show resolved
Hide resolved
backend/FwLite/LcmCrdt.Tests/Changes/ChangeDeserializationRegressionData.latest.verified.txt
Outdated
Show resolved
Hide resolved
backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyChangeModels.verified.txt
Show resolved
Hide resolved
|
@CodeRabbit There is one potential issue that you did not mention in your review. The MorphTypeData table in the Sqlite database will have a unique index on the MorphType column (which corresponds to the MorphType enum in C#). In the NewEntity method of the CreateMorphTypeDataChange class, we check whether there is an existing MorphTypeData object with the same MorphType value, and if there is already an existing object, then the new MorphTypeData object should not be created. But because we're in the NewEntity method, which is not allowed to return null (if NewEntity could return null it would break many other parts of the CRDT implementation), we must return something, and so we return an already-deleted object (with the DeletedAt column being populated). However, in Sqlite there is no way to add a WHERE clause to a unique constraint: you cannot say "the MorphType column must be unique only among rows where DeletedAt is null". And so it is possible for CreateMorphTypeDataChange.NewEntity to return a MorphType object that will violate the unique constraint, even though the code attempts to avoid that scenario. This is unlikely to happen since the CrdtMiniLcmApi class attempts to prevent the creation of duplicates. But there still exists a possibility that a user (let's call him user A) might be offline for a while, and try to create a new MorphType object while offline. This would create a CreateMorphTypeDataChange instance, which would be stored untll that user comes back online. Meanwhile, a different user (let's call him user B) is online, so when he creates a new MorphType object (with the same MorphType value as the one that user A created), his change goes through right away. Later, user A comes back online, so all the CRDT changes that he created while he was offline (including the CreateMorphTypeDataChange) are transmitted to other projects. Now there are two CreateMorphTypeDataChange objects trying to create two MorphTypeData objects with the same MorphType value, and the guard in the API cannot protect against this scenario. This will cause a DB constraint violation, and our CRDT implementation does not handle DB constraint violations: if a constraint is violated, the CRDT sync process will stop and block any further syncs until a developer is able to look at the project and remove the duplicated Change object. The solution I plan to implement is to make the Sqlite constraint use ON CONFLICT IGNORE, which will mean that the conflicting MorphTypeData row will be ignored (skipped), but the rest of the database transaction will proceed. This will mean that creating MorphTypeData objects uses a "first one wins" resolution strategy. This is fine because in FieldWorks Classic, new morph types cannot be created (and we plan to have the same restriction in FieldWorks Lite), so there will never be any actual chance of new user-created MorphTypeData objects conflicting with each other. But the theoretical possibility of a DB constraint violation on this unique constraint still exists, so the best strategy for our use case is to use ON CONFLICT IGNORE on the unique constraint for MorphType. Edit: Actually, I see you did mention this. Good. |
|
Tip For best results, initiate chat on the files or code changes.
One important implementation detail worth flagging: SQLite's To implement this, you'd likely need to either:
Option 1 is cleaner from the schema perspective since the conflict resolution policy is baked into the schema itself, but it does require a table-rebuild migration (which SQLite typically requires for schema changes anyway). Option 2 is more surgical but adds EF Core-level complexity. Either way, it'd be worth adding a comment in the migration or the ✏️ Learnings added
🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes.
One important implementation detail worth flagging: SQLite's -- ❌ Not valid in SQLite
CREATE UNIQUE INDEX IX_MorphTypeData_MorphType ON MorphTypeData (MorphType) ON CONFLICT IGNORE;To use -- ✅ Valid
CREATE TABLE MorphTypeData (
...
MorphType INTEGER NOT NULL UNIQUE ON CONFLICT IGNORE,
...
);Since EF Core doesn't support this natively for SQLite, you have a couple of options for the migration: Option A – Raw SQL table recreation (SQLite style) Because SQLite doesn't support protected override void Up(MigrationBuilder migrationBuilder)
{
// Drop the standalone unique index added in the previous migration
migrationBuilder.DropIndex(
name: "IX_MorphTypeData_MorphType",
table: "MorphTypeData");
// Recreate it as a table-level constraint with ON CONFLICT IGNORE
// This requires rebuilding the table in SQLite
migrationBuilder.Sql(@"
CREATE TABLE MorphTypeData_new (
Id TEXT NOT NULL,
MorphType INTEGER NOT NULL UNIQUE ON CONFLICT IGNORE,
Name TEXT NOT NULL,
Abbreviation TEXT NOT NULL,
Description TEXT NOT NULL,
LeadingToken TEXT,
TrailingToken TEXT,
SecondaryOrder INTEGER NOT NULL,
DeletedAt TEXT,
SnapshotId TEXT,
PRIMARY KEY (Id),
FOREIGN KEY (SnapshotId) REFERENCES Snapshots(Id) ON DELETE SET NULL
);
INSERT INTO MorphTypeData_new SELECT * FROM MorphTypeData;
DROP TABLE MorphTypeData;
ALTER TABLE MorphTypeData_new RENAME TO MorphTypeData;
CREATE UNIQUE INDEX IX_MorphTypeData_SnapshotId ON MorphTypeData (SnapshotId);
");
}Option B – Catch and ignore at the application level Instead of changing the DB schema, catch Option C – Defer to a follow-up issue Since FieldWorks Classic doesn't allow creating new morph types, and FW Lite plans the same restriction, the practical risk is extremely low. This could be tracked as a follow-up hardening issue rather than blocking this PR. Would you like me to open a follow-up issue to track the ✏️ Learnings added
🧠 Learnings used |
|
There is another option for the ON CONFLICT IGNORE unique constraint. Because this PR, which has not been merged yet, contains the DB migration that will create the table in the first place, it is still possible to rewrite the PR to drop the commits creating the previous version of the DB migration, and create a new DB migration that will create the table with the ON CONFLICT IGNORE constraint already existing. @CodeRabbit Please provide sample code for creating the EF Core migration in a database environment where the table does not already exist, using ON CONFLICT IGNORE from the beginning. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2212
Since EF Core's Rewritten using System;
using Microsoft.EntityFrameworkCore.Migrations;
`#nullable` disable
namespace LcmCrdt.Migrations
{
/// <inheritdoc />
public partial class AddMorphTypeDataTable : Migration
{
/// <inheritdoc />
protected override void Up(MigrationBuilder migrationBuilder)
{
// EF Core's CreateTable() API does not support SQLite's ON CONFLICT clause,
// so we use raw SQL to embed UNIQUE ON CONFLICT IGNORE on the MorphType column.
//
// "First one wins" is the correct CRDT conflict-resolution strategy here:
// if two offline users each create a MorphTypeData with the same MorphType enum
// value, the second INSERT is silently ignored rather than crashing the sync.
// In practice this race is extremely unlikely because neither FieldWorks Classic
// nor FW Lite allows users to create new morph types, but we harden the schema
// regardless to prevent a stuck-sync situation.
migrationBuilder.Sql(@"
CREATE TABLE MorphTypeData (
Id TEXT NOT NULL,
MorphType INTEGER NOT NULL UNIQUE ON CONFLICT IGNORE,
Name TEXT NOT NULL,
Abbreviation TEXT NOT NULL,
Description TEXT NOT NULL,
LeadingToken TEXT,
TrailingToken TEXT,
SecondaryOrder INTEGER NOT NULL,
DeletedAt TEXT,
SnapshotId TEXT,
CONSTRAINT PK_MorphTypeData PRIMARY KEY (Id),
CONSTRAINT FK_MorphTypeData_Snapshots_SnapshotId
FOREIGN KEY (SnapshotId) REFERENCES Snapshots (Id) ON DELETE SET NULL
);
");
// Create a *named* unique index on MorphType so that EF Core's model snapshot
// (HasIndex(m => m.MorphType).IsUnique()) stays consistent with the database.
// SQLite allows this alongside the inline UNIQUE constraint above; the inline
// ON CONFLICT IGNORE fires first on any duplicate insert, so this named index
// is never violated in practice.
migrationBuilder.Sql(
"CREATE UNIQUE INDEX IX_MorphTypeData_MorphType ON MorphTypeData (MorphType);");
// SnapshotId does not need conflict-resolution handling, so the EF Core API is fine.
migrationBuilder.CreateIndex(
name: "IX_MorphTypeData_SnapshotId",
table: "MorphTypeData",
column: "SnapshotId",
unique: true);
}
/// <inheritdoc />
protected override void Down(MigrationBuilder migrationBuilder)
{
migrationBuilder.DropTable(name: "MorphTypeData");
}
}
}Key points:
The column types ( ✏️ Learnings added
🧠 Learnings used |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
@CodeRabbit fullreview |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs (1)
395-399:⚠️ Potential issue | 🟠 MajorBlock every patch op that targets
MorphType, not justreplace.Line 397 only catches
Replaceon/MorphType.add,remove,copy, ormoveto that same path still bypass this guard and get forwarded intoJsonPatchChange<MorphTypeData>, so the immutability rule is still escapable.Suggested guard
- if (operations.Any( - op => op.Path == $"/{nameof(MorphTypeData.MorphType)}" && - op.OperationType == SystemTextJsonPatch.Operations.OperationType.Replace)) + if (operations.Any(op => + string.Equals(op.Path, $"/{nameof(MorphTypeData.MorphType)}", StringComparison.OrdinalIgnoreCase))) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs` around lines 395 - 399, The current guard in the update handling (using update.Patch.GetOperations()) only blocks OperationType.Replace against the /MorphType path, allowing other operation types (add/remove/copy/move) to mutate MorphType; change the predicate to block any operation whose Path equals $"/{nameof(MorphTypeData.MorphType)}" regardless of op.OperationType, so no JsonPatch operation can target MorphTypeData.MorphType (this affects the code that builds/forwards JsonPatchChange<MorphTypeData>).
🧹 Nitpick comments (4)
backend/FwLite/MiniLcm/SyncHelpers/MorphTypeDataSync.cs (1)
22-28: Don't silently swallow immutable-kind drift.Returning
0preserves the invariant, but sync callers only treat that as “no counted change.” If this branch ever fires, the two stores can stay divergent with no signal. Please emit a warning/metric here so unexpectedMorphTypemutations are diagnosable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/FwLite/MiniLcm/SyncHelpers/MorphTypeDataSync.cs` around lines 22 - 28, The branch in MorphTypeDataSync that returns 0 when before.MorphType != after.MorphType silently swallows an immutable-kind drift; update the branch inside the method where the code checks if (before.MorphType != after.MorphType) to emit a diagnostic (e.g., process/logger warning and/or increment a metric counter) before returning 0 so callers and observability systems see that a rejected MorphType change occurred; reference the before.MorphType != after.MorphType check and the return 0 in MorphTypeDataSync to add the warning/metric increment and include contextual identifiers (object Id, before/after values) in the message.backend/FwLite/FwLiteProjectSync/MiniLcmImport.cs (1)
73-76: Log the canonical morph-type ID returned by the write API.
CreateMorphTypeData()de-dupes byMorphType, so it can hand back an existing row with a differentIdthanmorphTypeData.Id. Logging the source Guid here makes retries/races look like a different object was inserted.♻️ Suggested tweak
- await importTo.CreateMorphTypeData(morphTypeData); - logger.LogInformation("Imported morph type {Id} ({typ})", morphTypeData.Id, morphTypeData.MorphType); + var importedMorphTypeData = await importTo.CreateMorphTypeData(morphTypeData); + logger.LogInformation( + "Imported morph type {Id} ({MorphType})", + importedMorphTypeData.Id, + importedMorphTypeData.MorphType);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/FwLite/FwLiteProjectSync/MiniLcmImport.cs` around lines 73 - 76, The log is using the source morphTypeData.Id but CreateMorphTypeData(MorphTypeData) may return a de-duped/existing record with a different canonical Id; update the await foreach block to capture the return value from importTo.CreateMorphTypeData(morphTypeData) and log the returned object's Id (the canonical morph-type Id) along with MorphType to avoid confusing retries/races; reference the await foreach over importFrom.GetAllMorphTypeData(), the call to importTo.CreateMorphTypeData, and the logger.LogInformation call so you replace the logged morphTypeData.Id with the Id from the method's return value.backend/FwLite/LcmCrdt/Changes/CreateMorphTypeDataChange.cs (1)
21-32: Document the tombstone-on-duplicate behavior.This check intentionally treats tombstoned rows as duplicates, which looks wrong on first read. A short comment here would make it much harder for someone to “fix” this to
FilterDeleted()later and accidentally break the first-one-wins contract.📝 Suggested clarification
- var alreadyExists = await context.GetObjectsOfType<MorphTypeData>().AnyAsync(m => m.MorphType == MorphType); + // Deleted rows still count here: MorphType is globally unique, and duplicate inserts + // are resolved by the SQLite UNIQUE ... ON CONFLICT IGNORE constraint. + var alreadyExists = await context.GetObjectsOfType<MorphTypeData>().AnyAsync(m => m.MorphType == MorphType);Based on learnings:
NewEntity()cannot return null, and MorphType duplicates are intentionally resolved “first one wins” via SQLiteUNIQUE ON CONFLICT IGNORE.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/FwLite/LcmCrdt/Changes/CreateMorphTypeDataChange.cs` around lines 21 - 32, Add a concise comment above the duplicate-check explaining that the AnyAsync check on MorphTypeData intentionally does not call FilterDeleted() so that tombstoned rows count as duplicates (i.e., tombstone-on-duplicate), preserving the "first-one-wins" semantics enforced by SQLite UNIQUE ON CONFLICT IGNORE; reference the MorphTypeData type and the alreadyExists variable/logic in CreateMorphTypeDataChange and note that NewEntity() cannot return null so this behavior is required and must not be changed to FilterDeleted().backend/FwLite/LcmCrdt/Migrations/20260306045826_AddMorphTypeDataTable.cs (1)
14-38: Explain whyMorphTypeis declared unique twice.The raw DDL already gives
MorphTypean inlineUNIQUE ON CONFLICT IGNORE, so the later named unique index reads like redundant schema. A short note that the index is kept only to align EF Core's model snapshot withHasIndex(m => m.MorphType).IsUnique()would make this much safer to maintain.📝 Suggested comment
migrationBuilder.CreateIndex( + // Keep this named index in addition to the inline UNIQUE ON CONFLICT IGNORE + // constraint above so EF Core's model snapshot stays aligned with + // HasIndex(m => m.MorphType).IsUnique(). name: "IX_MorphTypeData_MorphType", table: "MorphTypeData", column: "MorphType", unique: true);Based on learnings: "A separate migrationBuilder.CreateIndex() for IX_MorphTypeData_MorphType is also emitted so EF Core's model snapshot stays consistent with HasIndex(...).IsUnique() configuration."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/FwLite/LcmCrdt/Migrations/20260306045826_AddMorphTypeDataTable.cs` around lines 14 - 38, Add a brief explanatory comment in the migration near the raw SQL CREATE TABLE for MorphTypeData and the subsequent migrationBuilder.CreateIndex call: explain that MorphType is declared UNIQUE inline in the DDL for ON CONFLICT IGNORE behavior, and the separate CreateIndex (IX_MorphTypeData_MorphType) is emitted solely so EF Core's model snapshot stays consistent with HasIndex(m => m.MorphType).IsUnique() — reference the CREATE TABLE block and the migrationBuilder.CreateIndex("IX_MorphTypeData_MorphType", ...) to make the intent clear for future maintainers.
🤖 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/FwLiteProjectSync.Tests/Import/ResumableTests.cs`:
- Around line 41-52: expectedMorphTypes is lazily generated and calls
Guid.NewGuid() each enumeration, so retries see different data; materialize the
sequence once (e.g., call ToList() when creating expectedMorphTypes) and use
that single stable collection for all assertions, and change the equivalence
checks to assert the paired values (e.g., compare objects or the tuples {
MorphType, Name }) rather than relying on re-enumeration; update the creation of
expectedMorphTypes (the Enumerable.Range(...).Select(...) that builds
MorphTypeData) to produce a concrete List<MorphTypeData> and adjust the
BeEquivalentTo assertions to compare either the full MorphTypeData instances or
the (MorphType, Name) projection so the fixture is stable across retries.
In `@backend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.cs`:
- Around line 125-127: The sync currently uses SimpleStringDiff for LeadingToken
and TrailingToken in MorphTypeDataSync.Sync which lets a null CRDT value produce
JSON Patch remove ops back to FWData; modify the token diffing so it is
lossless: update MorphTypeDataSync.Sync (or the SimpleStringDiff logic it calls)
to treat a null/absent token on the source side as “no change” (do not emit
remove) unless the source explicitly indicates an intentional deletion, and only
emit add/replace/remove when the source contains a non-null explicit operation;
specifically ensure LeadingToken and TrailingToken comparisons skip generating
remove patches when crdt value is null during the CRDT→FWData pass.
In
`@backend/FwLite/LcmCrdt/Migrations/20260306045826_AddMorphTypeDataTable.Designer.cs`:
- Around line 259-300: The Designer.cs snapshot for the entity
"MiniLcm.Models.MorphTypeData" is missing the unique index for the MorphType
column; update the model configuration for MorphTypeData (the lambda where
modelBuilder.Entity("MiniLcm.Models.MorphTypeData", b => { ... })) to include
b.HasIndex("MorphType").IsUnique(); — either regenerate the migration artifacts
so the snapshot is rebuilt correctly or add that HasIndex call manually in the
MorphTypeData configuration to match the migration's CreateIndex for
IX_MorphTypeData_MorphType.
---
Duplicate comments:
In `@backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs`:
- Around line 395-399: The current guard in the update handling (using
update.Patch.GetOperations()) only blocks OperationType.Replace against the
/MorphType path, allowing other operation types (add/remove/copy/move) to mutate
MorphType; change the predicate to block any operation whose Path equals
$"/{nameof(MorphTypeData.MorphType)}" regardless of op.OperationType, so no
JsonPatch operation can target MorphTypeData.MorphType (this affects the code
that builds/forwards JsonPatchChange<MorphTypeData>).
---
Nitpick comments:
In `@backend/FwLite/FwLiteProjectSync/MiniLcmImport.cs`:
- Around line 73-76: The log is using the source morphTypeData.Id but
CreateMorphTypeData(MorphTypeData) may return a de-duped/existing record with a
different canonical Id; update the await foreach block to capture the return
value from importTo.CreateMorphTypeData(morphTypeData) and log the returned
object's Id (the canonical morph-type Id) along with MorphType to avoid
confusing retries/races; reference the await foreach over
importFrom.GetAllMorphTypeData(), the call to importTo.CreateMorphTypeData, and
the logger.LogInformation call so you replace the logged morphTypeData.Id with
the Id from the method's return value.
In `@backend/FwLite/LcmCrdt/Changes/CreateMorphTypeDataChange.cs`:
- Around line 21-32: Add a concise comment above the duplicate-check explaining
that the AnyAsync check on MorphTypeData intentionally does not call
FilterDeleted() so that tombstoned rows count as duplicates (i.e.,
tombstone-on-duplicate), preserving the "first-one-wins" semantics enforced by
SQLite UNIQUE ON CONFLICT IGNORE; reference the MorphTypeData type and the
alreadyExists variable/logic in CreateMorphTypeDataChange and note that
NewEntity() cannot return null so this behavior is required and must not be
changed to FilterDeleted().
In `@backend/FwLite/LcmCrdt/Migrations/20260306045826_AddMorphTypeDataTable.cs`:
- Around line 14-38: Add a brief explanatory comment in the migration near the
raw SQL CREATE TABLE for MorphTypeData and the subsequent
migrationBuilder.CreateIndex call: explain that MorphType is declared UNIQUE
inline in the DDL for ON CONFLICT IGNORE behavior, and the separate CreateIndex
(IX_MorphTypeData_MorphType) is emitted solely so EF Core's model snapshot stays
consistent with HasIndex(m => m.MorphType).IsUnique() — reference the CREATE
TABLE block and the migrationBuilder.CreateIndex("IX_MorphTypeData_MorphType",
...) to make the intent clear for future maintainers.
In `@backend/FwLite/MiniLcm/SyncHelpers/MorphTypeDataSync.cs`:
- Around line 22-28: The branch in MorphTypeDataSync that returns 0 when
before.MorphType != after.MorphType silently swallows an immutable-kind drift;
update the branch inside the method where the code checks if (before.MorphType
!= after.MorphType) to emit a diagnostic (e.g., process/logger warning and/or
increment a metric counter) before returning 0 so callers and observability
systems see that a rejected MorphType change occurred; reference the
before.MorphType != after.MorphType check and the return 0 in MorphTypeDataSync
to add the warning/metric increment and include contextual identifiers (object
Id, before/after values) in the message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 02d9a0ab-7542-4984-907b-5fa52b34d400
📒 Files selected for processing (33)
backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateMorphTypeDataProxy.csbackend/FwLite/FwLiteProjectSync.Tests/Import/ResumableTests.csbackend/FwLite/FwLiteProjectSync.Tests/ProjectSnapshotSerializationTests.csbackend/FwLite/FwLiteProjectSync.Tests/Snapshots/sena-3_snapshot.2025-09-30.verified.txtbackend/FwLite/FwLiteProjectSync.Tests/Snapshots/sena-3_snapshot.2025-09-31.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/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
backend/FwLite/FwLiteProjectSync.Tests/Import/ResumableTests.cs
Outdated
Show resolved
Hide resolved
backend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.cs
Outdated
Show resolved
Hide resolved
backend/FwLite/LcmCrdt/Migrations/20260306045826_AddMorphTypeDataTable.Designer.cs
Outdated
Show resolved
Hide resolved
Done in commit 264f6d6.
Commit e6a5bb1 fixes this by throwing an InvalidOperationException instead of silently swallowing an invalid update, allowing this to become a runtime error that will be reported to the user.
Not worth doing, as we also log the MorphType enum value which will make it clear what happened if a different GUID shows up the logs. This will not confuse any of our developers.
Comment added in commit e6a5bb1.
Comment added in commit e6a5bb1. |
|
@CodeRabbit fullreview |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/FwLite/FwLiteProjectSync.Tests/Import/ResumableTests.cs (1)
41-52:⚠️ Potential issue | 🟡 MinorExercise all non-fallback
MorphTypevalues in this resumability test.This fixture only imports values
1..10, butbackend/FwLite/MiniLcm/Models/MorphType.csdefines additional real morph types beyond that range. A resumability bug tied to one of the later enum members would still pass here. Please buildexpectedMorphTypesfrom the full non-fallback enum set instead of a hard-coded range.Based on learnings, "When fixing a sync bug, reproduce it with a test."
Also applies to: 95-96, 128-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/FwLite/FwLiteProjectSync.Tests/Import/ResumableTests.cs` around lines 41 - 52, The test currently builds expectedMorphTypes using Enumerable.Range(1,10) which misses later real MorphType enum members; update the construction to iterate all non-fallback members of the MorphType enum (e.g., use Enum.GetValues(typeof(MorphType)) and filter out the fallback/default value) and create a MorphTypeData for each such enum member (keeping the same fields: Id, Name, Abbreviation, Description, LeadingToken, TrailingToken, MorphType, SecondaryOrder). Do the same replacement for the other occurrences mentioned (around the blocks that start at the locations corresponding to lines 95-96 and 128-134) so the test exercises every real MorphType rather than a hard-coded 1..10 range.
🧹 Nitpick comments (3)
backend/FwLite/LcmCrdt.Tests/SnapshotAtCommitServiceTests.cs (1)
27-27: Add a MorphTypeData rollback case alongside this.This fixes order sensitivity, but none of the tests in this file actually create or mutate
AllMorphTypeData, so snapshot-at-commit regressions for morph types could still slip through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/FwLite/LcmCrdt.Tests/SnapshotAtCommitServiceTests.cs` at line 27, The tests currently disable strict ordering for AllMorphTypeData but never exercise it, so add a MorphTypeData rollback case to the test setup in SnapshotAtCommitServiceTests: where the mocked rollback sequence (the code that builds rollback events used by the snapshot-at-commit tests) constructs other rollback items, append a rollback event for a morph type (e.g., create/update/delete of a MorphTypeData entry) targeting AllMorphTypeData so the tests actually create or mutate that collection; ensure the new case is added alongside the existing WithoutStrictOrderingFor(x => x.AllMorphTypeData) configuration and uses the same rollback event shape used elsewhere in the file so it participates in the snapshot/commit assertions.backend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.cs (1)
198-203: Add a MorphTypeData update case here too.This only exercises creation. The new editable-field update path—and the
MorphTypeimmutability rule—still are not covered by the shared “all changes” test generator, so regressions there can pass unnoticed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.cs` around lines 198 - 203, Add a MorphTypeData update case after the CreateMorphTypeDataChange: construct new editable values (e.g., new MultiString for Name and Abbreviation, new RichMultiString for Description), create an UpdateMorphTypeDataChange using the same morphTypeData.Id and the new editable-field values, and yield it via new ChangeWithDependencies(UpdateMorphTypeDataChange). Also add a second case that attempts to change the immutable MorphType property (e.g., an UpdateMorphTypeDataChange that sets a different MorphType) so the MorphType immutability rule is exercised by the shared “all changes” test generator.backend/FwLite/MiniLcm/SyncHelpers/MorphTypeDataSync.cs (1)
55-55: Consider adding MorphType validation toFwDataMiniLcmApi.UpdateMorphTypeData()for defense-in-depth.While the sync layer (here) and CRDT layer both guard against MorphType changes,
FwDataMiniLcmApi.UpdateMorphTypeData()atbackend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs:581-594does not validate the patch. Although theUpdateMorphTypeDataProxydoesn't persist MorphType changes (since it doesn't override the property to map to LCM), adding validation there would provide consistency across all API implementations.♻️ Optional: Add validation in FwDataMiniLcmApi.UpdateMorphTypeData
In
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs, add validation similar toCrdtMiniLcmApi:public Task<MorphTypeData> UpdateMorphTypeData(Guid id, UpdateObjectInput<MorphTypeData> update) { // Add this validation for consistency with CrdtMiniLcmApi if (update.Patch.Operations.Any(op => op.Path == $"/{nameof(MorphTypeData.MorphType)}")) { throw new InvalidOperationException("MorphTypes cannot be changed to a different kind after creation"); } var lcmMorphType = MorphTypeRepository.GetObject(id); // ... rest of implementation }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/FwLite/MiniLcm/SyncHelpers/MorphTypeDataSync.cs` at line 55, Add defensive validation in FwDataMiniLcmApi.UpdateMorphTypeData to reject any JSON Patch operation that targets MorphTypeData.MorphType: inspect the incoming UpdateObjectInput<MorphTypeData>.Patch.Operations and if any op.Path equals $"/{nameof(MorphTypeData.MorphType)}" throw an InvalidOperationException with a clear message (e.g., "MorphTypes cannot be changed to a different kind after creation"); keep the rest of the method flow (retrieving lcmMorphType via MorphTypeRepository.GetObject and existing update logic) unchanged for consistency with CrdtMiniLcmApi.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/FwLite/FwLiteProjectSync.Tests/Import/ResumableTests.cs`:
- Around line 41-52: The test currently builds expectedMorphTypes using
Enumerable.Range(1,10) which misses later real MorphType enum members; update
the construction to iterate all non-fallback members of the MorphType enum
(e.g., use Enum.GetValues(typeof(MorphType)) and filter out the fallback/default
value) and create a MorphTypeData for each such enum member (keeping the same
fields: Id, Name, Abbreviation, Description, LeadingToken, TrailingToken,
MorphType, SecondaryOrder). Do the same replacement for the other occurrences
mentioned (around the blocks that start at the locations corresponding to lines
95-96 and 128-134) so the test exercises every real MorphType rather than a
hard-coded 1..10 range.
---
Nitpick comments:
In `@backend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.cs`:
- Around line 198-203: Add a MorphTypeData update case after the
CreateMorphTypeDataChange: construct new editable values (e.g., new MultiString
for Name and Abbreviation, new RichMultiString for Description), create an
UpdateMorphTypeDataChange using the same morphTypeData.Id and the new
editable-field values, and yield it via new
ChangeWithDependencies(UpdateMorphTypeDataChange). Also add a second case that
attempts to change the immutable MorphType property (e.g., an
UpdateMorphTypeDataChange that sets a different MorphType) so the MorphType
immutability rule is exercised by the shared “all changes” test generator.
In `@backend/FwLite/LcmCrdt.Tests/SnapshotAtCommitServiceTests.cs`:
- Line 27: The tests currently disable strict ordering for AllMorphTypeData but
never exercise it, so add a MorphTypeData rollback case to the test setup in
SnapshotAtCommitServiceTests: where the mocked rollback sequence (the code that
builds rollback events used by the snapshot-at-commit tests) constructs other
rollback items, append a rollback event for a morph type (e.g.,
create/update/delete of a MorphTypeData entry) targeting AllMorphTypeData so the
tests actually create or mutate that collection; ensure the new case is added
alongside the existing WithoutStrictOrderingFor(x => x.AllMorphTypeData)
configuration and uses the same rollback event shape used elsewhere in the file
so it participates in the snapshot/commit assertions.
In `@backend/FwLite/MiniLcm/SyncHelpers/MorphTypeDataSync.cs`:
- Line 55: Add defensive validation in FwDataMiniLcmApi.UpdateMorphTypeData to
reject any JSON Patch operation that targets MorphTypeData.MorphType: inspect
the incoming UpdateObjectInput<MorphTypeData>.Patch.Operations and if any
op.Path equals $"/{nameof(MorphTypeData.MorphType)}" throw an
InvalidOperationException with a clear message (e.g., "MorphTypes cannot be
changed to a different kind after creation"); keep the rest of the method flow
(retrieving lcmMorphType via MorphTypeRepository.GetObject and existing update
logic) unchanged for consistency with CrdtMiniLcmApi.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 918315d6-990c-4d34-ac13-e2e77bd6c59c
📒 Files selected for processing (33)
backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateMorphTypeDataProxy.csbackend/FwLite/FwLiteProjectSync.Tests/Import/ResumableTests.csbackend/FwLite/FwLiteProjectSync.Tests/ProjectSnapshotSerializationTests.csbackend/FwLite/FwLiteProjectSync.Tests/Snapshots/sena-3_snapshot.2025-09-30.verified.txtbackend/FwLite/FwLiteProjectSync.Tests/Snapshots/sena-3_snapshot.2025-09-31.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/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 - Done with CodeRabbit, ready for human review now. |
myieye
left a comment
myieye
left a comment
There was a problem hiding this comment.
Looking good! I found some things that I think are worth changing.
FYI, you can refer to this emoji-legend if you're curious what my emojis mean 😉.
backend/FwLite/FwLiteProjectSync.Tests/Import/ResumableTests.cs
Outdated
Show resolved
Hide resolved
backend/FwLite/FwLiteProjectSync.Tests/Snapshots/sena-3_snapshot.2025-09-30.verified.txt
Show resolved
Hide resolved
backend/FwLite/LcmCrdt/Migrations/20260306045826_AddMorphTypeDataTable.cs
Outdated
Show resolved
Hide resolved
|
@CodeRabbit fullreview |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
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
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.
backend/FwLite/FwLiteProjectSync.Tests/Import/ResumableTests.cs
Outdated
Show resolved
Hide resolved
github-actions
bot
added
the
📦 Lexbox
|
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. |
|
Turns out the test failures were because of a use of the MorphType enum in |
|
@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. |
myieye
left a comment
myieye
left a comment
There was a problem hiding this comment.
I think this is almost there. 👍
I just noted the validation changes we agreed on that you haven't done yet.
Also, I think we should not merge this. Instead I've enabled the fw-lite CI actions to run on PRs against this branch. So, we'll merge in a bunch of other morph-type stuff like:
- tests
- my morph-token PR
- adding canonical morph-types to existing crdt projects (see new sub-issue)
- adding an interceptor for updating headwords on morph-type changes (see new sub-issue)
- etc.
I think we want at least all of those as a "bundle" or else we can't release.
backend/FwLite/FwLiteProjectSync.Tests/Import/ResumableTests.cs
Outdated
Show resolved
Hide resolved
Validation! Slaps forehead. I was thinking yesterday "I know there's one more thing but I don't remember what it is." :-) |
myieye
left a comment
myieye
left a comment
There was a problem hiding this comment.
Arg, I forgot to submit this feedback, sorry 🙁
myieye
force-pushed
the
feat/sync-morph-types
branch
2 times, most recently
from
c30abbb to
dc9495c
Compare
Implement CRUD for MorphTypeData, add DB migration Make LeadingToken and TrailingToken nullable Register JsonPatchChange for morphTypeData Reject MorphTypeData updates that change MorphType Do not allow ANY JsonPatch changes to MorphType Rename MorphType enum to MorphTypeKind Rename MorphTypeData to MorphType LeadingToken -> Prefix, TrailingToken -> Postfix Matches the names from liblcm, which may help reduce confusion.
* Add filtering on token aware headwords * Respect SecondaryOrder when sorting * Make FTS Headword column contain morph-tokens and all vernacular WS's
myieye
force-pushed
the
feat/sync-morph-types
branch
2 times, most recently
from
e3ec8c7 to
87c5dad
Compare
* Initial work on morph types in UI Morph types now show leading/trailing tokens in headword, but do not yet have a dropdown for editing them in the entry UI. * Citation forms should not be decorated Lexeme forms should be decorated with prefix/postfix tokens according to the morph type, but citation forms are meant as "overrides" and should be reproduced exactly as-is, without morph type tokens. This is the rule used by FLEx for how it displays words, so FW Lite should do the same. As a bonus, there is now only one `headword` function in the writing system service, instead of two functions with the same name that did two slightly different things. * Fix tests --------- Co-authored-by: Tim Haasdyk <tim_haasdyk@sil.org>
myieye
force-pushed
the
feat/sync-morph-types
branch
from
87c5dad to
8ed192f
Compare
* Seed canonical morph-types into CRDT projects - Add CanonicalMorphTypes with all 19 morph-type definitions (GUIDs from LibLCM) - Seed morph-types for new projects via PreDefinedData.PredefinedMorphTypes - Seed morph-types for existing projects in MigrateDb (before FTS refresh) - Add EF migration to clear FTS table so headwords are rebuilt with morph tokens - Patch legacy snapshots (empty MorphTypes) in sync layer to prevent duplicates * Stop creating morph-types in tests. They're now prepopulated * Stop printing verify diff content. It's too much. * Seed morph types before API testing * Add descriptions to canonical morph types * Sync morph-types when importing, because they already exist in CRDT * Verify our canonical morph-types match new fwdata projects * Fix non-FTS relevance order with morph-tokens in query
myieye
force-pushed
the
feat/sync-morph-types
branch
from
8ed192f to
374b1b2
Compare
myieye
force-pushed
the
feat/sync-morph-types
branch
from
a7868a3 to
4028077
Compare
2 participants
Fixes #1847 ...
...at least the first commit does.
This has grown into a large morph-type feature branch.
Most/all commits represent a sub-issue of the #1845 epic.
The sub-issues have been closed as they are merged into this branch/PR.