Skip to content

fix: enhance cache key management in useQuery#3229

Closed
eduardoformiga wants to merge 13 commits intodevfrom
fix/SFS-3037-cache-busting-use-query
Closed

fix: enhance cache key management in useQuery#3229
eduardoformiga wants to merge 13 commits intodevfrom
fix/SFS-3037-cache-busting-use-query

Conversation

@eduardoformiga
Copy link
Member

@eduardoformiga eduardoformiga commented Feb 24, 2026

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?

  • 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.

Problem
The SWR cache key in useQuery was only operationName + variables. Because ProductShelf variables (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

  • PR title and commit messages follow the Conventional Commits specification
    • Available prefixes: feat, fix, chore, docs, style, refactor, ci and test

PR Description

  • Added a label according to the PR goal - breaking change, bug, contributing, performance, documentation..

Dependencies

  • Committed the pnpm-lock.yaml file when there were changes to the packages

Documentation

  • PR description
  • For documentation changes, ping @Mariana-Caetano to review and update (Or submit a doc request)

Summary by CodeRabbit

  • New Features

    • App now forces a full reload when you return after logging out to refresh session-dependent data and ensure up-to-date state.
  • Chores

    • Cache keys are now session-aware, separating logged-in and anonymous caches to avoid cross-session leakage.
    • Added runtime diagnostic logging to help troubleshoot request and cache-busting behavior.

- 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.
@eduardoformiga eduardoformiga requested a review from a team as a code owner February 24, 2026 21:21
@eduardoformiga eduardoformiga requested review from lucasfp13 and ommeirelles and removed request for a team February 24, 2026 21:21
@coderabbitai
Copy link

coderabbitai bot commented Feb 24, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds 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

Cohort / File(s) Summary
Session-aware SWR Cache
packages/core/src/sdk/graphql/useQuery.ts
Introduces a private getSessionCacheKeySuffix() that reads getClientCacheBustingValue() and appends a session-specific suffix to SWR keys (unless doNotRun), separating anonymous vs authenticated cache entries.
Logout reload hook & integration
packages/core/src/components/account/MyAccountDrawer/OrganizationDrawer/useReloadAfterLogoutReturn.ts, packages/core/src/components/account/MyAccountDrawer/OrganizationDrawer/OrganizationDrawer.tsx, packages/core/src/pages/_app.tsx
New module exports RELOAD_AFTER_LOGOUT_KEY, setReloadAfterLogoutReturn(), and useReloadAfterLogoutReturn() hook. OrganizationDrawer invokes setReloadAfterLogoutReturn() before logout; _app.tsx initializes the hook to listen for pageshow and trigger window.location.reload() when flag present.
Debug logging additions
packages/core/src/sdk/graphql/request.ts, packages/core/src/utils/cookieCacheBusting.ts
Added console logging to surface cache-busting and cookie diagnostics; no change to functional control flow or public APIs.

Sequence Diagram

sequenceDiagram
    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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • chore: version 2026 21 1144 #3182 — modifies logout/session handling and OrganizationDrawer logout flow; directly related to these reload and session cookie changes.

Suggested reviewers

  • ommeirelles
  • renatomaurovtex

Poem

✨ Keys split by session, cache doors softly part,
A flag tucked in sessionStorage plays its quiet part.
Pageshow listens, and if the note is found,
The app reloads, fresh state newly crowned.
Small signals, tidy flow — the user greets a clearer start.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: enhancing cache key management in useQuery by adding session-aware cache suffixes to prevent stale query results across login/logout transitions.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/SFS-3037-cache-busting-use-query

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 24, 2026

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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8318044 and f356de7.

📒 Files selected for processing (1)
  • packages/core/src/sdk/graphql/useQuery.ts

Comment on lines +45 to +50
() => {
if (options?.doNotRun) return null
const baseKey = getKey(operation['__meta__']['operationName'], variables)
const sessionSuffix = getSessionCacheKeySuffix()
return `${baseKey}::${sessionSuffix}`
},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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 f

Repository: 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" packages

Repository: 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" packages

Repository: 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/sdk

Repository: 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" packages

Repository: vtex/faststore

Length of output: 892


🏁 Script executed:

# Search for SWRConfig and fallback/prefetch patterns
rg -n --type=ts "SWRConfig|fallback|prefetch" packages

Repository: 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.ts

Repository: vtex/faststore

Length of output: 1884


🏁 Script executed:

# Search for getKey usage across SDK
rg -n "getKey\(" packages/core/src/sdk

Repository: vtex/faststore

Length of output: 733


🏁 Script executed:

# Check prefetchQuery.ts implementation
cat ./packages/core/src/sdk/graphql/prefetchQuery.ts

Repository: vtex/faststore

Length of output: 635


🏁 Script executed:

# Check useLazyQuery.ts implementation
cat ./packages/core/src/sdk/graphql/useLazyQuery.ts

Repository: vtex/faststore

Length of output: 854


🏁 Script executed:

# Check usePageProductsQuery.ts for cache key usage
cat ./packages/core/src/sdk/product/usePageProductsQuery.ts

Repository: 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.
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟡 Minor

Change return type from string | null to string.

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' in useQuery.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: nowSpy is 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.

getAnonymousSessionValue and storeAnonymousSessionValue use 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-tab sessionStorage isolation 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 with localStorage.

If cross-tab cache sharing for anonymous sessions is desirable in the future, localStorage would 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 the request() 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 the BaseRequestOptions.value?: string | null contract for any direct callers of baseRequest, 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

📥 Commits

Reviewing files that changed from the base of the PR and between f356de7 and 24f28b6.

📒 Files selected for processing (3)
  • packages/core/src/sdk/graphql/request.ts
  • packages/core/src/utils/cookieCacheBusting.ts
  • packages/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.
@eduardoformiga
Copy link
Member Author

superseded by

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant