Skip to content

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

Merged
atomantic merged 1 commit intomainfrom
better/code-quality
Mar 6, 2026
Merged

fix: improve code quality - log errors, remove non-null assertions, extract constants#40
atomantic merged 1 commit intomainfrom
better/code-quality

Conversation

@atomantic
Copy link
Owner

Better Audit — Code Quality

5 findings fixed across 5 files. Replaced silent .catch() with error logging, removed unsafe non-null assertions, extracted magic numbers to named constants.

Copilot AI review requested due to automatic review settings March 6, 2026 02:26
@atomantic atomantic force-pushed the better/code-quality branch from 68c2119 to 00c61b2 Compare March 6, 2026 02:28
@atomantic atomantic merged commit d8b68e2 into main Mar 6, 2026
1 check passed
@atomantic atomantic deleted the better/code-quality branch March 6, 2026 02:28
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Improves server-side code quality by adding error logging for previously silent failures, removing unsafe non-null assertions, and extracting repeated “magic number” cache constants into named constants.

Changes:

  • Log scrape/DOM-eval failures in sync.service instead of silently swallowing errors.
  • Remove non-null assertions from BFS queue handling in path.service.
  • Extract cache sizing/TTL/eviction “magic numbers” into named constants across services.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
server/src/services/sync.service.ts Adds logger usage to report scrape and result-link lookup failures.
server/src/services/path.service.ts Replaces queue.shift()! with a safe null check to avoid non-null assertions.
server/src/services/multi-platform-comparison.service.ts Removes an ineffective return null inside a .catch used for logging download failures.
server/src/services/id-mapping.service.ts Extracts eviction ratio into EVICTION_RATIO constant.
server/src/services/cache.service.ts Extracts cache size/TTL values into named constants for query/person/list caches.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines 63 to +67
const scraped = await this.scrapeFromProvider(provider, personId)
.catch(() => null);
.catch(err => {
logger.error('sync', `Failed to scrape ${provider}/${personId}: ${err.message}`);
return null;
});
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants