Skip to content

Conversation

@valentinpalkovic
Copy link
Contributor

@valentinpalkovic valentinpalkovic commented Nov 28, 2025

Closes #

What I did

I have added telemetry for the playwright prompt.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>

Summary by CodeRabbit

  • New Features

    • Enhanced Playwright installation tracking with detailed installation status outcomes (installed, skipped, aborted, or failed).
    • Added telemetry monitoring for Playwright installation decisions to improve user experience insights.
  • Bug Fixes

    • Improved handling of aborted operations during addon configuration with clearer result propagation.

✏️ Tip: You can customize this high-level summary in your review settings.

…orted); expose abort outcome from tasks; wire TelemetryService and fix tests
@nx-cloud
Copy link

nx-cloud bot commented Nov 28, 2025

View your CI Pipeline Execution ↗ for commit 1bb3387

Command Status Duration Result
nx run-many -t check -c production --parallel=7 ✅ Succeeded 1s View ↗
nx run-many -t compile -c production --parallel=3 ✅ Succeeded 2s View ↗

☁️ Nx Cloud last updated this comment at 2025-11-29 18:21:34 UTC

@valentinpalkovic valentinpalkovic changed the title CLI: Add playwright-prompt decision Telemetry: Add playwright-prompt Nov 28, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 28, 2025

📝 Walkthrough

Walkthrough

This PR enhances the Playwright installation flow to track installation outcomes as structured result states ('installed', 'skipped', 'aborted', 'failed'), integrates telemetry to report these results, and refactors AddonConfigurationCommand to accept command options at the constructor level rather than through method parameters.

Changes

Cohort / File(s) Summary
Return Type Extension
code/core/src/cli/AddonVitestService.ts, code/core/src/node-logger/tasks.ts
Extended installPlaywright() and task execution functions to return Promise<{ errors: string[]; result: 'installed' | 'skipped' | 'aborted' | 'failed' }> and Promise<'aborted' | void> respectively, capturing result states in all control flow paths (success, abort, skip, failure).
Test Updates
code/core/src/node-logger/tasks.test.ts, code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
Added unit tests for executeTaskWithSpinner abort/error handling; updated mocks to match new return shapes and constructor signatures, including AddonVitestService.installPlaywright mocked return value and AddonConfigurationCommand constructor parameter changes.
Command Refactoring & Telemetry
code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts, code/lib/create-storybook/src/services/TelemetryService.ts
Moved CommandOptions (yes, disableTelemetry) from execute method to class constructor; added TelemetryService dependency; integrated trackPlaywrightPromptDecision() to report installation results as telemetry; removed options parameter from execute() and configureAddons() methods.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant AddonConfigurationCommand
    participant AddonVitestService
    participant executeTaskWithSpinner
    participant TelemetryService

    Client->>AddonConfigurationCommand: execute({ addons, configDir, ... })
    Note over AddonConfigurationCommand: Uses constructor-bound commandOptions
    
    AddonConfigurationCommand->>AddonConfigurationCommand: configureAddons()
    
    AddonConfigurationCommand->>AddonVitestService: installPlaywright({ yes })
    
    AddonVitestService->>executeTaskWithSpinner: Run installation task
    
    executeTaskWithSpinner-->>AddonVitestService: Returns 'aborted' | void
    
    alt Installation Success
        AddonVitestService-->>AddonConfigurationCommand: { errors: [], result: 'installed' }
    else User Aborted
        AddonVitestService-->>AddonConfigurationCommand: { errors: [], result: 'aborted' }
    else Installation Skipped
        AddonVitestService-->>AddonConfigurationCommand: { errors: [], result: 'skipped' }
    else Installation Failed
        AddonVitestService-->>AddonConfigurationCommand: { errors: [...], result: 'failed' }
    end
    
    Note over AddonConfigurationCommand: Capture result
    AddonConfigurationCommand->>TelemetryService: trackPlaywrightPromptDecision(result)
    TelemetryService-->>AddonConfigurationCommand: Telemetry recorded
    
    AddonConfigurationCommand-->>Client: Execution complete
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

  • Constructor signature changes in AddonConfigurationCommand with new parameters and dependencies requiring validation of all instantiation sites
  • Return type modifications across multiple functions (AddonVitestService.installPlaywright, executeTask, executeTaskWithSpinner) with result state tracking logic in all branches
  • Telemetry integration introducing new method and tracking call site requiring understanding of telemetry semantics
  • Parameter propagation changes removing options parameter from method signatures while ensuring command options remain accessible through instance state
  • Test mock complexity with async/thenable mocking patterns and updated return shape expectations

Possibly related PRs

✨ Finishing touches
  • 📝 Generate docstrings

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

Copy link
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: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts (1)

