feat(desktop): add OAuth authentication for desktop app#360
feat(desktop): add OAuth authentication for desktop app#360saddlepaddle merged 13 commits intomainfrom
Conversation
WalkthroughAdds PKCE-based desktop OAuth with JWT auth codes, access/refresh tokens, refresh rotation, deep-link handlers, desktop auth client (persistence + refresh scheduling), TRPC context fallback to desktop JWTs, and related env, constants, router, and UI updates. Changes
Sequence DiagramsequenceDiagram
actor User
participant Desktop as "Desktop App"
participant Browser as "System Browser"
participant Web as "Web App"
participant API as "API"
participant Store as "Token Storage"
User->>Desktop: Click "Sign in"
Desktop->>Desktop: PKCE: generate verifier, challenge, state
Desktop->>Browser: Open Web App auth URL with code_challenge & state
Browser->>Web: GET /api/auth/desktop/[provider]?code_challenge=...&state=...
Web->>Web: Ensure user authenticated (Clerk)
Web->>Browser: Redirect to superset://auth/callback?code=<auth_code>&state=...
Browser->>Desktop: Deep link callback (code,state)
Desktop->>Desktop: Validate deep link, consume PKCE state
Desktop->>API: POST /api/auth/desktop/token { code, code_verifier }
API->>API: Verify auth code & PKCE, issue access & refresh tokens
API-->>Desktop: { access_token, access_expires_at, refresh_token, refresh_expires_at }
Desktop->>Store: Persist session (encrypted)
Desktop->>Desktop: Schedule background refresh before expiry
Desktop->>Desktop: Emit auth state-changed
rect rgba(200,200,255,0.15)
Desktop->>Desktop: On near-expiry, POST /api/auth/desktop/refresh { refresh_token }
API->>API: Verify refresh token, rotate tokens
API-->>Desktop: New tokens -> Desktop updates Store
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 Preview resources have been processed for cleanup |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (21)
.env.example (1)
29-29: Consider alphabetical ordering for consistency.The environment variable is correctly documented, but static analysis suggests ordering it before
VITE_DEV_SERVER_PORTfor consistency. While this is a minor style concern, maintaining alphabetical order within sections can improve maintainability.Apply this diff if you'd like to address the ordering:
# Desktop App -VITE_DEV_SERVER_PORT=4927 DESKTOP_AUTH_SECRET=your_desktop_auth_secret_here_min_32_chars +VITE_DEV_SERVER_PORT=4927apps/desktop/src/main/lib/auth/token-storage.ts (1)
20-30: Consider error handling for save operation.The
save()method currently returns silently when encryption is unavailable, but this might cause confusion during debugging. Consider whether consumers should be notified of this state.Optionally, you could return a success indicator:
-async save(session: AuthSession): Promise<void> { +async save(session: AuthSession): Promise<boolean> { if (!safeStorage.isEncryptionAvailable()) { console.warn( "[auth] Secure storage not available, session will not be persisted", ); - return; + return false; } const encrypted = safeStorage.encryptString(JSON.stringify(session)); await fs.writeFile(this.filePath, encrypted); + return true; }apps/desktop/scripts/patch-dev-protocol.ts (1)
74-80: Consider logging PlistBuddy command failures for debugging.The script silently ignores all PlistBuddy command errors, which makes it difficult to debug partial failures. While idempotency is good (ignoring "already exists" errors), other failures (e.g., permissions, malformed commands) should be visible.
Consider differentiating between expected and unexpected errors:
for (const cmd of commands) { try { execSync(`/usr/libexec/PlistBuddy -c "${cmd}" "${PLIST_PATH}" 2>/dev/null`); - } catch { + } catch (error) { // Ignore errors (e.g., key already exists) + const errorMessage = error instanceof Error ? error.message : String(error); + if (!errorMessage.includes("Already Exists")) { + console.debug(`[patch-dev-protocol] Command skipped: ${cmd}`); + } } }apps/desktop/src/shared/auth.ts (1)
17-23: Add JSDoc comments toAuthSessionfields to clarify timestamp units.The
accessTokenExpiresAtandrefreshTokenExpiresAtfields should be documented to indicate they are Unix timestamps in milliseconds. While the codebase is already consistent (tokens are generated asDate.now() + TOKEN_CONFIG.* * 1000), the interface itself lacks this documentation, which can lead to developer confusion when integrating with these fields.export interface AuthSession { accessToken: string; + /** Unix timestamp in milliseconds when access token expires */ accessTokenExpiresAt: number; refreshToken: string; + /** Unix timestamp in milliseconds when refresh token expires */ refreshTokenExpiresAt: number; user: AuthUser; }apps/desktop/src/renderer/hooks/useAuth.ts (1)
14-22: Subscription invalidation approach is fine.
Minor: if you want to be explicit about ignoring the returned promise, you canvoid utils.auth.getState.invalidate().apps/api/src/trpc/context.ts (1)
83-99: Mock Clerk session object may need maintenance as Clerk evolves.The manually constructed session object works but is tightly coupled to Clerk's internal structure. The
as unknown ascast suppresses type checking, which could hide future breaking changes if Clerk updates its session type.Consider adding a comment documenting which Clerk version this was tested against, or creating a dedicated type for desktop sessions that
createTRPCContextcan accept alongside Clerk sessions.apps/desktop/src/renderer/components/AuthButton/AuthButton.tsx (2)
23-37: Consider replacingalert()with a proper toast notification.Using browser
alert()blocks the UI thread and provides a poor user experience in a desktop app. Consider using a toast/notification component from@superset/uifor non-blocking feedback.
84-89: Potential runtime error when computing initials from names with edge cases.If
displayNamecontains consecutive spaces,.split(" ")produces empty strings, and accessingn[0]on an empty string returnsundefined. While.slice(0, 2)handles the overall length, the.join("")could produce unexpected results.- const initials = displayName - .split(" ") - .map((n) => n[0]) - .join("") - .toUpperCase() - .slice(0, 2); + const initials = displayName + .split(" ") + .filter(Boolean) + .map((n) => n[0]) + .join("") + .toUpperCase() + .slice(0, 2);apps/api/src/app/api/auth/desktop/refresh/route.ts (1)
71-85: Token creation logic duplicated from token/route.ts.The access token creation (lines 75-85) duplicates
createAccessTokenfromapps/api/src/app/api/auth/desktop/token/route.ts. Consider extracting shared token utilities to reduce duplication.apps/web/src/app/auth/desktop/callback/page.tsx (1)
27-34: Race condition possible between state update and redirect.The
setHasAttempted(true)andopenDesktopApp()calls happen synchronously, but React's state update is asynchronous. If the component re-renders before the redirect completes (e.g., due to a parent update), the guard works correctly. However, adding the redirect call inside the state setter callback would be more explicit about the intent.This is a minor concern since the redirect typically happens immediately.
apps/api/src/app/api/auth/desktop/token/route.ts (1)
119-128: Type assertion on JWT payload warrants runtime validation.The
payload = result.payload as AuthCodePayloadcast assumes the payload structure matches without verifying all fields. Whilepayload.type !== "auth_code"check exists, fields likecodeChallengeare not validated before use on line 143.Consider adding explicit field validation similar to
verifyDesktopTokenincontext.ts:try { const result = await jwtVerify(code, secret); - payload = result.payload as AuthCodePayload; + const p = result.payload; + if ( + typeof p.userId !== "string" || + typeof p.email !== "string" || + typeof p.codeChallenge !== "string" + ) { + return NextResponse.json( + { error: "Invalid auth code payload" }, + { status: 400 }, + ); + } + payload = p as AuthCodePayload; } catch (verifyError) {apps/desktop/src/main/lib/auth/deep-link-handler.ts (2)
4-5: Environment variable naming inconsistency.
NEXT_PUBLIC_API_URLis a Next.js convention for client-side env vars. In Electron main process, consider using a more appropriate naming convention likeAPI_URLorSUPERSET_API_URL. The current naming may cause confusion about where this variable is used.
102-125: Add timeout to prevent hanging requests.The
fetchcall has no timeout configured. If the API is unresponsive, this could leave the auth flow in a pending state indefinitely. Consider usingAbortControllerwith a reasonable timeout.async function exchangeCodeForTokens( code: string, codeVerifier: string, ): Promise<TokenExchangeResponse> { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 30000); + - const response = await fetch(`${API_URL}/api/auth/desktop/token`, { + const response = await fetch(`${API_URL}/api/auth/desktop/token`, { method: "POST", headers: { "Content-Type": "application/json", }, body: JSON.stringify({ code, code_verifier: codeVerifier, }), + signal: controller.signal, }); + + clearTimeout(timeoutId);apps/desktop/src/main/lib/auth/pkce.ts (1)
30-80: Singleton store may cause issues with concurrent auth attempts.If a user initiates a sign-in, then initiates another before the first completes, the second
createChallenge()call overwrites the first verifier. The first flow would then fail silently when its callback arrives. This is likely acceptable for a desktop app (user can only click sign-in once at a time), but consider documenting this behavior or logging when a verifier is overwritten.createChallenge(): { codeChallenge: string; codeVerifier: string } { + if (this.codeVerifier) { + console.log("[pkce] Overwriting existing verifier - previous auth flow abandoned"); + } this.codeVerifier = generateCodeVerifier(); this.createdAt = Date.now();apps/desktop/src/lib/trpc/routers/auth/index.ts (3)
48-52: Unnecessary type assertion.The
as AuthProvidercast is unnecessary since Zod'sz.enum(["google", "github"])already narrows the type. IfAuthProviderhas additional values, this could mask type errors.signIn: publicProcedure .input(z.object({ provider: z.enum(["google", "github"]) })) .mutation(async ({ input }) => { - return authService.signIn(input.provider as AuthProvider, getWindow); + return authService.signIn(input.provider, getWindow); }),If the types don't align, consider importing and using
AuthProviderin the Zod schema or updating the schema to match.
65-98: Add return type and timeout fortestApiCall.The mutation returns an untyped object which reduces type safety. Also, the fetch call lacks a timeout, risking indefinite hangs.
Consider defining a return type and adding timeout:
+type TestApiResult = + | { success: true; data: unknown } + | { success: false; error: string }; + testApiCall: publicProcedure.mutation(async () => { const token = await authService.getAccessToken(); if (!token) { return { success: false, error: "Not authenticated" }; } + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10000); + try { const response = await fetch(`${API_URL}/api/trpc/user.me`, { method: "GET", headers: { Authorization: `Bearer ${token}`, "Content-Type": "application/json", }, + signal: controller.signal, }); + clearTimeout(timeoutId);
8-9: Consider centralizing API_URL configuration.
API_URLis defined identically in multiple files (deep-link-handler.ts,auth-service.ts, and here). Consider extracting to a shared config module to ensure consistency and single point of change.apps/desktop/src/main/lib/auth/auth-service.ts (4)
42-68: WraprefreshAccessTokencall in try-catch during initialization.If
refreshAccessToken()throws unexpectedly duringinitialize(), the session is restored but refresh scheduling may not complete, potentially leaving the service in an inconsistent state.// Check if access token needs refresh if (this.shouldRefreshAccessToken()) { console.log("[auth] Access token expired/expiring, refreshing..."); - await this.refreshAccessToken(); + try { + await this.refreshAccessToken(); + } catch (err) { + console.error("[auth] Initial token refresh failed:", err); + // Continue with expired token - next getAccessToken call will retry + } } // Schedule next refresh this.scheduleRefresh();
108-134: Unused_getWindowparameter.The
_getWindowparameter is declared but never used. If it's reserved for future use (e.g., focusing the app window after auth), consider adding a TODO comment explaining the intent. Otherwise, it could be removed from the interface.
178-233: Add timeout to refresh token request.Similar to the token exchange, the refresh request has no timeout. A hanging request could block token refresh indefinitely, eventually causing auth failures when the access token expires.
private async refreshAccessToken(): Promise<void> { if (!this.session || this.isRefreshing) return; this.isRefreshing = true; + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 30000); try { console.log("[auth] Refreshing access token..."); const response = await fetch(`${API_URL}/api/auth/desktop/refresh`, { method: "POST", headers: { "Content-Type": "application/json", }, body: JSON.stringify({ refresh_token: this.session.refreshToken, }), + signal: controller.signal, }); + clearTimeout(timeoutId);
34-38: Consider adding TypeScript event typing.The
EventEmitterusage is untyped, meaningemitandoncalls for"state-changed"aren't type-checked. For better type safety, consider using a typed event emitter pattern.interface AuthServiceEvents { 'state-changed': (state: AuthState) => void; } // Or use a typed EventEmitter wrapper/library
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
.env.example(1 hunks)apps/api/package.json(1 hunks)apps/api/src/app/api/auth/desktop/refresh/route.ts(1 hunks)apps/api/src/app/api/auth/desktop/token/route.ts(1 hunks)apps/api/src/env.ts(1 hunks)apps/api/src/trpc/context.ts(1 hunks)apps/desktop/package.json(2 hunks)apps/desktop/scripts/patch-dev-protocol.ts(1 hunks)apps/desktop/src/lib/trpc/routers/auth/index.ts(1 hunks)apps/desktop/src/lib/trpc/routers/index.ts(2 hunks)apps/desktop/src/main/index.ts(3 hunks)apps/desktop/src/main/lib/auth/auth-service.ts(1 hunks)apps/desktop/src/main/lib/auth/deep-link-handler.ts(1 hunks)apps/desktop/src/main/lib/auth/index.ts(1 hunks)apps/desktop/src/main/lib/auth/pkce.ts(1 hunks)apps/desktop/src/main/lib/auth/token-storage.ts(1 hunks)apps/desktop/src/renderer/components/AuthButton/AuthButton.tsx(1 hunks)apps/desktop/src/renderer/components/AuthButton/index.ts(1 hunks)apps/desktop/src/renderer/hooks/useAuth.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/index.tsx(2 hunks)apps/desktop/src/shared/auth.ts(1 hunks)apps/web/package.json(1 hunks)apps/web/src/app/api/auth/desktop/[provider]/route.ts(1 hunks)apps/web/src/app/auth/desktop/callback/page.tsx(1 hunks)apps/web/src/env.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for code formatting and linting, running at root level for speed
Files:
apps/api/src/env.tsapps/desktop/src/renderer/components/AuthButton/AuthButton.tsxapps/web/package.jsonapps/desktop/src/lib/trpc/routers/index.tsapps/web/src/env.tsapps/api/package.jsonapps/desktop/src/renderer/screens/main/components/TopBar/index.tsxapps/desktop/src/renderer/components/AuthButton/index.tsapps/desktop/src/main/lib/auth/token-storage.tsapps/api/src/app/api/auth/desktop/token/route.tsapps/desktop/scripts/patch-dev-protocol.tsapps/desktop/src/lib/trpc/routers/auth/index.tsapps/desktop/src/main/lib/auth/auth-service.tsapps/web/src/app/auth/desktop/callback/page.tsxapps/desktop/package.jsonapps/desktop/src/main/lib/auth/index.tsapps/api/src/app/api/auth/desktop/refresh/route.tsapps/web/src/app/api/auth/desktop/[provider]/route.tsapps/desktop/src/shared/auth.tsapps/api/src/trpc/context.tsapps/desktop/src/renderer/hooks/useAuth.tsapps/desktop/src/main/index.tsapps/desktop/src/main/lib/auth/pkce.tsapps/desktop/src/main/lib/auth/deep-link-handler.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid
anytype and prioritize type safety in TypeScript code
Files:
apps/api/src/env.tsapps/desktop/src/renderer/components/AuthButton/AuthButton.tsxapps/desktop/src/lib/trpc/routers/index.tsapps/web/src/env.tsapps/desktop/src/renderer/screens/main/components/TopBar/index.tsxapps/desktop/src/renderer/components/AuthButton/index.tsapps/desktop/src/main/lib/auth/token-storage.tsapps/api/src/app/api/auth/desktop/token/route.tsapps/desktop/scripts/patch-dev-protocol.tsapps/desktop/src/lib/trpc/routers/auth/index.tsapps/desktop/src/main/lib/auth/auth-service.tsapps/web/src/app/auth/desktop/callback/page.tsxapps/desktop/src/main/lib/auth/index.tsapps/api/src/app/api/auth/desktop/refresh/route.tsapps/web/src/app/api/auth/desktop/[provider]/route.tsapps/desktop/src/shared/auth.tsapps/api/src/trpc/context.tsapps/desktop/src/renderer/hooks/useAuth.tsapps/desktop/src/main/index.tsapps/desktop/src/main/lib/auth/pkce.tsapps/desktop/src/main/lib/auth/deep-link-handler.ts
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/renderer/components/AuthButton/AuthButton.tsxapps/desktop/src/lib/trpc/routers/index.tsapps/desktop/src/renderer/screens/main/components/TopBar/index.tsxapps/desktop/src/renderer/components/AuthButton/index.tsapps/desktop/src/main/lib/auth/token-storage.tsapps/desktop/scripts/patch-dev-protocol.tsapps/desktop/src/lib/trpc/routers/auth/index.tsapps/desktop/src/main/lib/auth/auth-service.tsapps/desktop/src/main/lib/auth/index.tsapps/desktop/src/shared/auth.tsapps/desktop/src/renderer/hooks/useAuth.tsapps/desktop/src/main/index.tsapps/desktop/src/main/lib/auth/pkce.tsapps/desktop/src/main/lib/auth/deep-link-handler.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/renderer/components/AuthButton/AuthButton.tsxapps/desktop/src/lib/trpc/routers/index.tsapps/desktop/src/renderer/screens/main/components/TopBar/index.tsxapps/desktop/src/renderer/components/AuthButton/index.tsapps/desktop/src/main/lib/auth/token-storage.tsapps/desktop/scripts/patch-dev-protocol.tsapps/desktop/src/lib/trpc/routers/auth/index.tsapps/desktop/src/main/lib/auth/auth-service.tsapps/desktop/src/main/lib/auth/index.tsapps/desktop/src/shared/auth.tsapps/desktop/src/renderer/hooks/useAuth.tsapps/desktop/src/main/index.tsapps/desktop/src/main/lib/auth/pkce.tsapps/desktop/src/main/lib/auth/deep-link-handler.ts
**/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/components/**/*.{ts,tsx}: Structure project folders as one folder per component with PascalCase naming (ComponentName/ComponentName.tsx + index.ts barrel export)
Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Use one component per file (no multi-component files)
Files:
apps/desktop/src/renderer/components/AuthButton/AuthButton.tsxapps/desktop/src/renderer/screens/main/components/TopBar/index.tsxapps/desktop/src/renderer/components/AuthButton/index.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Call IPC methods from renderer process using window.ipcRenderer.invoke with type-safe object parameters
Files:
apps/desktop/src/renderer/components/AuthButton/AuthButton.tsxapps/desktop/src/renderer/screens/main/components/TopBar/index.tsxapps/desktop/src/renderer/components/AuthButton/index.tsapps/desktop/src/renderer/hooks/useAuth.ts
apps/desktop/src/main/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
Load environment variables in apps/desktop/src/main/index.ts with override: true before any imports
Files:
apps/desktop/src/main/index.ts
🧠 Learnings (7)
📚 Learning: 2025-12-12T05:45:09.673Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T05:45:09.673Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables in apps/desktop/src/main/index.ts with override: true before any imports
Applied to files:
apps/api/src/env.ts.env.exampleapps/web/src/env.ts
📚 Learning: 2025-12-12T05:45:09.672Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T05:45:09.672Z
Learning: Applies to packages/db/src/**/*.{ts,tsx} : Use Drizzle ORM for all database operations in the `packages/db` package
Applied to files:
apps/api/package.json
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx,js,jsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/renderer/screens/main/components/TopBar/index.tsxapps/api/src/trpc/context.ts
📚 Learning: 2025-12-12T05:45:09.673Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T05:45:09.673Z
Learning: Use Bun as the package manager (not npm, yarn, or pnpm) for the monorepo
Applied to files:
apps/desktop/scripts/patch-dev-protocol.ts
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Please use alias as defined in `tsconfig.json` when possible
Applied to files:
apps/desktop/src/main/lib/auth/index.tsapps/desktop/src/shared/auth.ts
📚 Learning: 2025-12-12T05:45:09.672Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T05:45:09.672Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx,js,jsx} apps/desktop/src/lib/**/*.{ts,tsx,js,jsx} apps/desktop/src/shared/**/*.{ts,tsx,js,tsx} : Never import Node.js modules in renderer process or shared code, only in main process
Applied to files:
apps/desktop/src/shared/auth.ts
📚 Learning: 2025-12-12T05:45:09.673Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T05:45:09.673Z
Learning: Applies to apps/desktop/src/main/lib/*.ts : Implement IPC handlers accepting object parameters (not positional parameters) in apps/desktop/src/main/lib/*.ts files
Applied to files:
apps/desktop/src/main/lib/auth/deep-link-handler.ts
🧬 Code graph analysis (9)
apps/desktop/src/renderer/components/AuthButton/AuthButton.tsx (5)
apps/desktop/src/renderer/hooks/useAuth.ts (1)
useAuth(8-65)packages/ui/src/components/ui/button.tsx (1)
Button(60-60)packages/ui/src/components/ui/dropdown-menu.tsx (5)
DropdownMenu(249-249)DropdownMenuTrigger(251-251)DropdownMenuContent(252-252)DropdownMenuItem(255-255)DropdownMenuSeparator(259-259)apps/desktop/src/main/lib/auth/auth-service.ts (2)
signIn(108-134)signOut(160-163)packages/ui/src/components/ui/avatar.tsx (3)
Avatar(53-53)AvatarImage(53-53)AvatarFallback(53-53)
apps/desktop/src/lib/trpc/routers/index.ts (1)
apps/desktop/src/lib/trpc/routers/auth/index.ts (1)
createAuthRouter(15-100)
apps/desktop/src/renderer/screens/main/components/TopBar/index.tsx (2)
apps/desktop/src/renderer/components/AuthButton/AuthButton.tsx (1)
AuthButton(19-128)apps/desktop/src/renderer/components/AuthButton/index.ts (1)
AuthButton(1-1)
apps/desktop/src/main/lib/auth/token-storage.ts (3)
apps/desktop/src/main/lib/app-environment.ts (1)
SUPERSET_HOME_DIR(8-8)apps/desktop/src/shared/auth.ts (1)
AuthSession(17-23)apps/desktop/src/main/lib/auth/index.ts (1)
tokenStorage(4-4)
apps/api/src/app/api/auth/desktop/token/route.ts (2)
apps/desktop/src/shared/auth.ts (1)
TOKEN_CONFIG(28-35)apps/api/src/app/api/auth/desktop/refresh/route.ts (1)
POST(35-116)
apps/desktop/src/lib/trpc/routers/auth/index.ts (4)
apps/desktop/src/lib/trpc/index.ts (1)
router(15-15)apps/desktop/src/main/lib/auth/auth-service.ts (1)
authService(278-278)apps/desktop/src/main/lib/auth/index.ts (1)
authService(1-1)apps/desktop/src/shared/auth.ts (2)
AuthState(12-15)AuthProvider(37-37)
apps/api/src/app/api/auth/desktop/refresh/route.ts (2)
apps/desktop/src/shared/auth.ts (1)
TOKEN_CONFIG(28-35)apps/api/src/app/api/auth/desktop/token/route.ts (1)
POST(95-183)
apps/desktop/src/renderer/hooks/useAuth.ts (2)
apps/desktop/src/main/lib/auth/auth-service.ts (2)
signIn(108-134)signOut(160-163)apps/desktop/src/shared/auth.ts (1)
AuthProvider(37-37)
apps/desktop/src/main/lib/auth/deep-link-handler.ts (3)
apps/desktop/src/main/lib/auth/index.ts (3)
AuthDeepLinkResult(2-2)handleAuthDeepLink(3-3)isAuthDeepLink(3-3)apps/desktop/src/shared/auth.ts (2)
AuthSession(17-23)AuthUser(5-10)apps/desktop/src/main/lib/auth/pkce.ts (1)
pkceStore(82-82)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 29-29: [UnorderedKey] The DESKTOP_AUTH_SECRET key should go before the VITE_DEV_SERVER_PORT key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Admin
- GitHub Check: Build
🔇 Additional comments (33)
apps/web/package.json (1)
27-27: LGTM! Consistent JOSE version across packages.The addition of
jose@^6.1.3is appropriate for JWT handling in the desktop authentication flows. The version is consistent across web, api, and desktop packages, which is good for maintainability.apps/api/package.json (1)
23-23: LGTM! Consistent JOSE dependency.The
jose@^6.1.3dependency is correctly added and matches the version used in other packages, ensuring consistency across the monorepo.apps/desktop/package.json (2)
19-19: LGTM! Development workflow enhancement.The updated
predevscript now registers thesuperset-dev://URL scheme before starting development, which is essential for testing the OAuth deep-link flow in development.
70-70: LGTM! Consistent JOSE version.The
jose@^6.1.3dependency is correctly added and consistent with the versions in the web and api packages.apps/desktop/scripts/patch-dev-protocol.ts (1)
17-51: LGTM! Well-designed idempotent script.The script includes:
- Platform-specific guard to skip non-macOS systems
- Existence check for Electron.app before attempting to patch
- Idempotency check to avoid redundant patching
- Early exit for already-patched scenarios
This design prevents unnecessary work and ensures the script can be run multiple times safely.
apps/desktop/src/shared/auth.ts (2)
28-35: LGTM! Well-documented token configuration.The
TOKEN_CONFIGconstant provides clear documentation of token lifetimes with human-readable comments. The values are reasonable:
- 1 hour access tokens
- 30 day refresh tokens
- 5 minute refresh threshold
5-15: LGTM! Clean type definitions.The
AuthUserandAuthStateinterfaces are well-structured and appropriately handle nullable fields (avatarUrl,user).apps/desktop/src/renderer/components/AuthButton/index.ts (1)
1-1: LGTM! Follows component structure guidelines.The barrel export follows the recommended pattern of one folder per component with PascalCase naming and index.ts export file.
As per coding guidelines, structure project folders as one folder per component with PascalCase naming (ComponentName/ComponentName.tsx + index.ts barrel export).
apps/desktop/src/renderer/screens/main/components/TopBar/index.tsx (2)
1-1: Verifyrenderer/components/AuthButtonresolves via a proper barrel export.
The provided snippet forapps/desktop/src/renderer/components/AuthButton/index.tsdoesn’t clearly show an export; if it’s incorrect,TopBarwill fail to compile.Also applies to: 29-33
27-33: TopBar composition change looks good.
PlacingAuthButtonnext to workspace/settings/help is a sensible UX location.apps/api/src/env.ts (1)
4-12: Good: enforce a minimum length forDESKTOP_AUTH_SECRET(server-only).
This is the right place to fail fast on misconfig.apps/web/src/env.ts (1)
13-18: Good: web server env now aligns with API forDESKTOP_AUTH_SECRET.apps/desktop/src/lib/trpc/routers/index.ts (1)
1-3: tRPC wiring forauthsubtree is clean and consistent with other routers.Also applies to: 24-29
apps/desktop/src/main/lib/auth/index.ts (1)
1-4: Barrel exports are fine (nice ergonomics for main process imports).apps/desktop/src/main/index.ts (3)
12-15: PROTOCOL_SCHEME split (dev vs prod) is sensible.
1-7: Check the repo guideline: env vars should be loaded (override: true) before any imports inapps/desktop/src/main/index.ts.
I don’t see that in the shown file header; if env-loading happens elsewhere and this guideline is obsolete, ignore—otherwise this can cause confusing prod/dev config issues.
47-50:await authService.initialize()placement looks reasonable.
Doing it after DB/app-state init but before window creation is a good default.apps/api/src/trpc/context.ts (1)
20-51: Well-structured token verification with proper type discrimination.The
verifyDesktopTokenfunction correctly rejects auth_code and refresh tokens, ensuring only access tokens are accepted for API authentication. The validation of required fields (userId, email) before returning the payload is a good defensive practice.apps/desktop/src/renderer/components/AuthButton/AuthButton.tsx (1)
1-14: Clean component implementation following project conventions.The component correctly uses tRPC for IPC as per coding guidelines, follows the single-component-per-file pattern, and properly leverages
@superset/uiprimitives. The loading, signed-out, and signed-in states are well-handled.Also applies to: 39-127
apps/api/src/app/api/auth/desktop/refresh/route.ts (1)
87-101: Comment about token invalidation is misleading for stateless JWTs.The comment "rotation - old one becomes invalid" suggests revocation, but stateless JWTs remain valid until expiration unless actively blocklisted. If a refresh token is intercepted before rotation, both tokens work until their respective expiry.
This is acceptable for many use cases, but the comment should accurately reflect the behavior, or consider implementing a token family/generation counter if stricter revocation is needed.
- // Create a new refresh token (rotation - old one becomes invalid) + // Create a new refresh token (rotation - client should discard old token) + // Note: Old token remains cryptographically valid until expiryapps/web/src/app/auth/desktop/callback/page.tsx (1)
100-121: Correct use of Suspense foruseSearchParams.Wrapping
CallbackContentinSuspenseis required in Next.js App Router when usinguseSearchParamsto avoid hydration issues. The loading fallback provides good UX.apps/api/src/app/api/auth/desktop/token/route.ts (2)
138-149: Correct PKCE S256 implementation.The PKCE verification correctly implements the S256 method: computing SHA256 of the code_verifier and comparing it to the stored code_challenge using base64url encoding, as specified in RFC 7636.
42-85: Well-structured token creation helpers.The
createAccessTokenandcreateRefreshTokenfunctions are cleanly separated and return both the token and expiry timestamp. These should be extracted to a shared module sorefresh/route.tscan reuse them instead of duplicating the logic.apps/desktop/src/main/lib/auth/deep-link-handler.ts (3)
10-21: Minor inconsistency inuserproperty casing.The
TokenExchangeResponse.usertype usesavatarUrl(camelCase), which matches theAuthUserinterface. However, ensure the API endpoint at/api/auth/desktop/tokenreturns this exact casing, as snake_case/camelCase mismatches are a common source of bugs.
36-97: LGTM! Solid error handling with PKCE cleanup.The function correctly handles various failure scenarios (error response, missing code, missing verifier) and ensures
pkceStore.clear()is called on failures to prevent stale state. The catch block also clears PKCE state appropriately.
130-141: LGTM!The
isAuthDeepLinkfunction correctly validates both production and development protocol schemes with proper error handling for malformed URLs.apps/desktop/src/main/lib/auth/pkce.ts (3)
12-15: LGTM! Correct PKCE verifier generation.The implementation correctly generates a 32-byte random value encoded as base64url, resulting in 43 characters which meets the RFC 7636 requirement of 43-128 characters.
21-24: LGTM! Correct S256 challenge method.The SHA-256 hash with base64url encoding follows the RFC 7636 S256 method specification correctly.
57-71: LGTM! Proper consume-once pattern.The
consumeVerifiercorrectly implements a one-time-use pattern by clearing the verifier after retrieval, and properly checks expiry before returning. This prevents replay attacks.apps/desktop/src/lib/trpc/routers/auth/index.ts (1)
27-43: LGTM! Well-implemented subscription pattern.The subscription correctly emits initial state, attaches the listener, and properly cleans up by returning an unsubscribe function. This follows tRPC observable patterns correctly.
apps/desktop/src/main/lib/auth/auth-service.ts (3)
238-261: LGTM! Solid refresh scheduling logic.The method properly clears existing timers before setting new ones, calculates the refresh time considering the threshold, and handles edge cases where immediate refresh isn't needed. The logging provides good observability.
263-275: LGTM! Clean session cleanup.The
clearSessionmethod properly clears the timer, nullifies the session, clears persistent storage, and emits state change. All cleanup actions are correctly sequenced.
139-155: LGTM! Proper session handling with persistence.The
handleDeepLinkAuthmethod correctly saves the session, persists it to storage, schedules refresh, and emits state change. Error handling properly clears the session on failure.
| async load(): Promise<AuthSession | null> { | ||
| if (!safeStorage.isEncryptionAvailable()) { | ||
| return null; | ||
| } | ||
|
|
||
| try { | ||
| const encrypted = await fs.readFile(this.filePath); | ||
| const decrypted = safeStorage.decryptString(encrypted); | ||
| return JSON.parse(decrypted) as AuthSession; | ||
| } catch { | ||
| // File doesn't exist or can't be decrypted | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate loaded session data to prevent runtime errors.
The load() method deserializes JSON without validating its shape. If the stored data is corrupted, partially migrated from an older format, or tampered with, JSON.parse() could return an object that doesn't match the AuthSession interface, causing runtime errors in consumers.
Apply this diff to add Zod validation:
+import { z } from "zod";
import fs from "node:fs/promises";
import { join } from "node:path";
import { safeStorage } from "electron";
import type { AuthSession } from "shared/auth";
import { SUPERSET_HOME_DIR } from "../app-environment";
const SESSION_FILE_NAME = "auth-session.enc";
+const AuthSessionSchema = z.object({
+ accessToken: z.string(),
+ accessTokenExpiresAt: z.number(),
+ refreshToken: z.string(),
+ refreshTokenExpiresAt: z.number(),
+ user: z.object({
+ id: z.string(),
+ name: z.string(),
+ email: z.string(),
+ avatarUrl: z.string().nullable(),
+ }),
+});
+
/**
* Securely stores authentication session using Electron's safeStorage API
* Session data is encrypted at rest using the OS keychain
*/
class TokenStorage {
private readonly filePath: string;
constructor() {
this.filePath = join(SUPERSET_HOME_DIR, SESSION_FILE_NAME);
}
async save(session: AuthSession): Promise<void> {
if (!safeStorage.isEncryptionAvailable()) {
console.warn(
"[auth] Secure storage not available, session will not be persisted",
);
return;
}
const encrypted = safeStorage.encryptString(JSON.stringify(session));
await fs.writeFile(this.filePath, encrypted);
}
async load(): Promise<AuthSession | null> {
if (!safeStorage.isEncryptionAvailable()) {
return null;
}
try {
const encrypted = await fs.readFile(this.filePath);
const decrypted = safeStorage.decryptString(encrypted);
- return JSON.parse(decrypted) as AuthSession;
+ const parsed = JSON.parse(decrypted);
+ const validated = AuthSessionSchema.parse(parsed);
+ return validated;
- } catch {
+ } catch (error) {
// File doesn't exist or can't be decrypted
+ console.warn("[auth] Failed to load session:", error);
return null;
}
}
async clear(): Promise<void> {
try {
await fs.unlink(this.filePath);
- } catch {
+ } catch (error) {
// File doesn't exist, that's fine
+ console.debug("[auth] Session file already cleared or never existed");
}
}
}
export const tokenStorage = new TokenStorage();As per coding guidelines, prioritize type safety in TypeScript code.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/auth/token-storage.ts around lines 32-45, the
load() method JSON.parse()s the decrypted string without validating it; create
or import a Zod schema for AuthSession, parse the JSON result with
schema.safeParse(parsed) and only return the value when valid, otherwise return
null (and optionally log a validation error); ensure types are preserved by
casting to AuthSession only from the validated result so consumers never receive
malformed session objects.
| const signInMutation = trpc.auth.signIn.useMutation({ | ||
| onMutate: () => { | ||
| setIsSigningIn(true); | ||
| }, | ||
| onSettled: () => { | ||
| // Keep signing in state until we get the callback | ||
| // It will be reset when auth state changes | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Reset isSigningIn on immediate sign-in failures (not just on deep-link completion).
If the main-process signIn fails to open a browser (or returns { success: false }), the UI can stay stuck in “Signing in…”. Consider handling onError and/or inspecting the mutation result.
Also applies to: 40-45
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/api/src/trpc/context.ts (1)
38-43: Remove unusedresHeadersparameter.The
resHeadersparameter is declared but never used in the function body. Consider removing it to keep the signature clean.export const createContext = async ({ req, }: { req: Request; - resHeaders: Headers; }) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/api/src/trpc/context.ts(1 hunks)packages/trpc/src/router/organization.ts(1 hunks)packages/trpc/src/router/task.ts(1 hunks)packages/trpc/src/router/user.ts(1 hunks)packages/trpc/src/trpc.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for code formatting and linting, running at root level for speed
Files:
packages/trpc/src/router/user.tspackages/trpc/src/router/task.tsapps/api/src/trpc/context.tspackages/trpc/src/router/organization.tspackages/trpc/src/trpc.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid
anytype and prioritize type safety in TypeScript code
Files:
packages/trpc/src/router/user.tspackages/trpc/src/router/task.tsapps/api/src/trpc/context.tspackages/trpc/src/router/organization.tspackages/trpc/src/trpc.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx,js,jsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx,js,jsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
packages/trpc/src/trpc.ts
🧬 Code graph analysis (5)
packages/trpc/src/router/user.ts (2)
packages/db/src/index.ts (1)
eq(1-1)packages/db/src/schema/schema.ts (1)
users(15-35)
packages/trpc/src/router/task.ts (2)
packages/db/src/index.ts (1)
eq(1-1)packages/db/src/schema/schema.ts (1)
users(15-35)
apps/api/src/trpc/context.ts (3)
apps/api/src/env.ts (1)
env(4-23)packages/trpc/src/trpc.ts (1)
createTRPCContext(20-24)packages/trpc/src/index.ts (1)
createTRPCContext(9-9)
packages/trpc/src/router/organization.ts (2)
packages/db/src/index.ts (1)
eq(1-1)packages/db/src/schema/schema.ts (1)
users(15-35)
packages/trpc/src/trpc.ts (1)
packages/db/src/schema/schema.ts (1)
users(15-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Marketing
- GitHub Check: Build
🔇 Additional comments (7)
apps/api/src/trpc/context.ts (1)
44-63: LGTM! Well-structured dual authentication flow.The authentication priority (Clerk first, then desktop JWT) is clearly documented and correctly implemented. The fallback to unauthenticated context is appropriate.
packages/trpc/src/router/task.ts (1)
74-78: LGTM! Correctly migrated to userId-based context.The user lookup now uses
ctx.userIdinstead ofctx.session.userId, aligning with the new TRPC context model. The query logic remains correct.packages/trpc/src/router/user.ts (1)
9-13: LGTM! Correctly migrated to userId-based context.The user lookup in the
meresolver now usesctx.userIdinstead ofctx.session.userId, consistent with the new authentication model.packages/trpc/src/router/organization.ts (1)
59-63: LGTM! Correctly migrated to userId-based context.The user lookup in the organization
createmutation now usesctx.userIdinstead ofctx.session.userId, consistent with the refactored TRPC context.packages/trpc/src/trpc.ts (3)
9-24: LGTM! Simplified context model supports dual authentication.The refactored
TRPCContextto a simpleuserId-based model is cleaner and supports both Clerk sessions and desktop JWT authentication. The nullable type correctly handles unauthenticated requests.
46-59: LGTM! Protected procedure correctly enforces authentication.The updated
protectedProcedurecorrectly checksctx.userIdand maintains the same security guarantees as before. The error message is clear and appropriate.
61-85: Verify desktop tokens contain Clerk user ID.The
adminProcedurequeries the user byclerkIdusingctx.userId. Ensure that desktop JWT tokens issued by the desktop auth endpoints use the Clerk user ID as theuserIdclaim, otherwise this lookup will fail for desktop-authenticated users.#!/bin/bash # Check how desktop auth tokens are issued to verify userId claim contains clerkId # Search for desktop token issuance logic rg -n "SignJWT\|new SignJWT" --type ts -A 10 -B 3 # Search for where userId is set in desktop token payload rg -n "userId.*payload\|payload.*userId" --type ts -C 5 apps/api/src/app/api/auth/desktop/
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
apps/api/src/app/api/auth/desktop/token/route.ts (1)
91-105: Optional: validatecode_verifierlength/charset per RFC 7636 to fail fast on obviously bad input.
This helps reduce useless hashing work and makes errors clearer.Also applies to: 129-140
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
apps/api/src/app/api/auth/desktop/refresh/route.ts(1 hunks)apps/api/src/app/api/auth/desktop/token/route.ts(1 hunks)apps/desktop/package.json(3 hunks)apps/desktop/src/main/lib/auth/auth-service.ts(1 hunks)apps/desktop/src/shared/auth.ts(1 hunks)packages/shared/src/constants.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/package.json
- apps/desktop/src/shared/auth.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for code formatting and linting, running at root level for speed
Files:
packages/shared/src/constants.tsapps/desktop/src/main/lib/auth/auth-service.tsapps/api/src/app/api/auth/desktop/refresh/route.tsapps/api/src/app/api/auth/desktop/token/route.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid
anytype and prioritize type safety in TypeScript code
Files:
packages/shared/src/constants.tsapps/desktop/src/main/lib/auth/auth-service.tsapps/api/src/app/api/auth/desktop/refresh/route.tsapps/api/src/app/api/auth/desktop/token/route.ts
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/main/lib/auth/auth-service.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/main/lib/auth/auth-service.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx,js,jsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Please use alias as defined in `tsconfig.json` when possible
Applied to files:
apps/api/src/app/api/auth/desktop/refresh/route.ts
🧬 Code graph analysis (2)
apps/desktop/src/main/lib/auth/auth-service.ts (4)
apps/desktop/src/shared/auth.ts (4)
AuthSession(17-23)AuthState(12-15)AuthProvider(25-25)SignInResult(27-30)apps/desktop/src/main/lib/auth/token-storage.ts (1)
tokenStorage(56-56)apps/desktop/src/main/lib/auth/pkce.ts (1)
pkceStore(82-82)packages/shared/src/constants.ts (1)
TOKEN_CONFIG(16-23)
apps/api/src/app/api/auth/desktop/token/route.ts (2)
packages/shared/src/constants.ts (1)
TOKEN_CONFIG(16-23)apps/api/src/app/api/auth/desktop/refresh/route.ts (1)
POST(26-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Admin
- GitHub Check: Build
🔇 Additional comments (2)
apps/desktop/src/main/lib/auth/auth-service.ts (1)
139-155: Please verifyauthServiceis only exposed to the renderer via tRPC (not ad-hoc IPC/events), and thattokenStorageis securely backed (Keychain/Credential Manager). Based on learnings/guidelines, desktop IPC should go throughsrc/lib/trpc; also token-at-rest security is critical for refresh tokens.Also applies to: 278-278
packages/shared/src/constants.ts (1)
13-23:TOKEN_CONFIGlooks consistent and well-documented (seconds +as const).
This should help keep API + desktop refresh timing aligned.
| export async function POST(request: NextRequest) { | ||
| try { | ||
| const body = await request.json(); | ||
| const { refresh_token } = body; | ||
|
|
||
| // Validate required parameters | ||
| if (!refresh_token || typeof refresh_token !== "string") { | ||
| return NextResponse.json( | ||
| { error: "Missing or invalid refresh_token parameter" }, | ||
| { status: 400 }, | ||
| ); | ||
| } | ||
|
|
||
| // Verify and decode the refresh token | ||
| const secret = new TextEncoder().encode(env.DESKTOP_AUTH_SECRET); | ||
| let payload: RefreshTokenPayload; | ||
|
|
||
| try { | ||
| const result = await jwtVerify(refresh_token, secret); | ||
| payload = result.payload as RefreshTokenPayload; | ||
| } catch (verifyError) { | ||
| console.error("[refresh] Token verification failed:", verifyError); | ||
| return NextResponse.json( | ||
| { error: "Invalid or expired refresh token" }, | ||
| { status: 401 }, | ||
| ); | ||
| } | ||
|
|
||
| // Verify this is a refresh token | ||
| if (payload.type !== "refresh") { | ||
| return NextResponse.json( | ||
| { error: "Invalid token type" }, | ||
| { status: 400 }, | ||
| ); | ||
| } | ||
|
|
||
| // Create a new access token | ||
| const accessTokenExpiresAt = | ||
| Date.now() + TOKEN_CONFIG.ACCESS_TOKEN_EXPIRY * 1000; | ||
|
|
||
| const accessToken = await new SignJWT({ | ||
| userId: payload.userId, | ||
| email: payload.email, | ||
| name: payload.name, | ||
| avatarUrl: payload.avatarUrl, | ||
| type: "access", | ||
| }) | ||
| .setProtectedHeader({ alg: "HS256" }) | ||
| .setIssuedAt() | ||
| .setExpirationTime(`${TOKEN_CONFIG.ACCESS_TOKEN_EXPIRY}s`) | ||
| .sign(secret); | ||
|
|
||
| // Create a new refresh token (rotation - old one becomes invalid) | ||
| const refreshTokenExpiresAt = | ||
| Date.now() + TOKEN_CONFIG.REFRESH_TOKEN_EXPIRY * 1000; | ||
|
|
||
| const newRefreshToken = await new SignJWT({ | ||
| userId: payload.userId, | ||
| email: payload.email, | ||
| name: payload.name, | ||
| avatarUrl: payload.avatarUrl, | ||
| type: "refresh", | ||
| }) | ||
| .setProtectedHeader({ alg: "HS256" }) | ||
| .setIssuedAt() | ||
| .setExpirationTime(`${TOKEN_CONFIG.REFRESH_TOKEN_EXPIRY}s`) | ||
| .sign(secret); | ||
|
|
||
| return NextResponse.json({ | ||
| access_token: accessToken, | ||
| access_token_expires_at: accessTokenExpiresAt, | ||
| refresh_token: newRefreshToken, | ||
| refresh_token_expires_at: refreshTokenExpiresAt, | ||
| }); | ||
| } catch (error) { | ||
| console.error("[refresh] Token refresh failed:", error); | ||
| return NextResponse.json( | ||
| { error: "Token refresh failed" }, | ||
| { status: 500 }, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, explore the repository structure to understand the codebase
git ls-files | grep -E "(auth|token|session)" | head -20Repository: superset-sh/superset
Length of output: 953
🏁 Script executed:
# Search for RefreshTokenPayload type definition
rg "RefreshTokenPayload" --type ts --type tsx -A 5 -B 2Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Search for jti usage in the codebase (JWT ID for token tracking)
rg "jti" --type ts --type tsx -A 2 -B 2Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Search for any database or server-side token invalidation logic
rg "(tokenVersion|refreshToken|token.*store|token.*db|session)" --type ts --type tsx | grep -i "store\|db\|version\|invalid" | head -15Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Check the jose library usage and algorithm configuration
rg "jwtVerify\|SignJWT" --type ts --type tsx -A 3 -B 1 | head -40Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Look for any environment config or token config
rg "TOKEN_CONFIG" --type ts --type tsx -A 5 -B 1Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Search for RefreshTokenPayload type without tsx filter
rg "RefreshTokenPayload" --type ts -A 5 -B 2Repository: superset-sh/superset
Length of output: 1731
🏁 Script executed:
# Search for TOKEN_CONFIG definition
rg "TOKEN_CONFIG\s*=" --type ts -A 8Repository: superset-sh/superset
Length of output: 643
🏁 Script executed:
# Check the token route file
cat -n apps/api/src/app/api/auth/desktop/token/route.tsRepository: superset-sh/superset
Length of output: 5696
🏁 Script executed:
# Look for session or token database schema
rg "refreshToken|tokenVersion|jti" --type ts -A 2 -B 1Repository: superset-sh/superset
Length of output: 3680
🏁 Script executed:
# Check token storage implementation
cat -n apps/desktop/src/main/lib/auth/token-storage.tsRepository: superset-sh/superset
Length of output: 1881
🏁 Script executed:
# Look for auth service implementation
cat -n apps/desktop/src/main/lib/auth/auth-service.ts | head -100Repository: superset-sh/superset
Length of output: 3364
🏁 Script executed:
# Check for any database migrations or schema files related to tokens/sessions
find . -name "*.sql" -o -name "*migration*" | xargs grep -l "token\|session\|refresh" 2>/dev/null | head -10Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check the refresh route file fully to see if there's any database lookup
wc -l apps/api/src/app/api/auth/desktop/refresh/route.tsRepository: superset-sh/superset
Length of output: 118
🏁 Script executed:
# Check if there's any database layer or token tracking
rg "database\|prisma\|drizzle\|db\." apps/api/src/app/api/auth/desktop/ --type ts -A 2Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Verify jose library's jwtVerify behavior with algorithm handling
rg "import.*jose\|from.*jose" --type ts -B 2 -A 2 | head -20Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Read the complete refresh route file
cat -n apps/api/src/app/api/auth/desktop/refresh/route.tsRepository: superset-sh/superset
Length of output: 3672
🏁 Script executed:
# Check for any algorithm restrictions in jose usage
rg "jwtVerify.*algorithm\|jwtVerify.*alg\|algorithms:" --type ts -A 3 -B 3Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Quick verification: check jose library behavior with jwtVerify default options
python3 << 'EOF'
# Check what jose's jwtVerify does with algorithm handling
# Based on jose documentation: jwtVerify verifies signature but doesn't restrict algorithms
# by default unless you pass {algorithms: [...]} option
print("jose jwtVerify behavior:")
print("- Called with jwtVerify(token, secret) and no options")
print("- Verifies HMAC signature with the provided secret")
print("- Does NOT restrict algorithm unless options.algorithms is specified")
print("- However, if token was signed with HS256, verification succeeds")
print("")
print("Key finding: The signature check IMPLICITLY enforces HS256")
print("because the token must have been signed with HS256 to verify")
print("with the HS256 secret. But no explicit algorithm restriction.")
EOFRepository: superset-sh/superset
Length of output: 150
Critical: "Token rotation" doesn't invalidate old refresh tokens—they remain valid until expiry with this stateless JWT implementation.
The code issues new refresh and access tokens but provides no mechanism to invalidate the old refresh token. Since tokens are stateless JWTs with no server-side tracking (no jti, tokenVersion, or token revocation list), an attacker who obtains a refresh token can reuse it repeatedly for the full 30-day expiry window, even after the client "rotates" it. The comment at line 78 claiming "old one becomes invalid" is misleading.
Fix options:
- Add server-side token state: Store a
jti(JWT ID) or per-usertokenVersionin the database and validate against it on each refresh request. - Switch to opaque tokens: Use server-stored refresh token secrets instead of stateless JWTs, allowing true invalidation on rotation.
Additionally, the payload cast at line 45 uses as RefreshTokenPayload without runtime validation—only the type field is checked. Consider validating the full payload structure to catch malformed tokens early.
🤖 Prompt for AI Agents
In apps/api/src/app/api/auth/desktop/refresh/route.ts around lines 26 to 107:
the current implementation issues a new refresh JWT but does not invalidate the
old one (the comment "old one becomes invalid" is false for stateless JWTs), and
the payload is cast without full runtime validation; fix by adding server-side
refresh state and payload validation — either (A) add a per-user tokenVersion
(or stored currentRefreshJti) in your DB, include that value in refresh JWTs,
validate it on jwtVerify, and on rotation persist the new tokenVersion/JTI (or
increment tokenVersion) so the previous token is rejected, or (B) switch to
opaque refresh tokens stored server-side and mark the old token revoked on
rotation; additionally perform runtime validation of all required payload fields
(userId, email, name, avatarUrl, type, jti/tokenVersion) before trusting them
and return 401 for malformed payloads.
| // Verify and decode the refresh token | ||
| const secret = new TextEncoder().encode(env.DESKTOP_AUTH_SECRET); | ||
| let payload: RefreshTokenPayload; | ||
|
|
||
| try { | ||
| const result = await jwtVerify(refresh_token, secret); | ||
| payload = result.payload as RefreshTokenPayload; | ||
| } catch (verifyError) { | ||
| console.error("[refresh] Token verification failed:", verifyError); | ||
| return NextResponse.json( | ||
| { error: "Invalid or expired refresh token" }, | ||
| { status: 401 }, | ||
| ); | ||
| } | ||
|
|
||
| // Verify this is a refresh token | ||
| if (payload.type !== "refresh") { | ||
| return NextResponse.json( | ||
| { error: "Invalid token type" }, | ||
| { status: 400 }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/api/src/app/api/auth/desktop/refresh/route.tsRepository: superset-sh/superset
Length of output: 3672
🌐 Web query:
jose library jwtVerify algorithms option TypeScript
💡 Result:
Short answer: In TypeScript the jose jwtVerify options accept an algorithms?: string[] property — i.e. an optional array of JWA algorithm names (e.g. "RS256", "ES256", "HS256", "EdDSA", etc.). Pass it as the third-argument options object: { algorithms: ['RS256'] }.
Sources:
- jose type definitions (VerifyOptions → algorithms?: string[]). [1]
- jose usage examples showing algorithms option passed to jwtVerify. [2]
Harden jwtVerify options and add runtime validation for required payload fields.
The current implementation should specify jwtVerify(refresh_token, secret, { algorithms: ["HS256"] }) to restrict accepted algorithms and prevent algorithm substitution attacks. Additionally, required fields (userId, email, name) should be validated after decoding before they are used to create new tokens on lines 67–70 and 83–86. A missing or null field would result in malformed tokens being issued. Consider adding explicit runtime checks or using a validation library for the payload structure.
🤖 Prompt for AI Agents
In apps/api/src/app/api/auth/desktop/refresh/route.ts around lines 39 to 60,
update the jwtVerify call to harden options (e.g. pass { algorithms: ["HS256"]
}) to prevent algorithm substitution, and add explicit runtime validation after
decoding that payload contains non-empty userId, email and name (and still
ensure payload.type === "refresh"); if any required field is missing or invalid,
return a 400 error instead of proceeding to the token-creation code around lines
67–70 and 83–86. Ensure the validation is defensive (check types and non-empty
strings), log or return a clear error, and only use the payload to create new
tokens after these checks pass.
| export async function POST(request: NextRequest) { | ||
| try { | ||
| const body = await request.json(); | ||
| const { code, code_verifier } = body; | ||
|
|
||
| // Validate required parameters | ||
| if (!code || typeof code !== "string") { | ||
| return NextResponse.json( | ||
| { error: "Missing or invalid code parameter" }, | ||
| { status: 400 }, | ||
| ); | ||
| } | ||
|
|
||
| if (!code_verifier || typeof code_verifier !== "string") { | ||
| return NextResponse.json( | ||
| { error: "Missing or invalid code_verifier parameter" }, | ||
| { status: 400 }, | ||
| ); | ||
| } | ||
|
|
||
| // Verify and decode the auth code | ||
| const secret = new TextEncoder().encode(env.DESKTOP_AUTH_SECRET); | ||
| let payload: AuthCodePayload; | ||
|
|
||
| try { | ||
| const result = await jwtVerify(code, secret); | ||
| payload = result.payload as AuthCodePayload; | ||
| } catch (verifyError) { | ||
| console.error("[token] Auth code verification failed:", verifyError); | ||
| return NextResponse.json( | ||
| { error: "Invalid or expired auth code" }, | ||
| { status: 401 }, | ||
| ); | ||
| } | ||
|
|
||
| // Verify this is an auth code (not a session token) | ||
| if (payload.type !== "auth_code") { | ||
| return NextResponse.json( | ||
| { error: "Invalid token type" }, | ||
| { status: 400 }, | ||
| ); | ||
| } | ||
|
|
||
| // Verify PKCE: SHA256(code_verifier) should equal code_challenge | ||
| const computedChallenge = createHash("sha256") | ||
| .update(code_verifier) | ||
| .digest("base64url"); | ||
|
|
||
| if (computedChallenge !== payload.codeChallenge) { | ||
| console.error("[token] PKCE verification failed"); | ||
| return NextResponse.json( | ||
| { error: "PKCE verification failed" }, | ||
| { status: 401 }, | ||
| ); | ||
| } | ||
|
|
||
| // PKCE verified! Create access and refresh tokens | ||
| const userInfo: UserInfo = { | ||
| userId: payload.userId, | ||
| email: payload.email, | ||
| name: payload.name, | ||
| avatarUrl: payload.avatarUrl, | ||
| }; | ||
|
|
||
| const [accessToken, refreshToken] = await Promise.all([ | ||
| createAccessToken(userInfo, secret), | ||
| createRefreshToken(userInfo, secret), | ||
| ]); | ||
|
|
||
| return NextResponse.json({ | ||
| access_token: accessToken.token, | ||
| access_token_expires_at: accessToken.expiresAt, | ||
| refresh_token: refreshToken.token, | ||
| refresh_token_expires_at: refreshToken.expiresAt, | ||
| user: { | ||
| id: payload.userId, | ||
| email: payload.email, | ||
| name: payload.name, | ||
| avatarUrl: payload.avatarUrl, | ||
| }, | ||
| }); | ||
| } catch (error) { | ||
| console.error("[token] Token exchange failed:", error); | ||
| return NextResponse.json( | ||
| { error: "Token exchange failed" }, | ||
| { status: 500 }, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where auth codes are created
rg -t ts -t tsx "createAuth.*ode\|type.*auth.*code\|\"auth_code\"" --max-count 20Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Find AuthCodePayload type definition
rg -t ts -t tsx "AuthCodePayload" -A 5 -B 2Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Check how jwtVerify is used in the codebase for algorithm patterns
rg -t ts -t tsx "jwtVerify" -A 2 -B 1Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Find token creation functions (createAccessToken, createRefreshToken)
rg -t ts -t tsx "createAccessToken|createRefreshToken" -A 8 -B 1 | head -60Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Find where auth codes are created using ts type only
rg "createAuth|type.*auth.*code|\"auth_code\"" -t ts --max-count 20Repository: superset-sh/superset
Length of output: 1066
🏁 Script executed:
#!/bin/bash
# Find AuthCodePayload type definition
rg "AuthCodePayload" -t ts -A 5 -B 2Repository: superset-sh/superset
Length of output: 3004
🏁 Script executed:
#!/bin/bash
# Check how jwtVerify is used in the codebase
rg "jwtVerify" -t ts -A 2 -B 1Repository: superset-sh/superset
Length of output: 2032
🏁 Script executed:
#!/bin/bash
# Find token creation functions
rg "createAccessToken|createRefreshToken|export.*function.*create" -t ts -A 8 -B 1 | head -100Repository: superset-sh/superset
Length of output: 6848
🏁 Script executed:
#!/bin/bash
# Read the createAuthCode function in detail
cat -n apps/web/src/app/api/auth/desktop/[provider]/route.ts | sed -n '1,200p'Repository: superset-sh/superset
Length of output: 4133
🏁 Script executed:
#!/bin/bash
# Check how SignJWT is used (algorithm specification)
rg "new SignJWT" -A 10 apps/api/src/app/api/auth/desktop/token/route.tsRepository: superset-sh/superset
Length of output: 636
🏁 Script executed:
#!/bin/bash
# Check how SignJWT is used in auth creation
rg "new SignJWT" -A 10 apps/web/src/app/api/auth/desktop/[provider]/route.tsRepository: superset-sh/superset
Length of output: 433
🏁 Script executed:
#!/bin/bash
# Check the full token route to see if there's validation after the cast
cat -n apps/api/src/app/api/auth/desktop/token/route.ts | sed -n '1,180p'Repository: superset-sh/superset
Length of output: 5696
🏁 Script executed:
#!/bin/bash
# Check if there's a schema validation using Zod or similar
rg "z\.object|Zod|schema" apps/api/src/app/api/auth/desktop/token/route.tsRepository: superset-sh/superset
Length of output: 46
Add algorithm restriction to jwtVerify and validate required payload fields at runtime.
The jwtVerify(code, secret) call lacks algorithm restriction (should be { algorithms: ["HS256"] }), creating a theoretical algorithm confusion vulnerability. Additionally, the TypeScript cast as AuthCodePayload does not provide runtime validation—if required fields like codeChallenge, userId, or email are missing or malformed, direct property access will silently produce undefined, leading to confusing error messages rather than explicit validation failures. Add explicit field checks or use a schema validator (e.g., Zod) to validate payload.userId, payload.email, payload.codeChallenge, and other required fields immediately after jwtVerify.
🤖 Prompt for AI Agents
In apps/api/src/app/api/auth/desktop/token/route.ts around lines 86 to 174, the
jwtVerify call lacks an algorithms restriction and the decoded payload is only
type-cast (no runtime validation); update jwtVerify to include { algorithms:
["HS256"] } and immediately validate required payload fields (at minimum: type
=== "auth_code", codeChallenge, userId, email and any other fields you later
access) using explicit runtime checks or a lightweight schema validator (e.g.,
Zod) so you return a 400/401 with a clear error when fields are missing/invalid
instead of proceeding with undefined values.
| // Web app URL for OAuth - defaults to production, can be overridden | ||
| const WEB_APP_URL = | ||
| process.env.NEXT_PUBLIC_WEB_URL ?? "https://app.superset.sh"; | ||
|
|
||
| // API URL for token refresh - defaults to production, can be overridden | ||
| const API_URL = process.env.NEXT_PUBLIC_API_URL ?? "https://api.superset.sh"; |
There was a problem hiding this comment.
Be careful using NEXT_PUBLIC_* env vars in the Electron main process; consider stricter validation/allowlisting for WEB_APP_URL/API_URL.
Right now any environment override can redirect auth/refresh traffic; recommend constraining to expected origins (especially in production builds).
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/auth/auth-service.ts around lines 13–18, the
current use of NEXT_PUBLIC_WEB_URL and NEXT_PUBLIC_API_URL allows any
environment override to redirect OAuth/token traffic; change to validate and
allowlist overrides: parse the env value as a URL, compare its origin/hostname
against a configured whitelist (differentiate prod vs dev), and only accept it
if it matches; otherwise fall back to the hardcoded production URL and log/warn;
for production builds consider disabling overrides entirely or restrict to exact
hostnames, and ensure malformed URLs throw/are ignored so main process never
uses arbitrary origins.
| async initialize(): Promise<void> { | ||
| const session = await tokenStorage.load(); | ||
|
|
||
| if (!session) { | ||
| return; | ||
| } | ||
|
|
||
| // Check if refresh token is expired (session is truly over) | ||
| if (session.refreshTokenExpiresAt < Date.now()) { | ||
| console.log("[auth] Refresh token expired, clearing session"); | ||
| await this.clearSession(); | ||
| return; | ||
| } | ||
|
|
||
| // Restore session | ||
| this.session = session; | ||
| console.log("[auth] Session restored for user:", this.session.user.email); | ||
|
|
||
| // Check if access token needs refresh | ||
| if (this.shouldRefreshAccessToken()) { | ||
| console.log("[auth] Access token expired/expiring, refreshing..."); | ||
| await this.refreshAccessToken(); | ||
| } | ||
|
|
||
| // Schedule next refresh | ||
| this.scheduleRefresh(); | ||
| } |
There was a problem hiding this comment.
Refresh concurrency gap: getAccessToken() can return a stale/expired token while a refresh is already in flight.
refreshAccessToken() returns early when isRefreshing is true, and callers don’t await the in-flight refresh. Consider tracking an internal refreshPromise and awaiting it from getAccessToken() (and anywhere else) to prevent racing on an expiring token.
Also applies to: 84-102, 178-233
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/auth/auth-service.ts around lines 42 to 68 (and
similarly 84-102, 178-233), getAccessToken can return a stale token while
refreshAccessToken returns early when isRefreshing is true; fix by introducing
an internal refreshPromise: have refreshAccessToken set isRefreshing=true and
assign refreshPromise to the Promise for the refresh operation, clear
isRefreshing and refreshPromise on resolve/reject, and ensure any caller
(getAccessToken and other code paths) awaits refreshPromise when isRefreshing is
true before returning the token; propagate refresh errors to callers, avoid
swallowing them, and ensure schedule/initialize use the same refreshPromise flow
so concurrent callers all await the single in-flight refresh.
| private async refreshAccessToken(): Promise<void> { | ||
| if (!this.session || this.isRefreshing) return; | ||
|
|
||
| this.isRefreshing = true; | ||
|
|
||
| try { | ||
| console.log("[auth] Refreshing access token..."); | ||
|
|
||
| const response = await fetch(`${API_URL}/api/auth/desktop/refresh`, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ | ||
| refresh_token: this.session.refreshToken, | ||
| }), | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| const errorBody = await response.json().catch(() => ({})); | ||
|
|
||
| // If refresh token is invalid/expired, clear session | ||
| if (response.status === 401) { | ||
| console.log("[auth] Refresh token invalid, clearing session"); | ||
| await this.clearSession(); | ||
| return; | ||
| } | ||
|
|
||
| throw new Error( | ||
| errorBody.error || `Refresh failed: ${response.status}`, | ||
| ); | ||
| } | ||
|
|
||
| const data: RefreshResponse = await response.json(); | ||
|
|
||
| // Update session with new access token and rotated refresh token | ||
| this.session = { | ||
| ...this.session, | ||
| accessToken: data.access_token, | ||
| accessTokenExpiresAt: data.access_token_expires_at, | ||
| refreshToken: data.refresh_token, | ||
| refreshTokenExpiresAt: data.refresh_token_expires_at, | ||
| }; | ||
|
|
||
| // Persist updated session | ||
| await tokenStorage.save(this.session); | ||
| console.log("[auth] Access token refreshed"); | ||
|
|
||
| // Reschedule next refresh | ||
| this.scheduleRefresh(); | ||
| } catch (err) { | ||
| console.error("[auth] Token refresh failed:", err); | ||
| } finally { | ||
| this.isRefreshing = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Refresh retry/scheduling is fragile: a transient refresh failure can stop auto-refresh indefinitely.
If the timer-triggered refresh fails (network, 5xx), you only log and don’t reschedule with backoff, so the app may remain “signed in” with an expired access token until some other call triggers refresh. Suggest rescheduling on failure (e.g., exponential backoff capped at N minutes) and adding a request timeout via AbortController.
class AuthService extends EventEmitter {
private session: AuthSession | null = null;
private refreshTimer: ReturnType<typeof setTimeout> | null = null;
private isRefreshing = false;
+ private refreshPromise: Promise<void> | null = null;
@@
async getAccessToken(): Promise<string | null> {
@@
- if (this.shouldRefreshAccessToken()) {
- await this.refreshAccessToken();
- }
+ if (this.shouldRefreshAccessToken()) {
+ await (this.refreshPromise ?? this.refreshAccessToken());
+ }
@@
private async refreshAccessToken(): Promise<void> {
- if (!this.session || this.isRefreshing) return;
+ if (!this.session) return;
+ if (this.refreshPromise) return this.refreshPromise;
- this.isRefreshing = true;
+ this.isRefreshing = true;
+ this.refreshPromise = (async () => {
try {
@@
- const response = await fetch(`${API_URL}/api/auth/desktop/refresh`, {
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), 15_000);
+ const response = await fetch(`${API_URL}/api/auth/desktop/refresh`, {
method: "POST",
headers: {
"Content-Type": "application/json",
},
+ signal: controller.signal,
body: JSON.stringify({
refresh_token: this.session.refreshToken,
}),
});
+ clearTimeout(timeout);
@@
this.scheduleRefresh();
} catch (err) {
console.error("[auth] Token refresh failed:", err);
+ // retry soon (could be backoff)
+ this.refreshTimer = setTimeout(() => this.refreshAccessToken(), 30_000);
} finally {
this.isRefreshing = false;
+ this.refreshPromise = null;
}
+ })();
+ return this.refreshPromise;
}Also applies to: 238-261
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/auth/auth-service.ts around lines 178-233 (and also
apply same fixes to 238-261), the refreshAccessToken flow currently logs
failures and leaves auto-refresh unscheduled on transient errors; change it to
(1) wrap the fetch in an AbortController with a configurable timeout so slow
requests fail fast, (2) implement exponential backoff retry scheduling on
failure (e.g., double delay up to a capped max like N minutes) instead of only
logging, storing the current backoff state on the instance, and (3) ensure
scheduleRefresh() is called after failures (when a session still exists) so the
next attempt is queued, while still clearing session immediately on 401; keep
isRefreshing correctly reset in finally and persist updated session on success.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (6)
apps/desktop/src/lib/electron-router-dom.ts (1)
32-37:env.NODE_ENVdefaulting to"development"can still break preview unlessNODE_ENVis explicitly set.
Becauseapps/desktop/src/env.tsdefaultsNODE_ENVto"development",electron-vite previewwill behave like dev if the start script doesn’t setNODE_ENV=production(or similar). This is the same root cause called out in the prior review—please ensure the script-level fix is in place.#!/bin/bash # Verify preview/start scripts set NODE_ENV explicitly (and that env defaults won't hijack preview). rg -n --glob='**/apps/desktop/package.json' '"start"\s*:\s*' -n rg -n --glob='**/apps/desktop/package.json' 'electron-vite\s+preview' -n rg -n --glob='**/apps/desktop/package.json' 'NODE_ENV\s*=' -napps/api/src/app/api/auth/desktop/token/route.ts (1)
90-111: RestrictjwtVerifyalgorithms + validate required payload fields at runtime (not just TS cast).- const result = await jwtVerify(code, secret); - payload = result.payload as AuthCodePayload; + const result = await jwtVerify(code, secret, { algorithms: ["HS256"] }); + const p = result.payload as Partial<AuthCodePayload>; + if ( + p.type !== "auth_code" || + typeof p.userId !== "string" || + typeof p.codeChallenge !== "string" + ) { + return NextResponse.json( + { error: "Invalid auth code payload" }, + { status: 401 }, + ); + } + payload = p as AuthCodePayload;apps/desktop/src/main/lib/auth/token-storage.ts (1)
32-44: Validate decrypted session JSON before returning it (corruption/migration safety).At minimum, don’t return unchecked
JSON.parse()output; validate shape (e.g., ZodsafeParse) and log failures so callers never receive malformed sessions.apps/desktop/src/main/lib/auth/auth-service.ts (3)
96-113: Environment variable validation concern persists.
env.NEXT_PUBLIC_WEB_URL(Line 106) andenv.NEXT_PUBLIC_API_URL(used at Line 178) accept any environment override, allowing potential redirection of OAuth/token traffic to arbitrary origins. Consider validating and allowlisting overrides against expected production/development hostnames, especially in production builds.
72-90: Refresh concurrency gap: concurrent callers may receive stale tokens.When
isRefreshingis true,refreshAccessToken()returns early (Line 170), causinggetAccessToken()to proceed with a potentially expired token (Lines 85-89). Multiple concurrent callers during a refresh window will all receive the stale token instead of awaiting the in-flight refresh.Consider tracking an internal
refreshPromiseand awaiting it ingetAccessToken():class AuthService extends EventEmitter { private session: AuthSession | null = null; private refreshTimer: ReturnType<typeof setTimeout> | null = null; private isRefreshing = false; + private refreshPromise: Promise<void> | null = null;async getAccessToken(): Promise<string | null> { // ... if (this.shouldRefreshAccessToken()) { - await this.refreshAccessToken(); + await (this.refreshPromise ?? this.refreshAccessToken()); } return this.session?.accessToken ?? null; } private async refreshAccessToken(): Promise<void> { - if (!this.session || this.isRefreshing) return; + if (!this.session) return; + if (this.refreshPromise) return this.refreshPromise; - this.isRefreshing = true; + this.isRefreshing = true; + this.refreshPromise = (async () => { try { // ... existing refresh logic ... } finally { this.isRefreshing = false; + this.refreshPromise = null; } + })(); + return this.refreshPromise; }Also applies to: 169-227
169-227: Transient refresh failures stop auto-refresh indefinitely.When
refreshAccessToken()encounters a non-401 error (e.g., network timeout, 5xx response), it only logs the error (Line 223) without rescheduling. The timer-based refresh is only set on success (Line 221), leaving the app with an expired access token until a manual trigger occurs.Consider:
- Adding request timeout via
AbortController- Implementing exponential backoff retry scheduling on failure
- Calling
scheduleRefresh()even after transient failures (when session still exists)private async refreshAccessToken(): Promise<void> { // ... try { + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 15_000); const response = await fetch(`${env.NEXT_PUBLIC_API_URL}/api/auth/desktop/refresh`, { method: "POST", headers: { "Content-Type": "application/json" }, + signal: controller.signal, body: JSON.stringify({ refresh_token: this.session.refreshToken }), }); + clearTimeout(timeout); // ... existing success logic ... this.scheduleRefresh(); } catch (err) { console.error("[auth] Token refresh failed:", err); + // Retry with backoff (e.g., 30s, then 60s, up to max) + if (this.session) { + this.refreshTimer = setTimeout(() => this.refreshAccessToken(), 30_000); + } } finally { this.isRefreshing = false; } }
🧹 Nitpick comments (4)
apps/desktop/src/lib/electron-router-dom.ts (1)
4-4: Prefer a tsconfig alias import forenv(if available) to match desktop guidelines.
Relative../envworks, but this file already uses an alias import (shared/constants); consider switching../envto the configured alias to keep imports consistent and resilient to moves..env.example (1)
27-29: Good: documentDESKTOP_AUTH_SECRET; consider reordering per dotenv-linter.# Desktop App -VITE_DEV_SERVER_PORT=4927 DESKTOP_AUTH_SECRET=your_desktop_auth_secret_here_min_32_chars +VITE_DEV_SERVER_PORT=4927apps/desktop/src/lib/trpc/routers/user/index.ts (1)
7-16: Optional: drop redundantasync/return, and consider normalizing upstream errors to a tRPC error.- me: publicProcedure.query(async () => { - return apiClient.user.me.query(); - }), + me: publicProcedure.query(() => apiClient.user.me.query()),apps/desktop/src/main/lib/api-client.ts (1)
11-26: Add request timeouts (AbortController) to avoid hung API calls.Consider passing a custom
fetchtohttpBatchLinkthat enforces a reasonable timeout (and maps aborts to a predictable error) so the desktop app doesn’t wait indefinitely on network hangs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (52)
.env.example(1 hunks).github/workflows/deploy-preview.yml(3 hunks).github/workflows/deploy-production.yml(3 hunks)apps/api/package.json(1 hunks)apps/api/src/app/api/auth/desktop/refresh/route.ts(1 hunks)apps/api/src/app/api/auth/desktop/token/route.ts(1 hunks)apps/api/src/env.ts(1 hunks)apps/api/src/trpc/context.ts(1 hunks)apps/api/src/trpc/utils/verifyDesktopToken.ts(1 hunks)apps/desktop/package.json(3 hunks)apps/desktop/scripts/patch-dev-protocol.ts(1 hunks)apps/desktop/src/env.ts(1 hunks)apps/desktop/src/lib/electron-app/factories/app/setup.ts(3 hunks)apps/desktop/src/lib/electron-router-dom.ts(2 hunks)apps/desktop/src/lib/trpc/routers/auth/index.ts(1 hunks)apps/desktop/src/lib/trpc/routers/index.ts(3 hunks)apps/desktop/src/lib/trpc/routers/user/index.ts(1 hunks)apps/desktop/src/main/index.ts(2 hunks)apps/desktop/src/main/lib/api-client.ts(1 hunks)apps/desktop/src/main/lib/app-environment.ts(1 hunks)apps/desktop/src/main/lib/auth/auth-service.ts(1 hunks)apps/desktop/src/main/lib/auth/deep-link-handler.ts(1 hunks)apps/desktop/src/main/lib/auth/index.ts(1 hunks)apps/desktop/src/main/lib/auth/pkce.ts(1 hunks)apps/desktop/src/main/lib/auth/token-storage.ts(1 hunks)apps/desktop/src/main/lib/auto-updater.ts(4 hunks)apps/desktop/src/main/lib/menu.ts(2 hunks)apps/desktop/src/main/lib/sound-paths.ts(2 hunks)apps/desktop/src/main/lib/terminal-history.ts(2 hunks)apps/desktop/src/renderer/components/ConfigFilePreview/ConfigFilePreview.tsx(2 hunks)apps/desktop/src/renderer/components/SetupConfigModal/SetupConfigModal.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/HelpMenu/HelpMenu.tsx(2 hunks)apps/desktop/src/renderer/screens/main/index.tsx(3 hunks)apps/desktop/src/renderer/screens/sign-in/components/SupersetLogo/SupersetLogo.tsx(1 hunks)apps/desktop/src/renderer/screens/sign-in/components/SupersetLogo/index.ts(1 hunks)apps/desktop/src/renderer/screens/sign-in/index.tsx(1 hunks)apps/desktop/src/renderer/stores/app-state.ts(1 hunks)apps/desktop/src/shared/auth.ts(1 hunks)apps/desktop/src/shared/constants.ts(3 hunks)apps/web/package.json(1 hunks)apps/web/src/app/api/auth/desktop/[provider]/route.ts(1 hunks)apps/web/src/app/auth/desktop/callback/page.tsx(1 hunks)apps/web/src/env.ts(1 hunks)packages/shared/src/constants.ts(1 hunks)packages/trpc/src/router/organization.ts(1 hunks)packages/trpc/src/router/task.ts(1 hunks)packages/trpc/src/router/user.ts(1 hunks)packages/trpc/src/trpc.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (27)
- .github/workflows/deploy-preview.yml
- apps/desktop/src/renderer/screens/main/components/TopBar/HelpMenu/HelpMenu.tsx
- apps/desktop/src/renderer/components/SetupConfigModal/SetupConfigModal.tsx
- apps/web/package.json
- apps/web/src/env.ts
- apps/desktop/src/main/lib/auth/index.ts
- apps/desktop/src/renderer/components/ConfigFilePreview/ConfigFilePreview.tsx
- apps/desktop/src/renderer/screens/sign-in/index.tsx
- apps/desktop/src/shared/auth.ts
- apps/desktop/src/env.ts
- apps/desktop/src/lib/electron-app/factories/app/setup.ts
- apps/api/package.json
- apps/desktop/src/main/lib/sound-paths.ts
- apps/desktop/scripts/patch-dev-protocol.ts
- .github/workflows/deploy-production.yml
- apps/desktop/src/lib/trpc/routers/auth/index.ts
- apps/api/src/trpc/context.ts
- apps/api/src/app/api/auth/desktop/refresh/route.ts
- apps/desktop/src/lib/trpc/routers/index.ts
- apps/api/src/trpc/utils/verifyDesktopToken.ts
- apps/desktop/src/main/lib/auto-updater.ts
- apps/web/src/app/auth/desktop/callback/page.tsx
- packages/trpc/src/router/organization.ts
- apps/desktop/src/main/lib/terminal-history.ts
- apps/desktop/src/main/lib/app-environment.ts
- packages/trpc/src/trpc.ts
- apps/desktop/package.json
🧰 Additional context used
📓 Path-based instructions (8)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsxapps/desktop/src/main/lib/auth/token-storage.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/lib/electron-router-dom.tsapps/desktop/src/renderer/screens/sign-in/components/SupersetLogo/SupersetLogo.tsxapps/desktop/src/lib/trpc/routers/user/index.tsapps/desktop/src/main/lib/api-client.tsapps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/index.tsapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/main/lib/menu.tsapps/desktop/src/main/lib/auth/auth-service.tsapps/desktop/src/shared/constants.tsapps/desktop/src/main/index.tsapps/desktop/src/renderer/screens/sign-in/components/SupersetLogo/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsxapps/desktop/src/main/lib/auth/deep-link-handler.tsapps/desktop/src/main/lib/auth/pkce.tsapps/desktop/src/renderer/stores/app-state.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsxapps/desktop/src/main/lib/auth/token-storage.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/lib/electron-router-dom.tsapps/desktop/src/renderer/screens/sign-in/components/SupersetLogo/SupersetLogo.tsxapps/desktop/src/lib/trpc/routers/user/index.tsapps/desktop/src/main/lib/api-client.tsapps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/index.tsapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/main/lib/menu.tsapps/desktop/src/main/lib/auth/auth-service.tsapps/desktop/src/shared/constants.tsapps/desktop/src/main/index.tsapps/desktop/src/renderer/screens/sign-in/components/SupersetLogo/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsxapps/desktop/src/main/lib/auth/deep-link-handler.tsapps/desktop/src/main/lib/auth/pkce.tsapps/desktop/src/renderer/stores/app-state.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for code formatting and linting, running at root level for speed
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsxapps/api/src/env.tsapps/desktop/src/main/lib/auth/token-storage.tsapps/web/src/app/api/auth/desktop/[provider]/route.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/lib/electron-router-dom.tsapps/desktop/src/renderer/screens/sign-in/components/SupersetLogo/SupersetLogo.tsxapps/desktop/src/lib/trpc/routers/user/index.tspackages/trpc/src/router/user.tsapps/desktop/src/main/lib/api-client.tsapps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/index.tsapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/main/lib/menu.tsapps/desktop/src/main/lib/auth/auth-service.tsapps/desktop/src/shared/constants.tsapps/desktop/src/main/index.tspackages/shared/src/constants.tsapps/api/src/app/api/auth/desktop/token/route.tsapps/desktop/src/renderer/screens/sign-in/components/SupersetLogo/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsxapps/desktop/src/main/lib/auth/deep-link-handler.tsapps/desktop/src/main/lib/auth/pkce.tspackages/trpc/src/router/task.tsapps/desktop/src/renderer/stores/app-state.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid
anytype and prioritize type safety in TypeScript code
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsxapps/api/src/env.tsapps/desktop/src/main/lib/auth/token-storage.tsapps/web/src/app/api/auth/desktop/[provider]/route.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/lib/electron-router-dom.tsapps/desktop/src/renderer/screens/sign-in/components/SupersetLogo/SupersetLogo.tsxapps/desktop/src/lib/trpc/routers/user/index.tspackages/trpc/src/router/user.tsapps/desktop/src/main/lib/api-client.tsapps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/index.tsapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/main/lib/menu.tsapps/desktop/src/main/lib/auth/auth-service.tsapps/desktop/src/shared/constants.tsapps/desktop/src/main/index.tspackages/shared/src/constants.tsapps/api/src/app/api/auth/desktop/token/route.tsapps/desktop/src/renderer/screens/sign-in/components/SupersetLogo/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsxapps/desktop/src/main/lib/auth/deep-link-handler.tsapps/desktop/src/main/lib/auth/pkce.tspackages/trpc/src/router/task.tsapps/desktop/src/renderer/stores/app-state.ts
**/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/components/**/*.{ts,tsx}: Structure project folders as one folder per component with PascalCase naming (ComponentName/ComponentName.tsx + index.ts barrel export)
Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Use one component per file (no multi-component files)
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/renderer/screens/sign-in/components/SupersetLogo/SupersetLogo.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/index.tsapps/desktop/src/renderer/screens/sign-in/components/SupersetLogo/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Call IPC methods from renderer process using window.ipcRenderer.invoke with type-safe object parameters
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/renderer/screens/sign-in/components/SupersetLogo/SupersetLogo.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/index.tsapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/sign-in/components/SupersetLogo/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsxapps/desktop/src/renderer/stores/app-state.ts
apps/desktop/src/main/lib/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Implement IPC handlers accepting object parameters (not positional parameters) in apps/desktop/src/main/lib/*.ts files
Files:
apps/desktop/src/main/lib/api-client.tsapps/desktop/src/main/lib/menu.ts
apps/desktop/src/main/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
Load environment variables in apps/desktop/src/main/index.ts with override: true before any imports
Files:
apps/desktop/src/main/index.ts
🧠 Learnings (5)
📚 Learning: 2025-12-12T05:45:09.673Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T05:45:09.673Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables in apps/desktop/src/main/index.ts with override: true before any imports
Applied to files:
apps/api/src/env.tsapps/desktop/src/lib/electron-router-dom.tsapps/desktop/src/main/lib/auth/auth-service.tsapps/desktop/src/shared/constants.tsapps/desktop/src/main/index.ts.env.example
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx,js,jsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/lib/electron-router-dom.tsapps/desktop/src/main/lib/api-client.tsapps/desktop/src/main/lib/auth/auth-service.tsapps/desktop/src/main/index.ts
📚 Learning: 2025-12-12T05:45:09.672Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T05:45:09.672Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx,js,jsx} apps/desktop/src/lib/**/*.{ts,tsx,js,jsx} apps/desktop/src/shared/**/*.{ts,tsx,js,tsx} : Never import Node.js modules in renderer process or shared code, only in main process
Applied to files:
apps/desktop/src/main/lib/auth/auth-service.tsapps/desktop/src/main/index.ts
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Please use alias as defined in `tsconfig.json` when possible
Applied to files:
apps/desktop/src/shared/constants.tsapps/desktop/src/main/index.tsapps/desktop/src/renderer/screens/sign-in/components/SupersetLogo/index.ts
📚 Learning: 2025-12-12T05:45:09.673Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T05:45:09.673Z
Learning: Applies to apps/desktop/src/main/lib/*.ts : Implement IPC handlers accepting object parameters (not positional parameters) in apps/desktop/src/main/lib/*.ts files
Applied to files:
apps/desktop/src/main/index.tsapps/desktop/src/main/lib/auth/deep-link-handler.ts
🧬 Code graph analysis (13)
apps/desktop/src/main/lib/auth/token-storage.ts (3)
apps/desktop/src/main/lib/app-environment.ts (1)
SUPERSET_HOME_DIR(5-5)apps/desktop/src/shared/auth.ts (1)
AuthSession(6-11)apps/desktop/src/main/lib/auth/index.ts (1)
tokenStorage(4-4)
apps/web/src/app/api/auth/desktop/[provider]/route.ts (3)
packages/shared/src/constants.ts (2)
AUTH_PROVIDERS(2-2)AuthProvider(3-3)apps/api/src/env.ts (1)
env(4-23)apps/web/src/env.ts (1)
env(5-40)
apps/desktop/src/lib/electron-router-dom.ts (1)
apps/desktop/src/env.ts (1)
env(4-18)
apps/desktop/src/lib/trpc/routers/user/index.ts (2)
apps/desktop/src/lib/trpc/index.ts (1)
router(15-15)apps/desktop/src/main/lib/api-client.ts (1)
apiClient(11-27)
packages/trpc/src/router/user.ts (1)
packages/db/src/schema/schema.ts (1)
users(15-35)
apps/desktop/src/main/lib/api-client.ts (1)
apps/desktop/src/lib/trpc/routers/index.ts (1)
AppRouter(44-44)
apps/desktop/src/renderer/screens/main/index.tsx (4)
apps/desktop/src/renderer/stores/app-state.ts (2)
useCurrentView(61-61)useOpenSettings(68-68)apps/desktop/src/renderer/screens/main/components/Background.tsx (1)
Background(1-39)apps/desktop/src/renderer/screens/main/components/AppFrame.tsx (1)
AppFrame(5-7)apps/desktop/src/renderer/screens/sign-in/index.tsx (1)
SignInScreen(16-101)
apps/desktop/src/main/lib/menu.ts (1)
packages/shared/src/constants.ts (1)
COMPANY(6-17)
apps/desktop/src/shared/constants.ts (1)
apps/desktop/src/env.ts (1)
env(4-18)
apps/desktop/src/main/index.ts (3)
apps/desktop/src/main/lib/auth/deep-link-handler.ts (2)
isAuthDeepLink(125-139)handleAuthDeepLink(29-89)apps/desktop/src/main/lib/auth/auth-service.ts (1)
authService(272-272)apps/desktop/src/shared/constants.ts (1)
PROTOCOL_SCHEME(33-34)
apps/api/src/app/api/auth/desktop/token/route.ts (3)
packages/shared/src/constants.ts (1)
TOKEN_CONFIG(26-33)apps/api/src/app/api/auth/desktop/refresh/route.ts (1)
POST(23-98)apps/desktop/src/env.ts (1)
env(4-18)
apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx (2)
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsx (1)
AccountSettings(7-76)apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/index.ts (1)
AccountSettings(1-1)
apps/desktop/src/main/lib/auth/deep-link-handler.ts (5)
apps/desktop/src/main/lib/auth/index.ts (1)
AuthDeepLinkResult(2-2)apps/desktop/src/shared/auth.ts (1)
AuthSession(6-11)apps/desktop/src/main/lib/auth/pkce.ts (1)
pkceStore(104-104)apps/desktop/src/env.ts (1)
env(4-18)apps/desktop/src/shared/constants.ts (1)
PROTOCOL_SCHEMES(29-32)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 29-29: [UnorderedKey] The DESKTOP_AUTH_SECRET key should go before the VITE_DEV_SERVER_PORT key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Deploy Web
- GitHub Check: Deploy API
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Marketing
- GitHub Check: Build
🔇 Additional comments (22)
apps/desktop/src/renderer/screens/sign-in/components/SupersetLogo/index.ts (1)
1-1: LGTM! Clean barrel export pattern.This follows the component organization guideline perfectly, enabling cleaner imports via the folder path.
apps/desktop/src/renderer/screens/sign-in/components/SupersetLogo/SupersetLogo.tsx (3)
1-1: Good use of path alias.The import correctly uses the
@superset/uialias as specified in the coding guidelines.
3-5: LGTM! Type-safe interface.The props interface is properly typed without using
any, following TypeScript best practices.
7-25: Excellent implementation with proper accessibility!The component demonstrates several best practices:
- Proper accessibility with both
aria-labeland<title>for screen readers- Theme-compatible using
currentColorfor the fill- Clean className merging with the
cnutility- Follows the "one component per file" guideline
- Type-safe implementation without
anytypes- React 19.2.3 compatible functional component structure
apps/desktop/src/main/lib/menu.ts (1)
1-66: LGTM: centralizing help URLs viaCOMPANYconstants is a clean improvement.apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx (1)
1-24: LGTM: wiring the new “Account” section into SettingsContent is straightforward.apps/api/src/env.ts (1)
4-12: LGTM:DESKTOP_AUTH_SECRETserver env validation (min 32 chars) looks right.apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsx (1)
1-46: LGTM: “Account” section addition is clean and consistent with existing sidebar entries.apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/index.ts (1)
1-1: LGTM: barrel export is consistent with the component folder structure.apps/desktop/src/renderer/stores/app-state.ts (1)
5-12: LGTM:SettingsSectionunion expansion is safe and keeps defaults valid.apps/desktop/src/renderer/screens/main/index.tsx (1)
46-53: Good: workspace query is correctly gated behindisSignedIn.apps/web/src/app/api/auth/desktop/[provider]/route.ts (2)
19-70: Solid PKCE + provider validation; flow looks correct.Also applies to: 86-99
19-24: The current typing is correct for Next.js 16 route handlers. In Next.js 16, route handler params are asynchronous and must be typed asPromise<{ provider: string }>and awaited before use, which the code correctly implements. No changes needed.Likely an incorrect or invalid review comment.
packages/shared/src/constants.ts (3)
1-3: LGTM: Clean auth provider configuration.The const assertion and derived type provide type-safe provider literals throughout the codebase.
11-16: LGTM: Company URL constants.The URL additions are straightforward and provide centralized configuration for app links.
25-33: LGTM: Token configuration follows OAuth best practices.The 1-hour access token lifetime with 5-minute refresh threshold provides a good balance between security and UX. The 30-day refresh token is reasonable for a desktop application.
apps/desktop/src/shared/constants.ts (1)
1-34: LGTM: Consistent environment-based configuration.The migration to
env.NODE_ENVprovides centralized environment detection. The new protocol scheme constants properly support both development and production deep linking scenarios.apps/desktop/src/main/lib/auth/pkce.ts (1)
1-104: LGTM: RFC 7636-compliant PKCE implementation.The implementation correctly follows the PKCE specification with S256 challenge method, proper entropy (32 bytes for verifier, 16 bytes for state), and secure consumption pattern with state validation for CSRF protection. The 10-minute expiry window is appropriate for OAuth flow completion.
apps/desktop/src/main/index.ts (2)
25-81: LGTM: Complete cross-platform deep-link handling.The implementation correctly handles deep links on all platforms:
- macOS:
open-urlevent- Windows/Linux:
second-instanceevent with argv parsing + cold-start handlingThe single-instance lock ensures Windows/Linux users get proper deep-link processing when the app is already running, and
focusMainWindow()provides good UX.
83-110: LGTM: Robust initialization sequence.The async initialization properly sequences dependencies (DB → app state → auth service → agent hooks → app setup) and handles cold-start deep links. Wrapping agent hooks in try-catch with non-fatal error handling ensures the app can continue if hooks fail.
apps/desktop/src/main/lib/auth/auth-service.ts (1)
31-57: LGTM: Session initialization and deep-link handling.The initialization correctly restores persisted sessions with proper expiry checks, and
handleDeepLinkAuth()properly integrates deep-link results with session persistence and refresh scheduling.Also applies to: 130-146
apps/desktop/src/main/lib/auth/deep-link-handler.ts (1)
1-139: LGTM: Secure PKCE token exchange implementation.The deep-link handler correctly implements the OAuth PKCE flow with proper validation:
- State parameter validation via
pkceStore.consumeVerifier()provides CSRF protection- Error responses are handled gracefully with descriptive messages
- PKCE state is consistently cleared on all error paths
- Both development and production protocols are supported
| // Validate required parameters | ||
| if (!code || typeof code !== "string") { | ||
| return NextResponse.json( | ||
| { error: "Missing or invalid code parameter" }, | ||
| { status: 400 }, | ||
| ); | ||
| } | ||
|
|
||
| if (!code_verifier || typeof code_verifier !== "string") { | ||
| return NextResponse.json( | ||
| { error: "Missing or invalid code_verifier parameter" }, | ||
| { status: 400 }, | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Add code_verifier size/format bounds; consider constant-time compare for the challenge.
if (!code_verifier || typeof code_verifier !== "string") {
return NextResponse.json(
{ error: "Missing or invalid code_verifier parameter" },
{ status: 400 },
);
}
+
+ // RFC 7636: code_verifier is 43-128 chars from [A-Z / a-z / 0-9 / "-" / "." / "_" / "~"]
+ if (
+ code_verifier.length < 43 ||
+ code_verifier.length > 128 ||
+ !/^[A-Za-z0-9._~-]+$/.test(code_verifier)
+ ) {
+ return NextResponse.json(
+ { error: "Invalid code_verifier format" },
+ { status: 400 },
+ );
+ }
@@
- if (computedChallenge !== payload.codeChallenge) {
+ if (computedChallenge !== payload.codeChallenge) {
console.error("[token] PKCE verification failed");
return NextResponse.json(
{ error: "PKCE verification failed" },
{ status: 401 },
);
}(If you want the constant-time compare, use timingSafeEqual(Buffer.from(...), Buffer.from(...)) with equal-length inputs.)
Also applies to: 113-124
🤖 Prompt for AI Agents
In apps/api/src/app/api/auth/desktop/token/route.ts around lines 75-89 (and also
apply same changes at 113-124): the parameter validation currently only checks
presence and string type for code_verifier; add explicit PKCE bounds and format
checks (e.g., ensure code_verifier length is within PKCE limits, typically
43-128 chars, and contains only allowed URL-safe characters) and return 400 if
invalid. When comparing the derived challenge to the stored code_challenge,
perform a constant-time comparison using timingSafeEqual by converting both
values to Buffers of equal length (pad/normalize or reject mismatched lengths to
avoid leaking timing) to prevent timing attacks. Ensure the error responses
remain descriptive and consistent when these validations fail.
| // PKCE verified! Create access and refresh tokens | ||
| const [accessToken, refreshToken] = await Promise.all([ | ||
| createAccessToken(payload.userId, secret), | ||
| createRefreshToken(payload.userId, secret), | ||
| ]); | ||
|
|
||
| return NextResponse.json({ | ||
| access_token: accessToken.token, | ||
| access_token_expires_at: accessToken.expiresAt, | ||
| refresh_token: refreshToken.token, | ||
| refresh_token_expires_at: refreshToken.expiresAt, | ||
| }); |
There was a problem hiding this comment.
Replay protection isn’t actually enforced here (auth code jti is unused).
If replay resistance is a requirement, you’ll need a server-side “redeem once” mechanism (e.g., store jti with a 5-minute TTL and atomically reject re-use) before minting tokens.
🤖 Prompt for AI Agents
In apps/api/src/app/api/auth/desktop/token/route.ts around lines 126 to 137 the
auth code's jti is never recorded or checked so replay protection is not
enforced; before minting access/refresh tokens, atomically check-and-create a
server-side record for payload.jti (e.g., Redis SETNX or DB unique insert) with
a short TTL (≈5 minutes) and reject if the jti already exists, then proceed to
create tokens only after successful reservation; ensure the check+insert is
atomic to avoid race conditions and return a proper error if redemption fails.
| constructor() { | ||
| this.filePath = join(SUPERSET_HOME_DIR, SESSION_FILE_NAME); | ||
| } | ||
|
|
||
| async save(session: AuthSession): Promise<void> { | ||
| if (!safeStorage.isEncryptionAvailable()) { | ||
| console.warn( | ||
| "[auth] Secure storage not available, session will not be persisted", | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| const encrypted = safeStorage.encryptString(JSON.stringify(session)); | ||
| await fs.writeFile(this.filePath, encrypted); | ||
| } |
There was a problem hiding this comment.
Ensure SUPERSET_HOME_DIR exists before writing the session file.
import fs from "node:fs/promises";
import { join } from "node:path";
@@
async save(session: AuthSession): Promise<void> {
@@
+ await fs.mkdir(SUPERSET_HOME_DIR, { recursive: true });
const encrypted = safeStorage.encryptString(JSON.stringify(session));
await fs.writeFile(this.filePath, encrypted);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/auth/token-storage.ts around lines 16 to 30, the
code writes the encrypted session file to this.filePath without ensuring
SUPERSET_HOME_DIR exists; before calling fs.writeFile, obtain the directory via
dirname(this.filePath) and call await fs.mkdir(dir, { recursive: true }) (or
check exists and create) to ensure the directory is present, then proceed to
write; handle and surface errors (log or rethrow) from mkdir/write so failures
are visible.
| const { data: user, isLoading } = trpc.user.me.useQuery(); | ||
| const signOutMutation = trpc.auth.signOut.useMutation({ | ||
| onSuccess: () => toast.success("Signed out"), | ||
| }); | ||
|
|
||
| const signOut = () => signOutMutation.mutate(); | ||
|
|
There was a problem hiding this comment.
Disable “Sign Out” while pending + add error toast (prevents double-submit + silent failures).
const signOutMutation = trpc.auth.signOut.useMutation({
onSuccess: () => toast.success("Signed out"),
+ onError: () => toast.error("Sign out failed"),
});
- const signOut = () => signOutMutation.mutate();
+ const signOut = () => signOutMutation.mutate();
@@
- <Button variant="outline" onClick={() => signOut()}>
+ <Button
+ variant="outline"
+ onClick={signOut}
+ disabled={signOutMutation.isPending}
+ >
Sign Out
</Button>Also applies to: 69-71
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsx
around lines 8-14 (and also apply same change at lines 69-71), the Sign Out
action currently allows double submits and doesn't surface errors; update the
signOutMutation to include an onError handler that calls toast.error(...) with
the error message, use the mutation's isLoading state to disable the Sign Out
button(s), and guard the signOut function to return early if
signOutMutation.isLoading to prevent duplicate calls.
| const utils = trpc.useUtils(); | ||
| const { data: authState } = trpc.auth.getState.useQuery(); | ||
| const isSignedIn = authState?.isSignedIn ?? false; | ||
| const isAuthLoading = !authState; | ||
|
|
||
| // Subscribe to auth state changes | ||
| trpc.auth.onStateChange.useSubscription(undefined, { | ||
| onData: () => utils.auth.getState.invalidate(), | ||
| }); |
There was a problem hiding this comment.
Handle auth.getState errors explicitly (avoids “infinite loading” when the query fails).
- const { data: authState } = trpc.auth.getState.useQuery();
- const isSignedIn = authState?.isSignedIn ?? false;
- const isAuthLoading = !authState;
+ const {
+ data: authState,
+ isLoading: isAuthLoading,
+ isError: isAuthError,
+ refetch: refetchAuthState,
+ } = trpc.auth.getState.useQuery();
+ const isSignedIn = authState?.isSignedIn ?? false;
@@
- if (isAuthLoading) {
+ if (isAuthLoading) {
return (
<>
<Background />
<AppFrame>
<div className="flex h-full w-full items-center justify-center bg-background">
<LoadingSpinner />
</div>
</AppFrame>
</>
);
}
+
+ if (isAuthError) {
+ // Consider reusing the existing retry UI pattern
+ return (
+ <>
+ <Background />
+ <AppFrame>
+ <div className="flex h-full w-full items-center justify-center bg-background">
+ <Button variant="outline" onClick={() => refetchAuthState()}>
+ Retry
+ </Button>
+ </div>
+ </AppFrame>
+ </>
+ );
+ }Also applies to: 160-184
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/screens/main/index.tsx around lines 32-40 (and
similarly for 160-184), the auth.getState query currently treats absence of data
as "loading", which can hang UI when the query errors; update the useQuery call
to read error and loading flags (e.g., const { data: authState, isLoading, error
} = trpc.auth.getState.useQuery()) and compute isAuthLoading using the query's
loading state but treat an error as not loading (isAuthLoading = isLoading &&
!error). Also add explicit error handling: pass an onError callback to the query
(or check error) to log or surface the failure, and add onError to the
trpc.auth.onStateChange.useSubscription options while keeping onData to
invalidate utils.auth.getState; replicate these changes for the block at lines
160-184.
| .mutation(async ({ ctx, input }) => { | ||
| const user = await db.query.users.findFirst({ | ||
| where: eq(users.clerkId, ctx.session.userId), | ||
| where: eq(users.clerkId, ctx.userId), | ||
| }); | ||
| if (!user) throw new Error("User not found"); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find context type definition and protectedProcedure implementation
find . -name "*.ts" -type f | grep -E "(context|trpc)" | head -20Repository: superset-sh/superset
Length of output: 1116
🏁 Script executed:
#!/bin/bash
# Search for ctx type definition and userId references
rg -n "ctx\.userId|context\s*:" --type=ts -A 2 -B 2 | head -100Repository: superset-sh/superset
Length of output: 2594
🏁 Script executed:
#!/bin/bash
# Find protectedProcedure definition
rg -n "protectedProcedure\s*=" --type=ts -A 5 | head -80Repository: superset-sh/superset
Length of output: 439
🏁 Script executed:
#!/bin/bash
# Find where ctx is created/defined in context files
find . -name "*context*" -type f \( -name "*.ts" -o -name "*.tsx" \)Repository: superset-sh/superset
Length of output: 196
🏁 Script executed:
#!/bin/bash
# Read task.ts around the mutation to see the full context
sed -n '60,85p' packages/trpc/src/router/task.tsRepository: superset-sh/superset
Length of output: 765
🏁 Script executed:
#!/bin/bash
# Check the structure of the task router
head -80 packages/trpc/src/router/task.tsRepository: superset-sh/superset
Length of output: 2095
🏁 Script executed:
#!/bin/bash
# Check apps/api/src/trpc/context.ts to understand context type
cat apps/api/src/trpc/context.tsRepository: superset-sh/superset
Length of output: 1086
🏁 Script executed:
#!/antml/bin/bash
# Check packages/trpc/src/trpc.ts for complete context type definition
cat packages/trpc/src/trpc.tsRepository: superset-sh/superset
Length of output: 1898
Use TRPCError instead of Error for consistent error handling.
The mutation correctly uses protectedProcedure, which guarantees ctx.userId is non-null (checked at line 47 in packages/trpc/src/trpc.ts). The semantics are also correct—ctx.userId is the Clerk ID, consistently matched against users.clerkId throughout the codebase.
However, for consistency with adminProcedure (line 72 in packages/trpc/src/trpc.ts) and proper tRPC error propagation, replace the plain Error with TRPCError:
+import { TRPCError } from "@trpc/server";
...
const user = await db.query.users.findFirst({
where: eq(users.clerkId, ctx.userId),
});
- if (!user) throw new Error("User not found");
+ if (!user) throw new TRPCError({ code: "UNAUTHORIZED", message: "User not found in database." });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .mutation(async ({ ctx, input }) => { | |
| const user = await db.query.users.findFirst({ | |
| where: eq(users.clerkId, ctx.session.userId), | |
| where: eq(users.clerkId, ctx.userId), | |
| }); | |
| if (!user) throw new Error("User not found"); | |
| .mutation(async ({ ctx, input }) => { | |
| const user = await db.query.users.findFirst({ | |
| where: eq(users.clerkId, ctx.userId), | |
| }); | |
| if (!user) throw new TRPCError({ code: "UNAUTHORIZED", message: "User not found in database." }); |
🤖 Prompt for AI Agents
In packages/trpc/src/router/task.ts around lines 74 to 79, the mutation
currently throws a plain Error("User not found"); replace this with a tRPC error
for consistent error handling: import TRPCError from '@trpc/server' (or add to
existing imports) and throw new TRPCError({ code: 'NOT_FOUND', message: 'User
not found' }) instead of the plain Error so the error propagates correctly
through tRPC and matches adminProcedure behavior.
| export const userRouter = { | ||
| me: protectedProcedure.query(async ({ ctx }) => { | ||
| return db.query.users.findFirst({ | ||
| where: eq(users.clerkId, ctx.session.userId), | ||
| where: eq(users.clerkId, ctx.userId), | ||
| }); | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate the files we need to examine
git ls-files | grep -E "(router|user|task)" | head -20Repository: superset-sh/superset
Length of output: 1209
🏁 Script executed:
# Search specifically for the trpc package routers
find packages/trpc -type f -name "*.ts" -o -name "*.tsx" | head -30Repository: superset-sh/superset
Length of output: 347
🏁 Script executed:
# Read the user.ts file to see the code in context
cat -n packages/trpc/src/router/user.tsRepository: superset-sh/superset
Length of output: 581
🏁 Script executed:
# Read task.ts to see the pattern that should be applied
cat -n packages/trpc/src/router/task.tsRepository: superset-sh/superset
Length of output: 4178
🏁 Script executed:
# Read trpc.ts to understand context definition and protectedProcedure
cat -n packages/trpc/src/trpc.tsRepository: superset-sh/superset
Length of output: 2493
🏁 Script executed:
# Search for uses of userRouter.me or the me procedure in the codebase
rg "me\(" packages/ -A 2 -B 2 --max-count=20 | head -100Repository: superset-sh/superset
Length of output: 2012
🏁 Script executed:
# Search more specifically for trpc user.me usage
rg "user\.me|trpc\.user" apps/ --max-count=20Repository: superset-sh/superset
Length of output: 509
🏁 Script executed:
# Read layout.tsx to see how user.me is used
cat -n apps/admin/src/app/\(dashboard\)/layout.tsx | head -100Repository: superset-sh/superset
Length of output: 1850
Add user existence check in the me procedure.
The procedure should verify that the queried user exists in the database before returning, matching the pattern used in adminProcedure (trpc.ts, lines 62-71) and the create mutation in task.ts (lines 75-78). The caller in layout.tsx (lines 28-30) explicitly checks for null and throws an error—this validation should be enforced at the procedure level instead.
Replace the direct return with:
const user = await db.query.users.findFirst({
where: eq(users.clerkId, ctx.userId),
});
if (!user) {
throw new TRPCError({
code: "UNAUTHORIZED",
message: "User not found in database.",
});
}
return user;
🤖 Prompt for AI Agents
In packages/trpc/src/router/user.ts around lines 8 to 13, the me procedure
currently returns the result of db.query.users.findFirst directly without
verifying existence; update it to await the query into a user variable, check if
user is null, and if so throw a TRPCError with code "UNAUTHORIZED" and message
"User not found in database.", otherwise return the user — matching the
existence check pattern used in adminProcedure and task.create.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsx (1)
8-14: Prevent double-submit + surface sign-out failures (disable button while pending, add onError).
This is still vulnerable to repeated clicks and silent mutation failures.export function AccountSettings() { const { data: user, isLoading } = trpc.user.me.useQuery(); const signOutMutation = trpc.auth.signOut.useMutation({ onSuccess: () => toast.success("Signed out"), + onError: (err) => + toast.error(err.message || "Sign out failed"), }); - const signOut = () => signOutMutation.mutate(); + const signOut = () => { + if (signOutMutation.isPending) return; + signOutMutation.mutate(); + }; @@ - <Button variant="outline" onClick={() => signOut()}> + <Button + variant="outline" + onClick={signOut} + disabled={signOutMutation.isPending} + > Sign Out </Button>Also applies to: 69-71
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsx (2)
15-20: Harden initials + consideraltfor AvatarImage.
split(" ")without trimming/filtering can yield empty segments; alsoAvatarImagelikely benefits from analtfor accessibility.- const initials = user?.name - ?.split(" ") - .map((n) => n[0]) + const initials = user?.name + ?.trim() + .split(/\s+/) + .filter(Boolean) + .map((n) => n[0]) .join("") .toUpperCase() .slice(0, 2); @@ - <AvatarImage src={user.avatarUrl ?? undefined} /> + <AvatarImage + src={user.avatarUrl ?? undefined} + alt={user.name ?? "User avatar"} + />Also applies to: 47-55
36-59: Differentiate “not signed in” vs “failed to load” (use query error state).
Right nowuserfalsy renders a generic failure message, which may also occur when unauthenticated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsx(1 hunks)apps/desktop/src/renderer/screens/sign-in/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/screens/sign-in/index.tsx
🧰 Additional context used
📓 Path-based instructions (6)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsx
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsx
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for code formatting and linting, running at root level for speed
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid
anytype and prioritize type safety in TypeScript code
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsx
**/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/components/**/*.{ts,tsx}: Structure project folders as one folder per component with PascalCase naming (ComponentName/ComponentName.tsx + index.ts barrel export)
Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Use one component per file (no multi-component files)
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsx
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Call IPC methods from renderer process using window.ipcRenderer.invoke with type-safe object parameters
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsx
🧬 Code graph analysis (1)
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsx (6)
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/index.ts (1)
AccountSettings(1-1)packages/ui/src/components/ui/sonner.tsx (1)
toast(40-40)apps/desktop/src/main/lib/auth/auth-service.ts (1)
signOut(151-154)packages/ui/src/components/ui/skeleton.tsx (1)
Skeleton(13-13)packages/ui/src/components/ui/avatar.tsx (3)
Avatar(53-53)AvatarImage(53-53)AvatarFallback(53-53)packages/ui/src/components/ui/button.tsx (1)
Button(60-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Marketing
- GitHub Check: Build
1 participant
Summary
superset-dev://deep links in development on macOSChanges
/auth/desktop/callbackpage for OAuth redirects/api/auth/desktop/[provider]endpoint to initiate OAuth flowTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.