Skip to content

feat(dev-hot-reload): streamline hot-reload process with ProcessManag…#7731

Open
sid-bruno wants to merge 1 commit intousebruno:mainfrom
sid-bruno:fix/hot-reload-streamline
Open

feat(dev-hot-reload): streamline hot-reload process with ProcessManag…#7731
sid-bruno wants to merge 1 commit intousebruno:mainfrom
sid-bruno:fix/hot-reload-streamline

Conversation

@sid-bruno
Copy link
Copy Markdown
Collaborator

@sid-bruno sid-bruno commented Apr 10, 2026

Description

Recreate #7599

Contribution Checklist:

  • I've used AI significantly to create this pull request
  • The pull request only addresses one issue or adds one feature.
  • The pull request does not introduce any breaking changes
  • I have added screenshots or gifs to help explain the change if applicable.
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request.

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

  • Chores
    • Improved development environment startup and process management with enhanced dependency resolution and lifecycle handling for more reliable hot-reload functionality.

Copilot AI review requested due to automatic review settings April 10, 2026 11:30
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 10, 2026

Walkthrough

The dev launcher script transitions from a hardcoded concurrently pipeline to a dynamic, dependency-aware system. It discovers @usebruno/* packages, builds a dependency graph, performs topological sorting, and orchestrates multi-stage builds and watches with coordinated process lifecycle management and signal handling.

Changes

Cohort / File(s) Summary
Dev Hot Reload Pipeline
scripts/dev-hot-reload.js
Replaced hardcoded concurrent startup with dynamic package discovery and dependency-aware orchestration. Added ProcessManager for child process lifecycle and signal handling. Implemented topological sort for build stages. Changed from shell-wrapped concurrently to direct spawn execution with per-stage coordination. Long-lived watch processes now resolve after "created" output; one-time builds reject on non-zero exit. Removed CONFIG.ELECTRON_START_DELAY. React/Electron startup deferred until watcher stages complete.

Sequence Diagram(s)

sequenceDiagram
    participant Main as Dev Launcher
    participant PM as ProcessManager
    participant Discover as Package Discovery
    participant Graph as Dependency Graph
    participant Sort as Topological Sort
    participant Builder as Build/Watch Executor
    participant Watch as npm watch processes
    participant Build as npm build processes
    participant App as React/Electron Startup

    Main->>Discover: Scan packages/*/package.json
    Discover-->>Main: List `@usebruno/`* packages with build scripts
    
    Main->>Graph: Build dependency graph from internal deps
    Graph-->>Main: Dependency relations

    Main->>Sort: Perform topological sort
    Sort-->>Main: Build stages (or throw on cycle)

    Main->>PM: Initialize ProcessManager
    PM->>PM: Register signal handlers (SIGINT/SIGTERM/SIGHUP)

    loop For each stage
        Main->>Builder: Execute stage packages
        
        par Parallel Processing
            Builder->>Watch: npm run watch (if watch script exists)
            Watch->>PM: Register child process
            Watch-->>Builder: Wait for "created" substring output
            
            Builder->>Build: npm run build (if no watch script)
            Build->>PM: Register child process
            Build-->>Builder: Resolve on completion or reject on exit code
        end
        
        Builder-->>Main: Stage complete
    end

    Main->>App: All watcher stages ready → Start React/Electron
    App-->>Main: Application running

    alt Process terminates or signal received
        PM->>PM: Trigger coordinated shutdown
        PM->>Watch: Kill child processes
        PM->>Build: Kill child processes
        PM->>PM: Force exit with timeout
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

  • helloanoop
  • lohit-bruno
  • naman-bruno
  • bijin-bruno

Poem

🚀 From rigid pipes to graphs that flow,

Dependencies dance in topological show,

Processes born, managed, and knowing when done,

Watch and build in coordinated fun! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: introducing ProcessManager and streamlining the hot-reload process, which aligns with the core refactoring described in the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 (2)
scripts/dev-hot-reload.js (2)

132-139: ⚠️ Potential issue | 🟠 Major

command -v is Unix-only; use where on Windows.

Per the coding guidelines, shell commands must account for platform differences. This function will always return false on Windows.

🛠️ Proposed 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', shell: true });
     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 commandExists function
currently uses the Unix-only `command -v` which fails on Windows; update
commandExists to choose the correct platform-specific probe (use `where <cmd>`
when process.platform === 'win32' and `command -v <cmd>` otherwise), run the
chosen probe via execSync (preserving { stdio: 'ignore' } and try/catch
behavior), and return true on success or false on failure; reference the
function name commandExists and the use of execSync in your changes.

161-165: ⚠️ Potential issue | 🟠 Major

Unix-only shell commands break on Windows.

The find and rm -rf commands are not available on Windows. Consider using Node.js APIs or a cross-platform approach.

🛠️ Proposed cross-platform fix using Node.js
+const { rmSync } = require('fs');
+const { globSync } = require('glob'); // or use fast-glob

 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 dirs = globSync('**/node_modules', { onlyDirectories: true });
+  for (const dir of dirs) {
+    rmSync(dir, { recursive: true, force: true });
+  }
   log(LOG_LEVELS.SUCCESS, 'Node modules cleanup completed');
 }

Alternatively, if glob isn't available, use npx rimraf '**/node_modules' which is cross-platform.

🤖 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 uses Unix-only shell commands via execSync('find ... rm -rf
...'), which fails on Windows; replace that call with a cross-platform
implementation: either use a Node.js API-based approach (e.g., use glob or
fast-glob to locate '**/node_modules' and fs.rm or fs.rmdir with recursive=true
to delete each directory) or run a cross-platform tool such as invoking 'npx
rimraf "**/node_modules"' via execSync; update the execSync invocation in
cleanNodeModules to the chosen approach and ensure stdio handling and error
propagation remain consistent with the existing log(LOG_LEVELS.*) calls.
🧹 Nitpick comments (2)
scripts/dev-hot-reload.js (2)

204-205: Dependency graph excludes peerDependencies.

Unlike scripts/changed-packages.js which includes dependencies, devDependencies, and peerDependencies when building its internal dependency map, this only considers the first two. If any @usebruno/* package declares another as a peerDependency, the build order may be incorrect.

🔧 Suggested alignment with changed-packages.js
-    const allDeps = { ...raw.dependencies, ...raw.devDependencies };
+    const allDeps = { ...raw.dependencies, ...raw.devDependencies, ...raw.peerDependencies };
🤖 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 204 - 205, The dependency collection
in dev-hot-reload.js currently builds allDeps from raw.dependencies and
raw.devDependencies only, causing peerDependencies to be ignored; update the
logic that constructs allDeps (used by the variable internalDeps and functions
that rely on it) to also include raw.peerDependencies so that
Object.keys(allDeps).filter(k => k.startsWith('@usebruno/')) correctly picks up
peer deps; ensure you handle the case where raw.peerDependencies may be
undefined (merge safely) so internalDeps reflects dependencies, devDependencies,
and peerDependencies consistently with changed-packages.js.

428-428: startDevelopment() is not awaited.

Any rejection from startDevelopment() after the main IIFE resolves won't be caught by the .catch() handler. Consider awaiting it:

-  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` at line 428, The call to startDevelopment() is not
awaited so its rejection can escape the enclosing IIFE's .catch; change the
invocation so the async IIFE awaits or returns startDevelopment() (i.e., use
await startDevelopment() or return startDevelopment()) so any thrown/rejected
error from startDevelopment() is propagated into the existing .catch handler and
handled consistently.
🤖 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 signal registration loop in scripts/dev-hot-reload.js
currently adds 'SIGHUP' (for const sig of ['SIGINT', 'SIGTERM', 'SIGHUP']) which
is unsupported on Windows; update the registration to skip or conditionally add
'SIGHUP' when process.platform !== 'win32' (or construct the signal list based
on platform) and keep the same handler this.shutdown(sig) for the remaining
signals so Windows won’t receive an unsupported signal registration.
- Around line 332-343: The promise waiting for a watcher ready signal (the new
Promise that registers onData on proc.stdout/proc.stderr and resolves when
data.includes('created ')) can hang if the output never contains "created "; add
a timeout guard and optional configurable ready signal: when creating the
promise in scripts/dev-hot-reload.js, accept/configure a timeoutMs and/or
readyPattern (env var or parameter), start a timer that will reject or resolve
(choose behavior) after timeoutMs, and ensure both the timer and the
stdout/stderr listeners (onData) are cleaned up in all code paths; update
references to proc, onData, log and LOG_LEVELS.SUCCESS/shortName so the handler
clears listeners and timer before resolving on match or finishing on timeout.

---

Outside diff comments:
In `@scripts/dev-hot-reload.js`:
- Around line 132-139: The commandExists function currently uses the Unix-only
`command -v` which fails on Windows; update commandExists to choose the correct
platform-specific probe (use `where <cmd>` when process.platform === 'win32' and
`command -v <cmd>` otherwise), run the chosen probe via execSync (preserving {
stdio: 'ignore' } and try/catch behavior), and return true on success or false
on failure; reference the function name commandExists and the use of execSync in
your changes.
- Around line 161-165: The cleanNodeModules function currently uses Unix-only
shell commands via execSync('find ... rm -rf ...'), which fails on Windows;
replace that call with a cross-platform implementation: either use a Node.js
API-based approach (e.g., use glob or fast-glob to locate '**/node_modules' and
fs.rm or fs.rmdir with recursive=true to delete each directory) or run a
cross-platform tool such as invoking 'npx rimraf "**/node_modules"' via
execSync; update the execSync invocation in cleanNodeModules to the chosen
approach and ensure stdio handling and error propagation remain consistent with
the existing log(LOG_LEVELS.*) calls.

