From 7f4120f5dc0ce4f3aa6422c3675cf3b5e0818aee Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Sun, 30 Aug 2026 22:56:38 +0000 Subject: [PATCH 1/2] fix: always close comparison scrape pages (#162) --- .../multi-platform-comparison.service.ts | 26 +-- .../services/multiPlatformComparison.spec.ts | 185 ++++++++++++++++++ 2 files changed, 200 insertions(+), 11 deletions(-) create mode 100644 tests/unit/services/multiPlatformComparison.spec.ts diff --git a/server/src/services/multi-platform-comparison.service.ts b/server/src/services/multi-platform-comparison.service.ts index 23af51dd..3fdcf46f 100644 --- a/server/src/services/multi-platform-comparison.service.ts +++ b/server/src/services/multi-platform-comparison.service.ts @@ -733,19 +733,23 @@ export const multiPlatformComparisonService = { const personUrl = platformRef.url || `https://www.${provider}.com`; logger.browser('compare', `Scraping ${provider} from URL: ${personUrl}`); const page = await browserService.createPage(); + let scrapedData: ScrapedPersonData | null = null; - if (provider === 'ancestry' && platformRef.url) { - await page.goto(platformRef.url, { waitUntil: 'domcontentloaded' }); - await page.waitForTimeout(2000); - } - - const scrapedData = await scraper.scrapePersonById(page, externalId).catch(err => { - logger.error('compare', `Failed to scrape ${provider}/${externalId}: ${err.message}`); - return null; - }); + try { + if (provider === 'ancestry' && platformRef.url) { + await page.goto(platformRef.url, { waitUntil: 'domcontentloaded' }); + await page.waitForTimeout(2000); + } - // Close the page to free resources - await page.close().catch(() => {}); + scrapedData = await scraper.scrapePersonById(page, externalId).catch(err => { + logger.error('compare', `Failed to scrape ${provider}/${externalId}: ${err.message}`); + return null; + }); + } finally { + await page.close().catch(err => { + logger.warn('compare', `Failed to close scrape page for ${provider}/${externalId}: ${err.message}`); + }); + } if (!scrapedData) { return null; diff --git a/tests/unit/services/multiPlatformComparison.spec.ts b/tests/unit/services/multiPlatformComparison.spec.ts new file mode 100644 index 00000000..3c48198b --- /dev/null +++ b/tests/unit/services/multiPlatformComparison.spec.ts @@ -0,0 +1,185 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + page: { + goto: vi.fn(), + waitForTimeout: vi.fn(), + close: vi.fn(), + }, + scraper: { + scrapePersonById: vi.fn(), + }, + browserService: { + isConnected: vi.fn(), + createPage: vi.fn(), + }, + augmentationService: { + getAugmentation: vi.fn(), + saveAugmentation: vi.fn(), + addPlatform: vi.fn(), + }, + idMappingService: { + resolveId: vi.fn(), + getExternalId: vi.fn(), + registerExternalId: vi.fn(), + }, + databaseService: { + getPerson: vi.fn(), + }, + logger: { + browser: vi.fn(), + data: vi.fn(), + error: vi.fn(), + warn: vi.fn(), + }, + fs: { + existsSync: vi.fn(), + readFileSync: vi.fn(), + statSync: vi.fn(), + writeFileSync: vi.fn(), + unlinkSync: vi.fn(), + }, + downloadImage: vi.fn(), + ensureDir: vi.fn(), +})); + +vi.mock('fs', () => ({ default: mocks.fs })); + +vi.mock('../../../server/src/services/browser.service.js', () => ({ + browserService: mocks.browserService, +})); + +vi.mock('../../../server/src/services/scrapers/index.js', () => ({ + getScraper: vi.fn(() => mocks.scraper), +})); + +vi.mock('../../../server/src/services/augmentation.service.js', () => ({ + augmentationService: mocks.augmentationService, +})); + +vi.mock('../../../server/src/services/id-mapping.service.js', () => ({ + idMappingService: mocks.idMappingService, +})); + +vi.mock('../../../server/src/services/database.service.js', () => ({ + databaseService: mocks.databaseService, +})); + +vi.mock('../../../server/src/db/sqlite.service.js', () => ({ + sqliteService: {}, +})); + +vi.mock('../../../server/src/services/familysearch-refresh.service.js', () => ({ + familySearchRefreshService: {}, +})); + +vi.mock('../../../server/src/lib/familysearch/index.js', () => ({ + json2person: vi.fn(), +})); + +vi.mock('../../../server/src/lib/logger.js', () => ({ + logger: mocks.logger, +})); + +vi.mock('../../../server/src/services/local-override.service.js', () => ({ + localOverrideService: {}, +})); + +vi.mock('../../../server/src/utils/applyOverrides.js', () => ({ + applyLocalOverrides: vi.fn(), +})); + +vi.mock('../../../server/src/utils/paths.js', () => ({ + PHOTOS_DIR: '/tmp/sparsetree-test-photos', + PROVIDER_CACHE_DIR: '/tmp/sparsetree-test-provider-cache', + ensureDir: mocks.ensureDir, +})); + +vi.mock('../../../server/src/utils/downloadImage.js', () => ({ + downloadImage: mocks.downloadImage, +})); + +vi.mock('../../../server/src/utils/providerCache.js', () => ({ + getPhotoSuffix: vi.fn(() => '-ancestry'), + getCachedProviderData: vi.fn(() => null), +})); + +vi.mock('../../../server/src/utils/normalizePhotoUrl.js', () => ({ + normalizePhotoUrl: vi.fn((url: string) => url), +})); + +const { multiPlatformComparisonService } = await import( + '../../../server/src/services/multi-platform-comparison.service.js' +); + +const getProviderData = () => + multiPlatformComparisonService.getProviderData('person-1', 'ancestry', true, 'db-1'); + +function expectNoPartialMutation(): void { + expect(mocks.fs.writeFileSync).not.toHaveBeenCalled(); + expect(mocks.fs.unlinkSync).not.toHaveBeenCalled(); + expect(mocks.downloadImage).not.toHaveBeenCalled(); + expect(mocks.augmentationService.saveAugmentation).not.toHaveBeenCalled(); + expect(mocks.augmentationService.addPlatform).not.toHaveBeenCalled(); + expect(mocks.idMappingService.registerExternalId).not.toHaveBeenCalled(); +} + +describe('multiPlatformComparisonService.getProviderData', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.browserService.isConnected.mockReturnValue(true); + mocks.browserService.createPage.mockResolvedValue(mocks.page); + mocks.augmentationService.getAugmentation.mockReturnValue({ + personId: 'person-1', + platforms: [ + { + platform: 'ancestry', + externalId: 'ancestry-1', + url: 'https://www.ancestry.com/family-tree/person/tree/123/person/456/facts', + }, + ], + photos: [], + }); + mocks.idMappingService.resolveId.mockReturnValue(null); + mocks.page.goto.mockResolvedValue(undefined); + mocks.page.waitForTimeout.mockResolvedValue(undefined); + mocks.page.close.mockResolvedValue(undefined); + mocks.scraper.scrapePersonById.mockResolvedValue({ + externalId: 'ancestry-1', + provider: 'ancestry', + name: 'Example Person', + scrapedAt: '2026-08-30T00:00:00.000Z', + }); + }); + + it('closes the page and preserves the navigation error when Ancestry setup fails', async () => { + const navigationError = new Error('navigation unavailable'); + mocks.page.goto.mockRejectedValue(navigationError); + mocks.page.close.mockRejectedValue(new Error('browser disconnected')); + + await expect(getProviderData()).rejects.toBe(navigationError); + + expect(mocks.scraper.scrapePersonById).not.toHaveBeenCalled(); + expect(mocks.page.close).toHaveBeenCalledTimes(1); + expect(mocks.logger.warn).toHaveBeenCalledWith( + 'compare', + 'Failed to close scrape page for ancestry/ancestry-1: browser disconnected' + ); + expectNoPartialMutation(); + }); + + it('closes the page and returns null when the scraper rejects', async () => { + mocks.scraper.scrapePersonById.mockRejectedValue(new Error('scrape failed')); + + await expect(getProviderData()).resolves.toBeNull(); + + expect(mocks.page.goto).toHaveBeenCalledTimes(1); + expect(mocks.page.waitForTimeout).toHaveBeenCalledTimes(1); + expect(mocks.page.close).toHaveBeenCalledTimes(1); + expect(mocks.logger.error).toHaveBeenCalledWith( + 'compare', + 'Failed to scrape ancestry/ancestry-1: scrape failed' + ); + expectNoPartialMutation(); + }); +}); From 467047bc3c362d4d9892cefb3b53ae738d4f7f13 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Sun, 30 Aug 2026 22:56:50 +0000 Subject: [PATCH 2/2] docs: log issue #162 --- .changelog/NEXT.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.changelog/NEXT.md b/.changelog/NEXT.md index 64db6f7b..fb9f3b07 100644 --- a/.changelog/NEXT.md +++ b/.changelog/NEXT.md @@ -50,6 +50,7 @@ ## Fixed +- **[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. - Search results now keep their alphabetical ordering. The batch person-loader (`getPersonsBatch`) re-orders rows back to the requested order, fixing a regression where SQLite's `WHERE person_id IN (...)` returned rows in table order and silently discarded the search query's `ORDER BY display_name` (so the default, unsorted search view appeared randomly ordered). - Platform comparison now treats equivalent place spellings as matches: "Dallas, Texas, USA" vs "Dallas, Texas, United States" (and U.S.A. / United States of America / state abbreviations like TX vs Texas, UK vs United Kingdom, etc.) — no longer flagged as `different`. Place containment is now suffix-based, so "Texas" no longer falsely matches "Texarkana" - Platform comparison now treats equivalent date formats as matches (e.g., "1979-07-31" vs "31 JUL 1979")