Move and handle writing systems effectively #1890
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis change introduces support for moving and reordering writing systems in the MiniLcm API, updates error handling for writing system retrieval, and refactors interfaces and models to better separate ordering concerns. It also implements order-aware synchronization and adds corresponding tests and infrastructure updates across multiple API implementations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
💻 FW Lite
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
backend/FwLite/MiniLcm/Models/WritingSystemId.cs (1)
96-101: Align TryFormat/TryParse with Try semantics (no throw, handle buffer/invalid input)*Current implementations can (a) overrun destination in TryFormat and (b) throw from TryParse via the constructor. These should not throw.
Apply these targeted fixes:
@@ - public bool TryFormat(Span<char> destination, out int charsWritten, ReadOnlySpan<char> format, IFormatProvider? provider) - { - Code.AsSpan().CopyTo(destination); - charsWritten = Code.Length; - return true; - } + public bool TryFormat(Span<char> destination, out int charsWritten, ReadOnlySpan<char> format, IFormatProvider? provider) + { + var src = Code.AsSpan(); + if (destination.Length < src.Length) + { + charsWritten = 0; + return false; + } + src.CopyTo(destination); + charsWritten = src.Length; + return true; + } @@ - public static bool TryParse(string? s, IFormatProvider? provider, out WritingSystemId result) - { - result = default; - if (s is null) return false; - result = s; - return true; - } + public static bool TryParse(string? s, IFormatProvider? provider, out WritingSystemId result) + { + result = default; + if (string.IsNullOrEmpty(s)) return false; + if (s == "default" || s == "__key" || + IetfLanguageTag.TryGetParts(s, out _, out _, out _, out _)) + { + result = new WritingSystemId(s); + return true; + } + return false; + } @@ - public static bool TryParse(ReadOnlySpan<char> s, IFormatProvider? provider, out WritingSystemId result) - { - result = s; - return true; - } + public static bool TryParse(ReadOnlySpan<char> s, IFormatProvider? provider, out WritingSystemId result) + { + result = default; + if (s.IsEmpty) return false; + // Avoid allocations except when necessary + if (s.Equals("default".AsSpan(), StringComparison.OrdinalIgnoreCase) || + s.SequenceEqual("__key".AsSpan())) + { + result = new WritingSystemId(new string(s)); + return true; + } + var str = new string(s); + if (IetfLanguageTag.TryGetParts(str, out _, out _, out _, out _)) + { + result = new WritingSystemId(str); + return true; + } + return false; + }Also applies to: 108-114, 121-125
🧹 Nitpick comments (9)
backend/FwLite/MiniLcm/Models/WritingSystem.cs (1)
7-9: Nit: capitalize XML doc sentenceMinor grammar polish for consistency with codebase docs.
- /// this ID is always empty when working with FW data, it is only used when working with CRDTs + /// This ID is always empty when working with FW data; it is only used when working with CRDTs.backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs (1)
48-52: Avoid unnecessary async state machine; also include type in log for clarityThe method is async but only awaits Task.CompletedTask. Return Task directly and align with the style used elsewhere in this class (e.g., MoveSense, DeleteSense, etc.). Also, include the writing system type in the dry-run message.
- public async Task MoveWritingSystem(WritingSystemId id, WritingSystemType type, BetweenPosition<WritingSystemId?> between) - { - DryRunRecords.Add(new DryRunRecord(nameof(MoveWritingSystem), $"Move writing system {id} between {between.Previous} and {between.Next}")); - await Task.CompletedTask; - } + public Task MoveWritingSystem(WritingSystemId id, WritingSystemType type, BetweenPosition<WritingSystemId?> between) + { + DryRunRecords.Add(new DryRunRecord(nameof(MoveWritingSystem), + $"Move {type} writing system {id} between {between.Previous} and {between.Next}")); + return Task.CompletedTask; + }backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs (1)
248-256: Provide a synchronous Map alongside MapAsyncMany mappings will be synchronous (e.g., lookups in memory). Offering a non-async Map avoids unnecessary async overhead when not required.
public record BetweenPosition<T>(T? Previous, T? Next) { public async Task<BetweenPosition> MapAsync(Func<T, Task<Guid?>> map) { return new BetweenPosition( Previous is null ? null : await map(Previous), Next is null ? null : await map(Next)); } + + public BetweenPosition Map(Func<T, Guid?> map) + { + return new BetweenPosition( + Previous is null ? null : map(Previous), + Next is null ? null : map(Next)); + } }backend/FwLite/MiniLcm/Models/IOrderable.cs (1)
3-11: Align IOrderable with IObjectWithId to avoid Id duplicationGiven OrderPicker now constrains T as IOrderableNoId + IObjectWithId, consider making IOrderable extend IObjectWithId instead of re-declaring Guid Id. This reduces duplication and keeps the Id contract consistent across the model layer.
-public interface IOrderable: IOrderableNoId -{ - Guid Id { get; } -} +public interface IOrderable : IOrderableNoId, IObjectWithId +{ +}backend/FwLite/MiniLcm.Tests/WritingSystemTestsBase.cs (1)
76-107: Consider adding a couple of edge-case assertionsOptional: add follow-ups to harden behavior.
- Move to end (previous = last stable, next = null).
- Move to start (previous = null, next = first stable) when already first (no-op idempotency).
- Invalid ids (ensure graceful handling/exception).
Happy to draft these tests if desired.
backend/FwLite/LcmCrdt/OrderPicker.cs (1)
8-9: Avoid ToListAsync over entire siblings set when only two items are neededCurrently, when between is provided, the method materializes all siblings just to locate previous/next. Consider fetching only the required items by Id (and only select Id + Order) for better performance on large sets.
Example approach (outside the shown lines):
var previousId = between?.Previous; var nextId = between?.Next; if (previousId is null && nextId is null) { var currMaxOrder = await siblings.Select(s => s.Order).DefaultIfEmpty().MaxAsync(); return currMaxOrder + 1; } var ids = new[] { previousId, nextId } .Where(id => id.HasValue) .Select(id => id!.Value) .Distinct() .ToArray(); var lookup = await siblings .Where(s => ids.Contains(s.Id)) .Select(s => new { s.Id, s.Order }) .AsNoTracking() .ToListAsync(); var previous = previousId is Guid pid ? lookup.FirstOrDefault(x => x.Id == pid) : null; var next = nextId is Guid nid ? lookup.FirstOrDefault(x => x.Id == nid) : null; // compute as you already do using previous?.Order and next?.Orderbackend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs (1)
108-116: Multiple repos opened in a tight loop
between.MapAsynccallsGetWritingSystemfor each neighbour, and every call opens a new repository instance.
When many moves are performed this becomes an N² explosion of DB connections.Cache the repo:
await using var repo = await repoFactory.CreateRepoAsync(); ... Func<WritingSystemId?, Task<Guid?>> map = async wsId => wsId is null ? null : (await repo.GetWritingSystem(wsId.Value, type))?.Id; var betweenIds = await between.MapAsync(map);backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (1)
223-230: Ambiguous null-checkYou throw if both neighbours are null, but the API caller may intend to move a WS to the very start or end of the list (both neighbours null is a valid “append” use-case).
Consider permitting(null,null)and mapping it to index 0 orlist.Count.backend/FwLite/MiniLcm/SyncHelpers/WritingSystemSync.cs (1)
100-105: Created WS loses intended order
Addsimply callsCreateWritingSystemand ignores the desired position, so the new system is always appended.
IfafterWritingSystemsexpects a specific order the subsequent diff will fire an immediate Move, doubling the work.Set
Orderbefore creation or invokeMoveWritingSystemright after create.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs(5 hunks)backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.cs(2 hunks)backend/FwLite/FwLiteProjectSync.Tests/WritingSystemSyncTests.cs(1 hunks)backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs(1 hunks)backend/FwLite/LcmCrdt.Tests/Changes/RegressionDeserializationData.json(1 hunks)backend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.cs(1 hunks)backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyChangeModels.verified.txt(1 hunks)backend/FwLite/LcmCrdt/Changes/SetOrderChange.cs(1 hunks)backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs(1 hunks)backend/FwLite/LcmCrdt/LcmCrdtKernel.cs(1 hunks)backend/FwLite/LcmCrdt/OrderPicker.cs(1 hunks)backend/FwLite/MiniLcm.Tests/WritingSystemTestsBase.cs(1 hunks)backend/FwLite/MiniLcm/IMiniLcmReadApi.cs(1 hunks)backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs(1 hunks)backend/FwLite/MiniLcm/Models/IOrderable.cs(1 hunks)backend/FwLite/MiniLcm/Models/WritingSystem.cs(1 hunks)backend/FwLite/MiniLcm/Models/WritingSystemId.cs(1 hunks)backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs(1 hunks)backend/FwLite/MiniLcm/SyncHelpers/WritingSystemSync.cs(3 hunks)backend/LfClassicData/LfClassicMiniLcmApi.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1537
File: frontend/viewer/src/SvelteUxProjectView.svelte:151-153
Timestamp: 2025-03-12T06:32:08.277Z
Learning: When reviewing PRs where files have been moved or code has been relocated from one file to another, focus the review on actual modifications to the code rather than raising issues about pre-existing code patterns that were simply relocated.
Learnt from: imnasnainaec
PR: sillsdev/languageforge-lexbox#1867
File: platform.bible-extension/src/utils/fw-lite-api.ts:48-53
Timestamp: 2025-07-31T17:31:59.999Z
Learning: In the sillsdev/languageforge-lexbox platform.bible-extension, the FwLiteApi.doesProjectMatchLanguage() method uses JSON.stringify() on writingSystems.vernacular data as an intentional temporary placeholder ("stand-in") until proper language code access can be implemented. This is not a code quality issue but a deliberate temporary solution.
Learnt from: imnasnainaec
PR: sillsdev/languageforge-lexbox#1867
File: platform.bible-extension/src/main.ts:239-246
Timestamp: 2025-07-31T19:10:41.178Z
Learning: In the sillsdev/languageforge-lexbox repository, user imnasnainaec prefers to defer code improvements when there are related TODO comments indicating planned refactoring work, choosing to bundle related changes together rather than making incremental improvements that would need to be modified again during the larger refactoring.
📚 Learning: 2025-07-31T17:31:59.999Z
Learnt from: imnasnainaec
PR: sillsdev/languageforge-lexbox#1867
File: platform.bible-extension/src/utils/fw-lite-api.ts:48-53
Timestamp: 2025-07-31T17:31:59.999Z
Learning: In the sillsdev/languageforge-lexbox platform.bible-extension, the FwLiteApi.doesProjectMatchLanguage() method uses JSON.stringify() on writingSystems.vernacular data as an intentional temporary placeholder ("stand-in") until proper language code access can be implemented. This is not a code quality issue but a deliberate temporary solution.
Applied to files:
backend/FwLite/LcmCrdt.Tests/Changes/RegressionDeserializationData.jsonbackend/FwLite/MiniLcm/Models/WritingSystem.csbackend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.csbackend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.csbackend/FwLite/FwLiteProjectSync.Tests/WritingSystemSyncTests.cs
📚 Learning: 2025-06-27T09:24:39.507Z
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable<IChange>) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
Applied to files:
backend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.csbackend/FwLite/LcmCrdt/OrderPicker.csbackend/FwLite/MiniLcm/IMiniLcmReadApi.csbackend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.csbackend/FwLite/LcmCrdt/Changes/SetOrderChange.csbackend/FwLite/LcmCrdt/LcmCrdtKernel.csbackend/FwLite/MiniLcm/SyncHelpers/DiffCollection.csbackend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.csbackend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.csbackend/FwLite/MiniLcm/IMiniLcmWriteApi.csbackend/FwLite/LcmCrdt/CrdtMiniLcmApi.csbackend/FwLite/MiniLcm/SyncHelpers/WritingSystemSync.cs
📚 Learning: 2025-07-22T09:19:37.386Z
Learnt from: rmunn
PR: sillsdev/languageforge-lexbox#1836
File: frontend/viewer/src/lib/components/audio/AudioDialog.svelte:25-25
Timestamp: 2025-07-22T09:19:37.386Z
Learning: In the sillsdev/languageforge-lexbox project, when file size limits or other constants need to be shared between C# backend and TypeScript frontend code, prefer exposing them through Reinforced.Typings type generation rather than hardcoding the values separately. This ensures consistency and prevents discrepancies when values change.
Applied to files:
backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.csbackend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs
📚 Learning: 2025-07-29T07:10:53.388Z
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1836
File: backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/MediaTests.cs:28-33
Timestamp: 2025-07-29T07:10:53.388Z
Learning: In test code for FwData-specific functionality (like MediaTests in FwDataMiniLcmBridge.Tests), direct casting to FwDataMiniLcmApi is preferred over safe casting because it serves as an assertion that the test setup is correct. If the cast fails, it indicates a test configuration bug that should be caught immediately rather than silently ignored.
Applied to files:
backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.csbackend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.csbackend/FwLite/FwLiteProjectSync.Tests/WritingSystemSyncTests.cs
📚 Learning: 2025-07-03T09:52:39.059Z
Learnt from: rmunn
PR: sillsdev/languageforge-lexbox#1795
File: backend/FwLite/MiniLcm/Validators/PublicationValidator.cs:15-23
Timestamp: 2025-07-03T09:52:39.059Z
Learning: In the sillsdev/languageforge-lexbox project, MultiString objects have a custom .ToString() method that joins all strings in the MultiString with ", ". When generating identifiers for publications, the code intentionally uses MultiString.ToString() to show all names from different writing systems, not just the first one.
Applied to files:
backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.cs
📚 Learning: 2025-07-31T16:00:49.635Z
Learnt from: imnasnainaec
PR: sillsdev/languageforge-lexbox#1867
File: platform.bible-extension/src/types/fw-lite-extension.d.ts:4-22
Timestamp: 2025-07-31T16:00:49.635Z
Learning: In the sillsdev/languageforge-lexbox repository, the platform.bible-extension is intentionally tightly coupled with the frontend's dotnet-types. The relative imports from `../../../frontend/viewer/src/lib/dotnet-types/index.js` in the extension's type declarations are by design, not a maintainability issue that needs to be addressed.
Applied to files:
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs
📚 Learning: 2025-04-17T02:52:44.986Z
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1609
File: backend/FwLite/LcmCrdt/CompiledModels/ComplexFormComponentEntityType.cs:138-182
Timestamp: 2025-04-17T02:52:44.986Z
Learning: Files in the LcmCrdt.CompiledModels namespace contain auto-generated Entity Framework Core model code that should be ignored during code reviews since they shouldn't be manually modified.
Applied to files:
backend/FwLite/MiniLcm/SyncHelpers/WritingSystemSync.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build FwHeadless / publish-fw-headless
- GitHub Check: Build API / publish-api
- GitHub Check: Build FW Lite and run tests
- GitHub Check: frontend-component-unit-tests
- GitHub Check: frontend
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (15)
backend/LfClassicData/LfClassicMiniLcmApi.cs (1)
67-76: LGTM: Consistent, null-safe retrieval by type and WsIdImplementation aligns with other backends, returns null when not found, and throws on invalid enum values. No issues.
backend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.cs (1)
202-204: Good addition: covers SetOrderChange in change matrixThis ensures the kernel-registered type is exercised and dependency on creation is respected, consistent with other orderable entities.
backend/FwLite/LcmCrdt.Tests/Changes/RegressionDeserializationData.json (1)
280-284: Type discriminator matches runtime TypeName convention"$type": "SetOrderChange:WritingSystem" aligns with the class’s TypeName, ensuring stable regression deserialization coverage.
backend/FwLite/MiniLcm/Models/WritingSystem.cs (1)
5-5: Implements IOrderableNoId as intendedThis enables participation in SetOrderChange without imposing Id constraints. Copy() includes Order; good.
backend/FwLite/LcmCrdt/Changes/SetOrderChange.cs (1)
8-8: Constraint update to IOrderableNoId is correctMatches the interface split and allows WritingSystem to participate without requiring an Id on the entity interface. Serialization TypeName remains stable.
backend/FwLite/MiniLcm/Models/WritingSystemId.cs (1)
50-50: Good guard clause; validates invariants earlyThrowing on null/empty upfront is correct and aligns with downstream uses. No concerns here.
backend/FwLite/MiniLcm/IMiniLcmReadApi.cs (1)
12-12: Verify implementations updated for newGetWritingSystemsignature
No concrete classes implementingIMiniLcmReadApiwere detected in this repository. Please manually confirm that every implementation (including those in other assemblies or projects) has been updated to use:
Task<WritingSystem?> GetWritingSystem(WritingSystemId id, WritingSystemType type)- Properly handle a
nullresult instead of throwing when a writing system is missingLocations to check:
- Interface definition: backend/FwLite/MiniLcm/IMiniLcmReadApi.cs:12
Ensure downstream consumers also handle the nullable return without assuming an exception will be thrown.
backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyChangeModels.verified.txt (1)
174-178: Snapshot updated for SetOrderChange — OKThe discriminator mapping is added and consistent with the CRDT kernel additions.
backend/FwLite/LcmCrdt/LcmCrdtKernel.cs (1)
262-262: Test coverage for SetOrderChange confirmedUseChangesTests.GetAllChanges includes a
SetOrderChange<WritingSystem>instance (backend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.cs:202–204), and the DataModelSnapshotTests verify its registration. No further action required.backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs (1)
48-52: Confirm generic argument nullability for BetweenPositionElsewhere (tests, helpers), BetweenPosition already makes Previous/Next optional. Passing T as WritingSystemId? can lead to nested nullability (T? where T is already nullable). Please confirm IMiniLcmWriteApi uses BetweenPosition rather than BetweenPosition<WritingSystemId?>, and align this implementation accordingly to avoid confusion and potential compiler warnings.
backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs (1)
248-256: Potential nested-nullability footgun with BetweenPosition usageThe generic record defines properties as T?. If callers supply T = WritingSystemId? (nullable already), this effectively becomes “T??” at the usage sites and can be confusing or even problematic. Recommend standardizing on T = WritingSystemId (non-nullable), letting BetweenPosition handle nullability via its T? members. Please verify interface and call sites and adjust if needed.
backend/FwLite/MiniLcm.Tests/WritingSystemTestsBase.cs (1)
76-107: Good, focused coverage of move semanticsCreates two WS, verifies initial order, moves ws2 before ws1 using BetweenPosition, then asserts the new order—clean and effective.
backend/FwLite/LcmCrdt/OrderPicker.cs (1)
8-9: Constraint change is appropriateSwitching to where T : IOrderableNoId, IObjectWithId matches the interface split and clarifies intent for types (like writing systems) that shouldn’t implement IOrderable directly.
backend/FwLite/FwLiteProjectSync.Tests/WritingSystemSyncTests.cs (1)
37-48: Test leaves created writing systems behind
DisposeAsynccleans up entries but never deletes the extra “fr” writing system you create inInitializeAsync.
Because the fixture is shared, subsequent tests may observe unexpected state/order.Add explicit clean-up or reset the fixture after each test to keep tests independent.
backend/FwLite/MiniLcm/SyncHelpers/WritingSystemSync.cs (1)
114-123: Potential KeyNotFoundException
Mapping[between.Previous.Value](or.Next) assumes that every GUID seen byDiffOrderablehas already been added toMapping. In pathological reorder sequences that isn’t guaranteed.Guard with
TryGetValueand throw a clear error if the mapping is missing.
There was a problem hiding this comment.
I tried my hand at refactoring DiffOrderable to support non Guid IDs.
I'm not convinced I improved things overall, so I didn't bother digging into why I broke the feature.
I pushed the commit to a different branch, so you can see if it's at all inspiring:
fea80ba
Other than that, I think there are a few touch ups still to go as noted in the comments.
|
@myieye are we going to have the same issue we had with PublishIn where the WS order is rewritten? Example
|
hahn-kev
removed
the
📦 Lexbox
2 participants
closes #1887
Updated the implementation to support moving writing systems. Key changes include:
LcmCrdt.FwData.