---

Nitpick comments:
In `@scripts/dev-hot-reload.js`:
- Around line 204-205: The dependency collection in dev-hot-reload.js currently
builds allDeps from raw.dependencies and raw.devDependencies only, causing
peerDependencies to be ignored; update the logic that constructs allDeps (used
by the variable internalDeps and functions that rely on it) to also include
raw.peerDependencies so that Object.keys(allDeps).filter(k =>
k.startsWith('@usebruno/')) correctly picks up peer deps; ensure you handle the
case where raw.peerDependencies may be undefined (merge safely) so internalDeps
reflects dependencies, devDependencies, and peerDependencies consistently with
changed-packages.js.
- Line 428: The call to startDevelopment() is not awaited so its rejection can
escape the enclosing IIFE's .catch; change the invocation so the async IIFE
awaits or returns startDevelopment() (i.e., use await startDevelopment() or
return startDevelopment()) so any thrown/rejected error from startDevelopment()
is propagated into the existing .catch handler and handled consistently.
🪄 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: f0d6c321-6c73-4f35-9d26-ce67d11c3ee3

📥 Commits

Reviewing files that changed from the base of the PR and between cd06f28 and 51f9f50.

📒 Files selected for processing (1)
  • scripts/dev-hot-reload.js

Comment on lines +75 to +77
for (const sig of ['SIGINT', 'SIGTERM', 'SIGHUP']) {
process.on(sig, () => this.shutdown(sig));
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

SIGHUP is not supported on Windows.

Per the coding guidelines, avoid Unix-only signals without Windows fallbacks. SIGHUP will fail silently or error on Windows. Consider conditionally registering it:

🛠️ Proposed fix
-    for (const sig of ['SIGINT', 'SIGTERM', 'SIGHUP']) {
+    const signals = process.platform === 'win32'
+      ? ['SIGINT', 'SIGTERM']
+      : ['SIGINT', 'SIGTERM', 'SIGHUP'];
+    for (const sig of signals) {
       process.on(sig, () => this.shutdown(sig));
     }
📝 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
for (const sig of ['SIGINT', 'SIGTERM', 'SIGHUP']) {
process.on(sig, () => this.shutdown(sig));
}
const signals = process.platform === 'win32'
? ['SIGINT', 'SIGTERM']
: ['SIGINT', 'SIGTERM', '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 signal registration loop
in scripts/dev-hot-reload.js currently adds 'SIGHUP' (for const sig of
['SIGINT', 'SIGTERM', 'SIGHUP']) which is unsupported on Windows; update the
registration to skip or conditionally add 'SIGHUP' when process.platform !==
'win32' (or construct the signal list based on platform) and keep the same
handler this.shutdown(sig) for the remaining signals so Windows won’t receive an
unsupported signal registration.

Comment on lines +332 to +343
return new Promise(resolve => {
const onData = data => {
if (data.toString().includes('created ')) {
proc.stdout.off('data', onData);
proc.stderr.off('data', onData);
log(LOG_LEVELS.SUCCESS, `${shortName} — initial watch build done`);
resolve();
}
};
proc.stdout.on('data', onData);
proc.stderr.on('data', onData);
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Potential hang if watcher output never contains "created ".

This assumes all watchers are rollup-based and emit "created ". If a package uses a different bundler or fails silently, this promise never resolves. Consider adding a timeout or a configurable ready signal.

🔧 Suggested timeout guard
     if (pkg.hasWatch) {
       const proc = procManager.register(
         spawnWithPrefix('npm', ['run', 'watch', workspaceFlag], shortName, nextWatcherColor()),
         shortName
       );
-      return new Promise(resolve => {
+      return new Promise((resolve, reject) => {
+        const timeout = setTimeout(() => {
+          reject(new Error(`${shortName} — timed out waiting for initial build`));
+        }, 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 waiting for a
watcher ready signal (the new Promise that registers onData on
proc.stdout/proc.stderr and resolves when data.includes('created ')) can hang if
the output never contains "created "; add a timeout guard and optional
configurable ready signal: when creating the promise in
scripts/dev-hot-reload.js, accept/configure a timeoutMs and/or readyPattern (env
var or parameter), start a timer that will reject or resolve (choose behavior)
after timeoutMs, and ensure both the timer and the stdout/stderr listeners
(onData) are cleaned up in all code paths; update references to proc, onData,
log and LOG_LEVELS.SUCCESS/shortName so the handler clears listeners and timer
before resolving on match or finishing on timeout.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the dev hot-reload script to remove the concurrently dependency and to orchestrate package watch/build startup via a custom ProcessManager, with a dependency-aware (topologically sorted) startup sequence before launching React and Electron.

Changes:

  • Replace concurrently-based orchestration with a custom ProcessManager that registers child processes and handles shutdown on signals / unexpected exits.
  • Auto-discover @usebruno/* packages with a build script, topologically sort them by internal dependencies, and start watch/build stages in order.
  • Start React (dev:web) and Electron (via nodemon) only after watchers report initial readiness.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


const tasks = stage.map(pkg => {
const shortName = pkg.name.replace('@usebruno/', '');
const workspaceFlag = `--workspace=${pkg.dir}`;
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

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

workspaceFlag is built from pkg.dir (an absolute filesystem path). npm workspaces typically expect a workspace name (e.g. @usebruno/common) or a repo-relative path (e.g. packages/bruno-common). If npm doesn't recognize the absolute path, it can fall back to the root watch script (which exists) and recursively re-run this script. Use --workspace=${pkg.name} or --workspace=${path.relative(rootDir, pkg.dir)} instead.

Suggested change
const workspaceFlag = `--workspace=${pkg.dir}`;
const workspaceFlag = `--workspace=${pkg.name}`;

Copilot uses AI. Check for mistakes.
Comment on lines +81 to +84
this._procs.push({ proc, name });
proc.on('close', code => {
if (!this._shuttingDown && code !== null && code !== 0) {
log(LOG_LEVELS.ERROR, `Process "${name}" exited unexpectedly (code ${code}), shutting down...`);
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

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

ProcessManager.register doesn't attach a listener for the child process error event. If spawning fails (e.g. command missing, permissions), Node will emit error and it can become an unhandled 'error' event, crashing the script without a controlled shutdown. Add proc.on('error', ...) to log the failure and invoke shutdown() (and consider exiting non-zero).

Suggested change
this._procs.push({ proc, name });
proc.on('close', code => {
if (!this._shuttingDown && code !== null && code !== 0) {
log(LOG_LEVELS.ERROR, `Process "${name}" exited unexpectedly (code ${code}), shutting down...`);
this._procs.push({ proc, name });
proc.on('error', err => {
if (!this._shuttingDown) {
log(LOG_LEVELS.ERROR, `Process "${name}" failed to start or crashed with an error: ${err.message}`);
process.exitCode = 1;
this.shutdown('child-error');
}
});
proc.on('close', code => {
if (!this._shuttingDown && code !== null && code !== 0) {
log(LOG_LEVELS.ERROR, `Process "${name}" exited unexpectedly (code ${code}), shutting down...`);
process.exitCode = 1;

Copilot uses AI. Check for mistakes.
Comment on lines +288 to +302
// Spawn a process and prefix every line of its output with [name]
function spawnWithPrefix(cmd, args, name, color) {
const prefix = `${color}[${name}]${COLORS.nc} `;
const proc = spawn(cmd, args, { stdio: ['ignore', 'pipe', 'pipe'] });

proc.stdout.on('data', data =>
data.toString().split('\n').filter(Boolean).forEach(line =>
process.stdout.write(prefix + line + '\n')
)
);
proc.stderr.on('data', data =>
data.toString().split('\n').filter(Boolean).forEach(line =>
process.stderr.write(prefix + line + '\n')
)
);
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

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

spawnWithPrefix splits each stdout/stderr chunk by \n and re-adds newlines. Stream chunk boundaries are arbitrary, so this can break lines (partial lines get treated as complete) and interleave prefixes incorrectly. Consider buffering per-stream until a newline is seen (or use readline on the stream) so prefixes are applied to complete lines only.

Suggested change
// Spawn a process and prefix every line of its output with [name]
function spawnWithPrefix(cmd, args, name, color) {
const prefix = `${color}[${name}]${COLORS.nc} `;
const proc = spawn(cmd, args, { stdio: ['ignore', 'pipe', 'pipe'] });
proc.stdout.on('data', data =>
data.toString().split('\n').filter(Boolean).forEach(line =>
process.stdout.write(prefix + line + '\n')
)
);
proc.stderr.on('data', data =>
data.toString().split('\n').filter(Boolean).forEach(line =>
process.stderr.write(prefix + line + '\n')
)
);
function attachPrefixedOutput(stream, output, prefix) {
let buffer = '';
stream.on('data', data => {
buffer += data.toString();
let newlineIndex = buffer.indexOf('\n');
while (newlineIndex !== -1) {
let line = buffer.slice(0, newlineIndex);
if (line.endsWith('\r')) {
line = line.slice(0, -1);
}
output.write(prefix + line + '\n');
buffer = buffer.slice(newlineIndex + 1);
newlineIndex = buffer.indexOf('\n');
}
});
stream.on('end', () => {
if (buffer.length > 0) {
output.write(prefix + buffer + '\n');
}
});
}
// Spawn a process and prefix every line of its output with [name]
function spawnWithPrefix(cmd, args, name, color) {
const prefix = `${color}[${name}]${COLORS.nc} `;
const proc = spawn(cmd, args, { stdio: ['ignore', 'pipe', 'pipe'] });
attachPrefixedOutput(proc.stdout, process.stdout, prefix);
attachPrefixedOutput(proc.stderr, process.stderr, prefix);

Copilot uses AI. Check for mistakes.
Comment on lines +91 to +101
shutdown(signal) {
if (this._shuttingDown) return;
this._shuttingDown = true;
log(LOG_LEVELS.INFO, `Received ${signal}, shutting down all processes...`);
for (const { proc } of this._procs) {
try { proc.kill(); } catch { /* already dead */ }
}
setTimeout(() => {
log(LOG_LEVELS.WARN, 'Force-exiting after timeout...');
process.exit(0);
}, 5000).unref();
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

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

shutdown() always force-exits with status 0 after the timeout, even when shutdown was triggered by an unexpected child exit. This makes it harder for callers/scripts to detect startup failures. Consider tracking a desired exit code (e.g. 1 on 'child-exit' / spawn errors) and using that in process.exit(...).

Copilot uses AI. Check for mistakes.
Comment on lines +332 to +342
return new Promise(resolve => {
const onData = data => {
if (data.toString().includes('created ')) {
proc.stdout.off('data', onData);
proc.stderr.off('data', onData);
log(LOG_LEVELS.SUCCESS, `${shortName} — initial watch build done`);
resolve();
}
};
proc.stdout.on('data', onData);
proc.stderr.on('data', onData);
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

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

The promise used to detect watcher readiness only resolves when output contains created . If the watch process exits early (e.g. rollup config error) before printing that line, this promise never resolves/rejects and startup hangs until the forced shutdown timeout. Consider rejecting on close (non-zero) and/or adding a readiness timeout with a clear error message.

Suggested change
return new Promise(resolve => {
const onData = data => {
if (data.toString().includes('created ')) {
proc.stdout.off('data', onData);
proc.stderr.off('data', onData);
log(LOG_LEVELS.SUCCESS, `${shortName} — initial watch build done`);
resolve();
}
};
proc.stdout.on('data', onData);
proc.stderr.on('data', onData);
return new Promise((resolve, reject) => {
let isReady = false;
const readinessTimeoutMs = 30000;
const cleanup = () => {
clearTimeout(readinessTimeout);
proc.stdout.off('data', onData);
proc.stderr.off('data', onData);
proc.off('close', onClose);
proc.off('error', onError);
};
const onData = data => {
if (data.toString().includes('created ')) {
isReady = true;
cleanup();
log(LOG_LEVELS.SUCCESS, `${shortName} — initial watch build done`);
resolve();
}
};
const onClose = code => {
if (!isReady) {
cleanup();
reject(new Error(`Watch failed for ${shortName} before readiness${code !== null ? ` (exit ${code})` : ''}`));
}
};
const onError = err => {
if (!isReady) {
cleanup();
reject(err);
}
};
const readinessTimeout = setTimeout(() => {
if (!isReady) {
cleanup();
reject(new Error(`Timed out waiting for ${shortName} watch readiness after ${readinessTimeoutMs}ms`));
}
}, readinessTimeoutMs);
proc.stdout.on('data', onData);
proc.stderr.on('data', onData);
proc.on('close', onClose);
proc.on('error', onError);

Copilot uses AI. Check for mistakes.
Comment on lines +366 to +374
// Setup development environment
async function startDevelopment() {
const rootDir = path.join(__dirname, '..');
const procManager = new ProcessManager();

const stages = buildDevPackageGraph(rootDir);
for (let i = 0; i < stages.length; i++) {
await startWatcherStage(stages[i], procManager, i);
}
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

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

startDevelopment is declared async and can throw/reject (e.g. cycle detection in topoSort, failed one-time builds). However the current entrypoint doesn't await it, so failures can become unhandled promise rejections instead of being caught by the outer main().catch(...). Either await startDevelopment() in the entrypoint or make startDevelopment non-async and handle errors internally.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants