diff --git a/.changelog/NEXT.md b/.changelog/NEXT.md index 9c114103..2ffcff4f 100644 --- a/.changelog/NEXT.md +++ b/.changelog/NEXT.md @@ -50,6 +50,7 @@ ## Fixed +- **[issue-163] Provider operation failures stay actionable** — Browser connection and provider scraper failures now surface as recoverable errors instead of being reported as a logged-out session or an empty tree list. - **[issue-162] Provider refresh page cleanup** — Provider comparison refreshes now close their temporary browser page even when Ancestry navigation or scraping fails, preventing failed retries from accumulating pages in the shared browser. - Nominatim geocoding requests now time out after 15 seconds and are cancelled when their map-stream client disconnects, preventing stalled upstream sockets from blocking the shared geocoding queue. - AI discovery now rejects unsafe batch settings, limits background runs to one per family database, and lets an active run be cancelled without leaving provider work behind. diff --git a/client/src/pages/GenealogyProviders.tsx b/client/src/pages/GenealogyProviders.tsx index b433c9ec..1a1ff8e5 100644 --- a/client/src/pages/GenealogyProviders.tsx +++ b/client/src/pages/GenealogyProviders.tsx @@ -15,7 +15,8 @@ import { ToggleLeft, ToggleRight, ExternalLink, - Monitor + Monitor, + AlertCircle } from 'lucide-react'; import toast from 'react-hot-toast'; import { api, CredentialsStatus } from '../services/api'; @@ -50,6 +51,7 @@ export function GenealogyProvidersPage() { } = useBrowserConnection(); const [providers, setProviders] = useState([]); const [sessionStatus, setSessionStatus] = useState>({} as Record); + const [sessionErrors, setSessionErrors] = useState>>({}); const [credentialsStatus, setCredentialsStatus] = useState>({} as Record); const [loading, setLoading] = useState(true); const [checkingSession, setCheckingSession] = useState(null); @@ -114,11 +116,22 @@ export function GenealogyProvidersPage() { const handleCheckSession = async (provider: BuiltInProvider) => { setCheckingSession(provider); const status = await api.checkProviderSession(provider).catch(err => { + setSessionErrors(prev => ({ ...prev, [provider]: err.message })); + setSessionStatus(prev => { + const next = { ...prev }; + delete next[provider]; + return next; + }); toast.error(`Failed to check session: ${err.message}`); return null; }); if (status) { + setSessionErrors(prev => { + const next = { ...prev }; + delete next[provider]; + return next; + }); setSessionStatus(prev => ({ ...prev, [provider]: status })); if (status.loggedIn) { toast.success(`${provider}: Logged in${status.userName ? ` as ${status.userName}` : ''}`); @@ -308,6 +321,7 @@ export function GenealogyProvidersPage() { {providers.map(({ provider, displayName, loginUrl }) => { const colors = providerColors[provider]; const status = sessionStatus[provider]; + const sessionError = sessionErrors[provider]; const creds = credentialsStatus[provider]; const isCheckingThis = checkingSession === provider; const isOpeningLoginThis = openingLogin === provider; @@ -331,7 +345,12 @@ export function GenealogyProvidersPage() { {/* Session Status */}
- {status ? ( + {sessionError ? ( + + + Status unavailable + + ) : status ? ( status.loggedIn ? ( @@ -368,6 +387,29 @@ export function GenealogyProvidersPage() {
+ {sessionError && ( +
+

+ Could not verify this provider: {sessionError} +

+
+ + Check browser settings + + +
+
+ )} + {/* Login Options */}
{/* Login Buttons */} diff --git a/client/src/services/api.ts b/client/src/services/api.ts index b8274721..22bd43d2 100644 --- a/client/src/services/api.ts +++ b/client/src/services/api.ts @@ -24,6 +24,7 @@ import type { ExpandAncestryRequest, BuiltInProvider, ProviderSessionStatus, + ProviderTreeInfo, UserProviderConfig, ProviderComparison, ScrapedPersonData, @@ -598,7 +599,7 @@ export const api = { }), listProviderTrees: (provider: BuiltInProvider) => - fetchJson>(`/scrape-providers/${provider}/trees`), + fetchJson(`/scrape-providers/${provider}/trees`), setProviderDefaultTree: (provider: BuiltInProvider, treeId?: string) => fetchJson(`/scrape-providers/${provider}/default-tree`, { diff --git a/server/src/routes/provider.routes.ts b/server/src/routes/provider.routes.ts index e7d104a6..49019c91 100644 --- a/server/src/routes/provider.routes.ts +++ b/server/src/routes/provider.routes.ts @@ -108,24 +108,35 @@ router.post('/:provider/confirm-browser-login', (req: Request, res: Response) => router.post('/:provider/check-session', async (req: Request, res: Response) => { const { provider } = req.params as { provider: BuiltInProvider }; - const status = await providerService.checkSession(provider) - .catch(err => ({ - provider, - enabled: false, - loggedIn: false, - lastChecked: new Date().toISOString(), - error: err.message - })); - - res.json({ success: true, data: status }); + const result = await providerService.checkSession(provider); + if (!result.success) { + res.status(503).json({ + success: false, + error: result.error.message, + details: result.error + }); + return; + } + + res.json({ success: true, data: result.data }); }); /** * Check sessions for all enabled providers */ router.post('/check-all-sessions', asyncHandler(async (_req: Request, res: Response) => { - const statuses = await providerService.checkAllSessions(); - res.json({ success: true, data: statuses }); + const summary = await providerService.checkAllSessions(); + const failedProviders = Object.keys(summary.failures); + if (failedProviders.length > 0) { + res.status(503).json({ + success: false, + error: `Failed to check ${failedProviders.length} provider session${failedProviders.length === 1 ? '' : 's'}`, + details: summary + }); + return; + } + + res.json({ success: true, data: summary.statuses }); })); /** @@ -173,10 +184,17 @@ router.post('/:provider/login-google', async (req: Request, res: Response) => { router.get('/:provider/trees', async (req: Request, res: Response) => { const { provider } = req.params as { provider: BuiltInProvider }; - const trees = await providerService.discoverTrees(provider) - .catch(() => []); + const result = await providerService.discoverTrees(provider); + if (!result.success) { + res.status(503).json({ + success: false, + error: result.error.message, + details: result.error + }); + return; + } - res.json({ success: true, data: trees }); + res.json({ success: true, data: result.data }); }); /** diff --git a/server/src/services/provider.service.ts b/server/src/services/provider.service.ts index a0409652..b7f72326 100644 --- a/server/src/services/provider.service.ts +++ b/server/src/services/provider.service.ts @@ -6,6 +6,9 @@ import type { UserProviderConfig, ProviderSessionStatus, ProviderTreeInfo, + ProviderOperation, + ProviderOperationResult, + ProviderSessionCheckSummary, EnsureAuthResult } from '@fsf/shared'; import { browserService, isFamilySearchAuthUrl } from './browser.service.js'; @@ -16,6 +19,33 @@ import { DATA_DIR } from '../utils/paths.js'; const CONFIG_FILE = path.join(DATA_DIR, 'provider-config.json'); +const getErrorMessage = (error: unknown): string => + error instanceof Error ? error.message : String(error); + +const runProviderOperation = async ( + provider: BuiltInProvider, + operation: ProviderOperation, + action: () => Promise +): Promise> => action() + .then(data => ({ success: true as const, data })) + .catch(error => { + const message = getErrorMessage(error); + logger.error( + 'provider-operation', + `provider=${provider} operation=${operation} error=${message}` + ); + + return { + success: false as const, + error: { + code: 'PROVIDER_OPERATION_FAILED' as const, + provider, + operation, + message + } + }; + }); + /** * Create default configuration for all providers */ @@ -186,36 +216,36 @@ export const providerService = { /** * Check browser login status for a provider */ - async checkSession(provider: BuiltInProvider): Promise { - const config = this.getConfig(provider); - const scraper = getScraper(provider); - - const status: ProviderSessionStatus = { - provider, - enabled: config.enabled, - loggedIn: false, - lastChecked: new Date().toISOString() - }; + async checkSession(provider: BuiltInProvider): Promise> { + return runProviderOperation(provider, 'check-session', async () => { + const config = this.getConfig(provider); + const scraper = getScraper(provider); - // Ensure browser is connected - if (!browserService.isConnected()) { - await browserService.connect().catch(() => null); - } + const status: ProviderSessionStatus = { + provider, + enabled: config.enabled, + loggedIn: false, + lastChecked: new Date().toISOString() + }; - if (!browserService.isConnected()) { - return status; - } + if (!browserService.isConnected()) { + await browserService.connect(); + } - const page = await browserService.getWorkerPage(); + if (!browserService.isConnected()) { + throw new Error('Browser connection was not established'); + } - status.loggedIn = await scraper.checkLoginStatus(page).catch(() => false); + const page = await browserService.getWorkerPage(); + status.loggedIn = await scraper.checkLoginStatus(page); - if (status.loggedIn) { - const userInfo = await scraper.getLoggedInUser(page).catch(() => null); - status.userName = userInfo?.name; - } + if (status.loggedIn) { + const userInfo = await scraper.getLoggedInUser(page).catch(() => null); + status.userName = userInfo?.name; + } - return status; + return status; + }); }, /** @@ -327,15 +357,21 @@ export const providerService = { /** * Check session status for all enabled providers */ - async checkAllSessions(): Promise> { - const registry = loadRegistry(); - const results: Record = {} as Record; + async checkAllSessions(): Promise { + const registry = this.getAllConfigs(); + const statuses: ProviderSessionCheckSummary['statuses'] = {}; + const failures: ProviderSessionCheckSummary['failures'] = {}; for (const provider of listProviders()) { if (registry.providers[provider].enabled) { - results[provider] = await this.checkSession(provider); + const result = await this.checkSession(provider); + if (result.success) { + statuses[provider] = result.data; + } else { + failures[provider] = result.error; + } } else { - results[provider] = { + statuses[provider] = { provider, enabled: false, loggedIn: false, @@ -344,23 +380,27 @@ export const providerService = { } } - return results; + return { statuses, failures }; }, /** * Discover available trees for a provider */ - async discoverTrees(provider: BuiltInProvider): Promise { - const scraper = getScraper(provider); + async discoverTrees(provider: BuiltInProvider): Promise> { + return runProviderOperation(provider, 'discover-trees', async () => { + const scraper = getScraper(provider); - if (!browserService.isConnected()) { - await browserService.connect(); - } + if (!browserService.isConnected()) { + await browserService.connect(); + } - const page = await browserService.getWorkerPage(); - const trees = await scraper.listTrees(page).catch(() => []); + if (!browserService.isConnected()) { + throw new Error('Browser connection was not established'); + } - return trees; + const page = await browserService.getWorkerPage(); + return scraper.listTrees(page); + }); }, /** diff --git a/shared/src/index.ts b/shared/src/index.ts index 45a8d0c0..e2298fae 100644 --- a/shared/src/index.ts +++ b/shared/src/index.ts @@ -189,6 +189,24 @@ export interface ProviderTreeInfo { rootPersonId?: string; } +export type ProviderOperation = 'check-session' | 'discover-trees'; + +export interface ProviderOperationFailure { + code: 'PROVIDER_OPERATION_FAILED'; + provider: BuiltInProvider; + operation: ProviderOperation; + message: string; +} + +export type ProviderOperationResult = + | { success: true; data: T } + | { success: false; error: ProviderOperationFailure }; + +export interface ProviderSessionCheckSummary { + statuses: Partial>; + failures: Partial>; +} + // Auto-login method type export type AutoLoginMethod = 'credentials' | 'google'; diff --git a/tests/integration/api/providers.spec.ts b/tests/integration/api/providers.spec.ts new file mode 100644 index 00000000..0d9d91ba --- /dev/null +++ b/tests/integration/api/providers.spec.ts @@ -0,0 +1,164 @@ +import express from 'express'; +import request from 'supertest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + providerService: { + checkSession: vi.fn(), + checkAllSessions: vi.fn(), + discoverTrees: vi.fn(), + }, +})); + +vi.mock('../../../server/src/services/provider.service.js', () => ({ + providerService: mocks.providerService, +})); + +vi.mock('../../../server/src/services/browser.service.js', () => ({ + browserService: {}, +})); + +vi.mock('../../../server/src/services/credentials.service.js', () => ({ + credentialsService: {}, +})); + +const { providerRouter } = await import('../../../server/src/routes/provider.routes.js'); + +const app = express(); +app.use(express.json()); +app.use('/api/scrape-providers', providerRouter); + +const failure = (operation: 'check-session' | 'discover-trees', message: string) => ({ + success: false as const, + error: { + code: 'PROVIDER_OPERATION_FAILED' as const, + provider: 'familysearch' as const, + operation, + message, + }, +}); + +describe('Provider routes', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('returns a successful logged-out session only when it was verified', async () => { + mocks.providerService.checkSession.mockResolvedValue({ + success: true, + data: { + provider: 'familysearch', + enabled: true, + loggedIn: false, + lastChecked: '2026-08-30T00:00:00.000Z', + }, + }); + + const response = await request(app) + .post('/api/scrape-providers/familysearch/check-session') + .expect(200); + + expect(response.body).toEqual({ + success: true, + data: { + provider: 'familysearch', + enabled: true, + loggedIn: false, + lastChecked: '2026-08-30T00:00:00.000Z', + }, + }); + }); + + it('returns a non-2xx error envelope for an operational session failure', async () => { + mocks.providerService.checkSession.mockResolvedValue( + failure('check-session', 'CDP refused connection') + ); + + const response = await request(app) + .post('/api/scrape-providers/familysearch/check-session') + .expect(503); + + expect(response.body).toEqual({ + success: false, + error: 'CDP refused connection', + details: { + code: 'PROVIDER_OPERATION_FAILED', + provider: 'familysearch', + operation: 'check-session', + message: 'CDP refused connection', + }, + }); + }); + + it('returns successful statuses and structured failures from aggregate checks', async () => { + mocks.providerService.checkAllSessions.mockResolvedValue({ + statuses: { + ancestry: { + provider: 'ancestry', + enabled: true, + loggedIn: true, + }, + }, + failures: { + familysearch: failure('check-session', 'CDP refused connection').error, + }, + }); + + const response = await request(app) + .post('/api/scrape-providers/check-all-sessions') + .expect(503); + + expect(response.body).toEqual({ + success: false, + error: 'Failed to check 1 provider session', + details: { + statuses: { + ancestry: { + provider: 'ancestry', + enabled: true, + loggedIn: true, + }, + }, + failures: { + familysearch: { + code: 'PROVIDER_OPERATION_FAILED', + provider: 'familysearch', + operation: 'check-session', + message: 'CDP refused connection', + }, + }, + }, + }); + }); + + it('returns an empty tree list only when discovery succeeded', async () => { + mocks.providerService.discoverTrees.mockResolvedValue({ success: true, data: [] }); + + const response = await request(app) + .get('/api/scrape-providers/familysearch/trees') + .expect(200); + + expect(response.body).toEqual({ success: true, data: [] }); + }); + + it('returns a non-2xx error envelope when tree discovery fails', async () => { + mocks.providerService.discoverTrees.mockResolvedValue( + failure('discover-trees', 'Tree scraper failed') + ); + + const response = await request(app) + .get('/api/scrape-providers/familysearch/trees') + .expect(503); + + expect(response.body).toEqual({ + success: false, + error: 'Tree scraper failed', + details: { + code: 'PROVIDER_OPERATION_FAILED', + provider: 'familysearch', + operation: 'discover-trees', + message: 'Tree scraper failed', + }, + }); + }); +}); diff --git a/tests/unit/services/providerService.spec.ts b/tests/unit/services/providerService.spec.ts new file mode 100644 index 00000000..1baa9897 --- /dev/null +++ b/tests/unit/services/providerService.spec.ts @@ -0,0 +1,168 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + browserService: { + isConnected: vi.fn(), + connect: vi.fn(), + getWorkerPage: vi.fn(), + }, + scraper: { + checkLoginStatus: vi.fn(), + getLoggedInUser: vi.fn(), + listTrees: vi.fn(), + }, + logger: { + error: vi.fn(), + }, +})); + +vi.mock('../../../server/src/services/browser.service.js', () => ({ + browserService: mocks.browserService, + isFamilySearchAuthUrl: vi.fn(), +})); + +vi.mock('../../../server/src/services/credentials.service.js', () => ({ + credentialsService: {}, +})); + +vi.mock('../../../server/src/services/scrapers/index.js', () => ({ + getScraper: vi.fn(() => mocks.scraper), + getProviderInfo: vi.fn(), + listProviders: vi.fn(() => ['familysearch']), + PROVIDER_DEFAULTS: {}, +})); + +vi.mock('../../../server/src/lib/logger.js', () => ({ + logger: mocks.logger, +})); + +const { providerService } = await import('../../../server/src/services/provider.service.js'); + +describe('providerService operational failures', () => { + beforeEach(() => { + vi.restoreAllMocks(); + vi.clearAllMocks(); + + vi.spyOn(providerService, 'getConfig').mockReturnValue({ + provider: 'familysearch', + enabled: true, + rateLimit: { minDelayMs: 1000, maxDelayMs: 2000 }, + browserScrapeEnabled: true, + browserLoggedIn: false, + }); + vi.spyOn(providerService, 'getAllConfigs').mockReturnValue({ + providers: { + familysearch: { + provider: 'familysearch', + enabled: true, + rateLimit: { minDelayMs: 1000, maxDelayMs: 2000 }, + browserScrapeEnabled: true, + browserLoggedIn: false, + }, + }, + lastUpdated: '2026-08-30T00:00:00.000Z', + } as ReturnType); + mocks.browserService.isConnected.mockReturnValue(true); + mocks.browserService.getWorkerPage.mockResolvedValue({}); + mocks.scraper.checkLoginStatus.mockResolvedValue(false); + mocks.scraper.getLoggedInUser.mockResolvedValue(null); + mocks.scraper.listTrees.mockResolvedValue([]); + }); + + it('returns a failure result and logs context when the CDP connection rejects', async () => { + mocks.browserService.isConnected.mockReturnValue(false); + mocks.browserService.connect.mockRejectedValue(new Error('CDP refused connection')); + + const result = await providerService.checkSession('familysearch'); + + expect(result).toEqual({ + success: false, + error: { + code: 'PROVIDER_OPERATION_FAILED', + provider: 'familysearch', + operation: 'check-session', + message: 'CDP refused connection', + }, + }); + expect(mocks.logger.error).toHaveBeenCalledWith( + 'provider-operation', + 'provider=familysearch operation=check-session error=CDP refused connection' + ); + }); + + it('keeps a verified logged-out session as a successful negative result', async () => { + const result = await providerService.checkSession('familysearch'); + + expect(result.success).toBe(true); + if (result.success) { + expect(result.data).toMatchObject({ + provider: 'familysearch', + enabled: true, + loggedIn: false, + }); + } + expect(mocks.logger.error).not.toHaveBeenCalled(); + }); + + it('returns a failure result when the provider login-status check rejects', async () => { + mocks.scraper.checkLoginStatus.mockRejectedValue(new Error('Login markup changed')); + + const result = await providerService.checkSession('familysearch'); + + expect(result).toMatchObject({ + success: false, + error: { + provider: 'familysearch', + operation: 'check-session', + message: 'Login markup changed', + }, + }); + expect(mocks.logger.error).toHaveBeenCalledWith( + 'provider-operation', + 'provider=familysearch operation=check-session error=Login markup changed' + ); + }); + + it('preserves successful statuses and structured failures in aggregate checks', async () => { + mocks.scraper.checkLoginStatus.mockRejectedValue(new Error('Login markup changed')); + + const summary = await providerService.checkAllSessions(); + + expect(summary.statuses).toEqual({}); + expect(summary.failures).toEqual({ + familysearch: { + code: 'PROVIDER_OPERATION_FAILED', + provider: 'familysearch', + operation: 'check-session', + message: 'Login markup changed', + }, + }); + }); + + it('keeps a verified empty tree list as a successful negative result', async () => { + const result = await providerService.discoverTrees('familysearch'); + + expect(result).toEqual({ success: true, data: [] }); + expect(mocks.logger.error).not.toHaveBeenCalled(); + }); + + it('returns a failure result and logs context when tree discovery rejects', async () => { + mocks.scraper.listTrees.mockRejectedValue(new Error('Tree scraper failed')); + + const result = await providerService.discoverTrees('familysearch'); + + expect(result).toEqual({ + success: false, + error: { + code: 'PROVIDER_OPERATION_FAILED', + provider: 'familysearch', + operation: 'discover-trees', + message: 'Tree scraper failed', + }, + }); + expect(mocks.logger.error).toHaveBeenCalledWith( + 'provider-operation', + 'provider=familysearch operation=discover-trees error=Tree scraper failed' + ); + }); +});