Spawn dashboard process from cli with openworkflow dashboard
#184
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR adds a new CLI command openworkflow dashboard to launch the dashboard web UI from the command line. The dashboard spawns as a child process using npx, allowing users to monitor workflow runs.
Changes:
- Added
.output/**to turbo.json build outputs for nitro-generated dashboard assets - Configured dashboard package.json with files field and start script for production deployment
- Implemented
dashboard()command function with config validation, process spawning, and signal handling
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| turbo.json | Added .output/** to build outputs to include nitro-generated dashboard build artifacts |
| packages/dashboard/package.json | Added files field for .output directory and start script to run built dashboard server |
| packages/cli/commands.ts | Implemented new dashboard() function to spawn dashboard process with config validation and graceful shutdown |
| packages/cli/cli.ts | Registered new dashboard command with appropriate description and error handling |
packages/cli/commands.ts
Outdated
| // eslint-disable-next-line sonarjs/no-os-command-from-path | ||
| const child = spawn("npx", ["@openworkflow/dashboard"], { | ||
| stdio: "inherit", | ||
| shell: true, |
There was a problem hiding this comment.
The spawn command does not specify a cwd (current working directory) option. This means the dashboard process will run in the same directory as the CLI process, which may not be the dashboard package directory. The dashboard needs to be run from its own directory to properly load the .output/server/index.mjs file and its dependencies. Consider setting the cwd option to the dashboard package directory.
| shell: true, | |
| shell: true, | |
| cwd: path.dirname(configFile), |
| const __dirname = dirname(fileURLToPath(import.meta.url)); | ||
| const serverPath = join(__dirname, ".output", "server", "index.mjs"); | ||
|
|
||
| await import(serverPath); |
There was a problem hiding this comment.
The bin wrapper imports the server file but doesn't handle potential import errors. If the .output/server/index.mjs file doesn't exist (e.g., if the dashboard hasn't been built yet), this will crash with an unhelpful error message. Consider adding error handling to provide a clear message directing users to build the dashboard first.
| await import(serverPath); | |
| try { | |
| await import(serverPath); | |
| } catch (error) { | |
| // Provide a clearer message if the dashboard server entrypoint is missing. | |
| if (error && (error.code === "ERR_MODULE_NOT_FOUND" || error.code === "MODULE_NOT_FOUND")) { | |
| console.error( | |
| `Dashboard server entrypoint not found at ${serverPath}.` + | |
| " Have you built the dashboard yet? Please run the dashboard build command (for example, `npm run build` in the dashboard package) and try again." | |
| ); | |
| } else { | |
| console.error(`Failed to start dashboard server from ${serverPath}.`); | |
| console.error(error); | |
| } | |
| process.exitCode = 1; | |
| } |
| export async function dashboard(): Promise<void> { | ||
| consola.start("Starting dashboard..."); | ||
|
|
||
| const { configFile } = await loadConfigWithEnv(); | ||
| if (!configFile) { | ||
| throw new CLIError( | ||
| "No config file found.", | ||
| "Run `ow init` to create a config file before starting the dashboard.", | ||
| ); | ||
| } | ||
| consola.info(`Using config: ${configFile}`); | ||
|
|
||
| // eslint-disable-next-line sonarjs/no-os-command-from-path | ||
| const child = spawn("npx", ["@openworkflow/dashboard"], { | ||
| stdio: "inherit", | ||
| }); | ||
|
|
||
| await new Promise<void>((resolve, reject) => { | ||
| /** remove signal handlers after the child exits */ | ||
| function cleanupSignalHandlers(): void { | ||
| process.off("SIGINT", signalHandler); | ||
| process.off("SIGTERM", signalHandler); | ||
| } | ||
|
|
||
| child.on("error", (error) => { | ||
| cleanupSignalHandlers(); | ||
| reject( | ||
| new CLIError( | ||
| "Failed to start dashboard.", | ||
| `Could not spawn npx: ${error.message}`, | ||
| ), | ||
| ); | ||
| }); | ||
|
|
||
| child.on("exit", (code) => { | ||
| cleanupSignalHandlers(); | ||
| if (code === 0 || code === null) { | ||
| resolve(); | ||
| } else { | ||
| reject( | ||
| new CLIError( | ||
| "Dashboard exited with an error.", | ||
| `Exit code: ${String(code)}`, | ||
| ), | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| /** | ||
| * Graceful shutdown on signals. | ||
| * @param signal - Signal | ||
| */ | ||
| function signalHandler(signal: NodeJS.Signals): void { | ||
| child.kill(signal); | ||
| } | ||
| process.on("SIGINT", signalHandler); | ||
| process.on("SIGTERM", signalHandler); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The dashboard() function lacks test coverage. Given that the CLI has test infrastructure (commands.test.ts exists) and the custom coding guideline requires appropriate tests for all new code, tests should be added for the dashboard command. At minimum, test cases should cover: successful dashboard launch, handling of missing config file, and error handling for spawn failures.
| function signalHandler(signal: NodeJS.Signals): void { | ||
| child.kill(signal); | ||
| } | ||
| process.on("SIGINT", signalHandler); | ||
| process.on("SIGTERM", signalHandler); |
There was a problem hiding this comment.
The signal handler function is defined inside the Promise callback but referenced in the cleanup function before it's defined. While JavaScript hoisting makes this work, it creates a subtle dependency. Consider defining signalHandler before using it in the event listeners, or restructure the code to avoid this forward reference pattern for better clarity.
No description provided.