Cpride/pagination#1130
Merged
Merged
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR migrates pagination state from component-local React state to URL-synchronized state across the application. The ChangesPagination Refactor
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/components/Pagination.tsx`:
- Around line 19-20: The default prop instanceId = 'pagination' causes duplicate
id/htmlFor collisions; update the Pagination component to generate a unique
fallback when instanceId is not provided (e.g., use React's useId or a short
unique generator) and use that generated value wherever instanceId is referenced
(look for the instanceId prop usage and any id/htmlFor attributes in
Pagination). Ensure the prop signature (instanceId) remains optional but falls
back to the generated id, and apply the same fix to the other occurrences noted
around lines 76–80 where the hardcoded 'pagination' is used.
In `@frontend/src/hooks/useUrlPagination.ts`:
- Around line 9-16: In useUrlPagination, parsing page and perPage from
searchParams with parseInt can produce NaN or non-positive values; update the
logic that reads searchParams.get(pageKey)/get(perPageKey) so that after
parseInt you validate and normalize values: if parsed page is NaN or < 1 use
defaultPage, and if parsed perPage is NaN or <= 0 use defaultPerPage (optionally
clamp perPage to a sensible max); reference the variables searchParams, pageKey,
perPageKey, defaultPage and defaultPerPage and replace the direct parseInt usage
with this guarded normalization so pagination math always receives safe positive
integers.
- Around line 18-29: setPage and setPerPage build URLSearchParams from the
render-time searchParams which can cause race/stale overwrites; change both to
use the functional updater form of setSearchParams(prev => ...) so each call
builds a new URLSearchParams from the current prev value, then set the
pageKey/perPageKey as needed (setPerPage should also reset pageKey to '1'), and
pass the appropriate replace option to setSearchParams({ replace: ... }) so
updates always merge against the latest params rather than a stale snapshot.
In `@frontend/src/pages/ClassesPage.tsx`:
- Line 97: The mount effect in ClassesPage that unconditionally calls
setCurrentPage(1) is clobbering the URL-backed pagination provided by
useUrlPagination; remove or guard that effect so it does not run on initial
render. Locate the useEffect in ClassesPage that calls setCurrentPage(1) (and
any related setItemsPerPage) and either delete the effect entirely or wrap it so
it only runs on real dependency changes (e.g., track an isMounted ref or check
previous values) rather than on mount, ensuring useUrlPagination's currentPage
from the URL is preserved.
In `@frontend/src/pages/ProgramsPage.tsx`:
- Line 179: The page URL state isn't reset when search/sort/filter criteria
change, causing out-of-range empty results; in ProgramsPage add a useEffect that
calls setPage(1) whenever any query criteria change (e.g., searchTerm,
selectedFilters, sortBy, category, etc.) so the URL-backed page resets to the
first page; locate useUrlPagination usage (const { page, perPage, setPage,
setPerPage } = useUrlPagination(1, 10)) and add the effect referencing setPage
and the specific criterion state variables used elsewhere in this component
(also apply the same fix where similar pagination is used around lines 289-290).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3f40077e-ac33-4225-b14a-e4fdd74f9147
📒 Files selected for processing (16)
frontend/src/components/Pagination.tsxfrontend/src/components/shared/Pagination.tsxfrontend/src/components/shared/index.tsfrontend/src/components/student/ActivityHistoryCard.tsxfrontend/src/hooks/useUrlPagination.tsfrontend/src/pages/ClassesPage.tsxfrontend/src/pages/ProgramsPage.tsxfrontend/src/pages/admin/AdminManagement.tsxfrontend/src/pages/admin/FacilityManagement.tsxfrontend/src/pages/admin/ProviderUserManagement.tsxfrontend/src/pages/class-detail/AuditTab.tsxfrontend/src/pages/knowledge-center/KnowledgeCenterManagement.tsxfrontend/src/pages/knowledge-center/ResidentKnowledgeCenter.tsxfrontend/src/pages/programs/AddClassEnrollments.tsxfrontend/src/pages/programs/ClassEnrollmentDetails.tsxfrontend/src/pages/programs/ProgramOverviewFacilityAdmin.tsx
💤 Files with no reviewable changes (1)
- frontend/src/components/shared/Pagination.tsx
corypride
force-pushed
the
cpride/pagination
branch
from
5635518 to
707ea41
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/pages/knowledge-center/KnowledgeCenterManagement.tsx (1)
339-344: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding setter functions to the dependency array.
While React guarantees that
setCurrentPageandsetCategoryFilterare stable, the exhaustive-deps ESLint rule would flag these missing dependencies. Adding them improves code clarity and satisfies linting rules.📋 Suggested dependency array update
useEffect(() => { if (currentTab !== 'libraries') { setCategoryFilter('all'); } setCurrentPage(1); -}, [currentTab]); +}, [currentTab, setCategoryFilter, setCurrentPage]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/pages/knowledge-center/KnowledgeCenterManagement.tsx` around lines 339 - 344, The useEffect that resets category and page when currentTab changes should include the setter functions in its dependency array to satisfy exhaustive-deps and clarify intent: update the effect watching currentTab so it lists setCategoryFilter and setCurrentPage along with currentTab (i.e., useEffect(..., [currentTab, setCategoryFilter, setCurrentPage])), leaving the existing body that calls setCategoryFilter('all') when currentTab !== 'libraries' and setCurrentPage(1).
♻️ Duplicate comments (2)
frontend/src/hooks/useUrlPagination.ts (1)
15-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse functional
setSearchParamsupdates to avoid stale query overwrites.
setPage/setPerPageare still based on render-timesearchParams, so sequential updates can drop each other’s changes. This is the same stale-param issue previously seen with pagination URL state.Proposed fix
const setPage = (newPage: number, options?: { replace?: boolean }) => { - const params = new URLSearchParams(searchParams); - params.set(pageKey, newPage.toString()); - setSearchParams(params, { replace: options?.replace ?? false }); + setSearchParams((prev) => { + const params = new URLSearchParams(prev); + params.set(pageKey, newPage.toString()); + return params; + }, { replace: options?.replace ?? false }); }; const setPerPage = (newPerPage: number) => { - const params = new URLSearchParams(searchParams); - params.set(perPageKey, newPerPage.toString()); - params.set(pageKey, '1'); - setSearchParams(params, { replace: false }); + setSearchParams((prev) => { + const params = new URLSearchParams(prev); + params.set(perPageKey, newPerPage.toString()); + params.set(pageKey, '1'); + return params; + }, { replace: false }); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/hooks/useUrlPagination.ts` around lines 15 - 25, setPage and setPerPage currently read from the render-time searchParams which can cause stale overwrites; change both to use the functional form of setSearchParams so you operate on the latest params (e.g., setSearchParams(prev => { const params = new URLSearchParams(prev); params.set(pageKey, newPage.toString()); return params; }, { replace: options?.replace ?? false }) for setPage, and likewise for setPerPage set perPageKey and reset pageKey to '1' inside setSearchParams(prev => { ... }) with replace: false) so updates compose safely.frontend/src/pages/ClassesPage.tsx (1)
217-219:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the page-reset effect on initial mount.
Line 217 runs
setCurrentPage(1)on first render, which overwrites URL-backed pagination and breaks refresh/back navigation persistence. Add a first-render guard before resetting.Suggested fix
-import { useState, useMemo, useEffect } from 'react'; +import { useState, useMemo, useEffect, useRef } from 'react'; ... +const isFirstRender = useRef(true); ... useEffect(() => { + if (isFirstRender.current) { + isFirstRender.current = false; + return; + } setCurrentPage(1); -}, [searchQuery, todayOnly, attendanceConcerns, facilityFilter, programFilter, statusFilter]); +}, [searchQuery, todayOnly, attendanceConcerns, facilityFilter, programFilter, statusFilter, setCurrentPage]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/pages/ClassesPage.tsx` around lines 217 - 219, The effect that calls setCurrentPage(1) (inside the useEffect at top-level of ClassesPage) runs on initial mount and clobbers URL-backed pagination; add a first-render guard (e.g., a useRef isInitialMount or check if currentPage is undefined) so the effect only resets the page when searchQuery, todayOnly, attendanceConcerns, facilityFilter, programFilter, or statusFilter change after mount, not on the initial render; update the useEffect to bail out if isInitialMount is true (or if currentPage already exists) and then flip the ref to false so subsequent dependency changes will call setCurrentPage(1).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@frontend/src/pages/knowledge-center/KnowledgeCenterManagement.tsx`:
- Around line 339-344: The useEffect that resets category and page when
currentTab changes should include the setter functions in its dependency array
to satisfy exhaustive-deps and clarify intent: update the effect watching
currentTab so it lists setCategoryFilter and setCurrentPage along with
currentTab (i.e., useEffect(..., [currentTab, setCategoryFilter,
setCurrentPage])), leaving the existing body that calls setCategoryFilter('all')
when currentTab !== 'libraries' and setCurrentPage(1).
---
Duplicate comments:
In `@frontend/src/hooks/useUrlPagination.ts`:
- Around line 15-25: setPage and setPerPage currently read from the render-time
searchParams which can cause stale overwrites; change both to use the functional
form of setSearchParams so you operate on the latest params (e.g.,
setSearchParams(prev => { const params = new URLSearchParams(prev);
params.set(pageKey, newPage.toString()); return params; }, { replace:
options?.replace ?? false }) for setPage, and likewise for setPerPage set
perPageKey and reset pageKey to '1' inside setSearchParams(prev => { ... }) with
replace: false) so updates compose safely.
In `@frontend/src/pages/ClassesPage.tsx`:
- Around line 217-219: The effect that calls setCurrentPage(1) (inside the
useEffect at top-level of ClassesPage) runs on initial mount and clobbers
URL-backed pagination; add a first-render guard (e.g., a useRef isInitialMount
or check if currentPage is undefined) so the effect only resets the page when
searchQuery, todayOnly, attendanceConcerns, facilityFilter, programFilter, or
statusFilter change after mount, not on the initial render; update the useEffect
to bail out if isInitialMount is true (or if currentPage already exists) and
then flip the ref to false so subsequent dependency changes will call
setCurrentPage(1).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a40a979a-270d-4140-aee2-7da95777d671
📒 Files selected for processing (15)
frontend/src/components/Pagination.tsxfrontend/src/components/shared/Pagination.tsxfrontend/src/components/shared/index.tsfrontend/src/components/student/ActivityHistoryCard.tsxfrontend/src/hooks/useUrlPagination.tsfrontend/src/pages/ClassesPage.tsxfrontend/src/pages/ProgramsPage.tsxfrontend/src/pages/admin/AdminManagement.tsxfrontend/src/pages/admin/FacilityManagement.tsxfrontend/src/pages/class-detail/AuditTab.tsxfrontend/src/pages/knowledge-center/KnowledgeCenterManagement.tsxfrontend/src/pages/knowledge-center/ResidentKnowledgeCenter.tsxfrontend/src/pages/programs/AddClassEnrollments.tsxfrontend/src/pages/programs/ClassEnrollmentDetails.tsxfrontend/src/pages/programs/ProgramOverviewFacilityAdmin.tsx
💤 Files with no reviewable changes (1)
- frontend/src/components/shared/Pagination.tsx
carddev81
requested changes
May 15, 2026
carddev81
left a comment
carddev81
left a comment
Contributor
There was a problem hiding this comment.
When selecting to display more on the page it does not display what is selected. See page below, holding off on the review till this is corrected since this will reflect all the changes made.
To reproduce, just go to the residents listing and select 40 from the items to display
Contributor
Author
test.mp4@carddev81 Could not reproduce as you stated. Send me a video |
carddev81
approved these changes
May 18, 2026
carddev81
left a comment
carddev81
left a comment
Contributor
There was a problem hiding this comment.
@corypride I don\t understand why I am not getting that error I posted, but here are the only findings I could spot. Great Job.
Audit History (Please check all locations) - See video--After selecting to display more than 20 records at a time the pagination controls disappear.
2026-05-18.10-09-40.mp4
Linting errors:
- ClassesPage.tsx (Line 219): error is -> React Hook useEffect has a missing dependency: 'setCurrentPage'. Either include it or remove the dependency array
- KnowledgeCenterManagement.tsx (344): error is -> React Hook useEffect has a missing dependency: 'setCurrentPage'. Either include it or remove the dependency array.
Inconsistency
The ClassesPage.tsxk (line 50) imports Pagination from @/components/shared while every other location it is needed imports from @/components/Pagination. The re-export in shared/index.ts makes it work, but it's inconsistent. Either update ClassesPage or delete the re-export. I say delete the re-export because it's not even being used by any other page.
Please fix these issues and merge your code into the total refactor branch please.
Contributor
Author
|
🫡
*Cory Pride*
Associate Engineer
unlockedlabs.org
…On Mon, May 18, 2026 at 10:34 AM Richard Salas ***@***.***> wrote:
***@***.**** approved this pull request.
@corypride <https://github.com/corypride> I don\t understand why I am not
getting that error I posted, but here are the only findings I could spot.
Great Job.
Audit History (Please check all locations) - See video--After selecting to
display more than 20 records at a time the pagination controls disappear.
https://github.com/user-attachments/assets/f3bbaf09-4474-491d-b686-5033c75a8284
Linting errors:
1. ClassesPage.tsx (Line 219): error is -> React Hook useEffect has a
missing dependency: 'setCurrentPage'. Either include it or remove the
dependency array
2. KnowledgeCenterManagement.tsx (344): error is -> React Hook
useEffect has a missing dependency: 'setCurrentPage'. Either include it or
remove the dependency array.
Inconsistency
The ClassesPage.tsxk (line 50) imports Pagination from @/components/shared
while every other location it is needed imports from
@/components/Pagination. The re-export in shared/index.ts makes it work,
but it's inconsistent. Either update ClassesPage or delete the re-export. I
say delete the re-export because it's not even being used by any other page.
Please fix these issues and merge your code into the total refactor branch
please.
—
Reply to this email directly, view it on GitHub
<#1130?email_source=notifications&email_token=A43WLSGVATWUNYOBQSJYAGL43MUOXA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMZRGE2TKNJVHA2KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJPLQOJPXEZLWNFSXOX3ON52GSZTJMNQXI2LPNZZV6Y3MNFRWW#pullrequestreview-4311555584>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A43WLSHG4LYNUEZSW7R7BI343MUOXAVCNFSM6AAAAACY6HLAJWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DGMJRGU2TKNJYGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/pages/knowledge-center/KnowledgeCenterManagement.tsx (1)
339-344:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve URL page on initial mount.
Line 343 unconditionally calls
setCurrentPage(1), which overwrites?page=...on first render and breaks refresh/back pagination persistence.Suggested fix
+ const hasInitializedTabReset = useRef(false); + useEffect(() => { + if (!hasInitializedTabReset.current) { + hasInitializedTabReset.current = true; + return; + } if (currentTab !== 'libraries') { setCategoryFilter('all'); } setCurrentPage(1); }, [currentTab, setCurrentPage]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/pages/knowledge-center/KnowledgeCenterManagement.tsx` around lines 339 - 344, The effect is resetting page on first render and overwriting any ?page=... param; update the useEffect that reads currentTab to skip calling setCurrentPage(1) on initial mount by adding a mounted ref (e.g., isInitialMount using useRef) or tracking previous tab, and only call setCurrentPage(1) when currentTab changes after mount (but still always apply the category reset logic for setCategoryFilter('all')); tweak the existing useEffect (the one referencing currentTab, setCurrentPage, setCategoryFilter) to guard the setCurrentPage call with that isInitialMount check so URL page state is preserved on initial load.
♻️ Duplicate comments (2)
frontend/src/hooks/useUrlPagination.ts (1)
16-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse functional
setSearchParamsupdates in both setters.Line 18 and Line 27 clone closed-over
searchParams; sequential updates can still drop keys (the same class of bug this PR hit withper_page). Use functional updates so each write merges from current params.Proposed fix
const setPage = useCallback( (newPage: number, options?: { replace?: boolean }) => { - const params = new URLSearchParams(searchParams); - params.set(pageKey, newPage.toString()); - setSearchParams(params, { replace: options?.replace ?? false }); + setSearchParams((prev) => { + const params = new URLSearchParams(prev); + params.set(pageKey, newPage.toString()); + return params; + }, { replace: options?.replace ?? false }); }, - [searchParams, setSearchParams, pageKey] + [setSearchParams, pageKey] ); @@ const setPerPage = useCallback( (newPerPage: number) => { - const params = new URLSearchParams(searchParams); - params.set(perPageKey, newPerPage.toString()); - params.set(pageKey, '1'); - setSearchParams(params, { replace: false }); + setSearchParams((prev) => { + const params = new URLSearchParams(prev); + params.set(perPageKey, newPerPage.toString()); + params.set(pageKey, '1'); + return params; + }, { replace: false }); }, - [searchParams, setSearchParams, pageKey, perPageKey] + [setSearchParams, pageKey, perPageKey] );For react-router-dom v6.23.0 `useSearchParams`, confirm: 1) functional updates (`setSearchParams(prev => ...)`) are supported, and 2) multiple `setSearchParams` calls in the same tick are not queued like React state updates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/hooks/useUrlPagination.ts` around lines 16 - 33, The setters setPage and setPerPage close over the stale searchParams and create new URLSearchParams from that snapshot, which can drop concurrent query keys; change both to use the functional form of setSearchParams (setSearchParams(prev => { ... })) so each update builds from the latest params, and in setPerPage ensure you set perPageKey and reset pageKey inside that functional update; verify useSearchParams' setSearchParams supports functional updates before applying.frontend/src/pages/ClassesPage.tsx (1)
217-219:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent mount-time page reset from clobbering URL state.
Line 217 runs on initial render and forces
page=1, which breaks refresh/back persistence for this page. Guard first render so reset only happens after real filter changes.Proposed fix
-import { useState, useMemo, useEffect } from 'react'; +import { useState, useMemo, useEffect, useRef } from 'react'; @@ export default function ClassesPage() { + const isFirstRender = useRef(true); @@ useEffect(() => { + if (isFirstRender.current) { + isFirstRender.current = false; + return; + } setCurrentPage(1); }, [searchQuery, todayOnly, attendanceConcerns, facilityFilter, programFilter, statusFilter, setCurrentPage]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/pages/ClassesPage.tsx` around lines 217 - 219, The effect that calls setCurrentPage(1) runs on mount and clobbers URL/page state; change the useEffect (the one referencing useEffect and setCurrentPage with dependencies searchQuery, todayOnly, attendanceConcerns, facilityFilter, programFilter, statusFilter) to skip its first invocation—e.g. add an isInitialMount useRef (or track previous filter values) and only call setCurrentPage(1) when not the initial render (i.e. when a real filter change occurs) so refresh/back preserve the page query.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@frontend/src/pages/knowledge-center/KnowledgeCenterManagement.tsx`:
- Around line 339-344: The effect is resetting page on first render and
overwriting any ?page=... param; update the useEffect that reads currentTab to
skip calling setCurrentPage(1) on initial mount by adding a mounted ref (e.g.,
isInitialMount using useRef) or tracking previous tab, and only call
setCurrentPage(1) when currentTab changes after mount (but still always apply
the category reset logic for setCategoryFilter('all')); tweak the existing
useEffect (the one referencing currentTab, setCurrentPage, setCategoryFilter) to
guard the setCurrentPage call with that isInitialMount check so URL page state
is preserved on initial load.
---
Duplicate comments:
In `@frontend/src/hooks/useUrlPagination.ts`:
- Around line 16-33: The setters setPage and setPerPage close over the stale
searchParams and create new URLSearchParams from that snapshot, which can drop
concurrent query keys; change both to use the functional form of setSearchParams
(setSearchParams(prev => { ... })) so each update builds from the latest params,
and in setPerPage ensure you set perPageKey and reset pageKey inside that
functional update; verify useSearchParams' setSearchParams supports functional
updates before applying.
In `@frontend/src/pages/ClassesPage.tsx`:
- Around line 217-219: The effect that calls setCurrentPage(1) runs on mount and
clobbers URL/page state; change the useEffect (the one referencing useEffect and
setCurrentPage with dependencies searchQuery, todayOnly, attendanceConcerns,
facilityFilter, programFilter, statusFilter) to skip its first invocation—e.g.
add an isInitialMount useRef (or track previous filter values) and only call
setCurrentPage(1) when not the initial render (i.e. when a real filter change
occurs) so refresh/back preserve the page query.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 28968791-e299-4c16-aba6-ec29860dd40f
📒 Files selected for processing (8)
frontend/src/components/shared/index.tsfrontend/src/components/student/ActivityHistoryCard.tsxfrontend/src/hooks/useUrlPagination.tsfrontend/src/pages/ClassesPage.tsxfrontend/src/pages/ProgramsPage.tsxfrontend/src/pages/class-detail/AuditTab.tsxfrontend/src/pages/knowledge-center/KnowledgeCenterManagement.tsxfrontend/src/pages/programs/ProgramOverviewFacilityAdmin.tsx
💤 Files with no reviewable changes (1)
- frontend/src/components/shared/index.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pre-Submission PR Checklist
Description of the change
Centralizes pagination state management site-wide by migrating all paginated pages from local
useStateto theuseUrlPaginationhook, so pagination state is preserved in the URL and survives browser refresh and back navigation.components/shared/Pagination.tsxduplicate; re-exportedcomponents/Pagination.tsxfrom the shared index so all consumers get the canonical componentprefixparam touseUrlPaginationfor pages with multiple independent paginators (e.g.activity_page,history_page,audit_page)instanceIdprop toPagination.tsxto keep select element IDs unique when multiple instances appear on one pageClassEnrollmentDetailsandAddClassEnrollmentswith the fullPaginationcomponentRelated issues: Asana — Split 4: Pagination
Screenshot(s)
Additional context
Bug found and fixed during testing —
per_pagedropped from URL when changing items-per-page: ThePagination.tsxitems-per-pageonChangehandler was calling bothonItemsPerPageChange(value)andonPageChange(1). Both closures captured stalesearchParams, so the secondsetSearchParamsoverwrote the first, silently droppingper_pagefrom the URL. Fixed by removing the redundantonPageChange(1)—setPerPageinuseUrlPaginationalready resets page to 1 internally.Bug found and fixed during testing —
activity_pageresets to 1 on every mount:ActivityHistoryCardhad auseEffect(() => setPage(1), [filterQuery])that fired on every mount. Before this PR it reset a localuseStatevalue harmlessly; after migration it calledsetSearchParams, overwritingactivity_pageback to 1 on every page load. Fixed by adding auseRef(true)first-render guard so the reset only fires whenfilterQueryactually changes from a user action.