fix(desktop): CodeRabbit PR #614 followup + PKCE URL scheme auth#627
Merged
jeevanpillay merged 23 commits intomainfrom May 6, 2026
Merged
fix(desktop): CodeRabbit PR #614 followup + PKCE URL scheme auth#627jeevanpillay merged 23 commits intomainfrom
jeevanpillay merged 23 commits intomainfrom
Conversation
Addresses four findings from CodeRabbit's review of the Clerk loopback sign-in PR: sign-out atomicity (#9), silent persist-failure (#8), JWT-in-URL leak (#7), and non-deterministic Clerk-signed-out bridge (#4). Phase 1 — auth-store correctness - persist/clear return boolean; setToken/signOut only emit on success - clearPersisted deletes disk BEFORE clearing memory so a failed unlink can't leave an out-of-sync session - load() auto-purges auth.bin on decrypt/schema failure with Sentry scopes auth-store.load / auth-store.load.schema - IPC signOut contract becomes Promise<boolean>; renderer surfaces a toast on user-click failure and a single Sentry event on the UNAUTHORIZED auto-sign-out path (idempotent latch) Phase 2 — POST-to-loopback handoff - Replace GET ?token= with POST JSON body; the browser URL/history and Referer chain never see the JWT - Allow-listed Origin (403 otherwise), OPTIONS preflight + Chrome Private Network Access header for public→loopback fetches, 16 KiB body cap, 400 on state mismatch, 500 + Sentry on persist failure - Serialize concurrent beginSignIn via module-scope inflight promise so rapid clicks don't spawn duplicate tabs/ports - ClientAuthBridge: discriminated union on mode ("post" | "redirect"), useRef didStart latch survives StrictMode double-invoke without double-POSTing, deterministic "error" when Clerk reports signed-out, new success panel replaces the HTML the loopback used to serve - Window focus on signed-out → signed-in transition only (gated by prev snapshot seeded from getAuthSnapshot, so token refresh doesn't yank focus) - Sentry observability across auth-flow.{bind,state_mismatch, forbidden_origin,timeout,persist_failed,handler_error,server_error, open_external} and auth-bridge.{invalid_callback,fetch_network_error, fetch_non_ok,unexpected_error} CLI loopback (cli-auth-client) retains the redirect mode explicitly for now — it can migrate to POST when its loopback gains POST support. Entire-Checkpoint: d07da2e69249
…verage Replaces the 10 manual verification scenarios in the PR #614 follow-up plan with 33 automated tests. Covers the full behavioral surface of the POST handoff, the window-focus transition gate, and the ClientAuthBridge handshake — including the paths that were previously hand-verified against a running desktop + browser. apps/desktop — new vitest suite (23 tests, 2 files) - auth-flow.test.ts boots the real HTTP loopback server and exercises ALLOWED_ORIGIN resolution (dev fallback / NODE_ENV=production / LIGHTFAST_API_URL override), forbidden origin → 403, missing Origin header → 403, unknown path → 404, OPTIONS preflight → 204 with every CORS + PNA header asserted, GET → 405 Allow: POST, invalid body shape → 400 bad_request, state mismatch → 400 + Sentry warning, happy path → 204 + setToken called + promise resolves with token, persist failure → 500 + Sentry exception, 16 KiB body cap, concurrent beginSignIn returns same promise, inflight cleared after settle, 5-min timeout fires auth-flow.timeout with vi.useFakeTimers() - auth-focus-gate.test.ts covers the false→true focus (first sign-in), true→true no-op (token refresh), true→false no-op (sign-out), re-sign-in, multi-window fan-out, lazy getWindows resolution Testability refactor: extracted createAuthFocusGate from index.ts as a pure function so it's coverable without Electron BrowserWindow. apps/app — new vitest suite (10 tests, 1 file) - client-auth-bridge.test.tsx uses React Testing Library + happy-dom to assert: POST with correct JSON body + credentials:"omit" + headers, StrictMode double-invoke fires exactly one POST (didStart latch), null buildPostCallback / fetch rejection / 4xx response all render the error panel and emit the right Sentry tag scope, null getToken → error, Clerk signed-out → deterministic error (fix for #4), Clerk not-loaded → loading panel, redirect mode sets window.location.href via vi.spyOn setter, null buildRedirectUrl → error Infra - apps/desktop: new "test" script, devDeps add vitest (catalog) + @repo/vitest-config + happy-dom; vite devDep bumped to ^7.1.10 so vitest 4's peer is satisfied (electron-forge/plugin-vite keeps its own vite 5 via direct dep, no conflict) - apps/app: @testing-library/react devDep Entire-Checkpoint: c1f2cfc2cfb4
Phase 4 of the CodeRabbit PR #614 follow-up plan. Adds the missing auth-boundary case that matches the canonical desktop unhappy path (expired 24h JWT + no cookie because the desktop has never been to lightfast.ai) — previously unverified. Also flips the Phase 3 and Phase 4 plan checkboxes to [DONE] and documents that Phase 3's scope landed inside Phase 2's ClientAuthBridge rewrite and is covered by the automated bridge tests from commit 9e1c07d. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 1d489bf5d4b2
Desktop: - electron 39 → 41 (chromium 140+) - vite 7 → 8, @vitejs/plugin-react 4 → 6 - Drop dead direct deps @electron/osx-sign + @electron/notarize (forge's @electron/packager@18.4.4 provides them transitively; our forge.config.ts only passes osxSign/osxNotarize as option objects) Root: - engines.node >=22.0.0 → >=22.12.0 (vite 8 minimum) App: - Add @vitejs/plugin-react to apps/app/vitest.config.ts — Vite 8 moved import-analysis ahead of the esbuild transform, so the existing esbuild.jsx: "automatic" override no longer catches .tsx before import-analysis rejects the jsx: preserve in tsconfig. The react plugin registers a pre-import-analysis JSX transform hook. Hold: @electron/fuses stays on ^1.8.0 (forge 7.x peer pins ^1.0.0; revisit when forge 8 stabilizes). Refs: thoughts/shared/plans/2026-04-24-desktop-deps-major-upgrade.md Entire-Checkpoint: 2bf87131bcb5
- Move @vitejs/plugin-react and happy-dom to pnpm-workspace.yaml catalog (both used identically in apps/app + apps/desktop; per CLAUDE.md "Use catalog: for shared externals") - Replace literal versions with "catalog:" refs in apps/app/package.json and apps/desktop/package.json - Drop --passWithNoTests from apps/desktop test script — desktop now has 2 real test files, flag would silently mask future discovery regressions - Re-order imports in apps/app/vitest.config.ts per Biome organizeImports Entire-Checkpoint: 7b2ff62f014f
chore(desktop): upgrade electron 41, vite 8, plugin-react 6
…bundle Vite 8 bundles the main process as CJS, where `import.meta` resolves to an empty object literal and `import.meta.url` becomes `undefined`. This crashed Electron boot with `ERR_INVALID_ARG_TYPE` from `fileURLToPath` before any window code ran. Use the CJS-native `__dirname` directly. Semantically identical to the ESM form; survives both bundle formats. Entire-Checkpoint: dadf5e6571a7
The new desktop auth routes (`/api/desktop/auth/code`, `/api/desktop/auth/exchange`) handle their own auth — `code` verifies a Clerk Bearer JWT at the route level, `exchange` is unauthed because the short-lived code itself proves possession. Without an entry in the `isApiRoute` matcher, Clerk middleware 307-redirected both to `/sign-in`. Mirror the existing `/api/cli/(.*)` entry. Entire-Checkpoint: 06b998039e1f
Replaces the desktop's loopback HTTP server with an OAuth 2.0 Authorization
Code + PKCE flow over a custom URL scheme (`lightfast://` packaged,
`lightfast-dev://` unpackaged). Brings the desktop in line with VS Code /
GitHub Desktop / Linear / Slack and removes ~150 LoC of HTTP server / CORS /
ephemeral-port plumbing.
Server (apps/app):
- `POST /api/desktop/auth/code` (Clerk JWT in Authorization header) issues a
short-lived code (32-byte base64url, 30s TTL in Upstash Redis) bound to
state + S256 code_challenge + redirect_uri (allowlist: `lightfast://` and
`lightfast-dev://`).
- `POST /api/desktop/auth/exchange` (no auth — code is the proof) atomically
consumes the code via GETDEL, verifies SHA256(verifier) == challenge, and
returns the JWT.
Web bridge:
- New `code-redirect` mode on `ClientAuthBridge` exchanges the JWT for a code
and assigns `window.location.href` to `<redirect_uri>?code=…&state=…`,
then best-effort `window.close()` after 250ms.
Desktop:
- `protocol.ts` registers the URL scheme via `app.setAsDefaultProtocolClient`
and dispatches `app.on('open-url')` (macOS) / `second-instance` argv
(Windows/Linux) to listeners. Forge `CFBundleURLTypes` mirrors for packaged
builds.
- `auth-flow.ts` rewritten around PKCE: composes the signin URL with state +
S256 challenge + scheme-derived redirect_uri, listens for the protocol
callback, exchanges code+verifier for the JWT, persists via `safeStorage`.
- Renames `LIGHTFAST_DESKTOP_AUTH_NO_OPEN` → `LIGHTFAST_DESKTOP_AGENT_MODE`
with broadened semantics: skip `shell.openExternal` AND emit a structured
stdout JSON event grammar (`auth_signin_url`, `auth_signed_in`,
`auth_already_signed_in`, `auth_signin_failed{reason}`) so agent harnesses
drive sign-in deterministically without log-grepping or CDP attach.
- `maybeAutoBeginSignIn()` fires on app-ready in agent mode: emits
`auth_already_signed_in` if a token is persisted, otherwise begins a
fresh sign-in. Single agent-browser session, no renderer click.
- New `LIGHTFAST_DESKTOP_AUTH_TIMEOUT_MS` (default 5min for humans, ~30s
for CI/agent runs).
- IPC: `authPendingSigninUrl` getter + `authPendingSigninUrlChanged`
broadcast for renderer status surfaces.
Tests (122 total passing):
- 13 unit tests for code/exchange routes (auth, schema, allowlist, GETDEL
one-shot).
- 5 new bridge tests for code-redirect mode.
- 12 protocol tests (scheme detection, dispatch, listener detach, argv
first-launch on Windows/Linux, foreign-scheme rejection).
- 15 auth-flow tests (PKCE composition, callback matching, exchange
failure / persist failure / handler error / timeout, agent vs
non-agent mode, maybeAutoBeginSignIn 3 states, event grammar).
Live verification (2026-04-25):
- Server round-trip: real Clerk JWT issued via `lightfast-clerk` →
POST /code → POST /exchange returned same JWT → second POST returned
invalid_code (Redis GETDEL atomicity).
- Real Electron 41: agent-mode boot emitted auth_signin_url; macOS
LaunchServices warm-dispatched `lightfast-dev://auth/callback?...` to
the running desktop; PKCE state matched; exchange call fired.
- Full UI-driven happy path with agent-browser headed: Clerk OTP sign-in →
navigate to signin URL → bridge redirected via `lightfast-dev://` → desktop
received → exchange → token persisted (851b auth.bin) → auth_signed_in
emitted ~14s end-to-end. Idempotent restart emitted auth_already_signed_in.
Risks documented in plan: agent-browser headless silently drops
custom-scheme navigations (mandate `AGENT_BROWSER_HEADED=true`); cold-launch
URL routing unreliable in unpackaged dev (precondition: app must be running
before redirect fires).
Runbook: `.agents/skills/lightfast-desktop-signin/SKILL.md`.
Plan: `thoughts/shared/plans/2026-04-25-desktop-auth-url-scheme-pkce.md`.
Entire-Checkpoint: d6793f336d6d
# Conflicts: # apps/desktop/package.json # apps/desktop/src/main/auth-flow.ts # pnpm-lock.yaml
Post-merge lockfile reconciliation: catalog entries for @vitejs/plugin-react ^6.0.1 and happy-dom ^20.9.0 (defined in pnpm-workspace.yaml on this branch) were not captured when the merge took main's lockfile + pnpm install. Vitest's transitive vite peer also tracks to 8.0.10 to match the desktop upgrade. Entire-Checkpoint: 3c27d89b128e
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Contributor
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements desktop app authentication via PKCE-based OAuth flow with protocol callbacks, adds corresponding web bridge modes, new API endpoints for code exchange, and refactors observability imports to vendor wrappers. Includes comprehensive test coverage and Electron forge protocol registration. ChangesDesktop Authentication System
Observability Refactor
Dependency & Configuration Management
Sequence DiagramsequenceDiagram
participant Renderer as Desktop Renderer
participant Main as Electron Main
participant Protocol as Protocol Module
participant Browser as OS Browser
participant Lightfast as Lightfast Web
participant AuthAPI as Auth API<br/>(code/exchange)
participant TokenStore as Redis<br/>Code Store
Renderer->>Main: beginSignIn()
Main->>Main: Generate PKCE challenge
Main->>Browser: shell.openExternal(sign-in URL)
Browser->>Lightfast: GET /desktop/auth?state=S&code_challenge=C
Lightfast->>Browser: ClientAuthBridge (code-redirect mode)
Browser->>AuthAPI: POST /api/desktop/auth/code<br/>(state, code_challenge, redirect_uri)
AuthAPI->>TokenStore: issueCode(record)
TokenStore-->>AuthAPI: code
AuthAPI-->>Browser: { code }
Browser->>Browser: Redirect to lightfast://auth/callback?code=X&state=S
Browser->>Protocol: Protocol dispatch
Protocol->>Main: onProtocolUrl(lightfast://...?code=X&state=S)
Main->>Main: Validate state
Main->>AuthAPI: POST /api/desktop/auth/exchange<br/>(code, code_verifier)
AuthAPI->>TokenStore: consumeCode(code)
TokenStore-->>AuthAPI: record
AuthAPI->>AuthAPI: Verify PKCE: SHA256(verifier) == challenge
AuthAPI-->>Main: { token: jwt }
Main->>Main: Persist token
Main->>Renderer: Broadcast auth snapshot<br/>(isSignedIn: true)
Renderer->>Renderer: Render AccountCard
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Rationale: Heterogeneous changes across 40+ files with dense, multi-pattern logic. New PKCE OAuth flow introduces state machine complexity (pending URL tracking, code expiry, exchange validation). Protocol dispatch, IPC bridging, and Electron lifecycle coordination span layers requiring separate reasoning per file. Comprehensive test suites (676 lines for auth-flow, 395 for protocol) demand careful assertion review. Observability refactor is repetitive re-export boilerplate but touches all service entry points. Vitest/React Testing Library integration in new test files adds surface area. Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
apps/desktop/src/main/__tests__/protocol.test.ts (1)
295-297: ⚡ Quick win[warning] Replace the double microtask flush with an explicit readiness signal.
These checks depend on the current
whenReady().then(...)scheduling depth. A harmless refactor inapps/desktop/src/main/protocol.tscan break the test without changing behavior.As per coding guidelines,
**/*.test.ts:No flaky patterns (timeouts, race conditions).Also applies to: 321-322
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/__tests__/protocol.test.ts` around lines 295 - 297, Replace the fragile double microtask flush in the tests by awaiting an explicit readiness signal from the protocol module: stop using two Promise.resolve() calls and instead await the module's whenReady() (or expose a ready Promise/event in protocol.ts and call that from the test). Update the two test sites that currently use the microtask flush to import/await protocol.whenReady() (or the new ready Promise) so tests no longer rely on scheduling depth and become deterministic.apps/app/src/proxy.ts (1)
59-65: ⚡ Quick win[warning] Scope this middleware bypass to the auth routes.
/api/desktop/(.*)exempts every current and future desktop endpoint from the middleware auth branch. The only routes in this PR that clearly self-authenticate are the two desktop auth handlers, so this should stay as narrow as possible, e.g./api/desktop/auth/(.*).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/app/src/proxy.ts` around lines 59 - 65, The isApiRoute matcher is too broad and exempts all desktop endpoints; narrow the bypass to only the desktop auth handlers by changing the "/api/desktop/(.*)" pattern to a more specific route such as "/api/desktop/auth/(.*)". Locate the isApiRoute definition (created via createRouteMatcher) and restrict the desktop pattern so middleware only skips authentication for the intended auth endpoints.apps/app/src/app/(app)/(user)/(pending-not-allowed)/_components/client-auth-bridge.tsx (1)
154-166: ⚡ Quick win[nit] Log invalid 200-payloads before falling back to the generic error UI.
If
/api/desktop/auth/codereturns 200 without a stringcode, this branch silently drops the only signal that the client and route contract drifted. Emit a warning here so rollout/debugging does not depend on reproducing the failure locally.Suggested fix
let parsed: { code?: unknown }; try { parsed = (await response.json()) as { code?: unknown }; } catch (error) { @@ } if (typeof parsed.code !== "string" || parsed.code.length === 0) { + captureMessage("auth-bridge: code endpoint returned invalid payload", { + level: "warning", + tags: { scope: "auth-bridge.code_invalid_payload" }, + }); setStatus("error"); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/app/src/app/`(app)/(user)/(pending-not-allowed)/_components/client-auth-bridge.tsx around lines 154 - 166, The branch that handles a 200 response with an invalid/non-string parsed.code should log the unexpected payload before falling back to setStatus("error"); update the block around parsed (variable parsed, function setStatus) to call captureException or a warning logger with the raw parsed object and identifying tags (e.g., scope: "auth-bridge.invalid_200_payload") and any useful context (response status or route) so the payload drift is recorded, then continue to call setStatus("error").apps/desktop/src/main/__tests__/auth-flow.test.ts (1)
90-121: ⚡ Quick win[warning] Drop the wall-clock polling from
captureSigninUrl.This helper spins for up to 2 seconds using real
setTimeout(10)sleeps. That makes the suite timing-sensitive for no gain: in this module bothshell.openExternal()and agent-modeauth_signin_urlemission happen synchronously whenbeginSignIn()starts, so the helper can read the mock/event state directly.
As per coding guidelines,**/*.test.ts: No flaky patterns (timeouts, race conditions).Suggested fix
async function captureSigninUrl( fromOpenExternal: boolean, emittedEvents: AuthLine[] ): Promise<CapturedSignin> { - for (let i = 0; i < 200; i++) { - if (fromOpenExternal && shellOpenExternalMock.mock.calls.length > 0) { - break; - } - if ( - !fromOpenExternal && - emittedEvents.some((e) => e.event === "auth_signin_url") - ) { - break; - } - await new Promise((r) => setTimeout(r, 10)); - } const raw = fromOpenExternal ? (shellOpenExternalMock.mock.calls.at(-1)?.[0] as string | undefined) : ( emittedEvents.find((e) => e.event === "auth_signin_url") as | { event: "auth_signin_url"; url: string } | undefined )?.url;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/__tests__/auth-flow.test.ts` around lines 90 - 121, The captureSigninUrl helper currently polls with setTimeout which makes tests flaky; instead, remove the for-loop and immediate sleeps and directly read the mock/event state: check shellOpenExternalMock.mock.calls for the last call when fromOpenExternal is true, otherwise find the auth_signin_url entry in emittedEvents, throw if raw is missing, then parse URL and return { url, state, codeChallenge, redirectUri }; update references to captureSigninUrl to rely on synchronous behavior of beginSignIn and avoid any setTimeout usage or waiting loops in the captureSigninUrl function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/app/src/app/`(app)/(user)/(pending-not-allowed)/desktop/auth/_components/desktop-auth-client.tsx:
- Around line 13-24: The guard inside buildExchangeRequest is too complex; split
the required-parameter existence check from the S256 method check to satisfy
lint/useSimplifiedLogicExpression without changing behavior: first retrieve
state, codeChallenge, method, redirectUri and return null if any of state,
codeChallenge, or redirectUri are missing; then separately check method ===
"S256" and return null if not; finally validate redirectUri against
ALLOWED_REDIRECT_URIS and return null if not, otherwise return the { state,
codeChallenge, redirectUri } object.
In `@apps/app/src/app/api/desktop/auth/code/route.ts`:
- Around line 20-31: The handler currently verifies the request with
verifyCliJwt(req) but then re-parses Authorization using a different
normalization before calling issueCode, which can mismatch the authenticated
token; update the flow so a single parser is used: either change verifyCliJwt to
return the verified raw token (or the normalized "Bearer" string) along with
session, or extract and export a shared parseAuthorization helper from
verify-jwt.ts and use that helper in route.ts before calling issueCode, then
pass the returned/normalized token into issueCode so the stored JWT exactly
matches what was verified by verifyCliJwt.
In `@apps/app/src/app/api/desktop/auth/lib/code-store.ts`:
- Around line 11-17: The CodeRecord interface members are not sorted according
to the useSortedInterfaceMembers rule; update the CodeRecord declaration so its
properties are ordered alphabetically (codeChallenge, jwt, redirectUri, state,
userId) to satisfy CI — modify the CodeRecord interface definition accordingly.
In `@apps/desktop/src/main/__tests__/protocol.test.ts`:
- Around line 119-125: The test and implementation currently only assert the
one-arg protocol registration; update the code in registerProtocolHandler (and
the test using loadProtocol) to detect unpackaged/dev mode via
process.defaultApp (or the same unpackaged detection used elsewhere) and, when
running on Windows, call setAsDefaultProtocolClient with three args: the
protocol name, process.execPath, and [path.resolve(process.argv[1])] instead of
the one-arg form; update the test assertion to expect the correct three-arg call
on Windows (using setAsDefaultProtocolClientMock and checking for
process.execPath and the resolved argv[1]) while keeping the one-arg expectation
for non-Windows or packaged flows.
In `@apps/desktop/src/main/auth-flow.ts`:
- Around line 148-203: The protocol handler registered via onProtocolUrl can run
after the auth flow has already settled (via settle or timeout); modify the
handler in auth-flow.ts to immediately return if the flow is already settled (or
maintain a local boolean like "finished" set when settle is called), and ensure
you clear/disable further callbacks by calling the unsubscribe function and
clearing the timer as soon as you decide to settle (both on timeout and on
successful/failed exchange), e.g. check the settled/finished flag at the top of
the onProtocolUrl callback, set it before calling settle(token|null), and call
unsubscribe() and clearTimeout(timer) whenever you settle to prevent
late/duplicate calls to exchangeCode, setToken, or emitAgentEvent.
In `@apps/desktop/src/main/auth-store.ts`:
- Around line 39-52: The rmSync calls inside load() (used when parsed.success is
false and inside the catch block) must be wrapped in try/catch the same way
clearPersisted() does to avoid throwing on deletion failures; update the code
around the parsed.error branch and the outer catch in load() to call
rmSync(path, { force: true }) inside a try { ... } catch (e) { console.warn(...
or processLogger.warn) } so deletion errors are logged but do not propagate and
cause startup crashes, referencing the existing clearPersisted() pattern for
exact error-handling behavior.
- Line 3: The code directly imports Sentry from "@sentry/electron/main" which
violates the vendor-abstraction rule; fix it by creating a vendor façade module
(e.g. "@vendor/observability/sentry-desktop") that imports from
"@sentry/electron/main" and re-exports the same public API (export default
Sentry and any named exports you use such as init, captureException, setUser,
configureScope, etc.), then update all desktop-side files that currently import
Sentry (auth-store.ts where Sentry is imported, plus auth-flow.ts, sentry.ts,
renderer/main.ts, app-shell.tsx) to import from
"@vendor/observability/sentry-desktop" instead, keeping the same symbols (Sentry
or named methods) so callers like Sentry.init(...) and
Sentry.captureException(...) continue to work unchanged.
In `@apps/desktop/src/main/protocol.ts`:
- Around line 40-45: The open-url listener is registered only after
app.whenReady(), so macOS cold-start URLs can be missed; move registration of
app.on("open-url", (event, url) => { event.preventDefault(); dispatch(url); })
to run before app.whenReady(), and implement a small queue (e.g., an array) that
app.on("open-url") pushes incoming URLs into when called early; then, in the
existing ready handler (index.ts initialization code), drain/process that queue
by calling dispatch for each queued URL so PKCE/auth URLs received before ready
are not lost.
---
Nitpick comments:
In
`@apps/app/src/app/`(app)/(user)/(pending-not-allowed)/_components/client-auth-bridge.tsx:
- Around line 154-166: The branch that handles a 200 response with an
invalid/non-string parsed.code should log the unexpected payload before falling
back to setStatus("error"); update the block around parsed (variable parsed,
function setStatus) to call captureException or a warning logger with the raw
parsed object and identifying tags (e.g., scope:
"auth-bridge.invalid_200_payload") and any useful context (response status or
route) so the payload drift is recorded, then continue to call
setStatus("error").
In `@apps/app/src/proxy.ts`:
- Around line 59-65: The isApiRoute matcher is too broad and exempts all desktop
endpoints; narrow the bypass to only the desktop auth handlers by changing the
"/api/desktop/(.*)" pattern to a more specific route such as
"/api/desktop/auth/(.*)". Locate the isApiRoute definition (created via
createRouteMatcher) and restrict the desktop pattern so middleware only skips
authentication for the intended auth endpoints.
In `@apps/desktop/src/main/__tests__/auth-flow.test.ts`:
- Around line 90-121: The captureSigninUrl helper currently polls with
setTimeout which makes tests flaky; instead, remove the for-loop and immediate
sleeps and directly read the mock/event state: check
shellOpenExternalMock.mock.calls for the last call when fromOpenExternal is
true, otherwise find the auth_signin_url entry in emittedEvents, throw if raw is
missing, then parse URL and return { url, state, codeChallenge, redirectUri };
update references to captureSigninUrl to rely on synchronous behavior of
beginSignIn and avoid any setTimeout usage or waiting loops in the
captureSigninUrl function.
In `@apps/desktop/src/main/__tests__/protocol.test.ts`:
- Around line 295-297: Replace the fragile double microtask flush in the tests
by awaiting an explicit readiness signal from the protocol module: stop using
two Promise.resolve() calls and instead await the module's whenReady() (or
expose a ready Promise/event in protocol.ts and call that from the test). Update
the two test sites that currently use the microtask flush to import/await
protocol.whenReady() (or the new ready Promise) so tests no longer rely on
scheduling depth and become deterministic.
🪄 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: f87dcbc2-1828-4d8b-8c5e-37911723c89f
⛔ Files ignored due to path filters (4)
.agents/skills/lightfast-desktop-signin/SKILL.mdis excluded by!.agents/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yamlthoughts/shared/plans/2026-04-24-coderabbit-pr614-fixes.mdis excluded by!thoughts/**thoughts/shared/plans/2026-04-25-desktop-auth-url-scheme-pkce.mdis excluded by!thoughts/**
📒 Files selected for processing (30)
api/app/src/__tests__/resolve-clerk-session.test.tsapps/app/package.jsonapps/app/src/app/(app)/(user)/(pending-not-allowed)/_components/client-auth-bridge.test.tsxapps/app/src/app/(app)/(user)/(pending-not-allowed)/_components/client-auth-bridge.tsxapps/app/src/app/(app)/(user)/(pending-not-allowed)/cli/auth/_components/cli-auth-client.tsxapps/app/src/app/(app)/(user)/(pending-not-allowed)/desktop/auth/_components/desktop-auth-client.tsxapps/app/src/app/api/desktop/auth/code/route.test.tsapps/app/src/app/api/desktop/auth/code/route.tsapps/app/src/app/api/desktop/auth/exchange/route.test.tsapps/app/src/app/api/desktop/auth/exchange/route.tsapps/app/src/app/api/desktop/auth/lib/code-store.tsapps/app/src/proxy.tsapps/app/vitest.config.tsapps/desktop/forge.config.tsapps/desktop/package.jsonapps/desktop/src/main/__tests__/auth-flow.test.tsapps/desktop/src/main/__tests__/auth-focus-gate.test.tsapps/desktop/src/main/__tests__/protocol.test.tsapps/desktop/src/main/auth-flow.tsapps/desktop/src/main/auth-focus-gate.tsapps/desktop/src/main/auth-store.tsapps/desktop/src/main/index.tsapps/desktop/src/main/protocol.tsapps/desktop/src/main/windows/factory.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/src/react/app-shell.tsxapps/desktop/src/shared/ipc.tsapps/desktop/vitest.config.tspackage.jsonpnpm-workspace.yaml
Resolves 9 conflicted files per thoughts/shared/plans/2026-05-06-pr627-merge-readiness.md: - forge.config.ts: adopt main's BUNDLE_ID rename + keep PR-branch URL_SCHEME. - pnpm-workspace.yaml: combine catalog adds, take main's @vercel/related-projects ^1.1.0. - apps/desktop/src/shared/ipc.ts: combine new IPC channels (auth-pending-signin-url + runtime-config-sync). - apps/desktop/src/main/auth-store.ts: keep PR-branch atomicity (boolean persist, set memory only on success). - apps/desktop/src/main/auth-flow.ts: keep PR-branch URL-scheme implementation; defer createAppUrl() adoption to Phase 5. - apps/desktop/src/main/index.ts: combine auth-flow + protocol + auth-focus-gate (PR) with app-url + runtime-config (main). - apps/desktop/src/renderer/src/react/app-shell.tsx: keep PR-branch signed-in UI + Toaster + error toasts; switch onLearnMore to bridge.openApp() (main's pattern). - client-auth-bridge.tsx: keep PR-branch useAuth() + code-redirect + Sentry observability. - pnpm-lock.yaml: accept main, then pnpm install --no-frozen-lockfile reconciled the catalog adds. Also restored apps/app/microfrontends.json to its original location (file remains canonical in apps/app/, lightfast.dev.json points to it). Phase 1 verification: - pnpm typecheck: 52/52 packages pass - pnpm --filter @lightfast/desktop test: 34/34 pass - pnpm --filter @lightfast/app test: 120/120 pass - pnpm --filter @api/app test: 6/6 pass - pnpm --filter @lightfast/app build: ok - pnpm build:platform: ok - pnpm install --frozen-lockfile: ok - pnpm exec ultracite check apps/: same 10 errors as before (Phase 4 fixes them)
Resolves CodeRabbit review feedback on PR #627 across three phases of work (Phase 2/3/4 of thoughts/shared/plans/2026-05-06-pr627-merge-readiness.md): Phase 2 — vendor abstraction: route all direct @sentry/* SDK imports through @vendor/observability. New façade modules sentry-electron-main, sentry-browser, sentry-nextjs use selective named re-exports matching the existing sentry.ts pattern. Updates 8 call sites (5 desktop, 3 next.js instrumentation) plus the auth-flow vi.mock target. Phase 3 — CodeRabbit blockers: - Hoist registerProtocolHandler() out of app.whenReady() so macOS cold-start open-url events delivered before whenReady are not lost. - Add callbackInFlight guard in auth-flow.ts to prevent duplicate exchangeCode calls when two open-url events arrive during the await window. - Wrap rmSync() in auth-store.load() with try/catch via purgePersisted helper so filesystem errors no longer crash startup. - Conditional Windows three-arg setAsDefaultProtocolClient for dev launches. - Single bearer-token parser: verifyCliJwt now returns { userId, jwt } so code/route.ts uses session.jwt directly (drops divergent regex re-parse). Phase 4 — Biome lint hygiene: - ultracite fix on 6 plan-listed files plus 3 organizeImports-only files introduced by Phase 2 import changes. - biome-ignore lint/correctness/noGlobalDirnameFilename on factory.ts:16 — Vite 8 emits the main bundle as CJS where import.meta.dirname is undefined. - biome-ignore lint/performance/noDelete on protocol.test.ts:73 — test cleanup must remove the property entirely; assigning undefined leaves a defined property where there was none. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase 5 of thoughts/shared/plans/2026-05-06-pr627-merge-readiness.md.
Replace hand-built `new URL("/desktop/auth", apiOrigin)` with
`createAppUrl("/desktop/auth")` so sign-in URL composition flows through
`getRuntimeConfig().appOrigin`. This decouples the sign-in page origin from
LIGHTFAST_API_URL — the latter still drives the exchangeCode POST endpoint via
getApiOrigin(), but the user-facing sign-in URL now goes through the same
runtime-config layer that main introduced for desktop URL composition.
Test mocks `../app-url` with a let-bound `testAppOrigin` so per-test origin
control stays test-local without depending on LIGHTFAST_APP_ORIGIN env var
plumbing through t3-env.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Phase 2 commit (38ac764) added the package.json export entries for sentry-browser, sentry-electron-main, and sentry-nextjs but missed staging the wrapper files themselves. CI typecheck failed with TS2307 on the renderer files importing @vendor/observability/sentry-browser since the package.json pointed at files that didn't exist on the CI checkout. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/app/src/app/api/cli/lib/verify-jwt.ts (1)
1-1:⚠️ Potential issue | 🔴 Critical | 💤 Low value[blocker] Use
@vendor/clerk/serverinstead of direct@clerk/nextjsimport.The
verifyTokenfunction is already exported by the vendor abstraction (vendor/clerk/src/server.ts). Per coding guidelines, third-party SDKs must not be imported directly. Change line 1 to:import { verifyToken } from "@vendor/clerk/server";🤖 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/app/src/app/api/cli/lib/verify-jwt.ts` at line 1, Replace the direct third‑party import in verify-jwt.ts: remove the import from "@clerk/nextjs/server" and import verifyToken from the vendor abstraction instead; update the import statement that currently references verifyToken to use "@vendor/clerk/server" so the module uses the vendor wrapper (symbol: verifyToken) rather than the direct Clerk SDK.
🧹 Nitpick comments (3)
vendor/observability/package.json (1)
78-80: ⚡ Quick win[warning] Add
@sentry/browserand@sentry/electronto the shared catalog.Per monorepo policy (**/package.json: Use
catalog:for shared external dependencies), both@sentry/browserand@sentry/electronare pinned locally in vendor/observability and apps/desktop instead of using the shared catalog. Add@sentry/browser: ^10.49.0and@sentry/electron: ^7.11.0to the root package.json catalog, then update both package.json files to referencecatalog:. This prevents version drift and duplication.🤖 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 `@vendor/observability/package.json` around lines 78 - 80, Add `@sentry/browser` and `@sentry/electron` to the root package.json catalog and switch local pins to catalog:; specifically, add " `@sentry/browser`": "^10.49.0" and "@sentry/electron": "^7.11.0" to the root "catalog" object, then update vendor/observability/package.json (replace "@sentry/browser": "^10.49.0" and "@sentry/electron": "^7.11.0") and the apps/desktop package.json entries for the same packages to use "catalog:" instead of pinned versions to ensure a single shared source of truth for `@sentry/browser` and `@sentry/electron`.apps/desktop/src/main/__tests__/auth-flow.test.ts (2)
96-127: ⚡ Quick win[warning] Remove wall-clock polling from
captureSigninUrl.This helper spins for up to 2 seconds on real timers, so most of this suite is racing the scheduler instead of waiting on the mocked signal it already owns. Wire
shellOpenExternalMock/spyStdout()to resolve a deferred and await that directly.As per coding guidelines, "No flaky patterns (timeouts, race conditions)".
🤖 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/main/__tests__/auth-flow.test.ts` around lines 96 - 127, The helper captureSigninUrl currently busy-waits up to 2s polling shellOpenExternalMock and emittedEvents; replace this wall-clock polling with an explicit promise/deferred that the test resolves when the mocked signal occurs: create a Deferred (resolve/reject) used by captureSigninUrl and have shellOpenExternalMock (and/or the spyStdout()/event emitter used in tests) call resolve when the signin URL is produced; then await that deferred inside captureSigninUrl instead of the for-loop, and keep the existing parsing of the URL/state/code_challenge/redirect_uri; reference captureSigninUrl, shellOpenExternalMock, emittedEvents and spyStdout() when wiring the deferred so the test deterministically awaits the mock trigger rather than polling.
627-645: ⚡ Quick win[warning] Don’t sleep 150ms to drain this test.
Line 644 makes the test slower and CI-timing dependent just to let the auth timeout fire. Switch to fake timers before
maybeAutoBeginSignIn()and advance them deterministically.Proposed fix
it("AGENT_MODE + no token: calls beginSignIn (auth_signin_url emitted)", async () => { getTokenMock.mockReturnValue(null); + vi.useFakeTimers(); const { events, restore: restoreStdout } = spyStdout(); const { mod, restore } = await loadAuthFlow({ NODE_ENV: "test", LIGHTFAST_API_URL: undefined, LIGHTFAST_DESKTOP_AGENT_MODE: "1", @@ expect(shellOpenExternalMock).not.toHaveBeenCalled(); - // Wait for the (auto) sign-in promise to time out so we don't leak - // pending timers into subsequent tests. - await new Promise((r) => setTimeout(r, 150)); + await vi.advanceTimersByTimeAsync(150); } finally { restoreStdout(); restore(); } });As per coding guidelines, "No flaky patterns (timeouts, race conditions)".
🤖 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/main/__tests__/auth-flow.test.ts` around lines 627 - 645, Replace the real-time sleep with deterministic fake timers: before calling mod.maybeAutoBeginSignIn() in this test, switch to fake timers (e.g., jest.useFakeTimers()), then call mod.maybeAutoBeginSignIn(), advance timers by the configured timeout (LIGHTFAST_DESKTOP_AUTH_TIMEOUT_MS / the 100ms value used in loadAuthFlow) plus a small margin using jest.advanceTimersByTime(...), assert the "auth_signin_url" event and shell behavior, and finally restore real timers (jest.useRealTimers()) and any stdout or module restores; target the test harness around maybeAutoBeginSignIn(), loadAuthFlow, and getTokenMock to make the timeout deterministic and remove the setTimeout-based sleep.
🤖 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/app/src/app/`(app)/(user)/(pending-not-allowed)/_components/client-auth-bridge.tsx:
- Around line 3-4: The file imports Sentry directly (captureException,
captureMessage) which violates the vendor-abstraction rule; replace the direct
import from "@sentry/nextjs" with the repo's Sentry vendor re-export (the module
that re-exports captureException and captureMessage) so the component uses the
standalone vendor abstraction, update the import statement accordingly and
ensure usages of captureException and captureMessage in client-auth-bridge.tsx
remain unchanged; keep useAuth import as-is.
In `@apps/desktop/src/main/__tests__/auth-flow.test.ts`:
- Around line 54-87: loadAuthFlow mutates process.env then dynamically imports
"../auth-flow" and only returns a restore() if the import succeeds, so if the
import throws the mutated env (touchedKeys) is never restored; wrap the dynamic
import in a try/catch/finally such that you always restore process.env on
failure and rethrow the error, or return an object with restore() even when
import fails; specifically ensure that the logic around the
import("../auth-flow") call in loadAuthFlow uses a finally block to restore any
touchedKeys in process.env (or constructs and returns the restore closure before
awaiting the import) so callers never observe leaked env state.
In `@vendor/observability/package.json`:
- Around line 16-27: The package exports for ./sentry-electron-main,
./sentry-browser and ./sentry-nextjs in vendor/observability/package.json
reference wrapper modules that do not exist; either add the missing wrapper
modules under vendor/observability/src matching those export names (implementing
the thin re-exports/initializers expected by consumers such as apps/desktop and
apps/platform) or remove the three export entries from package.json so consumers
stop resolving to non-existent files. Also replace the hard-pinned
`@sentry/browser` and `@sentry/electron` entries with the catalog: protocol
(matching how `@sentry/nextjs` and `@sentry/core` are declared) to ensure
catalog-managed versions are used and avoid version drift.
---
Outside diff comments:
In `@apps/app/src/app/api/cli/lib/verify-jwt.ts`:
- Line 1: Replace the direct third‑party import in verify-jwt.ts: remove the
import from "@clerk/nextjs/server" and import verifyToken from the vendor
abstraction instead; update the import statement that currently references
verifyToken to use "@vendor/clerk/server" so the module uses the vendor wrapper
(symbol: verifyToken) rather than the direct Clerk SDK.
---
Nitpick comments:
In `@apps/desktop/src/main/__tests__/auth-flow.test.ts`:
- Around line 96-127: The helper captureSigninUrl currently busy-waits up to 2s
polling shellOpenExternalMock and emittedEvents; replace this wall-clock polling
with an explicit promise/deferred that the test resolves when the mocked signal
occurs: create a Deferred (resolve/reject) used by captureSigninUrl and have
shellOpenExternalMock (and/or the spyStdout()/event emitter used in tests) call
resolve when the signin URL is produced; then await that deferred inside
captureSigninUrl instead of the for-loop, and keep the existing parsing of the
URL/state/code_challenge/redirect_uri; reference captureSigninUrl,
shellOpenExternalMock, emittedEvents and spyStdout() when wiring the deferred so
the test deterministically awaits the mock trigger rather than polling.
- Around line 627-645: Replace the real-time sleep with deterministic fake
timers: before calling mod.maybeAutoBeginSignIn() in this test, switch to fake
timers (e.g., jest.useFakeTimers()), then call mod.maybeAutoBeginSignIn(),
advance timers by the configured timeout (LIGHTFAST_DESKTOP_AUTH_TIMEOUT_MS /
the 100ms value used in loadAuthFlow) plus a small margin using
jest.advanceTimersByTime(...), assert the "auth_signin_url" event and shell
behavior, and finally restore real timers (jest.useRealTimers()) and any stdout
or module restores; target the test harness around maybeAutoBeginSignIn(),
loadAuthFlow, and getTokenMock to make the timeout deterministic and remove the
setTimeout-based sleep.
In `@vendor/observability/package.json`:
- Around line 78-80: Add `@sentry/browser` and `@sentry/electron` to the root
package.json catalog and switch local pins to catalog:; specifically, add "
`@sentry/browser`": "^10.49.0" and "@sentry/electron": "^7.11.0" to the root
"catalog" object, then update vendor/observability/package.json (replace
"@sentry/browser": "^10.49.0" and "@sentry/electron": "^7.11.0") and the
apps/desktop package.json entries for the same packages to use "catalog:"
instead of pinned versions to ensure a single shared source of truth for
`@sentry/browser` and `@sentry/electron`.
🪄 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: 17703efc-30a6-4ca1-83f2-82a8cadb9400
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yamlthoughts/shared/plans/2026-05-06-pr627-merge-readiness.mdis excluded by!thoughts/**
📒 Files selected for processing (30)
apps/app/microfrontends.jsonapps/app/package.jsonapps/app/src/app/(app)/(user)/(pending-not-allowed)/_components/client-auth-bridge.tsxapps/app/src/app/(app)/(user)/(pending-not-allowed)/desktop/auth/_components/desktop-auth-client.tsxapps/app/src/app/api/cli/lib/verify-jwt.tsapps/app/src/app/api/desktop/auth/code/route.test.tsapps/app/src/app/api/desktop/auth/code/route.tsapps/app/src/app/api/desktop/auth/exchange/route.test.tsapps/app/src/app/api/desktop/auth/lib/code-store.tsapps/app/src/instrumentation.tsapps/app/src/proxy.tsapps/desktop/forge.config.tsapps/desktop/package.jsonapps/desktop/src/main/__tests__/auth-flow.test.tsapps/desktop/src/main/__tests__/protocol.test.tsapps/desktop/src/main/auth-flow.tsapps/desktop/src/main/auth-store.tsapps/desktop/src/main/index.tsapps/desktop/src/main/protocol.tsapps/desktop/src/main/sentry.tsapps/desktop/src/main/windows/factory.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/src/main.tsapps/desktop/src/renderer/src/react/app-shell.tsxapps/desktop/src/shared/ipc.tsapps/platform/src/instrumentation.tsapps/www/src/instrumentation.tspackage.jsonpnpm-workspace.yamlvendor/observability/package.json
✅ Files skipped from review due to trivial changes (1)
- pnpm-workspace.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/preload/preload.ts
- apps/app/src/app/api/desktop/auth/lib/code-store.ts
The 2026-05-06 live verification got far enough to confirm Phase 5's behavioral change (createAppUrl() correctly routes through getRuntimeConfig().appOrigin — desktop emitted auth_signin_url with origin https://lightfast.localhost as expected). Beyond that, the lightfast-dev:// URL scheme dispatch did not deliver to the running unpackaged dev Electron; lsregister confirms no app claims that scheme on this host. This is the known unpackaged-Electron limitation called out in the desktop-signin skill, not a Phase 5 regression. Subsequent re-runs gated on either fixing the local URL-scheme registration or running against a packaged build. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…FAST_API_URL
The sign-in URL composition already routed through createAppUrl() (Phase 5),
but exchangeCode's POST kept calling getApiOrigin() which read a different
env var (LIGHTFAST_API_URL) than what scripts/with-desktop-env.mjs injects
(LIGHTFAST_APP_ORIGIN). With pnpm dev:desktop, getApiOrigin() fell through
to the legacy http://localhost:3024 default and the exchange POST failed
with auth_signin_failed{reason:"exchange_failed"}.
Drop getApiOrigin(); have exchangeCode build its URL via createAppUrl(),
so both the sign-in URL and the exchange POST consume the same
getRuntimeConfig().appOrigin source. Re-verified end-to-end with no
LIGHTFAST_API_URL set.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pull in 5 desktop release PRs that landed since fork: - #638 codesign pre-fixes - #639 release-pipeline fixes - #640 Vite sourcemaps - #641 observability hardening (IPC routing + debug-id symbolication) - #642 Sentry release/finalize fix Conflicts resolved: - apps/desktop/src/main/windows/factory.ts — comment-only divergence on the Vite/CJS __dirname workaround. Took main's more detailed wording. - apps/desktop/src/renderer/src/main.ts — main removed renderer-side Sentry init (the v10 SDK silently no-op'd; events forward via IPC to main now). Our HEAD's initSentryBrowser block referenced sentryInit which main also dropped. Took main's version. - pnpm-lock.yaml — regenerated against the merged package.json files. Verified: 38/38 desktop tests pass; @lightfast/desktop and @lightfast/app typechecks clean. Live PKCE sign-in re-verification deferred to post-commit step.
…v restore T2 (apps/app/.../client-auth-bridge.tsx): Swap direct @sentry/nextjs import for the @vendor/observability/sentry-nextjs re-export so this client component honors the repo's vendor-abstraction rule. Adds captureException and captureMessage to the wrapper's re-exports. T3 (apps/desktop/.../__tests__/auth-flow.test.ts): loadAuthFlow() mutated process.env then awaited a dynamic import without ensuring restore() runs on import failure. If the import throws, the mutation now leaks into every subsequent test. Hoist restore() above the import and wrap the import in try/catch so env state is reverted before the rejection propagates. Verified: 38/38 desktop tests pass; @lightfast/app and @vendor/observability typecheck clean.
vendor/observability/package.json had `@sentry/browser` and `@sentry/electron` on inline ^X.Y.Z specs while `@sentry/core` and `@sentry/nextjs` were on `catalog:`. Adds the two missing entries to pnpm-workspace.yaml's catalog (@sentry/browser ^10.49.0 to match the rest of the v10 Sentry surface, @sentry/electron ^7.11.0 to match the existing pin) and switches both deps to `catalog:`. Prevents version drift across the workspace. Verified: 38/38 desktop tests pass; @vendor/observability, @lightfast/desktop, @lightfast/app typecheck clean.
The T2 swap from `@sentry/nextjs` to `@vendor/observability/sentry-nextjs`
broke two CI checks on `bf1699fa5`:
1. Quality (biome organizeImports): `@vendor/clerk/client` must precede
`@vendor/observability/sentry-nextjs` alphabetically. Reorder the
import block.
2. Test (vitest, client-auth-bridge.test.tsx): the test still mocked
`@sentry/nextjs`, so after the swap the wrapper's real
captureException/captureMessage ran instead of the mocks. Update
`vi.mock("@sentry/nextjs")` → `vi.mock("@vendor/observability/sentry-nextjs")`.
Verified: 120/120 app tests pass; ultracite check clean on both files.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
setTokenreturns boolean),ClientAuthBridgeerror state when Clerk reports signed-outlightfast:///lightfast-dev://scheme and OAuth 2.0 code exchange (/api/desktop/auth/code+/api/desktop/auth/exchangevia Upstash Redis GETDEL). Agent-mode stdout event grammar (auth_signin_url,auth_signed_in,auth_already_signed_in,auth_signin_failed)/api/desktop/*,import.meta.url→__dirnamefor CJS bundle compatCommits
fix(desktop/auth): resolve CodeRabbit PR #614 review findingstest(desktop/auth): automate Phase 2 manual verification as vitest coveragetest(api/app): add expired-Bearer-no-cookie resolveClerkSession casechore(desktop): upgrade electron 41, vite 8, plugin-react 6chore(deps): address CodeRabbit PR #622 findingsfix(desktop/windows): replace import.meta.url with __dirname for CJS bundlefix(app/proxy): allow /api/desktop/* through Clerk middlewarefeat(desktop): custom URL scheme + PKCE sign-in flowchore(deps): reconcile pnpm-lock catalog entries after mergeVerification
All 5 phases live-verified end-to-end against real Clerk dev, real Electron 41, real Upstash Redis:
auth.binpersisted (~14s e2e)LIGHTFAST_DESKTOP_AGENT_MODE=1→ stdoutauth_signin_url→agent-browser→auth_signed_inauth_already_signed_inonlyauth_signin_failed{reason:"exchange_failed"}Test plan
pnpm --filter @api/app vitest run— resolveClerkSession + code/exchange route testspnpm --filter @lightfast/desktop vitest run— auth-flow, protocol, auth-focus-gate testspnpm --filter @lightfast/app typecheckpnpm --filter @lightfast/desktop typecheckpnpm check— Biome lint/formatauth.binatomically🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Refactor