feat: Separated Addon Server To A Separate Package#126
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughThis PR refactors the addon-server architecture into a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Build launch URLs with a query separator
Since baseUrl no longer contains ?secret=..., concatenating launch parameters with & produces malformed URLs (for example http://localhost:8080&launchGameId=...) instead of query strings. In both launch-hook and direct-launch flows this prevents launchGameId/hook params from being parsed by the renderer, so Steam shortcut launches and pre/post hook launches can silently lose their launch context.
OpenGameInstaller/packages/addon-server/lib/addon-connection.ts
Lines 459 to 464 in 35414cc
Remove closed websocket clients from addon registries
On websocket close, the connection only rejects pending responses and never unregisters itself from the server’s connections/clients collections. This leaves stale addon IDs marked as connected, so reconnect attempts with the same ID can be rejected as duplicates and APIs that enumerate active addons can return disconnected entries.
ℹ️ 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".
| return; | ||
| } | ||
| resolve(false); | ||
| resolve(stderr.includes('nixos-rebuild')); |
There was a problem hiding this comment.
command -v nixos-rebuild writes the match to stdout, but this check reads stderr, so detectNixOS() now resolves false even on NixOS. That means Nix-specific startup work (like resolving the steamtinkerlaunch path) is skipped for all Nix users, which can break launch configuration on that platform.
Useful? React with 👍 / 👎.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (5)
packages/ogi-addon/src/main.ts (1)
819-823: ⚡ Quick winUse the newly exported
Notificationtype to remove duplicate local interfacesNow that this type is exported, import it where duplicates exist (
application/src/electron/main.tsandapplication/src/electron/server/addon-server.ts) to prevent type drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ogi-addon/src/main.ts` around lines 819 - 823, Replace the duplicate local Notification interface declarations with the exported Notification type: remove the local interface named Notification in the modules that declared it, add an import of the exported Notification type (from the addon package) and update any type annotations/usages to reference that imported Notification type instead of redeclaring it; ensure no other local symbols conflict with the imported Notification and run type checks to confirm the types align for functions/methods that previously used the local interface.packages/addon-server/package.json (2)
22-23: 💤 Low valuePin
@types/bunto a specific version.Using
"latest"for@types/buncan cause non-reproducible builds. Consider pinning to a specific version for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/addon-server/package.json` around lines 22 - 23, The devDependency "@types/bun" is set to "latest" which makes builds non-reproducible; update the package.json devDependencies entry for "@types/bun" to a specific exact version (no "latest" or floating range) by replacing "latest" with a pinned semver (e.g. "x.y.z") to ensure consistent installs—edit the packages/addon-server package.json devDependencies "@types/bun" entry accordingly.
31-35: ⚖️ Poor tradeoffConsider using built-in WebSocket instead of
wslibrary.The
wsandwebsocketpackages are listed as dependencies, but according to the coding guidelines forpackages/addon-server/**, the built-inWebSocketshould be preferred over thewslibrary for WebSocket functionality.Additionally, the
elysiadependency appears unused based on the code inaddon.tswhich uses Node'shttp.createServerandws.WebSocketServerdirectly.As per coding guidelines: "Use built-in
WebSocketinstead ofwslibrary for WebSocket functionality".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/addon-server/package.json` around lines 31 - 35, Remove the external WebSocket deps and unused elysia: delete "ws" and "websocket" from the dependencies in package.json and also remove "elysia" if it's unused; then update addon.ts which currently creates the server with http.createServer and references ws.WebSocketServer to instead use the platform's built-in WebSocket/WebSocketServer APIs (replace any imports or references to the ws package), ensuring server creation and connection handling remain the same but rely on the built-in WebSocket implementation.packages/addon-server/lib/addon.ts (1)
43-48: ⚡ Quick winSecret generation uses weak entropy sources.
The secret is generated using
Date.getTime()combined withMath.random(), both of which are predictable. For security-sensitive secrets, consider usingcrypto.randomUUID()orcrypto.randomBytes()for better entropy.🔒 Proposed fix using crypto
+import { randomUUID } from 'crypto'; + export class AddonServer { // ... public constructor(private readonly config: AddonConfig) { // create a random secret if not provided if (config.securityCheck && !config.secret) { - config.secret = `${Math.floor(new Date().getTime() + Math.random() * 10000)}-${Math.floor(Math.random() * 10000)}`; + config.secret = randomUUID(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/addon-server/lib/addon.ts` around lines 43 - 48, The constructor currently seeds config.secret with predictable values using Date.getTime() and Math.random(); update the Addon constructor to use a cryptographically secure generator instead (e.g., call crypto.randomUUID() or generate bytes via crypto.randomBytes() and hex/base64 encode) when config.securityCheck is true and no config.secret is provided, assigning the secure value to config.secret; ensure you import/require Node's crypto and replace the current `${Math.floor...}` expression used in the constructor with the secure generator so secret creation is non-deterministic.packages/addon-server/lib/addon-connection.ts (1)
188-191: 💤 Low valueSilent return on addon owner mismatch may hide issues.
When
deferredTask.addonOwner !== this.addonInfo!.id, the code returns silently without logging. This could make debugging difficult if an addon incorrectly tries to update another addon's task.📝 Proposed fix to add logging
- if (deferredTask.addonOwner !== this.addonInfo!.id) return; + if (deferredTask.addonOwner !== this.addonInfo!.id) { + console.warn( + `Addon ${this.addonInfo!.id} attempted to update task owned by ${deferredTask.addonOwner}` + ); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/addon-server/lib/addon-connection.ts` around lines 188 - 191, When deferredTask.addonOwner !== this.addonInfo!.id the code returns silently; update that branch to log a warning/error before returning so owner mismatches are visible. Add a log call (using the existing logger instance, e.g., this.logger or processLogger) that records the deferredTask id (deferredTask.id or deferredTask.taskId), the observed deferredTask.addonOwner and the expected this.addonInfo!.id, and any relevant context (e.g., method name in addon-connection.ts) then return as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@application/src/electron/handlers/handler.addon.ts`:
- Around line 64-72: The call to addonServer.stop() is currently synchronous but
this.server.close() is async, causing a race where await startAddonServer() may
run before the port is freed; update the stop() implementation (in addon server
class where this.server.close() is called) to return a Promise that resolves in
the close callback (and rejects on error), and then change the restart flow
(restartAddonServer or the handler using addonServer.stop()) to await
addonServer.stop() before killing processes and calling await
startAddonServer(); alternatively, instead of changing stop(), wrap the existing
close call at the restart site in a Promise and await it before proceeding so
startAddonServer() only runs after the server is fully closed.
In `@application/src/electron/handlers/handler.app.ts`:
- Around line 192-195: The one-shot screen input callback stored in
screenInputCallbacks is not removed if callback(data.data) throws, leaving a
stale entry; update the IPC handler around the retrieval of callback (using
screenInputCallbacks.get(data.id)) so you always remove the entry
(screenInputCallbacks.delete(data.id)) in a finally-like guarantee: get the
callback, if present run it inside a try/catch but ensure
screenInputCallbacks.delete(data.id) is executed regardless of success or error,
and rethrow or handle the error after deletion so behavior is preserved.
In `@application/src/electron/manager/manager.addon.ts`:
- Around line 176-181: The call to executeScript('run', ...) is currently
fire-and-forget so any rejection escapes the surrounding try/catch and becomes
an unhandled rejection; change it to handle the returned Promise (either await
it or attach .catch) and on error invoke the same crash handling used for
startup (write run-crash.log and log the error). Locate executeScript usage in
manager.addon.ts and ensure the Promise returned by executeScript is awaited or
chained with .catch that writes run-crash.log (using addonName/addonPath
context) and rethrows or logs as appropriate so the error is not unhandled.
- Around line 6-7: The import of AddonConnection from '@ogi-sdk/addon-server'
fails because the package's build artifacts (./build/index.mjs,
./build/index.d.mts) are absent; update the repo build workflow to build
packages/addon-server before the app resolves it by adding packages/addon-server
to the root build:typecheck-deps script (or run the package's own npm run build
as a prestep), then re-run the root dependency/setup so the module referenced in
manager.addon.ts (AddonConnection and addon-server) can be resolved; ensure the
package.json export paths in packages/addon-server match the generated build
outputs.
In `@application/src/electron/server/addon-server.ts`:
- Around line 183-205: The event handlers on addonServer (the 'input-asked',
'notification', and 'disconnect' listeners) use implicitly typed parameters;
update the callback signatures to add explicit parameter types to fix the
pipeline errors—for example, annotate the 'input-asked' callback parameters
(title: string, description: string, configuration: YourConfigType | unknown,
reply: (result: unknown) => void) and the 'notification' and 'disconnect'
callbacks (notification: YourNotificationType | unknown, reason: string). Use
the existing types or declare/ import appropriate interfaces (or use unknown
instead of implicit any) and keep the usages in inputAskedHandler and
notificationHandler consistent with those types.
- Around line 236-240: The current use of the void operator on
addonServer.start() silently ignores async rejections; change the call to handle
promise rejections explicitly by awaiting the async start() or attaching a
.catch handler so errors go to onError — e.g., replace the void
addonServer.start() call with an awaited call (await addonServer.start()) inside
an async function or addonServer.start().catch(onError) so any async failures
during WebSocket/setup are passed to onError; keep the existing try/catch for
synchronous throws around the awaited call if you choose await.
- Around line 162-166: The code uses __dirname inside addon-server.ts but
doesn't import it; add the named import for __dirname from
'@/electron/manager/manager.paths.js' at the top of the file so the references
in the block that reads config/option/developer.json (where developerConfig and
isSecurityCheckEnabled are used) use the project’s managed __dirname instead of
an undeclared global.
- Line 4: The build pipeline is missing packages/addon-server so the imported
AddonServer (reference: AddonServer import in
application/src/electron/server/addon-server.ts) may not be built; update the
root build:typecheck-deps script to include cd packages/addon-server && bun run
build (add it after all-debrid) and update the application package.json
build:ogiaddon script to include cd ../addon-server/ && bun x tsdown (add it
after all-debrid) so packages/addon-server is built and typechecked along with
ogi-addon, real-debrid, and all-debrid.
In `@application/src/electron/server/api/defer.ts`:
- Around line 48-53: The current 404 branch prints the entire
deferredTasksManager.getTasks() registry which may expose unrelated task data;
change the behavior in the branch that checks
deferredTasksManager.getTasks()[input.taskID] === undefined so it does not dump
the full registry but logs a minimal warning containing only the missing task ID
(use input.taskID and context such as "task not found") and, if needed, return
the 404/appropriate error to the caller; update references in this block around
deferredTasksManager and input.taskID accordingly.
In `@application/src/electron/server/ipc.ts`:
- Line 1: The project typecheck fails because `@ogi-sdk/addon-server`'s published
exports point to ./build/ artifacts that are missing; update the root script
named build:typecheck-deps to include running the addon-server package build (so
the package's build script is invoked before type checking). Ensure the root
script invokes the addon-server build (referencing the package
`@ogi-sdk/addon-server`) so that the DeferrableTask and other exported symbols
referenced in files like application/src/electron/server/ipc.ts resolve to
generated artifacts prior to running typecheck.
In `@application/src/electron/startup.ts`:
- Around line 295-301: The NixOS detection callback for exec('command -v
nixos-rebuild', ...) currently inspects stderr which is empty on success; update
the callback used to set IS_NIXOS to check stdout (e.g., use stdout.trim() or
stdout.includes('nixos-rebuild')) instead of stderr, so resolve(true) when
stdout contains the command path and resolve(false) on error or empty stdout;
keep the existing error handling and resolution flow in the same callback.
In `@packages/addon-server/.gitignore`:
- Line 15: Replace the literal _.log entry in .gitignore with a wildcard pattern
so all log files are ignored: locate the "_.log" pattern and change it to
"*.log" so any filename ending in .log is matched.
In `@packages/addon-server/lib/addon-connection.ts`:
- Line 12: Update the misspelled import path from "@/deffered" to "@/deferred"
where DeferrableTask and DeferredTasksManager are imported (in the import
statement that references DeferrableTask, DeferredTasksManager); also rename the
source module file from deffered.ts to deferred.ts so the module resolution
matches the corrected import, and update any other imports that reference the
old "deffered" filename to "deferred".
In `@packages/addon-server/lib/addon.ts`:
- Around line 93-103: The stop() method may close the same WebSocket twice
because AddonConnection instances appear in both this.connections and
this.clients; update stop() to deduplicate WebSocket closes by tracking
already-closed sockets (e.g., a Set of ws instances) and only call ws.close() if
the ws is not already in the set. Instruct changes in the stop() method to
iterate this.connections and this.clients, add each connection.ws to the seen
set after closing (or skip if already seen), then call this.server.close() and
clear both collections as before.
- Around line 89-91: getSecret() currently uses a non-null assertion on
this.config.secret which can be undefined at runtime; update getSecret to
explicitly handle the missing-secret case: either change its signature to return
string | undefined and return this.config.secret, or (preferred) keep returning
string but validate and throw a descriptive Error when this.config.secret is
undefined (e.g., "Missing secret in config") so callers of getSecret() never
receive undefined; modify the method body in the getSecret implementation and
any callers that assume a string accordingly.
---
Nitpick comments:
In `@packages/addon-server/lib/addon-connection.ts`:
- Around line 188-191: When deferredTask.addonOwner !== this.addonInfo!.id the
code returns silently; update that branch to log a warning/error before
returning so owner mismatches are visible. Add a log call (using the existing
logger instance, e.g., this.logger or processLogger) that records the
deferredTask id (deferredTask.id or deferredTask.taskId), the observed
deferredTask.addonOwner and the expected this.addonInfo!.id, and any relevant
context (e.g., method name in addon-connection.ts) then return as before.
In `@packages/addon-server/lib/addon.ts`:
- Around line 43-48: The constructor currently seeds config.secret with
predictable values using Date.getTime() and Math.random(); update the Addon
constructor to use a cryptographically secure generator instead (e.g., call
crypto.randomUUID() or generate bytes via crypto.randomBytes() and hex/base64
encode) when config.securityCheck is true and no config.secret is provided,
assigning the secure value to config.secret; ensure you import/require Node's
crypto and replace the current `${Math.floor...}` expression used in the
constructor with the secure generator so secret creation is non-deterministic.
In `@packages/addon-server/package.json`:
- Around line 22-23: The devDependency "@types/bun" is set to "latest" which
makes builds non-reproducible; update the package.json devDependencies entry for
"@types/bun" to a specific exact version (no "latest" or floating range) by
replacing "latest" with a pinned semver (e.g. "x.y.z") to ensure consistent
installs—edit the packages/addon-server package.json devDependencies
"@types/bun" entry accordingly.
- Around line 31-35: Remove the external WebSocket deps and unused elysia:
delete "ws" and "websocket" from the dependencies in package.json and also
remove "elysia" if it's unused; then update addon.ts which currently creates the
server with http.createServer and references ws.WebSocketServer to instead use
the platform's built-in WebSocket/WebSocketServer APIs (replace any imports or
references to the ws package), ensuring server creation and connection handling
remain the same but rely on the built-in WebSocket implementation.
In `@packages/ogi-addon/src/main.ts`:
- Around line 819-823: Replace the duplicate local Notification interface
declarations with the exported Notification type: remove the local interface
named Notification in the modules that declared it, add an import of the
exported Notification type (from the addon package) and update any type
annotations/usages to reference that imported Notification type instead of
redeclaring it; ensure no other local symbols conflict with the imported
Notification and run type checks to confirm the types align for
functions/methods that previously used the local interface.
🪄 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: df4222ae-1e28-45d2-9a76-a5ac472308db
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
.vscode/settings.jsonapplication/electron.vite.config.tsapplication/package.jsonapplication/src/electron/handlers/handler.addon.tsapplication/src/electron/handlers/handler.app.tsapplication/src/electron/handlers/handler.rest.tsapplication/src/electron/main.tsapplication/src/electron/manager/manager.addon.tsapplication/src/electron/server/addon-server.tsapplication/src/electron/server/api/addons.tsapplication/src/electron/server/api/defer.tsapplication/src/electron/server/constants.tsapplication/src/electron/server/dev-server.tsapplication/src/electron/server/ipc.tsapplication/src/electron/startup.tsapplication/src/frontend/App.svelteapplication/src/frontend/components/PlayPage.svelteapplication/src/frontend/components/StorePage.svelteapplication/src/frontend/tsconfig.jsonapplication/src/frontend/views/FocusedAddonView.svelteapplication/typings/ogi-addon.d.tspackage.jsonpackages/addon-server/.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdcpackages/addon-server/.gitignorepackages/addon-server/CLAUDE.mdpackages/addon-server/README.mdpackages/addon-server/lib/addon-connection.tspackages/addon-server/lib/addon.tspackages/addon-server/lib/deffered.tspackages/addon-server/lib/index.tspackages/addon-server/lib/lib.tspackages/addon-server/package.jsonpackages/addon-server/tsconfig.jsonpackages/addon-server/tsdown.config.jspackages/ogi-addon/src/main.ts
💤 Files with no reviewable changes (3)
- application/typings/ogi-addon.d.ts
- application/src/electron/server/constants.ts
- application/src/electron/server/dev-server.ts
| addonServer.stop(); | ||
| // stop all of the addons | ||
| for (const process of Object.keys(processes)) { | ||
| console.log(`Killing process ${process}`); | ||
| const killed = processes[process].kill('SIGKILL'); | ||
| console.log(`Killed process ${process}: ${killed}`); | ||
| } | ||
| // start the server and wait for it to be listening before starting addons | ||
| await new Promise<void>((resolve, reject) => { | ||
| server.once('error', reject); | ||
| server.listen(port, () => { | ||
| server.removeListener('error', reject); | ||
| console.log(`Addon Server is running on http://localhost:${port}`); | ||
| console.log(`Server is being executed by electron!`); | ||
| resolve(); | ||
| }); | ||
| }); | ||
| await startAddonServer(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== restartAddonServer callsite =="
sed -n '61,76p' application/src/electron/handlers/handler.addon.ts
echo
echo "== addon server start/stop definitions =="
rg -n -C3 'class AddonServer|stop\s*\(|startAddonServer|listen\s*\(' \
packages/addon-server \
application/src/electron/serverRepository: Nat3z/OpenGameInstaller
Length of output: 4155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full AddonServer class implementation =="
rg -n -A 150 'export class AddonServer' packages/addon-server/lib/addon.ts | head -200
echo
echo "== Full startAddonServer implementation =="
rg -n -A 30 'function startAddonServer' application/src/electron/server/addon-server.ts
echo
echo "== Server object initialization in addon-server.ts =="
rg -n -B 5 'server\.listen|new.*Server|import.*Server|const.*server' application/src/electron/server/addon-server.ts | head -50Repository: Nat3z/OpenGameInstaller
Length of output: 4447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full addonServer initialization and setup =="
rg -n -A 20 'const addonServer = new AddonServer' application/src/electron/server/addon-server.ts
echo
echo "== Search for addonServer.extend calls =="
rg -n 'addonServer\.extend|\.extend\(' application/src/electron/server/addon-server.tsRepository: Nat3z/OpenGameInstaller
Length of output: 733
Make addonServer.stop() awaitable or add proper close handling before restarting the server.
The addonServer.stop() method calls this.server.close() synchronously (line 100 of packages/addon-server/lib/addon.ts), but http.Server.close() is asynchronous and doesn't block. In restartAddonServer(), calling stop() without awaiting means await startAddonServer() runs before the port is fully released, creating a potential EADDRINUSE race condition. Either make stop() return a Promise that waits for the close callback, or add explicit close handling in the restart flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@application/src/electron/handlers/handler.addon.ts` around lines 64 - 72, The
call to addonServer.stop() is currently synchronous but this.server.close() is
async, causing a race where await startAddonServer() may run before the port is
freed; update the stop() implementation (in addon server class where
this.server.close() is called) to return a Promise that resolves in the close
callback (and rejects on error), and then change the restart flow
(restartAddonServer or the handler using addonServer.stop()) to await
addonServer.stop() before killing processes and calling await
startAddonServer(); alternatively, instead of changing stop(), wrap the existing
close call at the restart site in a Promise and await it before proceeding so
startAddonServer() only runs after the server is fully closed.
| const callback = screenInputCallbacks.get(data.id); | ||
| if (callback) { | ||
| callback(data.data); | ||
| screenInputCallbacks.delete(data.id); |
There was a problem hiding this comment.
Always clear one-shot screen callbacks even if they throw.
If callback(data.data) throws, the map entry survives and this IPC handler rejects, so a stale callback can hang around indefinitely.
Suggested fix
const callback = screenInputCallbacks.get(data.id);
if (callback) {
- callback(data.data);
- screenInputCallbacks.delete(data.id);
+ screenInputCallbacks.delete(data.id);
+ callback(data.data);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const callback = screenInputCallbacks.get(data.id); | |
| if (callback) { | |
| callback(data.data); | |
| screenInputCallbacks.delete(data.id); | |
| const callback = screenInputCallbacks.get(data.id); | |
| if (callback) { | |
| screenInputCallbacks.delete(data.id); | |
| callback(data.data); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@application/src/electron/handlers/handler.app.ts` around lines 192 - 195, The
one-shot screen input callback stored in screenInputCallbacks is not removed if
callback(data.data) throws, leaving a stale entry; update the IPC handler around
the retrieval of callback (using screenInputCallbacks.get(data.id)) so you
always remove the entry (screenInputCallbacks.delete(data.id)) in a finally-like
guarantee: get the callback, if present run it inside a try/catch but ensure
screenInputCallbacks.delete(data.id) is executed regardless of success or error,
and rethrow or handle the error after deletion so behavior is preserved.
| const port = 7654; | ||
| import http from 'http'; | ||
| import { WebSocketServer } from 'ws'; | ||
| import { AddonServer } from '@ogi-sdk/addon-server'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the package is properly configured in the workspace
cat package.json 2>/dev/null | jq '.workspaces' || echo "No root package.json found"
# Check if the addon-server package exists and has build output
fd -t d "addon-server" packages/
ls -la packages/addon-server/build/ 2>/dev/null || echo "Build directory not found"
# Check tsconfig paths configuration
cat application/tsconfig.json 2>/dev/null | jq '.compilerOptions.paths' || echo "No paths config"Repository: Nat3z/OpenGameInstaller
Length of output: 190
🏁 Script executed:
# Check the package.json in packages/addon-server
cat packages/addon-server/package.json 2>/dev/null | jq '.name, .main, .exports, .type' || echo "No package.json in addon-server"
# Check application's package.json dependencies
cat application/package.json 2>/dev/null | jq '.dependencies, .workspaces' || echo "No package.json in application"
# Check tsconfig configurations
echo "=== application/tsconfig.json ==="
cat application/tsconfig.json 2>/dev/null | jq '.compilerOptions.baseUrl, .compilerOptions.paths, .extends' || echo "No config"
echo "=== packages/addon-server/tsconfig.json ==="
cat packages/addon-server/tsconfig.json 2>/dev/null | jq '.compilerOptions.baseUrl, .compilerOptions.paths, .name' || echo "No config"
# Verify the actual import in addon-server.ts
echo "=== Checking import in the file ==="
head -10 application/src/electron/server/addon-server.ts | cat -nRepository: Nat3z/OpenGameInstaller
Length of output: 1686
🏁 Script executed:
# Check build scripts in addon-server package
cat packages/addon-server/package.json | jq '.scripts'
# List the actual contents of addon-server directory
echo "=== Contents of packages/addon-server/ ==="
ls -la packages/addon-server/ | head -20
# Check if there's a source directory
echo "=== Source structure ==="
find packages/addon-server -type f -name "*.ts" -o -name "*.js" | head -10Repository: Nat3z/OpenGameInstaller
Length of output: 1002
🏁 Script executed:
# Check tsdown configuration
echo "=== packages/addon-server/tsdown.config.js ==="
cat packages/addon-server/tsdown.config.js
# Check tsconfig
echo "=== packages/addon-server/tsconfig.json ==="
cat packages/addon-server/tsconfig.json
# Check the main source file
echo "=== packages/addon-server/lib/index.ts ==="
cat packages/addon-server/lib/index.tsRepository: Nat3z/OpenGameInstaller
Length of output: 1315
🏁 Script executed:
# Check if build is gitignored
echo "=== packages/addon-server/.gitignore ==="
cat packages/addon-server/.gitignore
# Check root .gitignore
echo "=== Root .gitignore (if build patterns exist) ==="
grep -i "build" .gitignore 2>/dev/null || echo "No build patterns in root .gitignore"Repository: Nat3z/OpenGameInstaller
Length of output: 683
🏁 Script executed:
cd packages/addon-server && npm run buildRepository: Nat3z/OpenGameInstaller
Length of output: 504
🏁 Script executed:
# Check if tsdown is a dev dependency
echo "=== packages/addon-server/package.json devDependencies ==="
cat packages/addon-server/package.json | jq '.devDependencies'
# Check root package.json for tsdown
echo "=== Root package.json devDependencies (tsdown) ==="
cat package.json | jq '.devDependencies' | grep -A2 -B2 "tsdown"
# Check if node_modules exists in addon-server
ls -la packages/addon-server/node_modules 2>/dev/null | head -5 || echo "No node_modules in addon-server"
# Check if tsdown is in root node_modules
ls -la node_modules/.bin/tsdown 2>/dev/null || echo "tsdown not in root node_modules"Repository: Nat3z/OpenGameInstaller
Length of output: 321
🏁 Script executed:
# Check root package.json devDependencies completely
echo "=== Root package.json ==="
cat package.json | jq '.devDependencies' | head -30
# Check if node_modules exists at root
ls -la node_modules 2>/dev/null | head -10 || echo "No node_modules at root"
# Check what package manager is being used
echo "=== Package manager check ==="
ls -la | grep -E "package-lock|yarn.lock|bun.lockb|pnpm-lock"
# Check if there's a built version somewhere else
echo "=== Search for build outputs ==="
find . -path ./node_modules -prune -o -name "index.cjs" -o -name "index.mjs" | grep -v node_modulesRepository: Nat3z/OpenGameInstaller
Length of output: 272
🏁 Script executed:
# Check root package.json completely
cat package.json | jq .
# Check for lock files more broadly
echo "=== Lock files ==="
ls -la | grep -E "lock|bun"
# Check if there's a build script at root
echo "=== Root build script ==="
cat package.json | jq '.scripts'
# List what's in application directory
echo "=== application package.json scripts ==="
cat application/package.json | jq '.scripts' | head -20Repository: Nat3z/OpenGameInstaller
Length of output: 4333
🏁 Script executed:
# Check which packages are built in the dependency build script
echo "=== Checking which packages are actually built ==="
grep -A 5 "build:typecheck-deps\|build:ogiaddon" package.json application/package.json
# Check addon-server package.json again to confirm it has build script
cat packages/addon-server/package.json | jq '.scripts'
# List all packages to see what should be built
echo "=== All packages ==="
ls -d packages/*/package.json | xargs -I {} sh -c 'echo "{}:"; cat {} | jq ".name"'Repository: Nat3z/OpenGameInstaller
Length of output: 2845
🏁 Script executed:
# Verify that addon-server is NOT in any build scripts
echo "=== Checking all build-related scripts ==="
cat package.json application/package.json | jq '.scripts | keys[] | select(contains("build"))'
# Confirm the dependency chain
echo "=== application dependency on addon-server ==="
cat application/package.json | jq '.dependencies | keys[] | select(contains("addon"))'
# Check if there's any mention of addon-server in build docs
find . -maxdepth 2 -name "*.md" -exec grep -l "addon-server\|build" {} \; | head -5Repository: Nat3z/OpenGameInstaller
Length of output: 279
🏁 Script executed:
# Check the README files for build instructions
echo "=== README.md ==="
head -50 README.md
echo "=== packages/addon-server/README.md ==="
cat packages/addon-server/README.mdRepository: Nat3z/OpenGameInstaller
Length of output: 2702
Critical: @ogi-sdk/addon-server is missing from the build pipeline.
The application depends on @ogi-sdk/addon-server but the package is not included in any build scripts. The root build:typecheck-deps and application build:ogiaddon scripts only build ogi-addon, real-debrid, and all-debrid—addon-server is excluded.
Add packages/addon-server to the build pipeline by including it in the build scripts:
// In root package.json
"build:typecheck-deps": "cd packages/ogi-addon && bun run build && cd ../real-debrid && bun run build && cd ../all-debrid && bun run build && cd ../addon-server && bun run build"
// In application/package.json
"build:ogiaddon": "cd ../packages/ogi-addon/ && bun x tsdown && cd ../real-debrid/ && bun x tsdown && cd ../all-debrid/ && bun x tsdown && cd ../addon-server/ && bun x tsdown"
🧰 Tools
🪛 GitHub Actions: Typecheck
[error] 4-4: svelte-check: Cannot find module '@ogi-sdk/addon-server' or its corresponding type declarations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@application/src/electron/server/addon-server.ts` at line 4, The build
pipeline is missing packages/addon-server so the imported AddonServer
(reference: AddonServer import in
application/src/electron/server/addon-server.ts) may not be built; update the
root build:typecheck-deps script to include cd packages/addon-server && bun run
build (add it after all-debrid) and update the application package.json
build:ogiaddon script to include cd ../addon-server/ && bun x tsdown (add it
after all-debrid) so packages/addon-server is built and typechecked along with
ogi-addon, real-debrid, and all-debrid.
| exec('command -v nixos-rebuild', (error, _stdout, stderr) => { | ||
| if (error) { | ||
| console.error(`exec error: ${error}`); | ||
| resolve(false); | ||
| return; | ||
| } | ||
| if (stderr.includes('nixos-rebuild')) { | ||
| resolve(true); | ||
| return; | ||
| } | ||
| resolve(false); | ||
| resolve(stderr.includes('nixos-rebuild')); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current implementation checks stderr and demonstrate command output channel behavior.
set -euo pipefail
echo "1) Inspect detectNixOS implementation"
fd -i "startup.ts" | xargs -I{} sed -n '292,306p' {}
echo
echo "2) Demonstrate where command -v writes output"
{ command -v sh; } > /tmp/cmdv.stdout 2>/tmp/cmdv.stderr || true
echo "stdout bytes: $(wc -c < /tmp/cmdv.stdout)"
echo "stderr bytes: $(wc -c < /tmp/cmdv.stderr)"
echo "stdout content:"
cat /tmp/cmdv.stdoutRepository: Nat3z/OpenGameInstaller
Length of output: 566
Fix NixOS detection to check stdout instead of stderr
Line 300 checks stderr, but command -v nixos-rebuild writes the path to stdout when found and leaves stderr empty. This causes IS_NIXOS to always resolve to false even on valid NixOS systems.
Proposed fix
function detectNixOS(): Promise<boolean> {
return new Promise<boolean>((resolve) => {
try {
exec('command -v nixos-rebuild', (error, _stdout, stderr) => {
if (error) {
resolve(false);
return;
}
- resolve(stderr.includes('nixos-rebuild'));
+ resolve(_stdout.trim().length > 0);
});
} catch {
resolve(false);
}
});
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@application/src/electron/startup.ts` around lines 295 - 301, The NixOS
detection callback for exec('command -v nixos-rebuild', ...) currently inspects
stderr which is empty on success; update the callback used to set IS_NIXOS to
check stdout (e.g., use stdout.trim() or stdout.includes('nixos-rebuild'))
instead of stderr, so resolve(true) when stdout contains the command path and
resolve(false) on error or empty stdout; keep the existing error handling and
resolution flow in the same callback.
|
|
||
| # logs | ||
| logs | ||
| _.log |
There was a problem hiding this comment.
Fix the log file pattern.
Line 15 uses _.log which matches only a file literally named _.log (underscore is a literal character in .gitignore, not a wildcard). This should be *.log to properly ignore all log files.
🔧 Proposed fix
-_.log
+*.log📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _.log | |
| *.log |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/addon-server/.gitignore` at line 15, Replace the literal _.log entry
in .gitignore with a wildcard pattern so all log files are ignored: locate the
"_.log" pattern and change it to "*.log" so any filename ending in .log is
matched.
| public getSecret(): string { | ||
| return this.config.secret!; | ||
| } |
There was a problem hiding this comment.
getSecret() may return undefined at runtime when security is disabled.
When securityCheck is false and no secret is provided, config.secret remains undefined. The non-null assertion (!) will compile but return undefined at runtime, violating the string return type.
🛡️ Proposed fix to handle undefined case
public getSecret(): string {
- return this.config.secret!;
+ return this.config.secret ?? '';
}Or throw an error if caller requires a valid secret:
public getSecret(): string {
+ if (!this.config.secret) {
+ throw new Error('Secret not configured - securityCheck is disabled');
+ }
return this.config.secret;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public getSecret(): string { | |
| return this.config.secret!; | |
| } | |
| public getSecret(): string { | |
| return this.config.secret ?? ''; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/addon-server/lib/addon.ts` around lines 89 - 91, getSecret()
currently uses a non-null assertion on this.config.secret which can be undefined
at runtime; update getSecret to explicitly handle the missing-secret case:
either change its signature to return string | undefined and return
this.config.secret, or (preferred) keep returning string but validate and throw
a descriptive Error when this.config.secret is undefined (e.g., "Missing secret
in config") so callers of getSecret() never receive undefined; modify the method
body in the getSecret implementation and any callers that assume a string
accordingly.
| public stop(): void { | ||
| this.connections.forEach((connection) => { | ||
| connection.ws.close(); | ||
| }); | ||
| this.clients.forEach((client) => { | ||
| client.ws.close(); | ||
| }); | ||
| this.server.close(); | ||
| this.connections.clear(); | ||
| this.clients.clear(); | ||
| } |
There was a problem hiding this comment.
stop() may close the same WebSocket twice.
this.connections and this.clients can contain overlapping AddonConnection instances (a connection is added to connections on connect and to clients after authentication). Closing the same WebSocket twice may cause issues or log errors.
♻️ Proposed fix to avoid double-close
public stop(): void {
- this.connections.forEach((connection) => {
- connection.ws.close();
- });
- this.clients.forEach((client) => {
- client.ws.close();
- });
+ // Close all connections (clients are a subset of connections)
+ for (const connection of this.connections) {
+ connection.ws.close();
+ }
this.server.close();
this.connections.clear();
this.clients.clear();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/addon-server/lib/addon.ts` around lines 93 - 103, The stop() method
may close the same WebSocket twice because AddonConnection instances appear in
both this.connections and this.clients; update stop() to deduplicate WebSocket
closes by tracking already-closed sockets (e.g., a Set of ws instances) and only
call ws.close() if the ws is not already in the set. Instruct changes in the
stop() method to iterate this.connections and this.clients, add each
connection.ws to the seen set after closing (or skip if already seen), then call
this.server.close() and clear both collections as before.
…ion more uniform across versions of OGI
- Added support for the new '@ogi-sdk/executor' package in the project. - Updated build scripts to include the executor build process. - Refactored addon management to utilize the new Addon class from the executor, improving addon loading and execution consistency. - Implemented logging for setup scripts and improved error handling during addon installation. - Removed deprecated process management in favor of the new executor methods.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
This pull request introduces significant refactoring and modernization of the add-on server integration in the Electron application. The main changes include replacing internal implementations with the
@ogi-sdk/addon-serverpackage, improving type safety and process management, and simplifying the add-on server startup and IPC handling. Additionally, the PR updates development tooling and makes several dependency and configuration adjustments.Add-on Server Integration Refactor:
AddonConnectionand related logic with the@ogi-sdk/addon-serverpackage throughout the Electron codebase, updating imports and method calls accordingly. (application/src/electron/handlers/handler.addon.ts,application/src/electron/manager/manager.addon.ts,application/src/electron/handlers/handler.app.ts,application/src/electron/server/addon-server.ts) [1] [2] [3] [4]clientsandserverwithaddonServerandaddonIPCmethods, and updating how add-on server is started and stopped. (application/src/electron/handlers/handler.addon.ts,application/src/electron/main.ts) [1] [2]IPC and UI Interaction Improvements:
screenInputCallbacksmap, ensuring responses are handled asynchronously and safely. (application/src/electron/main.ts,application/src/electron/handlers/handler.app.ts) [1] [2]application/src/electron/main.ts)Security and Startup Changes:
applicationAddonSecretfrom URLs and configuration, simplifying the security model and startup logic. (application/src/electron/main.ts) [1] [2] [3] [4]application/src/electron/main.ts)Dependency and Configuration Updates:
package.jsonto useworkspace:*forreal-debrid-jsand added@ogi-sdk/addon-serveras a dependency. (application/package.json)application/package.json)ogi-addonfrom dependency externalization. (application/electron.vite.config.ts)server/directory for TypeScript and JavaScript. (.vscode/settings.json)Overall, these changes modernize the add-on server architecture, improve maintainability, and streamline both development and runtime behaviors.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores