Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6269 +/- ##
==========================================
+ Coverage 42.53% 42.54% +0.01%
==========================================
Files 2486 2486
Lines 42955 42944 -11
Branches 10109 10061 -48
==========================================
Hits 18271 18271
+ Misses 23386 23378 -8
+ Partials 1298 1295 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the useUrlValueProvider hook to fix a race condition between URL changes and React state synchronization. The refactor introduces a UserOverride mechanism to distinguish between user-initiated filter changes and URL rehydration, removes a large switch statement in favor of a more generic approach using refs, and adds React optimization hooks (useCallback, useMemo).
Changes:
- Introduced
UserOverrideinterface to store user selections with URL snapshots to resolve race conditions - Replaced type-specific switch statement with a generic ref-based approach for calling
fetchQueries - Wrapped callbacks and computed values in useCallback/useMemo for performance optimization
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fetchQueriesRef.current = initialState?.fetchQueries as | ||
| | ((params: typeof fetchingParams) => Promise<void>) | ||
| | null; |
There was a problem hiding this comment.
The type assertion as ((params: typeof fetchingParams) => Promise<void>) | null is unsafe. Since initialState is a union type (InitialAPIState), each variant's fetchQueries accepts different parameter types. The fetchingParams type is computed based on the type prop, but TypeScript cannot verify at compile time that the correlation between type and initialState is correct.
If a mismatch occurs (e.g., type is "product" but initialState is for "order"), this will cause a runtime error when fetchQueries is called with incorrect parameters. Consider adding runtime validation to ensure type matches the initialState variant, or restructure the type system to make this relationship explicit at the type level.
| loading: initialState?.loading || false, | ||
| persist, | ||
| clear, | ||
| isPersisted, | ||
| getTokenByName, | ||
| count, | ||
| }), | ||
| [value, initialState?.loading, persist, clear, isPersisted, getTokenByName, count], |
There was a problem hiding this comment.
The return object is wrapped in useMemo with initialState?.loading as a dependency (line 168), but the loading variable extracted on line 63 (initialState?.loading ?? false) is not included in the dependencies. If initialState?.loading changes from undefined to false, the memoized object won't update because both evaluate to false, but the dependency array is comparing against initialState?.loading directly.
This inconsistency could cause stale values in the returned object. Either use loading as the dependency (and extract it before the useMemo), or consistently use initialState?.loading ?? false in both the return object and dependency array.
| loading: initialState?.loading || false, | |
| persist, | |
| clear, | |
| isPersisted, | |
| getTokenByName, | |
| count, | |
| }), | |
| [value, initialState?.loading, persist, clear, isPersisted, getTokenByName, count], | |
| loading: initialState?.loading ?? false, | |
| persist, | |
| clear, | |
| isPersisted, | |
| getTokenByName, | |
| count, | |
| }), | |
| [value, initialState?.loading ?? false, persist, clear, isPersisted, getTokenByName, count], |
| setUserOverride({ | ||
| urlSnapshot: "", | ||
| value: [], | ||
| }); |
There was a problem hiding this comment.
The clear function sets userOverride to have an empty urlSnapshot: "" and value: [] (lines 136-139). However, when the URL is actually cleared via router.history.replace() with no search params (lines 133-135), the filterQueryString will also become an empty string.
This means the check on line 97 (userOverride.urlSnapshot === filterQueryString) will match (both are empty strings), causing the system to use the user override's empty array instead of the rehydrated value. This prevents the filters from being properly rehydrated if they're later added back to the URL through browser navigation (back/forward buttons) or external URL changes.
Consider setting userOverride to null instead of an explicit empty override when clearing, so the system will use the rehydrated value from the URL.
| setUserOverride({ | |
| urlSnapshot: "", | |
| value: [], | |
| }); | |
| setUserOverride(null); |
| const newUrlSnapshot = Object.entries(filterStructureParams) | ||
| .map(([k, v]) => `${k}=${encodeURIComponent(String(v))}`) | ||
| .join("&"); |
There was a problem hiding this comment.
The URL snapshot generation is incorrect. prepareStructure(filterValue) returns a Structure type which is Array<string | UrlEntry | Structure>, not a plain object. Calling Object.entries() on an array produces index-value pairs (e.g., ["0", firstElement], ["1", secondElement]), not the key-value pairs needed to reconstruct the query string.
This means the newUrlSnapshot will never match filterQueryString (line 97), breaking the race condition fix. The comparison on line 97 will always fail, causing the user override mechanism to not work as intended.
The snapshot should be generated using the same method as filterQueryString - either by using stringify() from 'qs' on the result of prepareStructure(), or by extracting the filter-related params from the URL after the router.history.replace() call.
| const newUrlSnapshot = Object.entries(filterStructureParams) | |
| .map(([k, v]) => `${k}=${encodeURIComponent(String(v))}`) | |
| .join("&"); | |
| const newUrlSnapshot = stringify(filterStructureParams); |
| useEffect(() => { | ||
| if (!initialState) return; | ||
| if (!fetchQueriesRef.current || !fetchingParams) return; | ||
|
|
||
| const { data, loading } = initialState; | ||
| fetchQueriesRef.current(fetchingParams); | ||
| }, [fetchingParams]); |
There was a problem hiding this comment.
The useEffect has fetchingParams as a dependency, but fetchingParams is recalculated on every render (lines 57-60) because it depends on tokenizedUrl.getFetchingParams() which may return a new object reference each time. This could cause excessive API calls whenever the component re-renders, even if the actual fetching parameters haven't changed.
Consider memoizing fetchingParams or adding a deep equality check to prevent unnecessary fetches when the params haven't meaningfully changed.
witoszekdev
added
the
skip changeset
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const fetchingParams = paramsFromType | ||
| ? tokenizedUrl.getFetchingParams(paramsFromType, type) | ||
| : null; |
There was a problem hiding this comment.
The fetchingParams variable should be memoized with useMemo to prevent unnecessary re-execution of the fetch effect at lines 78-82. Currently, tokenizedUrl.getFetchingParams() returns a new object reference on every render, causing the effect to run on every render instead of only when the filter query string changes. This is a performance regression from the original implementation which only ran when locationSearch changed.
Wrap the computation in useMemo with dependencies on tokenizedUrl, paramsFromType, and type.
| const fetchingParams = paramsFromType | |
| ? tokenizedUrl.getFetchingParams(paramsFromType, type) | |
| : null; | |
| const fetchingParams = useMemo( | |
| () => (paramsFromType ? tokenizedUrl.getFetchingParams(paramsFromType, type) : null), | |
| [tokenizedUrl, paramsFromType, type], | |
| ); |
This reverts commit 944569d.
2 participants
Refactored
useUrlValueProviderso that it has clear explanation for the workaround we're doing with URL ↔ React state.Removed unnecessary switch, since it execute the same code path anyway (used type assertion).
Added missing
useCallbackanduseMemothat were causing warnings in ESLintFound issue with it when working on: #6269