feat: improved prefix functionality to manage wine prefixes & overriden the task api and types to make it better for DX. #27
+2,613
−1,253
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ated classes to be better typed for askForInput
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughModularizes Electron handlers (Steam/library/redistributable) and helper utilities, replaces generic task-run with named task handlers + Task API, adds ActionOption generics to addon config, persists/migrates re-add state, updates preload/typings and frontend UI flows, and bumps package versions. Changes
Sequence Diagram(s)sequenceDiagram
participant Frontend as Frontend UI
participant WS as WebSocket Listener
participant AddonCore as OGIAddon Core
participant TaskHandler as Registered Task Handler
Frontend->>WS: emit 'task-run' (args with taskName?/manifest)
WS->>AddonCore: resolve taskName (args.taskName || manifest.__taskName)
AddonCore->>AddonCore: lookup handler in taskHandlers Map
alt handler found
AddonCore->>TaskHandler: instantiate Task(EventResponse) and invoke handler(manifest, downloadPath, name)
TaskHandler->>AddonCore: call task.log / setProgress / askForInput / complete/fail
AddonCore->>WS: send task updates and final completion/failure
else no handler
AddonCore->>WS: send error "No handler registered for taskName"
end
WS->>Frontend: deliver result or error
sequenceDiagram
participant User as User
participant PlayPage as PlayPage UI
participant Electron as Electron API
participant Store as appUpdates Store
participant FS as internals/update-state.json
User->>PlayPage: click "Add to Steam"
PlayPage->>PlayPage: disable button (accessibility)
PlayPage->>Electron: call addToSteam(appID, oldSteamAppId?)
alt success
Electron-->>PlayPage: { success: true }
PlayPage->>Store: remove entry from requiredReadds
Store->>FS: persist ./internals/update-state.json (debounced)
PlayPage->>User: show success
else failure
Electron-->>PlayPage: { success: false, error }
PlayPage->>PlayPage: log error, re-enable button
PlayPage->>User: show error
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/ogi-addon/src/config/Configuration.ts (1)
43-61: Skip validation for action options to avoid false errors.
Action options have no value, yetvalidate()still runs; if a custom validator expects a value, it can throw or error. Consider bypassing validation fortype === 'action'.💡 Suggested adjustment
- const result = this.storedConfigTemplate[key].validate( - this.definiteConfig[key] - ); - if (!result[0]) { - erroredKeys.set(key, result[1]); - } + if (this.storedConfigTemplate[key].type !== 'action') { + const result = this.storedConfigTemplate[key].validate( + this.definiteConfig[key] + ); + if (!result[0]) { + erroredKeys.set(key, result[1]); + } + }application/src/frontend/lib/setup/setup.ts (1)
312-318: Operator precedence issue in condition.The condition mixes
&&and||without parentheses, making the logic unclear and potentially incorrect:if ( ((await window.electronAPI.app.getOS()) === 'linux' && beforeLibraryApp && beforeLibraryApp?.cwd != data.cwd) || beforeLibraryApp?.launchExecutable != data.launchExecutable || beforeLibraryApp?.launchArguments != data.launchArguments )Due to operator precedence, this evaluates as:
(linux && beforeLibraryApp && cwd_changed) || exe_changed || args_changedThis means executable/argument changes trigger the block on any OS, not just Linux. If the intent is to only run this on Linux when any of the three properties changed, the parentheses need adjustment.
🔧 Proposed fix (if Linux-only is intended)
if ( - ((await window.electronAPI.app.getOS()) === 'linux' && - beforeLibraryApp && - beforeLibraryApp?.cwd != data.cwd) || - beforeLibraryApp?.launchExecutable != data.launchExecutable || - beforeLibraryApp?.launchArguments != data.launchArguments + (await window.electronAPI.app.getOS()) === 'linux' && + beforeLibraryApp && + (beforeLibraryApp.cwd !== data.cwd || + beforeLibraryApp.launchExecutable !== data.launchExecutable || + beforeLibraryApp.launchArguments !== data.launchArguments) ) {
🤖 Fix all issues with AI agents
In `@application/src/electron/handlers/handler.app.ts`:
- Around line 1075-1113: The handler (ipcMain.handle for 'app:move-prefix')
performs multiple filesystem operations (getNonSteamGameAppID, fs.mkdirSync,
fs.renameSync) without validation, atomicity, or error handling; fix by
validating inputs (ensure originalPrefix exists and is a directory via
fs.statSync/fs.existsSync, and protect against path traversal by resolving paths
and confirming they begin with expected base directories), replace the fragile
tmpNewPrefixPath creation with a unique non-existent temp directory (use a
timestamp or mkdtemp-like strategy) so renameSync won't fail if the path exists,
wrap the sequence of moves in a try/catch and implement rollback logic to
restore originalProton/newProton state on any failure (use the variables
newProtonPath, tmpNewPrefixPath, tempPath, originalPrefix for references),
ensure cleanup of any created directories on error, and return 'failed' with
logged error details on exceptions; keep getNonSteamGameAppID usage and platform
check but add these validations and error/rollback handling around the
rename/mkdir operations.
In `@application/src/frontend/lib/setup/setup.ts`:
- Around line 339-346: The call to window.electronAPI.app.movePrefix is passing
newPrefix (a filesystem path) as the second argument but the backend handler for
app:move-prefix (handler.app.ts) expects the game name string to look up an
appId via getNonSteamGameAppID; change the caller to pass the game name (not the
path) as the second argument when invoking movePrefix (or derive the name from
the selected game object before calling window.electronAPI.app.movePrefix) so
the backend receives the expected "name" parameter used by getNonSteamGameAppID.
In `@packages/ogi-addon/src/main.ts`:
- Around line 884-886: The current code returns an unsafe cast "{} as U" when
"socket" is falsy which breaks the ConfigurationBuilder contract; replace that
branch to surface a real failure (throw an Error like "Socket unavailable for
ConfigurationBuilder" or return a rejected Promise) instead of fabricating a U,
and then update callers that consume this function to either catch the thrown
error or handle a nullable/Promise rejection; ensure the symbol "socket" branch
is removed and the function's return type/signature (the generic U) is adjusted
if you choose to make the absence explicit (e.g., U | undefined or Promise
rejection) so callers no longer rely on an invalid empty object.
- Around line 696-701: The Task.complete() method currently calls
this.event.resolve(), but since Task wraps an EventResponse<void> it should
delegate to the void-specific method this.event.complete(); update the
complete() implementation to call this.event.complete() (replace any
this.event.resolve() usage), ensuring you reference the Task.complete method and
EventResponse.complete when making the change.
In `@test-addon/main.ts`:
- Around line 116-155: The action option uses setTaskName('task-test-2') but
there is no corresponding addon.onTask('task-test-2', ...) handler; add an
addon.onTask('task-test-2', (task) => { ... }) implementation (mirroring the
existing addon.onTask('task-test') handler behavior: log, setProgress, and
complete or whatever behavior is intended) or, if omission is intentional, add
an inline comment near the ConfigurationBuilder/action option documenting that
the missing handler is deliberate for testing error scenarios.
🧹 Nitpick comments (2)
application/src/frontend/views/ClientOptionsView.svelte (1)
1306-1314: Redundant background-color declaration.The
bg-accent-lighterTailwind class is applied via@apply, but then immediately overridden by the hardcodedbackground-color:#e1f4f0``. This duplication means theme changes toaccent-lighterwon't affect this element. Consider removing one:🔧 Suggested fix: remove redundant declaration
If
#e1f4f0is the intended value regardless of theme:.range-value-input { - `@apply` w-16 text-center px-3 py-1 bg-accent-lighter text-accent-dark rounded-lg font-archivo font-semibold text-lg border-none focus:ring-2 focus:ring-accent-light outline-none; + `@apply` w-16 text-center px-3 py-1 text-accent-dark rounded-lg font-archivo font-semibold text-lg border-none focus:ring-2 focus:ring-accent-light outline-none; background-color: `#e1f4f0`;Or if the theme value should be used:
.range-value-input { `@apply` w-16 text-center px-3 py-1 bg-accent-lighter text-accent-dark rounded-lg font-archivo font-semibold text-lg border-none focus:ring-2 focus:ring-accent-light outline-none; - background-color: `#e1f4f0`;application/src/frontend/components/modal/ConfigurationModal.svelte (1)
251-276: Consider UX for forms with both action and non-action options.When a form contains both action options and non-action options (e.g., a text input and an action button), both the action buttons row and the Submit button will be displayed. This could lead to user confusion:
- Action buttons immediately submit the form
- The Submit button also submits the form but without any action flagged
Is this intentional? If not, you may want to hide the Submit button when action options are present, or provide clearer visual distinction between the two submission paths.
…n validation in runSetupAppUpdate
…it button for non-action forms and adjust button variants
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ogi-addon/src/main.ts (1)
1099-1143: Progress updates won't be sent during task handler execution.The
setIntervalfor sendingdefer-updatemessages (lines 1131-1142) is placed after the handler execution block. This means for handlers registered viaonTask(), progress updates won't be sent to the client while the handler is running - only after completion/failure.The interval should be started before invoking the handler to ensure real-time progress updates are transmitted.
Proposed fix
if ( taskName && typeof taskName === 'string' && this.addon.hasTaskHandler(taskName) ) { // Use the registered task handler const handler = this.addon.getTaskHandler(taskName)!; const task = new Task(taskRunEvent); + + // Start the defer-update interval before running the handler + const interval = setInterval(() => { + if (taskRunEvent.resolved) { + clearInterval(interval); + return; + } + this.send('defer-update', { + logs: taskRunEvent.logs, + deferID: message.args.deferID, + progress: taskRunEvent.progress, + failed: taskRunEvent.failed, + } as ClientSentEventTypes['defer-update']); + }, 100); + try { const result = handler(task, { manifest: message.args.manifest || {}, downloadPath: message.args.downloadPath || '', name: message.args.name || '', }); // If handler returns a promise, wait for it if (result instanceof Promise) { await result; } } catch (error) { taskRunEvent.fail( error instanceof Error ? error.message : String(error) ); } + + const taskRunResult = await this.waitForEventToRespond(taskRunEvent); + this.respondToMessage(message.id!!, taskRunResult.data, taskRunEvent); + break; } else { // No handler found - fail the task taskRunEvent.fail( taskName ? `No task handler registered for task name: ${taskName}` : 'No task name provided' ); } - const interval = setInterval(() => { - if (taskRunEvent.resolved) { - clearInterval(interval); - return; - } - this.send('defer-update', { - logs: taskRunEvent.logs, - deferID: message.args.deferID, - progress: taskRunEvent.progress, - failed: taskRunEvent.failed, - } as ClientSentEventTypes['defer-update']); - }, 100); const taskRunResult = await this.waitForEventToRespond(taskRunEvent); this.respondToMessage(message.id!!, taskRunResult.data, taskRunEvent); break;
🤖 Fix all issues with AI agents
In `@application/src/frontend/lib/setup/setup.ts`:
- Around line 64-67: The object spread that builds the retry/failed-update
payload is defaulting the "for" field to 'game' when currentLibraryInfo is
missing; change the construction to always set for: forType (use the forType
parameter) and only conditionally include currentLibraryInfo (e.g., add
currentLibraryInfo property only if currentLibraryInfo is defined) so that
retries remain tagged as 'update' when forType === 'update'; locate usages
around the downloadedItem, forType, and currentLibraryInfo variables/functions
in this module and adjust those object creations accordingly.
- Around line 312-334: The current check that decides to move prefixes uses
(newPrefix !== originalPrefix && (newPrefix !== '' || originalPrefix !== ''))
which still allows calling movePrefix when originalPrefix is empty; change the
condition to require both paths be non-empty (newPrefix !== originalPrefix &&
newPrefix !== '' && originalPrefix !== ''), so the code that triggers the prefix
swap/notification (around createNotification and the subsequent movePrefix call)
only runs when both originalPrefix and newPrefix are present.
- Around line 243-255: The code casts getApp(downloadedItem.appID) to
LibraryInfo then proceeds, but getApp can return undefined; update setup should
abort if the library entry is missing. Change the flow around
currentLibraryInfo: call getApp(downloadedItem.appID) without casting, check if
it is undefined, and if so fail fast (e.g., log an error or throw and return)
referencing downloadedItem.appID; only when currentLibraryInfo is present call
createSetupPayload(...) and createSetupCallbacks(...). Ensure the
early-return/error uses the same execution context as the caller so you don't
run update setup for a non-existent library entry.
♻️ Duplicate comments (1)
application/src/frontend/lib/setup/setup.ts (1)
335-338: API mismatch:movePrefixexpects the game name, not the new prefix path.The backend
app:move-prefixhandler uses the second argument as the game name to resolve the Steam app ID; passingnewPrefix(a path) breaks that. UsedownloadedItem.name(or the selected game name) instead.🐛 Proposed fix
const result = await window.electronAPI.app.movePrefix( originalPrefix, - newPrefix + downloadedItem.name );#!/bin/bash # Verify the backend signature for app:move-prefix rg -n "app:move-prefix|movePrefix" application/src/electron/handlers/handler.app.ts -C3
🧹 Nitpick comments (4)
packages/ogi-addon/src/config/Configuration.ts (1)
43-66: Consider consolidating action-type check at the start of the loop.The action type is checked in three separate places (lines 44, 58, 64). Moving the check to the beginning of the loop would simplify the logic and remove redundant conditions.
♻️ Suggested refactor
for (const key in this.storedConfigTemplate) { + if (this.storedConfigTemplate[key].type === 'action') { + continue; + } + if ( - this.storedConfigTemplate[key].type !== 'action' && - (this.definiteConfig[key] === null || - this.definiteConfig[key] === undefined) + this.definiteConfig[key] === null || + this.definiteConfig[key] === undefined ) { console.warn( 'Option ' + key + ' is not defined. Using default value Value: ' + this.storedConfigTemplate[key].defaultValue ); this.definiteConfig[key] = this.storedConfigTemplate[key] .defaultValue as string | number | boolean; } if ( - this.storedConfigTemplate[key].type !== 'action' && this.storedConfigTemplate[key].type !== typeof this.definiteConfig[key] ) { throw new Error('Option ' + key + ' is not of the correct type.'); } - if (this.storedConfigTemplate[key].type === 'action') { - continue; - } - const result = this.storedConfigTemplate[key].validate( this.definiteConfig[key] );application/src/electron/handlers/handler.app.ts (1)
1141-1157: Redundant rename operation (dead code).Line 1142 assigns
tmpNewPrefixPath = tempPath, making both variables reference the same path. The subsequent rename at line 1156 (fs.renameSync(tmpNewPrefixPath, tempPath)) is a no-op since source and destination are identical.This doesn't cause a runtime error on Linux but is confusing dead code that should be removed.
♻️ Proposed fix to remove redundant code
if (fs.existsSync(newProtonPath)) { tmpNewPrefixPath = tempPath; fs.renameSync(newProtonPath, tmpNewPrefixPath); newProtonMoved = true; } else { const parentDir = path.dirname(newProtonPath); if (!fs.existsSync(parentDir)) { fs.mkdirSync(parentDir, { recursive: true }); } } fs.renameSync(resolvedOriginalPrefix, resolvedNewProtonPath); originalPrefixMoved = true; - if (tmpNewPrefixPath && fs.existsSync(tmpNewPrefixPath)) { - fs.renameSync(tmpNewPrefixPath, tempPath); - } return 'success';packages/ogi-addon/src/main.ts (2)
464-476: Consider warning when overwriting existing task handlers.The
onTaskmethod silently overwrites any previously registered handler for the sametaskName. This could lead to subtle bugs if an addon accidentally registers two handlers with the same name.Optional: Add a warning for handler overwrites
public onTask( taskName: string, handler: ( task: Task, data: { manifest: Record<string, unknown>; downloadPath: string; name: string; } ) => Promise<void> | void ): void { + if (this.taskHandlers.has(taskName)) { + console.warn(`Warning: Overwriting existing task handler for "${taskName}"`); + } this.taskHandlers.set(taskName, handler); }
1091-1101: Simplify taskName extraction and remove redundant type check.The nested ternary for extracting
taskNameis hard to follow, and thetypeof taskName === 'string'check at line 1101 is redundant since the extraction logic already ensures it's a string.Suggested simplification
- // Check for taskName: first from args directly (from SearchResult), then from manifest.__taskName (for ActionOption) - const taskName = - message.args.taskName && typeof message.args.taskName === 'string' - ? message.args.taskName - : message.args.manifest && - typeof message.args.manifest === 'object' - ? (message.args.manifest as Record<string, unknown>).__taskName - : undefined; - - if ( - taskName && - typeof taskName === 'string' && - this.addon.hasTaskHandler(taskName) - ) { + // Check for taskName: first from args directly (from SearchResult), then from manifest.__taskName (for ActionOption) + const taskName: string | undefined = + typeof message.args.taskName === 'string' + ? message.args.taskName + : typeof message.args.manifest?.__taskName === 'string' + ? message.args.manifest.__taskName + : undefined; + + if (taskName && this.addon.hasTaskHandler(taskName)) {
…tion in runSetupAppUpdate
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@application/src/frontend/lib/setup/setup.ts`:
- Around line 353-357: The current in-progress notification is created as a
success before movePrefix finishes; update the flow in the setup logic so that
createNotification for the "Swapping wine prefixes..." message uses a neutral
type (e.g., 'info') while the operation runs, and only emit a createNotification
with type 'success' after the awaited movePrefix(...) completes successfully (or
emit an error notification on catch). Locate the call to createNotification and
the subsequent movePrefix invocation in setup.ts and change the pre-operation
notification type to 'info' or move the success createNotification into the
.then/after-await path, ensuring errors trigger an error notification instead.
In `@packages/ogi-addon/src/main.ts`:
- Around line 73-75: The declared type for the 'input-asked' payload currently
uses ConfigurationBuilder, which is not serializable and doesn't match what
userInputAsked actually sends ({ config, name, description }). Replace the
ConfigurationBuilder reference for the 'input-asked' entry with a plain
serializable payload type (for example an interface/type like { config:
Record<string, string | number | boolean>; name: string; description?: string })
so the declared type matches the wire format emitted by userInputAsked; update
any usages relying on the old type to use the new serializable payload shape and
ensure serialization paths expect the simple object instead of a builder
instance.
♻️ Duplicate comments (1)
application/src/frontend/lib/setup/setup.ts (1)
102-112: Update failures without library info still fall back tofor: 'game'.When
forType === 'update'butcurrentLibraryInfois undefined, the setup failure is saved withfor: 'game', which may misroute retry logic.Consider preserving
for: forTypeunconditionally:Suggested fix
const setupData: Parameters<EventListenerTypes['setup']>[0] = - forType === 'update' && currentLibraryInfo + forType === 'update' ? { ...baseData, for: 'update', - currentLibraryInfo, + ...(currentLibraryInfo ? { currentLibraryInfo } : {}), } : { ...baseData, for: 'game', };
🧹 Nitpick comments (2)
application/src/frontend/lib/setup/setup.ts (2)
336-337: Consider using strict equality (!==) for comparisons.Loose equality (
!=) can lead to unexpected coercion. SincelaunchExecutableandlaunchArgumentscould benull,undefined, or strings, strict comparison is safer.Suggested fix
- beforeLibraryApp?.launchExecutable != data.launchExecutable || - beforeLibraryApp?.launchArguments != data.launchArguments + beforeLibraryApp?.launchExecutable !== data.launchExecutable || + beforeLibraryApp?.launchArguments !== data.launchArguments
362-364: Consider including the result in the error message for debugging.The error message is generic. Including
resultwould help diagnose failures.Suggested fix
if (result !== 'success') { - throw new Error('Failed to move prefix'); + throw new Error(`Failed to move prefix: ${result}`); }
… is used in runSetupAppUpdate
… Linux to maintain save data integrity
… and improve type safety for appID
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@application/src/frontend/components/PlayPage.svelte`:
- Around line 403-462: The onclick async handler uses event.currentTarget after
awaits which becomes null; capture the button element synchronously at the top
(e.g., const btn = event.currentTarget as HTMLButtonElement) and replace all
uses of event.currentTarget.disabled with btn.disabled (including the early
re-enable before throwing on movePrefix failure and the final re-enable in the
finally block), so references in this handler related to libraryInfo.appID,
appUpdates.prefixMoveInfo, createNotification and window.electronAPI.app.* use
the captured btn instead of event.currentTarget.
- Around line 407-452: The prefix-move logic must run before invoking
window.electronAPI.app.addToSteam(libraryInfo.appID); reorder the code so the os
check (window.electronAPI.app.getOS()), appUpdates.prefixMoveInfo lookup,
checkPrefixExists(Number(libraryInfo.appID)), currentPrefix vs
prefixInfo.originalPrefix comparison, createNotification(...) and the call to
window.electronAPI.app.movePrefix(prefixInfo.originalPrefix,
prefixInfo.gameName) (with the same error handling that re-enables
event.currentTarget.disabled and throws on failure) all execute prior to calling
addToSteam; keep the same conditionals and notifications but move them above the
addToSteam call and preserve the use of addToSteam, getOS, checkPrefixExists,
createNotification, and movePrefix.
♻️ Duplicate comments (1)
application/src/frontend/lib/setup/setup.ts (1)
102-112: Failed update recovery still tagged as'game'whencurrentLibraryInfois missing.When
forType === 'update'butcurrentLibraryInfoisundefined, the saved setup data falls back tofor: 'game', which can misroute retry attempts. Consider preserving thefortype while omittingcurrentLibraryInfo.♻️ Proposed fix
const setupData: Parameters<EventListenerTypes['setup']>[0] = forType === 'update' && currentLibraryInfo ? { ...baseData, for: 'update', currentLibraryInfo, } - : { + : forType === 'update' + ? { ...baseData, - for: 'game', - }; + for: 'update', + currentLibraryInfo: baseData as unknown as LibraryInfo, // Placeholder - recovery will need to re-fetch + } + : { + ...baseData, + for: 'game', + };Alternatively, if the
setupevent type requirescurrentLibraryInfofor updates, consider logging a warning that update recovery may not work correctly without it.
🧹 Nitpick comments (1)
application/src/frontend/lib/setup/setup.ts (1)
335-337: Use strict equality (!==) for comparisons.Using
!=instead of!==can lead to unexpected type coercion. Consider using strict equality for consistency.♻️ Proposed fix
if ( (await window.electronAPI.app.getOS()) === 'linux' && - (beforeLibraryApp?.launchExecutable != data.launchExecutable || - beforeLibraryApp?.launchArguments != data.launchArguments) + (beforeLibraryApp?.launchExecutable !== data.launchExecutable || + beforeLibraryApp?.launchArguments !== data.launchArguments) ) {
Comment on lines
407
to
452
| await window.electronAPI.app.addToSteam(libraryInfo.appID); | ||
|
|
||
| // Check if prefix move is needed before adding to Steam | ||
| const os = await window.electronAPI.app.getOS(); | ||
| console.log( | ||
| 'appUpdates.prefixMoveInfo', | ||
| appUpdates.prefixMoveInfo | ||
| ); | ||
| if ( | ||
| os === 'linux' && | ||
| appUpdates.prefixMoveInfo[Number(libraryInfo.appID)] | ||
| ) { | ||
| const prefixInfo = | ||
| appUpdates.prefixMoveInfo[Number(libraryInfo.appID)]; | ||
| const { exists, prefixPath } = | ||
| await window.electronAPI.app.checkPrefixExists( | ||
| Number(libraryInfo.appID) | ||
| ); | ||
| const currentPrefix = exists ? (prefixPath ?? '') : ''; | ||
| console.log('currentPrefix', currentPrefix); | ||
| console.log( | ||
| 'prefixInfo.originalPrefix', | ||
| prefixInfo.originalPrefix | ||
| ); | ||
| console.log('prefixInfo.gameName', prefixInfo.gameName); | ||
| // If current prefix is different from original, move it | ||
| if ( | ||
| currentPrefix !== prefixInfo.originalPrefix && | ||
| currentPrefix !== '' && | ||
| prefixInfo.originalPrefix !== '' | ||
| ) { | ||
| createNotification({ | ||
| id: Math.random().toString(36).substring(2, 9), | ||
| type: 'info', | ||
| message: `Swapping wine prefixes to maintain save data...`, | ||
| }); | ||
| const result = await window.electronAPI.app.movePrefix( | ||
| prefixInfo.originalPrefix, | ||
| prefixInfo.gameName | ||
| ); | ||
| if (result !== 'success') { | ||
| event.currentTarget.disabled = false; | ||
| throw new Error('Failed to move prefix'); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Inconsistent order: prefix move should happen before addToSteam.
In GameConfiguration.svelte, the prefix is moved before calling addToSteam. Here, addToSteam is called first (line 407), then the prefix move happens (lines 443-446). This inconsistency could cause issues if addToSteam modifies the prefix path or Steam shortcut configuration.
🐛 Proposed fix: Move prefix before addToSteam
onclick={async (event) => {
+ const button = event.currentTarget as HTMLButtonElement;
try {
- (event.currentTarget as HTMLButtonElement).disabled = true;
-
- await window.electronAPI.app.addToSteam(libraryInfo.appID);
+ button.disabled = true;
// Check if prefix move is needed before adding to Steam
const os = await window.electronAPI.app.getOS();
- console.log(
- 'appUpdates.prefixMoveInfo',
- appUpdates.prefixMoveInfo
- );
if (
os === 'linux' &&
appUpdates.prefixMoveInfo[Number(libraryInfo.appID)]
) {
// ... prefix move logic ...
}
+
+ await window.electronAPI.app.addToSteam(libraryInfo.appID);
+
appUpdates.requiredReadds = appUpdates.requiredReadds.filter(🤖 Prompt for AI Agents
In `@application/src/frontend/components/PlayPage.svelte` around lines 407 - 452,
The prefix-move logic must run before invoking
window.electronAPI.app.addToSteam(libraryInfo.appID); reorder the code so the os
check (window.electronAPI.app.getOS()), appUpdates.prefixMoveInfo lookup,
checkPrefixExists(Number(libraryInfo.appID)), currentPrefix vs
prefixInfo.originalPrefix comparison, createNotification(...) and the call to
window.electronAPI.app.movePrefix(prefixInfo.originalPrefix,
prefixInfo.gameName) (with the same error handling that re-enables
event.currentTarget.disabled and throws on failure) all execute prior to calling
addToSteam; keep the same conditionals and notifications but move them above the
addToSteam call and preserve the use of addToSteam, getOS, checkPrefixExists,
createNotification, and movePrefix.
…ndling and improve prefix move info assignment
…REFIX support for non-Steam games
…eamline game addition logic for Linux
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
application/src/frontend/lib/setup/setup.ts (1)
340-357: Version mismatch sets success status despite error notification.When
data.version !== downloadedItem.updateVersion, an error notification is shown, but the download status is still set to'setup-complete'or'seeding'. This creates inconsistent UX where the user sees an error but the download appears successful.🐛 Proposed fix
} else { createNotification({ id: Math.random().toString(36).substring(2, 9), type: 'error', message: `Failed to update ${downloadedItem.name} to target version`, }); console.log( 'Failed to update app. Target version: ', data.version, 'Current version: ', downloadedItem.updateVersion ); + updateDownloadStatus(downloadedItem.id, { + status: 'error', + error: `Version mismatch: expected ${downloadedItem.updateVersion}, got ${data.version}`, + }); + return data; } const finalStatus = isTorrent ? 'seeding' : 'setup-complete'; updateDownloadStatus(downloadedItem.id, { status: finalStatus, downloadPath: downloadedItem.downloadPath, });
🤖 Fix all issues with AI agents
In `@application/src/electron/handlers/handler.app.ts`:
- Around line 539-555: The code currently strips any existing WINEPREFIX before
attempting to resolve the Steam app ID and then, on lookup failure, persists the
stripped launch arguments which can unintentionally change the prefix; to fix,
preserve the original appData.launchArguments (save it to a variable) and only
modify/replace WINEPREFIX when getNonSteamGameAppID(versionedGameName) returns
success, moving the regex removal and construction of the new protonPath into
the success branch; on failure leave appData.launchArguments unchanged (or
explicitly reassign the saved original) and optionally log a warning so we don't
silently drop an existing WINEPREFIX.
In `@application/src/frontend/states.svelte.ts`:
- Around line 16-20: Persisted requiredReadds may contain non-numeric values;
when parsing stored JSON in the load path (where parsed and requiredReadds are
used) filter and coerce entries to safe numbers before returning: transform
parsed.requiredReadds by mapping to Number(item) and keep only
Number.isFinite(...) results (or parseInt/parseFloat plus isFinite) so
requiredReadds becomes an array of finite numeric IDs used later for appID
filtering.
- Around line 6-14: Replace uses of relative paths like
'./internals/update-state.json' with a stable user-data base path exposed by the
main process (e.g. expose app.getPath('userData') via the preload bridge such as
window.electronAPI.getUserDataPath()), then join that base with
'internals/update-state.json' before calling
window.electronAPI.fs.exists/mkdir/read; update all similar usages (e.g.
'./library', './config') to use the same userData base to ensure persistence
works in packaged Electron apps and keep using the existing
window.electronAPI.fs methods (exists, mkdir, read) but operating on the joined
user-data paths instead of relative paths.
♻️ Duplicate comments (1)
application/src/frontend/lib/setup/setup.ts (1)
102-112: Update recovery may be misrouted whencurrentLibraryInfois missing.When
forTypeis'update'butcurrentLibraryInfois undefined, the fallback setsfor: 'game', which can cause failed update retries to be routed incorrectly.
🧹 Nitpick comments (1)
application/src/frontend/lib/setup/setup.ts (1)
323-339: Consider using strict equality for config change detection.Lines 325-327 use loose equality (
!=). While this works, strict equality (!==) is more explicit and aligns with TypeScript best practices.♻️ Suggested change
if ( (await window.electronAPI.app.getOS()) === 'linux' && - (beforeLibraryApp?.launchExecutable != data.launchExecutable || - beforeLibraryApp?.launchArguments != data.launchArguments || - beforeLibraryApp?.cwd != data.cwd) + (beforeLibraryApp?.launchExecutable !== data.launchExecutable || + beforeLibraryApp?.launchArguments !== data.launchArguments || + beforeLibraryApp?.cwd !== data.cwd) ) {
… and cleanup for improved reliability
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
application/src/frontend/App.svelte (1)
89-111: Prevent persisted hydration from clobbering early updates (Line 110).If any code adds to
appUpdates.requiredReaddsbefore the delayed callback fires (e.g., update events during boot), this assignment overwrites them. Consider merging or only hydrating when empty.✅ Suggested fix (merge + dedupe)
- appUpdates.requiredReadds = loadPersistedUpdateState().requiredReadds; + const persisted = loadPersistedUpdateState().requiredReadds; + if (persisted.length) { + const merged = new Set([ + ...appUpdates.requiredReadds, + ...persisted, + ]); + appUpdates.requiredReadds = Array.from(merged); + }
…bjects with appID and steamAppId; update related logic for adding games to Steam
…pty screenRendering state
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@application/src/frontend/components/modal/ConfigurationModal.svelte`:
- Around line 251-277: The modal can become undismissable when
boundsClose={false} and screenRendering contains only action options; update the
rendering logic in ConfigurationModal.svelte so a Close/Submit control is always
shown when there are no non-action inputs. Specifically, in the block that
decides whether to render the primary ButtonModal (currently using
Object.keys(screenRendering).length === 0 ||
Object.keys(screenRendering).some(...)), compute a hasNonAction variable (e.g.,
const hasNonAction = Object.keys(screenRendering).some(key =>
!isActionOption(screenRendering[key]))) and change the condition to render the
primary button when screenRendering is empty OR hasNonAction is true OR when
boundsClose is false (to ensure a Close is available). Ensure the primary
ButtonModal calls the existing handleSubmit/close handler and retains its text
switching (Close vs Submit).
…ned and add debug logging for migration checks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@application/src/electron/handlers/handler.app.ts`:
- Around line 993-998: getNonSteamGameAppID is only called with the versioned
name, causing false “prefix missing” when shortcuts still use the plain game
name; change the flow in the block that computes versionedGameName (and uses
getNonSteamGameAppID) to attempt a legacy fallback: if
getNonSteamGameAppID(versionedGameName) returns !success, call
getNonSteamGameAppID(libraryInfo.name) and use that result before concluding
missing prefixes; update any downstream logic that inspects success/appId to
prefer the versioned result but accept the plain-name fallback so the code that
computes prefix-missing state uses the fallback appId when available.
- Around line 951-956: The current call uses
getNonSteamGameAppID(versionedGameName) which can fail for games already present
under the plain name; change the logic in the block around
getVersionedGameName/getNonSteamGameAppID so that if the first lookup returns
success === false, you call getNonSteamGameAppID again with appInfo.name (plain
name) as a fallback and use that result (appId/success) for subsequent logic;
ensure you reference versionedGameName, appInfo.name, and getNonSteamGameAppID
when implementing the fallback.
- Around line 1037-1042: The current lookup only tries the versioned name from
getVersionedGameName and bails when getNonSteamGameAppID returns !success;
update the flow so that if getNonSteamGameAppID(versionedGameName) fails, you
retry getNonSteamGameAppID(appInfo.name) (the plain Steam name) before treating
it as a failure; use the returned { success, appId } from the retry to proceed
and ensure any downstream logic that uses appId can handle whichever result
succeeded.
🧹 Nitpick comments (1)
application/src/electron/handlers/handler.app.ts (1)
594-783: Avoid blocking the main process during prefix migration.
renameSync+ recursive copy/delete can freeze the UI for large prefixes. Consider async FS (fs.promises) or offloading to a worker process.
…d improve logging for better clarity
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@application/src/electron/handlers/handler.app.ts`:
- Around line 669-779: The migration currently updates appInfo.launchArguments
to the new WINEPREFIX unconditionally, which breaks installs if migration fails;
modify the flow in the block that handles compatdata migration (the try/catch
around fs.renameSync/copyRecursiveSync and the catch handling migrationError) to
only set appInfo.launchArguments (the protonPath assignment and fs.writeFileSync
call) after a successful rename/copy path or when oldAppIdDir is absent but no
error occurred; alternately save the original appInfo.launchArguments into a
temp variable before attempting migration and restore it in the catch
(migrationError) branch before calling fs.writeFileSync, and ensure
sendNotification remains correct for success vs error cases.
♻️ Duplicate comments (3)
application/src/electron/handlers/handler.app.ts (3)
951-956: Legacy-name fallback still missing here.
Same issue as previously flagged: if the versioned lookup fails, retry with the plain name to support legacy shortcuts.
993-998: Legacy-name fallback still missing here.
Same issue as previously flagged: if the versioned lookup fails, retry with the plain name to avoid false “prefix missing.”
1037-1042: Legacy-name fallback still missing here.
Same issue as previously flagged: add a plain-name retry before failing redistributable installs.
…n, and register sub-handlers for improved modularity
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
application/src/electron/handlers/handler.app.ts (2)
188-211: WrapJSON.parsein try-catch to handle malformed addon.json.If
addon.jsoncontains invalid JSON,JSON.parsewill throw an unhandled exception, crashing the IPC handler.🐛 Suggested fix
ipcMain.handle('app:get-addon-icon', async (_, addonID: string) => { let client = clients.get(addonID); if (!client || !client.filePath) { return null; } - // read the addon.json file to get the icon path - const addonJson = JSON.parse( - fs.readFileSync(join(client.filePath, 'addon.json'), 'utf-8') - ); + let addonJson; + try { + addonJson = JSON.parse( + fs.readFileSync(join(client.filePath, 'addon.json'), 'utf-8') + ); + } catch (err) { + console.error('Failed to parse addon.json for addon: ' + addonID, err); + return null; + } if (!addonJson.icon) { return null; }
212-236: Path traversal risk inapp:get-local-image.The handler reads any file the caller specifies without validating the path. A malicious renderer script could read arbitrary files (e.g.,
/etc/passwd,~/.ssh/id_rsa) by passing their paths. Consider restricting reads to known safe directories.🔒 Suggested mitigation
ipcMain.handle('app:get-local-image', async (_, filePath: string) => { + // Validate path is within allowed directories + const resolvedPath = path.resolve(filePath); + const allowedPrefixes = [ + app.getPath('userData'), + os.homedir() + '/Pictures', + // Add other safe directories as needed + ]; + const isAllowed = allowedPrefixes.some(prefix => resolvedPath.startsWith(prefix)); + if (!isAllowed) { + console.error('Attempted to read image outside allowed directories:', filePath); + return null; + } + - if (!fs.existsSync(path)) { + if (!fs.existsSync(resolvedPath)) { return null; } - const ext = path.split('.').pop()?.toLowerCase() || 'png'; + const ext = resolvedPath.split('.').pop()?.toLowerCase() || 'png'; // ... rest of handler
🤖 Fix all issues with AI agents
In `@application/src/electron/handlers/helpers.app/steam.ts`:
- Around line 51-53: The exec call that builds a shell string with
`${STEAMTINKERLAUNCH_PATH} getid "${escapeShellArg(gameName)}"` is unsafe for
paths with spaces and injection-prone; replace it with child_process.execFile
using STEAMTINKERLAUNCH_PATH as the executable and args ['getid', gameName]
(remove escapeShellArg) and pass { cwd: __dirname } as options so arguments are
passed directly without shell interpretation; update the call site where exec is
invoked to use execFile and preserve existing callback/error handling.
In `@application/src/electron/handlers/library-handlers.ts`:
- Around line 189-193: The current catch in the redistributable install loop
only logs the error for each redistributable (the catch around
redistributable.name / data.name) but the handler still returns 'setup-success';
add a boolean failure flag (e.g., redistributableFailed) that you set to true
inside the catch for each failed install, keep logging the error, and after the
install loop check that flag and return 'setup-redistributables-failed' instead
of 'setup-success' when any install failed; update any references to the
handler's return points to prefer the new failure status.
- Around line 178-183: The loop that calls spawnSync for each redistributable
uses spawnSync(redistributable.path, { shell: true }), which allows shell
interpretation of a user-controlled path; change it to avoid shell execution by
using shell: false and passing an explicit argv array (or validate/sanitize
redistributable.path to ensure it points to an expected executable), e.g. update
the code around the for-loop referencing redistributable.path and the spawnSync
call to either validate the path against a whitelist/exists check or call
spawnSync with shell: false and explicit args so no shell metacharacters are
interpreted.
In `@application/src/electron/handlers/redistributable-handlers.ts`:
- Around line 220-241: The setTimeout used in the spawn blocks leaves a timer
alive after the child process exits, risking killing unrelated processes; update
each spawn block (the child variable and its listeners in
redistributable-handlers.ts) to store the timeout id before attaching listeners
and implement a single finalize function that clears the timeout, removes or
ignores extra listeners, and then resolves the Promise; call finalize from the
'close' and 'error' handlers as well as before invoking child.kill('SIGTERM') in
the timeout callback so the timer is cleared and resolution happens exactly
once.
- Around line 106-121: Wrap the getProtonPrefixPath(appId) call in a try/catch
to handle cases where it throws (e.g., missing HOME/USERPROFILE); on catch, call
sendNotification with a clear error message and generateNotificationId() and
return 'failed' so the IPC handler fails gracefully instead of crashing. Ensure
the existing fs.existsSync(protonPath) check still runs when getProtonPrefixPath
succeeds, and log the caught error (or include it in the notification) for
debugging. Target the code around getProtonPrefixPath, fs.existsSync,
sendNotification and generateNotificationId in the redistributable handler.
- Around line 142-345: The Promise executor is declared async which can swallow
rejections; remove the async keyword from the new Promise executor and move the
existing async logic into an inner async IIFE (e.g. (async () => { ... })()) for
each branch, then attach a .catch(err => { console.error(..., err);
resolve(false); }) to that IIFE so any thrown errors from getSilentInstallFlags,
dirname, basename, or other synchronous code are caught and cause
resolve(false); keep existing child process listeners and timeouts inside the
IIFE so they still call resolve(true/false) on success/failure.
In `@application/src/electron/handlers/steam-handlers.ts`:
- Around line 89-103: The spawn('steam', ...) call in the promise (variable
child) lacks an error handler so failures are unobserved and the promise always
resolves; attach an 'error' event listener to child (and clear the pending
setTimeout) so on error you resolve/return { success: false, error: err.message
} (or reject) instead of resolving success, and also consider listening for
'exit' to detect immediate failures; update the Promise around the spawn in
steam-handlers.ts to use child.on('error', ...) and cancel the success timeout
when an error occurs.
🧹 Nitpick comments (6)
application/src/electron/handlers/helpers.app/notifications.ts (1)
6-7: Prefercrypto.randomUUID()for notification IDs.
Math.random()can collide under bursts; UUIDs are safer for UI notification tracking.♻️ Proposed update
+import { randomUUID } from 'crypto'; + export function generateNotificationId(): string { - return Math.random().toString(36).substring(7); + return randomUUID(); }application/src/electron/handlers/helpers.app/platform.ts (1)
9-18: Useos.homedir()and guard Linux-only compat paths.This avoids missing/incorrect home detection and prevents accidental use on non-Linux platforms.
♻️ Proposed update
+import { homedir } from 'os'; + export function getHomeDir(): string | null { - return process.env.HOME || process.env.USERPROFILE || null; + const dir = homedir(); + return dir || null; } export function getCompatDataDir(): string { + if (!isLinux()) { + throw new Error('Compatdata directory is only available on Linux'); + } const homeDir = getHomeDir(); if (!homeDir) { throw new Error('Home directory not found'); }application/src/electron/handlers/library-handlers.ts (4)
32-38: DuplicateescapeShellArgfunction.This function is identical to the one exported from
handler.app.ts(lines 19-27). Import it from there instead of duplicating.♻️ Suggested refactor
-/** - * Escapes a string for safe use in shell commands by escaping special characters - */ -function escapeShellArg(arg: string): string { - return arg - .replace(/\\/g, '\\\\') - .replace(/"/g, '\\"') - .replace(/\$/g, '\\$') - .replace(/`/g, '\\`'); -} +import { escapeShellArg } from './handler.app.js';
45-48: Return a consistent value when app info is not found.Returning
undefinedimplicitly makes the IPC contract unclear. Consider returning an error object or throwing to signal failure to the renderer.♻️ Suggested fix
const appInfo = loadLibraryInfo(appid); if (!appInfo) { - return; + return { success: false, error: 'App not found' }; }
57-59: Shell interpretation requires explicitshell: trueoption.The command string uses shell escaping (
escapeShellArg), butexecwithout options defaults to shell execution which varies by platform. Explicitly setshell: trueto ensure the command is interpreted correctly.♻️ Suggested fix
const spawnedItem = exec(args, { cwd: appInfo.cwd, + shell: true, });
117-121: RedundantsaveLibraryInfocall.
saveLibraryInfo(data.appID, data)is called at line 120, and again at line 171 whenhasRedistributablesis true. Line 154 also saves. Consider consolidating saves or only saving at line 154/171 after all modifications are complete.Also applies to: 169-171
Comment on lines
189
to
193
| } catch (error) { | ||
| console.error( | ||
| `[redistributable] failed to install ${redistributable.name} for ${data.name}: ${error}` | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Errors from redistributable installation are swallowed.
The catch block logs the error but doesn't return a failure status. The handler still returns 'setup-success' even if all redistributables fail. Consider tracking failures and returning 'setup-redistributables-failed'.
🐛 Suggested fix
+ let redistributableFailed = false;
if (data.redistributables && data.redistributables.length > 0) {
for (const redistributable of data.redistributables) {
try {
spawnSync(redistributable.path, {
stdio: 'inherit',
shell: true,
});
sendNotification({
message: `Installed ${redistributable.name} for ${data.name}`,
id: generateNotificationId(),
type: 'success',
});
} catch (error) {
+ redistributableFailed = true;
console.error(
`[redistributable] failed to install ${redistributable.name} for ${data.name}: ${error}`
);
}
}
}
+ if (redistributableFailed) {
+ return 'setup-redistributables-failed';
+ }🤖 Prompt for AI Agents
In `@application/src/electron/handlers/library-handlers.ts` around lines 189 -
193, The current catch in the redistributable install loop only logs the error
for each redistributable (the catch around redistributable.name / data.name) but
the handler still returns 'setup-success'; add a boolean failure flag (e.g.,
redistributableFailed) that you set to true inside the catch for each failed
install, keep logging the error, and after the install loop check that flag and
return 'setup-redistributables-failed' instead of 'setup-success' when any
install failed; update any references to the handler's return points to prefer
the new failure status.
…andling and path validation
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@application/src/electron/handlers/library-handlers.ts`:
- Around line 133-159: The code calls addGameToSteam(...) and then proceeds to
resolve Steam ID and call getProtonPrefixPath/saveLibraryInfo even if
addGameToSteam failed; move the failure check for the addGameToSteam result
immediately after the call and return 'setup-failed' if result is falsy so you
do not call getNonSteamGameAppID, getProtonPrefixPath, or saveLibraryInfo when
the Steam add failed. Locate addGameToSteam, getNonSteamGameAppID,
getProtonPrefixPath, saveLibraryInfo, and getVersionedGameName in the snippet
and ensure data.launchArguments and data state are only modified/saved after a
successful addGameToSteam.
In `@application/src/electron/handlers/redistributable-handlers.ts`:
- Around line 142-169: The current install loop over appInfo.redistributables
clears/saves the redistributables and returns success even if some installs
failed; change the logic in the handler that iterates appInfo.redistributables
(the block that sends sendNotification and creates the new Promise with
finalize/resolve/timeoutId/resolved) to accumulate per-item failures (e.g., a
boolean or failedItems array), do not clear or overwrite
appInfo.redistributables unless all items succeeded, and return a failure status
when any install failed; ensure finalize/resolve still runs per-item but the
post-loop code checks the aggregated success before calling the code that
clears/saves the redistributables or reports overall success.
♻️ Duplicate comments (2)
application/src/electron/handlers/helpers.app/steam.ts (1)
4-20: Avoid shell string invocation for SteamTinkerLaunch.
Line 135 builds a shell command; paths with spaces inSTEAMTINKERLAUNCH_PATHwill break and escaping remains fragile. PreferexecFilewith argv (as already done ingetNonSteamGameAppID).🔧 Suggested fix
-import { exec, execFile } from 'child_process'; +import { execFile } from 'child_process'; @@ -/** - * Escapes a string for safe use in shell commands by escaping special characters - */ -function escapeShellArg(arg: string): string { - // Replace any backslashes first (to avoid double-escaping) - // Then escape double quotes, dollar signs, backticks, and backslashes - return arg - .replace(/\\/g, '\\\\') - .replace(/"/g, '\\"') - .replace(/\$/g, '\\$') - .replace(/`/g, '\\`'); -} @@ - return new Promise<boolean>((resolve) => - exec( - `${STEAMTINKERLAUNCH_PATH} addnonsteamgame --appname="${escapeShellArg(versionedGameName)}" --exepath="${escapeShellArg(params.launchExecutable)}" --startdir="${escapeShellArg(params.cwd)}" --launchoptions="${escapeShellArg(params.launchOptions)}" --compatibilitytool="proton_experimental" --use-steamgriddb`, - { - cwd: __dirname, - }, - (error, stdout, stderr) => { + return new Promise<boolean>((resolve) => + execFile( + STEAMTINKERLAUNCH_PATH, + [ + 'addnonsteamgame', + `--appname=${versionedGameName}`, + `--exepath=${params.launchExecutable}`, + `--startdir=${params.cwd}`, + `--launchoptions=${params.launchOptions}`, + '--compatibilitytool=proton_experimental', + '--use-steamgriddb', + ], + { cwd: __dirname }, + (error, stdout, stderr) => { if (error) { console.error(error); notifyError('Failed to add game to Steam');Also applies to: 125-153
application/src/electron/handlers/library-handlers.ts (1)
178-206: Windows redistributable failures never flip the failure flag.
Because the failure assignment is commented out, the handler always returns success even when installs fail.🔧 Suggested fix
} catch (error) { - // TODO: for now do this, because some wine stuff fails - // redistributableFailed = true; + redistributableFailed = true; console.error( `[redistributable] failed to install ${redistributable.name} for ${data.name}: ${error}` ); }
…e recommended addons from others and enhance UI for displaying results
…date logic for enhanced addon management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@application/src/frontend/lib/setup/setup.ts`:
- Around line 352-362: The code currently pushes an entry with steamAppId: 0
into appUpdates.requiredReadds when getSteamAppId fails; instead, avoid adding a
sentinel that can break downstream consumers. Update the block handling
downloadedItem.appID so that if getSteamAppId returns no valid ID you do not
append the { appID, steamAppId } entry to appUpdates.requiredReadds (keep only
the existing filter logic), or alternatively use an explicit undefined/-1 and
update the type of appUpdates.requiredReadds and all consumers that read
steamAppId; focus your change around the failed-get path that references
downloadedItem.appID, getSteamAppId, and appUpdates.requiredReadds.
- Around line 325-328: The boolean expression comparing OS and launch fields
uses loose inequality operators; update the three comparisons inside the
conditional that reference beforeLibraryApp?.launchExecutable,
beforeLibraryApp?.launchArguments, and beforeLibraryApp?.cwd to use strict
inequality (!==) instead of != so the checks (in the code around
window.electronAPI.app.getOS() === 'linux' && ...) avoid type-coercion issues.
♻️ Duplicate comments (1)
application/src/frontend/lib/setup/setup.ts (1)
102-112: Failed update recovery still falls back to'game'whencurrentLibraryInfois missing.When
forType === 'update'butcurrentLibraryInfois undefined, the payload defaults to{ for: 'game' }. While this matches theEventListenerTypes['setup']discriminated union (which requirescurrentLibraryInfofor update), it means retry attempts for failed updates may lose their update context.Consider whether this fallback is intentional—if so, a comment explaining the design decision would help maintainability.
🧹 Nitpick comments (2)
application/src/frontend/components/built/UpdateAppModal.svelte (2)
65-95: Stabilize “Other Sources” ordering.Group order currently depends on which addon resolves first, which can shuffle the list between runs. Consider sorting
otherAddons(e.g., byaddonId) to keep the UI stable.♻️ Suggested tweak
const originalAddonSource = libraryInfo.addonsource; let recommendedAddon: AddonGroup | null = null; const otherAddons: AddonGroup[] = []; addonMap.forEach((results, addonId) => { const addonGroup: AddonGroup = { addonId, addonName: addonId, results, }; if (addonId === originalAddonSource) { recommendedAddon = addonGroup; } else { otherAddons.push(addonGroup); } }); + otherAddons.sort((a, b) => a.addonId.localeCompare(b.addonId)); return { recommendedAddon, otherAddons };
283-475: Consider extracting the repeated addon section UI.Both sections duplicate the same header + result card markup. Extracting a small reusable section component would reduce drift and simplify future tweaks.
…rate condition checks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.