Skip to content

Conversation

@AymanAlSuleihi
Copy link

@AymanAlSuleihi AymanAlSuleihi commented Jan 7, 2026

Update the dependency list on handlePageChange to use the latest router and updateParams references 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

  1. Navigate to /websites/[id]/Sessions.
  2. Select "Last 30 days" from the date dropdown.
  3. Confirm URL becomes: /websites/[id]/sessions?date=30day.
  4. Click next page arrow in the pager.
  5. Observe URL becomes /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 handlePageChange causing the function to send stale query param values.

@vercel
Copy link

vercel bot commented Jan 7, 2026

@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-apps
Copy link
Contributor

greptile-apps bot commented Jan 7, 2026

Greptile Summary

  • Fixes stale closure bug in DataGrid component by adding missing dependencies to handlePageChange callback's dependency array
  • Ensures pagination preserves existing query parameters (like date filters) that were previously being dropped during navigation

Important Files Changed

Filename Overview
src/components/common/DataGrid.tsx Fixed stale closure by adding router and updateParams to handlePageChange dependency array

Confidence score: 5/5

  • This PR is safe to merge with minimal risk
  • Score reflects a targeted fix to a well-identified stale closure bug with clear reproduction steps and minimal code changes
  • No files require special attention

Sequence Diagram

sequenceDiagram
    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"
Loading

@AymanAlSuleihi AymanAlSuleihi marked this pull request as draft January 9, 2026 22:57
@AymanAlSuleihi AymanAlSuleihi marked this pull request as ready for review January 9, 2026 23:02
Copy link
Contributor

@greptile-apps greptile-apps bot left a 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:

  1. When a user changes the date filter, the URL updates with ?date=30day
  2. Next.js searchParams changes, triggering useNavigation's useEffect to update queryParams state
  3. useNavigation re-renders, creating a new updateParams function that references the updated queryParams
  4. Since updateParams is now in the dependency array, handlePageChange is recreated with the fresh reference
  5. 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
Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant