diff --git a/src/lib/models/PokedexExportIntegration.ts b/src/lib/models/PokedexExportIntegration.ts index 531926f..7ada9ae 100644 --- a/src/lib/models/PokedexExportIntegration.ts +++ b/src/lib/models/PokedexExportIntegration.ts @@ -15,6 +15,8 @@ export interface PokedexExportIntegration { metadata: Record | null; lastExportedAt: string | null; lastError: string | null; + /** Set by a database trigger on every write, so it doubles as the row's version. */ + updatedAt: string | null; } export interface PokedexExportIntegrationDB { @@ -32,4 +34,5 @@ export interface PokedexExportIntegrationDB { metadata: Record | null; lastExportedAt: string | null; lastError: string | null; + updatedAt: string | null; } diff --git a/src/lib/repositories/PokedexExportIntegrationRepository.ts b/src/lib/repositories/PokedexExportIntegrationRepository.ts index be12526..4156fd8 100644 --- a/src/lib/repositories/PokedexExportIntegrationRepository.ts +++ b/src/lib/repositories/PokedexExportIntegrationRepository.ts @@ -30,7 +30,8 @@ class PokedexExportIntegrationRepository { accessTokenExpiresAt: db.accessTokenExpiresAt, metadata: db.metadata, lastExportedAt: db.lastExportedAt, - lastError: db.lastError + lastError: db.lastError, + updatedAt: db.updatedAt ?? null }; } @@ -136,6 +137,10 @@ class PokedexExportIntegrationRepository { } } + /** + * Returns whether a row was updated. With `ifUpdatedAt`, the write only applies if the row is + * unchanged since it was read, so a stale export can't overwrite credentials a reconnect saved. + */ async updateExportStatus( id: string, patch: { @@ -145,20 +150,25 @@ class PokedexExportIntegrationRepository { metadata?: Record | null; folderId?: string | null; path?: string | null; - } - ): Promise { - const query = this.supabase + }, + ifUpdatedAt?: string + ): Promise { + let query = this.supabase .from('pokedex_export_integrations') .update(patch) .eq('id', id) .eq('userId', this.userId); - const { error } = this.pokedexId - ? await query.eq('pokedexId', this.pokedexId) - : await query.is('pokedexId', null); + if (ifUpdatedAt) query = query.eq('updatedAt', ifUpdatedAt); + const scoped = this.pokedexId + ? query.eq('pokedexId', this.pokedexId) + : query.is('pokedexId', null); + const { data, error } = await scoped.select('id'); if (error) { console.error('Failed to update export integration status:', error); + return false; } + return (data?.length ?? 0) > 0; } } diff --git a/src/lib/services/PokedexExportService.ts b/src/lib/services/PokedexExportService.ts index 67deee8..72b2ba3 100644 --- a/src/lib/services/PokedexExportService.ts +++ b/src/lib/services/PokedexExportService.ts @@ -435,18 +435,25 @@ export async function exportPokedexIfConfigured( successes++; } catch (error) { const message = error instanceof Error ? error.message : String(error); - // A revoked grant never succeeds on retry, so pause this integration until the user reconnects. - const reconnectRequired = error instanceof ReconnectRequiredError; + let reconnectRequired = false; + if (error instanceof ReconnectRequiredError) { + // A revoked grant never succeeds on retry, so pause this integration until the user + // reconnects - but only if its row is unchanged since this export read it. A reconnect + // in the meantime saved new credentials, which this stale failure must not disable. + reconnectRequired = await scopedRepo.updateExportStatus( + integration._id, + { lastError: message, enabled: false }, + integration.updatedAt ?? undefined + ); + } else { + await scopedRepo.updateExportStatus(integration._id, { lastError: message }); + } failures.push({ integrationId: integration._id, provider: integration.provider, error: message, reconnectRequired }); - await scopedRepo.updateExportStatus(integration._id, { - lastError: message, - ...(reconnectRequired ? { enabled: false } : {}) - }); console.error('Failed to export pokedex:', integration.provider, message); } } diff --git a/src/lib/stores/backupStatus.ts b/src/lib/stores/backupStatus.ts index a3aa2df..5071186 100644 --- a/src/lib/stores/backupStatus.ts +++ b/src/lib/stores/backupStatus.ts @@ -14,25 +14,37 @@ export const backupsNeedingReconnect = writable([]); type IntegrationSummary = { provider: ExportProvider; enabled: boolean }; +// A response is only applied if no newer refresh has started and nothing has changed the status +// since it was requested, so a slow response can't overwrite newer or cleared state. +let refreshSequence = 0; +let mutationGeneration = 0; + export function setBackupStatus(integrations: IntegrationSummary[]): void { + mutationGeneration++; backupsNeedingReconnect.set(integrations.filter((i) => !i.enabled).map((i) => i.provider)); } export async function refreshBackupStatus(): Promise { if (typeof window === 'undefined' || !navigator.onLine) return; + const sequence = ++refreshSequence; + const generation = mutationGeneration; try { const response = await fetch('/api/export-integrations', { credentials: 'include' }); if (!response.ok) return; - setBackupStatus((await response.json()) as IntegrationSummary[]); + const integrations = (await response.json()) as IntegrationSummary[]; + if (sequence !== refreshSequence || generation !== mutationGeneration) return; + setBackupStatus(integrations); } catch (error) { console.error('Unable to check backup status', error); } } export function markReconnectNeeded(providers: ExportProvider[]): void { + mutationGeneration++; backupsNeedingReconnect.update((current) => [...new Set([...current, ...providers])]); } export function clearBackupStatus(): void { + mutationGeneration++; backupsNeedingReconnect.set([]); } diff --git a/tests/unit/backupStatus.test.ts b/tests/unit/backupStatus.test.ts index 1bdd3ce..1161fc0 100644 --- a/tests/unit/backupStatus.test.ts +++ b/tests/unit/backupStatus.test.ts @@ -104,6 +104,64 @@ describe('refreshBackupStatus', () => { expect(get(backupsNeedingReconnect)).toEqual(['google_drive']); }); + function deferredResponse() { + let resolve!: (response: Response) => void; + const promise = new Promise((r) => (resolve = r)); + return { promise, resolve }; + } + + const json = (body: unknown) => new Response(JSON.stringify(body)); + + it('ignores a response overtaken by a newer refresh', async () => { + const slow = deferredResponse(); + fetchMock + .mockReturnValueOnce(slow.promise) + .mockResolvedValueOnce(json([{ provider: 'google_drive', enabled: true }])); + + const first = refreshBackupStatus(); + await refreshBackupStatus(); + slow.resolve(json([{ provider: 'google_drive', enabled: false }])); + await first; + + expect(get(backupsNeedingReconnect)).toEqual([]); + }); + + it('ignores a response that arrives after the status was flagged directly', async () => { + const slow = deferredResponse(); + fetchMock.mockReturnValueOnce(slow.promise); + + const pending = refreshBackupStatus(); + markReconnectNeeded(['dropbox']); + slow.resolve(json([{ provider: 'dropbox', enabled: true }])); + await pending; + + expect(get(backupsNeedingReconnect)).toEqual(['dropbox']); + }); + + it('ignores a response that arrives after the status was cleared', async () => { + const slow = deferredResponse(); + fetchMock.mockReturnValueOnce(slow.promise); + + const pending = refreshBackupStatus(); + clearBackupStatus(); + slow.resolve(json([{ provider: 'google_drive', enabled: false }])); + await pending; + + expect(get(backupsNeedingReconnect)).toEqual([]); + }); + + it('ignores a response that arrives after the status was set from another source', async () => { + const slow = deferredResponse(); + fetchMock.mockReturnValueOnce(slow.promise); + + const pending = refreshBackupStatus(); + setBackupStatus([{ provider: 'google_drive', enabled: true }]); + slow.resolve(json([{ provider: 'google_drive', enabled: false }])); + await pending; + + expect(get(backupsNeedingReconnect)).toEqual([]); + }); + it('keeps the last known status when the request fails', async () => { const consoleError = vi.spyOn(console, 'error').mockImplementation(() => undefined); markReconnectNeeded(['dropbox']); diff --git a/tests/unit/pokedexExportIntegrationRepository.test.ts b/tests/unit/pokedexExportIntegrationRepository.test.ts new file mode 100644 index 0000000..6d4d590 --- /dev/null +++ b/tests/unit/pokedexExportIntegrationRepository.test.ts @@ -0,0 +1,257 @@ +import { describe, expect, it, vi } from 'vitest'; +import type { SupabaseClient } from '@supabase/supabase-js'; +import PokedexExportIntegrationRepository from '$lib/repositories/PokedexExportIntegrationRepository'; + +type Result = { data: unknown; error: unknown }; + +/** A chainable stand-in for the Supabase query builder that records each call made on it. */ +function fakeSupabase(result: Result) { + const calls: unknown[][] = []; + const builder: Record = {}; + for (const method of ['select', 'update', 'upsert', 'single', 'eq', 'is', 'or']) { + builder[method] = (...args: unknown[]) => { + calls.push([method, ...args]); + return builder; + }; + } + // Awaiting the builder runs the query, as it does in supabase-js. + builder.then = (resolve: (value: Result) => unknown, reject?: (reason: unknown) => unknown) => + Promise.resolve(result).then(resolve, reject); + const from = vi.fn(() => builder); + return { supabase: { from } as unknown as SupabaseClient, calls, from }; +} + +const VERSION = '2026-09-14T12:00:00.123456+00:00'; +const WRITTEN: Result = { data: [{ id: 'int-1' }], error: null }; + +describe('PokedexExportIntegrationRepository.updateExportStatus', () => { + it('writes only while the row still has the version the caller read', async () => { + const { supabase, calls, from } = fakeSupabase(WRITTEN); + const repo = new PokedexExportIntegrationRepository(supabase, 'user-1', null); + + const applied = await repo.updateExportStatus('int-1', { enabled: false }, VERSION); + + expect(applied).toBe(true); + expect(from).toHaveBeenCalledWith('pokedex_export_integrations'); + expect(calls).toEqual([ + ['update', { enabled: false }], + ['eq', 'id', 'int-1'], + ['eq', 'userId', 'user-1'], + ['eq', 'updatedAt', VERSION], + ['is', 'pokedexId', null], + ['select', 'id'] + ]); + }); + + it('writes unconditionally without a version, scoped to the Pokédex', async () => { + const { supabase, calls } = fakeSupabase(WRITTEN); + const repo = new PokedexExportIntegrationRepository(supabase, 'user-1', 'dex-1'); + + expect(await repo.updateExportStatus('int-1', { lastError: null })).toBe(true); + expect(calls).toEqual([ + ['update', { lastError: null }], + ['eq', 'id', 'int-1'], + ['eq', 'userId', 'user-1'], + ['eq', 'pokedexId', 'dex-1'], + ['select', 'id'] + ]); + }); + + it.each([ + ['no row matched, e.g. after a reconnect changed it', { data: [], error: null }], + ['the response carries no rows', { data: null, error: null }] + ])('reports nothing written when %s', async (_label, result) => { + const { supabase } = fakeSupabase(result); + const repo = new PokedexExportIntegrationRepository(supabase, 'user-1', null); + + expect(await repo.updateExportStatus('int-1', { enabled: false }, VERSION)).toBe(false); + }); + + it('logs and reports nothing written when the update fails', async () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => undefined); + const { supabase } = fakeSupabase({ data: null, error: { message: 'boom' } }); + const repo = new PokedexExportIntegrationRepository(supabase, 'user-1', null); + + expect(await repo.updateExportStatus('int-1', { enabled: false })).toBe(false); + expect(consoleError).toHaveBeenCalled(); + consoleError.mockRestore(); + }); +}); + +describe('PokedexExportIntegrationRepository.listAll', () => { + it('maps each row version, defaulting a missing one to null', async () => { + const row = { + userId: 'user-1', + pokedexId: null, + provider: 'google_drive', + enabled: true, + fileName: null, + folderId: null, + path: null, + accessToken: 'access', + refreshToken: 'refresh', + accessTokenExpiresAt: null, + metadata: null, + lastExportedAt: null, + lastError: null + }; + const { supabase } = fakeSupabase({ + data: [ + { ...row, id: 'with-version', updatedAt: VERSION }, + { ...row, id: 'without-version' } + ], + error: null + }); + const repo = new PokedexExportIntegrationRepository(supabase, 'user-1', null); + + const [withVersion, withoutVersion] = await repo.listAll(); + + expect(withVersion).toMatchObject({ _id: 'with-version', updatedAt: VERSION }); + expect(withoutVersion).toMatchObject({ _id: 'without-version', updatedAt: null }); + }); +}); + +function dbRow(id: string) { + return { + id, + userId: 'user-1', + pokedexId: null, + provider: 'dropbox', + enabled: true, + fileName: null, + folderId: null, + path: '/Backups', + accessToken: 'access', + refreshToken: 'refresh', + accessTokenExpiresAt: null, + metadata: null, + lastExportedAt: null, + lastError: null, + updatedAt: VERSION + }; +} + +describe('PokedexExportIntegrationRepository queries', () => { + it('lists enabled integrations scoped to one Pokédex', async () => { + const { supabase, calls } = fakeSupabase({ data: [dbRow('int-1')], error: null }); + const repo = new PokedexExportIntegrationRepository(supabase, 'user-1', 'dex-1'); + + const [integration] = await repo.listEnabled(); + + expect(integration).toMatchObject({ _id: 'int-1', provider: 'dropbox', path: '/Backups' }); + expect(calls).toEqual([ + ['select', '*'], + ['eq', 'userId', 'user-1'], + ['eq', 'pokedexId', 'dex-1'], + ['eq', 'enabled', true] + ]); + }); + + it.each([ + ['dex-1', ['or', 'pokedexId.eq.dex-1,pokedexId.is.null']], + [null, ['is', 'pokedexId', null]] + ])('lists enabled integrations for Pokédex %j or the whole account', async (pokedexId, scope) => { + const { supabase, calls } = fakeSupabase({ data: [dbRow('int-1')], error: null }); + const repo = new PokedexExportIntegrationRepository(supabase, 'user-1', pokedexId); + + expect(await repo.listEnabledForPokedexOrUser()).toHaveLength(1); + expect(calls).toEqual([ + ['select', '*'], + ['eq', 'userId', 'user-1'], + ['eq', 'enabled', true], + scope + ]); + }); + + it.each(['listAll', 'listEnabled', 'listEnabledForPokedexOrUser'] as const)( + '%s returns nothing when no rows come back', + async (method) => { + const { supabase } = fakeSupabase({ data: null, error: null }); + const repo = new PokedexExportIntegrationRepository(supabase, 'user-1', null); + + expect(await repo[method]()).toEqual([]); + } + ); + + it.each(['listAll', 'listEnabled', 'listEnabledForPokedexOrUser'] as const)( + '%s throws when the query fails', + async (method) => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => undefined); + const { supabase } = fakeSupabase({ data: null, error: { message: 'boom' } }); + const repo = new PokedexExportIntegrationRepository(supabase, 'user-1', null); + + await expect(repo[method]()).rejects.toThrow('Failed to load export integrations: boom'); + consoleError.mockRestore(); + } + ); +}); + +describe('PokedexExportIntegrationRepository.upsert', () => { + it('saves one account-wide integration per provider', async () => { + const { supabase, calls } = fakeSupabase({ data: dbRow('int-1'), error: null }); + const repo = new PokedexExportIntegrationRepository(supabase, 'user-1', 'dex-1'); + + const saved = await repo.upsert({ provider: 'dropbox', enabled: true, lastError: null }); + + expect(saved).toMatchObject({ _id: 'int-1', updatedAt: VERSION }); + expect(calls).toEqual([ + [ + 'upsert', + { + userId: 'user-1', + pokedexId: null, + provider: 'dropbox', + enabled: true, + lastError: null + }, + { onConflict: 'userId,provider' } + ], + ['select'], + ['single'] + ]); + }); + + it.each([ + ['the save fails', { data: null, error: { message: 'boom' } }, 'boom'], + ['nothing comes back', { data: null, error: null }, 'No result returned'] + ])('throws when %s', async (_label, result, message) => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => undefined); + const { supabase } = fakeSupabase(result); + const repo = new PokedexExportIntegrationRepository(supabase, 'user-1', null); + + await expect(repo.upsert({ provider: 'dropbox' })).rejects.toThrow( + `Failed to save export integration: ${message}` + ); + consoleError.mockRestore(); + }); +}); + +describe('PokedexExportIntegrationRepository.updateTokens', () => { + it.each([ + ['dex-1', ['eq', 'pokedexId', 'dex-1']], + [null, ['is', 'pokedexId', null]] + ])('stores refreshed tokens scoped to Pokédex %j', async (pokedexId, scope) => { + const { supabase, calls } = fakeSupabase({ data: null, error: null }); + const repo = new PokedexExportIntegrationRepository(supabase, 'user-1', pokedexId); + const patch = { accessToken: 'new-access', accessTokenExpiresAt: null }; + + await repo.updateTokens('int-1', patch); + + expect(calls).toEqual([ + ['update', patch], + ['eq', 'id', 'int-1'], + ['eq', 'userId', 'user-1'], + scope + ]); + }); + + it('logs instead of throwing when the token update fails', async () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => undefined); + const { supabase } = fakeSupabase({ data: null, error: { message: 'boom' } }); + const repo = new PokedexExportIntegrationRepository(supabase, 'user-1', null); + + await expect(repo.updateTokens('int-1', { accessToken: 'x' })).resolves.toBeUndefined(); + expect(consoleError).toHaveBeenCalled(); + consoleError.mockRestore(); + }); +}); diff --git a/tests/unit/pokedexExportIntegrations.test.ts b/tests/unit/pokedexExportIntegrations.test.ts index 739a730..f0682b9 100644 --- a/tests/unit/pokedexExportIntegrations.test.ts +++ b/tests/unit/pokedexExportIntegrations.test.ts @@ -35,6 +35,8 @@ import { exportPokedexIfConfigured } from '$lib/services/PokedexExportService'; const supabase = {} as SupabaseClient; const EXPIRED = () => new Date(Date.now() - 60_000).toISOString(); const FRESH = () => new Date(Date.now() + 3_600_000).toISOString(); +/** The row version an export read; a guarded write only applies while it still matches. */ +const VERSION = '2026-09-14T12:00:00.123456+00:00'; function integration(overrides: Partial = {}): PokedexExportIntegration { return { @@ -53,6 +55,7 @@ function integration(overrides: Partial = {}): Pokedex metadata: null, lastExportedAt: null, lastError: null, + updatedAt: VERSION, ...overrides }; } @@ -105,6 +108,8 @@ beforeEach(() => { mocks.pokedex = DEX; mocks.integrations = []; mocks.updateExportStatus.mockReset(); + // The repository reports whether a row was written; by default every write applies. + mocks.updateExportStatus.mockResolvedValue(true); mocks.updateTokens.mockReset(); fetchMock.mockReset(); vi.stubGlobal('fetch', fetchMock); @@ -138,10 +143,11 @@ describe('exportPokedexIfConfigured when a provider revokes access', () => { reconnectRequired: true } ]); - expect(mocks.updateExportStatus).toHaveBeenCalledWith(id, { - lastError: expect.stringMatching(message), - enabled: false - }); + expect(mocks.updateExportStatus).toHaveBeenCalledWith( + id, + { lastError: expect.stringMatching(message), enabled: false }, + VERSION + ); expect(uploadCalls()).toHaveLength(0); expect(mocks.updateTokens).not.toHaveBeenCalled(); }); @@ -152,13 +158,32 @@ describe('exportPokedexIfConfigured when a provider revokes access', () => { const result = await exportPokedexIfConfigured(supabase, 'user-1', 'dex-1'); expect(result.failed[0]).toMatchObject({ reconnectRequired: true }); - expect(mocks.updateExportStatus).toHaveBeenCalledWith('google-1', { - lastError: expect.stringMatching(/Reconnect Google Drive/), - enabled: false - }); + expect(mocks.updateExportStatus).toHaveBeenCalledWith( + 'google-1', + { lastError: expect.stringMatching(/Reconnect Google Drive/), enabled: false }, + VERSION + ); expect(fetchMock).not.toHaveBeenCalled(); }); + it('leaves a backup reconnected during the export enabled and unflagged', async () => { + mocks.integrations = [integration()]; + stubProvider(REVOKED); + // The guarded pause matches no row: a reconnect changed it after this export read it. + mocks.updateExportStatus.mockResolvedValue(false); + + const result = await exportPokedexIfConfigured(supabase, 'user-1', 'dex-1'); + + expect(mocks.updateExportStatus).toHaveBeenCalledTimes(1); + expect(mocks.updateExportStatus).toHaveBeenCalledWith( + 'google-1', + { lastError: expect.stringMatching(/Reconnect Google Drive/), enabled: false }, + VERSION + ); + // The client must not tell the user to reconnect a connection that is already fresh. + expect(result.failed[0]).toMatchObject({ reconnectRequired: false }); + }); + it.each([ ['a server error', { status: 500, body: 'upstream down' }], ['a different OAuth error', { status: 400, body: { error: 'invalid_client' } }] diff --git a/vitest.config.mts b/vitest.config.mts index ccc7732..80b35c5 100644 --- a/vitest.config.mts +++ b/vitest.config.mts @@ -22,10 +22,10 @@ export default defineConfig({ exclude: ['src/lib/models/**', 'src/lib/stores/**', 'src/lib/actions/**'], // Set to the measured baseline. Ratchet these up as coverage grows; never down. thresholds: { - statements: 53.84, - functions: 82.89, - lines: 53.84, - branches: 86.93 + statements: 59.73, + functions: 85.88, + lines: 59.73, + branches: 88.35 } } }