Fix five issues from admin users page code review
- admin.users: use useEffect to close inline edit on success so validation errors are not silently discarded when the save button fires onClick - root: add /admin to ONBOARDING_EXEMPT so admin users without a username are not redirect-looped away from admin pages - settings: skip username validation when the username field is empty so updating timezone alone does not break for accounts without a username - models/user: exclude soft-deleted users from findUsersPaginated - admin.users: remove unused hasMore from loader return; simplify header div https://claude.ai/code/session_01E4UUQqQedhFP2F5YcBRArs
This commit is contained in:
parent
f2daea6c39
commit
a45a8619b1
5 changed files with 45 additions and 27 deletions
|
|
@ -1,4 +1,4 @@
|
||||||
import { eq, inArray, and, or, ilike, sql } from "drizzle-orm";
|
import { eq, inArray, and, or, ilike, isNull, sql } from "drizzle-orm";
|
||||||
import { database } from "~/database/context";
|
import { database } from "~/database/context";
|
||||||
import * as schema from "~/database/schema";
|
import * as schema from "~/database/schema";
|
||||||
import { generateFlagConfig } from "~/lib/flag-generator";
|
import { generateFlagConfig } from "~/lib/flag-generator";
|
||||||
|
|
@ -113,17 +113,21 @@ export async function findUsersPaginated(options: {
|
||||||
search?: string;
|
search?: string;
|
||||||
}): Promise<{ users: User[]; total: number; hasMore: boolean }> {
|
}): Promise<{ users: User[]; total: number; hasMore: boolean }> {
|
||||||
const db = database();
|
const db = database();
|
||||||
const searchFilter = options.search
|
const notDeleted = isNull(schema.users.deletedAt);
|
||||||
? or(
|
const filter = options.search
|
||||||
ilike(schema.users.email, `%${options.search}%`),
|
? and(
|
||||||
ilike(schema.users.username, `%${options.search}%`),
|
notDeleted,
|
||||||
ilike(schema.users.displayName, `%${options.search}%`),
|
or(
|
||||||
|
ilike(schema.users.email, `%${options.search}%`),
|
||||||
|
ilike(schema.users.username, `%${options.search}%`),
|
||||||
|
ilike(schema.users.displayName, `%${options.search}%`),
|
||||||
|
),
|
||||||
)
|
)
|
||||||
: undefined;
|
: notDeleted;
|
||||||
|
|
||||||
const [users, [{ count }]] = await Promise.all([
|
const [users, [{ count }]] = await Promise.all([
|
||||||
db.query.users.findMany({
|
db.query.users.findMany({
|
||||||
where: searchFilter,
|
where: filter,
|
||||||
orderBy: (users, { asc }) => [asc(users.displayName)],
|
orderBy: (users, { asc }) => [asc(users.displayName)],
|
||||||
limit: options.limit,
|
limit: options.limit,
|
||||||
offset: options.offset,
|
offset: options.offset,
|
||||||
|
|
@ -131,7 +135,7 @@ export async function findUsersPaginated(options: {
|
||||||
db
|
db
|
||||||
.select({ count: sql<number>`count(*)::int` })
|
.select({ count: sql<number>`count(*)::int` })
|
||||||
.from(schema.users)
|
.from(schema.users)
|
||||||
.where(searchFilter),
|
.where(filter),
|
||||||
]);
|
]);
|
||||||
return { users, total: count, hasMore: options.offset + users.length < count };
|
return { users, total: count, hasMore: options.offset + users.length < count };
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -23,7 +23,7 @@ import logoUrl from "../public/logo.svg?url";
|
||||||
|
|
||||||
// Paths that should never trigger the onboarding redirect.
|
// Paths that should never trigger the onboarding redirect.
|
||||||
// Trailing-slash entries are matched as prefixes; others require exact match or sub-path.
|
// 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"];
|
const ONBOARDING_EXEMPT = ["/onboarding", "/api/", "/admin", "/login", "/register", "/check-email", "/forgot-password", "/reset-password"];
|
||||||
|
|
||||||
export function isOnboardingExempt(pathname: string): boolean {
|
export function isOnboardingExempt(pathname: string): boolean {
|
||||||
return ONBOARDING_EXEMPT.some((p) =>
|
return ONBOARDING_EXEMPT.some((p) =>
|
||||||
|
|
|
||||||
|
|
@ -33,6 +33,12 @@ describe("isOnboardingExempt", () => {
|
||||||
expect(isOnboardingExempt("/loginpage")).toBe(false);
|
expect(isOnboardingExempt("/loginpage")).toBe(false);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("exempts /admin and all sub-paths", () => {
|
||||||
|
expect(isOnboardingExempt("/admin")).toBe(true);
|
||||||
|
expect(isOnboardingExempt("/admin/users")).toBe(true);
|
||||||
|
expect(isOnboardingExempt("/admin/sports-seasons/123")).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
it("does not exempt normal app routes", () => {
|
it("does not exempt normal app routes", () => {
|
||||||
expect(isOnboardingExempt("/")).toBe(false);
|
expect(isOnboardingExempt("/")).toBe(false);
|
||||||
expect(isOnboardingExempt("/leagues/abc")).toBe(false);
|
expect(isOnboardingExempt("/leagues/abc")).toBe(false);
|
||||||
|
|
|
||||||
|
|
@ -1,4 +1,4 @@
|
||||||
import { useState } from "react";
|
import { useState, useEffect } from "react";
|
||||||
import { Form, redirect, useSearchParams } from "react-router";
|
import { Form, redirect, useSearchParams } from "react-router";
|
||||||
import type { Route } from "./+types/admin.users";
|
import type { Route } from "./+types/admin.users";
|
||||||
|
|
||||||
|
|
@ -48,14 +48,14 @@ export async function loader(args: Route.LoaderArgs) {
|
||||||
const page = Math.max(0, parseInt(searchParams.get("page") ?? "0", 10) || 0);
|
const page = Math.max(0, parseInt(searchParams.get("page") ?? "0", 10) || 0);
|
||||||
const search = searchParams.get("search") ?? "";
|
const search = searchParams.get("search") ?? "";
|
||||||
|
|
||||||
const { users, total, hasMore } = await findUsersPaginated({
|
const { users, total } = await findUsersPaginated({
|
||||||
limit: PAGE_SIZE,
|
limit: PAGE_SIZE,
|
||||||
offset: page * PAGE_SIZE,
|
offset: page * PAGE_SIZE,
|
||||||
search: search || undefined,
|
search: search || undefined,
|
||||||
});
|
});
|
||||||
|
|
||||||
const totalPages = Math.ceil(total / PAGE_SIZE);
|
const totalPages = Math.ceil(total / PAGE_SIZE);
|
||||||
return { users, page, totalPages, total, hasMore, search };
|
return { users, page, totalPages, total, search };
|
||||||
}
|
}
|
||||||
|
|
||||||
export async function action(args: Route.ActionArgs) {
|
export async function action(args: Route.ActionArgs) {
|
||||||
|
|
@ -107,6 +107,13 @@ export default function AdminUsers({ loaderData, actionData }: Route.ComponentPr
|
||||||
const [editingId, setEditingId] = useState<string | null>(null);
|
const [editingId, setEditingId] = useState<string | null>(null);
|
||||||
const [editValue, setEditValue] = useState("");
|
const [editValue, setEditValue] = useState("");
|
||||||
|
|
||||||
|
useEffect(() => {
|
||||||
|
if (actionData && "success" in actionData && actionData.success) {
|
||||||
|
setEditingId(null);
|
||||||
|
setEditValue("");
|
||||||
|
}
|
||||||
|
}, [actionData]);
|
||||||
|
|
||||||
function startEdit(userId: string, currentUsername: string | null) {
|
function startEdit(userId: string, currentUsername: string | null) {
|
||||||
setEditingId(userId);
|
setEditingId(userId);
|
||||||
setEditValue(currentUsername ?? "");
|
setEditValue(currentUsername ?? "");
|
||||||
|
|
@ -125,11 +132,9 @@ export default function AdminUsers({ loaderData, actionData }: Route.ComponentPr
|
||||||
|
|
||||||
return (
|
return (
|
||||||
<div className="p-8">
|
<div className="p-8">
|
||||||
<div className="mb-6 flex items-center justify-between">
|
<div className="mb-6">
|
||||||
<div>
|
<h1 className="text-2xl font-bold">Users</h1>
|
||||||
<h1 className="text-2xl font-bold">Users</h1>
|
<p className="text-muted-foreground text-sm mt-1">{total} total users</p>
|
||||||
<p className="text-muted-foreground text-sm mt-1">{total} total users</p>
|
|
||||||
</div>
|
|
||||||
</div>
|
</div>
|
||||||
|
|
||||||
<Card>
|
<Card>
|
||||||
|
|
@ -199,7 +204,6 @@ export default function AdminUsers({ loaderData, actionData }: Route.ComponentPr
|
||||||
variant="ghost"
|
variant="ghost"
|
||||||
className="h-7 w-7"
|
className="h-7 w-7"
|
||||||
title="Save"
|
title="Save"
|
||||||
onClick={() => setEditingId(null)}
|
|
||||||
>
|
>
|
||||||
<Check className="h-4 w-4" />
|
<Check className="h-4 w-4" />
|
||||||
</Button>
|
</Button>
|
||||||
|
|
|
||||||
|
|
@ -123,17 +123,21 @@ export async function action(args: Route.ActionArgs): Promise<ActionData | Respo
|
||||||
const timezone = formData.get("timezone") as string | null;
|
const timezone = formData.get("timezone") as string | null;
|
||||||
const timezoneValue = timezone && VALID_TIMEZONES.has(timezone) ? timezone : undefined;
|
const timezoneValue = timezone && VALID_TIMEZONES.has(timezone) ? timezone : undefined;
|
||||||
|
|
||||||
if (!USERNAME_RE.test(username)) {
|
if (username !== "") {
|
||||||
return { intent: "update-profile", error: "Username must be 3–30 characters and contain only letters, numbers, underscores, or hyphens." };
|
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);
|
const existing = await findUserByUsername(username);
|
||||||
if (existing && existing.id !== session.user.id) {
|
if (existing && existing.id !== session.user.id) {
|
||||||
return { intent: "update-profile", error: "That username is already taken." };
|
return { intent: "update-profile", error: "That username is already taken." };
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
try {
|
try {
|
||||||
await updateUser(session.user.id, { username, timezone: timezoneValue });
|
await updateUser(session.user.id, {
|
||||||
|
...(username !== "" ? { username } : {}),
|
||||||
|
timezone: timezoneValue,
|
||||||
|
});
|
||||||
} catch {
|
} catch {
|
||||||
return { intent: "update-profile", error: "That username is already taken." };
|
return { intent: "update-profile", error: "That username is already taken." };
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue