feat: add cross-platform dev script (Windows/macOS/Linux support)#162
feat: add cross-platform dev script (Windows/macOS/Linux support)#162webdevcody merged 1 commit intoAutoMaker-Org:mainfrom
Conversation
Replace Unix-only init.sh with cross-platform init.mjs Node.js script. Changes: - Add init.mjs: Cross-platform Node.js implementation of init.sh - Update package.json: Change dev script from ./init.sh to node init.mjs - Add tree-kill dependency for reliable cross-platform process termination Key features of init.mjs: - Cross-platform port detection (netstat on Windows, lsof on Unix) - Cross-platform process killing using tree-kill package - Uses cross-spawn for reliable npm/npx command execution on Windows - Interactive prompts via Node.js readline module - Colored terminal output (works on modern Windows terminals) - Proper cleanup handlers for Ctrl+C/SIGTERM Bug fix: - Fixed Playwright browser check to run from apps/app directory where @playwright/test is actually installed (was silently failing before) The original init.sh is preserved for backward compatibility.
WalkthroughA new cross-platform Node.js bootstrapper (init.mjs) replaces the shell script. It manages process lifecycle, checks backend health, installs dependencies, and launches either web or desktop applications based on user selection. The package.json dev script is updated to invoke this new bootstrapper, with tree-kill added as a dependency for process cleanup. Changes
Sequence DiagramsequenceDiagram
participant User
participant init as init.mjs
participant npm as npm/Node
participant Server as Backend<br/>(port 3008)
participant Frontend as Frontend<br/>(Browser)
participant Electron
User->>init: Run: npm run dev
init->>init: Detect platform, setup utilities
init->>npm: Check & install dependencies
init->>init: Kill existing processes on ports 3007, 3008
init->>User: Display mode menu (1=Web, 2=Desktop)
User->>init: Select mode (1 or 2)
alt Web Mode (Choice 1)
init->>npm: npm run dev:server
npm->>Server: Start backend on port 3008
init->>Server: Poll /api/health (with timeout)
Server-->>init: Health check passed ✓
init->>npm: npm run dev:web
npm->>Frontend: Start browser frontend
Frontend-->>User: Display in browser
Frontend->>init: Wait for termination
else Electron Mode (Choice 2)
init->>npm: npm run dev:electron
npm->>Electron: Launch desktop app
Electron-->>User: Display Electron window
Electron->>init: Wait for termination
end
User->>init: SIGINT/SIGTERM (Ctrl+C)
init->>init: Graceful shutdown
init->>Server: Kill process tree
init->>Frontend: Kill process tree
init->>init: Cleanup complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring close attention:
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @leonvanzyl, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the project's development workflow by introducing a new cross-platform Node.js initialization script ( Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a cross-platform Node.js script, init.mjs, to replace the Unix-only init.sh for setting up the development environment. This is a great improvement for Windows developers. The new script is well-structured and handles cross-platform differences for process and port management effectively.
My review includes a few suggestions to enhance the script's maintainability and robustness:
- Refactoring hardcoded values into constants.
- Improving error handling by logging in silent catch blocks.
- Simplifying logic by removing a redundant
try-catchblock. - Using a more graceful process termination strategy when clearing ports.
These changes should make the script even more reliable and easier to maintain in the future.
| } catch { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This catch block currently swallows errors silently. While the function is designed to return null on failure, it would be beneficial for debugging to log the error, especially if an unexpected issue occurs (e.g., command not found). Consider adding a log message, perhaps conditional on a debug flag.
} catch (error) {
// console.debug(`execCommand failed for '${command}':`, error);
return null;
}| /** | ||
| * Kill a process by PID (cross-platform) | ||
| */ | ||
| function killProcess(pid) { | ||
| try { | ||
| if (isWindows) { | ||
| execCommand(`taskkill /F /PID ${pid}`); | ||
| } else { | ||
| process.kill(pid, 'SIGKILL'); | ||
| } | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Following the suggested change to use killProcessTree inside killPort, this killProcess function becomes redundant and can be removed to simplify the code.
| for (const pid of pids) { | ||
| killProcess(pid); | ||
| } |
There was a problem hiding this comment.
Instead of iterating and calling killProcess (which uses SIGKILL immediately), you can leverage the existing killProcessTree function for a more graceful shutdown (SIGTERM then SIGKILL). This can be done concurrently using Promise.all.
await Promise.all(pids.map(pid => killProcessTree(pid)));| try { | ||
| await new Promise((resolve) => { | ||
| const playwright = crossSpawn( | ||
| 'npx', | ||
| ['playwright', 'install', 'chromium'], | ||
| { stdio: 'ignore', cwd: path.join(__dirname, 'apps', 'app') } | ||
| ); | ||
| playwright.on('close', () => resolve()); | ||
| playwright.on('error', () => resolve()); | ||
| }); | ||
| } catch { | ||
| // Ignore errors - Playwright install is optional | ||
| } |
There was a problem hiding this comment.
The outer try...catch block is redundant because the Promise on line 290 is constructed to never reject. The playwright.on('error', ...) event handler calls resolve(), making the catch block on line 299 unreachable. You can simplify this by removing the try...catch.
await new Promise((resolve) => {
const playwright = crossSpawn(
'npx',
['playwright', 'install', 'chromium'],
{ stdio: 'ignore', cwd: path.join(__dirname, 'apps', 'app') }
);
// Playwright install is optional, so we resolve on both close and error.
playwright.on('close', resolve);
playwright.on('error', resolve);
});| await killPort(3007); | ||
| await killPort(3008); |
There was a problem hiding this comment.
For better maintainability and readability, it's a good practice to extract hardcoded values like port numbers into constants defined at the top of the script. This makes it easier to update them in the future if they change. Other values like URLs (http://localhost:3008/api/health), retry counts (30), and timeouts (1000) throughout the script would also benefit from this refactoring.
For example, you could define at the top of the file:
const PORT_WEB = 3007;
const PORT_SERVER = 3008;And then use them here:
await killPort(PORT_WEB);
await killPort(PORT_SERVER);![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
init.mjs (3)
71-108: Windows port detection may match unintended processes.The current regex
line.match(/:\d+\s+.*?(\d+)\s*$/)extracts the PID but doesn't verify the specific port number. Usingfindstr :${port}could match port 30080 when searching for 3008.Consider a more precise match:
🔎 Suggested improvement
- const match = line.match(/:\d+\s+.*?(\d+)\s*$/); - if (match) { + // Match the exact port and extract PID + const portRegex = new RegExp(`:${port}\\s`); + if (portRegex.test(line)) { + const match = line.match(/(\d+)\s*$/); + if (match) {Alternatively, use
findstr /Rwith a regex that anchors the port number.
167-178: Health check implementation is functional but could clean up resources more explicitly.The HTTP request on the success path doesn't explicitly destroy the socket. For a polling scenario (30 retries), this could lead to lingering connections, though Node.js should handle cleanup.
🔎 Optional: Ensure socket cleanup
function checkHealth() { return new Promise((resolve) => { const req = http.get('http://localhost:3008/api/health', (res) => { + res.resume(); // Consume response to free up memory resolve(res.statusCode === 200); }); req.on('error', () => resolve(false)); req.setTimeout(2000, () => { req.destroy(); resolve(false); }); }); }
287-301: Playwright browser installation is completely silent.Using
stdio: 'ignore'suppresses all output including errors. If Playwright installation fails for a legitimate reason (e.g., disk space, network issues), users won't see why.Consider at least showing stderr:
const playwright = crossSpawn( 'npx', ['playwright', 'install', 'chromium'], - { stdio: 'ignore', cwd: path.join(__dirname, 'apps', 'app') } + { stdio: ['ignore', 'ignore', 'inherit'], cwd: path.join(__dirname, 'apps', 'app') } );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
init.mjs(1 hunks)package.json(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
init.mjs (2)
apps/app/electron/main.js (1)
command(128-128)apps/server/src/services/auto-mode-service.ts (1)
sleep(2469-2492)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (8)
package.json (2)
11-11: LGTM!The dev script update to use
node init.mjsis appropriate for cross-platform compatibility, replacing the Unix-only shell script.
34-37: Dependencies look appropriate for cross-platform process management.Both
cross-spawnandtree-killare well-established packages for the stated purpose. However, since these are added as root-leveldependenciesrather thandevDependencies, they'll be installed in production builds.Consider whether these should be
devDependenciessinceinit.mjsis a development-time script, not production code:- "dependencies": { + "devDependencies": { "cross-spawn": "^7.0.6", "tree-kill": "^1.2.2" }init.mjs (6)
1-39: LGTM!The ES module setup with
createRequirefor CJS dependencies is the correct approach. The__dirnamerecreation and color codes are appropriate for cross-platform Node.js scripting.
60-66: LGTM!The
execCommandhelper appropriately wrapsexecSyncand returnsnullon failure, which is suitable for commands likelsofornetstatthat may legitimately fail when no processes are found.
226-241: LGTM!The
killProcessTreeimplementation correctly uses SIGTERM first with a SIGKILL fallback, and the Promise always resolves which is appropriate for cleanup operations where failures shouldn't block shutdown.
246-264: LGTM!The cleanup function properly guards against undefined processes and killed states before attempting termination, and uses
Promise.allfor efficient parallel cleanup.
318-328: SIGINT/SIGTERM handlers are well-implemented with re-entry guard.The
cleaningUpflag prevents multiple cleanup attempts. Good defensive pattern for handling Ctrl+C.
331-405: The main menu loop and application launch logic looks correct.The infinite loop with break on valid choice, the server health polling with timeout, and the process await patterns are all implemented correctly for both web and electron modes.
| if (!serverReady) { | ||
| log('Error: Server failed to start', 'red'); | ||
| console.log('Check logs/server.log for details'); | ||
| cleanup(); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
Missing await on cleanup() may cause premature exit.
When the server fails to start, cleanup() is called without await, so process.exit(1) executes before cleanup completes. This could leave orphaned processes.
🔎 Apply this diff to await cleanup
if (!serverReady) {
log('Error: Server failed to start', 'red');
console.log('Check logs/server.log for details');
- cleanup();
+ await cleanup();
process.exit(1);
}📝 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.
| if (!serverReady) { | |
| log('Error: Server failed to start', 'red'); | |
| console.log('Check logs/server.log for details'); | |
| cleanup(); | |
| process.exit(1); | |
| } | |
| if (!serverReady) { | |
| log('Error: Server failed to start', 'red'); | |
| console.log('Check logs/server.log for details'); | |
| await cleanup(); | |
| process.exit(1); | |
| } |
🤖 Prompt for AI Agents
In init.mjs around lines 372 to 377, cleanup() is invoked without awaiting so
process.exit(1) can run before cleanup finishes; make the call asynchronous by
awaiting cleanup() (i.e., add await cleanup()) and ensure the enclosing function
is declared async or otherwise supports awaiting (or chain a Promise.then that
calls process.exit(1)), and preserve error logging if cleanup throws by wrapping
the await in try/catch and then call process.exit(1) after cleanup completes.
| // Run main function | ||
| main().catch((err) => { | ||
| console.error(err); | ||
| cleanup(); | ||
| process.exit(1); | ||
| }); |
There was a problem hiding this comment.
Same issue: missing await on cleanup() in error handler.
The top-level error handler also calls cleanup() without awaiting, which could leave orphaned processes on unexpected errors.
🔎 Apply this diff
main().catch((err) => {
console.error(err);
- cleanup();
+ cleanup().finally(() => process.exit(1));
- process.exit(1);
});Or use an async IIFE:
-main().catch((err) => {
- console.error(err);
- cleanup();
- process.exit(1);
-});
+main().catch(async (err) => {
+ console.error(err);
+ await cleanup();
+ process.exit(1);
+});📝 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.
| // Run main function | |
| main().catch((err) => { | |
| console.error(err); | |
| cleanup(); | |
| process.exit(1); | |
| }); | |
| // Run main function | |
| main().catch((err) => { | |
| console.error(err); | |
| cleanup().finally(() => process.exit(1)); | |
| }); |
| // Run main function | |
| main().catch((err) => { | |
| console.error(err); | |
| cleanup(); | |
| process.exit(1); | |
| }); | |
| // Run main function | |
| main().catch(async (err) => { | |
| console.error(err); | |
| await cleanup(); | |
| process.exit(1); | |
| }); |
🤖 Prompt for AI Agents
In init.mjs around lines 408 to 413, the top-level promise rejection handler
calls cleanup() without awaiting it; make the handler async and await cleanup()
(or use an async IIFE) so cleanup completes before calling process.exit(1),
ensuring any async teardown finishes; also preserve logging of the error and
ensure process.exit is invoked only after the awaited cleanup.
- Merge PR #162: cross-platform dev script (init.mjs) - Updated init.mjs to reference apps/ui instead of apps/app - Updated root package.json scripts to use apps/ui workspace 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2 participants
Replace Unix-only init.sh with cross-platform init.mjs Node.js script.
Changes:
Key features of init.mjs:
Bug fix:
The original init.sh is preserved for backward compatibility.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.