Add semantic domain filter #2041
Conversation
github-actions
bot
added
the
💻 FW Lite
|
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 📝 WalkthroughWalkthroughAdds null-option support to Select editor, updates Switch styling/markup, refactors SearchFilter to use new FieldSelect, MissingSelect, and SemanticDomainSelect components, and updates locale .po files to align with the new filter structure and strings, including adding “Any,” “Include subdomains,” and consolidating “Missing…” options. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
UI unit Tests 1 files ± 0 3 suites - 42 0s ⏱️ -29s Results for commit 4382ffc. ± Comparison against base commit e8a8232. This pull request removes 111 and adds 10 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
|
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
🧹 Nitpick comments (8)
frontend/viewer/src/lib/components/field-editors/select.svelte (2)
128-139: Consider simplifying the null option check icon logic.While the null option CommandItem is well-structured with proper keywords, aria-label, and styling, the check icon logic could be simplified after fixing line 136.
If you want to be more explicit about the condition, consider:
- <Icon icon="i-mdi-check" class={cn('md:hidden', value === null || 'invisible')} /> + <Icon icon="i-mdi-check" class={cn('md:hidden', value == null ? '' : 'invisible')} />This makes the intent clearer: show the check mark when value is null/undefined, hide it otherwise.
101-101: No instances of combined usage found
Ripgrep across.sveltefiles returned no<Select>components using bothplaceholderandnullOption; reversing precedence isn’t necessary. Consider adding documentation to clarify the existingplaceholder ?? nullOption ?? defaultbehavior when both props are provided.frontend/viewer/src/lib/components/ui/switch/switch.svelte (1)
28-28: Hide spacer from assistive techMark the spacer as presentation-only to avoid confusing screen readers.
- <span class="max-w-0"> </span> + <span class="max-w-0" aria-hidden="true"> </span>frontend/viewer/src/project/browse/filter/MissingSelect.svelte (1)
4-12: Strengthen typing of options withas constThis makes
MissingOptiona precise union of ids/labels and prevents accidental typos.- const missingOptions = [ + const missingOptions = [ { id: 'semanticDomains', label: gt`Semantic Domains` }, { id: 'partOfSpeech', label: gt`Part of Speech` }, { id: 'senses', label: gt`Senses` }, { id: 'examples', label: gt`Example Sentences` }, - ] + ] as constfrontend/viewer/src/project/browse/SearchFilter.svelte (4)
73-75: Escape semantic domain code before injecting into filter.Safer to escape even if codes are numeric with dots; future codes or user-entered values won’t break parsing.
Apply this diff:
- if (semanticDomain) { - newFilter.push(`Senses.SemanticDomains.Code${includeSubDomains ? '^' : '='}${semanticDomain.code}`); - } + if (semanticDomain) { + const code = escapeGridifyValue(semanticDomain.code); + newFilter.push(`Senses.SemanticDomains.Code${includeSubDomains ? '^' : '='}${code}`); + }
77-78: Prefer undefined for “no filters” instead of empty string.Keeps the prop’s contract (
string | undefined) and avoids sending an empty filter token downstream.Apply this diff:
- gridifyFilter = newFilter.join(', '); + gridifyFilter = newFilter.length ? newFilter.join(', ') : undefined;
124-127: Selected WS may not match the chosen field’s WS type.
selectedWsis initialized to vernacular; switching to “Gloss” (analysis-only) could leave invalid WS IDs selected unlessWsSelectprunes them. IfWsSelectdoesn’t enforce, resetselectedWswhenselectedField.wschanges.Optional effect:
+ $effect(() => { + if (!selectedField) return; + if (selectedField.ws === 'analysis-no-audio') { + selectedWs = wsService.analysisNoAudio.map(ws => ws.wsId); + } else if (selectedField.ws === 'vernacular-no-audio') { + selectedWs = wsService.vernacularNoAudio.map(ws => ws.wsId); + } else { + selectedWs = [...wsService.vernacularNoAudio, ...wsService.analysisNoAudio].map(ws => ws.wsId); + } + });
141-143: Optional: clear “Include subdomains” when domain is cleared.State stays true when disabled; harmless, but you can auto-reset for cleaner UX.
+ $effect(() => { + if (!semanticDomain) includeSubDomains = false; + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
frontend/viewer/src/lib/components/field-editors/select.svelte(5 hunks)frontend/viewer/src/lib/components/ui/switch/switch.svelte(2 hunks)frontend/viewer/src/locales/en.po(11 hunks)frontend/viewer/src/locales/es.po(11 hunks)frontend/viewer/src/locales/fr.po(11 hunks)frontend/viewer/src/locales/id.po(11 hunks)frontend/viewer/src/locales/ko.po(11 hunks)frontend/viewer/src/locales/ms.po(11 hunks)frontend/viewer/src/locales/sw.po(11 hunks)frontend/viewer/src/project/browse/SearchFilter.svelte(5 hunks)frontend/viewer/src/project/browse/filter/FieldSelect.svelte(1 hunks)frontend/viewer/src/project/browse/filter/MissingSelect.svelte(1 hunks)frontend/viewer/src/project/browse/filter/SemanticDomainSelect.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-18T05:13:00.591Z
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1757
File: frontend/viewer/src/lib/components/field-editors/multi-select.svelte:130-136
Timestamp: 2025-06-18T05:13:00.591Z
Learning: In frontend/viewer/src/lib/components/field-editors/multi-select.svelte, the computeCommandScore function from 'bits-ui' handles empty filter strings appropriately and does not hide all options when the filter is empty, contrary to initial analysis assumptions.
Applied to files:
frontend/viewer/src/project/browse/filter/MissingSelect.sveltefrontend/viewer/src/project/browse/SearchFilter.svelte
⏰ 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). (6)
- GitHub Check: Build UI / publish-ui
- GitHub Check: Build API / publish-api
- GitHub Check: check-and-lint
- GitHub Check: Build FW Lite and run tests
- GitHub Check: frontend-component-unit-tests
- GitHub Check: frontend
🔇 Additional comments (13)
frontend/viewer/src/lib/components/field-editors/select.svelte (3)
29-29: LGTM! Type signature correctly supports undefined.The widened signature allows the component to handle null option selections.
31-31: LGTM! New nullOption prop is correctly defined and propagated.The new prop enables rendering an optional null/unset option in the select list.
Also applies to: 44-45
71-75: LGTM! Function signature correctly accepts undefined.Consistent with the updated onchange type and enables selecting the null option.
frontend/viewer/src/project/browse/filter/MissingSelect.svelte (1)
21-32: LGTMBinding maps cleanly between select id and the exported
valueobject; placeholder and options are correct.frontend/viewer/src/locales/fr.po (1)
142-145: Provide translations for new filter strings (avoid fallback to English)New keys have empty translations: “Any”, “Include subdomains”, “Incomplete entries”, “Semantic domain”, “Specific field”, “Missing...”. Please supply translations or confirm intentional fallback before release.
Also applies to: 615-618, 620-622, 986-989, 1049-1052, 734-737
frontend/viewer/src/locales/ms.po (1)
142-145: Fill in new filter translations or confirm fallbackEmpty msgstr for: “Any”, “Include subdomains”, “Incomplete entries”, “Semantic domain”, “Specific field”, “Missing...”. Ensure these are translated via Crowdin or acceptable to ship with English fallback.
Also applies to: 615-618, 620-622, 986-989, 1049-1052, 734-737
frontend/viewer/src/locales/es.po (1)
142-145: Add translations for new filter UI stringsEmpty msgstr for: “Any”, “Include subdomains”, “Incomplete entries”, “Semantic domain”, “Specific field”, “Missing...”. Please translate or confirm fallback behavior.
Also applies to: 615-618, 620-622, 986-989, 1049-1052, 734-737
frontend/viewer/src/locales/id.po (1)
142-145: Complete translations for new filter stringsEmpty msgstr for: “Any”, “Include subdomains”, “Incomplete entries”, “Semantic domain”, “Specific field”, “Missing...”. Translate or confirm fallback is acceptable.
Also applies to: 615-618, 620-622, 986-989, 1049-1052, 734-737
frontend/viewer/src/project/browse/SearchFilter.svelte (2)
48-53: Verify missing Part of Speech filter semantics.Other “missing …” cases use
=null, but Part of Speech uses an empty equality (Senses.PartOfSpeechId=). Confirm which your API expects for “missing” PoS. If PoS is nullable, prefer=nullfor consistency.Option if nullable:
- case 'partOfSpeech': newFilter.push('Senses.PartOfSpeechId='); break; + case 'partOfSpeech': newFilter.push('Senses.PartOfSpeechId=null'); break;
55-63: Confirm Gridify operator mapping.Ensure these operator tokens (
^,=*,$,=,!=) match your Gridify backend expectations for starts-with/contains/ends-with/equals/not-equals. A mismatch could silently return wrong results.frontend/viewer/src/locales/en.po (1)
137-140: Locales rehomed correctly; keys align with new components.New/relocated msgids (“Any”, “Field”, “Display as”, “Lexeme Form”, “Semantic domain”, “Specific field”, etc.) map to FieldSelect/SemanticDomainSelect/SearchFilter as expected. No issues spotted.
Also applies to: 208-211, 345-349, 470-473, 564-568, 610-617, 643-646, 981-984, 1044-1047, 1228-1235
frontend/viewer/src/locales/ko.po (1)
142-145: New filter strings present; translations pending.New msgids (“Any”, “Citation Form”, “Display as”, “Field”, “Gloss”, “Include subdomains”, “Incomplete entries”, “Lexeme Form”, “Semantic domain”, “Specific field”, “Word”) are wired to the new components. Many msgstr are empty; that’s fine if Crowdin will handle. Ensure extraction/compile runs.
Also applies to: 213-216, 350-354, 475-478, 569-573, 615-618, 619-622, 648-651, 986-989, 1049-1052, 1233-1240
frontend/viewer/src/locales/sw.po (1)
142-145: Swahili locale updated to new filter structure; untranslated entries are expected.Keys are correctly re-associated with FieldSelect/MissingSelect/SemanticDomainSelect. Several msgstr remain empty; confirm your Crowdin sync will backfill or mark for translation.
Also applies to: 213-216, 350-354, 475-478, 569-573, 615-618, 619-622, 648-651, 986-989, 1049-1052, 1233-1240
myieye
force-pushed
the
1684-semantic-domain-filter
branch
from
0aebc72 to
be97438
Compare
myieye
force-pushed
the
1684-semantic-domain-filter
branch
from
1b638ad to
4382ffc
Compare
Resolves: #1684
Include subdomainsis OFF by default.