fix: enhance cache key management in useQuery#3230
Open
eduardoformiga wants to merge 1 commit intodevfrom
Open
fix: enhance cache key management in useQuery#3230eduardoformiga wants to merge 1 commit intodevfrom
eduardoformiga wants to merge 1 commit intodevfrom
Conversation
- Modified the comment in the checkAndReloadIfReturnedFromLogout function to include a period for improved readability and consistency.
eduardoformiga
requested review from
emersonlaurentino and
lemagnetic
and removed request for
a team
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds session-based cache-busting (switching from cookie to session person ID), introduces a reload-after-logout mechanism, cleans IndexedDB during logout via idb-keyval, ensures logout Set-Cookie headers are consumed, and wires the reload hook into app initialization. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as React App
participant API as Logout API
participant Browser as Browser Storage<br/>(sessionStorage, IndexedDB)
participant SWR as SWR Cache
participant Return as Return Page Load
User->>App: Click Logout
App->>Browser: setReloadAfterLogoutReturn() (set sessionStorage flag)
App->>API: POST /logout
API-->>App: 200 OK + Set-Cookie
App->>App: await res.json() (consume body to process Set-Cookie)
App->>Browser: Clear sessionStorage (cookies, faststore_person_id)
App->>Browser: import idb-keyval, delete 'fs::session' key, delete 'keyval-store' DB
App->>Browser: window.location.replace(logout_url)
User->>Return: Return to site (fresh load or bfcache)
Return->>Browser: Check sessionStorage flag
Return->>Browser: Clear flag, call forceRefreshWithoutCache()
Return->>Return: window.location.replace(url + ts)
Return->>SWR: Requests use new session-based cache key
SWR-->>Return: Fresh data for current session
Return->>User: Page fully refreshed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
eduardoformiga
changed the title
5 tasks
|
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. |
![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
🧹 Nitpick comments (4)
packages/core/src/utils/cookieCacheBusting.ts (1)
104-111:clearStorage()is called on every render via the SWR key function — worth noting it's a no-op after the first call but adds repeated storage access.
getClientCacheBustingValueis invoked bygetSessionCacheKeySuffixinuseQuery.ts, which runs on every SWR key evaluation (i.e., every render). When the user is not logged in,clearStorage()is called each time. SinceremoveItemon a non-existent key is a no-op, this is functionally safe but generates unnecessary storage churn on every render.Consider short-circuiting early if already cleared, or moving the cleanup to a one-time effect.
🤖 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 104 - 111, The function getClientCacheBustingValue currently calls clearStorage() on every render when getPersonId() returns null; to avoid repeated storage access, add a short-circuit (e.g., a module-level boolean like storageCleared) that is checked before calling clearStorage() and set to true after the first call, or move the cleanup to a one-time effect in the consumer (getSessionCacheKeySuffix / useQuery.ts); update getClientCacheBustingValue to check the flag before calling clearStorage() so subsequent calls are no-ops without touching storage repeatedly.packages/core/src/sdk/graphql/useQuery.ts (1)
22-28:getSessionCacheKeySuffixcallsgetClientCacheBustingValue, which has storage side effects on every SWR key evaluation.When the user is logged out, each SWR key evaluation triggers
clearStorage()insidegetClientCacheBustingValue. While benign (removeItem is idempotent), it's worth knowing this runs on every render cycle for everyuseQuerycall. The impact is low but consider memoizing or using a lightweight flag to avoid redundant storage access in the hot path.🤖 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 - 28, getSessionCacheKeySuffix currently calls getClientCacheBustingValue on every evaluation causing repeated storage side-effects; change getSessionCacheKeySuffix to cache the computed suffix in a module-level variable (or a memo flag) so that after the first browser-side call it returns the cached suffix ('', value, or 'anonymous') thereafter, still preserving the server check (typeof window === 'undefined') and the fallback logic; reference getSessionCacheKeySuffix and getClientCacheBustingValue when implementing the cache to avoid repeated clearStorage/removeItem calls in hot paths.packages/core/src/sdk/sessionAuthState.ts (1)
6-6: Consider exportingPersonIdGetterfor type-safe call sites.Callers passing a function to
setCacheBustingSourcehave to manually match the signature or rely on inference. Exporting the type makes the contract explicit and improves DX.-type PersonIdGetter = () => string | null +export type PersonIdGetter = () => string | null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/sdk/sessionAuthState.ts` at line 6, Export the PersonIdGetter type so callers can import it and get a typed signature for functions passed to setCacheBustingSource: change the local alias declaration to an exported type (e.g. "export type PersonIdGetter = () => string | null") and ensure any module-level exports (e.g. index/barrel) include PersonIdGetter so consumer code can import it and annotate the parameter for setCacheBustingSource.packages/core/src/pages/_app.tsx (1)
9-9:useReloadAfterLogoutReturnis now a global app concern — consider relocating it out ofOrganizationDrawer.The hook is imported from deep inside
OrganizationDrawer/, but it's wired at the top-level_app.tsx. This placement suggests it belongs in a more general location, e.g.,src/sdk/orsrc/hooks/, to reflect its app-wide scope and avoid misleading coupling to the drawer component.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/pages/_app.tsx` at line 9, The hook useReloadAfterLogoutReturn is imported into the top-level _app.tsx but implemented deep in OrganizationDrawer, so move it to a global location (e.g., src/hooks/useReloadAfterLogoutReturn.ts or src/sdk/useReloadAfterLogoutReturn.ts) and update exports/imports accordingly; specifically, extract the implementation from OrganizationDrawer's folder into the new file, export the hook from there, update _app.tsx to import from the new module name, and update OrganizationDrawer (and any other files) to import the hook from the new global path to avoid misleading coupling.
🤖 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/components/account/MyAccountDrawer/OrganizationDrawer/OrganizationDrawer.tsx`:
- Around line 60-75: The outer try is swallowing errors from the first cleanup
step and preventing the subsequent IndexedDB deletion (new Promise deleting
'keyval-store') from running; split the logic into two guarded blocks so each
step runs and failures are handled independently: wrap the idb-keyval import and
await del('fs::session') in its own try/catch (referencing the dynamic import
and del call), then always run the IndexedDB deletion logic (referencing
window.indexedDB, idb.deleteDatabase('keyval-store') and the
onsuccess/onerror/onblocked resolve handlers) in a separate try/catch so that an
error in the first step won’t skip deleting 'keyval-store'.
In `@packages/core/src/pages/_app.tsx`:
- Around line 28-29: The conditional call to the hook useScrollRestoration (via
storeConfig.experimental?.scrollRestoration && useScrollRestoration()) violates
the Rules of Hooks; change the call site to call useScrollRestoration
unconditionally next to useReloadAfterLogoutReturn(), and move the feature flag
check into the hook implementation of useScrollRestoration so the hook returns
early when storeConfig.experimental?.scrollRestoration is falsy. Ensure
useScrollRestoration contains the guard (early return) and any side-effects only
run when the flag is enabled.
In `@packages/core/src/sdk/graphql/request.ts`:
- Line 32: Fix the typo in the comment that reads "Get cache busting value based
person?.id from session" by inserting the missing preposition to read "Get cache
busting value based on person?.id from session" so the comment referencing
person?.id and session in request.ts is grammatically correct.
---
Nitpick comments:
In `@packages/core/src/pages/_app.tsx`:
- Line 9: The hook useReloadAfterLogoutReturn is imported into the top-level
_app.tsx but implemented deep in OrganizationDrawer, so move it to a global
location (e.g., src/hooks/useReloadAfterLogoutReturn.ts or
src/sdk/useReloadAfterLogoutReturn.ts) and update exports/imports accordingly;
specifically, extract the implementation from OrganizationDrawer's folder into
the new file, export the hook from there, update _app.tsx to import from the new
module name, and update OrganizationDrawer (and any other files) to import the
hook from the new global path to avoid misleading coupling.
In `@packages/core/src/sdk/graphql/useQuery.ts`:
- Around line 22-28: getSessionCacheKeySuffix currently calls
getClientCacheBustingValue on every evaluation causing repeated storage
side-effects; change getSessionCacheKeySuffix to cache the computed suffix in a
module-level variable (or a memo flag) so that after the first browser-side call
it returns the cached suffix ('', value, or 'anonymous') thereafter, still
preserving the server check (typeof window === 'undefined') and the fallback
logic; reference getSessionCacheKeySuffix and getClientCacheBustingValue when
implementing the cache to avoid repeated clearStorage/removeItem calls in hot
paths.
In `@packages/core/src/sdk/sessionAuthState.ts`:
- Line 6: Export the PersonIdGetter type so callers can import it and get a
typed signature for functions passed to setCacheBustingSource: change the local
alias declaration to an exported type (e.g. "export type PersonIdGetter = () =>
string | null") and ensure any module-level exports (e.g. index/barrel) include
PersonIdGetter so consumer code can import it and annotate the parameter for
setCacheBustingSource.
In `@packages/core/src/utils/cookieCacheBusting.ts`:
- Around line 104-111: The function getClientCacheBustingValue currently calls
clearStorage() on every render when getPersonId() returns null; to avoid
repeated storage access, add a short-circuit (e.g., a module-level boolean like
storageCleared) that is checked before calling clearStorage() and set to true
after the first call, or move the cleanup to a one-time effect in the consumer
(getSessionCacheKeySuffix / useQuery.ts); update getClientCacheBustingValue to
check the flag before calling clearStorage() so subsequent calls are no-ops
without touching storage repeatedly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by none
📒 Files selected for processing (10)
packages/core/package.jsonpackages/core/src/components/account/MyAccountDrawer/OrganizationDrawer/OrganizationDrawer.tsxpackages/core/src/components/account/MyAccountDrawer/OrganizationDrawer/useReloadAfterLogoutReturn.tspackages/core/src/pages/_app.tsxpackages/core/src/sdk/graphql/request.tspackages/core/src/sdk/graphql/useQuery.tspackages/core/src/sdk/session/index.tspackages/core/src/sdk/sessionAuthState.tspackages/core/src/utils/cookieCacheBusting.tspackages/core/test/utils/cookieCacheBusting.test.ts
Comment on lines
+60
to
+75
| try { | ||
| if ('indexedDB' in window) { | ||
| const { del } = await import('idb-keyval') | ||
| await del('fs::session').catch(() => {}) | ||
| } | ||
|
|
||
| const idb = window.indexedDB | ||
| if (idb) { | ||
| await new Promise<void>((resolve) => { | ||
| const req = idb.deleteDatabase('keyval-store') | ||
| req.onsuccess = () => resolve() | ||
| req.onerror = () => resolve() | ||
| req.onblocked = () => resolve() | ||
| }) | ||
| } | ||
| } catch {} |
There was a problem hiding this comment.
Separate the guarded cleanup steps so DB deletion always runs.
With one outer try, any failure in the first step skips deleteDatabase('keyval-store'), which can leave stale IndexedDB state after logout.
💡 Proposed fix
- try {
- if ('indexedDB' in window) {
- const { del } = await import('idb-keyval')
- await del('fs::session').catch(() => {})
- }
-
- const idb = window.indexedDB
- if (idb) {
- await new Promise<void>((resolve) => {
- const req = idb.deleteDatabase('keyval-store')
- req.onsuccess = () => resolve()
- req.onerror = () => resolve()
- req.onblocked = () => resolve()
- })
- }
- } catch {}
+ if ('indexedDB' in window) {
+ try {
+ const { del } = await import('idb-keyval')
+ await del('fs::session')
+ } catch {}
+
+ try {
+ const idb = window.indexedDB
+ if (idb) {
+ await new Promise<void>((resolve) => {
+ const req = idb.deleteDatabase('keyval-store')
+ req.onsuccess = () => resolve()
+ req.onerror = () => resolve()
+ req.onblocked = () => resolve()
+ })
+ }
+ } catch {}
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| if ('indexedDB' in window) { | |
| const { del } = await import('idb-keyval') | |
| await del('fs::session').catch(() => {}) | |
| } | |
| const idb = window.indexedDB | |
| if (idb) { | |
| await new Promise<void>((resolve) => { | |
| const req = idb.deleteDatabase('keyval-store') | |
| req.onsuccess = () => resolve() | |
| req.onerror = () => resolve() | |
| req.onblocked = () => resolve() | |
| }) | |
| } | |
| } catch {} | |
| if ('indexedDB' in window) { | |
| try { | |
| const { del } = await import('idb-keyval') | |
| await del('fs::session') | |
| } catch {} | |
| try { | |
| const idb = window.indexedDB | |
| if (idb) { | |
| await new Promise<void>((resolve) => { | |
| const req = idb.deleteDatabase('keyval-store') | |
| req.onsuccess = () => resolve() | |
| req.onerror = () => resolve() | |
| req.onblocked = () => resolve() | |
| }) | |
| } | |
| } catch {} | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/core/src/components/account/MyAccountDrawer/OrganizationDrawer/OrganizationDrawer.tsx`
around lines 60 - 75, The outer try is swallowing errors from the first cleanup
step and preventing the subsequent IndexedDB deletion (new Promise deleting
'keyval-store') from running; split the logic into two guarded blocks so each
step runs and failures are handled independently: wrap the idb-keyval import and
await del('fs::session') in its own try/catch (referencing the dynamic import
and del call), then always run the IndexedDB deletion logic (referencing
window.indexedDB, idb.deleteDatabase('keyval-store') and the
onsuccess/onerror/onblocked resolve handlers) in a separate try/catch so that an
error in the first step won’t skip deleting 'keyval-store'.
Comment on lines
+28
to
29
| useReloadAfterLogoutReturn() | ||
| storeConfig.experimental?.scrollRestoration && useScrollRestoration() |
There was a problem hiding this comment.
useScrollRestoration is called conditionally — pre-existing Rules of Hooks violation.
Line 29 (storeConfig.experimental?.scrollRestoration && useScrollRestoration()) conditionally calls a hook, which violates the Rules of Hooks. While this is pre-existing and not introduced by this PR, the new useReloadAfterLogoutReturn() call on line 28 is correctly unconditional. Consider fixing the adjacent violation to avoid subtle bugs during React reconciliation.
🛠️ Suggested fix for the conditional hook call
Move the condition inside the hook itself, or call unconditionally:
- storeConfig.experimental?.scrollRestoration && useScrollRestoration()
+ useScrollRestoration()Then guard inside useScrollRestoration:
// inside useScrollRestoration
if (!storeConfig.experimental?.scrollRestoration) return
// ... rest of the hook🧰 Tools
🪛 Biome (2.4.4)
[error] 29-29: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/pages/_app.tsx` around lines 28 - 29, The conditional call
to the hook useScrollRestoration (via
storeConfig.experimental?.scrollRestoration && useScrollRestoration()) violates
the Rules of Hooks; change the call site to call useScrollRestoration
unconditionally next to useReloadAfterLogoutReturn(), and move the feature flag
check into the hook implementation of useScrollRestoration so the hook returns
early when storeConfig.experimental?.scrollRestoration is falsy. Ensure
useScrollRestoration contains the guard (early return) and any side-effects only
run when the flag is enabled.
| options?: RequestOptions | ||
| ) => { | ||
| // Get cache busting value based on cookie changes | ||
| // Get cache busting value based person?.id from session |
There was a problem hiding this comment.
Typo in comment — missing "on".
- // Get cache busting value based person?.id from session
+ // Get cache busting value based on person?.id from session📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Get cache busting value based person?.id from session | |
| // Get cache busting value based on person?.id from session |
🤖 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 32, Fix the typo in the
comment that reads "Get cache busting value based person?.id from session" by
inserting the missing preposition to read "Get cache busting value based on
person?.id from session" so the comment referencing person?.id and session in
request.ts is grammatically correct.
2eaf5b6 to
648c34a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Bug Fixes
Refactor
Chores