fix: remediate branch review findings

tests/bdd/features/account-access.feature

@@ -26,22 +26,36 @@ Feature: Account access
 
   Scenario: Sign out
     Given I am signed in
+    And my offline copy is synchronized
     When I sign out
     Then I return to the public home page
+    And my offline copy is removed
+
+  Scenario: Keep the session when sign out fails
+    Given I am signed in
+    And my offline copy is synchronized
+    When the sign-out request fails
+    Then I remain signed in with an error
+    And my offline copy remains
 
   Scenario: Request a password reset
     Given I have a confirmed account
     When I request a password reset
     Then a password reset email is captured locally
 
+  Scenario: Reject a normal session on the recovery page
+    Given I am signed in
+    When I visit the password recovery page directly
+    Then the replacement password form is unavailable
+
   @product-review
   Scenario: Reject mismatched replacement passwords
-    Given I am signed in on the password reset page
+    Given I follow a valid password reset link
     When I enter two different replacement passwords
     Then I am told that the passwords do not match
 
   Scenario: Complete a password reset
-    Given I am signed in on the password reset page
+    Given I follow a valid password reset link
     When I enter a valid replacement password
     Then I am told that my password was updated
-
+    And only the replacement password signs me in

tests/bdd/features/backup-export.feature

@@ -40,4 +40,3 @@ Feature: Backup and export
     When I update collection progress
     Then the catch remains marked caught
     And the provider failure is shown in backup settings
-

tests/bdd/features/dex-composition.feature

@@ -29,4 +29,3 @@ Feature: Pokédex composition
     Given I have a Form Dex named "Black Forms" scoped to game "Black" and dex "Unova"
     When I inspect its entries with forms
     Then Rotom includes its named default form without duplicate forms
-

tests/bdd/features/pokedex-lifecycle.feature

@@ -54,4 +54,3 @@ Feature: Pokédex lifecycle
     Given another trainer has a Pokédex
     When I request the other trainer's Pokédex
     Then the Pokédex is not disclosed
-

tests/bdd/features/progress-tracking.feature

@@ -35,4 +35,3 @@ Feature: Progress tracking
     When I select the "Compact" box layout
     And I reload the Pokédex
     Then the "Compact" box layout remains selected
-

tests/bdd/features/pwa-offline.feature

@@ -7,16 +7,24 @@ Feature: Offline-friendly application
     When I open the built application
     Then a service worker controls the page
     And the application shell is precached
+    And no legacy service worker is requested
 
   Scenario: Reload the home page while offline
     Given I have opened the built application online
     When I go offline and reload the home page
-    Then the application remains available
+    Then the read-only offline viewer is available
 
-  Scenario: Navigate to another route while offline
+  Scenario: Reload a nested route while offline
     Given I have opened the built application online
-    When I go offline and navigate to the sign-in page
-    Then the sign-in form is available offline
+    When I go offline and reload the sign-in page
+    Then the read-only offline viewer is available
+
+  Scenario: Read a synchronized collection offline
+    Given I am signed in
+    And I have a Living Dex named "Offline Collection"
+    And my offline copy is synchronized
+    When I go offline and reload the current Pokédex
+    Then the offline copy contains "Offline Collection"
 
   Scenario: Restore network access
     Given I have opened the built application online

tests/bdd/fixtures.ts

@@ -11,6 +11,7 @@ export type ScenarioState = {
 	lastResponseStatus: number | null;
 	lastMessage: string | null;
 	caughtEntryLabel: string | null;
+	legacyServiceWorkerRequested: boolean;
 };
 
 type Fixtures = { state: ScenarioState; providerMock: void };
@@ -51,7 +52,8 @@ export const test = base.extend<Fixtures>({
 			entries: [],
 			lastResponseStatus: null,
 			lastMessage: null,
-			caughtEntryLabel: null
+			caughtEntryLabel: null,
+			legacyServiceWorkerRequested: false
 		});
 	}
 });

tests/bdd/steps/account.steps.ts

@@ -14,6 +14,28 @@ async function mailCountFor(email: string, subject: string): Promise<number> {
 	return body.total ?? body.messages?.length ?? 0;
 }
 
