Skip to content

Conversation

@yannbf
Copy link
Member

@yannbf yannbf commented Jan 7, 2026

Closes #

What I did

This PR introduces improved detection of known packages for the metadata, so that we have a better understanding of projects and can use the data to improve Storybook.

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

  1. Run Storybook
  2. Access /project.json
  3. Assess the information

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

  • New Features

    • Enhanced telemetry to analyze and categorize project dependencies by type (testing, styling, state management, data fetching, UI, internationalization, and routing).
  • Tests

    • Added comprehensive test coverage for ecosystem package analysis functionality.

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

@yannbf yannbf self-assigned this Jan 7, 2026
@yannbf yannbf added maintenance User-facing maintenance tasks telemetry ci:normal labels Jan 7, 2026
@nx-cloud
Copy link

nx-cloud bot commented Jan 7, 2026

View your CI Pipeline Execution ↗ for commit 47e3dac

Command Status Duration Result
nx run-many -t compile,check,knip,test,pretty-d... ✅ Succeeded 11m 26s View ↗

☁️ Nx Cloud last updated this comment at 2026-01-07 19:29:48 UTC

@nx-cloud
Copy link

nx-cloud bot commented Jan 7, 2026

View your CI Pipeline Execution ↗ for commit f5e4f6d


☁️ Nx Cloud last updated this comment at 2026-01-07 14:20:57 UTC

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 7, 2026

📝 Walkthrough

Walkthrough

Adds a new telemetry module that analyzes a project's dependencies into seven known package groups and resolves test-package versions; integrates the analysis into storybook metadata via a new knownPackages field and updates related tests and types.

Changes

