Fix Auto-Scroll Issue and Total Count Card in facility details page #9275
Conversation
WalkthroughThe changes in this pull request introduce enhancements to the pagination and data handling in the application. A new interface Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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: 4
🧹 Outside diff range and nitpick comments (3)
src/components/Common/Pagination.tsx (1)
86-92: Improve scroll behavior implementationThe current implementation has a few concerns:
- It depends on a specific element with ID "pages" which might not exist
- The scroll behavior could be more flexible
Consider these improvements:
- const pageContainer = window.document.getElementById("pages"); - if (ScrollToTop) { - pageContainer?.scroll({ - top: 0, - left: 0, - behavior: "smooth", - }); - } + if (scrollToTop) { + window.scrollTo({ + top: 0, + behavior: "smooth" + }); + }src/components/Facility/FacilityStaffList.tsx (2)
36-36: Add error handling for missing total_doctorsConsider adding error handling for cases where total_doctors is undefined:
- setTotalDoctors(data?.total_doctors ?? 0); + const total = data?.total_doctors; + if (typeof total !== 'number') { + console.warn('Expected total_doctors to be a number'); + } + setTotalDoctors(total ?? 0);
94-97: Consider using data-testid for testingInstead of using id for testing, consider using data-testid attribute which is more maintainable:
<div className="mt-5 rounded bg-white p-3 shadow-sm md:p-6" - id="staff_capacity" + id="staff_capacity" + data-testid="staff-capacity-section" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/Utils/request/types.ts(1 hunks)src/components/Common/Pagination.tsx(2 hunks)src/components/Facility/FacilityStaffList.tsx(3 hunks)
src/Utils/request/types.ts
Outdated
| @@ -41,4 +41,5 @@ export interface PaginatedResponse<TItem> { | |||
| next: string | null; | |||
| previous: string | null; | |||
| results: TItem[]; | |||
| total_doctors: number; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving domain-specific field to a separate interface
Adding total_doctors to the generic PaginatedResponse interface violates the Interface Segregation Principle, as not all paginated responses will need this field. Consider creating a specific interface for doctor-related responses:
interface DoctorPaginatedResponse<T> extends PaginatedResponse<T> {
total_doctors: number;
}
src/components/Common/Pagination.tsx
Outdated
| @@ -12,13 +12,15 @@ interface PaginationProps { | |||
| defaultPerPage: number; | |||
| cPage: number; | |||
| className?: string; | |||
| ScrollToTop?: boolean; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Follow TypeScript naming conventions
The prop name ScrollToTop should follow camelCase convention as per TypeScript standards.
- ScrollToTop?: boolean;
+ scrollToTop?: boolean;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ScrollToTop?: boolean; | |
| scrollToTop?: boolean; |
| ScrollToTop={false} | ||
| onChange={(page: number) => handlePageChange(page)} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistent prop naming
The ScrollToTop prop should match the casing used in the Pagination component:
- ScrollToTop={false}
+ scrollToTop={false}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ScrollToTop={false} | |
| onChange={(page: number) => handlePageChange(page)} | |
| scrollToTop={false} | |
| onChange={(page: number) => handlePageChange(page)} |
| const handlePageChange = (page: number) => { | ||
| updatePage(page); | ||
| const staffCapacityElement = document.getElementById("staff_capacity"); | ||
| if (staffCapacityElement) { | ||
| staffCapacityElement.scrollIntoView({ behavior: "smooth" }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check before scrollIntoView
The current implementation might throw an error if the element is not found. Consider adding a null check:
const handlePageChange = (page: number) => {
updatePage(page);
const staffCapacityElement = document.getElementById("staff_capacity");
- if (staffCapacityElement) {
- staffCapacityElement.scrollIntoView({ behavior: "smooth" });
- }
+ staffCapacityElement?.scrollIntoView({ behavior: "smooth" });
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handlePageChange = (page: number) => { | |
| updatePage(page); | |
| const staffCapacityElement = document.getElementById("staff_capacity"); | |
| if (staffCapacityElement) { | |
| staffCapacityElement.scrollIntoView({ behavior: "smooth" }); | |
| } | |
| }; | |
| const handlePageChange = (page: number) => { | |
| updatePage(page); | |
| const staffCapacityElement = document.getElementById("staff_capacity"); | |
| staffCapacityElement?.scrollIntoView({ behavior: "smooth" }); | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/components/Facility/FacilityStaffList.tsx (1)
41-45: Consider separating scroll behavior from page update logicWhile the implementation is correct and fixes the auto-scroll issue, consider separating the scroll behavior into its own function for better maintainability:
+ const scrollToStaffCapacity = () => { + const staffCapacityElement = document.getElementById("staff_capacity"); + staffCapacityElement?.scrollIntoView({ behavior: "smooth" }); + }; const handlePageChange = (page: number) => { updatePage(page); - const staffCapacityElement = document.getElementById("staff_capacity"); - staffCapacityElement?.scrollIntoView({ behavior: "smooth" }); + scrollToStaffCapacity(); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/Utils/request/api.tsx(2 hunks)src/Utils/request/types.ts(1 hunks)src/components/Common/Pagination.tsx(2 hunks)src/components/Facility/FacilityStaffList.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Common/Pagination.tsx
- src/Utils/request/types.ts
🔇 Additional comments (6)
src/Utils/request/api.tsx (2)
92-95: LGTM: Clean import organization
The addition of DoctorPaginatedResponse alongside PaginatedResponse is well-organized and maintains clean import structure.
677-677: LGTM: Type enhancement improves API response handling
The change from PaginatedResponse<DoctorModal> to DoctorPaginatedResponse<DoctorModal> aligns with the PR objective to utilize total_doctors for displaying overall staff capacity, which should help eliminate unnecessary API calls.
Let's verify the interface implementation:
✅ Verification successful
LGTM: Type enhancement correctly implements total_doctors field
The verification confirms that DoctorPaginatedResponse is properly defined in src/Utils/request/types.ts as an interface that extends PaginatedResponse<T> with the additional total_doctors field. The implementation aligns with the PR's objective to utilize total_doctors for staff capacity display.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify DoctorPaginatedResponse interface implementation
# Expected: Should find the interface definition with total_doctors property
# Search for DoctorPaginatedResponse interface definition
ast-grep --pattern 'interface DoctorPaginatedResponse<$_> {
$$$
total_doctors: number;
$$$
}'
Length of output: 101
Script:
#!/bin/bash
# Let's try different approaches to find the DoctorPaginatedResponse type/interface
# Search for any file containing DoctorPaginatedResponse
rg "DoctorPaginatedResponse" -A 5
# Also search for type definition as it might be a type alias
ast-grep --pattern 'type DoctorPaginatedResponse<$_> = $_'
# Search for PaginatedResponse as it might be related
rg "PaginatedResponse" -A 5
Length of output: 28835
src/components/Facility/FacilityStaffList.tsx (4)
36-36: LGTM: Improved total doctors calculation
The change simplifies the logic by directly using total_doctors from the API response, eliminating unnecessary iteration and fixing the total count display issue.
92-95: LGTM: Added necessary id for scroll functionality
The added id attribute enables the smooth scroll functionality and follows the existing naming convention.
134-135: LGTM: Pagination configuration properly handles scroll behavior
The changes correctly configure the Pagination component to prevent automatic scrolling to top and use the new page change handler.
36-36: Verify the fixes for all pagination scenarios
The implementation addresses both the auto-scroll and total count issues. Please verify:
- Scrolling behavior works correctly when navigating between all pages (not just page 1 to 2)
- Total doctor count remains accurate after pagination
Also applies to: 41-45, 134-135
✅ Verification successful
Verification completed for pagination and scroll behavior
The implementation has been properly handled:
- The component uses smooth scrolling to the staff capacity section when changing pages:
scrollIntoView({ behavior: "smooth" }) - The total doctors count is correctly updated from the API response:
setTotalDoctors(data?.total_doctors ?? 0) - Pagination is implemented with
scrollToTop={false}to prevent unwanted page jumps - The component is properly integrated in
FacilityHome.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other scroll-related code that might interfere
rg -i 'scroll(to|into)' --type ts --type tsx
# Check for other usages of total_doctors to ensure consistency
ast-grep --pattern 'total_doctors'
Length of output: 109
Script:
#!/bin/bash
# Search for scroll-related code with proper file extensions
rg -i 'scroll(to|into)' -t ts -t tsx -t jsx -t js
# Check for total_doctors usage in TypeScript/JavaScript files
rg "total_doctors" -t ts -t tsx -t jsx -t js
# Find all TypeScript/JavaScript files in the Facility component directory
fd -e ts -e tsx -e js -e jsx . src/components/Facility
Length of output: 5873
Script:
#!/bin/bash
# Try with different ripgrep options for scroll-related code
rg -i 'scroll' --type=typescript --type=javascript
# Search for total_doctors in FacilityStaffList.tsx specifically
rg "total_doctors" src/components/Facility/FacilityStaffList.tsx
# Look for pagination-related code in FacilityStaffList.tsx
rg "pagination|setPage|currentPage" src/components/Facility/FacilityStaffList.tsx
Length of output: 285
Script:
#!/bin/bash
# Search with basic file extensions
rg -i 'scroll' -g '*.ts' -g '*.tsx' -g '*.js' -g '*.jsx'
# Look for pagination and page-related state in FacilityStaffList.tsx
cat src/components/Facility/FacilityStaffList.tsx
# Search for any references to FacilityStaffList component
rg "FacilityStaffList" -g '*.ts' -g '*.tsx' -g '*.js' -g '*.jsx'
Length of output: 23303
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/FacilityStaffList.tsx (1)
92-95: Consider adding aria-label for accessibilityWhile the id is correctly added for scroll functionality, consider enhancing accessibility.
<div className="mt-5 rounded bg-white p-3 shadow-sm md:p-6" id="staff_capacity" + aria-label="Staff Capacity Section" >src/components/Common/Pagination.tsx (1)
86-92: Consider using a more specific element selectorWhile the scroll functionality works, using ID 'pages' might be too generic. Consider using a more specific ID or data attribute.
-const pageContainer = window.document.getElementById("pages"); +const pageContainer = window.document.getElementById("pagination-container");src/Utils/request/api.tsx (1)
92-95: Clean implementation for staff capacity improvements.The changes are minimal, focused, and well-structured:
- New type import for specialized doctor pagination
- Updated return type for the listDoctor endpoint
These changes effectively support the PR objective of fixing the total count card in the facility details page.Consider documenting the
DoctorPaginatedResponsetype usage in the API documentation to help other developers understand when to use this specialized pagination type versus the generic one.Also applies to: 677-677
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/Utils/request/api.tsx(2 hunks)src/Utils/request/types.ts(1 hunks)src/components/Common/Pagination.tsx(2 hunks)src/components/Facility/FacilityStaffList.tsx(3 hunks)
🔇 Additional comments (7)
src/Utils/request/types.ts (1)
46-48: LGTM! Well-structured interface extension
The new DoctorPaginatedResponse interface properly extends PaginatedResponse to include the domain-specific total_doctors field, following the Interface Segregation Principle.
src/components/Facility/FacilityStaffList.tsx (3)
36-36: LGTM! Simplified total doctors calculation
Using total_doctors from the API response is more efficient than calculating it client-side. The nullish coalescing operator provides a safe fallback.
41-45: LGTM! Well-implemented scroll behavior
The handlePageChange function properly implements the PR objective of controlling scroll behavior. The use of optional chaining (?.) provides null safety.
134-135: LGTM! Proper pagination configuration
The pagination is correctly configured with scrollToTop={false} and the new handlePageChange function, implementing the PR objective of controlling scroll behavior.
src/components/Common/Pagination.tsx (1)
15-15: LGTM! Well-typed optional prop
The scrollToTop prop is properly typed as an optional boolean, following TypeScript conventions.
src/Utils/request/api.tsx (2)
92-95: LGTM! Clean import addition.
The new import follows the existing pattern and is properly grouped with related pagination types.
677-677: Verify DoctorPaginatedResponse implementation.
The type change aligns with the PR objectives to utilize total_doctors for staff capacity display. However, let's verify the implementation.
✅ Verification successful
Type implementation verified successfully
The DoctorPaginatedResponse type is correctly implemented in src/Utils/request/types.ts, extending PaginatedResponse and adding the total_doctors field. The type change in the listDoctor endpoint is appropriate and consistent with the codebase's type system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify DoctorPaginatedResponse type implementation
# Expected: DoctorPaginatedResponse should extend PaginatedResponse and add total_doctors
# Search for the type definition
ast-grep --pattern 'interface DoctorPaginatedResponse<$_> extends PaginatedResponse<$_> {
$$$
}'
# Verify usage in components
rg -t tsx -t ts "DoctorPaginatedResponse"
Length of output: 362
Script:
#!/bin/bash
# Let's try a different approach to verify the implementation and usage
# Search for DoctorPaginatedResponse definition and usage with ripgrep
rg "DoctorPaginatedResponse" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' -t tsx -t ts
# Also verify PaginatedResponse to ensure it's properly defined
rg "PaginatedResponse" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' -t tsx -t ts
Length of output: 7388
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/components/Facility/models.tsx (1)
130-130: LGTM! Consider adding JSDoc comments.The addition of the optional
total_doctorsfield aligns with the PR objective to utilize it for displaying the overall staff capacity. This change will help eliminate unnecessary API calls.Consider adding JSDoc comments to document the purpose of each field in the
DoctorModalinterface:export interface DoctorModal { + /** Unique identifier for the doctor record */ id?: number; + /** Area or department where the doctor works */ area?: number; + /** Number of doctors in the current page */ count?: number; + /** Total number of doctors across all pages */ total_doctors?: number; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/Facility/FacilityStaffList.tsx(3 hunks)src/components/Facility/models.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Facility/FacilityStaffList.tsx
Proposed Changes
total_doctorsin the API response to show overall staff capacity and avoid extra API calls.total_staff_scroll.mp4
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
total_doctorsin the paginated response, providing additional information on the total number of doctors.scrollToTopproperty to the pagination component, allowing users to control scrolling behavior when navigating pages.handlePageChangemethod for improved user experience.Bug Fixes