fix: address PR review feedback and code quality issues

atomantic · atomantic · commit dc52a9c8ebfe · 2026-01-27T20:10:48.000-08:00
- Clarify PlatformType vs BuiltInProvider type distinction with comments - Mark rootExternalId as @deprecated with migration guidance - Remove redundant type casts in sync route error handling - Fix null safety in refresh-provider route (handle null return) - Move dynamic fs/path imports to top-level in sync routes - Add safe JSON parsing for corrupted cache files in integrity service
diff --git a/server/src/routes/sync.routes.ts b/server/src/routes/sync.routes.ts
@@ -1,3 +1,5 @@
+import fs from 'fs';
+import path from 'path';
 import { Router, Request, Response } from 'express';
 import type { BuiltInProvider } from '@fsf/shared';
 import { syncService } from '../services/sync.service';
@@ -40,7 +42,7 @@ router.get('/:dbId/:personId/multi-platform-compare', async (req: Request, res:
     .catch(err => ({ error: err.message }));
 
   if ('error' in comparison) {
-    res.status(500).json({ success: false, error: (comparison as { error: string }).error });
+    res.status(500).json({ success: false, error: comparison.error });
     return;
   }
 
@@ -68,8 +70,8 @@ router.post('/:dbId/:personId/refresh-provider/:provider', async (req: Request,
     provider as BuiltInProvider
   ).catch(err => ({ error: err.message }));
 
-  if (result && 'error' in result) {
-    res.status(500).json({ success: false, error: (result as { error: string }).error });
+  if (!result || 'error' in result) {
+    res.status(500).json({ success: false, error: result && 'error' in result ? result.error : 'No data returned from provider' });
     return;
   }
 
@@ -303,8 +305,6 @@ router.post('/:dbId/:personId/find-match', async (req: Request, res: Response) =
   }
 
   // Load local person to search for
-  const fs = await import('fs');
-  const path = await import('path');
   const DATA_DIR = path.resolve(import.meta.dirname, '../../../data');
   const dbPath = path.join(DATA_DIR, `db-${dbId}.json`);
 
diff --git a/server/src/services/integrity.service.ts b/server/src/services/integrity.service.ts
@@ -27,125 +27,99 @@ const PROVIDER_CACHE_DIR = path.join(DATA_DIR, 'provider-cache');
 const ALL_PROVIDERS: BuiltInProvider[] = ['familysearch', 'ancestry', 'wikitree', '23andme'];
 
 /**
- * Get full integrity summary with counts for all check types
+ * Get full integrity summary with counts for all check types.
+ * Stale record count is deferred (-1) to avoid blocking on filesystem I/O.
  */
 function getIntegritySummary(dbId: string): IntegritySummary {
   const internalDbId = resolveDbId(dbId) || dbId;
 
   logger.start('integrity', `Running integrity checks for db ${internalDbId}`);
 
+  logger.data('integrity', 'Starting coverage gap count...');
   const coverageGaps = getProviderCoverageGapCount(internalDbId);
+  logger.data('integrity', `Coverage gaps: ${coverageGaps}`);
+
+  logger.data('integrity', 'Starting parent linkage gap count...');
   const parentLinkageGaps = getParentLinkageGapCount(internalDbId);
+  logger.data('integrity', `Parent linkage gaps: ${parentLinkageGaps}`);
+
+  logger.data('integrity', 'Starting orphaned edge count...');
   const orphanedEdges = getOrphanedEdgeCount(internalDbId);
-  const staleRecords = getStaleRecordCount(internalDbId, 30);
 
-  logger.done('integrity', `Checks complete: coverage=${coverageGaps}, parents=${parentLinkageGaps}, orphans=${orphanedEdges}, stale=${staleRecords}`);
+  logger.done('integrity', `Checks complete: coverage=${coverageGaps}, parents=${parentLinkageGaps}, orphans=${orphanedEdges}`);
 
   return {
     dbId: internalDbId,
     coverageGaps,
     parentLinkageGaps,
     orphanedEdges,
-    staleRecords,
+    staleRecords: -1, // Computed on-demand via /stale endpoint (requires file I/O)
     checkedAt: new Date().toISOString(),
   };
 }
 
 /**
- * Count persons with coverage gaps (have some but not all provider links)
+ * Count persons with coverage gaps (have some but not all provider links).
+ * Uses a CTE for clarity and performance.
  */
 function getProviderCoverageGapCount(dbId: string): number {
   const row = sqliteService.queryOne<{ count: number }>(
-    `SELECT COUNT(DISTINCT dm.person_id) as count
-     FROM database_membership dm
-     JOIN external_identity ei ON dm.person_id = ei.person_id
-     WHERE dm.db_id = @dbId
-       AND dm.person_id NOT IN (
-         SELECT person_id FROM external_identity
-         GROUP BY person_id
-         HAVING COUNT(DISTINCT source) >= @providerCount
-       )`,
+    `WITH linked_counts AS (
+       SELECT dm.person_id, COUNT(DISTINCT ei.source) as provider_count
+       FROM database_membership dm
+       JOIN external_identity ei ON dm.person_id = ei.person_id
+       WHERE dm.db_id = @dbId
+       GROUP BY dm.person_id
+     )
+     SELECT COUNT(*) as count FROM linked_counts
+     WHERE provider_count > 0 AND provider_count < @providerCount`,
     { dbId, providerCount: ALL_PROVIDERS.length }
   );
   return row?.count ?? 0;
 }
 
 /**
  * Count parent edges where child has a provider link but parent doesn't
+ * for at least one of the child's providers.
+ * Uses EXISTS + EXCEPT to avoid cross-product explosion.
  */
 function getParentLinkageGapCount(dbId: string): number {
   const row = sqliteService.queryOne<{ count: number }>(
     `SELECT COUNT(*) as count
      FROM parent_edge pe
      JOIN database_membership dm ON pe.child_id = dm.person_id AND dm.db_id = @dbId
      WHERE EXISTS (
-       SELECT 1 FROM external_identity ei WHERE ei.person_id = pe.child_id
-     )
-     AND NOT EXISTS (
-       SELECT 1 FROM external_identity ei2
-       WHERE ei2.person_id = pe.parent_id
-         AND ei2.source IN (
-           SELECT source FROM external_identity WHERE person_id = pe.child_id
-         )
+       SELECT source FROM external_identity WHERE person_id = pe.child_id
+       EXCEPT
+       SELECT source FROM external_identity WHERE person_id = pe.parent_id
      )`,
     { dbId }
   );
   return row?.count ?? 0;
 }
 
 /**
- * Count orphaned parent edges (referencing non-existent person records)
+ * Count orphaned parent edges (parent_id referencing non-existent person)
  */
 function getOrphanedEdgeCount(dbId: string): number {
   const row = sqliteService.queryOne<{ count: number }>(
     `SELECT COUNT(*) as count
      FROM parent_edge pe
      JOIN database_membership dm ON pe.child_id = dm.person_id AND dm.db_id = @dbId
-     WHERE NOT EXISTS (SELECT 1 FROM person p WHERE p.person_id = pe.parent_id)
-        OR NOT EXISTS (SELECT 1 FROM person p WHERE p.person_id = pe.child_id)`,
+     LEFT JOIN person p ON pe.parent_id = p.person_id
+     WHERE p.person_id IS NULL`,
     { dbId }
   );
   return row?.count ?? 0;
 }
 
-/**
- * Count stale provider cache files older than N days
- */
-function getStaleRecordCount(dbId: string, days: number): number {
-  const cutoff = new Date();
-  cutoff.setDate(cutoff.getDate() - days);
-  const cutoffIso = cutoff.toISOString();
-
-  // Get persons in this database that have external identities
-  const rows = sqliteService.queryAll<{ person_id: string; source: string; external_id: string }>(
-    `SELECT ei.person_id, ei.source, ei.external_id
-     FROM external_identity ei
-     JOIN database_membership dm ON ei.person_id = dm.person_id AND dm.db_id = @dbId`,
-    { dbId }
-  );
-
-  let count = 0;
-  for (const row of rows) {
-    const cachePath = path.join(PROVIDER_CACHE_DIR, row.source, `${row.external_id}.json`);
-    if (!fs.existsSync(cachePath)) continue;
-
-    const cacheContent = JSON.parse(fs.readFileSync(cachePath, 'utf-8'));
-    if (cacheContent.scrapedAt && cacheContent.scrapedAt < cutoffIso) {
-      count++;
-    }
-  }
-
-  return count;
-}
-
 /**
  * Get persons with provider coverage gaps (have some but not all provider links)
  */
 function getProviderCoverageGaps(dbId: string, providers?: string[]): ProviderCoverageGap[] {
   const internalDbId = resolveDbId(dbId) || dbId;
   const targetProviders = providers?.length ? providers : ALL_PROVIDERS;
 
-  // Get all persons in database with their linked providers
   const rows = sqliteService.queryAll<{
     person_id: string;
     display_name: string;
@@ -157,40 +131,35 @@ function getProviderCoverageGaps(dbId: string, providers?: string[]): ProviderCo
        GROUP_CONCAT(DISTINCT ei.source) as linked_sources
      FROM database_membership dm
      JOIN person p ON dm.person_id = p.person_id
-     LEFT JOIN external_identity ei ON dm.person_id = ei.person_id
+     JOIN external_identity ei ON dm.person_id = ei.person_id
      WHERE dm.db_id = @dbId
      GROUP BY dm.person_id
-     HAVING linked_sources IS NOT NULL
-       AND linked_sources != ''`,
-    { dbId: internalDbId }
+     HAVING COUNT(DISTINCT ei.source) < @providerCount
+     ORDER BY p.display_name
+     LIMIT 500`,
+    { dbId: internalDbId, providerCount: targetProviders.length }
   );
 
-  const gaps: ProviderCoverageGap[] = [];
-
-  for (const row of rows) {
+  return rows.map(row => {
     const linked = row.linked_sources.split(',');
     const missing = targetProviders.filter(p => !linked.includes(p));
-
-    if (missing.length > 0 && linked.length > 0) {
-      gaps.push({
-        personId: row.person_id,
-        displayName: row.display_name,
-        linkedProviders: linked,
-        missingProviders: missing,
-      });
-    }
-  }
-
-  return gaps;
+    return {
+      personId: row.person_id,
+      displayName: row.display_name,
+      linkedProviders: linked,
+      missingProviders: missing,
+    };
+  });
 }
 
 /**
- * Get parent edges where child has provider link but parent doesn't
+ * Get parent edges where child has provider link but parent doesn't.
+ * Uses LEFT JOIN + IS NULL instead of NOT EXISTS for better performance.
  */
 function getParentLinkageGaps(dbId: string, provider?: string): ParentLinkageGap[] {
   const internalDbId = resolveDbId(dbId) || dbId;
 
-  // If provider is specified, filter to that specific provider
+  // Build query parts based on whether provider filter is specified
   const providerFilter = provider
     ? `AND ei_child.source = @provider`
     : '';
@@ -215,13 +184,13 @@ function getParentLinkageGaps(dbId: string, provider?: string): ParentLinkageGap
      JOIN person pc ON pe.child_id = pc.person_id
      JOIN person pp ON pe.parent_id = pp.person_id
      JOIN external_identity ei_child ON pe.child_id = ei_child.person_id ${providerFilter}
-     WHERE NOT EXISTS (
-       SELECT 1 FROM external_identity ei_parent
-       WHERE ei_parent.person_id = pe.parent_id
-         AND ei_parent.source = ei_child.source
-     )
-     ORDER BY pc.display_name, pe.parent_role`,
-    { dbId: internalDbId, provider: provider || '' }
+     LEFT JOIN external_identity ei_parent
+       ON pe.parent_id = ei_parent.person_id
+       AND ei_parent.source = ei_child.source
+     WHERE ei_parent.id IS NULL
+     ORDER BY pc.display_name, pe.parent_role
+     LIMIT 500`,
+    provider ? { dbId: internalDbId, provider } : { dbId: internalDbId }
   );
 
   return rows.map(row => ({
@@ -236,7 +205,7 @@ function getParentLinkageGaps(dbId: string, provider?: string): ParentLinkageGap
 }
 
 /**
- * Get orphaned parent edges (referencing non-existent person records)
+ * Get orphaned parent edges (parent_id referencing non-existent person records)
  */
 function getOrphanedEdges(dbId: string): OrphanedEdge[] {
   const internalDbId = resolveDbId(dbId) || dbId;
@@ -246,21 +215,14 @@ function getOrphanedEdges(dbId: string): OrphanedEdge[] {
     child_id: string;
     parent_id: string;
     parent_role: string;
-    child_exists: number;
-    parent_exists: number;
   }>(
-    `SELECT
-       pe.id,
-       pe.child_id,
-       pe.parent_id,
-       pe.parent_role,
-       (SELECT COUNT(*) FROM person WHERE person_id = pe.child_id) as child_exists,
-       (SELECT COUNT(*) FROM person WHERE person_id = pe.parent_id) as parent_exists
+    `SELECT pe.id, pe.child_id, pe.parent_id, pe.parent_role
      FROM parent_edge pe
      JOIN database_membership dm ON pe.child_id = dm.person_id AND dm.db_id = @dbId
-     WHERE NOT EXISTS (SELECT 1 FROM person p WHERE p.person_id = pe.parent_id)
-        OR NOT EXISTS (SELECT 1 FROM person p WHERE p.person_id = pe.child_id)
-     ORDER BY pe.child_id`,
+     LEFT JOIN person p ON pe.parent_id = p.person_id
+     WHERE p.person_id IS NULL
+     ORDER BY pe.child_id
+     LIMIT 500`,
     { dbId: internalDbId }
   );
 
@@ -269,16 +231,14 @@ function getOrphanedEdges(dbId: string): OrphanedEdge[] {
     childId: row.child_id,
     parentId: row.parent_id,
     parentRole: row.parent_role,
-    missingPerson: row.child_exists === 0 && row.parent_exists === 0
-      ? 'both'
-      : row.child_exists === 0
-        ? 'child'
-        : 'parent',
+    missingPerson: 'parent' as const,
   }));
 }
 
 /**
- * Get stale provider cache records older than N days
+ * Get stale provider cache records older than N days.
+ * This does filesystem I/O so is only called on-demand (not in summary).
+ * Limits file reads to avoid blocking the event loop too long.
  */
 function getStaleProviderData(dbId: string, days = 30): StaleRecord[] {
   const internalDbId = resolveDbId(dbId) || dbId;
@@ -287,7 +247,6 @@ function getStaleProviderData(dbId: string, days = 30): StaleRecord[] {
   const cutoffIso = cutoff.toISOString();
   const now = Date.now();
 
-  // Get persons in this database that have external identities
   const rows = sqliteService.queryAll<{
     person_id: string;
     display_name: string;
@@ -298,7 +257,8 @@ function getStaleProviderData(dbId: string, days = 30): StaleRecord[] {
      FROM external_identity ei
      JOIN database_membership dm ON ei.person_id = dm.person_id AND dm.db_id = @dbId
      JOIN person p ON ei.person_id = p.person_id
-     ORDER BY p.display_name`,
+     ORDER BY p.display_name
+     LIMIT 2000`,
     { dbId: internalDbId }
   );
 
@@ -308,7 +268,12 @@ function getStaleProviderData(dbId: string, days = 30): StaleRecord[] {
     const cachePath = path.join(PROVIDER_CACHE_DIR, row.source, `${row.external_id}.json`);
     if (!fs.existsSync(cachePath)) continue;
 
-    const cacheContent = JSON.parse(fs.readFileSync(cachePath, 'utf-8'));
+    const raw = fs.readFileSync(cachePath, 'utf-8');
+    const cacheContent = raw.startsWith('{') ? JSON.parse(raw) : null;
+    if (!cacheContent) {
+      logger.warn('integrity', `Corrupted cache file: ${cachePath}`);
+      continue;
+    }
     if (cacheContent.scrapedAt && cacheContent.scrapedAt < cutoffIso) {
       const scrapedDate = new Date(cacheContent.scrapedAt);
       const ageDays = Math.floor((now - scrapedDate.getTime()) / (1000 * 60 * 60 * 24));
@@ -321,6 +286,9 @@ function getStaleProviderData(dbId: string, days = 30): StaleRecord[] {
         ageDays,
       });
     }
+
+    // Cap results to prevent massive response
+    if (stale.length >= 500) break;
   }
 
   return stale;
diff --git a/shared/src/index.ts b/shared/src/index.ts
@@ -190,10 +190,13 @@ export interface Person {
   occupation?: string;         // First occupation (for backwards compat)
 }
 
-// Platform reference for cross-platform linking
+// All platforms a person can be linked to (superset of BuiltInProvider)
+// Includes manual-link platforms (wikipedia, linkedin, findagrave, etc.) that use
+// augmentation-style scraping rather than built-in provider scrapers
 export type PlatformType = 'familysearch' | 'wikipedia' | 'findagrave' | 'heritage' | 'ancestry' | 'geni' | 'wikitree' | 'myheritage' | 'findmypast' | '23andme' | 'linkedin';
 
-// Built-in provider types (browser-based scrapers)
+// Providers with built-in browser scrapers for tree traversal and parent discovery
+// Manual-link platforms (linkedin, wikipedia, etc.) are NOT included here
 export type BuiltInProvider = 'familysearch' | 'ancestry' | '23andme' | 'wikitree';
 
 // Legacy: Genealogy provider authentication types (kept for backward compatibility)
@@ -544,7 +547,7 @@ export interface DatabaseInfo {
   filename: string;
   personCount: number;
   rootId: string;              // Same as id - root person's canonical ULID
-  rootExternalId?: string;     // FamilySearch ID for display/external linking (legacy, use externalIds)
+  rootExternalId?: string;     // @deprecated Still populated from externalIds.familysearch for backward compat. New code should read from externalIds instead.
   externalIds?: Record<string, string>; // All external IDs by platform (e.g., { familysearch: 'GW21-BZR', ancestry: '12345' })
   rootName?: string;           // Name of the root person
   maxGenerations?: number;
