-
Notifications
You must be signed in to change notification settings - Fork 78
fix(FR-1446): refactor useDeferredQueryParams #4229
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: main
Are you sure you want to change the base?
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has required the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
7b66114 to
cca9416
Compare
Coverage report for
|
St.❔ |
Category | Percentage | Covered / Total |
|---|---|---|---|
| 🔴 | Statements | 4.59% (-0% 🔻) |
501/10916 |
| 🔴 | Branches | 3.7% (+0% 🔼) |
285/7697 |
| 🔴 | Functions | 2.62% (+0% 🔼) |
90/3434 |
| 🔴 | Lines | 4.56% (-0% 🔻) |
487/10688 |
Show new covered files 🐣
St.❔ |
File | Statements | Branches | Functions | Lines |
|---|---|---|---|---|---|
| 🔴 | ... / useTransitionSafeQueryParams.tsx |
3.03% | 0% | 0% | 3.13% |
Test suite run success
114 tests passing in 13 suites.
Report generated by 🧪jest coverage report action from 2b72266
cca9416 to
99d7fac
Compare
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.
Pull Request Overview
This PR fixes state persistence issues in query parameters by replacing the global useDeferredQueryParams hook with a new page-scoped useTransitionSafeQueryParams hook. The changes prevent cross-page state pollution and ensure pagination/filter values persist correctly after page refresh.
Key changes:
- Replaced global state management with page-specific state isolation
- Renamed hook to better reflect its purpose of handling React transitions safely
- Added proper atom lifecycle management to prevent memory leaks
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| react/src/hooks/useTransitionSafeQueryParams.tsx | New implementation with page-scoped state management and context provider |
| react/src/hooks/useDeferredQueryParams.tsx | Removed old global state implementation |
| react/src/App.tsx | Added PageQueryParamAtomProvider wrapper for proper state isolation |
| react/src/pages/*.tsx | Updated imports to use new hook name across 3 page components |
| react/src/components/AgentSummaryList.tsx | Updated import to use new hook name |
| react/src/hooks/reactPaginationQueryOptions.tsx | Updated import to use new hook name |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Create page-specific atoms that reset when pagePath changes | ||
| const localQueryParamsAtom = useMemo(() => { | ||
| // Internal state atom for this specific page | ||
| let hasLoadedFromURL = false; | ||
|
|
||
| // Derived atom that manages the query parameters | ||
| const queryParamsAtom = atomWithDefault((get) => { | ||
| const pageQueryParamDelta = get(pageQueryParamDeltaAtom); | ||
|
|
||
| // Use URL query params on first access, then use internal state | ||
| if (!hasLoadedFromURL) { | ||
| hasLoadedFromURL = true; |
Copilot
AI
Sep 5, 2025
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.
The hasLoadedFromURL variable is declared inside the useMemo callback but shared across re-renders. This will cause the variable to reset on every render, making the URL loading logic unreliable. Move this variable outside the useMemo or use a ref to maintain its state across renders.
| // Create page-specific atoms that reset when pagePath changes | |
| const localQueryParamsAtom = useMemo(() => { | |
| // Internal state atom for this specific page | |
| let hasLoadedFromURL = false; | |
| // Derived atom that manages the query parameters | |
| const queryParamsAtom = atomWithDefault((get) => { | |
| const pageQueryParamDelta = get(pageQueryParamDeltaAtom); | |
| // Use URL query params on first access, then use internal state | |
| if (!hasLoadedFromURL) { | |
| hasLoadedFromURL = true; | |
| // Persist flag across renders and atom invocations | |
| const hasLoadedFromURLRef = useRef(false); | |
| // Create page-specific atoms that reset when pagePath changes | |
| const localQueryParamsAtom = useMemo(() => { | |
| // Derived atom that manages the query parameters | |
| const queryParamsAtom = atomWithDefault((get) => { | |
| const pageQueryParamDelta = get(pageQueryParamDeltaAtom); | |
| // Use URL query params on first access, then use internal state | |
| if (!hasLoadedFromURLRef.current) { | |
| hasLoadedFromURLRef.current = true; |
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.
I share the same curiosity as this review.
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.
The review is incorrect. The hasLoadedFromURL variable is intentionally designed to reset when useMemo creates a new atom instance. This ensures each atom gets proper initial URL loading, then switches to using pageQueryParamDelta. Moving it outside would break this intended behavior - it's a feature, not a bug.
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.
Simplified the implementation by removing the hasLoadedFromURL logic entirely. The hook now directly merges query with pageQueryParamDelta, making the original review concern obsolete.
| const PageQueryParamDeltaAtomContext = createContext< | ||
| PrimitiveAtom<Record<string, any>> | undefined | ||
| >(undefined); |
Copilot
AI
Sep 5, 2025
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.
The context default value should not be undefined as it creates potential runtime errors. Consider providing a proper default atom or throwing an error in the context creation to make the contract clearer.
| const PageQueryParamDeltaAtomContext = createContext< | |
| PrimitiveAtom<Record<string, any>> | undefined | |
| >(undefined); | |
| const PageQueryParamDeltaAtomContext = createContext<PrimitiveAtom<Record<string, any>>>( | |
| atom(() => { | |
| throw new Error( | |
| 'PageQueryParamDeltaAtomContext must be used within <PageQueryParamAtomProvider>' | |
| ); | |
| }) | |
| ); |
| // Create page-specific atoms that reset when pagePath changes | ||
| const localQueryParamsAtom = useMemo(() => { | ||
| // Internal state atom for this specific page | ||
| let hasLoadedFromURL = false; | ||
|
|
||
| // Derived atom that manages the query parameters | ||
| const queryParamsAtom = atomWithDefault((get) => { | ||
| const pageQueryParamDelta = get(pageQueryParamDeltaAtom); | ||
|
|
||
| // Use URL query params on first access, then use internal state | ||
| if (!hasLoadedFromURL) { | ||
| hasLoadedFromURL = true; |
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.
I share the same curiosity as this review.
99d7fac to
2f52a79
Compare
| throw new Error( | ||
| 'usePageQueryParamDeltaAtom must be used within <PageQueryParamAtomP44rovider>', |
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.
| throw new Error( | |
| 'usePageQueryParamDeltaAtom must be used within <PageQueryParamAtomP44rovider>', | |
| throw new Error( | |
| 'usePageQueryParamDeltaAtom must be used within <PageQueryParamAtomProvider>', |
2f52a79 to
2b72266
Compare

Fix Query Parameter State Management and Persistence
Resolves #4251 (FR-1446)
Summary
This PR fixes the state persistence issue in the query parameters hook where pagination and filter values were not persisting correctly after page refresh, and addresses cross-page state pollution issues.
Problem
Users experienced issues where:
Solution
useDeferredQueryParamstouseTransitionSafeQueryParamsto better reflect its purpose of handling React transitions safelyTechnical Implementation
Files Changed
react/src/hooks/useDeferredQueryParams.tsx(deleted)react/src/hooks/useTransitionSafeQueryParams.tsx(new implementation)Testing