Use normalization wrapper in js-invokable #2068
Conversation
github-actions
bot
added
the
💻 FW Lite
📝 WalkthroughWalkthroughThis PR introduces string normalization into the MiniLcm API pipeline by creating a new normalization wrapper and refactoring test infrastructure. Tests now use a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes The PR involves systematic refactoring across many test files (homogeneous pattern of Api→BaseApi replacements), balanced against meaningful architectural changes to wrapper composition order, constructor signature updates, and new test logic for normalization coverage. Mixed complexity across API wiring changes and test infrastructure updates. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
|
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/FwLite/MiniLcm/Normalization/MiniLcmApiStringNormalizationWrapper.cs (1)
42-45: Bug: wrapper breaks disposal chain (leak).IDisposable.Dispose() is empty, so disposing the outer wrapped API won’t dispose inner wrappers or the base API. Fix by forwarding disposal; also offer IAsyncDisposable.
Apply:
+public partial class MiniLcmApiStringNormalizationWrapper( + IMiniLcmApi api) : IMiniLcmApi, IAsyncDisposable +{ @@ - void IDisposable.Dispose() - { - } + public async ValueTask DisposeAsync() + { + if (_api is IAsyncDisposable asyncDisposable) + await asyncDisposable.DisposeAsync(); + else if (_api is IDisposable disposable) + disposable.Dispose(); + } + + void IDisposable.Dispose() + { + if (_api is IAsyncDisposable asyncDisposable) + asyncDisposable.DisposeAsync().AsTask().GetAwaiter().GetResult(); + else if (_api is IDisposable disposable) + disposable.Dispose(); + } +}
🧹 Nitpick comments (2)
backend/FwLite/MiniLcm/Normalization/MiniLcmApiStringNormalizationWrapper.cs (1)
27-40: Broaden normalization coverage.Currently normalizes SearchEntries/CountEntries/GetEntries and options. Consider also normalizing inputs for create/update paths and other query surfaces to ensure end-to-end consistency (e.g., CreateEntry/UpdateEntry, Create/UpdateSense, PartOfSpeech/ComplexFormType names, semantic domain search, media metadata if applicable).
backend/FwLite/MiniLcm.Tests/MiniLcmTestBase.cs (1)
18-21: Avoid passing null project to WrapWith.Use a test stub implementing IProjectIdentifier (e.g., “TestProject” with DataFormat appropriate to the suite) instead of null to de-risk future factories that read project data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/BasicApiTests.cs(1 hunks)backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/ComplexFormComponentTests.cs(3 hunks)backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/CreateEntryTests.cs(1 hunks)backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/MediaTests.cs(1 hunks)backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/PartOfSpeechTests.cs(2 hunks)backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/QueryEntryTests.cs(1 hunks)backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/UpdateEntryTests.cs(4 hunks)backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs(2 hunks)backend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.cs(1 hunks)backend/FwLite/FwLiteShared/Services/MiniLcmJsInvokable.cs(2 hunks)backend/FwLite/MiniLcm.Tests/MiniLcmTestBase.cs(1 hunks)backend/FwLite/MiniLcm.Tests/NormalizationTests.cs(1 hunks)backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs(1 hunks)backend/FwLite/MiniLcm/Normalization/MiniLcmApiStringNormalizationWrapper.cs(1 hunks)backend/FwLite/MiniLcm/Validators/MiniLcmValidators.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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.Tests/MiniLcmTests/CreateEntryTests.csbackend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/BasicApiTests.csbackend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/PartOfSpeechTests.csbackend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/UpdateEntryTests.csbackend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/MediaTests.cs
🧬 Code graph analysis (10)
backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs (2)
backend/FwLite/LcmCrdt/Data/MiniLcmRepository.cs (10)
Task(26-29)Task(95-103)Task(105-112)Task(114-118)Task(120-125)Task(157-195)Task(215-233)Task(235-241)Task(243-249)Task(251-256)backend/FwLite/LcmCrdt/FullTextSearch/EntrySearchService.cs (1)
LexemeForm(76-82)
backend/FwLite/FwLiteShared/Services/MiniLcmJsInvokable.cs (4)
backend/FwLite/MiniLcm/Validators/MiniLcmApiValidationWrapper.cs (3)
MiniLcmApiValidationWrapperFactory(8-16)IMiniLcmApi(10-10)IMiniLcmApi(12-15)backend/FwLite/MiniLcm/Normalization/MiniLcmApiStringNormalizationWrapper.cs (3)
MiniLcmApiStringNormalizationWrapperFactory(7-15)IMiniLcmApi(9-9)IMiniLcmApi(11-14)backend/FwLite/FwLiteShared/Services/MiniLcmApiNotifyWrapper.cs (1)
IMiniLcmApi(12-15)backend/FwLite/MiniLcm/Wrappers/MiniLcmWrappers.cs (2)
IMiniLcmApi(7-7)IMiniLcmApi(12-20)
backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/CreateEntryTests.cs (2)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (1)
FwDataMiniLcmApi(26-1764)backend/FwLite/FwDataMiniLcmBridge.Tests/Fixtures/ProjectLoaderFixture.cs (1)
FwDataMiniLcmApi(28-34)
backend/FwLite/MiniLcm.Tests/MiniLcmTestBase.cs (1)
backend/FwLite/MiniLcm/Normalization/MiniLcmApiStringNormalizationWrapper.cs (1)
MiniLcmApiStringNormalizationWrapperFactory(7-15)
backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/BasicApiTests.cs (2)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (1)
FwDataMiniLcmApi(26-1764)backend/FwLite/FwDataMiniLcmBridge.Tests/Fixtures/ProjectLoaderFixture.cs (1)
FwDataMiniLcmApi(28-34)
backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/PartOfSpeechTests.cs (2)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (1)
FwDataMiniLcmApi(26-1764)backend/FwLite/FwDataMiniLcmBridge.Tests/Fixtures/ProjectLoaderFixture.cs (1)
FwDataMiniLcmApi(28-34)
backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs (1)
backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs (1)
EntrySync(8-327)
backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/UpdateEntryTests.cs (1)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (1)
FwDataMiniLcmApi(26-1764)
backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/QueryEntryTests.cs (2)
backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs (1)
FwDataMiniLcmApi(42-45)backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (1)
FwDataMiniLcmApi(26-1764)
backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/MediaTests.cs (3)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (1)
FwDataMiniLcmApi(26-1764)backend/FwLite/FwDataMiniLcmBridge.Tests/Fixtures/ProjectLoaderFixture.cs (1)
FwDataMiniLcmApi(28-34)backend/FwLite/MiniLcm.Tests/MiniLcmTestBase.cs (2)
Task(14-14)Task(16-22)
⏰ 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: frontend-component-unit-tests
- GitHub Check: frontend
- GitHub Check: Build FW Lite and run tests
- GitHub Check: Analyze (csharp)
🔇 Additional comments (17)
backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs (2)
1-1: LGTM!The
using System.Text;directive is necessary for theNormalizationFormenum used in the new test.
70-101: Good test demonstrating normalization behavior.The test effectively verifies that string normalization to NFD works during sync operations. The test structure is clear with well-defined arrange/act/assert sections, and the assertions on lines 77-80 properly validate the test setup.
The commented-out assertions for
entry2with the reference to issue #2065 appropriately document a known limitation, which aligns with the PR objectives.One minor suggestion: consider adding a brief comment before line 83 explaining the test pattern where
entry1is created empty in the database, but the sync "before" state (line 84) represents what the external source thinks exists. This pattern might not be immediately obvious to readers unfamiliar with sync testing.backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs (1)
333-362: LGTM! Good normalization test coverage.The addition of the
identicalparameter improves test readability when cases differ only in normalization form. The Cyrillic test cases (lines 344-351) provide valuable coverage for both FormC and FormD normalization variants.The TODO comment on line 356 appropriately references issue #2065 for future cleanup.
backend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.cs (1)
10-10: LGTM! Using directive addition aligns with normalization namespace.This addition supports the normalization wrapper usage introduced in this PR.
backend/FwLite/MiniLcm.Tests/NormalizationTests.cs (1)
1-1: LGTM! Namespace update reflects normalization code relocation.The using directive correctly references the new
MiniLcm.Normalizationnamespace.backend/FwLite/MiniLcm/Validators/MiniLcmValidators.cs (1)
4-4: LGTM! Using directive supports normalization integration.This addition enables access to normalization wrapper types in this file.
backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/CreateEntryTests.cs (1)
20-20: LGTM! BaseApi access pattern is correct.The change from
ApitoBaseApicorrectly accesses the unwrapped FwDataMiniLcmApi instance for direct repository access. This aligns with the test architecture whereBaseApiprovides the underlying API before normalization wrapping.backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/QueryEntryTests.cs (1)
16-16: LGTM! BaseApi access pattern is correct.The change from
ApitoBaseApicorrectly accesses the unwrapped FwDataMiniLcmApi instance for direct repository access.backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/UpdateEntryTests.cs (1)
29-29: LGTM! BaseApi access pattern consistently applied.All four instances correctly use
BaseApiinstead ofApito access the unwrapped FwDataMiniLcmApi for direct repository and cache operations. This aligns with the test architecture where repository-level operations require the unwrapped API instance.Also applies to: 78-78, 118-118, 181-181
backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/MediaTests.cs (1)
24-24: LGTM! BaseApi access pattern correctly applied in test lifecycle methods.Both
InitializeAsyncandDisposeAsynccorrectly useBaseApito access the unwrapped FwDataMiniLcmApi instance for cache and project folder operations.Also applies to: 30-30
backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/ComplexFormComponentTests.cs (1)
30-33: Good switch to BaseApi for FwData internals.Direct casting to FwDataMiniLcmApi from BaseApi is the right assertion in FwData-specific tests and matches the new wrapper layering. LGTM.
Based on learnings
Also applies to: 72-76, 110-114
backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/BasicApiTests.cs (1)
25-31: LGTM: use BaseApi for repository/UoW access.Switching to (FwDataMiniLcmApi)BaseApi keeps tests precise and asserts correct setup.
Based on learnings
backend/FwLite/MiniLcm/Normalization/MiniLcmApiStringNormalizationWrapper.cs (1)
20-21: Confirm normalization form (FormD) is intentional.Verify FormD matches how storage, search index, and JS client normalize to avoid mismatches with composed forms (NFC).
backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/PartOfSpeechTests.cs (1)
29-35: LGTM: BaseApi cast for FwData-specific setup.Matches the suite-wide pattern and asserts test configuration early.
Based on learnings
Also applies to: 64-70
backend/FwLite/MiniLcm.Tests/MiniLcmTestBase.cs (1)
12-13: BaseApi exposure is appropriate.Exporting BaseApi alongside wrapped Api cleanly separates raw FwData access from normalized behavior in tests. LGTM.
backend/FwLite/FwLiteShared/Services/MiniLcmJsInvokable.cs (2)
25-26: Wrapper order is correct (normalize → validate → notify).Given WrapWith reverses factories, this yields Normalization as the outermost wrapper. Good.
Please confirm DI registration now supplies MiniLcmApiStringNormalizationWrapperFactory to this constructor across all hosting environments.
21-24: Constructor DI surface change.Adding normalizationWrapperFactory is fine; ensure all composition roots (server, tests, any tools) are updated to avoid activation failures.
With this change users will be able to find data from FieldWorks regardless of whether they type in NFD or NFC. Previously they would have had to type in NFD. That's the vast majority of data in a project, so that's a big win.
Unfortunately, users will now not be able to find NFC data that was created in FieldWorks Lite (and hasn't yet been round-tripped through a sync), because we actually save it in NFC. (See #2065)
I also:
The normalization wrapper was added a long time ago, but we overlooked actually using it 🙃.
The wrapper should maybe be more deeply embedded in our IoC container, but...I'm not sure how much effort that would be.
I think this is good for now.