From 7786d460789ad8a0c4575f3f8ff85f46076b024c Mon Sep 17 00:00:00 2001 From: Josh Creek <8179928+jcreek@users.noreply.github.com> Date: Mon, 14 Sep 2026 19:03:29 +0100 Subject: [PATCH] fix(backup): don't let a stale export pause a reconnected backup - An export now pauses a revoked integration only if its row is unchanged since the export read it, using the trigger-maintained updatedAt column as the row version. If the user reconnected in the meantime, the stale failure no longer disables the fresh credentials or asks the user to reconnect again. updateExportStatus now reports whether a row was written. - The backup status store ignores a response overtaken by a newer refresh, or by the status being flagged, cleared or set directly, so a slow response can't overwrite newer state. - Unit tests cover the whole integration repository, the guarded pause and stale status responses. Coverage thresholds are raised to the new baseline. --- src/lib/models/PokedexExportIntegration.ts | 3 + .../PokedexExportIntegrationRepository.ts | 24 +- src/lib/services/PokedexExportService.ts | 19 +- src/lib/stores/backupStatus.ts | 14 +- tests/unit/backupStatus.test.ts | 58 ++++ ...pokedexExportIntegrationRepository.test.ts | 257 ++++++++++++++++++ tests/unit/pokedexExportIntegrations.test.ts | 41 ++- vitest.config.mts | 8 +- 8 files changed, 398 insertions(+), 26 deletions(-) create mode 100644 tests/unit/pokedexExportIntegrationRepository.test.ts 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 } } }