feat: add search functionality to filters#349
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds optional Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant F as FiltersUI
participant D as Debouncer (500ms)
participant API as Invoice Block API
participant M as Mock Mapper
participant R as Results
U->>F: type into search input
F->>D: onChange -> schedule debounced submit
D--)API: submit query { search }
API->>M: mapInvoices(query)
M->>M: filter invoices where invoice.id contains search (case-insensitive)
M-->>API: return filtered list
API-->>R: render filtered invoices
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
Comment |
![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 (1)
packages/ui/src/components/Filters/Filters.stories.tsx (1)
243-281: New stories effectively demonstrate search functionality.The
WithSearchandWithSearchLeadingstories provide good coverage of the search feature, including the leading item behavior which triggers automatic submission on value change.Consider adding a story variant with a pre-populated search value to demonstrate how the component handles non-empty initial search state:
+export const WithSearchPreselected: Story = { + args: { + filters: { + ...basicFilters, + items: [ + { ...searchFilterItem, isLeading: true }, + categoryFilterItem, + priceFilterItem, + ratingFilterItem, + sortFilterItem, + ], + }, + initialValues: { + category: 'electronics', + price: '', + rating: [], + sort: 'relevance', + search: 'laptop', + }, + hasLeadingItem: true, + }, +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.changeset/wise-crews-smoke.md(1 hunks)packages/blocks/invoice-list/src/api-harmonization/invoice-list.request.ts(1 hunks)packages/blocks/invoice-list/src/frontend/InvoiceList.client.tsx(1 hunks)packages/framework/src/modules/invoices/invoices.request.ts(1 hunks)packages/framework/src/utils/models/filters.ts(2 hunks)packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.invoice-list.mapper.ts(3 hunks)packages/integrations/mocked/src/modules/invoices/invoices.mapper.ts(1 hunks)packages/ui/package.json(1 hunks)packages/ui/src/components/Filters/FilterItem.tsx(3 hunks)packages/ui/src/components/Filters/Filters.stories.tsx(7 hunks)
🔇 Additional comments (14)
packages/framework/src/modules/invoices/invoices.request.ts (1)
14-14: LGTM!The addition of the optional
searchparameter follows the existing pattern and is consistent with other filter fields.packages/blocks/invoice-list/src/frontend/InvoiceList.client.tsx (1)
35-35: LGTM!The search field initialization is consistent with the pattern used for other filters and correctly initializes to an empty string.
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.invoice-list.mapper.ts (3)
51-57: LGTM!The FilterText item for search is correctly configured with appropriate localization and
isLeading: truefor prominent placement.
174-180: LGTM!The German localization for the search filter is properly configured.
297-303: LGTM!The Polish localization for the search filter is properly configured.
.changeset/wise-crews-smoke.md (1)
1-8: LGTM!The changeset correctly documents the feature addition with appropriate minor version bumps for all affected packages.
packages/integrations/mocked/src/modules/invoices/invoices.mapper.ts (1)
222-228: LGTM!The search implementation is correct and user-friendly:
- Case-insensitive matching for better UX
- Searches the invoice ID field as indicated by the placeholder text
- Properly placed before other filters in the filter chain
packages/blocks/invoice-list/src/api-harmonization/invoice-list.request.ts (1)
9-9: LGTM!The addition of the optional
searchparameter is consistent with the framework's request type and follows the existing pattern.packages/ui/src/components/Filters/FilterItem.tsx (2)
2-9: LGTM!The new imports are correctly added to support the FilterText functionality.
183-208: FilterText implementation looks good overall.The FilterText case correctly renders a search input with:
- Proper field binding
- Search icon adornment
- Placeholder text
- Debounced submission for leading filters
The core logic is sound, pending the debounce timing and cleanup issues noted separately.
packages/ui/src/components/Filters/Filters.stories.tsx (3)
55-56: Type definitions updated consistently.The
FilterTypeandFilterValuestypes are correctly extended with thesearchfield, maintaining consistency with the framework changes.
130-135: searchFilterItem definition looks good.The new filter item is properly typed and includes appropriate labels and placeholder text for a search input.
161-161: All initialValues consistently updated.The
searchfield is properly initialized to an empty string across all story variants, ensuring consistent behavior.Also applies to: 175-175, 189-189, 203-203
packages/framework/src/utils/models/filters.ts (1)
50-53: FilterText integration is complete and properly handled in UI rendering.The verification confirms that packages/ui/src/components/Filters/FilterItem.tsx at line 183 contains a case statement handling
'FilterText', which means the UI component is already set up to render FilterText filter items. The newFilterTextclass follows the established pattern of other filter types and integrates seamlessly into the existing rendering logic.
… feature/search-filter-item
…nd update dependencies
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/framework/src/utils/models/filters.ts (1)
9-9: Verify all Filters usages have been updated to include the required search field.This line introduces a breaking change by requiring
search: stringon allFilters<T>instances. A previous review identified 12 incompatible usages across the codebase where types are missing the requiredsearchfield.Run the following script to verify all affected usages have been updated:
#!/bin/bash # Description: Verify that all Filters<T> usages now include the required search field echo "=== Checking model files that define filter types ===" rg -n "Filters<" --type=ts --type=tsx -A2 -B2 | rg -C3 "(Order|Ticket|Contract|Product|OrderItem|Invoice|Notification)" echo -e "\n=== Verifying type definitions include search field ===" rg -n "type.*=.*\{.*sort.*\}" --type=ts --type=tsx -C3 | rg -C2 "search"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
packages/framework/src/utils/models/filters.ts(2 hunks)packages/ui/package.json(2 hunks)packages/ui/src/components/Filters/FilterItem.tsx(4 hunks)packages/ui/src/components/Filters/Filters.stories.tsx(7 hunks)packages/ui/src/elements/input.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ui/package.json
- packages/ui/src/components/Filters/FilterItem.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
packages/framework/src/utils/models/filters.ts (1)
packages/ui/src/components/Filters/FilterItem.tsx (1)
FilterItem(16-217)
packages/ui/src/components/Filters/Filters.stories.tsx (1)
packages/framework/src/utils/models/filters.ts (1)
FilterText(50-54)
packages/ui/src/elements/input.tsx (1)
packages/ui/src/lib/utils.ts (1)
cn(5-7)
⏰ 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). (1)
- GitHub Check: deploy-preview
🔇 Additional comments (5)
packages/ui/src/elements/input.tsx (1)
129-138: LGTM with dependency on fixing the aria-label computation.The conditional rendering of the label wrapper and the propagation of
aria-labelto theInputcomponent are correctly implemented. This approach properly hides the visual label while attempting to maintain accessibility.Note: This implementation depends on fixing the
ariaLabelcomputation at line 125 to ensure accessibility is maintained in all cases.packages/ui/src/components/Filters/Filters.stories.tsx (3)
130-135: LGTM!The
searchFilterItemdefinition is correctly typed asFilterTextand includes appropriate label and placeholder properties.
161-161: LGTM!The
search: ''field has been consistently added toinitialValuesacross all story variants, properly satisfying the new type constraint.Also applies to: 175-175, 189-189, 203-203
243-281: LGTM!The new stories effectively demonstrate the search filter functionality.
WithSearchshows basic usage, whileWithSearchLeadingshowcases advanced features likeisLeadingandisLabelHidden, providing comprehensive coverage of the new functionality.packages/framework/src/utils/models/filters.ts (1)
50-54: LGTM!The
FilterTextclass definition is well-structured and follows the established pattern of other filter classes. The optionalisLabelHiddenandplaceholderproperties align with the UI implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/ui/src/components/Filters/FilterItem.tsx (1)
36-38: Add cleanup for the debounced function to prevent memory leaks.The debounced function needs to be cancelled on component unmount to prevent calling
submitFormafter the component has unmounted, which could cause memory leaks or errors.Apply this diff to add cleanup:
+ useEffect(() => { + return () => { + onTextFilterChange.cancel?.(); + }; + }, [onTextFilterChange]); + const onTextFilterChange = debounce(500, async () => { await submitForm(); });Note: You may need to wrap
onTextFilterChangeinuseMemooruseCallbackto ensure the cleanup effect has a stable reference.
🧹 Nitpick comments (1)
packages/ui/src/components/Filters/FilterItem.tsx (1)
25-34: Consider simplifying by removing refs and useEffects.As noted in previous review feedback, this ref synchronization pattern adds unnecessary complexity. The debounced function can directly reference
isLeadingandsubmitFormwithout needing refs and useEffect hooks, improving both code clarity and performance.Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.invoice-list.mapper.ts(3 hunks)packages/ui/src/components/Filters/FilterItem.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.invoice-list.mapper.ts
⏰ 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). (1)
- GitHub Check: Determine which apps changed / dry-run
🔇 Additional comments (4)
packages/ui/src/components/Filters/FilterItem.tsx (4)
2-9: Imports look good.The new imports (Search icon, useEffect, debounce, InputWithLabel) are all necessary for the text filter functionality.
89-89: Nice simplification of the ALL handling logic.The direct boolean assignment makes the code clearer and more concise.
151-183: Layout wrapper improves consistency.The grid container with gap spacing provides better vertical alignment between the label and select elements.
184-213: Text filter implementation looks good, but verify edge case behavior.The new FilterText case properly implements the search functionality with the isLabelHidden prop addressing previous feedback. The debounced submission is only triggered when isLeading is true at the time of input change.
However, there's a potential edge case: if
isLeadingchanges fromtruetofalseduring the 500ms debounce delay, the form will still submit when the debounced function executes. Consider whether this edge case needs handling or if it's acceptable in your use case.Based on learnings.
marcinkrasowski
changed the title
…-item # Conflicts: # package-lock.json # packages/ui/package.json
2 participants
What does this PR do?
Related Ticket(s)
Key Changes
How to test
Media (Loom or gif)
Summary by CodeRabbit
New Features
Chores