Add pagination and search to OrganizationUsers #9967
Conversation
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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. |
Jacobjeevan
added
needs testing
needs review
P1
|
the pagination is not working, even though query params are being modified, the things are not changing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/pages/Organization/OrganizationUsers.tsx (3)
76-87: Remove unnecessary type castingThe type casting of
e.target.valueto string is redundant as it's already a string.- search: e.target.value as string, + search: e.target.value,
90-102: Align skeleton loader count with page sizeThe skeleton loader shows 3 items while the page size is set to 18. This creates an inconsistent loading experience.
- Array.from({ length: 3 }).map((_, i) => ( + Array.from({ length: qParams.limit }).map((_, i) => (
181-181: Add loading state for paginationThe Pagination component should show a loading state during page transitions to provide better user feedback.
- <Pagination totalCount={users?.count || 0} /> + <Pagination totalCount={users?.count || 0} loading={isLoadingUsers} />
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/pages/Organization/OrganizationUsers.tsx (3)
78-89: Consider enhancing the search input with loading state and clear button.The search implementation could benefit from additional UX improvements:
- Show loading indicator during search
- Add a clear button when search has value
- Add debouncing to prevent excessive API calls
<Input type="text" placeholder="Search users..." value={qParams.search || ""} onChange={(e) => updateQuery({ search: e.target.value as string, }) } - className="max-w-sm" + className="max-w-sm relative" + disabled={isLoadingUsers} + rightIcon={ + qParams.search && ( + <Button + variant="ghost" + size="sm" + className="absolute right-2 top-1/2 -translate-y-1/2" + onClick={() => updateQuery({ search: "" })} + > + <CareIcon icon="l-times" className="h-4 w-4" /> + </Button> + ) + } data-cy="search-user" />
92-104: Optimize skeleton loading performance.Consider memoizing the skeleton loader array to prevent unnecessary re-renders:
+const SKELETON_CARDS = Array.from({ length: 3 }).map((_, i) => ( + <Card key={i}> + <CardContent className="p-6"> + <div className="flex items-center space-x-4"> + <div className="h-12 w-12 rounded-full bg-gray-200 animate-pulse" /> + <div className="space-y-2 flex-1"> + <div className="h-4 bg-gray-200 rounded animate-pulse w-1/4" /> + <div className="h-4 bg-gray-200 rounded animate-pulse w-1/3" /> + </div> + </div> + </CardContent> + </Card> +)); -Array.from({ length: 3 }).map((_, i) => ( - <Card key={i}> - // ... skeleton content - </Card> -)) +{SKELETON_CARDS}
114-179: Enhance accessibility of user cards.Consider adding ARIA labels and roles to improve accessibility:
-<Card key={userRole.id} className="h-full"> +<Card + key={userRole.id} + className="h-full" + role="article" + aria-label={`User card for ${userRole.user.first_name} ${userRole.user.last_name}`} +> <CardContent className="p-4 sm:p-6"> // ... card content </CardContent> </Card>Also consider adding keyboard navigation support for the "More Details" button:
<Button variant="secondary" size="sm" className="w-full gap-2" + aria-label={`View more details for ${userRole.user.first_name} ${userRole.user.last_name}`} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Organization/OrganizationUsers.tsx(3 hunks)
🔇 Additional comments (1)
src/pages/Organization/OrganizationUsers.tsx (1)
183-183: Verify pagination behavior with large datasets.The pagination implementation looks correct. However, given the reported issue about pagination not working correctly, please verify:
- The page size matches the API's expectations
- The total count is accurately reflected in the UI
- Navigation between pages works as expected with large datasets
✅ Verification successful
Pagination implementation is correctly implemented and standardized across the application.
The verification shows that:
- Page size is consistently set to 14 items
- Offset calculation is correct:
(page - 1) * resultsPerPage- Total count is properly handled and reflected in the UI
- Implementation is standardized across all list views
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check pagination implementation across the codebase # Check for pagination limit constants or configurations rg -l "resultsPerPage|pageSize|itemsPerPage" --type ts # Check other components using the same pagination pattern ast-grep --pattern 'const { Pagination, resultsPerPage } = useFilters'Length of output: 411
Script:
#!/bin/bash # Check useFilters implementation and Pagination component usage # Examine useFilters hook implementation echo "=== useFilters.tsx content ===" cat src/hooks/useFilters.tsx # Look for Pagination component definition echo -e "\n=== Pagination component usage patterns ===" ast-grep --pattern 'const Pagination = ({ totalCount }: { totalCount: number }) => {' # Check how resultsPerPage is used in components echo -e "\n=== resultsPerPage usage in components ===" rg "resultsPerPage" -B 2 -A 2 --type tsLength of output: 14562
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/pages/Organization/OrganizationUsers.tsx (2)
85-87: Remove unnecessary type cast.The type cast to string is redundant as
e.target.valueis already of type string.- search: e.target.value as string, + search: e.target.value,
93-106: Add ARIA attributes for better accessibility.The skeleton loader should indicate its loading state to screen readers.
- Array.from({ length: 3 }).map((_, i) => ( + Array.from({ length: 3 }).map((_, i) => ( - <Card key={i}> + <Card key={i} aria-busy="true" aria-label="Loading user card">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Organization/OrganizationUsers.tsx(3 hunks)
🔇 Additional comments (4)
src/pages/Organization/OrganizationUsers.tsx (4)
29-45: LGTM! Pagination and search parameters are properly implemented.The implementation correctly:
- Includes pagination parameters in the query
- Calculates proper offset based on page number
- Uses debounced query for search
- Blacklists search from cache persistence
63-74: LGTM! Sheet state management through URL parameters.Good use of URL parameters for managing sheet visibility, enabling shareable URLs and browser navigation.
185-185: LGTM! Pagination implementation is complete.The pagination component correctly uses the total count from the API response with proper fallback for undefined state.
38-45: Verify that pagination is now working as expected.The previous issue where "pagination itself is not functioning correctly" should be resolved as the implementation now includes all necessary parameters. Please verify that:
- The page changes when using pagination controls
- The correct items are displayed for each page
- The search functionality works in combination with pagination
|
LGTM |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/pages/Organization/OrganizationUsers.tsx (2)
80-92: Remove unnecessary type assertion.The type assertion
as stringis redundant since the event target value is already a string.- search: e.target.value as string, + search: e.target.value,
109-184: Consider restructuring nested conditionals for better readability.The current implementation uses nested ternary operators which can be hard to maintain. Consider extracting the user list rendering into a separate component or using early returns.
Example restructure:
const UserList = ({ users }) => { if (users?.results?.length === 0) { return ( <Card className="col-span-full"> <CardContent className="p-6 text-center text-gray-500"> {t("no_users_found")} </CardContent> </Card> ); } return users?.results?.map((userRole) => ( // ... existing user card implementation )); }; // In the main render <div className="grid grid-cols-1 md:grid-cols-2 xl:grid-cols-3 gap-4"> <UserList users={users} /> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pages/Organization/OrganizationPatients.tsx(1 hunks)src/pages/Organization/OrganizationUsers.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (5)
src/pages/Organization/OrganizationPatients.tsx (1)
32-32: LGTM! Consistent pagination limit across components.The change aligns the pagination limit with OrganizationUsers component, maintaining consistency in the UI.
src/pages/Organization/OrganizationUsers.tsx (4)
29-32: LGTM! Proper hook configuration for search and pagination.The useFilters hook is correctly configured with appropriate limit and cache blacklist for search parameter.
94-107: LGTM! Clean loading state implementation.The loading skeleton provides good visual feedback with appropriate animations and placeholder content.
39-46: LGTM! Pagination implementation complete and functional.The pagination is properly implemented with:
- Correct query parameters (limit, offset)
- Query key includes pagination state
- Pagination component receives accurate total count
Also applies to: 186-186
29-46: Verify the completeness of pagination and search implementation.Let's verify that all necessary changes for pagination and search are in place.
✅ Verification successful
Pagination and search implementation verified successfully
The implementation correctly handles pagination and search functionality with proper API integration, debouncing, and cache invalidation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify pagination and search implementation # Check if pagination and search parameters are consistently handled echo "Checking pagination and search parameter handling..." rg -l "useFilters|qParams.search|qParams.page" src/pages/Organization/ # Verify if there are any remaining useQueryParams instances that should be migrated echo "Checking for remaining useQueryParams instances..." rg "useQueryParams" src/pages/Organization/ # Check for proper query parameter usage in API calls echo "Checking API query parameter usage..." rg "queryParams.*username|queryParams.*limit|queryParams.*offset" src/pages/Organization/Length of output: 775
Script:
#!/bin/bash # Check pagination and search implementation details # Check pagination implementation consistency echo "Checking pagination implementation..." ast-grep --pattern 'useFilters({$$$})' # Check API integration for pagination and search echo "Checking API integration..." ast-grep --pattern 'queryFn: $$$({ $$$ queryParams: { $$$ } })' # Check search handling echo "Checking search implementation..." rg -A 5 "qParams.search" src/pages/Organization/Length of output: 3754
|
@Jacobjeevan Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Improvements