fix: enhance cache key management in useQuery#3229
fix: enhance cache key management in useQuery#3229eduardoformiga wants to merge 13 commits intodevfrom
Conversation
- Introduced a session-based cache key suffix in useQuery to differentiate between logged-in and anonymous users. - This change prevents stale product data from being served when users switch session states, improving data accuracy in the application.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds session-aware SWR cache keying and a post-logout reload coordination mechanism: SWR keys now include a session suffix, a sessionStorage flag is set before logout, and a hook triggers a full reload on pageshow when returning from logout. Changes
Sequence DiagramsequenceDiagram
participant User
participant App
participant LogoutFlow
participant SessionStorage
participant Browser
User->>App: Click "Logout"
App->>LogoutFlow: doLogout()
LogoutFlow->>SessionStorage: setReloadAfterLogoutReturn() (store flag)
LogoutFlow->>Browser: Navigate to logout URL / redirect out
Browser->>User: External logout completes
User->>Browser: Return to app (may be bfcache/persisted)
Browser->>App: pageshow event (persisted=true)
App->>SessionStorage: read RELOAD_AFTER_LOGOUT_KEY
alt flag present
App->>SessionStorage: remove flag
App->>Browser: window.location.reload()
Browser->>App: Full reload (fresh session state)
else flag absent
App->>App: Continue without reload
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/sdk/graphql/useQuery.ts (1)
22-50: Avoid side effects inside SWR key derivation to reduce storage churn.
getClientCacheBustingValue()can clear storage when no auth cookie; called inside the SWR key function it may run repeatedly (multiple hooks + revalidations). Precompute the key once per render so the key function stays pure and avoids repeated storage work.Suggested refactor
-export const useQuery = <Data, Variables = Record<string, unknown>>( - operation: Operation, - variables: Variables, - options?: QueryOptions -) => - useSWR<Data>( - () => { - if (options?.doNotRun) return null - const baseKey = getKey(operation['__meta__']['operationName'], variables) - const sessionSuffix = getSessionCacheKeySuffix() - return `${baseKey}::${sessionSuffix}` - }, - { - fetcher: () => { - return new Promise((resolve) => { - setTimeout(async () => { - resolve( - await request<Data, Variables>(operation, variables, options) - ) - }) - }) - }, - ...DEFAULT_OPTIONS, - ...options, - } - ) +export const useQuery = <Data, Variables = Record<string, unknown>>( + operation: Operation, + variables: Variables, + options?: QueryOptions +) => { + const key = options?.doNotRun + ? null + : `${getKey(operation['__meta__']['operationName'], variables)}::${getSessionCacheKeySuffix()}` + + return useSWR<Data>(key, { + fetcher: () => { + return new Promise((resolve) => { + setTimeout(async () => { + resolve(await request<Data, Variables>(operation, variables, options)) + }) + }) + }, + ...DEFAULT_OPTIONS, + ...options, + }) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/sdk/graphql/useQuery.ts` around lines 22 - 50, The SWR key function in useQuery currently calls getSessionCacheKeySuffix()/getClientCacheBustingValue() which may mutate storage repeatedly; instead compute the sessionSuffix and baseKey once during render (call getClientCacheBustingValue() and getKey(...) outside the function) and store a precomputedKey variable, then pass a pure key function to useSWR that only returns null when options?.doNotRun is true or returns the precomputedKey; update references in useQuery to use the precomputedKey and keep getSessionCacheKeySuffix/getClientCacheBustingValue calls out of the key derivation function to avoid repeated side effects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/sdk/graphql/useQuery.ts`:
- Around line 45-50: prefetchQuery and useLazyQuery build cache keys with
getKey(...) only, but useQuery includes the session suffix via
getSessionCacheKeySuffix(), causing cache misses; update the key construction in
prefetchQuery (function prefetchQuery) and useLazyQuery (function useLazyQuery)
to append the session suffix so keys become
`${getKey(...)}::${getSessionCacheKeySuffix()}`, ensuring the same key format as
useQuery and preventing unnecessary refetches.
---
Nitpick comments:
In `@packages/core/src/sdk/graphql/useQuery.ts`:
- Around line 22-50: The SWR key function in useQuery currently calls
getSessionCacheKeySuffix()/getClientCacheBustingValue() which may mutate storage
repeatedly; instead compute the sessionSuffix and baseKey once during render
(call getClientCacheBustingValue() and getKey(...) outside the function) and
store a precomputedKey variable, then pass a pure key function to useSWR that
only returns null when options?.doNotRun is true or returns the precomputedKey;
update references in useQuery to use the precomputedKey and keep
getSessionCacheKeySuffix/getClientCacheBustingValue calls out of the key
derivation function to avoid repeated side effects.
| () => { | ||
| if (options?.doNotRun) return null | ||
| const baseKey = getKey(operation['__meta__']['operationName'], variables) | ||
| const sessionSuffix = getSessionCacheKeySuffix() | ||
| return `${baseKey}::${sessionSuffix}` | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the useQuery.ts file to understand the context
find . -path "*/packages/core/src/sdk/graphql/useQuery.ts" -type fRepository: vtex/faststore
Length of output: 101
🏁 Script executed:
# Search for imports and usage of useQuery and getKey
rg -n --type=ts --type=tsx "from.*sdk/graphql/useQuery|import.*getKey" packagesRepository: vtex/faststore
Length of output: 85
🏁 Script executed:
# Search for SWRConfig and fallback/prefetch patterns
rg -n --type=ts --type=tsx "SWRConfig|fallback.*getKey|prefetch.*getKey" packagesRepository: vtex/faststore
Length of output: 85
🏁 Script executed:
# Search for getKey function definition and uses
rg -n --type=ts --type=tsx "getKey\(" packages/core/src/sdkRepository: vtex/faststore
Length of output: 85
🏁 Script executed:
# Search for imports and usage of useQuery and getKey (using ts only)
rg -n --type=ts "from.*sdk/graphql/useQuery|import.*getKey" packagesRepository: vtex/faststore
Length of output: 892
🏁 Script executed:
# Search for SWRConfig and fallback/prefetch patterns
rg -n --type=ts "SWRConfig|fallback|prefetch" packagesRepository: vtex/faststore
Length of output: 4648
🏁 Script executed:
# Search for getKey function in the useQuery file and surrounding code
cat ./packages/core/src/sdk/graphql/useQuery.tsRepository: vtex/faststore
Length of output: 1884
🏁 Script executed:
# Search for getKey usage across SDK
rg -n "getKey\(" packages/core/src/sdkRepository: vtex/faststore
Length of output: 733
🏁 Script executed:
# Check prefetchQuery.ts implementation
cat ./packages/core/src/sdk/graphql/prefetchQuery.tsRepository: vtex/faststore
Length of output: 635
🏁 Script executed:
# Check useLazyQuery.ts implementation
cat ./packages/core/src/sdk/graphql/useLazyQuery.tsRepository: vtex/faststore
Length of output: 854
🏁 Script executed:
# Check usePageProductsQuery.ts for cache key usage
cat ./packages/core/src/sdk/product/usePageProductsQuery.tsRepository: vtex/faststore
Length of output: 4528
Update prefetchQuery and useLazyQuery to include the session suffix in their cache keys.
Both functions currently build keys using getKey() alone, but useQuery now includes the session suffix (baseKey::sessionSuffix). This mismatch prevents prefetch and lazy query results from being found in the cache, causing unnecessary refetches.
Update:
packages/core/src/sdk/graphql/prefetchQuery.ts(line 13)packages/core/src/sdk/graphql/useLazyQuery.ts(line 12)
Both need to construct keys as ${getKey(...)}::${getSessionCacheKeySuffix()} to align with useQuery.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/sdk/graphql/useQuery.ts` around lines 45 - 50,
prefetchQuery and useLazyQuery build cache keys with getKey(...) only, but
useQuery includes the session suffix via getSessionCacheKeySuffix(), causing
cache misses; update the key construction in prefetchQuery (function
prefetchQuery) and useLazyQuery (function useLazyQuery) to append the session
suffix so keys become `${getKey(...)}::${getSessionCacheKeySuffix()}`, ensuring
the same key format as useQuery and preventing unnecessary refetches.
- Added functionality to handle anonymous sessions in cookie cache busting logic. - Introduced new session storage key for anonymous sessions and updated logic to generate a unique value when the user is not authenticated. - Enhanced tests to verify the correct behavior of anonymous session handling during authentication state changes.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/utils/cookieCacheBusting.ts (1)
141-178:⚠️ Potential issue | 🟡 MinorChange return type from
string | nulltostring.All code paths return a non-null string: the anonymous branch creates a timestamp if needed, and authenticated branches always return either a timestamp or the stored last value. The current type annotation forces unnecessary null-guards in callers (e.g.,
value ?? 'anonymous'inuseQuery.ts) and violates type safety.Diff
-export const getClientCacheBustingValue = (): string | null => { +export const getClientCacheBustingValue = (): string => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/utils/cookieCacheBusting.ts` around lines 141 - 178, Update the getClientCacheBustingValue function signature to return a plain string (not string | null) because every control path already produces a non-null string; change its type annotation from string | null to string in getClientCacheBustingValue, and then remove or simplify unnecessary null-guards in callers (e.g., usages in useQuery.ts that do `value ?? 'anonymous'`) to use the returned string directly. Ensure any related helper functions (getAnonymousSessionValue/getLastValue) remain compatible, and run type checks to catch any remaining places that assumed a nullable return.
🧹 Nitpick comments (5)
packages/core/test/utils/cookieCacheBusting.test.ts (2)
58-79:nowSpyis declared but never asserted in the logout test.All other timestamp-related tests (e.g., lines 44–56) explicitly assert
expect(nowSpy).toHaveBeenCalled(). The logout test skips this, making it inconsistent and less explicit.💚 Proposed fix
expect(sessionStorage.getItem(STORAGE_KEY_ANONYMOUS_SESSION)).toBe( '1700000000999' ) + expect(nowSpy).toHaveBeenCalled() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/utils/cookieCacheBusting.test.ts` around lines 58 - 79, The logout test declares nowSpy (jest.spyOn(Date, 'now')) but never asserts it was called; update the test case "should clear storage and return new anonymous value when user logs out" to include an assertion like expect(nowSpy).toHaveBeenCalled() after calling getClientCacheBustingValue(), ensuring Date.now was invoked by getClientCacheBustingValue/clearAllCookies flow and keeping the test consistent with other timestamp-related tests.
44-56: Add an idempotency test for the anonymous session.The core promise of the anonymous-session feature is that
getClientCacheBustingValue()returns the same value on repeated calls within a session (so the SWR cache key remains stable across re-renders). There is no test covering this, which is the most important invariant to verify.it('should return the same anonymous value on repeated calls (idempotent)', () => { jest.spyOn(Date, 'now') .mockReturnValueOnce(1700000000000) .mockReturnValueOnce(9999999999999) // should never be reached const first = getClientCacheBustingValue() const second = getClientCacheBustingValue() expect(first).toBe('1700000000000') expect(second).toBe('1700000000000') // same value reused from sessionStorage expect(Date.now).toHaveBeenCalledTimes(1) // Date.now called only once })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/utils/cookieCacheBusting.test.ts` around lines 44 - 56, Add an idempotency test to verify getClientCacheBustingValue() returns the same anonymous-session value across repeated calls: mock Date.now() to return a fixed value on the first call and a different value if called again, call getClientCacheBustingValue() twice, assert both returns equal the first timestamp string, assert Date.now was called only once, and assert STORAGE_KEY_ANONYMOUS_SESSION is used to persist/retrieve the value so the second call reads from sessionStorage rather than invoking Date.now again.packages/core/src/utils/cookieCacheBusting.ts (2)
97-116: Style inconsistency with existing helpers.
getAnonymousSessionValueandstoreAnonymousSessionValueuse single-line early returns without braces, while all existing helpers (getStoredAuthCookieValue,storeAuthCookieValue,getLastValue,storeLastValue,clearStorage) use block-body style. Pick one convention and stick to it.♻️ Proposed fix — align with existing style
-const getAnonymousSessionValue = (): string | null => { - if (typeof sessionStorage === 'undefined') return null - try { +const getAnonymousSessionValue = (): string | null => { + if (typeof sessionStorage === 'undefined') { + return null + } + try { return sessionStorage.getItem(STORAGE_KEY_ANONYMOUS_SESSION) } catch { return null } } -const storeAnonymousSessionValue = (value: string): void => { - if (typeof sessionStorage === 'undefined') return - try { +const storeAnonymousSessionValue = (value: string): void => { + if (typeof sessionStorage === 'undefined') { + return + } + try { sessionStorage.setItem(STORAGE_KEY_ANONYMOUS_SESSION, value) } catch { // Ignore storage errors } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/utils/cookieCacheBusting.ts` around lines 97 - 116, The two helpers getAnonymousSessionValue and storeAnonymousSessionValue use single-line early returns without braces which is inconsistent with other helpers (getStoredAuthCookieValue, storeAuthCookieValue, getLastValue, storeLastValue, clearStorage); update both functions to use block-bodied conditionals and try/catch blocks with explicit braces around early returns and catch bodies (preserve the same semantics with typeof sessionStorage checks, STORAGE_KEY_ANONYMOUS_SESSION, and the same null/void returns) so the style matches the existing helper implementations.
144-155: Logic is correct, but be aware of per-tabsessionStorageisolation for anonymous users.The anonymous session value is stored in
sessionStorage, which is scoped to the individual tab. Opening a new browser tab will generate a fresh anonymous timestamp key, so two anonymous tabs on the same site will have different SWR cache keys and issue separate fetches. This is not a bug, but it means the feature doesn't deduplicate network traffic across tabs for anonymous users the way it would withlocalStorage.If cross-tab cache sharing for anonymous sessions is desirable in the future,
localStoragewould be the right storage layer here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/utils/cookieCacheBusting.ts` around lines 144 - 155, The current logic in getAnonymousSessionValue/storeAnonymousSessionValue uses session-scoped storage so each tab gets a unique anonymousValue (causing separate SWR keys and fetches); if you want cross-tab deduplication, switch the anonymous session storage from sessionStorage to localStorage by updating getAnonymousSessionValue and storeAnonymousSessionValue to read/write localStorage (preserving the existing fallback/timestamp behavior) and keep clearStorage/getStoredAuthCookieValue logic unchanged so logout still clears the shared value.packages/core/src/sdk/graphql/request.ts (1)
69-69:?? 'anonymous'fallback is unreachable via therequest()call path.
getClientCacheBustingValue()(line 33) now always returns a non-null string, so the'anonymous'literal is never actually used in production. The fallback is still safe and preserves theBaseRequestOptions.value?: string | nullcontract for any direct callers ofbaseRequest, but note the literal'anonymous'is a different string from the actual timestamp the anonymous-session logic produces — keep this in mind if you ever diff logs or URLs to distinguish session types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/sdk/graphql/request.ts` at line 69, The '?? "anonymous"' fallback in the GET cache-busting spread is effectively unreachable because getClientCacheBustingValue() always returns a non-null string; remove the unreachable literal and use the provided value directly (or default to getClientCacheBustingValue() at the call site) so GET uses the real timestamp value; update references in baseRequest / request() logic and keep BaseRequestOptions.value?: string | null contract intact for any direct callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/core/src/utils/cookieCacheBusting.ts`:
- Around line 141-178: Update the getClientCacheBustingValue function signature
to return a plain string (not string | null) because every control path already
produces a non-null string; change its type annotation from string | null to
string in getClientCacheBustingValue, and then remove or simplify unnecessary
null-guards in callers (e.g., usages in useQuery.ts that do `value ??
'anonymous'`) to use the returned string directly. Ensure any related helper
functions (getAnonymousSessionValue/getLastValue) remain compatible, and run
type checks to catch any remaining places that assumed a nullable return.
---
Nitpick comments:
In `@packages/core/src/sdk/graphql/request.ts`:
- Line 69: The '?? "anonymous"' fallback in the GET cache-busting spread is
effectively unreachable because getClientCacheBustingValue() always returns a
non-null string; remove the unreachable literal and use the provided value
directly (or default to getClientCacheBustingValue() at the call site) so GET
uses the real timestamp value; update references in baseRequest / request()
logic and keep BaseRequestOptions.value?: string | null contract intact for any
direct callers.
In `@packages/core/src/utils/cookieCacheBusting.ts`:
- Around line 97-116: The two helpers getAnonymousSessionValue and
storeAnonymousSessionValue use single-line early returns without braces which is
inconsistent with other helpers (getStoredAuthCookieValue, storeAuthCookieValue,
getLastValue, storeLastValue, clearStorage); update both functions to use
block-bodied conditionals and try/catch blocks with explicit braces around early
returns and catch bodies (preserve the same semantics with typeof sessionStorage
checks, STORAGE_KEY_ANONYMOUS_SESSION, and the same null/void returns) so the
style matches the existing helper implementations.
- Around line 144-155: The current logic in
getAnonymousSessionValue/storeAnonymousSessionValue uses session-scoped storage
so each tab gets a unique anonymousValue (causing separate SWR keys and
fetches); if you want cross-tab deduplication, switch the anonymous session
storage from sessionStorage to localStorage by updating getAnonymousSessionValue
and storeAnonymousSessionValue to read/write localStorage (preserving the
existing fallback/timestamp behavior) and keep
clearStorage/getStoredAuthCookieValue logic unchanged so logout still clears the
shared value.
In `@packages/core/test/utils/cookieCacheBusting.test.ts`:
- Around line 58-79: The logout test declares nowSpy (jest.spyOn(Date, 'now'))
but never asserts it was called; update the test case "should clear storage and
return new anonymous value when user logs out" to include an assertion like
expect(nowSpy).toHaveBeenCalled() after calling getClientCacheBustingValue(),
ensuring Date.now was invoked by getClientCacheBustingValue/clearAllCookies flow
and keeping the test consistent with other timestamp-related tests.
- Around line 44-56: Add an idempotency test to verify
getClientCacheBustingValue() returns the same anonymous-session value across
repeated calls: mock Date.now() to return a fixed value on the first call and a
different value if called again, call getClientCacheBustingValue() twice, assert
both returns equal the first timestamp string, assert Date.now was called only
once, and assert STORAGE_KEY_ANONYMOUS_SESSION is used to persist/retrieve the
value so the second call reads from sessionStorage rather than invoking Date.now
again.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/sdk/graphql/request.tspackages/core/src/utils/cookieCacheBusting.tspackages/core/test/utils/cookieCacheBusting.test.ts
- Removed the anonymous session management from cookie cache busting, streamlining the logic for handling cache busting when the auth cookie is missing. - Updated tests to reflect the new behavior, ensuring that storage is cleared and null is returned when the auth cookie is absent.
- Added a new utility to manage session state upon logout, ensuring the application reloads to reflect the correct session data when returning from the VTEX ID logout. - Integrated the reload logic into the OrganizationDrawer's logout process and utilized it in the main application component.
- Introduced console logs in the GraphQL request and cookie cache busting logic to aid in debugging and tracking the cache busting value and cookie states. - Enhanced visibility into the values being processed during requests and cache management, facilitating easier troubleshooting.
- Enhanced debug logging in the cookie cache busting utility to provide more visibility into the authentication cookie name, the cookies present, and the retrieved auth cookie. - This update aims to facilitate easier troubleshooting and understanding of cookie states during the cache busting process.
- Introduced a new module to manage session authentication state, allowing retrieval of the current session's person ID for cache busting. - Updated the GraphQL request logic to utilize the person ID instead of cookies for cache management, enhancing security by avoiding reliance on httpOnly cookies. - Refactored cookie cache busting utility to work with session data, ensuring accurate cache invalidation based on user authentication state changes. - Enhanced tests to validate the new behavior and ensure proper functionality of the cache busting mechanism.
- Updated the logout function to await the response from the API, ensuring Set-Cookie headers are processed before redirecting. - Improved the reload logic in the OrganizationDrawer to handle both fresh loads and bfcache restores, ensuring session-dependent data is accurate after logout. - Added detailed comments to clarify the flow and conditions for reloading the application state.
- Introduced the idb-keyval library to manage IndexedDB operations, enhancing the application's ability to clear session data effectively. - Updated the OrganizationDrawer component to include logic for deleting the session key and the entire keyval-store database upon logout, improving data handling and storage management.
- Replaced the direct window reload with a cache-busting navigation method that appends a timestamp to the URL, ensuring fresh content is loaded after logout. - Introduced a delay before the navigation to enhance user experience during the logout process.
- Modified the comment in the checkAndReloadIfReturnedFromLogout function to include a period for improved readability and consistency.
|
superseded by
|
What's the purpose of this pull request?
This PR aims to improve cache data from queries between logged-in and anonymous users.
How it works?
Problem
The SWR cache key in useQuery was only
operationName+variables. BecauseProductShelfvariables (term,sort,selectedFacets, etc.) are the same for logged-in and logged-out users, SWR reused the same cache and showed data from the previous session (e.g. wrong prices or availability after login/logout).Solution
The cache key now includes the authentication state:
getSessionCacheKeySuffix()Uses the same logic as the request:
getClientCacheBustingValue()from cookieCacheBusting.ts.Resulting keys:
operationName::variables::anonymous for logged-out users
operationName::variables:: for logged-in users
When the user logs in or out, the key changes so SWR treats it as a different cache entry: it fetches again and does not reuse data from the other session.
How to test it?
Starters Deploy Preview
References
Checklist
You may erase this after checking them all 😉
PR Title and Commit Messages
feat,fix,chore,docs,style,refactor,ciandtestPR Description
breaking change,bug,contributing,performance,documentation..Dependencies
pnpm-lock.yamlfile when there were changes to the packagesDocumentation
@Mariana-Caetanoto review and update (Or submit a doc request)Summary by CodeRabbit
New Features
Chores