Skip to content

CLI: Prompt for init crash reports #34316

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JReinhold merged 3 commits into from
Mar 27, 2026
Merged

CLI: Prompt for init crash reports #34316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a63119
prompt for crash reports during init errors
JReinhold Mar 25, 2026
7808856
dont prompt on non-blocking errors during init
JReinhold Mar 26, 2026
82b7f69
format
JReinhold Mar 27, 2026
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
170 changes: 168 additions & 2 deletions code/core/src/core-server/withTelemetry.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { beforeEach, describe, expect, it, vi } from 'vitest';
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';

import { cache, loadAllPresets } from 'storybook/internal/common';
import { cache, isCI, loadAllPresets } from 'storybook/internal/common';
import { prompt } from 'storybook/internal/node-logger';
import { ErrorCollector, oneWayHash, telemetry } from 'storybook/internal/telemetry';

Expand All @@ -11,6 +11,18 @@ vi.mock('storybook/internal/telemetry', { spy: true });
vi.mock('storybook/internal/node-logger', { spy: true });

const cliOptions = {};
const originalStdoutIsTTY = process.stdout.isTTY;

const setStdoutIsTTY = (value: boolean | undefined) => {
Object.defineProperty(process.stdout, 'isTTY', {
value,
configurable: true,
});
};

afterEach(() => {
setStdoutIsTTY(originalStdoutIsTTY);
});

