Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
atomantic merged 1 commit into from
Mar 6, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions server/src/services/cache.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
});

Expand Down
4 changes: 2 additions & 2 deletions server/src/services/id-mapping.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand Down
1 change: 0 additions & 1 deletion server/src/services/multi-platform-comparison.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down
9 changes: 6 additions & 3 deletions server/src/services/path.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }>(
Expand Down Expand Up @@ -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 }>(
Expand Down Expand Up @@ -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 }>(
Expand Down
11 changes: 9 additions & 2 deletions server/src/services/sync.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
});
Comment on lines 63 to +67
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compareAcrossProviders swallows scrapeFromProvider errors and continues, but scrapeFromProvider does not close the Puppeteer page if scrapePersonById throws (it only closes after a successful scrape). This can leak pages/tabs over time (especially in loops) and eventually destabilize the browser process. Consider updating scrapeFromProvider to close the page in a finally block (or ensure the page is closed in the error path) so failures don’t leak resources.

Copilot uses AI. Check for mistakes.

comparison.providerData[provider] = scraped;
}
Expand Down Expand Up @@ -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(() => {});
Expand Down
Loading