Cohort / File(s) Summary
New Package Analysis Module
code/core/src/telemetry/get-known-packages.ts
Adds analyzeEcosystemPackages(packageJson: PackageJson): Promise<KnownPackagesList> which merges deps/devDeps/peerDeps, categorizes exact-match packages into seven groups, and asynchronously resolves test package versions via getActualPackageVersion. Omits empty groups from results.
Package Analysis Tests
code/core/src/telemetry/get-known-packages.test.ts
New tests for analyzeEcosystemPackages and related helpers, mocking getActualPackageVersion/package-json to validate grouping, scoped names/subpaths handling, mixed groups, invalid/empty spec handling, and prerelease/null behaviors.
Telemetry Metadata Integration
code/core/src/telemetry/storybook-metadata.ts, code/core/src/telemetry/storybook-metadata.test.ts
Replaces prior test-package extraction with metadata.knownPackages = await analyzeEcosystemPackages(packageJson); test updated to mock and assert the new knownPackages field is present and structured.
Type Definitions
code/core/src/telemetry/types.ts
Replaces `testPackages?: Record<string,string

Sequence Diagram(s)

sequenceDiagram
    autonumber
    actor Caller
    participant Analyzer as analyzeEcosystemPackages
    participant PackageJson as PackageJson (input)
    participant Resolver as getActualPackageVersion

    Caller->>Analyzer: analyzeEcosystemPackages(packageJson)
    rect rgb(240,248,255)
      Note over Analyzer,PackageJson: Merge dependencies
      Analyzer->>PackageJson: read dependencies, devDependencies, peerDependencies
    end

    rect rgb(240,245,250)
      Note over Analyzer,Resolver: Resolve test package versions (async)
      loop for each matching TEST_PACKAGES
        Analyzer->>Resolver: request actual package version
        Resolver-->>Analyzer: version or undefined
      end
    end

    rect rgb(245,240,250)
      Note over Analyzer: Map other groups using declared specifiers\n(styling, state, data-fetching, UI, i18n, router)
    end

    Analyzer-->>Caller: return KnownPackagesList (only non-empty groups)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

✨ Finishing touches
  • 📝 Generate docstrings

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @code/core/src/telemetry/get-known-packages.ts:
- Line 144: The list currently contains 'radix-ui' which only matches the bare
package name; update detection so scoped Radix packages are recognized by either
adding explicit scoped entries like '@radix-ui/*' (or common ones such as
'@radix-ui/react-dialog', '@radix-ui/react-popover') to the known packages list
or by altering the matching logic inside pickMatches to support prefix/scoped
matches for '@radix-ui/' package names.
🧹 Nitpick comments (3)
code/core/src/telemetry/get-known-packages.ts (2)

212-216: Matching strategy differs between test packages and other groups.

The TODO comment at line 213 acknowledges this, but it's worth noting: test packages use includes matching (line 229), which captures scoped packages like @testing-library/react via the testing-library entry. However, other groups use exact matching (line 216), so @emotion/react only matches because it's explicitly listed.

This means if you add a new scoped package to non-test groups without the exact name, it won't be detected. Consider streamlining the approach or documenting this behavior clearly.

Also applies to: 225-229


246-256: Use shorthand property syntax.

The return object uses redundant property assignments like { testPackages: testPackages }.

♻️ Simplify with shorthand syntax
   return {
-    ...(testPackages && { testPackages: testPackages }),
-    ...(stylingPackages && { stylingPackages: stylingPackages }),
-    ...(stateManagementPackages && {
-      stateManagementPackages: stateManagementPackages,
-    }),
-    ...(dataFetchingPackages && { dataFetchingPackages: dataFetchingPackages }),
-    ...(uiLibraryPackages && { uiLibraryPackages: uiLibraryPackages }),
-    ...(i18nPackages && { i18nPackages: i18nPackages }),
-    ...(routerPackages && { routerPackages: routerPackages }),
+    ...(testPackages && { testPackages }),
+    ...(stylingPackages && { stylingPackages }),
+    ...(stateManagementPackages && { stateManagementPackages }),
+    ...(dataFetchingPackages && { dataFetchingPackages }),
+    ...(uiLibraryPackages && { uiLibraryPackages }),
+    ...(i18nPackages && { i18nPackages }),
+    ...(routerPackages && { routerPackages }),
   };
code/core/src/telemetry/get-known-packages.test.ts (1)

40-109: Test coverage is good, consider adding edge case for undefined dependency fields.

The tests cover the main scenarios well. One edge case worth considering: what happens when packageJson has undefined or missing dependency fields (e.g., { name: 'pkg' } without any dependencies)?

The implementation handles this via optional chaining (packageJson?.dependencies), but a test would document this behavior.

💡 Optional test for empty/missing dependencies
it('should handle packageJson with no dependency fields', async () => {
  const packageJson: PackageJson = {
    name: 'empty-project',
    version: '1.0.0',
  };

  const result = await analyzeEcosystemPackages(packageJson);

  expect(result).toEqual({});
});
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 13b7cff and f5e4f6d.

📒 Files selected for processing (5)
  • code/core/src/telemetry/get-known-packages.test.ts
  • code/core/src/telemetry/get-known-packages.ts
  • code/core/src/telemetry/storybook-metadata.test.ts
  • code/core/src/telemetry/storybook-metadata.ts
  • code/core/src/telemetry/types.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Format code using Prettier with yarn prettier --write <file>

Files:

  • code/core/src/telemetry/types.ts
  • code/core/src/telemetry/get-known-packages.test.ts
  • code/core/src/telemetry/get-known-packages.ts
  • code/core/src/telemetry/storybook-metadata.test.ts
  • code/core/src/telemetry/storybook-metadata.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Run ESLint checks using yarn lint:js:cmd <file> or the full command cross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directives to fix linting errors before committing

Files:

  • code/core/src/telemetry/types.ts
  • code/core/src/telemetry/get-known-packages.test.ts
  • code/core/src/telemetry/get-known-packages.ts
  • code/core/src/telemetry/storybook-metadata.test.ts
  • code/core/src/telemetry/storybook-metadata.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Enable TypeScript strict mode across all packages

Files:

  • code/core/src/telemetry/types.ts
  • code/core/src/telemetry/get-known-packages.test.ts
  • code/core/src/telemetry/get-known-packages.ts
  • code/core/src/telemetry/storybook-metadata.test.ts
  • code/core/src/telemetry/storybook-metadata.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size

Files:

  • code/core/src/telemetry/types.ts
  • code/core/src/telemetry/get-known-packages.test.ts
  • code/core/src/telemetry/get-known-packages.ts
  • code/core/src/telemetry/storybook-metadata.test.ts
  • code/core/src/telemetry/storybook-metadata.ts
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use logger from storybook/internal/node-logger for server-side logging in Node.js code

Files:

  • code/core/src/telemetry/types.ts
  • code/core/src/telemetry/get-known-packages.test.ts
  • code/core/src/telemetry/get-known-packages.ts
  • code/core/src/telemetry/storybook-metadata.test.ts
  • code/core/src/telemetry/storybook-metadata.ts
**/*.{test,spec}.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

**/*.{test,spec}.{ts,tsx}: Test files should follow the naming pattern *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Follow the spy mocking rules defined in .cursor/rules/spy-mocking.mdc for consistent mocking patterns with Vitest

Files:

  • code/core/src/telemetry/get-known-packages.test.ts
  • code/core/src/telemetry/storybook-metadata.test.ts
**/*.test.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)

**/*.test.{ts,tsx,js,jsx}: Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Use vi.mocked() to type and access the mocked functions in Vitest tests
Implement mock behaviors in beforeEach blocks in Vitest tests
Mock all required dependencies that the test subject uses
Each mock implementation should return a Promise for async functions in Vitest
Mock implementations should match the expected return type of the original function
Mock all required properties and methods that the test subject uses in Vitest tests
Avoid direct function mocking without vi.mocked() in Vitest tests
Avoid mock implementations outside of beforeEach blocks in Vitest tests
Avoid mocking without the spy: true option in Vitest tests
Avoid inline mock implementations within test cases in Vitest tests
Avoid mocking only a subset of required dependencies in Vitest tests
Mock at the highest level of abstraction needed in Vitest tests
Keep mock implementations simple and focused in Vitest tests
Use type-safe mocking with vi.mocked() in Vitest tests
Document complex mock behaviors in Vitest tests
Group related mocks together in Vitest tests

Files:

  • code/core/src/telemetry/get-known-packages.test.ts
  • code/core/src/telemetry/storybook-metadata.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{test,spec}.{ts,tsx,js,jsx}: Write meaningful unit tests that actually import and call the functions being tested, not just verify syntax patterns
Achieve high test coverage of business logic, aiming for 75%+ coverage of statements/lines
Cover all branches, conditions, edge cases, error paths, and different input variations in unit tests
Use vi.mock() to mock file system, loggers, and other external dependencies in tests

Files:

  • code/core/src/telemetry/get-known-packages.test.ts
  • code/core/src/telemetry/storybook-metadata.test.ts
🧠 Learnings (21)
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies in Vitest tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
  • code/core/src/telemetry/storybook-metadata.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together in Vitest tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
  • code/core/src/telemetry/storybook-metadata.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required dependencies that the test subject uses

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
  • code/core/src/telemetry/storybook-metadata.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors in Vitest tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
  • code/core/src/telemetry/storybook-metadata.test.ts
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Write meaningful unit tests that actually import and call the functions being tested, not just verify syntax patterns

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
  • code/core/src/telemetry/storybook-metadata.test.ts
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Use `vi.mock()` to mock file system, loggers, and other external dependencies in tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
  • code/core/src/telemetry/storybook-metadata.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused in Vitest tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses in Vitest tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
  • code/core/src/telemetry/storybook-metadata.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases in Vitest tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
  • code/core/src/telemetry/storybook-metadata.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.mock()` with the `spy: true` option for all package and file mocks in Vitest tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
  • code/core/src/telemetry/storybook-metadata.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in `beforeEach` blocks in Vitest tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
  • code/core/src/telemetry/storybook-metadata.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of `beforeEach` blocks in Vitest tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
  • code/core/src/telemetry/storybook-metadata.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Each mock implementation should return a Promise for async functions in Vitest

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
  • code/core/src/telemetry/storybook-metadata.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock at the highest level of abstraction needed in Vitest tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
