feat(dev-hot-reload): streamline hot-reload #7599
feat(dev-hot-reload): streamline hot-reload #7599sid-bruno wants to merge 1 commit intousebruno:mainfrom
Conversation
…er and package discovery
sid-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
WalkthroughThe dev hot-reload script replaces fixed concurrently-based orchestration with dynamic, dependency-aware pipeline management. It scans monorepo packages, topologically sorts internal dependencies, stages builds concurrently, waits for watch signals, and centrally manages process lifecycle through a new ProcessManager. Changes
Sequence Diagram(s)sequenceDiagram
actor Dev as Developer
participant Script as dev-hot-reload.js
participant PM as ProcessManager
participant Build as Build Processes
participant Watch as Watch Processes
participant Electron as Electron/nodemon
Dev->>Script: npm run dev
Script->>Script: Scan packages & topo-sort
par Stage 1 (Concurrent)
Script->>Build: spawn npm run build (pkg A)
Script->>Build: spawn npm run build (pkg B)
end
Build->>Build: Build completes
par Stage 2 (Concurrent, Wait for Watch)
Script->>Watch: spawn npm run watch (pkg A)
Script->>Watch: spawn npm run watch (pkg B)
Watch->>Watch: Emit "created " signal
Watch-->>Script: Signal received
end
Script->>PM: Register watch processes
PM->>PM: Listen for SIGINT/SIGTERM
par Final Stage
Script->>Electron: spawn nodemon with args
Electron->>PM: Register process
end
Dev->>Dev: Code changes
Watch->>Watch: Rebuild triggered
Electron->>Electron: Hot reload via nodemon
Dev->>Dev: Ctrl+C
PM->>Build: Terminate all processes
PM->>Watch: Terminate all processes
PM->>Electron: Terminate electron
PM->>Script: Exit cleanly
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
scripts/dev-hot-reload.js (3)
161-165:⚠️ Potential issue | 🟠 MajorUnix-only shell command — fails on Windows.
find ... -exec rm -rfdoesn't work on Windows. Use Node.jsfsAPIs or a cross-platform approach.Cross-platform alternative using Node.js
+const { rmSync } = require('fs'); + function cleanNodeModules() { log(LOG_LEVELS.INFO, 'Removing all node_modules directories...'); - execSync('find . -name "node_modules" -type d -prune -exec rm -rf {} +', { stdio: 'inherit' }); + const removeNodeModules = (dir) => { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const fullPath = path.join(dir, entry.name); + if (entry.isDirectory()) { + if (entry.name === 'node_modules') { + log(LOG_LEVELS.DEBUG, `Removing ${fullPath}`); + rmSync(fullPath, { recursive: true, force: true }); + } else if (entry.name !== '.git') { + removeNodeModules(fullPath); + } + } + } + }; + removeNodeModules(path.join(__dirname, '..')); log(LOG_LEVELS.SUCCESS, 'Node modules cleanup completed'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/dev-hot-reload.js` around lines 161 - 165, The cleanNodeModules function currently runs a Unix-only shell command via execSync ('find ... -exec rm -rf') which fails on Windows; replace that with a cross-platform implementation: either use a reliable npm utility (e.g., rimraf) or use Node's fs APIs (fs.promises.rm or fs.rmSync with recursive:true) combined with a glob or directory-walk to locate "node_modules" folders. Update cleanNodeModules to enumerate matching directories (starting from project root), remove them using the chosen cross-platform approach, and keep the existing logging calls (log(LOG_LEVELS.INFO/ SUCCESS, ...)); remove the execSync invocation and ensure errors are handled/logged appropriately.
132-139:⚠️ Potential issue | 🟠 Major
command -vis Unix-only — breaks on Windows.This shell built-in doesn't exist on Windows. The function will always return
falseon Windows, triggering unnecessary global installs.Cross-platform fix
function commandExists(command) { try { - execSync(`command -v ${command}`, { stdio: 'ignore' }); + const checkCmd = process.platform === 'win32' + ? `where ${command}` + : `command -v ${command}`; + execSync(checkCmd, { stdio: 'ignore' }); return true; } catch { return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/dev-hot-reload.js` around lines 132 - 139, The function commandExists currently uses execSync(`command -v ${command}`) which is Unix-only and returns false on Windows; update commandExists to be cross-platform by checking process.platform and using the appropriate command (use 'where' on win32, 'command -v' or 'which' on posix) or by using a Node-native approach (e.g., child_process.spawnSync/execSync with platform-aware command or a small lookup using require('child_process').spawnSync) to reliably detect the executable; adjust references to execSync in commandExists so Windows correctly reports installed commands and avoid forcing global installs.
427-428:⚠️ Potential issue | 🟡 MinorMissing
await— errors fromstartDevelopment()won't be caught.
startDevelopment()is async but not awaited, so any rejection becomes unhandled.// Start development environment - startDevelopment(); + await startDevelopment();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/dev-hot-reload.js` around lines 427 - 428, The call to startDevelopment() is not awaited so rejections become unhandled; change the invocation to await startDevelopment() from an async context (or append .catch(...)) and ensure errors are handled—either wrap the call in a try/catch inside an async function (e.g., an immediately-invoked async wrapper) or call startDevelopment().catch(err => processLogger.error(...)/process.exit(1)); update the invocation site where startDevelopment() is currently called so Promise rejections are properly awaited and logged.
🧹 Nitpick comments (1)
scripts/dev-hot-reload.js (1)
332-343: Consider adding a timeout for the "created " signal.If a watcher process fails to emit the expected signal, the promise hangs indefinitely. For a dev script this may be acceptable, but a timeout with a warning would improve debuggability.
Optional timeout implementation
return new Promise(resolve => { + const timeout = setTimeout(() => { + log(LOG_LEVELS.WARN, `${shortName} — still waiting for initial build after 60s...`); + }, 60000); const onData = data => { if (data.toString().includes('created ')) { proc.stdout.off('data', onData); proc.stderr.off('data', onData); + clearTimeout(timeout); log(LOG_LEVELS.SUCCESS, `${shortName} — initial watch build done`); resolve(); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/dev-hot-reload.js` around lines 332 - 343, The Promise that waits for the "created " message can hang forever; add a timeout (e.g., 30s) that cleans up listeners and logs a warning so the promise doesn't never resolve. Modify the code around the new Promise where onData is defined: start a timer (store a timeoutId) when attaching proc.stdout.on('data', onData) and proc.stderr.on('data', onData), clear the timer inside onData on success (before removing listeners and calling resolve), and in the timeout handler remove the same listeners, log a warning (using log and LOG_LEVELS), and resolve or reject the promise appropriately so callers won't hang; ensure you reference onData, proc.stdout/proc.stderr, shortName and LOG_LEVELS.SUCCESS when implementing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/dev-hot-reload.js`:
- Around line 75-77: The loop registering signal handlers currently includes
'SIGHUP', which is unsupported on Windows; update the registration so 'SIGHUP'
is only added on non-Windows platforms (e.g., check process.platform !== 'win32'
or use os.platform()) and keep the existing handlers for 'SIGINT' and 'SIGTERM'
calling this.shutdown(sig) — locate the for-loop that iterates over
['SIGINT','SIGTERM','SIGHUP'] and change it to conditionally include 'SIGHUP'
(or register it separately inside an if block) to avoid a no-op listener on
Windows.
---
Outside diff comments:
In `@scripts/dev-hot-reload.js`:
- Around line 161-165: The cleanNodeModules function currently runs a Unix-only
shell command via execSync ('find ... -exec rm -rf') which fails on Windows;
replace that with a cross-platform implementation: either use a reliable npm
utility (e.g., rimraf) or use Node's fs APIs (fs.promises.rm or fs.rmSync with
recursive:true) combined with a glob or directory-walk to locate "node_modules"
folders. Update cleanNodeModules to enumerate matching directories (starting
from project root), remove them using the chosen cross-platform approach, and
keep the existing logging calls (log(LOG_LEVELS.INFO/ SUCCESS, ...)); remove the
execSync invocation and ensure errors are handled/logged appropriately.
- Around line 132-139: The function commandExists currently uses
execSync(`command -v ${command}`) which is Unix-only and returns false on
Windows; update commandExists to be cross-platform by checking process.platform
and using the appropriate command (use 'where' on win32, 'command -v' or 'which'
on posix) or by using a Node-native approach (e.g.,
child_process.spawnSync/execSync with platform-aware command or a small lookup
using require('child_process').spawnSync) to reliably detect the executable;
adjust references to execSync in commandExists so Windows correctly reports
installed commands and avoid forcing global installs.
- Around line 427-428: The call to startDevelopment() is not awaited so
rejections become unhandled; change the invocation to await startDevelopment()
from an async context (or append .catch(...)) and ensure errors are
handled—either wrap the call in a try/catch inside an async function (e.g., an
immediately-invoked async wrapper) or call startDevelopment().catch(err =>
processLogger.error(...)/process.exit(1)); update the invocation site where
startDevelopment() is currently called so Promise rejections are properly
awaited and logged.
---
Nitpick comments:
In `@scripts/dev-hot-reload.js`:
- Around line 332-343: The Promise that waits for the "created " message can
hang forever; add a timeout (e.g., 30s) that cleans up listeners and logs a
warning so the promise doesn't never resolve. Modify the code around the new
Promise where onData is defined: start a timer (store a timeoutId) when
attaching proc.stdout.on('data', onData) and proc.stderr.on('data', onData),
clear the timer inside onData on success (before removing listeners and calling
resolve), and in the timeout handler remove the same listeners, log a warning
(using log and LOG_LEVELS), and resolve or reject the promise appropriately so
callers won't hang; ensure you reference onData, proc.stdout/proc.stderr,
shortName and LOG_LEVELS.SUCCESS when implementing.
🪄 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: 828e9d21-cbb1-4aea-a460-f91e6553e603
📒 Files selected for processing (1)
scripts/dev-hot-reload.js
| for (const sig of ['SIGINT', 'SIGTERM', 'SIGHUP']) { | ||
| process.on(sig, () => this.shutdown(sig)); | ||
| } |
There was a problem hiding this comment.
SIGHUP is Unix-only — ineffective on Windows.
Windows doesn't support SIGHUP. Node.js will ignore the listener or it simply won't fire. Consider conditionally registering it or removing it for a cleaner cross-platform experience.
Suggested fix
- for (const sig of ['SIGINT', 'SIGTERM', 'SIGHUP']) {
+ const signals = ['SIGINT', 'SIGTERM'];
+ if (process.platform !== 'win32') {
+ signals.push('SIGHUP');
+ }
+ for (const sig of signals) {
process.on(sig, () => this.shutdown(sig));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/dev-hot-reload.js` around lines 75 - 77, The loop registering signal
handlers currently includes 'SIGHUP', which is unsupported on Windows; update
the registration so 'SIGHUP' is only added on non-Windows platforms (e.g., check
process.platform !== 'win32' or use os.platform()) and keep the existing
handlers for 'SIGINT' and 'SIGTERM' calling this.shutdown(sig) — locate the
for-loop that iterates over ['SIGINT','SIGTERM','SIGHUP'] and change it to
conditionally include 'SIGHUP' (or register it separately inside an if block) to
avoid a no-op listener on Windows.
There was a problem hiding this comment.
Pull request overview
This PR refactors the dev-hot-reload script to remove the concurrently dependency and replace it with a spawn-based process manager intended to improve signal (SIGINT) propagation and streamline watcher startup.
Changes:
- Replaces
concurrentlywith a customProcessManagerandspawn-based execution with prefixed logs. - Auto-discovers buildable
@usebruno/*packages, topologically sorts them by internal dependencies, and starts watchers in stages. - Updates Electron restart logic to run
nodemonwithout an extra shell wrapper.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Build nodemon args without a shell wrapper so signals propagate correctly | ||
| const nodemonArgs = [ | ||
| ...CONFIG.ELECTRON_WATCH_PATHS.flatMap(p => ['--watch', p]), | ||
| '--ext', 'js,jsx,ts,tsx', | ||
| '--delay', `${CONFIG.NODEMON_WATCH_DELAY}ms`, | ||
| '--exec', 'npm run dev --workspace=packages/bruno-electron' | ||
| ]; | ||
| procManager.register( | ||
| spawnWithPrefix('nodemon', nodemonArgs, 'electron', '\x1b[33m'), | ||
| 'electron' |
There was a problem hiding this comment.
The nodemon invocation passes --exec as a single argv element containing spaces ('npm run dev --workspace=packages/bruno-electron'). nodemon does not shell-parse/split this value; it will treat the entire string as the executable name, which will fail to spawn. Pass --exec as 'npm' and provide the remaining tokens (run, dev, --workspace=...) as subsequent arguments (or use nodemon’s -- args mechanism).
| setTimeout(() => { | ||
| log(LOG_LEVELS.WARN, 'Force-exiting after timeout...'); | ||
| process.exit(0); | ||
| }, 5000).unref(); |
There was a problem hiding this comment.
ProcessManager.shutdown() always process.exit(0) after the timeout, even when it was triggered by an unexpected child exit (register()’s close handler) or a failed build. This can report success despite failures. Consider tracking an exit code (e.g. set to 1 on child failure) and exiting with that code.
| // One-time build — not registered with ProcessManager | ||
| log(LOG_LEVELS.INFO, `${shortName} — running one-time build (no watch script)`); | ||
| return new Promise((resolve, reject) => { | ||
| const proc = spawn('npm', ['run', 'build', workspaceFlag], { stdio: 'inherit' }); |
There was a problem hiding this comment.
One-time build processes (packages without a watch script) are explicitly not registered with ProcessManager. If the user hits SIGINT during these builds, shutdown() won’t kill them and they can continue running orphaned in the background. Consider registering these build procs as well (possibly with a flag to allow normal exit) so they’re included in shutdown cleanup.
| // One-time build — not registered with ProcessManager | |
| log(LOG_LEVELS.INFO, `${shortName} — running one-time build (no watch script)`); | |
| return new Promise((resolve, reject) => { | |
| const proc = spawn('npm', ['run', 'build', workspaceFlag], { stdio: 'inherit' }); | |
| // One-time build — register with ProcessManager so it is cleaned up on shutdown | |
| log(LOG_LEVELS.INFO, `${shortName} — running one-time build (no watch script)`); | |
| return new Promise((resolve, reject) => { | |
| const proc = procManager.register( | |
| spawn('npm', ['run', 'build', workspaceFlag], { stdio: 'inherit' }), | |
| shortName | |
| ); |
| // Run setup if requested | ||
| if (runSetup) { |
There was a problem hiding this comment.
startDevelopment is now async, but it’s invoked later without await. If buildDevPackageGraph throws (e.g. cycle) or a one-time build rejects, the rejection won’t be caught by the outer .catch, and the script may emit an unhandled promise rejection. Ensure the call site awaits/returns the startDevelopment() promise so failures terminate deterministically.
| const shortName = pkg.name.replace('@usebruno/', ''); | ||
| const workspaceFlag = `--workspace=${pkg.dir}`; | ||
|
|
||
| if (pkg.hasWatch) { | ||
| const proc = procManager.register( | ||
| spawnWithPrefix('npm', ['run', 'watch', workspaceFlag], shortName, nextWatcherColor()), |
There was a problem hiding this comment.
workspaceFlag is built from pkg.dir, which is an absolute path (path.join(rootDir, 'packages', folder)). Everywhere else in the repo uses --workspace=packages/<name> (relative), and npm’s --workspace generally expects a workspace name or a path relative to the workspace root. Consider storing a relative workspace spec (e.g. packages/${folder}) or using the package name instead, to avoid npm failing to resolve the workspace.
Description
Change to using just spawns and removing the dependency on
concurrencyand removes shell nesting issues for SIGINTContribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit