Conversation
metacurb
force-pushed
the
codex/steam-sync-worker-pipeline
branch
from
6f33a88 to
55e8537
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f33a882a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const libraryManifests = await Promise.all( | ||
| libraries.map(async (libraryPath) => { | ||
| const manifestPath = path.join(libraryPath, "steamapps"); | ||
| const files = await fs.readdir(manifestPath); |
There was a problem hiding this comment.
readSteamInstalledGames now scans all Steam libraries in a single Promise.all without a per-library try/catch, so one bad path (for example an unmounted drive or stale entry in libraryfolders.vdf) rejects the whole read. In the new worker flow this bubbles up as job-failed, which aborts the entire Steam library sync even when other library folders are valid; the previous strategy code tolerated individual path failures and continued.
Useful? React with 👍 / 👎.
| if (result.status === "fulfilled" && result.value) { | ||
| return { |
There was a problem hiding this comment.
This branch classifies a fulfilled null metadata response as a hard failure. fetchGameMetadata returns null on HTTP 404, and SyncService.synchroniseSteamMetadata only persists metadataSyncedAt for success entries, so not-found Steam games remain forever in the unsynced set and are retried on every sync cycle. That creates permanent failure churn and repeated API calls for titles the backend does not know about.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Steam synchronization process by offloading library scanning and metadata fetching to background worker threads, significantly improving performance and UI responsiveness. Key changes include the introduction of SteamSyncWorkerService, batch metadata processing in GameStore, and frontend optimizations to prevent redundant startup syncs. Feedback focuses on addressing potential SQLite parameter limits for large libraries, adding timeouts to worker promises to prevent resource leaks, simplifying type casting for nullable fields, and increasing metadata batch sizes to minimize IPC overhead.
| }, | ||
| where: { isInstalled: true, library }, | ||
| }), | ||
| installedGameIds.length ? this.repository.findBy({ gameId: In(installedGameIds), library }) : [], |
There was a problem hiding this comment.
Using the In operator with a potentially large array of IDs can lead to performance issues or even crashes in SQLite due to the limit on the number of host parameters (usually 999). For users with very large Steam libraries, installedGameIds could exceed this limit. Consider chunking the IDs or using a different query strategy if the count is high.
| await manager.update( | ||
| GameEntity, | ||
| { gameId: In(gamesToUninstall), library }, | ||
| { installationDetails: null as unknown as GameInstallationDetails, isInstalled: false }, |
There was a problem hiding this comment.
The double cast null as unknown as GameInstallationDetails is unnecessary and masks type safety. Since the column is nullable in the database, you can simply use null as any or update the GameInstallationDetails type to be nullable if you want to maintain strict typing.
{ installationDetails: null as any, isInstalled: false },|
|
||
| this.logger.debug("Starting Steam library worker job", { jobId }); | ||
|
|
||
| return await new Promise<Extract<SteamSyncWorkerResponse, { type: "library-scan-results" }>>((resolve, reject) => { |
There was a problem hiding this comment.
The worker job promise lacks a timeout mechanism. If the worker thread hangs or fails to send a job-complete or job-failed message, this promise will remain pending indefinitely, potentially leaking resources. Consider adding a timeout to ensure the promise eventually settles.
| private async synchroniseSteamMetadata(games: GameStoreModel[]) { | ||
| if (!games.length) return; | ||
|
|
||
| await this.steamSyncWorkerService.runMetadataJob( | ||
| { | ||
| apiBaseUrl: getStakloadApiBaseUrl(), | ||
| games: games.map(({ _id, gameId, name }) => ({ _id, gameId, name })), | ||
| }, | ||
| async (results, progress) => { | ||
| const successfulEntries = results.flatMap((result) => | ||
| result.status === "success" | ||
| ? [ | ||
| { | ||
| id: result.game._id, | ||
| metadata: result.metadata, | ||
| }, | ||
| ] | ||
| : [], | ||
| ); | ||
|
|
||
| if (successfulEntries.length) { | ||
| await this.gameStore.applyMetadataSyncBatch(successfulEntries); | ||
| } | ||
|
|
||
| results | ||
| .filter((result) => result.status === "failure") | ||
| .forEach((result) => { | ||
| this.logger.warn("Steam metadata synchronisation failed", { | ||
| error: result.error, | ||
| game: result.game.name, | ||
| }); | ||
| this.addFailureEntry({ | ||
| action: "metadata", | ||
| code: "UNKNOWN_ERROR", | ||
| gameName: result.game.name, | ||
| library: "steam", | ||
| }); | ||
| }); | ||
|
|
||
| this.processing += results.length; | ||
| this.emitMetadataProgress(progress.processed === progress.total); | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The synchroniseSteamMetadata method processes games in batches of 10 (default). While this improves responsiveness, a batch size of 10 might be too small for large libraries, leading to excessive IPC overhead and frequent database transactions. Consider increasing the default batch size (e.g., to 50 or 100) to find a better balance between UI responsiveness and sync performance.
1 participant
Summary
Details
SteamSyncWorkerService, worker message contracts, and a dedicatedsteam-sync.worker.tsGameStorebatch helpers for installed-game reconciliation and metadata sync writesSyncService, while leaving the other launchers on the existing path for nowValidation
pnpm --dir apps/frontend typecheckpnpm --dir apps/desktop lint(passes with existing warnings only)pnpm --dir apps/desktop typecheckstill fails on pre-existing unrelated issues ingame-lifecycle.service.ts,integrations/auth.ts,epic-games-store-api.service.ts, andapps/frontend/src/ipc.types.tspnpm --dir apps/desktop exec electron-vite build --config electron.vite.config.tsNotes
#44and keeps the renderer IPC contract unchanged.