Fix default crdt WS doesn't respect order #2008
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 📝 WalkthroughWalkthroughIntroduces a deterministic WritingSystemsOrdered query and adopts it across APIs and services. Refactors LCM helper to return CoreWritingSystemDefinition with a wrapper for handles. Reorders parameters for FromLcmWritingSystem and adjusts GetWritingSystem to use cache-based lookup. Adds a test for default writing-system reordering. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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 ↗︎
|
UI unit Tests 1 files 45 suites 30s ⏱️ Results for commit 4033e4b. ♻️ This comment has been updated with latest results. |
![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 (4)
backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs (1)
19-20: Add an index to back frequent ordered scans.Optional, but Sorting by (Order, Id) is now a hot path; an index will help on larger projects.
Apply in OnModelCreating:
protected override void OnModelCreating(ModelBuilder modelBuilder) { modelBuilder.UseCrdt(options.Value); + modelBuilder.Entity<WritingSystem>() + .HasIndex(ws => new { ws.Order, ws.Id }) + .HasDatabaseName("IX_WritingSystem_Order_Id");backend/FwLite/LcmCrdt/FullTextSearch/EntrySearchService.cs (1)
182-187: Centralize WS comparer to avoid drift.You re-implemented the (Order, Id) tiebreak. Extract a shared comparer to keep parity with DbContext ordering.
Apply this diff locally and reuse where needed:
+private static readonly IComparer<WritingSystem> WsComparer = + Comparer<WritingSystem>.Create((a, b) => + { + var c = a.Order.CompareTo(b.Order); + return c != 0 ? c : a.Id.CompareTo(b.Id); + }); ... -Array.Sort(writingSystems, (ws1, ws2) => -{ - var orderComparison = ws1.Order.CompareTo(ws2.Order); - if (orderComparison != 0) return orderComparison; - return ws1.Id.CompareTo(ws2.Id); -}); +Array.Sort(writingSystems, WsComparer);backend/FwLite/LcmCrdt/Data/MiniLcmRepository.cs (1)
84-93: Default WS now sourced from ordered view—watch cache staleness.Per-instance caching (_defaultVernacularWs/_defaultAnalysisWs) is fine if repositories are short‑lived per API call; stale defaults could appear if a repo instance outlives a Move/Create. If reuse is possible, drop the fields or invalidate them on moves/creates.
Would you like a small patch to invalidate the cached defaults within this type when writing systems are mutated through this repository?
backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.cs (1)
169-180: Clarify default-id behavior when type is null.If wsId == default and type is null, this will throw via switch default. Make it explicit to prevent surprises, or support a sensible default.
Apply one of:
- Enforce explicit type:
if (wsId == default) { - return type switch + if (type is null) throw new ArgumentNullException(nameof(type), "type is required when wsId is default"); + return type switch { WritingSystemType.Analysis => wsContainer.DefaultAnalysisWritingSystem, WritingSystemType.Vernacular => wsContainer.DefaultVernacularWritingSystem, _ => throw new ArgumentOutOfRangeException(nameof(type), type, null) }; }
- Or pick a default (e.g., Vernacular) if acceptable for your domain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs(3 hunks)backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.cs(2 hunks)backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs(1 hunks)backend/FwLite/LcmCrdt/Data/MiniLcmRepository.cs(3 hunks)backend/FwLite/LcmCrdt/FullTextSearch/EntrySearchService.cs(3 hunks)backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs(1 hunks)backend/FwLite/MiniLcm.Tests/WritingSystemTestsBase.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-07T06:02:41.194Z
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1804
File: backend/FwLite/LcmCrdt/CurrentProjectService.cs:18-19
Timestamp: 2025-07-07T06:02:41.194Z
Learning: In the CurrentProjectService class, the service locator pattern is intentionally used to retrieve IDbContextFactory<LcmCrdtDbContext> and EntrySearchServiceFactory because these services indirectly depend on CurrentProjectService to have the current project set, creating a circular dependency. This is an acceptable use of service locator to break the circular dependency while keeping project context responsibility consolidated.
Applied to files:
backend/FwLite/LcmCrdt/Data/MiniLcmRepository.cs
🧬 Code graph analysis (5)
backend/FwLite/MiniLcm.Tests/WritingSystemTestsBase.cs (2)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (17)
Task(86-97)Task(117-123)Task(146-184)Task(186-205)Task(207-216)Task(218-263)Task(282-288)Task(290-305)Task(307-319)Task(321-325)Task(327-337)Task(339-350)Task(363-376)Task(378-382)Task(384-392)Task(415-419)WritingSystemId(76-79)backend/FwLite/MiniLcm/Models/WritingSystemId.cs (6)
WritingSystemId(10-15)WritingSystemId(22-25)WritingSystemId(41-46)WritingSystemId(48-70)WritingSystemId(103-106)WritingSystemId(116-119)
backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs (3)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (2)
WritingSystem(99-115)Order(743-761)backend/FwLite/MiniLcm/Models/WritingSystem.cs (1)
WritingSystem(52-66)backend/FwLite/LcmCrdt/Data/SetupCollationInterceptor.cs (1)
WritingSystem(16-37)
backend/FwLite/LcmCrdt/Data/MiniLcmRepository.cs (3)
backend/FwLite/LcmCrdt/LcmCrdtDbContext.cs (1)
LcmCrdtDbContext(11-89)backend/FwLite/LcmCrdt/Data/DbContextDesignFactory.cs (1)
LcmCrdtDbContext(11-20)backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (1)
WritingSystem(99-115)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (2)
backend/FwLite/MiniLcm/Models/WritingSystem.cs (1)
WritingSystem(52-66)backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.cs (2)
CoreWritingSystemDefinition(169-205)WritingSystemId(164-167)
backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.cs (2)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (3)
CoreWritingSystemDefinition(265-271)WritingSystemId(76-79)GetWritingSystemHandle(81-84)backend/FwLite/MiniLcm/Models/WritingSystemId.cs (6)
WritingSystemId(10-15)WritingSystemId(22-25)WritingSystemId(41-46)WritingSystemId(48-70)WritingSystemId(103-106)WritingSystemId(116-119)
⏰ 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). (5)
- GitHub Check: Build FwHeadless / publish-fw-headless
- GitHub Check: Analyze (csharp)
- GitHub Check: Build FW Lite and run tests
- GitHub Check: frontend-component-unit-tests
- GitHub Check: frontend
🔇 Additional comments (10)
backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs (1)
68-73: Deterministic WS ordering via pre-ordered source looks good.Switching to repo.WritingSystemsOrdered ensures stable defaults and headword behavior.
Please confirm WritingSystemsOrdered in the DbContext orders by Order then Id (tie-breaker) across providers, which the test suite assumes.
backend/FwLite/MiniLcm.Tests/WritingSystemTestsBase.cs (1)
149-177: Solid regression test for default WS reordering.Covers both GetWritingSystem(default, Vernacular) and list head position after insertion.
Is test data guaranteed to include "en" as initial default in all runners (CRDT and FWData)? If not, consider arranging it in test setup to avoid environment-coupled failures.
backend/FwLite/LcmCrdt/FullTextSearch/EntrySearchService.cs (2)
140-143: Good: use pre-ordered WS for single-entry updates.Removes local ordering and aligns with global semantics.
206-213: Good: pre-ordered WS for full regeneration as well.Matches UpdateEntrySearchTable and ensures deterministic headwords.
backend/FwLite/LcmCrdt/Data/MiniLcmRepository.cs (2)
17-18: DI type cleanup: ok.Switch to IDbContextFactory is fine and consistent with EF patterns.
70-71: Expose WritingSystemsOrdered: ok.Keeps ordering concerns in the data layer.
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (3)
90-94: Signature/order change in FromLcmWritingSystem is coherent.Index passed explicitly; aligns with tests not relying on Order semantics beyond position.
99-106: Comment about not relying on Order is helpful.Leaving Order as index here is acceptable given consumers ignore it.
183-184: Return value construction after create: ok.Index computed from container count – 1; consistent with earlier semantics.
backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.cs (1)
207-211: Handle wrapper is fine.Keeps existing handle-based call sites working while centralizing resolution.
1 participant
No description provided.