📚 Learning: 2025-11-24T17:49:31.838Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-24T17:49:31.838Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Follow the spy mocking rules defined in `.cursor/rules/spy-mocking.mdc` for consistent mocking patterns with Vitest

Applied to files:

  • code/core/src/telemetry/storybook-metadata.test.ts
📚 Learning: 2025-10-01T15:24:01.060Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32594
File: code/core/src/components/components/Popover/WithPopover.tsx:7-9
Timestamp: 2025-10-01T15:24:01.060Z
Learning: In the Storybook repository, "react-aria-components/patched-dist/*" (e.g., "react-aria-components/patched-dist/Dialog", "react-aria-components/patched-dist/Popover", "react-aria-components/patched-dist/Tooltip") are valid import paths created by a patch applied to the react-aria-components package. These imports should not be flagged as broken or invalid until a maintainer explicitly states they are no longer acceptable.

Applied to files:

  • code/core/src/telemetry/storybook-metadata.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.mocked()` to type and access the mocked functions in Vitest tests

Applied to files:

  • code/core/src/telemetry/storybook-metadata.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use type-safe mocking with `vi.mocked()` in Vitest tests

Applied to files:

  • code/core/src/telemetry/storybook-metadata.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid direct function mocking without `vi.mocked()` in Vitest tests

Applied to files:

  • code/core/src/telemetry/storybook-metadata.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking without the `spy: true` option in Vitest tests

Applied to files:

  • code/core/src/telemetry/storybook-metadata.test.ts
📚 Learning: 2025-09-17T07:31:04.432Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32484
File: code/core/package.json:326-326
Timestamp: 2025-09-17T07:31:04.432Z
Learning: In Storybook's core package, dependencies like `open` are bundled into the final distribution during the build process, so they should remain in devDependencies rather than being moved to dependencies. End users don't need these packages as separate runtime dependencies since they're included in the bundled code.

Applied to files:

  • code/core/src/telemetry/storybook-metadata.test.ts
  • code/core/src/telemetry/storybook-metadata.ts
🧬 Code graph analysis (4)
code/core/src/telemetry/get-known-packages.test.ts (3)
code/core/src/common/js-package-manager/JsPackageManager.ts (1)
  • packageJson (790-792)
code/core/src/types/modules/core-common.ts (1)
  • PackageJson (159-159)
code/core/src/telemetry/get-known-packages.ts (1)
  • analyzeEcosystemPackages (203-257)
code/core/src/telemetry/get-known-packages.ts (2)
code/core/src/common/js-package-manager/JsPackageManager.ts (1)
  • packageJson (790-792)
scripts/dangerfile.js (1)
  • pkg (30-30)
code/core/src/telemetry/storybook-metadata.test.ts (4)
code/core/src/telemetry/get-portable-stories-usage.ts (1)
  • getPortableStoriesFileCount (18-22)
code/core/src/telemetry/get-application-file-count.ts (1)
  • getApplicationFileCount (27-31)
code/core/src/telemetry/get-known-packages.ts (1)
  • analyzeEcosystemPackages (203-257)
code/core/src/telemetry/storybook-metadata.ts (1)
  • computeStorybookMetadata (57-226)
code/core/src/telemetry/storybook-metadata.ts (2)
code/core/src/telemetry/get-known-packages.ts (1)
  • analyzeEcosystemPackages (203-257)
code/core/src/common/js-package-manager/JsPackageManager.ts (1)
  • packageJson (790-792)
⏰ 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). (6)
  • GitHub Check: normal
  • GitHub Check: nx
  • GitHub Check: Core Unit Tests, windows-latest
  • GitHub Check: nx
  • GitHub Check: nx
  • GitHub Check: nx
🔇 Additional comments (8)
code/core/src/telemetry/storybook-metadata.ts (1)

25-25: LGTM!

Clean integration of analyzeEcosystemPackages. The import and usage are straightforward, and the function handles empty results gracefully by returning an empty object, which aligns with the optional nature of knownPackages in the type definition.

Also applies to: 95-95

code/core/src/telemetry/get-known-packages.test.ts (1)

1-16: LGTM!

The test setup follows the coding guidelines correctly:

  • Uses vi.mock(import(...), { spy: true }) pattern
  • Implements mock behavior in beforeEach block
  • Uses vi.mocked() to access the mocked function
code/core/src/telemetry/storybook-metadata.test.ts (3)

18-22: LGTM!

The new imports and mocks are properly structured following the coding guidelines:

  • Uses vi.mock(import(...), { spy: true }) pattern
  • Mocks are grouped together at the top of the file

Also applies to: 33-37


80-99: LGTM!

Mock implementations are correctly placed in the beforeEach block and use vi.mocked() for type-safe access. The mock for analyzeEcosystemPackages returns a comprehensive sample covering all package groups.


504-548: LGTM!

The test verifies the integration of knownPackages in the metadata output. The inline snapshot correctly reflects the mocked return value, confirming that computeStorybookMetadata properly includes the result from analyzeEcosystemPackages.

code/core/src/telemetry/types.ts (1)

79-87: Type definition correctly models the nested ecosystem packages structure.

The type aligns with the implementation in get-known-packages.ts and all telemetry code already uses the nested structure. The verification found no dangling references to an older pattern, indicating this is either a fresh enhancement or all consumers have already been updated.

code/core/src/telemetry/get-known-packages.ts (2)

189-190: No changes needed. react-easy-router is a legitimate router package with 8.3 million weekly downloads and fits appropriately in the router packages list.


152-152: No changes needed — shadcn is a valid package for detection.

The shadcn package exists on npm (latest: 3.6.3) and is the correct entry for detecting shadcn/ui usage. While an older shadcn-ui package (v0.9.5) also exists, the modern implementation is distributed under the shadcn package name. The entry is appropriate for telemetry purposes.

// 1,4m
'antd',
// 800k
'radix-ui',
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if radix-ui package exists on npm and get details
echo "=== Checking radix-ui package ==="
curl -s "https://registry.npmjs.org/radix-ui" | jq '{name: .name, description: .description, versions: (.versions | keys | length)}' 2>/dev/null || echo "Failed to fetch radix-ui"

echo ""
echo "=== Checking @radix-ui/react-dialog (example scoped package) ==="
curl -s "https://registry.npmjs.org/@radix-ui/react-dialog" | jq '{name: .name, description: .description}' 2>/dev/null || echo "Failed to fetch @radix-ui/react-dialog"

echo ""
echo "=== Checking @radix-ui scope packages on npm ==="
curl -s "https://registry.npmjs.org/-/v1/search?text=scope:radix-ui&size=5" | jq '.objects[].package.name' 2>/dev/null || echo "Failed to search scoped packages"

Repository: storybookjs/storybook

Length of output: 645


🏁 Script executed:

cat code/core/src/telemetry/get-known-packages.ts

Repository: storybookjs/storybook

Length of output: 5606


Consider the matching strategy for radix-ui and scoped packages.

While radix-ui is a valid package on npm, the exact matching in pickMatches will only detect the bare radix-ui package, not the more commonly used scoped packages like @radix-ui/react-dialog. If the intention is to detect Radix UI usage more broadly, either explicitly list relevant @radix-ui/* packages (as done with other scoped packages like @mui/material), or update the matching strategy (which is already flagged in the TODO comment above pickMatches).

🤖 Prompt for AI Agents
In @code/core/src/telemetry/get-known-packages.ts at line 144, The list
currently contains 'radix-ui' which only matches the bare package name; update
detection so scoped Radix packages are recognized by either adding explicit
scoped entries like '@radix-ui/*' (or common ones such as
'@radix-ui/react-dialog', '@radix-ui/react-popover') to the known packages list
or by altering the matching logic inside pickMatches to support prefix/scoped
matches for '@radix-ui/' package names.

@ndelangen
Copy link
Member

@yannbf Can you elaborate what "Assess the information" entails in your manual steps to reproduce.
I think a QA person possibly wouldn't have enough info to assess if the data presented would be correct.

routerPackages?: PackageGroupResult;
};

const TEST_PACKAGES = [
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend moving the constants to a new file, so this file can focus on the technical implementation.

Comment on lines +204 to +208
const allDependencies = {
...packageJson?.dependencies,
...packageJson?.devDependencies,
...packageJson?.peerDependencies,
};
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest that in monorepo situation there might be multiple package.json file you would want to check.

As the storybook can (and is likely to be) reference stories in other packages within the monorepo.

Copy link
Member

Choose a reason for hiding this comment

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

You could combine all of those package.json files into a single data-structure, since you do not seem to do anything with the versions string of the packages, but are only using the keys?

Copy link
Member Author

Choose a reason for hiding this comment

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

The function I wrote is just a small utility on something we already use in the Storybook metadata function. Extracting multiple package.json files and merging them would be a whole other thing I'd say

import { getApplicationFileCount } from './get-application-file-count';
import { getChromaticVersionSpecifier } from './get-chromatic-version';
import { getFrameworkInfo } from './get-framework-info';
import { getHasRouterPackage } from './get-has-router-package';
Copy link
Member

Choose a reason for hiding this comment

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

Did this already exist? should it not be related to the get-known-packages's router packages?
https://github.com/storybookjs/storybook/pull/33477/changes#diff-36e49b33f702c4ce03998bd8012a939a3c09226a6c4c6af03ac32b87e47b87c3R185-R200

Copy link
Member Author

Choose a reason for hiding this comment

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

sort of, I thought of removing that and just use !!knownPackages.routerPackages but the logic in get-has-router- detects the existence of metaframeworks which would affect the data we're looking for

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @code/core/src/telemetry/get-known-packages.ts:
- Line 7: The type alias PackageGroupResult is declared as Record<string, string
| undefined> but getSafeVersionSpecifier returns string | null, causing a
mismatch; update PackageGroupResult to use string | null (i.e., Record<string,
string | null>) or otherwise align the return type of getSafeVersionSpecifier to
string | undefined so both match; locate the declaration of PackageGroupResult
and the getSafeVersionSpecifier function and make their nullable type consistent
across the module.
🧹 Nitpick comments (3)
code/core/src/telemetry/get-known-packages.ts (3)

19-202: Consider moving constants to a separate file.

As previously suggested in past review comments, extracting these constants to a dedicated file (e.g., known-packages-constants.ts) would improve maintainability and allow this file to focus on the technical implementation logic.

Based on past review feedback.


214-221: Consider monorepo scenarios for package.json aggregation.

As noted in past review comments, in monorepo setups, Storybook may reference stories from multiple packages, each with their own package.json. The current implementation only analyzes a single package.json. Consider whether aggregating dependencies from multiple package.json files would provide more accurate telemetry data.

Based on past review feedback regarding monorepo situations.


267-277: Return statement is verbose but explicit.

The return statement uses conditional spreads to omit null values. While verbose, it's explicit and clear. For a minor improvement, you could simplify with a helper function or use Object.entries with filtering.

♻️ Optional simplification
return Object.fromEntries(
  Object.entries({
    testPackages,
    stylingPackages,
    stateManagementPackages,
    dataFetchingPackages,
    uiLibraryPackages,
    i18nPackages,
    routerPackages,
  }).filter(([_, value]) => value !== null)
);
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f5e4f6d and 47e3dac.

📒 Files selected for processing (3)
  • code/core/src/telemetry/get-known-packages.test.ts
  • code/core/src/telemetry/get-known-packages.ts
  • code/core/src/telemetry/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • code/core/src/telemetry/types.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Format code using Prettier with yarn prettier --write <file>

Files:

  • code/core/src/telemetry/get-known-packages.ts
  • code/core/src/telemetry/get-known-packages.test.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Run ESLint checks using yarn lint:js:cmd <file> or the full command cross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directives to fix linting errors before committing

Files:

  • code/core/src/telemetry/get-known-packages.ts
  • code/core/src/telemetry/get-known-packages.test.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Enable TypeScript strict mode across all packages

Files:

  • code/core/src/telemetry/get-known-packages.ts
  • code/core/src/telemetry/get-known-packages.test.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size

Files:

  • code/core/src/telemetry/get-known-packages.ts
  • code/core/src/telemetry/get-known-packages.test.ts
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use logger from storybook/internal/node-logger for server-side logging in Node.js code

Files:

  • code/core/src/telemetry/get-known-packages.ts
  • code/core/src/telemetry/get-known-packages.test.ts
**/*.{test,spec}.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

**/*.{test,spec}.{ts,tsx}: Test files should follow the naming pattern *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Follow the spy mocking rules defined in .cursor/rules/spy-mocking.mdc for consistent mocking patterns with Vitest

Files:

  • code/core/src/telemetry/get-known-packages.test.ts
**/*.test.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)

**/*.test.{ts,tsx,js,jsx}: Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Use vi.mocked() to type and access the mocked functions in Vitest tests
Implement mock behaviors in beforeEach blocks in Vitest tests
Mock all required dependencies that the test subject uses
Each mock implementation should return a Promise for async functions in Vitest
Mock implementations should match the expected return type of the original function
Mock all required properties and methods that the test subject uses in Vitest tests
Avoid direct function mocking without vi.mocked() in Vitest tests
Avoid mock implementations outside of beforeEach blocks in Vitest tests
Avoid mocking without the spy: true option in Vitest tests
Avoid inline mock implementations within test cases in Vitest tests
Avoid mocking only a subset of required dependencies in Vitest tests
Mock at the highest level of abstraction needed in Vitest tests
Keep mock implementations simple and focused in Vitest tests
Use type-safe mocking with vi.mocked() in Vitest tests
Document complex mock behaviors in Vitest tests
Group related mocks together in Vitest tests

Files:

  • code/core/src/telemetry/get-known-packages.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{test,spec}.{ts,tsx,js,jsx}: Write meaningful unit tests that actually import and call the functions being tested, not just verify syntax patterns
Achieve high test coverage of business logic, aiming for 75%+ coverage of statements/lines
Cover all branches, conditions, edge cases, error paths, and different input variations in unit tests
Use vi.mock() to mock file system, loggers, and other external dependencies in tests

Files:

  • code/core/src/telemetry/get-known-packages.test.ts
🧠 Learnings (20)
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.

Applied to files:

  • code/core/src/telemetry/get-known-packages.ts
📚 Learning: 2025-10-01T15:24:01.060Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32594
File: code/core/src/components/components/Popover/WithPopover.tsx:7-9
Timestamp: 2025-10-01T15:24:01.060Z
Learning: In the Storybook repository, "react-aria-components/patched-dist/*" (e.g., "react-aria-components/patched-dist/Dialog", "react-aria-components/patched-dist/Popover", "react-aria-components/patched-dist/Tooltip") are valid import paths created by a patch applied to the react-aria-components package. These imports should not be flagged as broken or invalid until a maintainer explicitly states they are no longer acceptable.

Applied to files:

  • code/core/src/telemetry/get-known-packages.ts
📚 Learning: 2025-10-02T09:22:13.215Z
Learnt from: JReinhold
Repo: storybookjs/storybook PR: 32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.215Z
Learning: The Storybook repository uses Yarn v^4 (any 4.x version) as the package manager, configured via .yarnrc.yml and package.json packageManager field. Specific patch versions within v4 can be upgraded as needed.

Applied to files:

  • code/core/src/telemetry/get-known-packages.ts
📚 Learning: 2025-09-29T13:20:23.346Z
Learnt from: mrginglymus
Repo: storybookjs/storybook PR: 32556
File: code/core/package.json:309-313
Timestamp: 2025-09-29T13:20:23.346Z
Learning: The `fast-printf` dependency in Storybook's core package is bundled into the final distribution during the build process, so it should remain in devDependencies rather than being moved to dependencies, following the same pattern as other bundled dependencies like `open`.

Applied to files:

  • code/core/src/telemetry/get-known-packages.ts
📚 Learning: 2025-09-17T07:31:04.432Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32484
File: code/core/package.json:326-326
Timestamp: 2025-09-17T07:31:04.432Z
Learning: In Storybook's core package, dependencies like `open` are bundled into the final distribution during the build process, so they should remain in devDependencies rather than being moved to dependencies. End users don't need these packages as separate runtime dependencies since they're included in the bundled code.

Applied to files:

  • code/core/src/telemetry/get-known-packages.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies in Vitest tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together in Vitest tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required dependencies that the test subject uses

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Write meaningful unit tests that actually import and call the functions being tested, not just verify syntax patterns

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors in Vitest tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused in Vitest tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.mock()` with the `spy: true` option for all package and file mocks in Vitest tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases in Vitest tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Use `vi.mock()` to mock file system, loggers, and other external dependencies in tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses in Vitest tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in `beforeEach` blocks in Vitest tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of `beforeEach` blocks in Vitest tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Each mock implementation should return a Promise for async functions in Vitest

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock at the highest level of abstraction needed in Vitest tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid direct function mocking without `vi.mocked()` in Vitest tests

