diff --git a/.claude/skills/data-fetching-perf-audit/SKILL.md b/.claude/skills/data-fetching-perf-audit/SKILL.md new file mode 100644 index 000000000..65ee78a32 --- /dev/null +++ b/.claude/skills/data-fetching-perf-audit/SKILL.md @@ -0,0 +1,101 @@ +--- +name: data-fetching-perf-audit +description: >- + Audit and optimize data fetching across TanStack Start route loaders, server + functions (createServerFn), and Drizzle/PlanetScale queries in wodsmith-start. + Use when a page or route group feels slow, when asked to do a "performance + analysis" / "perf audit" of data fetching, or to investigate loader + waterfalls, N+1 query patterns, sequential awaits of independent queries, + fake parallelism, over-fetching, or missing indexes. Triggers: slow compete + pages, slow organizer dashboard, "why is this loader slow", "optimize the + queries on X", "eliminate N+1", "reduce round trips", "batch these fetches". + Models the methodology proven in PRs #502 and #512. +--- + +# Data Fetching Performance Audit + +Find and fix data-fetching slowness in `wodsmith-start` — the layer between a +route loader and the database. The wins are almost never "make the query +faster"; they are **fewer round trips** and **real parallelism**. This skill +encodes the audit methodology and the fix catalog proven in PR #502 (batch N+1 +across organizer routes) and PR #512 (eliminate waterfalls on public compete +pages). + +## Why this stack is different + +Three facts drive every decision here. Internalize them before touching code: + +1. **A server fn called from a loader is an HTTP round trip.** On client-side + navigation, each `createServerFn` call the loader awaits is a separate + request. Three sequential `await fooFn()` calls = three serial network waves + before the page renders. Collapsing waves matters more than query speed. +2. **`getDb()` wraps a single mysql2 connection, and mysql2 serializes commands + per connection.** `Promise.all([db.select()…, db.select()…])` on **one** `db` + runs sequentially on the wire — fake parallelism. Real parallelism requires + each branch to use its **own** `getDb()`. +3. **The push-based DB workflow has no migrations dir.** Index changes are + schema edits applied with `pnpm db:push`, not generated migrations. + +## The audit workflow + +Work top-down: a page is slow because of how its loader composes server fns, +which is slow because of how each server fn composes queries. + +1. **Scope.** Pick the route or route group (e.g. everything under + `/compete/organizer/$competitionId/`, or the public `/compete/$slug` tree). + Enumerate every route file in scope — don't sample. +2. **Trace each loader → server fn → query chain.** For each route's `loader`, + list the server fns it awaits and whether they run sequentially or in + `Promise.all`. Open each server fn and list its internal DB queries and + whether *those* are sequential. Note anything called once per item in a list. +3. **Classify** every finding against the [anti-pattern catalog](#anti-pattern-catalog). + Read `references/anti-patterns.md` for the detection signal, the fix, and + real before/after code for each. +4. **Fix**, smallest blast radius first. Preserve loader data shapes so + components and their props are untouched (verify with `pnpm type-check`). + Keep single-item server fns alongside new batched ones — targeted refreshes + still need them. +5. **Watch the two traps** in `references/stack-gotchas.md` — they will silently + undo your work (fake parallelism) or break the client build + (`cloudflare:workers` leaking into the bundle). +6. **Verify** with the checklist below. + +For a thorough audit of a large route group, fan out: one agent per +page/loader tracing its full chain, then an adversarial pass that re-checks +each proposed fix for regressions (deferred-data drops, serialization, +behavior changes). PR #512's analysis was a 10-agent trace reviewed by a +20-agent adversarial pass. Scale the agent count to the scope. + +## Anti-pattern catalog + +Quick reference — full detection + fix + code in `references/anti-patterns.md`. + +| Anti-pattern | Smell | Fix | +|---|---|---| +| **Loader waterfall** | Loader `await`s fn A, then B with A's result, then C — each a round trip on client nav | Consolidate into one server fn (or ≤2 parallel) that composes the work in-process | +| **N+1 server-fn call** | `items.map(i => someFn({ data: i.id }))` / a per-item call inside a loop | One batched fn taking `ids[]`, one `inArray` query, returns a `Record` map | +| **Sequential independent awaits** | `const a = await x(); const b = await y()` where `y` doesn't use `a` | `const [a, b] = await Promise.all([x(), y()])` | +| **Fake parallelism** | `Promise.all` over one shared `getDb()` instance | One `getDb()` per parallel branch (mysql2 serializes per connection) | +| **Over-fetching** | `select()` whole rows / full `users` (ships `passwordHash`, profile JSON); filtering in JS | Project only consumed columns; push filters into SQL (`where`) | +| **Scan-to-check** | Fetch all rows then `.find`/`.some` in JS to test existence | Single `EXISTS`-scoped / `inArray` query | +| **Missing hot-path index** | `inArray` / `eq` on a junction table (`workout_movements`, `workout_tags`) doing full scans | Add `index(...)` in the schema, apply with `pnpm db:push` | +| **Dead query** | A fetched value that nothing in the loader/component consumes | Delete it | +| **Per-item on critical path** | Non-essential data blocking first render | Defer it off the loader (chain un-awaited, hydrate client-side) | + +## Security while optimizing + +Tracing every fetch surfaces authz holes — fix them in the same pass (PR #512 +found two). Watch for: client-supplied `userId`/ids trusted without a session +check (IDOR — derive identity from `getSessionFromCookie()`), and unpublished/ +draft data (e.g. heats with athlete names) returned to anonymous viewers. See +`references/stack-gotchas.md`. + +## Verification checklist + +- [ ] `pnpm type-check` — loader data shapes unchanged (component contracts intact) +- [ ] `pnpm build` — confirms no `@/db` / `cloudflare:workers` leaked into the client bundle +- [ ] `pnpm test` — at parity with `main` (run suspected pre-existing failures on a clean `main` to confirm) +- [ ] `pnpm check` (Biome) clean on changed files +- [ ] If indexes changed: `pnpm db:push` applied (no migration files) +- [ ] `lat check` passes; update `lat.md/architecture.md` to document new consolidated/batched fns +- [ ] Each parallel branch verified to use its own `getDb()` (no fake parallelism) diff --git a/.claude/skills/data-fetching-perf-audit/references/anti-patterns.md b/.claude/skills/data-fetching-perf-audit/references/anti-patterns.md new file mode 100644 index 000000000..3bcdb9266 --- /dev/null +++ b/.claude/skills/data-fetching-perf-audit/references/anti-patterns.md @@ -0,0 +1,273 @@ +# Anti-pattern catalog: detection + fix + code + +Each section: how to spot it, how to fix it, and real before/after from +`wodsmith-start` (PRs #502, #512). Code is illustrative — match the existing +file's imports and style when applying. + +## Contents + +1. [Loader waterfall → consolidated server fn](#1-loader-waterfall) +2. [N+1 server-fn call → batched map fn](#2-n1-server-fn-call) +3. [Sequential independent awaits → Promise.all](#3-sequential-independent-awaits) +4. [Fake parallelism → one getDb() per branch](#4-fake-parallelism) +5. [Over-fetching → project + filter in SQL](#5-over-fetching) +6. [Scan-to-check → EXISTS-scoped query](#6-scan-to-check) +7. [Missing hot-path index](#7-missing-hot-path-index) +8. [Dead query → delete](#8-dead-query) +9. [Per-item on critical path → defer](#9-per-item-on-critical-path) + +--- + +## 1. Loader waterfall + +**Detect.** A route `loader` awaits one server fn, then awaits a second using +the first's result, then a third. Each await is an HTTP round trip on +client-side navigation. A page that issues 3–6 sequential waves pays 3–6 serial +network latencies before render. + +**Fix.** Collapse the waves into **one consolidated server fn** that composes +the existing fns/queries in-process (server-to-server, no extra round trips), or +into **≤2 parallel** fns when one is public and one is session-specific. Both +take the same input (e.g. `slug`) so neither waits on the other — one extra +cheap indexed lookup beats a serial HTTP wave. Keep the loader's returned data +shape identical so components don't change. + +Reference fns: `getPublicCompetitionPageDataFn` + `getViewerCompetitionContextFn` +(`src/server-fns/competition-page-fns.ts`), `getPublicWorkoutsPageDataFn` +(`competition-workouts-page-fns.ts`), `getPublicEventPageDataFn` +(`competition-event-page-fns.ts`). + +```ts +// BEFORE — loader: 3 sequential round trips +const comp = await getCompetitionFn({ data: { slug } }) +const divisions = await getDivisionsFn({ data: { competitionId: comp.id } }) +const sponsors = await getSponsorsFn({ data: { competitionId: comp.id } }) + +// AFTER — one consolidated fn; public branches fan out in-process, each on its +// own connection. Anonymous viewers get empty stubs with zero DB work. +export const getPublicCompetitionPageDataFn = createServerFn({ method: "GET" }) + .inputValidator((d: unknown) => competitionPageInputSchema.parse(d)) + .handler(async ({ data }) => { + const { competition } = await getCompetitionBySlugFn({ data: { slug: data.slug } }) + if (!competition) return { competition: null, divisions: [], sponsors: emptySponsors } + const [divisionsResult, sponsors, judgesScheduleResult] = await Promise.all([ + getPublicCompetitionDivisionsForCompetition({ competition }), + getCompetitionSponsorsFn({ data: { competitionId: competition.id } }), + hasJudgesScheduleFn({ data: { competitionId: competition.id } }), + ]) + return { competition, divisions: divisionsResult.divisions, sponsors, /* … */ } + }) +``` + +Split public vs. session data when one half needs no session: the public fn +does zero session work and is cacheable; the viewer fn reads the session **once** +and short-circuits to empty stubs for anonymous visitors. + +--- + +## 2. N+1 server-fn call + +**Detect.** A server fn (or multi-query fn) invoked once per item in a list: +`volunteers.map(v => canInputScoresFn({ data: v.userId }))`, or a parent fetching +child data once per child. Cost scales with list length — the classic N+1. + +**Fix.** Add a **batched fn** that takes an array of ids, issues **one +`inArray` query**, and returns a `Record` map (pre-filled with defaults so +every requested id is present). Callers do a map lookup instead of a call. +Exploit shared scope: if children share a parent's competition-scoped data, fetch +it once for all of them rather than per child. + +Reference fns: `getScoreAccessMapFn` (`src/server-fns/volunteer-fns.ts`), +`getJudgeSchedulingDataForEventsFn` (`judge-scheduling-fns.ts`), +`getEventScoreEntryDataWithHeatsBatchFn` (`competition-score-fns.ts`), +`getHeatsByWorkoutIdsInternal` (`competition-heats-fns.ts`). + +```ts +// BEFORE — N queries, one per volunteer +const access = {} +for (const v of volunteers) { + access[v.userId] = await canInputScoresFn({ data: { userId: v.userId, competitionTeamId } }) +} + +// AFTER — one inArray query, returns userId -> boolean map +export const getScoreAccessMapFn = createServerFn({ method: "GET" }) + .inputValidator((d: unknown) => + z.object({ userIds: z.array(z.string()), competitionTeamId: competitionTeamIdSchema }).parse(d), + ) + .handler(async ({ data }): Promise> => { + const accessMap: Record = {} + for (const userId of data.userIds) accessMap[userId] = false // defaults + if (data.userIds.length === 0) return accessMap + const db = getDb() + const entitlements = await db + .select({ userId: entitlementTable.userId }) + .from(entitlementTable) + .where(and( + inArray(entitlementTable.userId, data.userIds), + eq(entitlementTable.teamId, data.competitionTeamId), + eq(entitlementTable.entitlementTypeId, SCORE_INPUT_TYPE_ID), + isNull(entitlementTable.deletedAt), + )) + for (const e of entitlements) if (e.userId) accessMap[e.userId] = true + return accessMap + }) +``` + +When batching collapses N calls to 1, the per-item result may now be missing for +a bad id. Preserve prior behavior explicitly — e.g. `throw` "Event not found" if a +child's data is absent from the batch result (matching the old per-child error) +rather than silently dropping it. Also avoid re-introducing O(items × rows): group +rows by their key in a single pass, don't `.filter` all rows per item. + +--- + +## 3. Sequential independent awaits + +**Detect.** Inside a fn or loader, `const a = await x(); const b = await y()` +where `y` does **not** use `a`. They are independent but run serially. + +**Fix.** `const [a, b] = await Promise.all([x(), y()])`. But across **server fns** +this is real concurrency; across **raw queries on one `db`** it is not — see #4. + +Reference: pricing loader (5 independent fetches → one `Promise.all`), sponsors +loader, `getCompetitionRevenueStats` (`src/server/commerce/fee-calculator.ts`), +all in PR #502. + +--- + +## 4. Fake parallelism + +**Detect.** `Promise.all([...])` whose branches all use the **same** `db` / +`getDb()` instance. mysql2 serializes commands on a single connection, so this +runs sequentially despite the `Promise.all` — the most insidious trap because it +*looks* parallel. + +**Fix.** Give each parallel branch its **own** `getDb()`. In-process server-fn +composition gets this for free (each fn calls `getDb()` internally), but raw +queries you parallelize by hand must each open a connection. + +Reference: `getCompetitionLeaderboard` (`src/server/competition-leaderboard.ts`) +runs three dependency-ordered parallel groups, each branch on its own connection. + +```ts +// BEFORE — fake parallelism: one connection serializes both +const db = getDb() +const [competition, registrations] = await Promise.all([ + db.select().from(competitionsTable).where(...), + db.select().from(registrationsTable).where(...), +]) + +// AFTER — one connection per branch actually runs them concurrently +const competitionDb = getDb() +const registrationsDb = getDb() +const [competition, registrations] = await Promise.all([ + competitionDb.select().from(competitionsTable).where(...), + registrationsDb.select().from(registrationsTable).where(...), +]) +``` + +Dependency-order the groups: group 1 fetches what group 2 needs; within a group, +everything is independent and each branch has its own connection. + +--- + +## 5. Over-fetching + +**Detect.** `db.select()` (whole row) when only a few columns are used — +especially joining `users`, which ships `passwordHash` and `athleteProfile` JSON +for every athlete onto the wire. Or fetching all rows and filtering by a field in +JS. + +**Fix.** Project only the consumed columns with `select({ … })`, and push the +filter into SQL with `where(...)` instead of a JS `.filter`. + +Reference: leaderboard registrations now project only the +registration/user/division columns the ranking consumes and filter division in +SQL (`src/server/competition-leaderboard.ts`, PR #512). + +```ts +// BEFORE — full user rows + JS division filter +const regs = (await db.select().from(registrationsTable) + .innerJoin(users, eq(users.id, registrationsTable.userId))) + .filter(r => r.division_id === divisionId) + +// AFTER — projected columns + SQL filter +const regs = await db + .select({ id: registrationsTable.id, name: users.firstName, divisionId: registrationsTable.divisionId }) + .from(registrationsTable) + .innerJoin(users, eq(users.id, registrationsTable.userId)) + .where(eq(registrationsTable.divisionId, divisionId)) +``` + +--- + +## 6. Scan-to-check + +**Detect.** Fetching every row in a scope, then `.find`/`.some` in JS to test +whether one exists — e.g. loading every registration in a competition just to see +if a given email has a pending team invite. + +**Fix.** One `EXISTS`-correlated or `inArray`-scoped query that answers the +question in SQL. + +Reference: pending team invites now use a single EXISTS-scoped query on the +invitee email instead of scanning every registration +(`getPendingTeamInvitesForEmail`, `src/server/competition-detail.ts`, PR #512). + +--- + +## 7. Missing hot-path index + +**Detect.** Frequent `inArray`/`eq` lookups on a junction/global table that has +no index on the looked-up column — a full table scan on every hit. PR #512 found +`workout_movements` and `workout_tags` (global junction tables) were full-scanned +on hot `inArray` lookups, plus `judge_heat_assignments(rotationId)`. + +**Fix.** Add the index in the Drizzle schema's table-extras callback, then apply +with `pnpm db:push` — **no migration files** (push-based workflow; no migrations +dir exists). Flag the `db:push` step explicitly in the PR. + +```ts +// src/db/schemas/workouts.ts +export const workoutTagsTable = mysqlTable( + "workout_tags", + { ...commonColumns, id: varchar({ length: 255 }).primaryKey(), + workoutId: varchar({ length: 255 }).notNull(), tagId: varchar({ length: 255 }).notNull() }, + (table) => [ + index("workout_tags_workout_idx").on(table.workoutId), + index("workout_tags_tag_idx").on(table.tagId), + ], +) +``` + +--- + +## 8. Dead query + +**Detect.** A fetched value that nothing downstream reads — a `totalEvents` count +returned but never rendered, a `getTeamContactEmailFn` loader fetch whose result +is unused, a duplicate mapping query. Grep the symbol; if no consumer, it's dead. + +**Fix.** Delete it. PR #512 removed `totalEvents`, a dead `getTeamContactEmailFn` +fetch, a duplicate leaderboard mappings query, and redundant inline registration +fns. Each deletion is one fewer query on the critical path. + +--- + +## 9. Per-item on critical path + +**Detect.** Non-essential, expensive data (heats, schedules) awaited in the +loader, blocking first paint, when the component could hydrate it after render. + +**Fix.** Keep it **off** the loader's critical path: chain the fn off the +page-data promise **un-awaited** and let the component resolve it client-side +(deferred data). Distinct from #1 — here the goal isn't fewer round trips but +keeping a slow fetch out of the blocking path. + +Reference: event-detail published heats — the route chains `getPublicEventHeatsFn` +off the page-data promise un-awaited and the heat-schedule component resolves it +client-side (`src/routes/compete/$slug/workouts/$eventId.tsx`, PR #512). + +> Regression watch: when deferring, ensure the deferred promise is actually +> forwarded to the component and not accidentally awaited or dropped — PR #512's +> adversarial review caught a heats-deferral regression. diff --git a/.claude/skills/data-fetching-perf-audit/references/stack-gotchas.md b/.claude/skills/data-fetching-perf-audit/references/stack-gotchas.md new file mode 100644 index 000000000..5f0d93c27 --- /dev/null +++ b/.claude/skills/data-fetching-perf-audit/references/stack-gotchas.md @@ -0,0 +1,125 @@ +# Stack gotchas + +Codebase-specific traps that silently undo a perf fix or break the build. The +first two are the ones that bite hardest; read them before optimizing. + +## Contents + +1. [mysql2 single-connection serialization](#1-mysql2-single-connection-serialization) +2. [cloudflare:workers leaking into the client bundle](#2-cloudflareworkers-leaking-into-the-client-bundle) +3. [Push-based DB workflow (indexes)](#3-push-based-db-workflow) +4. [Preserve loader data shapes](#4-preserve-loader-data-shapes) +5. [Server fn = HTTP round trip; in-process composition is free](#5-server-fn--http-round-trip) +6. [Security holes surface during the audit](#6-security-holes-surface-during-the-audit) +7. [Caching is deferred, not free](#7-caching-is-deferred-not-free) + +--- + +## 1. mysql2 single-connection serialization + +`getDb()` returns a Drizzle instance wrapping a **single mysql2 connection**, and +mysql2 serializes commands per connection. Consequences: + +- `Promise.all([db.q1(), db.q2()])` on **one** `db` is **fake parallelism** — the + queries run back-to-back on the wire. The `Promise.all` makes it *look* + concurrent; it isn't. +- Real concurrency requires **one `getDb()` per parallel branch**. In-process + server-fn composition gets this automatically (each fn opens its own + connection). Hand-written parallel raw queries must each call `getDb()`. + +```ts +// Each parallel branch gets its OWN getDb() connection: a Drizzle instance +// wraps a single mysql2 connection which serializes commands, so sharing one +// instance across Promise.all branches would still execute sequentially. +const competitionDb = getDb() +const registrationsDb = getDb() +const [competition, registrations] = await Promise.all([ + competitionDb.select()..., + registrationsDb.select()..., +]) +``` + +> Do **not** "fix" this by memoizing `getDb()` into a shared pooled instance — +> naive memoization would serialize all parallel queries again. Per-request +> connection pooling is a deliberate infra decision, not a perf-audit change. + +## 2. cloudflare:workers leaking into the client bundle + +`@/db` imports `cloudflare:workers`, which Vite **cannot resolve in the browser**. +TanStack Start's client compile strips `createServerFn` **handler bodies** (and +their imports), but it **cannot strip exported plain functions**. So an exported +plain helper that calls `getDb()`, living in a `src/server-fns/*` module, drags +`@/db` → `cloudflare:workers` into the client graph and breaks the build: + +``` +[vite:import-analysis] Failed to resolve import "cloudflare:workers" from "src/db/index.ts" +``` + +This bites specifically when consolidating fns: extracting shared DB helpers as +exported plain functions is the natural refactor, and it's the one that breaks. + +**Fix — follow the `src/server/` convention:** + +- DB-backed plain helpers go in `src/server/*.ts` with `import "server-only"` at + the top (e.g. `src/server/competition-detail.ts`, + `src/server/competition-divisions.ts`). +- Client-safe pieces (Zod parsers, constants, plain types like + `parseCompetitionSettings`, `PublicCompetitionDivision`) go in `src/utils/*` + (e.g. `src/utils/competition-settings.ts`) so server modules can use them with + no cycle. Re-export from the original `*-fns.ts` so existing importers (incl. + client routes) stay unchanged. +- `src/server-fns/*` files then reference the helpers **only inside handler + bodies**, which the client compiler strips. + +Always run `pnpm build` (not just `type-check`) after a consolidation — the +client-bundle resolution error only appears at build time. + +## 3. Push-based DB workflow + +No migrations directory exists. Schema/index changes are applied with +`pnpm db:push` during development. When a perf fix adds an index, the index lives +in the Drizzle schema's table-extras callback and the PR must call out the +`pnpm db:push` step explicitly. Do **not** hand-write SQL migrations. + +## 4. Preserve loader data shapes + +The goal is faster loading with **zero** change to what components receive. Keep +each loader's returned object shape identical (same keys, same types) so child +components and their props are untouched. `pnpm type-check` is the guardrail — if +it passes, the contract held. The only acceptable shape change is deleting a +genuinely dead field (e.g. `organizerContactEmail`), and only after confirming no +consumer reads it. + +## 5. Server fn = HTTP round trip + +A `createServerFn` call awaited in a loader is a network request on client-side +navigation. Composing fns **in-process** (one server fn calling another's logic +server-side) costs no extra round trip. That's why consolidation works: the +loader makes 1–2 calls, and the fan-out happens server-to-server. When you need +two loader-level calls, make them parallel and have each resolve its own +prerequisites (e.g. both take the `slug`) so neither blocks the other. + +## 6. Security holes surface during the audit + +Tracing every fetch is also an authz audit. Two real holes found in PR #512: + +- **IDOR:** `getUserCompetitionRegistrationsFn` accepted a client-supplied + `userId` with no session check — any visitor could read another user's + registrations. Fix: derive the user from `getSessionFromCookie()`, never trust + client input for identity. +- **Draft exposure:** `getHeatsForCompetitionFn` returned unpublished heats — + including athlete names — to anonymous visitors. Fix: filter + `schedulePublishedAt IS NOT NULL` unless the viewer is a site admin or holds + owner/admin/`manage_programming` on the organizing team. Add regression tests + and verify every existing caller is unaffected. + +When over-fetching also leaks sensitive columns (`passwordHash`, profile JSON via +full `users` rows), the projection fix in anti-pattern #5 is a security fix too. + +## 7. Caching is deferred, not free + +These optimizations are about call structure, not caching. Caching the +now-consolidated public payloads (KV), enabling Hyperdrive query caching, and +per-request connection pooling are separate infra decisions — Hyperdrive query +caching is explicitly disabled in `alchemy.run.ts`; verify *why* before flipping +it. Don't bundle these into a structural perf audit.