Skip to content

Conversation

@ghengeveld
Copy link
Member

@ghengeveld ghengeveld commented Nov 22, 2025

What I did

Updated getDocsUrl to ensure it returns a properly versioned URL (using latest) when on a canary version. Added unit tests to verify.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Make 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/core team 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

Release Notes

  • Bug Fixes

    • Improved documentation URL handling for canary and preview versions, ensuring users are directed to the correct documentation resources.
  • Tests

    • Added test coverage for canary version scenarios to ensure reliability of documentation URL generation.

✏️ Tip: You can customize this high-level summary in your review settings.

versionDiff === 'patch' ||
versionDiff === null ||
// assume latest version when current version is a 0.0.0 canary
semver.satisfies(current.version, '0.0.0', { includePrerelease: true });
Copy link
Member Author

Choose a reason for hiding this comment

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

This didn't actually match on a 0.0.0-canary.0. A startsWith('0.0.0') match is actually good enough for our needs here.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 22, 2025

📝 Walkthrough

Walkthrough

The getDocsUrl function refactors version selection to introduce activeVersion with special canary handling (using latest when current is 0.0.0), simplifies the isLatestDocs comparison logic, and centralizes version references. Test coverage is extended with canary scenarios.

Changes

Cohort / File(s) Summary
Core version handling refactoring
code/core/src/manager-api/modules/versions.ts
Introduces activeVersion derived from current and latest versions with canary special handling. Refactors URL construction and isLatestDocs logic to use activeVersion instead of current.version directly. Simplifies version comparison to check only "patch" or null semver diff.
Canary test coverage
code/core/src/manager-api/tests/versions.test.js
Adds canary test data and two new test cases covering getDocsUrl behavior when current version is canary (0.0.0), validating both versioned and asset URL construction paths.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • The activeVersion derivation logic and canary version detection require careful validation against expected behavior
  • The simplified isLatestDocs condition changes comparison semantics — verify that removing prerelease/canary considerations from this branch is intentional and doesn't affect other version types
  • New test cases should be reviewed for completeness in covering canary edge cases

Possibly related PRs

  • storybookjs/storybook#33111: Modifies the same getDocsUrl function to change signature/options (asset subpath and ref query param handling), creating a direct code-level dependency that may require coordination
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c04d322 and 4ecca3d.

📒 Files selected for processing (2)
  • code/core/src/manager-api/modules/versions.ts (1 hunks)
  • code/core/src/manager-api/tests/versions.test.js (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
code/core/src/manager-api/tests/versions.test.js (1)
code/core/src/manager-api/modules/versions.ts (1)
  • init (74-190)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: trigger-circle-ci-workflow
  • GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (5)
code/core/src/manager-api/modules/versions.ts (2)

105-109: LGTM! Elegant canary version handling.

The activeVersion logic correctly handles canary versions by falling back to latestVersion when the current version starts with 0.0.0. The boolean short-circuit logic is concise and handles all edge cases appropriately (undefined current/latest versions).


113-121: LGTM! Simplified and correct version logic.

The URL construction now correctly uses activeVersion throughout, ensuring canary versions get the latest documentation. The simplified isLatestDocs check (patch or null diff only) is cleaner and works correctly for all scenarios including canary.

code/core/src/manager-api/tests/versions.test.js (3)

67-70: LGTM! Well-defined canary test data.

The canary test fixture follows the established pattern and provides appropriate test data for verifying canary version handling.


193-202: LGTM! Correct test for canary versioned URL.

The test correctly verifies that canary versions return the latest docs URL without a version segment, matching the expected behavior where activeVersion equals latestVersion for canary builds.


269-280: LGTM! Correct test for canary asset URL.

The test correctly verifies that canary versions use the latest version (7.6) for asset paths, ensuring canary builds reference the appropriate documentation assets.


Comment @coderabbitai help to get the list of available commands and usage tips.

@nx-cloud
Copy link

nx-cloud bot commented Nov 22, 2025

View your CI Pipeline Execution ↗ for commit bdb6da0

Command Status Duration Result
nx run-many -t build --parallel=3 ✅ Succeeded 46s View ↗

☁️ Nx Cloud last updated this comment at 2025-11-23 23:39:40 UTC

@ghengeveld ghengeveld requested a review from kylegach November 22, 2025 16:06
@ghengeveld ghengeveld merged commit cf8beb1 into next Nov 24, 2025
66 checks passed
@ghengeveld ghengeveld deleted the fix-canary-urls branch November 24, 2025 12:12
@github-actions github-actions bot mentioned this pull request Nov 24, 2025
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants