Conversation
WalkthroughAdds bounded server readiness polling, idempotent/centralized server shutdown, fail-fast server startup, SIGINT/SIGTERM shutdown handlers, startup try/catch with clean quits, and introduces a gesture reset helper that clears gesture refs and conditionally preserves the dragging timeout. Changes
Sequence Diagram(s)sequenceDiagram
participant ElectronApp as Electron App
participant Starter as startServer()
participant ServerProc as Server Process
participant Poll as waitForServer()
participant WebServer as Nitro HTTP Server
participant OS as OS Signals
ElectronApp->>Starter: invoke startServer()
Starter->>ServerProc: spawn server process
ServerProc-->>Starter: spawn error / early exit (optional)
alt spawn error / early exit
Starter->>Starter: reject Promise / call stopServer()
Starter-->>ElectronApp: startup fails
else spawned
Starter->>Poll: begin polling (maxRetries, per-request timeout)
Poll->>WebServer: HTTP GET /health
alt 200 OK
WebServer-->>Poll: 200
Poll-->>Starter: resolve
Starter-->>ElectronApp: startServer resolves
ElectronApp->>ElectronApp: createWindow()
else non-200 / timeout (retries)
WebServer-->>Poll: non-200 / no response
Poll-->>Poll: retry until maxRetries
Poll-->>Starter: reject after maxRetries
Starter->>Starter: call stopServer()
Starter-->>ElectronApp: startup fails
end
end
OS-->>ElectronApp: SIGINT / SIGTERM
ElectronApp->>Starter: shutdown()
Starter->>ServerProc: stopServer() (SIGTERM, kill fallback)
Starter->>ElectronApp: app.quit()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/main.cjs`:
- Around line 129-137: Create a single helper (e.g., ensureServerStopped or
killServerProcess) that encapsulates the child-teardown logic currently in
shutdown (checking serverProcess and calling serverProcess.kill('SIGTERM') and
clearing/setting serverProcess to null), then replace the inline kill logic in
shutdown(), the 'window-all-closed' handler, and the catch block after
waitForServer() so each path calls this helper before calling app.quit(); also
ensure the helper is idempotent and safe to call if serverProcess is already
null or has exited.
- Around line 89-103: The startup Promise only listens for
serverProcess.once('exit') but not serverProcess.once('error'), so spawn
failures bypass the Promise and crash the main process; add a
serverProcess.once('error', (err) => { if (!resolved) reject(new Error(...
include err.message or err))); } handler (paired with the existing exit handler
and the resolved flag) so errors from spawn/fork/signup are routed to reject,
and ensure any existing waitForServer().catch(reject) remains unchanged; target
the serverProcess variable and the resolved/resolve/reject logic around
waitForServer to implement this.
- Around line 36-46: The readiness probe in function check uses http.get(url,
...) without a timeout, so a hung TCP connection can block retries; update the
check logic that calls http.get to set a per-request timeout (e.g., via
request.setTimeout or passing a timeout option), listen for the 'timeout' event
and abort/destroy the request/response, then call retry() (and emit an error
path if needed) so the retry cap advances; ensure you also wire the 'error' and
'response' callbacks to properly clear any timers and avoid leaks when using the
http.get/request object and keep references to url, retry, and check to locate
changes.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 30cfd4ba-7c52-4bc8-863a-4c78956ab775
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
electron/main.cjssrc/hooks/useTrackpadGesture.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/useTrackpadGesture.ts (1)
30-31: 🧹 Nitpick | 🔵 TrivialConsider using explicit typing for the
sendcallback.The
sendparameter is typed as(msg: unknown) => void. For better type safety, consider importing and using theInputMessageinterface fromsrc/server/InputHandler.ts.♻️ Suggested improvement
+import type { InputMessage } from "../server/InputHandler" + export const useTrackpadGesture = ( - send: (msg: unknown) => void, + send: (msg: InputMessage) => void, scrollMode: boolean,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useTrackpadGesture.ts` around lines 30 - 31, The send parameter in useTrackpadGesture is currently typed as (msg: unknown) => void; update it to use the concrete InputMessage type for stronger typing: import the InputMessage interface and change the signature to send: (msg: InputMessage) => void (and update any call sites or generics accordingly) so useTrackpadGesture and its callers compile with the stricter type; reference: useTrackpadGesture, send, InputMessage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/main.cjs`:
- Around line 97-104: The probe is using a hardcoded localhost URL while Nitro
is bound to serverHost, causing false "down" detections; create a single helper
(e.g., getServerUrl(serverHost, serverPort)) that derives the connect host (use
127.0.0.1 or [::1] when serverHost is a wildcard like 0.0.0.0/::, otherwise use
serverHost) and returns the full URL, then replace the probe URL and the
mainWindow.loadURL(...) call to use getServerUrl() and ensure spawn still sets
env HOST and PORT from serverHost/serverPort.
- Around line 31-39: stopServer currently sends SIGTERM and immediately sets
serverProcess = null, losing the handle to observe exit or escalate; change
stopServer to keep serverProcess until it emits 'exit' or is force-killed: call
serverProcess.kill('SIGTERM'), attach an 'exit' and 'error' listener on
serverProcess that clears a 5-second timeout, removes listeners and then sets
serverProcess = null, and start a timer (e.g., 5000ms) that if reached will call
serverProcess.kill('SIGKILL') and then cleanup; ensure listeners are removed in
both normal and forced paths and that errors from kill() are caught/logged to
avoid throwing.
---
Outside diff comments:
In `@src/hooks/useTrackpadGesture.ts`:
- Around line 30-31: The send parameter in useTrackpadGesture is currently typed
as (msg: unknown) => void; update it to use the concrete InputMessage type for
stronger typing: import the InputMessage interface and change the signature to
send: (msg: InputMessage) => void (and update any call sites or generics
accordingly) so useTrackpadGesture and its callers compile with the stricter
type; reference: useTrackpadGesture, send, InputMessage.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 78e20436-b59a-4453-a0bf-018db0a7eaf9
📒 Files selected for processing (2)
electron/main.cjssrc/hooks/useTrackpadGesture.ts
| function stopServer() { | ||
| if (serverProcess) { | ||
| try { | ||
| serverProcess.kill('SIGTERM'); | ||
| } catch (e) { | ||
| console.warn('Error stopping server:', e.message); | ||
| } | ||
| serverProcess = null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In current Node.js, does child_process.ChildProcess.kill('SIGTERM') wait for the child to exit, or does it only send the signal and return immediately? Also, what does a false return value from kill() mean?
💡 Result:
In current Node.js, child_process.ChildProcess.kill('SIGTERM') sends the SIGTERM signal to the child process and returns immediately without waiting for the child to exit. The child process may handle the signal, exit gracefully, or ignore it depending on its implementation. A false return value from kill means the underlying kill(2) system call failed (e.g., the process no longer exists, invalid signal on the platform, or permission denied). It does not throw an error synchronously but may emit an 'error' event asynchronously if the signal could not be delivered.
Citations:
- 1: https://docs.deno.com/api/node/child_process/~/ChildProcess.prototype.kill
- 2: https://github.com/nodejs/node/blob/main/lib/internal/child_process.js
- 3: https://nodejs.org/api/child_process.html
- 4: https://bun.com/reference/node/child_process/ChildProcess/kill
- 5: child_process kill() swallows synchronously known problems (does not error out) nodejs/node#30668
Wait for child process exit before clearing the reference.
stopServer() sends SIGTERM and immediately nulls serverProcess, but kill('SIGTERM') returns instantly without waiting for the child to exit. If Nitro ignores the signal or takes time to shut down, the reference is lost and cannot be used to observe exit or escalate to SIGKILL. This allows the child process to be orphaned even though you intend to terminate it.
Keep the reference, listen for the 'exit' event, then apply a timeout-based force-kill if the child doesn't terminate within a reasonable window (e.g., 5 seconds).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/main.cjs` around lines 31 - 39, stopServer currently sends SIGTERM
and immediately sets serverProcess = null, losing the handle to observe exit or
escalate; change stopServer to keep serverProcess until it emits 'exit' or is
force-killed: call serverProcess.kill('SIGTERM'), attach an 'exit' and 'error'
listener on serverProcess that clears a 5-second timeout, removes listeners and
then sets serverProcess = null, and start a timer (e.g., 5000ms) that if reached
will call serverProcess.kill('SIGKILL') and then cleanup; ensure listeners are
removed in both normal and forced paths and that errors from kill() are
caught/logged to avoid throwing.
There was a problem hiding this comment.
Thanks for the detailed review!
I've addressed all the reported issues:
- Added a timeout to the HTTP readiness probe in
waitForServerto prevent hanging requests when the server accepts a connection but does not respond. - Handled
serverProcess.once('error')to properly catch spawn failures and avoid unhandled crashes. - Introduced a centralized
stopServer()helper to encapsulate teardown logic and ensure idempotent cleanup. - Replaced duplicate
serverProcess.killcalls acrossshutdown,window-all-closed, and startup failure paths with the new helper. - Ensured the server process is properly terminated on startup failure to avoid orphan processes.
These changes make the startup flow more robust and prevent edge-case hangs and resource leaks.
Please let me know if anything else needs improvement
There was a problem hiding this comment.
@Shashank-Mengani, thank you for the detailed summary — the improvements you've made are solid. However, the core concern from the original review comment is still unaddressed in the current stopServer():
function stopServer() {
if (serverProcess) {
try {
serverProcess.kill('SIGTERM');
} catch (e) {
console.warn('Error stopping server:', e.message);
}
serverProcess = null; // ← reference lost immediately
}
}kill('SIGTERM') returns instantly — it only sends the signal. If Nitro takes time to exit (or ignores SIGTERM), the reference is gone and you can no longer observe the exit or escalate to SIGKILL, risking an orphaned process.
Here is the recommended fix — wait for 'exit', and force-kill after a timeout:
function stopServer() {
if (!serverProcess) return;
const proc = serverProcess;
serverProcess = null; // prevent re-entry
let cleanedUp = false;
const cleanup = () => {
if (cleanedUp) return;
cleanedUp = true;
proc.removeAllListeners('exit');
proc.removeAllListeners('error');
clearTimeout(forceKillTimer);
};
const forceKillTimer = setTimeout(() => {
if (!cleanedUp) {
console.warn('Server did not exit in time, sending SIGKILL');
try {
proc.kill('SIGKILL');
} catch (e) {
console.warn('Error force-killing server:', e.message);
}
cleanup();
}
}, 5000);
proc.once('exit', () => {
console.log('Server process exited cleanly');
cleanup();
});
proc.once('error', (e) => {
console.warn('Error from server process during shutdown:', e.message);
cleanup();
});
try {
proc.kill('SIGTERM');
} catch (e) {
console.warn('Error sending SIGTERM:', e.message);
cleanup();
}
}Key points:
serverProcessis nulled at entry to prevent re-entrant calls (idempotency preserved).- A local
procholds the reference so it can be used in listeners and the force-kill timer. - If the process doesn't exit within 5 seconds,
SIGKILLis sent. - All listeners and the timer are cleaned up in both normal and forced exit paths.
| serverProcess = spawn('node', [serverPath], { | ||
| stdio: 'ignore', // no terminal | ||
| windowsHide: true, // hide CMD | ||
| stdio: 'ignore', | ||
| windowsHide: true, | ||
| env: { | ||
| ...process.env, | ||
| HOST: serverHost, | ||
| PORT: serverPort.toString(), | ||
| }, |
There was a problem hiding this comment.
Use a derived connect URL instead of hardcoded localhost.
This startup path binds Nitro to serverHost, but the new probe still hits http://localhost:${serverPort}. Any config that binds Nitro to a specific non-loopback interface or hostname will look "down" here and trip the retry limit even though the server is healthy. Build a shared server URL from a connect host (127.0.0.1/[::1] for wildcard binds, otherwise serverHost) and reuse it here and in createWindow().
Suggested fix
+function getServerUrl() {
+ const connectHost =
+ serverHost === '0.0.0.0'
+ ? '127.0.0.1'
+ : serverHost === '::'
+ ? '[::1]'
+ : serverHost;
+
+ return `http://${connectHost}:${serverPort}`;
+}
+
...
- waitForServer(`http://localhost:${serverPort}`)
+ waitForServer(getServerUrl())Update mainWindow.loadURL(...) to reuse getServerUrl() as well.
Also applies to: 130-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/main.cjs` around lines 97 - 104, The probe is using a hardcoded
localhost URL while Nitro is bound to serverHost, causing false "down"
detections; create a single helper (e.g., getServerUrl(serverHost, serverPort))
that derives the connect host (use 127.0.0.1 or [::1] when serverHost is a
wildcard like 0.0.0.0/::, otherwise use serverHost) and returns the full URL,
then replace the probe URL and the mainWindow.loadURL(...) call to use
getServerUrl() and ensure spawn still sets env HOST and PORT from
serverHost/serverPort.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
electron/main.cjs (2)
31-39:⚠️ Potential issue | 🟠 MajorReference cleared before confirming child exit—process may be orphaned.
kill('SIGTERM')sends the signal and returns immediately without waiting for the child to exit. SettingserverProcess = nullright after loses the handle needed to observe the'exit'event or escalate toSIGKILLif Nitro doesn't terminate promptly.Consider keeping the reference until exit is confirmed or a timeout elapses:
Suggested improvement
+const KILL_TIMEOUT_MS = 5000; + function stopServer() { - if (serverProcess) { + if (!serverProcess) return; + + const proc = serverProcess; + serverProcess = null; // Prevent re-entry + + const forceKill = setTimeout(() => { try { - serverProcess.kill('SIGTERM'); + proc.kill('SIGKILL'); + console.warn('Force-killed server after timeout'); } catch (e) { - console.warn('Error stopping server:', e.message); + // Already dead or inaccessible } - serverProcess = null; - } + }, KILL_TIMEOUT_MS); + + proc.once('exit', () => clearTimeout(forceKill)); + + try { + proc.kill('SIGTERM'); + } catch (e) { + clearTimeout(forceKill); + console.warn('Error stopping server:', e.message); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/main.cjs` around lines 31 - 39, stopServer currently sends SIGTERM to serverProcess and immediately clears serverProcess, which can orphan the child; update stopServer to retain the serverProcess reference until the child emits the 'exit' event or a configurable timeout elapses, attach an 'exit' listener to serverProcess that clears the reference and optionally escalates to kill('SIGKILL') if the timeout passes, and ensure any errors from kill are handled in the existing try/catch; reference functions/variables: stopServer, serverProcess, the 'exit' event, SIGTERM, SIGKILL, and the timeout/escalation logic.
130-137:⚠️ Potential issue | 🟠 MajorHardcoded
localhostmay not match the configured server host.The probe uses
http://localhost:${serverPort}, but the server binds toserverHost. IfserverHostis a specific non-loopback interface (e.g.,192.168.1.10), the probe will never reach the server and exhaust retries even though Nitro is healthy.Derive the connect URL based on the bind address:
Suggested fix
+function getConnectUrl() { + // Map wildcard binds to loopback for probing + const connectHost = + serverHost === '0.0.0.0' ? '127.0.0.1' : + serverHost === '::' ? '[::1]' : + serverHost; + return `http://${connectHost}:${serverPort}`; +} + ... - waitForServer(`http://localhost:${serverPort}`) + waitForServer(getConnectUrl())Also update
mainWindow.loadURL()at Line 151 to use the same helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/main.cjs` around lines 130 - 137, The probe currently calls waitForServer(`http://localhost:${serverPort}`) which fails when the server binds to serverHost; change the code to derive the connect URL from serverHost and serverPort (e.g., use a helper getConnectUrl(serverHost, serverPort) that maps '0.0.0.0'/'::' to 'localhost' but otherwise uses the actual bind address) and pass that URL into waitForServer instead of the hardcoded localhost; also update the mainWindow.loadURL(...) call that currently uses localhost to use the same getConnectUrl(serverHost, serverPort) helper so both probe and window target the correct host.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/main.cjs`:
- Around line 47-65: The check() function can double-trigger retry() after
resolve() if the timeout fires near completion; add a local settled flag (e.g.,
let settled = false) in check(), set settled = true immediately before calling
resolve(), and early-return from the timeout callback and the retry handler if
settled is true; also remove or ignore the 'error' listener (e.g.,
removeListener('error', retry) or check settled inside retry) and clear the
request timeout (req.setTimeout(0) or clear stored timer) when settling to
prevent spurious retry logs and extra checks.
---
Duplicate comments:
In `@electron/main.cjs`:
- Around line 31-39: stopServer currently sends SIGTERM to serverProcess and
immediately clears serverProcess, which can orphan the child; update stopServer
to retain the serverProcess reference until the child emits the 'exit' event or
a configurable timeout elapses, attach an 'exit' listener to serverProcess that
clears the reference and optionally escalates to kill('SIGKILL') if the timeout
passes, and ensure any errors from kill are handled in the existing try/catch;
reference functions/variables: stopServer, serverProcess, the 'exit' event,
SIGTERM, SIGKILL, and the timeout/escalation logic.
- Around line 130-137: The probe currently calls
waitForServer(`http://localhost:${serverPort}`) which fails when the server
binds to serverHost; change the code to derive the connect URL from serverHost
and serverPort (e.g., use a helper getConnectUrl(serverHost, serverPort) that
maps '0.0.0.0'/'::' to 'localhost' but otherwise uses the actual bind address)
and pass that URL into waitForServer instead of the hardcoded localhost; also
update the mainWindow.loadURL(...) call that currently uses localhost to use the
same getConnectUrl(serverHost, serverPort) helper so both probe and window
target the correct host.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 55a9c298-1191-4c29-9f0b-b868fa20a64e
📒 Files selected for processing (1)
electron/main.cjs
| const check = () => { | ||
| http | ||
| .get(url, () => resolve()) | ||
| .on('error', () => setTimeout(check, 500)); | ||
| const req = http | ||
| .get(url, (res) => { | ||
| res.resume(); | ||
|
|
||
| if (res.statusCode === 200) { | ||
| console.log('Server is ready'); | ||
| resolve(); | ||
| } else { | ||
| retry(); | ||
| } | ||
| }) | ||
| .on('error', retry); | ||
|
|
||
| // Add timeout to prevent hanging | ||
| req.setTimeout(delay, () => { | ||
| req.destroy(new Error('Request timeout')); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Guard against double settlement when request completes near timeout boundary.
If the response arrives just before the timeout fires, resolve() is called, but the timeout callback can still execute req.destroy(), which emits an 'error' event and invokes retry(). This won't break the Promise (already resolved), but it logs spurious retry messages and schedules unnecessary checks.
Suggested fix
const check = () => {
+ let done = false;
const req = http
.get(url, (res) => {
res.resume();
if (res.statusCode === 200) {
console.log('Server is ready');
+ done = true;
resolve();
} else {
retry();
}
})
- .on('error', retry);
+ .on('error', () => {
+ if (!done) retry();
+ });
// Add timeout to prevent hanging
req.setTimeout(delay, () => {
- req.destroy(new Error('Request timeout'));
+ if (!done) req.destroy(new Error('Request timeout'));
});
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/main.cjs` around lines 47 - 65, The check() function can
double-trigger retry() after resolve() if the timeout fires near completion; add
a local settled flag (e.g., let settled = false) in check(), set settled = true
immediately before calling resolve(), and early-return from the timeout callback
and the retry handler if settled is true; also remove or ignore the 'error'
listener (e.g., removeListener('error', retry) or check settled inside retry)
and clear the request timeout (req.setTimeout(0) or clear stored timer) when
settling to prevent spurious retry logs and extra checks.
1 participant
Addressed Issues:
Fixes #312
Description
This PR fixes an issue where the Electron app could hang indefinitely if the Nitro server failed to start.
Previously, the
waitForServerfunction retried indefinitely without any limit. If the Nitro server crashed or failed during startup, Electron would remain stuck waiting forever.This PR introduces:
waitForServerto prevent infinite waitingexiteventThese changes ensure the app fails fast and exits cleanly instead of hanging.
Screenshots/Recordings:
N/A (no UI changes)
Functional Verification
Screen Mirror
Authentication
Basic Gestures
Modes & Settings
Advanced Input
Any other gesture or input behavior introduced:
Additional Notes:
Checklist
This PR was created with the assistance of AI tools. All changes have been reviewed, tested, and verified to ensure correctness and alignment with project requirements.
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
Summary by CodeRabbit