Repository navigation
Fix desktop app stuck in skeleton loading state - #453
Conversation
The desktop app was getting stuck showing an infinite skeleton loader when loading pages. Root cause: auth endpoints used by desktop (mobile login, device refresh, desktop OAuth exchange, Google OAuth exchange) were not setting session cookies. The Next.js middleware requires a session cookie for page route requests, so without one, page loads would fail silently and the SWR fetch would hang without surfacing an error. Changes: - Set session cookie in mobile login endpoint when platform is 'desktop' - Set session cookie in device refresh endpoint when platform is 'desktop' - Set session cookie in desktop OAuth exchange endpoint - Set session cookie in Google OAuth exchange when platform is 'desktop' - Add credentials: 'include' to desktop login fetch for cookie handling - Add 15s fetch timeout and SWR error retry config to usePageTree - Add error state UI with retry button in CenterPanel - Add loading timeout (12s) with "Taking longer than expected" hint https://claude.ai/code/session_011dazNWCw89QmzeT8QGejAB
… IPC On desktop, loadSession() retrieves the session token via Electron IPC (reading encrypted file from disk), but this doesn't populate the auth-fetch module's session cache. When usePageTree fires shortly after and calls fetchWithAuth, getSessionFromElectron() does another IPC call to read the same token again. This adds 50-500ms+ of latency to the first page tree fetch. Add warmSessionCache() to AuthFetch that pre-populates the 5-second session cache. Call it from loadSession() after successfully retrieving the desktop session token (both direct read and post-refresh paths). This ensures fetchWithAuth gets a cache hit instead of a redundant IPC. https://claude.ai/code/session_011dazNWCw89QmzeT8QGejAB
The desktop app creates the BrowserWindow with `show: false` and defers showing until `ready-to-show`. During this hidden phase, Chromium's default backgroundThrottling throttles MessageChannel and rAF, which React 18's scheduler relies on for flushing state updates. When SWR data arrives during this throttled window, React queues the re-render but the scheduler never fires it. The skeleton persists even though data is in the cache. Clicking sometimes fixes it because discrete events force React to synchronously flush pending updates. Setting backgroundThrottling: false ensures timers and scheduling work at full speed regardless of window visibility state. This is appropriate for a primary desktop app window. https://claude.ai/code/session_011dazNWCw89QmzeT8QGejAB
The service worker had two issues causing problems for the desktop app: 1. RSC flight data requests (same URL, different Accept header) fell through to the cache-first fallthrough at line 78, serving stale component trees after web deploys. Fixed by detecting RSC/prefetch headers and bypassing the cache entirely for those requests. 2. The "everything else" fallthrough used cache-first, which could serve stale dynamic content. Changed to network-first. 3. In Electron, the persistent profile means the SW cache persists across web deployments indefinitely. Since offline support provides no value for a remote-only desktop app, skip SW registration entirely in Electron and clean up any existing SW registrations and caches. Also bumped cache version to v2 to force cleanup of stale v1 caches on next SW activation for web users. https://claude.ai/code/session_011dazNWCw89QmzeT8QGejAB
Cherry-picked from codex/fix-desktop-sw-chunk-cache (2ec4405). Resolved conflict with our SW fix (a386ce2) — kept both _next/ bypass and RSC header detection as defense-in-depth. https://claude.ai/code/session_011dazNWCw89QmzeT8QGejAB
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds desktop-specific session-cookie propagation and BrowserWindow startup tweaks; revamps service worker caching and registration for Electron vs web; introduces page-tree fetch timeouts, isValidating + retry controls with UI fallbacks; implements one-time desktop tab-restore and auth cache-warming/cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant DesktopApp as Desktop App (main)
participant BrowserWin as BrowserWindow (renderer)
participant AuthServer as Web API (auth exchange)
participant CookieStore as Electron Cookie Store
DesktopApp->>BrowserWin: open OAuth window (backgroundThrottling: false)
BrowserWin->>AuthServer: navigate to OAuth provider / callback
AuthServer-->>BrowserWin: redirect with auth code -> exchange
BrowserWin->>AuthServer: POST /api/auth/desktop/exchange (code)
AuthServer-->>BrowserWin: 200 + body + Set-Cookie (createSessionCookie)
BrowserWin->>DesktopApp: signal success (sessionToken)
DesktopApp->>CookieStore: session.defaultSession.cookies.set(cookie with sessionToken)
DesktopApp->>BrowserWin: show main window (ready-to-show)
DesktopApp->>BrowserWin: webContents.setBackgroundThrottling(true)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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 |
P1: CenterPanel error screen now only shows when no stale data exists. SWR keeps `data` populated during failed revalidations, so the previous `isError && !isValidating` check would blank the page even when cached tree data was available to display. P2: Logout now clears persisted tab state via closeAllTabs(), preventing desktop startup from restoring stale or inaccessible routes from a previous session or different user. P3: Removed `revalidateOnFocus: false` from usePageTree SWR config. Focus revalidation keeps breadcrumbs and tree metadata fresh after app focus/tab switches. The editing guard (isPaused) already prevents revalidation during active document editing. Test: Updated usePageTree fetchAndMergeChildren assertion to expect AbortSignal option from the timeout-enabled fetcher. https://claude.ai/code/session_011dazNWCw89QmzeT8QGejAB
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/stores/useAuthStore.ts (1)
559-566:⚠️ Potential issue | 🟡 MinorNote:
localStoragevia Zustand.The
partializefunction persistsuser.emailtolocalStorage. Depending on your GDPR/CCPA posture, storing PII in unencrypted browser storage may warrant a review. This isn't introduced by this PR, so flagging for awareness only.
🤖 Fix all issues with AI agents
In `@apps/web/src/components/layout/middle-content/CenterPanel.tsx`:
- Around line 41-48: The timeout overlay's state isn't reset when retrying
because clicking the Retry button calls retry while isLoading stays true; update
the retry flow by adding setLoadingTimedOut(false) at the start of the retry
handler (e.g., inside handleRetry) so loadingTimedOut is cleared when retry
begins, then use handleRetry (not retry) for both Retry button onClick handlers
so the overlay/skeleton is briefly shown as feedback; keep existing retry
invocation after resetting loadingTimedOut.
🧹 Nitpick comments (10)
apps/web/src/app/api/auth/device/refresh/route.ts (1)
195-196: Pre-existing: web path lacks null guard forsessionClaims.The mobile/desktop path (lines 220-224) correctly returns a 500 if
sessionClaimsis null, but the web path here silently falls through withsessionClaims?.sessionId ?? '', generating an unvalidatable CSRF token. Not introduced by this PR, but since you're already in this file, it may be worth aligning the two paths.Suggested alignment
const sessionClaims = await sessionService.validateSession(sessionToken); + if (!sessionClaims) { + loggers.auth.error('Failed to validate newly created session during web device refresh'); + return Response.json({ error: 'Failed to generate session.' }, { status: 500 }); + } - const csrfToken = generateCSRFToken(sessionClaims?.sessionId ?? ''); + const csrfToken = generateCSRFToken(sessionClaims.sessionId);apps/web/src/hooks/__tests__/useTabSync.test.ts (2)
112-129: Good coverage of the desktop bootstrap restore path.The test correctly simulates the Electron environment and verifies that
router.replaceis called with the persisted active tab path. Two edge cases that would strengthen confidence:
- Restore is one-shot: re-render or re-trigger the effect and assert
mockRouterReplacewas called exactly once (validatesdidAttemptDesktopRestoreguard).- No restore when pathname ≠
/dashboard: e.g., deep-link launch should skip the restore branch entirely.These are non-blocking but would protect against regressions in the guard logic.
44-47: Minor:mockResetis redundant beforeclearAllMocks.
vi.clearAllMocks()on line 47 already clears call history and return values for every mock. The explicitmockRouterReplace.mockReset()on line 45 adds no extra effect here unless you specifically need to reset a custom implementation (which doesn't exist for this mock). Harmless, but removing it reduces noise.apps/web/src/hooks/useTabSync.ts (2)
27-50: Stalestatesnapshot after healing can confuse future readers.After healing on line 32, the
statecaptured on line 27 still holds the oldactiveTabId. The desktop-restore block correctly re-reads viauseTabsStore.getState()(line 39), but the later code at lines 63-73 still references the original stalestate. It works becausenavigateInActiveTabinternally reads the current store, but the inconsistency is subtle.Consider re-reading state after the desktop-restore block so the rest of the function always operates on the latest snapshot:
♻️ Suggested simplification
// Desktop bootstrap: ... if (isDesktop && !didAttemptDesktopRestore.current && pathname === '/dashboard' && hasTabs) { - const refreshedState = useTabsStore.getState(); - const activeTab = selectActiveTab(refreshedState); + const activeTab = selectActiveTab(useTabsStore.getState()); const restorePath = activeTab?.path; didAttemptDesktopRestore.current = true; @@ ... } // Skip if we already synced this path if (lastSyncedPath.current === pathname) return; + // Re-read after potential healing / restore + const currentState = useTabsStore.getState(); + // If no tabs exist, create one from current path - if (state.tabs.length === 0) { - state.createTab({ path: pathname }); + if (currentState.tabs.length === 0) { + currentState.createTab({ path: pathname }); lastSyncedPath.current = pathname; return; } // Get active tab's current path - const activeTab = selectActiveTab(state); + const activeTab = selectActiveTab(currentState);
37-37:typeof window !== 'undefined'check is unnecessary in a"use client"component'suseEffect.
useEffectonly runs on the client, sowindowis always defined. The check is harmless but adds dead code to every effect invocation.apps/web/public/sw.js (1)
69-78: Barefetch(request)for RSC requests has no offline/error fallback.The RSC header bypass is correct — these responses are header-dependent and must never be served from cache. However, if the network fetch fails (offline, timeout, etc.),
event.respondWith(fetch(request))will reject with no fallback, surfacing as an opaque network error to the page.Consider wrapping in a minimal catch so the browser gets a well-formed error response instead of a rejected promise:
💡 Suggested: graceful fallback for RSC fetch failures
if (request.headers.get('rsc') || request.headers.get('next-router-prefetch') || request.headers.get('next-router-state-tree') || request.headers.get('next-url')) { - event.respondWith(fetch(request)); + event.respondWith( + fetch(request).catch(() => + new Response('', { status: 503, statusText: 'Service Unavailable' }) + ) + ); return; }apps/desktop/src/main/index.ts (1)
194-194:backgroundThrottling: falseapplies for the window's entire lifetime, not just startup.This prevents Chromium from throttling timers/rAF when the window is hidden, which fixes the startup race. However, it also means the app will never be throttled when minimized or hidden to tray (Line 253–256), potentially increasing CPU and battery usage in the background.
If startup is the only concern, consider re-enabling throttling after the first render completes (e.g., via
mainWindow.webContents.setBackgroundThrottling(true)insideready-to-showor after a renderer IPC signal). This would give you the best of both worlds.💡 Example: re-enable throttling after startup
mainWindow.once('ready-to-show', () => { mainWindow?.show(); + // Re-enable background throttling now that the initial render is complete + mainWindow?.webContents.setBackgroundThrottling(true); });apps/web/src/components/layout/middle-content/CenterPanel.tsx (1)
71-77: Consider using shadcn/uiButtoninstead of raw<button>elements.The coding guidelines specify using shadcn/ui components for UI. These inline-styled buttons could be replaced with the
Buttoncomponent for consistency with the rest of the app (variants, focus rings, accessibility attributes, etc.).Example for the error-state button
+import { Button } from '@/components/ui/button'; ... - <button - onClick={retry} - className="inline-flex items-center gap-2 px-4 py-2 text-sm font-medium rounded-md bg-primary text-primary-foreground hover:bg-primary/90 transition-colors" - > - <RefreshCw className="h-4 w-4" /> - Try again - </button> + <Button onClick={retry} size="sm"> + <RefreshCw className="h-4 w-4" /> + Try again + </Button>As per coding guidelines: "Use Tailwind CSS with shadcn/ui components for all UI styling and components."
Also applies to: 90-96
apps/web/src/hooks/usePageTree.ts (1)
159-164:retryduplicatesinvalidateTreeminus the editing guard — intentional?
retryandinvalidateTree(lines 113-126) share the samecache.delete(swrKey); mutate()core, butinvalidateTreeadds anisAnyEditing()guard. Sinceretryis user-initiated (click), bypassing the guard makes sense. A small comment clarifying the distinction (or extracting the shared logic) would help future readers, but not blocking.apps/web/src/hooks/__tests__/usePageTree.test.ts (1)
52-64: MissingisValidatingin SWR mock and no tests for the newretryfunction.The SWR mock doesn't return
isValidating, so it'll beundefinedin all tests. While no current test accesses it, this leaves the two new public APIs (retryandisValidating) — which CenterPanel directly relies on for error/loading UX — without any test coverage.Consider adding:
isValidating: falseto the mock return (line 56-ish).- A
describe('retry')block verifying it callscache.delete+mutate(similar to theinvalidateTreetests).
closeAllTabs() preserves pinned tabs by design, but logout must clear all session-specific state. A surviving pinned tab pointing at a previous user's drive would be auto-restored by useTabSync on the next desktop startup, navigating to an inaccessible route. Use setState directly to clear tabs and activeTabId, which the persist middleware writes through to localStorage. https://claude.ai/code/session_011dazNWCw89QmzeT8QGejAB
- CenterPanel: add handleRetry wrapper to reset loadingTimedOut on retry, use shadcn Button component instead of raw button elements - SW: add 503 fallback for RSC fetch failures (offline/timeout resilience) - Electron: re-enable backgroundThrottling after ready-to-show event - useTabSync: remove stale state snapshot, use fresh getState() after healing/restore, remove unnecessary typeof window check, clean up redundant mockReset in test, add edge case tests for desktop restore - usePageTree: add isValidating to SWR mock, add retry() test coverage with editing guard bypass verification, add clarifying comment for retry vs invalidateTree distinction - Device refresh route: add null guard for sessionClaims on web path Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Addressing CodeRabbit Review FeedbackAll feedback addressed in commit Actionable Issue (Fixed)
Nitpicks Addressed
Out of Scope (Acknowledged)
All 506 tests pass locally (30 test files). CI checks now running on the latest push. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/public/sw.js`:
- Line 84: Update the misleading comment above the cache-first route to reflect
the actual behavior of isStaticAsset and staticExtensions: remove the mention of
generic ".js/.css" since staticExtensions no longer includes them, and clarify
that the cache-first path covers fonts, images and content-hashed assets (or
only /_next/ hashed JS/CSS if that's the intended scope); reference the
isStaticAsset function and the staticExtensions constant and ensure the comment
aligns with the network-first branch handled later (the network-first branch
around the other handler).
🧹 Nitpick comments (3)
apps/web/public/sw.js (1)
1-6: Consider updating the file-level docstring to reflect the new bypass rules.The header still says "cache-first for static assets" which is only part of the picture now. With the
/_next/bypass and RSC header bypass, the caching strategy has become more nuanced. A brief mention of these bypasses would help future readers.apps/web/src/hooks/usePageTree.ts (1)
49-64: Usereturn awaitinsidetry/finallyto keep timeout and error handling effective.Without
await, thefinallyblock (andclearTimeout) runs as soon asresponse.json()is called, not when the body has been fully read. This means:
- The abort-timeout no longer guards body reading — if the body stream stalls, the
AbortControllerhas already been defused.- A JSON-parse rejection will bypass the
catchblock, so it can never be mapped to the"Page tree request timed out"message (or any other custom handling you add later).
return awaitensures the promise settles beforecatch/finallyexecute.Suggested fix
- return response.json(); + return await response.json();apps/web/src/hooks/useTabSync.ts (1)
54-55: Re-reading state after healing is the right call, but thestate/currentStateduality adds cognitive overhead.The first read (
state, line 27) is used for healing and guard checks; the second (currentState, line 55) is used for the actual tab operations. This is functionally correct, but having two differently-named snapshots of the same store in scope invites accidental use of the stale one.A small simplification: you could re-assign the same binding after healing, or extract the heal-and-get into a helper, so there's only one
statevariable in scope past the healing block.♻️ Optional: collapse to a single binding
- const state = useTabsStore.getState(); - const hasTabs = state.tabs.length > 0; - - // Heal invalid state: tabs exist but activeTabId is missing/stale. - if (hasTabs && !selectActiveTab(state)) { - state.setActiveTab(state.tabs[0].id); - } + let state = useTabsStore.getState(); + const hasTabs = state.tabs.length > 0; + + // Heal invalid state: tabs exist but activeTabId is missing/stale. + if (hasTabs && !selectActiveTab(state)) { + state.setActiveTab(state.tabs[0].id); + state = useTabsStore.getState(); // refresh after heal + }Then replace
currentStateon lines 55–74 with the samestatevariable, and drop the separate re-read on line 55.
The staticExtensions array only covers fonts and images, not JS/CSS. Updated the comment to match reality per CodeRabbit review. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…state during retries P1: After desktop OAuth exchange, the Set-Cookie from the exchange endpoint only reached the Node.js main process fetch, not the BrowserWindow cookie jar. This meant the Next.js middleware (which checks for a session cookie on page routes) would redirect /dashboard to /auth/signin after OAuth exchange. Fix: explicitly set the session cookie in session.defaultSession.cookies before navigating. P2: When the initial tree fetch fails and SWR auto-retries, isLoading is false (error exists) but tree is still empty. Without the new guard, the component falls through to "Page not found" during retry windows instead of showing a loading skeleton. Fix: show skeleton when isValidating && tree.length === 0. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Additional Fixes (P1 + P2)After deeper investigation of the desktop auth flow and loading states: [P1] Desktop OAuth exchange cookie not reaching BrowserWindow (commit
|
Gate the isValidating retry skeleton on !isLoading so the initial load still reaches the isLoading branch with its 12s timeout and retry button. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…blic in middleware Device refresh, mobile login, and desktop exchange endpoints authenticate via body tokens (device token, email/password, exchange code), not session cookies. Without this change, once the 7-day session cookie expires while the 90-day desktop session is still valid, middleware blocks the device refresh call itself — preventing cookie recovery and forcing unnecessary re-authentication. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fix: Cookie-expiry deadlock in middleware (commit
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/index.ts`:
- Around line 1018-1029: The cookie `secure` flag logic is inconsistent with
getAppUrl() — change the secure calculation used in the
session.defaultSession.cookies.set call so it treats both 'localhost' and
'127.0.0.1' as non-secure origins (same behavior as getAppUrl()); locate the
code around appUrl = new URL(getAppUrl()) and update the secure assignment
(currently `secure: !appUrl.origin.includes('localhost')`) to also check for
'127.0.0.1' before setting secure to true, ensuring cookies for local HTTP dev
origins are not marked secure.
In `@apps/web/src/components/layout/middle-content/CenterPanel.tsx`:
- Around line 46-54: The loading-timeout effect doesn't restart when a retry
occurs but isLoading stays true; add a retry counter state (e.g.,
loadingRetryCount) that handleRetry increments when invoking retry(), and
include that counter in the useEffect dependency list alongside isLoading so the
timer created in the effect (which uses setLoadingTimedOut and
LOADING_TIMEOUT_MS) is reset on each retry; update handleRetry to reset
loadingTimedOut and increment loadingRetryCount before calling retry().
🧹 Nitpick comments (1)
apps/web/public/sw.js (1)
96-97: Catch-all now network-first — correct direction, but the offline fallback returns JSON for non-API consumers.Switching from cache-first to network-first for the catch-all is the right default to avoid stale content. However,
networkFirstWithCache(line 116) returns aapplication/jsonerror body when offline and nothing is cached. This branch also serves the catch-all, so a non-API consumer (e.g., a fetch for a.jsonmanifest or other non-HTML resource) would receive a JSON error with{ "error": "offline" }.In practice the impact is low — this path is only reached when (a) offline, (b) nothing cached, and (c) the request doesn't match any earlier branch — but the comment on line 115 ("Return a JSON error for API requests when offline") is now slightly misleading since this function also serves the catch-all.
Consider either:
- Adding a small note to the comment, or
- Creating a thin wrapper / separate fallback for the catch-all that returns a plain 503 instead of JSON.
Not blocking; the current behavior is functional.
- Desktop exchange cookie: check for both localhost and 127.0.0.1 when setting secure flag, matching getAppUrl() behavior. - CenterPanel: add timerKey state that increments on retry, used as an effect dependency to force the 12s timeout timer to restart even when isLoading stays true throughout the retry cycle. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When desktop started with no persisted tabs, didAttemptDesktopRestore was never set because the hasTabs guard skipped the entire block. Later navigation back to /dashboard was misclassified as bootstrap, bouncing the user to the previous page. Now marks the restore attempt on the first hydrated pass regardless of tab state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
The desktop app was getting stuck showing an infinite skeleton loader when
loading pages. Root cause: auth endpoints used by desktop (mobile login,
device refresh, desktop OAuth exchange) were not setting session cookies.
The Next.js middleware requires a session cookie for page route requests,
so without one, page loads would fail silently and the SWR fetch would
hang without surfacing an error.
Changes
Core fix: Session cookies for desktop auth endpoints
Cookie-expiry deadlock fix
so device refresh can recover from expired cookies (these endpoints
authenticate via body tokens, not session cookies)
session.defaultSession.cookies.set()(Node.js main-process fetchdoesn't share cookies with the renderer)
Loading UX improvements
loadingTimedOutnot resetting when Retry is clicked (handleRetrywrapper)during transient failures)
Buttoncomponent instead of raw<button>elementsDesktop startup improvements
Service worker hardening
_next/in service worker to prevent stale bundle mismatchesCode quality (CodeRabbit review feedback)
typeof windowcheck in useEffectsessionClaimson web device refresh pathisValidatingto SWR mock andretry()test coverageretryvsinvalidateTreedistinctionHow to validate
https://claude.ai/code/session_011dazNWCw89QmzeT8QGejAB
Summary by CodeRabbit
New Features
Bug Fixes