feat: client sdk for application layer, migration away from "safeFetch"#127
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a typed protocol library ( ChangesCore protocol library (
Addon server and addon SDK refactor
Client SDK and application migration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: b17ec5ac4a
ℹ️ 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".
Comment on lines
+675
to
+677
| deferID: | ||
| message.args as AddonClientToServerEventArgs['defer-update']['deferID'], | ||
| progress: setupEvent.progress, |
There was a problem hiding this comment.
In the setup message handler, deferID is set to message.args (the whole setup payload) instead of the task identifier. That value is then used by the server to look up the deferred task, so progress updates for setup resolve to an invalid key (e.g. [object Object]) and are rejected as an invalid defer ID, which can terminate the addon connection while setup is running. This is user-impacting for setup flows that take longer than the first progress tick.
Useful? React with 👍 / 👎.
Comment on lines
+16
to
+19
| server.emit( | ||
| 'notification', | ||
| message.args as AddonClientToServerEventArgs['notification'] | ||
| ); |
There was a problem hiding this comment.
This now forwards message.args directly, but the addon SDK still sends notifications as [notification] (see OGIAddon.notify). As a result, SDK clients receive an array instead of the expected { type, message, id } object, so frontend notification handlers that validate detail.type/detail.message will drop addon notifications.
Useful? React with 👍 / 👎.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
packages/connection/README.md (1)
1-16: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd meaningful library documentation.
This README is too minimal for a library package. It lacks:
- What the package does (typed protocol for addon communication)
- API overview or links
- Usage examples (how to import and use the protocol, socket, types)
- Integration guidance
The "To run: bun run lib/index.ts" instruction is misleading—libraries are imported, not run directly.
📚 Suggested README structure
# `@ogi-sdk/connect` Typed WebSocket protocol for OGI addon communication. ## Installation ```bash bun install `@ogi-sdk/connect`Usage
import { defineProtocol, EventResponseSocket } from '`@ogi-sdk/connect`'; // Example protocol usage const protocol = defineProtocol({ // your protocol definition });API
defineProtocol()- Define typed event protocolsEventResponseSocket- Request/response WebSocket transportupgradeToWebSocket()- HTTP upgrade helperSee the API documentation for details.
Development
bun install bun testThis project was created using
bun initin bun v1.3.13.</details> Based on learnings: Document complex logic and non-obvious design decisions with clear explanations. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@packages/connection/README.mdaround lines 1 - 16, Update the minimal README
to be a real library doc: explain that@ogi-sdk/connectis a typed WebSocket
protocol for addon communication, remove or change the misleading "To run: bun
run lib/index.ts" note (libraries are imported, not run), and add sections for
Installation, Usage examples showing import of defineProtocol and
EventResponseSocket, brief API overview referencing defineProtocol,
EventResponseSocket, and upgradeToWebSocket, and a short Development section
(install/test/build) plus a link to fuller API docs; keep explanations for any
non-obvious design choices and integration guidance for consumers.</details> </blockquote></details> <details> <summary>packages/addon-server/lib/server.ts (1)</summary><blockquote> `143-159`: _⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_ **Clean up addon connections on socket close as well.** The new `/sdk` branch removes closed SDK clients from `sdkConnections`, but the addon branch still leaves closed addons in `connections` and `clients`. That will make `query-connected-addons()` return stale entries after a disconnect. <details> <summary>Suggested change</summary> ```diff const connection = new AddonConnection(ws, this.config, this); this.connections.add(connection); + ws.on('close', () => { + this.connections.delete(connection); + if (connection.addonInfo?.id) { + this.clients.delete(connection.addonInfo.id); + } + }); connection.setupWebsocket().then((success) => { if (!success) { this.connections.delete(connection); this.eventEmitter.emit('disconnect', 'Failed to setup websocket');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/addon-server/lib/server.ts` around lines 143 - 159, The AddonConnection branch doesn't remove closed addon sockets from this.connections (and any this.clients mapping), leaving stale entries; add a ws.on('close', ...) handler after creating AddonConnection in the connection callback (same pattern as the SDK branch) that deletes the connection from this.connections, removes any corresponding entry from this.clients (if applicable), and emits the 'disconnect' event with a clear reason; locate the code around AddonConnection construction/connection.setupWebsocket() to insert the close handler so closed addon sockets are cleaned up properly.application/src/frontend/components/built/UpdateAppModal.svelte (1)
82-86:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the mapped addon display name when building groups.
resultsalready carriesaddonName: addon.name, but the grouping step overwrites it with the raw id. The modal will keep showing internal addon ids instead of the friendly names you just added.Suggested fix
const addonGroup: AddonGroup = { addonId, - addonName: addonId, + addonName: results[0]?.addonName ?? addonId, results, };Also applies to: 157-159
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/src/frontend/components/built/UpdateAppModal.svelte` around lines 82 - 86, When building AddonGroup in addonMap.forEach, don't overwrite the friendly name with the raw addonId; use the precomputed display name from results (e.g. set addonName: results.addonName || addonId or addonName: results[0]?.addonName || addonId if results is an array) instead of addonName: addonId. Apply the same change to the other construction at lines 157-159 where addonName is currently set to addonId so the modal shows the friendly addon name.application/src/frontend/components/StorePage.svelte (1)
237-243:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
gameDatabefore copying its images into search results.If no addon returns store details,
gameDatastaysundefined, and this mapper will throw as soon as the first search result arrives.Suggested fix
return { ...result, - coverImage: (gameData as StoreData).coverImage, - capsuleImage: (gameData as StoreData).capsuleImage, + coverImage: gameData?.coverImage ?? '', + capsuleImage: gameData?.capsuleImage ?? '', name: result.name, addonSource: addon.id, addonName: addon.name,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/src/frontend/components/StorePage.svelte` around lines 237 - 243, The mapper unconditionally reads images from gameData which can be undefined; update the mapping in the searchResults.map callback to guard gameData before copying coverImage and capsuleImage (e.g., use a conditional or fallback values) so that when gameData is undefined the mapped result still returns safely (keep name and other result fields unchanged). Locate the mapping that builds mappedResults from searchResults (the searchResults.map arrow function that spreads result and sets coverImage/capsuleImage) and replace the direct casts to (gameData as StoreData).coverImage / .capsuleImage with guarded accesses or default values.application/src/frontend/views/ConfigView.svelte (1)
409-416:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused
.update-badge/.update-iconselectors.These selectors are currently unused and are already flagged by CI warnings.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/src/frontend/views/ConfigView.svelte` around lines 409 - 416, Remove the unused CSS selectors `.update-badge` and `.update-icon` from ConfigView.svelte: locate the rules for these classes in the component's style block and delete both selector blocks (the `.update-badge { ... }` and `.update-icon { ... }` entries) so the CI warning is resolved and no dead styles remain.application/src/frontend/lib/tasks/runner.ts (1)
97-101:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSet failed tasks to error status when
onFailedfires.At Line 97, the task only gets
failedtext but keepsstatus: 'running'. That can leave the task visually/semantically in the wrong state.Suggested fix
onFailed: (error: string) => { if (!taskID) return; deferredTasks.update((tasks) => - tasks.map((t) => (t.id === taskID ? { ...t, failed: error } : t)) + tasks.map((t) => + t.id === taskID ? { ...t, status: 'error', failed: error } : t + ) ); createNotification({ id: Math.random().toString(36).substring(7), type: 'error', message: error, }); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/src/frontend/lib/tasks/runner.ts` around lines 97 - 101, The onFailed handler currently only sets the failed message but leaves the task's status as 'running'; update the deferredTasks.update callback inside onFailed (the function referencing taskID and tasks.map) to return the task with both failed: error and status: 'failed' for the matching t.id === taskID so the task's status and UI reflect the failure.application/src/frontend/views/ClientOptionsView.svelte (1)
1231-1233:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused
.input-numberselectors to clear current pipeline warnings.The warnings point to selectors that are currently unused in this component (Line 1231-1233 and Line 1370-1373). Removing or wiring them to an actual element will clean CI noise.
Also applies to: 1370-1373
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/src/frontend/views/ClientOptionsView.svelte` around lines 1231 - 1233, Remove the unused .input-number selector from the component styles (or wire it to an element) to eliminate CI warnings: inspect the stylesheet block that defines ".input-number, .input-select" and either delete ".input-number" from that selector or add the .input-number class to the relevant input element(s) in the template (the same place where .input-select is used) so the selector is actually applied; update any related styles to keep .input-select intact if needed.
🟡 Minor comments (7)
packages/connection/lib/socket.ts-212-231 (1)
212-231:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGate
send()onreadyState === 1(OPEN) and fix misleading closed-state handling.
EventResponseSocket.sendonly rejects whenreadyState === 3; forCONNECTING/CLOSINGit still callssocket.send, which may throw implementation-specific errors. Add anOPENguard (readyState !== 1) before sending.The proposed “register pending responses before
send” ordering change isn’t necessary here:resolveIncomingResponseis only reached afterawait this.parseRawMessage(...)inhandleRawMessage, so correlation can’t run in the same synchronous call stack assocket.send.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/connection/lib/socket.ts` around lines 212 - 231, EventResponseSocket.send currently only rejects when readyState === 3 and still calls this.socket.send for CONNECTING/ CLOSING; change the guard to only allow sending when this.socket.readyState === 1 (OPEN) and otherwise reject with a clear error (include the readyState value), so socket.send is never invoked in non-OPEN states. Keep the pendingResponses registration logic as-is (no reorder needed) because resolveIncomingResponse runs asynchronously after parseRawMessage in handleRawMessage; reference EventResponseSocket.send, this.socket.readyState, this.socket.send, this.pendingResponses, handleRawMessage, resolveIncomingResponse, and parseRawMessage when locating where to apply the check and error.packages/client-kit/.gitignore-15-16 (1)
15-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix log ignore globs to prevent generated logs from being committed.
The current patterns are too narrow and likely miss standard log/report files.
Suggested diff
-_.log -report.[0-9]_.[0-9]_.[0-9]_.[0-9]_.json +*.log +report.[0-9]*.[0-9]*.[0-9]*.[0-9]*.json🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/client-kit/.gitignore` around lines 15 - 16, Update the gitignore globs so generated logs and report JSONs aren't missed: replace the literal/incorrect pattern "_.log" with a broader "*.log" and replace the narrow "report.[0-9]_.[0-9]_.[0-9]_.[0-9]_.json" pattern with "report.*.json" (or add both "report.*.json" and "report.*_*.json" if you expect additional underscore-delimited variants) so standard log and report files are correctly ignored.application/src/electron/main.ts-467-468 (1)
467-468:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAwait startup IPC messages to guarantee order and avoid dropped readiness sequencing.
Both sends are async; firing them without
awaitcan reorder delivery under the readiness gate and make renderer startup state nondeterministic.Proposed fix
- sendIPCMessage('all-addons-started'); - sendIPCMessage('addon-runtime-ready'); + await sendIPCMessage('all-addons-started'); + await sendIPCMessage('addon-runtime-ready');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/src/electron/main.ts` around lines 467 - 468, The two async calls to sendIPCMessage('all-addons-started') and sendIPCMessage('addon-runtime-ready') can race; change the call sites in main.ts to await sendIPCMessage(...) so delivery order is preserved and the readiness sequencing cannot be dropped—await sendIPCMessage('all-addons-started') first, then await sendIPCMessage('addon-runtime-ready') to guarantee deterministic renderer startup state.application/src/frontend/components/GameConfiguration.svelte-78-79 (1)
78-79:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve empty-string defaults for select-style options.
Using
||here treats''as missing and silently swaps in the first allowed value. If an addon intentionally uses the empty string as its default, the form now renders the wrong selection.Suggested fix
- formData[key] = option.defaultValue || option.allowedValues![0]; + formData[key] = option.defaultValue ?? option.allowedValues![0];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/src/frontend/components/GameConfiguration.svelte` around lines 78 - 79, In GameConfiguration.svelte the assignment to formData[key] uses `option.defaultValue || option.allowedValues![0]`, which treats an empty string as falsy and overwrites intentional '' defaults; change the logic in the select-style branch (the code referencing option.allowedValues, option.defaultValue and formData[key]) to use a nullish check (e.g. test for undefined/null) or the nullish coalescing operator so that '' is preserved and only undefined/null falls back to option.allowedValues![0].application/src/frontend/components/modal/ConfigurationModal.svelte-84-85 (1)
84-85:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse nullish fallback instead of boolean fallback for string defaults.
option.defaultValue || option.allowedValues![0]will replace valid empty-string defaults. Use??so onlynull/undefinedfalls back.Also applies to: 151-153
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/src/frontend/components/modal/ConfigurationModal.svelte` around lines 84 - 85, The code assigns defaults using a boolean fallback which treats empty strings as falsey; update places that set formData[key] from option.defaultValue or option.allowedValues[0] (e.g., the assignment inside the ConfigurationModal component where formData[key] = option.defaultValue || option.allowedValues![0]) to use the nullish coalescing operator (??) so only null/undefined fall back; apply the same change to the other occurrence around lines 151-153 so empty-string defaults are preserved.application/src/frontend/views/ConfigView.svelte-30-37 (1)
30-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd error handling to addon polling requests.
Polling uses promises without rejection handling, so transient connection errors can produce repeated unhandled rejections.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/src/frontend/views/ConfigView.svelte` around lines 30 - 37, The polling and initial load call to queryConnectedAddons<ConfigTemplateAndInfo>() use .then(...) without rejection handling which leads to unhandled promise rejections; update both the initial call and the setInterval callback (references: queryConnectedAddons, ConfigTemplateAndInfo, pollingInterval, addons) to append .catch(...) (or use try/catch with async/await) that handles errors—e.g., log the error and avoid throwing, optionally set a retry/backoff or clearInterval on fatal errors—so all promise rejections are handled and do not surface as unhandled exceptions.application/src/frontend/lib/tasks/deferred.ts-22-27 (1)
22-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
erroris populated with a boolean instead of a message.
error: task.failed || undefinedcan producetrue, which is not a useful error payload for UI or logs.Based on learnings: "Use meaningful error messages that help developers understand what went wrong and why".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/src/frontend/lib/tasks/deferred.ts` around lines 22 - 27, The returned object currently sets error to a boolean via "error: task.failed || undefined"; change this so error contains a descriptive string: replace that expression with a string-valued error payload derived from task (e.g., task.errorMessage || task.error || (task.failed === true ? lastLogLineFrom(task.logs) : undefined)), ensuring "task" properties referenced exist and fall back to undefined if none are present; update the error field in the same object in deferred.ts where "failed", "logs", "timestamp", "duration", and "type" are set.
🧹 Nitpick comments (3)
application/package.json (1)
36-36: ⚡ Quick winReplace
nodewithbunfor consistency.The
devscript usesnode -p "require('electron')"to resolve the Electron executable path. As per coding guidelines, Bun should be used instead of Node.♻️ Suggested change
- "dev": "bun run build:all && ELECTRON_EXEC_PATH=\"$(node -p \"require('electron')\" )\" electron-vite dev", + "dev": "bun run build:all && ELECTRON_EXEC_PATH=\"$(bun -p \"require('electron')\" )\" electron-vite dev",Based on learnings: Use
bun <file>instead ofnode <file>orts-node <file>for running files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/package.json` at line 36, Update the "dev" npm script to use Bun instead of Node: locate the "dev" script entry that sets ELECTRON_EXEC_PATH using node -p "require('electron')", and change that invocation to the equivalent Bun command so the environment variable resolution uses Bun; ensure the rest of the command (the electron-vite dev part) remains unchanged and that the script still exports ELECTRON_EXEC_PATH correctly.packages/addon-server/lib/connections/client.connection.ts (1)
110-110: ⚡ Quick winUse a collision-resistant deferred task ID.
Math.random().toString(36).substring(7)can collide, and a collision here would overwrite another deferred task becausetaskIDis the lookup key.Suggested change
- const taskID = Math.random().toString(36).substring(7); + const taskID = crypto.randomUUID();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/addon-server/lib/connections/client.connection.ts` at line 110, The generated taskID using Math.random().toString(36).substring(7) is not collision-resistant; replace that line in client.connection.ts (the taskID assignment in the deferred task creation path) with a collision-safe ID generator such as crypto.randomUUID() (or a nanoid/uuid.v4 import if targeting older runtimes), and update imports/types as needed so the rest of the code that looks up deferred tasks by taskID continues to work unchanged.packages/addon-server/scripts/generate-event-proxy.ts (1)
50-54: ⚡ Quick winUse
interfacefor emitted object-shape types in generated TS.
EventMessagePackersis generated as atypeobject shape. Please emit it as aninterfaceto keep addon-server TypeScript output aligned with repo conventions.♻️ Proposed update in the generator template
-type EventMessagePackers = { +interface EventMessagePackers { [Event in AddonServerToClientEventName]: ( args: AddonServerToClientEventArgs[Event] ) => Omit<AddonServerToClientWebsocketMessage, 'event'>; -}; +}As per coding guidelines,
packages/addon-server/**/*.{ts,tsx}: Preferinterfaceovertypefor defining object shapes in TypeScript.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/addon-server/scripts/generate-event-proxy.ts` around lines 50 - 54, The generated declaration for EventMessagePackers should be emitted as an interface rather than a type alias: update the generator template that produces the EventMessagePackers declaration so it emits "interface EventMessagePackers { [Event in AddonServerToClientEventName]: (args: AddonServerToClientEventArgs[Event]) => Omit<AddonServerToClientWebsocketMessage, 'event'>; }" (preserving the mapped/index signature and referenced symbols AddonServerToClientEventName, AddonServerToClientEventArgs, and AddonServerToClientWebsocketMessage) so the output conforms to the repo convention of using interfaces for object shapes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@application/src/frontend/App.svelte`:
- Around line 165-168: The initializeSearch flow narrows addons using
queryConnectedAddons() but later overwrites that list with
fetchAddonsWithConfigure(), causing librarySearch() to run against disconnected
addons; revert that by keeping the original connected list (addons from
queryConnectedAddons) and either (a) filter the results of
fetchAddonsWithConfigure() to only include IDs present in addons before
assigning, or (b) avoid assigning fetchAddonsWithConfigure() to the addons
variable and instead pass the connected-only addons to librarySearch(); update
references to addons, initializeSearch, fetchAddonsWithConfigure, and
librarySearch accordingly so only connected addons are used for search.
In `@application/src/frontend/components/built/UpdateAppModal.svelte`:
- Around line 106-117: The loop over detailAddons currently awaits
addonServer.addon(addon.id).gameDetails(...) without handling rejections which
lets a single failing addon reject loadUpdateSources(); wrap the per-addon call
in a try/catch inside the for loop (in the block using
findAddonsSupportingStorefront and the variable response) so that if
addonServer.addon(...).gameDetails throws or rejects you log/ignore that error
and continue to the next addon rather than rethrowing, preserving the existing
behavior of breaking out when a valid response is found.
In `@application/src/frontend/components/modal/ConfigurationModal.svelte`:
- Around line 103-110: The modal closes even when the async screenReply call
fails because its rejection is unhandled; change the submit flow so you await or
chain screenReply(data) and handle rejections before calling closeModal: call
screenReply(data) (or the else branch window.electronAPI.app.inputSend(...) if
appropriate) and await its result, catch any errors, surface them to the user
(e.g., show an error state/message) and only call closeModal() on success;
reference the symbols screenReply, formData, screenID, and closeModal to locate
the submission block.
In `@application/src/frontend/components/StorePage.svelte`:
- Around line 193-204: The loop over detailAddons can abort early if
addonServer.addon(addon.id).gameDetails(...) throws, causing a rejected promise
to bubble out and preventing later addons from being tried; wrap the await call
inside a try/catch around the call to addonServer.addon(addon.id).gameDetails({
appID, storefront }) in the loop (referencing detailAddons, addon, and response)
and on error log or swallow the error and continue to the next addon so only a
successful response sets response and breaks; ensure you still break when
response is truthy.
In `@application/src/frontend/lib/config/client.ts`:
- Around line 14-16: fetchAddonsWithConfigure currently resolves the outer
Promise immediately and never rejects if queryConnectedAddons or subsequent
configUpdate calls fail; change it to await queryConnectedAddons and then use
Promise.all (or equivalent) on the array of configUpdate promises so the outer
Promise only resolves after every configUpdate completes and rejects if any of
them (or queryConnectedAddons) throw; update the same pattern where similar
early-resolve logic appears (the other blocks around the queryConnectedAddons
and configUpdate calls referenced in the comment) so both queryConnectedAddons
and configUpdate errors are propagated to the caller.
In `@application/src/frontend/lib/core/addons.ts`:
- Around line 13-20: Replace the fail-fast Promise.all fan-out so one addon
error doesn't reject the whole batch: call addonServer.addon(addon.id).launchApp
for each entry in addons but use Promise.allSettled (or wrap each call in a
try/catch) to collect per-addon outcomes into the results array, preserving
successful launch responses and recording errors for failed addons; ensure the
code that consumes results can handle settled values (fulfilled/rejected) from
the launchApp calls.
In `@application/src/frontend/lib/core/ipc.ts`:
- Around line 28-33: The code currently type-asserts and then dereferences
clientSdkUrl on getConfigClientOption('developer'), which can throw if the
developer config is missing; change the access to safely handle a missing value
(e.g., check for null/undefined or use optional chaining on the result of
getConfigClientOption('developer') before reading clientSdkUrl) so the fallback
'ws://127.0.0.1:7654' is applied robustly; update the expression around
getConfigClientOption('developer') and .clientSdkUrl to use a safe guard
(result?.clientSdkUrl or an explicit null check) rather than a direct
dereference.
- Around line 22-24: reconnectClientSdk currently overwrites addonServer without
tearing down the old connection; add a public teardown method on the connection
type (e.g., Connection.close() or Connection.dispose()) that closes the
underlying websocket and removes/unsubscribes any EventResponseSocket.on(...)
and server.on(...) handlers, then call that teardown from reconnectClientSdk()
before reassigning addonServer so previous listeners are removed and the
transport is closed.
In `@application/src/frontend/managers/AppUpdateManager.svelte`:
- Around line 9-20: checkForAppUpdates currently awaits
core.library.getAllApps() outside any try/catch so a rejection will escape the
addon-runtime-ready listener; wrap the call (or the initial part of
checkForAppUpdates) in a try/catch around core.library.getAllApps() (and the
subsequent initial setup like incrementing updateCheckRunId and
updatesManager.clearAppUpdates()) to catch and log the error (using the existing
logging mechanism) and return early so the per-app loop never aborts the whole
scan; reference checkForAppUpdates, core.library.getAllApps, updateCheckRunId,
and updatesManager.clearAppUpdates when making the change.
In `@application/src/frontend/views/ClientOptionsView.svelte`:
- Around line 474-480: The restartAddonServer function currently calls
reconnectClientSdk() (which returns void) and immediately invokes
fetchAddonsWithConfigure(), causing race conditions; change the sequencing to
wait for an actual "addon-connected" readiness signal instead of awaiting
reconnectClientSdk(): remove the direct call to fetchAddonsWithConfigure() from
restartAddonServer, and instead listen for the existing "addon-connected" event
in ClientOptionsView.svelte (or implement an awaitable helper in
application/src/frontend/lib/core/ipc.ts that exposes a promise-resolving
SDK-ready signal) and trigger fetchAddonsWithConfigure() from that handler; also
ensure fetchAddonsWithConfigure() returns a proper Promise that rejects on
network/error conditions so callers can handle failures.
In `@application/src/frontend/views/CommunityAddonsList.svelte`:
- Around line 44-47: The delete path currently calls
window.electronAPI.restartAddonServer() but does not call reconnectClientSdk(),
causing a stale SDK connection; modify the delete/remove flow (where
restartAddonServer() is invoked) to await the restart and then call
reconnectClientSdk() just like the install path (i.e., after await
window.electronAPI.restartAddonServer(), call reconnectClientSdk()), ensuring
both install and delete restart paths consistently re-establish the client SDK
connection.
In `@application/src/frontend/views/FocusedAddonView.svelte`:
- Around line 106-110: The current promise chain calls
addonServer.addon(selectedAddon.id).configUpdate(config) and treats
null/undefined as success, which can persist invalid config and ignores rejected
updates; change the flow so you verify the actual response success (e.g., check
response?.success === true or data.success) and only call the persistence/write
routine after that verification, and add error handling for rejected promises
(use .catch or convert to async/await with try/catch) to abort persistence and
surface/log the error. Specifically update the chain around
addon(...).configUpdate to validate the returned data from configUpdate before
proceeding and move any config-write/persist logic into the success branch;
ensure rejected configUpdate calls do not reach the persistence step.
In `@packages/addon-server/lib/connections/addon.connection.ts`:
- Around line 1-2: The AddonConnection currently leaks the ws package into the
handler boundary; replace the ws-specific import and type with Bun's built-in
WebSocket (or an internal adapter interface) so handlers only see standard
WebSocket semantics. Concretely: remove import wsLib from 'ws', change the
AddonConnection.public member (currently referenced as connection.ws) to be
typed as the global WebSocket or an interface (e.g., IWebSocket) and adapt any
methods used (close, send, onmessage/onclose handlers) to use the standard
WebSocket API; update HandlerContext and all handler code (e.g., places using
connection.ws.close in lib/handlers/helpers.ts) to rely on that standard API or
the adapter methods. Do the same refactor for the parallel client.connection.ts
to eliminate ws-typed public members and remove the 'ws' dependency from the
layer boundary.
In `@packages/addon-server/lib/handlers/client-message-handlers.ts`:
- Around line 26-31: The code currently sets connection.addonInfo and calls
server.addClient before performing secret validation and duplicate-ID checks,
allowing requireAuthenticated() to misidentify an invalid connection as
authenticated; update the authenticate handling so that you first validate
authenticateArgs (check config.securityCheck, authenticateArgs.secret ===
config.secret) and verify no duplicate client ID, and only after both checks
pass assign connection.addonInfo = authenticateArgs and call
server.addClient(connection, authenticateArgs) (apply the same reorder to the
second authenticate branch around lines 41-52); keep requireAuthenticated usage
unchanged but ensure it only sees addonInfo after successful validation.
- Around line 163-175: The handler is deleting finished tasks too early which
causes subsequent get-deferred-task() calls to return "Task not found"; remove
the premature deletion and let getDeferredTasksManager().removeTask(...) stay
solely owned by the get-deferred-task path. Concretely, keep the assignments to
task.progress/logs/finished/failed and the failed branch that sets task.finished
= true and returns, but delete the conditional block that calls
context.server.getDeferredTasksManager().removeTask(taskUpdateArgs.id) so
finished tasks remain until get-deferred-task() serializes and removes them.
In `@packages/addon-server/lib/server.ts`:
- Around line 61-71: The input-asked handler currently only calls reply when an
SDK connection exists and uses .then without error handling; change the
'input-asked' event handler to be async, always invoke reply (resolve the addon
request) whether there is a connection or not, and wrap the askInput call in
try/catch so a rejection results in a deterministic reply (e.g. an error/empty
response) instead of never resolving. Also ensure AddonConnection instances are
removed from server connection lists when their websocket closes: add/remove
logic in the ws 'close' handler (or implement server.removeConnection) so
server.connections/server.clients no longer hold stale AddonConnection entries;
this will prevent addonIdsForQuery and other helpers from iterating dead
connections. Reference symbols: the 'input-asked' event handler and
Connection.askInput, AddonConnection, websocket 'close' handler,
server.getConnections/server.connections/server.clients, and addonIdsForQuery.
In `@packages/client-kit/lib/connection.ts`:
- Around line 270-277: The current Promise awaiting the socket 'open' only
resolves and never rejects, so this.ready can hang forever; update the await
block in connection.ts to also listen for failure events (socket 'error' and
'close', or addEventListener 'error'/'close') and reject the Promise on those
events (and consider adding a configurable timeout), ensuring you remove all
attached listeners in both resolve and reject paths; reference this.socket and
the existing 'open' handler to add symmetric 'error'/'close' (or
onerror/ondisconnect) handlers and cleanups so callers don't block indefinitely.
- Around line 189-224: The polling loop using setInterval can start overlapping
async polls; change it to avoid concurrent requests by either (a) introducing an
inFlight boolean checked/set around the body so a new interval tick exits if
inFlight is true, or (b) replace setInterval with an async recursive setTimeout
loop that awaits getDeferredTask<T>(taskID) before scheduling the next timeout;
keep the same handling of settled, clearInterval/clearTimeout on settle, and
preserve calls to getDeferredTask<T>,
options.onProgress/options.onLogs/options.onFailed, resolve, and reject.
In `@packages/connection/lib/protocol.ts`:
- Around line 141-175: The protocol uses unsafe any payloads; replace them with
unknown or the precise derived types: change
addonProtocol.serverToAddon.response (serverCommand<..., any>) to
serverCommand<..., unknown> or the matching AddonServerToClientEventArgs[Event]
tuple type, change addonProtocol.addonToServer.response (addonMessage<any>()) to
addonMessage<unknown>() or the concrete response type, and update
AddonServerToClientWebsocketMessage.args from any to unknown or the correct
AddonServerToClientEventArgs[...] type; ensure any call sites narrow/cast the
payloads appropriately where handlers read specific event arg shapes (look for
serverCommand, addonMessage and AddonServerToClientWebsocketMessage to locate
code).
---
Outside diff comments:
In `@application/src/frontend/components/built/UpdateAppModal.svelte`:
- Around line 82-86: When building AddonGroup in addonMap.forEach, don't
overwrite the friendly name with the raw addonId; use the precomputed display
name from results (e.g. set addonName: results.addonName || addonId or
addonName: results[0]?.addonName || addonId if results is an array) instead of
addonName: addonId. Apply the same change to the other construction at lines
157-159 where addonName is currently set to addonId so the modal shows the
friendly addon name.
In `@application/src/frontend/components/StorePage.svelte`:
- Around line 237-243: The mapper unconditionally reads images from gameData
which can be undefined; update the mapping in the searchResults.map callback to
guard gameData before copying coverImage and capsuleImage (e.g., use a
conditional or fallback values) so that when gameData is undefined the mapped
result still returns safely (keep name and other result fields unchanged).
Locate the mapping that builds mappedResults from searchResults (the
searchResults.map arrow function that spreads result and sets
coverImage/capsuleImage) and replace the direct casts to (gameData as
StoreData).coverImage / .capsuleImage with guarded accesses or default values.
In `@application/src/frontend/lib/tasks/runner.ts`:
- Around line 97-101: The onFailed handler currently only sets the failed
message but leaves the task's status as 'running'; update the
deferredTasks.update callback inside onFailed (the function referencing taskID
and tasks.map) to return the task with both failed: error and status: 'failed'
for the matching t.id === taskID so the task's status and UI reflect the
failure.
In `@application/src/frontend/views/ClientOptionsView.svelte`:
- Around line 1231-1233: Remove the unused .input-number selector from the
component styles (or wire it to an element) to eliminate CI warnings: inspect
the stylesheet block that defines ".input-number, .input-select" and either
delete ".input-number" from that selector or add the .input-number class to the
relevant input element(s) in the template (the same place where .input-select is
used) so the selector is actually applied; update any related styles to keep
.input-select intact if needed.
In `@application/src/frontend/views/ConfigView.svelte`:
- Around line 409-416: Remove the unused CSS selectors `.update-badge` and
`.update-icon` from ConfigView.svelte: locate the rules for these classes in the
component's style block and delete both selector blocks (the `.update-badge {
... }` and `.update-icon { ... }` entries) so the CI warning is resolved and no
dead styles remain.
In `@packages/addon-server/lib/server.ts`:
- Around line 143-159: The AddonConnection branch doesn't remove closed addon
sockets from this.connections (and any this.clients mapping), leaving stale
entries; add a ws.on('close', ...) handler after creating AddonConnection in the
connection callback (same pattern as the SDK branch) that deletes the connection
from this.connections, removes any corresponding entry from this.clients (if
applicable), and emits the 'disconnect' event with a clear reason; locate the
code around AddonConnection construction/connection.setupWebsocket() to insert
the close handler so closed addon sockets are cleaned up properly.
In `@packages/connection/README.md`:
- Around line 1-16: Update the minimal README to be a real library doc: explain
that `@ogi-sdk/connect` is a typed WebSocket protocol for addon communication,
remove or change the misleading "To run: bun run lib/index.ts" note (libraries
are imported, not run), and add sections for Installation, Usage examples
showing import of defineProtocol and EventResponseSocket, brief API overview
referencing defineProtocol, EventResponseSocket, and upgradeToWebSocket, and a
short Development section (install/test/build) plus a link to fuller API docs;
keep explanations for any non-obvious design choices and integration guidance
for consumers.
---
Minor comments:
In `@application/src/electron/main.ts`:
- Around line 467-468: The two async calls to
sendIPCMessage('all-addons-started') and sendIPCMessage('addon-runtime-ready')
can race; change the call sites in main.ts to await sendIPCMessage(...) so
delivery order is preserved and the readiness sequencing cannot be dropped—await
sendIPCMessage('all-addons-started') first, then await
sendIPCMessage('addon-runtime-ready') to guarantee deterministic renderer
startup state.
In `@application/src/frontend/components/GameConfiguration.svelte`:
- Around line 78-79: In GameConfiguration.svelte the assignment to formData[key]
uses `option.defaultValue || option.allowedValues![0]`, which treats an empty
string as falsy and overwrites intentional '' defaults; change the logic in the
select-style branch (the code referencing option.allowedValues,
option.defaultValue and formData[key]) to use a nullish check (e.g. test for
undefined/null) or the nullish coalescing operator so that '' is preserved and
only undefined/null falls back to option.allowedValues![0].
In `@application/src/frontend/components/modal/ConfigurationModal.svelte`:
- Around line 84-85: The code assigns defaults using a boolean fallback which
treats empty strings as falsey; update places that set formData[key] from
option.defaultValue or option.allowedValues[0] (e.g., the assignment inside the
ConfigurationModal component where formData[key] = option.defaultValue ||
option.allowedValues![0]) to use the nullish coalescing operator (??) so only
null/undefined fall back; apply the same change to the other occurrence around
lines 151-153 so empty-string defaults are preserved.
In `@application/src/frontend/lib/tasks/deferred.ts`:
- Around line 22-27: The returned object currently sets error to a boolean via
"error: task.failed || undefined"; change this so error contains a descriptive
string: replace that expression with a string-valued error payload derived from
task (e.g., task.errorMessage || task.error || (task.failed === true ?
lastLogLineFrom(task.logs) : undefined)), ensuring "task" properties referenced
exist and fall back to undefined if none are present; update the error field in
the same object in deferred.ts where "failed", "logs", "timestamp", "duration",
and "type" are set.
In `@application/src/frontend/views/ConfigView.svelte`:
- Around line 30-37: The polling and initial load call to
queryConnectedAddons<ConfigTemplateAndInfo>() use .then(...) without rejection
handling which leads to unhandled promise rejections; update both the initial
call and the setInterval callback (references: queryConnectedAddons,
ConfigTemplateAndInfo, pollingInterval, addons) to append .catch(...) (or use
try/catch with async/await) that handles errors—e.g., log the error and avoid
throwing, optionally set a retry/backoff or clearInterval on fatal errors—so all
promise rejections are handled and do not surface as unhandled exceptions.
In `@packages/client-kit/.gitignore`:
- Around line 15-16: Update the gitignore globs so generated logs and report
JSONs aren't missed: replace the literal/incorrect pattern "_.log" with a
broader "*.log" and replace the narrow "report.[0-9]_.[0-9]_.[0-9]_.[0-9]_.json"
pattern with "report.*.json" (or add both "report.*.json" and "report.*_*.json"
if you expect additional underscore-delimited variants) so standard log and
report files are correctly ignored.
In `@packages/connection/lib/socket.ts`:
- Around line 212-231: EventResponseSocket.send currently only rejects when
readyState === 3 and still calls this.socket.send for CONNECTING/ CLOSING;
change the guard to only allow sending when this.socket.readyState === 1 (OPEN)
and otherwise reject with a clear error (include the readyState value), so
socket.send is never invoked in non-OPEN states. Keep the pendingResponses
registration logic as-is (no reorder needed) because resolveIncomingResponse
runs asynchronously after parseRawMessage in handleRawMessage; reference
EventResponseSocket.send, this.socket.readyState, this.socket.send,
this.pendingResponses, handleRawMessage, resolveIncomingResponse, and
parseRawMessage when locating where to apply the check and error.
---
Nitpick comments:
In `@application/package.json`:
- Line 36: Update the "dev" npm script to use Bun instead of Node: locate the
"dev" script entry that sets ELECTRON_EXEC_PATH using node -p
"require('electron')", and change that invocation to the equivalent Bun command
so the environment variable resolution uses Bun; ensure the rest of the command
(the electron-vite dev part) remains unchanged and that the script still exports
ELECTRON_EXEC_PATH correctly.
In `@packages/addon-server/lib/connections/client.connection.ts`:
- Line 110: The generated taskID using Math.random().toString(36).substring(7)
is not collision-resistant; replace that line in client.connection.ts (the
taskID assignment in the deferred task creation path) with a collision-safe ID
generator such as crypto.randomUUID() (or a nanoid/uuid.v4 import if targeting
older runtimes), and update imports/types as needed so the rest of the code that
looks up deferred tasks by taskID continues to work unchanged.
In `@packages/addon-server/scripts/generate-event-proxy.ts`:
- Around line 50-54: The generated declaration for EventMessagePackers should be
emitted as an interface rather than a type alias: update the generator template
that produces the EventMessagePackers declaration so it emits "interface
EventMessagePackers { [Event in AddonServerToClientEventName]: (args:
AddonServerToClientEventArgs[Event]) =>
Omit<AddonServerToClientWebsocketMessage, 'event'>; }" (preserving the
mapped/index signature and referenced symbols AddonServerToClientEventName,
AddonServerToClientEventArgs, and AddonServerToClientWebsocketMessage) so the
output conforms to the repo convention of using interfaces for object shapes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0066fc3d-4948-4f86-b81a-6804ab67b9e8
⛔ Files ignored due to path filters (3)
bun.lockis excluded by!**/*.lockpackages/addon-server/lib/_generated/event-proxy.tsis excluded by!**/_generated/**packages/client-kit/lib/_generated/addon-proxy.tsis excluded by!**/_generated/**
📒 Files selected for processing (83)
application/package.jsonapplication/src/electron/handlers/handler.addon.tsapplication/src/electron/handlers/handler.library.tsapplication/src/electron/handlers/handler.redists.tsapplication/src/electron/handlers/handler.umu.tsapplication/src/electron/handlers/helpers.app/library.tsapplication/src/electron/main.tsapplication/src/electron/preload.mtsapplication/src/electron/server/addon-server.tsapplication/src/electron/server/api/addons.tsapplication/src/electron/startup.tsapplication/src/frontend/App.svelteapplication/src/frontend/components/GameConfiguration.svelteapplication/src/frontend/components/GameLaunchOverlay.svelteapplication/src/frontend/components/NotificationSideView.svelteapplication/src/frontend/components/PlayPage.svelteapplication/src/frontend/components/StorePage.svelteapplication/src/frontend/components/built/UpdateAppModal.svelteapplication/src/frontend/components/modal/CheckboxModal.svelteapplication/src/frontend/components/modal/ConfigurationModal.svelteapplication/src/frontend/global.d.tsapplication/src/frontend/lib/config/client.tsapplication/src/frontend/lib/core/addons.tsapplication/src/frontend/lib/core/ipc.tsapplication/src/frontend/lib/core/library.tsapplication/src/frontend/lib/downloads/services/RequestService.tsapplication/src/frontend/lib/recovery/failedSetups.tsapplication/src/frontend/lib/setup/setup.tsapplication/src/frontend/lib/tasks/deferred.tsapplication/src/frontend/lib/tasks/runner.tsapplication/src/frontend/managers/AppUpdateManager.svelteapplication/src/frontend/managers/Debug.svelteapplication/src/frontend/managers/DownloadManager.svelteapplication/src/frontend/managers/GameManager.svelteapplication/src/frontend/store.tsapplication/src/frontend/utils.tsapplication/src/frontend/views/ClientOptionsView.svelteapplication/src/frontend/views/CommunityAddonsList.svelteapplication/src/frontend/views/ConfigView.svelteapplication/src/frontend/views/DiscoverView.svelteapplication/src/frontend/views/FocusedAddonView.svelteapplication/src/frontend/views/LibraryView.sveltepackages/addon-server/lib/addon-connection.tspackages/addon-server/lib/connections/addon.connection.tspackages/addon-server/lib/connections/client.connection.tspackages/addon-server/lib/handlers/client-message-handlers.tspackages/addon-server/lib/handlers/helpers.tspackages/addon-server/lib/handlers/types.tspackages/addon-server/lib/index.tspackages/addon-server/lib/server.tspackages/addon-server/package.jsonpackages/addon-server/scripts/generate-event-proxy.tspackages/client-kit/.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdcpackages/client-kit/.gitignorepackages/client-kit/CLAUDE.mdpackages/client-kit/README.mdpackages/client-kit/lib/connection.tspackages/client-kit/lib/index.tspackages/client-kit/lib/types.tspackages/client-kit/package.jsonpackages/client-kit/scripts/generate-addon-proxy.tspackages/client-kit/tsconfig.jsonpackages/client-kit/tsdown.config.jspackages/connection/.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdcpackages/connection/.gitignorepackages/connection/CLAUDE.mdpackages/connection/README.mdpackages/connection/lib/http-upgrade.tspackages/connection/lib/index.tspackages/connection/lib/protocol-base.tspackages/connection/lib/protocol-builder.tspackages/connection/lib/protocol.tspackages/connection/lib/socket.tspackages/connection/lib/types.tspackages/connection/package.jsonpackages/connection/tsconfig.jsonpackages/connection/tsdown.config.jspackages/ogi-addon/package.jsonpackages/ogi-addon/src/SearchEngine.tspackages/ogi-addon/src/config/Configuration.tspackages/ogi-addon/src/config/ConfigurationBuilder.tspackages/ogi-addon/src/main.tspackages/ogi-addon/tsconfig.json
💤 Files with no reviewable changes (1)
- packages/addon-server/lib/addon-connection.ts
Nat3z
changed the title
Nat3z
changed the title
…icated deferToAddon method
There was a problem hiding this comment.
Pull request overview
This PR introduces a typed WebSocket-based SDK layer for addon/server/client communication and migrates the application away from the legacy safeFetch IPC-style calls. It centralizes protocol types/events into a shared @ogi-sdk/connect package, adds a client-side SDK (@ogi-sdk/client-kit), refactors ogi-addon and @ogi-sdk/addon-server to use the shared transport, and updates the Electron/Svelte application to use the new client SDK.
Changes:
- Added
@ogi-sdk/connect(shared protocol +EventResponseSocket) and@ogi-sdk/client-kit(client SDK + generated addon proxy). - Refactored
ogi-addonand@ogi-sdk/addon-serverto use the shared protocol/transport and added an/sdkWebSocket endpoint for client SDK connections. - Migrated frontend/electron code from
safeFetchtoaddonServer(client SDK) calls; updated types imports and build workflow tooling.
Reviewed changes
Copilot reviewed 85 out of 87 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ogi-addon/tsconfig.json | Switch TS module resolution to bundler for workspace exports. |
| packages/ogi-addon/src/SearchEngine.ts | Re-export SearchResult from shared SDK package. |
| packages/ogi-addon/src/main.ts | Replace ad-hoc WS message handling with EventResponseSocket + shared protocol types. |
| packages/ogi-addon/src/config/ConfigurationBuilder.ts | Pull config wire types/guards from @ogi-sdk/connect. |
| packages/ogi-addon/src/config/Configuration.ts | Align exported config types with @ogi-sdk/connect; adjust validation typing. |
| packages/ogi-addon/package.json | Add dependency on @ogi-sdk/connect. |
| packages/connection/tsdown.config.js | Add tsdown build config for the new shared connection package. |
| packages/connection/tsconfig.json | Add TS config for the new shared connection package. |
| packages/connection/README.md | Document @ogi-sdk/connect usage and development. |
| packages/connection/package.json | Define @ogi-sdk/connect package metadata, exports, build script. |
| packages/connection/lib/types.ts | Define base websocket message type. |
| packages/connection/lib/socket.ts | Add EventResponseSocket request/response wrapper and parsing utilities. |
| packages/connection/lib/protocol.ts | Define canonical addon protocol registry and derived types. |
| packages/connection/lib/protocol-builder.ts | Add protocol builder/type-level machinery for deriving unions/maps. |
| packages/connection/lib/protocol-base.ts | Add shared domain shapes for protocol (config, library, search, etc.). |
| packages/connection/lib/index.ts | Export public surface of @ogi-sdk/connect (socket/protocol helpers). |
| packages/connection/lib/http-upgrade.ts | Provide WS HTTP upgrade helper used by addon server. |
| packages/connection/CLAUDE.md | Package-level development guidance (Bun). |
| packages/connection/.gitignore | Ignore build artifacts and deps for the new package. |
| packages/client-kit/tsdown.config.js | Add tsdown build config for client SDK package. |
| packages/client-kit/tsconfig.json | Add TS config for client SDK package. |
| packages/client-kit/scripts/generate-addon-proxy.ts | Generate typed addon proxy from the shared protocol registry. |
| packages/client-kit/README.md | Basic client-kit package instructions. |
| packages/client-kit/package.json | Define client-kit package metadata, scripts, deps. |
| packages/client-kit/lib/types.ts | Re-export SDK/protocol helper types for consumers. |
| packages/client-kit/lib/index.ts | Export client-kit connection + generated proxy types. |
| packages/client-kit/lib/connection.ts | Implement SDK Connection over WebSocket + deferred task helpers. |
| packages/client-kit/lib/_generated/addon-proxy.ts | Generated addon proxy implementation/types. |
| packages/client-kit/CLAUDE.md | Package-level development guidance (Bun). |
| packages/client-kit/.gitignore | Ignore build artifacts and deps for the client kit. |
| packages/addon-server/scripts/generate-event-proxy.ts | Generate event proxy from shared protocol registry (no parsing ogi-addon). |
| packages/addon-server/package.json | Add @ogi-sdk/connect dep and typecheck script. |
| packages/addon-server/lib/server.ts | Add /sdk WS handling; forward notifications/input to SDK clients. |
| packages/addon-server/lib/index.ts | Re-export updated server/connection modules. |
| packages/addon-server/lib/handlers/types.ts | Switch handler types to shared protocol message/event types. |
| packages/addon-server/lib/handlers/helpers.ts | Switch event name type to OGIAddonSDKEventListener. |
| packages/addon-server/lib/handlers/client-message-handlers.ts | Update handlers to shared protocol types; adjust task/notification/config handling. |
| packages/addon-server/lib/connections/client.connection.ts | New SDK client connection implementation (forward/query/deferred tasks). |
| packages/addon-server/lib/connections/addon.connection.ts | New addon connection implementation using EventResponseSocket. |
| packages/addon-server/lib/addon-connection.ts | Remove legacy addon connection implementation. |
| packages/addon-server/lib/_generated/event-proxy.ts | Generated server→addon event message packers/types now based on shared protocol. |
| application/src/frontend/views/LibraryView.svelte | Update type imports to @ogi-sdk/connect. |
| application/src/frontend/views/FocusedAddonView.svelte | Replace safeFetch calls with client SDK usage; improve allowedValues null-safety. |
| application/src/frontend/views/DiscoverView.svelte | Replace safeFetch with client SDK catalog calls; update imports. |
| application/src/frontend/views/ConfigView.svelte | Use queryConnectedAddons and reconnect SDK after server restarts. |
| application/src/frontend/views/CommunityAddonsList.svelte | Reconnect client SDK after addon server restarts. |
| application/src/frontend/views/ClientOptionsView.svelte | Add configurable SDK URL option; reconnect SDK on restart; adjust listeners/styles. |
| application/src/frontend/utils.ts | Re-export new addons helpers module. |
| application/src/frontend/store.ts | Switch types to @ogi-sdk/connect; adjust DeferredTask status union and setup data typing. |
| application/src/frontend/managers/GameManager.svelte | Replace safeFetch('launchApp') with addon helper runLaunchAppAddons. |
| application/src/frontend/managers/DownloadManager.svelte | Update LibraryInfo import to shared package. |
| application/src/frontend/managers/Debug.svelte | Update config type imports; adjust z-index class. |
| application/src/frontend/managers/AppUpdateManager.svelte | Move update check trigger to addon-runtime-ready; use SDK calls for update checks. |
| application/src/frontend/lib/tasks/runner.ts | Run addon tasks via client SDK deferred flow; adjust task status updates. |
| application/src/frontend/lib/tasks/deferred.ts | Load deferred tasks via client SDK instead of electron API. |
| application/src/frontend/lib/setup/setup.ts | Run addon setup via client SDK deferred flow; update setup typing. |
| application/src/frontend/lib/recovery/failedSetups.ts | Update setup payload typing to shared SetupCommandData. |
| application/src/frontend/lib/downloads/services/RequestService.ts | Replace safeFetch('requestDownload') with requestDl via SDK. |
| application/src/frontend/lib/core/library.ts | Update LibraryInfo import to shared package. |
| application/src/frontend/lib/core/ipc.ts | Replace safeFetch with persistent client SDK connection + event dispatch integration. |
| application/src/frontend/lib/core/addons.ts | Add helper functions for launch hooks + addon filtering by storefront/events. |
| application/src/frontend/lib/config/client.ts | Configure addons via client SDK (configUpdate) and connected addon querying. |
| application/src/frontend/global.d.ts | Update global LibraryInfo/ConfigurationFile types to shared package. |
| application/src/frontend/components/StorePage.svelte | Replace safeFetch search/details with SDK calls; improve image null-safety. |
| application/src/frontend/components/PlayPage.svelte | Replace safeFetch launch/search with SDK calls. |
| application/src/frontend/components/NotificationSideView.svelte | Adjust task polling timing. |
| application/src/frontend/components/modal/ConfigurationModal.svelte | Switch to shared config types; support SDK reply callback; show submit errors. |
| application/src/frontend/components/modal/CheckboxModal.svelte | Update checkbox checkmark styling. |
| application/src/frontend/components/GameLaunchOverlay.svelte | Replace safeFetch launch hooks with addon helper. |
| application/src/frontend/components/GameConfiguration.svelte | Switch to shared config types; use shared option type guards. |
| application/src/frontend/components/built/UpdateAppModal.svelte | Replace safeFetch with SDK calls; preserve addonName; improve null-safety. |
| application/src/frontend/App.svelte | Replace safeFetch query/search with SDK calls; reconnect SDK on runtime restart. |
| application/src/electron/startup.ts | Update LibraryInfo import to shared package. |
| application/src/electron/server/api/addons.ts | Update types and fix some .args return handling for deferred procedures. |
| application/src/electron/server/addon-server.ts | Remove obsolete UI handler wiring; route disconnect notifications through server emit. |
| application/src/electron/preload.mts | Update LibraryInfo import; dispatch new addon-runtime-ready event. |
| application/src/electron/main.ts | Remove legacy UI handler registration; emit addon-runtime-ready after startup. |
| application/src/electron/handlers/helpers.app/library.ts | Update LibraryInfo import to shared package. |
| application/src/electron/handlers/handler.umu.ts | Update LibraryInfo import to shared package. |
| application/src/electron/handlers/handler.redists.ts | Update LibraryInfo import to shared package. |
| application/src/electron/handlers/handler.library.ts | Update LibraryInfo import to shared package. |
| application/src/electron/handlers/handler.addon.ts | Emit addon-runtime-ready after restarting addon server. |
| application/package.json | Consolidate build scripts; add SDK packages deps; tweak electron-builder config. |
| .github/workflows/build-release.yml | Bump Bun version used in workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+99
to
+103
| configure: serverCommand<[config: ConfigurationFile], unknown, true>({ | ||
| addonListener: true, | ||
| }), | ||
| 'config-update': serverCommand<[config: ConfigurationFile], unknown>(), | ||
| 'launch-app': serverCommand< |
Comment on lines
+61
to
+63
| public configure(config: ConfigurationFile): void { | ||
| this.events.noResponse.configUpdate(config); | ||
| } |
Comment on lines
+163
to
173
| task.progress = taskUpdateArgs.progress; | ||
| task.logs = taskUpdateArgs.logs; | ||
| task.finished = taskUpdateArgs.finished; | ||
| task.failed = taskUpdateArgs.failed; | ||
|
|
||
| if (taskUpdate.failed) { | ||
| if (taskUpdateArgs.failed) { | ||
| task.finished = true; | ||
| return; | ||
| } | ||
|
|
||
| if (taskUpdate.finished && !taskUpdate.failed) { | ||
| context.server.getDeferredTasksManager().removeTask(message.args.id); | ||
| } | ||
| }; |
Comment on lines
122
to
136
| public build(includeFunctions: boolean): ConfigurationFile { | ||
| let config: ConfigurationFile = {}; | ||
| this.options.forEach((option) => { | ||
| // remove all functions from the option object | ||
| if (!includeFunctions) { | ||
| option = JSON.parse(JSON.stringify(option)); | ||
| const optionData = configValidation.safeParse(option); | ||
| if (!optionData.success) { | ||
| throw new ZodError(optionData.error.errors); | ||
| } | ||
|
|
||
| config[option.name] = option; | ||
| config[option.name] = option as unknown as ConfigurationFile[string]; | ||
| } else { | ||
| config[option.name] = option; | ||
| config[option.name] = option as unknown as ConfigurationFile[string]; | ||
| } |
Comment on lines
+928
to
942
| public async requestResponse<T>( | ||
| event: AddonClientToServerEventName, | ||
| args: AddonClientToServerEventArgs[AddonClientToServerEventName] | ||
| ): Promise<T> { | ||
| const response = await this.transport.send( | ||
| { event, args } as AddonClientToServerWebsocketMessage, | ||
| { expectResponse: true } | ||
| ); | ||
| return response.args as T; | ||
| } | ||
|
|
||
| public send( | ||
| event: OGIAddonClientSentEvent, | ||
| args: ClientSentEventTypes[OGIAddonClientSentEvent] | ||
| event: AddonClientToServerEventName, | ||
| args: AddonClientToServerEventArgs[AddonClientToServerEventName] | ||
| ): string { |
Comment on lines
97
to
103
| onFailed: (error: string) => { | ||
| if (!taskID) return; | ||
| deferredTasks.update((tasks) => | ||
| tasks.map((t) => (t.id === taskID ? { ...t, failed: error } : t)) | ||
| tasks.map((t) => | ||
| t.id === taskID ? { ...t, failed: error, status: 'failed' } : t | ||
| ) | ||
| ); |
Comment on lines
59
to
65
| export type DeferredTask = { | ||
| id: string; | ||
| name: string; | ||
| description: string; | ||
| addonOwner: string; | ||
| status: 'pending' | 'running' | 'completed' | 'error' | 'cancelled'; | ||
| status: 'pending' | 'running' | 'completed' | 'failed' | 'error' | 'cancelled'; | ||
| progress: number; |
Comment on lines
+24
to
+28
| export type ConnectionOptions = { | ||
| url: string; | ||
| secret?: string; | ||
| webSocket?: WebSocketConstructor; | ||
| }; |
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
application/src/frontend/store.ts (1)
59-65:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTreat
'failed'as a terminal task state everywhere.Adding
'failed'here is fine, butclearCompletedTasks()still only removescompleted,error, andcancelled. Failed tasks created byrunTask()will now stick around even when the user clears finished work.Suggested fix
export function clearCompletedTasks() { deferredTasks.update((tasks: DeferredTask[]) => tasks.filter( (task: DeferredTask) => task.status !== 'completed' && + task.status !== 'failed' && task.status !== 'error' && task.status !== 'cancelled' ) ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/src/frontend/store.ts` around lines 59 - 65, The DeferredTask union now includes 'failed' but the cleanup logic in clearCompletedTasks() (and any other places that filter terminal states after runTask()) still only treats 'completed', 'error', and 'cancelled' as finished; update clearCompletedTasks() to also treat 'failed' as a terminal state so failed tasks are removed when clearing finished work, and search for any other filters or conditionals around runTask() or task cleanup that enumerate terminal statuses and add 'failed' there as well.application/src/frontend/lib/core/addons.ts (1)
13-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDerive the aggregate
successflag from the settled results.
Promise.allSettled()now keeps per-addon failures, but this helper still returnssuccess: trueunconditionally. Any caller that only checkssuccesswill treat partial launch-hook failures as a clean pass.Suggested fix
const results = await Promise.allSettled( addons.map((addon) => addonServer.addon(addon.id).launchApp({ libraryInfo, launchType, }) ) ); - return { success: true, results }; + return { + success: results.every((result) => result.status === 'fulfilled'), + results, + }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/src/frontend/lib/core/addons.ts` around lines 13 - 22, The helper currently calls Promise.allSettled and always returns { success: true, results }, which masks per-addon failures; change it to compute an aggregate success flag from the settled results (e.g., set success = results.every(r => r.status === 'fulfilled') so any rejected addon marks overall success false) and return { success, results } instead; locate the Promise.allSettled call and the return statement in addons.ts (the variable results and the returned object) and update the return to include the derived success boolean while preserving the original results array.application/src/frontend/views/ClientOptionsView.svelte (1)
474-478:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear the restart lock when the restart/reconnect step fails.
If
window.electronAPI.restartAddonServer()rejects here, theaddon-connectedhandler never runs, soisRestartingServerstaystrueand the button remains disabled until reload.Suggested fix
async function restartAddonServer() { isRestartingServer = true; - await window.electronAPI.restartAddonServer(); - reconnectClientSdk(); + try { + await window.electronAPI.restartAddonServer(); + reconnectClientSdk(); + } catch (error) { + isRestartingServer = false; + createNotification({ + id: Math.random().toString(36).substring(7), + message: 'Failed to restart addon server', + type: 'error', + }); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/src/frontend/views/ClientOptionsView.svelte` around lines 474 - 478, The restartAddonServer() function sets isRestartingServer = true but never clears it if window.electronAPI.restartAddonServer() rejects, leaving the UI stuck; update restartAddonServer to wrap the await call in try/catch/finally (or use try { await window.electronAPI.restartAddonServer(); reconnectClientSdk(); } catch (err) { /* optionally log or show error */ } finally { isRestartingServer = false; }) so that isRestartingServer is always reset on success or failure and the button is re-enabled.application/src/frontend/views/ConfigView.svelte (1)
68-79:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMove the success toast after the restart path completes.
This reports
"Addons updated successfully"beforerestartAddonServer()/reconnectClientSdk()run. If either later step fails, the user gets a false success signal for a flow that did not finish cleanly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/src/frontend/views/ConfigView.svelte` around lines 68 - 79, The success toast is shown before the restart and reconnect steps complete; move the createNotification call so it runs only after await window.electronAPI.restartAddonServer() and reconnectClientSdk() succeed. Specifically, in the try block around updateAddons(), call addonUpdates.set([]) as needed, then await window.electronAPI.restartAddonServer(); await (or ensure) reconnectClientSdk() completes, and only after both succeed call createNotification(...) to show "Addons updated successfully"; keep error handling unchanged so failures prevent the success toast.
♻️ Duplicate comments (1)
packages/addon-server/lib/connections/addon.connection.ts (1)
19-25: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
AddonWebSocketstill models thewsAPI, not Bun/standardWebSocket.Removing the import helped, but this boundary is still structurally coupled to
ws: Bun/standardWebSocketdoes not expose.on('message' | 'close' | 'error', ...). So this change does not actually make the connection layer portable to the runtime the repo guidelines call for. The same issue still exists inclient.connection.ts, so it would be better to hide the runtime-specific socket behind a small adapter/interface that uses standard WebSocket semantics.#!/bin/bash set -euo pipefail echo "== addon.connection.ts socket contract ==" sed -n '1,90p' packages/addon-server/lib/connections/addon.connection.ts echo echo "== client.connection.ts socket contract ==" sed -n '1,90p' packages/addon-server/lib/connections/client.connection.ts echo echo "== constructor call sites / websocket implementation hints ==" rg -n -C3 'new AddonConnection|new ClientConnection|\.on\(|Bun\.serve|WebSocketServer|from .ws.|from "ws"' \ packages/addon-server application/src/electronExpected result: the connection classes still depend on emitter-style
.on(...)hooks rather than a standard/BunWebSocketshape.As per coding guidelines "
packages/addon-server/**/*.{ts,tsx,js,jsx}: Use built-inWebSocketinstead ofwspackage".Also applies to: 29-30, 43-45
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/addon-server/lib/connections/addon.connection.ts` around lines 19 - 25, The AddonWebSocket interface currently models the ws emitter API (on('message'...)) which is incompatible with Bun/standard WebSocket; create a small adapter interface (e.g., StandardSocket with send(data), close(code?, reason?), readonly readyState, and DOM-style event handlers onmessage, onclose, onerror) and change AddonConnection and ClientConnection constructors/signatures to accept that adapter type instead of AddonWebSocket; provide concrete adapter implementations that wrap a ws.Socket (or server-side ws) and the native/Bun WebSocket to translate between emitter .on(...) and DOM event handlers, then update usages in addon.connection.ts and client.connection.ts to use the adapter so runtime-specific code is encapsulated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@application/src/frontend/lib/config/client.ts`:
- Around line 20-31: When seeding new addon configs in the initializer, don't
persist defaultValue verbatim — mirror the same fallback rules used in
ConfigurationModal.svelte: for each key in addon.configTemplate (refer to
addon.configTemplate and defaultValue) use defaultValue when present; otherwise
if option.type is "boolean" set false, if "number" set option.min (or 0 if min
missing), if option.allowedValues is a non-empty array use allowedValues[0], and
for strings use ''. Ensure you never write undefined into the generated config
before calling window.electronAPI.fs.write for `./config/${addon.id}.json`.
- Around line 20-33: The code uses addon.id directly in file paths for
window.electronAPI.fs.exists/read/write (in the block that builds
`./config/${addon.id}.json`); validate and sanitize addon.id (from
queryConnectedAddons) before constructing the path to prevent path-traversal:
enforce an allowlist or regex (e.g., only [A-Za-z0-9_-]) and reject/normalize
any other values, then build the file path relative to a fixed config directory
(do not concatenate raw strings) and only operate if the sanitized id passes
validation; update the logic that reads/writes `./config/${addon.id}.json` to
use the validated id and handle validation failures (skip or log error).
In `@application/src/frontend/lib/tasks/runner.ts`:
- Around line 97-103: onFailed currently marks the task as failed via
deferredTasks.update but the surrounding taskRun() resolution still performs
success cleanup (removing the task and emitting a success notification); change
the cleanup path to verify the task's final status before treating it as
success: after taskRun() resolves, query the current deferredTasks (or the task
by taskID) and only call the removal/emit-success logic if the task's status !==
'failed' (or a dedicated failed flag is false). Update the logic around
taskRun(), onFailed, and any removeTask/emitSuccessNotification calls so that a
failure callback prevents subsequent success cleanup and notification.
In `@application/src/frontend/views/FocusedAddonView.svelte`:
- Around line 111-146: The loop over data?.errors currently does "if (!element)
return console.error(...)" which aborts the entire validation handling; change
that to log the missing element and continue the for-loop instead (e.g., call
console.error(...) and then continue) so the rest of the keys are still
processed; update the block that references element/inputElement and the
notifications.update call remains reachable (look for the for (const key in
data?.errors ?? {}) loop, the element variable and inputElement handling, and
ensure no early return is used).
---
Outside diff comments:
In `@application/src/frontend/lib/core/addons.ts`:
- Around line 13-22: The helper currently calls Promise.allSettled and always
returns { success: true, results }, which masks per-addon failures; change it to
compute an aggregate success flag from the settled results (e.g., set success =
results.every(r => r.status === 'fulfilled') so any rejected addon marks overall
success false) and return { success, results } instead; locate the
Promise.allSettled call and the return statement in addons.ts (the variable
results and the returned object) and update the return to include the derived
success boolean while preserving the original results array.
In `@application/src/frontend/store.ts`:
- Around line 59-65: The DeferredTask union now includes 'failed' but the
cleanup logic in clearCompletedTasks() (and any other places that filter
terminal states after runTask()) still only treats 'completed', 'error', and
'cancelled' as finished; update clearCompletedTasks() to also treat 'failed' as
a terminal state so failed tasks are removed when clearing finished work, and
search for any other filters or conditionals around runTask() or task cleanup
that enumerate terminal statuses and add 'failed' there as well.
In `@application/src/frontend/views/ClientOptionsView.svelte`:
- Around line 474-478: The restartAddonServer() function sets isRestartingServer
= true but never clears it if window.electronAPI.restartAddonServer() rejects,
leaving the UI stuck; update restartAddonServer to wrap the await call in
try/catch/finally (or use try { await window.electronAPI.restartAddonServer();
reconnectClientSdk(); } catch (err) { /* optionally log or show error */ }
finally { isRestartingServer = false; }) so that isRestartingServer is always
reset on success or failure and the button is re-enabled.
In `@application/src/frontend/views/ConfigView.svelte`:
- Around line 68-79: The success toast is shown before the restart and reconnect
steps complete; move the createNotification call so it runs only after await
window.electronAPI.restartAddonServer() and reconnectClientSdk() succeed.
Specifically, in the try block around updateAddons(), call addonUpdates.set([])
as needed, then await window.electronAPI.restartAddonServer(); await (or ensure)
reconnectClientSdk() completes, and only after both succeed call
createNotification(...) to show "Addons updated successfully"; keep error
handling unchanged so failures prevent the success toast.
---
Duplicate comments:
In `@packages/addon-server/lib/connections/addon.connection.ts`:
- Around line 19-25: The AddonWebSocket interface currently models the ws
emitter API (on('message'...)) which is incompatible with Bun/standard
WebSocket; create a small adapter interface (e.g., StandardSocket with
send(data), close(code?, reason?), readonly readyState, and DOM-style event
handlers onmessage, onclose, onerror) and change AddonConnection and
ClientConnection constructors/signatures to accept that adapter type instead of
AddonWebSocket; provide concrete adapter implementations that wrap a ws.Socket
(or server-side ws) and the native/Bun WebSocket to translate between emitter
.on(...) and DOM event handlers, then update usages in addon.connection.ts and
client.connection.ts to use the adapter so runtime-specific code is
encapsulated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0c3e4823-1e38-463e-9ba6-ea8041915038
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackages/addon-server/lib/_generated/event-proxy.tsis excluded by!**/_generated/**
📒 Files selected for processing (31)
.github/workflows/build-release.ymlapplication/package.jsonapplication/src/electron/main.tsapplication/src/electron/server/api/addons.tsapplication/src/frontend/App.svelteapplication/src/frontend/components/GameConfiguration.svelteapplication/src/frontend/components/StorePage.svelteapplication/src/frontend/components/built/UpdateAppModal.svelteapplication/src/frontend/components/modal/ConfigurationModal.svelteapplication/src/frontend/lib/config/client.tsapplication/src/frontend/lib/core/addons.tsapplication/src/frontend/lib/core/ipc.tsapplication/src/frontend/lib/tasks/deferred.tsapplication/src/frontend/lib/tasks/runner.tsapplication/src/frontend/managers/AppUpdateManager.svelteapplication/src/frontend/store.tsapplication/src/frontend/views/ClientOptionsView.svelteapplication/src/frontend/views/CommunityAddonsList.svelteapplication/src/frontend/views/ConfigView.svelteapplication/src/frontend/views/FocusedAddonView.sveltepackage.jsonpackages/addon-server/lib/connections/addon.connection.tspackages/addon-server/lib/connections/client.connection.tspackages/addon-server/lib/handlers/client-message-handlers.tspackages/addon-server/lib/server.tspackages/addon-server/scripts/generate-event-proxy.tspackages/client-kit/.gitignorepackages/client-kit/lib/connection.tspackages/connection/README.mdpackages/connection/lib/protocol.tspackages/connection/lib/socket.ts
✅ Files skipped from review due to trivial changes (1)
- packages/client-kit/.gitignore
Comment on lines
+20
to
+31
| if (!window.electronAPI.fs.exists(`./config/${addon.id}.json`)) { | ||
| config = {}; | ||
| for (const key in addon.configTemplate) { | ||
| config[key] = addon.configTemplate[key].defaultValue as | ||
| | number | ||
| | boolean | ||
| | string; | ||
| } | ||
| resolve(addons); | ||
| window.electronAPI.fs.write( | ||
| `./config/${addon.id}.json`, | ||
| JSON.stringify(config, null, 2) | ||
| ); |
Contributor
There was a problem hiding this comment.
Mirror the modal's fallback rules when seeding new addon configs.
This initializer writes defaultValue verbatim for every option, but defaultValue is optional in the wire format. For options without an explicit default, you immediately persist/send undefined here even though ConfigurationModal.svelte falls back to false, min, allowedValues[0], or ''.
Suggested fix
+import {
+ isBooleanOption,
+ isNumberOption,
+ isStringOption,
+} from 'ogi-addon/config';
+
export async function fetchAddonsWithConfigure() {
const addons = await queryConnectedAddons<ConfigTemplateAndInfo>();
await Promise.all(
addons.map(async (addon) => {
let config: Record<string, number | boolean | string>;
if (!window.electronAPI.fs.exists(`./config/${addon.id}.json`)) {
config = {};
- for (const key in addon.configTemplate) {
- config[key] = addon.configTemplate[key].defaultValue as
- | number
- | boolean
- | string;
+ for (const key of Object.keys(addon.configTemplate)) {
+ const option = addon.configTemplate[key];
+ if (isBooleanOption(option)) {
+ config[key] = option.defaultValue ?? false;
+ } else if (isNumberOption(option)) {
+ config[key] = option.defaultValue ?? option.min;
+ } else if (isStringOption(option)) {
+ config[key] =
+ option.defaultValue ?? option.allowedValues?.[0] ?? '';
+ }
}
window.electronAPI.fs.write(
`./config/${addon.id}.json`,
JSON.stringify(config, null, 2)
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@application/src/frontend/lib/config/client.ts` around lines 20 - 31, When
seeding new addon configs in the initializer, don't persist defaultValue
verbatim — mirror the same fallback rules used in ConfigurationModal.svelte: for
each key in addon.configTemplate (refer to addon.configTemplate and
defaultValue) use defaultValue when present; otherwise if option.type is
"boolean" set false, if "number" set option.min (or 0 if min missing), if
option.allowedValues is a non-empty array use allowedValues[0], and for strings
use ''. Ensure you never write undefined into the generated config before
calling window.electronAPI.fs.write for `./config/${addon.id}.json`.
Comment on lines
+20
to
+33
| if (!window.electronAPI.fs.exists(`./config/${addon.id}.json`)) { | ||
| config = {}; | ||
| for (const key in addon.configTemplate) { | ||
| config[key] = addon.configTemplate[key].defaultValue as | ||
| | number | ||
| | boolean | ||
| | string; | ||
| } | ||
| resolve(addons); | ||
| window.electronAPI.fs.write( | ||
| `./config/${addon.id}.json`, | ||
| JSON.stringify(config, null, 2) | ||
| ); | ||
| } else { | ||
| config = JSON.parse(window.electronAPI.fs.read(`./config/${addon.id}.json`)); |
Contributor
There was a problem hiding this comment.
Validate addon.id before using it in a filesystem path.
addon.id comes from queryConnectedAddons() and is interpolated directly into ./config/${addon.id}.json for both reads and writes. Without a local allowlist here, an id containing ../ can escape the config directory.
Suggested fix
+function getAddonConfigPath(addonId: string) {
+ if (!/^[A-Za-z0-9._-]+$/.test(addonId)) {
+ throw new Error(`Invalid addon id: ${addonId}`);
+ }
+ return `./config/${addonId}.json`;
+}
+
export async function fetchAddonsWithConfigure() {
const addons = await queryConnectedAddons<ConfigTemplateAndInfo>();
await Promise.all(
addons.map(async (addon) => {
+ const configPath = getAddonConfigPath(addon.id);
let config: Record<string, number | boolean | string>;
- if (!window.electronAPI.fs.exists(`./config/${addon.id}.json`)) {
+ if (!window.electronAPI.fs.exists(configPath)) {
config = {};
// ...
window.electronAPI.fs.write(
- `./config/${addon.id}.json`,
+ configPath,
JSON.stringify(config, null, 2)
);
} else {
- config = JSON.parse(window.electronAPI.fs.read(`./config/${addon.id}.json`));
+ config = JSON.parse(window.electronAPI.fs.read(configPath));
}Based on learnings: Review code carefully for security vulnerabilities, especially when handling user input or external data.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@application/src/frontend/lib/config/client.ts` around lines 20 - 33, The code
uses addon.id directly in file paths for window.electronAPI.fs.exists/read/write
(in the block that builds `./config/${addon.id}.json`); validate and sanitize
addon.id (from queryConnectedAddons) before constructing the path to prevent
path-traversal: enforce an allowlist or regex (e.g., only [A-Za-z0-9_-]) and
reject/normalize any other values, then build the file path relative to a fixed
config directory (do not concatenate raw strings) and only operate if the
sanitized id passes validation; update the logic that reads/writes
`./config/${addon.id}.json` to use the validated id and handle validation
failures (skip or log error).
Comment on lines
97
to
103
| onFailed: (error: string) => { | ||
| if (!taskID) return; | ||
| deferredTasks.update((tasks) => | ||
| tasks.map((t) => (t.id === taskID ? { ...t, failed: error } : t)) | ||
| tasks.map((t) => | ||
| t.id === taskID ? { ...t, failed: error, status: 'failed' } : t | ||
| ) | ||
| ); |
Contributor
There was a problem hiding this comment.
Don't follow a reported task failure with success cleanup.
onFailed now marks the task as failed, but the function still removes the task and emits a success notification after taskRun() resolves. If the transport reports failure via the callback without rejecting the request, the UI will briefly show an error and then immediately claim success.
Suggested fix
export async function runTask(
result: SearchResultWithAddon,
originalFilePath: string,
libraryInfo?: LibraryInfo
) {
let taskID: string;
+ let taskFailed = false;
const args: {
addonID: string;
manifest: Record<string, unknown>;
downloadPath: string;
@@
onFailed: (error: string) => {
if (!taskID) return;
+ taskFailed = true;
deferredTasks.update((tasks) =>
tasks.map((t) =>
t.id === taskID ? { ...t, failed: error, status: 'failed' } : t
)
);
@@
}).taskRun(args);
- deferredTasks.update((tasks) => tasks.filter((t) => t.id !== taskID));
- createNotification({
- id: Math.random().toString(36).substring(7),
- type: 'success',
- message: 'Task completed',
- });
+ if (!taskFailed) {
+ deferredTasks.update((tasks) => tasks.filter((t) => t.id !== taskID));
+ createNotification({
+ id: Math.random().toString(36).substring(7),
+ type: 'success',
+ message: 'Task completed',
+ });
+ }
return response;
}Also applies to: 112-117
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@application/src/frontend/lib/tasks/runner.ts` around lines 97 - 103, onFailed
currently marks the task as failed via deferredTasks.update but the surrounding
taskRun() resolution still performs success cleanup (removing the task and
emitting a success notification); change the cleanup path to verify the task's
final status before treating it as success: after taskRun() resolves, query the
current deferredTasks (or the task by taskID) and only call the
removal/emit-success logic if the task's status !== 'failed' (or a dedicated
failed flag is false). Update the logic around taskRun(), onFailed, and any
removeTask/emitSuccessNotification calls so that a failure callback prevents
subsequent success cleanup and notification.
Comment on lines
+111
to
+146
| if (data?.success !== true) { | ||
| for (const key in data?.errors ?? {}) { | ||
| const element = document.getElementById(key); | ||
| if (!element) return console.error('element not found'); | ||
| const inputElement = element.matches('[data-input]') | ||
| ? element | ||
| : element | ||
| .closest('[data-input-parent]') | ||
| ?.querySelector('[data-input]'); | ||
|
|
||
| if (inputElement) { | ||
| inputElement.classList.add( | ||
| 'outline-red-500', | ||
| 'outline-2', | ||
| 'outline' | ||
| ); | ||
| } | ||
|
|
||
| const errorMessageElement = element.parentElement!!.querySelector( | ||
| '[data-error-message]' | ||
| ); | ||
| if (errorMessageElement) { | ||
| errorMessageElement.innerHTML = `<img src="./error.svg" alt="error" class="w-6 h-6" />`; | ||
| errorMessageElement.setAttribute('data-context', data.errors[key]); | ||
| const errorMessageElement = element.parentElement!!.querySelector( | ||
| '[data-error-message]' | ||
| ); | ||
| if (errorMessageElement) { | ||
| errorMessageElement.innerHTML = `<img src="./error.svg" alt="error" class="w-6 h-6" />`; | ||
| errorMessageElement.setAttribute('data-context', data.errors[key]); | ||
| } | ||
| } | ||
| notifications.update((update) => [ | ||
| ...update, | ||
| { | ||
| id: Math.random().toString(36).substring(7), | ||
| type: 'error', | ||
| message: | ||
| 'Failed to validate configuration. Configuration will not be usable.', | ||
| }, | ||
| ]); | ||
| return; |
Contributor
There was a problem hiding this comment.
Don’t abort the whole validation pass on one missing element.
return console.error(...) exits the entire callback here. If the server returns one error key that is not rendered in the DOM, the rest of the field highlights and the validation notification are skipped too. This should log and continue instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@application/src/frontend/views/FocusedAddonView.svelte` around lines 111 -
146, The loop over data?.errors currently does "if (!element) return
console.error(...)" which aborts the entire validation handling; change that to
log the missing element and continue the for-loop instead (e.g., call
console.error(...) and then continue) so the rest of the keys are still
processed; update the block that references element/inputElement and the
notifications.update call remains reachable (look for the for (const key in
data?.errors ?? {}) loop, the element variable and inputElement handling, and
ensure no early return is used).
Remove ws from @ogi-sdk/connect and use built-in WebSocket in ogi-addon. Harden addon server secrets, setup flow, and frontend reconnect/config handling.
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
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
Refactor
New Features
Bug Fixes
Chores