Fixed : User Search Bar #9750
Fixed : User Search Bar #9750
Conversation
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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. |
![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 (1)
src/components/Facility/FacilityUsers.tsx (1)
40-49: Consider memoizing or debouncing heavy filtering logic.
While the current filtering solution is correct and easy to read, it will re-run every time this component re-renders. For large datasets, you may want to optimize by memoizing the filtered result or debouncing user input.+// Example of debouncing or using React.useMemo for 'filteredUsers': +// const filteredUsers = React.useMemo(() => { +// const searchString = searchTerm.toLowerCase(); +// return userListData.results.filter((user) => { +// return ( +// user.username?.toLowerCase().includes(searchString) || +// user.first_name?.toLowerCase().includes(searchString) || +// user.last_name?.toLowerCase().includes(searchString) +// ); +// }); +// }, [searchTerm, userListData.results]);
📜 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 (2)
src/components/Facility/FacilityUsers.tsx(3 hunks)src/components/Users/UserListAndCard.tsx(1 hunks)
🔇 Additional comments (7)
src/components/Facility/FacilityUsers.tsx (6)
17-17: Great usage of the filters hook.
No issues identified; the configuration for pagination and cache blacklist looks appropriate.
21-21: State initialization for searchTerm is straightforward.
This is well-implemented and keeps the component logic clean.
36-37: Internationalized “no users found” message.
Returning an i18n-based message is consistent with best practices for multilingual support.
50-50: No direct functional changes here.
No new or updated logic in this line.
52-52: Page component usage is consistent.
Implementation is in line with established project patterns for layout.
62-63: Efficient data flow from parent to child.
Passing the filtered list and controlling the search term state from the parent fosters a clean one-directional data flow. Consider adding a debounce if the user list is large.src/components/Users/UserListAndCard.tsx (1)
348-348: Good internationalization upgrade.
Replacing hardcoded text with{t("no_users_found")}aligns well with the i18n approach.
package-lock.json
Outdated
| @@ -146,8 +146,8 @@ | |||
| "node": ">=22.8.0" | |||
| }, | |||
| "optionalDependencies": { | |||
| "@esbuild/linux-arm64": "*", | |||
| const filteredUsers = searchTerm | ||
| ? userListData.results.filter((user) => { | ||
| const searchString = searchTerm.toLowerCase(); | ||
| return ( | ||
| user.username?.toLowerCase().includes(searchString) || | ||
| user.first_name?.toLowerCase().includes(searchString) || | ||
| user.last_name?.toLowerCase().includes(searchString) | ||
| ); | ||
| }) | ||
| : userListData.results; | ||
|
|
There was a problem hiding this comment.
Assume user page has 100 users, your api will only give you 1st 14, this search will only on that 14!!!
|
Doesn't solve the issue. |
|
You have to pass the search params to the backend and the search should happen at the backend. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/Facility/FacilityUsers.tsx (1)
33-40:⚠️ Potential issueCurrent implementation doesn't solve the core search issue
As mentioned in the PR comments by @bodhish, the current implementation only searches through the paginated results (default 18 users) instead of the entire user list. This is a critical limitation that needs to be addressed.
The search should be performed on the server-side across all users. Consider these changes:
- Remove client-side filtering
- Pass the search parameter to the API
- Let the backend handle the search across all users
- Return the filtered and paginated results
Example API endpoint structure:
interface SearchParams { facility_id: number; username?: string; // Search term limit: number; // Page size offset: number; // Pagination offset }
🧹 Nitpick comments (5)
src/components/Facility/FacilityUsers.tsx (5)
23-24: Search implementation needs architectural changesBased on the PR comments, the search functionality should be implemented on the server-side. The current approach of extracting the search parameter locally doesn't fully address this requirement.
Consider implementing these changes:
- Ensure the search query is passed to the backend API
- Let the backend handle the filtering of all users
- Return only the filtered results to the frontend
25-30: Add debouncing to search functionThe current implementation triggers an API call on every keystroke, which could lead to performance issues. Consider implementing debouncing to optimize the search functionality.
+ import { debounce } from 'lodash'; + + const debouncedSearch = debounce((key: string, value: string) => { + updateQuery({ + ...qParams, + [key]: value || undefined, + }); + }, 300); + const handleSearch = (key: string, value: string) => { - updateQuery({ - ...qParams, - [key]: value || undefined, - }); + debouncedSearch(key, value); };
33-40: Extract hardcoded limit valueThe limit value of 18 is duplicated in multiple places. Consider extracting it to a constant to follow the DRY principle.
+ const DEFAULT_PAGE_LIMIT = 18; + const { qParams, updateQuery, Pagination } = useFilters({ - limit: 18, + limit: DEFAULT_PAGE_LIMIT, cacheBlacklist: ["username"], }); // ... later in the code ... queryParams: { username, - limit: qParams.limit || 18, - offset: ((qParams.page || 1) - 1) * (qParams.limit || 18), + limit: qParams.limit || DEFAULT_PAGE_LIMIT, + offset: ((qParams.page || 1) - 1) * (qParams.limit || DEFAULT_PAGE_LIMIT), },
64-65: Improve type safety of search value handlingThe empty string fallback could be handled more explicitly to prevent potential undefined behavior.
- searchValue={username || ""} + searchValue={typeof username === 'string' ? username : ""}
70-72: Improve pagination condition clarityThe pagination condition uses a hardcoded limit and could be more explicit.
- {userListData.count > (qParams.limit || 18) && ( + const pageLimit = qParams.limit || DEFAULT_PAGE_LIMIT; + {userListData.count > pageLimit && ( <Pagination totalCount={userListData.count} /> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Facility/FacilityUsers.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
|
@bodhish I passed the search parameters to the backend, but I’m unable to filter. Could you please review my implementation if there’s anything I have missed? |
| @@ -48,13 +61,15 @@ export default function FacilityUsers(props: { facilityId: number }) { | |||
|
|
|||
| <UserListView | |||
| users={userListData?.results ?? []} | |||
| onSearch={(username) => updateQuery({ username })} | |||
| searchValue={qParams.username} | |||
| onSearch={(username) => handleSearch("username", username)} | |||
There was a problem hiding this comment.
No need to complicate this:
| onSearch={(username) => handleSearch("username", username)} | |
| onSearch={(username) => updateQuery({ username })} |
What we had earlier is enough.
| queryFn: query(routes.facility.getUsers, { | ||
| pathParams: { facility_id: facilityId }, | ||
| queryParams: { | ||
| username, | ||
| limit: qParams.limit || 18, |
There was a problem hiding this comment.
qParams.limit is enough, no need to add the or.
Same below.
There was a problem hiding this comment.
Also go ahead and remove the userListLoading check below (line 45); that way enter component isn't refreshing on search.
| activeTab={activeTab} | ||
| onTabChange={setActiveTab} | ||
| /> | ||
|
|
||
| <Pagination totalCount={userListData.count} /> | ||
| {userListData.count > (qParams.limit || 18) && ( |
There was a problem hiding this comment.
Pagination will auto handle this, no need for a check.
Jacobjeevan
added
waiting for backend
and removed
question
|
@Jacobjeevan Now It is working. searchbutton.mp4 |
Yes, the backend PR was merged 😃 |
| @@ -94,7 +103,7 @@ export default function FacilityUsers(props: { facilityId: number }) { | |||
| <UserListView | |||
| users={userListData?.results ?? []} | |||
| onSearch={(username) => updateQuery({ username })} | |||
| searchValue={qParams.username} | |||
| searchValue={username || ""} | |||
There was a problem hiding this comment.
let's not over complicate this, you can just use qParams.username, it will handle it.
| queryFn: query(routes.facility.getUsers, { | ||
| pathParams: { facility_id: facilityId }, | ||
| queryParams: { | ||
| username, |
There was a problem hiding this comment.
Likewise use qParams.username, no need to de-structure since there's only 2 uses anyways :)
Jacobjeevan
removed
question
Jeffrin2005
force-pushed
the
search-bar
branch
from
2bcebf3 to
2b37472
Compare
rithviknishad
requested review from
Jacobjeevan,
rithviknishad and
bodhish
and removed request for
rithviknishad
Jeffrin2005
force-pushed
the
search-bar
branch
from
2b37472 to
f60d1a2
Compare
| }), | ||
| enabled: !!facilityId, | ||
| }); | ||
|
|
||
| if (userListLoading) { | ||
| return <div>Loading...</div>; | ||
| return ( |
There was a problem hiding this comment.
Let's move this inside the return statement (compare how search functions in this page vs encounters page - we don't wanna re-render everything on page on search).
So, render this if it's loading if not render UserListView.
|
👋 Hi, @Jeffrin2005, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
searchbutton.mp4
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes