Dev#2
Conversation
inside a desktop client later down the road
- Mount better-auth handler on Hono API server at /api/auth/** - Configure CORS with credentials for cookie-based sessions - Add baseURL to auth client using type-safe env - Create login and signup pages with useMutation - Add authenticated layout route to gate protected content - Add shadcn label component - Fix all FK columns from text to uuid to match PK types
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughMigrates the web app from Next.js to a Vite + TanStack Router frontend, adds frontend auth routes and an auth client, integrates the auth handler into the API, and expands the database schema to support guilds, roles, and UUID primary/foreign keys; also adds environment schema and Vite/TS configs. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (User)
participant Web as Web App (Vite + Router)
participant AuthClient as authClient (frontend)
participant API as API Server (/api/auth -> auth.handler)
participant AuthPkg as Auth Package (auth.handler)
participant DB as Database
Browser->>Web: Navigate to /login and submit credentials
Web->>AuthClient: authClient.signIn.email(email,password)
AuthClient->>API: POST /api/auth/signin (proxied)
API->>AuthPkg: Forward raw request to auth.handler
AuthPkg->>DB: Verify user credentials / session creation
DB-->>AuthPkg: user/session result
AuthPkg-->>API: response (token/session)
API-->>AuthClient: auth response
AuthClient-->>Web: resolve mutation, update session
Web-->>Browser: navigate to authenticated route
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/package.json (1)
30-30:⚠️ Potential issue | 🟠 MajorZod version mismatch between api and shared packages: api uses v3 (
^3.24.2) while shared@repo/envpackage uses v4 (^4.3.6).Since
apps/apidirectly depends on@repo/env(workspace:*), this creates a version conflict. Zod 4 has breaking changes including unified error customization, stricter number validation, and new default-value semantics. Whenapps/apiresolves its dependencies, it will install zod@^3.24.2, but@repo/envcode expects zod@^4.3.6, causing type incompatibilities and potential runtime validation failures. Alignapps/apito use zod@^4.3.6 to match the shared package.
🤖 Fix all issues with AI agents
In `@apps/api/src/app.ts`:
- Around line 12-16: The cors middleware in app.ts currently hardcodes origin to
"http://localhost:3000"; change it to read from an environment variable (e.g.,
process.env.CORS_ORIGIN or process.env.WEB_URL) with a sensible default for
local dev, and pass that value into the cors({ origin, credentials: true }) call
so staging/production origins work; update any config loading/typing to ensure
CORS_ORIGIN is available and documented.
In `@apps/web/package.json`:
- Line 9: The "start" script in package.json currently uses "vite preview",
which is only for local preview and not suitable as a production server; update
the "start" script to use a real static server or your chosen deployment
approach (for example replace the "start" command with a production server
command such as using the "serve" package like "serve -s dist" after running the
build, or configure your Node adapter/nginx/static host startup command), and
ensure corresponding build ("build") and postbuild steps produce the dist
directory before the new "start" command runs; target the "start" script entry
in package.json to make this change.
In `@apps/web/src/routes/login.tsx`:
- Around line 26-51: The form onSubmit calls signIn() which is the mutateAsync
promise (aliased from mutateAsync) causing potential unhandled promise
rejections; switch to the fire-and-forget mutate variant by changing the
useMutation destructure from mutateAsync: signIn to mutate: signIn (or call
signIn.mutate) so onSubmit can call signIn() without awaiting, or alternatively
await signIn() inside an async onSubmit and handle errors; update the
useMutation call (function name: useMutation) and the onSubmit handler (form
onSubmit) to use the non-async mutate symbol instead of mutateAsync to avoid
unhandled rejections.
In `@apps/web/src/routes/signup.tsx`:
- Around line 28-43: The submit handler is calling the mutateAsync alias
(signUp) which throws and causes an unhandled rejection; change the destructure
from mutateAsync: signUp to mutate: signUp in the useMutation call (keep the
existing async mutationFn and onSuccess), and update the form submit code to
call signUp(...) without awaiting or wrapping in try/catch so the mutation's
internal error state (isPending, error) handles failures; ensure no other code
awaits signUp to avoid unhandled promise rejections.
In `@packages/db/src/schemas/guilds.ts`:
- Around line 22-24: The comment on ownerId is misleading: update the comment
near ownerId and the onDelete: "restrict" clause to accurately state that using
onDelete: "restrict" prevents deletion of the referenced user (user.id) while
the guild row exists, i.e., the user deletion is blocked rather than the guild
being deleted; reference ownerId, user.id and onDelete: "restrict" in the
comment and replace the current text with a short, correct explanation like
"prevents deleting the user while a guild references them; guild row remains,
user deletion is restricted."
In `@packages/env/src/server.ts`:
- Around line 11-14: The addProtocol helper currently defaults to https://
silently; update it to validate and handle local addresses explicitly: modify
addProtocol(url: string) to first trim and check for an existing protocol, then
if none present detect localhost/127.0.0.1 or numeric loopback and default to
http:// for those cases, otherwise default to https://; alternatively (or
additionally) throw or log a clear error when a protocol is missing so callers
must supply an explicit scheme for non-https development cases. Ensure the
change is applied inside the addProtocol function and add a short
comment/docstring describing the behavior.
🧹 Nitpick comments (9)
packages/env/src/server.ts (1)
25-25:NEXT_PUBLIC_prefix is a Next.js convention — consider renaming for the Vite migration.Since the web app has migrated from Next.js to Vite, the
NEXT_PUBLIC_prefix no longer has framework-level meaning. Vite usesVITE_for client-exposed variables. While this env var is parsed server-side here, the naming is misleading and will cause confusion as the codebase moves further from Next.js.Consider renaming to something framework-agnostic like
API_URLorPUBLIC_API_URL, and updating references inpackages/auth/src/lib/auth-client.tsandapps/web/vite.config.tsaccordingly.apps/web/src/routes/_authenticated/index.tsx (1)
9-24: Consider handling loading state fromuseSession.
authClient.useSession()likely returnsisPending/errorstates alongsidedata. Currently, the sign-out button renders even whensessionisnull(still loading or unauthenticated), which may cause a brief flash of UI before the auth layout redirects. Consider destructuringisPendingand showing a loading indicator, or conditionally rendering the entire block.apps/web/vite.config.ts (1)
6-27: Consider migrating away from theNEXT_PUBLIC_prefix now that the app uses Vite.The config works correctly, but retaining the
NEXT_PUBLIC_prefix after moving to Vite is confusing for new contributors. Vite natively exposesVITE_-prefixed vars viaimport.meta.env, which would eliminate the need for the manualdefineblock entirely. This could be a follow-up task.packages/db/src/schemas/guild-members.ts (1)
6-23: Consider adding a unique constraint on(guildId, userId)to prevent duplicate memberships.Without a composite unique index, a user could be inserted as a member of the same guild multiple times. This is typically enforced at the DB level.
Proposed fix
+import { index, pgTable, text, timestamp, uniqueIndex, uuid } from "drizzle-orm/pg-core" ... (table) => [ index("guildMember_guildId_idx").on(table.guildId), index("guildMember_userId_idx").on(table.userId), + uniqueIndex("guildMember_guildId_userId_uidx").on(table.guildId, table.userId), ]apps/web/src/routes/signup.tsx (1)
34-40: Variableerrorshadows the outererrorfromuseMutationdestructuring.The
errordestructured fromauthClient.signUp.emailon line 34 shadows theerrorfromuseMutationon line 31. This works correctly here since the inner scope is isolated, but renaming the inner variable (e.g.,signUpError) would improve readability.apps/web/src/routes/_authenticated.tsx (1)
9-31: Consider using TanStack Router'sbeforeLoadfor auth guards.The current
useEffect-based redirect works but is client-side only and allows a brief flash ofnullbefore navigation. TanStack Router'sbeforeLoadcan handle this more cleanly by redirecting before the component renders.export const Route = createFileRoute("/_authenticated")({ beforeLoad: async () => { // check session and throw redirect("/login") if unauthenticated }, component: AuthenticatedLayout, })This is optional — the current approach is functional.
apps/web/src/routes/login.tsx (1)
30-34: Naming collision:errorshadows outer destructured variable.On line 32, the destructured
{ error }fromauthClient.signIn.emailshadows theerrorfrom theuseMutationdestructuring on line 29. While this works due to scoping, it reduces readability. Consider renaming the inner one (e.g.,signInError).packages/db/src/schemas/guild-roles.ts (1)
5-23: Consider adding a unique constraint on(guildId, role)to prevent duplicate role names per guild.Without a unique index, the same role name (e.g., "moderator") can be inserted multiple times for the same guild, leading to ambiguous permission lookups.
Proposed addition
(table) => [ index("guildRole_guildId_idx").on(table.guildId), index("guildRole_role_idx").on(table.role), + uniqueIndex("guildRole_guildId_role_uidx").on(table.guildId, table.role), ]packages/db/src/schemas/guilds.ts (1)
30-34: Consider naming the owner relationownerinstead ofuserfor clarity.Since
guildMemberalso referencesuser, naming this relationuseris ambiguous.ownerwould make the relationship semantics immediately clear.Proposed rename
export const guildRelations = relations(guild, ({ one, many }) => ({ - user: one(user, { + owner: one(user, { fields: [guild.ownerId], references: [user.id], }),
| "check-types": "next typegen && tsc --noEmit" | ||
| "dev": "vite --port 3000", | ||
| "build": "vite build", | ||
| "start": "vite preview --port 3000", |
There was a problem hiding this comment.
vite preview is not a production server.
vite preview is a lightweight static file server intended for locally previewing production builds. If this app will be deployed, you'll need a proper static hosting solution or a server (e.g., serve, nginx, or a Node adapter). Fine if this script is only used during development.
🤖 Prompt for AI Agents
In `@apps/web/package.json` at line 9, The "start" script in package.json
currently uses "vite preview", which is only for local preview and not suitable
as a production server; update the "start" script to use a real static server or
your chosen deployment approach (for example replace the "start" command with a
production server command such as using the "serve" package like "serve -s dist"
after running the build, or configure your Node adapter/nginx/static host
startup command), and ensure corresponding build ("build") and postbuild steps
produce the dist directory before the new "start" command runs; target the
"start" script entry in package.json to make this change.
| ownerId: uuid("owner_id") // this is the source of truth for the owner of the guild, the guildMember who owns this guild will also have role === "owner" so we will need to keep these in sync | ||
| .notNull() | ||
| .references(() => user.id, { onDelete: "restrict" }), // don't delete guild if owner deletes account |
There was a problem hiding this comment.
Misleading comment on onDelete: "restrict".
The comment says "don't delete guild if owner deletes account," but restrict actually prevents the user from being deleted while a guild references them. The guild stays untouched — it's the user deletion that gets blocked.
Suggested comment fix
- ownerId: uuid("owner_id") // this is the source of truth for the owner of the guild, the guildMember who owns this guild will also have role === "owner" so we will need to keep these in sync
- .notNull()
- .references(() => user.id, { onDelete: "restrict" }), // don't delete guild if owner deletes account
+ ownerId: uuid("owner_id") // source of truth for guild ownership; keep in sync with guildMember role === "owner"
+ .notNull()
+ .references(() => user.id, { onDelete: "restrict" }), // prevent deleting a user who still owns a guild📝 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.
| ownerId: uuid("owner_id") // this is the source of truth for the owner of the guild, the guildMember who owns this guild will also have role === "owner" so we will need to keep these in sync | |
| .notNull() | |
| .references(() => user.id, { onDelete: "restrict" }), // don't delete guild if owner deletes account | |
| ownerId: uuid("owner_id") // source of truth for guild ownership; keep in sync with guildMember role === "owner" | |
| .notNull() | |
| .references(() => user.id, { onDelete: "restrict" }), // prevent deleting a user who still owns a guild |
🤖 Prompt for AI Agents
In `@packages/db/src/schemas/guilds.ts` around lines 22 - 24, The comment on
ownerId is misleading: update the comment near ownerId and the onDelete:
"restrict" clause to accurately state that using onDelete: "restrict" prevents
deletion of the referenced user (user.id) while the guild row exists, i.e., the
user deletion is blocked rather than the guild being deleted; reference ownerId,
user.id and onDelete: "restrict" in the comment and replace the current text
with a short, correct explanation like "prevents deleting the user while a guild
references them; guild row remains, user deletion is restricted."
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/auth/src/lib/auth.ts`:
- Around line 11-12: trustedOrigins is set to an empty array in production
causing cross-origin auth requests to be rejected; update the config that sets
trustedOrigins to read a new environment variable (e.g., WEB_URL or CORS_ORIGIN)
instead of defaulting to [] so production includes the frontend origin(s).
Change the logic around trustedOrigins (referencing trustedOrigins and
env.NODE_ENV) to: in development keep ["http://localhost:3000"] plus any
provided override, and in non-development read and parse the WEB_URL/CORS_ORIGIN
env var (support comma-separated values) and assign those origins to
trustedOrigins; ensure the same env var is used consistently elsewhere (e.g.,
CORS setup) so the frontend origin is allowed in production.
- Around line 27-39: The references object on additionalFields.ownerId includes
an undocumented table property; update the ownerId.references (in the
additionalFields block for ownerId) to either remove the redundant table: "user"
entry so the references object only uses the documented { model, field, onDelete
} keys (e.g., keep model: "user", field: "id", onDelete: "restrict"), or if your
Drizzle adapter requires it, explicitly document/validate that requirement and
keep it—ensure the change is applied where additionalFields.ownerId.references
is defined so the schema uses the supported syntax.
| trustedOrigins: | ||
| env.NODE_ENV === "development" ? ["http://localhost:3000"] : [], |
There was a problem hiding this comment.
trustedOrigins is empty in production — cross-origin auth requests will be rejected.
In non-development environments, trustedOrigins resolves to []. If the frontend is served from a different origin than the API (which is typical), better-auth will block cross-origin requests (redirects, CSRF checks) in production. You need the frontend's origin here, not an empty list.
This is the same class of problem flagged in apps/api/src/app.ts for CORS: there's no dedicated env var for the frontend/web origin. Consider adding a WEB_URL (or CORS_ORIGIN) environment variable and using it in both places.
Proposed fix (illustrative)
trustedOrigins:
- env.NODE_ENV === "development" ? ["http://localhost:3000"] : [],
+ env.NODE_ENV === "development"
+ ? ["http://localhost:3000"]
+ : [env.WEB_URL], // add WEB_URL to your env schema🤖 Prompt for AI Agents
In `@packages/auth/src/lib/auth.ts` around lines 11 - 12, trustedOrigins is set to
an empty array in production causing cross-origin auth requests to be rejected;
update the config that sets trustedOrigins to read a new environment variable
(e.g., WEB_URL or CORS_ORIGIN) instead of defaulting to [] so production
includes the frontend origin(s). Change the logic around trustedOrigins
(referencing trustedOrigins and env.NODE_ENV) to: in development keep
["http://localhost:3000"] plus any provided override, and in non-development
read and parse the WEB_URL/CORS_ORIGIN env var (support comma-separated values)
and assign those origins to trustedOrigins; ensure the same env var is used
consistently elsewhere (e.g., CORS setup) so the frontend origin is allowed in
production.
| additionalFields: { | ||
| ownerId: { | ||
| type: "string", | ||
| fieldName: "ownerId", | ||
| references: { | ||
| field: "id", | ||
| table: "user", | ||
| model: "user", | ||
| onDelete: "restrict", | ||
| }, | ||
| required: true, | ||
| returned: true, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
better-auth organization plugin additionalFields references syntax 2024 2025
💡 Result:
In Better Auth (2024–2025+), the references syntax is part of a field definition inside a schema/additionalFields object:
- Supported base field
types in schema fields arestring | number | boolean | date. referencesis an optional object:{ model: string; field: string; onDelete?: "cascade" | ... }(defaultonDeleteiscascade).[1]
Organization plugin: additionalFields + references example
import { betterAuth } from "better-auth";
import { organization } from "better-auth/plugins";
export const auth = betterAuth({
plugins: [
organization({
schema: {
member: {
additionalFields: {
// e.g. FK to your "sites" table
siteId: {
type: "string",
required: false,
references: {
model: "sites",
field: "id",
onDelete: "cascade",
},
},
},
},
},
}),
],
});additionalFields can be added to Organization plugin models (e.g. organization, invitation, member, team, and also organizationRole), and Better Auth will include those fields in the relevant endpoints.[2]
Sources: Better Auth plugin schema field docs (incl. references).[1] Organization plugin docs on additionalFields (incl. supported models / examples).[2]
🏁 Script executed:
# First, let's examine the file in question and understand the context
cat -n packages/auth/src/lib/auth.ts | head -100Repository: BuckyMcYolo/townhall
Length of output: 2427
Remove or verify the undocumented table property in the ownerId references object.
The better-auth references syntax supports { model, field, onDelete }, but the code includes an additional table: "user" property that is not part of the documented API. The model: "user" correctly targets the built-in user entity (which is not aliased elsewhere in the organization plugin config), so remove the redundant table property or confirm it's required by your Drizzle adapter configuration.
🤖 Prompt for AI Agents
In `@packages/auth/src/lib/auth.ts` around lines 27 - 39, The references object on
additionalFields.ownerId includes an undocumented table property; update the
ownerId.references (in the additionalFields block for ownerId) to either remove
the redundant table: "user" entry so the references object only uses the
documented { model, field, onDelete } keys (e.g., keep model: "user", field:
"id", onDelete: "restrict"), or if your Drizzle adapter requires it, explicitly
document/validate that requirement and keep it—ensure the change is applied
where additionalFields.ownerId.references is defined so the schema uses the
supported syntax.
1 participant
Summary by CodeRabbit
New Features
Refactor