Merge pull request #40 from atomantic/better/code-quality

fix: improve code quality - log errors, remove non-null assertions, extract constants

Author: atomantic

server/src/services/cache.service.ts

@@ -130,22 +130,30 @@ class LRUCache<T> {
   }
 }
 
+// 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<unknown>({
-  maxSize: 2000,
-  ttlMs: 5 * 60 * 1000, // 5 minutes
+  maxSize: QUERY_CACHE_MAX_SIZE,
+  ttlMs: QUERY_CACHE_TTL_MS,
   name: 'query'
 });
 
 const personCache = new LRUCache<unknown>({
-  maxSize: 5000,
-  ttlMs: 10 * 60 * 1000, // 10 minutes
+  maxSize: PERSON_CACHE_MAX_SIZE,
+  ttlMs: PERSON_CACHE_TTL_MS,
   name: 'person'
 });
 
 const listCache = new LRUCache<unknown>({
-  maxSize: 100,
-  ttlMs: 60 * 1000, // 1 minute - lists change more frequently
+  maxSize: LIST_CACHE_MAX_SIZE,
+  ttlMs: LIST_CACHE_TTL_MS,
   name: 'list'
 });
 

server/src/services/id-mapping.service.ts

@@ -17,6 +17,7 @@ const canonicalToExternalCache = new Map<string, Map<string, string>>();
 
 // 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;

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)) {

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 }>(

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(() => {});