Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 49 additions & 36 deletions code/lib/create-storybook/src/initiate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ import { telemetry } from 'storybook/internal/telemetry';

import boxen from 'boxen';
import * as find from 'empathic/find';
// eslint-disable-next-line depend/ban-dependencies
import execa from 'execa';
import picocolors from 'picocolors';
import { getProcessAncestry } from 'process-ancestry';
import prompts from 'prompts';
Expand Down Expand Up @@ -807,45 +809,56 @@ export async function initiate(options: CommandOptions): Promise<void> {
);

if (initiateResult?.shouldRunDev) {
const { projectType, packageManager, storybookCommand } = initiateResult;
logger.log('\nRunning Storybook');
await runStorybookDev(initiateResult);
}
}

try {
const supportsOnboarding = [
ProjectType.REACT_SCRIPTS,
ProjectType.REACT,
ProjectType.WEBPACK_REACT,
ProjectType.REACT_PROJECT,
ProjectType.NEXTJS,
ProjectType.VUE3,
ProjectType.ANGULAR,
].includes(projectType);

const flags = [];

// npm needs extra -- to pass flags to the command
// in the case of Angular, we are calling `ng run` which doesn't need the extra `--`
if (packageManager.type === 'npm' && projectType !== ProjectType.ANGULAR) {
flags.push('--');
}
/** Run Storybook dev server after installation */
async function runStorybookDev(result: {
projectType: ProjectType;
packageManager: JsPackageManager;
storybookCommand?: string;
shouldOnboard: boolean;
}): Promise<void> {
const { projectType, packageManager, storybookCommand, shouldOnboard } = result;

if (!storybookCommand) {
return;
}

if (supportsOnboarding && initiateResult.shouldOnboard) {
flags.push('--initial-path=/onboarding');
}
try {
const supportsOnboarding = [
ProjectType.REACT_SCRIPTS,
ProjectType.REACT,
ProjectType.WEBPACK_REACT,
ProjectType.REACT_PROJECT,
ProjectType.NEXTJS,
ProjectType.VUE3,
ProjectType.ANGULAR,
].includes(projectType);
Comment on lines +830 to +838
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Use the existing ONBOARDING_PROJECT_TYPES constant.

The supportsOnboarding check duplicates the ONBOARDING_PROJECT_TYPES constant defined at lines 66-75. This creates a maintenance burden if the list changes.

Apply this diff to use the existing constant:

-    const supportsOnboarding = [
-      ProjectType.REACT_SCRIPTS,
-      ProjectType.REACT,
-      ProjectType.WEBPACK_REACT,
-      ProjectType.REACT_PROJECT,
-      ProjectType.NEXTJS,
-      ProjectType.VUE3,
-      ProjectType.ANGULAR,
-    ].includes(projectType);
+    const supportsOnboarding = ONBOARDING_PROJECT_TYPES.includes(projectType);
📝 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
const supportsOnboarding = [
ProjectType.REACT_SCRIPTS,
ProjectType.REACT,
ProjectType.WEBPACK_REACT,
ProjectType.REACT_PROJECT,
ProjectType.NEXTJS,
ProjectType.VUE3,
ProjectType.ANGULAR,
].includes(projectType);
const supportsOnboarding = ONBOARDING_PROJECT_TYPES.includes(projectType);
🤖 Prompt for AI Agents
In code/lib/create-storybook/src/initiate.ts around lines 830 to 838, the
supportsOnboarding variable re-declares the list of onboarding project types
instead of using the existing ONBOARDING_PROJECT_TYPES constant defined earlier
(lines 66-75); replace the inline array with
ONBOARDING_PROJECT_TYPES.includes(projectType) so the check reuses the canonical
constant (ensure the constant is in scope/imported if necessary).


const flags = [];

// npm needs extra -- to pass flags to the command
// in the case of Angular, we are calling `ng run` which doesn't need the extra `--`
if (packageManager.type === 'npm' && projectType !== ProjectType.ANGULAR) {
flags.push('--');
}

flags.push('--quiet');

// instead of calling 'dev' automatically, we spawn a subprocess so that it gets
// executed directly in the user's project directory. This avoid potential issues
// with packages running in npxs' node_modules
packageManager.runPackageCommandSync(
storybookCommand.replace(/^yarn /, ''),
flags,
undefined,
'inherit'
);
} catch (e) {
// Do nothing here, as the command above will spawn a `storybook dev` process which does the error handling already. Else, the error will get bubbled up and sent to crash reports twice
if (supportsOnboarding && shouldOnboard) {
flags.push('--initial-path=/onboarding');
}

flags.push('--quiet');

// instead of calling 'dev' automatically, we spawn a subprocess so that it gets
// executed directly in the user's project directory. This avoid potential issues
// with packages running in npxs' node_modules
logger.log('\nRunning Storybook');
execa.command(`${storybookCommand} ${flags.join(' ')}`, {
stdio: 'inherit',
});
Comment on lines +858 to +860
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Missing await on the execa.command call.

The execa.command() call at line 858 is not awaited, which means the function returns immediately without waiting for the Storybook dev server to start. This appears unintentional based on the async function signature and may cause issues if the caller expects the process to complete or if error handling is needed.

If the intent is to spawn a long-running dev server and return immediately (fire-and-forget), consider:

  1. Removing the try-catch wrapper since errors won't be caught from an unawaited promise
  2. Adding a comment explaining the fire-and-forget pattern
  3. Optionally handling the returned ChildProcess for cleanup

If the process should be awaited, apply this diff:

-    execa.command(`${storybookCommand} ${flags.join(' ')}`, {
+    await execa.command(`${storybookCommand} ${flags.join(' ')}`, {
       stdio: 'inherit',
     });

🏁 Script executed:

#!/bin/bash
# View the file context around lines 858-860 with surrounding code and try-catch wrapper
sed -n '820,900p' code/lib/create-storybook/src/initiate.ts

Length of output: 1375


🏁 Script executed:

#!/bin/bash
# Search for similar execa patterns with fire-and-forget or .catch() usage
rg -n -B2 -A2 'execa\.(command|.*)\(' code/ --type ts --type tsx | head -100

Length of output: 27


🏁 Script executed:

#!/bin/bash
# Search for execa patterns in TypeScript files to find similar patterns
rg -n 'execa\.' code/ -g '*.ts' -A3 | head -150

Length of output: 626


🏁 Script executed:

#!/bin/bash
# View context around the awaited execa call for comparison
sed -n '180,210p' code/lib/create-storybook/src/scaffold-new-project.ts

Length of output: 796


🏁 Script executed:

#!/bin/bash
# Search for .catch() patterns on execa or similar async calls
rg -n '\.catch\(' code/lib/create-storybook/ -g '*.ts' -B2 -A2

Length of output: 744


Await the execa.command() call.

The review comment is correct. The initiate() function is called with await in code/lib/create-storybook/src/bin/run.ts:59 (.catch(() => process.exit(1))), which means the caller expects the returned promise to complete before handling errors. Without awaiting execa.command() at line 858, the function returns immediately and breaks the promise chain, making the try-catch ineffective.

Apply this fix:

-    execa.command(`${storybookCommand} ${flags.join(' ')}`, {
+    await execa.command(`${storybookCommand} ${flags.join(' ')}`, {
       stdio: 'inherit',
     });
📝 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
execa.command(`${storybookCommand} ${flags.join(' ')}`, {
stdio: 'inherit',
});
await execa.command(`${storybookCommand} ${flags.join(' ')}`, {
stdio: 'inherit',
});
🤖 Prompt for AI Agents
In code/lib/create-storybook/src/initiate.ts around lines 858 to 860, the
execa.command(...) call is not awaited so the function returns before the
spawned process completes, breaking the caller's promise chain and error
handling; update the code to either prepend await to execa.command(...) or
return the execa.command(...) promise so the initiate() function only resolves
after the command finishes, ensuring errors are propagated to the caller.

} catch {
// Do nothing here, as the command above will spawn a `storybook dev` process which does the error handling already
}
}
Loading