Skip to content

feat: restructured metadata provider#72

Merged
Sudashiii merged 2 commits into
masterfrom
feature/metadata-provider-port
Apr 21, 2026
Merged

feat: restructured metadata provider#72
Sudashiii merged 2 commits into
masterfrom
feature/metadata-provider-port

Conversation

@Sudashiii
Copy link
Copy Markdown
Owner

No description provided.

@Sudashiii Sudashiii requested a review from Copilot April 21, 2026 18:19
Copy link
Copy Markdown

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

This PR restructures external book metadata lookup into a provider-based architecture with a shared port/type layer and an aggregator to combine and rank results across multiple metadata sources.

Changes:

  • Introduces MetadataProviderPort + MetadataProviderId types and shared provider utilities.
  • Adds Google Books and Open Library metadata provider implementations plus a MetadataAggregatorService.
  • Refactors ExternalBookMetadataService to use the aggregator and wires the shared instance into server composition.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
sake/src/lib/types/Metadata/Provider.ts Adds canonical metadata provider ID list + union type.
sake/src/lib/server/application/ports/MetadataProviderPort.ts Defines the provider port, query, and candidate data model.
sake/src/lib/server/infrastructure/metadata-providers/metadataProviderUtils.ts Adds shared parsing/normalization helpers for providers.
sake/src/lib/server/infrastructure/metadata-providers/googleBooksMetadataProvider.ts Implements Google Books provider returning MetadataCandidates.
sake/src/lib/server/infrastructure/metadata-providers/openLibraryMetadataProvider.ts Implements Open Library provider returning MetadataCandidates.
sake/src/lib/server/application/services/MetadataAggregatorService.ts Aggregates providers with per-provider timeout + ranking.
sake/src/lib/server/application/services/ExternalBookMetadataService.ts Switches to aggregator-based lookup and maps candidates to legacy external metadata shape.
sake/src/lib/server/infrastructure/metadata-providers/metadataProviderFactory.ts Adds factory helpers for constructing providers by ID.
sake/src/lib/server/config/activatedMetadataProviders.ts Adds parsing for activated metadata providers from env config.
sake/src/lib/server/application/composition.ts Instantiates and injects shared aggregator/external metadata service into use cases.

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

return typeof value === 'string' && value.trim().length > 0 ? value.trim() : null;
}

export function asNumber(value: unknown): number | null {
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

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

asNumber filters out 0 (requires value > 0). This causes valid values like ratingsCount: 0 / ratings_count: 0 to be coerced to null, even though downstream code treats >= 0 as valid (e.g., storing external_rating_count). Consider either allowing 0 here (use >= 0) or introducing separate helpers (e.g., asPositiveNumber for page counts and asNonNegativeNumber for rating counts).

Suggested change
export function asNumber(value: unknown): number | null {
export function asNumber(value: unknown): number | null {
return typeof value === 'number' && Number.isFinite(value) && value >= 0 ? value : null;
}
export function asPositiveNumber(value: unknown): number | null {

Copilot uses AI. Check for mistakes.
Comment on lines +61 to +66
return Promise.race([
promise,
new Promise<T>((_, reject) =>
setTimeout(() => reject(new Error(`${label} timed out after ${ms}ms`)), ms)
)
]);
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

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

withTimeout uses setTimeout inside Promise.race but never clears the timer when the wrapped promise settles. This can leave stray timers around (and in high traffic, unnecessary work) even when providers respond quickly. Track the timeout handle and clearTimeout it in a finally/cleanup path; if possible, also consider propagating an AbortSignal to providers so the underlying fetch can be cancelled on timeout.

Suggested change
return Promise.race([
promise,
new Promise<T>((_, reject) =>
setTimeout(() => reject(new Error(`${label} timed out after ${ms}ms`)), ms)
)
]);
let timeoutId: ReturnType<typeof setTimeout>;
const timeoutPromise = new Promise<T>((_, reject) => {
timeoutId = setTimeout(() => reject(new Error(`${label} timed out after ${ms}ms`)), ms);
});
return Promise.race([promise, timeoutPromise]).finally(() => {
clearTimeout(timeoutId);
});

Copilot uses AI. Check for mistakes.
Comment on lines 32 to 39
@@ -46,393 +38,64 @@ function pickFirst<T>(...values: Array<T | null | undefined>): T | null {
return null;
}
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

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

pickFirst duplicates the behavior of pickFirstValue that was added in metadataProviderUtils.ts. Reusing the shared helper would reduce duplication and keep null/undefined selection logic consistent across metadata code paths.

Copilot uses AI. Check for mistakes.
@Sudashiii Sudashiii merged commit ed891bf into master Apr 21, 2026
4 checks passed
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