CLI: Add vike CLI metadata#34189
Conversation
|
View your CI Pipeline Execution ↗ for commit d5e0d30
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughThis PR extends the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
📝 Coding Plan
Comment Tip CodeRabbit can use your project's `biome` configuration to improve the quality of JS/TS/CSS/JSON code reviews.Add a configuration file to your project to customize how CodeRabbit runs |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/lib/create-storybook/src/services/VersionService.test.ts (1)
186-192: Add one negative regression test for non-invocationviketokens.This new test covers the happy path. Please also add a case where
vikeappears only as an argument (not a CLI integration) to prevent future overmatching regressions.Suggested additional test
it('should detect vike new command', () => { const ancestry = [{ command: 'pnpm create vike@latest --- --react --storybook' }]; const integration = versionService.getCliIntegrationFromAncestry(ancestry as any); expect(integration).toBe('vike'); }); + + it('should not detect vike when it is only a non-command argument', () => { + const ancestry = [{ command: 'npm create vite@latest -- --template vike' }]; + + const integration = versionService.getCliIntegrationFromAncestry(ancestry as any); + + expect(integration).toBeUndefined(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/lib/create-storybook/src/services/VersionService.test.ts` around lines 186 - 192, Add a negative regression test in VersionService.test.ts that ensures getCliIntegrationFromAncestry does not return 'vike' when the token "vike" appears only as an argument (not as the invoked CLI). Specifically, add a new it(...) case where ancestry contains a command string like 'pnpm create `@something` latest --template vike' or 'node script.js vike' and assert that versionService.getCliIntegrationFromAncestry(ancestry as any) returns null/undefined/'' (matching existing negative expectation style); place the test near the existing 'should detect vike new command' case and reference getCliIntegrationFromAncestry and the integration variable to mirror the test pattern.
🤖 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/lib/create-storybook/src/services/VersionService.ts`:
- Around line 67-69: The current Vike detection in VersionService (the check on
ancestor.command?.includes('vike')) can misclassify commands that merely contain
the substring "vike"; update the detection logic in the same place where you
currently return 'vike' so it only matches actual invocations (e.g., use a
word-boundary regex or tokenized command check against ancestor.command to match
whole word "vike" or known binaries like "vike" or "npx vike"), ensuring the
check targets the ancestor.command string exactly rather than any substring to
avoid false telemetry attribution.
---
Nitpick comments:
In `@code/lib/create-storybook/src/services/VersionService.test.ts`:
- Around line 186-192: Add a negative regression test in VersionService.test.ts
that ensures getCliIntegrationFromAncestry does not return 'vike' when the token
"vike" appears only as an argument (not as the invoked CLI). Specifically, add a
new it(...) case where ancestry contains a command string like 'pnpm create
`@something` latest --template vike' or 'node script.js vike' and assert that
versionService.getCliIntegrationFromAncestry(ancestry as any) returns
null/undefined/'' (matching existing negative expectation style); place the test
near the existing 'should detect vike new command' case and reference
getCliIntegrationFromAncestry and the integration variable to mirror the test
pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dbaf7e6c-ca16-4720-a88b-37eb3c586f7a
📒 Files selected for processing (2)
code/lib/create-storybook/src/services/VersionService.test.tscode/lib/create-storybook/src/services/VersionService.ts
| // Check for vike | ||
| if (ancestor.command?.includes('vike')) { | ||
| return 'vike'; |
There was a problem hiding this comment.
Tighten Vike detection to avoid false telemetry attribution.
Line 68 uses a raw substring check, so commands that merely contain vike (not actual Vike invocation) can be misclassified as cliIntegration: 'vike'.
Proposed fix
- // Check for vike
- if (ancestor.command?.includes('vike')) {
+ // Check for Vike invocation forms
+ const command = ancestor.command ?? '';
+ const isVikeCommand =
+ /(?:^|\s)create\s+vike(?:@[^ ]+)?(?:\s|$)/i.test(command) ||
+ /(?:^|\s)vike(?:@[^ ]+)?\s+(?:new|init|create)\b/i.test(command);
+ if (isVikeCommand) {
return 'vike';
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check for vike | |
| if (ancestor.command?.includes('vike')) { | |
| return 'vike'; | |
| // Check for Vike invocation forms | |
| const command = ancestor.command ?? ''; | |
| const isVikeCommand = | |
| /(?:^|\s)create\s+vike(?:@[^ ]+)?(?:\s|$)/i.test(command) || | |
| /(?:^|\s)vike(?:@[^ ]+)?\s+(?:new|init|create)\b/i.test(command); | |
| if (isVikeCommand) { | |
| return 'vike'; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/lib/create-storybook/src/services/VersionService.ts` around lines 67 -
69, The current Vike detection in VersionService (the check on
ancestor.command?.includes('vike')) can misclassify commands that merely contain
the substring "vike"; update the detection logic in the same place where you
currently return 'vike' so it only matches actual invocations (e.g., use a
word-boundary regex or tokenized command check against ancestor.command to match
whole word "vike" or known binaries like "vike" or "npx vike"), ensuring the
check targets the ancestor.command string exactly rather than any substring to
avoid false telemetry attribution.
Closes #
What I did
This PR adds Vike as part of the cliIntegration detection for telemetry
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This is a small change on an already solid work. I believe this can only be properly tested once released.
npm create vike@latest --- --react --storybookwith STORYBOOK_TELEMETRY_DEBUG=1 set in the environmentcliIntegrationin the payloadDocumentation
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
Tests