fix: support electron development window#356
Open
vinayak302004 wants to merge 1 commit into
Open
Conversation
Contributor
WalkthroughThis PR refactors Electron's main process to enable reliable development builds by distinguishing dev and production server startup paths. It removes configuration file loading, introduces environment-driven routing via ChangesElectron dev/prod server flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@electron/main.cjs`:
- Around line 26-48: startProductionServer currently spawns the server but has
no error/exit handling or cleanup, so add robust spawn failure monitoring: when
calling spawn in startProductionServer attach 'error' and 'exit' listeners on
serverProcess to log the error/exit code and reject/throw so the caller can
handle it, and ensure you kill/cleanup the process on failure; also wrap the
spawn+waitForServer sequence so if waitForServer times out or rejects you
terminate serverProcess and propagate the error. Reference:
startProductionServer, serverProcess, spawn, and waitForServer.
- Around line 59-62: The current BrowserWindow webPreferences dangerously enable
nodeIntegration and disable contextIsolation (webPreferences: { nodeIntegration:
true, contextIsolation: false }), creating a critical security risk; change
these to safe defaults (nodeIntegration: false, contextIsolation: true), add a
preload script (pass preload path in webPreferences.preload) and move any
required Node APIs into a limited contextBridge API inside that preload so
renderer access is explicit and minimal; update any renderer code that relied on
global Node APIs to use the new exposed preload methods instead.
- Around line 13-23: The waitForServer function currently polls forever; update
waitForServer to accept an optional timeoutMs (default e.g. 30_000) and return a
Promise that rejects with a clear Error if the server hasn't responded within
that timeout. Implement a timer (setTimeout) that rejects after timeoutMs and
ensure you clear that timer and any pending retries when http.get succeeds
(resolve) or when rejecting; keep the existing retry logic (check and
setTimeout(check, 500)) but stop retries on reject/resolve. Reference
waitForServer and the inner http.get/.on('error') handler so you replace the
infinite polling with a timeout-aware implementation that provides a descriptive
error (including the URL and timeout) when it fails.
- Around line 67-85: The catch block for the window load sequence only logs
errors to console, leaving users with a blank window; update the error handling
in the try/catch around waitForServer/startProductionServer and
mainWindow.loadURL to show a user-facing error dialog and/or load a local error
HTML page: use Electron's dialog (e.g., dialog.showErrorBox or
dialog.showMessageBox) to display a clear message containing the error and a
friendly title, and as a fallback call mainWindow.loadFile or mainWindow.loadURL
to display an embedded error page; reference the existing symbols mainWindow,
waitForServer, startProductionServer, DEV_URL and mainWindow.loadURL when
implementing the dialog and fallback load.
- Line 65: The call to mainWindow.webContents.openDevTools() should only run in
development; update the code around where mainWindow is created so that
openDevTools() is invoked conditionally (e.g., guard it with a development check
such as process.env.NODE_ENV === 'development' or an isDev/isElectronDev flag or
using electron-is-dev) and ensure no DevTools are opened when running production
builds; locate the call to mainWindow.webContents.openDevTools() and wrap it in
that dev-only conditional (or replace it with a helper like isDev &&
mainWindow.webContents.openDevTools()).
- Around line 40-44: Replace the hardcoded env values HOST and PORT in the
spawned process env (the object that currently spreads ...process.env and sets
HOST: '0.0.0.0' and PORT: '3000') so they are read from the shared server
configuration file (src/server-config.json) instead; load/require the JSON (or
import) at top of the module, pull host and port values (e.g., config.host and
config.port) and use those to set the env.HOST and env.PORT before spawning the
production server, and apply the same replacement for the other occurrence
referenced near line 79 so both spawn sites use the single source of truth
config.
- Around line 27-33: The current construction of serverPath always assumes an
ASAR-unpacked layout (serverPath built from process.resourcesPath +
'app.asar.unpacked' + '.output/server/index.mjs'); update it to check for the
presence of the ASAR-unpacked path and fall back to the non-ASAR layout (e.g.,
process.resourcesPath + '.output/server/index.mjs') when app.asar.unpacked is
absent or the file doesn't exist. Modify the code that sets serverPath
(referencing serverPath and process.resourcesPath and the 'app.asar.unpacked'
segment) to attempt the unpacked path first and then a fallback path, using a
synchronous existence check or try/catch to select the correct path at runtime.
In `@package.json`:
- Line 13: The electron-dev npm script hardcodes port 3000 which couples it to
src/server-config.json; update the script to read the frontend port dynamically
(or at minimum document the dependency). Replace the current "electron-dev"
entry with a tiny Node helper (e.g., scripts/run-electron-dev.js) that requires
../src/server-config.json, builds the URL from config.frontendPort, and spawns
the concurrently command using that URL (or add a clear comment by the
electron-dev script referencing src/server-config.json frontendPort and noting
the required port if you prefer the simpler change). Ensure the helper is
referenced from package.json as "electron-dev": "node
scripts/run-electron-dev.js" and that the helper uses the built URL for both
wait-on and VITE_DEV_SERVER_URL.
🪄 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: 2ad2e5dc-f80d-4757-8723-5d06bac6b6a2
📒 Files selected for processing (2)
electron/main.cjspackage.json
Comment on lines
13
to
23
| function waitForServer(url) { | ||
| return new Promise((resolve) => { | ||
| const check = () => { | ||
| http | ||
| .get(url, () => resolve()) | ||
| .on('error', () => setTimeout(check, 500)); | ||
| }; | ||
|
|
||
| check(); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Add timeout to prevent indefinite hang.
The waitForServer function polls indefinitely without a timeout. If the server fails to start (e.g., port already in use, server crash, misconfiguration), the Electron app will hang forever without feedback to the user.
⏱️ Add configurable timeout with clear error
-function waitForServer(url) {
+function waitForServer(url, timeoutMs = 30000) {
return new Promise((resolve, reject) => {
+ const startTime = Date.now();
const check = () => {
+ if (Date.now() - startTime > timeoutMs) {
+ return reject(new Error(`Server at ${url} did not start within ${timeoutMs}ms`));
+ }
http
.get(url, () => resolve())
.on('error', () => setTimeout(check, 500));
};
check();
});
}🤖 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 `@electron/main.cjs` around lines 13 - 23, The waitForServer function currently
polls forever; update waitForServer to accept an optional timeoutMs (default
e.g. 30_000) and return a Promise that rejects with a clear Error if the server
hasn't responded within that timeout. Implement a timer (setTimeout) that
rejects after timeoutMs and ensure you clear that timer and any pending retries
when http.get succeeds (resolve) or when rejecting; keep the existing retry
logic (check and setTimeout(check, 500)) but stop retries on reject/resolve.
Reference waitForServer and the inner http.get/.on('error') handler so you
replace the infinite polling with a timeout-aware implementation that provides a
descriptive error (including the URL and timeout) when it fails.
Comment on lines
+26
to
+48
| async function startProductionServer() { | ||
| const serverPath = path.join( | ||
| process.resourcesPath, | ||
| 'app.asar.unpacked', | ||
| '.output', | ||
| 'server', | ||
| 'index.mjs' | ||
| ); | ||
|
|
||
| console.log('Starting production server:', serverPath); | ||
|
|
||
| serverProcess = spawn('node', [serverPath], { | ||
| stdio: 'inherit', | ||
| windowsHide: true, | ||
| env: { | ||
| ...process.env, | ||
| HOST: '0.0.0.0', | ||
| PORT: '3000', | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| // Create window | ||
| function createWindow() { | ||
| if (mainWindow) return; | ||
| await waitForServer('http://localhost:3000'); | ||
| } |
Contributor
There was a problem hiding this comment.
Add error handling for production server spawn failures.
The startProductionServer function spawns the Node.js server process but does not handle spawn errors or server crashes. If the server executable is missing, Node.js is unavailable, or the server exits immediately due to an error, the function will either hang (waiting for a server that never starts) or silently fail, leaving the user with a non-functional app.
🛡️ Add spawn error handling and process monitoring
async function startProductionServer() {
const serverPath = path.join(
process.resourcesPath,
'app.asar.unpacked',
'.output',
'server',
'index.mjs'
);
console.log('Starting production server:', serverPath);
serverProcess = spawn('node', [serverPath], {
stdio: 'inherit',
windowsHide: true,
env: {
...process.env,
HOST: '0.0.0.0',
PORT: '3000',
},
});
+
+ serverProcess.on('error', (err) => {
+ console.error('Failed to start server process:', err);
+ throw err;
+ });
+
+ serverProcess.on('exit', (code, signal) => {
+ if (code !== null && code !== 0) {
+ console.error(`Server process exited with code ${code}`);
+ }
+ if (signal) {
+ console.error(`Server process killed by signal ${signal}`);
+ }
+ });
await waitForServer('http://localhost:3000');
}📝 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.
Suggested change
| async function startProductionServer() { | |
| const serverPath = path.join( | |
| process.resourcesPath, | |
| 'app.asar.unpacked', | |
| '.output', | |
| 'server', | |
| 'index.mjs' | |
| ); | |
| console.log('Starting production server:', serverPath); | |
| serverProcess = spawn('node', [serverPath], { | |
| stdio: 'inherit', | |
| windowsHide: true, | |
| env: { | |
| ...process.env, | |
| HOST: '0.0.0.0', | |
| PORT: '3000', | |
| }, | |
| }); | |
| } | |
| // Create window | |
| function createWindow() { | |
| if (mainWindow) return; | |
| await waitForServer('http://localhost:3000'); | |
| } | |
| async function startProductionServer() { | |
| const serverPath = path.join( | |
| process.resourcesPath, | |
| 'app.asar.unpacked', | |
| '.output', | |
| 'server', | |
| 'index.mjs' | |
| ); | |
| console.log('Starting production server:', serverPath); | |
| serverProcess = spawn('node', [serverPath], { | |
| stdio: 'inherit', | |
| windowsHide: true, | |
| env: { | |
| ...process.env, | |
| HOST: '0.0.0.0', | |
| PORT: '3000', | |
| }, | |
| }); | |
| serverProcess.on('error', (err) => { | |
| console.error('Failed to start server process:', err); | |
| throw err; | |
| }); | |
| serverProcess.on('exit', (code, signal) => { | |
| if (code !== null && code !== 0) { | |
| console.error(`Server process exited with code ${code}`); | |
| } | |
| if (signal) { | |
| console.error(`Server process killed by signal ${signal}`); | |
| } | |
| }); | |
| await waitForServer('http://localhost:3000'); | |
| } |
🤖 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 `@electron/main.cjs` around lines 26 - 48, startProductionServer currently
spawns the server but has no error/exit handling or cleanup, so add robust spawn
failure monitoring: when calling spawn in startProductionServer attach 'error'
and 'exit' listeners on serverProcess to log the error/exit code and
reject/throw so the caller can handle it, and ensure you kill/cleanup the
process on failure; also wrap the spawn+waitForServer sequence so if
waitForServer times out or rejects you terminate serverProcess and propagate the
error. Reference: startProductionServer, serverProcess, spawn, and
waitForServer.
Comment on lines
+27
to
+33
| const serverPath = path.join( | ||
| process.resourcesPath, | ||
| 'app.asar.unpacked', | ||
| '.output', | ||
| 'server', | ||
| 'index.mjs' | ||
| ); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that electron-builder config correctly unpacks .output for the production server path
# Check that .output is in asarUnpack array
jq -e '.build.asarUnpack | any(. == ".output/**/*")' package.json
if [ $? -eq 0 ]; then
echo "✓ .output is configured for ASAR unpacking"
else
echo "✗ WARNING: .output is not in asarUnpack - production server path will fail"
fi
# Also check extraResources
jq -e '.build.extraResources | any(.from == ".output")' package.json
if [ $? -eq 0 ]; then
echo "✓ .output is in extraResources"
else
echo "✗ .output is not in extraResources"
fiRepository: AOSSIE-Org/Rein
Length of output: 143
Server path matches current electron-builder ASAR-unpack config for .output
package.json configures build.asarUnpack to include .output/**/* and also ships .output via build.extraResources, which makes process.resourcesPath/app.asar.unpacked/.output/server/index.mjs in electron/main.cjs consistent with the current packaging setup.
Optional: add a fallback/guard for non-ASAR or layout changes (e.g., process.resourcesPath/.output/... when app.asar.unpacked is absent).
🤖 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 `@electron/main.cjs` around lines 27 - 33, The current construction of
serverPath always assumes an ASAR-unpacked layout (serverPath built from
process.resourcesPath + 'app.asar.unpacked' + '.output/server/index.mjs');
update it to check for the presence of the ASAR-unpacked path and fall back to
the non-ASAR layout (e.g., process.resourcesPath + '.output/server/index.mjs')
when app.asar.unpacked is absent or the file doesn't exist. Modify the code that
sets serverPath (referencing serverPath and process.resourcesPath and the
'app.asar.unpacked' segment) to attempt the unpacked path first and then a
fallback path, using a synchronous existence check or try/catch to select the
correct path at runtime.
Comment on lines
+40
to
+44
| env: { | ||
| ...process.env, | ||
| HOST: '0.0.0.0', | ||
| PORT: '3000', | ||
| }, |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Hardcoded HOST and PORT create maintenance risk.
The production server spawn hardcodes HOST: '0.0.0.0' and PORT: '3000', while the actual server configuration is defined in src/server-config.json. This duplication creates a maintenance risk: if the port or host in server-config.json is changed, the Electron app will attempt to connect to the wrong endpoint and fail to load.
♻️ Load configuration from server-config.json
+const serverConfig = require('../src/server-config.json');
+
async function startProductionServer() {
const serverPath = path.join(
process.resourcesPath,
'app.asar.unpacked',
'.output',
'server',
'index.mjs'
);
console.log('Starting production server:', serverPath);
serverProcess = spawn('node', [serverPath], {
stdio: 'inherit',
windowsHide: true,
env: {
...process.env,
- HOST: '0.0.0.0',
- PORT: '3000',
+ HOST: serverConfig.host,
+ PORT: String(serverConfig.frontendPort),
},
});
- await waitForServer('http://localhost:3000');
+ await waitForServer(`http://localhost:${serverConfig.frontendPort}`);
}Then update line 79 as well:
- await mainWindow.loadURL("http://localhost:3000");
+ await mainWindow.loadURL(`http://localhost:${serverConfig.frontendPort}`);📝 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.
Suggested change
| env: { | |
| ...process.env, | |
| HOST: '0.0.0.0', | |
| PORT: '3000', | |
| }, | |
| const serverConfig = require('../src/server-config.json'); | |
| async function startProductionServer() { | |
| const serverPath = path.join( | |
| process.resourcesPath, | |
| 'app.asar.unpacked', | |
| '.output', | |
| 'server', | |
| 'index.mjs' | |
| ); | |
| console.log('Starting production server:', serverPath); | |
| serverProcess = spawn('node', [serverPath], { | |
| stdio: 'inherit', | |
| windowsHide: true, | |
| env: { | |
| ...process.env, | |
| HOST: serverConfig.host, | |
| PORT: String(serverConfig.frontendPort), | |
| }, | |
| }); | |
| await waitForServer(`http://localhost:${serverConfig.frontendPort}`); | |
| } |
Suggested change
| env: { | |
| ...process.env, | |
| HOST: '0.0.0.0', | |
| PORT: '3000', | |
| }, | |
| await mainWindow.loadURL(`http://localhost:${serverConfig.frontendPort}`); |
🤖 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 `@electron/main.cjs` around lines 40 - 44, Replace the hardcoded env values
HOST and PORT in the spawned process env (the object that currently spreads
...process.env and sets HOST: '0.0.0.0' and PORT: '3000') so they are read from
the shared server configuration file (src/server-config.json) instead;
load/require the JSON (or import) at top of the module, pull host and port
values (e.g., config.host and config.port) and use those to set the env.HOST and
env.PORT before spawning the production server, and apply the same replacement
for the other occurrence referenced near line 79 so both spawn sites use the
single source of truth config.
Comment on lines
+59
to
+62
| webPreferences: { | ||
| nodeIntegration: true, | ||
| contextIsolation: false, | ||
| }, |
Contributor
There was a problem hiding this comment.
Critical security risk: nodeIntegration enabled with contextIsolation disabled.
Enabling nodeIntegration: true and disabling contextIsolation: false allows the renderer process (which loads web content) to directly access Node.js APIs and Electron internals. This creates a severe security vulnerability: any XSS attack or malicious code execution in the web content can gain full system access. If the app loads any external content, user-generated content, or has any XSS vulnerability, an attacker can execute arbitrary code with full Node.js privileges.
Recommended mitigation:
- Set
nodeIntegration: falseandcontextIsolation: true(secure defaults) - If you need Node.js functionality in the renderer, use a preload script with
contextBridgeto expose only the specific APIs needed - Review Electron's security guidelines: https://www.electronjs.org/docs/latest/tutorial/security
🔒 Secure webPreferences configuration
webPreferences: {
- nodeIntegration: true,
- contextIsolation: false,
+ nodeIntegration: false,
+ contextIsolation: true,
+ preload: path.join(__dirname, 'preload.cjs'), // Create preload script if needed
},🤖 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 `@electron/main.cjs` around lines 59 - 62, The current BrowserWindow
webPreferences dangerously enable nodeIntegration and disable contextIsolation
(webPreferences: { nodeIntegration: true, contextIsolation: false }), creating a
critical security risk; change these to safe defaults (nodeIntegration: false,
contextIsolation: true), add a preload script (pass preload path in
webPreferences.preload) and move any required Node APIs into a limited
contextBridge API inside that preload so renderer access is explicit and
minimal; update any renderer code that relied on global Node APIs to use the new
exposed preload methods instead.
| // Debug only if needed | ||
| mainWindow.webContents.on('did-fail-load', (e, code, desc) => { | ||
| console.log("LOAD FAILED:", code, desc); | ||
| mainWindow.webContents.openDevTools(); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Remove automatic DevTools in production builds.
Opening DevTools automatically (mainWindow.webContents.openDevTools()) is useful for development but should not be enabled in production builds. This exposes internal application details to end users and creates a poor user experience.
🔧 Conditionally open DevTools only in dev mode
- mainWindow.webContents.openDevTools();
+ if (isDev) {
+ mainWindow.webContents.openDevTools();
+ }🤖 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 `@electron/main.cjs` at line 65, The call to
mainWindow.webContents.openDevTools() should only run in development; update the
code around where mainWindow is created so that openDevTools() is invoked
conditionally (e.g., guard it with a development check such as
process.env.NODE_ENV === 'development' or an isDev/isElectronDev flag or using
electron-is-dev) and ensure no DevTools are opened when running production
builds; locate the call to mainWindow.webContents.openDevTools() and wrap it in
that dev-only conditional (or replace it with a helper like isDev &&
mainWindow.webContents.openDevTools()).
Comment on lines
+67
to
+85
| try { | ||
| if (isDev) { | ||
| console.log("Loading DEV URL:", DEV_URL); | ||
|
|
||
| await waitForServer(DEV_URL); | ||
|
|
||
| await mainWindow.loadURL(DEV_URL); | ||
| } else { | ||
| console.log("Loading PROD URL"); | ||
|
|
||
| await startProductionServer(); | ||
|
|
||
| await mainWindow.loadURL("http://localhost:3000"); | ||
| } | ||
|
|
||
| console.log("WINDOW LOADED"); | ||
| } catch (err) { | ||
| console.error("LOAD ERROR:", err); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Improve error handling with user-facing feedback.
The current error handling logs to console but provides no feedback to the user when the window fails to load. If the server fails to start or the URL fails to load, the user sees a blank window with no indication of what went wrong.
📢 Show error dialog on load failure
+ const { dialog } = require('electron');
+
try {
if (isDev) {
console.log("Loading DEV URL:", DEV_URL);
await waitForServer(DEV_URL);
await mainWindow.loadURL(DEV_URL);
} else {
console.log("Loading PROD URL");
await startProductionServer();
await mainWindow.loadURL("http://localhost:3000");
}
console.log("WINDOW LOADED");
} catch (err) {
console.error("LOAD ERROR:", err);
+ dialog.showErrorBox(
+ 'Failed to Start Application',
+ `The application failed to start: ${err.message}\n\nPlease check the logs for details.`
+ );
+ app.quit();
}🤖 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 `@electron/main.cjs` around lines 67 - 85, The catch block for the window load
sequence only logs errors to console, leaving users with a blank window; update
the error handling in the try/catch around waitForServer/startProductionServer
and mainWindow.loadURL to show a user-facing error dialog and/or load a local
error HTML page: use Electron's dialog (e.g., dialog.showErrorBox or
dialog.showMessageBox) to display a clear message containing the error and a
friendly title, and as a fallback call mainWindow.loadFile or mainWindow.loadURL
to display an embedded error page; reference the existing symbols mainWindow,
waitForServer, startProductionServer, DEV_URL and mainWindow.loadURL when
implementing the dialog and fallback load.
| "build": "vite build", | ||
| "electron": "npx electron .", | ||
| "electron-dev": "concurrently \"vite\" \"wait-on http://localhost:3000 && VITE_DEV_SERVER_URL=http://localhost:3000 electron .\"", | ||
| "electron-dev": "concurrently \"vite\" \"wait-on http://localhost:3000 && cross-env VITE_DEV_SERVER_URL=http://localhost:3000 electron .\"", |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Script synchronization assumes Vite dev server URL matches hardcoded value.
The electron-dev script waits for http://localhost:3000, then sets VITE_DEV_SERVER_URL=http://localhost:3000 when launching Electron. However, if the Vite dev server is configured to run on a different port (e.g., via vite.config.ts or command-line flags), the script will wait for the wrong URL and potentially hang or connect to an incorrect server.
This is currently correct based on src/server-config.json (frontendPort: 3000), but creates a maintenance coupling between the script and the config file.
Consider reading the port from server-config.json dynamically, or at minimum, add a comment documenting the port dependency:
📝 Document port dependency
- "electron-dev": "concurrently \"vite\" \"wait-on http://localhost:3000 && cross-env VITE_DEV_SERVER_URL=http://localhost:3000 electron .\"",
+ "electron-dev": "concurrently \"vite\" \"wait-on http://localhost:3000 && cross-env VITE_DEV_SERVER_URL=http://localhost:3000 electron .\"",
+ "_comment_electron-dev": "Port 3000 must match frontendPort in src/server-config.json",Or for a more robust solution, use a small helper script to read the port:
"electron-dev": "node scripts/run-electron-dev.js"// scripts/run-electron-dev.js
const { spawn } = require('child_process');
const config = require('../src/server-config.json');
const port = config.frontendPort;
const url = `http://localhost:${port}`;
spawn('concurrently', [
'"vite"',
`"wait-on ${url} && cross-env VITE_DEV_SERVER_URL=${url} electron ."`
], { stdio: 'inherit', shell: true });📝 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.
Suggested change
| "electron-dev": "concurrently \"vite\" \"wait-on http://localhost:3000 && cross-env VITE_DEV_SERVER_URL=http://localhost:3000 electron .\"", | |
| "electron-dev": "concurrently \"vite\" \"wait-on http://localhost:3000 && cross-env VITE_DEV_SERVER_URL=http://localhost:3000 electron .\"", | |
| "_comment_electron-dev": "Port 3000 must match frontendPort in src/server-config.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 `@package.json` at line 13, The electron-dev npm script hardcodes port 3000
which couples it to src/server-config.json; update the script to read the
frontend port dynamically (or at minimum document the dependency). Replace the
current "electron-dev" entry with a tiny Node helper (e.g.,
scripts/run-electron-dev.js) that requires ../src/server-config.json, builds the
URL from config.frontendPort, and spawns the concurrently command using that URL
(or add a clear comment by the electron-dev script referencing
src/server-config.json frontendPort and noting the required port if you prefer
the simpler change). Ensure the helper is referenced from package.json as
"electron-dev": "node scripts/run-electron-dev.js" and that the helper uses the
built URL for both wait-on and VITE_DEV_SERVER_URL.
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
1 participant
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.
Addressed Issues:
Fixes #284
Description
Screenshots/Recordings:
Functional Verification
Use "npm run electron-dev" for development
Use "npm run build" and "npm run dist" for production
Screen Mirror
Authentication
Basic Gestures
One-finger tap: Verified as Left Click.
Two-finger tap: Verified as Right Click.
Click and drag: Verified selection behavior.
Pinch to zoom: Verified zoom functionality (if applicable).
Modes & Settings
Cursor mode: Cursor moves smoothly and accurately.
Scroll mode: Page scrolls as expected.
Sensitivity: Verified changes in cursor speed/sensitivity settings.
Copy and Paste: Verified both Copy and Paste functionality.
Invert Scrolling: Verified scroll direction toggles correctly.
Advanced Input
Key combinations: Verified "hold" behavior for modifiers (e.g., Ctrl+C) and held keys are shown in buffer.
Keyboard input: Verified Space, Backspace, and Enter keys work correctly.
Glide typing: Verified path drawing and text output.
Voice input: Verified speech-to-text functionality for full sentences.
Backspace doesn't send the previous input.
Any other gesture or input behavior introduced:
Additional Notes:
Checklist
[ X ] My PR addresses a single issue, fixes a single bug or makes a single improvement.
[ X ] My code follows the project's code style and conventions
[ X ] I have performed a self-review of my own code
[ X ] I have commented my code, particularly in hard-to-understand areas
If applicable, I have made corresponding changes or additions to the documentation
If applicable, I have made corresponding changes or additions to tests
[ X ] My changes generate no new warnings or errors
[ X ] I have joined the and I will share a link to this PR with the project maintainers there
[ X ] I have read the
[ X ] Once I submit my PR, CodeRabbit AI will automatically review it and I will address CodeRabbit's comments.
[ X ] Incase of UI change I've added a demo video.
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