CLI: Review onboarding trigger logic on first dev run#34552
Merged
yannbf merged 8 commits intoApr 16, 2026
Conversation
|
View your CI Pipeline Execution ↗ for commit 96d1021
☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit a34e916 ☁️ Nx Cloud last updated this comment at |
Member
Author
|
@yannbf making a canary to test this because |
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds cache-driven onboarding signaling: a new exported helper resolves the initial browser path by consulting an Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI / user args
participant Init as create-storybook:initiate
participant Cache as cache
participant Core as core-server:buildDev
participant Detect as detectAgent
CLI->>Init: initiate (new user)
Init->>Cache: set('onboarding-pending', true)
CLI->>Core: start dev-server (may include initialPath)
Core->>Detect: detectAgent()
alt CLI provided initialPath
Core->>Core: return CLI initialPath
else not agent and no CLI path
Core->>Cache: get('onboarding-pending')
alt cache hit
Core->>Cache: remove('onboarding-pending')
Core->>Core: return '/onboarding'
else cache miss
Core->>Core: return undefined
end
else agent detected
Core->>Core: return undefined
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
code/core/src/core-server/build-dev.onboarding.test.ts (1)
9-47: Align this test with the repository's Vitest spy-mocking conventions.The mock at line 9 is missing the
spy: trueoption. Additionally, mock behaviors are being set inline within test cases (lines 29, 36, 43) rather than in thebeforeEachblock. Refactor to usevi.mock('storybook/internal/common', { spy: true }), move all mock behavior setup tobeforeEach, and usevi.mocked()to access the mocked functions per the spy-mocking guidelines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/core-server/build-dev.onboarding.test.ts` around lines 9 - 47, Change the top-level mock for 'storybook/internal/common' to use the spy mode (vi.mock(..., { spy: true })) and move all per-test mock behavior into the beforeEach block: use vi.mocked(...) to obtain typed mocks for mockCacheGet and mockCacheRemove and call mockResolvedValue(...) or reset implementations there instead of inside individual it() blocks; keep the assertions in the tests the same but remove in-test mock setup so resolveOnboardingInitialPath, mockCacheGet and mockCacheRemove are consistently prepared via beforeEach.
🤖 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/build-dev.ts`:
- Around line 64-67: Wrap the cache calls in try/catch so cache.get and
cache.remove failures are best-effort and do not block startup: call
cache.get('onboarding-pending') inside a try, if it returns truthy attempt
cache.remove('onboarding-pending') inside its own try/catch (so a remove failure
is logged/ignored), and only return '/onboarding' when onboardingPending is
truthy; ensure no errors from cache.get or cache.remove are re-thrown (use the
existing logger or console.warn to record errors). Reference symbols:
onboardingPending, cache.get, cache.remove in build-dev.ts.
In `@code/lib/create-storybook/src/initiate.ts`:
- Line 165: The onboarding cache key is currently global ("onboarding-pending")
and can collide across repos; update the write in
create-storybook/src/initiate.ts where cache.set('onboarding-pending', true) is
called to include a project-specific namespace (e.g., append or prefix with a
project identifier such as projectRoot, repoName, or a hashed workspace id) and
ensure the corresponding read in resolveOnboardingInitialPath uses the exact
same namespaced key; modify the key construction in both the cache.set call and
the lookup in resolveOnboardingInitialPath (referencing the cache.set usage and
the resolveOnboardingInitialPath function) so onboarding state is scoped per
project.
- Around line 164-166: The onboarding cache write
(cache.set('onboarding-pending', true)) can throw and currently lets init fail;
make this persistence best-effort by wrapping the cache.set call used when
shouldOnboard && FeatureCompatibilityService.supportsOnboarding(projectType) in
a try/catch inside the init flow (or the function that contains these symbols),
await the set as before but catch and log the error (or debug) without
rethrowing so init proceeds even if cache persistence fails.
---
Nitpick comments:
In `@code/core/src/core-server/build-dev.onboarding.test.ts`:
- Around line 9-47: Change the top-level mock for 'storybook/internal/common' to
use the spy mode (vi.mock(..., { spy: true })) and move all per-test mock
behavior into the beforeEach block: use vi.mocked(...) to obtain typed mocks for
mockCacheGet and mockCacheRemove and call mockResolvedValue(...) or reset
implementations there instead of inside individual it() blocks; keep the
assertions in the tests the same but remove in-test mock setup so
resolveOnboardingInitialPath, mockCacheGet and mockCacheRemove are consistently
prepared via 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: 8d6c6c7a-bd2a-4977-b24f-18149d8685fd
📒 Files selected for processing (3)
code/core/src/core-server/build-dev.onboarding.test.tscode/core/src/core-server/build-dev.tscode/lib/create-storybook/src/initiate.ts
Comment on lines
+164
to
+166
| if (shouldOnboard && FeatureCompatibilityService.supportsOnboarding(projectType)) { | ||
| await cache.set('onboarding-pending', true); | ||
| } |
Contributor
There was a problem hiding this comment.
Make onboarding cache persistence best-effort so init cannot fail after successful setup.
A failure in cache.set(...) currently bubbles up and can fail init, even though onboarding redirection is non-critical state.
Proposed fix
const shouldOnboard = newUser;
if (shouldOnboard && FeatureCompatibilityService.supportsOnboarding(projectType)) {
- await cache.set('onboarding-pending', true);
+ try {
+ await cache.set('onboarding-pending', true);
+ } catch (error) {
+ logger.debug(
+ `Failed to persist onboarding state: ${
+ error instanceof Error ? error.message : String(error)
+ }`
+ );
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/lib/create-storybook/src/initiate.ts` around lines 164 - 166, The
onboarding cache write (cache.set('onboarding-pending', true)) can throw and
currently lets init fail; make this persistence best-effort by wrapping the
cache.set call used when shouldOnboard &&
FeatureCompatibilityService.supportsOnboarding(projectType) in a try/catch
inside the init flow (or the function that contains these symbols), await the
set as before but catch and log the error (or debug) without rethrowing so init
proceeds even if cache persistence fails.
Sidnioulz
commented
Apr 15, 2026
…d post-config call site
…veOnboardingInitialPath
yannbf
approved these changes
Apr 16, 2026
This was referenced Apr 16, 2026
This was referenced Apr 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What I did
Replaced the old
init->dev --initial-path=/onboardingworkflow with one whereinitregisters when onboarding must happen in the local project cache, and any subsequentdevcall consumes this registered flag if no--initial-pathis set, allowing the user to be onboarded.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
dev🦋 Canary release
This pull request has been released as version
0.0.0-pr-34552-sha-a34e9165. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-34552-sha-a34e9165 sandboxor in an existing project withnpx storybook@0.0.0-pr-34552-sha-a34e9165 upgrade.More information
0.0.0-pr-34552-sha-a34e9165sidnioulz/fix-onboarding-ux-triggera34e91651776263275)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=34552Summary by CodeRabbit
Tests
Refactor