-
Notifications
You must be signed in to change notification settings - Fork 6.4k
Fix datagrid dropping query params on pagination #3944
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
|
@AymanAlSuleihi is attempting to deploy a commit to the umami-software Team on Vercel. A member of the Team first needs to authorize it. |
Greptile Summary
Important Files Changed
Confidence score: 5/5
Sequence DiagramsequenceDiagram
participant User
participant Pager
participant DataGrid
participant useNavigation
participant Router
participant URL
User->>Pager: "clicks next/prev page button"
Pager->>Pager: "handlePageChange(value)"
Pager->>DataGrid: "onPageChange(nextPage)"
DataGrid->>DataGrid: "handlePageChange(page)"
DataGrid->>useNavigation: "updateParams({ search, page })"
useNavigation->>useNavigation: "buildPath(pathname, { ...queryParams, search, page })"
useNavigation-->>DataGrid: "returns new URL path"
DataGrid->>Router: "router.push(newPath)"
Router->>URL: "updates URL with preserved params"
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR fixes a stale closure bug in the DataGrid component that caused query parameters (specifically the date filter) to be dropped during pagination.
Root Cause: The handlePageChange callback had an incomplete dependency array containing only [search], causing it to capture stale references to router and updateParams at component mount time.
The Fix: Added router and updateParams to the dependency array, ensuring the callback always has access to the latest updateParams function, which closes over the current queryParams state from the useNavigation hook.
How It Works:
- When a user changes the date filter, the URL updates with
?date=30day - Next.js
searchParamschanges, triggeringuseNavigation'suseEffectto updatequeryParamsstate useNavigationre-renders, creating a newupdateParamsfunction that references the updatedqueryParams- Since
updateParamsis now in the dependency array,handlePageChangeis recreated with the fresh reference - When pagination occurs, the callback uses the latest query parameters including the date filter
The fix is minimal, targeted, and follows React's rules of hooks correctly. All values referenced within the callback are now properly included in the dependency array.
Confidence Score: 5/5
- This PR is safe to merge with minimal risk
- The fix correctly addresses a well-identified stale closure bug with a minimal, targeted change. The solution follows React best practices by including all referenced values in the useCallback dependency array. The PR has clear reproduction steps, and the fix directly resolves the issue without introducing side effects or breaking changes.
- No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/components/common/DataGrid.tsx | 5/5 | Fixed stale closure bug by adding router and updateParams to handlePageChange dependency array, ensuring pagination preserves query parameters |
Sequence Diagram
sequenceDiagram
participant User
participant DateFilter
participant URL
participant useNavigation
participant DataGrid
participant Pager
Note over User,Pager: Initial State - No Date Filter
User->>DateFilter: Selects "Last 30 days"
DateFilter->>URL: Updates to ?date=30day
URL->>useNavigation: searchParams changes
useNavigation->>useNavigation: useEffect updates queryParams state
useNavigation->>useNavigation: Creates new updateParams function<br/>with fresh queryParams closure
useNavigation->>DataGrid: Returns new updateParams reference
DataGrid->>DataGrid: handlePageChange recreates<br/>due to updateParams dependency
Note over DataGrid: handlePageChange now has fresh<br/>updateParams with date=30day
User->>Pager: Clicks next page
Pager->>DataGrid: Calls handlePageChange(2)
DataGrid->>useNavigation: Calls updateParams({search, page: 2})
useNavigation->>useNavigation: Merges {...queryParams, search, page: 2}<br/>queryParams includes date=30day
useNavigation-->>DataGrid: Returns "/path?search=&date=30day&page=2"
DataGrid->>URL: router.push with preserved date param
URL->>User: Navigation to page 2 with date filter preserved
Update the dependency list on
handlePageChangeto use the latestrouterandupdateParamsreferences preventing potential stale closures.Issue
When navigating between pages in the data grid, the current date filter (e.g.
?date=30day) is dropped from the URL. This resets the date range back to the default 24 hours. This can be seen in pages such as Sessions, and Events.Replication Steps
/websites/[id]/Sessions./websites/[id]/sessions?date=30day./websites/[id]/sessions?search=&page=2(note date is missing).Expected
Pagination preserves existing query params such as date. e.g.
/websites/[id]/sessions?search=&date=30day&page=2.Actual
Clicking next page drops the date param, e.g.
/websites/[id]/sessions?search=&page=2.Cause
Stale closure: Incomplete dependency list on
handlePageChangecausing the function to send stale query param values.