-
Notifications
You must be signed in to change notification settings - Fork 667
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Search Feature in Devices with name and location wise | Add Clear button in Location Search #10831
base: develop
Are you sure you want to change the base?
Conversation
WalkthroughThe changes update two components. In the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@rithviknishad @Jacobjeevan Needed backend support care#2872 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pages/Facility/settings/devices/DevicesList.tsx (1)
62-87
: Implementation of the search and location filter UI is well executed.
The combination ofInput
for the device name search andLocationSearch
for location filtering is straightforward, and resetting the page on search input change is a good user experience.Consider adding a label or
aria-label
for the input to enhance accessibility for screen readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Facility/settings/devices/DevicesList.tsx
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: Test
- GitHub Check: cypress-run (1)
🔇 Additional comments (7)
src/pages/Facility/settings/devices/DevicesList.tsx (7)
10-10
: No issues with the new import of the Input component.
The usage is straightforward and aligns well with the UI library structure.
15-15
: ImportingLocationSearch
appears valid and consistent.
No concerns regarding the dependency or import path.
20-20
: The import ofLocationList
is correctly placed.
Ensuring strong typing for location data is a good practice here.
29-32
: State variables for search and selected location look appropriate.
Having a nullableLocationList
state helps manage scenarios where no location is selected.
37-44
: IncludingsearchQuery
andselectedLocation
in the queryKey is solid.
This ensures that the query automatically re-fetches and reflects changes in either state.
50-51
: Neat parameter handling forregistered_name
andlocation
.
Passingundefined
when values are empty or null is a valid approach to avoid sending unwanted query parameters.
58-58
: Well-structured layout modifications.
The updated classnames effectively maintain a responsive layout for the container.
@rithviknishad @Jacobjeevan Backend PR got merged. Now this PR is Ready for review |
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
…update DevicesList to use useFilters for pagination and search
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/pages/Facility/settings/devices/DevicesList.tsx (2)
29-31
: Consider storing just the location ID for improved stability.
Storing the entireLocationList
object in state might lead to unnecessary re-renders or refetches whenever the object reference changes, even if the ID remains the same. Persisting onlyselectedLocation?.id
is a simpler alternative unless the full object is needed elsewhere.
39-45
: Avoid passing entire objects as query keys to prevent redundant refetches.
Keys likeqParams
andselectedLocation
may trigger additional network requests if their references change frequently. Passing only stable representations (likeqParams.page
,qParams.search_text
, etc., plusselectedLocation?.id
) can help mitigate this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json
(1 hunks)src/components/Location/LocationSearch.tsx
(4 hunks)src/pages/Facility/settings/devices/DevicesList.tsx
(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- public/locale/en.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Location/LocationSearch.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
🔇 Additional comments (8)
src/pages/Facility/settings/devices/DevicesList.tsx (8)
10-10
: All newly imported modules seem appropriate and consistent.
These imports appear to be correctly used later in the file. No concerns.Also applies to: 14-14, 16-16, 21-21
33-36
: Kudos on using a caching hook with a blacklist.
UsinguseFilters
and blacklisting query params helps avoid cache key collisions. This is a neat approach to controlling unwanted caching behavior for search parameters.
59-59
: Responsive layout changes look good.
This line adds helpful spacing and flex layout classes. No issues found.
63-65
: Search input configuration is clean and user-friendly.
You have proper coherence with i18n for the placeholder, andonChange
seamlessly updatesqParams.search_text
. Good job integrating with the debounced query.Also applies to: 66-72
74-74
: LocationSearch integration is straightforward and consistent.
Allowingnull
foronSelect
effectively clears the selected location. This satisfies the requirement for single-input searching while also filtering by location when needed.Also applies to: 75-83
86-92
: "Add Device" button is well-placed and intuitive.
Linking to"/devices/create"
is consistent if your routes handle facility context at or after creation. No concerns here.
114-114
: Pagination usage is correctly integrated.
Connecting the total count todata?.count
ensures accurate pagination. Implementation looks good.
49-52
:❓ Verification inconclusive
Confirm that offset resets to page 1 when new search filters are applied.
IfqParams.page
retains its previous value whensearch_text
orcurrent_location
changes, users might land on an empty page. Verify that the pagination logic resets page to 1 on filter changes.
🏁 Script executed:
#!/bin/bash # Searching for explicit reset of page to 1 upon search_text/current_location changes rg -A 3 'updateQuery\(\{.*page:\s*1'Length of output: 38
Action Required: Confirm Pagination Reset on Filter Changes
The current implementation calculates
offset
based onqParams.page
without any explicit reset when the search filters (search_text
orcurrent_location
) change. This could lead to scenarios where a user, upon updating the filters, might retain a non-first page value and consequently land on an empty result page.
- Verify: Ensure that when
search_text
orcurrent_location
changes, the pagination is correctly reset by updatingqParams.page
to 1.- Review Location: Please inspect and confirm this logic in
src/pages/Facility/settings/devices/DevicesList.tsx
(around lines 49-52) and any related handlers or effects that might update the query parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pages/Facility/settings/devices/DevicesList.tsx (1)
57-86
: Debounce search to reduce rapid re-fetches.
Consider wrappingupdateQuery
with a debounce to avoid initiating frequent queries while typing, improving performance.- onChange={(e) => { - updateQuery({ search_text: e.target.value }); - }} + onChange={debounce((e) => { + updateQuery({ search_text: e.target.value }); + }, 300)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Facility/settings/devices/DevicesList.tsx
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: Test
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (10)
src/pages/Facility/settings/devices/DevicesList.tsx (10)
10-10
: No issues with this import.
This addition is consistent with the existing UI framework usage.
14-14
: LocationSearch import is valid.
No concerns here; it matches the usage below for location-based filtering.
16-16
: UsinguseFilters
.
Adopting a dedicated filtering hook is a good approach for handling pagination and query state.
21-21
: Type import forLocationList
.
This import is aligned with the state handling inselectedLocation
.
29-31
: State initialization is clear.
InitializingselectedLocation
tonull
accommodates scenarios where no location is selected.
33-36
: Proper use ofuseFilters
.
Setting a default limit and blacklisting specific params is appropriate for this context.
39-39
: Query key update.
Including all relevant parameters (facilityId
,qParams
,resultsPerPage
) ensures consistent re-fetch.
43-46
: Offset calculation.
Be mindful of edge cases ifqParams.page
is 0 or negative. Clamping the page to a minimum of 1 can prevent negative offsets.- offset: (qParams.page - 1) * resultsPerPage, + offset: (Math.max(1, qParams.page) - 1) * resultsPerPage,
53-53
: Styling adjustment.
The flex layout updates appear intentional and maintain responsiveness.
108-108
: Pagination usage is correct.
Passingdata?.count ?? 0
to the<Pagination>
component aligns with the updated query approach.
@rithviknishad @Jacobjeevan Backend PR got merged can review this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's adjust the style. Right now the select looks like it's disabled (adjust the bg, maybe add a border so it looks similar to the search input).

Also adjust the trigger width in location search (you can look into shadcn's documentation and other examples of popover in the codebase on how to do this). Make sure to test the other components where LocationSearch is called.
@Rishith25 when working on elements commonly used across the platform, ensure UI consistency can you share the ETD to get this done |
Hey @nihal467 Should i add location_id as search filed ?? |
@Rishith25 could you rethink the design? 🤔 i don't think this looks good enough to be merged; |
@rithviknishad This is the design given by @vivekrajuv18. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Location/LocationSearch.tsx (1)
68-82
: Consider adding an accessibility label for the clear button.
InvokingonSelect(null)
to clear the selection is correct, and usingstopPropagation
prevents unintended parent event triggers. However, adding anaria-label
(e.g.,aria-label="Clear location"
) to the button would improve accessibility for screen readers.<Button onClick={(e) => { e.stopPropagation(); onSelect(null); }} + aria-label="Clear location" variant="ghost" className="hover:bg-transparent" > <CareIcon icon="l-times" className="h-4 w-4" /> </Button>src/pages/Facility/settings/devices/DevicesList.tsx (1)
62-84
: Dropdown approach for switching search modes.
Toggling between "name" and "location" effectively zeroes out the other field inupdateQuery
. This is straightforward, though it discards the user’s prior input. If that’s intended, it’s fine. Otherwise, consider caching text inputs separately for a smoother user experience when switching back and forth.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Location/LocationSearch.tsx
(4 hunks)src/pages/Facility/settings/devices/DevicesList.tsx
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
🔇 Additional comments (17)
src/components/Location/LocationSearch.tsx (4)
3-9
: Additional imports look good.
These newly added imports foruseTranslation
,cn
,CareIcon
, andButton
are well-structured and serve the updated functionality cleanly.
31-35
: FlexibleonSelect
signature and newclassName
prop.
Allowingnull
in theonSelect
callback properly supports a “clear selected location” action. The addedclassName
prop is beneficial for layout customization.
43-45
: Correct integration of i18n.
UsinguseTranslation
with thet
function is consistent and ensures all user-facing text can be localized.
61-64
: Use ofcn
utility for class management.
This is a good approach to handle conditional classes and keep the code expressive and maintainable.src/pages/Facility/settings/devices/DevicesList.tsx (13)
10-17
: Good choice of UI components.
ImportingInput
andSelect
from theui
library helps maintain a consistent design system.
21-21
: Integration ofLocationSearch
.
Bringing in theLocationSearch
component is aligned with the new location-based search requirement.
23-23
: Custom hook usage is appreciated.
useFilters
is an elegant way to manage query parameters, improving code modularity.
28-28
: Location type import.
ImportingLocationList
ensures strong typing for theselectedLocation
state and related logic.
36-39
: Well-defined states for search.
SeparatingsearchType
andselectedLocation
is logical, supporting either a name or location-based query.
41-44
:useFilters
configuration looks solid.
Providing a cache blacklist forsearch_text
andcurrent_location
prevents stale query states.
47-47
: EnhancedqueryKey
usage.
IncludingqParams
andresultsPerPage
in the query key ensures consistent caching and re-fetch behavior.
51-55
: Refined query parameters.
Passingsearch_text
,current_location
, andsearch_type
to the API is clear. Resetting offset to(page - 1) * resultsPerPage
is typical for pagination.
86-113
: Conditional rendering betweenInput
andLocationSearch
.
This is a clean approach to adapt the form to the current search type. Good job resetting the complementary field inupdateQuery
to prevent conflicting search params.
114-121
: "Add Device" button logic.
Linking to the device creation page is consistent with the usual workflow. No issues here.
130-130
: Responsive grid layout.
Updating the className ensures the devices are presented in a crisp, grid-based approach.
136-137
: User feedback on empty device list.
Adding this fallbackCard
with a clear message helps inform users that no devices are available.
143-143
:Pagination
usage is well-integrated.
PassingtotalCount
from the fetched data ensures correct pagination, with minimal overhead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/Location/LocationSearch.tsx (1)
70-81
: Consider improving button accessibilityThe clear button implementation looks good, but consider adding an aria-label to improve accessibility.
<Button onClick={(e) => { e.stopPropagation(); onSelect(null); }} variant="ghost" className="hover:bg-transparent" + aria-label="Clear location selection" > <CareIcon icon="l-times" className="h-4 w-4" /> </Button>src/pages/Facility/settings/devices/DevicesList.tsx (2)
87-115
: Consider extracting common styling to reduce duplicationThe search input and location search components have similar styling. Consider extracting common styles to reduce duplication.
// At the top of the component + const searchInputClass = "w-full border border-gray-300 rounded-lg px-3 py-2 text-sm sm:text-base shadow-sm focus:ring-2 focus:ring-primary focus:border-primary rounded-l-none"; // Then in the render <Input placeholder={t("search_by_name")} value={qParams.search_text || ""} onChange={(e) => { updateQuery({ search_text: e.target.value, current_location: "", }); setSelectedLocation(null); }} - className="w-full h-9 border border-gray-300 rounded-lg px-3 py-2 text-sm sm:text-base shadow-sm focus:ring-2 focus:ring-primary focus:border-primary rounded-l-none" + className={`h-9 ${searchInputClass}`} /> // And <LocationSearch facilityId={facilityId} onSelect={(location: LocationList | null) => { updateQuery({ current_location: location?.id || "", search_text: "", }); setSelectedLocation(location); }} value={selectedLocation} - className="w-full border border-gray-300 rounded-lg px-3 py-2 text-sm sm:text-base shadow-sm focus:ring-2 focus:ring-primary focus:border-primary rounded-l-none" + className={searchInputClass} />
138-138
: Make existence check more explicitConsider making the array existence check more explicit for better readability.
- {data?.results?.length ? ( + {data?.results?.length > 0 ? (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Location/LocationSearch.tsx
(4 hunks)src/pages/Facility/settings/devices/DevicesList.tsx
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (10)
src/components/Location/LocationSearch.tsx (5)
5-9
: Great addition of clear button functionality!The imports for
cn
,CareIcon
, andButton
support the new clear button feature, improving usability by allowing users to reset their location selection.
31-34
: Good type definition updatesUpdating the
onSelect
function to acceptnull
enables the clear functionality, and adding the optionalclassName
prop improves component reusability.
60-63
: Improved styling with cn utilityUsing the
cn
utility for class composition is a good practice, allowing flexible styling and component customization while maintaining readability.
84-84
: Good PopoverContent width constraintsSetting min and max widths on the PopoverContent improves the user experience by ensuring the dropdown is neither too narrow nor too wide.
92-92
: Proper internationalization for empty state messageUsing the translation function for the "No locations found" message is a good practice for maintaining consistent internationalization across the application.
src/pages/Facility/settings/devices/DevicesList.tsx (5)
36-44
: Good implementation of search type and location filteringThe state management for search type and location filtering is well-implemented. Using
useFilters
hook with appropriate caching configuration addresses the previous feedback about page reset when searching.
47-58
: Comprehensive query parameters for search functionalityThe query parameters now correctly include search text, location, and search type, enabling the backend to filter results appropriately. Including these parameters in the query key ensures proper cache invalidation when search criteria change.
69-86
: Well-implemented search type selectorThe search type selector is well-implemented with proper state management. When switching between search types, it correctly clears the other search parameter and resets the location selection.
132-151
: Improved layout and paginationThe grid layout provides a good responsive experience, and the simplified pagination logic using the
Pagination
component fromuseFilters
is clean and maintainable.
104-110
:❓ Verification inconclusive
Verify API behavior with empty location
Ensure the backend API correctly handles an empty string for
current_location
when clearing the location selection.
🏁 Script executed:
#!/bin/bash # Check how the API handles empty location parameter rg "current_location.*=.*\"\"" -g "*.ts*" -g "*.js*" -A 5 -B 5Length of output: 62
Manual Verification Required: Confirm API Handling for Empty
current_location
The code in
src/pages/Facility/settings/devices/DevicesList.tsx
now sends an empty string (""
) forcurrent_location
when no location is selected. Our automated search for references confirming the backend's handling of this empty value did not return any conclusive evidence.Please ensure that:
- The backend API correctly interprets an empty string for
current_location
(e.g., treats it as a clear or unset state).- There is either an automated test or manual confirmation verifying that the API does not return errors or unexpected behavior when receiving an empty
current_location
.Current snippet for review:
onSelect={(location: LocationList | null) => { updateQuery({ current_location: location?.id || "", search_text: "", }); setSelectedLocation(location); }}Kindly perform manual verification of the API behavior if automated tests are lacking.
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Screenshots
Merge Checklist
Summary by CodeRabbit
New Features
UI Improvements