Port Clerk to Better Auth Attempt #2 #621
Conversation
📝 WalkthroughWalkthroughReplaces Clerk with Better Auth across repo: environment vars, workflows, dependencies, DB schema and migrations, server/client auth clients, middleware/proxy, TRPC context, desktop OAuth flows, and numerous UI/provider/components to use session-based auth and new auth tables (auth.*). Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser / Web App
participant AuthClient as authClient (browser)
participant AuthAPI as /api/auth/[...all]
participant BetterAuth as Better Auth Server
participant Database as Database
Browser->>AuthClient: signIn.social(provider)
AuthClient->>AuthAPI: request social sign-in
AuthAPI->>BetterAuth: initiate OAuth flow / redirect
Browser->>BetterAuth: OAuth provider callback
BetterAuth->>Database: create/update user + session
BetterAuth-->>Browser: set cookie / redirect back
Browser->>AuthClient: useSession() subscribes
AuthClient-->>Browser: session state updated
sequenceDiagram
participant Client as Client (web/admin)
participant Middleware as Proxy/Middleware
participant AuthAPI as auth.api
participant Database as Database
Client->>Middleware: request with cookies/headers
Middleware->>AuthAPI: getSession(headers)
AuthAPI->>Database: read session & user
Database-->>AuthAPI: session + user
AuthAPI-->>Middleware: session object
alt session present & allowed
Middleware->>Client: forward request (NextResponse.next)
else unauthorized
Middleware-->>Client: redirect to /sign-in
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom Pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
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 (7)
apps/desktop/src/renderer/screens/sign-in/index.tsx (1)
11-20: Add error handling and loading state for authentication.The sign-in mutation lacks error handling and loading state management, resulting in poor user experience:
- No error feedback: If authentication fails, users receive no indication of what went wrong
- No loading state: Users don't know when authentication is in progress
- No button disabling: Users could potentially trigger multiple authentication attempts
🔎 Proposed enhancement with error handling and loading state
export function SignInScreen() { - const signInMutation = trpc.auth.signIn.useMutation(); + const signInMutation = trpc.auth.signIn.useMutation({ + onError: (error) => { + console.error('[auth/sign-in] Authentication failed:', error); + // Consider adding toast notification or error message display + }, + }); useEffect(() => { posthog.capture("desktop_opened"); }, []); const signIn = (provider: AuthProvider) => { posthog.capture("auth_started", { provider }); signInMutation.mutate({ provider }); }; + + const isLoading = signInMutation.isPending; return ( <div className="flex flex-col h-full w-full bg-background"> <div className="h-12 w-full drag shrink-0" /> <div className="flex flex-1 items-center justify-center"> <div className="flex flex-col items-center w-full max-w-md px-8"> <div className="mb-8"> <SupersetLogo className="h-12 w-auto" /> </div> <div className="text-center mb-8"> <h1 className="text-xl font-semibold text-foreground mb-2"> Welcome to Superset </h1> <p className="text-sm text-muted-foreground"> Sign in to get started </p> </div> <div className="flex flex-col gap-3 w-full max-w-xs"> <Button variant="outline" size="lg" onClick={() => signIn("github")} className="w-full gap-3" + disabled={isLoading} > <FaGithub className="size-5" /> - Continue with GitHub + {isLoading ? "Signing in..." : "Continue with GitHub"} </Button> <Button variant="outline" size="lg" onClick={() => signIn("google")} className="w-full gap-3" + disabled={isLoading} > <FcGoogle className="size-5" /> - Continue with Google + {isLoading ? "Signing in..." : "Continue with Google"} </Button> </div> + + {signInMutation.isError && ( + <p className="mt-4 text-sm text-destructive"> + Authentication failed. Please try again. + </p> + )} <p className="mt-8 text-xs text-muted-foreground/70 text-center max-w-xs">packages/trpc/src/router/analytics/analytics.ts (1)
24-30: Breaking API change: Field rename requires coordination.The
LeaderboardEntryinterface changedavatarUrltoimage, which is a breaking change for external API consumers. While internal consistency is maintained (admin UI components updated accordingly), external clients will break.Consider:
- Documenting this breaking change in release notes
- Providing a migration guide for API consumers
- Evaluating whether to support both fields temporarily with a deprecation notice
.github/workflows/deploy-preview.yml (1)
173-221: Ensure all required GitHub secrets are configured for deployment.The workflow references 35 secrets across all deployment jobs (including BETTER_AUTH_SECRET, VERCEL_TOKEN, DATABASE credentials, API keys, etc.). Missing or misconfigured secrets will cause deployment failures. Verify all referenced secrets are properly configured in the repository settings before merging.
packages/trpc/src/router/organization/organization.ts (4)
79-95: Missing authorization check in update mutation.The
updatemutation doesn't verify that the caller is a member (or admin/owner) of the organization before allowing updates. This could allow any authenticated user to modify any organization.🔎 Proposed fix
update: protectedProcedure .input( z.object({ - id: z.string(), + id: z.string().uuid(), name: z.string().min(1).optional(), logo: z.string().url().optional(), }), ) .mutation(async ({ ctx, input }) => { + // Verify caller has permission to update + const membership = await db.query.members.findFirst({ + where: and( + eq(members.organizationId, input.id), + eq(members.userId, ctx.session.user.id), + ), + }); + if (!membership || (membership.role !== "admin" && membership.role !== "owner")) { + throw new Error("Admin access required to update organization"); + } + const { id, ...data } = input;
97-102: Missing authorization check in delete mutation.The
deletemutation allows any authenticated user to delete any organization. It should verify that the caller is an owner of the organization.🔎 Proposed fix
delete: protectedProcedure .input(z.string().uuid()) - .mutation(async ({ input }) => { + .mutation(async ({ ctx, input }) => { + // Verify caller is an owner + const membership = await db.query.members.findFirst({ + where: and( + eq(members.organizationId, input), + eq(members.userId, ctx.session.user.id), + ), + }); + if (!membership || membership.role !== "owner") { + throw new Error("Only owners can delete organizations"); + } + await db.delete(organizations).where(eq(organizations.id, input)); return { success: true }; }),
104-121: Missing authorization check in addMember mutation.The
addMembermutation should verify that the caller has admin/owner privileges on the organization before allowing them to add members.🔎 Proposed fix
addMember: protectedProcedure .input( z.object({ organizationId: z.string().uuid(), userId: z.string().uuid(), }), ) - .mutation(async ({ input }) => { + .mutation(async ({ ctx, input }) => { + // Verify caller has admin/owner access + const callerMembership = await db.query.members.findFirst({ + where: and( + eq(members.organizationId, input.organizationId), + eq(members.userId, ctx.session.user.id), + ), + }); + if (!callerMembership || (callerMembership.role !== "admin" && callerMembership.role !== "owner")) { + throw new Error("Admin access required to add members"); + } + const [member] = await db
123-140: Missing authorization check in removeMember mutation.The
removeMembermutation should verify that the caller has admin/owner privileges (or is removing themselves) before allowing member removal.🔎 Proposed fix
removeMember: protectedProcedure .input( z.object({ organizationId: z.string().uuid(), userId: z.string().uuid(), }), ) - .mutation(async ({ input }) => { + .mutation(async ({ ctx, input }) => { + // Allow self-removal or require admin/owner access + const isSelfRemoval = input.userId === ctx.session.user.id; + if (!isSelfRemoval) { + const callerMembership = await db.query.members.findFirst({ + where: and( + eq(members.organizationId, input.organizationId), + eq(members.userId, ctx.session.user.id), + ), + }); + if (!callerMembership || (callerMembership.role !== "admin" && callerMembership.role !== "owner")) { + throw new Error("Admin access required to remove other members"); + } + } + await db
🤖 Fix all issues with AI Agents
In @apps/admin/package.json:
- Line 15: Update the better-auth dependency in package.json for the
@superset/auth entry: replace the non-existent ^1.4.9 version with a resolvable
release such as ^1.4.7 (or the intended published version), and ensure any other
occurrences of better-auth in the repo are updated consistently; also review the
linked security advisories and changelog to determine whether additional patches
or a different safe version are required before promoting to production.
In
@apps/admin/src/app/(dashboard)/components/AppSidebar/components/NavUser/NavUser.tsx:
- Around line 42-50: handleSignOut currently calls authClient.signOut() without
handling rejections; update handleSignOut to catch errors from
authClient.signOut (or wrap in try/catch) and on success keep the existing
redirect to env.NEXT_PUBLIC_WEB_URL, but on failure log the error with the
prefixed pattern (e.g. "[auth/signOut] <message>") using console.error and
provide minimal user feedback (e.g. set an error state or call an existing
notification/toast helper) so the user is informed and the error isn’t
swallowed; reference the handleSignOut function and authClient.signOut call when
making the changes.
In @apps/admin/src/components/PostHogUserIdentifier/PostHogUserIdentifier.tsx:
- Around line 18-24: The effect in PostHogUserIdentifier leaves a gap when
session?.user is truthy but user is falsy and also uses a different reset check
than the web version; update the useEffect (in PostHogUserIdentifier) to
explicitly handle all states: if (user) call posthog.identify(user.id, { email:
user.email, name: user.name }); else if (session === null) call posthog.reset();
otherwise do nothing (covers loading/failed/null query states), and adjust the
dependency from session?.user to session so the reset uses session === null like
the web version.
In @apps/admin/src/proxy.ts:
- Around line 13-19: The redirect check calls session.user.email.endsWith which
can throw if email is null/undefined; update the guard to first ensure an email
exists before calling endsWith (e.g., change the second condition to check
!session?.user?.email || !session.user.email.endsWith(COMPANY.EMAIL_DOMAIN) and
keep using NextResponse.redirect with the same URL), referencing the existing
symbols session, session.user.email, COMPANY.EMAIL_DOMAIN and
NextResponse.redirect.
In @apps/api/package.json:
- Around line 17-25: The dependency entry "better-auth" in package.json must be
verified to include the CVE-2025-61928 fix (fixed in >=1.3.26); check the
better-auth release/changelog or npm package metadata to confirm v1.4.9 contains
the patch and, to be safe, update the version constraint for the "better-auth"
dependency to a known-good release (for example ^1.4.10 or >=1.3.26) in
package.json, run your package manager to update the lockfile, and re-run
security audits (npm audit / yarn audit / Snyk) to ensure CVE-2025-61928 is
resolved in the installed tree and that automatic patching via the caret range
is acceptable for your deployment; if you cannot upgrade, add a
resolutions/overrides entry to force a fixed version for "better-auth".
In @apps/api/src/app/api/auth/desktop/connect/route.ts:
- Around line 6-17: The GET route manually checks URLSearchParams instead of
using a Zod schema; create a Zod schema (e.g., const ConnectQuery = z.object({
provider: z.enum(["google","github"]), state: z.string().nonempty() })) and use
ConnectQuery.safeParse or .parse to validate the incoming query values (from new
URL(request.url).searchParams) in the GET function, returning a 400 Response
with the schema error message when validation fails and proceeding with the
validated values when it succeeds.
- Around line 30-37: The code unsafely asserts the external auth API response
into body via (await result.json()) as { url?: string; redirect?: boolean }; add
runtime validation using a Zod schema (e.g., AuthInitSchema) to parse the JSON
from result.json() before using body.url; if parsing fails or the schema
indicates no url, return the 500 Response with the validated error details.
Ensure you reference the parsed/validated object (replace body with the parsed
result) and keep the existing cookies handling intact.
In @apps/api/src/app/api/electric/[...path]/utils.ts:
- Around line 51-90: The auth.organizations branch should use the authenticated
session userId instead of arbitrarily picking the first member from
db.query.members.findMany; change the logic in the auth.organizations case to
require a passed-in userId (from the session), validate the user is a member of
the provided organizationId (e.g., query members with where:
eq(members.organizationId, organizationId) AND eq(members.userId, userId)), then
fetch all organizations for that authenticated user via
db.query.members.findMany where members.userId = userId, build orgIds and the
whereExpr with inArray/sql/organizations.id as before, and update callers to
pass the session userId into this utility.
In @apps/api/src/trpc/context.ts:
- Around line 4-14: The createContext function currently declares an unused
resHeaders parameter; remove resHeaders from the destructured parameters and
from the function type so the signature becomes only ({ req }: { req: Request })
=> Promise<...>, keeping the existing body that calls auth.api.getSession and
returns createTRPCContext({ session }); update the parameter annotation where
createContext is defined to drop resHeaders and ensure any callers still pass
the same object shape (only req is required).
In @apps/desktop/src/main/lib/auth/auth.ts:
- Around line 205-216: The signOut method currently swallows all errors from
fs.unlink; change the empty catch to catch the error (e.g., catch (err)) and
only ignore not-found errors (err.code === 'ENOENT'), while logging other
failures (permissions/I/O) with a clear message that includes TOKEN_FILE and the
error (use console.error or the module logger). Keep the rest of signOut
(clearing this.token/this.expiresAt/this.session, calling this.getState() and
this.emit("state-changed", state)) unchanged.
- Around line 70-73: The empty catch in Auth initialization silently swallows
errors; update the catch to capture the exception (e.g., catch (err)) and if it
is a "file not found" / ENOENT case still set this.token and this.expiresAt to
null, but for any other error log the full error details (using the existing
logger or console.error) with context like "Failed to load/decrypt token" so
problems in auth.ts (where this.token / this.expiresAt are set) are visible for
debugging.
In
@apps/desktop/src/renderer/contexts/CollectionsProvider/CollectionsProvider.tsx:
- Around line 23-43: The current useMemo block in CollectionsProvider calls
setError during render (inside collections calculation), which violates React
rules; remove setError from the useMemo error catch (leave useMemo to only
return the created value or null from createCollections) and add a useEffect
that watches [token, activeOrgId, collections] to call setError when token &&
activeOrgId but collections is null (set a proper Error instance), and clear
error (setError(null)) when collections is successfully created; keep
createCollections and the console logs inside useMemo but ensure all state
updates happen in the new useEffect.
In
@apps/desktop/src/renderer/screens/main/components/TasksView/components/OrganizationSwitcher/OrganizationSwitcher.tsx:
- Around line 40-42: switchOrganization currently awaits
setActiveOrg.mutateAsync without handling failures; wrap the call in a try/catch
around the setActiveOrg.mutateAsync({ organizationId: newOrgId }) invocation
inside the switchOrganization function, log the caught error with context
(including newOrgId and function name) and surface user-visible feedback (e.g.,
call the existing toast/error UI or set an error state) so failures are not
silently swallowed. Ensure the catch block also preserves/rethrows the error
when appropriate for upstream handling.
In @apps/desktop/src/renderer/screens/main/components/TasksView/TasksView.tsx:
- Around line 347-355: The TasksView component currently wraps TasksViewContent
in an unused OrganizationsProvider; remove that wrapper so TasksView only
returns <CollectionsProvider><TasksViewContent /></CollectionsProvider>, and
also remove any now-unused OrganizationsProvider import if present; ensure the
exported function TasksView and its JSX reflect this simplified hierarchy.
In
@apps/web/src/app/auth/desktop/success/components/DesktopRedirect/DesktopRedirect.tsx:
- Around line 7-10: DesktopRedirect directly assigns window.location.href to the
incoming url prop which can allow navigation to malformed or unsafe URLs;
validate the url before navigating by parsing it (e.g., new URL(url,
window.location.origin) or a whitelist check) inside the useEffect in
DesktopRedirect and only set window.location.href when the parsed URL is valid
and uses an allowed protocol (http/https) and origin rules; if validation fails,
avoid navigation and optionally call a fallback handler or log an error.
In @apps/web/src/app/auth/desktop/success/page.tsx:
- Around line 15-50: Several identical error UI blocks are duplicated in this
file; extract them into a single reusable component or function (e.g.,
AuthErrorPanel or renderAuthError) and replace each duplicated block (the three
returns that render the "Missing auth state" / "Authentication failed" panels
shown around the checks for state, the catch for auth.api.getSession, and the
!session branch) with a call to that helper. The helper should accept the
variable parts (title and optional description) and render the same JSX
structure currently duplicated, then import/use it in this page so the three
return sites simply return the helper component instead of repeating markup.
- Around line 71-79: The db.insert(sessions).values(...) call is not guarded
against failures; wrap this insertion in a try-catch around the code that builds
token/expiresAt and calls db.insert(sessions).values to catch DB errors (e.g.,
unique constraint or connection issues), log the error (include the error
object) and handle it by returning a graceful response/redirect or rendering an
error message instead of letting the exception bubble; ensure you still return
or redirect with a safe fallback (e.g., treat session creation as non-blocking
or redirect to an error/reauth page) and reference the same symbols:
db.insert(sessions).values, token, session.user.id, expiresAt, ipAddress,
userAgent, activeOrganizationId, updatedAt.
- Around line 71-79: Before inserting the desktop session into the sessions
table, validate that session.user.id and session.session.activeOrganizationId
are present and valid; in the block around the db.insert(sessions).values({...})
call, check that session.user?.id is non-empty and
session.session?.activeOrganizationId is not null/undefined, and if either is
missing either abort (throw or return an error response) or set a safe
fallback/explicitly-null field only after deciding behavior; ensure token,
expiresAt, ipAddress, userAgent and updatedAt remain unchanged and include a
clear error path when validation fails so no incomplete session row is created.
- Around line 66-68: Extract the magic number used to compute session expiry
into a named top-level constant (e.g., SESSION_EXPIRY_MS) and replace the inline
multiplication in the expiresAt calculation with that constant; update the
module top to declare the constant (documenting that it represents 30 days in
milliseconds) and ensure expiresAt = new Date(Date.now() + SESSION_EXPIRY_MS) so
the intent is clear and maintainable.
In @apps/web/src/app/layout.tsx:
- Around line 40-53: The Providers component is missing the Better Auth
authentication provider, breaking auth initialization; update the Providers
implementation to include the BetterAuthProvider (or the app's chosen auth
provider) by importing the provider symbol (e.g., BetterAuthProvider) and
wrapping the existing children/Toaster/TRPC/PostHog/Theme providers with it (or
placing it at the top-level inside Providers) and pass any required
initialization/config props (client, apiKey, or config object) from your
environment or existing config so the auth context is available to the entire
app.
In @packages/auth/package.json:
- Around line 1-34: Remove the unused dependency "@t3-oss/env-nextjs" from the
dependencies section of package.json; the auth package only imports
"@t3-oss/env-core" (see src/env.ts) so delete the "@t3-oss/env-nextjs" entry to
keep dependencies accurate and run npm/yarn install to update lockfiles.
In @packages/auth/src/client.ts:
- Around line 6-9: authClient is using process.env.NEXT_PUBLIC_API_URL directly;
validate and surface errors instead of silently failing by replacing that direct
access with a validated env value (e.g., import and use an env export from your
env schema like env.NEXT_PUBLIC_API_URL) when constructing createAuthClient, and
add a clear runtime check that throws/logs a descriptive error if the validated
value is missing or invalid so baseURL is guaranteed to be a non-empty valid URL
before calling createAuthClient.
In @packages/auth/src/index.ts:
- Around line 55-63: Guard against null user.name and reduce slug collision risk
by defaulting the display name when building organization name and creating a
stronger slug: in the after: async (user) => callback (the block that calls
auth.api.createOrganization) replace `${user.name}` with a safe fallback like
(user.name ?? 'User') or use user.email/username, and build the slug using a
less-colliding value such as the full user.id or full user.id plus a short
random suffix (e.g., `${user.id}-${nanoid(6)}`) or a timestamp instead of
slicing to 8 chars; also sanitize/normalize the slug (lowercase, replace
spaces/special chars) before passing it to auth.api.createOrganization to ensure
uniqueness and valid characters.
In @packages/db/drizzle/0006_add_better_auth_tables.sql:
- Around line 38-46: The organizations table definition is missing a default
timestamp for the created_at column; update the CREATE TABLE
"auth"."organizations" statement to set "created_at" timestamp NOT NULL DEFAULT
now() so new rows get an automatic timestamp (modify the "auth"."organizations"
table creation, specifically the "created_at" column).
- Around line 30-36: The CREATE TABLE for "auth"."members" is missing a DEFAULT
now() on the "created_at" column; update the "auth"."members" table definition
(the CREATE TABLE that defines "members") to set "created_at" timestamp DEFAULT
now() NOT NULL so it matches the other auth tables (accounts, invitations,
sessions, users, verifications) and will auto-populate on insert.
In @packages/db/src/env.ts:
- Line 22: Replace the hardcoded skipValidation: true with a conditional
expression so environment validation runs in prod; set skipValidation to true
only when running locally or when an explicit opt-out is provided (e.g.,
process.env.NODE_ENV === 'development' or process.env.SKIP_ENV_VALIDATION ===
'true'), otherwise set it to false so DATABASE_URL and DATABASE_URL_UNPOOLED
schemas are validated at startup; update the skipValidation setting in the env
config where skipValidation is defined.
In @packages/db/src/schema/auth.ts:
- Around line 105-125: The members table allows duplicate memberships and lacks
an updated timestamp and enum constraint: add a unique constraint on
(organizationId, userId) for the members table to prevent duplicate rows, add an
updatedAt: timestamp("updated_at").defaultNow().onUpdateNow().notNull() (or
equivalent) alongside createdAt for auditability, and change role to use a
Drizzle enum type (or create a DB-level enum) instead of text to enforce allowed
values; update the members table definition (symbol: members) and its types
(SelectMember/InsertMember) accordingly to reflect these schema changes.
- Around line 29-47: The sessions table's activeOrganizationId column lacks a
foreign key constraint; update the activeOrganizationId definition in the
sessions schema to reference the organizations primary key (e.g.,
organizations.id) and add an appropriate onDelete action (suggest onDelete: "SET
NULL" to avoid orphaned IDs), e.g., change the uuid("active_organization_id")
definition to call .references(() => organizations.id, { onDelete: "set null" })
while keeping the column nullable.
In @packages/db/src/utils/sql.ts:
- Line 24: Add a short inline comment near the sql`SELECT
pg_current_xact_id()::text as txid` usage in packages/db/src/utils/sql.ts
stating that pg_current_xact_id() requires PostgreSQL 13+ (and that
@neondatabase/serverless targets PG13+), or update the module/file header to
document the minimum PostgreSQL version as 13; reference the sql`SELECT
pg_current_xact_id()::text as txid` expression so readers know this is why the
PG13 requirement exists.
In @packages/trpc/src/router/organization/organization.ts:
- Line 82: The input schema currently validates id as z.string() which loosens
checks; change the id validator back to z.string().uuid() in the organization
router input (the object that contains id: z.string()) so requests are validated
as UUIDs (e.g., replace the id field in the organization procedure/input schema
with id: z.string().uuid()).
🟡 Minor comments (12)
apps/desktop/src/main/lib/auth/auth.ts-205-216 (1)
205-216: Silent error swallowing in signOut.The empty catch block for
fs.unlinkswallows all errors. While "file not found" is expected, other errors (permissions, I/O issues) should be logged for debugging.🔎 Suggested fix
try { await fs.unlink(TOKEN_FILE); - } catch {} + } catch (error) { + // ENOENT (file not found) is expected, log others + if ((error as NodeJS.ErrnoException).code !== "ENOENT") { + console.error("[auth] Failed to remove token file:", error); + } + }apps/desktop/src/main/lib/auth/auth.ts-70-73 (1)
70-73: Silent error swallowing during initialization.The empty catch block violates the guideline to never swallow errors silently. If the token file is corrupted or decryption fails for reasons other than "file not found," this should be logged for debugging.
🔎 Suggested fix
} catch (error) { + // Log at debug level - expected on first run or if file doesn't exist + console.log("[auth] No existing session to restore:", error instanceof Error ? error.message : error); this.token = null; this.expiresAt = null; }packages/db/src/utils/sql.ts-24-24 (1)
24-24: Document minimum PostgreSQL version requirement.The change from
txid_current()topg_current_xact_id()is correct—pg_current_xact_id()is the official replacement introduced in PostgreSQL 13 and is not deprecated. Since the project uses@neondatabase/serverless, which requires PostgreSQL 13+, this change is compatible with the production environment.However, add a comment or update documentation to explicitly state the minimum PostgreSQL 13 requirement if not already documented elsewhere.
apps/admin/src/proxy.ts-13-19 (1)
13-19: Add null check forsession.user.emailbefore callingendsWith.Line 17 will throw a TypeError if
session.user.emailisnullorundefined. This same check already uses optional chaining inapps/admin/src/app/(dashboard)/layout.tsxline 38, establishing the defensive pattern as the standard in this codebase.Proposed 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)); }apps/desktop/src/renderer/contexts/CollectionsProvider/collections.ts-53-67 (1)
53-67: Add defensive check fortransaction.mutations[0].Accessing
transaction.mutations[0]directly without checking if the array is non-empty could cause runtime errors if an empty mutations array is passed.🔎 Proposed fix for all mutation handlers
onInsert: async ({ transaction }) => { - const item = transaction.mutations[0].modified; + const mutation = transaction.mutations[0]; + if (!mutation) { + throw new Error("[collections/tasks] No mutation found in transaction"); + } + const item = mutation.modified; const result = await apiClient.task.create.mutate(item); return { txid: result.txid }; }, onUpdate: async ({ transaction }) => { - const { modified } = transaction.mutations[0]; + const mutation = transaction.mutations[0]; + if (!mutation) { + throw new Error("[collections/tasks] No mutation found in transaction"); + } + const { modified } = mutation; const result = await apiClient.task.update.mutate(modified); return { txid: result.txid }; }, onDelete: async ({ transaction }) => { - const item = transaction.mutations[0].original; + const mutation = transaction.mutations[0]; + if (!mutation) { + throw new Error("[collections/tasks] No mutation found in transaction"); + } + const item = mutation.original; const result = await apiClient.task.delete.mutate(item.id); return { txid: result.txid }; },Apply the same pattern to the repositories collection handlers.
packages/auth/src/index.ts-55-63 (1)
55-63: Guard against nulluser.nameand potential slug collisions.
user.namemay benullif the user signed up without providing a name, which would result in"null's Workspace".- Using
user.id.slice(0, 8)for the slug creates collision risk for users with IDs sharing the same 8-character prefix.🔎 Proposed fix
after: async (user) => { + const workspaceName = user.name?.trim() || "My"; const org = await auth.api.createOrganization({ body: { - name: `${user.name}'s Workspace`, - slug: `${user.id.slice(0, 8)}-workspace`, + name: `${workspaceName}'s Workspace`, + slug: `${user.id}-workspace`, userId: user.id, }, });packages/auth/package.json-1-34 (1)
1-34: Remove unused@t3-oss/env-nextjsdependency.The package structure is well-configured with proper ESM support, correct export paths, and matching
drizzle-ormversion (0.45.1) withpackages/db. However,@t3-oss/env-nextjsis listed as a dependency but is never imported or used in the auth package—only@t3-oss/env-coreis imported inpackages/auth/src/env.ts. Remove@t3-oss/env-nextjsto clean up the dependencies.packages/db/drizzle/0006_add_better_auth_tables.sql-38-46 (1)
38-46: MissingDEFAULT now()oncreated_atcolumn.Similarly,
organizations.created_at(line 43) lacks a default. New organizations created via the application will need to explicitly set this value.🔎 Suggested fix
CREATE TABLE "auth"."organizations" ( "id" uuid PRIMARY KEY DEFAULT gen_random_uuid() NOT NULL, "name" text NOT NULL, "slug" text NOT NULL, "logo" text, - "created_at" timestamp NOT NULL, + "created_at" timestamp DEFAULT now() NOT NULL, "metadata" text, CONSTRAINT "organizations_slug_unique" UNIQUE("slug") );packages/db/drizzle/0006_add_better_auth_tables.sql-30-36 (1)
30-36: MissingDEFAULT now()oncreated_atcolumn.The
members.created_atcolumn (line 35) lacks aDEFAULT now()clause, unlike other tables (accounts,invitations,sessions,users,verifications). This inconsistency could cause issues when inserting new members without explicitly providingcreated_at.🔎 Suggested fix
CREATE TABLE "auth"."members" ( "id" uuid PRIMARY KEY DEFAULT gen_random_uuid() NOT NULL, "organization_id" uuid NOT NULL, "user_id" uuid NOT NULL, "role" text DEFAULT 'member' NOT NULL, - "created_at" timestamp NOT NULL + "created_at" timestamp DEFAULT now() NOT NULL );apps/desktop/src/renderer/screens/main/components/TasksView/components/OrganizationSwitcher/OrganizationSwitcher.tsx-40-42 (1)
40-42: Missing error handling inswitchOrganization.The async function doesn't handle potential failures from the mutation. Users won't receive feedback if the organization switch fails.
🔎 Suggested fix
const switchOrganization = async (newOrgId: string) => { - await setActiveOrg.mutateAsync({ organizationId: newOrgId }); + try { + await setActiveOrg.mutateAsync({ organizationId: newOrgId }); + } catch (error) { + console.error("[OrganizationSwitcher] Failed to switch organization:", error); + } };Based on coding guidelines: "Never swallow errors silently - at minimum log them with context".
apps/api/src/trpc/context.ts-4-14 (1)
4-14: Remove unusedresHeadersparameter.The context creation correctly passes the session to
createTRPCContextand the type signature matches —TRPCContextexpects{ session: Session | null }. However,resHeadersis declared in the function parameters but never used; remove it unless it's explicitly required by the framework.packages/trpc/src/router/organization/organization.ts-82-82 (1)
82-82: Weakened input validation:idshould validate as UUID.The
idfield validation was changed fromz.string().uuid()toz.string(), allowing any string value. Sinceorganizations.idis a UUID column, this should maintain UUID validation for consistency and to fail fast on invalid input.🔎 Proposed fix
- id: z.string(), + id: z.string().uuid(),
🧹 Nitpick comments (29)
apps/desktop/src/renderer/contexts/MonacoProvider/index.ts (1)
1-8: Consider direct imports over barrel file pattern.The coding guidelines state: "Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies." While this barrel file provides a clean API boundary for the MonacoProvider context, consumers could import directly from
./MonacoProvider/MonacoProviderinstead.If this pattern is intentional for maintaining a stable public API, ensure it doesn't create circular dependencies as the codebase grows.
As per coding guidelines, prefer direct imports from concrete files.
apps/admin/src/components/PostHogUserIdentifier/PostHogUserIdentifier.tsx (1)
13-16: Consider removing the redundant TRPC query.The web version of this component (shown in relevant snippets) directly uses
session.userdata for PostHog identification without an additional TRPC query. SinceauthClient.useSession()already provides the necessary user properties (id, email, name), this extra query adds unnecessary latency and complexity.Unless the TRPC endpoint returns additional data required specifically for the admin app, consider aligning with the web implementation:
🔎 Suggested simplification matching the web version
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 }); - } else if (!session?.user) { + 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/main/lib/api-client.ts (1)
28-44: Auth error handling looks good; considerconsole.errorfor network failures.The 401/403 handling with session clearing and the preservation of sessions on network errors is well-designed. However, Line 41 uses
console.logfor what is an error condition—network failures typically warrantconsole.errorfor consistency with error severity.🔎 Suggested fix
} catch (error) { // Network errors - preserve session for offline work - console.log("[api-client] Network error, preserving session", error); + console.error("[api-client] Network error, preserving session:", error); throw error; }apps/desktop/src/main/lib/auth/auth.ts (3)
76-81: Extract magic number to named constant.The 5-minute buffer is a magic number. Per coding guidelines, extract hardcoded values to named constants at module top for clarity.
🔎 Suggested fix
+const TOKEN_EXPIRY_BUFFER_MS = 5 * 60 * 1000; // 5 minutes before actual expiry + const TOKEN_FILE = join(SUPERSET_HOME_DIR, "auth-token.enc");Then update
isExpired():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; }
83-94: Consider reducing verbose logging in frequently-called methods.
getState()logs detailed info on every call. This will create significant log noise since it's called by subscriptions and UI state checks. Consider reducing to debug level or removing the detailed logging.
148-171: Extract state expiry duration to named constant.The 10-minute state expiry is a magic number. Extract to a module-level constant for consistency with the buffer constant suggestion above.
🔎 Suggested fix
+const STATE_EXPIRY_MS = 10 * 60 * 1000; // 10 minutes + const TOKEN_FILE = join(SUPERSET_HOME_DIR, "auth-token.enc");Then update
signIn():- const tenMinutesAgo = Date.now() - 10 * 60 * 1000; + const stateExpiryThreshold = Date.now() - STATE_EXPIRY_MS; for (const [s, ts] of stateStore) { - if (ts < tenMinutesAgo) stateStore.delete(s); + if (ts < stateExpiryThreshold) stateStore.delete(s); }apps/desktop/src/renderer/contexts/PostHogProvider/index.ts (1)
1-1: Consider direct imports instead of barrel exports.This barrel export adds an indirection layer. As per coding guidelines: "Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies."
While this is a simple re-export, consumers could import directly from
"./PostHogProvider/PostHogProvider"instead of relying on the barrel at"./PostHogProvider".apps/desktop/src/renderer/contexts/AppProviders/index.ts (1)
1-1: Consider direct imports instead of barrel exports.This barrel export adds an indirection layer. As per coding guidelines: "Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies."
Consumers could import directly from
"./AppProviders/AppProviders"instead of relying on the barrel.apps/desktop/src/renderer/contexts/TRPCProvider/index.ts (1)
1-1: Consider direct imports instead of barrel exports.This barrel export adds an indirection layer. As per coding guidelines: "Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies."
Consumers could import directly from
"./TRPCProvider/TRPCProvider"instead of relying on the barrel.apps/desktop/src/renderer/contexts/AppProviders/AppProviders.tsx (1)
3-5: Import paths updated correctly for restructured providers.The import paths have been correctly updated to reflect the providers moving from nested to sibling directories. However, these imports now resolve through the new barrel exports (index.ts files).
Per coding guidelines, consider importing directly from concrete files:
import { MonacoProvider } from "../MonacoProvider/MonacoProvider"; import { PostHogProvider } from "../PostHogProvider/PostHogProvider"; import { TRPCProvider } from "../TRPCProvider/TRPCProvider";apps/desktop/src/renderer/index.tsx (1)
9-9: Consider importing from the concrete file to avoid potential circular dependencies.Per coding guidelines, prefer importing directly from the concrete file rather than the barrel export:
"./contexts/AppProviders/AppProviders"instead of"./contexts/AppProviders". This helps prevent circular dependency issues.As per coding guidelines, "Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies."
🔎 Proposed change
-import { AppProviders } from "./contexts/AppProviders"; +import { AppProviders } from "./contexts/AppProviders/AppProviders";apps/web/src/app/auth/desktop/success/components/DesktopRedirect/DesktopRedirect.tsx (1)
14-22: Consider extracting magic strings to named constants.The hardcoded strings like
"/title.svg","Superset", and display messages could be extracted to named constants at the module top for easier maintenance and consistency.🔎 Proposed refactor
+"use client"; + +import Image from "next/image"; +import Link from "next/link"; +import { useEffect } from "react"; + +const TITLE_IMAGE_PATH = "/title.svg"; +const TITLE_ALT_TEXT = "Superset"; +const REDIRECTING_MESSAGE = "Redirecting to desktop app..."; +const MANUAL_REDIRECT_TEXT = "Click here if not redirected"; + export function DesktopRedirect({ url }: { url: string }) { useEffect(() => { window.location.href = url; }, [url]); return ( <div className="flex flex-col items-center gap-6"> - <Image src="/title.svg" alt="Superset" width={280} height={86} priority /> + <Image src={TITLE_IMAGE_PATH} alt={TITLE_ALT_TEXT} width={280} height={86} priority /> <p className="text-xl text-muted-foreground"> - Redirecting to desktop app... + {REDIRECTING_MESSAGE} </p> <Link 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 + {MANUAL_REDIRECT_TEXT} </Link> </div> ); }apps/web/src/app/auth/desktop/success/page.tsx (1)
80-82: Consider extracting protocol selection logic to a named constant.The hardcoded protocol strings and environment check could be extracted to improve maintainability.
🔎 Proposed refactor
+const DESKTOP_PROTOCOL = process.env.NODE_ENV === "development" ? "superset-dev" : "superset"; + export default async function DesktopSuccessPage({ // ... earlier code ... - const protocol = - process.env.NODE_ENV === "development" ? "superset-dev" : "superset"; - const desktopUrl = `${protocol}://auth/callback?token=${encodeURIComponent(token)}&expiresAt=${encodeURIComponent(expiresAt.toISOString())}&state=${encodeURIComponent(state)}`; + const desktopUrl = `${DESKTOP_PROTOCOL}://auth/callback?token=${encodeURIComponent(token)}&expiresAt=${encodeURIComponent(expiresAt.toISOString())}&state=${encodeURIComponent(state)}`;apps/admin/src/trpc/react.tsx (1)
52-57: Consider adding auth error handling in the fetch override.While
credentials: "include"is correct for session cookies, consider adding error handling for auth failures (401/403) similar to the desktop app's implementation.🔎 Proposed enhancement with error handling
fetch(url, options) { - return fetch(url, { - ...options, - credentials: "include", - }); + return fetch(url, { + ...options, + credentials: "include", + }).then(async (response) => { + // Handle auth errors - session expired/invalid + if (response.status === 401 || response.status === 403) { + console.log("[trpc] Auth error, redirecting to sign-in"); + window.location.href = "/sign-in"; + } + return response; + }); }Based on the similar pattern in
apps/desktop/src/main/lib/api-client.ts(lines 27-43).apps/api/src/app/api/auth/desktop/connect/route.ts (1)
33-37: Consider sanitizing error responses to avoid leaking sensitive data.The error message on line 34 includes the full response body via
JSON.stringify(body), which might leak sensitive information if the auth API returns tokens or secrets in error cases.🔎 Safer error response
if (!body.url) { - return new Response(`Failed to initiate OAuth: ${JSON.stringify(body)}`, { + return new Response("Failed to initiate OAuth flow", { status: 500, }); }apps/api/src/proxy.ts (1)
5-9: Consider extracting the desktop port to a named constant.The hardcoded
5927port for the desktop app in development could benefit from being a named constant for clarity and maintainability.🔎 Suggested improvement
+const DESKTOP_DEV_PORT = 5927; + const allowedOrigins = [ env.NEXT_PUBLIC_WEB_URL, env.NEXT_PUBLIC_ADMIN_URL, - env.NODE_ENV === "development" && "http://localhost:5927", + env.NODE_ENV === "development" && `http://localhost:${DESKTOP_DEV_PORT}`, ].filter(Boolean);Based on coding guidelines, magic numbers should be extracted to named constants.
packages/auth/src/index.ts (1)
25-31: Extract session configuration constants.The session expiry and cache values are magic numbers. Per coding guidelines, extract these to named constants at module top for clarity and maintainability.
🔎 Proposed fix
+const SESSION_EXPIRY_SECONDS = 60 * 60 * 24 * 30; // 30 days +const SESSION_UPDATE_AGE_SECONDS = 60 * 60 * 24; // 1 day +const COOKIE_CACHE_MAX_AGE_SECONDS = 60 * 5; // 5 minutes + export const auth = betterAuth({ // ... session: { - expiresIn: 60 * 60 * 24 * 30, // 30 days - updateAge: 60 * 60 * 24, // refresh daily on activity + expiresIn: SESSION_EXPIRY_SECONDS, + updateAge: SESSION_UPDATE_AGE_SECONDS, cookieCache: { enabled: true, - maxAge: 60 * 5, // 5 minutes + maxAge: COOKIE_CACHE_MAX_AGE_SECONDS, }, },apps/web/src/proxy.ts (1)
16-35: Consider error handling for session retrieval failures.If
auth.api.getSessionthrows (e.g., database unavailable), the proxy will crash and return a 500 error. Consider wrapping in try/catch to gracefully handle auth service failures, potentially redirecting to an error page or allowing access to public routes.🔎 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 (err) { + console.error("[proxy] Failed to get session:", err); + // Allow public routes even if auth service is down + const pathname = req.nextUrl.pathname; + if (isPublicRoute(pathname)) { + return NextResponse.next(); + } + // Redirect to sign-in for protected routes when auth is unavailable + return NextResponse.redirect(new URL("/sign-in", req.url)); + } const pathname = req.nextUrl.pathname;apps/web/src/components/PostHogUserIdentifier/PostHogUserIdentifier.tsx (1)
7-19: Consider consistency with admin's PostHogUserIdentifier implementation.This implementation identifies users directly from
session.user, while the admin version (seeapps/admin/src/components/PostHogUserIdentifier/PostHogUserIdentifier.tsx) fetches the user via TRPC before identifying. If the TRPC user object contains enriched or canonical data, you may want to align these implementations.The
session === nullcheck correctly distinguishes between "loading" (undefined) and "logged out" (null) states.🔎 Optional: Align with admin implementation 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 }); } else if (!session?.user) { posthog.reset(); } }, [user, session?.user]); return null; }packages/auth/src/env.ts (3)
6-6: Path resolution may be fragile.Using
process.cwd()for the dotenv path can be unreliable since the current working directory varies depending on how the application is started. Consider using__dirnameorimport.meta.urlfor a more reliable resolution relative to this file's location.🔎 Suggested fix using import.meta.url
-config({ path: path.resolve(process.cwd(), "../../../.env") }); +import { fileURLToPath } from "node:url"; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +config({ path: path.resolve(__dirname, "../../../.env") });
9-15: Add minimum length validation to secrets.Other env files in the codebase (e.g.,
apps/api/src/env.ts) use.min(1)or.min(16)constraints on secrets. Consider adding similar constraints for consistency and to catch misconfiguration early.🔎 Suggested validation constraints
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: Make validation conditional like other env files.Other env configurations use
skipValidation: !!process.env.SKIP_ENV_VALIDATIONto allow validation to be toggled. Unconditionally skipping validation can hide configuration errors.🔎 Suggested fix
- skipValidation: true, + skipValidation: !!process.env.SKIP_ENV_VALIDATION,packages/trpc/src/router/admin/admin.ts (1)
16-22: Consider adding UUID validation and verifying deletion occurred.The mutation accepts any string as
userIdwithout UUID format validation, and doesn't verify the user existed before deletion. While the database will handle invalid UUIDs gracefully, explicit validation provides better error messages.🔎 Suggested improvements
deleteUser: adminProcedure - .input(z.object({ userId: z.string() })) + .input(z.object({ userId: z.string().uuid() })) .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 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: This would require re-adding the
TRPCErrorimport.packages/db/drizzle/0006_add_better_auth_tables.sql (1)
171-171: Redundant unique index onslug.Line 45 already defines
CONSTRAINT "organizations_slug_unique" UNIQUE("slug"), which implicitly creates a unique index. The explicitCREATE UNIQUE INDEX "organizations_slug_idx"on line 171 is redundant and adds maintenance overhead.🔎 Suggested fix
-CREATE UNIQUE INDEX "organizations_slug_idx" ON "auth"."organizations" USING btree ("slug");--> statement-breakpointapps/api/src/app/api/electric/[...path]/route.ts (1)
45-59: Consider handling fetch errors.The
fetchcall to Electric could fail (network issues, Electric unavailable). Currently, any error would propagate as an unhandled exception.🔎 Suggested enhancement
-const response = await fetch(originUrl.toString()); +let response: Response; +try { + response = await fetch(originUrl.toString()); +} catch (error) { + console.error("[Electric/proxy] Upstream fetch failed:", error); + return new Response("Electric service unavailable", { status: 502 }); +}packages/trpc/src/router/organization/organization.ts (1)
68-74: Consider adding explicit error handling for failed organization creation.The code checks
if (organization)but doesn't handle the failure case. If the insert fails silently (returns empty array), the function returnsundefinedwithout creating the owner membership, which could leave the system in an inconsistent state.🔎 Proposed fix
- if (organization) { - await db.insert(members).values({ - organizationId: organization.id, - userId: ctx.session.user.id, - role: "owner", - }); + if (!organization) { + throw new Error("Failed to create organization"); } + + await db.insert(members).values({ + organizationId: organization.id, + userId: ctx.session.user.id, + role: "owner", + }); return organization;packages/trpc/src/router/integration/linear/utils.ts (1)
55-71: Function signature follows positional parameters pattern.Per coding guidelines, functions with 2+ parameters should use a single params object. Consider refactoring for better self-documentation and extensibility.
🔎 Proposed refactor
-export async function verifyOrgMembership( - userId: string, - organizationId: string, -) { +export async function verifyOrgMembership(params: { + userId: string; + organizationId: string; +}) { + const { userId, organizationId } = params;packages/db/src/schema/auth.ts (2)
89-103: AddupdatedAtfield for consistency.The
organizationstable is missing anupdatedAtfield, which is present in most other tables (users, sessions, accounts, verifications). This field is valuable for audit trails and change tracking.🔎 Proposed fix to add updatedAt field
logo: text("logo"), createdAt: timestamp("created_at").defaultNow().notNull(), + updatedAt: timestamp("updated_at") + .defaultNow() + .$onUpdate(() => new Date()) + .notNull(), metadata: text("metadata"),Optional: Remove redundant unique index.
Line 99 creates a
uniqueIndexonslug, but line 94 already applies.unique()to the column, making the index redundant.🔎 Proposed fix to remove redundant index
- (table) => [uniqueIndex("organizations_slug_idx").on(table.slug)], + (table) => [],Or simply remove the callback if no other indexes are needed:
-export const organizations = authSchema.table( - "organizations", - { +export const organizations = authSchema.table("organizations", { id: uuid("id").primaryKey().defaultRandom(), name: text("name").notNull(), slug: text("slug").notNull().unique(), logo: text("logo"), createdAt: timestamp("created_at").defaultNow().notNull(), metadata: text("metadata"), - }, - (table) => [uniqueIndex("organizations_slug_idx").on(table.slug)], -); +});
127-147: AddupdatedAtfield for consistency.The
invitationstable is missing anupdatedAtfield, which breaks consistency with the audit trail pattern used in other tables.🔎 Proposed fix to add updatedAt field
expiresAt: timestamp("expires_at").notNull(), createdAt: timestamp("created_at").defaultNow().notNull(), + updatedAt: timestamp("updated_at") + .defaultNow() + .$onUpdate(() => new Date()) + .notNull(), inviterId: uuid("inviter_id")Consider using enums for
roleandstatusfields.Both
roleandstatusfields use plain text, which doesn't enforce valid values at the database level. Consider defining allowed values using Drizzle's enum type (e.g.,status: pgEnum('invitation_status', ['pending', 'accepted', 'rejected'])).
| "dependencies": { | ||
| "@clerk/nextjs": "^6.36.2", | ||
| "@sentry/nextjs": "^10.32.1", | ||
| "@superset/auth": "workspace:*", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
better-auth npm package version 1.4.9 security vulnerabilities
💡 Result:
I found multiple public security advisories for the better-auth package. Short summary relevant to version 1.4.9:
- Multiple disclosures exist (open-redirect / verify-email, HTML injection, and other router / multi-session issues) documented in public advisories. (advisories.gitlab.com)
- One CVE (CVE-2024-56734) is an open-redirect in the verify-email endpoint; it was fixed in 1.1.6 (so versions >=1.1.6 are not vulnerable to that CVE). (advisories.gitlab.com)
- Other more recent advisories describe router baseURL poisoning, a multi-session sign-out cookie-trust issue, and other flaws — check the package security advisories and change logs for which specific patch version fixes each issue. (advisories.gitlab.com)
Recommendation: review the project’s official security advisories and changelog and upgrade to the latest patched release (or apply the vendor-recommended patches) before running 1.4.9 in production. Useful references: Snyk package page, GitLab/npm advisory page, and vendor security advisory entries. (security.snyk.io)
If you want, I can (a) list which specific advisories affect 1.4.9 exactly, or (b) fetch the changelog/commit that fixes a particular advisory. Which would you prefer?
Citations:
- 1: https://advisories.gitlab.com/pkg/npm/better-auth/?utm_source=openai
- 2: https://advisories.gitlab.com/pkg/npm/better-auth/CVE-2024-56734/?utm_source=openai
- 3: https://advisories.gitlab.com/pkg/npm/better-auth/?utm_source=openai
- 4: https://security.snyk.io/package/npm/better-auth?utm_source=openai
🌐 Web query:
better-auth latest stable version npm registry
💡 Result:
The latest stable version on the npm registry is 1.4.7 (as of January 6, 2026). [1][2]
Sources:
[1] Snyk package page for better-auth (shows latest version 1.4.7).
[2] Better Auth release/blog pages announcing the 1.4 series.
Correct the better-auth version—1.4.9 does not exist on npm.
The specified version ^1.4.9 cannot be resolved; the latest stable version on npm is 1.4.7 (as of January 2026). Update the dependency to ^1.4.7 or verify the intended version.
Additionally, better-auth has multiple known security advisories including router baseURL poisoning, multi-session sign-out cookie-trust issues, and HTML injection flaws. Review the official security advisories and changelog to understand which patches apply to the version you deploy, and plan upgrades accordingly before production use.
Also applies to: 26-26
🤖 Prompt for AI Agents
In @apps/admin/package.json at line 15, Update the better-auth dependency in
package.json for the @superset/auth entry: replace the non-existent ^1.4.9
version with a resolvable release such as ^1.4.7 (or the intended published
version), and ensure any other occurrences of better-auth in the repo are
updated consistently; also review the linked security advisories and changelog
to determine whether additional patches or a different safe version are required
before promoting to production.
| 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.
Logic gap in PostHog state management.
The current if/else-if logic has an unhandled case that could leave PostHog in a stale state:
- If branch (Line 19): Executes when
useris truthy - Else-if branch (Line 21): Executes when
!session?.user - Gap: When
session?.useris truthy butuseris falsy (query loading/failed/null)
In this gap scenario, neither PostHog identification nor reset occurs. This can happen during query loading, query failure, or if the endpoint returns null.
Additionally, the reset condition !session?.user differs from the web version's session === null, which could cause inconsistent behavior during session initialization (undefined vs. null).
🔎 Suggested fix to align with web version logic
If the TRPC query is necessary, add proper handling for all states:
useEffect(() => {
- if (user) {
+ if (session?.user && 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]);However, the web version's simpler approach (previous comment) eliminates this complexity entirely.
📝 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.
| 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]); | |
| useEffect(() => { | |
| if (session?.user && user) { | |
| posthog.identify(user.id, { email: user.email, name: user.name }); | |
| } else if (session === null) { | |
| posthog.reset(); | |
| } | |
| }, [user, session]); |
🤖 Prompt for AI Agents
In @apps/admin/src/components/PostHogUserIdentifier/PostHogUserIdentifier.tsx
around lines 18 - 24, The effect in PostHogUserIdentifier leaves a gap when
session?.user is truthy but user is falsy and also uses a different reset check
than the web version; update the useEffect (in PostHogUserIdentifier) to
explicitly handle all states: if (user) call posthog.identify(user.id, { email:
user.email, name: user.name }); else if (session === null) call posthog.reset();
otherwise do nothing (covers loading/failed/null query states), and adjust the
dependency from session?.user to session so the reset uses session === null like
the web version.
| "@superset/auth": "workspace:*", | ||
| "@superset/db": "workspace:*", | ||
| "@superset/shared": "workspace:*", | ||
| "@superset/trpc": "workspace:*", | ||
| "@t3-oss/env-nextjs": "^0.13.8", | ||
| "@trpc/server": "^11.7.1", | ||
| "@upstash/qstash": "^2.8.4", | ||
| "@vercel/blob": "^2.0.0", | ||
| "better-auth": "^1.4.9", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
better-auth npm package latest version security vulnerabilities
💡 Result:
Summary (checked online Jan 6, 2026)
- Latest release: better-auth latest published release series is v1.4.x (Snyk reports latest 1.4.10). [1]
- Known advisories / CVEs:
- CVE-2024-56734 — open-redirect in verify-email endpoint; fixed in >= 1.1.6. [2]
- CVE-2025-27143 — scheme-less callback open-redirect bypass (bypass of prior fix); patched in 1.1.21. [3][4]
- CVE-2025-61928 — critical authentication bypass allowing unauthenticated API-key creation; fixed in 1.3.26. [5]
Recommended actions
- Upgrade to the latest 1.4.x release (or at minimum >= 1.3.26 and >= 1.1.21 to cover the above fixes). Verify the exact npm tag you consume (stable vs beta/canary) and test changes.
- Review changelogs / security advisories in the repo before upgrading and audit any plugins (e.g., better-auth-nuxt) for separate advisories. [2][5]
Sources
- Snyk package info for better-auth (latest versions). [1]
- GitLab / GHSA advisory for CVE-2024-56734 (open redirect) and fix. [2]
- CVE details / vulnerability database for CVE-2025-27143 (scheme-less callback bypass). [3]
- Vuln reports summarizing the open-redirect issue and patch. [4]
- GitLab advisory / CVE-2025-61928 (unauthenticated API-key creation) and fix. [5]
Would you like me to check the exact npm "latest" tag and provide the npm install command for the safest version?
Verify better-auth includes patch for CVE-2025-61928 (critical authentication bypass).
The migration to better-auth@^1.4.9 introduces a security-critical dependency with known vulnerabilities:
- CVE-2025-61928 (critical authentication bypass for unauthenticated API-key creation): fixed in >= 1.3.26
- CVE-2025-27143 (scheme-less callback open-redirect bypass): fixed in >= 1.1.21
- CVE-2024-56734 (open-redirect in verify-email): fixed in >= 1.1.6
Since v1.4.9 is released and the latest stable is v1.4.10, confirm that v1.4.9 includes the CVE-2025-61928 fix. The caret constraint allows for automatic patch updates to 1.4.10; ensure this is acceptable for your deployment process.
🤖 Prompt for AI Agents
In @apps/api/package.json around lines 17 - 25, The dependency entry
"better-auth" in package.json must be verified to include the CVE-2025-61928 fix
(fixed in >=1.3.26); check the better-auth release/changelog or npm package
metadata to confirm v1.4.9 contains the patch and, to be safe, update the
version constraint for the "better-auth" dependency to a known-good release (for
example ^1.4.10 or >=1.3.26) in package.json, run your package manager to update
the lockfile, and re-run security audits (npm audit / yarn audit / Snyk) to
ensure CVE-2025-61928 is resolved in the installed tree and that automatic
patching via the caret range is acceptable for your deployment; if you cannot
upgrade, add a resolutions/overrides entry to force a fixed version for
"better-auth".
| 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.
Calling setError inside useMemo violates React rules.
Setting state during render (inside useMemo) can cause unpredictable behavior and infinite render loops. React's concurrent features may call this multiple times before committing. Move error handling outside the memo or use useEffect for error state management.
🔎 Proposed fix using useEffect for error handling
export function CollectionsProvider({ children }: { children: ReactNode }) {
const { data: session } = trpc.auth.onSessionChange.useSubscription();
const { data: tokenData } = trpc.auth.onAccessToken.useSubscription();
const [error, setError] = useState<Error | null>(null);
const activeOrgId = session?.session.activeOrganizationId;
const token = tokenData?.accessToken;
- 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]);
+ 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 { result: null, error: null };
+ }
+
+ try {
+ return { result: createCollections({ token, activeOrgId }), error: null };
+ } catch (err) {
+ console.error("[CollectionsProvider] Failed to create collections:", err);
+ return { result: null, error: err instanceof Error ? err : new Error(String(err)) };
+ }
+ }, [token, activeOrgId]);
+
+ useEffect(() => {
+ if (collections.error) {
+ setError(collections.error);
+ }
+ }, [collections.error]);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 current useMemo block in CollectionsProvider calls
setError during render (inside collections calculation), which violates React
rules; remove setError from the useMemo error catch (leave useMemo to only
return the created value or null from createCollections) and add a useEffect
that watches [token, activeOrgId, collections] to call setError when token &&
activeOrgId but collections is null (set a proper Error instance), and clear
error (setError(null)) when collections is successfully created; keep
createCollections and the console logs inside useMemo but ensure all state
updates happen in the new useEffect.
| export function DesktopRedirect({ url }: { url: string }) { | ||
| useEffect(() => { | ||
| window.location.href = url; | ||
| }, [url]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add URL validation before navigation.
The component directly navigates to the provided url without validation. A malicious or malformed URL could cause unexpected behavior or security issues.
🔎 Proposed fix to add URL validation
export function DesktopRedirect({ url }: { url: string }) {
useEffect(() => {
+ // Validate that the URL uses the expected protocol
+ try {
+ const parsedUrl = new URL(url);
+ if (!parsedUrl.protocol.startsWith('superset')) {
+ console.error('[DesktopRedirect] Invalid protocol:', parsedUrl.protocol);
+ return;
+ }
+ } catch (error) {
+ console.error('[DesktopRedirect] Invalid URL:', error);
+ return;
+ }
window.location.href = url;
}, [url]);🤖 Prompt for AI Agents
In
@apps/web/src/app/auth/desktop/success/components/DesktopRedirect/DesktopRedirect.tsx
around lines 7 - 10, DesktopRedirect directly assigns window.location.href to
the incoming url prop which can allow navigation to malformed or unsafe URLs;
validate the url before navigating by parsing it (e.g., new URL(url,
window.location.origin) or a whitelist check) inside the useEffect in
DesktopRedirect and only set window.location.href when the parsed URL is valid
and uses an allowed protocol (http/https) and origin rules; if validation fails,
avoid navigation and optionally call a fallback handler or log an error.
| <html lang="en" suppressHydrationWarning> | ||
| <body | ||
| className={cn( | ||
| "bg-background text-foreground min-h-screen font-sans antialiased", | ||
| inter.variable, | ||
| ibmPlexMono.variable, | ||
| )} | ||
| > | ||
| <Providers> | ||
| {children} | ||
| <Toaster /> | ||
| </Providers> | ||
| </body> | ||
| </html> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the Providers component implementation in the app directory
echo "=== Checking for Providers component in apps/web/src/app ==="
fd -t f "providers" apps/web/src/app --exec ls -la {}
echo -e "\n=== Providers component content ==="
fd -t f "providers\.(tsx|ts|jsx|js)$" apps/web/src/app --exec cat {}
echo -e "\n=== Search for Better Auth provider usage ==="
rg -n --type=tsx --type=ts "BetterAuth|SessionProvider|AuthProvider|getSession" apps/web/src/app/ -C3
echo -e "\n=== Check for auth setup in root ==="
rg -n --type=tsx --type=ts "better-auth|@auth|auth.*provider" apps/web/src/ -C2Repository: superset-sh/superset
Length of output: 1283
Better Auth initialization is missing from the <Providers> component.
The <Providers> component currently contains PostHogProvider, TRPCReactProvider, and ThemeProvider, but does not include any authentication provider setup. ClerkProvider was removed, but no replacement authentication system has been initialized. This breaks authentication across the entire application.
Add the Better Auth provider (or equivalent authentication provider) to the <Providers> component and ensure authentication context is properly initialized for the app to function.
🤖 Prompt for AI Agents
In @apps/web/src/app/layout.tsx around lines 40 - 53, The Providers component is
missing the Better Auth authentication provider, breaking auth initialization;
update the Providers implementation to include the BetterAuthProvider (or the
app's chosen auth provider) by importing the provider symbol (e.g.,
BetterAuthProvider) and wrapping the existing
children/Toaster/TRPC/PostHog/Theme providers with it (or placing it at the
top-level inside Providers) and pass any required initialization/config props
(client, apiKey, or config object) from your environment or existing config so
the auth context is available to the entire app.
| export const authClient = createAuthClient({ | ||
| baseURL: process.env.NEXT_PUBLIC_API_URL, | ||
| plugins: [organizationClient()], | ||
| }); |
There was a problem hiding this comment.
Validate environment variable before use.
The baseURL is constructed from process.env.NEXT_PUBLIC_API_URL without validation. If this environment variable is undefined or misconfigured, the auth client will fail silently or produce cryptic runtime errors.
🔎 Recommended fix: Use validated environment variables
Instead of accessing process.env directly, use a validated env schema (e.g., with @t3-oss/env-core or Zod):
"use client";
import { organizationClient } from "better-auth/client/plugins";
import { createAuthClient } from "better-auth/react";
+import { env } from "./env"; // or wherever env validation is defined
export const authClient = createAuthClient({
- baseURL: process.env.NEXT_PUBLIC_API_URL,
+ baseURL: env.NEXT_PUBLIC_API_URL,
plugins: [organizationClient()],
});If env validation doesn't exist yet, add it:
// env.ts
import { createEnv } from "@t3-oss/env-core";
import { z } from "zod";
export const env = createEnv({
clientPrefix: "NEXT_PUBLIC_",
client: {
NEXT_PUBLIC_API_URL: z.string().url(),
},
runtimeEnv: process.env,
});As per coding guidelines, never swallow errors silently and maintain type safety.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @packages/auth/src/client.ts around lines 6 - 9, authClient is using
process.env.NEXT_PUBLIC_API_URL directly; validate and surface errors instead of
silently failing by replacing that direct access with a validated env value
(e.g., import and use an env export from your env schema like
env.NEXT_PUBLIC_API_URL) when constructing createAuthClient, and add a clear
runtime check that throws/logs a descriptive error if the validated value is
missing or invalid so baseURL is guaranteed to be a non-empty valid URL before
calling createAuthClient.
| ALTER TABLE "organization_members" RENAME TO "organization_members_backup";--> statement-breakpoint | ||
| ALTER TABLE "organizations" RENAME TO "organizations_backup";--> statement-breakpoint | ||
| ALTER TABLE "users" RENAME TO "users_backup";--> statement-breakpoint | ||
| ALTER TABLE "organization_members_backup" DROP CONSTRAINT "organization_members_unique";--> statement-breakpoint | ||
| ALTER TABLE "organizations_backup" DROP CONSTRAINT "organizations_clerk_org_id_unique";--> statement-breakpoint | ||
| ALTER TABLE "organizations_backup" DROP CONSTRAINT "organizations_slug_unique";--> statement-breakpoint | ||
| ALTER TABLE "users_backup" DROP CONSTRAINT "users_clerk_id_unique";--> statement-breakpoint | ||
| ALTER TABLE "users_backup" DROP CONSTRAINT "users_email_unique";--> statement-breakpoint | ||
| ALTER TABLE "integration_connections" DROP CONSTRAINT "integration_connections_organization_id_organizations_id_fk"; | ||
| --> statement-breakpoint | ||
| ALTER TABLE "integration_connections" DROP CONSTRAINT "integration_connections_connected_by_user_id_users_id_fk"; | ||
| --> statement-breakpoint | ||
| ALTER TABLE "organization_members_backup" DROP CONSTRAINT "organization_members_organization_id_organizations_id_fk"; | ||
| --> statement-breakpoint | ||
| ALTER TABLE "organization_members_backup" DROP CONSTRAINT "organization_members_user_id_users_id_fk"; | ||
| --> statement-breakpoint | ||
| ALTER TABLE "repositories" DROP CONSTRAINT "repositories_organization_id_organizations_id_fk"; | ||
| --> statement-breakpoint | ||
| ALTER TABLE "tasks" DROP CONSTRAINT "tasks_organization_id_organizations_id_fk"; | ||
| --> statement-breakpoint | ||
| ALTER TABLE "tasks" DROP CONSTRAINT "tasks_assignee_id_users_id_fk"; | ||
| --> statement-breakpoint | ||
| ALTER TABLE "tasks" DROP CONSTRAINT "tasks_creator_id_users_id_fk"; | ||
| --> statement-breakpoint | ||
| DROP INDEX "organization_members_organization_id_idx";--> statement-breakpoint | ||
| DROP INDEX "organization_members_user_id_idx";--> statement-breakpoint | ||
| DROP INDEX "organizations_slug_idx";--> statement-breakpoint | ||
| DROP INDEX "organizations_clerk_org_id_idx";--> statement-breakpoint | ||
| DROP INDEX "users_email_idx";--> statement-breakpoint | ||
| DROP INDEX "users_clerk_id_idx";--> statement-breakpoint | ||
| DROP INDEX "users_deleted_at_idx";--> statement-breakpoint | ||
| ALTER TABLE "integration_connections" ADD CONSTRAINT "integration_connections_organization_id_organizations_backup_id_fk" FOREIGN KEY ("organization_id") REFERENCES "public"."organizations_backup"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "integration_connections" ADD CONSTRAINT "integration_connections_connected_by_user_id_users_backup_id_fk" FOREIGN KEY ("connected_by_user_id") REFERENCES "public"."users_backup"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "organization_members_backup" ADD CONSTRAINT "organization_members_backup_organization_id_organizations_backup_id_fk" FOREIGN KEY ("organization_id") REFERENCES "public"."organizations_backup"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "organization_members_backup" ADD CONSTRAINT "organization_members_backup_user_id_users_backup_id_fk" FOREIGN KEY ("user_id") REFERENCES "public"."users_backup"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "repositories" ADD CONSTRAINT "repositories_organization_id_organizations_backup_id_fk" FOREIGN KEY ("organization_id") REFERENCES "public"."organizations_backup"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "tasks" ADD CONSTRAINT "tasks_organization_id_organizations_backup_id_fk" FOREIGN KEY ("organization_id") REFERENCES "public"."organizations_backup"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "tasks" ADD CONSTRAINT "tasks_assignee_id_users_backup_id_fk" FOREIGN KEY ("assignee_id") REFERENCES "public"."users_backup"("id") ON DELETE set null ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "tasks" ADD CONSTRAINT "tasks_creator_id_users_backup_id_fk" FOREIGN KEY ("creator_id") REFERENCES "public"."users_backup"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| CREATE INDEX "organization_members_backup_organization_id_idx" ON "organization_members_backup" USING btree ("organization_id");--> statement-breakpoint | ||
| CREATE INDEX "organization_members_backup_user_id_idx" ON "organization_members_backup" USING btree ("user_id");--> statement-breakpoint | ||
| CREATE INDEX "organizations_backup_slug_idx" ON "organizations_backup" USING btree ("slug");--> statement-breakpoint | ||
| CREATE INDEX "organizations_backup_clerk_org_id_idx" ON "organizations_backup" USING btree ("clerk_org_id");--> statement-breakpoint | ||
| CREATE INDEX "users_backup_email_idx" ON "users_backup" USING btree ("email");--> statement-breakpoint | ||
| CREATE INDEX "users_backup_clerk_id_idx" ON "users_backup" USING btree ("clerk_id");--> statement-breakpoint | ||
| CREATE INDEX "users_backup_deleted_at_idx" ON "users_backup" USING btree ("deleted_at");--> statement-breakpoint | ||
| ALTER TABLE "organization_members_backup" ADD CONSTRAINT "organization_members_backup_unique" UNIQUE("organization_id","user_id");--> statement-breakpoint | ||
| ALTER TABLE "organizations_backup" ADD CONSTRAINT "organizations_backup_clerk_org_id_unique" UNIQUE("clerk_org_id");--> statement-breakpoint | ||
| ALTER TABLE "organizations_backup" ADD CONSTRAINT "organizations_backup_slug_unique" UNIQUE("slug");--> statement-breakpoint | ||
| ALTER TABLE "users_backup" ADD CONSTRAINT "users_backup_clerk_id_unique" UNIQUE("clerk_id");--> statement-breakpoint | ||
| ALTER TABLE "users_backup" ADD CONSTRAINT "users_backup_email_unique" UNIQUE("email"); No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List all SQL migration files in the drizzle directory
echo "=== All migration files ==="
fd -e sql . packages/db/drizzle | sort
echo -e "\n=== Migration journal if exists ==="
if [ -f packages/db/drizzle/meta/_journal.json ]; then
cat packages/db/drizzle/meta/_journal.json | jq '.' || cat packages/db/drizzle/meta/_journal.json
else
echo "Journal not found"
fi
echo -e "\n=== Check for down/rollback migrations ==="
ls -la packages/db/drizzle/*.down* 2>/dev/null || echo "No .down files found"Repository: superset-sh/superset
Length of output: 1854
🏁 Script executed:
#!/bin/bash
# Check the 0006 migration to see how it relates to backup tables
echo "=== Content of 0006_add_better_auth_tables.sql ==="
wc -l packages/db/drizzle/0006_add_better_auth_tables.sql
head -100 packages/db/drizzle/0006_add_better_auth_tables.sql
echo -e "\n=== Check if drizzle config or package.json has rollback info ==="
cat packages/db/drizzle.config.ts 2>/dev/null || cat drizzle.config.ts 2>/dev/null || echo "Config not found"
echo -e "\n=== Check package.json for migration commands ==="
jq '.scripts | select(. != null) | keys[]' package.json 2>/dev/null | grep -i -E 'migrat|drizzle'Repository: superset-sh/superset
Length of output: 3526
🏁 Script executed:
#!/bin/bash
# Check if Drizzle generates reversions and look for snapshot/migration metadata
echo "=== Check drizzle meta directory for snapshots/reversions ==="
ls -la packages/db/drizzle/meta/ 2>/dev/null || echo "Meta directory not found"
echo -e "\n=== View meta directory contents ==="
find packages/db/drizzle/meta -type f 2>/dev/null | head -20
echo -e "\n=== Check if any migration includes rollback statements ==="
rg -A 2 "DROP TABLE|DROP SCHEMA" packages/db/drizzle/*.sql | head -50
echo -e "\n=== Verify transaction wrapping across migrations ==="
echo "Checking if migration tool wraps multiple migrations in transaction..."
rg "BEGIN|COMMIT|TRANSACTION" packages/db/drizzle/ || echo "No explicit transaction statements found in migrations"Repository: superset-sh/superset
Length of output: 2176
Tightly couple 0005 and 0006 migrations—they must be deployed and rolled back together.
This migration (0005) renames core tables to backup variants, and the immediately following migration (0006) migrates data from backups to new auth schema tables, then drops the backups. If 0006 fails after data migration but before the DROP TABLE statements, or if either migration is rolled back independently, the system enters an inconsistent state where application code expecting original table names will fail.
Drizzle Kit auto-generates reversions via snapshots, so rollback infrastructure exists. However, the lack of explicit transaction wrapping across these two migrations means a mid-execution failure in 0006 leaves stale backup tables and broken foreign keys.
Recommendations:
- Document this as a coupled migration pair that must succeed or fail together
- Validate the full chain (0005 → 0006 → 0007) in staging before production
- Implement safeguards to prevent partial rollbacks of just 0005 without 0006
saddlepaddle
force-pushed
the
feat/better-auth-restoration
branch
from
b175d89 to
06b1ba5
Compare
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 15
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 (3)
packages/trpc/src/router/user/user.ts (1)
11-20: Non-deterministic result when user has multiple memberships.Using
findFirstwithout an explicitorderByclause means the returned organization could vary unpredictably if a user belongs to multiple organizations. Consider adding deterministic ordering (e.g., bycreatedAtor a "primary" flag) or clarifying the intended behavior for multi-org users.🔎 Suggested fix with explicit ordering
myOrganization: protectedProcedure.query(async ({ ctx }) => { const membership = await db.query.members.findFirst({ where: eq(members.userId, ctx.session.user.id), + orderBy: (members, { asc }) => [asc(members.createdAt)], with: { organization: true, }, }); return membership?.organization ?? null; }),apps/api/src/proxy.ts (1)
11-20: Set CORS origin header conditionally instead of using empty string.Line 14 sets
Access-Control-Allow-Originto an empty string for disallowed origins. Per the CORS specification, this header should either contain a valid origin or be omitted entirely. Browsers may not handle empty strings correctly.🔎 Proposed fix to conditionally set header
function getCorsHeaders(origin: string | null) { const isAllowed = origin && allowedOrigins.includes(origin); - return { - "Access-Control-Allow-Origin": isAllowed ? origin : "", + const headers: Record<string, string> = { "Access-Control-Allow-Methods": "GET, POST, PUT, PATCH, DELETE, OPTIONS", "Access-Control-Allow-Headers": "Content-Type, Authorization, x-trpc-source, trpc-accept", "Access-Control-Allow-Credentials": "true", }; + if (isAllowed) { + headers["Access-Control-Allow-Origin"] = origin; + } + return headers; }apps/api/src/app/api/electric/[...path]/utils.ts (1)
92-110: Authorization check missing for auth.users query.The auth.users case returns all user IDs in an organization without verifying the requesting user is authorized to access that organization. This allows anyone with an organizationId to enumerate all members.
The function should accept and verify the authenticated user's ID (from session) and confirm they are a member of the requested organization before returning user data.
🔎 Proposed approach
Update the function signature to accept userId from the authenticated session:
export async function buildWhereClause( tableName: string, organizationId: string, + userId: string, ): Promise<WhereClause | null> {Then for auth.users:
case "auth.users": { + // Verify the authenticated user is a member of this organization + const userMembership = await db.query.members.findFirst({ + where: and( + eq(members.organizationId, organizationId), + eq(members.userId, userId) + ), + }); + + if (!userMembership) { + return { fragment: "1 = 0", params: [] }; + } + const orgMembers = await db.query.members.findMany({ where: eq(members.organizationId, organizationId), columns: { userId: true }, });
🤖 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 lacks error handling;
wrap the authClient.signOut call in a try/catch (or provide an onError callback
in fetchOptions) and log failures using the project pattern (e.g.,
processLogger.error or console.error with a prefix like "[auth/signOut] Failed
to sign out: <error>"). Ensure the success behavior (window.location.href =
env.NEXT_PUBLIC_WEB_URL) remains on success and surface a user-friendly fallback
or no-op on error.
In @apps/admin/src/app/(dashboard)/users/components/UsersTable/UsersTable.tsx:
- Line 63: Update the deletion success toast text to match the dialog's
"permanently delete" phrasing: change the message emitted by toast.success
(where userToDelete is used) to use "has been permanently deleted" so it matches
the dialog labels ("Permanently delete user?" and "Delete Permanently") in
UsersTable.tsx; ensure any other user-facing delete success messages in this
component use the same "permanently deleted" wording for consistency.
In @apps/api/src/app/api/auth/desktop/connect/route.ts:
- Around line 6-17: Replace the manual checks in the GET handler with Zod-based
validation: define a Zod schema (e.g., const QuerySchema = z.object({ provider:
z.enum(["google","github"]), state: z.string().min(1) })) and use
QuerySchema.safeParse or parse on the URL search params inside the GET function
to validate and extract provider and state; on failure return a 400 Response
with a clear validation error message, and on success use the parsed values
instead of the previous provider/state variables.
- Around line 30-37: The code unsafely asserts the external auth API response
into { url?: string; redirect?: boolean } (variable body) without runtime
validation; replace the assertion by parsing result.json() through a Zod schema
(e.g., define a z.object({ url: z.string().optional(), redirect:
z.boolean().optional() })) and use schema.safeParse or parse to validate the
shape before using body.url; if validation fails or url is missing, return the
500 Response with the validation error details (or a sanitized message) and
ensure you still read set-cookie headers via result.headers.getSetCookie() as
before.
In @apps/desktop/src/renderer/contexts/CollectionsProvider/collections.ts:
- Around line 53-67: Wrap each mutation handler (onInsert, onUpdate, onDelete)
in a try/catch, log the caught error with contextual info (operation name and
the item/id involved) using the available logger (e.g., processLogger or
console.error) and then rethrow the error so it doesn’t get swallowed and
calling code can handle rollback; update the bodies of
onInsert/onUpdate/onDelete to perform the apiClient.task.*.mutate call inside
try, log on catch (including the mutation payload and error), and rethrow the
error.
In
@apps/desktop/src/renderer/screens/main/components/TasksView/components/OrganizationSwitcher/OrganizationSwitcher.tsx:
- Around line 40-42: The switchOrganization handler (switchOrganization)
currently calls setActiveOrg.mutateAsync without error handling; wrap the await
call in a try/catch, log the error with context (e.g., include the newOrgId and
error) using the app logger or console.error, and surface user feedback
(toast/notification or error state) when the mutation fails so the error is not
swallowed and the user is informed; ensure you still rethrow only if appropriate
for upstream handling or return after showing the error.
- Around line 33-38: The initials calculation can produce empty strings when
splitting on spaces; update the logic that computes initials (the initials
constant using activeOrganization.name) to guard for falsy name and to split on
trimmed whitespace or filter out empty parts (e.g., use trim() and split(/\s+/)
or split(" ").filter(Boolean)) before mapping to first letters and uppercasing,
so consecutive/leading/trailing spaces won't create empty initials.
In @apps/desktop/src/renderer/screens/main/components/TasksView/TasksView.tsx:
- Around line 347-355: The OrganizationsProvider wrapper around TasksView is
unnecessary; remove OrganizationsProvider so TasksView returns only
CollectionsProvider wrapping TasksViewContent. Specifically, edit the TasksView
component (function TasksView) to delete the OrganizationsProvider element and
its opening/closing tags, leaving CollectionsProvider and TasksViewContent
intact; you can verify no useOrganizations() calls exist under TasksView and
that CollectionsProvider already consumes organization data via
trpc.auth.onSessionChange.
In @apps/marketing/src/app/components/CTAButtons/CTAButtons.tsx:
- Line 9: Wrap the session retrieval in CTAButtons (the line calling
auth.api.getSession) in a try-catch so failures don't crash the component: catch
any error from auth.api.getSession, log it with contextual information (e.g.,
"CTAButtons: failed to get session" plus the error), and fall back to an
unauthenticated state (treat session as null/undefined) so the component can
render the sign-in option gracefully.
- Line 1: The auth.api.getSession call in the CTAButtons component lacks error
handling; wrap the await auth.api.getSession({ headers: await headers() }) call
in a try-catch, assign the result to the existing session variable (e.g., let
session: Session | null = null) inside the try, and in the catch log the error
with context (for example using console.error or the project logger) including a
clear prefix like "[CTAButtons] Failed to retrieve session:" so failures are
captured but do not crash the component.
In @apps/web/src/app/(auth)/sign-up/[[...sign-up]]/page.tsx:
- Line 26: Replace the bare console.error calls in the sign-up page with
prefixed logging using the "[domain/operation] message" pattern; e.g., change
console.error("Sign up failed:", err) to console.error("[auth/sign-up] Sign up
failed:", err) (and apply the same prefix format to the second console.error
occurrence in the same file) so logs include the required domain/operation
context.
- Around line 21-23: The sign-up page has two issues: console logs lack the
required context prefix and the OAuth handlers are duplicated; update the error
logging inside both signUpWithGoogle and signUpWithGithub to use
console.error("[auth/sign-up]", err) and extract a shared function (e.g.,
signUpWithProvider) that accepts the provider string and the setter for its
loading state, moves common logic (setIsLoading(true), setError(null), call to
authClient.signIn.social with callbackURL: env.NEXT_PUBLIC_WEB_URL, catch block
that logs with "[auth/sign-up]" and sets the generic error and loading=false)
into that function, then replace calls to signUpWithGoogle/signUpWithGithub with
onClick={() => signUpWithProvider("google", setIsLoadingGoogle)} and similarly
for "github".
In @apps/web/src/app/(dashboard)/components/Header/Header.tsx:
- Around line 24-27: The handleSignOut function must check the signOut response
instead of assuming it throws: call authClient.signOut(), inspect the returned
object for response.error, and only navigate (router.push("/sign-in")) when
there is no error; on error, surface it (e.g., processLogger/console.error and
show a user-facing message or toast) so the user isn’t redirected into an
inconsistent state; update the handleSignOut implementation to follow this
Better Auth pattern and reference the authClient.signOut and router.push calls
when making the change.
In @apps/web/src/app/auth/desktop/success/page.tsx:
- Around line 15-50: The three identical error UI return blocks (when !state,
when auth.api.getSession throws, and when !session) should be replaced by a
single reusable helper: extract the JSX into a small component or function like
AuthErrorMessage (or renderAuthError) defined above the page component, and then
return that helper in each of the three places instead of duplicating the
markup; reference the existing symbols state, auth.api.getSession, and headers()
where you replace the returns, and if you prefer make the helper accept an
optional title/body props for future variation.
- Line 31: The console.error call in
apps/web/src/app/auth/desktop/success/page.tsx lacks the required prefixed
logging pattern; update the console.error invocation that currently logs "Failed
to get session for desktop auth:" to include the context prefix (e.g.
"[auth/desktop/success]") before the message so it follows the
"[domain/operation] message" pattern—locate the console.error in the desktop
success page component and change the logged string to include the prefix while
keeping the error object as a separate argument.
- Around line 71-79: The db.insert(sessions).values(...) call can throw and is
not caught; wrap the insertion in a try-catch around the db.insert call (in the
same function handling the success page) to catch DB errors (including unique
constraint/connection errors), log the error (e.g., via processLogger or
console.error) including the token/session info, and return a graceful response
(e.g., redirect to an error page or render a friendly message) instead of
letting the exception bubble; if you want finer handling, inspect the error
type/code to handle duplicates specially (e.g., skip insert or update) before
rethrowing or returning the user-facing error.
In @apps/web/src/proxy.ts:
- Around line 16-35: The session check in proxy() is too loose:
auth.api.getSession() can return a session object without a user, so replace the
truthy check of session with a defensive check for session?.user (aligning with
the Electric route behavior). Update the conditional that redirects
authenticated users away from "/sign-in" and "/sign-up" to use session?.user,
and ensure any other session checks in proxy() use the same session?.user
pattern to avoid treating an empty session object as authenticated.
In @packages/auth/src/index.ts:
- Around line 57-63: Inside the user.create.after hook, replace the direct call
to auth.api.createOrganization with the adapter or a background job: use the
hook context's adapter (ctx.context.adapter) to create the organization record
(e.g., via the adapter's createOrganization or equivalent method) or enqueue a
job to perform createOrganization asynchronously; specifically remove the
auth.api.createOrganization call inside user.create.after and perform the
organization creation through ctx.context.adapter (or dispatch to your
background job queue) to avoid the documented anti-pattern and race conditions.
- Around line 55-73: The organization name creation uses user.name which can be
null; update the after: async (user) => { ... } block to use a safe fallback
(e.g., user.name || user.email || `User`) when building the name passed to
auth.api.createOrganization so you don't get "undefined's Workspace"; keep the
slug generation based on user.id.slice(0,8) but ensure the display name uses the
fallback and is sanitized (remove/escape problematic characters) before
interpolating into `${...}'s Workspace`.
In @packages/db/drizzle/meta/0006_snapshot.json:
- Around line 488-493: The sessions table's activeOrganizationId lacks a foreign
key constraint, allowing orphaned references; update the schema by changing the
activeOrganizationId column definition to reference organizations.id with
onDelete set to "set null" (use the uuid(...).references(() => organizations.id,
{ onDelete: "set null" }) pattern in the auth schema where activeOrganizationId
is defined), then re-run the migration generator (drizzle-kit generate) to
produce an updated migration/snapshot reflecting the new foreign key.
In @packages/trpc/src/router/admin/admin.ts:
- Around line 16-22: In the deleteUser adminProcedure mutation, add existence
and self-delete checks, wrap the delete in a database transaction, and surface
DB errors instead of silently returning success: first query users by id (use
db.select or db.get for users with input.userId) and throw a TRPC error if not
found; compare input.userId to the current admin’s id from ctx/session and throw
a forbidden error to prevent self-deletion; perform the deletion inside a
transaction (use your DB transaction API around
db.delete(users).where(eq(users.id, input.userId))) and catch DB errors (inspect
constraint violation messages) to throw a descriptive TRPC error with details;
finally, only return { success: true } after the transaction commits
successfully.
🟡 Minor comments (6)
apps/marketing/src/app/components/CTAButtons/CTAButtons.tsx-1-1 (1)
1-1: Add error handling around thegetSessioncall.The truthy check pattern (
if (session)) is correct and matches the standard usage throughout the codebase. However, theawait auth.api.getSession()call lacks error handling. Per coding guidelines, errors must be logged with context at minimum. Wrap the call in a try-catch block to handle any unexpected failures gracefully:let session: Session | null = null; try { session = await auth.api.getSession({ headers: await headers() }); } catch (error) { console.error("[CTAButtons] Failed to retrieve session:", error); }apps/desktop/src/renderer/screens/main/components/TasksView/components/OrganizationSwitcher/OrganizationSwitcher.tsx-33-38 (1)
33-38: Handle edge case: empty string in name parts.The initials computation could fail if
activeOrganization.name.split(" ")produces empty strings (e.g., from multiple consecutive spaces or leading/trailing spaces).🔎 Proposed fix
const initials = activeOrganization.name ?.split(" ") + .filter((n) => n.length > 0) .map((n: string) => n[0]) .join("") .toUpperCase() .slice(0, 2);apps/admin/src/app/(dashboard)/users/components/UsersTable/UsersTable.tsx-63-63 (1)
63-63: Minor: Inconsistent deletion terminology.The success toast message says "has been deleted" (line 63), but the dialog uses "Permanently delete user?" (line 214) and "Delete Permanently" (line 238). Consider using consistent terminology throughout:
- Either use "delete" everywhere, or
- Use "permanently delete" everywhere to emphasize the irreversible nature of the action.
🔎 Proposed fix for consistent terminology
- toast.success(`${userToDelete?.name} has been deleted`); + toast.success(`${userToDelete?.name} has been permanently deleted`);Also applies to: 214-214, 238-238
packages/auth/src/index.ts-55-73 (1)
55-73: Potential issue:user.namemay be null for some OAuth providers.Some OAuth providers (especially GitHub with private email settings) may not return a name. Consider adding a fallback to prevent
"undefined's Workspace"as the organization name.🔎 Suggested fix
after: async (user) => { // Create organization for new user + const displayName = user.name || user.email?.split("@")[0] || "User"; const org = await auth.api.createOrganization({ body: { - name: `${user.name}'s Workspace`, + name: `${displayName}'s Workspace`, slug: `${user.id.slice(0, 8)}-workspace`, userId: user.id, }, });apps/web/src/proxy.ts-16-35 (1)
16-35: Inconsistent session check may cause issues.Line 24 checks
session(truthy), butauth.api.getSessionmay return an object with a null user. The Electric route (line 10) checks!sessionData?.user, which is more defensive. Consider aligning for consistency.🔎 Suggested fix for consistency
export default async function proxy(req: NextRequest) { const session = await auth.api.getSession({ headers: await headers(), }); const pathname = req.nextUrl.pathname; if ( - session && + session?.user && (pathname.startsWith("/sign-in") || pathname.startsWith("/sign-up")) ) { return NextResponse.redirect(new URL("/", req.url)); } - if (!session && !isPublicRoute(pathname)) { + if (!session?.user && !isPublicRoute(pathname)) { return NextResponse.redirect(new URL("/sign-in", req.url)); } return NextResponse.next(); }apps/web/src/app/(auth)/sign-up/[[...sign-up]]/page.tsx-21-23 (1)
21-23: Console logging should include context prefix, and duplicate OAuth handlers should be consolidated.Two issues need addressing:
Console logging format (lines 26, 42): Add context prefix per guidelines. Change
console.error("Sign up failed:", err)toconsole.error("[auth/sign-up]", err).Code duplication (lines 16–46): The
signUpWithGoogleandsignUpWithGithubfunctions are nearly identical. Extract a shared handler to reduce duplication:Suggested refactor
const signUpWithProvider = async ( provider: "google" | "github", setIsLoading: React.Dispatch<React.SetStateAction<boolean>> ) => { setIsLoading(true); setError(null); try { await authClient.signIn.social({ provider, callbackURL: env.NEXT_PUBLIC_WEB_URL, }); } catch (err) { console.error("[auth/sign-up]", err); setError("Failed to sign up. Please try again."); setIsLoading(false); } };Then call:
onClick={() => signUpWithProvider("google", setIsLoadingGoogle)}Note: The use of
signIn.social()for a sign-up page and the cross-domain callback toNEXT_PUBLIC_WEB_URLare both correct configurations in Better Auth.
🧹 Nitpick comments (9)
apps/desktop/src/renderer/screens/sign-in/index.tsx (1)
11-20: Add error handling and loading states to the sign-in mutation.The sign-in mutation currently lacks error handling and loading state feedback, which impacts user experience:
- Users receive no feedback if authentication fails
- No visual indication during the async operation
- Buttons remain enabled, allowing multiple rapid clicks
🔎 Suggested improvements
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) => { + console.error("[auth/signIn] Sign-in failed:", error); + // TODO: Show error toast/notification to user + }, + } + ); }; return ( <div className="flex flex-col h-full w-full bg-background"> <div className="h-12 w-full drag shrink-0" /> <div className="flex flex-1 items-center justify-center"> <div className="flex flex-col items-center w-full max-w-md px-8"> <div className="mb-8"> <SupersetLogo className="h-12 w-auto" /> </div> <div className="text-center mb-8"> <h1 className="text-xl font-semibold text-foreground mb-2"> Welcome to Superset </h1> <p className="text-sm text-muted-foreground"> Sign in to get started </p> </div> <div className="flex flex-col gap-3 w-full max-w-xs"> <Button variant="outline" size="lg" onClick={() => signIn("github")} className="w-full gap-3" + disabled={signInMutation.isPending} > <FaGithub className="size-5" /> - Continue with GitHub + {signInMutation.isPending ? "Signing in..." : "Continue with GitHub"} </Button> <Button variant="outline" size="lg" onClick={() => signIn("google")} className="w-full gap-3" + disabled={signInMutation.isPending} > <FcGoogle className="size-5" /> - Continue with Google + {signInMutation.isPending ? "Signing in..." : "Continue with Google"} </Button> </div>apps/api/src/proxy.ts (2)
5-9: Improve type safety forallowedOriginsarray.The conditional expression on Line 8 produces a
(string | false)type that requires runtime filtering. TypeScript may not properly infer that the filtered result isstring[].🔎 Proposed fix using conditional spreading
const allowedOrigins = [ env.NEXT_PUBLIC_WEB_URL, env.NEXT_PUBLIC_ADMIN_URL, - env.NODE_ENV === "development" && "http://localhost:5927", -].filter(Boolean); + ...(env.NODE_ENV === "development" ? ["http://localhost:5927"] : []), +];This approach maintains compile-time type safety without requiring runtime filtering.
As per coding guidelines: Avoid using
anytype - maintain type safety in TypeScript code.
28-28: Extract HTTP status code to named constant.Line 28 uses the magic number
204for the no-content response status. Per coding guidelines, hardcoded values should be extracted to named constants.🔎 Proposed refactor
+const HTTP_NO_CONTENT = 204; + export default function proxy(req: NextRequest) { const origin = req.headers.get("origin"); const corsHeaders = getCorsHeaders(origin); // Handle preflight if (req.method === "OPTIONS") { - return new NextResponse(null, { status: 204, headers: corsHeaders }); + return new NextResponse(null, { status: HTTP_NO_CONTENT, headers: corsHeaders }); }As per coding guidelines: Extract magic numbers and hardcoded values to named constants at module top.
apps/desktop/src/renderer/contexts/CollectionsProvider/collections.ts (1)
103-134: Extract table names to constants.The magic strings
"auth.members","auth.users","auth.organizations","tasks", and"repositories"should be extracted to named constants at module top for maintainability and to avoid typos.🔎 Proposed refactor
+const TABLE_NAMES = { + TASKS: "tasks", + REPOSITORIES: "repositories", + AUTH_MEMBERS: "auth.members", + AUTH_USERS: "auth.users", + AUTH_ORGANIZATIONS: "auth.organizations", +} as const; + export function createCollections({ token, activeOrgId, }: CreateCollectionsParams) { // ... const tasks = createCollection( electricCollectionOptions<SelectTask>({ id: `tasks-${activeOrgId}`, shapeOptions: { url: electricUrl, params: { - table: "tasks", + table: TABLE_NAMES.TASKS, org: activeOrgId, }, // ...Apply similar changes to all table references.
As per coding guidelines, extract magic numbers and hardcoded values to named constants at module top.
apps/desktop/src/main/index.ts (1)
43-48: Consider showing user-facing error feedback on auth failure.When authentication fails, the error is only logged to the console without notifying the user. Consider displaying an error dialog or notification so users understand why the authentication didn't complete.
🔎 Proposed enhancement
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", + result.error?.message || "Failed to complete authentication" + ); }apps/admin/src/app/(dashboard)/layout.tsx (1)
30-46: Clarify the dual session + TRPC user fetch pattern.The code fetches the session user (lines 30-40) and then separately queries for the user via TRPC (lines 42-46). This dual-fetch pattern raises questions:
Redundancy: If
session.useralready contains user information, why is the additional TRPC query necessary? Consider whether the TRPC query provides additional fields not available in the session.Data consistency: Could the session and TRPC user data become out of sync? For example, if a user is deleted but their session persists.
Performance: The additional TRPC call adds latency to every admin page load.
Please clarify the necessity of this pattern. If the TRPC user query provides essential data not in the session, consider adding a comment explaining why both are needed.
Additionally, consider logging unauthorized access attempts for security monitoring:
if (!session?.user) { // Log unauthorized access attempt redirect(env.NEXT_PUBLIC_WEB_URL); } if (!session.user.email?.endsWith(COMPANY.EMAIL_DOMAIN)) { // Log non-admin access attempt redirect(env.NEXT_PUBLIC_WEB_URL); }apps/web/src/proxy.ts (1)
5-14: Consider extractingpublicRoutesto a shared constant.The
apps/admin/src/proxy.ts(from relevant snippets) doesn't define public routes but has a similar pattern. If more routes need to be public across apps, a shared config could reduce duplication.apps/desktop/src/main/lib/auth/auth.ts (1)
76-81: Extract expiry buffer to a named constant.The 5-minute expiry buffer (line 79) is a magic number that should be extracted to a module-level constant for clarity and maintainability.
🔎 Proposed refactor
At the top of the file after imports:
+const TOKEN_EXPIRY_BUFFER_MS = 5 * 60 * 1000; // 5 minutes +const STATE_CLEANUP_AGE_MS = 10 * 60 * 1000; // 10 minutes + const TOKEN_FILE = join(SUPERSET_HOME_DIR, "auth-token.enc"); const stateStore = new Map<string, number>();Then use in isExpired:
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; }And in signIn (line 153):
- const tenMinutesAgo = Date.now() - 10 * 60 * 1000; + const cleanupThreshold = Date.now() - STATE_CLEANUP_AGE_MS; for (const [s, ts] of stateStore) { - if (ts < tenMinutesAgo) stateStore.delete(s); + if (ts < cleanupThreshold) stateStore.delete(s); }apps/web/src/app/(auth)/sign-up/[[...sign-up]]/page.tsx (1)
16-46: Consider extracting duplicate OAuth logic.The
signUpWithGoogleandsignUpWithGithubfunctions contain nearly identical logic. Consider extracting a shared function to reduce duplication.🔎 Proposed refactor
+ const signUpWithProvider = async ({ + provider, + setLoading, + }: { + 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(`[auth/sign-up] Sign up with ${provider} failed:`, err); + setError("Failed to sign up. Please try again."); + setLoading(false); + } + }; + const signUpWithGoogle = async () => { - setIsLoadingGoogle(true); - setError(null); - - try { - 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."); - setIsLoadingGoogle(false); - } + await signUpWithProvider({ + provider: "google", + setLoading: setIsLoadingGoogle, + }); }; const signUpWithGithub = async () => { - setIsLoadingGithub(true); - setError(null); - - try { - await authClient.signIn.social({ - provider: "github", - callbackURL: env.NEXT_PUBLIC_WEB_URL, - }); - } catch (err) { - console.error("Sign up failed:", err); - setError("Failed to sign up. Please try again."); - setIsLoadingGithub(false); - } + await signUpWithProvider({ + provider: "github", + setLoading: setIsLoadingGithub, + }); };
| const handleSignOut = async () => { | ||
| await authClient.signOut({ | ||
| fetchOptions: { | ||
| onSuccess: () => { | ||
| window.location.href = env.NEXT_PUBLIC_WEB_URL; | ||
| }, | ||
| }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Error handling still missing in sign-out flow.
The handleSignOut function continues to lack error handling as previously flagged. Without try-catch or an onError handler, sign-out failures (network issues, API errors) will leave users in an inconsistent auth state with no feedback.
As noted in the previous review, please add error handling with logging following the pattern [domain/operation] message per coding guidelines.
🤖 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 lacks error handling;
wrap the authClient.signOut call in a try/catch (or provide an onError callback
in fetchOptions) and log failures using the project pattern (e.g.,
processLogger.error or console.error with a prefix like "[auth/signOut] Failed
to sign out: <error>"). Ensure the success behavior (window.location.href =
env.NEXT_PUBLIC_WEB_URL) remains on success and surface a user-friendly fallback
or no-op on error.
| onInsert: async ({ transaction }) => { | ||
| const item = transaction.mutations[0].modified; | ||
| const result = await httpTrpcClient.task.create.mutate(item); | ||
| const result = await apiClient.task.create.mutate(item); | ||
| return { txid: result.txid }; | ||
| }, | ||
|
|
||
| onUpdate: async ({ transaction }) => { | ||
| const { modified } = transaction.mutations[0]; | ||
| const result = await httpTrpcClient.task.update.mutate(modified); | ||
| const result = await apiClient.task.update.mutate(modified); | ||
| return { txid: result.txid }; | ||
| }, | ||
|
|
||
| onDelete: async ({ transaction }) => { | ||
| const item = transaction.mutations[0].original; | ||
| const result = await httpTrpcClient.task.delete.mutate(item.id); | ||
| const result = await apiClient.task.delete.mutate(item.id); | ||
| return { txid: result.txid }; | ||
| }, |
There was a problem hiding this comment.
Add error handling to mutation handlers.
The onInsert, onUpdate, and onDelete handlers lack error handling. If the API mutation fails, the error will propagate uncaught, potentially leaving the local collection in an inconsistent state.
🔎 Recommended error handling pattern
onInsert: async ({ transaction }) => {
const item = transaction.mutations[0].modified;
- const result = await apiClient.task.create.mutate(item);
- return { txid: result.txid };
+ try {
+ const result = await apiClient.task.create.mutate(item);
+ return { txid: result.txid };
+ } catch (error) {
+ console.error("[collections/tasks] Insert failed:", error);
+ throw error;
+ }
},Apply similar error handling to all mutation handlers across all collections (tasks, repositories).
As per coding guidelines, never swallow errors silently - at minimum log them with context.
📝 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.
| onInsert: async ({ transaction }) => { | |
| const item = transaction.mutations[0].modified; | |
| const result = await httpTrpcClient.task.create.mutate(item); | |
| const result = await apiClient.task.create.mutate(item); | |
| return { txid: result.txid }; | |
| }, | |
| onUpdate: async ({ transaction }) => { | |
| const { modified } = transaction.mutations[0]; | |
| const result = await httpTrpcClient.task.update.mutate(modified); | |
| const result = await apiClient.task.update.mutate(modified); | |
| return { txid: result.txid }; | |
| }, | |
| onDelete: async ({ transaction }) => { | |
| const item = transaction.mutations[0].original; | |
| const result = await httpTrpcClient.task.delete.mutate(item.id); | |
| const result = await apiClient.task.delete.mutate(item.id); | |
| return { txid: result.txid }; | |
| }, | |
| onInsert: async ({ transaction }) => { | |
| const item = transaction.mutations[0].modified; | |
| try { | |
| const result = await apiClient.task.create.mutate(item); | |
| return { txid: result.txid }; | |
| } catch (error) { | |
| console.error("[collections/tasks] Insert failed:", error); | |
| throw error; | |
| } | |
| }, | |
| onUpdate: async ({ transaction }) => { | |
| const { modified } = transaction.mutations[0]; | |
| try { | |
| const result = await apiClient.task.update.mutate(modified); | |
| return { txid: result.txid }; | |
| } catch (error) { | |
| console.error("[collections/tasks] Update failed:", error); | |
| throw error; | |
| } | |
| }, | |
| onDelete: async ({ transaction }) => { | |
| const item = transaction.mutations[0].original; | |
| try { | |
| const result = await apiClient.task.delete.mutate(item.id); | |
| return { txid: result.txid }; | |
| } catch (error) { | |
| console.error("[collections/tasks] Delete failed:", error); | |
| throw error; | |
| } | |
| }, |
🤖 Prompt for AI Agents
In @apps/desktop/src/renderer/contexts/CollectionsProvider/collections.ts around
lines 53 - 67, Wrap each mutation handler (onInsert, onUpdate, onDelete) in a
try/catch, log the caught error with contextual info (operation name and the
item/id involved) using the available logger (e.g., processLogger or
console.error) and then rethrow the error so it doesn’t get swallowed and
calling code can handle rollback; update the bodies of
onInsert/onUpdate/onDelete to perform the apiClient.task.*.mutate call inside
try, log on catch (including the mutation payload and error), and rethrow the
error.
| const switchOrganization = async (newOrgId: string) => { | ||
| await setActiveOrg.mutateAsync({ organizationId: newOrgId }); | ||
| }; |
There was a problem hiding this comment.
Add error handling to organization switch.
The switchOrganization handler lacks error handling. If the mutation fails, the user won't receive feedback, and the error will propagate uncaught.
🔎 Recommended error handling
const switchOrganization = async (newOrgId: string) => {
- await setActiveOrg.mutateAsync({ organizationId: newOrgId });
+ try {
+ await setActiveOrg.mutateAsync({ organizationId: newOrgId });
+ } catch (error) {
+ console.error("[OrganizationSwitcher] Switch failed:", error);
+ // Optionally show user feedback via toast
+ }
};As per coding guidelines, never swallow errors silently - at minimum log them with context.
📝 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 switchOrganization = async (newOrgId: string) => { | |
| await setActiveOrg.mutateAsync({ organizationId: newOrgId }); | |
| }; | |
| const switchOrganization = async (newOrgId: string) => { | |
| try { | |
| await setActiveOrg.mutateAsync({ organizationId: newOrgId }); | |
| } catch (error) { | |
| console.error("[OrganizationSwitcher] Switch failed:", error); | |
| // Optionally show user feedback via toast | |
| } | |
| }; |
🤖 Prompt for AI Agents
In
@apps/desktop/src/renderer/screens/main/components/TasksView/components/OrganizationSwitcher/OrganizationSwitcher.tsx
around lines 40 - 42, The switchOrganization handler (switchOrganization)
currently calls setActiveOrg.mutateAsync without error handling; wrap the await
call in a try/catch, log the error with context (e.g., include the newOrgId and
error) using the app logger or console.error, and surface user feedback
(toast/notification or error state) when the mutation fails so the error is not
swallowed and the user is informed; ensure you still rethrow only if appropriate
for upstream handling or return after showing the error.
| export function TasksView() { | ||
| return ( | ||
| <OrganizationsProvider> | ||
| <ActiveOrganizationProvider> | ||
| <CollectionsProvider> | ||
| <TasksViewContent /> | ||
| </CollectionsProvider> | ||
| </ActiveOrganizationProvider> | ||
| <CollectionsProvider> | ||
| <TasksViewContent /> | ||
| </CollectionsProvider> | ||
| </OrganizationsProvider> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unnecessary OrganizationsProvider wrapper.
The OrganizationsProvider appears unused—no component in this tree calls useOrganizations(). The CollectionsProvider derives its organization context from the tRPC session subscription (trpc.auth.onSessionChange), not from OrganizationsProvider. This wrapper should be removed.
export function TasksView() {
return (
- <OrganizationsProvider>
- <CollectionsProvider>
- <TasksViewContent />
- </CollectionsProvider>
- </OrganizationsProvider>
+ <CollectionsProvider>
+ <TasksViewContent />
+ </CollectionsProvider>
);
}📝 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 function TasksView() { | |
| return ( | |
| <OrganizationsProvider> | |
| <ActiveOrganizationProvider> | |
| <CollectionsProvider> | |
| <TasksViewContent /> | |
| </CollectionsProvider> | |
| </ActiveOrganizationProvider> | |
| <CollectionsProvider> | |
| <TasksViewContent /> | |
| </CollectionsProvider> | |
| </OrganizationsProvider> | |
| ); | |
| } | |
| export function TasksView() { | |
| return ( | |
| <CollectionsProvider> | |
| <TasksViewContent /> | |
| </CollectionsProvider> | |
| ); | |
| } |
🤖 Prompt for AI Agents
In @apps/desktop/src/renderer/screens/main/components/TasksView/TasksView.tsx
around lines 347 - 355, The OrganizationsProvider wrapper around TasksView is
unnecessary; remove OrganizationsProvider so TasksView returns only
CollectionsProvider wrapping TasksViewContent. Specifically, edit the TasksView
component (function TasksView) to delete the OrganizationsProvider element and
its opening/closing tags, leaving CollectionsProvider and TasksViewContent
intact; you can verify no useOrganizations() calls exist under TasksView and
that CollectionsProvider already consumes organization data via
trpc.auth.onSessionChange.
|
|
||
| export async function CTAButtons() { | ||
| const { userId } = await auth(); | ||
| const session = await auth.api.getSession({ headers: await headers() }); |
There was a problem hiding this comment.
Add error handling for session retrieval.
The auth.api.getSession() call lacks error handling, which could cause the entire component to fail if the auth service is unavailable or if there's a network issue. According to the coding guidelines, errors should never be silently swallowed—at minimum, log them with context.
Consider wrapping the session check in a try-catch block and falling back to the unauthenticated state to ensure users can still access the sign-in option.
🔎 Proposed fix with error handling
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("[CTAButtons] Failed to retrieve session:", error);
+ // Fall through to unauthenticated state
+ }
if (session) {As per coding guidelines, errors should be logged with context and handled gracefully rather than causing component failures.
📝 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 session = await auth.api.getSession({ headers: await headers() }); | |
| export async function CTAButtons() { | |
| let session; | |
| try { | |
| session = await auth.api.getSession({ headers: await headers() }); | |
| } catch (error) { | |
| console.error("[CTAButtons] Failed to retrieve session:", error); | |
| // Fall through to unauthenticated state | |
| } | |
| if (session) { |
🤖 Prompt for AI Agents
In @apps/marketing/src/app/components/CTAButtons/CTAButtons.tsx at line 9, Wrap
the session retrieval in CTAButtons (the line calling auth.api.getSession) in a
try-catch so failures don't crash the component: catch any error from
auth.api.getSession, log it with contextual information (e.g., "CTAButtons:
failed to get session" plus the error), and fall back to an unauthenticated
state (treat session as null/undefined) so the component can render the sign-in
option gracefully.
| const handleSignOut = async () => { | ||
| await authClient.signOut(); | ||
| router.push("/sign-in"); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual Header.tsx file to see the context
cat -n apps/web/src/app/\(dashboard\)/components/Header/Header.tsx | head -50Repository: superset-sh/superset
Length of output: 1882
🏁 Script executed:
# Search for authClient definition in the codebase
rg "authClient\s*=" apps/web/src/app/\(dashboard\)/components/Header/Header.tsx -A 5 -B 5Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Look for authClient imports and type definitions
rg "authClient|auth" apps/web/src/app/\(dashboard\)/components/Header/Header.tsx -B 2 -A 2Repository: superset-sh/superset
Length of output: 474
🌐 Web query:
Better Auth signOut method error handling documentation JavaScript TypeScript
💡 Result:
Short summary — signOut returns a response object with data and error (same shape as other client calls). Handle errors by checking the returned error, or by passing fetchOptions.onError / onSuccess callbacks. The client is TypeScript-first so you can type the response via the client instance. [1][2][3]
Example (JavaScript):
const { data, error } = await authClient.signOut();
if (error) {
// handle or show error.message / error.status
}
Example with callbacks (JS):
await authClient.signOut({
fetchOptions: {
onSuccess() { router.push("/login"); },
onError(ctx) { /* ctx contains error details */ }
}
});
Example (TypeScript — typed response):
const authClient = createAuthClient();
type Session = typeof authClient.$Infer.Session;
const res = await authClient.signOut();
if (res.error) {
console.error(res.error.message, res.error.status);
// optional: switch on res.error.code using authClient.$ERROR_CODES
}
Notes:
- Use res.error?.code or authClient.$ERROR_CODES to map server error codes to messages. [1]
- signOut is documented under Basic Usage / Email & Password; it accepts the same fetchOptions pattern as other client methods. [2][3]
Sources:
[1] Better Auth — Client: Handling Errors. (better-auth.com)
[2] Better Auth — Basic Usage (signOut). (beta.better-auth.com)
[3] Better Auth — Email & Password (Sign Out). (beta.better-auth.com)
Citations:
- 1: https://www.better-auth.com/docs/concepts/client?utm_source=openai
- 2: https://beta.better-auth.com/docs/basic-usage?utm_source=openai
- 3: https://beta.better-auth.com/docs/authentication/email-password?utm_source=openai
Add error handling to the sign-out flow using the correct Better Auth pattern.
The handleSignOut function doesn't handle potential errors from authClient.signOut(). Per the coding guideline "Never swallow errors silently," errors must be handled. When signOut() fails, the user is redirected anyway, leaving them in an inconsistent authentication state.
Note: authClient.signOut() returns a response object with error property rather than throwing errors, so the response must be checked explicitly.
🔎 Proposed fix with proper error handling
const handleSignOut = async () => {
- await authClient.signOut();
- router.push("/sign-in");
+ const { error } = await authClient.signOut();
+ if (error) {
+ console.error("[Header/signOut] Sign-out failed:", error);
+ return;
+ }
+ router.push("/sign-in");
};📝 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(); | |
| router.push("/sign-in"); | |
| }; | |
| const handleSignOut = async () => { | |
| const { error } = await authClient.signOut(); | |
| if (error) { | |
| console.error("[Header/signOut] Sign-out failed:", error); | |
| return; | |
| } | |
| router.push("/sign-in"); | |
| }; |
🤖 Prompt for AI Agents
In @apps/web/src/app/(dashboard)/components/Header/Header.tsx around lines 24 -
27, The handleSignOut function must check the signOut response instead of
assuming it throws: call authClient.signOut(), inspect the returned object for
response.error, and only navigate (router.push("/sign-in")) when there is no
error; on error, surface it (e.g., processLogger/console.error and show a
user-facing message or toast) so the user isn’t redirected into an inconsistent
state; update the handleSignOut implementation to follow this Better Auth
pattern and reference the authClient.signOut and router.push calls when making
the change.
| if (!state) { | ||
| return ( | ||
| <div className="flex min-h-screen flex-col items-center justify-center bg-background p-4"> | ||
| <p className="text-xl text-muted-foreground">Missing auth state</p> | ||
| <p className="text-muted-foreground/70"> | ||
| Please try signing in again from the desktop app. | ||
| </p> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| // Get session from Better Auth | ||
| let session: Awaited<ReturnType<typeof auth.api.getSession>> | null = null; | ||
| 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> | ||
| ); | ||
| } | ||
|
|
||
| if (!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> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract repeated error UI to a helper component or function.
The error UI rendering is duplicated three times (lines 17-22, 33-38, 45-49) with identical structure, violating DRY principles and making maintenance harder.
🤖 Prompt for AI Agents
In @apps/web/src/app/auth/desktop/success/page.tsx around lines 15 - 50, The
three identical error UI return blocks (when !state, when auth.api.getSession
throws, and when !session) should be replaced by a single reusable helper:
extract the JSX into a small component or function like AuthErrorMessage (or
renderAuthError) defined above the page component, and then return that helper
in each of the three places instead of duplicating the markup; reference the
existing symbols state, auth.api.getSession, and headers() where you replace the
returns, and if you prefer make the helper accept an optional title/body props
for future variation.
| const org = await auth.api.createOrganization({ | ||
| body: { | ||
| name: `${user.name}'s Workspace`, | ||
| slug: `${user.id.slice(0, 8)}-workspace`, | ||
| userId: user.id, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
better-auth databaseHooks user.create.after auth.api access pattern
💡 Result:
Short answer
- Use the databaseHooks.user.create.after hook for side-effects (create orgs, Stripe customers, analytics) but perform those side-effects with your own DB/ORM or the adapter/internal-adapter — not by calling auth.api from inside the hook (there are reported race/availability issues). Use the hook’s ctx to access adapter/DB utilities or schedule a background job if you need reliability. (better-auth.com)
Pattern (concise)
- Hook location: databaseHooks.user.create.after — runs after the user record is created (side-effects only). (better-auth.com)
- Prefer: call your DB/ORM or adapter/internal-adapter directly (ctx.context.adapter or your own db client) to create related records and link them to the new user. This avoids calling auth.api inside the hook. (better-auth.com)
- If you must call auth.api (e.g., createOrganization helper), prefer doing it from a background/queued job triggered by the hook (enqueue job with user id) to avoid race conditions reported when calling auth.api inside the after hook. (github.com)
Minimal example (idea)
- In auth.ts:
- databaseHooks.user.create.after: enqueue a job with user.id (or call your DB to create org immediately using your ORM/adapter).
- Worker: pick up job and call auth.api.createOrganization or create the organization via your DB + link to user.
References
- Better Auth — Database hooks docs (before/after description & examples). (better-auth.com)
- Better Auth — Hooks / ctx.adapter and context utilities (use adapter/ORM). (better-auth.com)
- GitHub issue reporting failures when calling auth.api from user.create.after (race/availability; prefer background job or adapter). (github.com)
If you want, I can paste a small code snippet showing (A) enqueue-job pattern or (B) immediate ORM call inside the after hook. Which would you prefer?
Citations:
- 1: https://www.better-auth.com/docs/concepts/database?utm_source=openai
- 2: https://www.better-auth.com/docs/concepts/database?utm_source=openai
- 3: https://www.better-auth.com/docs/concepts/hooks?utm_source=openai
- 4:
createOrganizationin user -> after -> create hook returnsnullsince user could not be found in the database better-auth/better-auth#4718 - 5: https://www.better-auth.com/docs/concepts/database?utm_source=openai
- 6: https://www.better-auth.com/docs/concepts/hooks?utm_source=openai
- 7:
createOrganizationin user -> after -> create hook returnsnullsince user could not be found in the database better-auth/better-auth#4718
🏁 Script executed:
# First, let's examine the actual code context
cat -n packages/auth/src/index.ts | head -80Repository: superset-sh/superset
Length of output: 2724
Refactor to use adapter directly or background job instead of calling auth.api inside the hook.
Calling auth.api.createOrganization() inside user.create.after is a documented anti-pattern in better-auth and creates race conditions. Instead, use the adapter directly via ctx.context.adapter to create the organization, or enqueue a background job to handle organization creation after the user is created. See better-auth docs on databaseHooks for recommended patterns.
🤖 Prompt for AI Agents
In @packages/auth/src/index.ts around lines 57 - 63, Inside the
user.create.after hook, replace the direct call to auth.api.createOrganization
with the adapter or a background job: use the hook context's adapter
(ctx.context.adapter) to create the organization record (e.g., via the adapter's
createOrganization or equivalent method) or enqueue a job to perform
createOrganization asynchronously; specifically remove the
auth.api.createOrganization call inside user.create.after and perform the
organization creation through ctx.context.adapter (or dispatch to your
background job queue) to avoid the documented anti-pattern and race conditions.
| "active_organization_id": { | ||
| "name": "active_organization_id", | ||
| "type": "uuid", | ||
| "primaryKey": false, | ||
| "notNull": false | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the snapshot file exists and examine it
fd "0006_snapshot.json" --type fRepository: superset-sh/superset
Length of output: 157
🏁 Script executed:
# Also search for the schema files mentioned
fd "auth.ts" --type fRepository: superset-sh/superset
Length of output: 133
🏁 Script executed:
# Search for activeOrganizationId or active_organization_id in schema files
rg "activeOrganizationId|active_organization_id" --type ts --type json -A 3 -B 1Repository: superset-sh/superset
Length of output: 10034
🏁 Script executed:
# View the auth.ts schema file around activeOrganizationId
cat packages/db/src/schema/auth.tsRepository: superset-sh/superset
Length of output: 4546
🏁 Script executed:
# Check the full sessions table definition in the snapshot
grep -A 50 '"sessions"' packages/db/drizzle/meta/0006_snapshot.json | head -100Repository: superset-sh/superset
Length of output: 2493
Add foreign key constraint to activeOrganizationId field.
The activeOrganizationId field in the sessions table lacks a foreign key constraint to the organizations table, allowing orphaned references if an organization is deleted while sessions still reference it.
Add the foreign key reference in packages/db/src/schema/auth.ts:
activeOrganizationId: uuid("active_organization_id").references(
() => organizations.id,
{ onDelete: "set null" }
)Then regenerate the migration with drizzle-kit generate to update the snapshot.
🤖 Prompt for AI Agents
In @packages/db/drizzle/meta/0006_snapshot.json around lines 488 - 493, The
sessions table's activeOrganizationId lacks a foreign key constraint, allowing
orphaned references; update the schema by changing the activeOrganizationId
column definition to reference organizations.id with onDelete set to "set null"
(use the uuid(...).references(() => organizations.id, { onDelete: "set null" })
pattern in the auth schema where activeOrganizationId is defined), then re-run
the migration generator (drizzle-kit generate) to produce an updated
migration/snapshot reflecting the new foreign key.
Regenerated lockfile after rebasing onto latest origin/main. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
saddlepaddle
force-pushed
the
feat/better-auth-restoration
branch
from
74bd29a to
5f0bd36
Compare
There was a problem hiding this comment.
Actionable comments posted: 20
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 (6)
packages/trpc/src/router/organization/organization.ts (1)
68-76: Silent failure when organization creation fails.If the
insertreturns an empty array (e.g., due to a constraint violation),organizationwill beundefined. The mutation silently skips member creation and returnsundefinedinstead of throwing an error, leaving the caller unaware of the failure.🔎 Proposed fix
- if (organization) { - await db.insert(members).values({ - organizationId: organization.id, - userId: ctx.session.user.id, - role: "owner", - }); - } - - return organization; + if (!organization) { + throw new Error("[organization/create] Failed to create organization"); + } + + await db.insert(members).values({ + organizationId: organization.id, + userId: ctx.session.user.id, + role: "owner", + }); + + return organization;packages/trpc/src/router/user/user.ts (1)
11-20: Add explicit ordering or clarify single-organization assumption.The
myOrganizationendpoint usesfindFirstwithout anorderByclause. The members table schema allows users to have multiple memberships (no unique constraint onuserId+organizationId), and the existence of themyOrganizationsendpoint confirms this is intended. Without ordering,findFirstreturns an arbitrary result, causing non-deterministic behavior where different requests may return different organizations.Suggested fixes
Option 1: Add explicit ordering (e.g., by creation date)
const membership = await db.query.members.findFirst({ where: eq(members.userId, ctx.session.user.id), with: { organization: true, }, orderBy: (members, { asc }) => [asc(members.createdAt)], });Option 2: If users should only have one organization, add a comment clarifying this assumption
// Note: Users are expected to have exactly one organization membership const membership = await db.query.members.findFirst({ where: eq(members.userId, ctx.session.user.id), with: { organization: true, }, });Option 3: Add a "primary" or "current" flag to the members schema
const membership = await db.query.members.findFirst({ where: and( eq(members.userId, ctx.session.user.id), eq(members.isPrimary, true) ), with: { organization: true, }, });apps/api/src/app/api/electric/[...path]/utils.ts (1)
37-40: Function signature needs userId parameter and should use object parameters.Two issues:
Critical: The function needs a
userIdparameter to properly authorize theauth.organizationscase (see separate comment on lines 51-90). The current signature cannot securely determine which organizations the authenticated user can access.Coding guideline: Functions with 2+ parameters should use object parameters for self-documentation and extensibility.
🔎 Proposed signature change
export async function buildWhereClause( - tableName: string, - organizationId: string, + params: { + tableName: string; + organizationId: string; + userId: string; + } ): Promise<WhereClause | null> {Update callers to pass an object and use
params.tableName,params.organizationId, andparams.userIdwithin the function.Based on coding guidelines: Functions with 2+ parameters should accept a single params object with named properties.
apps/api/src/proxy.ts (1)
22-37: Add error handling for robustness.The proxy function has no error handling. If
NextResponse.next()or header operations fail, errors would propagate uncaught. Per coding guidelines: "Never swallow errors silently - at minimum log them with context."🔎 Suggested error handling
export default function proxy(req: NextRequest) { - const origin = req.headers.get("origin"); - const corsHeaders = getCorsHeaders(origin); + try { + const origin = req.headers.get("origin"); + const corsHeaders = getCorsHeaders(origin); - // Handle preflight - if (req.method === "OPTIONS") { - return new NextResponse(null, { status: 204, headers: corsHeaders }); - } + // Handle preflight + if (req.method === "OPTIONS") { + return new NextResponse(null, { status: 204, headers: corsHeaders }); + } - // Add CORS headers to all responses - const response = NextResponse.next(); - for (const [key, value] of Object.entries(corsHeaders)) { - response.headers.set(key, value); - } - return response; + // Add CORS headers to all responses + const response = NextResponse.next(); + for (const [key, value] of Object.entries(corsHeaders)) { + response.headers.set(key, value); + } + return response; + } catch (error) { + console.error("[api/proxy] Error in proxy:", error); + return new NextResponse("Internal Server Error", { status: 500 }); + } }apps/api/src/app/api/integrations/linear/connect/route.ts (1)
26-38: Add error handling for database query.The membership lookup query lacks error handling. Database failures (connection issues, timeouts, schema mismatches) will crash the request handler with an unhandled promise rejection.
🔎 Add try-catch wrapper
+ try { const membership = await db.query.members.findFirst({ where: and( eq(members.organizationId, organizationId), eq(members.userId, session.user.id), ), }); if (!membership) { return Response.json( { error: "User is not a member of this organization" }, { status: 403 }, ); } + } catch (error) { + console.error("[integrations/linear/connect] Database query failed:", error); + return Response.json( + { error: "Failed to verify organization membership" }, + { status: 500 }, + ); + }Note: Uses prefixed console logging as per coding guidelines.
apps/web/src/app/(auth)/layout.tsx (1)
8-12: Add Suspense boundary around async session fetch in auth layout.This async Server Component fetches session data during render. An async layout component will block the entire route from rendering until
auth.api.getSession()completes. Without a Suspense boundary, Next.js cannot stream the initial HTML early, impacting perceived performance. Wrap the layout with<Suspense>and provide a fallback UI.
🤖 Fix all issues with AI agents
In @apps/admin/src/app/(dashboard)/layout.tsx:
- Around line 38-40: The current check uses
session.user.email?.endsWith(COMPANY.EMAIL_DOMAIN) which can be spoofed; instead
extract the email domain and compare exactly. Locate the guard around
session.user.email (and COMPANY.EMAIL_DOMAIN) and replace the endsWith check
with logic that safely parses the domain (e.g., split on "@", handle
missing/invalid email) and does a strict equality against COMPANY.EMAIL_DOMAIN
(or the literal domain), then call redirect(env.NEXT_PUBLIC_WEB_URL) when the
domain does not match.
- Around line 30-32: The auth.api.getSession call in layout.tsx is unprotected
and can throw, causing the layout to crash; wrap the call to auth.api.getSession
(where you assign to session) in a try/catch, handle errors by logging (or
processLogger) and return a safe fallback (e.g., null session or redirect to
login) so the component can render gracefully; ensure you still await headers()
inside the try and keep the variable name session and any downstream checks
intact so existing conditional rendering works with the fallback.
In @apps/admin/src/proxy.ts:
- Around line 17-19: The guard currently calls session.user.email.endsWith(...)
which can throw if email is null/undefined; update the check around session to
verify session?.user?.email exists before calling endsWith (e.g., change the
conditional to first ensure session.user.email is truthy or use optional
chaining) so that if email is missing you still redirect via
NextResponse.redirect(new URL(env.NEXT_PUBLIC_WEB_URL)); reference the symbols
session, session.user.email, COMPANY.EMAIL_DOMAIN, NextResponse.redirect and
env.NEXT_PUBLIC_WEB_URL when making the change.
- Line 8: Update the proxy handler signature to accept the NextRequest
parameter: change the function declaration for proxy to take a single parameter
of type NextRequest (i.e., proxy(request: NextRequest)) and import NextRequest
from 'next/server'; then replace any usage of implicit globals with the
passed-in request (e.g., request.url, request.nextUrl, request.headers,
request.cookies) so proxy logic uses the provided request context.
- Around line 9-11: The call to auth.api.getSession (used to populate session)
lacks error handling; wrap the await auth.api.getSession({ headers: await
headers() }) in a try-catch, log the caught error with context (e.g., using the
same logger pattern as in success/page.tsx), and return an appropriate redirect
or error response instead of letting the proxy crash; update any callers that
expect session to handle a null/undefined session return if necessary.
In @apps/api/src/app/api/electric/[...path]/utils.ts:
- Around line 51-90: The code in the "auth.organizations" branch currently picks
an arbitrary user from members for the given organizationId (userMemberships[0])
which causes an authorization bypass; change the function that builds this
fragment to accept an authenticated userId parameter, first verify membership by
querying db.query.members.findFirst or findMany with where: and
eq(members.userId, userId) && eq(members.organizationId, organizationId) and
return "1 = 0" if not a member, then fetch all organizations for that
authenticated user by querying db.query.members.findMany where
eq(members.userId, userId) (use the userId param instead of
userMemberships[0].userId), build orgIds from those results, construct
whereExpr/inArray and the QueryBuilder SQL fragment as before. Ensure references
to members, organizations, organizationId, userId, QueryBuilder, inArray, and
sql.identifier(organizations.id.name) are preserved.
In @apps/desktop/src/main/lib/api-client.ts:
- Around line 28-44: The fetch wrapper in api-client.ts uses console.log for
error conditions and lacks the required “[domain/operation]” prefix; update the
async fetch(url, options) implementation so both error paths use console.error
and include a prefixed context like “[api-client/fetch]” (e.g., when
response.status is 401/403 before calling authService.signOut, and in the catch
block include the caught error details), preserving the existing behavior of
calling authService.signOut() and rethrowing the error.
In @apps/web/src/app/(auth)/layout.tsx:
- Around line 13-19: Wrap the call to auth.api.getSession({ headers: await
headers() }) in a try/catch to prevent the layout from crashing on network/auth
errors; if the call throws, catch the error, log it with the prefixed console
message style (e.g. "[auth] getSession error:" plus error), and treat the result
as no session (i.e., do not call redirect("/")); keep the existing redirect("/")
behavior only when session is truthy.
In @apps/web/src/app/(auth)/sign-up/[[...sign-up]]/page.tsx:
- Line 26: Replace the bare console.error calls that log sign-up failures with
prefixed logging per guidelines: find the console.error("Sign up failed:", err)
call and the other console.error occurrence in this sign-up page and prepend a
context prefix like "[auth/sign-up]" to the message (e.g. "[auth/sign-up] Sign
up failed: <error details>"), ensuring you still include the err value so full
error details are logged.
In @apps/web/src/app/auth/desktop/success/page.tsx:
- Around line 66-68: The session expiry uses a magic number in the expiresAt
calculation; extract the value 60 * 60 * 24 * 30 * 1000 into a clearly named
module-level constant (e.g., SESSION_EXPIRY_MS or THIRTY_DAYS_MS) placed at the
top of the file, then replace the inline expression in the expiresAt assignment
with that constant to improve clarity and follow the magic-number guideline.
In @packages/auth/src/env.ts:
- Line 18: NEXT_PUBLIC_COOKIE_DOMAIN currently accepts any string; update its
Zod schema in packages/auth/src/env.ts so it validates cookie domain format
(allow optional leading dot and valid domain labels, e.g., .example.com or
example.com). Replace z.string() for NEXT_PUBLIC_COOKIE_DOMAIN with a
regex-based z.string().regex(...) or z.string().refine(...) that checks for an
optional leading '.' followed by valid domain label characters and TLD, and
provide a clear validation error message; reference the
NEXT_PUBLIC_COOKIE_DOMAIN schema symbol when making this change.
- Line 26: Replace the hard-coded skipValidation: true with an opt-in boolean
derived from a clearly named environment variable (e.g. SKIP_ENV_VALIDATION) so
validation is enforced by default; update the env schema initialization in
env.ts to compute skipValidation = process.env.SKIP_ENV_VALIDATION === 'true'
(or equivalent truthy check) and document that the env var should only be used
for CI/builds, ensuring required auth secrets like BETTER_AUTH_SECRET,
GH_CLIENT_SECRET, and GOOGLE_CLIENT_SECRET are validated unless the opt-in flag
is explicitly set.
- Line 6: Replace the brittle hardcoded "../../../.env" path used in the
config(...) call in env.ts with a cwd-relative resolution or configurable env
path: call config with path resolved from process.cwd() (e.g.
path.resolve(process.cwd(), ".env")) or read an ENV_PATH env variable and fall
back to process.cwd(), and include override: true when needed by the desktop
app; update the config(...) invocation to use the resolved path (and override
option) so environment loading is robust across different workspace depths and
build outputs.
In @packages/db/drizzle/0006_add_better_auth_tables.sql:
- Around line 119-143: The slug generation for personal workspaces is unsafe
because it uses LEFT(u.id::text, 8) in both the INSERT INTO
"auth"."organizations" and the JOIN for adding members, creating high collision
risk; fix by using the full user UUID (u.id::text) or another
collision-resistant value for the organization slug when creating orgs and when
joining back (replace LEFT(u.id::text, 8) || '-workspace' with a full-UUID-based
slug or deterministic unique value), and update the JOIN condition in the INSERT
INTO "auth"."members" to match that same slug strategy so lookups are
unambiguous; if short slugs are required instead, implement a deterministic
deduplication strategy (e.g., append a sequence or random suffix with uniqueness
check and retry) and apply it consistently in both the organization creation and
the member-assignment JOIN.
In @packages/db/src/env.ts:
- Line 22: The hardcoded skipValidation: true disables validation permanently;
change the skipValidation logic in the env config so validation runs in
production and only skips when a developer explicitly opts out locally (e.g.,
use process.env.NODE_ENV and an opt-out env var like SKIP_ENV_VALIDATION to
decide skipValidation), ensure the DATABASE_URL and DATABASE_URL_UNPOOLED checks
remain active by default, and update the skipValidation assignment referenced in
the env configuration object (the skipValidation property in
packages/db/src/env.ts) accordingly.
In @packages/db/src/schema/auth.ts:
- Around line 29-47: The sessions table's activeOrganizationId column lacks a
foreign key constraint; update the sessions schema by changing the
activeOrganizationId definition in the sessions table to reference
organizations.id (e.g., .references(() => organizations.id, { onDelete: "SET
NULL" }) or your preferred onDelete behavior) so the DB enforces referential
integrity; locate the activeOrganizationId symbol in the sessions table
definition and add the .references call (and adjust or add an index if desired).
🟡 Minor comments (1)
packages/trpc/src/router/organization/organization.ts-82-82 (1)
82-82: Inconsistent UUID validation foridfield.The
idparameter usesz.string()without UUID validation, while other procedures (byId,delete,addMember,removeMember) usez.string().uuid(). This inconsistency could allow malformed IDs to reach the database layer.🔎 Proposed fix
- id: z.string(), + id: z.string().uuid(),
🧹 Nitpick comments (15)
apps/desktop/src/renderer/contexts/MonacoProvider/index.ts (1)
1-8: Consider importing directly from concrete files to avoid potential circular dependencies.This barrel export adds an indirection layer. Per coding guidelines, prefer importing from concrete files directly to avoid barrel file abuse and circular dependencies.
You could import directly from
./MonacoProvider/MonacoProviderinstead of going through this index, or verify this pattern is intentional and doesn't create circular dependencies.#!/bin/bash # Description: Check for circular dependency risks with MonacoProvider barrel # Expected: No imports from other contexts back into MonacoProvider # Find files that import from MonacoProvider echo "=== Files importing from MonacoProvider ===" rg -n "from.*MonacoProvider" apps/desktop/src/renderer/ -g '!*.test.*' -A 2 # Check if MonacoProvider imports from other contexts (potential circular dep) echo -e "\n=== MonacoProvider imports from other contexts ===" rg -n "from.*\.\./.*Provider" apps/desktop/src/renderer/contexts/MonacoProvider/MonacoProvider.tsx -A 1 || echo "No cross-context imports found (good)"apps/desktop/src/renderer/screens/main/components/TasksView/components/OrganizationSwitcher/OrganizationSwitcher.tsx (1)
64-81: Consider adding a loading indicator when switching organizations.While the dropdown renders correctly, the
setActiveOrg.isPendingstate from the mutation could be used to show feedback during the switch operation (e.g., disable items or show a spinner).🔎 Optional enhancement
<DropdownMenuItem key={organization.id} onSelect={() => switchOrganization(organization.id)} className="gap-2" + disabled={setActiveOrg.isPending} >packages/db/src/utils/sql.ts (1)
32-32: Add validation for NaN after parsing.
Number.parseInt()can returnNaNif the txid string contains non-numeric characters. While unlikely in normal operation, adding a safety check would make this more robust.🔎 Proposed defensive check
- return Number.parseInt(txid, 10); + const parsedTxid = Number.parseInt(txid, 10); + if (Number.isNaN(parsedTxid)) { + throw new Error(`Invalid transaction ID format: ${txid}`); + } + return parsedTxid;apps/web/src/app/auth/desktop/success/page.tsx (1)
62-64: Consider using a static import fornode:crypto.The dynamic import adds unnecessary complexity since this is a server component running in Node.js. Static imports are cleaner and allow better tree-shaking analysis.
🔎 Proposed fix
Add at file top with other imports:
import crypto from "node:crypto";Then simplify the usage:
- // Generate a unique session token for the desktop app - const crypto = await import("node:crypto"); - const token = crypto.randomBytes(32).toString("base64url"); + // Generate a unique session token for the desktop app + const token = crypto.randomBytes(32).toString("base64url");apps/api/src/app/api/electric/[...path]/utils.ts (2)
58-58: Extract repeated "1 = 0" magic string to a named constant.The SQL fragment
"1 = 0"(representing an always-false condition) appears 4 times. Extract it to a constant at the module top for clarity and maintainability.🔎 Proposed refactor
At the top of the file, after imports:
+const EMPTY_WHERE_CLAUSE = "1 = 0"; + export type AllowedTable =Then replace all occurrences:
- return { fragment: "1 = 0", params: [] }; + return { fragment: EMPTY_WHERE_CLAUSE, params: [] };Based on coding guidelines: Extract magic numbers and hardcoded values to named constants at module top.
Also applies to: 63-63, 72-72, 98-98
37-115: Consider adding error context for database queries.The function performs multiple database queries without explicit error handling. While letting errors propagate may be acceptable for a utility function, adding context would improve debuggability.
🔎 Example pattern
try { const userMemberships = await db.query.members.findMany({ where: eq(members.userId, userId), columns: { organizationId: true }, }); // ... rest of logic } catch (error) { console.error(`[electric/utils] Failed to fetch memberships for table ${tableName}:`, error); throw error; // Re-throw after logging }Based on coding guidelines: Use prefixed console logging with context pattern and never swallow errors silently.
apps/api/src/proxy.ts (1)
5-20: CORS implementation is secure and correct.The origin validation and header injection logic is sound. The function correctly returns empty strings for disallowed origins, which is a good security practice.
Consider extracting the CORS header names to named constants at the module top for better maintainability, as per coding guidelines:
🔎 Optional refactor to extract CORS header constants
+const CORS_HEADERS = { + ALLOW_ORIGIN: "Access-Control-Allow-Origin", + ALLOW_METHODS: "Access-Control-Allow-Methods", + ALLOW_HEADERS: "Access-Control-Allow-Headers", + ALLOW_CREDENTIALS: "Access-Control-Allow-Credentials", +} as const; + function getCorsHeaders(origin: string | null) { const isAllowed = origin && allowedOrigins.includes(origin); return { - "Access-Control-Allow-Origin": isAllowed ? origin : "", - "Access-Control-Allow-Methods": "GET, POST, PUT, PATCH, DELETE, OPTIONS", - "Access-Control-Allow-Headers": - "Content-Type, Authorization, x-trpc-source, trpc-accept", - "Access-Control-Allow-Credentials": "true", + [CORS_HEADERS.ALLOW_ORIGIN]: isAllowed ? origin : "", + [CORS_HEADERS.ALLOW_METHODS]: "GET, POST, PUT, PATCH, DELETE, OPTIONS", + [CORS_HEADERS.ALLOW_HEADERS]: "Content-Type, Authorization, x-trpc-source, trpc-accept", + [CORS_HEADERS.ALLOW_CREDENTIALS]: "true", }; }apps/admin/src/app/(dashboard)/layout.tsx (2)
42-43: Add error handling around TRPC query.Similar to the session retrieval, the
trpc.user.me.query()call should have error handling to prevent layout crashes if the API call fails.🔎 Add error handling to TRPC call
- const trpc = await api(); - const user = await trpc.user.me.query(); + const trpc = await api(); + let user; + try { + user = await trpc.user.me.query(); + } catch (error) { + console.error("[admin/dashboard] Failed to fetch user:", error); + redirect(env.NEXT_PUBLIC_WEB_URL); + }
34-47: Consider extracting authentication logic to a helper function.The authentication flow spans multiple checks (session, email domain, database user). Extracting this to a named helper function would improve readability and make the layout component focus on rendering.
Example refactor
async function getAuthenticatedUser() { const session = await auth.api.getSession({ headers: await headers(), }); if (!session?.user) { redirect(env.NEXT_PUBLIC_WEB_URL); } if (!session.user.email?.endsWith(COMPANY.EMAIL_DOMAIN)) { redirect(env.NEXT_PUBLIC_WEB_URL); } const trpc = await api(); const user = await trpc.user.me.query(); if (!user) { redirect(env.NEXT_PUBLIC_WEB_URL); } return user; }Then in the layout:
export default async function DashboardLayout({ children }: { children: React.ReactNode }) { const user = await getAuthenticatedUser(); // ... rest of component }apps/desktop/src/main/lib/api-client.ts (1)
19-27: Remove unnecessaryasynckeyword from headers function.The
headers()function is markedasyncbutauthService.getAccessToken()is now synchronous. This creates unnecessary overhead.🔎 Proposed fix
- async headers() { + headers() { const token = authService.getAccessToken(); if (token) { return {apps/desktop/src/lib/trpc/routers/auth/index.ts (1)
87-92: Consider stricter UUID validation for organizationId.The input uses
z.string()which accepts any string. Since organization IDs are UUIDs, consider usingz.uuid()(Zod 4 syntax) for stricter validation.🔎 Suggested change
setActiveOrganization: publicProcedure - .input(z.object({ organizationId: z.string() })) + .input(z.object({ organizationId: z.uuid() })) .mutation(async ({ input }) => {packages/trpc/src/trpc.ts (1)
44-53: Add null safety for user email in admin check.If the session somehow lacks a user or email property, this will throw an unhandled error. Consider adding a defensive check.
🔎 Suggested defensive check
export const adminProcedure = protectedProcedure.use(async ({ ctx, next }) => { + if (!ctx.session.user?.email) { + throw new TRPCError({ + code: "UNAUTHORIZED", + message: "User email not available in session.", + }); + } if (!ctx.session.user.email.endsWith(COMPANY.EMAIL_DOMAIN)) { throw new TRPCError({ code: "FORBIDDEN", message: `Admin access requires ${COMPANY.EMAIL_DOMAIN} email.`, }); }apps/desktop/src/main/lib/auth/auth.ts (1)
19-19: Consider periodic cleanup of the state store.The state store cleanup only runs during
signIn(). If sign-in operations are infrequent, stale entries could accumulate in memory. Consider adding periodic cleanup (e.g., on initialization or as a background task).🔎 Suggested enhancement
Add cleanup to the
initialize()method:async initialize(): Promise<void> { // Cleanup old state entries const tenMinutesAgo = Date.now() - 10 * 60 * 1000; for (const [s, ts] of stateStore) { if (ts < tenMinutesAgo) stateStore.delete(s); } // ... existing initialization logic }Also applies to: 153-156
packages/db/src/schema/auth.ts (2)
89-103: AddupdatedAtfield for audit trail consistency.The organizations table is missing an
updatedAttimestamp. Since organizations can be modified (name, logo, metadata), adding this field would provide a consistent audit trail across all tables.🔎 Proposed fix to add updatedAt field
logo: text("logo"), createdAt: timestamp("created_at").defaultNow().notNull(), + updatedAt: timestamp("updated_at") + .defaultNow() + .$onUpdate(() => new Date()) + .notNull(), metadata: text("metadata"),
127-147: AddupdatedAtand enum constraints forroleandstatus.The invitations table would benefit from:
- An
updatedAttimestamp to track when invitation status changes- Enum types for
roleandstatusfields to enforce valid values at the database level (e.g., status: "pending" | "accepted" | "expired")🔎 Proposed enhancements
+import { pgEnum } from "drizzle-orm/pg-core"; + +export const invitationRole = pgEnum("invitation_role", ["member", "admin", "owner"]); +export const invitationStatus = pgEnum("invitation_status", ["pending", "accepted", "expired", "revoked"]); export const invitations = authSchema.table( "invitations", { id: uuid("id").primaryKey().defaultRandom(), organizationId: uuid("organization_id") .notNull() .references(() => organizations.id, { onDelete: "cascade" }), email: text("email").notNull(), - role: text("role"), - status: text("status").default("pending").notNull(), + role: invitationRole("role"), + status: invitationStatus("status").default("pending").notNull(), expiresAt: timestamp("expires_at").notNull(), createdAt: timestamp("created_at").defaultNow().notNull(), + updatedAt: timestamp("updated_at") + .defaultNow() + .$onUpdate(() => new Date()) + .notNull(), inviterId: uuid("inviter_id") .notNull() .references(() => users.id, { onDelete: "cascade" }), },
| const session = await auth.api.getSession({ | ||
| headers: await headers(), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual file
cat -n apps/admin/src/app/\(dashboard\)/layout.tsxRepository: superset-sh/superset
Length of output: 2411
🏁 Script executed:
# Check if Better Auth API is used elsewhere in the codebase
rg "auth\.api\." --type ts --type tsxRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Look for error handling patterns in similar async operations
rg "try\s*\{" apps/admin --type ts --type tsx | head -20Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Check the env configuration to verify env.NEXT_PUBLIC_WEB_URL
rg "NEXT_PUBLIC_WEB_URL" --type ts --type tsxRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Search for auth.api usage patterns without type restrictions
rg "auth\.api" --max-count 20Repository: superset-sh/superset
Length of output: 1546
🏁 Script executed:
# Check for try-catch patterns around async auth/api calls
rg "try\s*\{" -A 5 --max-count 15 | head -50Repository: superset-sh/superset
Length of output: 3232
🏁 Script executed:
# Look for Better Auth package info to understand the API
find . -name "package.json" -o -name "package-lock.json" | xargs grep -l "better-auth" 2>/dev/null | head -5Repository: superset-sh/superset
Length of output: 166
🌐 Web query:
Better Auth getSession API Next.js server component error handling best practices
💡 Result:
Here are concise best practices for calling Better Auth’s auth.api.getSession (and similar session APIs) from Next.js server components, focused on robust error handling and correct behavior.
Key problems to avoid
- Calling your own auth API without forwarding cookies/headers — server-side fetches don’t automatically include the user’s cookies, so session calls return null. [1][3]
- Assuming client-only helpers work identical on the server — use the server-side API patterns the library documents. [1]
Practical checklist / error-handling patterns
- Forward request headers/cookies when calling auth.api.getSession from a server component:
- Pass headers() (or cookies) into the call: await auth.api.getSession({ headers: await headers() }). If you don’t, expect null. [1][3]
- Prefer server-focused session helpers where provided:
- Use the library’s server API (auth.api.getSession) correctly; for Next.js middleware, prefer getSessionCookie to check existence cheaply without DB calls. [1][2]
- Always wrap session calls in try/catch and treat null as “no session”:
- try { const session = await auth.api.getSession({ headers: await headers() }); if (!session) handleUnauthenticated(); } catch (err) { logError(err); fallbackBehavior(); }
- Distinguish between “null session” (no auth) and exceptions (network, config, parsing).
- Validate inputs and headers before calling:
- Ensure headers()/cookies are available and match your auth cookie names/attributes (sameSite, secure, domain) — mismatches cause false-null results. [1]
- Timeouts, retries, and circuit breaking:
- Put a sensible timeout on cross-service session calls; retry transient network errors but avoid retrying on 4xx auth failures.
- Logging and observability:
- Log the full error and the headers/cookie presence (not cookie values) to diagnose missing cookies vs. server errors.
- Avoid expensive blocking in middleware/server components:
- In Next.js middleware, check session cookie presence (getSessionCookie) rather than making DB/API calls — use full session resolution only when needed. [2]
- Client vs server fallbacks:
- If server call returns null but user might be authenticated in the browser, consider rendering a client component that uses authClient.useSession or a client-side fetch to recover session and update UI.
- Edge / runtime considerations:
- Confirm the Better Auth integration is compatible with your runtime (edge vs node). Some cookie helpers or server APIs behave differently on edge runtimes — test accordingly. [1]
- Keep SDK/docs up-to-date:
- If behavior changes or you see nulls unexpectedly, check the official docs and GitHub issues for known bugs (community reports of getSession returning null in server components exist). [1][3]
Minimal error-handling template (server component)
- Pseudocode:
try {
const session = await auth.api.getSession({ headers: await headers() });
if (!session) return redirectToLoginOrRenderPublic();
return renderWithSession(session);
} catch (err) {
logError(err);
return renderErrorUIOrSafeFallback();
}
Sources
- Better Auth API docs (calling API from server; headers/cookies guidance). [1]
- Better Auth guidance on middleware/getSessionCookie for cheap session checks. [2]
- Community/GitHub reports on getSession returning null in Next.js server components (diagnostic reference). [3]
If you want, I can: (A) review a short snippet of your code to point out the exact missing header/cookie handling, or (B) produce a ready-to-drop-in server-component example using auth.api.getSession + robust error handling.
Add error handling around session retrieval.
The auth.api.getSession call lacks error handling. If the Better Auth API throws an error (e.g., network issues, misconfiguration, or service unavailability), the entire layout will crash without a user-friendly fallback.
Suggested error handling
- const session = await auth.api.getSession({
- headers: await headers(),
- });
+ let session;
+ try {
+ session = await auth.api.getSession({
+ headers: await headers(),
+ });
+ } catch (error) {
+ console.error("[admin/dashboard] Failed to retrieve session:", error);
+ redirect(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.
| const session = await auth.api.getSession({ | |
| headers: await headers(), | |
| }); | |
| let session; | |
| try { | |
| session = await auth.api.getSession({ | |
| headers: await headers(), | |
| }); | |
| } catch (error) { | |
| console.error("[admin/dashboard] Failed to retrieve session:", error); | |
| redirect(env.NEXT_PUBLIC_WEB_URL); | |
| } |
🤖 Prompt for AI Agents
In @apps/admin/src/app/(dashboard)/layout.tsx around lines 30 - 32, The
auth.api.getSession call in layout.tsx is unprotected and can throw, causing the
layout to crash; wrap the call to auth.api.getSession (where you assign to
session) in a try/catch, handle errors by logging (or processLogger) and return
a safe fallback (e.g., null session or redirect to login) so the component can
render gracefully; ensure you still await headers() inside the try and keep the
variable name session and any downstream checks intact so existing conditional
rendering works with the fallback.
| if (!session.user.email?.endsWith(COMPANY.EMAIL_DOMAIN)) { | ||
| redirect(env.NEXT_PUBLIC_WEB_URL); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "EMAIL_DOMAIN\s*[=:]" --type ts --type tsx -A 1Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
rg -n "EMAIL_DOMAIN\s*[=:]" -A 1Repository: superset-sh/superset
Length of output: 222
🏁 Script executed:
cat -n apps/admin/src/app/\(dashboard\)/layout.tsx | head -50Repository: superset-sh/superset
Length of output: 1497
Use strict email domain matching instead of endsWith().
The endsWith("@superset.sh") check is vulnerable because it allows emails from any domain ending with "@superset.sh"—for example, "[email protected]" would pass validation. Extract the domain portion and perform an exact comparison:
const domain = session.user.email?.split("@")[1];
if (domain !== "superset.sh") {
redirect(env.NEXT_PUBLIC_WEB_URL);
}
🤖 Prompt for AI Agents
In @apps/admin/src/app/(dashboard)/layout.tsx around lines 38 - 40, The current
check uses session.user.email?.endsWith(COMPANY.EMAIL_DOMAIN) which can be
spoofed; instead extract the email domain and compare exactly. Locate the guard
around session.user.email (and COMPANY.EMAIL_DOMAIN) and replace the endsWith
check with logic that safely parses the domain (e.g., split on "@", handle
missing/invalid email) and does a strict equality against COMPANY.EMAIL_DOMAIN
(or the literal domain), then call redirect(env.NEXT_PUBLIC_WEB_URL) when the
domain does not match.
| -- Backfill: Create personal workspace for users without an org | ||
| INSERT INTO "auth"."organizations" (id, name, slug, created_at) | ||
| SELECT | ||
| gen_random_uuid(), | ||
| u.name || '''s Workspace', | ||
| LEFT(u.id::text, 8) || '-workspace', | ||
| NOW() | ||
| FROM "auth"."users" u | ||
| WHERE NOT EXISTS ( | ||
| SELECT 1 FROM "auth"."members" m WHERE m.user_id = u.id | ||
| ); | ||
| --> statement-breakpoint | ||
| -- Backfill: Add users as owners of their new workspaces | ||
| INSERT INTO "auth"."members" (id, organization_id, user_id, role, created_at) | ||
| SELECT | ||
| gen_random_uuid(), | ||
| o.id, | ||
| u.id, | ||
| 'owner', | ||
| NOW() | ||
| FROM "auth"."users" u | ||
| JOIN "auth"."organizations" o ON o.slug = LEFT(u.id::text, 8) || '-workspace' | ||
| WHERE NOT EXISTS ( | ||
| SELECT 1 FROM "auth"."members" m WHERE m.user_id = u.id | ||
| ); |
There was a problem hiding this comment.
Critical: Workspace slug generation risks collisions.
The backfill logic uses only the first 8 characters of the user UUID for the workspace slug (line 124), then relies on the same pattern to JOIN back (line 140). This creates a significant collision risk:
- 8 hex characters = ~4.3 billion combinations
- At ~65,000 users, collision probability reaches ~50% (birthday paradox)
- Collision consequences:
- Migration fails on unique constraint violation
- Wrong organization assigned to users
- Data integrity corruption
🔎 Proposed fix using full UUID
-- Backfill: Create personal workspace for users without an org
INSERT INTO "auth"."organizations" (id, name, slug, created_at)
SELECT
gen_random_uuid(),
u.name || '''s Workspace',
- LEFT(u.id::text, 8) || '-workspace',
+ u.id::text || '-workspace',
NOW()
FROM "auth"."users" u
WHERE NOT EXISTS (
SELECT 1 FROM "auth"."members" m WHERE m.user_id = u.id
);
--> statement-breakpoint
-- Backfill: Add users as owners of their new workspaces
INSERT INTO "auth"."members" (id, organization_id, user_id, role, created_at)
SELECT
gen_random_uuid(),
o.id,
u.id,
'owner',
NOW()
FROM "auth"."users" u
-JOIN "auth"."organizations" o ON o.slug = LEFT(u.id::text, 8) || '-workspace'
+JOIN "auth"."organizations" o ON o.slug = u.id::text || '-workspace'
WHERE NOT EXISTS (
SELECT 1 FROM "auth"."members" m WHERE m.user_id = u.id
);Alternative: If shorter slugs are required for UX, implement a collision-resistant generation strategy with retry logic or sequential suffixes.
📝 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.
| -- Backfill: Create personal workspace for users without an org | |
| INSERT INTO "auth"."organizations" (id, name, slug, created_at) | |
| SELECT | |
| gen_random_uuid(), | |
| u.name || '''s Workspace', | |
| LEFT(u.id::text, 8) || '-workspace', | |
| NOW() | |
| FROM "auth"."users" u | |
| WHERE NOT EXISTS ( | |
| SELECT 1 FROM "auth"."members" m WHERE m.user_id = u.id | |
| ); | |
| --> statement-breakpoint | |
| -- Backfill: Add users as owners of their new workspaces | |
| INSERT INTO "auth"."members" (id, organization_id, user_id, role, created_at) | |
| SELECT | |
| gen_random_uuid(), | |
| o.id, | |
| u.id, | |
| 'owner', | |
| NOW() | |
| FROM "auth"."users" u | |
| JOIN "auth"."organizations" o ON o.slug = LEFT(u.id::text, 8) || '-workspace' | |
| WHERE NOT EXISTS ( | |
| SELECT 1 FROM "auth"."members" m WHERE m.user_id = u.id | |
| ); | |
| -- Backfill: Create personal workspace for users without an org | |
| INSERT INTO "auth"."organizations" (id, name, slug, created_at) | |
| SELECT | |
| gen_random_uuid(), | |
| u.name || '''s Workspace', | |
| u.id::text || '-workspace', | |
| NOW() | |
| FROM "auth"."users" u | |
| WHERE NOT EXISTS ( | |
| SELECT 1 FROM "auth"."members" m WHERE m.user_id = u.id | |
| ); | |
| --> statement-breakpoint | |
| -- Backfill: Add users as owners of their new workspaces | |
| INSERT INTO "auth"."members" (id, organization_id, user_id, role, created_at) | |
| SELECT | |
| gen_random_uuid(), | |
| o.id, | |
| u.id, | |
| 'owner', | |
| NOW() | |
| FROM "auth"."users" u | |
| JOIN "auth"."organizations" o ON o.slug = u.id::text || '-workspace' | |
| WHERE NOT EXISTS ( | |
| SELECT 1 FROM "auth"."members" m WHERE m.user_id = u.id | |
| ); |
🤖 Prompt for AI Agents
In @packages/db/drizzle/0006_add_better_auth_tables.sql around lines 119 - 143,
The slug generation for personal workspaces is unsafe because it uses
LEFT(u.id::text, 8) in both the INSERT INTO "auth"."organizations" and the JOIN
for adding members, creating high collision risk; fix by using the full user
UUID (u.id::text) or another collision-resistant value for the organization slug
when creating orgs and when joining back (replace LEFT(u.id::text, 8) ||
'-workspace' with a full-UUID-based slug or deterministic unique value), and
update the JOIN condition in the INSERT INTO "auth"."members" to match that same
slug strategy so lookups are unambiguous; if short slugs are required instead,
implement a deterministic deduplication strategy (e.g., append a sequence or
random suffix with uniqueness check and retry) and apply it consistently in both
the organization creation and the member-assignment JOIN.
| */ | ||
| emptyStringAsUndefined: true, | ||
| skipValidation: !!process.env.SKIP_ENV_VALIDATION, | ||
| skipValidation: true, |
There was a problem hiding this comment.
Hardcoded skipValidation: true remains unaddressed—validation is permanently disabled.
This issue was previously flagged but has not been resolved. Hardcoding skipValidation: true permanently disables validation of DATABASE_URL and DATABASE_URL_UNPOOLED, allowing the application to start with malformed or missing database URLs. This results in cryptic runtime failures instead of clear startup errors.
🔎 Restore conditional validation
- skipValidation: true,
+ skipValidation: process.env.SKIP_ENV_VALIDATION === "true",This ensures validation runs in production while allowing developers to opt out locally when needed.
📝 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 === "true", |
🤖 Prompt for AI Agents
In @packages/db/src/env.ts at line 22, The hardcoded skipValidation: true
disables validation permanently; change the skipValidation logic in the env
config so validation runs in production and only skips when a developer
explicitly opts out locally (e.g., use process.env.NODE_ENV and an opt-out env
var like SKIP_ENV_VALIDATION to decide skipValidation), ensure the DATABASE_URL
and DATABASE_URL_UNPOOLED checks remain active by default, and update the
skipValidation assignment referenced in the env configuration object (the
skipValidation property in packages/db/src/env.ts) accordingly.
| 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.
The activeOrganizationId field references organizations but lacks a foreign key constraint, which can lead to orphaned references if organizations are deleted.
🔎 Proposed fix to add FK constraint
activeOrganizationId: uuid("active_organization_id")
+ .references(() => organizations.id, { onDelete: "set null" }),📝 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 29 - 47, The sessions table's
activeOrganizationId column lacks a foreign key constraint; update the sessions
schema by changing the activeOrganizationId definition in the sessions table to
reference organizations.id (e.g., .references(() => organizations.id, {
onDelete: "SET NULL" }) or your preferred onDelete behavior) so the DB enforces
referential integrity; locate the activeOrganizationId symbol in the sessions
table definition and add the .references call (and adjust or add an index if
desired).
| export const members = authSchema.table( | ||
| "members", | ||
| { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| organizationId: uuid("organization_id") | ||
| .notNull() | ||
| .references(() => organizations.id, { onDelete: "cascade" }), | ||
| userId: uuid("user_id") | ||
| .notNull() | ||
| .references(() => users.id, { onDelete: "cascade" }), | ||
| role: text("role").default("member").notNull(), | ||
| createdAt: timestamp("created_at").defaultNow().notNull(), | ||
| }, | ||
| (table) => [ | ||
| index("members_organization_id_idx").on(table.organizationId), | ||
| index("members_user_id_idx").on(table.userId), | ||
| ], | ||
| ); | ||
|
|
||
| export type SelectMember = typeof members.$inferSelect; | ||
| export type InsertMember = typeof members.$inferInsert; |
There was a problem hiding this comment.
Add unique constraint, updatedAt field, and enum constraint for role.
The members table has several data integrity issues:
- Missing unique constraint: Without a unique constraint on
(organizationId, userId), duplicate memberships are possible - Missing updatedAt: No audit trail for role changes
- Plain text role: Doesn't enforce valid role values at the database level
🔎 Proposed fixes
role: text("role").default("member").notNull(),
createdAt: timestamp("created_at").defaultNow().notNull(),
+ updatedAt: timestamp("updated_at")
+ .defaultNow()
+ .$onUpdate(() => new Date())
+ .notNull(),
},
(table) => [
index("members_organization_id_idx").on(table.organizationId),
index("members_user_id_idx").on(table.userId),
+ uniqueIndex("members_organization_user_idx").on(
+ table.organizationId,
+ table.userId,
+ ),
],For the role field, consider creating a Drizzle enum:
import { pgEnum } from "drizzle-orm/pg-core";
export const memberRole = pgEnum("member_role", ["member", "admin", "owner"]);
// Then in the members table:
role: memberRole("role").default("member").notNull(),📝 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 members = authSchema.table( | |
| "members", | |
| { | |
| id: uuid("id").primaryKey().defaultRandom(), | |
| organizationId: uuid("organization_id") | |
| .notNull() | |
| .references(() => organizations.id, { onDelete: "cascade" }), | |
| userId: uuid("user_id") | |
| .notNull() | |
| .references(() => users.id, { onDelete: "cascade" }), | |
| role: text("role").default("member").notNull(), | |
| createdAt: timestamp("created_at").defaultNow().notNull(), | |
| }, | |
| (table) => [ | |
| index("members_organization_id_idx").on(table.organizationId), | |
| index("members_user_id_idx").on(table.userId), | |
| ], | |
| ); | |
| export type SelectMember = typeof members.$inferSelect; | |
| export type InsertMember = typeof members.$inferInsert; | |
| export const members = authSchema.table( | |
| "members", | |
| { | |
| id: uuid("id").primaryKey().defaultRandom(), | |
| organizationId: uuid("organization_id") | |
| .notNull() | |
| .references(() => organizations.id, { onDelete: "cascade" }), | |
| userId: uuid("user_id") | |
| .notNull() | |
| .references(() => users.id, { onDelete: "cascade" }), | |
| role: text("role").default("member").notNull(), | |
| createdAt: timestamp("created_at").defaultNow().notNull(), | |
| updatedAt: timestamp("updated_at") | |
| .defaultNow() | |
| .$onUpdate(() => new Date()) | |
| .notNull(), | |
| }, | |
| (table) => [ | |
| index("members_organization_id_idx").on(table.organizationId), | |
| index("members_user_id_idx").on(table.userId), | |
| uniqueIndex("members_organization_user_idx").on( | |
| table.organizationId, | |
| table.userId, | |
| ), | |
| ], | |
| ); | |
| export type SelectMember = typeof members.$inferSelect; | |
| export type InsertMember = typeof members.$inferInsert; |
| 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.
The safety concerns from the previous review remain unaddressed.
The deleteUser mutation still lacks:
- User existence validation
- Self-deletion prevention
- Transaction wrapping
- Error handling for database constraints
These critical safety issues were identified in the previous review and should be resolved before merging.
saddlepaddle
temporarily deployed
to
production
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @apps/desktop/src/main/lib/auth/auth.ts:
- Around line 79-84: Extract the 5-minute buffer into a module-level named
constant (e.g., EXPIRY_BUFFER_MS = 5 * 60 * 1000) declared near the top of the
file, then update the isExpired method to use that constant instead of the
hardcoded 5 * 60 * 1000 value (replace bufferMs with EXPIRY_BUFFER_MS) so the
magic number is centralized and self-descriptive.
- Around line 151-174: The signIn method uses a hardcoded "10 * 60 * 1000"
millisecond threshold when cleaning up stateStore; extract this magic number
into a clearly named module-level constant (e.g., STATE_EXPIRY_MS or
STATE_CLEANUP_THRESHOLD_MS) declared near the top of the file (after imports)
and replace the inline expression in signIn with that constant, updating both
the tenMinutesAgo computation and any related comments to use the new constant
name for clarity.
🧹 Nitpick comments (4)
packages/auth/src/client.ts (1)
22-26: Consider explicit error handling over silentnullreturn.Returning
nullwhenNEXT_PUBLIC_API_URLis unavailable forces all consumers to null-check and fails silently. Per coding guidelines, errors should not be swallowed silently.Options:
- Log a warning when the client cannot be initialized
- Throw an error if this client is expected to always exist
- Add explicit type annotation to make the nullable return obvious to consumers
🔎 Suggested improvement with logging
+// Default instance for environments where NEXT_PUBLIC_API_URL is available +export const authApiClient: ReturnType<typeof createAuthApiClient> | null = (() => { + if (typeof process !== "undefined" && process.env.NEXT_PUBLIC_API_URL) { + return createAuthApiClient(process.env.NEXT_PUBLIC_API_URL); + } + if (typeof process !== "undefined") { + console.warn("[auth/client] NEXT_PUBLIC_API_URL not set - authApiClient unavailable"); + } + return null; +})(); -// Default instance for environments where NEXT_PUBLIC_API_URL is available -export const authApiClient = - typeof process !== "undefined" && process.env.NEXT_PUBLIC_API_URL - ? createAuthApiClient(process.env.NEXT_PUBLIC_API_URL) - : null;As per coding guidelines, use prefixed console logging with context pattern and never swallow errors silently.
apps/desktop/src/main/lib/auth/auth.ts (3)
24-44: Add error logging in the catch block.The catch block silently swallows all errors without logging. While parse failures are expected for invalid URLs, unexpected errors should be logged for debugging.
🔎 Proposed fix
} catch { + console.log("[auth/parseAuthDeepLink] Failed to parse deep link URL"); return null; }Based on coding guidelines: "Never swallow errors silently - at minimum log them with context"
58-77: Add error logging in the catch block.The catch block silently swallows errors during session restoration. While file-not-found is expected on first run, logging helps distinguish between expected and unexpected failures (e.g., decryption errors, filesystem issues).
🔎 Proposed fix
} catch { + console.log("[auth] No stored session or failed to restore"); this.token = null; this.expiresAt = null; }Based on coding guidelines: "Never swallow errors silently - at minimum log them with context"
208-219: Add error logging in the catch block.The catch block silently swallows errors when deleting the token file. While file-not-found is expected if already signed out, other filesystem errors could occur and should be logged for operational visibility.
🔎 Proposed fix
try { await fs.unlink(TOKEN_FILE); - } catch {} + } catch (error) { + console.log("[auth] Failed to delete token file (may not exist):", error); + }Based on coding guidelines: "Never swallow errors silently - at minimum log them with context"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/main/lib/auth/auth.tspackages/auth/src/client.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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:
packages/auth/src/client.tsapps/desktop/src/main/lib/auth/auth.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:
packages/auth/src/client.tsapps/desktop/src/main/lib/auth/auth.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/lib/auth/auth.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/main/lib/auth/auth.ts
🧬 Code graph analysis (1)
apps/desktop/src/main/lib/auth/auth.ts (4)
packages/auth/src/client.ts (1)
createAuthApiClient(15-20)apps/desktop/src/main/lib/app-environment.ts (1)
SUPERSET_HOME_DIR(5-5)packages/shared/src/constants.ts (2)
PROTOCOL_SCHEMES(6-9)AuthProvider(3-3)apps/desktop/src/main/lib/auth/crypto-storage.ts (2)
decrypt(97-119)encrypt(76-90)
⏰ 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). (7)
- GitHub Check: Deploy Web
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Docs
- GitHub Check: Deploy API
- GitHub Check: Build - macOS (arm64)
- GitHub Check: Build
🔇 Additional comments (2)
packages/auth/src/client.ts (2)
1-6: LGTM - Clean import structure.The aliasing of
createBetterAuthClientfor the vanilla client vscreateAuthClientfor React is clear and prevents naming collisions.
13-20: LGTM - Well-designed factory function.Clean factory pattern that allows non-React environments (like Electron) to create auth clients without coupling to
process.env. Good separation of concerns.
| 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; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract magic number to named constant.
The 5-minute expiry buffer is a hardcoded magic number that should be extracted to a named constant at the module top for clarity and maintainability.
🔎 Proposed fix
Add near the top of the file (after line 22):
const stateStore = new Map<string, number>();
+const TOKEN_EXPIRY_BUFFER_MS = 5 * 60 * 1000; // 5 minutesThen update the method:
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;
+ // Consider expired before actual expiry for safety
+ return Date.now() > this.expiresAt.getTime() - TOKEN_EXPIRY_BUFFER_MS;
}Based on coding guidelines: "Extract magic numbers and hardcoded values to named constants at module top"
📝 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.
| 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; | |
| } | |
| private isExpired(): boolean { | |
| if (!this.expiresAt) return true; | |
| // Consider expired before actual expiry for safety | |
| return Date.now() > this.expiresAt.getTime() - TOKEN_EXPIRY_BUFFER_MS; | |
| } |
🤖 Prompt for AI Agents
In @apps/desktop/src/main/lib/auth/auth.ts around lines 79 - 84, Extract the
5-minute buffer into a module-level named constant (e.g., EXPIRY_BUFFER_MS = 5 *
60 * 1000) declared near the top of the file, then update the isExpired method
to use that constant instead of the hardcoded 5 * 60 * 1000 value (replace
bufferMs with EXPIRY_BUFFER_MS) so the magic number is centralized and
self-descriptive.
| 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); | ||
| } | ||
|
|
||
| const connectUrl = new URL( | ||
| `${env.NEXT_PUBLIC_API_URL}/api/auth/desktop/connect`, | ||
| ); | ||
| connectUrl.searchParams.set("provider", provider); | ||
| connectUrl.searchParams.set("state", state); | ||
| await shell.openExternal(connectUrl.toString()); | ||
| return { success: true }; | ||
| } catch (err) { | ||
| return { | ||
| success: false, | ||
| error: err instanceof Error ? err.message : "Failed to open browser", | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract magic number to named constant.
The 10-minute state cleanup threshold is a hardcoded magic number that should be extracted to a named constant at the module top for clarity and maintainability.
🔎 Proposed fix
Add near the top of the file (after line 22):
const stateStore = new Map<string, number>();
+const STATE_CLEANUP_THRESHOLD_MS = 10 * 60 * 1000; // 10 minutesThen update the method:
const state = crypto.randomBytes(32).toString("base64url");
stateStore.set(state, Date.now());
- const tenMinutesAgo = Date.now() - 10 * 60 * 1000;
+ const cleanupThreshold = Date.now() - STATE_CLEANUP_THRESHOLD_MS;
for (const [s, ts] of stateStore) {
- if (ts < tenMinutesAgo) stateStore.delete(s);
+ if (ts < cleanupThreshold) stateStore.delete(s);
}Based on coding guidelines: "Extract magic numbers and hardcoded values to named constants at module top"
📝 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.
| 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); | |
| } | |
| const connectUrl = new URL( | |
| `${env.NEXT_PUBLIC_API_URL}/api/auth/desktop/connect`, | |
| ); | |
| connectUrl.searchParams.set("provider", provider); | |
| connectUrl.searchParams.set("state", state); | |
| await shell.openExternal(connectUrl.toString()); | |
| return { success: true }; | |
| } catch (err) { | |
| return { | |
| success: false, | |
| error: err instanceof Error ? err.message : "Failed to open browser", | |
| }; | |
| } | |
| } | |
| async signIn(provider: AuthProvider): Promise<SignInResult> { | |
| try { | |
| const state = crypto.randomBytes(32).toString("base64url"); | |
| stateStore.set(state, Date.now()); | |
| const cleanupThreshold = Date.now() - STATE_CLEANUP_THRESHOLD_MS; | |
| for (const [s, ts] of stateStore) { | |
| if (ts < cleanupThreshold) stateStore.delete(s); | |
| } | |
| const connectUrl = new URL( | |
| `${env.NEXT_PUBLIC_API_URL}/api/auth/desktop/connect`, | |
| ); | |
| connectUrl.searchParams.set("provider", provider); | |
| connectUrl.searchParams.set("state", state); | |
| await shell.openExternal(connectUrl.toString()); | |
| return { success: true }; | |
| } catch (err) { | |
| return { | |
| success: false, | |
| error: err instanceof Error ? err.message : "Failed to open browser", | |
| }; | |
| } | |
| } |
🤖 Prompt for AI Agents
In @apps/desktop/src/main/lib/auth/auth.ts around lines 151 - 174, The signIn
method uses a hardcoded "10 * 60 * 1000" millisecond threshold when cleaning up
stateStore; extract this magic number into a clearly named module-level constant
(e.g., STATE_EXPIRY_MS or STATE_CLEANUP_THRESHOLD_MS) declared near the top of
the file (after imports) and replace the inline expression in signIn with that
constant, updating both the tenMinutesAgo computation and any related comments
to use the new constant name for clarity.
- Updated desktop app to use correct Better Auth client API methods: - authClient.getSession() instead of auth.api.getSession() - authClient.organization.setActive() instead of auth.api.setActiveOrganization() - Renamed packages/auth/src/index.ts to server.ts - Changed package exports from "." to "./server" for clarity - Removed unnecessary createAuthApiClient factory function - Updated all imports from @superset/auth to @superset/auth/server This fixes the typecheck errors and database dependency issue in the desktop app by properly separating server-side auth (requires DB) from client-side auth (HTTP API calls only). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
saddlepaddle
temporarily deployed
to
production
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/app/api/integrations/linear/connect/route.ts (1)
7-55: Add error handling for session retrieval and database operations.The route lacks error handling around
auth.api.getSession()anddb.query.members.findFirst(). If either operation fails (auth service unavailable, database connection issues, etc.), the route will crash with an unhandled exception, resulting in a generic 500 error with no logging context.🔎 Proposed fix with try-catch wrapper
export async function GET(request: Request) { + try { const session = await auth.api.getSession({ headers: request.headers, }); if (!session?.user) { return Response.json({ error: "Unauthorized" }, { status: 401 }); } const url = new URL(request.url); const organizationId = url.searchParams.get("organizationId"); if (!organizationId) { return Response.json( { error: "Missing organizationId parameter" }, { status: 400 }, ); } const membership = await db.query.members.findFirst({ where: and( eq(members.organizationId, organizationId), eq(members.userId, session.user.id), ), }); if (!membership) { return Response.json( { error: "User is not a member of this organization" }, { status: 403 }, ); } const state = Buffer.from( JSON.stringify({ organizationId, userId: session.user.id }), ).toString("base64url"); const linearAuthUrl = new URL("https://linear.app/oauth/authorize"); 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("state", state); return Response.redirect(linearAuthUrl.toString()); + } catch (error) { + console.error("[linear/connect] Failed to initiate Linear OAuth:", error); + return Response.json( + { error: "Internal server error" }, + { status: 500 }, + ); + } }
🤖 Fix all issues with AI agents
In @apps/api/src/app/api/auth/desktop/connect/route.ts:
- Around line 6-17: The GET route is manually validating query params; replace
that with Zod validation by defining a schema (e.g., for "provider" use
z.enum(["google","github"]) and for "state" use z.string().nonempty()) and use
schema.parse or schema.safeParse inside the GET function to validate the URL
search params (where you currently extract provider and state from new
URL(request.url)); on validation failure return a 400 Response including the
validation error details, and on success use the parsed values instead of the
raw provider/state variables.
- Around line 30-37: The code currently does an unsafe assertion on the external
auth API response via "const body = (await result.json()) as { url?: string;
redirect?: boolean }" and then uses body.url; replace this with runtime
validation using a Zod schema (e.g., define a schema for { url: string,
redirect?: boolean }), parse the JSON with await result.json(), then validate
with schema.safeParse or schema.parse before accessing body.url; handle
validation failures by returning the 500 Response with the inspected validation
error (or a safe error message) and ensure you no longer use the unsafe "as {
... }" cast so unexpected shapes won’t crash at runtime.
- Around line 22-28: Wrap the call to auth.api.signInSocial in a try/catch: call
auth.api.signInSocial(...) as before, but if it throws catch the error, log it
using the prefixed pattern (e.g. console.error('[auth/signInSocial] failed for
provider=%s callbackURL=%s:', provider, successUrl.toString(), err)) to include
context and the error object, then either rethrow or return a meaningful error
Response (HTTP 500) so the failure is observable to callers; keep references to
auth.api.signInSocial, provider, successUrl, and result to locate the change.
In @apps/api/src/app/api/electric/[...path]/route.ts:
- Line 11: Extract all hardcoded error messages and HTTP status codes used in
route handler into module-level named constants (e.g., AUTH_UNAUTHORIZED_MSG,
AUTH_FORBIDDEN_MSG, BAD_REQUEST_MSG and STATUS_UNAUTHORIZED = 401,
STATUS_FORBIDDEN = 403, STATUS_BAD_REQUEST = 400). Add these constants at the
top of the file and replace the inline uses in the route handler (the
Response("Unauthorized", { status: 401 }) and the other occurrences around the
handler—lines referenced in the review) with Response(AUTH_UNAUTHORIZED_MSG, {
status: STATUS_UNAUTHORIZED }) and analogously for the other error
messages/statuses so all responses use the new constants.
In @apps/desktop/src/main/lib/auth/auth.ts:
- Around line 55-74: The initialize method currently swallows all errors in its
catch block; change the catch to capture the error (e.g., catch (err)) and log a
contextual message including TOKEN_FILE, decrypt/JSON parse context and the
error (use console.error or the module's logger) before clearing this.token and
this.expiresAt and proceeding to signOut if needed; update references around
initialize, TOKEN_FILE, decrypt, and signOut to ensure the log provides enough
info to debug decryption/FS/parse failures.
In @apps/web/src/app/auth/desktop/success/page.tsx:
- Around line 66-68: Extract the magic millisecond value used to compute
expiresAt into a named constant (e.g., SESSION_EXPIRY_MS) declared at the top of
the module (or imported from the shared auth config) and replace the inline
expression in the expiresAt assignment with that constant; ensure the constant
name reflects it's in milliseconds and keep the value equal to 60 * 60 * 24 * 30
* 1000 so it remains in sync with the auth settings.
- Line 31: Update the console.error call in page.tsx to use the required
prefixed logging pattern; replace the current message "Failed to get session for
desktop auth:" with a prefixed message like "[auth/desktop] Failed to get
session" while still passing the error object as the second argument so the call
becomes console.error("[auth/desktop] Failed to get session", error). Locate the
existing console.error call in the desktop auth success page (the line that
currently reads console.error("Failed to get session for desktop auth:", error))
and change only the string to the bracketed prefix format to comply with the
logging convention.
In @apps/web/src/proxy.ts:
- Around line 17-19: Wrap the call to auth.api.getSession(...) in a try/catch to
handle exceptions from session retrieval: catch errors thrown by
auth.api.getSession (and errors from headers()) and return an appropriate
response instead of letting the middleware crash—e.g., log the error and return
a 401/503 or propagate a handled error to the middleware pipeline. Update the
code around the session variable (the await auth.api.getSession call that
assigns session) to perform this guarded call, log the caught error with
context, and ensure downstream code checks for a null/undefined session before
proceeding.
In @packages/trpc/src/trpc.ts:
- Around line 44-53: The adminProcedure middleware currently accesses
ctx.session.user.email without null checks and uses a fragile
endsWith(COMPANY.EMAIL_DOMAIN); update adminProcedure (the
protectedProcedure.use async ({ ctx, next }) block) to first null-check that
ctx.session?.user?.email is a non-empty string and throw a FORBIDDEN TRPCError
if missing, then validate the email by parsing the part after the last "@" (or
splitting on "@") and compare that domain exactly to COMPANY.DOMAIN (e.g.,
"superset.sh") rather than using EMAIL_DOMAIN or endsWith, and finally return
next({ ctx }) as before.
🧹 Nitpick comments (4)
apps/api/src/app/api/auth/desktop/connect/route.ts (1)
19-19: Extract hardcoded path to named constant.The path
"/auth/desktop/success"is hardcoded and could be extracted to a module-level constant for better maintainability and consistency.🔎 Proposed refactor
+const DESKTOP_AUTH_SUCCESS_PATH = "/auth/desktop/success"; + export async function GET(request: Request) { // ... validation code ... - const successUrl = new URL(`${env.NEXT_PUBLIC_WEB_URL}/auth/desktop/success`); + const successUrl = new URL(`${env.NEXT_PUBLIC_WEB_URL}${DESKTOP_AUTH_SUCCESS_PATH}`); successUrl.searchParams.set("desktop_state", state);Based on coding guidelines: "Extract magic numbers and hardcoded values to named constants at module top."
apps/web/src/app/auth/desktop/success/page.tsx (1)
29-29: Avoid duplicateheaders()calls.
headers()is called twice (line 29 and line 55). Store the result once and reuse it to avoid redundant async operations.🔎 Proposed fix
+ const headersObj = await headers(); + // Get session from Better Auth let session: Awaited<ReturnType<typeof auth.api.getSession>> | null = null; try { - session = await auth.api.getSession({ headers: await headers() }); + session = await auth.api.getSession({ headers: headersObj }); } catch (error) { // ... } // ... // Create a separate session for the desktop app instead of reusing the browser session // This ensures desktop and web have independent sessions with separate activeOrganizationId - const headersObj = await headers(); const userAgent = headersObj.get("user-agent") || "Superset Desktop App";Also applies to: 55-55
apps/api/src/app/api/integrations/linear/connect/route.ts (2)
8-38: Consider adding logging for authentication/authorization events.The authentication flow lacks logging, which makes security auditing and debugging difficult. For critical auth paths, logging successful and failed authorization attempts helps with:
- Security monitoring and incident response
- Debugging production issues
- Compliance and audit trails
As per coding guidelines, use prefixed console logging with context.
🔎 Suggested logging additions
const session = await auth.api.getSession({ headers: request.headers, }); if (!session?.user) { + console.warn("[linear/connect] Unauthorized access attempt"); return Response.json({ error: "Unauthorized" }, { status: 401 }); } const url = new URL(request.url); const organizationId = url.searchParams.get("organizationId"); if (!organizationId) { + console.warn("[linear/connect] Missing organizationId parameter", { + userId: session.user.id, + }); return Response.json( { error: "Missing organizationId parameter" }, { status: 400 }, ); } const membership = await db.query.members.findFirst({ where: and( eq(members.organizationId, organizationId), eq(members.userId, session.user.id), ), }); if (!membership) { + console.warn("[linear/connect] User not member of organization", { + userId: session.user.id, + organizationId, + }); return Response.json( { error: "User is not a member of this organization" }, { status: 403 }, ); } + + console.log("[linear/connect] Initiating Linear OAuth", { + userId: session.user.id, + organizationId, + });
44-52: Consider extracting hardcoded OAuth configuration to constants.The Linear OAuth URL and scope string are hardcoded. Extracting these to named constants at the module top improves maintainability and clarifies intent, especially if multiple Linear integration routes exist.
🔎 Suggested constant extraction
Add constants at the top of the file:
import { auth } from "@superset/auth/server"; import { db } from "@superset/db/client"; import { members } from "@superset/db/schema"; import { and, eq } from "drizzle-orm"; import { env } from "@/env"; +const LINEAR_OAUTH_URL = "https://linear.app/oauth/authorize"; +const LINEAR_OAUTH_SCOPE = "read,write,issues:create"; + export async function GET(request: Request) {Then use the constants:
- const linearAuthUrl = new URL("https://linear.app/oauth/authorize"); + const linearAuthUrl = new URL(LINEAR_OAUTH_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);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
apps/admin/src/app/(dashboard)/layout.tsxapps/admin/src/proxy.tsapps/api/src/app/api/auth/[...all]/route.tsapps/api/src/app/api/auth/desktop/connect/route.tsapps/api/src/app/api/electric/[...path]/route.tsapps/api/src/app/api/integrations/linear/connect/route.tsapps/api/src/trpc/context.tsapps/desktop/src/main/lib/auth/auth.tsapps/marketing/src/app/components/CTAButtons/CTAButtons.tsxapps/web/src/app/(auth)/layout.tsxapps/web/src/app/(dashboard)/layout.tsxapps/web/src/app/auth/desktop/success/page.tsxapps/web/src/proxy.tspackages/auth/package.jsonpackages/auth/src/client.tspackages/auth/src/server.tspackages/trpc/src/trpc.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/marketing/src/app/components/CTAButtons/CTAButtons.tsx
- apps/api/src/app/api/auth/[...all]/route.ts
- apps/api/src/trpc/context.ts
- apps/web/src/app/(dashboard)/layout.tsx
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{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)/layout.tsxapps/admin/src/app/(dashboard)/layout.tsxapps/admin/src/proxy.tsapps/web/src/proxy.tsapps/api/src/app/api/electric/[...path]/route.tspackages/auth/src/server.tsapps/api/src/app/api/integrations/linear/connect/route.tsapps/desktop/src/main/lib/auth/auth.tspackages/auth/src/client.tspackages/trpc/src/trpc.tsapps/api/src/app/api/auth/desktop/connect/route.tsapps/web/src/app/auth/desktop/success/page.tsx
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
One component per file - do not create multi-component files
Files:
apps/web/src/app/(auth)/layout.tsxapps/admin/src/app/(dashboard)/layout.tsxapps/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)/layout.tsxapps/admin/src/app/(dashboard)/layout.tsxapps/admin/src/proxy.tsapps/web/src/proxy.tsapps/api/src/app/api/electric/[...path]/route.tsapps/api/src/app/api/integrations/linear/connect/route.tsapps/desktop/src/main/lib/auth/auth.tsapps/api/src/app/api/auth/desktop/connect/route.tsapps/web/src/app/auth/desktop/success/page.tsx
**/*.{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)/layout.tsxapps/admin/src/app/(dashboard)/layout.tsxapps/admin/src/proxy.tsapps/web/src/proxy.tsapps/api/src/app/api/electric/[...path]/route.tspackages/auth/src/server.tsapps/api/src/app/api/integrations/linear/connect/route.tsapps/desktop/src/main/lib/auth/auth.tspackages/auth/src/client.tspackages/trpc/src/trpc.tsapps/api/src/app/api/auth/desktop/connect/route.tsapps/web/src/app/auth/desktop/success/page.tsx
apps/**/proxy.ts
📄 CodeRabbit inference engine (AGENTS.md)
Never use
middleware.tsin Next.js 16. Useproxy.tsinstead for request interception.
Files:
apps/admin/src/proxy.tsapps/web/src/proxy.ts
apps/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/api/**/*.{ts,tsx}: Use TRPCError with appropriate error codes: NOT_FOUND, UNAUTHORIZED, FORBIDDEN, BAD_REQUEST, INTERNAL_SERVER_ERROR, NOT_IMPLEMENTED
tRPC procedures and API route handlers should validate and delegate; keep orchestrators thin
Extract business logic from tRPC procedures into utility functions when logic exceeds ~50 lines, is used by multiple procedures, or needs independent testing
Validate at boundaries using Zod schemas for tRPC inputs and API route bodies
Files:
apps/api/src/app/api/electric/[...path]/route.tsapps/api/src/app/api/integrations/linear/connect/route.tsapps/api/src/app/api/auth/desktop/connect/route.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/lib/auth/auth.ts
🧠 Learnings (11)
📚 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)/layout.tsxapps/admin/src/app/(dashboard)/layout.tsxapps/admin/src/proxy.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/**/proxy.ts : Never use `middleware.ts` in Next.js 16. Use `proxy.ts` instead for request interception.
Applied to files:
apps/admin/src/proxy.tsapps/web/src/proxy.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/api/**/*.{ts,tsx} : tRPC procedures and API route handlers should validate and delegate; keep orchestrators thin
Applied to files:
apps/admin/src/proxy.tspackages/trpc/src/trpc.tsapps/api/src/app/api/auth/desktop/connect/route.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 **/*.{ts,tsx} : Extract magic numbers and hardcoded values to named constants at module top
Applied to files:
apps/desktop/src/main/lib/auth/auth.tsapps/web/src/app/auth/desktop/success/page.tsx
📚 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/api/**/*.{ts,tsx} : Use TRPCError with appropriate error codes: NOT_FOUND, UNAUTHORIZED, FORBIDDEN, BAD_REQUEST, INTERNAL_SERVER_ERROR, NOT_IMPLEMENTED
Applied to files:
packages/trpc/src/trpc.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/api/**/*.{ts,tsx} : Extract business logic from tRPC procedures into utility functions when logic exceeds ~50 lines, is used by multiple procedures, or needs independent testing
Applied to files:
packages/trpc/src/trpc.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} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
packages/trpc/src/trpc.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: Boundary layers (tRPC procedures, API routes) own error handling - domain utilities return data or throw specific errors
Applied to files:
packages/trpc/src/trpc.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/api/**/*.{ts,tsx} : Validate at boundaries using Zod schemas for tRPC inputs and API route bodies
Applied to files:
apps/api/src/app/api/auth/desktop/connect/route.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
📚 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} : Use prefixed console logging with context pattern: [domain/operation] message
Applied to files:
apps/web/src/app/auth/desktop/success/page.tsx
🧬 Code graph analysis (7)
apps/web/src/app/(auth)/layout.tsx (1)
packages/auth/src/server.ts (1)
auth(11-101)
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/admin/src/proxy.ts (3)
packages/auth/src/server.ts (1)
auth(11-101)apps/admin/src/env.ts (1)
env(5-44)packages/shared/src/constants.ts (1)
COMPANY(12-24)
apps/web/src/proxy.ts (2)
apps/admin/src/proxy.ts (2)
proxy(8-22)config(24-29)packages/auth/src/server.ts (1)
auth(11-101)
packages/auth/src/server.ts (1)
packages/db/src/schema/auth.ts (2)
authSchema(11-11)members(105-122)
apps/api/src/app/api/integrations/linear/connect/route.ts (2)
packages/auth/src/server.ts (1)
auth(11-101)packages/db/src/schema/auth.ts (1)
members(105-122)
apps/web/src/app/auth/desktop/success/page.tsx (2)
packages/auth/src/server.ts (1)
auth(11-101)packages/db/src/schema/auth.ts (1)
sessions(29-47)
⏰ 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). (7)
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Web
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Admin
- GitHub Check: Deploy API
- GitHub Check: Build - macOS (arm64)
- GitHub Check: Build
🔇 Additional comments (21)
apps/web/src/app/auth/desktop/success/page.tsx (4)
1-6: LGTM!Imports are correctly structured, using the server-side auth module and Drizzle ORM as required.
80-88: LGTM!URL construction properly encodes all parameters, and the protocol switching between development and production environments is handled correctly for the desktop app callback.
15-24: Extract repeated error UI to a helper.The error UI rendering is duplicated three times in this file with identical structure. Extract to a helper component or function.
Likely an incorrect or invalid review comment.
70-79: Add error handling for database insertion.The
db.insert()call is not wrapped in try-catch. A database failure (constraint violation, connection issue) would result in an unhandled error. Wrap in try-catch and return a graceful error UI.Likely an incorrect or invalid review comment.
apps/api/src/app/api/integrations/linear/connect/route.ts (1)
1-5: LGTM! Clean imports for Better Auth migration.The imports correctly reference the new authentication system and members table schema.
apps/desktop/src/main/lib/auth/auth.ts (6)
1-19: LGTM! Clean imports and module-level constants.The imports are concrete and appropriate for the authentication service. Module-level constants TOKEN_FILE and stateStore are properly defined, following best practices for configuration and state management.
21-41: LGTM! Robust deep link parsing with proper validation.The function correctly validates the protocol, host, pathname, and required query parameters. The empty catch block returning null is an acceptable pattern for parsing functions where any error should be treated as invalid input.
83-109: LGTM! Clear getter methods with appropriate logging.The getter methods properly check expiry state and include helpful debug logging with the
[auth]prefix pattern. The logic is correct and follows coding guidelines.
111-160: LGTM! Proper error handling and session management.Both methods correctly use the Authorization header, handle errors with appropriate logging, and maintain session state consistency. The pattern of refreshing the session after setActiveOrganization ensures the client state stays synchronized.
187-231: LGTM! Robust authentication flow with proper state validation.The callback handler correctly validates CSRF state, persists encrypted tokens, and ensures session data is fetched before emitting state changes. The signOut method properly cleans up all state. The empty catch for file deletion is acceptable as the file may not exist.
233-233: LGTM! Singleton pattern for centralized auth management.Exporting a singleton instance provides a clean API for the desktop app to manage authentication state.
apps/api/src/app/api/electric/[...path]/route.ts (2)
14-17: Verify type safety for session.activeOrganizationId.Ensure that the
sessionobject from Better Auth includes theactiveOrganizationIdproperty in its type definition. If this is a custom extension to the session schema, verify it's properly typed.Additionally, the error message "No active organization" is hardcoded. Per coding guidelines, extract error messages to named constants at the module top for consistency and maintainability.
#!/bin/bash # Description: Check if activeOrganizationId is properly typed in the Better Auth session schema # Search for session type definitions and activeOrganizationId usage ast-grep --pattern 'interface $_ { $$$ activeOrganizationId$_ $$$ }' # Also search for session schema definitions rg -n "activeOrganizationId" --type=ts -C 3
7-12: Verify Better Auth API usage and type annotations; review for additional guideline compliance.The Better Auth
auth.api.getSession({ headers: request.headers })API usage is correct per Better Auth documentation for server-side session retrieval, and the optional chaining null check is appropriate.However, I'm unable to access the full file context to verify: (1) explicit type annotations for
sessionData, (2) whether hardcoded error strings are extracted to constants per guidelines, (3) error handling on fetch/URL operations, and (4) console logging patterns. Please review the complete file against the coding guidelines:
- Extract hardcoded error messages and magic values to named constants
- Ensure all fetch calls and operations have error handling (never swallow errors silently)
- Use prefixed console logging:
[domain/operation] message- Avoid deep nesting with early returns
packages/auth/package.json (1)
1-34: Package structure looks good.The dual export pattern for
./serverand./clientcorrectly separates server-side auth logic (with database dependencies) from the lightweight client API. This aligns with the PR goal of removing database dependencies from the desktop app.Note:
drizzle-ormis pinned to0.45.1while other deps use^ranges. Verify this is intentional (e.g., due to breaking changes in newer versions).packages/auth/src/server.ts (1)
76-93: Session hook implementation looks correct.The
session.create.beforehook properly uses direct database queries viadb.query.members.findFirst()to set the initialactiveOrganizationId. This avoids the anti-pattern seen in the user creation hook.packages/trpc/src/trpc.ts (1)
7-11: Session-based context pattern is clean.The migration from database-based context to session-based context simplifies the TRPC setup. The
protectedProcedureproperly validates and narrows the session type before passing it downstream.Also applies to: 33-42
apps/web/src/proxy.ts (5)
1-3: LGTM!Imports are clean and follow best practices with concrete imports and proper type modifiers.
12-14: LGTM!The helper function correctly identifies public routes using a clear and maintainable pattern.
23-34: LGTM!The redirect logic correctly handles both authenticated and unauthenticated user flows:
- Authenticated users are redirected away from sign-in/sign-up pages
- Unauthenticated users are redirected to sign-in when accessing protected routes
37-42: LGTM!The matcher configuration correctly excludes Next.js internals, static assets, and monitoring endpoints while catching all other routes. Based on learnings, using
proxy.tsinstead ofmiddleware.tsis the correct approach for Next.js 16.
5-10: Verify that both desktop auth routes are necessary.The
publicRoutesarray includes both"/auth/desktop"and"/api/auth/desktop". Confirm whether both routes are actively used or if one is redundant.
| export async function GET(request: Request) { | ||
| const url = new URL(request.url); | ||
| const provider = url.searchParams.get("provider"); | ||
| const state = url.searchParams.get("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 }); | ||
| } |
There was a problem hiding this comment.
Missing Zod validation for query parameters (still unresolved).
The route continues to manually validate query parameters using string comparisons instead of Zod schemas. This bypasses type safety and violates the coding guidelines requiring Zod validation at boundaries.
Based on coding guidelines: "Validate at boundaries using Zod schemas for tRPC inputs and API route bodies."
🤖 Prompt for AI Agents
In @apps/api/src/app/api/auth/desktop/connect/route.ts around lines 6 - 17, The
GET route is manually validating query params; replace that with Zod validation
by defining a schema (e.g., for "provider" use z.enum(["google","github"]) and
for "state" use z.string().nonempty()) and use schema.parse or schema.safeParse
inside the GET function to validate the URL search params (where you currently
extract provider and state from new URL(request.url)); on validation failure
return a 400 Response including the validation error details, and on success use
the parsed values instead of the raw provider/state variables.
| const result = await auth.api.signInSocial({ | ||
| body: { | ||
| provider, | ||
| callbackURL: successUrl.toString(), | ||
| }, | ||
| asResponse: true, | ||
| }); |
There was a problem hiding this comment.
Add error handling and logging for auth API call.
The auth API call lacks error handling and observability. If auth.api.signInSocial throws an exception, it will propagate without context, making debugging difficult.
🔎 Proposed fix with error handling and logging
+ let result;
+ try {
- const result = await auth.api.signInSocial({
+ console.log("[auth/desktop/connect] Initiating OAuth", { provider, state: state.slice(0, 8) });
+ result = await auth.api.signInSocial({
body: {
provider,
callbackURL: successUrl.toString(),
},
asResponse: true,
});
+ console.log("[auth/desktop/connect] OAuth initiated successfully", { provider });
+ } catch (error) {
+ console.error("[auth/desktop/connect] Failed to initiate OAuth", { provider, error });
+ return new Response("Failed to initiate OAuth", { status: 500 });
+ }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."
📝 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 result = await auth.api.signInSocial({ | |
| body: { | |
| provider, | |
| callbackURL: successUrl.toString(), | |
| }, | |
| asResponse: true, | |
| }); | |
| let result; | |
| try { | |
| console.log("[auth/desktop/connect] Initiating OAuth", { provider, state: state.slice(0, 8) }); | |
| result = await auth.api.signInSocial({ | |
| body: { | |
| provider, | |
| callbackURL: successUrl.toString(), | |
| }, | |
| asResponse: true, | |
| }); | |
| console.log("[auth/desktop/connect] OAuth initiated successfully", { provider }); | |
| } catch (error) { | |
| console.error("[auth/desktop/connect] Failed to initiate OAuth", { provider, error }); | |
| return new Response("Failed to initiate OAuth", { status: 500 }); | |
| } |
🤖 Prompt for AI Agents
In @apps/api/src/app/api/auth/desktop/connect/route.ts around lines 22 - 28,
Wrap the call to auth.api.signInSocial in a try/catch: call
auth.api.signInSocial(...) as before, but if it throws catch the error, log it
using the prefixed pattern (e.g. console.error('[auth/signInSocial] failed for
provider=%s callbackURL=%s:', provider, successUrl.toString(), err)) to include
context and the error object, then either rethrow or return a meaningful error
Response (HTTP 500) so the failure is observable to callers; keep references to
auth.api.signInSocial, provider, successUrl, and result to locate the change.
| const cookies = result.headers.getSetCookie(); | ||
| const body = (await result.json()) as { url?: string; redirect?: boolean }; | ||
|
|
||
| if (!body.url) { | ||
| return new Response(`Failed to initiate OAuth: ${JSON.stringify(body)}`, { | ||
| status: 500, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Unsafe type assertion on external API response (still unresolved).
Line 31 continues to perform an unsafe type assertion on the auth API response without runtime validation. If the auth API returns an unexpected shape, this will fail at runtime.
Based on coding guidelines: "Validate at boundaries using Zod schemas for tRPC inputs and API route bodies."
🤖 Prompt for AI Agents
In @apps/api/src/app/api/auth/desktop/connect/route.ts around lines 30 - 37, The
code currently does an unsafe assertion on the external auth API response via
"const body = (await result.json()) as { url?: string; redirect?: boolean }" and
then uses body.url; replace this with runtime validation using a Zod schema
(e.g., define a schema for { url: string, redirect?: boolean }), parse the JSON
with await result.json(), then validate with schema.safeParse or schema.parse
before accessing body.url; handle validation failures by returning the 500
Response with the inspected validation error (or a safe error message) and
ensure you no longer use the unsafe "as { ... }" cast so unexpected shapes won’t
crash at runtime.
| if (!user) { | ||
| return new Response("User not found", { status: 401 }); | ||
| if (!sessionData?.user) { | ||
| return new Response("Unauthorized", { status: 401 }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract hardcoded error messages and status codes to constants.
Multiple error messages and HTTP status codes are hardcoded throughout the function. Per coding guidelines, extract these to named constants at the module top for better maintainability and consistency.
🔎 Proposed refactor to extract constants
Add constants at the top of the file:
+const HTTP_STATUS = {
+ UNAUTHORIZED: 401,
+ BAD_REQUEST: 400,
+} as const;
+
+const ERROR_MESSAGES = {
+ UNAUTHORIZED: "Unauthorized",
+ NO_ACTIVE_ORGANIZATION: "No active organization",
+ MISSING_TABLE_PARAMETER: "Missing table parameter",
+ UNKNOWN_TABLE: (tableName: string) => `Unknown table: ${tableName}`,
+} as const;
+
import { ELECTRIC_PROTOCOL_QUERY_PARAMS } from "@electric-sql/client";Then update the error responses:
if (!sessionData?.user) {
- return new Response("Unauthorized", { status: 401 });
+ return new Response(ERROR_MESSAGES.UNAUTHORIZED, { status: HTTP_STATUS.UNAUTHORIZED });
}
if (!organizationId) {
- return new Response("No active organization", { status: 400 });
+ return new Response(ERROR_MESSAGES.NO_ACTIVE_ORGANIZATION, { status: HTTP_STATUS.BAD_REQUEST });
}
if (!tableName) {
- return new Response("Missing table parameter", { status: 400 });
+ return new Response(ERROR_MESSAGES.MISSING_TABLE_PARAMETER, { status: HTTP_STATUS.BAD_REQUEST });
}
if (!whereClause) {
- return new Response(`Unknown table: ${tableName}`, { status: 400 });
+ return new Response(ERROR_MESSAGES.UNKNOWN_TABLE(tableName), { status: HTTP_STATUS.BAD_REQUEST });
}Also applies to: 16-16, 31-31, 36-36
🤖 Prompt for AI Agents
In @apps/api/src/app/api/electric/[...path]/route.ts at line 11, Extract all
hardcoded error messages and HTTP status codes used in route handler into
module-level named constants (e.g., AUTH_UNAUTHORIZED_MSG, AUTH_FORBIDDEN_MSG,
BAD_REQUEST_MSG and STATUS_UNAUTHORIZED = 401, STATUS_FORBIDDEN = 403,
STATUS_BAD_REQUEST = 400). Add these constants at the top of the file and
replace the inline uses in the route handler (the Response("Unauthorized", {
status: 401 }) and the other occurrences around the handler—lines referenced in
the review) with Response(AUTH_UNAUTHORIZED_MSG, { status: STATUS_UNAUTHORIZED
}) and analogously for the other error messages/statuses so all responses use
the new constants.
| return new Response("No active organization", { status: 400 }); | ||
| } | ||
|
|
||
| const url = new URL(request.url); |
There was a problem hiding this comment.
Add error handling for URL parsing and network operations.
The code lacks error handling for operations that can fail:
new URL(request.url)at line 19 can throw if the URL is malformedfetch(originUrl.toString())at line 45 can fail due to network errors, timeouts, or service unavailability
Per coding guidelines, never swallow errors silently—at minimum log them with context. Consider wrapping these operations in try-catch blocks and returning appropriate error responses with logging.
🔎 Proposed fix to add error handling
export async function GET(request: Request): Promise<Response> {
+ try {
const sessionData = await auth.api.getSession({
headers: request.headers,
});
// ... rest of authentication and validation logic ...
- const response = await fetch(originUrl.toString());
+ let response: Response;
+ try {
+ response = await fetch(originUrl.toString());
+ } catch (error) {
+ console.error("[electric/proxy] Failed to fetch from Electric:", error);
+ return new Response("Failed to connect to Electric service", { status: 502 });
+ }
const headers = new Headers();
// ... rest of response handling ...
+ } catch (error) {
+ console.error("[electric/proxy] Request processing failed:", error);
+ return new Response("Internal server error", { status: 500 });
+ }
}Also applies to: 45-45
| async initialize(): Promise<void> { | ||
| try { | ||
| const data = decrypt(await fs.readFile(TOKEN_FILE)); | ||
| const parsed: StoredAuth = JSON.parse(data); | ||
| this.token = parsed.token; | ||
| this.expiresAt = new Date(parsed.expiresAt); | ||
|
|
||
| if (this.isExpired()) { | ||
| console.log("[auth] Session expired, clearing"); | ||
| await this.signOut(); | ||
| } else { | ||
| console.log("[auth] Session restored"); | ||
| // Fetch session data from API | ||
| await this.refreshSession(); | ||
| } | ||
| } catch { | ||
| this.token = null; | ||
| this.expiresAt = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Log errors in the initialize catch block for debuggability.
The empty catch block at lines 70-73 silently handles all errors (decryption failures, JSON parse errors, filesystem issues), making debugging difficult. At minimum, log the error with context before clearing state.
🔎 Proposed fix
} catch {
+ console.log("[auth] Failed to restore session, starting fresh");
this.token = null;
this.expiresAt = null;
}Or for more detailed debugging:
- } catch {
+ } catch (error) {
+ console.error("[auth] Failed to restore session:", error);
this.token = null;
this.expiresAt = null;
}Based on coding guidelines: "Never swallow errors silently - at minimum log them with context"
📝 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.
| async initialize(): Promise<void> { | |
| try { | |
| const data = decrypt(await fs.readFile(TOKEN_FILE)); | |
| const parsed: StoredAuth = JSON.parse(data); | |
| this.token = parsed.token; | |
| this.expiresAt = new Date(parsed.expiresAt); | |
| if (this.isExpired()) { | |
| console.log("[auth] Session expired, clearing"); | |
| await this.signOut(); | |
| } else { | |
| console.log("[auth] Session restored"); | |
| // Fetch session data from API | |
| await this.refreshSession(); | |
| } | |
| } catch { | |
| this.token = null; | |
| this.expiresAt = null; | |
| } | |
| } | |
| async initialize(): Promise<void> { | |
| try { | |
| const data = decrypt(await fs.readFile(TOKEN_FILE)); | |
| const parsed: StoredAuth = JSON.parse(data); | |
| this.token = parsed.token; | |
| this.expiresAt = new Date(parsed.expiresAt); | |
| if (this.isExpired()) { | |
| console.log("[auth] Session expired, clearing"); | |
| await this.signOut(); | |
| } else { | |
| console.log("[auth] Session restored"); | |
| // Fetch session data from API | |
| await this.refreshSession(); | |
| } | |
| } catch (error) { | |
| console.error("[auth] Failed to restore session:", error); | |
| this.token = null; | |
| this.expiresAt = null; | |
| } | |
| } |
🤖 Prompt for AI Agents
In @apps/desktop/src/main/lib/auth/auth.ts around lines 55 - 74, The initialize
method currently swallows all errors in its catch block; change the catch to
capture the error (e.g., catch (err)) and log a contextual message including
TOKEN_FILE, decrypt/JSON parse context and the error (use console.error or the
module's logger) before clearing this.token and this.expiresAt and proceeding to
signOut if needed; update references around initialize, TOKEN_FILE, decrypt, and
signOut to ensure the log provides enough info to debug decryption/FS/parse
failures.
| try { | ||
| session = await auth.api.getSession({ headers: await headers() }); | ||
| } catch (error) { | ||
| console.error("Failed to get session for desktop auth:", error); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use prefixed console logging pattern.
Console logging should follow [domain/operation] message format per coding guidelines.
🤖 Prompt for AI Agents
In @apps/web/src/app/auth/desktop/success/page.tsx at line 31, Update the
console.error call in page.tsx to use the required prefixed logging pattern;
replace the current message "Failed to get session for desktop auth:" with a
prefixed message like "[auth/desktop] Failed to get session" while still passing
the error object as the second argument so the call becomes
console.error("[auth/desktop] Failed to get session", error). Locate the
existing console.error call in the desktop auth success page (the line that
currently reads console.error("Failed to get session for desktop auth:", error))
and change only the string to the bracketed prefix format to comply with the
logging convention.
| const expiresAt = new Date( | ||
| Date.now() + 60 * 60 * 24 * 30 * 1000, // 30 days (matching auth config) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract magic number to named constant.
Extract 60 * 60 * 24 * 30 * 1000 to a named constant like SESSION_EXPIRY_MS at module top. Consider importing from a shared location to stay in sync with the auth config.
🤖 Prompt for AI Agents
In @apps/web/src/app/auth/desktop/success/page.tsx around lines 66 - 68, Extract
the magic millisecond value used to compute expiresAt into a named constant
(e.g., SESSION_EXPIRY_MS) declared at the top of the module (or imported from
the shared auth config) and replace the inline expression in the expiresAt
assignment with that constant; ensure the constant name reflects it's in
milliseconds and keep the value equal to 60 * 60 * 24 * 30 * 1000 so it remains
in sync with the auth settings.
| const session = await auth.api.getSession({ | ||
| headers: await headers(), | ||
| }); |
There was a problem hiding this comment.
Add error handling for session retrieval.
The session retrieval has no error handling. If auth.api.getSession throws (e.g., database connection failure, network issue), the middleware will crash and break the entire application.
🔎 Proposed fix with error handling
export default async function proxy(req: NextRequest) {
+ let session;
+ try {
- const session = await auth.api.getSession({
+ session = await auth.api.getSession({
headers: await headers(),
});
+ } catch (error) {
+ console.error("[proxy/session] Failed to retrieve session:", error);
+ // Treat as unauthenticated on error to maintain availability
+ session = null;
+ }
const pathname = req.nextUrl.pathname;🤖 Prompt for AI Agents
In @apps/web/src/proxy.ts around lines 17 - 19, Wrap the call to
auth.api.getSession(...) in a try/catch to handle exceptions from session
retrieval: catch errors thrown by auth.api.getSession (and errors from
headers()) and return an appropriate response instead of letting the middleware
crash—e.g., log the error and return a 401/503 or propagate a handled error to
the middleware pipeline. Update the code around the session variable (the await
auth.api.getSession call that assigns session) to perform this guarded call, log
the caught error with context, and ensure downstream code checks for a
null/undefined session before proceeding.
| export const adminProcedure = protectedProcedure.use(async ({ ctx, next }) => { | ||
| const user = await db.query.users.findFirst({ | ||
| where: eq(users.clerkId, ctx.userId), | ||
| }); | ||
|
|
||
| if (!user) { | ||
| throw new TRPCError({ | ||
| code: "UNAUTHORIZED", | ||
| message: "User not found in database.", | ||
| }); | ||
| } | ||
|
|
||
| if (!user.email.endsWith(COMPANY.EMAIL_DOMAIN)) { | ||
| if (!ctx.session.user.email.endsWith(COMPANY.EMAIL_DOMAIN)) { | ||
| throw new TRPCError({ | ||
| code: "FORBIDDEN", | ||
| message: `Admin access requires ${COMPANY.EMAIL_DOMAIN} email.`, | ||
| }); | ||
| } | ||
|
|
||
| return next({ | ||
| ctx: { | ||
| user, | ||
| }, | ||
| }); | ||
| return next({ ctx }); | ||
| }); |
There was a problem hiding this comment.
Fix email domain validation and add null safety.
Two issues:
-
Potential null access:
ctx.session.user.emailis accessed without null check. If email is undefined,.endsWith()will throw. -
Vulnerable domain check: Same
endsWith()vulnerability—emails like[email protected]would pass.
🔎 Proposed fix
export const adminProcedure = protectedProcedure.use(async ({ ctx, next }) => {
+ const email = ctx.session.user.email;
+ if (!email) {
+ throw new TRPCError({
+ code: "FORBIDDEN",
+ message: "Email is required for admin access.",
+ });
+ }
+
+ const domain = email.split("@")[1];
- if (!ctx.session.user.email.endsWith(COMPANY.EMAIL_DOMAIN)) {
+ if (domain !== COMPANY.DOMAIN) {
throw new TRPCError({
code: "FORBIDDEN",
- message: `Admin access requires ${COMPANY.EMAIL_DOMAIN} email.`,
+ message: `Admin access requires a ${COMPANY.DOMAIN} email.`,
});
}
return next({ ctx });
});Note: Use COMPANY.DOMAIN ("superset.sh") for comparison instead of COMPANY.EMAIL_DOMAIN ("@superset.sh").
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @packages/trpc/src/trpc.ts around lines 44 - 53, The adminProcedure
middleware currently accesses ctx.session.user.email without null checks and
uses a fragile endsWith(COMPANY.EMAIL_DOMAIN); update adminProcedure (the
protectedProcedure.use async ({ ctx, next }) block) to first null-check that
ctx.session?.user?.email is a non-empty string and throw a FORBIDDEN TRPCError
if missing, then validate the email by parsing the part after the last "@" (or
splitting on "@") and compare that domain exactly to COMPANY.DOMAIN (e.g.,
"superset.sh") rather than using EMAIL_DOMAIN or endsWith, and finally return
next({ ctx }) as before.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @apps/api/src/app/api/desktop/version/route.ts:
- Line 1: MINIMUM_DESKTOP_VERSION in route.ts is set to "0.0.48" but desktop
release is v0.0.47, which will block clients; either revert
MINIMUM_DESKTOP_VERSION to "0.0.47" in the route.ts constant or coordinate with
the desktop team to publish v0.0.48 first and only then update
MINIMUM_DESKTOP_VERSION to "0.0.48" (update the constant MINIMUM_DESKTOP_VERSION
accordingly).
In @apps/marketing/src/env.ts:
- Line 13: Validate BETTER_AUTH_SECRET to require a minimum length of 32
characters: update the Zod schema entry for BETTER_AUTH_SECRET (currently
z.string()) to enforce min(32) and provide a clear error message so weak/short
secrets are rejected at startup; modify the BETTER_AUTH_SECRET schema
declaration in apps/marketing/src/env.ts (the Zod schema symbol
BETTER_AUTH_SECRET) accordingly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/admin/src/env.tsapps/api/src/app/api/desktop/version/route.tsapps/api/src/env.tsapps/marketing/src/env.tsapps/web/src/env.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web/src/env.ts
- apps/api/src/env.ts
- apps/admin/src/env.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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/marketing/src/env.tsapps/api/src/app/api/desktop/version/route.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/marketing/src/env.tsapps/api/src/app/api/desktop/version/route.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/marketing/src/env.tsapps/api/src/app/api/desktop/version/route.ts
apps/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/api/**/*.{ts,tsx}: Use TRPCError with appropriate error codes: NOT_FOUND, UNAUTHORIZED, FORBIDDEN, BAD_REQUEST, INTERNAL_SERVER_ERROR, NOT_IMPLEMENTED
tRPC procedures and API route handlers should validate and delegate; keep orchestrators thin
Extract business logic from tRPC procedures into utility functions when logic exceeds ~50 lines, is used by multiple procedures, or needs independent testing
Validate at boundaries using Zod schemas for tRPC inputs and API route bodies
Files:
apps/api/src/app/api/desktop/version/route.ts
🧠 Learnings (1)
📚 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/marketing/src/env.ts
⏰ 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 Admin
- GitHub Check: Deploy Web
- GitHub Check: Deploy Docs
- GitHub Check: Deploy API
- GitHub Check: Deploy Marketing
- GitHub Check: Build
| @@ -1,4 +1,4 @@ | |||
| const MINIMUM_DESKTOP_VERSION = "0.0.39"; | |||
| const MINIMUM_DESKTOP_VERSION = "0.0.48"; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify desktop app v0.0.48 exists and check for Better Auth references
# Check if desktop app exists and has releases/tags for v0.0.48
echo "=== Checking for desktop app version tags ==="
git tag | grep -E "^(v|desktop-)?0\.0\.48" || echo "No exact v0.0.48 tag found"
# Check directory structure
echo -e "\n=== Checking for desktop app directory ==="
if [ -d "apps/desktop" ]; then
echo "Desktop app directory exists"
ls -la apps/desktop 2>/dev/null | head -10
else
echo "No apps/desktop directory found"
fi
# Search for package.json in desktop or similar directories
echo -e "\n=== Searching for package.json files ==="
fd -t f "package\.json$" --max-one-result . 2>/dev/null | head -20
# Check current route file for context
echo -e "\n=== Checking current route file ==="
if [ -f "apps/api/src/app/api/desktop/version/route.ts" ]; then
cat "apps/api/src/app/api/desktop/version/route.ts"
else
echo "Route file not found"
fi
# Search for any Better Auth migration references
echo -e "\n=== Checking for Better Auth references in codebase ==="
rg "better-auth|betterAuth|BETTER_AUTH" --max-count 5 2>/dev/null || echo "No Better Auth references found in search"
# Check git log for relevant commits
echo -e "\n=== Checking recent commits mentioning desktop version ==="
git log --oneline -20 --all 2>/dev/null | grep -i "desktop\|version\|auth" || echo "No relevant commits found"Repository: superset-sh/superset
Length of output: 4512
🏁 Script executed:
#!/bin/bash
# Check for version files in desktop app
echo "=== Searching for version definitions in desktop app ==="
fd -t f "version|package" apps/desktop --max-depth 3 -x basename {} \;
# Check main package.json in desktop app
echo -e "\n=== Desktop app package.json content ==="
if [ -f "apps/desktop/package.json" ]; then
cat "apps/desktop/package.json" | head -30
fi
# Check for version file or constant
echo -e "\n=== Searching for version constants in desktop app ==="
rg "0\.0\.(4[7-9]|5[0-9])" apps/desktop -t typescript -t tsx --max-count 10 2>/dev/null || echo "No matching versions found"
# Check if there's a RELEASE.md or CHANGELOG
echo -e "\n=== Checking RELEASE.md in desktop app ==="
if [ -f "apps/desktop/RELEASE.md" ]; then
cat "apps/desktop/RELEASE.md"
fi
# Look for version references in current branch/commits
echo -e "\n=== Checking git log for v0.0.48 mentions ==="
git log --all --oneline --grep="0.0.48\|version.*48" 2>/dev/null | head -10 || echo "No commits mentioning 0.0.48"
# Check current branch status
echo -e "\n=== Current git status and branch ==="
git branch --show-current 2>/dev/null
git log -1 --oneline 2>/dev/nullRepository: superset-sh/superset
Length of output: 3542
Desktop v0.0.48 does not exist—this will break the desktop app upon deployment.
The API enforces MINIMUM_DESKTOP_VERSION = "0.0.48", but the latest desktop release is v0.0.47 (per apps/desktop/package.json). Deploying this change will prevent all users from connecting via the desktop app. Either bump and release desktop to v0.0.48 first, or revert MINIMUM_DESKTOP_VERSION to "0.0.47".
🤖 Prompt for AI Agents
In @apps/api/src/app/api/desktop/version/route.ts at line 1,
MINIMUM_DESKTOP_VERSION in route.ts is set to "0.0.48" but desktop release is
v0.0.47, which will block clients; either revert MINIMUM_DESKTOP_VERSION to
"0.0.47" in the route.ts constant or coordinate with the desktop team to publish
v0.0.48 first and only then update MINIMUM_DESKTOP_VERSION to "0.0.48" (update
the constant MINIMUM_DESKTOP_VERSION accordingly).
| }, | ||
| server: { | ||
| CLERK_SECRET_KEY: z.string(), | ||
| BETTER_AUTH_SECRET: z.string(), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Better Auth BETTER_AUTH_SECRET requirements minimum length format entropy
💡 Result:
Short answer
- Minimum: at least 32 characters (i.e., 32 bytes / 256 bits recommended). [1][2]
- Recommended generation: 32 random bytes, e.g. openssl rand -base64 32 (Base64) or a 64‑char hex string from 32 bytes — both provide ~256 bits of entropy. [1][2]
- Storage/format: any high‑entropy secret is acceptable; store in env var BETTER_AUTH_SECRET and never commit to source. Better Auth will read process.env.BETTER_AUTH_SECRET (or AUTH_SECRET). [2][3]
Commands/examples
- OpenSSL (Base64, ~44 chars): openssl rand -base64 32. [1][2]
- OpenSSL (hex, 64 chars): openssl rand -hex 32. [1]
Why 32 bytes?
- 32 bytes = 256 bits of entropy, which is cryptographically strong for signing/encryption/hashing. Better Auth explicitly recommends 32 bytes. [1][2]
Sources
[1] Better Auth docs — Installation / secret recommendation.
[2] Better Auth docs — Options/secret (env var names, default behavior, recommended generation).
[3] Better Auth secret generator / guidance (hex vs base64 notes).
🏁 Script executed:
# Check better-auth version in dependencies
find . -name "package.json" -type f | head -20 | xargs grep -l "better-auth" | head -5Repository: superset-sh/superset
Length of output: 166
🏁 Script executed:
# Check other env.ts files for secret validation patterns
find . -name "env.ts" -o -name "env.*.ts" | head -10Repository: superset-sh/superset
Length of output: 410
🏁 Script executed:
# Search for other BETTER_AUTH_SECRET definitions in codebase
rg "BETTER_AUTH_SECRET" -A 1 -B 1Repository: superset-sh/superset
Length of output: 1118
Add minimum length validation to BETTER_AUTH_SECRET.
The BETTER_AUTH_SECRET must be at least 32 characters (256 bits of entropy). Better Auth's documentation explicitly requires this minimum length for cryptographic security. Without validation, weak or empty secrets could be accepted in production, compromising authentication.
Required fix:
- BETTER_AUTH_SECRET: z.string(),
+ BETTER_AUTH_SECRET: z.string().min(32),📝 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.
| BETTER_AUTH_SECRET: z.string(), | |
| BETTER_AUTH_SECRET: z.string().min(32), |
🤖 Prompt for AI Agents
In @apps/marketing/src/env.ts at line 13, Validate BETTER_AUTH_SECRET to require
a minimum length of 32 characters: update the Zod schema entry for
BETTER_AUTH_SECRET (currently z.string()) to enforce min(32) and provide a clear
error message so weak/short secrets are rejected at startup; modify the
BETTER_AUTH_SECRET schema declaration in apps/marketing/src/env.ts (the Zod
schema symbol BETTER_AUTH_SECRET) accordingly.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Removals
Bug Fixes
Other
✏️ Tip: You can customize this high-level summary in your review settings.