Fix search + missing senses #2006
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 📝 WalkthroughWalkthroughChanges reorder filtering in MiniLcmRepository: exemplar and Gridify filters are applied at the start of GetEntries before full-text search and sorting. FilterEntries no longer applies these filters. Tests add SearchEntries GridifyFilter cases mirroring GetEntries coverage. No public APIs changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 ↗︎
|
![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
🧹 Nitpick comments (2)
backend/FwLite/LcmCrdt/Data/MiniLcmRepository.cs (1)
160-166: Use a domain‑specific exception instead of NullReferenceException.Throwing NullReferenceException for a missing writing system is misleading. Prefer a NotFound/InvalidOperation exception for clarity and consistency with other repo methods.
Suggested change:
- if (ws is null) - throw new NullReferenceException($"writing system {options.Exemplar.WritingSystem} not found"); + if (ws is null) + throw new InvalidOperationException($"Writing system {options.Exemplar.WritingSystem} not found");If available, consider
throw new NotFoundException($"Writing system {options.Exemplar.WritingSystem} not found");to match your existing NotFound semantics.backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs (1)
103-110: De‑duplicate repeated test patterns with a helper.Most new tests follow “run SearchEntries with gridify, project LexemeForm[en], assert set.” A small helper will cut repetition and improve readability.
Example helper and one refactor:
public abstract class QueryEntryTestsBase : MiniLcmTestBase { + private async Task<string[]> SearchLexemesAsync(string? term, string gridify) + => (await Api.SearchEntries(term, new(Filter: new() { GridifyFilter = gridify })) + .ToArrayAsync()) + .Select(e => e.LexemeForm["en"]).ToArray(); @@ - public async Task CanFilterLexemeFormContains_AndSearch() - { - var results = await Api.SearchEntries(Banana, new(Filter: new() { GridifyFilter = "LexemeForm[en]=*nan" })).ToArrayAsync(); - results.Select(e => e.LexemeForm["en"]).Should().BeEquivalentTo(Banana); - } + public async Task CanFilterLexemeFormContains_AndSearch() + { + var lexemes = await SearchLexemesAsync(Banana, "LexemeForm[en]=*nan"); + lexemes.Should().BeEquivalentTo(Banana); + }Optional: add one exemplar+search test to exercise the new exemplar‑first pipeline.
Also applies to: 118-124, 133-140, 150-158, 166-172, 195-202, 239-245, 262-268, 276-282, 297-303
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/FwLite/LcmCrdt/Data/MiniLcmRepository.cs(1 hunks)backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/FwLite/LcmCrdt/Data/MiniLcmRepository.cs (1)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (1)
WritingSystem(105-119)
backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs (1)
backend/FwLite/LcmCrdt/Data/MiniLcmRepository.cs (10)
Task(26-29)Task(94-102)Task(104-111)Task(113-117)Task(119-124)Task(156-194)Task(214-232)Task(234-240)Task(242-247)Task(249-254)
⏰ 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: frontend-component-unit-tests
- GitHub Check: Build FW Lite and run tests
- GitHub Check: frontend
🔇 Additional comments (2)
backend/FwLite/LcmCrdt/Data/MiniLcmRepository.cs (1)
160-173: Pre‑FTS exemplar + Gridify filtering is the right call.Applying exemplar and ApplyFiltering before FTS avoids EF expression composition issues and reduces the search set early. Looks good.
backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs (1)
103-110: Solid parity coverage for Gridify + SearchEntries.These tests mirror the GetEntries cases and validate the pre‑FTS filter order. Assertions focus on membership, avoiding ordering coupling. Good additions.
Also applies to: 118-124, 133-140, 150-158, 166-172, 195-202, 239-245, 262-268, 276-282, 297-303
No description provided.