diff --git a/code/core/src/cli/AddonVitestService.ts b/code/core/src/cli/AddonVitestService.ts index ae2218306a47..b4c2a8e64c5f 100644 --- a/code/core/src/cli/AddonVitestService.ts +++ b/code/core/src/cli/AddonVitestService.ts @@ -114,12 +114,16 @@ export class AddonVitestService { * @param options - Installation options * @returns Array of error messages if installation fails */ - async installPlaywright(options: { yes?: boolean } = {}): Promise<{ errors: string[] }> { + async installPlaywright( + options: { yes?: boolean } = {} + ): Promise<{ errors: string[]; result: 'installed' | 'skipped' | 'aborted' | 'failed' }> { const errors: string[] = []; const playwrightCommand = ['playwright', 'install', 'chromium', '--with-deps']; const playwrightCommandString = this.packageManager.getPackageCommand(playwrightCommand); + let result: 'installed' | 'skipped' | 'aborted' | 'failed'; + try { const shouldBeInstalled = options.yes ? true @@ -135,7 +139,7 @@ export class AddonVitestService { })(); if (shouldBeInstalled) { - await prompt.executeTaskWithSpinner( + const processAborted = await prompt.executeTaskWithSpinner( (signal) => this.packageManager.runPackageCommand({ args: playwrightCommand, @@ -150,10 +154,17 @@ export class AddonVitestService { abortable: true, } ); + if (processAborted) { + result = 'aborted'; + } else { + result = 'installed'; + } } else { logger.warn('Playwright installation skipped'); + result = 'skipped'; } } catch (e) { + result = 'failed'; ErrorCollector.addError(e); if (e instanceof Error) { errors.push(e.stack ?? e.message); @@ -162,7 +173,7 @@ export class AddonVitestService { } } - return { errors }; + return { errors, result }; } /** diff --git a/code/core/src/node-logger/tasks.test.ts b/code/core/src/node-logger/tasks.test.ts new file mode 100644 index 000000000000..fd546e46d2dc --- /dev/null +++ b/code/core/src/node-logger/tasks.test.ts @@ -0,0 +1,75 @@ +import { describe, expect, it, vi } from 'vitest'; + +// eslint-disable-next-line depend/ban-dependencies +import type { ExecaChildProcess } from 'execa'; + +import { executeTaskWithSpinner } from './tasks'; + +// Create a minimal fake ExecaChildProcess +const makeChild = (onStart?: (cp: Partial) => void): ExecaChildProcess => { + const listeners: Record = {}; + const stdout = { + on: vi.fn((event: string, cb: (data: Buffer) => void) => { + listeners[event] ||= []; + listeners[event].push(cb); + }), + } as any; + + const cp: Partial = { + stdout: stdout as any, + then: undefined as any, + catch: undefined as any, + finally: undefined as any, + }; + + const promise = Promise.resolve() as any; + Object.setPrototypeOf(cp, promise); + (cp as any).then = promise.then.bind(promise); + (cp as any).catch = promise.catch.bind(promise); + (cp as any).finally = promise.finally.bind(promise); + + onStart?.(cp); + return cp as ExecaChildProcess; +}; + +describe('executeTaskWithSpinner', () => { + it('returns "aborted" when the child process rejects with an abort error', 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( + () => { + const err = new Error('The operation was aborted'); + const p = Promise.reject(err); + // Avoid unhandled rejection warnings + p.catch(() => {}); + const cp: any = makeChild(); + // Make the thenable reject + const rejected = p as any; + Object.setPrototypeOf(cp, rejected); + cp.then = rejected.then.bind(rejected); + cp.catch = rejected.catch.bind(rejected); + cp.finally = rejected.finally.bind(rejected); + return cp; + }, + { + id: 'test2', + intro: 'Intro', + error: 'Error', + success: 'Success', + abortable: true, + } + ); + + expect(outcome2).toBe('aborted'); + }); +}); diff --git a/code/core/src/node-logger/tasks.ts b/code/core/src/node-logger/tasks.ts index de9ed3192521..ae63d2997fed 100644 --- a/code/core/src/node-logger/tasks.ts +++ b/code/core/src/node-logger/tasks.ts @@ -61,7 +61,7 @@ export const executeTask = async ( success, abortable = false, }: { intro: string; error: string; success: string; abortable?: boolean } -) => { +): Promise<'aborted' | void> => { logTracker.addLog('info', intro); log(intro); @@ -99,7 +99,7 @@ export const executeTask = async ( if (isAborted) { logTracker.addLog('info', `${intro} aborted`); log(CLI_COLORS.error(`${intro} aborted`)); - return; + return 'aborted'; } const errorMessage = err instanceof Error ? (err.stack ?? err.message) : String(err); logTracker.addLog('error', error, { error: errorMessage }); @@ -108,6 +108,7 @@ export const executeTask = async ( } finally { cleanup?.(); } + return undefined; }; export const executeTaskWithSpinner = async ( @@ -119,7 +120,7 @@ export const executeTaskWithSpinner = async ( success, abortable = false, }: { id: string; intro: string; error: string; success: string; abortable?: boolean } -) => { +): Promise<'aborted' | void> => { logTracker.addLog('info', intro); let abortController: AbortController | undefined; @@ -159,7 +160,7 @@ export const executeTaskWithSpinner = async ( if (isAborted) { logTracker.addLog('info', `${intro} aborted`); task.cancel(CLI_COLORS.warning(`${intro} aborted`)); - return; + return 'aborted'; } const errorMessage = err instanceof Error ? (err.stack ?? err.message) : String(err); logTracker.addLog('error', error, { error: errorMessage }); @@ -168,4 +169,5 @@ export const executeTaskWithSpinner = async ( } finally { cleanup?.(); } + return undefined; }; diff --git a/code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts b/code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts index 113a073b91f1..e4ad291db47d 100644 --- a/code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts +++ b/code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts @@ -7,9 +7,10 @@ import { AddonConfigurationCommand } from './AddonConfigurationCommand'; vi.mock('storybook/internal/node-logger', { spy: true }); -vi.mock('storybook/internal/cli', () => ({ +vi.mock('storybook/internal/cli', async (actualImport) => ({ + ...(await actualImport()), AddonVitestService: vi.fn().mockImplementation(() => ({ - installPlaywright: vi.fn().mockResolvedValue([]), + installPlaywright: vi.fn().mockResolvedValue({ errors: [] }), })), })); @@ -42,11 +43,14 @@ describe('AddonConfigurationCommand', () => { const { AddonVitestService } = await import('storybook/internal/cli'); mockAddonVitestService = vi.mocked(AddonVitestService as any); const mockInstance = { - installPlaywright: vi.fn().mockResolvedValue([]), + installPlaywright: vi.fn().mockResolvedValue({ errors: [] }), }; - mockAddonVitestService.mockImplementation(() => mockInstance); + mockAddonVitestService.mockImplementation(() => mockInstance as any); - command = new AddonConfigurationCommand(mockPackageManager); + command = new AddonConfigurationCommand(mockPackageManager, { + yes: true, + disableTelemetry: true, + } as any); mockTask = { success: vi.fn(), @@ -64,16 +68,11 @@ describe('AddonConfigurationCommand', () => { describe('execute', () => { it('should skip configuration when no addons are provided', async () => { const addons: string[] = []; - const options = { - packageManager: PackageManagerName.NPM, - features: [], - }; const result = await command.execute({ dependencyInstallationResult: { status: 'success' }, addons, configDir: '.storybook', - options, }); expect(result.status).toBe('success'); @@ -83,17 +82,11 @@ describe('AddonConfigurationCommand', () => { it('should configure test addons when test feature is enabled', async () => { const addons = ['@storybook/addon-a11y', '@storybook/addon-vitest']; - const options = { - packageManager: PackageManagerName.NPM, - features: [], - yes: true, - }; const result = await command.execute({ dependencyInstallationResult: { status: 'success' }, addons, configDir: '.storybook', - options, }); expect(result.status).toBe('success'); @@ -105,10 +98,6 @@ describe('AddonConfigurationCommand', () => { it('should handle configuration errors gracefully', async () => { const addons = ['@storybook/addon-a11y', '@storybook/addon-vitest']; - const options = { - packageManager: PackageManagerName.NPM, - features: [], - }; const error = new Error('Configuration failed'); mockPostinstallAddon.mockRejectedValue(error); @@ -117,7 +106,6 @@ describe('AddonConfigurationCommand', () => { dependencyInstallationResult: { status: 'success' }, addons, configDir: '.storybook', - options, }); expect(result.status).toBe('failed'); @@ -128,17 +116,11 @@ describe('AddonConfigurationCommand', () => { it('should complete successfully with valid configuration', async () => { const addons = ['@storybook/addon-a11y', '@storybook/addon-vitest']; - const options = { - packageManager: PackageManagerName.NPM, - features: [], - yes: true, - }; const result = await command.execute({ dependencyInstallationResult: { status: 'success' }, addons, configDir: '.storybook', - options, }); expect(result.status).toBe('success'); diff --git a/code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts b/code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts index 09c83de32ba4..fced261362fd 100644 --- a/code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts +++ b/code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts @@ -6,6 +6,7 @@ import { ErrorCollector } from 'storybook/internal/telemetry'; import { dedent } from 'ts-dedent'; import type { CommandOptions } from '../generators/types'; +import { TelemetryService } from '../services'; const ADDON_INSTALLATION_INSTRUCTIONS = { '@storybook/addon-vitest': @@ -14,7 +15,6 @@ const ADDON_INSTALLATION_INSTRUCTIONS = { type ExecuteAddonConfigurationParams = { addons: string[]; - options: CommandOptions; configDir?: string; dependencyInstallationResult: { status: 'success' | 'failed' }; }; @@ -35,18 +35,19 @@ export type ExecuteAddonConfigurationResult = { export class AddonConfigurationCommand { constructor( readonly packageManager: JsPackageManager, - private readonly addonVitestService = new AddonVitestService(packageManager) + private readonly commandOptions: CommandOptions, + private readonly addonVitestService = new AddonVitestService(packageManager), + private readonly telemetryService = new TelemetryService(commandOptions.disableTelemetry) ) {} /** Execute addon configuration */ async execute({ - options, addons, configDir, dependencyInstallationResult, }: ExecuteAddonConfigurationParams): Promise { const areDependenciesInstalled = - dependencyInstallationResult.status === 'success' && !options.skipInstall; + dependencyInstallationResult.status === 'success' && !this.commandOptions.skipInstall; if (!areDependenciesInstalled && this.getAddonsWithInstructions(addons).length > 0) { this.logManualAddonInstructions(addons); @@ -58,12 +59,14 @@ export class AddonConfigurationCommand { } try { - const { hasFailures, addonResults } = await this.configureAddons(configDir, addons, options); + const { hasFailures, addonResults } = await this.configureAddons(configDir, addons); if (addonResults.has('@storybook/addon-vitest')) { - await this.addonVitestService.installPlaywright({ - yes: options.yes, + const { result } = await this.addonVitestService.installPlaywright({ + yes: this.commandOptions.yes, }); + // Map outcome to telemetry decision + await this.telemetryService.trackPlaywrightPromptDecision(result); } return { status: hasFailures ? 'failed' : 'success' }; @@ -103,7 +106,7 @@ export class AddonConfigurationCommand { } /** Configure test addons (a11y and vitest) */ - private async configureAddons(configDir: string, addons: string[], options: CommandOptions) { + private async configureAddons(configDir: string, addons: string[]) { // Import postinstallAddon from cli-storybook package const { postinstallAddon } = await import('../../../cli-storybook/src/postinstallAddon'); @@ -123,7 +126,7 @@ export class AddonConfigurationCommand { await postinstallAddon(addon, { packageManager: this.packageManager.type, configDir, - yes: options.yes, + yes: this.commandOptions.yes, skipInstall: true, skipDependencyManagement: true, logger, @@ -161,7 +164,11 @@ export class AddonConfigurationCommand { export const executeAddonConfiguration = ({ packageManager, - ...options -}: ExecuteAddonConfigurationParams & { packageManager: JsPackageManager }) => { - return new AddonConfigurationCommand(packageManager).execute(options); + options, + ...rest +}: ExecuteAddonConfigurationParams & { + packageManager: JsPackageManager; + options: CommandOptions; +}) => { + return new AddonConfigurationCommand(packageManager, options).execute(rest); }; diff --git a/code/lib/create-storybook/src/services/TelemetryService.ts b/code/lib/create-storybook/src/services/TelemetryService.ts index 4d395e6cb33d..6c33af841159 100644 --- a/code/lib/create-storybook/src/services/TelemetryService.ts +++ b/code/lib/create-storybook/src/services/TelemetryService.ts @@ -18,7 +18,7 @@ export class TelemetryService { /** Track a new user check step */ async trackNewUserCheck(newUser: boolean): Promise { - this.runTelemetryIfEnabled('init-step', { + await this.runTelemetryIfEnabled('init-step', { step: 'new-user-check', newUser, }); @@ -32,6 +32,16 @@ export class TelemetryService { }); } + /** Track Playwright prompt decision (install | skip | aborted) */ + async trackPlaywrightPromptDecision( + result: 'installed' | 'skipped' | 'aborted' | 'failed' + ): Promise { + await this.runTelemetryIfEnabled('init-step', { + step: 'playwright-install', + result, + }); + } + /** Track the main init event with all metadata */ async trackInit(data: { projectType: ProjectType;