Repository navigation
Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (63)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (60)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughThe pull request replaces ChangesVerkit migration
Merge Risk: ⚪ Minimal · up to The dependency replacement does not introduce a supported correctness or runtime risk. A localized test-mocking guideline cleanup remains, but no actionable merge-blocking risk remains after normal review. ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/lib/create-storybook/src/generators/ANGULAR/index.ts (1)
128-136: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle invalid
findMinimumForRangeranges before reading version parts.
parseRangethrowsTypeErrorfor invalid ranges beforefindMinimumForRangereturnsnull, sotoDevkitVersioncan crash when an Angular dependency specifier cannot be parsed. Catch or validate invalid ranges before readingmajor/minor/patch/prerelease.🤖 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/lib/create-storybook/src/generators/ANGULAR/index.ts` around lines 128 - 136, Update toDevkitVersion around findMinimumForRange so invalid Angular dependency ranges are handled without propagating parseRange’s TypeError. Catch the parsing failure or validate the range before accessing min.major, min.minor, min.patch, or min.prerelease, and preserve the existing undefined result for ranges that cannot produce a minimum version.
🧹 Nitpick comments (2)
code/lib/create-storybook/src/services/ProjectTypeService.ts (1)
223-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the stale
eqMajorcomment.The comment still refers to
validRangeandminVersion, but the implementation now usesisValidRangeandfindMinimumForRange. Keep the rationale accurate for future maintenance.Proposed update
- // Uses validRange to avoid a throw from minVersion if an invalid range gets passed + // Validate the range before finding its minimum to handle invalid ranges safely.🤖 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/lib/create-storybook/src/services/ProjectTypeService.ts` around lines 223 - 226, Update the comment above ProjectTypeService.eqMajor to reference isValidRange and findMinimumForRange, preserving the rationale that validating the range prevents errors when processing invalid input.code/lib/cli-storybook/src/autoblock/block-experimental-addon-test.test.ts (1)
100-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert blocker results instead of mock call details.
expect(isLess).toHaveBeenCalledWith(...)tests an internal dependency call. Assert the publicblocker.checkresult for each version scenario instead.As per coding guidelines, tests should cover public contracts and externally observable effects rather than private implementation details.
Also applies to: 127-127
🤖 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/lib/cli-storybook/src/autoblock/block-experimental-addon-test.test.ts` at line 100, Replace the isLess mock call assertions in the blocker tests with assertions on the public blocker.check result for each version scenario, including the corresponding assertion near the other referenced line. Keep the scenarios and expected blocking behavior unchanged while removing reliance on the internal isLess invocation details.Source: Coding guidelines
🤖 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/addons/vitest/package.json`:
- Line 106: Move verkit from devDependencies to dependencies in the addon
package manifest, preserving its existing version constraint so the bundled
postinstall entrypoint can resolve it for production installs.
In `@code/core/src/cli/AddonVitestService.ts`:
- Around line 52-53: Add test coverage for getComparableVersion covering ^4,
disjunction ranges, upper-bounded ranges, and versionless ^3 and ^4 shorthand.
Preserve the existing exact, bounded, caret-version, and prerelease cases, and
assert the expected comparable versions used for `@vitest/browser` selection.
In `@code/lib/cli-storybook/src/autoblock/block-experimental-addon-test.test.ts`:
- Line 86: Move the vi.mocked(isLess) return-value setup out of the individual
test bodies and into beforeEach hooks. Add nested/scoped beforeEach blocks for
each version-specific behavior, preserving true and false returns for the
corresponding test contexts.
- Line 8: Update the verkit mock declaration in
block-experimental-addon-test.test.ts to enable spy mode by passing the `{ spy:
true }` options object to vi.mock, matching the repository’s mock convention.
In `@code/lib/cli-storybook/src/autoblock/block-major-version.test.ts`:
- Around line 22-27: Update the mockedSemver callsites in block-major-version
tests to use the mocked verkit members exposed by the vi.mock declaration:
tryParse, isPrerelease, isGreater, and getMajor. Alternatively, restore any
missing renamed mock members if those callsites must retain their current
behavior, ensuring no test references undefined semver properties.
- Around line 22-27: Update the verkit mock in block-major-version.test.ts to
use spy mode via vi.mock('verkit', { spy: true }) instead of a factory. Import
or access verkit directly and configure its mocked methods through
vi.mocked(verkit) in beforeEach, preserving the existing return-value setup.
In `@scripts/release/is-prerelease.ts`:
- Line 28: Update the result assignment around isPrereleaseVersion(version) to
explicitly compare its return value with true, ensuring parse failures
represented by null are exposed as false while valid prerelease results remain
true.
In `@scripts/release/version.ts`:
- Line 98: Update the ExactOptions.exact property type from IncrementType to
string, matching the optional string parsed by optionsSchema and assigned to
nextVersion in run.
- Line 274: Update scripts/release/version.ts lines 274-274 at the increment
call to pass the prerelease identifier through the expected identifier option
and handle a nullable result before assigning nextVersion. Update
scripts/sandbox/utils/yarn.ts lines 196-203 by parsing the string returned from
findMinimumForRange before accessing its major and minor fields.
In `@scripts/sandbox/utils/yarn.ts`:
- Around line 196-203: Parse the non-null result of findMinimumForRange in the
floor handling within narrowingDescriptor before accessing version fields. Pass
the parsed version to parse so caret ranges such as @^4.0.0 use defined major
and minor values, while preserving the existing null-return behavior.
---
Outside diff comments:
In `@code/lib/create-storybook/src/generators/ANGULAR/index.ts`:
- Around line 128-136: Update toDevkitVersion around findMinimumForRange so
invalid Angular dependency ranges are handled without propagating parseRange’s
TypeError. Catch the parsing failure or validate the range before accessing
min.major, min.minor, min.patch, or min.prerelease, and preserve the existing
undefined result for ranges that cannot produce a minimum version.
---
Nitpick comments:
In `@code/lib/cli-storybook/src/autoblock/block-experimental-addon-test.test.ts`:
- Line 100: Replace the isLess mock call assertions in the blocker tests with
assertions on the public blocker.check result for each version scenario,
including the corresponding assertion near the other referenced line. Keep the
scenarios and expected blocking behavior unchanged while removing reliance on
the internal isLess invocation details.
In `@code/lib/create-storybook/src/services/ProjectTypeService.ts`:
- Around line 223-226: Update the comment above ProjectTypeService.eqMajor to
reference isValidRange and findMinimumForRange, preserving the rationale that
validating the range prevents errors when processing invalid input.
🪄 Autofix
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 Plus
Run ID: 717ec33d-fa4f-4150-a5c1-f245dad8ea16
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (62)
.yarnrc.ymlcode/addons/vitest/package.jsoncode/addons/vitest/src/postinstall.tscode/builders/builder-webpack5/package.jsoncode/builders/builder-webpack5/src/preview/virtual-module-mapping.tscode/core/package.jsoncode/core/src/cli/AddonVitestService.tscode/core/src/cli/detectLanguage.tscode/core/src/cli/helpers.tscode/core/src/common/js-package-manager/BUNProxy.tscode/core/src/common/js-package-manager/JsPackageManager.tscode/core/src/common/js-package-manager/NPMProxy.tscode/core/src/common/js-package-manager/PNPMProxy.tscode/core/src/common/js-package-manager/util.tscode/core/src/common/node-version.tscode/core/src/core-server/utils/update-check.tscode/core/src/manager-api/modules/versions.tscode/core/src/telemetry/get-known-packages.tscode/frameworks/nextjs-vite/package.jsoncode/frameworks/nextjs-vite/src/preset.tscode/frameworks/nextjs/package.jsoncode/frameworks/nextjs/src/compatibility/compatibility-map.tscode/frameworks/nextjs/src/css/webpack.tscode/frameworks/nextjs/src/images/webpack.tscode/frameworks/nextjs/src/preset.tscode/frameworks/nextjs/src/utils.tscode/lib/cli-storybook/package.jsoncode/lib/cli-storybook/src/add.tscode/lib/cli-storybook/src/autoblock/block-experimental-addon-test.test.tscode/lib/cli-storybook/src/autoblock/block-experimental-addon-test.tscode/lib/cli-storybook/src/autoblock/block-major-version.test.tscode/lib/cli-storybook/src/autoblock/block-major-version.tscode/lib/cli-storybook/src/autoblock/block-node-version.tscode/lib/cli-storybook/src/autoblock/utils.tscode/lib/cli-storybook/src/automigrate/fixes/angular-to-angular-vite.tscode/lib/cli-storybook/src/automigrate/fixes/upgrade-storybook-related-dependencies.tscode/lib/cli-storybook/src/doctor/getIncompatibleStorybookPackages.tscode/lib/cli-storybook/src/doctor/getMismatchingVersionsWarning.tscode/lib/cli-storybook/src/doctor/hasMultipleVersions.tscode/lib/cli-storybook/src/sandbox.tscode/lib/cli-storybook/src/upgrade.tscode/lib/cli-storybook/src/util.tscode/lib/create-storybook/package.jsoncode/lib/create-storybook/src/generators/ANGULAR/index.tscode/lib/create-storybook/src/generators/REACT_SCRIPTS/index.tscode/lib/create-storybook/src/services/ProjectTypeService.tscode/lib/create-storybook/src/services/VersionService.test.tscode/lib/create-storybook/src/services/VersionService.tscode/presets/create-react-app/package.jsoncode/presets/create-react-app/src/helpers/processCraConfig.tscode/presets/react-webpack/package.jsoncode/presets/react-webpack/src/cra-config.tsscripts/package.jsonscripts/release/generate-pr-description.tsscripts/release/get-changelog-from-file.tsscripts/release/is-prerelease.tsscripts/release/publish.tsscripts/release/utils/get-changes.tsscripts/release/version.tsscripts/release/write-changelog.tsscripts/sandbox/utils/yarn.tsscripts/verdaccio.yaml
💤 Files with no reviewable changes (2)
- code/lib/create-storybook/src/services/VersionService.test.ts
- scripts/verdaccio.yaml
ef8f05b to
e31dae1
Compare
semver with verkit
e31dae1 to
92656fa
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
3c34765 to
8bedd8c
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
5d14cad to
b466cde
Compare
b466cde to
c9e0a25
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Closes #35638
What I did
Replaced
semverdependency withverkitChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Not needed for a dependency change, all existing tests pass
Documentation
N/A
No changes needed
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.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake 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>