feat (auth): implement Google and GitHub sign up and log in supportin…#88
Conversation
…g multi dev environments
✅ Deploy Preview for develop-devlovers ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughAdds Google/GitHub OAuth end-to-end: UI buttons and icons, initiation routes, callback handlers with token exchange and account resolution, oauth_state cookie helpers, authEnv configuration, user table columns + migration, and a large Drizzle schema/relations refactor. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser as Frontend App
participant AppAPI as App (/api/auth)
participant Provider as OAuth Provider
participant DB as Database
rect rgb(230,240,255)
Note over User,Browser: Initiation
User->>Browser: Click "Sign in with Provider"
Browser->>AppAPI: GET /api/auth/{provider}
AppAPI->>Browser: 302 Redirect to Provider (sets oauth_state cookie)
Browser->>Provider: Open consent/authorize page
end
rect rgb(240,250,240)
Note over Provider,AppAPI: Callback & exchange
Provider->>Browser: Redirect to /api/auth/{provider}/callback?code=...
Browser->>AppAPI: GET callback (cookie included)
AppAPI->>AppAPI: consumeOAuthState(validate)
AppAPI->>Provider: Exchange code -> access token
Provider->>AppAPI: Return access token & profile/emails
end
rect rgb(250,240,240)
Note over AppAPI,DB: Account resolution
AppAPI->>DB: Query by providerId
alt found
DB->>AppAPI: existing user
AppAPI->>DB: optional update provider fields
else not found
AppAPI->>DB: Query by email
alt email match
DB->>AppAPI: user -> update with provider info
else
AppAPI->>DB: create new user with provider info
end
end
end
rect rgb(245,245,255)
Note over AppAPI,Browser: Session
AppAPI->>AppAPI: Sign JWT, set auth cookie
AppAPI->>Browser: Redirect to /
Browser->>User: Logged-in homepage
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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: 9
🧹 Nitpick comments (6)
frontend/drizzle/0008_dizzy_james_howlett.sql (1)
1-2: Consider adding an index onprovider_idfor lookup performance.The OAuth callback routes query users by
provider_id(e.g.,where(eq(users.providerId, googleId))). Without an index, these lookups will perform full table scans as user volume grows.Additionally, consider a unique constraint on
(provider, provider_id)to prevent duplicate OAuth account linkages at the database level.🔎 Suggested migration additions
ALTER TABLE "users" ADD COLUMN "provider" text DEFAULT 'credentials' NOT NULL;--> statement-breakpoint -ALTER TABLE "users" ADD COLUMN "provider_id" text; +ALTER TABLE "users" ADD COLUMN "provider_id" text;--> statement-breakpoint +CREATE INDEX "users_provider_id_idx" ON "users" ("provider_id") WHERE "provider_id" IS NOT NULL;--> statement-breakpoint +CREATE UNIQUE INDEX "users_provider_provider_id_unique" ON "users" ("provider", "provider_id") WHERE "provider_id" IS NOT NULL;frontend/app/[locale]/login/page.tsx (1)
86-92: Consider localizing the "or" separator text.The app uses
next-intlfor internationalization, but the "or" text in the divider is hardcoded. For consistency with the rest of the localized UI, consider using a translation key.🔎 Example using next-intl
+import { useTranslations } from 'next-intl'; export default function LoginPage() { + const t = useTranslations('auth'); // ... <div className="my-4 flex items-center gap-3"> <div className="h-px flex-1 bg-gray-200" /> - <span className="text-xs text-gray-500">or</span> + <span className="text-xs text-gray-500">{t('or')}</span> <div className="h-px flex-1 bg-gray-200" /> </div>frontend/app/[locale]/signup/page.tsx (1)
98-104: Consider extracting the divider into a shared component.The "or" divider markup is duplicated between login and signup pages. Extracting it into a reusable component (e.g.,
<OrDivider />) would improve maintainability and ensure consistent styling.🔎 Example shared component
// components/auth/OrDivider.tsx export function OrDivider() { return ( <div className="my-4 flex items-center gap-3"> <div className="h-px flex-1 bg-gray-200" /> <span className="text-xs text-gray-500">or</span> <div className="h-px flex-1 bg-gray-200" /> </div> ); }frontend/db/schema/users.ts (1)
22-24: Consider adding a unique constraint on(provider, providerId)for data integrity.The
providerIdcolumn stores external provider IDs (e.g., Google ID, GitHub ID), but different providers could theoretically have overlapping ID spaces. Additionally, the OAuth callback routes (e.g.,frontend/app/api/auth/google/callback/route.tsline 74) query only byproviderIdwithout filtering byprovider, which could return incorrect users if IDs collide across providers.Suggested schema improvement
provider: text("provider").notNull().default("credentials"), providerId: text("provider_id"), + + // Consider adding a unique index on (provider, providerId) for data integrity + // and updating callback queries to filter by both columnsAlternatively, update callback queries to include provider filtering:
.where(and(eq(users.providerId, googleId), eq(users.provider, "google")))frontend/app/api/auth/google/callback/route.ts (1)
64-66: Consider validatingverified_emailfrom Google profile.The Google profile includes
verified_emailboolean, but the code trusts the email regardless of its verification status. For security, consider checking this field before creating/linking accounts.Suggested validation
const profile = (await profileRes.json()) as GoogleProfile; +if (!profile.verified_email) { + return NextResponse.redirect(new URL("/login?error=unverified_email", req.url)); +} + const email = profile.email;frontend/lib/env.ts (1)
1-7: Consider validatingAPP_ENVagainst allowed values.The type assertion
as AppEnv | undefineddoesn't validate that the value is actually one of the allowed values. An invalid value like"staging"would pass the existence check but cause unexpected behavior.Suggested validation
type AppEnv = "local" | "develop" | "production"; -const APP_ENV = process.env.APP_ENV as AppEnv | undefined; +const validEnvs: AppEnv[] = ["local", "develop", "production"]; +const rawAppEnv = process.env.APP_ENV; -if (!APP_ENV) { +if (!rawAppEnv) { throw new Error("APP_ENV is not defined"); } + +if (!validEnvs.includes(rawAppEnv as AppEnv)) { + throw new Error(`Invalid APP_ENV: ${rawAppEnv}. Must be one of: ${validEnvs.join(", ")}`); +} + +const APP_ENV = rawAppEnv as AppEnv;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
frontend/app/[locale]/login/page.tsxfrontend/app/[locale]/signup/page.tsxfrontend/app/api/auth/github/callback/route.tsfrontend/app/api/auth/github/route.tsfrontend/app/api/auth/google/callback/route.tsfrontend/app/api/auth/google/route.tsfrontend/components/auth/OAuthButtons.tsxfrontend/components/auth/ProviderButton.tsxfrontend/components/auth/icons/GitHubIcon.tsxfrontend/components/auth/icons/GoogleIcon.tsxfrontend/db/schema/users.tsfrontend/drizzle/0008_dizzy_james_howlett.sqlfrontend/drizzle/meta/0008_snapshot.jsonfrontend/drizzle/meta/_journal.jsonfrontend/drizzle/relations.tsfrontend/drizzle/schema.tsfrontend/lib/env.ts
🧰 Additional context used
🧬 Code graph analysis (7)
frontend/components/auth/ProviderButton.tsx (1)
frontend/components/ui/button.tsx (1)
Button(42-42)
frontend/app/[locale]/login/page.tsx (1)
frontend/components/auth/OAuthButtons.tsx (1)
OAuthButtons(5-21)
frontend/app/api/auth/google/route.ts (3)
frontend/app/api/auth/github/route.ts (1)
GET(4-14)frontend/app/api/auth/google/callback/route.ts (1)
GET(24-128)frontend/lib/env.ts (1)
env(17-49)
frontend/components/auth/OAuthButtons.tsx (3)
frontend/components/auth/ProviderButton.tsx (1)
ProviderButton(12-32)frontend/components/auth/icons/GoogleIcon.tsx (1)
GoogleIcon(1-15)frontend/components/auth/icons/GitHubIcon.tsx (1)
GitHubIcon(1-13)
frontend/app/api/auth/github/route.ts (3)
frontend/app/api/auth/github/callback/route.ts (1)
GET(28-150)frontend/app/api/auth/google/route.ts (1)
GET(4-16)frontend/lib/env.ts (1)
env(17-49)
frontend/app/[locale]/signup/page.tsx (1)
frontend/components/auth/OAuthButtons.tsx (1)
OAuthButtons(5-21)
frontend/drizzle/schema.ts (3)
frontend/db/schema/users.ts (1)
users(10-35)frontend/db/schema/questions.ts (2)
questions(14-27)questionTranslations(29-42)frontend/db/schema/categories.ts (2)
categories(12-19)categoryTranslations(21-33)
🔇 Additional comments (14)
frontend/drizzle/meta/_journal.json (1)
60-67: LGTM!The journal entry correctly tracks the new migration with appropriate index, version, and timestamp following the established pattern.
frontend/components/auth/OAuthButtons.tsx (1)
5-21: LGTM!Clean composition of the OAuth buttons using the ProviderButton abstraction. The component is simple, well-structured, and correctly delegates the OAuth initiation logic to the underlying ProviderButton.
frontend/components/auth/icons/GitHubIcon.tsx (1)
1-13: LGTM!Well-implemented icon component with proper accessibility attributes (
aria-hidden="true"for decorative icon) andcurrentColorfill for theme compatibility.frontend/components/auth/icons/GoogleIcon.tsx (1)
1-15: LGTM!Correctly implements the Google logo with proper brand colors and accessibility attributes. The multi-path SVG structure appropriately handles the distinct color segments of the Google "G".
frontend/components/auth/ProviderButton.tsx (1)
1-31: LGTM!Clean implementation of the OAuth provider button. Using
window.location.hreffor navigation is appropriate here since OAuth flows require full page redirects. The type-safe provider union ensures only valid providers are used.frontend/drizzle/meta/0008_snapshot.json (1)
969-981: Snapshot reflects missing unique constraint on OAuth provider columns.The snapshot shows
providerandprovider_idcolumns on the users table without a composite unique constraint. This aligns with the schema definition but reinforces the earlier recommendation to add a unique constraint on(provider, provider_id)to prevent duplicate OAuth accounts.This is an auto-generated file, so the fix should be applied in the source schema (
frontend/db/schema/users.ts) and a new migration generated.frontend/app/api/auth/google/callback/route.ts (1)
87-99: Account linking overwrites existing authentication method.When a user with an existing email signs in via Google, their
provideris updated to "google", which may prevent them from logging in via their original method (e.g., email/password). Consider whether this is the intended behavior or if multi-provider support per account is needed.If single-provider-per-account is intentional, document this behavior. Otherwise, consider a separate
user_providersjunction table to support multiple auth methods per user.frontend/lib/env.ts (1)
17-49: LGTM on the environment configuration structure.The centralized env config with environment-specific OAuth credentials is well-structured. The separation of GitHub credentials per environment (local/develop/prod) allows using different OAuth apps, which is a good practice for isolation.
frontend/drizzle/relations.ts (6)
1-2: LGTM!The imports are comprehensive and correctly pull in all the schema tables required for the relation definitions.
4-16: LGTM!The
ordersRelationsandusersRelationsare correctly defined with proper bidirectional mappings. Theonerelation from orders to users with explicit field/reference mapping and the correspondingmanyrelation from users to orders follow Drizzle ORM best practices.
18-36: LGTM!The quiz-related relations are well-structured with proper bidirectional mappings between
quizQuestions,quizzes,categories, and their associated entities (answers, attempts, translations, contents).
38-55: LGTM!The transaction and product relations are correctly defined with proper field mappings. Bidirectional relations between
pointTransactions↔usersandproductPrices↔productsare properly established.
57-69: LGTM!The additions of
questionTranslations,quizzes, andcategoryTranslationsto the existing relations properly extend the schema to support translations and the quiz-category relationship.
128-154: LGTM!The translation relation definitions (
categoryTranslationsRelations,questionTranslationsRelations,quizQuestionContentRelations) correctly establish theoneside of the relationships back to their parent entities, complementing themanyrelations defined in the parent relation exports.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
frontend/drizzle/schema.ts (3)
9-36: Critical: Forward reference issue persists.The
orderstable referencesusers.id(line 30) in its foreign key, butusersis defined much later at line 271. This will cause runtime errors when the schema is loaded, as theuserssymbol is not yet defined when the foreign key is constructed.🔎 Recommended fix
Move the
userstable definition before theorderstable (i.e., move lines 271-283 to appear before line 9), or restructure the file to declare all base tables before tables with foreign keys.
36-36: Missing check constraint for non-negative total amount.The database snapshot indicates a check constraint
orders_total_amount_minor_non_negativeshould enforcetotal_amount_minor >= 0, but this constraint is missing from the schema definition.🔎 Proposed fix
], (table) => [ foreignKey({...}).onDelete("set null"), unique("orders_idempotency_key_unique").on(table.idempotencyKey), check("orders_payment_intent_id_null_when_none", sql`(payment_provider <> 'none'::text) OR (payment_intent_id IS NULL)`), check("orders_payment_provider_valid", sql`payment_provider = ANY (ARRAY['stripe'::text, 'none'::text])`), + check("orders_total_amount_minor_non_negative", sql`total_amount_minor >= 0`), ]);
271-283: Critical:userstable is missing OAuth-related columns.The
userstable definition is missing theproviderandproviderIdcolumns that are:
- Defined in
frontend/db/schema/users.ts(lines 18-20)- Used by the OAuth callback handlers in this PR
- Essential for the OAuth authentication flow to function
Without these columns, the GitHub and Google OAuth callbacks will fail when attempting to store provider information.
🔎 Proposed fix
export const users = pgTable("users", { id: uuid().defaultRandom().primaryKey().notNull(), name: text(), email: text().notNull(), passwordHash: text("password_hash"), + provider: text().notNull().default("credentials"), + providerId: text("provider_id"), emailVerified: timestamp("email_verified", { mode: "string" }), image: text(), role: text().default("user").notNull(), points: integer().default(0).notNull(), createdAt: timestamp("created_at", { mode: "string" }).defaultNow().notNull(), }, (table) => [ unique("users_email_unique").on(table.email), ]);frontend/app/api/auth/google/route.ts (1)
4-16: Critical: Missing CSRF protection withstateparameter.The OAuth 2.0 authorization flow is missing a
stateparameter, which leaves it vulnerable to CSRF attacks. An attacker could initiate an OAuth flow and trick a victim into completing it, potentially linking the attacker's account to the victim's session.🔎 Recommended fix with state parameter
import { authEnv } from "@/lib/env/auth"; import { NextResponse } from "next/server"; +import { cookies } from "next/headers"; +import { randomBytes } from "crypto"; export async function GET() { + const state = randomBytes(16).toString("hex"); + const cookieStore = await cookies(); + cookieStore.set("oauth_state", state, { + httpOnly: true, + secure: process.env.NODE_ENV === "production", + sameSite: "lax", + maxAge: 60 * 10, // 10 minutes + }); + const params = new URLSearchParams({ client_id: authEnv.google.clientId!, redirect_uri: authEnv.google.redirectUri!, response_type: "code", scope: "openid email profile", prompt: "select_account", + state, }); return NextResponse.redirect( `https://accounts.google.com/o/oauth2/v2/auth?${params.toString()}` ); }Note: The callback route (
frontend/app/api/auth/google/callback/route.ts) must then validate thestateparameter matches the cookie before processing the authorization code.frontend/app/api/auth/github/callback/route.ts (3)
61-65: Missing requiredUser-Agentheader for GitHub API.GitHub's API requires a
User-Agentheader on all requests. While the requests may currently work, GitHub may reject or rate-limit requests missing this header, causing intermittent authentication failures.🔎 Proposed fix
const userRes = await fetch("https://api.github.com/user", { headers: { Authorization: `Bearer ${tokenData.access_token}`, + "User-Agent": "DevLovers-App", + Accept: "application/vnd.github.v3+json", }, });Note: The same fix should be applied to the
/user/emailsrequest at lines 73-77.
73-77: Missing requiredUser-Agentheader for GitHub API.Same issue as the
/userendpoint—theUser-Agentheader is required by GitHub's API.🔎 Proposed fix
const emailsRes = await fetch("https://api.github.com/user/emails", { headers: { Authorization: `Bearer ${tokenData.access_token}`, + "User-Agent": "DevLovers-App", + Accept: "application/vnd.github.v3+json", }, });
96-100: Major: Provider filter missing in user lookup query.The query searches by
providerIdalone, which could cause collisions if a user has the same ID from different OAuth providers (e.g., GitHub ID "12345" and Google ID "12345"). This will incorrectly match users across providers.🔎 Recommended fix
+import { and } from "drizzle-orm"; const [githubUser] = await db .select() .from(users) - .where(eq(users.providerId, githubId)) + .where(and(eq(users.providerId, githubId), eq(users.provider, "github"))) .limit(1);
🧹 Nitpick comments (4)
frontend/app/api/auth/google/route.ts (1)
6-7: Remove redundant non-null assertions.The non-null assertions (
!) onauthEnv.google.clientId!andauthEnv.google.redirectUri!are unnecessary becauseauthEnvis constructed usingrequireEnv()infrontend/lib/env/auth.ts, which already throws an error at module load time if these values are missing. The assertions here are redundant and can mask the actual type.🔎 Proposed simplification
const params = new URLSearchParams({ - client_id: authEnv.google.clientId!, - redirect_uri: authEnv.google.redirectUri!, + client_id: authEnv.google.clientId, + redirect_uri: authEnv.google.redirectUri, response_type: "code", scope: "openid email profile", prompt: "select_account", });frontend/app/api/auth/github/callback/route.ts (1)
43-46: Remove redundant non-null assertions.The non-null assertions on
authEnv.github.clientId!,authEnv.github.clientSecret!, andauthEnv.github.redirectUri!are unnecessary. These values are already validated byrequireEnv()infrontend/lib/env/auth.ts, which throws at module initialization if any are missing.🔎 Proposed simplification
body: new URLSearchParams({ - client_id: authEnv.github.clientId!, - client_secret: authEnv.github.clientSecret!, + client_id: authEnv.github.clientId, + client_secret: authEnv.github.clientSecret, code, - redirect_uri: authEnv.github.redirectUri!, + redirect_uri: authEnv.github.redirectUri, }),frontend/lib/env/auth.ts (2)
3-7: Add runtime validation forAPP_ENVvalue.While the code checks that
APP_ENVis defined, it doesn't validate that it's one of the expected values ("local","develop", or"production"). IfAPP_ENVis set to an invalid value like"staging", the conditional logic will silently fall through to theproductionbranch, potentially using incorrect credentials.🔎 Proposed fix with validation
type AppEnv = "local" | "develop" | "production"; const APP_ENV = process.env.APP_ENV as AppEnv | undefined; if (!APP_ENV) { throw new Error("APP_ENV is not defined"); } + +const validEnvs: AppEnv[] = ["local", "develop", "production"]; +if (!validEnvs.includes(APP_ENV)) { + throw new Error( + `APP_ENV must be one of: ${validEnvs.join(", ")}. Got: ${APP_ENV}` + ); +}
9-15: Consider validating against empty strings.The
requireEnvhelper only checks for missing/undefined values but allows empty strings. If an environment variable is set to an empty string (e.g.,GOOGLE_CLIENT_ID=""), it will pass validation but cause OAuth failures.🔎 Proposed enhancement
function requireEnv(name: string): string { const value = process.env[name]; - if (!value) { + if (!value || value.trim() === "") { throw new Error(`Missing env var: ${name}`); } return value; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/app/api/auth/github/callback/route.tsfrontend/app/api/auth/github/route.tsfrontend/app/api/auth/google/callback/route.tsfrontend/app/api/auth/google/route.tsfrontend/drizzle/schema.tsfrontend/lib/env/auth.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/app/api/auth/github/route.ts
- frontend/app/api/auth/google/callback/route.ts
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/app/api/auth/google/route.ts (4)
frontend/app/api/auth/github/callback/route.ts (1)
GET(28-150)frontend/app/api/auth/github/route.ts (1)
GET(4-14)frontend/app/api/auth/google/callback/route.ts (1)
GET(24-128)frontend/lib/env/auth.ts (1)
authEnv(17-49)
frontend/drizzle/schema.ts (3)
frontend/db/schema/users.ts (1)
users(10-35)frontend/db/schema/questions.ts (2)
questions(14-27)questionTranslations(29-42)frontend/db/schema/categories.ts (2)
categories(12-19)categoryTranslations(21-33)
🔇 Additional comments (1)
frontend/lib/env/auth.ts (1)
17-49: Well-structured multi-environment configuration.The environment-specific configuration for OAuth credentials is well-organized and makes the multi-environment setup clear. The use of
requireEnvensures fail-fast behavior if any required variables are missing, which is good for catching configuration issues early.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
frontend/app/api/auth/google/route.ts (1)
4-16: Critical: Missing CSRF protection viastateparameter.The OAuth authorization flow lacks a
stateparameter, leaving it vulnerable to CSRF attacks where an attacker could initiate an OAuth flow and trick a user into linking the attacker's Google account.The previous review comment provided a comprehensive fix. Generate a cryptographically secure random state (using
crypto.randomBytes), store it in a secure, httpOnly cookie with appropriatesameSiteandmaxAgesettings, include it in the authorization URL, and validate it in the callback route before processing the response.Note: With Next.js 15+, remember that
cookies()is now asynchronous and must be awaited.frontend/app/api/auth/github/route.ts (1)
4-14: Critical: Missing CSRF protection viastateparameter.Identical to the Google OAuth route, this GitHub authorization flow lacks a
stateparameter for CSRF protection. An attacker could initiate an OAuth flow and trick a user into linking the attacker's GitHub account.Apply the same fix as recommended for the Google route: generate a secure random state, store it in a secure httpOnly cookie, include it in the authorization URL, and validate it in the callback route.
frontend/app/api/auth/github/callback/route.ts (2)
61-65: Minor: Missing requiredUser-Agentheader for GitHub API requests.GitHub API requires a
User-Agentheader on all API requests. While the requests may currently work, GitHub may reject or rate-limit requests that omit this header.Add a
User-Agentheader to both the/userand/user/emailsAPI requests. The previous review suggested using an app identifier like"DevLovers-App"or reading from an environment variable.Also applies to: Lines 73-77
96-100: Major: Query byproviderIdwithoutproviderfilter creates collision risk.The user lookup queries only by
providerId, which could lead to cross-provider ID collisions. If a Google user and a GitHub user happen to have the same provider ID, this query could match the wrong user, potentially allowing account takeover.🔎 Proposed fix
+import { and } from "drizzle-orm"; const [githubUser] = await db .select() .from(users) - .where(eq(users.providerId, githubId)) + .where(and(eq(users.providerId, githubId), eq(users.provider, "github"))) .limit(1);
🧹 Nitpick comments (1)
frontend/app/api/auth/github/callback/route.ts (1)
102-139: Consider wrapping user creation/update in a transaction.The user lookup, update, and insert operations are not wrapped in a transaction. While the unique email constraint provides some protection, concurrent OAuth callbacks for the same new user could lead to a database conflict and a less graceful failure.
🔎 Example using Drizzle transaction
user = await db.transaction(async (tx) => { const [emailUser] = await tx .select() .from(users) .where(eq(users.email, primaryEmail)) .limit(1); if (emailUser) { await tx .update(users) .set({ provider: "github", providerId: githubId, emailVerified: emailUser.emailVerified ?? new Date(), image: emailUser.image ?? ghUser.avatar_url, name: emailUser.name ?? ghUser.name ?? ghUser.login, }) .where(eq(users.id, emailUser.id)); return emailUser; } else { const [created] = await tx .insert(users) .values({ email: primaryEmail, name: ghUser.name ?? ghUser.login, image: ghUser.avatar_url, provider: "github", providerId: githubId, emailVerified: new Date(), }) .returning(); return created; } });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/app/api/auth/github/callback/route.tsfrontend/app/api/auth/github/route.tsfrontend/app/api/auth/google/callback/route.tsfrontend/app/api/auth/google/route.tsfrontend/lib/env/auth.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/lib/env/auth.ts
- frontend/app/api/auth/google/callback/route.ts
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/app/api/auth/github/callback/route.ts (7)
frontend/app/api/auth/github/route.ts (1)
GET(4-14)frontend/app/api/auth/google/callback/route.ts (1)
GET(24-128)frontend/lib/env/auth.ts (1)
authEnv(17-49)frontend/db/index.ts (1)
db(17-17)frontend/db/schema/users.ts (1)
users(10-35)frontend/drizzle/schema.ts (1)
users(271-283)frontend/lib/auth.ts (2)
signAuthToken(35-39)setAuthCookie(75-85)
frontend/app/api/auth/google/route.ts (4)
frontend/app/api/auth/github/callback/route.ts (1)
GET(28-150)frontend/app/api/auth/github/route.ts (1)
GET(4-14)frontend/app/api/auth/google/callback/route.ts (1)
GET(24-128)frontend/lib/env/auth.ts (1)
authEnv(17-49)
frontend/app/api/auth/github/route.ts (4)
frontend/app/api/auth/github/callback/route.ts (1)
GET(28-150)frontend/app/api/auth/google/callback/route.ts (1)
GET(24-128)frontend/app/api/auth/google/route.ts (1)
GET(4-16)frontend/lib/env/auth.ts (1)
authEnv(17-49)
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
frontend/drizzle/relations.ts (1)
92-105: Minor naming inconsistency inquizAttemptsRelations.The relation is named
answers(line 103) but other relations use the full table name pattern (e.g.,quizAttemptAnswers). Consider renaming toquizAttemptAnswersfor consistency with the pattern used elsewhere.🔎 Suggested change
export const quizAttemptsRelations = relations( quizAttempts, ({ one, many }) => ({ user: one(users, { fields: [quizAttempts.userId], references: [users.id], }), quiz: one(quizzes, { fields: [quizAttempts.quizId], references: [quizzes.id], }), - answers: many(quizAttemptAnswers), + quizAttemptAnswers: many(quizAttemptAnswers), }) );frontend/drizzle/schema.ts (1)
20-22: Consider adding unique constraint on(provider, providerId).The OAuth callbacks lookup users by
(provider, providerId), but there's no unique constraint to prevent the same OAuth account from being linked to multiple local users. This could lead to data integrity issues.🔎 Proposed fix
}, (table) => [ unique("users_email_unique").on(table.email), + unique("users_provider_provider_id_unique").on(table.provider, table.providerId), ]);Note: A partial unique index may be more appropriate since
providerIdisNULLfor credential-based users:CREATE UNIQUE INDEX users_provider_provider_id_unique ON users (provider, provider_id) WHERE provider_id IS NOT NULL;frontend/db/schema/users.ts (1)
22-24: LGTM! OAuth provider columns added correctly.The
providercolumn with a sensible default of"credentials"ensures backward compatibility for existing users, andproviderIdis correctly nullable for credential-based accounts.Consider adding a composite unique constraint on
(provider, providerId)to prevent duplicate OAuth accounts from being linked to multiple users. This constraint does not currently exist in the schema.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/app/api/auth/github/callback/route.tsfrontend/app/api/auth/github/route.tsfrontend/app/api/auth/google/callback/route.tsfrontend/app/api/auth/google/route.tsfrontend/db/schema/users.tsfrontend/drizzle/relations.tsfrontend/drizzle/schema.tsfrontend/lib/auth/oauth-state.tsfrontend/lib/env/auth.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/app/api/auth/github/route.ts
- frontend/lib/env/auth.ts
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/db/schema/users.ts (1)
frontend/drizzle/schema.ts (1)
quizAttempts(220-241)
frontend/drizzle/schema.ts (3)
frontend/db/schema/users.ts (1)
users(10-35)frontend/db/schema/questions.ts (2)
questions(14-27)questionTranslations(29-42)frontend/db/schema/categories.ts (2)
categories(12-19)categoryTranslations(21-33)
🔇 Additional comments (6)
frontend/db/schema/users.ts (1)
37-39: LGTM! Relation key renamed for consistency.Renaming
attemptstoquizAttemptsaligns with the table name and maintains bidirectional consistency withquizAttemptsRelations.user.frontend/drizzle/relations.ts (1)
13-17: LGTM! Relations consolidated with bidirectional navigation.The
usersRelationsnow correctly includesquizAttempts: many(quizAttempts), completing the bidirectional relationship withquizAttemptsRelations.user. This addresses the previous review feedback.frontend/app/api/auth/github/callback/route.ts (1)
33-43: LGTM! OAuth state validation and code check are properly implemented.The early validation of state and code parameters with redirects to
/loginfollows the expected OAuth security pattern.frontend/app/api/auth/google/callback/route.ts (1)
75-78: Good: Unverified email handling with informative error.Unlike silent redirects elsewhere, this provides an error query param
?error=unverified_emailwhich can be displayed to the user. Consider adding similar error params to other failure cases for better UX.frontend/drizzle/schema.ts (2)
182-189: LGTM! Categories table correctly defined.The categories table is now defined before being referenced by
questionsandquizzes, avoiding forward reference issues for these relationships.
54-68: This forward reference is not a problem.
quizQuestionscorrectly uses theforeignKeyoperator in the table constraint callback (lines 63-67) to referencequizzes.id. This approach is the proper pattern in drizzle-orm for handling forward references, regardless of table definition order. The foreign key constraint is evaluated at schema registration time, not at module parse time, so the ordering ofquizzesafterquizQuestionsis not an issue.Likely an incorrect or invalid review comment.
| return crypto.timingSafeEqual( | ||
| Buffer.from(actual), | ||
| Buffer.from(expected) | ||
| ); |
There was a problem hiding this comment.
timingSafeEqual throws if buffer lengths differ.
If the actual state cookie is corrupted or truncated, Buffer.from(actual) and Buffer.from(expected) will have different lengths, causing timingSafeEqual to throw an exception rather than return false.
🔎 Proposed fix
if (!expected || !actual) return false;
+if (actual.length !== expected.length) return false;
return crypto.timingSafeEqual(
Buffer.from(actual),
Buffer.from(expected)
);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/lib/auth/oauth-state.ts around lines 29 to 32, timingSafeEqual can
throw when the two buffers differ in length; guard against this by checking the
byte length (or buffer length) of the two values first and return false if they
differ, ensure both inputs are converted to Buffers only once before the check,
and only call crypto.timingSafeEqual when lengths match so it never throws on
mismatched sizes.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/db/queries/users.tsfrontend/db/schema/quiz.ts
🔇 Additional comments (1)
frontend/db/schema/quiz.ts (1)
195-195: LGTM! Relation field renamed for consistency.The rename from
attemptstoquizAttemptsaligns with the actual table name and improves clarity across the schema.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
frontend/drizzle/schema.ts (1)
8-22: Add unique constraint for OAuth provider accounts.The
userstable hasproviderandproviderIdcolumns for OAuth authentication (lines 13-14), but lacks a unique constraint ensuring one account per provider. Without this, the same Google or GitHub account could be linked multiple times, creating duplicate user records.🔎 Proposed fix
}, (table) => [ unique("users_email_unique").on(table.email), + unique("users_provider_provider_id_unique").on(table.provider, table.providerId), ]);Note: Consider whether the constraint should allow
NULLproviderId values (for credential-based accounts) or if those should use a different pattern.
🧹 Nitpick comments (1)
frontend/app/api/auth/google/route.ts (1)
9-14: Config validation looks good. Consider explicit error response.Great fix using
authEnv.googleinstead of the previous GitHub config. The validation prevents runtime issues with missing configuration.💡 Optional: Return explicit error response for better client feedback
export async function GET() { const { clientId, redirectUri } = authEnv.google if (!clientId || !redirectUri) { - throw new Error("Google OAuth is not properly configured"); + return NextResponse.json( + { error: "Google OAuth is not properly configured" }, + { status: 500 } + ); }This provides a JSON error response instead of relying on Next.js error handling, which can be clearer for API consumers.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/app/api/auth/github/callback/route.tsfrontend/app/api/auth/google/callback/route.tsfrontend/app/api/auth/google/route.tsfrontend/db/queries/users.tsfrontend/drizzle/schema.tsfrontend/lib/auth/oauth-state.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/lib/auth/oauth-state.ts
- frontend/app/api/auth/google/callback/route.ts
- frontend/app/api/auth/github/callback/route.ts
- frontend/db/queries/users.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/drizzle/schema.ts (3)
frontend/db/schema/users.ts (1)
users(10-35)frontend/db/schema/questions.ts (2)
questions(14-27)questionTranslations(29-42)frontend/db/schema/categories.ts (2)
categories(12-19)categoryTranslations(21-33)
🔇 Additional comments (4)
frontend/app/api/auth/google/route.ts (4)
1-6: LGTM! Clean import structure.The imports are well-organized and appropriate for the OAuth initiation flow. Using dedicated helper functions for OAuth state management promotes code reusability across providers.
19-26: Excellent OAuth parameters configuration.All parameters are correctly configured for Google's OAuth 2.0 authorization code flow:
response_type: "code"for server-side flowscope: "openid email profile"for basic user informationprompt: "select_account"for better UXstateparameter now included for CSRF protection
28-30: Redirect implementation is correct.The use of
NextResponse.redirectwith Google's OAuth authorization endpoint is appropriate. The query string is properly constructed from the URLSearchParams.
16-17: OAuth state implementation is secure and complete.The helper functions implement all necessary security measures:
generateOAuthState()usescrypto.randomBytes(32)for cryptographically secure random generation, andsetOAuthStateCookie()sets all required cookie options (httpOnly: true,sameSite: "lax",secureconditionally in production, andmaxAgeof 10 minutes). The implementation also includescrypto.timingSafeEqual()for constant-time comparison to prevent timing attacks.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/drizzle/schema.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/drizzle/schema.ts (3)
frontend/db/schema/users.ts (1)
users(10-35)frontend/db/schema/categories.ts (2)
categories(12-19)categoryTranslations(21-33)frontend/db/schema/questions.ts (2)
questions(14-27)questionTranslations(29-42)
🔇 Additional comments (1)
frontend/drizzle/schema.ts (1)
1-355: Well-structured schema with comprehensive constraints and relationships.The schema refactoring addresses the previously identified forward reference issues and includes:
- Proper table ordering (users → orders, categories → quizzes → quizQuestions, etc.)
- Comprehensive foreign key constraints with appropriate cascade/restrict behavior
- Check constraints for data integrity (non-negative amounts, positive quantities, etc.)
- Indexes on frequently queried columns for performance
- Normalized translation tables with composite primary keys
- Type-safe enums for currency, payment status, and product badges
The addition of OAuth-related columns (
provider,providerId) in the users table properly supports the Google and GitHub authentication flows introduced in this PR.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/drizzle/schema.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/drizzle/schema.ts (3)
frontend/db/schema/users.ts (1)
users(10-35)frontend/db/schema/categories.ts (2)
categories(12-19)categoryTranslations(21-33)frontend/db/schema/questions.ts (2)
questions(14-27)questionTranslations(29-42)
🔇 Additional comments (15)
frontend/drizzle/schema.ts (15)
1-2: LGTM!The imports are comprehensive and include all necessary utilities for the schema definitions.
4-6: LGTM!The pgEnum definitions are well-structured and provide type-safe values for currency, payment status, and product badges.
8-23: OAuth schema correctly implemented with composite unique constraint.The
userstable now includes the required OAuth columns (provider,providerId) and the composite unique constraint on line 22 ensures that each provider-account pair is unique, which is essential for preventing duplicate OAuth account linking.This addresses the outstanding past review comment about the missing composite unique constraint.
25-53: LGTM!The orders table is properly structured with correct foreign keys, unique constraints, and check constraints. All previously identified issues have been addressed.
55-62: LGTM!The categories table is correctly positioned before tables that reference it, eliminating forward reference issues.
64-81: LGTM!The quizzes table has proper foreign key relationships and constraints. The table ordering ensures no forward reference issues.
83-97: LGTM!The quiz_questions table has correct foreign keys and appropriate indexes for query performance.
125-139: LGTM!The point_transactions table is properly structured with correct foreign key relationships and cascade deletion.
163-176: LGTM!The questions table has proper foreign key relationships and no forward reference issues.
178-190: LGTM!The quiz_answers table is properly structured with appropriate indexes and cascade deletion.
192-219: LGTM!The order_items table has comprehensive check constraints that enforce data integrity, including quantity validation and line total consistency.
221-242: LGTM!The quiz_attempts table has appropriate indexes for common query patterns and proper foreign key relationships.
244-268: LGTM!The quiz_attempt_answers table has well-designed foreign key relationships with appropriate cascade behaviors for different scenarios.
270-286: LGTM!The stripe_events table is well-structured for tracking payment events with proper deduplication via the unique index on eventId.
290-356: LGTM!The translation tables follow a consistent and correct pattern with composite primary keys on (entityId, locale) and appropriate cascade deletion. This structure properly supports internationalization.
| colors: text().array().default([""]).notNull(), | ||
| sizes: text().array().default([""]).notNull(), |
There was a problem hiding this comment.
Questionable default values for array columns.
The colors and sizes columns default to [""] (an array containing a single empty string) rather than an empty array []. This will create products with one empty color/size entry by default, which may not be the intended behavior.
🔎 Proposed fix
- colors: text().array().default([""]).notNull(),
- sizes: text().array().default([""]).notNull(),
+ colors: text().array().default([]).notNull(),
+ sizes: text().array().default([]).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.
| colors: text().array().default([""]).notNull(), | |
| sizes: text().array().default([""]).notNull(), | |
| colors: text().array().default([]).notNull(), | |
| sizes: text().array().default([]).notNull(), |
🤖 Prompt for AI Agents
In frontend/drizzle/schema.ts around lines 111 to 112, the colors and sizes
array columns use a default of [""] which inserts a single empty string entry;
change their default to an empty array ([]) so new products start with no
color/size entries (or remove the default if you prefer a NULL/undefined
default) and keep notNull if you want an empty array enforced.
| originalPriceMinor: integer("original_price_minor"), | ||
| }, (table) => [ | ||
| index("product_prices_currency_idx").using("btree", table.currency.asc().nullsLast().op("enum_ops")), | ||
| uniqueIndex("product_prices_product_currency_uq").using("btree", table.productId.asc().nullsLast().op("uuid_ops"), table.currency.asc().nullsLast().op("uuid_ops")), |
There was a problem hiding this comment.
Incorrect operator class for enum column in unique index.
Line 153 uses op("uuid_ops") for the currency column, but currency is an enum type (defined at line 144). This is inconsistent with line 152, which correctly uses op("enum_ops") for the same column. Using the wrong operator class may cause the index to fail during migration or behave incorrectly.
🔎 Proposed fix
- uniqueIndex("product_prices_product_currency_uq").using("btree", table.productId.asc().nullsLast().op("uuid_ops"), table.currency.asc().nullsLast().op("uuid_ops")),
+ uniqueIndex("product_prices_product_currency_uq").using("btree", table.productId.asc().nullsLast().op("uuid_ops"), table.currency.asc().nullsLast().op("enum_ops")),📝 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.
| uniqueIndex("product_prices_product_currency_uq").using("btree", table.productId.asc().nullsLast().op("uuid_ops"), table.currency.asc().nullsLast().op("uuid_ops")), | |
| uniqueIndex("product_prices_product_currency_uq").using("btree", table.productId.asc().nullsLast().op("uuid_ops"), table.currency.asc().nullsLast().op("enum_ops")), |
🤖 Prompt for AI Agents
In frontend/drizzle/schema.ts around line 153, the unique index incorrectly
applies op("uuid_ops") to the currency column (an enum) — change that operator
class to op("enum_ops") to match the enum type (same as used on line 152) so the
uniqueIndex uses op("enum_ops") for currency; update the index definition
accordingly to avoid migration/index failures.
There was a problem hiding this comment.
frontend/drizzle/schema.ts is not used by the app or by Drizzle in this setup. The canonical schemas live in frontend/db/schema/* (as configured in drizzle-config.ts), and that’s what migrations and runtime use.
I added comment in the file to avoid confusion in the future
//
// Canonical schemas live in frontend/db/schema/*
There was a problem hiding this comment.
@kryvosheyin, thank you for clarifying! I appreciate you adding the warning comment to the file—that will definitely help prevent confusion in the future. Since frontend/drizzle/schema.ts isn't actually used in the runtime or by Drizzle Kit (with the canonical schemas living in frontend/db/schema/*), my concern about the operator class doesn't apply here.
✏️ Learnings added
Learnt from: kryvosheyin
Repo: DevLoversTeam/devlovers.net PR: 88
File: frontend/drizzle/schema.ts:153-153
Timestamp: 2025-12-30T16:00:55.436Z
Learning: The file frontend/drizzle/schema.ts is a legacy file not used by Drizzle or the runtime. The canonical schemas live in frontend/db/schema/* as configured in drizzle-config.ts, and that's what migrations and runtime use.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
2 participants
…g multi dev environments
Description
This pull request introduces OAuth authentication with Google and GitHub, refactors the login and signup flows to support these providers, and updates the database schema and relations to track provider information for users. It also includes UI components for OAuth login and updates to the database migration and schema files.
OAuth Authentication Integration:
frontend/app/api/auth/google/route.ts,frontend/app/api/auth/google/callback/route.ts,frontend/app/api/auth/github/route.ts,frontend/app/api/auth/github/callback/route.ts). [1] [2] [3] [4]frontend/components/auth/OAuthButtons.tsx,frontend/components/auth/ProviderButton.tsx,frontend/components/auth/icons/GoogleIcon.tsx,frontend/components/auth/icons/GitHubIcon.tsx). [1] [2] [3] [4]Login and Signup Flow Updates:
frontend/app/[locale]/login/page.tsx,frontend/app/[locale]/signup/page.tsx). (frontend/app/[locale]/login/page.tsxR9, frontend/app/[locale]/login/page.tsxR86-R93, frontend/app/[locale]/signup/page.tsxR9, frontend/app/[locale]/signup/page.tsxR98-R105)Database Schema and Migration:
userstable to includeproviderandprovider_idcolumns for tracking OAuth provider information, with a migration to add these fields (frontend/db/schema/users.ts,frontend/drizzle/0008_dizzy_james_howlett.sql,frontend/drizzle/meta/_journal.json). [1] [2] [3]Database Relations Refactor:
frontend/drizzle/relations.ts). [1] [2] [3] [4]Database Changes (if applicable)
How Has This Been Tested?
Screenshots (if applicable)
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.