From b30be24fd52697f826bedaf45f5daa62143ac44b Mon Sep 17 00:00:00 2001 From: Josh Creek <8179928+jcreek@users.noreply.github.com> Date: Sat, 10 Jan 2026 12:37:43 +0000 Subject: [PATCH] feat(#56): Address PR comments --- src/lib/repositories/CatchRecordRepository.ts | 45 ++++++- src/lib/utils/catchRecordWriteQueue.ts | 2 +- .../pokedexes/[id]/catch-records/+server.ts | 19 ++- src/routes/my-pokedexes/+page.svelte | 3 +- src/routes/pokedex/[id]/+page.svelte | 18 ++- ...00_fix_catch_records_unique_constraint.sql | 113 +++++++++++++++++- 6 files changed, 187 insertions(+), 13 deletions(-) diff --git a/src/lib/repositories/CatchRecordRepository.ts b/src/lib/repositories/CatchRecordRepository.ts index ef67dad..4767265 100644 --- a/src/lib/repositories/CatchRecordRepository.ts +++ b/src/lib/repositories/CatchRecordRepository.ts @@ -1,6 +1,8 @@ import type { CatchRecord, CatchRecordDB } from '$lib/models/CatchRecord'; import type { SupabaseClient } from '@supabase/supabase-js'; +type PartialExcept = Partial & Pick; + class CatchRecordRepository { constructor( private supabase: SupabaseClient, @@ -50,12 +52,43 @@ class CatchRecordRepository { * If the database is not configured with that constraint, the caller should * catch the error and fall back to per-record upserts. */ - async bulkUpsert(records: Partial[]): Promise { - const dbRows: Partial[] = records.map((r) => ({ - userId: this.userId, - pokedexId: this.pokedexId, - ...this.transformToDatabase(r) - })); + async bulkUpsert( + records: Array> + ): Promise { + if (records.length === 0) return []; + + const dbRows: Partial[] = records.map((r, index) => { + if (r.pokedexEntryId === undefined || r.pokedexEntryId === null || r.pokedexEntryId === '') { + throw new Error( + `CatchRecordRepository.bulkUpsert: record at index ${index} is missing required field "pokedexEntryId"` + ); + } + + // Preserve repository scoping; if a caller supplies ids, they must match the repo scope. + if (r.userId !== undefined && r.userId !== this.userId) { + throw new Error( + `CatchRecordRepository.bulkUpsert: record at index ${index} has userId "${r.userId}" but repository is scoped to "${this.userId}"` + ); + } + if (r.pokedexId !== undefined && r.pokedexId !== this.pokedexId) { + throw new Error( + `CatchRecordRepository.bulkUpsert: record at index ${index} has pokedexId "${r.pokedexId}" but repository is scoped to "${this.pokedexId}"` + ); + } + + const mapped = this.transformToDatabase(r); + // Enforce repo scope after mapping so per-record input cannot override it. + mapped.userId = this.userId; + mapped.pokedexId = this.pokedexId; + + if (mapped.pokedexEntryId === undefined || mapped.pokedexEntryId === null) { + throw new Error( + `CatchRecordRepository.bulkUpsert: record at index ${index} produced no pokedexEntryId after transform` + ); + } + + return mapped; + }); const { data: result, error } = await this.supabase .from('catch_records') diff --git a/src/lib/utils/catchRecordWriteQueue.ts b/src/lib/utils/catchRecordWriteQueue.ts index 005cc4b..5bc89e1 100644 --- a/src/lib/utils/catchRecordWriteQueue.ts +++ b/src/lib/utils/catchRecordWriteQueue.ts @@ -71,7 +71,7 @@ export function createCatchRecordWriteQueue(options: CreateCatchRecordWriteQueue getPendingCount: () => number; clearError: () => void; } { - const { endpointUrl, fetchFn, batchSize = 100, concurrency = 2 } = options; + const { endpointUrl, fetchFn, batchSize = 100, concurrency = 1 } = options; const items = new Map(); let scheduled: ReturnType | null = null; diff --git a/src/routes/api/pokedexes/[id]/catch-records/+server.ts b/src/routes/api/pokedexes/[id]/catch-records/+server.ts index 7989757..f8a113b 100644 --- a/src/routes/api/pokedexes/[id]/catch-records/+server.ts +++ b/src/routes/api/pokedexes/[id]/catch-records/+server.ts @@ -88,12 +88,29 @@ export const POST = async (event: RequestEvent) => { try { const userId = await requireAuth(event); const { id: pokedexId } = event.params; - const records: Partial[] = await event.request.json(); + const body: unknown = await event.request.json(); if (!pokedexId) { return json({ error: 'Pokedex ID is required' }, { status: 400 }); } + if (!Array.isArray(body)) { + return json({ error: 'Request body must be an array of catch records' }, { status: 400 }); + } + const invalidIndex = body.findIndex((r) => { + if (!r || typeof r !== 'object') return true; + const entryId = (r as Partial).pokedexEntryId; + return entryId === undefined || entryId === null || entryId === ''; + }); + if (invalidIndex !== -1) { + return json( + { error: `Record at index ${invalidIndex} is missing required field "pokedexEntryId"` }, + { status: 400 } + ); + } + + const records = body as Array & Pick>; + const { session } = await event.locals.safeGetSession(); if (session) { await event.locals.supabase.auth.setSession(session); diff --git a/src/routes/my-pokedexes/+page.svelte b/src/routes/my-pokedexes/+page.svelte index ba83c18..9677d5f 100644 --- a/src/routes/my-pokedexes/+page.svelte +++ b/src/routes/my-pokedexes/+page.svelte @@ -40,7 +40,8 @@ async function readApiErrorMessage(response: Response): Promise { try { - const data = await response.json(); + // Use a clone so we can still fall back to reading the original body as text. + const data = await response.clone().json(); if (data && typeof data === 'object' && 'error' in data && typeof data.error === 'string') { return data.error; } diff --git a/src/routes/pokedex/[id]/+page.svelte b/src/routes/pokedex/[id]/+page.svelte index 77f021b..fe44eeb 100644 --- a/src/routes/pokedex/[id]/+page.svelte +++ b/src/routes/pokedex/[id]/+page.svelte @@ -41,6 +41,7 @@ let catchWriteQueue: ReturnType | null = null; let catchWriteQueueKey: string | null = null; + let catchWriteQueueUnsubscribe: (() => void) | null = null; let catchWriteStatus: CatchRecordWriteQueueStatus = { pending: 0, inFlight: 0, @@ -57,6 +58,10 @@ localUser = value; }); onDestroy(unsubscribe); + onDestroy(() => { + catchWriteQueueUnsubscribe?.(); + catchWriteQueueUnsubscribe = null; + }); function openPokemonModal(pokemon: CombinedData) { selectedPokemon = pokemon; @@ -75,15 +80,19 @@ const desiredKey = `${localUser.id}:${pokedexId}`; if (catchWriteQueue && catchWriteQueueKey === desiredKey) return; + // Unsubscribe from the previous queue's status store before replacing the queue. + catchWriteQueueUnsubscribe?.(); + catchWriteQueueUnsubscribe = null; + catchWriteQueue = createCatchRecordWriteQueue({ endpointUrl: `/api/pokedexes/${pokedexId}/catch-records`, fetchFn: fetch, batchSize: 200, - concurrency: 2 + concurrency: 1 }); catchWriteQueueKey = desiredKey; - catchWriteQueue.getStatus.subscribe((s) => { + catchWriteQueueUnsubscribe = catchWriteQueue.getStatus.subscribe((s) => { catchWriteStatus = s; }); } @@ -332,9 +341,11 @@ if (document.visibilityState === 'hidden') flushKeepalive(); }; + const onOnline = () => void catchWriteQueue?.flushNow(); + window.addEventListener('pagehide', flushKeepalive); document.addEventListener('visibilitychange', onVisibilityChange); - window.addEventListener('online', () => void catchWriteQueue?.flushNow()); + window.addEventListener('online', onOnline); // Periodic reconciliation to guard against any missed state (e.g. aborted tab-close flush). const reconcileInterval = window.setInterval(() => { @@ -347,6 +358,7 @@ return () => { window.removeEventListener('pagehide', flushKeepalive); document.removeEventListener('visibilitychange', onVisibilityChange); + window.removeEventListener('online', onOnline); window.clearInterval(reconcileInterval); }; }); diff --git a/supabase/migrations/20260110114000_fix_catch_records_unique_constraint.sql b/supabase/migrations/20260110114000_fix_catch_records_unique_constraint.sql index 5b5cb78..ec699e8 100644 --- a/supabase/migrations/20260110114000_fix_catch_records_unique_constraint.sql +++ b/supabase/migrations/20260110114000_fix_catch_records_unique_constraint.sql @@ -9,6 +9,118 @@ ALTER TABLE public.catch_records ADD COLUMN IF NOT EXISTS "pokedexId" UUID; +-- 1.5) Guards: fail early with diagnostics if the backfill would be unsafe. +-- +-- Why: +-- - The backfill below only updates rows for users that have at least one pokedex. +-- If any user has catch_records but no pokedexes, those rows would remain NULL +-- and the NOT NULL enforcement would fail. +-- - The backfill assigns all NULL pokedexId rows to each user's oldest pokedex. +-- That can create duplicate (userId, pokedexId, pokedexEntryId) combinations. +-- +-- If either condition is detected, this migration aborts and prints sample IDs. +DO $$ +DECLARE + orphan_user_count bigint; + orphan_row_count bigint; + orphan_user_samples text; + + dup_group_count bigint; + dup_group_samples text; +BEGIN + -- Guard A: Any catch_records with NULL pokedexId for users who have no pokedexes? + WITH orphan_users AS ( + SELECT cr."userId" AS user_id, + COUNT(*)::bigint AS null_row_count + FROM public.catch_records cr + WHERE cr."pokedexId" IS NULL + AND NOT EXISTS ( + SELECT 1 + FROM public.pokedexes p + WHERE p."userId" = cr."userId" + ) + GROUP BY cr."userId" + ) + SELECT + COUNT(*)::bigint, + COALESCE(SUM(null_row_count), 0)::bigint, + ( + SELECT string_agg(user_id::text, ', ' ORDER BY user_id::text) + FROM ( + SELECT user_id, null_row_count + FROM orphan_users + ORDER BY null_row_count DESC, user_id + LIMIT 10 + ) s + ) + INTO orphan_user_count, orphan_row_count, orphan_user_samples + FROM orphan_users; + + IF orphan_user_count > 0 THEN + RAISE EXCEPTION + 'USAFE MIGRATION: found % user(s) with % catch_records row(s) where "pokedexId" is NULL but the user has no pokedexes. Sample userId(s): %', + orphan_user_count, + orphan_row_count, + COALESCE(orphan_user_samples, '(none)'); + END IF; + + -- Guard B: Would assigning NULL pokedexId rows to each user''s oldest pokedex create duplicates? + WITH primary_pokedex AS ( + SELECT DISTINCT ON ("userId") id AS pokedex_id, "userId" + FROM public.pokedexes + ORDER BY "userId", "createdAt" ASC + ), + projected AS ( + SELECT + cr."userId", + COALESCE(cr."pokedexId", pp.pokedex_id) AS projected_pokedex_id, + cr."pokedexEntryId" + FROM public.catch_records cr + LEFT JOIN primary_pokedex pp + ON pp."userId" = cr."userId" + ), + dup_groups AS ( + SELECT + "userId", + projected_pokedex_id, + "pokedexEntryId", + COUNT(*)::bigint AS row_count + FROM projected + WHERE projected_pokedex_id IS NOT NULL + GROUP BY "userId", projected_pokedex_id, "pokedexEntryId" + HAVING COUNT(*) > 1 + ) + SELECT + COUNT(*)::bigint, + ( + SELECT string_agg( + format( + '(userId=%s, pokedexId=%s, pokedexEntryId=%s, rows=%s)', + "userId"::text, + projected_pokedex_id::text, + "pokedexEntryId"::text, + row_count::text + ), + E'\n' + ORDER BY row_count DESC, "userId"::text + ) + FROM ( + SELECT * FROM dup_groups + ORDER BY row_count DESC, "userId"::text + LIMIT 10 + ) s + ) + INTO dup_group_count, dup_group_samples + FROM dup_groups; + + IF dup_group_count > 0 THEN + RAISE EXCEPTION + 'USAFE MIGRATION: backfill would create % duplicate (userId, pokedexId, pokedexEntryId) group(s) after projecting NULL "pokedexId" to each user''s oldest pokedex. Sample group(s):\n%', + dup_group_count, + COALESCE(dup_group_samples, '(none)'); + END IF; +END $$; + -- 2) Best-effort backfill for any NULL pokedexId rows. -- Strategy: assign to the user's oldest pokedex. WITH primary_pokedex AS ( @@ -95,4 +207,3 @@ CREATE POLICY "Users can delete own catch records" ON public.catch_records AND public.pokedexes."userId" = auth.uid() ) ); -