+async function recoveryLinkFromMail(email: string): Promise<string> {
+	for (let attempt = 0; attempt < 50; attempt++) {
+		const search = await fetch(
+			`${MAILPIT_URL}/api/v1/search?query=${encodeURIComponent(`to:${email} subject:Reset`)}`
+		);
+		if (search.ok) {
+			const result = (await search.json()) as { messages?: Array<{ ID?: string; Id?: string }> };
+			const id = result.messages?.[0]?.ID ?? result.messages?.[0]?.Id;
+			if (id) {
+				const response = await fetch(`${MAILPIT_URL}/api/v1/message/${id}`);
+				if (response.ok) {
+					const message = JSON.stringify(await response.json());
+					const match = message.match(/https?:\/\/[^"'<>\s]+\/auth\/v1\/verify[^"'<>\s]+/);
+					if (match) return match[0].replaceAll('&amp;', '&').replaceAll('\\u0026', '&');
+				}
+			}
+		}
+		await new Promise((resolve) => setTimeout(resolve, 200));
+	}
+	throw new Error('No password recovery link arrived in MailPit');
+}
+
 Given('I am a new visitor', async ({ page }) => {
 	await page.goto('/');
 });
@@ -28,19 +50,16 @@ Given('I am signed in', async ({ page, state }) => {
 	await expect(page).toHaveURL(/\/my-pokedexes$/);
 });
 
-/**
- * Deliberately an ordinary signed-in session, not a recovery one: following a real recovery
- * action link currently bounces to /signin, because the browser client in src/routes/+layout.ts
- * has no cookie `set`/`remove` method and so cannot persist the session it parses out of the
- * URL. Until that is fixed, these scenarios cover the form, not the emailed-link flow - hence
- * the step name. `createRecoveryLink` in ../support/app.ts is ready for when it is.
- */
-Given('I am signed in on the password reset page', async ({ page, state }) => {
+Given('I follow a valid password reset link', async ({ page, state }) => {
 	await createConfirmedUser(state);
-	await signIn(page, state);
-	await expect(page).toHaveURL(/\/my-pokedexes$/);
-	await page.goto('/reset-password');
-	await expect(page.getByLabel('New Password')).toBeVisible();
+	await page.goto('/forgot-password');
+	await page.getByLabel('Email').fill(state.email);
+	await page.getByRole('button', { name: 'Send Reset Link' }).click();
+	await expect(page.getByText('Check your email for the password reset link')).toBeVisible();
+	const actionLink = await recoveryLinkFromMail(state.email);
+	await page.goto(actionLink);
+	await expect(page).toHaveURL(/\/reset-password/);
+	await expect(page.getByLabel('New Password')).toBeEnabled();
 });
 
 When('I register with valid account details', async ({ page, state }) => {
@@ -66,12 +85,28 @@ When('I sign out', async ({ page }) => {
 	await page.getByRole('button', { name: 'Sign Out', exact: true }).click();
 });
 
+When('the sign-out request fails', async ({ page }) => {
+	await page.route('**/auth/v1/logout*', (route) =>
+		route.fulfill({
+			status: 503,
+			contentType: 'application/json',
+			body: '{"message":"unavailable"}'
+		})
+	);
+	await page.getByRole('button', { name: 'usericon' }).click();
+	await page.getByRole('button', { name: 'Sign Out', exact: true }).click();
+});
+
 When('I request a password reset', async ({ page, state }) => {
 	await page.goto('/forgot-password');
 	await page.getByLabel('Email').fill(state.email);
 	await page.getByRole('button', { name: 'Send Reset Link' }).click();
 });
 
+When('I visit the password recovery page directly', async ({ page }) => {
+	await page.goto('/reset-password');
+});
+
 When('I enter two different replacement passwords', async ({ page, state }) => {
 	await page.getByLabel('New Password').fill(state.replacementPassword);
 	await page.getByLabel('Confirm Password').fill(`${state.replacementPassword}-different`);
@@ -105,11 +140,21 @@ Then('I return to the public home page', async ({ page }) => {
 	await expect(page).toHaveURL(/\/$/);
 });
 
+Then('I remain signed in with an error', async ({ page }) => {
+	await expect(page).toHaveURL(/\/my-pokedexes$/);
+	await expect(page.locator('.alert-error.rounded-none')).toContainText('Sign out failed');
+});
+
 Then('a password reset email is captured locally', async ({ page, state }) => {
 	await expect(page.getByText('Check your email for the password reset link')).toBeVisible();
 	await expect.poll(() => mailCountFor(state.email, 'Reset')).toBeGreaterThan(0);
 });
 
+Then('the replacement password form is unavailable', async ({ page }) => {
+	await expect(page.getByLabel('New Password')).toBeDisabled();
+	await expect(page.getByText(/invalid or expired/i)).toBeVisible();
+});
+
 Then('I am told that the passwords do not match', async ({ page }) => {
 	await expect(page.getByText('Passwords do not match')).toBeVisible();
 });
@@ -117,3 +162,12 @@ Then('I am told that the passwords do not match', async ({ page }) => {
 Then('I am told that my password was updated', async ({ page }) => {
 	await expect(page.getByText(/Password updated successfully/)).toBeVisible();
 });
+
+Then('only the replacement password signs me in', async ({ page, state }) => {
+	await expect(page).toHaveURL(/\/signin$/, { timeout: 10_000 });
+	await signIn(page, state, state.password);
+	await expect(page.locator('.alert-error')).toBeVisible();
+	await page.getByLabel('Password').fill(state.replacementPassword);
+	await page.getByRole('button', { name: 'Sign In' }).click();
+	await expect(page).toHaveURL(/\/my-pokedexes$/);
+});

tests/bdd/steps/pwa.steps.ts

@@ -28,12 +28,22 @@ async function cacheContents(page: Page) {
 	});
 }
 
-When('I open the built application', async ({ page }) => {
+function recordLegacyWorkerRequest(page: Page, state: { legacyServiceWorkerRequested: boolean }) {
+	page.on('request', (request) => {
+		if (new URL(request.url()).pathname === '/service-worker.js') {
+			state.legacyServiceWorkerRequested = true;
+		}
+	});
+}
+
+When('I open the built application', async ({ page, state }) => {
+	recordLegacyWorkerRequest(page, state);
 	await page.goto('/');
 	await waitForServiceWorker(page);
 });
 
-Given('I have opened the built application online', async ({ page }) => {
+Given('I have opened the built application online', async ({ page, state }) => {
+	recordLegacyWorkerRequest(page, state);
 	await page.context().setOffline(false);
 	await page.goto('/');
 	await waitForServiceWorker(page);
@@ -45,11 +55,45 @@ When('I go offline and reload the home page', async ({ page }) => {
 	await page.reload({ waitUntil: 'domcontentloaded' });
 });
 
-When('I go offline and navigate to the sign-in page', async ({ page }) => {
+When('I go offline and reload the sign-in page', async ({ page }) => {
+	await page.goto('/signin');
 	await page.context().setOffline(true);
-	// Client-side navigation, which only works if the route's chunks were precached.
-	await page.getByRole('link', { name: 'Sign In' }).click();
-	await expect(page).toHaveURL(/\/signin$/);
+	await page.reload({ waitUntil: 'domcontentloaded' });
+});
+
+Given('my offline copy is synchronized', async ({ page, state }) => {
+	await waitForServiceWorker(page);
+	await expect
+		.poll(() =>
+			page.evaluate(async (userId) => {
+				const meta = await (
+					await caches.open('livingdex-offline-meta-v1')
+				).match('/__offline/current');
+				if (!meta) return false;
+				const value = await meta.json();
+				return value.userId === userId;
+			}, state.userId)
+		)
+		.toBe(true);
+	const serializedSnapshot = await page.evaluate(async () => {
+		const metaResponse = await (
+			await caches.open('livingdex-offline-meta-v1')
+		).match('/__offline/current');
+		if (!metaResponse) return '';
+		const meta = await metaResponse.json();
+		const snapshotResponse = await (
+			await caches.open(meta.dataCache)
+		).match(`/__offline/snapshot/${encodeURIComponent(meta.userId)}`);
+		return snapshotResponse ? await snapshotResponse.text() : '';
+	});
+	expect(serializedSnapshot).not.toMatch(/access_token|refresh_token/i);
+});
+
+When('I go offline and reload the current Pokédex', async ({ page, state }) => {
+	await page.context().setOffline(true);
+	await page.goto(`/pokedex/${state.pokedexId}/offline`, {
+		waitUntil: 'domcontentloaded'
+	});
 });
 
 When('I go offline and then return online', async ({ page }) => {
@@ -61,6 +105,15 @@ When('I go offline and then return online', async ({ page }) => {
 
 Then('a service worker controls the page', async ({ page }) => {
 	expect(await waitForServiceWorker(page)).toMatch(/\/(?:sw|prompt-sw)\.js$/);
+	expect(
+		await page.evaluate(() =>
+			navigator.serviceWorker.getRegistrations().then((items) => items.length)
+		)
+	).toBe(1);
+});
+
+Then('no legacy service worker is requested', async ({ state }) => {
+	expect(state.legacyServiceWorkerRequested).toBe(false);
 });
 
 /**
@@ -77,7 +130,11 @@ Then('the application shell is precached', async ({ page }) => {
 	const origin = new URL(page.url()).origin;
 	const urls = contents[names[0]].map((url) => url.slice(`${origin}/`.length));
 
-	expect(urls, 'app shell is not precached').toContain('');
+	expect(urls, 'personalized SSR root must not be precached').not.toContain('');
+	expect(
+		urls.some((url) => /^offline(?:\.html)?(?:\?__WB_REVISION__=|$)/.test(url)),
+		'offline viewer is not precached'
+	).toBe(true);
 	expect(
 		urls.some((url) => url.startsWith('manifest.webmanifest?__WB_REVISION__=')),
 		'revisioned manifest.webmanifest is not precached'
@@ -100,8 +157,31 @@ Then('the application remains available', async ({ page }) => {
 	await expect(page.getByRole('heading', { name: /Start Your Pokédex Journey/ })).toBeVisible();
 });
 
-Then('the sign-in form is available offline', async ({ page }) => {
-	await expect(page.getByLabel('Email')).toBeVisible();
-	await expect(page.getByLabel('Password')).toBeVisible();
-	await expect(page.getByRole('button', { name: 'Sign In' })).toBeVisible();
+Then('the read-only offline viewer is available', async ({ page }) => {
+	await expect(page.getByText(/offline.*read-only/i)).toBeVisible();
+});
+
+Then('the offline copy contains {string}', async ({ page }, name: string) => {
+	await expect(page.getByRole('heading', { name })).toBeVisible();
+	await expect(page.getByText(/read-only copy/i)).toBeVisible();
+	await expect(page.locator('button, input, textarea, select')).toHaveCount(0);
+});
+
+Then('my offline copy is removed', async ({ page }) => {
+	await expect
+		.poll(() =>
+			page.evaluate(async () => {
+				const names = await caches.keys();
+				return names.some((name) => name.startsWith('livingdex-offline-'));
+			})
+		)
+		.toBe(false);
+});
+
+Then('my offline copy remains', async ({ page, state }) => {
+	const owner = await page.evaluate(async () => {
+		const meta = await (await caches.open('livingdex-offline-meta-v1')).match('/__offline/current');
+		return meta ? (await meta.json()).userId : null;
+	});
+	expect(owner).toBe(state.userId);
 });

tests/bdd/support/app.ts

@@ -43,34 +43,6 @@ export async function deleteAllPokedexes(state: ScenarioState): Promise<void> {
 	if (!response.ok) throw new Error(`Unable to clear Pokédexes: ${await response.text()}`);
 }
 
-/**
- * Asks Supabase for a real recovery action link - the same token-bearing URL the emailed link
- * carries - so the reset scenarios exercise token verification rather than an ordinary session.
- * `redirectTo` must be listed in `auth.additional_redirect_urls` in supabase/config.toml.
- */
-export async function createRecoveryLink(
-	state: ScenarioState,
-	redirectTo: string
-): Promise<string> {
-	const key = requireServiceRoleKey();
-	const response = await fetch(`${SUPABASE_URL}/auth/v1/admin/generate_link`, {
-		method: 'POST',
-		headers: {
-			apikey: key,
-			Authorization: `Bearer ${key}`,
-			'Content-Type': 'application/json'
-		},
-		body: JSON.stringify({ type: 'recovery', email: state.email, redirect_to: redirectTo })
-	});
-	if (!response.ok)
-		throw new Error(
-			`Unable to generate a recovery link: ${response.status} ${await response.text()}`
-		);
-	const body = (await response.json()) as { action_link?: string };
-	if (!body.action_link) throw new Error('Supabase returned no recovery action link');
-	return body.action_link;
-}
-
 export async function signIn(page: Page, state: ScenarioState, password = state.password) {
 	await page.goto('/signin');
 	await page.getByLabel('Email').fill(state.email);

tests/build/build.test.ts

@@ -29,13 +29,16 @@ describe(`test-build: ${nodeAdapter ? 'node' : 'static'} adapter`, () => {
 			match && match.length === 1,
 			'missing manifest.webmanifest in sw precache manifest'
 		).toBeTruthy();
-		// The generateSW manifest is emitted as JSON ("url": "/"), while prompt-sw.ts's own
-		// `precache([{ url: '/' }])` survives minification as an unquoted key (url:"/").
-		match = swContent.match(/"?url"?:\s*"(?:\/|index\.html)"/);
+		match = swContent.match(/"?url"?:\s*"\/?offline(?:\.html)?"/);
 		expect(
 			match && match.length === 1,
-			'missing offline entry point in sw precache manifest'
+			'missing credential-free offline entry point in sw precache manifest'
 		).toBeTruthy();
+		const outputRoot = `./build/${nodeAdapter ? 'client/' : ''}`;
+		expect(existsSync(`${outputRoot}offline.html`)).toBe(true);
+		expect(existsSync(`${outputRoot}offline-worker.js`)).toBe(true);
+		expect(existsSync(`${outputRoot}service-worker.js`)).toBe(false);
+		expect(swContent).not.toMatch(/"?url"?:\s*"\/"/);
 		if (nodeAdapter) {
 			match = swContent.match(/"url":\s*"server\//);
 			expect(match === null, 'found server/ entries in sw precache manifest').toBeTruthy();

tests/integration/databaseIntegrity.integration.test.ts

@@ -1,17 +1,24 @@
 import { createClient } from '@supabase/supabase-js';
-import { beforeAll, describe, expect, it } from 'vitest';
+import { afterAll, beforeAll, describe, expect, it } from 'vitest';
 
 const url = process.env.TEST_SUPABASE_URL ?? 'http://127.0.0.1:54321';
 const anonKey = process.env.TEST_SUPABASE_ANON_KEY;
 const serviceKey = process.env.E2E_SERVICE_ROLE_KEY ?? process.env.SUPABASE_SERVICE_ROLE_KEY;
 
 describe('database integrity and ownership', () => {
+	const createdUserIds: string[] = [];
 	beforeAll(() => {
 		if (!anonKey || !serviceKey) {
 			throw new Error('Integration tests require TEST_SUPABASE_ANON_KEY and E2E_SERVICE_ROLE_KEY');
 		}
 	});
 
+	afterAll(async () => {
+		if (!serviceKey) return;
+		const admin = createClient(url, serviceKey);
+		await Promise.all(createdUserIds.map((id) => admin.auth.admin.deleteUser(id)));
+	});
+
 	it('enforces catch-record uniqueness and cascades records when a Pokédex is deleted', async () => {
 		const admin = createClient(url, serviceKey!);
 		const email = `integration-cascade-${Date.now()}@example.test`;
@@ -22,6 +29,7 @@ describe('database integrity and ownership', () => {
 		});
 		expect(userError).toBeNull();
 		const userId = created.user!.id;
+		createdUserIds.push(userId);
 		const { data: dex, error: dexError } = await admin
 			.from('pokedexes')
 			.insert({ userId, name: 'Cascade', isLivingDex: true })
@@ -64,6 +72,7 @@ describe('database integrity and ownership', () => {
 		});
 		expect(first.error).toBeNull();
 		expect(second.error).toBeNull();
+		createdUserIds.push(first.data.user!.id, second.data.user!.id);
 		const { data: dex } = await admin
 			.from('pokedexes')
 			.insert({ userId: first.data.user!.id, name: 'Owner only', isLivingDex: true })
@@ -87,6 +96,7 @@ describe('database integrity and ownership', () => {
 			password: 'Integration123!',
 			email_confirm: true
 		});
+		createdUserIds.push(created.data.user!.id);
 		const { data: dex } = await admin
 			.from('pokedexes')
 			.insert({ userId: created.data.user!.id, name: 'Unique mapping', isLivingDex: true })

tests/unit/spriteUrl.test.ts

@@ -0,0 +1,34 @@
+import { describe, expect, it } from 'vitest';
+import { resolveSpriteUrl } from '$lib/utils/spriteUrl';
+
+describe('resolveSpriteUrl', () => {
+	it('uses a supplied sprite key and local shiny path', () => {
+		expect(
+			resolveSpriteUrl({ pokedexNumber: 25, form: '', spriteKey: '25-partner-cap' }, true, true)
+		).toBe('/sprites-small/home/shiny/25-partner-cap.webp');
+	});
+
+	it('places female forms in the female folder', () => {
+		expect(
+			resolveSpriteUrl({ pokedexNumber: 592, form: 'female', spriteKey: '592' }, false, true)
+		).toBe('/sprites-small/home/female/592.webp');
+	});
+
+	it('derives the established fallback key for regional forms', () => {
+		expect(resolveSpriteUrl({ pokedexNumber: 83, form: 'Galarian' }, false, false)).toBe(
+			'https://raw.githubusercontent.com/jcreek/LivingDexTracker/master/static/sprites-small/home/83-galar.webp'
+		);
+	});
+
+	it('uses the Pokédex number for the default male form', () => {
+		expect(resolveSpriteUrl({ pokedexNumber: 25, form: 'Male' }, false, true)).toBe(
+			'/sprites-small/home/25.webp'
+		);
+	});
+
+	it('normalizes decorated form names and an all-zero number', () => {
+		expect(
+			resolveSpriteUrl({ pokedexNumber: 0, form: 'Form 2 [event] (legacy)' }, false, true)
+		).toBe('/sprites-small/home/0-form-two.webp');
+	});
+});