describe('withTelemetry', () => {
beforeEach(() => {
Expand Down Expand Up @@ -74,6 +86,52 @@ describe('withTelemetry', () => {
);
});

it('prompts for crash reports when init fails without preset options', async () => {
vi.mocked(isCI).mockReturnValue(false);
vi.mocked(cache.get).mockResolvedValueOnce(undefined);
vi.mocked(prompt.confirm).mockResolvedValueOnce(true);
setStdoutIsTTY(true);

await expect(async () =>
withTelemetry('init', { cliOptions, printError: vi.fn() }, run)
).rejects.toThrow(error);

expect(prompt.confirm).toHaveBeenCalledTimes(1);
expect(cache.set).toHaveBeenCalledWith('enableCrashReports', true);
expect(telemetry).toHaveBeenCalledWith(
'error',
expect.objectContaining({
eventType: 'init',
error: expect.objectContaining({ message: 'An Error!', name: 'Error' }),
isErrorInstance: true,
}),
expect.objectContaining({ enableCrashReports: true })
);
});

it('does not send full error details when init prompt is rejected', async () => {
vi.mocked(isCI).mockReturnValue(false);
vi.mocked(cache.get).mockResolvedValueOnce(undefined);
vi.mocked(prompt.confirm).mockResolvedValueOnce(false);
setStdoutIsTTY(true);

await expect(async () =>
withTelemetry('init', { cliOptions, printError: vi.fn() }, run)
).rejects.toThrow(error);

expect(prompt.confirm).toHaveBeenCalledTimes(1);
expect(cache.set).toHaveBeenCalledWith('enableCrashReports', false);
expect(telemetry).toHaveBeenCalledWith(
'error',
expect.objectContaining({
eventType: 'init',
error: undefined,
isErrorInstance: true,
}),
expect.objectContaining({ enableCrashReports: false })
);
});

it('does not send error message when cli opt out is passed', async () => {
await expect(async () =>
withTelemetry('dev', { cliOptions: { disableTelemetry: true }, printError: vi.fn() }, run)
Expand Down Expand Up @@ -383,6 +441,67 @@ describe('sendTelemetryError', () => {
})
);
});

it('does not prompt for non-blocking init errors without cached consent', async () => {
const options: any = {
cliOptions: {},
skipPrompt: false,
};
const mockError = new Error('Init non-blocking error');

vi.mocked(isCI).mockReturnValue(false);
vi.mocked(cache.get).mockResolvedValueOnce(undefined);
vi.mocked(prompt.confirm).mockResolvedValueOnce(true);
setStdoutIsTTY(true);

await sendTelemetryError(mockError, 'init', options, false);

expect(prompt.confirm).not.toHaveBeenCalled();
expect(vi.mocked(cache.set).mock.calls).not.toContainEqual([
'enableCrashReports',
expect.anything(),
]);
expect(telemetry).toHaveBeenCalledWith(
'error',
expect.objectContaining({
eventType: 'init',
blocking: false,
error: undefined,
isErrorInstance: true,
}),
expect.objectContaining({
enableCrashReports: false,
immediate: true,
})
);
});

it('uses cached crash report consent for non-blocking init errors', async () => {
const options: any = {
cliOptions: {},
skipPrompt: false,
};
const mockError = new Error('Init non-blocking error');

vi.mocked(cache.get).mockResolvedValueOnce(true);

await sendTelemetryError(mockError, 'init', options, false);

expect(prompt.confirm).not.toHaveBeenCalled();
expect(telemetry).toHaveBeenCalledWith(
'error',
expect.objectContaining({
eventType: 'init',
blocking: false,
error: expect.objectContaining({ message: 'Init non-blocking error', name: 'Error' }),
isErrorInstance: true,
}),
expect.objectContaining({
enableCrashReports: true,
immediate: true,
})
);
});
});

describe('getErrorLevel', () => {
Expand Down Expand Up @@ -412,13 +531,60 @@ describe('getErrorLevel', () => {
},
presetOptions: undefined,
skipPrompt: false,
eventType: 'dev',
};

const errorLevel = await getErrorLevel(options);

expect(errorLevel).toBe('error');
});

it('returns "full" for init when presetOptions are not provided and prompt is accepted', async () => {
const options: any = {
cliOptions: {
disableTelemetry: false,
},
presetOptions: undefined,
skipPrompt: false,
eventType: 'init',
};

vi.mocked(isCI).mockReturnValue(false);
vi.mocked(cache.get).mockResolvedValueOnce(undefined);
vi.mocked(prompt.confirm).mockResolvedValueOnce(true);
setStdoutIsTTY(true);

const errorLevel = await getErrorLevel(options);

expect(errorLevel).toBe('full');
expect(loadAllPresets).not.toHaveBeenCalled();
expect(prompt.confirm).toHaveBeenCalledTimes(1);
expect(cache.set).toHaveBeenCalledWith('enableCrashReports', true);
});

it('returns "error" for init when presetOptions are not provided and prompt is rejected', async () => {
const options: any = {
cliOptions: {
disableTelemetry: false,
},
presetOptions: undefined,
skipPrompt: false,
eventType: 'init',
};

vi.mocked(isCI).mockReturnValue(false);
vi.mocked(cache.get).mockResolvedValueOnce(undefined);
vi.mocked(prompt.confirm).mockResolvedValueOnce(false);
setStdoutIsTTY(true);

const errorLevel = await getErrorLevel(options);

expect(errorLevel).toBe('error');
expect(loadAllPresets).not.toHaveBeenCalled();
expect(prompt.confirm).toHaveBeenCalledTimes(1);
expect(cache.set).toHaveBeenCalledWith('enableCrashReports', false);
});

it('returns "full" when core.enableCrashReports is true', async () => {
const options: any = {
cliOptions: {
Expand Down
32 changes: 19 additions & 13 deletions code/core/src/core-server/withTelemetry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ type TelemetryOptions = {
presetOptions?: Parameters<typeof loadAllPresets>[0];
printError?: (err: any) => void;
skipPrompt?: boolean;
eventType?: EventType;
};

const promptCrashReports = async () => {

@valentinpalkovic valentinpalkovic Mar 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are we making sure that we don't prompt if the error is a non-blocking error? blocking: false. Or do we even want to prompt in this scenario?

@shilman shilman Mar 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good catch @valentinpalkovic

@JReinhold JReinhold Mar 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why would we not prompt in a non-blocking scenario? Don't we still benefit from the information?

@Sidnioulz Sidnioulz Mar 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could prompt with a slightly different message to avoid confusion, e.g. "Some errors happened, though Storybook successfully installed. "

@JReinhold JReinhold Mar 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've disabled prompting for errors in non-blocking scenarios, specifically during init only. I've also updated the manual QA steps in the PR description with this case.

Expand All @@ -40,29 +41,30 @@ export async function getErrorLevel({
cliOptions,
presetOptions,
skipPrompt,
eventType,
}: TelemetryOptions): Promise<ErrorLevel> {
if (cliOptions.disableTelemetry) {
return 'none';
}

// If we are running init or similar, we just have to go with true here
if (!presetOptions) {
if (!presetOptions && eventType !== 'init') {
return 'error';
}

// should we load the preset?
const presets = await loadAllPresets(presetOptions);
if (presetOptions) {
const presets = await loadAllPresets(presetOptions);

// If the user has chosen to enable/disable crash reports in main.js
// or disabled telemetry, we can return that
const core = await presets.apply('core');
// If the user has chosen to enable/disable crash reports in main.js
// or disabled telemetry, we can return that
const core = await presets.apply('core');

if (core?.enableCrashReports !== undefined) {
return core.enableCrashReports ? 'full' : 'error';
}
if (core?.enableCrashReports !== undefined) {
return core.enableCrashReports ? 'full' : 'error';
}

if (core?.disableTelemetry) {
return 'none';
if (core?.disableTelemetry) {
return 'none';
}
}

// Deal with typo, remove in future version (7.1?)
Expand Down Expand Up @@ -96,7 +98,11 @@ export async function sendTelemetryError(
try {
let errorLevel = 'error';
try {
errorLevel = await getErrorLevel(options);
errorLevel = await getErrorLevel({
...options,
eventType,
skipPrompt: options.skipPrompt || (eventType === 'init' && !blocking),
});
} catch (err) {
// If this throws, eg. due to main.js breaking, we fall back to 'error'
}
Expand Down
Loading