Fix web client crash after connect; persist browser sessions; show account identity#845
Conversation
The web adapter's fallback proxy resolved any unimplemented namespace method
to null, so window.ade.account.status() returned null and the always-mounted
LaunchGate crashed on status.signedIn right after connecting. The adapter now
exposes a real account surface backed by the browser session (status with
email/name/imageUrl from Clerk userinfo, machine listing via the directory),
and fetchAccountStatus coerces null to signed-out as defense in depth.
Browser sessions now persist: the refresh token and OAuth identity live in
IndexedDB beside the trust store, restore on boot with a JWT-exp-aware refresh
first, and clear on sign-out or confirmed auth expiry — reloading no longer
signs the user out.
The machine picker shows the account email and avatar instead of the raw
Clerk user id, and web copy is device-neutral ("Choose a machine").
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
arul28
changed the title
|
@copilot review but do not make fixes |
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesBrowser account flow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/webclient/account/client.ts (1)
390-395: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftValidate the session generation before applying asynchronous account results.
Late operations from account A can mutate, restore, or expire account B after sign-out or reauthentication.
apps/desktop/src/renderer/webclient/account/client.ts#L390-L395: capture the session/generation before fetching machines and discard stale results.apps/desktop/src/renderer/webclient/account/client.ts#L609-L660: capture an operation generation before loading persistence and stop restoration after any account transition.apps/desktop/src/renderer/webclient/account/client.ts#L664-L697: verify the originating session before handling authorization failures or updating machines.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/webclient/account/client.ts` around lines 390 - 395, Guard asynchronous account operations with the existing session/generation mechanism: in apps/desktop/src/renderer/webclient/account/client.ts:390-395, capture the originating generation before fetchAccountMachines and discard the result if it changed before applyDirectoryResult; in :609-660, capture the operation generation before loading persistence and stop restoration after any account transition; in :664-697, verify the originating session before handling authorization failures or updating machines.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/webclient/shell/AccountIdentity.tsx (1)
1-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid using
useEffectto derive state from props.Resetting the
imageFailedstate inside auseEffectcauses a second render after paint, which can briefly flash the fallback initials before the new image starts loading. You can completely avoid this effect cycle by storing the URL of the failed image instead of a boolean.This simplifies the state management and aligns with React's recommendation to calculate derived state directly during render.
♻️ Proposed refactor
-import React, { useEffect, useState } from "react"; +import React, { useState } from "react"; import type { BrowserAccountSnapshot } from "../account/client"; import { COLORS, SANS_FONT } from "./shellTokens"; export function accountIdentityLabel(account: BrowserAccountSnapshot): string { return account.email ?? account.name ?? "ADE account"; } function accountIdentityInitials(account: BrowserAccountSnapshot): string { const name = account.name?.trim(); if (name) { const parts = name.split(/\s+/).filter(Boolean); if (parts.length > 1) return `${parts[0]![0]}${parts.at(-1)![0]}`.toUpperCase(); return parts[0]!.slice(0, 2).toUpperCase(); } return account.email?.trim()[0]?.toUpperCase() ?? "A"; } export function AccountIdentity({ account, size = 24, }: { account: BrowserAccountSnapshot; size?: number; }) { - const [imageFailed, setImageFailed] = useState(false); - useEffect(() => setImageFailed(false), [account.imageUrl]); - const showImage = Boolean(account.imageUrl) && !imageFailed; + const [failedUrl, setFailedUrl] = useState<string | null>(null); + const showImage = Boolean(account.imageUrl) && account.imageUrl !== failedUrl; return ( <div style={{ display: "flex", alignItems: "center", gap: 8, minWidth: 0 }}> <span aria-hidden="true" style={{ width: size, height: size, flex: `0 0 ${size}px`, display: "inline-flex", alignItems: "center", justifyContent: "center", overflow: "hidden", borderRadius: "50%", border: `1px solid ${COLORS.border}`, background: "color-mix(in srgb, var(--color-fg) 8%, transparent)", color: COLORS.textSecondary, fontFamily: SANS_FONT, fontSize: Math.max(9, Math.round(size * 0.42)), fontWeight: 600, }} > {showImage ? ( <img src={account.imageUrl!} alt="" - onError={() => setImageFailed(true)} + onError={() => setFailedUrl(account.imageUrl)} style={{ width: "100%", height: "100%", objectFit: "cover" }} /> ) : accountIdentityInitials(account)}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/webclient/shell/AccountIdentity.tsx` around lines 1 - 58, Refactor AccountIdentity to store the failed image URL rather than a boolean imageFailed state, and remove the useEffect import and reset logic. Derive showImage during render by requiring account.imageUrl to differ from the stored failed URL, and update the img onError handler to record the current URL so changing image URLs immediately attempts the new image without an effect-driven render.apps/desktop/src/renderer/webclient/account/client.test.ts (1)
414-415: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert every static origin allowed by the avatar policy.
This passes even if CSP omits
images.clerk.devorstorage.googleapis.com, althoughtrustedProfileImageUrlaccepts both.Proposed test expansion
expect(hostedHeaders).toContain("img-src 'self' data: blob: https://img.clerk.com"); +expect(hostedHeaders).toContain("https://images.clerk.dev"); +expect(hostedHeaders).toContain("https://storage.googleapis.com");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/webclient/account/client.test.ts` around lines 414 - 415, Expand the CSP assertion in the hostedHeaders test to verify every static origin accepted by trustedProfileImageUrl, including images.clerk.dev and storage.googleapis.com alongside the existing img.clerk.com origin. Keep the current directive assertion and add checks that each required origin is present in img-src.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/renderer/webclient/account/client.ts`:
- Around line 525-580: Make authentication state transitions failure-atomic in
setSession, sign-out, and auth-expiry flows: persist or clear the refresh
credential before exposing the corresponding session state, and roll back or
keep state consistent when storage operations reject. In
apps/desktop/src/renderer/webclient/account/client.ts lines 525-580, update
setSession to handle save/clear failures without leaving session and snapshot
partially committed; lines 361-378, commit the signed-out snapshot in finally
and explicitly handle credential-removal failure; lines 703-719, commit the
auth-expired snapshot even when IndexedDB clearing fails. Add rejection tests
covering save() and clear() during sign-in, sign-out, and expiry.
- Around line 131-137: Update decodeJwtPayload to interpret the atob-decoded JWT
payload bytes as UTF-8 before JSON.parse, preserving correct non-ASCII claim
values while retaining the existing base64url normalization and isRecord
validation.
---
Outside diff comments:
In `@apps/desktop/src/renderer/webclient/account/client.ts`:
- Around line 390-395: Guard asynchronous account operations with the existing
session/generation mechanism: in
apps/desktop/src/renderer/webclient/account/client.ts:390-395, capture the
originating generation before fetchAccountMachines and discard the result if it
changed before applyDirectoryResult; in :609-660, capture the operation
generation before loading persistence and stop restoration after any account
transition; in :664-697, verify the originating session before handling
authorization failures or updating machines.
---
Nitpick comments:
In `@apps/desktop/src/renderer/webclient/account/client.test.ts`:
- Around line 414-415: Expand the CSP assertion in the hostedHeaders test to
verify every static origin accepted by trustedProfileImageUrl, including
images.clerk.dev and storage.googleapis.com alongside the existing img.clerk.com
origin. Keep the current directive assertion and add checks that each required
origin is present in img-src.
In `@apps/desktop/src/renderer/webclient/shell/AccountIdentity.tsx`:
- Around line 1-58: Refactor AccountIdentity to store the failed image URL
rather than a boolean imageFailed state, and remove the useEffect import and
reset logic. Derive showImage during render by requiring account.imageUrl to
differ from the stored failed URL, and update the img onError handler to record
the current URL so changing image URLs immediately attempts the new image
without an effect-driven render.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ca7f304f-6308-45ec-82b8-57e563a4693d
⛔ Files ignored due to path filters (1)
docs/features/web-client/README.mdis excluded by!docs/**
📒 Files selected for processing (19)
apps/desktop/src/renderer/lib/account.test.tsapps/desktop/src/renderer/lib/account.tsapps/desktop/src/renderer/webclient/account/client.test.tsapps/desktop/src/renderer/webclient/account/client.tsapps/desktop/src/renderer/webclient/account/leaseMonitor.tsapps/desktop/src/renderer/webclient/account/sessionStore.tsapps/desktop/src/renderer/webclient/adapter/__tests__/adapter.test.tsapps/desktop/src/renderer/webclient/adapter/account.tsapps/desktop/src/renderer/webclient/adapter/index.tsapps/desktop/src/renderer/webclient/public/_headersapps/desktop/src/renderer/webclient/shell/AccountIdentity.tsxapps/desktop/src/renderer/webclient/shell/MachinePicker.tsxapps/desktop/src/renderer/webclient/shell/WebClientRoot.tsxapps/desktop/src/renderer/webclient/shell/WebShell.tsxapps/desktop/src/renderer/webclient/shell/__tests__/MachinePicker.test.tsxapps/desktop/src/renderer/webclient/shell/__tests__/WebClientRoot.test.tsxapps/desktop/src/renderer/webclient/sync/envStore.test.tsapps/desktop/src/renderer/webclient/sync/envStore.tsapps/desktop/src/renderer/webclient/sync/relayPolicy.ts
…persistence Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b512d796c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }; | ||
| }, | ||
| cancelLogin: async () => browserAccountStatus(accountClient.getSnapshot()), | ||
| signOut: async () => browserAccountStatus(await accountClient.signOut()), |
There was a problem hiding this comment.
When the mounted Account page calls this bridge method, it only clears BrowserAccountClient; it bypasses WebClientRoot.onAccountSignOut, which is the path that disconnects the sync client and prunes account-owned environments. If a user signs out from /account while connected to an account machine, the relay connection and shell account state remain active/stale until the lease monitor eventually runs, so explicit sign-out does not immediately revoke access.
Useful? React with 👍 / 👎.
| return { | ||
| status: async () => browserAccountStatus(accountClient.getSnapshot()), | ||
| startLogin: async () => { | ||
| const authorizeUrl = await accountClient.startSignIn(); |
There was a problem hiding this comment.
This bridge method is also used by the shared Account page's useAccountLogin, which expects startLogin to return an authorization URL and then calls openExternalUrl itself. Because accountClient.startSignIn() already does location.assign(url), clicking sign-in from the mounted web Account page both navigates the current tab and opens a second OAuth tab; with the adapter's noopener window open, that second tab does not have the PKCE verifier in sessionStorage, so completing auth there fails state verification.
Useful? React with 👍 / 👎.
1 participant
Summary
Production hotfix for the hosted web client (found immediately after the v1.2.29 deploy):
null, sowindow.ade.account.status()returned null and the always-mounted LaunchGate crashed onstatus.signedInright after a successful machine connect. The adapter now exposes a realaccountsurface (status with email/name/avatar via Clerk userinfo, machine listing via the directory), andfetchAccountStatuscoerces null → signed-out so the fallback-mismatch class can't crash the renderer again.user_…Clerk id.Testing
98/98 webclient+account tests (new: adapter surface shape, null coercion, session persist/restore/clear, picker identity), tsc clean, webclient build green with bundle greps confirming new copy and no raw user-id render path, docs validated (194 files), quality re-review clean.
🤖 Generated with Claude Code
Summary by CodeRabbit
Greptile Summary
This PR hardens the hosted web client account flow and machine picker. The main changes are:
window.ade.accountadapter surface.Confidence Score: 5/5
Safe to merge with low risk.
The changed account and session flows validate persisted data, refresh before expired JWT use, filter trusted profile images, and handle storage cleanup failures without leaving the in-memory session active incorrectly.
No files require special attention.
What T-Rex did
Important Files Changed
BrowserAccountClientsnapshots and operations onto the web adapter account namespace.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Browser as WebClientRoot participant Account as BrowserAccountClient participant Store as IndexedDB SessionStore participant Clerk as Clerk OAuth/Userinfo participant Directory as Account Directory participant Adapter as window.ade.account Browser->>Account: bootstrap() Account->>Store: load persisted refresh token/profile alt OAuth callback Account->>Clerk: exchange authorization code else persisted session Account->>Clerk: refresh token end Account->>Clerk: userinfo(access token) Account->>Store: save rotated refresh token/profile Account->>Directory: list account machines Account-->>Browser: BrowserAccountSnapshot Browser->>Adapter: createAdeWebAdapter(accountClient) Adapter-->>Browser: status/listMachines/signOut surface%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Browser as WebClientRoot participant Account as BrowserAccountClient participant Store as IndexedDB SessionStore participant Clerk as Clerk OAuth/Userinfo participant Directory as Account Directory participant Adapter as window.ade.account Browser->>Account: bootstrap() Account->>Store: load persisted refresh token/profile alt OAuth callback Account->>Clerk: exchange authorization code else persisted session Account->>Clerk: refresh token end Account->>Clerk: userinfo(access token) Account->>Store: save rotated refresh token/profile Account->>Directory: list account machines Account-->>Browser: BrowserAccountSnapshot Browser->>Adapter: createAdeWebAdapter(accountClient) Adapter-->>Browser: status/listMachines/signOut surfaceReviews (2): Last reviewed commit: "Fix snapshot test literals, UTF-8 JWT cl..." | Re-trigger Greptile