feat(#56): Address PR comments

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<T, K extends keyof T> = Partial<T> & Pick<T, K>;
+
 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<CatchRecord>[]): Promise<CatchRecord[]> {
+	async bulkUpsert(
-		const dbRows: Partial<CatchRecordDB>[] = records.map((r) => ({
+		records: Array<PartialExcept<CatchRecord, 'pokedexEntryId'>>
-			userId: this.userId,
+	): Promise<CatchRecord[]> {
-			pokedexId: this.pokedexId,
+		if (records.length === 0) return [];
-			...this.transformToDatabase(r)
+
-		}));
+		const dbRows: Partial<CatchRecordDB>[] = 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')

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<string, QueueItem>();
 	let scheduled: ReturnType<typeof setTimeout> | null = null;

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<CatchRecord>[] = 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<CatchRecord>).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<Partial<CatchRecord> & Pick<CatchRecord, 'pokedexEntryId'>>;
+
 		const { session } = await event.locals.safeGetSession();
 		if (session) {
 			await event.locals.supabase.auth.setSession(session);

src/routes/my-pokedexes/+page.svelte

@@ -40,7 +40,8 @@
 
 	async function readApiErrorMessage(response: Response): Promise<string> {
 		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;
 			}

src/routes/pokedex/[id]/+page.svelte

@@ -41,6 +41,7 @@
 
 	let catchWriteQueue: ReturnType<typeof createCatchRecordWriteQueue> | 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);
 		};
 	});

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()
 		)
 	);
-