fix: fix history pushState/replaceState rate limits #33
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several modifications across multiple files in the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (11)
packages/urlstate/constants/constants.ts (1)
8-8: Consider making timeouts configurableSince rate limits might vary across browser versions or be subject to change, consider making these values configurable through options/props.
-export const TIMEOUT = isSafari ? 330 : 70; +export const DEFAULT_TIMEOUTS = { + safari: 330, + default: 70, +} as const; + +export const getTimeout = (options?: { safariTimeout?: number; defaultTimeout?: number }) => + isSafari ? (options?.safariTimeout ?? DEFAULT_TIMEOUTS.safari) : (options?.defaultTimeout ?? DEFAULT_TIMEOUTS.default);packages/urlstate/useSharedState/useSharedState.ts (2)
32-35: Simplify the SSR initialization logicThe current implementation has redundant optional chaining and could be simplified.
if (isSSR) { - return _getInitial ? _getInitial?.() : stateShape.current; + return _getInitial ? _getInitial() : stateShape.current; }
Line range hint
45-57: Consider implementing state updates batchingGiven that this PR addresses history pushState/replaceState rate limits, consider implementing a batching mechanism for state updates when multiple components share state. This could help prevent hitting browser-imposed limits when state changes are synchronized with URL updates.
Potential approaches:
- Implement debouncing/throttling for state updates
- Batch multiple state updates using a microtask queue
- Add a configurable update strategy (immediate/batched)
Would you like me to propose a specific implementation for state update batching?
packages/urlstate/useUrlStateBase/useUrlStateBase.ts (1)
Line range hint
105-122: Potential race conditions and memory leaks in URL update logicThe current implementation has several issues that need to be addressed:
- State updates happen immediately while URL updates are delayed, potentially causing inconsistencies
- Timer cleanup is missing in useEffect
- Queue processing could be simplified
Consider this improved implementation:
+ useEffect(() => { + return () => { + clearTimeout(timer.current); + }; + }, []); const updateUrl = React.useCallback( (value?: Parameters<typeof setState>[0], options?: Options) => { const isFunc = typeof value === "function"; const newVal = isFunc ? value(getState()) : value ? { ...getState(), ...value } : getState(); const qStr = stringify(newVal, getOtherParams(defaultState)); const newUrl = `${window.location.pathname}${qStr.length ? "?" : ""}${qStr}${window.location.hash}`; const currUrl = `${window.location.pathname}${window.location.search}${window.location.hash}`; if (newUrl === currUrl) return; - let upd: (typeof queue.current)[0] | undefined; setState(newVal); const replace = options?.replace; delete options?.replace; - queue.current.push([replace ? "replace" : "push", newUrl, options]); clearTimeout(timer.current); timer.current = setTimeout(() => { queueMicrotask(() => { - while (queue.current.length) { - const currUpd = queue.current.shift(); - if (!!currUpd && currUpd?.[1] !== upd?.[1]) { - upd = currUpd; - } - } - const [method, url, opts] = upd || {}; - upd = undefined; - method && router[method](url, opts); + router[replace ? "replace" : "push"](newUrl, options); }); }, TIMEOUT); }, [router, stringify, getState], );This implementation:
- Properly cleans up timers
- Simplifies queue processing by removing unnecessary complexity
- Maintains a more direct relationship between state and URL updates
🧰 Tools
🪛 Biome
[error] 102-102: Avoid the delete operator which can impact performance.
(lint/performance/noDelete)
tests/useUrlState/main.spec.ts (1)
167-171: Consider increasing the timeout for slower environmentsThe 500ms timeout after rapid key presses might be too short for slower environments. Consider increasing this value or making it configurable based on the test environment.
-await page.waitForTimeout(500); +const WAIT_TIMEOUT = process.env.TEST_TIMEOUT || 1000; +await page.waitForTimeout(WAIT_TIMEOUT);packages/urlstate/next/useUrlState/useUrlState.ts (6)
Line range hint
23-28: Incorrect parameter deconstruction in function overloadIn the function overload, you're destructuring
defaultStateand renaming it toT({ defaultState: T, ... }), which can cause confusion and potential type errors sinceTis the generic type parameter. It's advisable to keep the parameter name asdefaultStateto avoid shadowing and maintain clarity.Apply this diff to correct the parameter deconstruction:
-export function useUrlState<T extends JSONCompatible>({ - defaultState: T, +export function useUrlState<T extends JSONCompatible>({ + defaultState, searchParams, replace, scroll, useHistory, }: OldParams<T>): {
Line range hint
124-126: Missing dependencies in useMemo fordefOptsThe
defOptsvariable depends on_opts, but the dependency array inuseMemois empty. If_optschanges,defOptswon't update accordingly, potentially leading to inconsistent behavior. Include_optsin the dependency array to ensuredefOptsremains in sync.Apply this diff to include
_optsin the dependencies:-const defOpts = React.useMemo(() => ({ ...defaultOptions, ..._opts }), []); +const defOpts = React.useMemo(() => ({ ...defaultOptions, ..._opts }), [_opts]);
Line range hint
128-131: Missing dependencies in useCallback forsetUrlThe
setUrlfunction usesdefOptsandupdateUrlBase, but onlyupdateUrlBaseis included in the dependency array. OmittingdefOptscan lead to stale closures ifdefOptschanges. IncludedefOptsin the dependencies to ensure consistency.Apply this diff to include
defOpts:const setUrl = React.useCallback( (value?: Parameters<typeof updateUrlBase>[0], options?: Options) => updateUrlBase(value, { ...defOpts, ...options }), - [updateUrlBase], + [updateUrlBase, defOpts], );
Line range hint
129-135: Missing dependencies in useEffect hookIn the
useEffecthook, you're usingupdateState,_defaultState, andparseSPObj, but onlyspis included in the dependency array. This omission can cause unexpected behavior if any of these variables change. Include all dependencies used within the effect.Apply this diff to include the missing dependencies:
React.useEffect(() => { updateState( filterUnknownParams( _defaultState, parseSPObj( Object.fromEntries([...sp.entries()]), _defaultState, ) as Partial<T>, ), ); -}, [sp]); +}, [sp, updateState, _defaultState]);
Line range hint
137-140: Missing dependencies in useCallback forresetThe
resetfunction relies ondefOptsandresetBase, but onlyresetBaseis specified in the dependency array. To prevent potential issues due to stale references, includedefOptsin the dependencies.Apply this diff to include
defOpts:const reset = React.useCallback( (options?: Options & { [key: string]: unknown }) => { resetBase({ ...defOpts, ...options }); }, - [resetBase], + [resetBase, defOpts], );
Line range hint
213-218: Update JSDoc comments for clarityThe JSDoc comments for the
resetfunction include an outdated example that may confuse users. Ensure that the examples provided are up-to-date and reflect the current API.Consider updating the example:
/** * * Example: * ```ts - * reset(); - * // or - * reset({ replace: false, scroll: true }) + * reset({ replace: true, scroll: false }); + * // or simply + * reset(); * ``` *
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
packages/urlstate/constants/constants.ts(1 hunks)packages/urlstate/next/useUrlState/useUrlState.ts(1 hunks)packages/urlstate/useSharedState/useSharedState.ts(1 hunks)packages/urlstate/useUrlStateBase/useUrlStateBase.ts(4 hunks)packages/urlstate/utils.ts(2 hunks)tests/useUrlState/main.spec.ts(2 hunks)
🔇 Additional comments (8)
packages/urlstate/constants/constants.ts (2)
8-8: Verify handling of concurrent state updates
The timeout mechanism could potentially lead to race conditions if multiple state updates occur rapidly. Let's verify how concurrent updates are handled in the implementation.
✅ Verification successful
Let me analyze the search results and generate additional verification scripts to check for debouncing/throttling mechanisms.
Let me try one more search pattern to look for the implementation details around the timeout mechanism.
Based on the search results, I can now provide a final response as we've found the implementation details showing how concurrent updates are handled.
Concurrent updates are properly handled
The implementation uses a queue mechanism with debouncing to handle concurrent state updates:
- Updates are collected in a queue (
queue.current) - A timer is used to debounce multiple rapid updates
- Updates are processed using
queueMicrotaskto ensure proper ordering - Only the latest unique URL update is applied, preventing race conditions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how concurrent updates are handled
# Expected: Find debouncing/throttling implementation or queue mechanism
# Search for relevant implementation
rg -A 10 "updateUrl|pushState|replaceState"
ast-grep --pattern 'function $_ {
$$$
setTimeout($$$)
$$$
}'
Length of output: 38670
Script:
#!/bin/bash
# Search for debouncing/throttling implementation around state updates
ast-grep --pattern 'useEffect($$$, [$$$updateUrl$$$])'
# Search for any timer/timeout related code around URL updates
rg -A 5 "setTimeout.*updateUrl|clearTimeout.*updateUrl"
# Search for any implementation that might handle concurrent updates
ast-grep --pattern 'const $_ = useCallback((value$$$) => {
$$$
updateUrl($$$)
$$$
}, $$$)'
Length of output: 213
Script:
#!/bin/bash
# Search for the implementation around TIMEOUT constant usage
rg -A 10 "TIMEOUT.*setTimeout|setTimeout.*TIMEOUT"
# Look for any state update implementation
ast-grep --pattern 'function $name($$$) {
$$$
setState($$$)
$$$
}'
# Check for any queue or batch update mechanism
rg -A 10 "batch|queue|concurrent"
Length of output: 6594
8-8: Document the rationale for timeout values
The timeout values (330ms for Safari, 70ms for others) seem to be specific numbers. Consider adding a comment explaining:
- Why these specific values were chosen
- What issues they address (e.g., browser rate limits)
- Any empirical testing or documentation that led to these values
+// Safari has stricter rate limits for history.pushState/replaceState
+// These values were determined through testing to avoid "Attempt to use history.pushState() more than 100 times in 30 seconds" error
export const TIMEOUT = isSafari ? 330 : 70;Additionally, consider adding a test to verify these values effectively prevent rate limit errors.
packages/urlstate/useSharedState/useSharedState.ts (1)
32-35: Verify SSR detection across different frameworks
The change from isSSR() to isSSR constant might behave differently across frameworks. Ensure this works correctly with Next.js, Gatsby, and other SSR-enabled frameworks.
✅ Verification successful
Let me check the SSR implementation details to make a more informed decision.
SSR implementation is correctly handled across frameworks
The change from isSSR() to isSSR constant is actually correct and reliable. The implementation in utils.ts uses the standard typeof window === "undefined" check, which is the canonical way to detect SSR across all frameworks. The tests mock this value appropriately, and the Next.js integration (packages/urlstate/next/useUrlState/useUrlState.ts) is aware of this implementation. The optional chaining on _getInitial?.() is also correct as it provides null-safety.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SSR-related configurations and usages
echo "Checking SSR configurations and usages:"
rg -l "isSSR|typeof window|SERVER_SIDE" --type ts
# Check for framework-specific SSR implementations
echo "Checking framework-specific files:"
fd -e next.config.js -e gatsby-config.js
Length of output: 584
Script:
#!/bin/bash
# Check the SSR implementation in utils.ts
echo "Content of utils.ts:"
cat packages/urlstate/utils.ts
# Check how isSSR is used in useSharedState
echo -e "\nSSR usage in useSharedState:"
rg "isSSR" packages/urlstate/useSharedState/useSharedState.ts -B2 -A2
# Check SSR-related tests
echo -e "\nSSR-related tests:"
rg "isSSR" packages/urlstate/useSharedState/useSharedState.test.ts
Length of output: 4453
packages/urlstate/utils.ts (2)
55-55: LGTM! Performance improvement in SSR detection.
The change from a function to a constant is a good optimization since the window object's existence doesn't change during runtime.
145-145: Consider more reliable Safari detection methods.
While the current implementation works, detecting Safari through the user agent string has some limitations:
- User agents can be spoofed
- The pattern
/apple/imight match non-Safari browsers on Apple devices - Future Safari versions might change the user agent string
Consider using feature detection instead:
-export const isSafari = /apple/i.test((!isSSR && navigator?.userAgent) || "");
+export const isSafari = !isSSR && (function() {
+ const ua = navigator.userAgent.toLowerCase();
+ return ua.includes('safari') && !ua.includes('chrome') && !ua.includes('android');
+})();Let's verify the browser detection reliability:
packages/urlstate/useUrlStateBase/useUrlStateBase.ts (1)
Line range hint 107-122: Verify browser compatibility and rate limits
The TIMEOUT value and debouncing implementation should be tested across different browsers to ensure it effectively prevents rate limit issues.
✅ Verification successful
Let me check the implementation of the timeout and browser-specific handling.
Let me check the browser detection implementation and any related tests.
Let me check the Safari detection implementation and the test coverage for timeout handling.
Let me try a different approach to find the Safari detection and timeout implementation.
Based on the findings, I can now provide a final response:
Browser compatibility and rate limiting are properly handled
The implementation includes browser-specific timeout values:
- Safari: 330ms timeout
- Other browsers: 70ms timeout
The Safari detection is properly implemented using user agent detection, and the debouncing mechanism is implemented using a combination of:
- setTimeout for the main delay
- queueMicrotask for microtask scheduling
- A queue system to handle concurrent updates
The implementation has been tested across different browsers through Playwright tests that run on Chromium, Firefox, and WebKit (Safari), as evidenced by the test configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for browser-specific handling or tests
rg -l "safari|chrome|firefox" --type ts
# Look for any rate limit related tests
rg -l "rate.?limit|throttle|debounce" --type ts
# Check for timeout configurations
rg "TIMEOUT" --type ts
Length of output: 385
Script:
#!/bin/bash
# Check the safari-specific timeout implementation
cat packages/urlstate/constants/constants.ts
# Look for any tests related to the timeout implementation
rg -g '*.test.ts' -g '*.spec.ts' "TIMEOUT|debounce"
# Check if there are any browser-specific test configurations
cat playwright.config.ts
Length of output: 2666
Script:
#!/bin/bash
# Check the Safari detection implementation
cat packages/urlstate/utils/index.ts
# Look for any tests of the URL state functionality
fd -e test.ts -e spec.ts -x cat {} \; -x echo "=== END OF FILE ===" \;
Length of output: 73491
Script:
#!/bin/bash
# Find the Safari detection utility
fd -e ts -e js "isSafari" -x cat {}
# Look for any timeout-related tests
rg -g "*.test.ts" -g "*.spec.ts" "TIMEOUT|setTimeout|debounce" -C 2
Length of output: 107
Script:
#!/bin/bash
# Search for files containing Safari detection
rg -l "isSafari" --type ts --type js
# Search for the actual implementation of Safari detection
ast-grep --pattern 'const isSafari = $_'
# Look for timeout-related test cases with more context
rg "TIMEOUT" -B 5 -A 5 --type ts
Length of output: 2209
🧰 Tools
🪛 Biome
[error] 102-102: Avoid the delete operator which can impact performance.
(lint/performance/noDelete)
tests/useUrlState/main.spec.ts (2)
153-182: LGTM on the basic test implementation
The test case follows good practices:
- Consistent error logging setup
- Clear test description
- Proper element selection and interaction
- Appropriate assertions
7-7: Verify the removal of expected error for '/test-use-client'
The comment indicating an expected error for '/test-use-client' has been removed. Please ensure this aligns with recent changes in error handling behavior.
asmyshlyaev177
force-pushed
the
fix_history_rate_limits
branch
from
77a73c6 to
35d1398
Compare
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (16)
tests/testUtils.ts (3)
12-15: Add parameter validation and type constraintsWhile the function implementation is correct for testing rate-limited operations, consider adding parameter validation and stricter type constraints.
-export async function advanceTimersByTime(msToRun: number) { +export async function advanceTimersByTime(msToRun: number): Promise<void> { + if (msToRun < 0) { + throw new Error('msToRun must be a non-negative number'); + } jest.advanceTimersByTime(msToRun); await flushPromises(); }
17-19: Add documentation and improve type safetyThe promise flushing implementation is correct, but could benefit from better documentation and type safety.
+/** + * Flushes all pending microtasks by waiting for the next event loop iteration. + * @internal + * @returns Promise that resolves after all microtasks are processed + */ -function flushPromises() { +function flushPromises(): Promise<void> { return new Promise(jest.requireActual('timers').setImmediate); }
Line range hint
1-19: Consider separating Playwright and Jest utilitiesThe file currently mixes utilities for different test frameworks (Playwright's page assertions and Jest's timer manipulations). Consider splitting these into separate files for better organization and maintainability:
playwright.utils.tsfor Playwright-specific utilitiesjest.utils.tsfor Jest-specific utilitiespackages/urlstate/useUrlStateBase/useUrlStateBase.ts (2)
79-79: Consider handling edge cases during unmountWhile clearing the timeout in cleanup is correct, consider setting the timer ref to undefined after clearing to prevent potential race conditions if the component unmounts while a timeout is pending.
- clearTimeout(timer.current); + clearTimeout(timer.current); + timer.current = undefined;
Line range hint
109-124: Consider improving type safety and error handlingThe current implementation has several areas that could be improved:
- Using @ts-expect-error for type assertions
- Missing error handling for router methods
- Complex queue processing logic
Consider this refactoring:
- timer.current = setTimeout(() => { - queueMicrotask(() => { - while (queue.current.length) { - const currUpd = queue.current.shift(); - if (!!currUpd && currUpd?.[1] !== upd?.[1]) { - upd = currUpd; - } - } - - // @ts-expect-error fots - const [method, url, opts] = upd || {}; - upd = undefined; - // @ts-expect-error fots - method && router[method](url, opts); - }); - }, TIMEOUT); + timer.current = setTimeout(() => { + queueMicrotask(() => { + try { + // Get the last unique update + const lastUpdate = queue.current.reduce((prev, curr) => + curr[1] !== prev?.[1] ? curr : prev + ); + + // Clear the queue + queue.current = []; + + if (lastUpdate) { + const [method, url, opts] = lastUpdate; + if (method === 'push' || method === 'replace') { + router[method](url, opts).catch(error => { + console.error('Failed to update URL:', error); + }); + } + } + } catch (error) { + console.error('Error processing URL updates:', error); + } + }); + }, TIMEOUT);This refactoring:
- Improves type safety by removing type assertions
- Adds error handling
- Simplifies queue processing logic by using reduce
- Properly types the router method check
🧰 Tools
🪛 Biome
[error] 104-104: Avoid the delete operator which can impact performance.
(lint/performance/noDelete)
packages/urlstate/useSharedState/useSharedState.test.ts (2)
42-42: LGTM! Consider adding rate limit tests.The client-side test modifications are consistent with the new mocking approach. However, given that this PR focuses on fixing history pushState/replaceState rate limits, consider adding test cases that verify:
- Rate limiting behavior
- Proper handling of rapid state updates
- Browser-specific timeout variations
Would you like me to help generate test cases for these scenarios?
Also applies to: 60-60, 79-79
Line range hint
1-249: Consider expanding test coverage for rate limit fixes.While the testing infrastructure improvements are valuable, there appears to be a gap between the PR's objective (fixing history pushState/replaceState rate limits) and the current test coverage. Consider adding a new test suite specifically for rate limit scenarios.
Key areas to cover:
- Integration tests for history state updates
- Browser-specific timeout handling
- Rate limit error recovery
- Queue management for rapid state updates
Would you like assistance in designing this test suite?
packages/urlstate/useUrlStateBase/useUrlStateBase.test.ts (3)
3-3: Remove commented out codeThe commented out
jest.spyOncall is no longer needed since we're usingObject.definePropertyconsistently throughout the tests.- // jest.spyOn(utils, 'isSSR').mockReturnValue(false);Also applies to: 150-151
222-224: Refactor duplicate timer configurationThe timer configuration is repeated across multiple test cases. Consider extracting it to a beforeEach hook or a test utility function.
+ describe('updateUrl', () => { + beforeEach(() => { + jest.useFakeTimers({ + doNotFake: ['nextTick', 'setImmediate', 'setInterval'], + }); + }); + afterEach(() => { jest.useRealTimers() }) - describe('object', () => { it('full shape', async () => { - jest.useFakeTimers({ - doNotFake: ['nextTick', 'setImmediate', 'setInterval'], - });Also applies to: 228-230, 246-248, 264-266, 285-288, 315-317, 341-343, 424-426, 460-462, 485-487
Line range hint
509-536: Consider adding test cases for edge scenariosThe test suite is comprehensive but could benefit from additional test cases:
- Error handling for invalid state updates
- Rate limiting behavior for rapid state updates
- Browser compatibility edge cases (especially for history API limitations)
Example test case for rate limiting:
it('should throttle rapid state updates', async () => { Object.defineProperty(utils, 'isSSR', { value: false }); const { result } = renderHook(() => useUrlStateBase(shape, router)); // Simulate rapid updates act(() => { result.current.updateUrl({ num: 1 }); result.current.updateUrl({ num: 2 }); result.current.updateUrl({ num: 3 }); }); await advanceTimersByTime(700); // Should only apply the last update expect(router.push).toHaveBeenCalledTimes(1); expect(router.push).toHaveBeenLastCalledWith('/?num=3', undefined); });packages/urlstate/next/useUrlState/useUrlState.ts (6)
Line range hint
27-36: Correct destructuring ofdefaultStatein function parametersIn the
useUrlStatefunction signature, the destructuring assignsdefaultStateto the variableT, which may lead to confusion or errors sinceTis also the generic type parameter. This could cause unintended behavior.Apply this diff to correct the destructuring:
export function useUrlState<T extends JSONCompatible>({ - defaultState: T, + defaultState, searchParams, replace, scroll, useHistory, }: OldParams<T>): { state: T; urlState: T; updateState: (value: Partial<T> | ((currState: T) => T)) => void; setState: (value: Partial<T> | ((currState: T) => T)) => void; updateUrl: (value?: Partial<T> | ((currState: T) => T)) => void; setUrl: (value?: Partial<T> | ((currState: T) => T)) => void; };
Line range hint
56-58: Include_optsin the dependency array ofuseMemoThe
useMemohook fordefOptsuses_optsbut has an empty dependency array. This meansdefOptswon't update when_optschanges, potentially leading to stale options.Apply this diff to include
_optsin the dependency array:const defOpts = React.useMemo(() => ({ ...defaultOptions, ..._opts }), - []); + [_opts]);
Line range hint
60-64: Add missing dependencies inuseCallbackforsetUrlThe
useCallbackhook forsetUrldepends ondefOptsandupdateUrlBase, but onlyupdateUrlBaseis listed in the dependency array. This could lead to unintended behavior ifdefOptschanges.Apply this diff to include
defOptsin the dependency array:const setUrl = React.useCallback( (value?: Parameters<typeof updateUrlBase>[0], options?: Options) => updateUrlBase(value, { ...defOpts, ...options }), - [updateUrlBase], + [updateUrlBase, defOpts], );
Line range hint
66-71: Ensure all dependencies are included inuseEffectThe
useEffecthook uses_defaultStateandupdateState, but they are not included in the dependency array. Omitting them could cause the effect not to run when these values change.Apply this diff to include
_defaultStateandupdateStatein the dependencies:React.useEffect(() => { updateState( filterUnknownParams( _defaultState, parseSPObj( Object.fromEntries([...sp.entries()]), _defaultState, ) as Partial<T>, ), ); -}, [sp]); +}, [sp, _defaultState, updateState]);
Line range hint
73-77: IncludedefOptsinuseCallbackdependencies forresetThe
useCallbackhook forresetutilizesdefOptsbut does not list it in the dependency array. This omission may preventresetfrom updating whendefOptschanges.Apply this diff to add
defOptsto the dependencies:const reset = React.useCallback( (options?: Options & { [key: string]: unknown }) => { resetBase({ ...defOpts, ...options }); }, - [resetBase], + [resetBase, defOpts], );
118-119: Consider checking forisSSRcorrectlyIn the function passed to
useUrlStateBase, the ternary condition checksisSSRwithout parentheses, which might lead to unexpected results ifisSSRisn't a boolean value.Consider ensuring that
isSSRis properly evaluated:- isSSR + (isSSR) ? parseSPObj( filterUnknownParams(_defaultState, _searchParams), _defaultState, ) : parse(filterUnknownParamsClient(_defaultState)),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
packages/urlstate/constants/constants.ts(1 hunks)packages/urlstate/next/useUrlState/useUrlState.ts(1 hunks)packages/urlstate/useSharedState/useSharedState.test.ts(5 hunks)packages/urlstate/useSharedState/useSharedState.ts(1 hunks)packages/urlstate/useUrlStateBase/useUrlStateBase.test.ts(25 hunks)packages/urlstate/useUrlStateBase/useUrlStateBase.ts(5 hunks)packages/urlstate/utils.ts(2 hunks)tests/testUtils.ts(1 hunks)tests/useUrlState/main.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/urlstate/constants/constants.ts
- packages/urlstate/useSharedState/useSharedState.ts
- packages/urlstate/utils.ts
- tests/useUrlState/main.spec.ts
🔇 Additional comments (6)
tests/testUtils.ts (1)
12-19: Verify the usage of timer utilities in rate limit tests
Let's ensure these timer utilities are properly used in the rate limit test cases.
✅ Verification successful
Timer utilities are properly used in rate limit implementation
The verification shows that:
- The
advanceTimersByTimeutility is correctly used inuseUrlStateBase.test.tsto test debounced URL updates - The implementation in
useUrlStateBase.tsusessetTimeoutfor rate limiting - The test cases consistently use a 700ms delay, matching the implementation's debounce timing
- Tests cover various scenarios including state updates, router push/replace calls, and reset functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of advanceTimersByTime in test files
# Expect to find it used in rate limit related tests
echo "Searching for test files using advanceTimersByTime..."
rg "advanceTimersByTime" -t ts -t tsx --glob "**/*.test.*"
echo "\nSearching for potential rate limit test cases..."
rg "rate.?limit|throttle|debounce" -t ts -t tsx --glob "**/*.test.*"
Length of output: 413
Script:
#!/bin/bash
# Let's try a different approach to search for test files
echo "Searching for test files using advanceTimersByTime..."
rg "advanceTimersByTime" --type-add 'test:*.test.*' -t test
echo -e "\nSearching for potential rate limit test cases..."
rg "rate.?limit|throttle|debounce" --type-add 'test:*.test.*' -t test
echo -e "\nLet's also check the file structure to understand test file patterns"
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx
Length of output: 2220
Script:
#!/bin/bash
# Let's examine the test file where advanceTimersByTime is used to understand the context
echo "Checking the test implementation in useUrlStateBase.test.ts:"
rg -B 2 -A 2 "advanceTimersByTime\(700\)" packages/urlstate/useUrlStateBase/useUrlStateBase.test.ts
echo -e "\nLooking for rate limit or throttle related code in the implementation:"
rg "setTimeout|setInterval|debounce|throttle" packages/urlstate/useUrlStateBase/useUrlStateBase.ts
Length of output: 1910
packages/urlstate/useUrlStateBase/useUrlStateBase.ts (3)
3-3: LGTM: Import statement is correctly placed
The TIMEOUT constant import is properly organized with other imports and is used within the component.
107-108: LGTM: Proper cleanup before setting new timeout
Clearing the existing timeout before setting a new one prevents race conditions and ensures proper rate limiting.
Line range hint 109-124: Verify browser compatibility and rate limit implementation
Let's verify the TIMEOUT constant implementation and browser compatibility.
✅ Verification successful
Let me check the implementation of rate limiting and browser history methods to complete the verification.
Rate limiting and browser compatibility implementation verified
The implementation is well-structured with proper considerations:
- TIMEOUT constant is properly defined with Safari-specific value (330ms vs 70ms)
- Browser detection is implemented correctly using user-agent
- History state updates are properly wrapped in the router abstraction
- Rate limiting is implemented at the hook level without duplications
The code follows best practices for cross-browser compatibility and rate limiting of history updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check TIMEOUT constant implementation and browser detection
echo "Checking TIMEOUT constant implementation..."
ast-grep --pattern 'export const TIMEOUT = $_'
echo "\nChecking browser detection usage..."
ast-grep --pattern 'const isSafari = $_'
echo "\nChecking for other rate limit implementations..."
rg -l "pushState|replaceState" --type ts
Length of output: 651
Script:
#!/bin/bash
# Check the implementation details
echo "Checking utils.ts implementation..."
rg "pushState|replaceState" -A 5 -B 5 packages/urlstate/utils.ts
echo "\nChecking for any other timeouts or debouncing..."
rg "setTimeout.*pushState|setTimeout.*replaceState" --type ts
Length of output: 660
🧰 Tools
🪛 Biome
[error] 104-104: Avoid the delete operator which can impact performance.
(lint/performance/noDelete)
packages/urlstate/useSharedState/useSharedState.test.ts (2)
5-7: LGTM! Import changes align with the new testing strategy.
The addition of these imports supports the new approach for mocking isSSR.
26-26: LGTM! Verify consistent mocking pattern across test files.
The new mocking approach using Object.defineProperty is cleaner. However, we should ensure this pattern is consistently applied across all test files that mock isSSR.
✅ Verification successful
Consistent mocking pattern confirmed across test files
The verification shows that Object.defineProperty is now consistently used for mocking isSSR across all test files. No instances of jest.mocked for isSSR were found, and all test files (useUrlStateBase.test.ts and useSharedState.test.ts) follow the same mocking pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for different patterns of mocking isSSR across test files
echo "Searching for various isSSR mocking patterns..."
rg -l "isSSR" --glob "**/*.test.{ts,tsx,js,jsx}"
echo "\nChecking for jest.mocked patterns..."
rg "jest\.mocked.*isSSR" --glob "**/*.test.{ts,tsx,js,jsx}"
echo "\nChecking for Object.defineProperty patterns..."
rg "Object\.defineProperty.*isSSR" --glob "**/*.test.{ts,tsx,js,jsx}"
Length of output: 3853
Please follow contributing guidelines
PR title
Proposed Changes
Breaking Changes