Reviewing files that changed from the base of the PR and between 9f8743a and 6e1bb50.

• code/yarn.lock is excluded by !**/yarn.lock, !**/*.lock

• code/addons/themes/src/postinstall.ts (1 hunks)
• code/addons/vitest/src/vitest-plugin/global-setup.ts (0 hunks)
• code/core/package.json (0 hunks)
• code/core/src/bin/dispatcher.ts (1 hunks)
• code/core/src/builder-manager/index.ts (1 hunks)
• code/core/src/cli/AddonVitestService.test.ts (1 hunks)
• code/core/src/cli/AddonVitestService.ts (1 hunks)
• code/core/src/common/js-package-manager/JsPackageManagerFactory.ts (2 hunks)
• code/core/src/common/utils/command.ts (0 hunks)
• code/core/src/core-server/dev-server.ts (1 hunks)
• code/core/src/core-server/utils/output-startup-information.ts (2 hunks)
• code/core/src/core-server/utils/server-statics.ts (1 hunks)
• code/core/src/node-logger/logger/colors.ts (1 hunks)
• code/core/src/node-logger/logger/logger.ts (1 hunks)
• code/core/src/node-logger/tasks.ts (0 hunks)
• code/core/src/node-logger/wrap-utils.ts (2 hunks)
• code/core/src/telemetry/exec-command-count-lines.ts (1 hunks)
• code/core/src/telemetry/notify.ts (1 hunks)
• code/lib/cli-storybook/src/upgrade.ts (1 hunks)
• code/lib/codemod/src/index.ts (0 hunks)
• code/lib/create-storybook/src/commands/FinalizationCommand.test.ts (0 hunks)
• code/lib/create-storybook/src/commands/FinalizationCommand.ts (2 hunks)
• code/lib/create-storybook/src/initiate.ts (2 hunks)
• code/lib/create-storybook/src/scaffold-new-project.ts (1 hunks)

• code/core/src/node-logger/tasks.ts
• code/addons/vitest/src/vitest-plugin/global-setup.ts
• code/core/src/common/utils/command.ts
• code/lib/codemod/src/index.ts
• code/core/package.json
• code/lib/create-storybook/src/commands/FinalizationCommand.test.ts

• GitHub Check: normal
• GitHub Check: Core Unit Tests, windows-latest

code/core/src/builder-manager/index.ts (1)

143-145: ✓ Logger usage is correct; message change looks good.

The log message is appropriately streamlined ("Starting..." vs. "Starting manager.."), and the code correctly uses the server-side logger from storybook/internal/node-logger per coding guidelines. The conditional check ensures the message only logs when not in quiet mode.

code/core/src/core-server/dev-server.ts (1)

97-97: LGTM! Log level adjustment reduces default verbosity.

The change from info to debug aligns with the PR's broader goal of reducing log verbosity during startup.

code/core/src/core-server/utils/server-statics.ts (1)

122-124: LGTM! Log level adjustment for static file serving.

Reducing this to debug level is appropriate, as static file serving is typically infrastructure-level information that doesn't need to be surfaced at the info level.

code/core/src/cli/AddonVitestService.ts (1)

146-146: LGTM! Good UX improvement with abort instruction.

Adding the abort hint improves user experience by making the cancellation capability discoverable. This aligns well with the abortable: true option on Line 149.

code/core/src/cli/AddonVitestService.test.ts (1)

382-382: LGTM! Test expectation correctly updated.

The test assertion properly reflects the updated intro message from the implementation.

code/core/src/node-logger/wrap-utils.ts (2)

32-34: LGTM! Simplified environment variable access.

Switching from shell command execution to direct process.env access is cleaner, faster, and removes an unnecessary dependency on shell execution.

---

63-66: LGTM! Error handling updated appropriately.

The catch block and comment correctly reflect the new direct environment access pattern, removing references to shell command execution.

code/core/src/common/js-package-manager/JsPackageManagerFactory.ts (4)

197-212: LGTM! Well-refactored command execution.

The refactoring to use executeCommandSync centralizes command execution logic and improves consistency. The try/catch pattern with boolean returns is appropriate for this detection use case.

---

214-229: LGTM! Consistent implementation.

The hasBun function follows the same clean pattern as hasNPM, maintaining consistency across the codebase.

---

231-247: LGTM! Consistent PNPM detection.

The implementation maintains consistency with other package manager detection functions.

---

249-264: LGTM! Version detection properly refactored.

The Yarn version detection correctly uses executeCommandSync and preserves the version parsing logic. The try/catch with undefined fallback is appropriate.

code/lib/create-storybook/src/commands/FinalizationCommand.ts (1)

11-14: LGTM! Clean API simplification.

The removal of selectedFeatures from the ExecuteFinalizationParams type and all related method signatures is consistent and correct. All call sites have been properly updated to match the new signatures.

Also applies to: 28-39, 66-91

code/core/src/telemetry/notify.ts (1)

20-25: LGTM! Simplified message formatting.

Removing the colorized prefix streamlines the message while preserving all essential content.

code/core/src/bin/dispatcher.ts (1)

73-76: LGTM! Safer command execution.

Removing shell: true improves security by avoiding shell interpretation and potential command injection vulnerabilities. The command array is properly constructed with the executable and arguments separated.

code/core/src/telemetry/exec-command-count-lines.ts (1)

17-17: LGTM! Aligned with security improvements.

Removing shell: true is consistent with the broader PR effort to avoid shell interpretation during command execution, improving security.

code/lib/create-storybook/src/initiate.ts (2)

106-109: LGTM! Aligned with FinalizationCommand changes.

The removal of selectedFeatures from the executeFinalization call is consistent with the updated API signature in FinalizationCommand.ts.

---

150-150: Verify the empty outro message is intentional.

The outro message is set to an empty string, which effectively silences the final log output. Please confirm this is the desired behavior rather than an oversight.

If the intent is to suppress the message, consider adding a comment:

-      logger.outro('');
+      // Suppress outro message as finalization handles all output
+      logger.outro('');

code/lib/create-storybook/src/scaffold-new-project.ts (1)

126-147: LGTM! Good UX improvement.

The addition of the "Other" option with a helpful hint and proper exit handling improves the user experience for unsupported frameworks. The early exit on line 141 correctly prevents accessing SUPPORTED_PROJECTS with an invalid key.

code/core/src/core-server/utils/output-startup-information.ts (1)

24-49: LGTM! Clean refactoring with improved output.

The replacement of cli-table3 with a simple array and logBox simplifies the code while maintaining clear output formatting. The use of the new storybook color formatter and structured box options enhances the visual presentation.

code/core/src/node-logger/logger/logger.ts (1)

154-156: Well-designed type refactor.

Delegating to clack.BoxOptions reduces duplication and ensures the type stays in sync with the upstream library, while keeping title explicit for clarity.

Reviewing files that changed from the base of the PR and between e54f891 and b041219.

• code/addons/themes/src/postinstall.ts (2 hunks)
• code/addons/vitest/src/postinstall.ts (1 hunks)
• code/core/src/bin/dispatcher.ts (2 hunks)
• code/lib/cli-storybook/src/link.ts (4 hunks)
• code/lib/cli-storybook/src/sandbox.ts (3 hunks)
• scripts/tasks/sandbox-parts.ts (0 hunks)
• scripts/utils/yarn.ts (1 hunks)

• scripts/tasks/sandbox-parts.ts

• GitHub Check: normal
• GitHub Check: Core Unit Tests, windows-latest

code/lib/cli-storybook/src/link.ts (3)

4-7: LGTM! Import changes align with command execution centralization.

The addition of executeCommand from storybook/internal/common and the narrowed import of spawnSync from cross-spawn are consistent with the PR's goal of centralizing command execution.

---

37-41: LGTM! executeCommand migrations are consistent and correct.

The migration of git clone, yarn link, yarn install, and yarn run storybook to use executeCommand is well-executed. Each call properly:

• Specifies the command and arguments
• Sets the appropriate working directory via cwd
• Awaits the async operation

The implicit error handling (throwing on command failure) is appropriate for this linking workflow where failures should abort the process.

Also applies to: 62-66, 89-93, 97-101

---

47-51: Consider migrating to executeCommand for consistency.

This yarn version check is the only command execution still using spawnSync directly. While synchronous execution and stdout capture may be necessary here, consider whether this could be refactored to use executeCommand (with .then() or await) for consistency with the rest of the file.

If a synchronous variant is needed, verify whether executeCommand supports synchronous execution or stdout capture patterns. If not, the current approach is acceptable, but a brief comment explaining why spawnSync is used here would be helpful.

code/addons/vitest/src/postinstall.ts (3)

331-332: LGTM: Correct argument splitting for shellless execution.

The change from '--loglevel=silent' to separate arguments is correct for shellless command execution where each argument must be a distinct array element.

---

338-338: LGTM: Consistent argument splitting.

The change correctly splits the package-manager argument into separate array elements, consistent with the shellless execution pattern.

---

346-346: LGTM: Consistent argument splitting.

The change correctly splits the config-dir argument into separate array elements, maintaining consistency with the shellless execution pattern throughout the command array.

code/addons/themes/src/postinstall.ts (1)

1-1: Correct shellless command execution implementation.

The command splitting logic correctly handles multi-word commands (e.g., 'pnpm dlx', 'yarn dlx') and the use of spawnSync with an explicit arguments array is the proper approach for shellless execution.

Also applies to: 15-18

code/lib/cli-storybook/src/sandbox.ts (4)

2-2: LGTM: Clean removal of unused import.

The mkdir import is no longer needed and has been correctly removed.

---

80-82: LGTM: Simplified border configuration.

The updated configuration aligns with the design feedback and uses a cleaner API.

---

260-260: LGTM: Simplified border configuration.

Consistent with the other updated logBox calls.

---

124-124: Good news: both borderColor (accepting hex values like '#F1618C') and borderStyle: 'round' are valid options in boxen. The as any casts at lines 124 and 143 indicate a TypeScript typing issue in the project, not an API issue. Since these are not API-breaking changes and the code is functionally valid, the original review comment's concern about inconsistency is valid but the lines are actually correct as-is.

scripts/utils/yarn.ts (1)

63-63: No issues found; the change is appropriate for sandboxed environments.

The yarn config set enableScripts false setting is intentionally applied to sandboxed template environments where reproducible, controlled builds are needed. This aligns with standard CI practices and causes no breaking changes—no critical dependencies in this project rely on lifecycle scripts for setup or execution. The export mappings (e.g., "./postinstall" entries) are file exports and are unaffected by this setting.

code/core/src/bin/dispatcher.ts (1)

64-64: Path construction is correct for non-shell spawn.

The direct path construction without quotes is appropriate for spawn without shell mode, where arguments are passed directly to the process without shell interpretation.

Reviewing files that changed from the base of the PR and between b041219 and eee9dbe.

• code/addons/vitest/src/postinstall.ts (2 hunks)
• code/core/src/common/js-package-manager/JsPackageManager.ts (2 hunks)
• code/core/src/common/js-package-manager/Yarn2Proxy.ts (4 hunks)
• scripts/tasks/sandbox-parts.ts (1 hunks)
• scripts/utils/cli-step.ts (1 hunks)

• code/addons/vitest/src/postinstall.ts
• scripts/tasks/sandbox-parts.ts

• GitHub Check: normal
• GitHub Check: Core Unit Tests, windows-latest

scripts/utils/cli-step.ts (1)

44-44: Correct the loglevel values constraint to include all supported levels.

The principle of adding value constraints is sound. However, the suggested values are incomplete. The codebase supports six log levels: trace, debug, info, warn, error, and silent.

Additionally, verification confirms that callers have already been updated to use loglevel instead of debug (as seen in scripts/tasks/sandbox-parts.ts).

Apply this corrected diff:

-      loglevel: { type: 'string' },
+      loglevel: { type: 'string', values: ['trace', 'debug', 'info', 'warn', 'error', 'silent'] },

code/core/src/common/js-package-manager/JsPackageManager.ts (1)

12-12: LGTM!

The import of coerce from semver is correctly added to support version normalization.

code/core/src/common/js-package-manager/Yarn2Proxy.ts (2)

14-14: LGTM! Logger import follows coding guidelines.

The addition of the logger import is appropriate and aligns with the coding guidelines to use Storybook loggers instead of console.* methods in application code.

---

151-151: LGTM! Debug logging placement and level are appropriate.

The debug log provides useful diagnostic information about found installations at an appropriate verbosity level.

Reviewing files that changed from the base of the PR and between 9ba0234 and a84ea41.

• code/addons/vitest/src/postinstall.ts (2 hunks)
• code/core/package.json (0 hunks)
• code/core/src/cli/AddonVitestService.test.ts (1 hunks)
• code/core/src/cli/AddonVitestService.ts (1 hunks)
• code/core/src/common/js-package-manager/JsPackageManager.ts (2 hunks)
• code/core/src/common/js-package-manager/Yarn2Proxy.ts (4 hunks)
• code/core/src/node-logger/logger/colors.ts (1 hunks)
• code/core/src/node-logger/logger/logger.ts (1 hunks)
• code/core/src/node-logger/prompts/prompt-functions.ts (2 hunks)
• code/core/src/node-logger/tasks.ts (0 hunks)

• code/core/package.json
• code/core/src/node-logger/tasks.ts

• code/core/src/cli/AddonVitestService.ts
• code/core/src/common/js-package-manager/Yarn2Proxy.ts
• code/core/src/node-logger/logger/logger.ts
• code/addons/vitest/src/postinstall.ts

• GitHub Check: normal
• GitHub Check: Core Unit Tests, windows-latest

code/core/src/cli/AddonVitestService.test.ts (1)

393-393: LGTM! Test correctly updated to match implementation.

The test expectation now includes the abort hint in the intro message, which aligns with the implementation change in AddonVitestService.ts.

code/core/src/common/js-package-manager/JsPackageManager.ts (2)

12-12: LGTM - semver.coerce import

The addition of coerce to the semver imports is appropriate for the version normalization logic added below.

---

638-644: Verification complete—version normalization is safe for all consumers.

I verified all 10+ call sites of getInstalledVersion() across the codebase. Every consumer uses the returned version in a way that's compatible with normalized semver:

• Primary usage: satisfies(vitestVersionSpecifier, '>= X.Y.Z') pattern works correctly with coerced versions
• Truthiness checks: Existing null/undefined checks remain unaffected
• String returns: Direct returns of the version string work as-is
• Build metadata: Per semver specification, build metadata (+build.xyz) doesn't affect version precedence comparisons, which is all the consumers perform

The code is safe to merge.

One minor note: code/core/src/cli/AddonVitestService.ts line 224 redundantly calls coerce() again on the already-coerced result, which is harmless but suggests the consumer may not be aware of the normalization now happening in getInstalledVersion(). No action needed, but could be cleaned up in a follow-up.

code/core/src/node-logger/prompts/prompt-functions.ts (2)

200-200: Remove redundant shouldLog check.

The ternary expression is redundant because the else block only executes when both !isInteractiveTerminal() and !shouldLog('info') are true (from line 160's condition). Therefore, shouldLog('info') is guaranteed to be false here, and maybeLog will always be the no-op function.

Apply this diff to simplify the code:

-    const maybeLog = shouldLog('info') ? logger.log : (_: string) => {};
-
+    const maybeLog = (_: string) => {};

Or inline the no-op directly in the return object if preferred.

⛔ Skipped due to learnings

Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to {code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs} : Do not use `console.log`, `console.warn`, or `console.error` directly unless in isolated files where importing loggers would significantly increase bundle size

---

160-160: Verify if taskLog's broader activation logic and lack of individual guards is intentional.

The inconsistency identified in the review is verified as accurate. The taskLog function activates under the condition if (isInteractiveTerminal() || shouldLog('info')), whereas spinner only activates via if (isInteractiveTerminal()). Additionally, within the interactive branch, spinner guards all method calls (start, stop, message) with individual if (shouldLog('info')) checks (lines 121, 128, 133), while taskLog does not—its wrapped methods directly invoke the underlying task without log-level filtering.

This creates a behavioral difference where:

• In non-interactive terminals with shouldLog('info') true, taskLog will activate the provider's task (designed for interactive output), whereas spinner will not
• When active in interactive mode, spinner respects log-level filtering for each method call; taskLog does not

Ensure this is the intended behavior and that the underlying TaskLog provider correctly handles non-interactive scenarios and respects log-level filtering appropriately.

Reviewing files that changed from the base of the PR and between a84ea41 and f9221a8.

• code/core/src/cli/AddonVitestService.ts (1 hunks)
• code/core/src/core-server/withTelemetry.ts (1 hunks)

• GitHub Check: normal
• GitHub Check: Core Unit Tests, windows-latest

code/core/src/cli/AddonVitestService.ts (2)

142-142: LGTM! stdio configuration supports abort functionality.

The change from 'ignore' to ['inherit', 'pipe', 'pipe'] correctly enables user interaction through stdin while piping stdout/stderr for spinner display. This aligns perfectly with the abortable: true option at line 150.

---

147-147: LGTM! Clear abort instruction improves UX.

The updated message informs users of the abort capability, which is consistent with the abortable: true configuration and the stdio changes that enable user interaction.