From 00c61b2a82fdc2182ce42ebf3e053dec4210ef30 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Thu, 5 Mar 2026 18:25:48 -0800 Subject: [PATCH] fix: improve code quality - log suppressed errors, remove non-null assertions, extract constants --- server/src/services/cache.service.ts | 20 +++++++++++++------ server/src/services/id-mapping.service.ts | 4 ++-- .../multi-platform-comparison.service.ts | 1 - server/src/services/path.service.ts | 9 ++++++--- server/src/services/sync.service.ts | 11 ++++++++-- 5 files changed, 31 insertions(+), 14 deletions(-) diff --git a/server/src/services/cache.service.ts b/server/src/services/cache.service.ts index 614211c..73f11ca 100644 --- a/server/src/services/cache.service.ts +++ b/server/src/services/cache.service.ts @@ -130,22 +130,30 @@ class LRUCache { } } +// Cache size and TTL constants per cache type +const QUERY_CACHE_MAX_SIZE = 2000; +const QUERY_CACHE_TTL_MS = 5 * 60 * 1000; // 5 minutes +const PERSON_CACHE_MAX_SIZE = 5000; +const PERSON_CACHE_TTL_MS = 10 * 60 * 1000; // 10 minutes +const LIST_CACHE_MAX_SIZE = 100; +const LIST_CACHE_TTL_MS = 60 * 1000; // 1 minute - lists change more frequently + // Create named caches for different query types const queryCache = new LRUCache({ - maxSize: 2000, - ttlMs: 5 * 60 * 1000, // 5 minutes + maxSize: QUERY_CACHE_MAX_SIZE, + ttlMs: QUERY_CACHE_TTL_MS, name: 'query' }); const personCache = new LRUCache({ - maxSize: 5000, - ttlMs: 10 * 60 * 1000, // 10 minutes + maxSize: PERSON_CACHE_MAX_SIZE, + ttlMs: PERSON_CACHE_TTL_MS, name: 'person' }); const listCache = new LRUCache({ - maxSize: 100, - ttlMs: 60 * 1000, // 1 minute - lists change more frequently + maxSize: LIST_CACHE_MAX_SIZE, + ttlMs: LIST_CACHE_TTL_MS, name: 'list' }); diff --git a/server/src/services/id-mapping.service.ts b/server/src/services/id-mapping.service.ts index dd3686e..876801a 100644 --- a/server/src/services/id-mapping.service.ts +++ b/server/src/services/id-mapping.service.ts @@ -17,6 +17,7 @@ const canonicalToExternalCache = new Map>(); // Cache size limit (LRU eviction) const MAX_CACHE_SIZE = 100000; +const EVICTION_RATIO = 0.1; // Evict 10% of entries when cache is full /** * Generate a cache key for external ID lookups @@ -30,8 +31,7 @@ function cacheKey(source: string, externalId: string): string { */ function evictIfNeeded(): void { if (externalToCanonicalCache.size > MAX_CACHE_SIZE) { - // Simple eviction: remove first 10% of entries - const toRemove = Math.floor(MAX_CACHE_SIZE * 0.1); + const toRemove = Math.floor(MAX_CACHE_SIZE * EVICTION_RATIO); const keys = externalToCanonicalCache.keys(); for (let i = 0; i < toRemove; i++) { const key = keys.next().value; diff --git a/server/src/services/multi-platform-comparison.service.ts b/server/src/services/multi-platform-comparison.service.ts index 25b84ea..ba965cb 100644 --- a/server/src/services/multi-platform-comparison.service.ts +++ b/server/src/services/multi-platform-comparison.service.ts @@ -94,7 +94,6 @@ async function downloadProviderPhoto( await downloadImage(normalizedUrl, photoPath).catch(err => { logger.error('compare', `Failed to download ${provider} photo: ${err.message}`); - return null; }); if (fs.existsSync(photoPath)) { diff --git a/server/src/services/path.service.ts b/server/src/services/path.service.ts index abea6fb..7236f86 100644 --- a/server/src/services/path.service.ts +++ b/server/src/services/path.service.ts @@ -17,7 +17,8 @@ function buildAncestryMap( const queue: Array<{ id: string; depth: number }> = [{ id: startId, depth: 0 }]; while (queue.length > 0) { - const current = queue.shift()!; + const current = queue.shift(); + if (!current) break; if (current.depth >= maxDepth) continue; const parents = sqliteService.queryAll<{ parent_id: string }>( @@ -246,7 +247,8 @@ export const pathService = { const queue: Array<{ id: string; depth: number }> = [{ id: canonicalId, depth: 0 }]; while (queue.length > 0) { - const current = queue.shift()!; + const current = queue.shift(); + if (!current) break; if (current.depth >= maxDepth) continue; const parents = sqliteService.queryAll<{ parent_id: string }>( @@ -282,7 +284,8 @@ export const pathService = { const queue: Array<{ id: string; depth: number }> = [{ id: canonicalId, depth: 0 }]; while (queue.length > 0) { - const current = queue.shift()!; + const current = queue.shift(); + if (!current) break; if (current.depth >= maxDepth) continue; const children = sqliteService.queryAll<{ child_id: string }>( diff --git a/server/src/services/sync.service.ts b/server/src/services/sync.service.ts index 56b0296..aaf98b4 100644 --- a/server/src/services/sync.service.ts +++ b/server/src/services/sync.service.ts @@ -11,6 +11,7 @@ import type { import { browserService } from './browser.service'; import { providerService } from './provider.service'; import { DATA_DIR } from '../utils/paths.js'; +import { logger } from '../lib/logger.js'; /** * Sync service for comparing and syncing data across providers @@ -60,7 +61,10 @@ export const syncService = { if (!config.enabled) continue; const scraped = await this.scrapeFromProvider(provider, personId) - .catch(() => null); + .catch(err => { + logger.error('sync', `Failed to scrape ${provider}/${personId}: ${err.message}`); + return null; + }); comparison.providerData[provider] = scraped; } @@ -139,7 +143,10 @@ export const syncService = { const resultLink = await page.$eval( 'a[href*="/person/"], a[href*="/tree/person/"], a[href*="/wiki/"]', el => el.getAttribute('href') - ).catch(() => null); + ).catch(err => { + logger.error('sync', `Failed to find result link for ${targetProvider}: ${err.message}`); + return null; + }); if (!resultLink) { await page.close().catch(() => {});