Address all code review issues on username onboarding

- Replace case-sensitive unique constraint with functional unique index on
  lower(username) so DB-level uniqueness is case-insensitive (fix #1)
- Share USERNAME_RE from models/user.ts; add same format + uniqueness
  validation to the settings update-profile action (fix #2)
- Wrap updateUser calls in try/catch to handle race-condition DB errors
  gracefully in both onboarding and settings (fix #3)
- Add unit tests for suggestUsername, safeRedirectTo, USERNAME_RE, and
  isOnboardingExempt (fix #4)
- Guard suggestUsername against returning strings shorter than 3 chars (fix #5)
- Preserve the user's intended destination via redirectTo query param
  through the onboarding flow (fix #6)
- Treat empty username in settings as a validation error instead of a
  silent no-op (fix #7)
- Use exact/sub-path matching in isOnboardingExempt to prevent
  false-positive prefix matches like /loginpage (fix #8)

https://claude.ai/code/session_01UinBMAy9d6dQzzbcUAdq8J

app/models/user.ts

@@ -6,6 +6,8 @@ import { generateFlagConfig } from "~/lib/flag-generator";
 export type User = typeof schema.users.$inferSelect;
 export type NewUser = typeof schema.users.$inferInsert;
 
+export const USERNAME_RE = /^[a-zA-Z0-9_-]{3,30}$/;
+
 export function getUserDisplayName(
   user: Pick<User, "username" | "displayName">
 ): string | null {

app/root.tsx

@@ -21,8 +21,18 @@ import { findUserById } from "~/models/user";
 import { AlertCircle, FileQuestion, Lock, ShieldOff, ServerCrash } from "lucide-react";
 import logoUrl from "../public/logo.svg?url";
 
+// Paths that should never trigger the onboarding redirect.
+// Trailing-slash entries are matched as prefixes; others require exact match or sub-path.
 const ONBOARDING_EXEMPT = ["/onboarding", "/api/", "/login", "/register", "/check-email", "/forgot-password", "/reset-password"];
 
+export function isOnboardingExempt(pathname: string): boolean {
+  return ONBOARDING_EXEMPT.some((p) =>
+    p.endsWith("/")
+      ? pathname.startsWith(p)
+      : pathname === p || pathname.startsWith(p + "/")
+  );
+}
+
 export async function loader({ request }: Route.LoaderArgs) {
   const session = await auth.api.getSession({ headers: request.headers });
   const currentUser = session ? await findUserById(session.user.id) : null;
@@ -30,8 +40,10 @@ export async function loader({ request }: Route.LoaderArgs) {
 
   if (currentUser && !currentUser.username) {
     const { pathname } = new URL(request.url);
-    const exempt = ONBOARDING_EXEMPT.some((p) => pathname === p || pathname.startsWith(p));
-    if (!exempt) return redirect("/onboarding");
+    if (!isOnboardingExempt(pathname)) {
+      const redirectTo = pathname === "/" ? "" : `?redirectTo=${encodeURIComponent(pathname)}`;
+      return redirect(`/onboarding${redirectTo}`);
+    }
   }
 
   return { isAdmin, currentUser };

app/routes/__tests__/onboarding.test.ts

@@ -0,0 +1,98 @@
+import { describe, it, expect } from "vitest";
+import { suggestUsername, safeRedirectTo } from "../onboarding";
+import { USERNAME_RE } from "~/models/user";
+
+describe("suggestUsername", () => {
+  it("returns empty string for null", () => {
+    expect(suggestUsername(null)).toBe("");
+  });
+
+  it("returns empty string for empty string", () => {
+    expect(suggestUsername("")).toBe("");
+  });
+
+  it("lowercases and strips spaces", () => {
+    expect(suggestUsername("John Smith")).toBe("johnsmith");
+  });
+
+  it("strips special characters", () => {
+    expect(suggestUsername("Jane O'Brien!")).toBe("janeobrien");
+  });
+
+  it("preserves underscores and hyphens", () => {
+    expect(suggestUsername("cool_user-name")).toBe("cool_user-name");
+  });
+
+  it("slices to 20 characters", () => {
+    expect(suggestUsername("averylongdisplaynamethatexceedstwenty")).toBe("averylongdisplayname");
+    expect(suggestUsername("averylongdisplaynamethatexceedstwenty").length).toBeLessThanOrEqual(20);
+  });
+
+  it("returns empty string when cleaned result is shorter than 3 characters", () => {
+    expect(suggestUsername("Jo")).toBe("");
+    expect(suggestUsername("AB")).toBe("");
+  });
+
+  it("returns the suggestion when cleaned result is exactly 3 characters", () => {
+    expect(suggestUsername("abc")).toBe("abc");
+  });
+
+  it("returns empty string for non-ASCII display names that strip to nothing", () => {
+    expect(suggestUsername("小林")).toBe("");
+  });
+});
+
+describe("safeRedirectTo", () => {
+  it("returns / for null", () => {
+    expect(safeRedirectTo(null)).toBe("/");
+  });
+
+  it("returns / for empty string", () => {
+    expect(safeRedirectTo("")).toBe("/");
+  });
+
+  it("returns / for bare slash", () => {
+    expect(safeRedirectTo("/")).toBe("/");
+  });
+
+  it("allows a root-relative path", () => {
+    expect(safeRedirectTo("/leagues/123/draft/456")).toBe("/leagues/123/draft/456");
+  });
+
+  it("rejects an absolute URL", () => {
+    expect(safeRedirectTo("https://evil.com")).toBe("/");
+  });
+
+  it("rejects a protocol-relative URL", () => {
+    expect(safeRedirectTo("//evil.com")).toBe("/");
+  });
+});
+
+describe("USERNAME_RE", () => {
+  it("allows 3-character usernames", () => {
+    expect(USERNAME_RE.test("abc")).toBe(true);
+  });
+
+  it("allows 30-character usernames", () => {
+    expect(USERNAME_RE.test("a".repeat(30))).toBe(true);
+  });
+
+  it("allows letters, numbers, underscores, and hyphens", () => {
+    expect(USERNAME_RE.test("Cool_User-123")).toBe(true);
+  });
+
+  it("rejects usernames shorter than 3 characters", () => {
+    expect(USERNAME_RE.test("ab")).toBe(false);
+    expect(USERNAME_RE.test("")).toBe(false);
+  });
+
+  it("rejects usernames longer than 30 characters", () => {
+    expect(USERNAME_RE.test("a".repeat(31))).toBe(false);
+  });
+
+  it("rejects special characters", () => {
+    expect(USERNAME_RE.test("user!")).toBe(false);
+    expect(USERNAME_RE.test("user name")).toBe(false);
+    expect(USERNAME_RE.test("user@host")).toBe(false);
+  });
+});

app/routes/__tests__/root.onboarding-exempt.test.ts

@@ -0,0 +1,41 @@
+import { describe, it, expect } from "vitest";
+import { isOnboardingExempt } from "../../root";
+
+describe("isOnboardingExempt", () => {
+  it("exempts /onboarding exactly", () => {
+    expect(isOnboardingExempt("/onboarding")).toBe(true);
+  });
+
+  it("exempts /onboarding sub-paths", () => {
+    expect(isOnboardingExempt("/onboarding/step2")).toBe(true);
+  });
+
+  it("exempts /api/ prefix paths", () => {
+    expect(isOnboardingExempt("/api/auth/callback")).toBe(true);
+    expect(isOnboardingExempt("/api/draft/start")).toBe(true);
+  });
+
+  it("does not exempt paths that merely start with /api without trailing slash", () => {
+    // /api/ has a trailing slash so startsWith("/api/") is used — /apiary would not match
+    expect(isOnboardingExempt("/apiary")).toBe(false);
+  });
+
+  it("exempts auth pages exactly", () => {
+    expect(isOnboardingExempt("/login")).toBe(true);
+    expect(isOnboardingExempt("/register")).toBe(true);
+    expect(isOnboardingExempt("/check-email")).toBe(true);
+    expect(isOnboardingExempt("/forgot-password")).toBe(true);
+    expect(isOnboardingExempt("/reset-password")).toBe(true);
+  });
+
+  it("does not exempt a path that merely starts with an exempt path without slash", () => {
+    expect(isOnboardingExempt("/registered-user")).toBe(false);
+    expect(isOnboardingExempt("/loginpage")).toBe(false);
+  });
+
+  it("does not exempt normal app routes", () => {
+    expect(isOnboardingExempt("/")).toBe(false);
+    expect(isOnboardingExempt("/leagues/abc")).toBe(false);
+    expect(isOnboardingExempt("/settings")).toBe(false);
+  });
+});

app/routes/onboarding.tsx

@@ -1,6 +1,6 @@
 import { redirect, Form } from "react-router";
 import { auth } from "~/lib/auth.server";
-import { findUserById, findUserByUsername, updateUser } from "~/models/user";
+import { findUserById, findUserByUsername, updateUser, USERNAME_RE } from "~/models/user";
 import type { Route } from "./+types/onboarding";
 import { Button } from "~/components/ui/button";
 import { Input } from "~/components/ui/input";
@@ -11,15 +11,19 @@ export function meta(): Route.MetaDescriptors {
   return [{ title: "Choose a Username - Brackt" }];
 }
 
-function suggestUsername(displayName: string | null): string {
+export function suggestUsername(displayName: string | null): string {
   if (!displayName) return "";
-  return displayName
+  const cleaned = displayName
     .toLowerCase()
     .replace(/[^a-z0-9_-]/g, "")
     .slice(0, 20);
+  return cleaned.length >= 3 ? cleaned : "";
 }
 
-const USERNAME_RE = /^[a-zA-Z0-9_-]{3,30}$/;
+export function safeRedirectTo(value: string | null): string {
+  if (!value) return "/";
+  return value.startsWith("/") && !value.startsWith("//") ? value : "/";
+}
 
 export async function loader({ request }: Route.LoaderArgs) {
   const session = await auth.api.getSession({ headers: request.headers });
@@ -27,7 +31,8 @@ export async function loader({ request }: Route.LoaderArgs) {
   const user = await findUserById(session.user.id);
   if (!user) return redirect("/login");
   if (user.username) return redirect("/");
-  return { suggestion: suggestUsername(user.displayName) };
+  const redirectTo = safeRedirectTo(new URL(request.url).searchParams.get("redirectTo"));
+  return { suggestion: suggestUsername(user.displayName), redirectTo };
 }
 
 export async function action({ request }: Route.ActionArgs) {
@@ -36,23 +41,32 @@ export async function action({ request }: Route.ActionArgs) {
 
   const formData = await request.formData();
   const username = (formData.get("username") as string | null)?.trim() ?? "";
+  const redirectTo = safeRedirectTo(formData.get("redirectTo") as string | null);
 
   if (!USERNAME_RE.test(username)) {
     return {
       error: "Username must be 3–30 characters and contain only letters, numbers, underscores, or hyphens.",
+      redirectTo,
     };
   }
 
   const existing = await findUserByUsername(username);
   if (existing && existing.id !== session.user.id) {
-    return { error: "That username is already taken." };
+    return { error: "That username is already taken.", redirectTo };
   }
 
-  await updateUser(session.user.id, { username });
-  return redirect("/");
+  try {
+    await updateUser(session.user.id, { username });
+  } catch {
+    return { error: "That username is already taken.", redirectTo };
+  }
+
+  return redirect(redirectTo);
 }
 
 export default function OnboardingPage({ loaderData, actionData }: Route.ComponentProps) {
+  const redirectTo = actionData && "redirectTo" in actionData ? actionData.redirectTo : loaderData.redirectTo;
+
   return (
     <div className="min-h-screen flex items-center justify-center p-4">
       <Card className="w-full max-w-md">
@@ -64,6 +78,7 @@ export default function OnboardingPage({ loaderData, actionData }: Route.Compone
         </CardHeader>
         <CardContent>
           <Form method="post" className="space-y-4">
+            <input type="hidden" name="redirectTo" value={redirectTo} />
             <div className="space-y-2">
               <Label htmlFor="username">Username</Label>
               <Input

app/routes/settings.tsx

@@ -2,7 +2,7 @@ import { redirect } from "react-router";
 import { useState } from "react";
 import { Bell, Key, Lock, Shield, User } from "lucide-react";
 import { auth } from "~/lib/auth.server";
-import { findUserById, isUserInActiveDraft, updateUser, anonymizeUserAccount } from "~/models/user";
+import { findUserById, findUserByUsername, isUserInActiveDraft, updateUser, anonymizeUserAccount, USERNAME_RE } from "~/models/user";
 import { findLinkedAccountsByUserId } from "~/models/account";
 import { findTeamsByOwnerId, removeTeamOwner } from "~/models/team";
 import { removeAllCommissionersByUserId } from "~/models/commissioner";
@@ -119,13 +119,24 @@ export async function action(args: Route.ActionArgs): Promise<ActionData | Respo
   }
 
   if (intent === "update-profile") {
-    const username = formData.get("username") as string | null;
+    const username = (formData.get("username") as string | null)?.trim() ?? "";
     const timezone = formData.get("timezone") as string | null;
     const timezoneValue = timezone && VALID_TIMEZONES.has(timezone) ? timezone : undefined;
-    await updateUser(session.user.id, {
-      username: username || undefined,
-      timezone: timezoneValue,
-    });
+
+    if (!USERNAME_RE.test(username)) {
+      return { intent: "update-profile", error: "Username must be 3–30 characters and contain only letters, numbers, underscores, or hyphens." };
+    }
+
+    const existing = await findUserByUsername(username);
+    if (existing && existing.id !== session.user.id) {
+      return { intent: "update-profile", error: "That username is already taken." };
+    }
+
+    try {
+      await updateUser(session.user.id, { username, timezone: timezoneValue });
+    } catch {
+      return { intent: "update-profile", error: "That username is already taken." };
+    }
     return { intent: "update-profile", success: true };
   }
 

database/schema.ts

@@ -6,7 +6,7 @@ export const users = pgTable("users", {
   clerkId: varchar("clerk_id", { length: 255 }).unique(), // legacy: only populated for users migrated from Clerk; new users are null
   email: varchar("email", { length: 255 }).notNull(),
   emailVerified: boolean("email_verified").notNull().default(false),
-  username: varchar("username", { length: 255 }).unique(),
+  username: varchar("username", { length: 255 }),
   displayName: varchar("display_name", { length: 255 }), // written by BetterAuth on OAuth login ("name" field); not user-editable
   imageUrl: varchar("image_url", { length: 512 }), // mapped as BetterAuth's "image" field
   flagConfig: jsonb("flag_config").$type<{ pattern: string; colors: string[] }>(),
@@ -19,7 +19,9 @@ export const users = pgTable("users", {
   deletedAt: timestamp("deleted_at"),
   createdAt: timestamp("created_at").defaultNow().notNull(),
   updatedAt: timestamp("updated_at").defaultNow().notNull(),
-});
+}, (t) => ({
+  usernameUnique: uniqueIndex("users_username_lower_unique").on(sql`lower(${t.username})`),
+}));
 
 // BetterAuth session/account/verification tables
 export const sessions = pgTable("sessions", {

drizzle/0107_outgoing_mikhail_rasputin.sql

@@ -0,0 +1,2 @@
+ALTER TABLE "users" DROP CONSTRAINT "users_username_unique";--> statement-breakpoint
+CREATE UNIQUE INDEX IF NOT EXISTS "users_username_lower_unique" ON "users" USING btree (lower("username"));

drizzle/meta/_journal.json

@@ -750,6 +750,13 @@
       "when": 1778867381171,
       "tag": "0106_chilly_chimera",
       "breakpoints": true
+    },
+    {
+      "idx": 107,
+      "version": "7",
+      "when": 1778868190284,
+      "tag": "0107_outgoing_mikhail_rasputin",
+      "breakpoints": true
     }
   ]
 }