feat(auth): migrate from Clerk to Better Auth #543
Conversation
📝 WalkthroughWalkthroughReplace Clerk with a new Better Auth stack: add Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as User Browser
participant Web as Next.js App
participant Auth as Better Auth (`@superset/auth`)
participant DB as Database
participant Desktop as Desktop App
rect rgb(230,245,255)
Note over Browser,Auth: Social sign-in (web flow)
Browser->>Web: GET /api/auth/desktop/connect?provider=google
Web->>Auth: auth.api.signInSocial(provider, { callbackURL })
Auth->>DB: findOrCreate user, create session
DB-->>Auth: user + session
Auth-->>Web: response with set-cookie + redirect URL
Web-->>Browser: redirect to provider / set cookies
end
rect rgb(245,255,230)
Note over Browser,Desktop: Desktop deep-link callback / session-based desktop success
Browser->>Web: GET /auth/desktop/success?desktop_state=...
Web->>Auth: auth.api.getSession({ headers })
Auth->>DB: validate session, return activeOrganizationId
DB-->>Auth: session + user + org
Auth-->>Web: session data
Web->>Browser: render DesktopRedirect with desktop URL (token+state)
Browser->>Desktop: open desktop:// callback URL
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
🔇 Additional comments (5)
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
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/trpc/src/router/organization/organization.ts (2)
59-81: Manual UUID generation conflicts with database defaults.Lines 62 and 72 use
crypto.randomUUID()to generate IDs, but the database schema (based on the migration) defines columns withDEFAULT gen_random_uuid(). The PR summary mentionsgenerateId: false, which suggests database-generated IDs are disabled in the Better Auth config.However, this creates inconsistency:
- Some code paths rely on database defaults (e.g., when values are not explicitly provided)
- Other code paths use manual generation
- Different UUID generation methods can have different formatting/casing
For consistency, either:
- Remove the database
DEFAULT gen_random_uuid()from the migration if manual generation is always used- Or rely on database defaults by omitting the
idfield from the insert values🔎 Recommended approach: Let database handle IDs
const [organization] = await db .insert(organizations) - .values({ - id: crypto.randomUUID(), + .values({ name: input.name, slug: input.slug, logo: input.logo, createdAt: new Date(), }) .returning(); if (organization) { await db.insert(members).values({ - id: crypto.randomUUID(), organizationId: organization.id, userId: ctx.session.user.id, role: "owner", createdAt: new Date(), }); }
83-99: Weakened type validation on organization ID.Line 86 changed the
idvalidation fromz.string().uuid()toz.string(). This allows non-UUID strings to pass validation, which will fail at the database level since theidcolumn is of typeuuid.🔎 Proposed fix
.input( z.object({ - id: z.string(), + id: z.string().uuid(), name: z.string().min(1).optional(), logo: z.string().url().optional(), }), )Based on library documentation, Zod 4 maintains
.uuid()validation for strings, so this should work correctly.
♻️ Duplicate comments (1)
apps/web/src/app/(auth)/sign-in/[[...sign-in]]/page.tsx (1)
32-46: Same loading state concern applies to GitHub sign-in.This has the same potential issue as the Google sign-in handler—verify that the loading state management works correctly with OAuth redirects.
🧹 Nitpick comments (16)
apps/desktop/src/main/lib/auth/auth.ts (3)
103-105: Empty catch block silently ignores all errors.While
ENOENT(file doesn't exist) is expected, other errors (permission denied, disk full) would be silently swallowed. Consider logging unexpected errors.🔎 Proposed fix
async signOut(): Promise<void> { this.token = null; try { await fs.unlink(TOKEN_FILE); - } catch {} + } catch (err) { + // ENOENT is expected if token file doesn't exist + if ((err as NodeJS.ErrnoException).code !== "ENOENT") { + console.warn("[auth] Failed to remove token file:", err); + } + } this.emit("state-changed", this.getState()); }
57-59: Add return type annotation for consistency.
getState()has an implicit return type while other methods are explicitly typed. Consider adding the return type for consistency and better IDE support.🔎 Proposed fix
-getAccessToken(): string | null { +getAccessToken(): string | null { return this.token; } -getState() { +getState(): { isSignedIn: boolean } { return { isSignedIn: !!this.token }; }
66-69: State pruning only occurs during signIn calls.The stale state cleanup only runs when
signInis called. If a user initiates sign-in but never completes it, and then doesn't attempt to sign in again, those stale states persist in memory indefinitely.This is a minor concern since:
- Memory impact is negligible (small strings)
- App restarts clear the store
- States older than 10 minutes are functionally useless anyway
Consider this acceptable for now, but if you want periodic cleanup:
🔎 Optional: Move cleanup to a separate method
+private pruneExpiredStates(): void { + const tenMinutesAgo = Date.now() - 10 * 60 * 1000; + for (const [s, ts] of stateStore) { + if (ts < tenMinutesAgo) stateStore.delete(s); + } +} async signIn(provider: AuthProvider): Promise<SignInResult> { try { const state = crypto.randomBytes(32).toString("base64url"); stateStore.set(state, Date.now()); - - const tenMinutesAgo = Date.now() - 10 * 60 * 1000; - for (const [s, ts] of stateStore) { - if (ts < tenMinutesAgo) stateStore.delete(s); - } + this.pruneExpiredStates();apps/web/src/app/(dashboard)/components/Header/Header.tsx (1)
24-27: Consider adding error handling for the sign-out flow.The async
handleSignOutfunction doesn't handle potential errors fromauthClient.signOut(). If the sign-out request fails, the user would be redirected to/sign-inanyway, potentially causing confusion.🔎 Proposed improvement
const handleSignOut = async () => { - await authClient.signOut(); - router.push("/sign-in"); + try { + await authClient.signOut(); + router.push("/sign-in"); + } catch (error) { + console.error("Sign out failed:", error); + // Optionally show a toast or still redirect + router.push("/sign-in"); + } };packages/auth/src/env.ts (3)
6-6: Hardcoded relative path may be fragile.The path
"../../../.env"assumes a fixed directory structure relative topackages/auth/src/. This could break if the package is moved or ifprocess.cwd()differs during execution (e.g., different build/test contexts).🔎 Consider using a more robust path resolution
-config({ path: path.resolve(process.cwd(), "../../../.env") }); +// Find monorepo root by looking for a marker file or use a shared utility +const monorepoRoot = path.resolve(__dirname, "../../../.."); +config({ path: path.resolve(monorepoRoot, ".env") });Alternatively, rely on environment variables being set by the host application rather than loading from a specific file path.
9-15: Add minimum length validation for secrets to catch empty strings early.While
emptyStringAsUndefined: trueconverts empty strings to undefined, adding.min(1)provides clearer error messages when secrets are missing. This aligns with the pattern inapps/api/src/env.tswhere secrets have minimum length validation.🔎 Proposed validation enhancement
server: { - GH_CLIENT_ID: z.string(), - GH_CLIENT_SECRET: z.string(), - GOOGLE_CLIENT_ID: z.string(), - GOOGLE_CLIENT_SECRET: z.string(), - BETTER_AUTH_SECRET: z.string(), + GH_CLIENT_ID: z.string().min(1), + GH_CLIENT_SECRET: z.string().min(1), + GOOGLE_CLIENT_ID: z.string().min(1), + GOOGLE_CLIENT_SECRET: z.string().min(1), + BETTER_AUTH_SECRET: z.string().min(32), },
26-26: Consider makingskipValidationconditional.With
skipValidation: true, environment validation is always skipped, which could mask missing or invalid environment variables until runtime. Other env files in the codebase use a conditional like!!process.env.SKIP_ENV_VALIDATION.🔎 Conditional validation pattern
-skipValidation: true, +skipValidation: !!process.env.SKIP_ENV_VALIDATION,apps/api/src/app/api/electric/[...path]/route.ts (1)
56-70: Consider adding a timeout to the fetch call.The
fetchcall to the Electric origin has no timeout, which could cause the request to hang indefinitely if the upstream service is unresponsive. This could exhaust connection resources under load.🔎 Add AbortSignal timeout
-const response = await fetch(originUrl.toString()); +const response = await fetch(originUrl.toString(), { + signal: AbortSignal.timeout(30_000), // 30 second timeout +});apps/web/src/app/(auth)/sign-up/[[...sign-up]]/page.tsx (1)
16-46: Consider extracting shared OAuth logic.The
signUpWithGoogleandsignUpWithGithubfunctions share nearly identical logic. Consider extracting a shared helper function:const signUpWithProvider = async ( provider: "google" | "github", setLoading: (loading: boolean) => void ) => { setLoading(true); setError(null); try { await authClient.signIn.social({ provider, callbackURL: env.NEXT_PUBLIC_WEB_URL, }); } catch (err) { console.error("Sign up failed:", err); setError("Failed to sign up. Please try again."); setLoading(false); } };This reduces duplication and makes future maintenance easier.
apps/marketing/src/app/components/CTAButtons/CTAButtons.tsx (1)
9-11: Consider error handling for session retrieval.The session retrieval looks correct, but
auth.api.getSession()could potentially throw an error or return an unexpected value if the auth service is unavailable.🔎 Optional: Add error boundary or fallback
export async function CTAButtons() { - const session = await auth.api.getSession({ headers: await headers() }); + let session; + try { + session = await auth.api.getSession({ headers: await headers() }); + } catch (error) { + console.error("Failed to get session:", error); + session = null; + } if (session) {Alternatively, let the error propagate to the nearest error boundary if that's the intended behavior.
apps/api/src/lib/auth.ts (1)
3-7: Consider adding an explicit return type annotation.While TypeScript can infer the return type from
auth.api.getSession, adding an explicit return type would improve code clarity and help catch type mismatches earlier.🔎 Proposed enhancement with explicit typing
+import type { Session } from "better-auth/types"; + -export async function getSession(request: Request) { +export async function getSession(request: Request): Promise<Session | null> { return auth.api.getSession({ headers: request.headers, }); }apps/admin/src/components/PostHogUserIdentifier/PostHogUserIdentifier.tsx (1)
9-24: Consider using session data directly instead of additional TRPC query.The component fetches user data via
trpc.user.me.queryOptions()whensession.userlikely contains the necessary fields (id, email, name). The web app's equivalent component (apps/web/src/components/PostHogUserIdentifier/PostHogUserIdentifier.tsx) usessession.userdirectly, avoiding the extra round-trip.🔎 Proposed simplification matching the web app pattern
export function PostHogUserIdentifier() { const { data: session } = authClient.useSession(); - const trpc = useTRPC(); - - const { data: user } = useQuery({ - ...trpc.user.me.queryOptions(), - enabled: !!session?.user, - }); useEffect(() => { - if (user) { - posthog.identify(user.id, { email: user.email, name: user.name }); + if (session?.user) { + posthog.identify(session.user.id, { + email: session.user.email, + name: session.user.name, + }); - } else if (!session?.user) { + } else if (session === null) { posthog.reset(); } - }, [user, session?.user]); + }, [session]); return null; }apps/admin/src/app/(dashboard)/components/AppSidebar/components/NavUser/NavUser.tsx (1)
42-50: Consider adding error handling for sign-out failures.The
handleSignOutfunction lacks error handling for potential sign-out failures. While sign-out operations typically succeed, network errors or API issues could occur.🔎 Proposed enhancement with error handling
const handleSignOut = async () => { + try { await authClient.signOut({ fetchOptions: { onSuccess: () => { window.location.href = env.NEXT_PUBLIC_WEB_URL; }, }, }); + } catch (error) { + console.error("Sign out failed:", error); + // Optionally show a toast notification to the user + } };packages/trpc/src/router/integration/integration.ts (1)
21-23: Use TRPCError for consistent error handling.The membership check throws a generic
Errorinstead ofTRPCError. This is inconsistent with the TRPC error handling pattern used elsewhere (e.g., intrpc.ts).🔎 Proposed fix
+import { TRPCError } from "@trpc/server"; + // ... if (!membership) { - throw new Error("Not a member of this organization"); + throw new TRPCError({ + code: "FORBIDDEN", + message: "Not a member of this organization", + }); }apps/web/src/proxy.ts (1)
18-37: LGTM with optional refactor suggestion.The proxy logic correctly handles authentication redirects:
- Logged-in users are redirected away from sign-in/sign-up pages
- Logged-out users are redirected to sign-in unless on a public route
For slightly better clarity and to avoid potential edge cases with
startsWith(e.g.,/sign-in-testmatching/sign-in), consider using exact matches or the existingisPublicRoutehelper:🔎 Optional refactor for consistency
const pathname = req.nextUrl.pathname; if ( session && - (pathname.startsWith("/sign-in") || pathname.startsWith("/sign-up")) + (pathname === "/sign-in" || pathname === "/sign-up") ) { return NextResponse.redirect(new URL("/", req.url)); }Or if sign-in/sign-up have sub-paths, this is fine as-is.
packages/trpc/src/router/admin/admin.ts (1)
16-22: Clarify comment and add error handling for non-existent users.The comment on line 19 is misleading. Cascading deletes are handled by the database-level foreign key constraints (configured with
onDelete: "cascade"in the schema), not by the Better Auth library.Additionally, the mutation currently returns
{ success: true }even if the user doesn't exist. Consider verifying that a user was actually deleted.🔎 Proposed improvements
deleteUser: adminProcedure .input(z.object({ userId: z.string() })) .mutation(async ({ input }) => { - // Delete user - Better Auth handles cascading session cleanup - await db.delete(users).where(eq(users.id, input.userId)); - return { success: true }; + // Delete user - database FK constraints handle cascading deletes + const result = await db.delete(users).where(eq(users.id, input.userId)); + + if (result.rowCount === 0) { + throw new TRPCError({ + code: "NOT_FOUND", + message: "User not found", + }); + } + + return { success: true }; }),Note: You'll need to re-add the
TRPCErrorimport if you implement this suggestion.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (96)
.env.exampleapps/admin/package.jsonapps/admin/src/app/(dashboard)/components/AppSidebar/AppSidebar.tsxapps/admin/src/app/(dashboard)/components/AppSidebar/components/NavUser/NavUser.tsxapps/admin/src/app/(dashboard)/components/LeaderboardTable/LeaderboardTable.tsxapps/admin/src/app/(dashboard)/users/components/UsersTable/UsersTable.tsxapps/admin/src/app/(dashboard)/users/deleted/components/DeletedUsersTable/DeletedUsersTable.tsxapps/admin/src/app/(dashboard)/users/deleted/components/DeletedUsersTable/index.tsapps/admin/src/app/(dashboard)/users/deleted/page.tsxapps/admin/src/app/layout.tsxapps/admin/src/components/PostHogUserIdentifier/PostHogUserIdentifier.tsxapps/admin/src/env.tsapps/admin/src/proxy.tsapps/admin/src/trpc/react.tsxapps/api/next.config.tsapps/api/package.jsonapps/api/src/app/api/auth/[...all]/route.tsapps/api/src/app/api/auth/desktop/connect/route.tsapps/api/src/app/api/auth/desktop/github/route.tsapps/api/src/app/api/auth/desktop/google/route.tsapps/api/src/app/api/auth/desktop/refresh/route.tsapps/api/src/app/api/auth/desktop/tokens.tsapps/api/src/app/api/electric/[...path]/route.tsapps/api/src/app/api/electric/[...path]/utils.tsapps/api/src/app/api/integrations/linear/connect/route.tsapps/api/src/env.tsapps/api/src/lib/auth.tsapps/api/src/proxy.tsapps/api/src/trpc/context.tsapps/desktop/src/lib/trpc/routers/auth/index.tsapps/desktop/src/lib/trpc/routers/index.tsapps/desktop/src/main/index.tsapps/desktop/src/main/lib/auth/auth-service.tsapps/desktop/src/main/lib/auth/auth.tsapps/desktop/src/main/lib/auth/crypto-storage.tsapps/desktop/src/main/lib/auth/deep-link-handler.tsapps/desktop/src/main/lib/auth/index.tsapps/desktop/src/main/lib/auth/pkce.tsapps/desktop/src/main/lib/auth/token-storage.tsapps/desktop/src/renderer/screens/main/components/AvatarDropdown/AvatarDropdown.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsxapps/desktop/src/renderer/screens/main/components/TasksView/components/OrganizationSwitcher/OrganizationSwitcher.tsxapps/desktop/src/shared/auth.tsapps/marketing/package.jsonapps/marketing/src/app/components/CTAButtons/CTAButtons.tsxapps/marketing/src/app/layout.tsxapps/marketing/src/env.tsapps/marketing/src/proxy.tsapps/web/package.jsonapps/web/src/app/(auth)/layout.tsxapps/web/src/app/(auth)/sign-in/[[...sign-in]]/page.tsxapps/web/src/app/(auth)/sign-up/[[...sign-up]]/page.tsxapps/web/src/app/(auth)/sso-callback/page.tsxapps/web/src/app/(dashboard)/components/Header/Header.tsxapps/web/src/app/api/auth/desktop/github/route.tsapps/web/src/app/api/auth/desktop/google/route.tsapps/web/src/app/auth/desktop/success/components/DesktopAuthSuccess/DesktopAuthSuccess.tsxapps/web/src/app/auth/desktop/success/components/DesktopAuthSuccess/index.tsapps/web/src/app/auth/desktop/success/components/DesktopRedirect/DesktopRedirect.tsxapps/web/src/app/auth/desktop/success/components/DesktopRedirect/index.tsapps/web/src/app/auth/desktop/success/components/LoadingFallback/LoadingFallback.tsxapps/web/src/app/auth/desktop/success/components/LoadingFallback/index.tsapps/web/src/app/auth/desktop/success/page.tsxapps/web/src/app/layout.tsxapps/web/src/components/PostHogUserIdentifier/PostHogUserIdentifier.tsxapps/web/src/env.tsapps/web/src/proxy.tsapps/web/src/trpc/react.tsxpackages/auth/package.jsonpackages/auth/src/client.tspackages/auth/src/env.tspackages/auth/src/index.tspackages/auth/tsconfig.jsonpackages/db/drizzle/0005_back_up_user_tables.sqlpackages/db/drizzle/0006_add_better_auth_tables.sqlpackages/db/drizzle/meta/0005_snapshot.jsonpackages/db/drizzle/meta/0006_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/package.jsonpackages/db/src/env.tspackages/db/src/schema/auth.tspackages/db/src/schema/index.tspackages/db/src/schema/relations.tspackages/db/src/schema/schema.tspackages/trpc/package.jsonpackages/trpc/src/env.tspackages/trpc/src/router/admin/admin.tspackages/trpc/src/router/analytics/analytics.tspackages/trpc/src/router/integration/integration.tspackages/trpc/src/router/integration/linear/linear.tspackages/trpc/src/router/integration/linear/utils.tspackages/trpc/src/router/organization/organization.tspackages/trpc/src/router/task/task.tspackages/trpc/src/router/user/user.tspackages/trpc/src/router/user/utils/sync-user-from-clerk.tspackages/trpc/src/trpc.ts
💤 Files with no reviewable changes (28)
- apps/web/src/app/auth/desktop/success/components/LoadingFallback/LoadingFallback.tsx
- apps/desktop/src/main/lib/auth/crypto-storage.ts
- apps/api/src/env.ts
- packages/trpc/src/env.ts
- apps/web/src/app/api/auth/desktop/github/route.ts
- apps/web/src/app/api/auth/desktop/google/route.ts
- apps/web/src/app/auth/desktop/success/components/LoadingFallback/index.ts
- apps/admin/src/app/(dashboard)/users/deleted/components/DeletedUsersTable/index.ts
- apps/admin/src/app/(dashboard)/users/deleted/components/DeletedUsersTable/DeletedUsersTable.tsx
- apps/desktop/src/main/lib/auth/token-storage.ts
- apps/desktop/src/main/lib/auth/auth-service.ts
- apps/web/src/app/(auth)/sso-callback/page.tsx
- apps/marketing/src/env.ts
- apps/admin/src/app/(dashboard)/users/deleted/page.tsx
- apps/api/src/app/api/auth/desktop/refresh/route.ts
- apps/admin/src/env.ts
- apps/web/src/app/auth/desktop/success/components/DesktopAuthSuccess/DesktopAuthSuccess.tsx
- apps/desktop/src/main/lib/auth/deep-link-handler.ts
- apps/desktop/src/shared/auth.ts
- apps/marketing/src/proxy.ts
- packages/trpc/src/router/user/utils/sync-user-from-clerk.ts
- apps/web/src/app/auth/desktop/success/components/DesktopAuthSuccess/index.ts
- apps/api/src/app/api/auth/desktop/github/route.ts
- apps/api/next.config.ts
- apps/api/src/app/api/auth/desktop/google/route.ts
- apps/api/src/app/api/auth/desktop/tokens.ts
- apps/web/src/env.ts
- apps/desktop/src/main/lib/auth/pkce.ts
🧰 Additional context used
📓 Path-based instructions (8)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/renderer/screens/main/components/TasksView/components/OrganizationSwitcher/OrganizationSwitcher.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsxapps/desktop/src/renderer/screens/main/components/AvatarDropdown/AvatarDropdown.tsxapps/desktop/src/lib/trpc/routers/index.tsapps/desktop/src/lib/trpc/routers/auth/index.tsapps/desktop/src/main/lib/auth/auth.tsapps/desktop/src/main/lib/auth/index.tsapps/desktop/src/main/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type safety and avoid
anytypes unless absolutely necessary in TypeScript files
Files:
apps/desktop/src/renderer/screens/main/components/TasksView/components/OrganizationSwitcher/OrganizationSwitcher.tsxapps/web/src/app/auth/desktop/success/components/DesktopRedirect/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsxpackages/db/src/schema/index.tsapps/desktop/src/renderer/screens/main/components/AvatarDropdown/AvatarDropdown.tsxpackages/trpc/src/router/integration/linear/linear.tsapps/web/src/app/layout.tsxapps/web/src/app/auth/desktop/success/components/DesktopRedirect/DesktopRedirect.tsxapps/api/src/app/api/auth/desktop/connect/route.tspackages/auth/src/client.tsapps/admin/src/app/(dashboard)/users/components/UsersTable/UsersTable.tsxpackages/auth/src/index.tsapps/admin/src/trpc/react.tsxpackages/trpc/src/router/task/task.tsapps/admin/src/proxy.tspackages/trpc/src/router/analytics/analytics.tsapps/admin/src/app/(dashboard)/components/AppSidebar/components/NavUser/NavUser.tsxpackages/auth/src/env.tsapps/desktop/src/lib/trpc/routers/index.tsapps/api/src/app/api/auth/[...all]/route.tsapps/api/src/app/api/integrations/linear/connect/route.tsapps/admin/src/app/(dashboard)/components/LeaderboardTable/LeaderboardTable.tsxpackages/db/src/schema/auth.tsapps/desktop/src/lib/trpc/routers/auth/index.tsapps/admin/src/app/(dashboard)/components/AppSidebar/AppSidebar.tsxpackages/trpc/src/router/integration/integration.tsapps/web/src/proxy.tspackages/db/src/env.tsapps/web/src/app/(auth)/sign-up/[[...sign-up]]/page.tsxapps/admin/src/components/PostHogUserIdentifier/PostHogUserIdentifier.tsxapps/web/src/app/(auth)/layout.tsxapps/desktop/src/main/lib/auth/auth.tsapps/admin/src/app/layout.tsxapps/api/src/app/api/electric/[...path]/route.tsapps/api/src/app/api/electric/[...path]/utils.tsapps/web/src/trpc/react.tsxapps/web/src/app/(dashboard)/components/Header/Header.tsxpackages/trpc/src/trpc.tsapps/web/src/app/(auth)/sign-in/[[...sign-in]]/page.tsxapps/web/src/components/PostHogUserIdentifier/PostHogUserIdentifier.tsxapps/desktop/src/main/lib/auth/index.tsapps/marketing/src/app/layout.tsxapps/api/src/trpc/context.tsapps/api/src/lib/auth.tsapps/desktop/src/main/index.tsapps/marketing/src/app/components/CTAButtons/CTAButtons.tsxpackages/trpc/src/router/integration/linear/utils.tspackages/db/src/schema/relations.tsapps/web/src/app/auth/desktop/success/page.tsxpackages/trpc/src/router/admin/admin.tspackages/trpc/src/router/user/user.tspackages/db/src/schema/schema.tsapps/api/src/proxy.tspackages/trpc/src/router/organization/organization.ts
apps/**/src/**/**/[A-Z]*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Structure component folders with one component per file using format
ComponentName/ComponentName.tsxwithindex.tsbarrel export
Files:
apps/desktop/src/renderer/screens/main/components/TasksView/components/OrganizationSwitcher/OrganizationSwitcher.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsxapps/desktop/src/renderer/screens/main/components/AvatarDropdown/AvatarDropdown.tsxapps/web/src/app/auth/desktop/success/components/DesktopRedirect/DesktopRedirect.tsxapps/admin/src/app/(dashboard)/users/components/UsersTable/UsersTable.tsxapps/admin/src/app/(dashboard)/components/AppSidebar/components/NavUser/NavUser.tsxapps/admin/src/app/(dashboard)/components/LeaderboardTable/LeaderboardTable.tsxapps/admin/src/app/(dashboard)/components/AppSidebar/AppSidebar.tsxapps/admin/src/components/PostHogUserIdentifier/PostHogUserIdentifier.tsxapps/web/src/app/(dashboard)/components/Header/Header.tsxapps/web/src/components/PostHogUserIdentifier/PostHogUserIdentifier.tsxapps/marketing/src/app/components/CTAButtons/CTAButtons.tsx
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Move Node.js functionality needed in Electron renderer to
src/main/lib/and communicate via IPC
Files:
apps/desktop/src/renderer/screens/main/components/TasksView/components/OrganizationSwitcher/OrganizationSwitcher.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsxapps/desktop/src/renderer/screens/main/components/AvatarDropdown/AvatarDropdown.tsxapps/desktop/src/lib/trpc/routers/index.tsapps/desktop/src/lib/trpc/routers/auth/index.tsapps/desktop/src/main/lib/auth/auth.tsapps/desktop/src/main/lib/auth/index.tsapps/desktop/src/main/index.ts
apps/desktop/src/{shared/ipc-channels.ts,main/**/*ipcs.ts,renderer/**/*.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Define all Electron IPC channel types in
apps/desktop/src/shared/ipc-channels.tsbefore implementing handlers
Files:
apps/desktop/src/renderer/screens/main/components/TasksView/components/OrganizationSwitcher/OrganizationSwitcher.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsxapps/desktop/src/renderer/screens/main/components/AvatarDropdown/AvatarDropdown.tsx
packages/db/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations in the
packages/dbpackage
Files:
packages/db/src/schema/index.tspackages/db/src/schema/auth.tspackages/db/src/env.tspackages/db/src/schema/relations.tspackages/db/src/schema/schema.ts
packages/db/src/schema/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Create database migrations by changing Drizzle schema and running
drizzle-kit generate --name="<sample_name_snake_case>"on a new Neon branch
Files:
packages/db/src/schema/index.tspackages/db/src/schema/auth.tspackages/db/src/schema/relations.tspackages/db/src/schema/schema.ts
apps/desktop/src/main/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
Load environment variables in
src/main/index.tswithoverride: truebefore any other imports in the desktop app
Files:
apps/desktop/src/main/index.ts
🧠 Learnings (12)
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables in `src/main/index.ts` with `override: true` before any other imports in the desktop app
Applied to files:
apps/web/src/app/auth/desktop/success/components/DesktopRedirect/index.tspackages/auth/src/env.tsapps/web/src/app/auth/desktop/success/page.tsx
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/desktop/src/**/*.{ts,tsx} : Move Node.js functionality needed in Electron renderer to `src/main/lib/` and communicate via IPC
Applied to files:
apps/web/src/app/auth/desktop/success/components/DesktopRedirect/index.tsapps/web/src/app/auth/desktop/success/components/DesktopRedirect/DesktopRedirect.tsxapps/admin/src/proxy.tsapps/desktop/src/lib/trpc/routers/auth/index.tsapps/desktop/src/main/lib/auth/index.tsapps/desktop/src/main/index.tsapps/web/src/app/auth/desktop/success/page.tsx
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Use alias as defined in `tsconfig.json` when possible
Applied to files:
apps/web/src/app/auth/desktop/success/components/DesktopRedirect/index.tspackages/db/package.jsonpackages/auth/tsconfig.json
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to packages/db/src/schema/**/*.ts : Create database migrations by changing Drizzle schema and running `drizzle-kit generate --name="<sample_name_snake_case>"` on a new Neon branch
Applied to files:
packages/db/src/schema/index.tspackages/db/drizzle/0006_add_better_auth_tables.sqlpackages/db/package.jsonpackages/db/drizzle/meta/0005_snapshot.jsonpackages/db/src/schema/auth.tspackages/db/src/schema/schema.ts
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to packages/db/**/*.ts : Use Drizzle ORM for all database operations in the `packages/db` package
Applied to files:
packages/auth/src/index.tspackages/db/package.jsonapps/admin/package.jsonpackages/db/src/schema/auth.tspackages/trpc/src/router/integration/integration.tsapps/api/src/app/api/electric/[...path]/utils.tspackages/db/src/schema/relations.tspackages/db/src/schema/schema.tspackages/trpc/src/router/organization/organization.ts
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/**/{utils,hooks,constants,stores,providers,config,*.stories.tsx} : Co-locate utilities, hooks, constants, config files, and stories next to the files that use them
Applied to files:
packages/db/package.json
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to **/*.{ts,tsx} : Use type safety and avoid `any` types unless absolutely necessary in TypeScript files
Applied to files:
packages/db/package.json
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/**/src/**/**/[A-Z]*.tsx : Structure component folders with one component per file using format `ComponentName/ComponentName.tsx` with `index.ts` barrel export
Applied to files:
packages/db/package.json
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/admin/src/trpc/react.tsxapps/desktop/src/lib/trpc/routers/auth/index.tsapps/web/src/trpc/react.tsxpackages/trpc/src/trpc.ts
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/**/src/(middleware|proxy).ts : In Next.js 16, never create `middleware.ts`. Always use `proxy.ts` instead for request interception
Applied to files:
apps/admin/src/proxy.tsapps/api/src/app/api/auth/[...all]/route.tsapps/web/src/proxy.tsapps/api/src/proxy.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from `trpc/server/observable` instead of async generators, as the library explicitly checks `isObservable(result)` and throws an error otherwise
Applied to files:
apps/desktop/src/lib/trpc/routers/auth/index.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
Applied to files:
apps/web/src/app/auth/desktop/success/page.tsx
🧬 Code graph analysis (30)
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsx (1)
packages/ui/src/components/ui/avatar.tsx (1)
AvatarImage(53-53)
packages/trpc/src/router/integration/linear/linear.ts (1)
packages/trpc/src/router/integration/linear/utils.ts (2)
verifyOrgMembership(55-71)verifyOrgAdmin(73-81)
apps/web/src/app/layout.tsx (4)
packages/ui/src/lib/utils.ts (1)
cn(4-6)apps/admin/src/app/providers.tsx (1)
Providers(13-31)apps/web/src/app/providers.tsx (1)
Providers(13-31)packages/ui/src/components/ui/sonner.tsx (1)
Toaster(40-40)
apps/api/src/app/api/auth/desktop/connect/route.ts (3)
packages/auth/src/env.ts (1)
env(8-27)apps/api/src/env.ts (1)
env(4-47)packages/auth/src/index.ts (1)
auth(9-71)
packages/auth/src/index.ts (2)
packages/auth/src/env.ts (1)
env(8-27)packages/db/src/schema/auth.ts (1)
authSchema(11-11)
apps/admin/src/proxy.ts (4)
apps/web/src/proxy.ts (1)
proxy(18-37)packages/auth/src/index.ts (1)
auth(9-71)apps/admin/src/env.ts (1)
env(5-44)packages/shared/src/constants.ts (1)
COMPANY(12-24)
packages/auth/src/env.ts (4)
apps/api/src/proxy.ts (1)
config(46-53)packages/db/src/env.ts (1)
env(9-23)apps/api/src/env.ts (1)
env(4-47)apps/marketing/src/env.ts (1)
env(5-36)
apps/desktop/src/lib/trpc/routers/index.ts (1)
apps/desktop/src/lib/trpc/routers/auth/index.ts (1)
createAuthRouter(11-87)
apps/api/src/app/api/auth/[...all]/route.ts (1)
packages/auth/src/index.ts (1)
auth(9-71)
apps/desktop/src/lib/trpc/routers/auth/index.ts (2)
apps/desktop/src/main/lib/auth/auth.ts (1)
authService(110-110)apps/desktop/src/main/lib/auth/index.ts (1)
authService(2-2)
packages/trpc/src/router/integration/integration.ts (1)
packages/db/src/schema/auth.ts (1)
members(99-116)
apps/web/src/proxy.ts (3)
apps/admin/src/proxy.ts (1)
proxy(17-37)apps/api/src/proxy.ts (1)
proxy(22-44)packages/auth/src/index.ts (1)
auth(9-71)
apps/web/src/app/(auth)/sign-up/[[...sign-up]]/page.tsx (2)
packages/auth/src/client.ts (1)
authClient(6-9)apps/web/src/env.ts (1)
env(5-46)
apps/admin/src/components/PostHogUserIdentifier/PostHogUserIdentifier.tsx (2)
apps/web/src/components/PostHogUserIdentifier/PostHogUserIdentifier.tsx (1)
PostHogUserIdentifier(7-22)packages/auth/src/client.ts (1)
authClient(6-9)
apps/web/src/app/(auth)/layout.tsx (1)
packages/auth/src/index.ts (1)
auth(9-71)
apps/desktop/src/main/lib/auth/auth.ts (3)
apps/desktop/src/main/lib/app-environment.ts (1)
SUPERSET_HOME_DIR(5-5)packages/shared/src/constants.ts (1)
PROTOCOL_SCHEMES(6-9)apps/desktop/src/main/lib/auth/crypto-storage.ts (2)
decrypt(97-119)encrypt(76-90)
apps/api/src/app/api/electric/[...path]/route.ts (3)
apps/api/src/app/api/integrations/linear/connect/route.ts (1)
GET(7-53)apps/api/src/lib/auth.ts (1)
getSession(3-7)packages/db/src/schema/auth.ts (1)
members(99-116)
packages/trpc/src/trpc.ts (2)
packages/auth/src/index.ts (1)
Session(73-73)packages/shared/src/constants.ts (1)
COMPANY(12-24)
apps/web/src/app/(auth)/sign-in/[[...sign-in]]/page.tsx (2)
packages/auth/src/client.ts (1)
authClient(6-9)apps/web/src/env.ts (1)
env(5-46)
apps/api/src/trpc/context.ts (3)
apps/api/src/lib/auth.ts (1)
getSession(3-7)packages/trpc/src/trpc.ts (1)
createTRPCContext(11-11)packages/trpc/src/index.ts (1)
createTRPCContext(9-9)
apps/api/src/lib/auth.ts (1)
packages/auth/src/index.ts (1)
auth(9-71)
apps/desktop/src/main/index.ts (2)
apps/desktop/src/main/lib/auth/auth.ts (2)
parseAuthDeepLink(20-39)authService(110-110)apps/desktop/src/main/lib/auth/index.ts (2)
parseAuthDeepLink(2-2)authService(2-2)
apps/marketing/src/app/components/CTAButtons/CTAButtons.tsx (1)
packages/auth/src/index.ts (1)
auth(9-71)
packages/trpc/src/router/integration/linear/utils.ts (1)
packages/db/src/schema/auth.ts (1)
members(99-116)
packages/db/src/schema/relations.ts (2)
packages/db/src/schema/auth.ts (6)
users(13-24)sessions(26-44)accounts(46-68)members(99-116)invitations(118-138)organizations(86-97)packages/db/src/schema/schema.ts (3)
tasks(58-129)integrationConnections(135-171)repositories(29-53)
apps/web/src/app/auth/desktop/success/page.tsx (2)
packages/auth/src/index.ts (1)
auth(9-71)apps/web/src/app/auth/desktop/success/components/DesktopRedirect/DesktopRedirect.tsx (1)
DesktopRedirect(7-26)
packages/trpc/src/router/admin/admin.ts (2)
packages/trpc/src/router/admin/index.ts (1)
adminRouter(1-1)packages/trpc/src/trpc.ts (1)
adminProcedure(44-53)
packages/trpc/src/router/user/user.ts (2)
packages/trpc/src/trpc.ts (1)
protectedProcedure(33-42)packages/db/src/schema/auth.ts (1)
members(99-116)
apps/api/src/proxy.ts (2)
apps/admin/src/proxy.ts (1)
proxy(17-37)apps/web/src/proxy.ts (1)
proxy(18-37)
packages/trpc/src/router/organization/organization.ts (1)
packages/db/src/schema/auth.ts (1)
members(99-116)
⏰ 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). (5)
- GitHub Check: Deploy API
- GitHub Check: Deploy Web
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Docs
- GitHub Check: Build
| if (!redirectUrl) { | ||
| return new Response(`Failed to initiate OAuth: ${JSON.stringify(result)}`, { | ||
| status: 500, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Avoid leaking internal details in error responses.
JSON.stringify(result) may expose internal auth library state or headers to clients. Use a generic error message instead.
🔎 Proposed fix
if (!redirectUrl) {
- return new Response(`Failed to initiate OAuth: ${JSON.stringify(result)}`, {
+ return new Response("Failed to initiate OAuth flow", {
status: 500,
});
}🤖 Prompt for AI Agents
In apps/api/src/app/api/auth/desktop/connect/route.ts around lines 29 to 33, the
error response currently returns JSON.stringify(result) which can leak internal
auth library state; change the response body to a generic message like "Failed
to initiate OAuth" and return the 500 status, and move any detailed information
about result into a server-side log (e.g., processLogger.error or console.error)
instead of sending it to the client.
| const signUpWithGoogle = async () => { | ||
| if (!isLoaded) return; | ||
|
|
||
| setIsLoadingGoogle(true); | ||
| setError(null); | ||
|
|
||
| try { | ||
| await signUp.authenticateWithRedirect({ | ||
| strategy: "oauth_google", | ||
| redirectUrl: "/sso-callback", | ||
| redirectUrlComplete: "/", | ||
| await authClient.signIn.social({ | ||
| provider: "google", | ||
| callbackURL: env.NEXT_PUBLIC_WEB_URL, | ||
| }); | ||
| } catch (err) { | ||
| console.error("Sign up failed:", err); | ||
| setError("Failed to sign up. Please try again."); | ||
| } finally { | ||
| setIsLoadingGoogle(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the auth client implementation
rg -n "signIn.*social" --type ts --type tsx -A 10Repository: superset-sh/superset
Length of output: 184
🏁 Script executed:
# Also check the page.tsx file to understand the full context and check for type safety issues
head -50 apps/web/src/app/\(auth\)/sign-up/\[\[...\]\]/page.tsx 2>/dev/null || find . -path "**/sign-up/**/page.tsx" -type fRepository: superset-sh/superset
Length of output: 121
🏁 Script executed:
# Search for authClient declaration and type
rg -n "authClient\s*=" -B 5 -A 2 apps/web/src/app/\(auth\)/sign-up/Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
cat -n ./apps/web/src/app/\(auth\)/sign-up/\[\[...\]\]/page.tsx | head -60Repository: superset-sh/superset
Length of output: 148
🏁 Script executed:
# Search for authClient import and definition
rg -n "authClient|import.*auth" ./apps/web/src/app/\(auth\)/sign-up/\[\[...\]\]/page.tsxRepository: superset-sh/superset
Length of output: 154
🏁 Script executed:
# Find auth client implementation in the codebase
find ./packages -name "*.ts" -o -name "*.tsx" | head -20 | xargs rg -l "signIn.*social" 2>/dev/null | head -5Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Use git ls-files to find the sign-up page
git ls-files | grep -i "sign-up.*page.tsx"Repository: superset-sh/superset
Length of output: 119
🏁 Script executed:
# Use fd to find the file by pattern
fd "page.tsx" --path "*sign-up*"Repository: superset-sh/superset
Length of output: 279
🏁 Script executed:
# Check if the repository structure matches expectations
ls -la apps/web/src/app/ 2>/dev/null | head -20Repository: superset-sh/superset
Length of output: 667
🏁 Script executed:
# Use Python to read the file with proper path handling
python3 << 'EOF'
import os
file_path = "apps/web/src/app/(auth)/sign-up/[[...sign-up]]/page.tsx"
try:
with open(file_path, 'r') as f:
lines = f.readlines()
for i, line in enumerate(lines[:80], 1):
print(f"{i:3}: {line}", end='')
except Exception as e:
print(f"Error: {e}")
EOFRepository: superset-sh/superset
Length of output: 150
🏁 Script executed:
# Search for authClient definition and setup
rg "authClient" apps/web/src -B 2 -A 2 --max-count 10Repository: superset-sh/superset
Length of output: 4746
🏁 Script executed:
# Look for type definitions and any types in the file
rg "any" apps/web/src/app/ --type ts --type tsxRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Find the auth client implementation
find . -path "*/auth/*" -name "client*" -type fRepository: superset-sh/superset
Length of output: 93
🏁 Script executed:
# Look for the auth package structure
ls -la packages/auth/ 2>/dev/null || find . -path "*/packages/auth*" -type dRepository: superset-sh/superset
Length of output: 347
🏁 Script executed:
# Search for signIn definition and social method
rg "signIn.*=" packages/ -A 5 --max-count 5Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check for any types in the entire codebase
rg ": any" apps/web/src/app/\(auth\)/ --type ts -A 1Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Read the auth client implementation
cat -n ./packages/auth/src/client.tsRepository: superset-sh/superset
Length of output: 387
🏁 Script executed:
# Check the auth package src directory
ls -la ./packages/auth/src/Repository: superset-sh/superset
Length of output: 348
🏁 Script executed:
# Search for type definitions in auth package
find ./packages/auth -name "*.ts" -o -name "*.d.ts" | xargs catRepository: superset-sh/superset
Length of output: 2710
🏁 Script executed:
# Read the sign-up page file content
python3 << 'EOF'
import os
file_path = "apps/web/src/app/(auth)/sign-up/[[...sign-up]]/page.tsx"
try:
with open(file_path, 'r') as f:
content = f.read()
lines = content.split('\n')
for i, line in enumerate(lines[:80], 1):
print(f"{i:3}: {line}")
except Exception as e:
print(f"Error: {e}")
EOFRepository: superset-sh/superset
Length of output: 2699
🏁 Script executed:
# Look for better-auth types and signIn method
rg "better-auth" packages/ --type ts -A 3 | head -50Repository: superset-sh/superset
Length of output: 772
🏁 Script executed:
# Search for any unused type annotations or error handling patterns
rg "catch.*err|any\b" apps/web/src/app/\(auth\)/ -B 2 -A 2Repository: superset-sh/superset
Length of output: 1840
🏁 Script executed:
# Check if there's an eslint config for catch clause typing
find . -name "eslint*" -o -name ".eslintrc*" | head -5Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check tsconfig for strict settings
cat ./apps/web/tsconfig.jsonRepository: superset-sh/superset
Length of output: 285
🏁 Script executed:
# Check the extended TypeScript config
find . -path "*typescript*" -name "*.json" | head -10Repository: superset-sh/superset
Length of output: 237
🏁 Script executed:
# Look at the base TypeScript config
cat ./packages/typescript/next.jsonRepository: superset-sh/superset
Length of output: 127
🏁 Script executed:
# Read the base TypeScript config
cat ./tooling/typescript/next.jsonRepository: superset-sh/superset
Length of output: 271
🏁 Script executed:
# Also check the base config
cat ./tooling/typescript/base.jsonRepository: superset-sh/superset
Length of output: 685
Add type annotation to the error parameter in catch block.
The error parameter err (line 25) lacks a type annotation, violating TypeScript strict mode and the type safety requirement. Under the strict configuration, use catch (err: unknown) to properly type the error, or narrow it to a more specific type like catch (err: Error) if you need to access error properties.
Note: The loading state behavior is correct—it remains true on successful OAuth redirect since the browser will navigate away; only reset it on error.
🤖 Prompt for AI Agents
In apps/web/src/app/(auth)/sign-up/[[...sign-up]]/page.tsx around lines 16 to
30, the catch block uses an untyped error parameter `err` which fails TypeScript
strict-mode; change the signature to `catch (err: unknown)` (or `catch (err:
Error)` if you only expect Error instances) and update the error
handling/logging to narrow the type before accessing properties (e.g., use `if
(err instanceof Error) console.error("Sign up failed:", err.message, err)` else
console.error("Sign up failed:", err) ), then keep setting the user-facing error
and resetting isLoadingGoogle as before.
| */ | ||
| emptyStringAsUndefined: true, | ||
| skipValidation: !!process.env.SKIP_ENV_VALIDATION, | ||
| skipValidation: true, |
There was a problem hiding this comment.
Unconditionally disabling environment validation removes critical safety checks.
Hardcoding skipValidation: true bypasses all zod schema validation for DATABASE_URL and DATABASE_URL_UNPOOLED. Missing or malformed database URLs will no longer be caught at startup, potentially causing runtime failures when database connections are attempted.
🔎 Proposed fix: restore conditional validation
- skipValidation: true,
+ skipValidation: !!process.env.SKIP_ENV_VALIDATION,This allows validation to be skipped only when explicitly needed (e.g., during certain build processes) while maintaining safety checks in production and development.
📝 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.
| skipValidation: true, | |
| skipValidation: !!process.env.SKIP_ENV_VALIDATION, |
🤖 Prompt for AI Agents
In packages/db/src/env.ts around line 22, remove the unconditional
skipValidation: true and instead make skipValidation conditional (for example:
skipValidation: process.env.SKIP_ENV_VALIDATION === 'true' or only when NODE_ENV
=== 'test' or a dedicated build flag); this preserves zod validation for
DATABASE_URL and DATABASE_URL_UNPOOLED in normal dev/prod runs while allowing
callers to explicitly opt out when required.
| export const sessions = authSchema.table( | ||
| "sessions", | ||
| { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| expiresAt: timestamp("expires_at").notNull(), | ||
| token: text("token").notNull().unique(), | ||
| createdAt: timestamp("created_at").defaultNow().notNull(), | ||
| updatedAt: timestamp("updated_at") | ||
| .$onUpdate(() => new Date()) | ||
| .notNull(), | ||
| ipAddress: text("ip_address"), | ||
| userAgent: text("user_agent"), | ||
| userId: uuid("user_id") | ||
| .notNull() | ||
| .references(() => users.id, { onDelete: "cascade" }), | ||
| activeOrganizationId: uuid("active_organization_id"), | ||
| }, | ||
| (table) => [index("sessions_user_id_idx").on(table.userId)], | ||
| ); |
There was a problem hiding this comment.
Add foreign key constraint for activeOrganizationId.
Line 41 defines activeOrganizationId but doesn't add a foreign key reference to organizations.id. This creates referential integrity risk - sessions could reference non-existent organizations.
🔎 Proposed fix
activeOrganizationId: uuid("active_organization_id")
+ .references(() => organizations.id, { onDelete: "set null" }),This aligns with the same issue found in the migration SQL file (packages/db/drizzle/0006_add_better_auth_tables.sql).
📝 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.
| export const sessions = authSchema.table( | |
| "sessions", | |
| { | |
| id: uuid("id").primaryKey().defaultRandom(), | |
| expiresAt: timestamp("expires_at").notNull(), | |
| token: text("token").notNull().unique(), | |
| createdAt: timestamp("created_at").defaultNow().notNull(), | |
| updatedAt: timestamp("updated_at") | |
| .$onUpdate(() => new Date()) | |
| .notNull(), | |
| ipAddress: text("ip_address"), | |
| userAgent: text("user_agent"), | |
| userId: uuid("user_id") | |
| .notNull() | |
| .references(() => users.id, { onDelete: "cascade" }), | |
| activeOrganizationId: uuid("active_organization_id"), | |
| }, | |
| (table) => [index("sessions_user_id_idx").on(table.userId)], | |
| ); | |
| export const sessions = authSchema.table( | |
| "sessions", | |
| { | |
| id: uuid("id").primaryKey().defaultRandom(), | |
| expiresAt: timestamp("expires_at").notNull(), | |
| token: text("token").notNull().unique(), | |
| createdAt: timestamp("created_at").defaultNow().notNull(), | |
| updatedAt: timestamp("updated_at") | |
| .$onUpdate(() => new Date()) | |
| .notNull(), | |
| ipAddress: text("ip_address"), | |
| userAgent: text("user_agent"), | |
| userId: uuid("user_id") | |
| .notNull() | |
| .references(() => users.id, { onDelete: "cascade" }), | |
| activeOrganizationId: uuid("active_organization_id") | |
| .references(() => organizations.id, { onDelete: "set null" }), | |
| }, | |
| (table) => [index("sessions_user_id_idx").on(table.userId)], | |
| ); |
🤖 Prompt for AI Agents
In packages/db/src/schema/auth.ts around lines 26 to 44, the
sessions.activeOrganizationId column is defined but lacks a foreign key
reference to organizations.id; update the column definition to add
.references(() => organizations.id, { onDelete: "SET NULL" }) (or another
desired delete rule like "CASCADE") so sessions cannot reference non-existent
organizations and ensure this change matches the migration SQL in
packages/db/drizzle/0006_add_better_auth_tables.sql.
Code reviewFound 2 issues:
superset/packages/trpc/src/router/admin/admin.ts Lines 15 to 22 in fff4289
superset/apps/desktop/src/main/lib/auth/auth.ts Lines 38 to 55 in fff4289 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/trpc/src/router/organization/organization.ts (3)
9-48: Missing soft-delete filtering in organization queries.Based on the PR objectives, the reviewer (saddlepaddle) flagged the loss of soft-delete functionality, noting that "the PR removes deletedAt filtering from organization queries." The
all,byId, andbySlugqueries now return all organizations without filtering out soft-deleted records.If soft-delete semantics need to be preserved (as requested by the reviewer), these queries should filter by
deletedAt IS NULLor equivalent. However, the current auth schema (from relevant code snippets) shows that theorganizationstable lacks adeletedAtcolumn entirely.Based on PR objectives indicating reviewer concerns about soft-delete functionality.
79-95: Restore UUID validation for organization ID.Line 82 weakens type safety by changing the
idvalidation fromz.string().uuid()toz.string(). The organization ID is a UUID field in the database schema (as confirmed by the relevant code snippets showingid: uuid("id")), and the input validation should enforce this constraint.🔎 Restore UUID validation
update: protectedProcedure .input( z.object({ - id: z.string(), + id: z.string().uuid(), name: z.string().min(1).optional(), logo: z.string().url().optional(), }), )As per coding guidelines: Use type safety and avoid weakening type constraints in TypeScript files.
97-102: Hard delete prevents organization recovery.Based on the PR objectives, the reviewer (saddlepaddle) flagged that "the PR converts user deletion from soft delete to hard delete" and noted that "previously there was a restoreUser mutation to recover deleted users; hard deletes prevent recovery and may conflict with compliance or user-requested restores."
The current implementation performs a hard delete on the
organizationstable, which:
- Permanently removes the organization and cascades to delete all associated members (due to foreign key constraints)
- Prevents any possibility of recovery or restoration
- May conflict with compliance requirements or audit trails
If soft-delete semantics need to be preserved (as requested by the reviewer), consider adding a
deletedAtcolumn to the schema and implementing soft-delete logic.Based on PR objectives indicating reviewer concerns about soft-delete functionality.
♻️ Duplicate comments (2)
apps/api/src/app/api/auth/desktop/connect/route.ts (2)
6-13: Past issue not addressed: Provider validation is still missing.The unsafe type assertion on line 8 remains unfixed. Without explicit validation that
provideris either"google"or"github", malicious requests can pass arbitrary strings tosignInSocial, potentially causing security issues or unexpected behavior.🔎 Recommended fix (as previously suggested)
export async function GET(request: Request) { const url = new URL(request.url); - const provider = url.searchParams.get("provider") as "google" | "github"; + const provider = url.searchParams.get("provider"); const state = url.searchParams.get("state"); - if (!provider || !state) { + if (!provider || !state) { return new Response("Missing provider or state", { status: 400 }); } + + if (provider !== "google" && provider !== "github") { + return new Response("Invalid provider", { status: 400 }); + }
29-33: Past issue not addressed: Error response still leaks internal details.Line 30 still uses
JSON.stringify(body)to expose internal auth library state to clients. This should be replaced with a generic error message, with detailed information logged server-side instead.🔎 Recommended fix (as previously suggested)
if (!body.url) { - return new Response(`Failed to initiate OAuth: ${JSON.stringify(body)}`, { + console.error("OAuth initiation failed:", body); + return new Response("Failed to initiate OAuth flow", { status: 500, }); }
🧹 Nitpick comments (6)
apps/api/src/app/api/auth/desktop/connect/route.ts (1)
18-27: Add error handling for the auth API call.The
auth.api.signInSocialcall (lines 18-24) lacks try-catch error handling. If the auth library throws an exception (network error, configuration issue, etc.), the entire route will crash with an unhandled error instead of returning a proper error response.🔎 Proposed fix
+ let result; + try { - const result = await auth.api.signInSocial({ + result = await auth.api.signInSocial({ body: { provider, callbackURL: successUrl.toString(), }, asResponse: true, }); + } catch (error) { + console.error("Failed to initiate OAuth:", error); + return new Response("Failed to initiate OAuth flow", { status: 500 }); + } const cookies = result.headers.get("set-cookie");packages/trpc/src/router/organization/organization.ts (1)
58-77: Consider removing unnecessary conditional check.The conditional check on Line 68 is redundant. The
.returning()method will either return the inserted record or the.insert()operation will throw an error. Theorganizationvalue cannot be undefined/null at this point without an exception being raised first.🔎 Suggested simplification
.mutation(async ({ ctx, input }) => { const [organization] = await db .insert(organizations) .values({ name: input.name, slug: input.slug, logo: input.logo, }) .returning(); - if (organization) { - await db.insert(members).values({ - organizationId: organization.id, - userId: ctx.session.user.id, - role: "owner", - }); - } + await db.insert(members).values({ + organizationId: organization.id, + userId: ctx.session.user.id, + role: "owner", + }); return organization; }),apps/api/src/proxy.ts (1)
5-9: Type inference produces a mixed array before filtering.The conditional expression on line 8 produces
string | false, soallowedOriginsis typed as(string | false)[]before the.filter(Boolean)call. While this works at runtime, TypeScript's type narrowing withfilter(Boolean)doesn't automatically narrow tostring[].Consider adding explicit type assertion for clarity:
Suggested improvement
const allowedOrigins = [ env.NEXT_PUBLIC_WEB_URL, env.NEXT_PUBLIC_ADMIN_URL, env.NODE_ENV === "development" && "http://localhost:5927", -].filter(Boolean); +].filter(Boolean) as string[];apps/web/src/proxy.ts (1)
23-28: Minor redundancy in route checking.The check
pathname.startsWith("/sign-in") || pathname.startsWith("/sign-up")duplicates logic thatisPublicRoutealready handles. Consider reusing the helper for consistency:Suggested simplification
if ( session && - (pathname.startsWith("/sign-in") || pathname.startsWith("/sign-up")) + (pathname.startsWith("/sign-in") || pathname.startsWith("/sign-up")) ) { return NextResponse.redirect(new URL("/", req.url)); }Alternatively, extract sign-in/sign-up paths into a constant shared with
publicRoutesto avoid drift.apps/desktop/src/renderer/lib/pglite/migrations/0001_auth_schema_migration.sql (1)
3-9: Consider adding foreign key constraints for referential integrity.The
memberstable referencesorganization_idanduser_idbut doesn't define foreign key constraints toauth.organizationsandauth.users. This could allow orphaned records and referential integrity violations.🔎 Suggested foreign key additions
CREATE TABLE "auth"."members" ( "id" uuid PRIMARY KEY NOT NULL, "organization_id" uuid NOT NULL, "user_id" uuid NOT NULL, "role" text NOT NULL, - "created_at" timestamp NOT NULL + "created_at" timestamp NOT NULL, + CONSTRAINT "members_organization_fk" FOREIGN KEY ("organization_id") REFERENCES "auth"."organizations"("id") ON DELETE CASCADE, + CONSTRAINT "members_user_fk" FOREIGN KEY ("user_id") REFERENCES "auth"."users"("id") ON DELETE CASCADE );apps/desktop/src/renderer/lib/pglite/schema/auth.ts (1)
42-55: Add foreign key constraints for data integrity.The
memberstable hasorganization_idanduser_idfields but no foreign key constraints. While Drizzle will work without them, adding constraints prevents orphaned records and ensures referential integrity, especially important for synced data.🔎 Recommended foreign key additions
export const members = authSchema.table( "members", { id: uuid("id").primaryKey(), - organization_id: uuid("organization_id").notNull(), - user_id: uuid("user_id").notNull(), + organization_id: uuid("organization_id") + .notNull() + .references(() => organizations.id, { onDelete: "cascade" }), + user_id: uuid("user_id") + .notNull() + .references(() => users.id, { onDelete: "cascade" }), role: text("role").notNull(), created_at: timestamp("created_at").notNull(), },Note: Adding foreign keys requires regenerating the migration with
drizzle-kit generate.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
apps/admin/src/proxy.tsapps/api/src/app/api/auth/desktop/connect/route.tsapps/api/src/app/api/electric/[...path]/utils.tsapps/api/src/proxy.tsapps/desktop/drizzle.config.tsapps/desktop/src/main/lib/api-client.tsapps/desktop/src/main/lib/auth/auth.tsapps/desktop/src/renderer/lib/pglite/migrations/0001_auth_schema_migration.sqlapps/desktop/src/renderer/lib/pglite/migrations/index.tsapps/desktop/src/renderer/lib/pglite/migrations/meta/0001_snapshot.jsonapps/desktop/src/renderer/lib/pglite/migrations/meta/_journal.jsonapps/desktop/src/renderer/lib/pglite/schema/auth.tsapps/desktop/src/renderer/lib/pglite/schema/index.tsapps/desktop/src/renderer/lib/pglite/schema/schema.tsapps/desktop/src/renderer/lib/pglite/sync.tsapps/marketing/src/proxy.tsapps/web/src/app/auth/desktop/success/page.tsxapps/web/src/proxy.tspackages/db/drizzle/0007_add_created_at_default.sqlpackages/db/drizzle/meta/0007_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/auth.tspackages/trpc/src/router/organization/organization.ts
💤 Files with no reviewable changes (1)
- apps/marketing/src/proxy.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/lib/pglite/migrations/meta/0001_snapshot.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/db/src/schema/auth.ts
- apps/desktop/src/main/lib/auth/auth.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/renderer/lib/pglite/schema/schema.tsapps/desktop/drizzle.config.tsapps/desktop/src/renderer/lib/pglite/schema/index.tsapps/desktop/src/renderer/lib/pglite/sync.tsapps/desktop/src/renderer/lib/pglite/migrations/index.tsapps/desktop/src/renderer/lib/pglite/schema/auth.tsapps/desktop/src/main/lib/api-client.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type safety and avoid
anytypes unless absolutely necessary in TypeScript files
Files:
apps/desktop/src/renderer/lib/pglite/schema/schema.tsapps/desktop/drizzle.config.tsapps/desktop/src/renderer/lib/pglite/schema/index.tsapps/api/src/app/api/auth/desktop/connect/route.tsapps/desktop/src/renderer/lib/pglite/sync.tsapps/api/src/app/api/electric/[...path]/utils.tsapps/web/src/app/auth/desktop/success/page.tsxapps/desktop/src/renderer/lib/pglite/migrations/index.tsapps/api/src/proxy.tsapps/desktop/src/renderer/lib/pglite/schema/auth.tspackages/trpc/src/router/organization/organization.tsapps/admin/src/proxy.tsapps/desktop/src/main/lib/api-client.tsapps/web/src/proxy.ts
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Move Node.js functionality needed in Electron renderer to
src/main/lib/and communicate via IPC
Files:
apps/desktop/src/renderer/lib/pglite/schema/schema.tsapps/desktop/src/renderer/lib/pglite/schema/index.tsapps/desktop/src/renderer/lib/pglite/sync.tsapps/desktop/src/renderer/lib/pglite/migrations/index.tsapps/desktop/src/renderer/lib/pglite/schema/auth.tsapps/desktop/src/main/lib/api-client.ts
🧠 Learnings (4)
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to packages/db/src/schema/**/*.ts : Create database migrations by changing Drizzle schema and running `drizzle-kit generate --name="<sample_name_snake_case>"` on a new Neon branch
Applied to files:
apps/desktop/src/renderer/lib/pglite/schema/schema.tsapps/desktop/drizzle.config.tspackages/db/drizzle/meta/0007_snapshot.jsonapps/desktop/src/renderer/lib/pglite/migrations/index.tsapps/desktop/src/renderer/lib/pglite/schema/auth.ts
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to packages/db/**/*.ts : Use Drizzle ORM for all database operations in the `packages/db` package
Applied to files:
apps/desktop/src/renderer/lib/pglite/schema/schema.tsapps/desktop/drizzle.config.tsapps/api/src/app/api/electric/[...path]/utils.tsapps/desktop/src/renderer/lib/pglite/schema/auth.tspackages/trpc/src/router/organization/organization.ts
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/desktop/src/**/*.{ts,tsx} : Move Node.js functionality needed in Electron renderer to `src/main/lib/` and communicate via IPC
Applied to files:
apps/web/src/app/auth/desktop/success/page.tsxapps/admin/src/proxy.ts
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/**/src/(middleware|proxy).ts : In Next.js 16, never create `middleware.ts`. Always use `proxy.ts` instead for request interception
Applied to files:
apps/api/src/proxy.tsapps/admin/src/proxy.tsapps/web/src/proxy.ts
🧬 Code graph analysis (8)
apps/api/src/app/api/auth/desktop/connect/route.ts (3)
apps/api/src/env.ts (1)
env(4-47)packages/auth/src/env.ts (1)
env(8-27)packages/auth/src/index.ts (1)
auth(9-71)
apps/api/src/app/api/electric/[...path]/utils.ts (2)
apps/desktop/src/renderer/lib/pglite/schema/auth.ts (2)
members(42-55)organizations(26-37)packages/db/src/schema/auth.ts (2)
members(99-116)organizations(86-97)
apps/web/src/app/auth/desktop/success/page.tsx (4)
packages/auth/src/index.ts (1)
auth(9-71)apps/desktop/src/main/lib/api-client.ts (1)
headers(19-27)apps/web/src/app/auth/desktop/success/components/DesktopRedirect/DesktopRedirect.tsx (1)
DesktopRedirect(7-26)apps/web/src/app/auth/desktop/success/components/DesktopRedirect/index.ts (1)
DesktopRedirect(1-1)
apps/api/src/proxy.ts (2)
apps/admin/src/proxy.ts (2)
proxy(9-23)config(25-31)apps/web/src/proxy.ts (2)
proxy(16-35)config(37-43)
packages/trpc/src/router/organization/organization.ts (2)
apps/desktop/src/renderer/lib/pglite/schema/auth.ts (2)
organizations(26-37)members(42-55)packages/db/src/schema/auth.ts (2)
organizations(86-97)members(99-116)
apps/admin/src/proxy.ts (3)
apps/web/src/proxy.ts (1)
proxy(16-35)apps/desktop/src/main/lib/api-client.ts (1)
headers(19-27)packages/shared/src/constants.ts (1)
COMPANY(12-24)
apps/desktop/src/main/lib/api-client.ts (2)
apps/desktop/src/main/lib/auth/auth.ts (1)
authService(148-148)apps/desktop/src/main/lib/auth/index.ts (1)
authService(2-2)
apps/web/src/proxy.ts (4)
apps/admin/src/proxy.ts (2)
proxy(9-23)config(25-31)apps/api/src/proxy.ts (2)
proxy(22-37)config(39-46)packages/auth/src/index.ts (1)
auth(9-71)apps/desktop/src/main/lib/api-client.ts (1)
headers(19-27)
⏰ 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 API
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Web
- GitHub Check: Build
🔇 Additional comments (32)
apps/desktop/drizzle.config.ts (1)
4-7: LGTM! Clean separation of auth schema.The migration from a single schema file to an array supporting both
auth.tsandschema.tsaligns well with the PR's Better Auth migration objectives. This separation improves modularity and maintainability by keeping authentication-related tables isolated from the main schema.apps/api/src/app/api/electric/[...path]/utils.ts (4)
7-11: LGTM!The
AllowedTabletype correctly reflects the newauth.*schema naming convention introduced with the Better Auth migration. The type is well-defined with proper string literals.
18-28: LGTM!The
buildhelper function is well-typed and its logic is preserved from the previous implementation. The approach of extracting the WHERE fragment via regex is functional for the expected SQL format.
38-42: LGTM!The case labels correctly use the new
auth.*prefixed table names, and the column references (members.organizationId,organizations.id) align with the schema definitions inpackages/db/src/schema/auth.ts.
44-62: LGTM!The users case correctly:
- Queries
db.query.members(renamed fromorganizationMembers)- Uses the correct camelCase column reference
userIdthat matches the server schema- Deduplicates user IDs using
Set- Builds an
INclause for filtering users belonging to the organizationThe logic is sound and the migration to the new
auth.userstable is properly handled.packages/db/drizzle/0007_add_created_at_default.sql (1)
1-2: LGTM! Well-structured migration adding timestamp defaults.This migration correctly adds
DEFAULT now()to thecreated_atcolumns forauth.membersandauth.organizations, ensuring consistency with other auth tables. This is a non-breaking change that aligns with the PR's goal to rely on database-generated defaults rather than manual timestamp generation.packages/db/drizzle/meta/_journal.json (1)
39-60: LGTM! Migration journal correctly updated.The three new migration entries are properly formatted and sequentially ordered, tracking the Better Auth migration (0005-0007). The journal structure follows Drizzle conventions.
packages/db/drizzle/meta/0007_snapshot.json (1)
1-1525: LGTM! Schema snapshot correctly reflects migration 0007.The snapshot accurately captures the database state after adding
created_atdefaults toauth.membersandauth.organizations. The schema is well-structured with:
- Appropriate foreign key cascade behaviors
- Proper indexing on key columns
- Consistent use of UUIDs with database-generated defaults
- All auth tables now having
created_atdefaultsThe snapshot aligns with the Better Auth migration objectives.
apps/desktop/src/main/lib/api-client.ts (3)
7-13: Clear documentation of offline-first behavior.The documentation effectively explains the distinction between auth errors and network errors, which directly addresses the reviewer's concern about preserving offline-first functionality.
28-44: Excellent offline-first error handling implementation.This fetch override correctly distinguishes between authentication failures and network errors, directly addressing the reviewer's concern:
- 401/403 responses: Properly triggers sign-out to clear invalid/revoked tokens
- Network errors: Preserves the session by catching and re-throwing, allowing offline work to continue
The implementation properly awaits both the fetch call and the sign-out operation, avoiding race conditions.
19-27: The code is correct. ThegetAccessToken()method is synchronous and returnsstring | nulldirectly (not a Promise), as shown in the auth service implementation at line 84-89 ofapps/desktop/src/main/lib/auth/auth.ts. Removing theawaitis appropriate and does not break authentication.Likely an incorrect or invalid review comment.
apps/api/src/app/api/auth/desktop/connect/route.ts (1)
1-4: LGTM!Imports are appropriate and necessary for the desktop OAuth connect functionality.
packages/trpc/src/router/organization/organization.ts (4)
2-2: LGTM: Import aligns with new auth schema structure.The switch from
organizationMembersanduserstomembersandorganizationscorrectly reflects the new auth schema introduced in this PR.
50-57: LGTM: Input schema correctly defines organization fields.The
logofield appropriately replaces the previousavatarUrl/githubOrgfields with proper URL validation.
104-121: LGTM: Member addition correctly uses new schema.The mutation properly inserts into the
memberstable with the appropriate role ("member") and references the new auth schema structure.
123-140: LGTM: Member removal correctly uses new schema.The mutation properly deletes from the
memberstable with appropriate composite key matching usingorganizationIdanduserId.apps/api/src/proxy.ts (2)
22-37: LGTM!The synchronous proxy function correctly handles CORS preflight requests and attaches CORS headers to all responses. The API authentication is appropriately delegated to the TRPC context layer rather than being handled at the proxy level.
39-46: LGTM!The matcher configuration correctly excludes Next.js internals, static assets, and telemetry routes while ensuring the proxy runs for all API and TRPC routes. The pattern is consistent with the other proxy files in the codebase.
apps/web/src/proxy.ts (3)
16-19: LGTM!The session retrieval pattern using
auth.api.getSessionwithawait headers()is consistent with the admin proxy implementation and correctly integrates with the new Better Auth system.
37-43: LGTM!The matcher configuration is consistent with the other proxy files in the monorepo, correctly excluding internal routes and static assets.
5-14: No changes needed - the review comment is based on an incorrect assumption about the application architecture.The web app and API app are separate Next.js applications. Better Auth callbacks are handled by the API app at
apps/api/src/app/api/auth/[...all]/route.ts, not by the web app. The web app makes HTTP requests to the separate API app (viaNEXT_PUBLIC_API_URL), so Better Auth callback routes never pass through the web app's proxy middleware. The currentpublicRoutesconfiguration is correct for the web app's actual routing needs.Likely an incorrect or invalid review comment.
apps/admin/src/proxy.ts (2)
9-23: LGTM!The admin proxy correctly enforces authentication and email domain restrictions. The pattern is consistent with the web proxy, and using
COMPANY.EMAIL_DOMAINfrom shared constants ensures the domain check stays synchronized across the codebase.
25-31: LGTM!The matcher configuration is consistent with the API and web proxy files.
apps/desktop/src/renderer/lib/pglite/migrations/meta/_journal.json (1)
12-18: LGTM!The migration journal entry follows the correct drizzle-kit format with proper versioning and timestamp.
apps/desktop/src/renderer/lib/pglite/migrations/index.ts (2)
15-15: LGTM!The migration import follows the correct pattern for browser-compatible bundled migrations using Vite's
?rawsuffix.
25-25: LGTM!The migration entry is correctly added with matching timestamp from
_journal.json.apps/desktop/src/renderer/lib/pglite/schema/index.ts (1)
1-2: LGTM!The re-export correctly exposes the new auth schema module alongside existing schema exports.
apps/desktop/src/renderer/lib/pglite/schema/schema.ts (1)
31-32: LGTM!The comment update correctly reflects that auth-related tables have been moved to the
authschema, leaving only the tasks table here.apps/desktop/src/renderer/lib/pglite/schema/auth.ts (1)
1-58: Schema misalignment requires fixes across multiple areas.The desktop schema diverges significantly from the server schema:
- Critical type mismatch:
email_verifiedistexton desktop butbooleanon server (defaults tofalse)- Incomplete schema: Desktop is missing 4 tables—
sessions,accounts,verifications,invitations- Missing defaults and constraints:
- UUIDs lack
.defaultRandom()- Timestamps lack
.defaultNow()or.$onUpdate()members.rolemissing default value"member"- Index type mismatch:
organizations.slugusesindex()instead ofuniqueIndex()- Missing foreign key relationships: No cascade delete references in
members,sessions,accounts,invitations- Property naming inconsistency: Desktop uses snake_case (
email_verified,created_at) while server uses camelCase (emailVerified,createdAt)Update the desktop schema to match the server implementation exactly.
⛔ Skipped due to learnings
Learnt from: CR Repo: superset-sh/superset PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-28T01:56:39.031Z Learning: Applies to packages/db/**/*.ts : Use Drizzle ORM for all database operations in the `packages/db` packageLearnt from: CR Repo: superset-sh/superset PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-28T01:56:39.031Z Learning: Applies to packages/db/src/schema/**/*.ts : Create database migrations by changing Drizzle schema and running `drizzle-kit generate --name="<sample_name_snake_case>"` on a new Neon branchapps/web/src/app/auth/desktop/success/page.tsx (3)
6-12: LGTM! Correct Next.js 15 async searchParams pattern.The function signature properly types
searchParamsas a Promise and correctly awaits it before destructuring. This aligns with Next.js 15 conventions.
38-42: The 5-minute safety buffer is correctly implemented in the desktop app's token validation logic (apps/desktop/src/main/lib/auth/auth.ts, lines 70-75 in theisExpired()method), not in the web app's URL generation. The web app correctly passes the actualexpiresAtvalue; the desktop app applies the buffer during token validation when it receives the deep link. This is the intended design and the PR objectives are satisfied.Likely an incorrect or invalid review comment.
38-39: Add explicit type annotation for the session variable and verify the property access pattern.The
sessionvariable fromauth.api.getSession()lacks an explicit type annotation. While a null check exists on line 27, the code accessessession.session.tokenandsession.session.expiresAt, which differs from the pattern used elsewhere in the codebase (session.user.*). Ensure this property structure matches Better Auth's return type, or add explicit typing:const session: Session | null = await auth.api.getSession(...).
| if (!session?.user) { | ||
| return NextResponse.redirect(new URL(env.NEXT_PUBLIC_WEB_URL)); | ||
| } | ||
|
|
||
| const user = await db.query.users.findFirst({ | ||
| where: eq(users.clerkId, clerkId), | ||
| }); | ||
|
|
||
| if (!user?.email.endsWith(COMPANY.EMAIL_DOMAIN)) { | ||
| if (!session.user.email.endsWith(COMPANY.EMAIL_DOMAIN)) { | ||
| return NextResponse.redirect(new URL(env.NEXT_PUBLIC_WEB_URL)); | ||
| } |
There was a problem hiding this comment.
Add null check for user email.
If session.user.email is null or undefined (possible with some OAuth providers that don't return email), calling .endsWith() will throw a runtime error. Consider adding a defensive check:
Suggested fix
if (!session?.user) {
return NextResponse.redirect(new URL(env.NEXT_PUBLIC_WEB_URL));
}
- if (!session.user.email.endsWith(COMPANY.EMAIL_DOMAIN)) {
+ if (!session.user.email?.endsWith(COMPANY.EMAIL_DOMAIN)) {
return NextResponse.redirect(new URL(env.NEXT_PUBLIC_WEB_URL));
}📝 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.
| if (!session?.user) { | |
| return NextResponse.redirect(new URL(env.NEXT_PUBLIC_WEB_URL)); | |
| } | |
| const user = await db.query.users.findFirst({ | |
| where: eq(users.clerkId, clerkId), | |
| }); | |
| if (!user?.email.endsWith(COMPANY.EMAIL_DOMAIN)) { | |
| if (!session.user.email.endsWith(COMPANY.EMAIL_DOMAIN)) { | |
| return NextResponse.redirect(new URL(env.NEXT_PUBLIC_WEB_URL)); | |
| } | |
| if (!session?.user) { | |
| return NextResponse.redirect(new URL(env.NEXT_PUBLIC_WEB_URL)); | |
| } | |
| if (!session.user.email?.endsWith(COMPANY.EMAIL_DOMAIN)) { | |
| return NextResponse.redirect(new URL(env.NEXT_PUBLIC_WEB_URL)); | |
| } |
🤖 Prompt for AI Agents
In apps/admin/src/proxy.ts around lines 14 to 20, the code calls
session.user.email.endsWith(...) without ensuring email is a non-empty string;
when email is null/undefined this throws. Add a defensive check that verifies
typeof session.user.email === "string" and that it is non-empty (or use optional
chaining plus a string guard) before calling .endsWith, and if the check fails
treat the user as unauthenticated and redirect to env.NEXT_PUBLIC_WEB_URL.
apps/desktop/src/renderer/lib/pglite/migrations/0001_auth_schema_migration.sql
Outdated
Show resolved
Hide resolved
| DROP TABLE "organization_members" CASCADE;--> statement-breakpoint | ||
| DROP TABLE "organizations" CASCADE;--> statement-breakpoint | ||
| DROP TABLE "users" CASCADE;--> statement-breakpoint |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/desktop/src/renderer/lib/pglite/migrations/0001_auth_schema_migration.sqlRepository: superset-sh/superset
Length of output: 1739
🏁 Script executed:
# Check for other migration files that might contain data migration logic
ls -la apps/desktop/src/renderer/lib/pglite/migrations/Repository: superset-sh/superset
Length of output: 430
🏁 Script executed:
# Search for backup tables or references to old table names
rg "organization_members|organizations|users" apps/desktop/src/renderer/lib/pglite/migrations/ -A 2 -B 2Repository: superset-sh/superset
Length of output: 18941
🏁 Script executed:
# Search for Electric sync configuration and data persistence strategy
rg "Electric|ephemeral|rebuild|sync" apps/desktop/src/renderer/lib/pglite/ -iRepository: superset-sh/superset
Length of output: 4995
🏁 Script executed:
# Check if there's any migration or setup documentation
find apps/desktop -type f -name "*.md" -o -name "*.ts" -o -name "*.js" | xargs rg -l "auth.*schema|migration|backup" -i | head -20Repository: superset-sh/superset
Length of output: 437
🏁 Script executed:
cat -n apps/desktop/src/renderer/lib/pglite/migrations/0000_init.sql | head -100Repository: superset-sh/superset
Length of output: 4057
🏁 Script executed:
# Look for clerk field references
rg "clerk_id|clerk_org_id" apps/desktop/src/renderer/lib/pglite/ -B 2 -A 2Repository: superset-sh/superset
Length of output: 7721
🏁 Script executed:
# Check the schema definition files
cat -n apps/desktop/src/renderer/lib/pglite/schema/auth.tsRepository: superset-sh/superset
Length of output: 2307
🏁 Script executed:
# Check SYNCED_TABLES definition
cat -n apps/desktop/src/renderer/lib/pglite/sync.ts | head -50Repository: superset-sh/superset
Length of output: 1076
🏁 Script executed:
# Check server schema to understand email_verified type
find apps -name "*.ts" -o -name "*.sql" | xargs rg "email_verified" -B 2 -A 2 | head -50Repository: superset-sh/superset
Length of output: 1050
Add foreign key constraints for referential integrity in the auth schema.
The migration removes the clerk_id and clerk_org_id fields (intentional for auth restructuring) and adds new fields like email_verified and metadata. However, the new auth.members table lacks foreign key constraints linking organization_id and user_id to their respective parent tables, which can cause referential integrity issues.
Additionally, while the migration drops old tables with CASCADE, data is safely repopulated via Electric sync (configured in sync.ts to sync auth.users, auth.organizations, and auth.members from the server). For clarity, consider adding a comment to the migration explaining this dependency on Electric sync.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/lib/pglite/migrations/0001_auth_schema_migration.sql
around lines 32-34, the new auth.members table lacks foreign key constraints for
organization_id and user_id and the migration drops legacy tables with CASCADE
without documenting reliance on Electric sync; update the migration to add
foreign keys linking auth.members.organization_id -> auth.organizations(id) and
auth.members.user_id -> auth.users(id) (choose ON DELETE CASCADE or RESTRICT per
domain rules) via ALTER TABLE or in the CREATE TABLE statement, and add a short
SQL comment above the DROP TABLEs noting that data is repopulated by Electric
sync (sync.ts) so the drops are intentional.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/trpc/src/router/integration/integration.ts (1)
13-13: **UUID validation is now correctly applied using Zod 4 syntax.**The past review comment flagged relaxed validation, but the current code now usesz.uuid()— a Zod 4 top-level function where string format validators moved from methods. The syntaxz.string().uuid()becomesz.uuid().z.uuid()validates RFC 9562/4122 compliant UUIDs.This correctly validates UUIDs and addresses the prior concern.
🧹 Nitpick comments (1)
apps/admin/src/app/(dashboard)/layout.tsx (1)
38-40: Consider case-insensitive email domain check.The
endsWith()method is case-sensitive, which could reject valid emails with different casing (e.g.,[email protected]). While email domains are typically normalized to lowercase, RFC 5321 allows mixed case.🔎 Proposed case-insensitive check
- if (!session.user.email?.endsWith(COMPANY.EMAIL_DOMAIN)) { + if (!session.user.email?.toLowerCase().endsWith(COMPANY.EMAIL_DOMAIN)) { redirect(env.NEXT_PUBLIC_WEB_URL); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/admin/src/app/(dashboard)/layout.tsxapps/api/src/app/api/auth/desktop/connect/route.tsapps/web/src/app/(dashboard)/layout.tsxapps/web/src/app/auth/desktop/success/page.tsxpackages/trpc/src/router/integration/integration.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/app/api/auth/desktop/connect/route.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type safety and avoid
anytypes unless absolutely necessary in TypeScript files
Files:
apps/web/src/app/auth/desktop/success/page.tsxapps/admin/src/app/(dashboard)/layout.tsxpackages/trpc/src/router/integration/integration.tsapps/web/src/app/(dashboard)/layout.tsx
🧠 Learnings (2)
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/desktop/src/**/*.{ts,tsx} : Move Node.js functionality needed in Electron renderer to `src/main/lib/` and communicate via IPC
Applied to files:
apps/web/src/app/auth/desktop/success/page.tsx
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to packages/db/**/*.ts : Use Drizzle ORM for all database operations in the `packages/db` package
Applied to files:
packages/trpc/src/router/integration/integration.ts
🧬 Code graph analysis (2)
apps/admin/src/app/(dashboard)/layout.tsx (2)
apps/admin/src/env.ts (1)
env(5-44)packages/shared/src/constants.ts (1)
COMPANY(12-24)
apps/web/src/app/(dashboard)/layout.tsx (1)
packages/auth/src/index.ts (1)
auth(9-71)
⏰ 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 API
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Web
- GitHub Check: Build
🔇 Additional comments (10)
packages/trpc/src/router/integration/integration.ts (2)
2-2: LGTM!Import updates align with the schema migration to use the new
memberstable. Removing theusersimport is appropriate since the user ID is now obtained directly from the session context.
15-23: LGTM!The membership query correctly uses the new
memberstable with session-based authentication. Theand()andeq()constructs are properly used, and accessingctx.session.user.iddirectly from the session context is cleaner than the previous user lookup approach.apps/web/src/app/(dashboard)/layout.tsx (1)
1-3: LGTM: Imports are appropriate for authentication gate.The imports correctly bring in the Better Auth instance, Next.js headers utility, and redirect function needed for the session-based route guard.
apps/admin/src/app/(dashboard)/layout.tsx (4)
1-2: LGTM! Clean imports for authentication guard.The imports are well-organized and appropriate for implementing server-side authentication with Better Auth in Next.js 15+.
Also applies to: 17-18, 20-20
34-36: LGTM! Proper session validation with optional chaining.The check correctly handles cases where the session is missing or doesn't contain a user, redirecting unauthorized users appropriately.
42-47: LGTM! Improved error handling with redirect.The change from throwing an error to redirecting when user data is missing is a good UX improvement. This provides a cleaner user experience by redirecting to the web app instead of showing an error page.
The separate user data fetch via TRPC after session validation is appropriate, as the session likely contains minimal user data while
AppSidebarneeds the complete user profile.
30-32: This follows the established pattern used throughout the codebase. Theauth.api.getSessioncall without try-catch is consistent with other layout files and middleware (e.g., apps/web/src/app/(dashboard)/layout.tsx, apps/admin/src/proxy.ts). Unhandled errors will propagate to Next.js error boundaries, which is the standard approach here.If explicit error handling is desired, consider addressing this systematically across the codebase rather than in isolated files.
apps/web/src/app/auth/desktop/success/page.tsx (3)
1-4: LGTM!Imports are clean and appropriate for a Next.js server component with Better Auth integration.
6-22: LGTM!State parameter validation is properly implemented with good type safety and user-friendly error messaging.
40-49: LGTM!Session existence check is appropriate and handles the missing session case correctly with user-friendly messaging.
| const session = await auth.api.getSession({ | ||
| headers: await headers(), | ||
| }); | ||
|
|
||
| if (!session) { | ||
| redirect("/sign-in"); | ||
| } |
There was a problem hiding this comment.
Add error handling to prevent layout crashes.
If auth.api.getSession() throws an error (e.g., database connection failure, auth service unavailable), the entire dashboard will crash. Wrap the auth call in try-catch to gracefully handle failures.
🔎 Proposed fix with error handling
+ let session;
+ try {
- const session = await auth.api.getSession({
+ session = await auth.api.getSession({
headers: await headers(),
});
+ } catch (error) {
+ console.error("Failed to fetch session:", error);
+ redirect("/sign-in");
+ }
if (!session) {
redirect("/sign-in");
}Consider explicit type annotation for better type safety.
Adding an explicit type to the session variable improves code clarity and aligns with the TypeScript coding guidelines.
As per coding guidelines, "Use type safety and avoid any types unless absolutely necessary in TypeScript files."
🔎 Suggested type annotation
- const session = await auth.api.getSession({
+ const session: Awaited<ReturnType<typeof auth.api.getSession>> | null = await auth.api.getSession({
headers: await headers(),
});Or import and use the Session type if exported from @superset/auth:
+ import type { Session } from "@superset/auth";
// ...
- const session = await auth.api.getSession({
+ const session: Session | null = await auth.api.getSession({
headers: await headers(),
});🤖 Prompt for AI Agents
In apps/web/src/app/(dashboard)/layout.tsx around lines 15 to 21, the call to
auth.api.getSession is unguarded and can throw, causing the dashboard to crash;
wrap the await auth.api.getSession(...) in a try-catch so any thrown error is
handled (log the error and redirect to "/sign-in" or render a fallback UI) to
prevent a hard crash, and add an explicit type annotation for the session
variable (import and use the Session type from the auth package or define the
appropriate interface) so the variable is strongly typed rather than inferred.
| // Get session from Better Auth | ||
| let session; | ||
| try { | ||
| session = await auth.api.getSession({ headers: await headers() }); | ||
| } catch (error) { | ||
| console.error("Failed to get session for desktop auth:", error); | ||
| return ( | ||
| <div className="flex min-h-screen flex-col items-center justify-center bg-background p-4"> | ||
| <p className="text-xl text-muted-foreground">Authentication failed</p> | ||
| <p className="text-muted-foreground/70"> | ||
| Please try signing in again from the desktop app. | ||
| </p> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Distinguish network errors from authentication failures to preserve offline-first functionality.
While the try-catch addresses the previous review comment about error handling, it treats all errors identically and breaks the offline-first behavior. According to the PR objectives, the reviewer specifically flagged this regression: the previous implementation (PR #512) distinguished network errors from invalid tokens to preserve sessions during transient network failures.
The current implementation would clear the session on any error, including temporary network issues, preventing the desktop app from functioning offline or on flaky networks.
Impact:
- Users on unstable connections would be forced to re-authenticate repeatedly
- Offline desktop functionality is broken
- Transient errors become permanent failures
Based on PR objectives, you should:
- Distinguish error types (network vs authentication failures)
- Preserve sessions during network errors (return error state without clearing session)
- Clear sessions only on invalid/401/403 responses
- Consider adding token refresh handling for expired tokens
🔎 Guidance for error handling
The error handling should check the error type and HTTP status:
try {
session = await auth.api.getSession({ headers: await headers() });
} catch (error) {
console.error("Failed to get session for desktop auth:", error);
// Check if it's a network error vs auth failure
const isNetworkError =
error instanceof Error &&
(error.message.includes('fetch failed') ||
error.message.includes('network') ||
error.cause instanceof TypeError);
if (isNetworkError) {
// Network error - preserve session, allow retry
return (
<div className="flex min-h-screen flex-col items-center justify-center bg-background p-4">
<p className="text-xl text-muted-foreground">Network error</p>
<p className="text-muted-foreground/70">
Unable to verify session. Please check your connection and try again.
</p>
</div>
);
}
// Auth failure - clear session
return (
<div className="flex min-h-screen flex-col items-center justify-center bg-background p-4">
<p className="text-xl text-muted-foreground">Authentication failed</p>
<p className="text-muted-foreground/70">
Please try signing in again from the desktop app.
</p>
</div>
);
}Note: The exact error detection logic depends on how Better Auth reports errors. You may need to check for specific error properties or status codes.
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
apps/desktop/src/lib/trpc/routers/auth/index.ts (2)
52-87: Missing network error distinction breaks offline-first behavior.Per the PR review feedback, this subscription doesn't distinguish network errors from authentication failures. The current implementation calls
emit.error()on any exception, which could cause the renderer to clear sessions on transient network issues.The prior implementation returned a three-way result type (
"success" | "invalid" | "network_error") to preserve sessions during network errors. Consider emitting a structured result that differentiates error types:🔎 Proposed approach for network error handling
- return observable<{ accessToken: string | null }>((emit) => { + return observable<{ accessToken: string | null; error?: "network_error" | "invalid" }>((emit) => { const emitToken = async () => { try { console.log("[auth/onAccessToken] emitToken called"); const accessToken = await authService.getAccessToken(); - console.log( - "[auth/onAccessToken] EMIT accessToken:", - accessToken ? "present" : "null", - ); emit.next({ accessToken }); } catch (err) { console.error("[auth/onAccessToken] Error getting token:", err); - emit.error(err instanceof Error ? err : new Error(String(err))); + // Distinguish network errors from auth failures + const isNetworkError = err instanceof TypeError || + (err instanceof Error && err.message.includes("fetch")); + if (isNetworkError) { + // Preserve session on network errors + emit.next({ accessToken: null, error: "network_error" }); + } else { + // Only emit error (clear session) on actual auth failures + emit.error(err instanceof Error ? err : new Error(String(err))); + } } };
126-138: Sign-in/out mutations need error handling for offline-first behavior.Per the PR review feedback, the prior desktop implementation distinguished network errors from invalid tokens to preserve sessions during transient failures. Neither
signInnorsignOuthandles errors:
- signIn: If the OAuth flow fails due to network issues, the error propagates without distinction.
- signOut: If sign-out fails due to network, should it still clear the local session or preserve it?
Consider wrapping these in try/catch and returning a discriminated result type (e.g.,
"success" | "invalid" | "network_error") to maintain offline-first behavior.🔎 Proposed approach
signIn: publicProcedure .input(z.object({ provider: z.enum(AUTH_PROVIDERS) })) .mutation(async ({ input }) => { - return authService.signIn(input.provider); + try { + const result = await authService.signIn(input.provider); + return { status: "success" as const, ...result }; + } catch (err) { + console.error("[auth/signIn] Error:", err); + const isNetworkError = err instanceof TypeError || + (err instanceof Error && err.message.includes("fetch")); + return { status: isNetworkError ? "network_error" : "invalid" } as const; + } }), signOut: publicProcedure.mutation(async () => { - await authService.signOut(); - return { success: true }; + try { + await authService.signOut(); + return { status: "success" as const }; + } catch (err) { + console.error("[auth/signOut] Error:", err); + // On network error, still clear local session but inform client + return { status: "network_error" as const }; + } }),packages/trpc/src/router/organization/organization.ts (7)
9-20: Critical: Restore soft-delete filtering for organizations.This query returns all organizations without filtering by
deletedAt. According to the PR objectives and reviewer feedback, the previous implementation filtered deleted organizations from queries. Hard-deleting or exposing deleted organizations breaks the existing soft-delete semantics that allowed for recovery and compliance requirements.Based on the reviewer's comments, you should:
- Add
deletedAtcolumn filtering (e.g.,where: isNull(organizations.deletedAt))- Consider separate endpoints for listing active vs. deleted organizations
- Preserve the ability to restore deleted organizations
🔎 Verification: Check if organizations schema includes deletedAt
#!/bin/bash # Check if the organizations table schema includes deletedAt column ast-grep --pattern 'export const organizations = $$$'
22-34: Critical: Missing soft-delete filtering in byId query.This query should filter out soft-deleted organizations to maintain consistency with the previous soft-delete behavior. Without filtering, deleted organizations remain accessible.
36-48: Critical: Missing soft-delete filtering in bySlug query.Deleted organizations should be filtered out to maintain soft-delete semantics.
79-95: Critical: Missing authorization check for organization updates.Any authenticated user can update any organization without verifying ownership or admin rights. This is a security vulnerability.
The mutation should verify that
ctx.session.user.idis a member with role "owner" (or "admin" if applicable) for the organization being updated before allowing the operation.🔎 Proposed fix
.mutation(async ({ input, ctx }) => { + // Verify user is an owner of the organization + const membership = await db.query.members.findFirst({ + where: and( + eq(members.organizationId, input.id), + eq(members.userId, ctx.session.user.id), + eq(members.role, "owner"), + ), + }); + + if (!membership) { + throw new Error("Unauthorized: Only organization owners can update organizations"); + } + const { id, ...data } = input;
97-102: Critical: Hard delete removes soft-delete functionality and lacks authorization.Two critical issues:
Hard delete breaks compliance and recovery requirements: This mutation performs a hard delete (
db.delete()) instead of a soft delete. Per the PR objectives and reviewer feedback, the previous implementation used soft deletes with adeletedAtcolumn to allow recovery for compliance or user-requested restores. Hard deletion is irreversible and prevents therestoreUsermutation functionality that existed previously.Missing authorization check: Any authenticated user can delete any organization without verifying ownership or admin rights.
🔎 Proposed fix for soft delete
.mutation(async ({ input, ctx }) => { + // Verify user is an owner of the organization + const membership = await db.query.members.findFirst({ + where: and( + eq(members.organizationId, input), + eq(members.userId, ctx.session.user.id), + eq(members.role, "owner"), + ), + }); + + if (!membership) { + throw new Error("Unauthorized: Only organization owners can delete organizations"); + } + - await db.delete(organizations).where(eq(organizations.id, input)); + // Soft delete by setting deletedAt timestamp + await db + .update(organizations) + .set({ deletedAt: new Date() }) + .where(eq(organizations.id, input)); return { success: true }; }),Note: This assumes the
organizationsschema includes adeletedAtcolumn. Verify with the schema definition.
104-121: Critical: Missing authorization check for adding members.Any authenticated user can add members to any organization without verifying ownership or admin rights. This is a security vulnerability that could allow unauthorized users to gain access to organizations.
The mutation should verify that
ctx.session.user.idis a member with role "owner" (or "admin") for the organization before allowing the operation.🔎 Proposed fix
.mutation(async ({ input, ctx }) => { + // Verify user is an owner/admin of the organization + const membership = await db.query.members.findFirst({ + where: and( + eq(members.organizationId, input.organizationId), + eq(members.userId, ctx.session.user.id), + eq(members.role, "owner"), // or check for "owner" | "admin" + ), + }); + + if (!membership) { + throw new Error("Unauthorized: Only organization owners can add members"); + } + const [member] = await db
123-140: Critical: Missing authorization check for removing members.Any authenticated user can remove members from any organization without verifying ownership or admin rights. This is a severe security vulnerability that could allow unauthorized users to remove legitimate members, including owners.
The mutation should verify that
ctx.session.user.idis a member with role "owner" (or "admin") for the organization before allowing the operation. Additionally, consider preventing removal of the last owner to avoid orphaned organizations.🔎 Proposed fix
.mutation(async ({ input, ctx }) => { + // Verify user is an owner/admin of the organization + const membership = await db.query.members.findFirst({ + where: and( + eq(members.organizationId, input.organizationId), + eq(members.userId, ctx.session.user.id), + eq(members.role, "owner"), + ), + }); + + if (!membership) { + throw new Error("Unauthorized: Only organization owners can remove members"); + } + await dbpackages/trpc/src/router/user/user.ts (1)
27-36: Add soft-delete filtering for organizations.Similar to
myOrganization, this query does not filter out soft-deleted organizations. The list of organizations returned should exclude any that have been soft-deleted to maintain the soft-delete contract and prevent users from accessing deleted data.🔎 Proposed fix to add deletedAt filtering
+import { and, eq, isNull } from "drizzle-orm"; +import { organizations } from "@superset/db/schema"; + myOrganizations: protectedProcedure.query(async ({ ctx }) => { const memberships = await db.query.members.findMany({ - where: eq(members.userId, ctx.session.user.id), + where: and( + eq(members.userId, ctx.session.user.id), + isNull(organizations.deletedAt) + ), with: { organization: true, }, }); return memberships.map((m) => m.organization); }),packages/trpc/src/router/integration/integration.ts (1)
21-22: UseTRPCErrorinstead of genericErrorfor consistent error handling.Per coding guidelines, tRPC procedures should use
TRPCErrorwith appropriate error codes. A membership check failure should returnFORBIDDEN.🔎 Proposed fix
+import { TRPCError } from "@trpc/server"; // ... if (!membership) { - throw new Error("Not a member of this organization"); + throw new TRPCError({ + code: "FORBIDDEN", + message: "Not a member of this organization", + }); }Based on learnings: "Use TRPCError with appropriate error codes: NOT_FOUND, UNAUTHORIZED, FORBIDDEN, BAD_REQUEST, INTERNAL_SERVER_ERROR, NOT_IMPLEMENTED"
.github/workflows/deploy-production.yml (1)
76-109: Inconsistency: DESKTOP_AUTH_SECRET still present despite PR objectives.The PR objectives state: "Cleanup: removes Clerk dependencies, deletes DESKTOP_AUTH_SECRET and Clerk-related env vars." However,
DESKTOP_AUTH_SECRETis still present in the workflow on lines 78, 109, 171, and 182.Please clarify:
- Is
DESKTOP_AUTH_SECRETstill needed for the desktop authentication flow?- If so, the PR objectives should be updated to reflect this.
- If not, these references should be removed.
🤖 Fix all issues with AI Agents
In
@apps/admin/src/app/(dashboard)/components/AppSidebar/components/NavUser/NavUser.tsx:
- Around line 42-50: The sign-out flow in handleSignOut currently swallows
errors; wrap the authClient.signOut call in a try/catch, only perform the
redirect to env.NEXT_PUBLIC_WEB_URL on success, and on failure log the error and
surface user feedback (e.g., trigger an existing toast/notification or set local
error state) so failures aren’t silent; update the handleSignOut function and
its authClient.signOut(fetchOptions) usage to reflect this error handling.
In @apps/admin/src/proxy.ts:
- Around line 14-20: session.user.email may be null/undefined so calling
.endsWith() can throw; update the guard around the existing redirect logic in
proxy.ts to explicitly verify session.user.email is a non-empty string (e.g.,
typeof session.user.email === "string" &&
session.user.email.endsWith(COMPANY.EMAIL_DOMAIN)) and redirect to
NextResponse.redirect(new URL(env.NEXT_PUBLIC_WEB_URL)) if the email is missing
or not a string before calling .endsWith(); keep the current redirect behavior
and references to session, COMPANY.EMAIL_DOMAIN and env.NEXT_PUBLIC_WEB_URL.
In @apps/api/src/app/api/electric/[...path]/utils.ts:
- Around line 51-93: The case "auth.organizations" block incorrectly derives the
authenticated user by taking the first member's userId from members; instead,
change the function signature that contains this switch (or the caller of this
utility) to accept an explicit userId parameter and use that value when querying
members (replace usage of the derived userId and the initial members lookup).
Remove the initial members.findMany call that picks the first userId, use the
provided userId to fetch allUserMemberships via db.query.members.findMany({
where: eq(members.userId, userId) ... }), build orgIds/from whereExpr as before,
and update all call sites to pass the session-authenticated userId into this
utility.
In @apps/desktop/src/main/index.ts:
- Around line 39-49: The auth flow currently saves tokens then calls
refreshSession() which calls auth.api.getSession() directly, losing the
apiClient's 401/403 vs network error discrimination and causing
handleAuthCallback() to always return success; fix refreshSession() to call the
apiClient fetch path (or replicate its status-code logic) so 401/403 triggers
authService.signOut() and network errors do not clear session, expand the
SignInResult type returned by refreshSession()/handleAuthCallback() to express
token-saved + sessionFetch status (e.g., success, transient_network_error,
auth_failure), ensure this result is propagated to handleAuthCallback() and
change its return value so callers can distinguish a token-only success from a
failed session fetch, and ensure any invalid/expired-token paths clear persisted
tokens only on definitive auth failures (401/403) while preserving them on
network errors.
In @apps/desktop/src/renderer/contexts/CollectionsProvider/collections.ts:
- Around line 24-39: createCollections currently builds a renderer-side tRPC
client (createTRPCProxyClient + httpBatchLink) with a simple headers object but
no fetch-level error handling, which will treat transient network failures the
same as auth failures and can clear sessions; implement an error-aware fetch for
the httpBatchLink used in createCollections that mirrors the main process
api-client behavior: perform the request with a custom fetch wrapper that
distinguishes network errors (e.g., DNS/timeouts/TypeError) from auth responses
(401/403), only propagate/handle auth failures as session-invalidating, and
preserve existing session/state when a network error occurs; update the
httpBatchLink options in createCollections to pass this custom fetch and ensure
Authorization header (headers / token) is still applied.
- Around line 135-146: The organizations collection is missing the org scoping
parameter; update the createCollection call for organizations (the
electricCollectionOptions passed to createCollection) to include org:
activeOrgId inside params so the Electric shape for "auth.organizations" is
scoped correctly (i.e., add org: activeOrgId to the params object used by
electricCollectionOptions in the organizations definition).
In
@apps/desktop/src/renderer/contexts/CollectionsProvider/CollectionsProvider.tsx:
- Around line 23-43: The memoized creation of collections in CollectionsProvider
currently lives in useMemo and never disposes Electric shape subscriptions; move
the creation logic into a useEffect that depends on [token, activeOrgId], call
createCollections({ token, activeOrgId }) inside the effect, store the returned
instance in state (or a ref), and return a cleanup function that calls the
collection instance's dispose/cleanup/unsubscribe method (add such a method to
the collection factory if missing) to close HTTP streams when token or
activeOrgId change; also remove setError from inside the creation path in the
effect (set error via effect-level catch and avoid setting state synchronously
inside useMemo) so state updates don’t trigger memo re-evaluation loops.
In @apps/web/src/app/(dashboard)/components/Header/Header.tsx:
- Around line 24-27: The sign-out flow in handleSignOut currently awaits
authClient.signOut() without error handling; wrap the call in a try/catch, call
await authClient.signOut() inside the try and only call router.push("/sign-in")
on success, and in the catch log the error (e.g., console.error or app logger)
and surface user feedback (toast or inline error) so failures are not swallowed
and the user isn’t redirected when sign-out fails.
In
@apps/web/src/app/auth/desktop/success/components/DesktopRedirect/DesktopRedirect.tsx:
- Around line 18-23: The DesktopRedirect component is using Next.js Link for a
custom-protocol URL which can mis-handle superset:// style links; replace the
Link import/element usage with a plain anchor element (<a>) in DesktopRedirect,
keep the same href and className props, and ensure any import of Link from
"next/link" is removed or not used for this anchor so the custom-protocol URL is
opened correctly.
In @packages/auth/src/client.ts:
- Around line 6-9: The auth client is created with createAuthClient using
process.env.NEXT_PUBLIC_API_URL without validation; update the module to
validate NEXT_PUBLIC_API_URL before calling createAuthClient (e.g., check
process.env.NEXT_PUBLIC_API_URL and if missing throw or log a clear error), and
then pass the validated value as baseURL to createAuthClient; refer to
authClient, createAuthClient, process.env.NEXT_PUBLIC_API_URL and
organizationClient() when making the change.
In @packages/db/drizzle/0006_add_better_auth_tables.sql:
- Around line 38-46: The new auth.organizations table must include soft-delete
support: add a "deleted_at" timestamp column to the auth.organizations CREATE
TABLE statement (matching the users table soft-delete pattern) and update any
organization-retrieval queries to include WHERE deleted_at IS NULL so only
active organizations are returned; reference the "auth.organizations" table and
the "deleted_at" column when making these changes.
- Around line 61-70: The new "auth"."users" table drop introduced a breaking
change by omitting soft-delete support; add a nullable "deleted_at" timestamp
column to the "auth"."users" table schema (column name: deleted_at) and update
all data-access logic to treat active users as WHERE deleted_at IS NULL, while
preserving endpoints/operations for listing deleted users and restoring them
(e.g., listActiveUsers, listDeletedUsers, restoreUser) so soft-deletes can be
listed and reverted rather than hard-deleted; if hard-delete is intentional,
document the retention policy and obtain compliance signoff instead.
- Around line 82-93: The INSERT into "auth"."users" from users_backup excludes
soft-deleted rows via the WHERE deleted_at IS NULL filter, causing irreversible
loss of soft-deleted users; remove that WHERE clause and migrate all rows from
users_backup (including deleted_at values) so deleted_at is preserved in the new
auth.users table (ensure the INSERT lists and maps the deleted_at column if
present) and keep ON CONFLICT (id) DO NOTHING behavior or adjust conflict
handling if you need to restore deleted_at on conflicts.
- Around line 108-117: The INSERT into "auth"."members" currently excludes
organization_members_backup rows whose user_id points to users_backup rows with
deleted_at NOT NULL, causing soft-deleted users' memberships to be dropped;
change the SELECT to remove the WHERE user_id IN (SELECT id FROM users_backup
WHERE deleted_at IS NULL) filter so all organization_members_backup rows
(including those referencing users_backup.deleted_at) are migrated into
auth.members (keeping ON CONFLICT (id) DO NOTHING), and ensure this update is
applied consistently with the earlier change to migrate soft-deleted users.
In @packages/db/drizzle/meta/0007_snapshot.json:
- Around line 544-610: The auth.users and auth.organizations table snapshots are
missing a nullable timestamp soft-delete column (deleted_at) causing
irreversible deletions; add a nullable "deleted_at" column to both auth.users
and auth.organizations (place it after metadata in organizations and alongside
other timestamps in users), add non-unique indexes on those deleted_at columns,
update any repository/ORM queries that return active records to include "WHERE
deleted_at IS NULL" (e.g., listActiveUsers, listActiveOrganizations), and
implement restoreUser/restoreOrganization mutations that set deleted_at = NULL
to re-enable soft-delete behavior.
In @packages/trpc/src/router/admin/admin.ts:
- Around line 16-22: The deleteUser mutation currently hard-deletes via
db.delete(users)...where(eq(users.id, input.userId)) and always returns {
success: true }; change it to a soft-delete by updating users.deletedAt (e.g.,
use db.update(users).set({ deletedAt: new Date() }).where(eq(users.id,
input.userId))) and check the result/affected rows to throw or return an error
when no user was found; additionally reintroduce companion endpoints:
listDeletedUsers (query filtering where users.deletedAt is not null),
restoreUser (mutation setting deletedAt: null), and optionally
permanentlyDeleteUser for true hard deletes when needed so recovery and
existence checks are supported.
In @packages/trpc/src/router/integration/linear/utils.ts:
- Around line 55-71: verifyOrgMembership currently uses positional params and
doesn't filter out soft-deleted users or organizations and lacks an explicit
return type; change the signature to accept a single object param (e.g., {
userId, organizationId }) and add a return type (e.g., Promise<{ membership:
MemberType }>), update the db.query.members.findFirst call to either include
related user and organization via with/include and check that
membership.user.deletedAt and membership.organization.deletedAt are null (or add
explicit where clauses requiring deletedAt to be null on both related tables if
supported), and throw the existing "Not a member of this organization" or a new
error when either related entity is deleted; keep the function name
verifyOrgMembership and return the membership object as before.
- Around line 73-81: Refactor verifyOrgAdmin to use an object parameter and an
explicit return type and replace magic role strings with constants: add
ROLE_ADMIN and ROLE_OWNER constants at the top of the module, change the
function signature from verifyOrgAdmin(userId: string, organizationId: string)
to verifyOrgAdmin(params: { userId: string; organizationId: string }) with an
explicit return type matching the verified membership shape (the same type
returned by verifyOrgMembership), and update the role checks to compare
membership.role against ROLE_ADMIN and ROLE_OWNER; ensure callers are updated to
pass an object.
In @packages/trpc/src/router/organization/organization.ts:
- Line 82: Replace the loose id validation (currently declared as id: z.string()
in the organization router) with UUID validation to match other endpoints;
change that schema to use z.string().uuid() so the id parameter in this endpoint
is validated consistently with byId, delete, addMember and removeMember handlers
and prevents non-UUID values from reaching the database.
🟡 Minor comments (5)
apps/web/src/app/auth/desktop/success/components/DesktopRedirect/DesktopRedirect.tsx-18-23 (1)
18-23: Use<a>tag instead ofLinkfor custom protocol URLs.
Linkfromnext/linkis designed for client-side navigation within Next.js apps and may not handle custom protocol URLs (superset://...) correctly. Use a regular anchor tag for external/custom protocol links.🔎 Suggested fix
-import Link from "next/link"; +import type { ComponentProps } from "react"; ... - <Link - href={url} - className="text-sm text-muted-foreground/70 underline decoration-muted-foreground/40 underline-offset-4 transition-colors hover:text-muted-foreground" - > + <a + href={url} + className="text-sm text-muted-foreground/70 underline decoration-muted-foreground/40 underline-offset-4 transition-colors hover:text-muted-foreground" + > Click here if not redirected - </Link> + </a>apps/desktop/src/renderer/contexts/CollectionsProvider/collections.ts-135-146 (1)
135-146: Organizations collection missingorgparameter for scoping.All other collections include
org: activeOrgIdin their params to scope the Electric shape. The organizations collection omits this, which may cause inconsistent data scoping or return organizations the user shouldn't see.Looking at
apps/api/src/app/api/electric/[...path]/utils.ts, theauth.organizationscase uses the org param to derive user membership. Without it, the shape may not be properly scoped.🔎 Proposed fix
const organizations = createCollection( electricCollectionOptions<SelectOrganization>({ id: "organizations", shapeOptions: { url: electricUrl, - params: { table: "auth.organizations" }, + params: { + table: "auth.organizations", + org: activeOrgId, + }, headers, columnMapper, }, getKey: (item) => item.id, }), );apps/desktop/src/renderer/contexts/CollectionsProvider/CollectionsProvider.tsx-15-69 (1)
15-69: Offline-first behavior is partially implemented but incomplete at the UI layer.The API client correctly distinguishes network errors from auth failures (401/403), preserving sessions on transient failures. However, the CollectionsProvider shows an error screen on any subscription failure without retry logic or graceful reconnection handling. The tRPC client lacks a retry link configuration, and the Electric shapes rely on undocumented auto-retry behavior. To fully implement offline-first:
- Add a retry link to the tRPC client (or handle retries in CollectionsProvider)
- Make CollectionsProvider show a loading state during transient failures instead of immediately displaying an error
- Explicitly verify Electric shapes auto-retry on network errors and document the behavior clearly (currently just a comment)
apps/web/src/app/(dashboard)/components/Header/Header.tsx-24-27 (1)
24-27: Add error handling to sign-out flow.The
handleSignOutfunction doesn't handle potential failures fromauthClient.signOut(). Per coding guidelines, never swallow errors silently. A failed sign-out could leave the user in an inconsistent state where they think they're logged out but aren't.🔎 Proposed fix
const handleSignOut = async () => { - await authClient.signOut(); - router.push("/sign-in"); + try { + await authClient.signOut(); + router.push("/sign-in"); + } catch (err: unknown) { + console.error("[auth/sign-out] Sign out failed:", err); + // Still redirect - the session may be partially invalidated + router.push("/sign-in"); + } };apps/admin/src/proxy.ts-14-20 (1)
14-20: Add null check for user email.If
session.user.emailisnullorundefined(possible with some OAuth providers that don't return email), calling.endsWith()will throw a runtime error.Suggested fix
if (!session?.user) { return NextResponse.redirect(new URL(env.NEXT_PUBLIC_WEB_URL)); } - if (!session.user.email.endsWith(COMPANY.EMAIL_DOMAIN)) { + if (!session.user.email?.endsWith(COMPANY.EMAIL_DOMAIN)) { return NextResponse.redirect(new URL(env.NEXT_PUBLIC_WEB_URL)); }
🧹 Nitpick comments (13)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (1)
232-260: Consider extracting the duplicate gradient mask style.The text fade gradient applied via
maskImage(lines 241-244) is duplicated for worktree workspaces (lines 267-270). Per coding guidelines to extract magic values to named constants, consider defining this at module top.🔎 Proposed refactor
At the module top (after imports):
+const TEXT_FADE_GRADIENT = { + maskImage: "linear-gradient(to right, black calc(100% - 16px), transparent 100%)", + WebkitMaskImage: "linear-gradient(to right, black calc(100% - 16px), transparent 100%)", +} as const; + const WORKSPACE_TYPE = "WORKSPACE";Then simplify usage:
<span className="text-sm font-medium whitespace-nowrap overflow-hidden flex-1 text-left" - style={{ - maskImage: - "linear-gradient(to right, black calc(100% - 16px), transparent 100%)", - WebkitMaskImage: - "linear-gradient(to right, black calc(100% - 16px), transparent 100%)", - }} + style={TEXT_FADE_GRADIENT} >And similarly for the worktree branch on lines 264-271.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
68-69: Consider adding error handling for workspace cwd query.The query doesn't handle errors, so failures (e.g., network issues) are silent. While the terminal has fallback behavior and will initialize without a workspace cwd, surfacing query errors could improve debuggability and user experience.
Optional: Add error logging
- const { data: workspaceCwd } = - trpc.terminal.getWorkspaceCwd.useQuery(workspaceId); + const { data: workspaceCwd, error: workspaceCwdError } = + trpc.terminal.getWorkspaceCwd.useQuery(workspaceId); + + useEffect(() => { + if (workspaceCwdError) { + console.error('[Terminal] Failed to load workspace cwd:', workspaceCwdError); + } + }, [workspaceCwdError]);apps/desktop/src/lib/trpc/routers/auth/index.ts (1)
113-121: Add error handling for offline resilience.This mutation lacks error handling. If
setActiveOrganizationfails due to a network error, the error will propagate and may disrupt the user experience. Consider catching errors and returning a structured result to maintain offline-first behavior.🔎 Proposed error handling
setActiveOrganization: publicProcedure .input(z.object({ organizationId: z.string() })) .mutation(async ({ input }) => { - await authService.setActiveOrganization(input.organizationId); - return { success: true }; + try { + await authService.setActiveOrganization(input.organizationId); + return { success: true, error: null }; + } catch (err) { + console.error("[auth/setActiveOrganization] Error:", err); + return { success: false, error: "failed" }; + } }),apps/admin/src/app/(dashboard)/components/AppSidebar/components/NavUser/NavUser.tsx (1)
110-113: Consider adding loading state during sign-out.The sign-out operation is asynchronous but provides no visual feedback. Users might click multiple times while the request is pending. Consider adding a loading state to disable the menu item and show a spinner during sign-out.
💡 Example implementation with loading state
export function NavUser({ user }: NavUserProps) { const { isMobile } = useSidebar(); + const [isSigningOut, setIsSigningOut] = React.useState(false); const userInitials = user.name .split(" ") .map((name) => name[0]) .join(""); const handleSignOut = async () => { + if (isSigningOut) return; + setIsSigningOut(true); try { await authClient.signOut({ fetchOptions: { onSuccess: () => { window.location.href = env.NEXT_PUBLIC_WEB_URL; }, }, }); } catch (error) { console.error("[NavUser/signOut] Sign-out error:", error); + setIsSigningOut(false); } }; return ( <SidebarMenu> {/* ... */} - <DropdownMenuItem onClick={handleSignOut}> + <DropdownMenuItem onClick={handleSignOut} disabled={isSigningOut}> <LuLogOut /> - Log out + {isSigningOut ? "Signing out..." : "Log out"} </DropdownMenuItem> {/* ... */} </SidebarMenu> ); }apps/web/src/app/auth/desktop/success/page.tsx (1)
53-55: Consider extracting protocol strings to named constants.Per coding guidelines, magic strings could be extracted for clarity. This is a minor improvement.
🔎 Optional refactor
+const DESKTOP_PROTOCOL_DEV = "superset-dev"; +const DESKTOP_PROTOCOL_PROD = "superset"; + export default async function DesktopSuccessPage({ ... - const protocol = - process.env.NODE_ENV === "development" ? "superset-dev" : "superset"; + const protocol = + process.env.NODE_ENV === "development" ? DESKTOP_PROTOCOL_DEV : DESKTOP_PROTOCOL_PROD;packages/trpc/src/router/organization/organization.ts (1)
72-72: Extract hardcoded role strings to named constants.The role strings "owner" and "member" are magic values that should be extracted to module-level constants for maintainability and type safety.
🔎 Proposed refactor
At the top of the file, add:
+const ORGANIZATION_ROLES = { + OWNER: "owner", + MEMBER: "member", +} as const; + export const organizationRouter = {Then update the usages:
- role: "owner", + role: ORGANIZATION_ROLES.OWNER,- role: "member", + role: ORGANIZATION_ROLES.MEMBER,Also applies to: 117-117
apps/desktop/src/renderer/contexts/CollectionsProvider/collections.ts (1)
77-101: Consider addingonDeletehandler for repositories collection.The repositories collection has
onInsertandonUpdatebut lacksonDelete. If repository deletion is a supported operation, this handler should be implemented for consistency with the tasks collection pattern.apps/web/src/app/(auth)/sign-up/[[...sign-up]]/page.tsx (1)
16-46: Consider extracting duplicated sign-up logic.The
signUpWithGoogleandsignUpWithGithubfunctions share identical structure with only the provider name and loading state differing. This could be consolidated into a single parameterized function.🔎 Proposed refactor
+ const signUpWithProvider = async (provider: "google" | "github") => { + const setLoading = provider === "google" ? setIsLoadingGoogle : setIsLoadingGithub; + setLoading(true); + setError(null); + + try { + await authClient.signIn.social({ + provider, + callbackURL: env.NEXT_PUBLIC_WEB_URL, + }); + } catch (err: unknown) { + console.error(`[auth/sign-up] ${provider} sign up failed:`, err); + setError("Failed to sign up. Please try again."); + setLoading(false); + } + }; + + const signUpWithGoogle = () => signUpWithProvider("google"); + const signUpWithGithub = () => signUpWithProvider("github");apps/desktop/src/renderer/screens/main/components/TasksView/components/OrganizationSwitcher/OrganizationSwitcher.tsx (3)
25-30: Remove debug logging statements.Debug console.log statements should be removed before production deployment. If logging is needed for observability, consider using a proper logging framework or at minimum ensure these are only active in development mode.
🔎 Remove or guard debug logs
- console.log("[OrganizationSwitcher] Debug:", { - hasSession: !!session, - activeOrganizationId, - organizationsCount: organizations?.length, - organizations: organizations?.map((o) => ({ id: o.id, name: o.name })), - }); + if (process.env.NODE_ENV === "development") { + console.log("[OrganizationSwitcher] Debug:", { + hasSession: !!session, + activeOrganizationId, + organizationsCount: organizations?.length, + organizations: organizations?.map((o) => ({ id: o.id, name: o.name })), + }); + }
41-46: Redundant type annotation in map callback.The type annotation
: stringon the map parameternis unnecessary since TypeScript can infer the type from the split operation.🔎 Remove redundant type annotation
const initials = activeOrganization.name ?.split(" ") - .map((n: string) => n[0]) + .map((n) => n[0]) .join("") .toUpperCase() .slice(0, 2);
36-39: Consider showing a loading state instead of returning null.Returning
nullwhen no active organization is found may result in the component disappearing during data loading or session initialization. Consider rendering a loading skeleton or placeholder to improve the user experience.🔎 Add loading state
if (!activeOrganization) { console.log("[OrganizationSwitcher] No active org found, returning null"); - return null; + return ( + <div className="flex items-center gap-2 w-full px-2 py-1.5"> + <div className="h-6 w-6 rounded-md bg-muted animate-pulse" /> + <div className="h-4 flex-1 bg-muted rounded animate-pulse" /> + </div> + ); }apps/desktop/src/renderer/screens/main/components/TasksView/TasksView.tsx (1)
58-67: Type annotation may be redundant.The
SelectTasktype annotation on thedraftparameter may be unnecessary if thecollections.tasks.updatemethod properly types its callback parameter. Consider removing it to reduce verbosity unless the type inference fails without it.🔎 Simplify callback signature
- await collections.tasks.update(task.id, (draft: SelectTask) => { + await collections.tasks.update(task.id, (draft) => { draft.title = title; draft.description = description || null; draft.priority = priority as | "urgent" | "high" | "medium" | "low" | "none"; });apps/desktop/src/main/index.ts (1)
43-48: Add user-facing error feedback.Authentication failures are only logged to the console without any user-facing feedback. Users won't know if the authentication deep link failed unless they check logs.
🔎 Add user notification
const result = await authService.handleAuthCallback(authParams); if (result.success) { focusMainWindow(); } else { console.error("[main] Auth deep link failed:", result.error); + // Show error dialog to user + dialog.showErrorBox( + "Authentication Failed", + `Failed to complete authentication: ${result.error}` + ); }
apps/desktop/drizzle.config.ts
Outdated
| schema: [ | ||
| "./src/renderer/lib/pglite/schema/auth.ts", | ||
| "./src/renderer/lib/pglite/schema/schema.ts", | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify schema files exist, check for table conflicts, and verify soft-delete support
echo "=== Checking schema file existence ==="
ls -la apps/desktop/src/renderer/lib/pglite/schema/ 2>/dev/null || echo "Directory not found"
echo -e "\n=== Checking auth.ts existence ==="
if [ -f "apps/desktop/src/renderer/lib/pglite/schema/auth.ts" ]; then
echo "auth.ts EXISTS"
else
echo "auth.ts NOT FOUND"
fi
echo -e "\n=== Checking schema.ts existence ==="
if [ -f "apps/desktop/src/renderer/lib/pglite/schema/schema.ts" ]; then
echo "schema.ts EXISTS"
else
echo "schema.ts NOT FOUND"
fi
echo -e "\n=== Listing tables/exports from auth.ts ==="
if [ -f "apps/desktop/src/renderer/lib/pglite/schema/auth.ts" ]; then
rg "export const" apps/desktop/src/renderer/lib/pglite/schema/auth.ts -A 2
fi
echo -e "\n=== Listing tables/exports from schema.ts ==="
if [ -f "apps/desktop/src/renderer/lib/pglite/schema/schema.ts" ]; then
rg "export const" apps/desktop/src/renderer/lib/pglite/schema/schema.ts -A 2 | head -40
fi
echo -e "\n=== Checking for deletedAt in auth.ts ==="
if [ -f "apps/desktop/src/renderer/lib/pglite/schema/auth.ts" ]; then
rg "deletedAt|deleted_at" apps/desktop/src/renderer/lib/pglite/schema/auth.ts -i
fi
echo -e "\n=== Extracting table/entity names for conflict check ==="
if [ -f "apps/desktop/src/renderer/lib/pglite/schema/auth.ts" ]; then
echo "From auth.ts:"
rg "export const \s*(\w+)" apps/desktop/src/renderer/lib/pglite/schema/auth.ts -o '$1'
fi
if [ -f "apps/desktop/src/renderer/lib/pglite/schema/schema.ts" ]; then
echo -e "\nFrom schema.ts:"
rg "export const \s*(\w+)" apps/desktop/src/renderer/lib/pglite/schema/schema.ts -o '$1'
fiRepository: superset-sh/superset
Length of output: 427
🏁 Script executed:
#!/bin/bash
# Find drizzle config and schema files
echo "=== Finding drizzle.config.ts ==="
fd -t f "drizzle\.config" --max-results 5
echo -e "\n=== Finding schema files named auth.ts ==="
fd -t f "auth\.ts" --max-results 10
echo -e "\n=== Checking apps/desktop directory structure ==="
fd -t d "^apps/desktop" --max-depth 3 | head -20
echo -e "\n=== Looking for pglite or schema directories ==="
fd -t d "pglite|schema" apps/desktop --max-depth 5
echo -e "\n=== Checking if apps/desktop exists and what's in it ==="
ls -la apps/desktop 2>/dev/null | head -20
echo -e "\n=== Finding any .ts files in apps/desktop/src ==="
fd -t f "\.ts$" apps/desktop/src --max-depth 3 2>/dev/null | head -20Repository: superset-sh/superset
Length of output: 2588
🏁 Script executed:
#!/bin/bash
echo "=== Content of apps/desktop/drizzle.config.ts ==="
cat -n apps/desktop/drizzle.config.ts
echo -e "\n=== Searching for any pglite references ==="
rg "pglite" --max-count 20
echo -e "\n=== Checking packages/db drizzle.config.ts for comparison ==="
cat -n packages/db/drizzle.config.ts
echo -e "\n=== Checking packages/local-db drizzle.config.ts ==="
cat -n packages/local-db/drizzle.config.tsRepository: superset-sh/superset
Length of output: 3055
Schema files referenced in drizzle.config.ts don't exist.
The configuration now references schema files at ./src/renderer/lib/pglite/schema/auth.ts and ./src/renderer/lib/pglite/schema/schema.ts, but neither file exists in the repository. These paths must be created with the appropriate schema definitions before Drizzle can generate migrations. Additionally, ensure the output directory ./src/renderer/lib/pglite/migrations is created to store generated migration files.
| async function processDeepLink(url: string): Promise<void> { | ||
| if (isAuthDeepLink(url)) { | ||
| const result = await handleAuthDeepLink(url); | ||
| if ( | ||
| result.success && | ||
| result.accessToken && | ||
| result.accessTokenExpiresAt && | ||
| result.refreshToken && | ||
| result.refreshTokenExpiresAt && | ||
| result.state | ||
| ) { | ||
| await authService.handleAuthCallback({ | ||
| accessToken: result.accessToken, | ||
| accessTokenExpiresAt: result.accessTokenExpiresAt, | ||
| refreshToken: result.refreshToken, | ||
| refreshTokenExpiresAt: result.refreshTokenExpiresAt, | ||
| state: result.state, | ||
| }); | ||
| focusMainWindow(); | ||
| } else { | ||
| console.error("[main] Auth deep link failed:", result.error); | ||
| } | ||
| const authParams = parseAuthDeepLink(url); | ||
| if (!authParams) return; | ||
|
|
||
| const result = await authService.handleAuthCallback(authParams); | ||
| if (result.success) { | ||
| focusMainWindow(); | ||
| } else { | ||
| console.error("[main] Auth deep link failed:", result.error); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find where authService is defined/imported
rg -n "authService" apps/desktop/src/main/index.ts -A 2 -B 2Repository: superset-sh/superset
Length of output: 573
🏁 Script executed:
# Search for authService module definition
fd -e ts -e tsx "auth" apps/desktop/src/main --type fRepository: superset-sh/superset
Length of output: 102
🏁 Script executed:
# Look for handleAuthCallback implementation
rg -n "handleAuthCallback" apps/desktop/src/main --type ts -B 5 -A 15Repository: superset-sh/superset
Length of output: 4250
🏁 Script executed:
# Check if there are auth service files
git ls-files apps/desktop/src/main | grep -i authRepository: superset-sh/superset
Length of output: 191
🏁 Script executed:
cat -n apps/desktop/src/main/lib/auth/auth.tsRepository: superset-sh/superset
Length of output: 7528
🏁 Script executed:
# Check the SignInResult type definition
rg -n "SignInResult" apps/desktop/src/main/lib/auth/ -B 3 -A 3Repository: superset-sh/superset
Length of output: 1739
🏁 Script executed:
# Check refreshSession implementation
rg -n "refreshSession" apps/desktop/src/main/lib/auth/ -B 2 -A 20Repository: superset-sh/superset
Length of output: 6455
🏁 Script executed:
# Check if auth.api has error type definitions
rg -n "error|Error|Exception" apps/desktop/src/main/lib/auth/ -t ts --type-listRepository: superset-sh/superset
Length of output: 453
🏁 Script executed:
# Look for any error handling patterns in auth API
rg -n "401|403|NetworkError|TimeoutError" apps/desktop/src/ -t tsRepository: superset-sh/superset
Length of output: 8611
🏁 Script executed:
cat -n apps/desktop/src/main/lib/api-client.ts | head -50Repository: superset-sh/superset
Length of output: 1764
🏁 Script executed:
# Check how refreshSession uses the API
rg -n "auth.api.getSession" apps/desktop/src/main/ -B 5 -A 5Repository: superset-sh/superset
Length of output: 1465
🏁 Script executed:
# Check what auth.api is
rg -n "from.*auth" apps/desktop/src/main/lib/auth/auth.ts -B 2 -A 2Repository: superset-sh/superset
Length of output: 308
handleAuthCallback() does NOT properly distinguish network errors from authentication failures, and bypasses error handling that should clear sessions on 401/403 responses.
The implementation has a critical flaw in how it handles session refresh errors. While the token is correctly saved to disk before calling refreshSession() (line 190), the actual error handling is broken:
-
Bypasses error discrimination layer:
refreshSession()callsauth.api.getSession()directly instead of going throughapiClient, which contains the logic to distinguish between:- 401/403 responses → should call
authService.signOut()to clear the session - Network errors → should preserve the session for offline work
- 401/403 responses → should call
-
No error type reporting: The result type
SignInResulthas no way to express "token saved successfully but session fetch failed due to network error." The caller cannot distinguish whether to show success UI or a transient error. -
Silent failures: Both network errors and 401/403 auth failures result in
this.session = null(line 129) with no distinction. Invalid/expired tokens persist in storage instead of being cleared. -
Caller is blind:
handleAuthCallback()unconditionally returns{ success: true }(line 202) even when session fetch fails, leaving the UI unaware of potential network issues.
The offline-first requirement requires refreshSession() to either:
- Use the
apiClientfetch layer that already has 401/403 discrimination, or - Replicate the error discrimination logic to detect HTTP status codes and only call
signOut()on definitive auth failures - Return or throw a result that communicates whether the session was actually fetched
🤖 Prompt for AI Agents
In @apps/desktop/src/main/index.ts around lines 39-49, The auth flow currently
saves tokens then calls refreshSession() which calls auth.api.getSession()
directly, losing the apiClient's 401/403 vs network error discrimination and
causing handleAuthCallback() to always return success; fix refreshSession() to
call the apiClient fetch path (or replicate its status-code logic) so 401/403
triggers authService.signOut() and network errors do not clear session, expand
the SignInResult type returned by refreshSession()/handleAuthCallback() to
express token-saved + sessionFetch status (e.g., success,
transient_network_error, auth_failure), ensure this result is propagated to
handleAuthCallback() and change its return value so callers can distinguish a
token-only success from a failed session fetch, and ensure any
invalid/expired-token paths clear persisted tokens only on definitive auth
failures (401/403) while preserving them on network errors.
| const collections = useMemo(() => { | ||
| console.log("[CollectionsProvider] Creating collections with:", { | ||
| hasToken: !!token, | ||
| activeOrgId, | ||
| }); | ||
|
|
||
| if (!token || !activeOrgId) { | ||
| console.log( | ||
| "[CollectionsProvider] Missing token or activeOrgId, returning null", | ||
| ); | ||
| return null; | ||
| } | ||
|
|
||
| try { | ||
| return createCollections({ token, activeOrgId }); | ||
| } catch (err) { | ||
| console.error("[CollectionsProvider] Failed to create collections:", err); | ||
| setError(err instanceof Error ? err : new Error(String(err))); | ||
| return null; | ||
| } | ||
| }, [token, activeOrgId]); |
There was a problem hiding this comment.
Critical: Add cleanup for Electric shape subscriptions to prevent resource leaks.
The useMemo creates collections with Electric shape subscriptions (see collections.ts lines 23-148) but never cleans them up when token or activeOrgId changes. Electric shapes establish persistent HTTP streaming connections that will leak when dependencies change, leading to:
- Unclosed HTTP connections accumulating over time
- Multiple concurrent subscriptions to the same data
- Memory leaks from abandoned collection instances
- Potential auth token conflicts if old shapes continue using stale tokens
Additionally, setting error state (line 40) inside useMemo can cause render loops if the error itself triggers a re-render that re-evaluates the memo.
🔎 Recommended fix: Use useEffect with cleanup
- const collections = useMemo(() => {
+ const [collections, setCollections] = useState<Collections | null>(null);
+
+ useEffect(() => {
console.log("[CollectionsProvider] Creating collections with:", {
hasToken: !!token,
activeOrgId,
});
if (!token || !activeOrgId) {
console.log(
"[CollectionsProvider] Missing token or activeOrgId, returning null",
);
- return null;
+ setCollections(null);
+ return;
}
try {
- return createCollections({ token, activeOrgId });
+ const newCollections = createCollections({ token, activeOrgId });
+ setCollections(newCollections);
+
+ // Cleanup function
+ return () => {
+ console.log("[CollectionsProvider] Cleaning up collections");
+ // Call dispose/cleanup methods on each collection
+ // Example: newCollections.tasks.dispose?.();
+ // If collections don't have cleanup methods, they need to be added
+ };
} catch (err) {
console.error("[CollectionsProvider] Failed to create collections:", err);
setError(err instanceof Error ? err : new Error(String(err)));
- return null;
}
}, [token, activeOrgId]);Note: Verify that collection instances returned by createCollections provide cleanup/dispose methods. If not, those need to be added to properly unsubscribe from Electric shapes.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@apps/desktop/src/renderer/contexts/CollectionsProvider/CollectionsProvider.tsx
around lines 23-43, The memoized creation of collections in CollectionsProvider
currently lives in useMemo and never disposes Electric shape subscriptions; move
the creation logic into a useEffect that depends on [token, activeOrgId], call
createCollections({ token, activeOrgId }) inside the effect, store the returned
instance in state (or a ref), and return a cleanup function that calls the
collection instance's dispose/cleanup/unsubscribe method (add such a method to
the collection factory if missing) to close HTTP streams when token or
activeOrgId change; also remove setError from inside the creation path in the
effect (set error via effect-level catch and avoid setting state synchronously
inside useMemo) so state updates don’t trigger memo re-evaluation loops.
| export const authClient = createAuthClient({ | ||
| baseURL: process.env.NEXT_PUBLIC_API_URL, | ||
| plugins: [organizationClient()], | ||
| }); |
There was a problem hiding this comment.
Validate required environment variable.
The baseURL configuration uses process.env.NEXT_PUBLIC_API_URL without validation. If this environment variable is undefined, the auth client will fail at runtime when attempting to make requests.
🔎 Add runtime validation
+if (!process.env.NEXT_PUBLIC_API_URL) {
+ throw new Error("NEXT_PUBLIC_API_URL environment variable is required");
+}
+
export const authClient = createAuthClient({
baseURL: process.env.NEXT_PUBLIC_API_URL,
plugins: [organizationClient()],
});🤖 Prompt for AI Agents
In @packages/auth/src/client.ts around lines 6-9, The auth client is created
with createAuthClient using process.env.NEXT_PUBLIC_API_URL without validation;
update the module to validate NEXT_PUBLIC_API_URL before calling
createAuthClient (e.g., check process.env.NEXT_PUBLIC_API_URL and if missing
throw or log a clear error), and then pass the validated value as baseURL to
createAuthClient; refer to authClient, createAuthClient,
process.env.NEXT_PUBLIC_API_URL and organizationClient() when making the change.
| "auth.users": { | ||
| "name": "users", | ||
| "schema": "auth", | ||
| "columns": { | ||
| "id": { | ||
| "name": "id", | ||
| "type": "uuid", | ||
| "primaryKey": true, | ||
| "notNull": true, | ||
| "default": "gen_random_uuid()" | ||
| }, | ||
| "name": { | ||
| "name": "name", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": true | ||
| }, | ||
| "email": { | ||
| "name": "email", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": true | ||
| }, | ||
| "email_verified": { | ||
| "name": "email_verified", | ||
| "type": "boolean", | ||
| "primaryKey": false, | ||
| "notNull": true, | ||
| "default": false | ||
| }, | ||
| "image": { | ||
| "name": "image", | ||
| "type": "text", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| }, | ||
| "created_at": { | ||
| "name": "created_at", | ||
| "type": "timestamp", | ||
| "primaryKey": false, | ||
| "notNull": true, | ||
| "default": "now()" | ||
| }, | ||
| "updated_at": { | ||
| "name": "updated_at", | ||
| "type": "timestamp", | ||
| "primaryKey": false, | ||
| "notNull": true, | ||
| "default": "now()" | ||
| } | ||
| }, | ||
| "indexes": {}, | ||
| "foreignKeys": {}, | ||
| "compositePrimaryKeys": {}, | ||
| "uniqueConstraints": { | ||
| "users_email_unique": { | ||
| "name": "users_email_unique", | ||
| "nullsNotDistinct": false, | ||
| "columns": [ | ||
| "email" | ||
| ] | ||
| } | ||
| }, | ||
| "policies": {}, | ||
| "checkConstraints": {}, | ||
| "isRLSEnabled": false | ||
| }, |
There was a problem hiding this comment.
Critical: Missing soft-delete column in auth.users table.
The auth.users table lacks a deleted_at column, making user deletion irreversible. This directly addresses the reviewer's concern about the loss of soft-delete functionality and has severe implications:
- Data loss: Hard deletion is permanent; users cannot be recovered for compliance audits, legal holds, or accidental deletion recovery.
- API contract breakage: The previous admin API exposed
listActiveUsers,listDeletedUsers, andrestoreUserendpoints that are no longer supported. - Compliance risk: Many regulations (GDPR, SOC2, HIPAA) require audit trails and the ability to recover data within retention windows.
- Inconsistency: The
public.taskstable (line 1255) retains adeleted_atcolumn, creating schema inconsistency.
The same issue applies to auth.organizations (lines 361-435), which also lacks deleted_at.
🔎 Proposed fix: Add deleted_at columns to auth schema tables
Add deleted_at column to auth.users:
"updated_at": {
"name": "updated_at",
"type": "timestamp",
"primaryKey": false,
"notNull": true,
"default": "now()"
- }
+ },
+ "deleted_at": {
+ "name": "deleted_at",
+ "type": "timestamp",
+ "primaryKey": false,
+ "notNull": false
+ }
},Apply the same change to auth.organizations (after the metadata column, before closing columns).
Additionally:
- Add a non-unique index on
deleted_atfor both tables to optimize queries filtering by soft-delete status - Update all queries to filter
WHERE deleted_at IS NULLfor active records - Implement
restoreUserandrestoreOrganizationmutations that setdeleted_at = NULL - Preserve separate listing endpoints for active vs. deleted entities
📝 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.
| "auth.users": { | |
| "name": "users", | |
| "schema": "auth", | |
| "columns": { | |
| "id": { | |
| "name": "id", | |
| "type": "uuid", | |
| "primaryKey": true, | |
| "notNull": true, | |
| "default": "gen_random_uuid()" | |
| }, | |
| "name": { | |
| "name": "name", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": true | |
| }, | |
| "email": { | |
| "name": "email", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": true | |
| }, | |
| "email_verified": { | |
| "name": "email_verified", | |
| "type": "boolean", | |
| "primaryKey": false, | |
| "notNull": true, | |
| "default": false | |
| }, | |
| "image": { | |
| "name": "image", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "created_at": { | |
| "name": "created_at", | |
| "type": "timestamp", | |
| "primaryKey": false, | |
| "notNull": true, | |
| "default": "now()" | |
| }, | |
| "updated_at": { | |
| "name": "updated_at", | |
| "type": "timestamp", | |
| "primaryKey": false, | |
| "notNull": true, | |
| "default": "now()" | |
| } | |
| }, | |
| "indexes": {}, | |
| "foreignKeys": {}, | |
| "compositePrimaryKeys": {}, | |
| "uniqueConstraints": { | |
| "users_email_unique": { | |
| "name": "users_email_unique", | |
| "nullsNotDistinct": false, | |
| "columns": [ | |
| "email" | |
| ] | |
| } | |
| }, | |
| "policies": {}, | |
| "checkConstraints": {}, | |
| "isRLSEnabled": false | |
| }, | |
| "auth.users": { | |
| "name": "users", | |
| "schema": "auth", | |
| "columns": { | |
| "id": { | |
| "name": "id", | |
| "type": "uuid", | |
| "primaryKey": true, | |
| "notNull": true, | |
| "default": "gen_random_uuid()" | |
| }, | |
| "name": { | |
| "name": "name", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": true | |
| }, | |
| "email": { | |
| "name": "email", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": true | |
| }, | |
| "email_verified": { | |
| "name": "email_verified", | |
| "type": "boolean", | |
| "primaryKey": false, | |
| "notNull": true, | |
| "default": false | |
| }, | |
| "image": { | |
| "name": "image", | |
| "type": "text", | |
| "primaryKey": false, | |
| "notNull": false | |
| }, | |
| "created_at": { | |
| "name": "created_at", | |
| "type": "timestamp", | |
| "primaryKey": false, | |
| "notNull": true, | |
| "default": "now()" | |
| }, | |
| "updated_at": { | |
| "name": "updated_at", | |
| "type": "timestamp", | |
| "primaryKey": false, | |
| "notNull": true, | |
| "default": "now()" | |
| }, | |
| "deleted_at": { | |
| "name": "deleted_at", | |
| "type": "timestamp", | |
| "primaryKey": false, | |
| "notNull": false | |
| } | |
| }, | |
| "indexes": {}, | |
| "foreignKeys": {}, | |
| "compositePrimaryKeys": {}, | |
| "uniqueConstraints": { | |
| "users_email_unique": { | |
| "name": "users_email_unique", | |
| "nullsNotDistinct": false, | |
| "columns": [ | |
| "email" | |
| ] | |
| } | |
| }, | |
| "policies": {}, | |
| "checkConstraints": {}, | |
| "isRLSEnabled": false | |
| }, |
🤖 Prompt for AI Agents
In @packages/db/drizzle/meta/0007_snapshot.json around lines 544-610, The
auth.users and auth.organizations table snapshots are missing a nullable
timestamp soft-delete column (deleted_at) causing irreversible deletions; add a
nullable "deleted_at" column to both auth.users and auth.organizations (place it
after metadata in organizations and alongside other timestamps in users), add
non-unique indexes on those deleted_at columns, update any repository/ORM
queries that return active records to include "WHERE deleted_at IS NULL" (e.g.,
listActiveUsers, listActiveOrganizations), and implement
restoreUser/restoreOrganization mutations that set deleted_at = NULL to
re-enable soft-delete behavior.
| deleteUser: adminProcedure | ||
| .input(z.object({ userId: z.string() })) | ||
| .mutation(async ({ input }) => { | ||
| const [user] = await db | ||
| .update(users) | ||
| .set({ deletedAt: null }) | ||
| .where(eq(users.id, input.userId)) | ||
| .returning(); | ||
|
|
||
| if (!user) { | ||
| throw new TRPCError({ | ||
| code: "NOT_FOUND", | ||
| message: "User not found", | ||
| }); | ||
| } | ||
|
|
||
| return user; | ||
| }), | ||
|
|
||
| permanentlyDeleteUser: adminProcedure | ||
| .input(z.object({ userId: z.string().uuid() })) | ||
| .mutation(async () => { | ||
| // TODO: Implement Clerk user deletion, avatar cleanup, etc. | ||
| throw new TRPCError({ | ||
| code: "NOT_IMPLEMENTED", | ||
| message: | ||
| "Permanent deletion not yet implemented - requires Clerk cleanup", | ||
| }); | ||
| // Delete user - Better Auth handles cascading session cleanup | ||
| await db.delete(users).where(eq(users.id, input.userId)); | ||
| return { success: true }; | ||
| }), |
There was a problem hiding this comment.
Hard delete removes recovery capability and lacks error handling.
Two concerns with this implementation:
-
Loss of soft-delete: The PR reviewer flagged that hard deletion is irreversible and prevents recovery for compliance or user-requested restores. The previous API included
listDeletedUsersandrestoreUserendpoints that are now removed. -
Silent failure on non-existent user: The mutation returns
{ success: true }even ifuserIddoesn't match any user. Consider checking affected rows or verifying the user exists first.
🔎 Suggested implementation with soft-delete and existence check
+import { isNull } from "drizzle-orm";
+import { TRPCError } from "@trpc/server";
deleteUser: adminProcedure
.input(z.object({ userId: z.string() }))
.mutation(async ({ input }) => {
- // Delete user - Better Auth handles cascading session cleanup
- await db.delete(users).where(eq(users.id, input.userId));
- return { success: true };
+ const user = await db.query.users.findFirst({
+ where: eq(users.id, input.userId),
+ });
+
+ if (!user) {
+ throw new TRPCError({
+ code: "NOT_FOUND",
+ message: "User not found",
+ });
+ }
+
+ // Soft delete - set deletedAt timestamp
+ await db
+ .update(users)
+ .set({ deletedAt: new Date() })
+ .where(eq(users.id, input.userId));
+
+ return { success: true };
}),If soft-delete is restored, also consider re-adding:
listDeletedUsersquery filteringwhere: isNotNull(users.deletedAt)restoreUsermutation settingdeletedAt: nullpermanentlyDeleteUserfor actual hard delete when needed
🤖 Prompt for AI Agents
In @packages/trpc/src/router/admin/admin.ts around lines 16-22, The deleteUser
mutation currently hard-deletes via db.delete(users)...where(eq(users.id,
input.userId)) and always returns { success: true }; change it to a soft-delete
by updating users.deletedAt (e.g., use db.update(users).set({ deletedAt: new
Date() }).where(eq(users.id, input.userId))) and check the result/affected rows
to throw or return an error when no user was found; additionally reintroduce
companion endpoints: listDeletedUsers (query filtering where users.deletedAt is
not null), restoreUser (mutation setting deletedAt: null), and optionally
permanentlyDeleteUser for true hard deletes when needed so recovery and
existence checks are supported.
| export async function verifyOrgMembership( | ||
| clerkUserId: string, | ||
| userId: string, | ||
| organizationId: string, | ||
| ) { | ||
| const user = await db.query.users.findFirst({ | ||
| where: eq(users.clerkId, clerkUserId), | ||
| }); | ||
|
|
||
| if (!user) { | ||
| throw new Error("User not found"); | ||
| } | ||
|
|
||
| const membership = await db.query.organizationMembers.findFirst({ | ||
| const membership = await db.query.members.findFirst({ | ||
| where: and( | ||
| eq(organizationMembers.organizationId, organizationId), | ||
| eq(organizationMembers.userId, user.id), | ||
| eq(members.organizationId, organizationId), | ||
| eq(members.userId, userId), | ||
| ), | ||
| }); | ||
|
|
||
| if (!membership) { | ||
| throw new Error("Not a member of this organization"); | ||
| } | ||
|
|
||
| return { user, membership }; | ||
| return { membership }; | ||
| } |
There was a problem hiding this comment.
Critical: Missing soft-delete filtering for users and organizations.
Based on the PR reviewer's feedback, the migration removed soft-delete functionality. This function queries the members table without verifying that the referenced user or organization is not soft-deleted (via deletedAt). This breaks the ability to filter out deleted entities and could expose deleted organizations or users.
Additionally:
- The function still lacks an explicit return type annotation (as noted in the previous review).
- Per coding guidelines, functions with 2+ parameters should use object parameters instead of positional arguments for self-documentation and extensibility.
🔎 Verify if soft-delete columns exist in related tables
#!/bin/bash
# Check if users or organizations tables have deletedAt columns
rg -n "deletedAt" packages/db/src/schema/ --type=ts -C3🔧 Suggested fixes
1. Add explicit return type and object parameters:
-export async function verifyOrgMembership(
- userId: string,
- organizationId: string,
-) {
+export async function verifyOrgMembership(params: {
+ userId: string;
+ organizationId: string;
+}): Promise<{ membership: typeof members.$inferSelect }> {
+ const { userId, organizationId } = params;
const membership = await db.query.members.findFirst({
where: and(
eq(members.organizationId, organizationId),
eq(members.userId, userId),
),
});2. Add soft-delete filtering (if schema supports it):
If the users and organizations tables have deletedAt columns, join and filter them:
const membership = await db.query.members.findFirst({
where: and(
eq(members.organizationId, organizationId),
eq(members.userId, userId),
),
with: {
user: true,
organization: true,
},
});
if (!membership) {
throw new Error("Not a member of this organization");
}
if (membership.user.deletedAt || membership.organization.deletedAt) {
throw new Error("User or organization has been deleted");
}📝 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.
| export async function verifyOrgMembership( | |
| clerkUserId: string, | |
| userId: string, | |
| organizationId: string, | |
| ) { | |
| const user = await db.query.users.findFirst({ | |
| where: eq(users.clerkId, clerkUserId), | |
| }); | |
| if (!user) { | |
| throw new Error("User not found"); | |
| } | |
| const membership = await db.query.organizationMembers.findFirst({ | |
| const membership = await db.query.members.findFirst({ | |
| where: and( | |
| eq(organizationMembers.organizationId, organizationId), | |
| eq(organizationMembers.userId, user.id), | |
| eq(members.organizationId, organizationId), | |
| eq(members.userId, userId), | |
| ), | |
| }); | |
| if (!membership) { | |
| throw new Error("Not a member of this organization"); | |
| } | |
| return { user, membership }; | |
| return { membership }; | |
| } | |
| export async function verifyOrgMembership(params: { | |
| userId: string; | |
| organizationId: string; | |
| }): Promise<{ membership: typeof members.$inferSelect }> { | |
| const { userId, organizationId } = params; | |
| const membership = await db.query.members.findFirst({ | |
| where: and( | |
| eq(members.organizationId, organizationId), | |
| eq(members.userId, userId), | |
| ), | |
| }); | |
| if (!membership) { | |
| throw new Error("Not a member of this organization"); | |
| } | |
| return { membership }; | |
| } |
🤖 Prompt for AI Agents
In @packages/trpc/src/router/integration/linear/utils.ts around lines 55-71,
verifyOrgMembership currently uses positional params and doesn't filter out
soft-deleted users or organizations and lacks an explicit return type; change
the signature to accept a single object param (e.g., { userId, organizationId })
and add a return type (e.g., Promise<{ membership: MemberType }>), update the
db.query.members.findFirst call to either include related user and organization
via with/include and check that membership.user.deletedAt and
membership.organization.deletedAt are null (or add explicit where clauses
requiring deletedAt to be null on both related tables if supported), and throw
the existing "Not a member of this organization" or a new error when either
related entity is deleted; keep the function name verifyOrgMembership and return
the membership object as before.
| export async function verifyOrgAdmin(userId: string, organizationId: string) { | ||
| const { membership } = await verifyOrgMembership(userId, organizationId); | ||
|
|
||
| if (membership.role !== "admin") { | ||
| if (membership.role !== "admin" && membership.role !== "owner") { | ||
| throw new Error("Admin access required"); | ||
| } | ||
|
|
||
| return { user, membership }; | ||
| return { membership }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/trpc/src/router/integration/linear/utils.ts | head -100Repository: superset-sh/superset
Length of output: 2438
🏁 Script executed:
rg -n "verifyOrgMembership|verifyOrgAdmin" packages/trpc/src/router/integration/linear/ -A5 -B2Repository: superset-sh/superset
Length of output: 5056
🏁 Script executed:
rg -n "role.*owner|role.*admin" packages/trpc/src/router/integration/linear/ -t tsRepository: superset-sh/superset
Length of output: 186
🏁 Script executed:
# Check the members schema definition for role type and soft-delete field
find . -name "schema*" -type f | head -20Repository: superset-sh/superset
Length of output: 230
🏁 Script executed:
# Look for members table definition
rg -n "export.*members|const members" packages/db/src/ -A10 -t tsRepository: superset-sh/superset
Length of output: 2338
🏁 Script executed:
# Check if there's a soft-delete pattern in the db queries
rg -n "isDeleted|deleted_at|softDelete" packages/db/src/schema/ -t tsRepository: superset-sh/superset
Length of output: 138
🏁 Script executed:
# Get the full members table definition
cat -n packages/db/src/schema/auth.ts | sed -n '105,130p'Repository: superset-sh/superset
Length of output: 1041
🏁 Script executed:
# Check if members table has deletedAt field
rg -n "deletedAt|deleted_at" packages/db/src/schema/auth.ts -B3 -A3Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Verify the role type and possible values
rg -n "role.*=" packages/db/src/schema/ -A2 | head -40Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check other tables to see which ones have deletedAt for soft-delete pattern
rg -n "deletedAt.*timestamp|deleted_at.*timestamp" packages/db/src/schema/ -B2Repository: superset-sh/superset
Length of output: 292
🏁 Script executed:
# Verify the actual call to verifyOrgAdmin and verifyOrgMembership to understand the full usage
cat -n packages/trpc/src/router/integration/linear/linear.ts | head -80Repository: superset-sh/superset
Length of output: 3166
🏁 Script executed:
# Check the usage context to see what callers expect from return value
rg -n "verifyOrgAdmin\|verifyOrgMembership" packages/trpc/src/router/integration/linear/linear.tsRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if there are other similar role constants or enums in the codebase
rg -n "admin.*owner|owner.*admin" packages/ -t ts | head -20Repository: superset-sh/superset
Length of output: 186
🏁 Script executed:
# Verify that members table is actually used without soft-delete filtering
rg -n "members.findFirst\|members.findMany" packages/trpc/src/router/integration/linear/ -A5Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check what return type inference would look like
rg -n "SelectMember\|typeof members" packages/db/src/schema/auth.ts -B2 -A2Repository: superset-sh/superset
Length of output: 46
Use object parameters, add explicit return type, and extract role constants.
The function has several improvements needed per coding guidelines:
- Object parameters - Should use a params object for 2+ parameters
- Explicit return type - Add type annotation for the return value
- Magic string literals - Extract "admin" and "owner" to named constants at module top
Suggested refactoring
Define role constants at module top:
+const ADMIN_ROLES = ["admin", "owner"] as const;
+
type Priority = "urgent" | "high" | "medium" | "low" | "none";Refactor function with object parameters and explicit types:
-export async function verifyOrgAdmin(userId: string, organizationId: string) {
- const { membership } = await verifyOrgMembership(userId, organizationId);
+export async function verifyOrgAdmin(params: {
+ userId: string;
+ organizationId: string;
+}): Promise<{ membership: typeof members.$inferSelect }> {
+ const { membership } = await verifyOrgMembership(params);
- if (membership.role !== "admin" && membership.role !== "owner") {
+ if (!ADMIN_ROLES.includes(membership.role as typeof ADMIN_ROLES[number])) {
throw new Error("Admin access required");
}
return { membership };
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @packages/trpc/src/router/integration/linear/utils.ts around lines 73-81,
Refactor verifyOrgAdmin to use an object parameter and an explicit return type
and replace magic role strings with constants: add ROLE_ADMIN and ROLE_OWNER
constants at the top of the module, change the function signature from
verifyOrgAdmin(userId: string, organizationId: string) to verifyOrgAdmin(params:
{ userId: string; organizationId: string }) with an explicit return type
matching the verified membership shape (the same type returned by
verifyOrgMembership), and update the role checks to compare membership.role
against ROLE_ADMIN and ROLE_OWNER; ensure callers are updated to pass an object.
| .input( | ||
| z.object({ | ||
| id: z.string().uuid(), | ||
| id: z.string(), |
There was a problem hiding this comment.
Major: Inconsistent UUID validation.
The id field uses z.string() while other endpoints (byId at line 22, delete at line 98, addMember/removeMember at lines 107-108, 126-127) use z.string().uuid(). This inconsistency could allow invalid (non-UUID) strings to pass validation and cause database errors.
🔎 Proposed fix
- id: z.string(),
+ id: z.string().uuid(),📝 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.
| id: z.string(), | |
| id: z.string().uuid(), |
🤖 Prompt for AI Agents
In @packages/trpc/src/router/organization/organization.ts around line 82,
Replace the loose id validation (currently declared as id: z.string() in the
organization router) with UUID validation to match other endpoints; change that
schema to use z.string().uuid() so the id parameter in this endpoint is
validated consistently with byId, delete, addMember and removeMember handlers
and prevents non-UUID values from reaching the database.
Electric SQL Fixes: - Add org parameter to Electric shape subscriptions to fix stale cache on org switch - Fix 400 "handle can't be blank" errors when switching organizations - Collections now create unique shapes per organization using activeOrgId Code Cleanup: - Reorganize renderer contexts to use barrel folder pattern - Delete centralized contexts/index.ts to enforce direct imports - Remove unused mySession query from user router - Delete redundant shared/auth.ts (now using @superset/shared/constants) - Clean up OrganizationSwitcher (removed debug logs) - Remove unnecessary comments from proxy.ts files - Fix nested button HTML error in WorkspaceTabs Auth Migration (Better Auth): - Migrate from Clerk to Better Auth across web, admin, and desktop apps - Update deployment scripts and environment variables - Add route-level guards and improve error handling - Implement desktop OAuth flow with Better Auth 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
saddlepaddle
force-pushed
the
fix-electric-setup
branch
from
fffd04f to
1b5ff3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/desktop/src/renderer/screens/sign-in/index.tsx (1)
11-20: Critical: Restore offline-first error handling as flagged in PR objectives.This implementation lacks error handling and does not distinguish between network errors and authentication failures, which directly addresses issue #2 raised in the PR objectives: "Loss of offline-first functionality for desktop."
Per the PR objectives, the prior implementation (PR #512) returned a three-way result type (
"success" | "invalid" | "network_error") to preserve sessions during network errors. The current implementation:
- No error handling – violates the coding guideline "Never swallow errors silently."
- No network error distinction – transient network failures could incorrectly clear sessions instead of preserving them.
- No loading/error feedback – users receive no indication of authentication progress or failure.
As noted in the PR objectives: "transient network failures could clear sessions instead of preserving them" and "sessions persist on network errors and are cleared only on invalid/401/403 responses."
🔎 Suggested approach to restore offline-first behavior
Add error handling that distinguishes network errors from authentication failures:
export function SignInScreen() { const signInMutation = trpc.auth.signIn.useMutation(); useEffect(() => { posthog.capture("desktop_opened"); }, []); const signIn = (provider: AuthProvider) => { posthog.capture("auth_started", { provider }); - signInMutation.mutate({ provider }); + signInMutation.mutate( + { provider }, + { + onError: (error) => { + // Distinguish network errors from auth failures + const isNetworkError = + error.message?.includes("fetch failed") || + error.data?.code === "NETWORK_ERROR"; + + if (isNetworkError) { + console.log("[auth/signin] Network error - preserving session"); + // Preserve session, show retry UI + } else { + console.error("[auth/signin] Authentication failed:", error); + // Clear session, show error + } + }, + } + ); }; + + // Add loading/error state display in the UIAdditionally, the tRPC backend should implement token expiry/refresh handling and return appropriate error codes to enable proper network vs auth failure distinction.
apps/desktop/src/renderer/contexts/PostHogProvider/PostHogProvider.tsx (1)
13-16: Ensure PostHog initialization completes before settingisInitializedto true.The
initPostHog()function is synchronous, butposthogFull.init()initializes PostHog asynchronously—the actual initialization completes in theloadedcallback (line 23-28 inapps/desktop/src/renderer/lib/posthog.ts). SettingisInitializedimmediately after callinginitPostHog()means the state becomes true before PostHog finishes initializing, risking lost events if the app tries to capture them before theloadedcallback fires.Either make
initPostHog()return a promise that resolves when theloadedcallback fires, or movesetIsInitialized(true)inside theloadedcallback.apps/api/src/app/api/integrations/linear/connect/route.ts (1)
26-38: Add error handling for database query.The database query lacks error handling. Database operations can fail due to connection issues, query errors, or other problems. Per coding guidelines: "Never swallow errors silently - at minimum log them with context."
🔎 Proposed error handling
+ let membership; + try { - const membership = await db.query.members.findFirst({ + membership = await db.query.members.findFirst({ where: and( eq(members.organizationId, organizationId), eq(members.userId, session.user.id), ), }); + } catch (error) { + console.error("[integrations/linear] Database query failed:", error); + return Response.json( + { error: "Internal server error" }, + { status: 500 }, + ); + } if (!membership) { + console.warn("[integrations/linear] Access denied: User not member of organization", { + userId: session.user.id, + organizationId, + }); return Response.json( { error: "User is not a member of this organization" }, { status: 403 }, ); }As per coding guidelines for API routes.
🤖 Fix all issues with AI Agents
In @apps/admin/src/app/(dashboard)/components/AppSidebar/AppSidebar.tsx:
- Line 23: The "Deleted Users" navigation item and LuUserX icon were removed
from AppSidebar, which contradicts the requested soft-delete UX; restore the
Deleted Users nav entry in AppSidebar (re-add the LuUserX import and its menu
item) and ensure it routes to the deleted-users listing component, re-enable the
soft-delete filter by preserving deletedAt filtering for organizations, and wire
the UI to call the existing restoreUser mutation (or reintroduce an equivalent)
so users can be listed as deleted and restored from the sidebar entry; update
any related components/pages to handle deleted users view and restore actions
accordingly.
In
@apps/admin/src/app/(dashboard)/components/AppSidebar/components/NavUser/NavUser.tsx:
- Around line 42-50: The sign-out flow in handleSignOut swallows failures; wrap
the authClient.signOut call in a try/catch, move the window.location.href
redirect into the successful branch (onSuccess or after awaited call), and in
the catch log the error (use console.error or process logger) and surface user
feedback (e.g., show a toast or set an error state) so failures aren’t silent;
reference handleSignOut and authClient.signOut when making these changes.
In @apps/admin/src/components/PostHogUserIdentifier/PostHogUserIdentifier.tsx:
- Around line 18-24: The effect in PostHogUserIdentifier resets PostHog during
the session loading state because it checks `!session?.user`; change the reset
condition to `session === null` so it only resets when the session is
definitively absent, and update the useEffect dependency to include `session`
(e.g., useEffect(..., [user, session])) so the effect re-runs when the session
transitions from loading to resolved.
In @apps/admin/src/proxy.ts:
- Around line 17-19: The current check calls session.user.email.endsWith(...)
which will throw if session.user.email is null/undefined; update the guard in
the proxy (where session and session.user.email are used) to first verify the
email exists and is a string (e.g., check session?.user?.email && typeof
session.user.email === 'string') and only then call
.endsWith(COMPANY.EMAIL_DOMAIN); if the email is missing or not a string, treat
it as unauthorized and return the same NextResponse.redirect(new
URL(env.NEXT_PUBLIC_WEB_URL)) response so you avoid runtime errors and preserve
behavior.
In @apps/api/src/app/api/electric/[...path]/utils.ts:
- Around line 51-90: The authorization branch for "auth.organizations"
incorrectly derives the authenticated user by querying members for an
organization and using the first members[0].userId; change the logic to accept
an explicit userId parameter (instead of reading members[0].userId) and use that
userId when querying db.query.members.findMany (the query that currently uses
members.userId) to gather all organizationIds, keep the same de-duplication and
QueryBuilder/whereExpr (sql`${sql.identifier(organizations.id.name)}`) logic,
and add an early return when the provided userId is missing or no memberships
exist; also update the call sites to pass the authenticated session userId into
this "auth.organizations" check.
In @apps/api/src/trpc/context.ts:
- Around line 4-13: The createContext function declares an unused resHeaders
parameter; remove resHeaders from the parameter destructuring and the typed
parameter object so the signature becomes createContext({ req }: { req: Request
}), and update any call sites that pass resHeaders to stop providing it; leave
the body (auth.api.getSession({ headers: req.headers })) and createTRPCContext({
session }) unchanged.
In @apps/desktop/src/main/lib/auth/auth.ts:
- Around line 55-74: The initialize() flow clears stored credentials whenever
refreshSession() fails, which causes valid tokens to be dropped on network
outages; update refreshSession() to throw distinguishable errors for auth
failures (401/403) vs network errors, then modify initialize(): after
decrypting/storing this.token and this.expiresAt and checking isExpired(), call
refreshSession() inside a try/catch that only calls this.signOut() when the
caught error indicates an auth failure (401/403) or explicit invalid token,
while treating network errors as non-fatal (log and keep token/expiresAt so
session persists offline).
- Around line 13-16: Replace the simple SignInResult interface with a
discriminated union so callers can distinguish full success, authentication
failure/invalid credentials, and transient network errors; update the type named
SignInResult to include variants like { status: "success"; tokenSaved: true;
session?: /*session type*/ } | { status: "invalid"; error: string } | { status:
"network_error"; tokenSaved?: boolean; error: string } (or similar) so code that
calls sign-in can check result.status to decide whether to preserve the saved
token/session; adjust any functions that return SignInResult to return the
appropriate discriminated variant.
- Around line 173-203: handleAuthCallback currently always returns { success:
true } even if refreshSession() fails; modify handleAuthCallback to await
refreshSession() and propagate its outcome into the returned SignInResult
instead of unconditionally returning success: wrap the await
this.refreshSession() call in a try/catch (or rely on refreshSession to throw)
and return an error result when it fails (e.g., return { success: false, error:
<message>, offline: true } or the expanded SignInResult shape you added), only
emitting "state-changed" and returning success when refreshSession completes
successfully; reference the handleAuthCallback method and the refreshSession
call to locate where to change the control flow and returned value.
- Around line 111-131: refreshSession currently clears this.session on any
exception; change it to only clear the session for definitive auth failures by
inspecting the error returned from auth.api.getSession (e.g., check error.status
or error instanceof Response) and only set this.session = null and
emit("session-changed", null) when status === 401 || status === 403; for other
errors (network/timeouts) preserve the existing this.session, log the error, and
avoid emitting a session change so offline-first behavior is retained.
In
@apps/desktop/src/renderer/screens/main/components/TasksView/components/OrganizationSwitcher/OrganizationSwitcher.tsx:
- Around line 40-42: The switchOrganization function in OrganizationSwitcher.tsx
currently calls setActiveOrg.mutateAsync({ organizationId: newOrgId }) with no
error handling or UI feedback; update switchOrganization to set a loading state
(or rely on setActiveOrg.isPending) to disable the org dropdown/show a spinner
while the mutation runs, wrap the mutateAsync call in try/catch, log failures
with context (e.g., include newOrgId and error) and show user-facing feedback
(toast or inline error) on failure, and ensure you clear the loading state after
completion; reference setActiveOrg.mutateAsync, setActiveOrg.isPending and the
switchOrganization handler when implementing these changes.
In
@apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx:
- Around line 120-123: The catch block in WorkspaceItem.tsx currently swallows
errors; update it to catch the error (e.g., catch (err)) and log it with context
before falling back to showing the dialog — for example call a logging facility
or console.error like console.error('WorkspaceItem: error checking status',
err), then call setShowDeleteDialog(true); ensure you reference the existing
setShowDeleteDialog usage and keep the fallback behavior unchanged.
In @apps/web/src/app/(auth)/sign-up/[[...sign-up]]/page.tsx:
- Around line 25-29: The catch blocks for the sign-up handlers need explicit
type annotations: change catch (err) to catch (err: unknown) in the Google
sign-up handler shown and in the GitHub sign-up handler ([SignUp/github]); then
narrow before logging by doing if (err instanceof Error) { console.error("Sign
up failed:", err.message, err); } else { console.error("Sign up failed:",
String(err)); } and keep setError / setIsLoading calls unchanged.
In @apps/web/src/app/auth/desktop/success/page.tsx:
- Around line 51-52: The code calls session.session.expiresAt.toISOString()
without guarding for null/undefined which can throw; update the assignment for
expiresAt (where token and session.session.expiresAt are used) to check
session?.session?.expiresAt first and only call .toISOString() when it is a
valid Date, otherwise set expiresAt to null (or an empty string) so downstream
code handles missing expiration safely; adjust any consumers expecting a string
to accept the nullable value.
In @apps/web/src/proxy.ts:
- Around line 5-14: The proxy's publicRoutes list only includes
"/api/auth/desktop", blocking other Better Auth endpoints; update the
publicRoutes array (used by isPublicRoute) to include a wildcard-like prefix
such as "/api/auth" or "/api/auth/" so isPublicRoute(pathname) returns true for
all Better Auth API paths (e.g., add "/api/auth" to publicRoutes) ensuring all
/api/auth/* endpoints are treated as public.
🧹 Nitpick comments (14)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (2)
126-152: Consider memoizing the attention check computation.This logic runs on every render, involving filtering, recursive traversal, and set operations. With many workspace items rendered, this could impact performance.
Proposed fix using useMemo
+ // Check if any pane in tabs belonging to this workspace needs attention + const needsAttention = useMemo(() => { + const workspaceTabs = tabs.filter((t) => t.workspaceId === id); + const collectPaneIds = (node: unknown): string[] => { + if (typeof node === "string") return [node]; + if ( + node && + typeof node === "object" && + "first" in node && + "second" in node + ) { + const branch = node as { first: unknown; second: unknown }; + return [ + ...collectPaneIds(branch.first), + ...collectPaneIds(branch.second), + ]; + } + return []; + }; + const workspacePaneIds = new Set( + workspaceTabs.flatMap((t) => collectPaneIds(t.layout)), + ); + return Object.values(panes) + .filter((p) => workspacePaneIds.has(p.id)) + .some((p) => p.needsAttention); + }, [tabs, panes, id]); - // Check if any pane in tabs belonging to this workspace needs attention - const workspaceTabs = tabs.filter((t) => t.workspaceId === id); - const workspacePaneIds = new Set( - workspaceTabs.flatMap((t) => { - // Extract pane IDs from the layout (which is a MosaicNode<string>) - const collectPaneIds = (node: unknown): string[] => { - if (typeof node === "string") return [node]; - if ( - node && - typeof node === "object" && - "first" in node && - "second" in node - ) { - const branch = node as { first: unknown; second: unknown }; - return [ - ...collectPaneIds(branch.first), - ...collectPaneIds(branch.second), - ]; - } - return []; - }; - return collectPaneIds(t.layout); - }), - ); - const needsAttention = Object.values(panes) - .filter((p) => workspacePaneIds.has(p.id)) - .some((p) => p.needsAttention);Note: Add
useMemoto the React import at line 6.
241-244: Extract duplicate mask-image style to a constant.The same fade-out gradient style is defined inline twice. Extract to a module-level constant for DRY compliance.
Proposed fix
Add constant at the top of the file (after line 23):
const TEXT_FADE_STYLE = { maskImage: "linear-gradient(to right, black calc(100% - 16px), transparent 100%)", WebkitMaskImage: "linear-gradient(to right, black calc(100% - 16px), transparent 100%)", } as const;Then replace both inline style objects:
<span className="text-sm font-medium whitespace-nowrap overflow-hidden flex-1 text-left" - style={{ - maskImage: - "linear-gradient(to right, black calc(100% - 16px), transparent 100%)", - WebkitMaskImage: - "linear-gradient(to right, black calc(100% - 16px), transparent 100%)", - }} + style={TEXT_FADE_STYLE} >Also applies to: 267-270
apps/web/src/app/auth/desktop/success/page.tsx (1)
53-55: Consider extracting protocol strings to named constants.Per coding guidelines, magic strings and hardcoded values should be extracted to named constants at module top for clarity and maintainability.
🔎 Suggested refactor
import { auth } from "@superset/auth"; import { headers } from "next/headers"; import { DesktopRedirect } from "./components/DesktopRedirect"; +const DESKTOP_PROTOCOL = { + development: "superset-dev", + production: "superset", +} as const; + export default async function DesktopSuccessPage({Then use:
- const protocol = - process.env.NODE_ENV === "development" ? "superset-dev" : "superset"; + const protocol = + process.env.NODE_ENV === "development" + ? DESKTOP_PROTOCOL.development + : DESKTOP_PROTOCOL.production;apps/api/src/app/api/integrations/linear/connect/route.ts (1)
40-54: Consider extracting OAuth configuration to constants.The OAuth flow implementation is correct and aligns with the callback handler's expectations. However, the hardcoded scope and other OAuth configuration values could be extracted to named constants at the module top for better maintainability.
🔎 Proposed refactor
+const LINEAR_OAUTH_SCOPE = "read,write,issues:create"; +const LINEAR_AUTH_URL = "https://linear.app/oauth/authorize"; + export async function GET(request: Request) { // ... existing code ... const state = Buffer.from( JSON.stringify({ organizationId, userId: session.user.id }), ).toString("base64url"); - const linearAuthUrl = new URL("https://linear.app/oauth/authorize"); + const linearAuthUrl = new URL(LINEAR_AUTH_URL); linearAuthUrl.searchParams.set("client_id", env.LINEAR_CLIENT_ID); linearAuthUrl.searchParams.set( "redirect_uri", `${env.NEXT_PUBLIC_API_URL}/api/integrations/linear/callback`, ); linearAuthUrl.searchParams.set("response_type", "code"); - linearAuthUrl.searchParams.set("scope", "read,write,issues:create"); + linearAuthUrl.searchParams.set("scope", LINEAR_OAUTH_SCOPE); linearAuthUrl.searchParams.set("state", state); return Response.redirect(linearAuthUrl.toString()); }As per coding guidelines: "Extract magic numbers and hardcoded values to named constants at module top."
apps/admin/src/trpc/react.tsx (1)
52-57: Add error handling and logging to fetch override.The current fetch override silently passes through all errors without logging or handling authentication failures. While the admin app doesn't require offline-first behavior like the desktop app, better error handling would improve debugging and UX.
Consider following the pattern from the desktop implementation (shown in
apps/desktop/src/main/lib/api-client.ts) which:
- Logs network errors for debugging
- Handles 401/403 responses to clear invalid sessions
- Uses prefixed logging with context
🔎 Proposed improvement with error handling
fetch(url, options) { - return fetch(url, { - ...options, - credentials: "include", - }); + return fetch(url, { + ...options, + credentials: "include", + }) + .then((response) => { + // Log auth errors for debugging (admin app typically redirects via middleware) + if (response.status === 401 || response.status === 403) { + console.log("[trpc/admin] Auth error detected", { status: response.status }); + } + return response; + }) + .catch((error) => { + // Log network errors for debugging + console.log("[trpc/admin] Network error", error); + throw error; + }); },Based on coding guidelines: "Never swallow errors silently - at minimum log them with context" and "Use prefixed console logging with context pattern: [domain/operation] message".
apps/admin/src/components/PostHogUserIdentifier/PostHogUserIdentifier.tsx (1)
10-16: Consider simplifying by using session.user directly.The admin app fetches user data via a separate tRPC query, but the web app implementation uses
session.userdirectly for the same fields (id, email, name). This adds unnecessary complexity and an extra round-trip.Compare with the web app pattern (
apps/web/src/components/PostHogUserIdentifier/PostHogUserIdentifier.tsx):if (session?.user) { posthog.identify(session.user.id, { email: session.user.email, name: session.user.name, }); }Unless the tRPC query provides additional fields not available in
session.user, consider using the session data directly for consistency and simplicity.🔎 Simplified implementation without extra query
export function PostHogUserIdentifier() { const { data: session } = authClient.useSession(); - const trpc = useTRPC(); - - const { data: user } = useQuery({ - ...trpc.user.me.queryOptions(), - enabled: !!session?.user, - }); useEffect(() => { - if (user) { - posthog.identify(user.id, { email: user.email, name: user.name }); + if (session?.user) { + posthog.identify(session.user.id, { + email: session.user.email, + name: session.user.name, + }); } else if (session === null) { posthog.reset(); } - }, [user, session?.user]); + }, [session]); return null; }apps/desktop/src/renderer/screens/main/components/TasksView/components/OrganizationSwitcher/OrganizationSwitcher.tsx (1)
33-38: Optional: Remove redundant type annotation.The type annotation
(n: string)on line 35 is unnecessary since TypeScript can infer the type fromsplit(" ")which returnsstring[].🔎 Proposed simplification
const initials = activeOrganization.name ?.split(" ") - .map((n: string) => n[0]) + .map((n) => n[0]) .join("") .toUpperCase() .slice(0, 2);apps/web/src/app/(auth)/sign-in/[[...sign-in]]/page.tsx (1)
16-46: Consider extracting shared OAuth sign-in logic to reduce duplication.Both
signInWithGoogleandsignInWithGithubfollow identical patterns. Consider a single helper function:🔎 Proposed refactor
+type OAuthProvider = "google" | "github"; + +const signInWithProvider = async ( + provider: OAuthProvider, + setLoading: (loading: boolean) => void, +) => { + setLoading(true); + setError(null); + + try { + await authClient.signIn.social({ + provider, + callbackURL: env.NEXT_PUBLIC_WEB_URL, + }); + } catch (err: unknown) { + console.error(`[SignIn/${provider}] Sign in failed:`, err); + setError("Failed to sign in. Please try again."); + setLoading(false); + } +}; + -const signInWithGoogle = async () => { - setIsLoadingGoogle(true); - setError(null); - - try { - await authClient.signIn.social({ - provider: "google", - callbackURL: env.NEXT_PUBLIC_WEB_URL, - }); - } catch (err) { - console.error("Sign in failed:", err); - setError("Failed to sign in. Please try again."); - setIsLoadingGoogle(false); - } -}; - -const signInWithGithub = async () => { - setIsLoadingGithub(true); - setError(null); - - try { - await authClient.signIn.social({ - provider: "github", - callbackURL: env.NEXT_PUBLIC_WEB_URL, - }); - } catch (err) { - console.error("Sign in failed:", err); - setError("Failed to sign in. Please try again."); - setIsLoadingGithub(false); - } -}; +const signInWithGoogle = () => signInWithProvider("google", setIsLoadingGoogle); +const signInWithGithub = () => signInWithProvider("github", setIsLoadingGithub);apps/web/src/app/(auth)/sign-up/[[...sign-up]]/page.tsx (1)
16-46: Consider consolidating OAuth logic with sign-in page.This page shares nearly identical OAuth logic with
sign-in/page.tsx. Consider extracting a shared hook or component to reduce maintenance burden.// Example: packages/auth/src/hooks/useOAuthSignIn.ts export function useOAuthSignIn(callbackURL: string) { const [isLoading, setIsLoading] = useState<Record<string, boolean>>({}); const [error, setError] = useState<string | null>(null); const signIn = async (provider: "google" | "github") => { setIsLoading((prev) => ({ ...prev, [provider]: true })); setError(null); try { await authClient.signIn.social({ provider, callbackURL }); } catch (err: unknown) { console.error(`[OAuth/${provider}] Failed:`, err); setError("Authentication failed. Please try again."); setIsLoading((prev) => ({ ...prev, [provider]: false })); } }; return { signIn, isLoading, error }; }apps/web/src/app/(dashboard)/components/Header/Header.tsx (1)
24-27: Missing error handling in sign-out flow.If
authClient.signOut()fails (network error, etc.), the user is still redirected to/sign-in, potentially causing confusion. Consider handling errors gracefully.🔎 Proposed fix with error handling
const handleSignOut = async () => { - await authClient.signOut(); - router.push("/sign-in"); + try { + await authClient.signOut(); + router.push("/sign-in"); + } catch (error) { + console.error("[auth/sign-out] Failed to sign out:", error); + // Still redirect on error since local state may be inconsistent + router.push("/sign-in"); + } };apps/web/src/proxy.ts (1)
16-35: Consider error handling for session retrieval.If
auth.api.getSessionthrows (e.g., database connection issue), the proxy will return a 500 error. Consider wrapping in try-catch and defaulting to treating the user as unauthenticated on error.🔎 Proposed fix
export default async function proxy(req: NextRequest) { - const session = await auth.api.getSession({ - headers: await headers(), - }); + let session = null; + try { + session = await auth.api.getSession({ + headers: await headers(), + }); + } catch (error) { + console.error("[proxy/session] Failed to retrieve session:", error); + } const pathname = req.nextUrl.pathname;apps/api/src/proxy.ts (1)
11-20: CORS for disallowed origins returns empty header instead of blocking.When
originis not inallowedOrigins, the function returns an empty string forAccess-Control-Allow-Origin. This allows the request to proceed but browsers will block the response. Consider whether you want to explicitly reject the request for clarity.The current behavior is functional (browsers will block), but returning early with a 403 for non-allowed cross-origin requests would be more explicit:
// Alternative: explicit rejection (optional) if (origin && !allowedOrigins.includes(origin)) { return new NextResponse(null, { status: 403 }); }apps/desktop/src/main/lib/auth/auth.ts (2)
76-81: Extract magic number to named constant.The 5-minute expiry buffer is a hardcoded value that should be extracted for maintainability.
🔎 Proposed refactor
+const TOKEN_EXPIRY_BUFFER_MS = 5 * 60 * 1000; // 5 minutes + private isExpired(): boolean { if (!this.expiresAt) return true; - // Consider expired 5 minutes before actual expiry for safety - const bufferMs = 5 * 60 * 1000; - return Date.now() > this.expiresAt.getTime() - bufferMs; + return Date.now() > this.expiresAt.getTime() - TOKEN_EXPIRY_BUFFER_MS; }As per coding guidelines, extract magic numbers and hardcoded values to named constants at module top.
148-171: Extract magic number to named constant.The 10-minute state cleanup threshold is a hardcoded value.
🔎 Proposed refactor
+const STATE_EXPIRY_MS = 10 * 60 * 1000; // 10 minutes + async signIn(provider: AuthProvider): Promise<SignInResult> { try { const state = crypto.randomBytes(32).toString("base64url"); stateStore.set(state, Date.now()); - const tenMinutesAgo = Date.now() - 10 * 60 * 1000; + const cutoffTime = Date.now() - STATE_EXPIRY_MS; for (const [s, ts] of stateStore) { - if (ts < tenMinutesAgo) stateStore.delete(s); + if (ts < cutoffTime) stateStore.delete(s); }As per coding guidelines, extract magic numbers and hardcoded values to named constants at module top.
| } from "@superset/ui/sidebar"; | ||
| import { usePathname } from "next/navigation"; | ||
| import { LuChevronRight, LuHouse, LuUsers, LuUserX } from "react-icons/lu"; | ||
| import { LuChevronRight, LuHouse, LuUsers } from "react-icons/lu"; |
There was a problem hiding this comment.
Critical: Removal contradicts reviewer's explicit feedback on soft-delete functionality.
The removal of the "Deleted Users" navigation item and LuUserX icon directly conflicts with the reviewer's documented expectations in the PR objectives. The reviewer explicitly requested:
"Reintroduce soft-delete semantics: preserve
deletedAtfiltering for organizations; restore separate listing of deleted users and arestoreUsermutation (or equivalent)."
Removing this navigation eliminates the UI entry point for:
- Viewing soft-deleted users
- Restoring accidentally deleted users
- Compliance-required user recovery
The reviewer noted that hard deletion is irreversible and prevents user recovery for compliance or user-requested restores. This change should be reverted or the soft-delete functionality should be reimplemented before merging.
</review_comment_end>
Also applies to: 37-48
🤖 Prompt for AI Agents
In @apps/admin/src/app/(dashboard)/components/AppSidebar/AppSidebar.tsx around
line 23, The "Deleted Users" navigation item and LuUserX icon were removed from
AppSidebar, which contradicts the requested soft-delete UX; restore the Deleted
Users nav entry in AppSidebar (re-add the LuUserX import and its menu item) and
ensure it routes to the deleted-users listing component, re-enable the
soft-delete filter by preserving deletedAt filtering for organizations, and wire
the UI to call the existing restoreUser mutation (or reintroduce an equivalent)
so users can be listed as deleted and restored from the sidebar entry; update
any related components/pages to handle deleted users view and restore actions
accordingly.
| const handleSignOut = async () => { | ||
| await authClient.signOut({ | ||
| fetchOptions: { | ||
| onSuccess: () => { | ||
| window.location.href = env.NEXT_PUBLIC_WEB_URL; | ||
| }, | ||
| }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Add error handling to the sign-out flow.
The handleSignOut function lacks error handling. If the sign-out request fails, the user receives no feedback. Per coding guidelines, never swallow errors silently.
🔎 Proposed fix
const handleSignOut = async () => {
- await authClient.signOut({
- fetchOptions: {
- onSuccess: () => {
- window.location.href = env.NEXT_PUBLIC_WEB_URL;
+ try {
+ await authClient.signOut({
+ fetchOptions: {
+ onSuccess: () => {
+ window.location.href = env.NEXT_PUBLIC_WEB_URL;
+ },
+ onError: (ctx) => {
+ console.error("[NavUser/signOut] Sign-out failed:", ctx.error);
+ },
},
- },
- });
+ });
+ } catch (error) {
+ console.error("[NavUser/signOut] Unexpected error:", error);
+ }
};📝 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.
| const handleSignOut = async () => { | |
| await authClient.signOut({ | |
| fetchOptions: { | |
| onSuccess: () => { | |
| window.location.href = env.NEXT_PUBLIC_WEB_URL; | |
| }, | |
| }, | |
| }); | |
| }; | |
| const handleSignOut = async () => { | |
| try { | |
| await authClient.signOut({ | |
| fetchOptions: { | |
| onSuccess: () => { | |
| window.location.href = env.NEXT_PUBLIC_WEB_URL; | |
| }, | |
| onError: (ctx) => { | |
| console.error("[NavUser/signOut] Sign-out failed:", ctx.error); | |
| }, | |
| }, | |
| }); | |
| } catch (error) { | |
| console.error("[NavUser/signOut] Unexpected error:", error); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In
@apps/admin/src/app/(dashboard)/components/AppSidebar/components/NavUser/NavUser.tsx
around lines 42-50, The sign-out flow in handleSignOut swallows failures; wrap
the authClient.signOut call in a try/catch, move the window.location.href
redirect into the successful branch (onSuccess or after awaited call), and in
the catch log the error (use console.error or process logger) and surface user
feedback (e.g., show a toast or set an error state) so failures aren’t silent;
reference handleSignOut and authClient.signOut when making these changes.
| useEffect(() => { | ||
| if (user) { | ||
| posthog.identify(user.id, { email: user.email, name: user.name }); | ||
| } else if (isSignedIn === false) { | ||
| } else if (!session?.user) { | ||
| posthog.reset(); | ||
| } | ||
| }, [user, isSignedIn]); | ||
| }, [user, session?.user]); |
There was a problem hiding this comment.
Incorrect reset condition may trigger during loading state.
The condition !session?.user (Line 21) will be true when session is undefined (loading state), causing posthog.reset() to execute prematurely before the session query completes.
The web app implementation (apps/web/src/components/PostHogUserIdentifier/PostHogUserIdentifier.tsx) uses the correct pattern:
} else if (session === null) {
posthog.reset();
}This only resets when the session is definitively absent, not during the loading state.
🔎 Proposed fix matching web app pattern
useEffect(() => {
if (user) {
posthog.identify(user.id, { email: user.email, name: user.name });
- } else if (!session?.user) {
+ } else if (session === null) {
posthog.reset();
}
-}, [user, session?.user]);
+}, [user, session]);🤖 Prompt for AI Agents
In @apps/admin/src/components/PostHogUserIdentifier/PostHogUserIdentifier.tsx
around lines 18-24, The effect in PostHogUserIdentifier resets PostHog during
the session loading state because it checks `!session?.user`; change the reset
condition to `session === null` so it only resets when the session is
definitively absent, and update the useEffect dependency to include `session`
(e.g., useEffect(..., [user, session])) so the effect re-runs when the session
transitions from loading to resolved.
| if (!session.user.email.endsWith(COMPANY.EMAIL_DOMAIN)) { | ||
| return NextResponse.redirect(new URL(env.NEXT_PUBLIC_WEB_URL)); | ||
| } |
There was a problem hiding this comment.
Add null check for user email.
If session.user.email is null or undefined (possible with some OAuth providers), calling .endsWith() will throw a runtime error.
🔎 Proposed fix
-if (!session.user.email.endsWith(COMPANY.EMAIL_DOMAIN)) {
+if (!session.user.email?.endsWith(COMPANY.EMAIL_DOMAIN)) {
return NextResponse.redirect(new URL(env.NEXT_PUBLIC_WEB_URL));
}🤖 Prompt for AI Agents
In @apps/admin/src/proxy.ts around lines 17-19, The current check calls
session.user.email.endsWith(...) which will throw if session.user.email is
null/undefined; update the guard in the proxy (where session and
session.user.email are used) to first verify the email exists and is a string
(e.g., check session?.user?.email && typeof session.user.email === 'string') and
only then call .endsWith(COMPANY.EMAIL_DOMAIN); if the email is missing or not a
string, treat it as unauthorized and return the same NextResponse.redirect(new
URL(env.NEXT_PUBLIC_WEB_URL)) response so you avoid runtime errors and preserve
behavior.
| case "auth.organizations": { | ||
| const userMemberships = await db.query.members.findMany({ | ||
| where: eq(members.organizationId, organizationId), | ||
| columns: { userId: true }, | ||
| }); | ||
|
|
||
| if (userMemberships.length === 0) { | ||
| return { fragment: "1 = 0", params: [] }; | ||
| } | ||
|
|
||
| const userId = userMemberships[0]?.userId; | ||
| if (!userId) { | ||
| return { fragment: "1 = 0", params: [] }; | ||
| } | ||
|
|
||
| const allUserMemberships = await db.query.members.findMany({ | ||
| where: eq(members.userId, userId), | ||
| columns: { organizationId: true }, | ||
| }); | ||
|
|
||
| case "organizations": | ||
| return build(organizations, organizations.id, organizationId); | ||
| if (allUserMemberships.length === 0) { | ||
| return { fragment: "1 = 0", params: [] }; | ||
| } | ||
|
|
||
| const orgIds = [ | ||
| ...new Set(allUserMemberships.map((m) => m.organizationId)), | ||
| ]; | ||
| const whereExpr = inArray( | ||
| sql`${sql.identifier(organizations.id.name)}`, | ||
| orgIds, | ||
| ); | ||
| const qb = new QueryBuilder(); | ||
| const { sql: query, params } = qb | ||
| .select() | ||
| .from(organizations) | ||
| .where(whereExpr) | ||
| .toSQL(); | ||
| const fragment = query.replace(/^select .* from .* where\s+/i, ""); | ||
| return { fragment, params }; | ||
| } |
There was a problem hiding this comment.
Security issue: User ID should be passed explicitly, not derived from organization membership.
The logic at lines 52-64 picks the first userId from the organization's members and assumes it's the authenticated user. The first member could be anyone in the organization, not necessarily the requester—this could expose all organizations of an arbitrary user.
🔎 Proposed fix: Accept userId as a parameter
export async function buildWhereClause(
tableName: string,
organizationId: string,
+ userId?: string,
): Promise<WhereClause | null> {
switch (tableName) {
// ... other cases ...
case "auth.organizations": {
- const userMemberships = await db.query.members.findMany({
- where: eq(members.organizationId, organizationId),
- columns: { userId: true },
- });
-
- if (userMemberships.length === 0) {
- return { fragment: "1 = 0", params: [] };
- }
-
- const userId = userMemberships[0]?.userId;
if (!userId) {
return { fragment: "1 = 0", params: [] };
}
const allUserMemberships = await db.query.members.findMany({
where: eq(members.userId, userId),
columns: { organizationId: true },
});
// ... rest of the logicThen update the call site to pass the authenticated user's ID from the session.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/api/src/app/api/electric/[...path]/utils.ts around lines 51-90, The
authorization branch for "auth.organizations" incorrectly derives the
authenticated user by querying members for an organization and using the first
members[0].userId; change the logic to accept an explicit userId parameter
(instead of reading members[0].userId) and use that userId when querying
db.query.members.findMany (the query that currently uses members.userId) to
gather all organizationIds, keep the same de-duplication and
QueryBuilder/whereExpr (sql`${sql.identifier(organizations.id.name)}`) logic,
and add an early return when the provided userId is missing or no memberships
exist; also update the call sites to pass the authenticated session userId into
this "auth.organizations" check.
| } catch { | ||
| // On error checking status, show dialog for user to decide | ||
| setShowDeleteDialog(true); | ||
| } |
There was a problem hiding this comment.
Log the error before showing the fallback dialog.
The catch block swallows errors silently. Per coding guidelines, errors should be logged with context for debugging.
Proposed fix
- } catch {
+ } catch (error) {
+ console.error("[WorkspaceItem/handleDeleteClick] Failed to check delete status:", error);
// On error checking status, show dialog for user to decide
setShowDeleteDialog(true);
}📝 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.
| } catch { | |
| // On error checking status, show dialog for user to decide | |
| setShowDeleteDialog(true); | |
| } | |
| } catch (error) { | |
| console.error("[WorkspaceItem/handleDeleteClick] Failed to check delete status:", error); | |
| // On error checking status, show dialog for user to decide | |
| setShowDeleteDialog(true); | |
| } |
🤖 Prompt for AI Agents
In
@apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx
around lines 120-123, The catch block in WorkspaceItem.tsx currently swallows
errors; update it to catch the error (e.g., catch (err)) and log it with context
before falling back to showing the dialog — for example call a logging facility
or console.error like console.error('WorkspaceItem: error checking status',
err), then call setShowDeleteDialog(true); ensure you reference the existing
setShowDeleteDialog usage and keep the fallback behavior unchanged.
| } catch (err) { | ||
| console.error("Sign up failed:", err); | ||
| setError("Failed to sign up. Please try again."); | ||
| } finally { | ||
| setIsLoadingGoogle(false); | ||
| } |
There was a problem hiding this comment.
Add type annotation to error parameters in catch blocks.
The error parameters lack type annotations. Use catch (err: unknown) for TypeScript strict mode compliance, then narrow the type when logging.
🔎 Proposed fix
- } catch (err) {
- console.error("Sign up failed:", err);
+ } catch (err: unknown) {
+ console.error("[SignUp/google] Sign up failed:", err);
setError("Failed to sign up. Please try again.");
setIsLoadingGoogle(false);
}Apply similarly to the GitHub handler with context [SignUp/github].
Also applies to: 41-45
🤖 Prompt for AI Agents
In @apps/web/src/app/(auth)/sign-up/[[...sign-up]]/page.tsx around lines 25-29,
The catch blocks for the sign-up handlers need explicit type annotations: change
catch (err) to catch (err: unknown) in the Google sign-up handler shown and in
the GitHub sign-up handler ([SignUp/github]); then narrow before logging by
doing if (err instanceof Error) { console.error("Sign up failed:", err.message,
err); } else { console.error("Sign up failed:", String(err)); } and keep
setError / setIsLoading calls unchanged.
| } catch (error) { | ||
| console.error("Failed to get session for desktop auth:", error); | ||
| return ( | ||
| <div className="flex min-h-screen flex-col items-center justify-center bg-background p-4"> | ||
| <p className="text-xl text-muted-foreground">Authentication failed</p> | ||
| <p className="text-muted-foreground/70"> | ||
| Please try signing in again from the desktop app. | ||
| </p> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Network error vs authentication failure distinction still missing.
The catch block treats all errors as authentication failures. Per PR objectives, the previous implementation distinguished network errors from invalid tokens to preserve sessions during transient failures. The current implementation shows "Authentication failed" even for temporary network issues, which breaks offline-first behavior for the desktop app.
Users on unstable connections would be forced to re-authenticate repeatedly, and the desktop app cannot function offline or on flaky networks.
| const publicRoutes = [ | ||
| "/sign-in", | ||
| "/sign-up", | ||
| "/auth/desktop", | ||
| "/api/auth/desktop", | ||
| ]; | ||
|
|
||
| function isPublicRoute(pathname: string): boolean { | ||
| return publicRoutes.some((route) => pathname.startsWith(route)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find Better Auth configuration and route files
find . -type f -name "*.ts" -o -name "*.tsx" | xargs grep -l "better.auth\|betterauth\|@better-auth" | head -20Repository: superset-sh/superset
Length of output: 168
🏁 Script executed:
# Look at the full proxy.ts file to understand the context
cat -n apps/web/src/proxy.tsRepository: superset-sh/superset
Length of output: 1389
🏁 Script executed:
# Search for API route handlers
fd -t f "route.ts" apps/Repository: superset-sh/superset
Length of output: 603
🏁 Script executed:
# Check if there's a better-auth configuration file
fd -t f "auth" apps/ | grep -E "config|setup|middleware" | head -20Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
cat -n apps/api/src/app/api/auth/[...all]/route.tsRepository: superset-sh/superset
Length of output: 238
🏁 Script executed:
# Check the auth package to understand Better Auth setup
cat -n packages/auth/src/index.tsRepository: superset-sh/superset
Length of output: 2810
🏁 Script executed:
# Look for any Better Auth configuration comments or setup
rg -A 5 -B 5 "auth.handler" apps/Repository: superset-sh/superset
Length of output: 46
Better Auth API routes need public access in proxy configuration.
The /api/auth/* endpoints from Better Auth handle authentication flows (sign-up, sign-in, OAuth callbacks, session validation) and must be accessible without a session. Currently only /api/auth/desktop is listed, which blocks standard Better Auth endpoints.
Proposed fix
const publicRoutes = [
"/sign-in",
"/sign-up",
"/auth/desktop",
- "/api/auth/desktop",
+ "/api/auth",
];📝 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.
| const publicRoutes = [ | |
| "/sign-in", | |
| "/sign-up", | |
| "/auth/desktop", | |
| "/api/auth/desktop", | |
| ]; | |
| function isPublicRoute(pathname: string): boolean { | |
| return publicRoutes.some((route) => pathname.startsWith(route)); | |
| } | |
| const publicRoutes = [ | |
| "/sign-in", | |
| "/sign-up", | |
| "/auth/desktop", | |
| "/api/auth", | |
| ]; | |
| function isPublicRoute(pathname: string): boolean { | |
| return publicRoutes.some((route) => pathname.startsWith(route)); | |
| } |
🤖 Prompt for AI Agents
In @apps/web/src/proxy.ts around lines 5-14, The proxy's publicRoutes list only
includes "/api/auth/desktop", blocking other Better Auth endpoints; update the
publicRoutes array (used by isPublicRoute) to include a wildcard-like prefix
such as "/api/auth" or "/api/auth/" so isPublicRoute(pathname) returns true for
all Better Auth API paths (e.g., add "/api/auth" to publicRoutes) ensuring all
/api/auth/* endpoints are treated as public.
… web Fixes desktop app using web browser cookies instead of its own bearer token, which caused session conflicts between desktop and web apps. **Problem:** - Desktop OAuth created separate bearer token session ✓ - BUT Electron's default session shared cookies with system browser ✗ - Desktop ended up using web cookies, causing: - Logout on web → desktop logs out immediately - Org changes sync between apps - API auth conflicts and errors **Solution:** 1. Add `partition: "persist:superset"` to Electron webPreferences - Isolates desktop session from system browser cookies - Forces desktop to use bearer token for all requests 2. Create truly separate desktop session during OAuth callback - Generate unique token for desktop (not reusing browser session token) - Store in separate database session record 3. Set activeOrganizationId on session creation - User creation hook: Update sessions after org is created - Session creation hook: Set initial org for existing users **Result:** - Desktop uses bearer token exclusively (no cookie fallback) - Web uses HTTP-only cookies - Independent sessions with separate activeOrganizationId - Logout/org changes isolated between desktop and web 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Better Auth uses BETTER_AUTH_SECRET, not DESKTOP_AUTH_SECRET. The DESKTOP_AUTH_SECRET was a legacy name that is no longer used in the codebase. Changes: - Removed DESKTOP_AUTH_SECRET env var from deploy-production.yml - Removed DESKTOP_AUTH_SECRET env var from deploy-preview.yml - BETTER_AUTH_SECRET is the only auth secret needed 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/windows/main.ts (1)
34-57: Window state persistence removed—document the UX tradeoff or reintroduce it.The window state persistence feature (size, position, maximized state) was removed in the session isolation commit. Users will lose their window layout on every restart. While this simplifies the code and the session isolation is necessary for the auth fix, the UX impact should be explicitly acknowledged.
Either:
- Reintroduce window state persistence with the new session partition setup, or
- If this removal is intentional, document the decision and ensure the default window size (primary display work area) provides acceptable UX across different display configurations.
🤖 Fix all issues with AI Agents
In @apps/desktop/src/main/windows/main.ts:
- Line 32: Wrap the call to screen.getPrimaryDisplay().workAreaSize in a
try/catch inside main.ts so the app doesn't crash if the screen API throws; on
error log the exception (or use processLogger) and fall back to safe default
dimensions (e.g., width=1024, height=768) and continue startup, replacing the
direct destructuring of { width, height } with values from the try block or the
fallback.
In @apps/web/src/app/auth/desktop/success/page.tsx:
- Around line 70-79: Wrap the db.insert(sessions).values(...) call in a
try-catch to handle DB errors gracefully: catch any error thrown by
db.insert(sessions), log it with contextual info (e.g., token, session.user.id,
session.session.activeOrganizationId) using the existing logger or
console.error, and then return or throw a controlled error/user-friendly
response instead of letting Next.js surface a generic crash; update the block
around the insert in page.tsx (the await db.insert(sessions).values({...})
statement) to perform this try-catch and logging.
🧹 Nitpick comments (2)
apps/web/src/app/auth/desktop/success/page.tsx (1)
66-68: Extract session expiry duration to a shared constant.The 30-day expiry (
60 * 60 * 24 * 30 * 1000) is duplicated here and inpackages/auth/src/index.ts(line 25:expiresIn: 60 * 60 * 24 * 30). If the auth config changes, this value could drift out of sync.Consider exporting the session duration from
@superset/authand importing it here.🔎 Suggested approach
In
packages/auth/src/index.ts:export const SESSION_EXPIRY_SECONDS = 60 * 60 * 24 * 30; // 30 daysThen in this file:
+import { SESSION_EXPIRY_SECONDS } from "@superset/auth"; + +const SESSION_EXPIRY_MS = SESSION_EXPIRY_SECONDS * 1000; + // ... const expiresAt = new Date( - Date.now() + 60 * 60 * 24 * 30 * 1000, // 30 days (matching auth config) + Date.now() + SESSION_EXPIRY_MS, );apps/desktop/src/main/windows/main.ts (1)
53-55: Extract partition name to a named constant.Per coding guidelines, hardcoded string values should be extracted to named constants at the module top for maintainability.
🔎 Proposed refactor
At the top of the file (after imports):
+// Electron session partition for desktop auth isolation +const DESKTOP_SESSION_PARTITION = "persist:superset"; + // Singleton IPC handler to prevent duplicate handlers on window reopen (macOS) let ipcHandler: ReturnType<typeof createIPCHandler> | null = null;Then update the usage:
webPreferences: { preload: join(__dirname, "../preload/index.js"), webviewTag: true, // Isolate Electron session from system browser cookies // This ensures desktop uses bearer token auth, not web cookies - partition: "persist:superset", + partition: DESKTOP_SESSION_PARTITION, },
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src/main/windows/main.tsapps/web/src/app/auth/desktop/success/page.tsxpackages/auth/src/index.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/web/src/app/auth/desktop/success/page.tsxapps/desktop/src/main/windows/main.ts
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
One component per file - do not create multi-component files
Files:
apps/web/src/app/auth/desktop/success/page.tsx
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/web/src/app/auth/desktop/success/page.tsxapps/desktop/src/main/windows/main.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/web/src/app/auth/desktop/success/page.tsxapps/desktop/src/main/windows/main.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/main/windows/main.ts
🧠 Learnings (8)
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to **/*.{ts,tsx} : Never swallow errors silently - at minimum log them with context
Applied to files:
apps/web/src/app/auth/desktop/success/page.tsx
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
Applied to files:
apps/desktop/src/main/windows/main.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/lib/electron-router-dom.ts : Do not import Node.js modules like node:path or dotenv in electron-router-dom.ts and similar shared files - they run in both main and renderer processes
Applied to files:
apps/desktop/src/main/windows/main.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Applied to files:
apps/desktop/src/main/windows/main.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/lib/*.ts : Never import Node.js modules in shared code like electron-router-dom.ts - it runs in both main and renderer processes
Applied to files:
apps/desktop/src/main/windows/main.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/main/windows/main.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/shared/ipc-channels.ts : Define IPC channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Applied to files:
apps/desktop/src/main/windows/main.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables from monorepo root .env in desktop app with override: true before any imports in src/main/index.ts and electron.vite.config.ts
Applied to files:
apps/desktop/src/main/windows/main.ts
🧬 Code graph analysis (1)
apps/web/src/app/auth/desktop/success/page.tsx (1)
packages/auth/src/index.ts (1)
auth(11-101)
⏰ 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 Docs
- GitHub Check: Deploy API
- GitHub Check: Deploy Web
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Admin
- GitHub Check: Build
🔇 Additional comments (6)
apps/web/src/app/auth/desktop/success/page.tsx (4)
1-6: LGTM!Imports are well-organized, using concrete file imports and the shared auth/db packages appropriately.
8-24: LGTM!Good use of early return pattern for validation. The typed
searchParamsasPromiseis correct for Next.js 15 server components.
80-88: LGTM!URL construction is secure with proper
encodeURIComponentusage on all parameters. The protocol switching between development and production environments is handled correctly.
30-40: Network error vs authentication failure handling remains unaddressed.The catch block treats all errors uniformly as authentication failures. Per PR objectives, this regression was explicitly flagged: the previous implementation distinguished network errors from invalid tokens to preserve sessions during transient network failures.
This breaks offline-first functionality—users on unstable connections would be forced to re-authenticate repeatedly.
Likely an incorrect or invalid review comment.
apps/desktop/src/main/windows/main.ts (2)
5-5: LGTM!The
screenimport is necessary for the new dynamic window sizing functionality.
166-175: LGTM!The close handler properly tears down the notifications server, clears listeners, and detaches the IPC handler to prevent resource leaks and duplicate registrations on window reopen (macOS). The removal of window state saving is consistent with the broader changes to eliminate the window-state module.
| export async function MainWindow() { | ||
| const savedWindowState = loadWindowState(); | ||
| const initialBounds = getInitialWindowBounds(savedWindowState); | ||
| const { width, height } = screen.getPrimaryDisplay().workAreaSize; |
There was a problem hiding this comment.
Add error handling for screen API to prevent startup crashes.
screen.getPrimaryDisplay() can throw in edge cases (no display available, headless systems, display disconnection). Without error handling, this will crash the app on startup with no recovery path.
🔎 Proposed fix with fallback dimensions
+// Fallback dimensions if screen API fails
+const DEFAULT_WINDOW_WIDTH = 1200;
+const DEFAULT_WINDOW_HEIGHT = 800;
+
export async function MainWindow() {
- const { width, height } = screen.getPrimaryDisplay().workAreaSize;
+ let width = DEFAULT_WINDOW_WIDTH;
+ let height = DEFAULT_WINDOW_HEIGHT;
+
+ try {
+ const workAreaSize = screen.getPrimaryDisplay().workAreaSize;
+ width = workAreaSize.width;
+ height = workAreaSize.height;
+ } catch (error) {
+ console.error("[window/main] Failed to get primary display, using defaults:", error);
+ }📝 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.
| const { width, height } = screen.getPrimaryDisplay().workAreaSize; | |
| // Fallback dimensions if screen API fails | |
| const DEFAULT_WINDOW_WIDTH = 1200; | |
| const DEFAULT_WINDOW_HEIGHT = 800; | |
| export async function MainWindow() { | |
| let width = DEFAULT_WINDOW_WIDTH; | |
| let height = DEFAULT_WINDOW_HEIGHT; | |
| try { | |
| const workAreaSize = screen.getPrimaryDisplay().workAreaSize; | |
| width = workAreaSize.width; | |
| height = workAreaSize.height; | |
| } catch (error) { | |
| console.error("[window/main] Failed to get primary display, using defaults:", error); | |
| } |
🤖 Prompt for AI Agents
In @apps/desktop/src/main/windows/main.ts around line 32, Wrap the call to
screen.getPrimaryDisplay().workAreaSize in a try/catch inside main.ts so the app
doesn't crash if the screen API throws; on error log the exception (or use
processLogger) and fall back to safe default dimensions (e.g., width=1024,
height=768) and continue startup, replacing the direct destructuring of { width,
height } with values from the try block or the fallback.
| // Create a new session record in the database | ||
| await db.insert(sessions).values({ | ||
| token, | ||
| userId: session.user.id, | ||
| expiresAt, | ||
| ipAddress, | ||
| userAgent, | ||
| activeOrganizationId: session.session.activeOrganizationId, | ||
| updatedAt: now, | ||
| }); |
There was a problem hiding this comment.
Wrap database insert in try-catch for graceful error handling.
If db.insert(sessions) fails (connection issues, constraint violations, etc.), the page will crash with a generic Next.js error instead of showing a user-friendly message. Based on learnings, errors should never be swallowed silently and should be logged with context.
🔎 Suggested fix
// Create a new session record in the database
+ try {
await db.insert(sessions).values({
token,
userId: session.user.id,
expiresAt,
ipAddress,
userAgent,
activeOrganizationId: session.session.activeOrganizationId,
updatedAt: now,
});
+ } catch (error) {
+ console.error("[auth/desktop] Failed to create desktop session:", error);
+ return (
+ <div className="flex min-h-screen flex-col items-center justify-center bg-background p-4">
+ <p className="text-xl text-muted-foreground">Failed to create session</p>
+ <p className="text-muted-foreground/70">
+ Please try signing in again from the desktop app.
+ </p>
+ </div>
+ );
+ }🤖 Prompt for AI Agents
In @apps/web/src/app/auth/desktop/success/page.tsx around lines 70-79, Wrap the
db.insert(sessions).values(...) call in a try-catch to handle DB errors
gracefully: catch any error thrown by db.insert(sessions), log it with
contextual info (e.g., token, session.user.id,
session.session.activeOrganizationId) using the existing logger or
console.error, and then return or throw a controlled error/user-friendly
response instead of letting Next.js surface a generic crash; update the block
around the insert in page.tsx (the await db.insert(sessions).values({...})
statement) to perform this try-catch and logging.
This reverts commit 9ee8911. Reason: The Better Auth migration had unintended side effects: - Desktop app required database dependencies it doesn't need - Many desktop UI files (sidebar, workspace views) were accidentally deleted - Local-db migrations were removed that were already deployed to users We're reverting to unblock users and will re-implement Better Auth properly in a focused PR that only changes the auth implementation. Additional changes: - Reduced minimum desktop version from 0.0.44 to 0.0.39 to unblock existing users
This reverts commit 9ee8911. Reason: The Better Auth migration had unintended side effects: - Desktop app required database dependencies it doesn't need - Many desktop UI files (sidebar, workspace views) were accidentally deleted - Local-db migrations were removed that were already deployed to users We're reverting to unblock users and will re-implement Better Auth properly in a focused PR that only changes the auth implementation. Additional changes: - Reduced minimum desktop version from 0.0.44 to 0.0.39 to unblock existing users
This reverts commit 9ee8911. Reason: The Better Auth migration had unintended side effects: - Desktop app required database dependencies it doesn't need - Many desktop UI files (sidebar, workspace views) were accidentally deleted - Local-db migrations were removed that were already deployed to users We're reverting to unblock users and will re-implement Better Auth properly in a focused PR that only changes the auth implementation. Additional changes: - Reduced minimum desktop version from 0.0.44 to 0.0.39 to unblock existing users
Summary
authPostgreSQL schema with UUID columnsChanges
Auth System
Database
authschema with tables:users,sessions,accounts,verifications,organizations,members,invitationsgenerateId: false)Desktop App
Cleanup
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Removed
✏️ Tip: You can customize this high-level summary in your review settings.