Applied to files:

  • code/core/src/telemetry/get-known-packages.test.ts
🧬 Code graph analysis (2)
code/core/src/telemetry/get-known-packages.ts (3)
code/core/src/manager-api/version.ts (1)
  • version (1-1)
code/core/src/common/js-package-manager/JsPackageManager.ts (1)
  • packageJson (790-792)
scripts/dangerfile.js (1)
  • pkg (30-30)
code/core/src/telemetry/get-known-packages.test.ts (2)
code/core/src/telemetry/get-known-packages.ts (2)
  • getSafeVersionSpecifier (204-212)
  • analyzeEcosystemPackages (214-278)
code/core/src/types/modules/core-common.ts (1)
  • PackageJson (159-159)
⏰ 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). (3)
  • GitHub Check: normal
  • GitHub Check: Core Unit Tests, windows-latest
  • GitHub Check: nx
🔇 Additional comments (12)
code/core/src/telemetry/get-known-packages.test.ts (7)

8-16: LGTM! Mock setup follows best practices.

The mock setup correctly uses vi.mock() with spy: true, places the mock at the top of the file, uses vi.mocked() for type-safe access, and implements the mock behavior in a beforeEach block. The mock returns a Promise as expected for the async function.

As per coding guidelines, this follows the spy mocking rules for Vitest.