62-66: vi.clearAllMocks() placement clears mock implementations set in beforeEach.

Calling vi.clearAllMocks() at line 65 after setting up mock implementations in lines 40-63 will clear those implementations. This likely causes tests to use empty/undefined mocks. Either move vi.clearAllMocks() to the start of beforeEach or verify this is intentional behavior.

   beforeEach(async () => {
+    vi.clearAllMocks();
+
     const { postinstallAddon } = await import('../../../cli-storybook/src/postinstallAddon');
     // ... rest of setup
-
-    vi.clearAllMocks();
   });
🧹 Nitpick comments (2)
code/core/src/node-logger/tasks.test.ts (2)

9-33: Biome warnings for then property are expected here.

The static analysis warnings about adding then to an object are intentional in this context—you're simulating ExecaChildProcess's thenable behavior. Consider adding an inline eslint/biome disable comment to suppress the noise:

+// biome-ignore lint/suspicious/noThenProperty: Intentionally simulating ExecaChildProcess thenable
 const cp: Partial<ExecaChildProcess> = {
   stdout: stdout as any,
   then: undefined as any,

35-74: Consider splitting into separate test cases for clarity.

The single test validates both the success path (returns undefined) and abort path (returns 'aborted'). Splitting these into two distinct it() blocks would improve test readability and failure diagnostics.

Additionally, this test doesn't mock the spinner or logTracker dependencies from ./tasks, so it will hit the real implementations. Depending on test isolation requirements, you may want to mock these.

 describe('executeTaskWithSpinner', () => {
-  it('returns "aborted" when the child process rejects with an abort error', async () => {
+  it('returns undefined when child process succeeds', async () => {
     const outcome = await executeTaskWithSpinner(() => makeChild(), {
       id: 'test',
       intro: 'Intro',
       error: 'Error',
       success: 'Success',
       abortable: true,
     });
-
-    // Non-abort path returns undefined
     expect(outcome).toBeUndefined();
+  });

-    // Simulate an aborted child process by rejecting with an abort-like error message
-    const outcome2 = await executeTaskWithSpinner(
+  it('returns "aborted" when the child process rejects with an abort error', async () => {
+    const outcome = await executeTaskWithSpinner(
       () => {
         // ... existing abort simulation code
       },
       { ... }
     );
-
-    expect(outcome2).toBe('aborted');
+    expect(outcome).toBe('aborted');
   });
 });
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 013a3d9 and c93a306.

📒 Files selected for processing (6)
  • code/core/src/cli/AddonVitestService.ts (4 hunks)
  • code/core/src/node-logger/tasks.test.ts (1 hunks)
  • code/core/src/node-logger/tasks.ts (6 hunks)
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts (2 hunks)
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts (6 hunks)
  • code/lib/create-storybook/src/services/TelemetryService.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{js,jsx,json,html,ts,tsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use ESLint and Prettier configurations that are enforced in the codebase

Files:

  • code/core/src/cli/AddonVitestService.ts
  • code/core/src/node-logger/tasks.test.ts
  • code/lib/create-storybook/src/services/TelemetryService.ts
  • code/core/src/node-logger/tasks.ts
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Enable TypeScript strict mode

Files:

  • code/core/src/cli/AddonVitestService.ts
  • code/core/src/node-logger/tasks.test.ts
  • code/lib/create-storybook/src/services/TelemetryService.ts
  • code/core/src/node-logger/tasks.ts
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
code/**/*.{ts,tsx,js,jsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size

Files:

  • code/core/src/cli/AddonVitestService.ts
  • code/core/src/node-logger/tasks.test.ts
  • code/lib/create-storybook/src/services/TelemetryService.ts
  • code/core/src/node-logger/tasks.ts
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
code/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Export functions that need to be tested from their modules

Files:

  • code/core/src/cli/AddonVitestService.ts
  • code/core/src/node-logger/tasks.test.ts
  • code/lib/create-storybook/src/services/TelemetryService.ts
  • code/core/src/node-logger/tasks.ts
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
code/**/*.{js,jsx,json,html,ts,tsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing

Files:

  • code/core/src/cli/AddonVitestService.ts
  • code/core/src/node-logger/tasks.test.ts
  • code/lib/create-storybook/src/services/TelemetryService.ts
  • code/core/src/node-logger/tasks.ts
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
**/*.{test,spec}.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

**/*.{test,spec}.{ts,tsx}: Test files should follow the naming pattern *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Follow the spy mocking rules defined in .cursor/rules/spy-mocking.mdc for consistent mocking patterns with Vitest

Files:

  • code/core/src/node-logger/tasks.test.ts
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
**/*.test.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)

**/*.test.{ts,tsx,js,jsx}: Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Use vi.mocked() to type and access the mocked functions in Vitest tests
Implement mock behaviors in beforeEach blocks in Vitest tests
Mock all required dependencies that the test subject uses
Each mock implementation should return a Promise for async functions in Vitest
Mock implementations should match the expected return type of the original function
Mock all required properties and methods that the test subject uses in Vitest tests
Avoid direct function mocking without vi.mocked() in Vitest tests
Avoid mock implementations outside of beforeEach blocks in Vitest tests
Avoid mocking without the spy: true option in Vitest tests
Avoid inline mock implementations within test cases in Vitest tests
Avoid mocking only a subset of required dependencies in Vitest tests
Mock at the highest level of abstraction needed in Vitest tests
Keep mock implementations simple and focused in Vitest tests
Use type-safe mocking with vi.mocked() in Vitest tests
Document complex mock behaviors in Vitest tests
Group related mocks together in Vitest tests

Files:

  • code/core/src/node-logger/tasks.test.ts
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
code/**/*.{test,spec}.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

code/**/*.{test,spec}.{ts,tsx,js,jsx}: Write meaningful unit tests that actually import and call the functions being tested
Mock external dependencies using vi.mock() for file system, loggers, and other external dependencies in tests
Aim for high test coverage of business logic (75%+ for statements/lines) using coverage reports

Files:

  • code/core/src/node-logger/tasks.test.ts
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
🧠 Learnings (18)
📚 Learning: 2025-11-28T14:50:24.872Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.872Z
Learning: Applies to code/**/*.{test,spec}.{ts,tsx,js,jsx} : Mock external dependencies using vi.mock() for file system, loggers, and other external dependencies in tests

Applied to files:

  • code/core/src/node-logger/tasks.test.ts
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies in Vitest tests

Applied to files:

  • code/core/src/node-logger/tasks.test.ts
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors in Vitest tests

Applied to files:

  • code/core/src/node-logger/tasks.test.ts
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases in Vitest tests

Applied to files:

  • code/core/src/node-logger/tasks.test.ts
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking without the `spy: true` option in Vitest tests

Applied to files:

  • code/core/src/node-logger/tasks.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses in Vitest tests

Applied to files:

  • code/core/src/node-logger/tasks.test.ts
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Each mock implementation should return a Promise for async functions in Vitest

Applied to files:

  • code/core/src/node-logger/tasks.test.ts
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together in Vitest tests

Applied to files:

  • code/core/src/node-logger/tasks.test.ts
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
📚 Learning: 2025-11-24T17:49:31.838Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-24T17:49:31.838Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Follow the spy mocking rules defined in `.cursor/rules/spy-mocking.mdc` for consistent mocking patterns with Vitest

Applied to files:

  • code/core/src/node-logger/tasks.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of `beforeEach` blocks in Vitest tests

Applied to files:

  • code/core/src/node-logger/tasks.test.ts
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in `beforeEach` blocks in Vitest tests

Applied to files:

  • code/core/src/node-logger/tasks.test.ts
  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock at the highest level of abstraction needed in Vitest tests

Applied to files:

  • code/core/src/node-logger/tasks.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock implementations should match the expected return type of the original function

Applied to files:

  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused in Vitest tests

Applied to files:

  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.mock()` with the `spy: true` option for all package and file mocks in Vitest tests

Applied to files:

  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.mocked()` to type and access the mocked functions in Vitest tests

Applied to files:

  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use type-safe mocking with `vi.mocked()` in Vitest tests

Applied to files:

  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid direct function mocking without `vi.mocked()` in Vitest tests

Applied to files:

  • code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
🧬 Code graph analysis (3)
code/core/src/node-logger/tasks.test.ts (1)
code/core/src/node-logger/tasks.ts (1)
  • executeTaskWithSpinner (114-173)
code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts (3)
code/lib/create-storybook/src/generators/types.ts (1)
  • CommandOptions (118-137)
code/core/src/cli/AddonVitestService.ts (1)
  • AddonVitestService (39-386)
code/lib/create-storybook/src/services/TelemetryService.ts (1)
  • TelemetryService (10-115)
code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts (1)
code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts (1)
  • AddonConfigurationCommand (35-163)
🪛 Biome (2.1.2)
code/core/src/node-logger/tasks.test.ts

[error] 20-20: Do not add then to an object.

(lint/suspicious/noThenProperty)


[error] 27-27: Do not add then to an object.

(lint/suspicious/noThenProperty)


[error] 59-59: Do not add then to an object.

(lint/suspicious/noThenProperty)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: normal
  • GitHub Check: nx
  • GitHub Check: nx
  • GitHub Check: Core Unit Tests, windows-latest
  • GitHub Check: nx
🔇 Additional comments (10)
code/core/src/cli/AddonVitestService.ts (1)

117-176: Well-structured result tracking for Playwright installation outcomes.

The explicit result state tracking ('installed' | 'skipped' | 'aborted' | 'failed') is a clean approach that enables proper telemetry tracking downstream. All control flow paths correctly assign the result before returning.

code/lib/create-storybook/src/services/TelemetryService.ts (1)

35-43: Clean telemetry integration for Playwright installation tracking.

The method follows the established pattern of other tracking methods in this service. The result type aligns correctly with AddonVitestService.installPlaywright() return type.

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

56-112: Consistent return type update for abort handling.

The updated signature Promise<'aborted' | void> properly communicates the possible outcomes. The explicit return 'aborted' on abort and return undefined on success provide type-safe discrimination for callers.


114-173: LGTM!

Mirrors the changes in executeTask with identical abort-handling semantics. The return type update is consistent across both task execution functions.

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

50-53: Consider adding test coverage for telemetry tracking.

The AddonConfigurationCommand now calls telemetryService.trackPlaywrightPromptDecision(result) after Playwright installation when @storybook/addon-vitest is configured. The tests mock disableTelemetry: true, which disables tracking, but there's no explicit verification that telemetry is called when enabled.

Consider adding a test case that verifies telemetry is tracked when disableTelemetry: false, or at minimum verify that the telemetry service is instantiated correctly. Based on learnings, ensure all required dependencies are mocked.

code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts (5)

1-9: LGTM!

Imports are correctly structured. The TelemetryService import aligns with its usage in the constructor.


35-41: LGTM!

Good use of dependency injection with default instantiation. This pattern supports testability while keeping production usage simple. The readonly modifiers appropriately enforce immutability.


108-162: LGTM!

The refactoring to use this.commandOptions.yes is consistent with the new constructor pattern. The method logic remains sound.


165-174: LGTM!

Clean wrapper function that properly separates constructor dependencies (packageManager, options) from execution parameters. The type extension is correct.


64-70: I'll verify the review comment by investigating the installPlaywright method and error handling patterns in the codebase.
<function_calls>


#!/bin/bash

Find AddonVitestService file

fd -t f "AddonVitestService" --extension ts --extension js


</function_calls>

Comment on lines +10 to 15
vi.mock('storybook/internal/cli', async (actualImport) => ({
...(await actualImport()),
AddonVitestService: vi.fn().mockImplementation(() => ({
installPlaywright: vi.fn().mockResolvedValue([]),
installPlaywright: vi.fn().mockResolvedValue({ errors: [] }),
})),
}));
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 | 🟡 Minor

Mock missing the result field from the updated return type.

The installPlaywright mock returns { errors: [] }, but the actual return type is { errors: string[]; result: 'installed' | 'skipped' | 'aborted' | 'failed' }. The production code accesses result to pass to telemetryService.trackPlaywrightPromptDecision(result). This may cause test failures or mask issues.

 vi.mock('storybook/internal/cli', async (actualImport) => ({
   ...(await actualImport()),
   AddonVitestService: vi.fn().mockImplementation(() => ({
-    installPlaywright: vi.fn().mockResolvedValue({ errors: [] }),
+    installPlaywright: vi.fn().mockResolvedValue({ errors: [], result: 'installed' }),
   })),
 }));

And in beforeEach:

     const mockInstance = {
-      installPlaywright: vi.fn().mockResolvedValue({ errors: [] }),
+      installPlaywright: vi.fn().mockResolvedValue({ errors: [], result: 'installed' }),
     };

Also applies to: 43-48

🤖 Prompt for AI Agents
In code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
around lines 10-15 (and similarly at lines 43-48), the mocked installPlaywright
return value is missing the required result field; update the mock to return the
full shape { errors: string[]; result: 'installed' | 'skipped' | 'aborted' |
'failed' } (e.g., { errors: [], result: 'installed' }) so the production code
can read result and telemetryService.trackPlaywrightPromptDecision(result) will
receive a valid value.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Great. Thank you!!!

@valentinpalkovic valentinpalkovic added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Nov 29, 2025
@valentinpalkovic valentinpalkovic merged commit 8db6c43 into next Nov 29, 2025
57 of 72 checks passed
@valentinpalkovic valentinpalkovic deleted the valentin/add-init-step-playwright-telemetry branch November 29, 2025 18:11
yannbf pushed a commit that referenced this pull request Dec 2, 2025
…ywright-telemetry

Telemetry: Add playwright-prompt
(cherry picked from commit 8db6c43)
@github-actions github-actions bot added the patch:done Patch/release PRs already cherry-picked to main/release branch label Dec 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug ci:normal cli patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch telemetry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants