Skip to content

Fix gestures #321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Shashank-Mengani wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fix gestures #321

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
161bd29
refactor: centralize gesture state reset logic in useTrackpadGesture …
Shashank-Mengani Mar 11, 2026
50df3da
fix: preserve draggingTimeout in resetGestureState to the prevent bro…
Shashank-Mengani Mar 11, 2026
3823d2d
fix: prevent Electron hang when Nitro server fails to start
Shashank-Mengani Mar 19, 2026
9e4e778
fix(electron): address CodeRabbit review comments
Shashank-Mengani Mar 19, 2026
b7d2dbb
fixed electron suggested by coderabbitai
Shashank-Mengani Mar 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 105 additions & 18 deletions electron/main.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,21 +27,63 @@ if (!gotLock) {
process.exit(0);
}

// Centralized server cleanup (idempotent)
function stopServer() {
if (serverProcess) {
try {
serverProcess.kill('SIGTERM');
} catch (e) {
console.warn('Error stopping server:', e.message);
}
serverProcess = null;
}
Comment on lines +31 to +39
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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:

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.

Copy link
Author

@Shashank-Mengani Shashank-Mengani Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed review!

I've addressed all the reported issues:

  • Added a timeout to the HTTP readiness probe in waitForServer to 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.kill calls across shutdown, 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

Copy link
Contributor

@coderabbitai coderabbitai bot Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

  • serverProcess is nulled at entry to prevent re-entrant calls (idempotency preserved).
  • A local proc holds the reference so it can be used in listeners and the force-kill timer.
  • If the process doesn't exit within 5 seconds, SIGKILL is sent.
  • All listeners and the timer are cleaned up in both normal and forced exit paths.

}

// Wait until server is ready
function waitForServer(url) {
return new Promise((resolve) => {
function waitForServer(url, maxRetries = 20, delay = 500) {
return new Promise((resolve, reject) => {
let retries = 0;

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'));
});
};
Comment on lines 47 to 65
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.


const retry = () => {
retries++;
console.log(`Waiting for server... (${retries}/${maxRetries})`);

if (retries >= maxRetries) {
return reject(
new Error(`Server failed to start after ${maxRetries} attempts`)
);
}

setTimeout(check, delay);
};

check();
});
}

// Start Nitro server (production)
// Start Nitro server
function startServer() {
return new Promise((resolve) => {
return new Promise((resolve, reject) => {
const serverPath = path.join(
process.resourcesPath,
'app.asar.unpacked',
Expand All @@ -50,19 +92,49 @@ function startServer() {
'index.mjs'
);

console.log("Starting server from:", serverPath);
console.log('Starting server from:', serverPath);

serverProcess = spawn('node', [serverPath], {
stdio: 'ignore', // no terminal
windowsHide: true, // hide CMD
stdio: 'ignore',
windowsHide: true,
env: {
...process.env,
HOST: serverHost,
PORT: serverPort.toString(),
},
Comment on lines 97 to 104
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

});

waitForServer(`http://localhost:${serverPort}`).then(resolve);
// Robust startup handling
let settled = false;

const fail = (err) => {
if (!settled) {
settled = true;
stopServer();
reject(err);
}
};

// Handle spawn errors
serverProcess.once('error', (err) => {
fail(new Error(`Failed to start Nitro server: ${err.message}`));
});

// Handle early exit
serverProcess.once('exit', (code) => {
if (!settled) {
fail(new Error(`Nitro server exited early with code ${code}`));
}
});

waitForServer(`http://localhost:${serverPort}`)
.then(() => {
if (!settled) {
settled = true;
resolve();
}
})
.catch(fail);
});
}

Expand All @@ -78,25 +150,40 @@ function createWindow() {

mainWindow.loadURL(`http://localhost:${serverPort}`);

// Show when ready
mainWindow.once('ready-to-show', () => {
mainWindow.show();
});

// Debug only if needed
mainWindow.webContents.on('did-fail-load', (e, code, desc) => {
console.log("LOAD FAILED:", code, desc);
console.log('LOAD FAILED:', code, desc);
});
}

// Graceful shutdown
function shutdown() {
console.log('Shutting down...');
stopServer();
app.quit();
}

process.on('SIGINT', shutdown);
process.on('SIGTERM', shutdown);

// App start
app.whenReady().then(async () => {
await startServer();
createWindow();
try {
await startServer();
createWindow();
} catch (err) {
console.error('Failed to start server:', err.message);
stopServer();
app.quit();
}
});

// Cleanup
// Cleanup on window close
app.on('window-all-closed', () => {
if (serverProcess) serverProcess.kill();
stopServer();

if (process.platform !== 'darwin') app.quit();
});
Loading
Loading