18-29: LGTM! Comprehensive edge case coverage for version specifiers.

The tests cover valid operators (^, ~), plain versions, prerelease versions, invalid specifiers (latest, *, file:), and undefined input. The expected behavior of stripping prerelease tags and returning null for invalid inputs is well-tested.


32-52: LGTM! Test validates actual version resolution for test packages.

The test correctly verifies that getActualPackageVersion is called and that the mocked version ('1.0.0') is used in the result instead of the declared versions in package.json. This aligns with the implementation's special handling of test packages.


54-123: LGTM! Comprehensive multi-group validation.

This test validates the categorization of packages across all seven groups (styling, state management, data fetching, UI libraries, i18n, and routing) within a single package.json. Note that 'emotion' (line 60) is intentionally excluded from the result since only '@emotion/react' and '@emotion/styled' are in the known packages list.


125-146: LGTM! Validates scoped package handling.

The test confirms that scoped packages like '@testing-library/react' and '@emotion/react' are correctly detected and categorized. The test packages use resolved versions ('1.0.0') while other packages use declared versions from package.json.


148-170: LGTM! Validates invalid version handling.

The test confirms that invalid semver specifiers (file paths, 'latest') return null values in the result, maintaining data integrity while still including the package name in the appropriate category.


172-183: LGTM! Validates empty result handling.

