UI: Fix showing and hiding copy prompt in the correct scenarios#34706
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChecklist initialization now reads project-scoped cache flags ( ChangesAI Setup Tracking & Checklist Initialization
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI aiSetup
participant Init as create-storybook
participant Cache as Local cache
participant Server as Dev Server (checklist)
participant IndexGen as StoryIndexGenerator
CLI->>Cache: set('ai-setup-ran', {timestamp, configDir: resolve(...)})
Init->>Cache: set('ai-init-opt-in', {timestamp, configDir: resolve(...)})
Server->>Cache: hasAiInitOptIn(configDir)
Cache-->>Server: boolean
Server->>Server: initializeChecklist(getStoryIndexGeneratorPromise, configDir)
Server->>IndexGen: (lazy) await storyIndexGeneratorPromise
IndexGen-->>Server: story index entries
Server->>Cache: hasAiSetupRun(configDir)
Cache-->>Server: boolean
Server->>Server: isAiSetupCompleted() checks setup flag + ai-generated entries
Server-->>Server: mark aiSetup done/open and schedule idle analytics/ghost stories
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/core-server/utils/ai-checklist-flags.ts`:
- Around line 20-23: The current hasAiSetupRun() treats any cached
'ai-setup-ran' key as globally true; change it to read the cached value (not
coerce to Boolean) and verify the stored configDir matches the current project
configDir before returning true — i.e., fetch cache.get('ai-setup-ran'),
parse/check that result has a configDir field equal to the passed/current
configDir and only then return true; update the hasAiSetupRun signature to
accept a configDir parameter (or otherwise receive the current configDir) and
update callers (notably initializeChecklist()) to pass the project configDir
through when invoking hasAiSetupRun().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f14d85ea-2ab2-42e2-8e74-90476e333259
📒 Files selected for processing (6)
code/core/src/core-server/presets/common-preset.tscode/core/src/core-server/utils/ai-checklist-flags.tscode/core/src/core-server/utils/checklist.test.tscode/core/src/core-server/utils/checklist.tscode/lib/cli-storybook/src/ai/index.tscode/lib/create-storybook/src/initiate.ts
…ook into yann/ai-feature-fixes
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/core/src/core-server/utils/ai-checklist-flags.test.ts`:
- Around line 5-20: Replace the current vi.hoisted + direct vi.mock usage with
the repo's spy-mocking pattern: call vi.mock('storybook/internal/common', () =>
({ cache: {} }), { spy: true }) at top-level, keep a top-level let
mockCacheStore: Map<string, unknown> and let mockedCache reference
(uninitialized), then in beforeEach initialize mockCacheStore = new Map() and
obtain the mock via mockedCache =
vi.mocked(require('storybook/internal/common').cache) (or import and then
vi.mocked(imported.cache)); set mockedCache.get = vi.fn(async (key:string) =>
mockCacheStore.get(key)) and mockedCache.set = vi.fn(async (key:string,
value:unknown) => mockCacheStore.set(key, value)); reset the map and call
mockedCache.mockReset() as needed in afterEach. Ensure all tests access the
mocked cache via vi.mocked(...) and that no behavior is defined outside
beforeEach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b65870f-08f7-45e9-8635-95bd9d9ac0b7
📒 Files selected for processing (6)
code/core/src/core-server/presets/common-preset.tscode/core/src/core-server/utils/ai-checklist-flags.test.tscode/core/src/core-server/utils/ai-checklist-flags.tscode/core/src/core-server/utils/checklist.test.tscode/core/src/core-server/utils/checklist.tscode/lib/create-storybook/src/initiate.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- code/core/src/core-server/presets/common-preset.ts
- code/lib/create-storybook/src/initiate.ts
- code/core/src/core-server/utils/checklist.ts
- code/core/src/core-server/utils/checklist.test.ts
| const { mockCacheStore, mockCache } = vi.hoisted(() => { | ||
| const store = new Map<string, unknown>(); | ||
| return { | ||
| mockCacheStore: store, | ||
| mockCache: { | ||
| get: async (key: string) => store.get(key), | ||
| set: async (key: string, value: unknown) => { | ||
| store.set(key, value); | ||
| }, | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| vi.mock('storybook/internal/common', () => ({ | ||
| cache: mockCache, | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In Vitest (current docs), what is the recommended pattern for module mocks using vi.mock(..., { spy: true })and typed access viavi.mocked()?
💡 Result:
The recommended pattern in current Vitest documentation (as of 2026) for module mocks using vi.mock(..., { spy: true }) combined with typed access via vi.mocked() is as follows: 1. Call vi.mock('./modulePath', { spy: true }) at the top of your test file. This hoists the mock, spies on all exports (replacing them with trackable spies), but preserves their original implementations. This is particularly useful in browser mode where direct vi.spyOn on imports isn't possible, or when you want to track calls across the entire module without replacing behavior [1][2]. Example setup: import { vi, expect, test } from 'vitest' import { calculator } from './src/calculator.ts' // Assume calculator is a function export vi.mock('./src/calculator.ts', { spy: true }) 2. Import the module exports as usual. The spies are automatically applied due to hoisting. 3. For TypeScript typed access to mock methods (enabling .mockX assertions and configurations), wrap the import or specific export with vi.mocked(). Example test: test('verifies calculator behavior', () => { const result = calculator(1, 2) expect(result).toBe(3) expect(vi.mocked(calculator)).toHaveBeenCalledWith(1, 2)[10][23] expect(vi.mocked(calculator)).toHaveReturnedWith(3)[10][23] }) This pattern allows asserting call details while the original logic runs, unlike full mocks with factories that replace implementations [3][1]. For partial mocks preserving some original behavior, use factory functions with importOriginal instead [4][5]. Use vi.mocked(export, { partial: true }) or { deep: true } for complex objects/classes [6][1][7]. Note: There was a Vitest 4 bug with deep spying (fixed later), but current docs confirm the pattern works [8].
Citations:
- 1: https://www.vitest.dev/api/vi
- 2: https://vitest.dev/api/vi.html
- 3: https://v4.vitest.dev/guide/mocking/modules
- 4: https://main.vitest.dev/guide/mocking.html
- 5: https://vitest.dev/guide/mocking
- 6: https://v4.vitest.dev/guide/mocking/classes
- 7: https://www.vitest.dev/guide/mocking/classes
- 8: vi.mock with spy: true stops working in Vitest 4 vitest-dev/vitest#9558
🏁 Script executed:
cd /repo && find . -path "*/code/core/src/core-server/utils/ai-checklist-flags.test.ts" -type fRepository: storybookjs/storybook
Length of output: 121
🏁 Script executed:
head -50 code/core/src/core-server/utils/ai-checklist-flags.test.tsRepository: storybookjs/storybook
Length of output: 1593
Align Vitest mocks with repo spy-mocking rules.
This test file must switch to the required spy: true + vi.mocked() pattern. The coding guidelines explicitly mandate using vi.mock() with the spy: true option for all package and file mocks, accessing mocks via vi.mocked(), and implementing mock behaviors in beforeEach blocks—all three are currently violated.
Required refactor
import { resolve } from 'node:path';
+import * as common from 'storybook/internal/common';
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
const { mockCacheStore } = vi.hoisted(() => {
const store = new Map<string, unknown>();
- return {
- mockCacheStore: store,
- mockCache: {
- get: async (key: string) => store.get(key),
- set: async (key: string, value: unknown) => {
- store.set(key, value);
- },
- },
- };
+ return { mockCacheStore: store };
});
-vi.mock('storybook/internal/common', () => ({
- cache: mockCache,
-}));
+vi.mock('storybook/internal/common', { spy: true });
describe('ai-checklist-flags', () => {
beforeEach(() => {
mockCacheStore.clear();
+ vi.mocked(common.cache.get).mockImplementation(async (key: string) => mockCacheStore.get(key));
+ vi.mocked(common.cache.set).mockImplementation(async (key: string, value: unknown) => {
+ mockCacheStore.set(key, value);
+ });
});
afterEach(() => {
+ vi.clearAllMocks();
vi.resetModules();
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/core/src/core-server/utils/ai-checklist-flags.test.ts` around lines 5 -
20, Replace the current vi.hoisted + direct vi.mock usage with the repo's
spy-mocking pattern: call vi.mock('storybook/internal/common', () => ({ cache:
{} }), { spy: true }) at top-level, keep a top-level let mockCacheStore:
Map<string, unknown> and let mockedCache reference (uninitialized), then in
beforeEach initialize mockCacheStore = new Map() and obtain the mock via
mockedCache = vi.mocked(require('storybook/internal/common').cache) (or import
and then vi.mocked(imported.cache)); set mockedCache.get = vi.fn(async
(key:string) => mockCacheStore.get(key)) and mockedCache.set = vi.fn(async
(key:string, value:unknown) => mockCacheStore.set(key, value)); reset the map
and call mockedCache.mockReset() as needed in afterEach. Ensure all tests access
the mocked cache via vi.mocked(...) and that no behavior is defined outside
beforeEach.
2 participants
Closes #
What I did
Decoupled the AI setup checklist UI from telemetry, and made the Copy prompt button stick around until the agent actually produces work.
aiOptInno longer depends on telemetry.storybook initnow writes a smallai-init-opt-inflag to the regular fs cache (in addition to the telemetry event), so users withSTORYBOOK_DISABLE_TELEMETRYset still see the Copy prompt button after opting in.storybook ai setupran, even if the user never pasted the prompt or the agent stalled. The dev server now requires bothai-setup-ran(new fs-cache flag, written by the CLI regardless of telemetry) and at least one story taggedai-generatedin the index before flipping the item to "done".core/src/core-server/utils/ai-checklist-flags.tsand plumbed the story-index getter throughinitializeChecklistso the dev server can detect agent work.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
yarn task sandbox --template react-vite/default-ts --start-from autoSTORYBOOK_DISABLE_TELEMETRY=1 npx storybook initand accept AI featuresSTORYBOOK_DISABLE_TELEMETRY=1 yarn storybook— confirm the "Set up Storybook with AI" card with Copy prompt button shows up in the Onboarding Checklist (this used to require telemetry to be enabled)npx storybook ai setup(don't paste anywhere yet) — confirm the card stays visible in the UI (it used to hide here)tags: ['ai-generated']Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake 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/coreteam 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
Bug Fixes
Tests