Show advanced field filter #1841
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 update introduces advanced filtering capabilities to the search filter UI, including field-based, operation-based, and writing system–based filtering. It adds new Svelte components for operation and writing system selection, extends existing select components for improved styling and icon customization, and enhances the writing system service with "no-audio" filtering options. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
the
💻 FW Lite
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
C# Unit Tests126 tests 126 ✅ 12s ⏱️ Results for commit a2f549e. ♻️ 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: 3
🧹 Nitpick comments (2)
frontend/viewer/src/project/browse/filter/OpFilter.svelte (1)
22-22: Consider a more neutral fallback icon.The
i-mdi-closeicon as a fallback might confuse users into thinking it's a clear/cancel button. Consider using a more neutral icon likei-mdi-helpori-mdi-dots-horizontalfor unmatched operations.frontend/viewer/src/project/browse/SearchFilter.svelte (1)
44-45: Consider removing the svelte-ignore directive if not needed.The
// svelte-ignore state_referenced_locallycomment on line 44 appears unnecessary sinceselectedFieldis properly used in the component. If there's no actual warning from Svelte, this directive can be removed to keep the code clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/viewer/src/lib/components/field-editors/select.svelte(3 hunks)frontend/viewer/src/lib/components/ui/select/select-trigger.svelte(2 hunks)frontend/viewer/src/lib/writing-system-service.svelte.ts(2 hunks)frontend/viewer/src/project/browse/BrowseView.svelte(1 hunks)frontend/viewer/src/project/browse/SearchFilter.svelte(4 hunks)frontend/viewer/src/project/browse/filter/OpFilter.svelte(1 hunks)frontend/viewer/src/project/browse/filter/WsSelect.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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.
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1698
File: backend/FwLite/LcmCrdt/Data/Filtering.cs:25-35
Timestamp: 2025-06-13T09:25:37.958Z
Learning: In backend/FwLite/LcmCrdt/Data/Filtering.cs `FtsFilter`, the `&&` combination between the FTS `MATCH` result and the `SearchValue` fallback is intentional to maintain existing search behavior; any future change to use `||` (or another approach) will be considered later.
frontend/viewer/src/lib/components/field-editors/select.svelte (5)
Learnt from: hahn-kev
PR: #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.
Learnt from: hahn-kev
PR: #1710
File: frontend/viewer/src/project/browse/BrowseView.svelte:17-19
Timestamp: 2025-05-27T06:18:33.852Z
Learning: The NewEntryButton component in frontend/viewer/src/project/NewEntryButton.svelte already internally checks features.write permission and conditionally renders based on write access, so external disabled props are not needed.
Learnt from: hahn-kev
PR: #1612
File: frontend/viewer/src/lib/entry-editor/DeleteDialog.svelte:39-52
Timestamp: 2025-04-18T10:33:51.961Z
Learning: Svelte 5 uses standard HTML attribute syntax for event handlers (e.g., onclick={handler}) instead of the Svelte-specific directive syntax (e.g., on:click={handler}) used in previous versions.
Learnt from: hahn-kev
PR: #1612
File: frontend/viewer/src/lib/entry-editor/DeleteDialog.svelte:39-52
Timestamp: 2025-04-18T10:33:51.961Z
Learning: Svelte 5 uses standard HTML attribute syntax for event handlers (e.g., onclick={handler}) instead of the Svelte-specific directive syntax (e.g., on:click={handler}) used in previous versions.
Learnt from: myieye
PR: #1802
File: frontend/viewer/src/project/NewEntryButton.svelte:36-36
Timestamp: 2025-07-04T17:00:57.368Z
Learning: In this codebase, $props.id() (Svelte rune) automatically returns a unique identifier per component instance, so components using it do not require an explicit id prop from parents.
frontend/viewer/src/lib/components/ui/select/select-trigger.svelte (2)
Learnt from: hahn-kev
PR: #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.
Learnt from: hahn-kev
PR: #1710
File: frontend/viewer/src/project/browse/BrowseView.svelte:17-19
Timestamp: 2025-05-27T06:18:33.852Z
Learning: The NewEntryButton component in frontend/viewer/src/project/NewEntryButton.svelte already internally checks features.write permission and conditionally renders based on write access, so external disabled props are not needed.
frontend/viewer/src/lib/writing-system-service.svelte.ts (1)
Learnt from: rmunn
PR: #1836
File: frontend/viewer/src/lib/components/audio/AudioDialog.svelte:25-25
Timestamp: 2025-07-22T09:19:37.367Z
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.
frontend/viewer/src/project/browse/filter/OpFilter.svelte (1)
Learnt from: hahn-kev
PR: #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.
frontend/viewer/src/project/browse/filter/WsSelect.svelte (2)
Learnt from: hahn-kev
PR: #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.
Learnt from: hahn-kev
PR: #1710
File: frontend/viewer/src/project/browse/BrowseView.svelte:17-19
Timestamp: 2025-05-27T06:18:33.852Z
Learning: The NewEntryButton component in frontend/viewer/src/project/NewEntryButton.svelte already internally checks features.write permission and conditionally renders based on write access, so external disabled props are not needed.
frontend/viewer/src/project/browse/SearchFilter.svelte (3)
Learnt from: hahn-kev
PR: #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.
Learnt from: hahn-kev
PR: #1698
File: backend/FwLite/LcmCrdt/Data/Filtering.cs:25-35
Timestamp: 2025-06-13T09:25:37.958Z
Learning: In backend/FwLite/LcmCrdt/Data/Filtering.cs FtsFilter, the && combination between the FTS MATCH result and the SearchValue fallback is intentional to maintain existing search behavior; any future change to use || (or another approach) will be considered later.
Learnt from: hahn-kev
PR: #1710
File: frontend/viewer/src/project/browse/BrowseView.svelte:17-19
Timestamp: 2025-05-27T06:18:33.852Z
Learning: The NewEntryButton component in frontend/viewer/src/project/NewEntryButton.svelte already internally checks features.write permission and conditionally renders based on write access, so external disabled props are not needed.
⏰ 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 UI / publish-ui
- GitHub Check: Build API / publish-api
- GitHub Check: check-and-lint
- GitHub Check: Analyze (csharp)
- GitHub Check: frontend
- GitHub Check: frontend-component-unit-tests
- GitHub Check: Build FW Lite and run tests
🔇 Additional comments (16)
frontend/viewer/src/lib/writing-system-service.svelte.ts (4)
18-19: LGTM! Type extension follows existing patterns.The new audio-filtered writing system selection types are well-named and consistent with the existing type structure.
60-66: Well-implemented filtering logic.The new getter methods correctly filter out audio writing systems using a clean and readable implementation that builds on existing getters.
93-96: Switch case implementation is correct.The new cases properly handle the audio-filtered writing system selections by delegating to the appropriate getter methods, maintaining consistency with the existing switch structure.
111-111: Good application of the new filtering capability.Using
vernacularNoAudiofor headword extraction makes logical sense, ensuring headwords don't come from audio-only writing systems.frontend/viewer/src/project/browse/BrowseView.svelte (1)
55-55: Container query class correctly applied.The
@container/listclass addition enables responsive styling for the enhanced filter UI within the left pane, which is appropriate for the new advanced filtering functionality.frontend/viewer/src/lib/components/field-editors/select.svelte (2)
32-32: Standard styling prop addition.The optional
classprop follows common UI component patterns and enables external styling customization.
45-45: Proper implementation of dynamic class handling.The
classNameextraction and application using thecn()utility correctly enables external styling while preserving existing button classes.Also applies to: 94-94
frontend/viewer/src/lib/components/ui/select/select-trigger.svelte (3)
5-5: Appropriate typing for customizable icon prop.The
IconClass | nulltype fordownIconproperly enables icon customization or complete removal, with correct TypeScript typing.Also applies to: 14-14
10-10: Backward-compatible default value.The default
downIconvalue maintains existing behavior while enabling customization, ensuring no breaking changes.
27-29: Clean conditional icon rendering.The truthy check for
downIconproperly enables icon customization or removal while maintaining consistent styling when rendered.frontend/viewer/src/project/browse/filter/OpFilter.svelte (2)
1-3: Well-designed operation type definition.The
Optype provides a comprehensive set of string filtering operations with clear, descriptive names. The module-level export enables proper reuse across components.
20-35: Clean Select component implementation.The dropdown implementation effectively uses the enhanced Select components with proper value binding, custom trigger styling, and appropriate fallback handling for unmatched operations.
frontend/viewer/src/project/browse/filter/WsSelect.svelte (1)
1-33: Well-structured writing system selector componentThe component is well-implemented with proper Svelte 5 syntax, good internationalization, and clear UI states. The multi-select functionality and display logic for different selection states are well thought out.
frontend/viewer/src/project/browse/SearchFilter.svelte (3)
16-21: LGTM! Appropriate imports for the new filtering functionality.The new imports align well with the PR objectives to add advanced field filtering capabilities.
120-143: Well-structured responsive layout for the advanced filter UI.The implementation provides a good user experience with:
- Logical flow from field selection to filter value input
- Responsive design that adapts to different screen sizes
- Proper use of flex layouts and container queries
39-46: Confirmed writing system categoriesThe literals
'vernacular-no-audio'and'analysis-no-audio'are defined in WritingSystemService and map correctly to thevernacularNoAudioandanalysisNoAudiogetters:
- In
frontend/viewer/src/lib/writing-system-service.svelte.ts
- case
'vernacular-no-audio': returnsthis.vernacularNoAudio(line 93)- case
'analysis-no-audio': returnsthis.analysisNoAudio(line 96)No further changes are required.
|
Looks really good! Found one bug. Repro:
|
There was a problem hiding this comment.
Apart from the one bug I found related to the WS selection not being cleared when switching from analysis (Gloss) to vernacular (Lexeme/Citation), and presumably vice-versa though I didn't test that, this LGTM. There's also a minor UX issue detailed below.
Approving so you don't need to request a re-review once that bug is fixed. The UX issue is really minor, and if it would be difficult to fix can easily wait until a later date, but if it's easy then the fix might as well go in now.
|
This looks like a cool improvement! One little nit... I personally find the "Contains" icon confusing. Since "Starts with" has a [ bracket at the start and "Ends with" has a ] bracket on the end, I would expect the "Contains" icon to be just ... ellipsis without [ ] brackets. |
|
Yeah that's fair, this is the contains icon in the Google icon set, so I'm hoping they did a lot of research about what the right icon should be. |
3 participants
This lets users be more specific about their filters.
Here's the UI:
on narrow lists:
clicking the filter by icon shows labels:
you can even filter for multiple or all writing systems
