Speed up loading with synchronous auth init and cache optimizations#956
Speed up loading with synchronous auth init and cache optimizations#956vdimarco wants to merge 4 commits into
Conversation
- Moved auth store initialization to synchronous reading from localStorage during store creation. - Removed useEffect in useAuthSync that initialized auth state asynchronously. - Eliminated full-page loading spinner in ChatLayout during auth loading; components handle their own loading. - Added extensive unit tests for synchronous auth store initialization and actions. - This optimization reduces load time for returning users from 10-30s to under 100ms. Co-authored-by: gatewayz-ai-inbox[bot] <gatewayz-ai-inbox[bot]@users.noreply.github.com>
…ata leak When a user logs out and another user logs in on the same browser, the session cache was returning cached sessions from the previous user. This could briefly expose session titles and metadata to the wrong user. Changes: - Scope localStorage cache key by user ID (gatewayz_session_cache_<userId>) - Add userId field to cached data for validation - Validate userId matches current user on cache read - Invalidate memory cache when user mismatch detected - Clear session cache on logout (clearSessionCacheOnLogout) - Add comprehensive tests for user-scoped caching Fixes: Cross-user cache leak identified in PR review 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Changes: - Guard initialData in useChatSessions to only return cached sessions when authenticated or during loading, preventing cross-user exposure during auth transitions - Remove unused authLoading variable in ChatLayout.tsx - Fix lint warning in session-cache.test.ts (forEach callback return value) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…er data exposure The React Query cache for ['chat-sessions'] was not being invalidated on logout, which could temporarily show a new user the previous user's data. Changes: - Add useQueryClient hook to GatewayzAuthProvider - Invalidate chat-sessions and chat-messages queries on logout - Call clearSessionCacheOnLogout before clearing credentials - Update auth-timeout tests to wrap GatewayzAuthProvider with QueryClientProvider 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR optimizes authentication initialization, session caching, and React Query state management. Auth state now synchronously initializes during store creation rather than on component mount. Session caches are scoped per-user with in-memory TTL layers. Chat history syncs are debounced to prevent concurrent requests. React Query defaults extend cache retention times and prevent automatic remount refetches. Cache invalidation patterns simplify to unified keys. Full-page auth-loading spinner is removed from ChatLayout. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Initialization
participant AuthStore as Auth Store
participant API as localStorage/API
participant SessionCache as Session Cache
participant QueryClient as React Query
App->>AuthStore: Create store (synchronous)
AuthStore->>API: getInitialAuthState() reads credentials
API-->>AuthStore: apiKey, userData from localStorage
AuthStore->>SessionCache: getInitialAuthState checks auth
SessionCache-->>AuthStore: Auth state determined
AuthStore->>QueryClient: Initialize with auth state
QueryClient-->>App: Ready (no mount effect delay)
Note over AuthStore,QueryClient: Previous: Effect on mount<br/>Now: Immediate initialization
sequenceDiagram
participant Component as Chat Component
participant useChatSessions as useChatSessions Hook
participant SessionCache as Session Cache
participant QueryClient as React Query
participant API as Backend API
Component->>useChatSessions: Render
useChatSessions->>SessionCache: getCachedSessions()
SessionCache->>SessionCache: Check memory cache (5s TTL)
alt Memory Hit
SessionCache-->>useChatSessions: Cached data
else Memory Miss
SessionCache->>API: Read from localStorage
API-->>SessionCache: Deserialize & validate userId
SessionCache->>SessionCache: Update memory cache
SessionCache-->>useChatSessions: Cached data
end
useChatSessions->>QueryClient: Set initialData, staleTime=5min
useChatSessions-->>Component: Render with cached data
QueryClient->>API: Background sync (debounced, 5s min interval)
API-->>QueryClient: Fresh sessions
QueryClient-->>Component: Update if changed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return useQuery({ | ||
| queryKey: ['chat-sessions', isAuthenticated], | ||
| // OPTIMIZATION: Remove isAuthenticated from queryKey to prevent cache invalidation | ||
| // when auth state changes. The queryFn handles guest vs authenticated logic. | ||
| queryKey: ['chat-sessions'], |
There was a problem hiding this comment.
Guest/auth sessions share cache
useChatSessions now uses a single queryKey: ['chat-sessions'] (after removing isAuthenticated) but the queryFn still returns different datasets depending on auth state (guest local cache vs authenticated sessions). This guarantees cache collisions: e.g. after logout the guest result overwrites ['chat-sessions'], and with refetchOnMount: false + 5m staleTime (src/lib/providers/query-provider.tsx:6-21) a subsequent login can render the guest list from cache until a manual invalidation/refetch. Please re-scope the key (e.g., include user identity/auth state) or otherwise ensure guest/auth results cannot overwrite each other.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/hooks/use-chat-queries.ts
Line: 52:55
Comment:
**Guest/auth sessions share cache**
`useChatSessions` now uses a single `queryKey: ['chat-sessions']` (after removing `isAuthenticated`) but the `queryFn` still returns *different datasets* depending on auth state (guest local cache vs authenticated sessions). This guarantees cache collisions: e.g. after logout the guest result overwrites `['chat-sessions']`, and with `refetchOnMount: false` + 5m `staleTime` (src/lib/providers/query-provider.tsx:6-21) a subsequent login can render the guest list from cache until a manual invalidation/refetch. Please re-scope the key (e.g., include user identity/auth state) or otherwise ensure guest/auth results cannot overwrite each other.
How can I resolve this? If you propose a fix, please make it concise.![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| // OPTIMIZATION: Static properties for sync deduplication | ||
| // Prevents duplicate background sync calls when getSessionsWithCache is called | ||
| // multiple times rapidly (e.g., during React strict mode or fast remounts) | ||
| private static syncInProgress: boolean = false; | ||
| private static lastSyncTime: number = 0; | ||
| private static readonly SYNC_DEBOUNCE_MS = 5000; // 5 seconds between syncs |
There was a problem hiding this comment.
Bug: The ChatHistoryAPI class uses static properties for sync guards, causing concurrent instances to incorrectly block each other's background sync operations, leading to skipped updates.
Severity: MEDIUM
Suggested Fix
Convert the static properties syncInProgress and lastSyncTime to private instance properties (e.g., private syncInProgress: boolean = false;). This will ensure that each ChatHistoryAPI instance manages its own sync state independently, preventing cross-instance interference and allowing sync operations to proceed as intended for each instance.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/lib/chat-history.ts#L105-L110
Potential issue: The `ChatHistoryAPI` class uses static properties `syncInProgress` and
`lastSyncTime` to manage background data synchronization. Because these properties are
static, they are shared across all instances of the class. When multiple instances are
created (e.g., by different hooks for the same user), one instance's sync operation can
incorrectly block another's by setting the shared `syncInProgress` flag to true. This
prevents concurrent syncs, leading to skipped background updates and potential
performance degradation, although it does not cause data corruption as each instance
uses its own credentials for fetching.
Did we get this right? 👍 / 👎 to inform future reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/hooks/use-auth-sync.ts (1)
249-252:⚠️ Potential issue | 🟡 Minor
useAuthStore.getState()in the return bypasses Zustand subscriptions.Calling
useAuthStore.getState()during render reads the store value without subscribing to changes. IfisAuthenticatedchanges after this hook's consumers mount, they won't re-render. The consumers ofuseAuthSync()will only see stale values unless something else causes a re-render.If this is intentional (to avoid render cascades), it's fine but worth a comment. If consumers need reactive updates, use
useAuthStore((s) => s.isAuthenticated)instead, or use the values already destructured at line 65.Suggested fix using already-destructured values or proper selectors
+ const isAuthenticated = useAuthStore((s) => s.isAuthenticated); + return { - isLoading: isLoading && !useAuthStore.getState().isAuthenticated, // Only loading if we don't have auth yet - isAuthenticated: useAuthStore.getState().isAuthenticated + isLoading: isLoading && !isAuthenticated, + isAuthenticated, };
🤖 Fix all issues with AI agents
In `@src/__tests__/context/auth-timeout.test.tsx`:
- Around line 51-58: The QueryClient is being recreated on every render because
createTestQueryClient() is called inside the wrapper component; hoist the
QueryClient creation out of the wrapper so the same client instance is reused
across renders (keep a single const queryClient = createTestQueryClient() at the
test scope) and pass that stable queryClient into the QueryClientProvider inside
wrapper (affecting the wrapper used by renderHook/rerender to avoid resetting
cached queries).
In `@src/lib/chat-history.ts`:
- Around line 688-699: The current code sets ChatHistoryAPI.lastSyncTime only on
successful fetch (inside the .then block), causing shouldSync to always return
true during failures; move the update of ChatHistoryAPI.lastSyncTime into the
.finally() block so the debounce window is applied regardless of outcome, keep
setCachedSessions(sessions) in the .then() so successful responses still update
cache, and ensure ChatHistoryAPI.syncInProgress is cleared after lastSyncTime is
updated in the .finally() to preserve the existing sync lifecycle.
In `@src/lib/session-cache.ts`:
- Around line 136-142: The current ownership guard only rejects cache entries
when data.userId is defined and differs from currentUserId, which lets legacy
entries lacking userId slip through; update the check in the session validation
branch (the block that calls clearSessionCache()) to treat missing userId as a
mismatch — i.e., if data.userId === undefined OR data.userId !== currentUserId
then clearSessionCache() and return []; adjust the condition where data.userId
is evaluated to enforce this stricter ownership policy.
- Around line 38-45: The getCacheKey function uses a falsy check (if (userId))
which treats userId === 0 as a guest; change the check to explicitly test for
presence (e.g., userId !== undefined && userId !== null or typeof userId !==
"undefined") so a legitimate 0 id will produce `${CACHE_KEY_PREFIX}_${userId}`;
update getCacheKey to use that explicit null/undefined check while still falling
back to `${CACHE_KEY_PREFIX}_guest` when no id is present.
In `@src/lib/store/__tests__/auth-store.test.ts`:
- Around line 128-143: The test for clearAuth is missing verification that
clearSessionCacheOnLogout is invoked; add a Jest mock for the
'@/lib/session-cache' module (create mockClearSessionCacheOnLogout = jest.fn()
and have clearSessionCacheOnLogout return/invoke it) alongside the other mocks
at the top of the test file, then in the clearAuth test (which calls
useAuthStore.getState().clearAuth()) add an assertion that
mockClearSessionCacheOnLogout was called (e.g., toHaveBeenCalledTimes(1)) after
the action; reference the clearAuth method on useAuthStore and the
clearSessionCacheOnLogout mock when updating the test.
In `@src/lib/store/auth-store.ts`:
- Around line 25-61: getInitialAuthState() is executed at module load and
returns different server vs client values causing hydration mismatches; change
initialization so the store starts in a neutral "loading" state on both server
and client and only reads localStorage inside a client-only effect: keep
getInitialAuthState/getApiKey/getUserData but set initialState to isLoading:
true/isAuthenticated: false by default (referencing initialState and AuthState),
then in a client-only effect (useEffect or useLayoutEffect) dispatch an action
or call a setter that reads localStorage via getInitialAuthState() to update
apiKey/userData/isAuthenticated/isLoading; update any components like
ChatLayout/GuestChatCounter to tolerate transient loading state (or use
suppressHydrationWarning) until the client-side update completes.
🧹 Nitpick comments (8)
src/lib/chat-history.ts (1)
105-110: Static sync state is shared across allChatHistoryAPIinstances.Since these are
static, they're shared by every instance. If a newChatHistoryAPIis constructed during an auth transition (e.g., user switches accounts, API key changes), the new instance inherits the old debounce/in-progress state and may skip its first sync. For a single-user app with sequential auth flows this is likely fine, but it's worth being aware of.src/lib/providers/query-provider.tsx (1)
12-20: GlobalrefetchOnMount: falsemay cause stale data for new or forgotten queries.The combination of
staleTime: 5min+refetchOnMount: falseas global defaults means any query that doesn't explicitly override these will never refetch on component mount, even after data becomes stale. This heavily depends on every cache-mutation path callinginvalidateQueriescorrectly. If any invalidation path is missed (or a new query is added without overriding), users will silently see stale data.Consider keeping
refetchOnMountat its default (true) globally and settingrefetchOnMount: falseonly on the specific queries that benefit from it (e.g.,chat-sessions). Alternatively, userefetchOnMount: 'always'only where freshness is critical, and leave the global default as-is if the team is confident all invalidation paths are covered.src/lib/hooks/__tests__/use-auth-sync.test.ts (1)
46-52:AUTH_REFRESH_EVENTis mocked but never tested.The mock adds
AUTH_REFRESH_EVENT: 'gatewayz:refresh-auth'(line 51), but no test exercises the event handler that listens for it. Consider adding a test that dispatchesAUTH_REFRESH_EVENTand verifiessetAuthis called with the stored credentials.src/lib/session-cache.ts (1)
110-151: Consider extracting the repeated read-with-validation pattern.
getCachedSessions,getCachedDefaultModel, andgetCachedRecentModelsall follow the same structure: check memory cache → fall back to localStorage → validate expiry → validate userId → update memory cache → extract field. This could be factored into a shared helper likegetValidatedCacheData(): SessionCacheData | nullthat handles the common validation logic, with each getter just extracting its field.Example helper extraction
function getValidatedCacheData(): SessionCacheData | null { const currentUserId = getCurrentUserId(); const memCached = getMemoryCachedData(); if (memCached && isCacheValid(memCached)) { if (memCached.userId === currentUserId) return memCached; invalidateMemoryCache(); } const cached = safeLocalStorageGet(getCacheKey()); if (!cached) return null; const data = JSON.parse(cached) as SessionCacheData; if (!isCacheValid(data)) { clearSessionCache(); return null; } if (data.userId !== undefined && data.userId !== currentUserId) { clearSessionCache(); return null; } setMemoryCachedData(data); return data; }Then each getter becomes:
export function getCachedSessions(): ChatSession[] { try { return getValidatedCacheData()?.sessions || []; } catch (error) { console.warn('[SessionCache] Failed to read cached sessions:', error); return []; } }Also applies to: 156-189, 194-227
src/context/gatewayz-auth-context.tsx (1)
1658-1667: ConsiderremoveQueriesinstead ofinvalidateQueriesfor logout.
invalidateQueriesmarks queries as stale and triggers a background refetch for any mounted observers. After logout, this meansuseChatSessions(if mounted) will refetch and hit thequeryFn, which will return guest sessions — a benign but unnecessary network call (or localStorage read). UsingqueryClient.removeQueries(...)would immediately clear the cached data without triggering a refetch, which is typically what you want on logout.This is not a security issue since
clearSessionCacheOnLogout()is correctly called first, but it's a cleaner pattern for logout flows.♻️ Suggested change
const logout = useCallback(async () => { // Clear session cache BEFORE clearing credentials (while user ID is still available) clearSessionCacheOnLogout(); clearStoredCredentials(); await privyLogout(); // Invalidate React Query cache to prevent stale data showing to next user - queryClient.invalidateQueries({ queryKey: ['chat-sessions'] }); - queryClient.invalidateQueries({ queryKey: ['chat-messages'] }); + queryClient.removeQueries({ queryKey: ['chat-sessions'] }); + queryClient.removeQueries({ queryKey: ['chat-messages'] }); setAuthStatus("unauthenticated", "logout"); }, [clearStoredCredentials, privyLogout, setAuthStatus, queryClient]);src/__tests__/context/auth-timeout.test.tsx (1)
9-32: Missing mock for@/lib/session-cache.
GatewayzAuthProvidernow importsclearSessionCacheOnLogoutfrom@/lib/session-cache. Without mocking this module, tests depend on the real implementation (includinglocalStorageside effects fromgetCacheKey,getCurrentUserId, etc.). Add a mock to keep the test isolated:jest.mock('@/lib/session-cache', () => ({ clearSessionCacheOnLogout: jest.fn(), }));src/lib/hooks/use-chat-queries.ts (2)
69-82: Review theinitialDatafallback for authenticated users with empty cache.The fallback to
getGuestSessions()on line 79 means that an authenticated user with no session cache will briefly see guest sessions (if any exist) as initial data before the server response arrives. This may cause a brief flash of guest data in the sidebar for users who chatted as guests then logged in.If this is intentional (showing continuity of guest sessions), it's fine. If not, you could guard the fallback:
initialData: () => { const cached = getCachedSessions(); if (cached.length > 0 && (isAuthenticated || isLoading)) return cached; - return getGuestSessions(); + // Only show guest sessions for unauthenticated users + if (!isAuthenticated && !isLoading) return getGuestSessions(); + return undefined; },
202-209: Redundant invalidation beforesetQueryData.
invalidateQuerieson line 204 marks the query as stale and schedules a background refetch, whilesetQueryDataon line 206 immediately updates the cache. The refetch triggered byinvalidateQuerieswill overwrite thesetQueryDataresult when it completes. If the intent is an optimistic update, you typically only needsetQueryData. If the intent is to ensure server consistency, onlyinvalidateQueriessuffices.Using both together means the optimistic update is immediately visible but then replaced by server data — which is a valid pattern if you expect the server response may differ (e.g., title normalization). If that's intentional, consider adding a brief comment clarifying the intent.
| const wrapper = ({ children }: { children: ReactNode }) => { | ||
| const queryClient = createTestQueryClient(); | ||
| return ( | ||
| <QueryClientProvider client={queryClient}> | ||
| <GatewayzAuthProvider>{children}</GatewayzAuthProvider> | ||
| </QueryClientProvider> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
QueryClient is recreated on every re-render, which will reset query state mid-test.
createTestQueryClient() is called inside the wrapper function body. Since renderHook treats this as a React component, every re-render (including rerender() at line 459) creates a brand-new QueryClient, discarding all cached queries. This can cause subtle test flakiness.
Hoist the client creation to be stable across renders:
🔧 Proposed fix
- const wrapper = ({ children }: { children: ReactNode }) => {
- const queryClient = createTestQueryClient();
- return (
- <QueryClientProvider client={queryClient}>
- <GatewayzAuthProvider>{children}</GatewayzAuthProvider>
- </QueryClientProvider>
- );
- };
+ let testQueryClient: QueryClient;
+
+ beforeEach(() => {
+ // ... existing beforeEach setup ...
+ testQueryClient = createTestQueryClient();
+ });
+
+ const wrapper = ({ children }: { children: ReactNode }) => (
+ <QueryClientProvider client={testQueryClient}>
+ <GatewayzAuthProvider>{children}</GatewayzAuthProvider>
+ </QueryClientProvider>
+ );🤖 Prompt for AI Agents
In `@src/__tests__/context/auth-timeout.test.tsx` around lines 51 - 58, The
QueryClient is being recreated on every render because createTestQueryClient()
is called inside the wrapper component; hoist the QueryClient creation out of
the wrapper so the same client instance is reused across renders (keep a single
const queryClient = createTestQueryClient() at the test scope) and pass that
stable queryClient into the QueryClientProvider inside wrapper (affecting the
wrapper used by renderHook/rerender to avoid resetting cached queries).
| .then(sessions => { | ||
| // Update cache with fresh data | ||
| setCachedSessions(sessions); | ||
| ChatHistoryAPI.lastSyncTime = Date.now(); | ||
| }) | ||
| .catch(error => { | ||
| console.warn('[ChatHistoryAPI] Background sync failed after retries:', error); | ||
| // Silently fail - we already have cached data to show | ||
| }) | ||
| .finally(() => { | ||
| ChatHistoryAPI.syncInProgress = false; | ||
| }); |
There was a problem hiding this comment.
lastSyncTime not updated on failure — debounce bypassed during outages.
When the background sync fails, lastSyncTime remains stale, so the shouldSync check on Line 657 passes on every subsequent call. During a backend outage with frequent component remounts, this turns into unbounded retries (each with maxRetries: 2 internally), negating the debounce protection.
Move the lastSyncTime update into .finally() so the debounce window applies regardless of outcome:
Proposed fix
.then(sessions => {
// Update cache with fresh data
setCachedSessions(sessions);
- ChatHistoryAPI.lastSyncTime = Date.now();
})
.catch(error => {
console.warn('[ChatHistoryAPI] Background sync failed after retries:', error);
// Silently fail - we already have cached data to show
})
.finally(() => {
+ ChatHistoryAPI.lastSyncTime = Date.now();
ChatHistoryAPI.syncInProgress = false;
});📝 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.
| .then(sessions => { | |
| // Update cache with fresh data | |
| setCachedSessions(sessions); | |
| ChatHistoryAPI.lastSyncTime = Date.now(); | |
| }) | |
| .catch(error => { | |
| console.warn('[ChatHistoryAPI] Background sync failed after retries:', error); | |
| // Silently fail - we already have cached data to show | |
| }) | |
| .finally(() => { | |
| ChatHistoryAPI.syncInProgress = false; | |
| }); | |
| .then(sessions => { | |
| // Update cache with fresh data | |
| setCachedSessions(sessions); | |
| }) | |
| .catch(error => { | |
| console.warn('[ChatHistoryAPI] Background sync failed after retries:', error); | |
| // Silently fail - we already have cached data to show | |
| }) | |
| .finally(() => { | |
| ChatHistoryAPI.lastSyncTime = Date.now(); | |
| ChatHistoryAPI.syncInProgress = false; | |
| }); |
🤖 Prompt for AI Agents
In `@src/lib/chat-history.ts` around lines 688 - 699, The current code sets
ChatHistoryAPI.lastSyncTime only on successful fetch (inside the .then block),
causing shouldSync to always return true during failures; move the update of
ChatHistoryAPI.lastSyncTime into the .finally() block so the debounce window is
applied regardless of outcome, keep setCachedSessions(sessions) in the .then()
so successful responses still update cache, and ensure
ChatHistoryAPI.syncInProgress is cleared after lastSyncTime is updated in the
.finally() to preserve the existing sync lifecycle.
| function getCacheKey(): string { | ||
| const userId = getCurrentUserId(); | ||
| if (userId) { | ||
| return `${CACHE_KEY_PREFIX}_${userId}`; | ||
| } | ||
| // Guest users don't have sessions to cache, but return a key for consistency | ||
| return `${CACHE_KEY_PREFIX}_guest`; | ||
| } |
There was a problem hiding this comment.
userId === 0 would be treated as a guest user.
if (userId) on line 40 is falsy for 0. If any user could have user_id: 0 (e.g., a system/default user), their cache would be stored under the guest key. In practice, database-assigned IDs are positive integers, so this is unlikely to be an issue, but a strict check would be safer.
Suggested stricter check
function getCacheKey(): string {
const userId = getCurrentUserId();
- if (userId) {
+ if (userId !== undefined && userId !== null) {
return `${CACHE_KEY_PREFIX}_${userId}`;
}🤖 Prompt for AI Agents
In `@src/lib/session-cache.ts` around lines 38 - 45, The getCacheKey function uses
a falsy check (if (userId)) which treats userId === 0 as a guest; change the
check to explicitly test for presence (e.g., userId !== undefined && userId !==
null or typeof userId !== "undefined") so a legitimate 0 id will produce
`${CACHE_KEY_PREFIX}_${userId}`; update getCacheKey to use that explicit
null/undefined check while still falling back to `${CACHE_KEY_PREFIX}_guest`
when no id is present.
|
|
||
| // Double-check userId matches current user (defense in depth) | ||
| if (data.userId !== undefined && data.userId !== currentUserId) { | ||
| // Cache belongs to different user - clear and return empty | ||
| clearSessionCache(); | ||
| return []; | ||
| } |
There was a problem hiding this comment.
Legacy cache entries without userId bypass the ownership check.
The guard data.userId !== undefined means old cache entries that predate the per-user scoping (and lack the userId field) will be served to any user who happens to read from the legacy key path. While getCacheKey() routes different users to different keys, there's a brief migration window where a user-scoped key could have been written by old code without userId set.
This is a low-risk issue since the old code didn't scope by user at all and the TTL will naturally expire old entries, but for defense-in-depth, consider treating data.userId === undefined as a cache miss:
Stricter ownership check
- if (data.userId !== undefined && data.userId !== currentUserId) {
+ if (data.userId === undefined || data.userId !== currentUserId) {
// Cache belongs to different user - clear and return empty
clearSessionCache();
return [];
}🤖 Prompt for AI Agents
In `@src/lib/session-cache.ts` around lines 136 - 142, The current ownership guard
only rejects cache entries when data.userId is defined and differs from
currentUserId, which lets legacy entries lacking userId slip through; update the
check in the session validation branch (the block that calls
clearSessionCache()) to treat missing userId as a mismatch — i.e., if
data.userId === undefined OR data.userId !== currentUserId then
clearSessionCache() and return []; adjust the condition where data.userId is
evaluated to enforce this stricter ownership policy.
| describe('clearAuth action', () => { | ||
| it('should clear authenticated state correctly', () => { | ||
| // Arrange: Set up authenticated state first | ||
| const { useAuthStore } = require('../auth-store'); | ||
| useAuthStore.getState().setAuth('some-key', { user_id: 1 } as any); | ||
|
|
||
| // Act | ||
| useAuthStore.getState().clearAuth(); | ||
| const state = useAuthStore.getState(); | ||
|
|
||
| // Assert | ||
| expect(state.apiKey).toBeNull(); | ||
| expect(state.userData).toBeNull(); | ||
| expect(state.isAuthenticated).toBe(false); | ||
| expect(state.isLoading).toBe(false); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Verify that clearSessionCacheOnLogout is called during clearAuth.
Per the store implementation (auth-store.ts line 70-72), clearAuth calls clearSessionCacheOnLogout() before resetting state. This is a security-critical behavior (prevents cross-user data leakage), but the test doesn't verify it. Mock @/lib/session-cache and assert the call:
🔧 Proposed enhancement
Add at the top of the file with the other mocks:
const mockClearSessionCacheOnLogout = jest.fn();
jest.mock('@/lib/session-cache', () => ({
clearSessionCacheOnLogout: () => mockClearSessionCacheOnLogout(),
}));Then update the clearAuth test:
it('should clear authenticated state correctly', () => {
// Arrange: Set up authenticated state first
const { useAuthStore } = require('../auth-store');
useAuthStore.getState().setAuth('some-key', { user_id: 1 } as any);
// Act
useAuthStore.getState().clearAuth();
const state = useAuthStore.getState();
// Assert
expect(state.apiKey).toBeNull();
expect(state.userData).toBeNull();
expect(state.isAuthenticated).toBe(false);
expect(state.isLoading).toBe(false);
+ // Verify session cache is cleared on logout (prevents cross-user data leakage)
+ expect(mockClearSessionCacheOnLogout).toHaveBeenCalled();
});🤖 Prompt for AI Agents
In `@src/lib/store/__tests__/auth-store.test.ts` around lines 128 - 143, The test
for clearAuth is missing verification that clearSessionCacheOnLogout is invoked;
add a Jest mock for the '@/lib/session-cache' module (create
mockClearSessionCacheOnLogout = jest.fn() and have clearSessionCacheOnLogout
return/invoke it) alongside the other mocks at the top of the test file, then in
the clearAuth test (which calls useAuthStore.getState().clearAuth()) add an
assertion that mockClearSessionCacheOnLogout was called (e.g.,
toHaveBeenCalledTimes(1)) after the action; reference the clearAuth method on
useAuthStore and the clearSessionCacheOnLogout mock when updating the test.
| const getInitialAuthState = (): Pick<AuthState, 'apiKey' | 'userData' | 'isAuthenticated' | 'isLoading'> => { | ||
| // During SSR, return loading state to avoid hydration mismatch | ||
| if (typeof window === 'undefined') { | ||
| return { | ||
| apiKey: null, | ||
| userData: null, | ||
| isAuthenticated: false, | ||
| isLoading: true, | ||
| }; | ||
| } | ||
|
|
||
| // Synchronously read cached credentials from localStorage | ||
| const apiKey = getApiKey(); | ||
| const userData = getUserData(); | ||
|
|
||
| if (apiKey && userData) { | ||
| // Cached credentials found - user is authenticated, not loading | ||
| return { | ||
| apiKey, | ||
| userData, | ||
| isAuthenticated: true, | ||
| isLoading: false, | ||
| }; | ||
| } | ||
|
|
||
| // No cached credentials - guest user, also not loading | ||
| // CRITICAL: Set isLoading to false so guest users see welcome screen immediately | ||
| return { | ||
| apiKey: null, | ||
| userData: null, | ||
| isAuthenticated: false, | ||
| isLoading: false, | ||
| }; | ||
| }; | ||
|
|
||
| // Initialize auth state synchronously from localStorage | ||
| const initialState = getInitialAuthState(); |
There was a problem hiding this comment.
Potential SSR/client hydration mismatch.
getInitialAuthState() runs at module scope (line 61) and returns different values on server (isLoading: true, isAuthenticated: false) vs. client (isLoading: false, isAuthenticated: true for returning users). Any component that branches its rendered output based on isAuthenticated or isLoading from this store will produce different HTML on the server vs. client initial render, triggering React hydration warnings.
For example, ChatLayout conditionally renders <GuestChatCounter> based on !isAuthenticated — this will appear in server HTML but be absent in client hydration for authenticated users.
This can be mitigated by deferring the auth-derived UI differences to a useEffect or using suppressHydrationWarning on affected elements. Alternatively, start with isLoading: true on the client as well, then synchronously update in a layout effect.
#!/bin/bash
# Check which components branch rendering based on isAuthenticated or isLoading from auth store
rg -nP --type=ts --type-add='tsx:*.tsx' --type=tsx 'isAuthenticated|isLoading.*auth|storeLoading' -g '!**/*.test.*' -g '!**/node_modules/**' | head -40🤖 Prompt for AI Agents
In `@src/lib/store/auth-store.ts` around lines 25 - 61, getInitialAuthState() is
executed at module load and returns different server vs client values causing
hydration mismatches; change initialization so the store starts in a neutral
"loading" state on both server and client and only reads localStorage inside a
client-only effect: keep getInitialAuthState/getApiKey/getUserData but set
initialState to isLoading: true/isAuthenticated: false by default (referencing
initialState and AuthState), then in a client-only effect (useEffect or
useLayoutEffect) dispatch an action or call a setter that reads localStorage via
getInitialAuthState() to update apiKey/userData/isAuthenticated/isLoading;
update any components like ChatLayout/GuestChatCounter to tolerate transient
loading state (or use suppressHydrationWarning) until the client-side update
completes.
1 participant
Summary
Significant performance improvements by eliminating an initialization effect, making auth state sync happen synchronously, and adding per-user memoized session caching. Also introduced smarter background session syncing to avoid duplicate calls. UI now renders immediately for returning guests and authenticated users without blocking full-page load.
Changes
Core functionality
Caching improvements (per-user, memoized)
Background sync optimization
Hooks, queries, and provider optimizations
UI and layout
Tests and tests structure
Test plan
Breaking changes / notes
Why this matters
If you want any adjustments to the testing approach or further onboarding docs, let me know and I’ll add them.
🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://terragon-www-production.up.railway.app/task/c55c4b87-2976-4309-8ca9-87209f46afb5
Summary by CodeRabbit
Release Notes
Performance Improvements
Security & Session Management
Greptile Overview
Greptile Summary
This PR speeds up initial render by moving auth initialization into synchronous Zustand store creation, removing the mount-time localStorage effect from
useAuthSync, and optimizing React Query defaults to prefer cached data (longer staleTime, no refetch-on-mount). It also introduces a per-user session cache (localStorage keys scoped by userId plus an in-memory TTL layer) and adds logout-time cache clearing + query invalidation to avoid cross-user data exposure. Background session syncing in chat history is debounced to avoid duplicate refresh calls.Main issue to address before merge:
useChatSessionsnow uses a single['chat-sessions']queryKey while still returning different datasets depending on auth state (guest vs authenticated). With the new React Query defaults (no refetch on mount + longer staleTime), this can make guest results overwrite authenticated results (or vice versa) and persist until an explicit invalidation/refetch.Confidence Score: 3/5
chat-sessionskey while returning different data per auth state will cause deterministic cache collisions under the new long staleTime / no-refetch-on-mount defaults.Important Files Changed
Sequence Diagram
sequenceDiagram participant UI as ChatLayout participant AuthStore as auth-store (Zustand) participant AuthSync as useAuthSync participant RQ as React Query participant Sessions as useChatSessions participant Cache as session-cache UI->>AuthStore: create store (sync init from local storage) UI->>AuthSync: register auth refresh listeners UI->>Sessions: mount sessions query (key = ['chat-sessions']) Sessions->>RQ: read cached data for key alt Guest mode Sessions->>Cache: getGuestSessions() Sessions->>RQ: write guest sessions into key else Authenticated Sessions->>Cache: getCachedSessions() (user-scoped) Sessions->>RQ: write user sessions into key end Note over Sessions,RQ: Same key used for different datasets Note over RQ,UI: With refetch-on-mount disabled + long staleTime, wrong dataset can persistContext used:
dashboard- CLAUDE.md (source)