-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Core: Improve known packages detection #33477
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
View your CI Pipeline Execution ↗ for commit 47e3dac
☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit f5e4f6d ☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughAdds 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 Changes
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)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
✨ Finishing touches
Comment |
There was a problem hiding this 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
includesmatching (line 229), which captures scoped packages like@testing-library/reactvia thetesting-libraryentry. However, other groups use exact matching (line 216), so@emotion/reactonly 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
packageJsonhasundefinedor 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
📒 Files selected for processing (5)
code/core/src/telemetry/get-known-packages.test.tscode/core/src/telemetry/get-known-packages.tscode/core/src/telemetry/storybook-metadata.test.tscode/core/src/telemetry/storybook-metadata.tscode/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.tscode/core/src/telemetry/get-known-packages.test.tscode/core/src/telemetry/get-known-packages.tscode/core/src/telemetry/storybook-metadata.test.tscode/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 commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/telemetry/types.tscode/core/src/telemetry/get-known-packages.test.tscode/core/src/telemetry/get-known-packages.tscode/core/src/telemetry/storybook-metadata.test.tscode/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.tscode/core/src/telemetry/get-known-packages.test.tscode/core/src/telemetry/get-known-packages.tscode/core/src/telemetry/storybook-metadata.test.tscode/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 useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/telemetry/types.tscode/core/src/telemetry/get-known-packages.test.tscode/core/src/telemetry/get-known-packages.tscode/core/src/telemetry/storybook-metadata.test.tscode/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
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/telemetry/types.tscode/core/src/telemetry/get-known-packages.test.tscode/core/src/telemetry/get-known-packages.tscode/core/src/telemetry/storybook-metadata.test.tscode/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.mdcfor consistent mocking patterns with Vitest
Files:
code/core/src/telemetry/get-known-packages.test.tscode/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}: Usevi.mock()with thespy: trueoption for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Usevi.mocked()to type and access the mocked functions in Vitest tests
Implement mock behaviors inbeforeEachblocks 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 withoutvi.mocked()in Vitest tests
Avoid mock implementations outside ofbeforeEachblocks in Vitest tests
Avoid mocking without thespy: trueoption 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 withvi.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.tscode/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
Usevi.mock()to mock file system, loggers, and other external dependencies in tests
Files:
code/core/src/telemetry/get-known-packages.test.tscode/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.tscode/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.tscode/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.tscode/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.tscode/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.tscode/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.tscode/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.tscode/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.tscode/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.tscode/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.tscode/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.tscode/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.tscode/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.tscode/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 ofknownPackagesin 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
beforeEachblock- Uses
vi.mocked()to access the mocked functioncode/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
beforeEachblock and usevi.mocked()for type-safe access. The mock foranalyzeEcosystemPackagesreturns a comprehensive sample covering all package groups.
504-548: LGTM!The test verifies the integration of
knownPackagesin the metadata output. The inline snapshot correctly reflects the mocked return value, confirming thatcomputeStorybookMetadataproperly includes the result fromanalyzeEcosystemPackages.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.tsand 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-routeris a legitimate router package with 8.3 million weekly downloads and fits appropriately in the router packages list.
152-152: No changes needed —shadcnis a valid package for detection.The
shadcnpackage exists on npm (latest: 3.6.3) and is the correct entry for detecting shadcn/ui usage. While an oldershadcn-uipackage (v0.9.5) also exists, the modern implementation is distributed under theshadcnpackage name. The entry is appropriate for telemetry purposes.
| // 1,4m | ||
| 'antd', | ||
| // 800k | ||
| 'radix-ui', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 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.tsRepository: 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.
|
@yannbf Can you elaborate what "Assess the information" entails in your manual steps to reproduce. |
| routerPackages?: PackageGroupResult; | ||
| }; | ||
|
|
||
| const TEST_PACKAGES = [ |
There was a problem hiding this comment.
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.
| const allDependencies = { | ||
| ...packageJson?.dependencies, | ||
| ...packageJson?.devDependencies, | ||
| ...packageJson?.peerDependencies, | ||
| }; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this 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
📒 Files selected for processing (3)
code/core/src/telemetry/get-known-packages.test.tscode/core/src/telemetry/get-known-packages.tscode/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.tscode/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 commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/telemetry/get-known-packages.tscode/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.tscode/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 useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/telemetry/get-known-packages.tscode/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
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/telemetry/get-known-packages.tscode/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.mdcfor 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}: Usevi.mock()with thespy: trueoption for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Usevi.mocked()to type and access the mocked functions in Vitest tests
Implement mock behaviors inbeforeEachblocks 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 withoutvi.mocked()in Vitest tests
Avoid mock implementations outside ofbeforeEachblocks in Vitest tests
Avoid mocking without thespy: trueoption 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 withvi.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
Usevi.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()withspy: true, places the mock at the top of the file, usesvi.mocked()for type-safe access, and implements the mock behavior in abeforeEachblock. 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
getActualPackageVersionis 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
pickMatchesfunction 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-dialogwon't match the bareradix-uientry (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>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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.
|
Closing this as it will be done in another PR |
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:
Manual testing
/project.jsonDocumentation
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
✏️ Tip: You can customize this high-level summary in your review settings.