The test confirms that when no known packages are found, an empty object is returned rather than an object with null values for each category.

code/core/src/telemetry/get-known-packages.ts (5)

1-6: LGTM! Clean imports and type definitions.

The imports are well-organized, using the proper internal types from 'storybook/internal/types' and necessary dependencies.


204-212: LGTM! Version specifier normalization is well-implemented.

The function correctly extracts range operators (^, ~), coerces versions using semver, and returns null for invalid specifiers. This ensures telemetry data contains only valid version information.


225-229: Matching strategy uses exact equality (by design).

The pickMatches function uses exact package name matching (dep === pkg). The TODO comment indicates this is a design decision point. With exact matching, scoped packages like @radix-ui/react-dialog won't match the bare radix-ui entry (line 145), and need to be explicitly listed.

Note: This design choice is acknowledged in the code comments and affects packages like 'radix-ui' (see separate comment on line 145).


243-258: Test packages use substring matching (inconsistent with other groups).

Test packages use dep.includes(pkg) (line 247) while other package groups use exact matching. The comment on line 246 acknowledges this inconsistency with previous behavior. This means test packages can match substrings (e.g., 'testing-library' matches '@testing-library/react'), but other groups require exact names.

Consider whether this inconsistency should be streamlined in a future update. The current behavior maintains backward compatibility, but may benefit from unified matching logic across all package groups.


