fix(pagination): auto-reset page on organization change #10881
Conversation
WalkthroughThe changes introduce a Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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. |
|
|
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
|
@MethodManav Any updates? |
|
working on it and will try to finish today, @Jacobjeevan |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/pages/Organization/components/OrganizationLayout.tsx (1)
119-125: 💡 Verification agent🧩 Analysis chain
Potential issue with path matching logic.
Now that the paths include dynamic page numbers, the strict equality check
path === item.pathmight not work as expected for highlighting the active menu item.Consider updating this logic to use a more flexible comparison, such as checking if the current path starts with or includes a base path without the page number.
🏁 Script executed:
#!/bin/bash # Check if this logic is implemented elsewhere with a more flexible approach rg "path\s*===\s*item\.path" --type tsLength of output: 136
Action Required: Revise Path Matching for Dynamic Segments
The current strict equality check (
path === item.path) atsrc/pages/Organization/components/OrganizationLayout.tsx(lines 119-125) will likely fail to correctly highlight the active menu item when dynamic page numbers are part of the URL. Consider updating the logic—for example, by using a check likepath.startsWith(item.path)or a suitable regex—to account for dynamic segments.
- Location:
src/pages/Organization/components/OrganizationLayout.tsx, lines 119-125- Suggestion: Replace
path === item.pathwith a more flexible comparison that correctly handles dynamic paths.
🧹 Nitpick comments (1)
src/Routers/routes/OrganizationRoutes.tsx (1)
1-68: Consider adding more robust page parameter validation.The current implementation converts the page parameter using
Number(page || 1), which handles null/undefined cases but doesn't validate against negative or non-integer values.Consider adding validation to ensure the page is a positive integer, possibly with a utility function for consistency.
- page={Number(page || 1)} + page={Math.max(1, Math.floor(Number(page || 1)))}Or create a utility function:
function validatePageNumber(page: string | undefined): number { const pageNum = Number(page || 1); return Math.max(1, Math.floor(pageNum)); }Then use it consistently:
- page={Number(page || 1)} + page={validatePageNumber(page)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Routers/routes/OrganizationRoutes.tsx(1 hunks)src/pages/Organization/OrganizationFacilities.tsx(2 hunks)src/pages/Organization/OrganizationPatients.tsx(2 hunks)src/pages/Organization/OrganizationUsers.tsx(2 hunks)src/pages/Organization/OrganizationView.tsx(6 hunks)src/pages/Organization/components/OrganizationLayout.tsx(3 hunks)src/pages/UserDashboard.tsx(1 hunks)
🔇 Additional comments (24)
src/pages/UserDashboard.tsx (1)
129-129: Good implementation of default pagination.The change to add
/1to the organization link correctly implements the default pagination page, ensuring that when a user clicks on an organization from the dashboard, they'll always start at page 1. This addresses the core issue of resetting pagination on organization change.src/pages/Organization/OrganizationFacilities.tsx (3)
28-28: LGTM: Added pagination prop to interface.Adding the
pageproperty to the Props interface allows the component to receive pagination information.
34-34: LGTM: Updated function parameters.The function signature now correctly destructures the
pageprop.
61-65: LGTM: Page prop passed to OrganizationLayout.The page parameter is now correctly passed to the OrganizationLayout component, allowing the layout to handle pagination appropriately.
src/pages/Organization/OrganizationPatients.tsx (3)
29-29: LGTM: Added pagination prop to interface.Adding the
pageproperty to the Props interface allows the component to receive pagination information.
32-36: LGTM: Updated function parameters.The function signature now correctly destructures the
pageprop.
103-103: LGTM: Page prop passed to OrganizationLayout.The page parameter is now correctly passed to the OrganizationLayout component, which handles the pagination context.
src/pages/Organization/OrganizationUsers.tsx (3)
32-32: LGTM: Added pagination prop to interface.Adding the
pageproperty to the Props interface allows the component to receive pagination information.
35-39: LGTM: Updated function parameters.The function signature now correctly destructures the
pageprop.
110-114: LGTM: Page prop passed to OrganizationLayout.The page parameter is now correctly passed to the OrganizationLayout component, ensuring consistent pagination behavior.
src/pages/Organization/OrganizationView.tsx (8)
2-2: Nice update to include navigate from raviger!The import change is appropriate for implementing navigation to pages with updated URL patterns.
26-26: LGTM! Good addition of the page prop.Adding the page prop to the Props interface correctly implements the requirement to handle pagination through the component props.
29-33: LGTM! Proper component signature update.The component signature has been correctly updated to include the page prop, which aligns with the PR objective of handling pagination through props.
52-53: Simplifying the baseUrl logic.This change removes the conditional logic that was previously checking navOrganizationId but always resulting in the same value.
56-60: LGTM! Correctly passing page prop to OrganizationLayout.The OrganizationLayout component now properly receives the page prop, ensuring consistent pagination behavior across components.
115-115: LGTM! Updated link correctly includes page parameter.The view details link has been properly updated to include the page parameter, maintaining consistent URL structure.
147-147: Great implementation of pagination navigation!Using the navigate function to update the URL with the new page number is the correct approach for maintaining state in the URL, which addresses the core requirement of this PR.
75-77:❓ Verification inconclusive
Search no longer resets pagination - is this intentional?
The comment indicates the search should reset to the first page, but the implementation no longer does this. Consider whether this behavior change is intended.
🏁 Script executed:
#!/bin/bash # Check if there are other places where search is resetting pagination rg -B 2 -A 2 "setSearchQuery.*page.*1" --type tsLength of output: 50
I'll run a follow‐up verification to check if any code in the file resets the pagination (e.g. by calling something like
setPage(1)) when the search query changes. Please execute the following shell script:
🏁 Script executed:
#!/bin/bash rg -n "setPage\(1\)" src/pages/Organization/OrganizationView.tsxLength of output: 66
Attention: Confirm Pagination Reset Behavior
The code in
src/pages/Organization/OrganizationView.tsx(lines 75–77) updates the search query but no longer resets the pagination, even though the inline comment implies it should. There’s no evidence of a call likesetPage(1)anywhere in this file.Please verify whether the removal of the pagination reset is intentional. If resetting the pagination on a search change is still desired, consider adding the necessary logic (e.g., invoking
setPage(1)) or updating the comment to accurately reflect the current behavior.src/pages/Organization/components/OrganizationLayout.tsx (3)
26-26: LGTM! Good addition of the page prop.Adding the page prop to the Props interface ensures the layout component can properly handle pagination.
40-40: LGTM! Component signature updated correctly.The page prop is now properly included in the component parameters.
76-76: LGTM! Updated navigation paths to include page parameter.All navigation items have been updated to include the page parameter, ensuring consistent URL structure throughout the application.
Also applies to: 82-82, 88-88, 94-94
src/Routers/routes/OrganizationRoutes.tsx (3)
10-12: LGTM! Routes properly updated to include page parameter.The route definition has been correctly updated to include the page parameter. The fallback logic
Number(page || 1)ensures a valid page number is always provided.
13-21: LGTM! All organization routes properly updated.All organization routes have been correctly updated to include the page parameter, maintaining consistency across the application.
22-32: LGTM! Child organization routes correctly updated.The routes for child organizations have been properly updated to include the page parameter, ensuring consistent navigation throughout the application.
Also applies to: 33-43, 44-54, 55-64
|
@Jacobjeevan and @rithviknishad, I have completed the changes. Please review the updates and let me know if they work fine 🙂 |
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
Proposed Changes
Screen.Recording.2025-02-28.at.7.35.19.PM.mov
@ohcnetwork/care-fe-code-reviewers
Summary by CodeRabbit
New Features
pageparameter in the URL structure for organization-related views.OrganizationLayoutcomponent.Bug Fixes
UserDashboardto include the default page number for organization links.