191-191: The package 'react-easy-router' is a legitimate routing library and should remain in this list.

This is a real npm package with active maintenance (latest version 2.2.0 published April 2023) and proper repository at https://github.com/yousufkalim/react-easy-router. It's appropriately included in the telemetry tracking of known router packages with approximately 8.3 million weekly downloads. No changes needed.

Likely an incorrect or invalid review comment.


import { getActualPackageVersion } from './package-json';

type PackageGroupResult = Record<string, string | undefined>;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Type mismatch: undefined should be null.

The type PackageGroupResult uses string | undefined, but getSafeVersionSpecifier (line 204) returns string | null. This creates a type inconsistency.

🔧 Proposed fix
-type PackageGroupResult = Record<string, string | undefined>;
+type PackageGroupResult = Record<string, string | null>;
📝 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.

Suggested change
type PackageGroupResult = Record<string, string | undefined>;
type PackageGroupResult = Record<string, string | null>;
🤖 Prompt for AI Agents
In @code/core/src/telemetry/get-known-packages.ts at line 7, The type alias
PackageGroupResult is declared as Record<string, string | undefined> but
getSafeVersionSpecifier returns string | null, causing a mismatch; update
PackageGroupResult to use string | null (i.e., Record<string, string | null>) or
otherwise align the return type of getSafeVersionSpecifier to string | undefined
so both match; locate the declaration of PackageGroupResult and the
getSafeVersionSpecifier function and make their nullable type consistent across
the module.

@yannbf
Copy link
Member Author

yannbf commented Jan 9, 2026

Closing this as it will be done in another PR

@yannbf yannbf closed this Jan 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:normal maintenance User-facing maintenance tasks telemetry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants