Manifests and MCP: Add e2e tests#33514
Conversation
JReinhold
added
build
|
View your CI Pipeline Execution ↗ for commit f4baa04
☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit 8430d78 ☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughAdds a Playwright end-to-end test for Storybook MCP integration, updates sandbox templates to include Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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)
🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
code/e2e-tests/addon-mcp.spec.ts (5)
13-46: Consider adding context to JSON parse errors.The helper is well-structured, but if
JSON.parsefails on line 45, the error message won't indicate which request failed. This could make debugging flaky tests harder.🔧 Suggested improvement
const dataMatch = text.match(/^data: (.+)$/m); if (!dataMatch) { throw new Error(`Invalid SSE response format: ${text}`); } - return JSON.parse(dataMatch[1]); + try { + return JSON.parse(dataMatch[1]); + } catch (e) { + throw new Error(`Failed to parse JSON from MCP response for method '${method}': ${dataMatch[1]}`); + } }
56-65: Add response status validation before parsing JSON.If the manifest endpoint returns an error (e.g., 404 or 500),
response.json()may throw a confusing error or return unexpected content. Validatingresponse.ok()first provides clearer test failure messages.🔧 Suggested fix
test('should have valid components.json structure', async ({ request }) => { const response = await request.get(`${storybookUrl}/manifests/components.json`); + expect(response.ok()).toBe(true); const json = await response.json();Apply the same pattern to the other manifest fetch calls at lines 68, 94, and 105.
131-147: Info Page tests are coupled to CSS class names.The selectors like
.toolset-status.enabledand.toolsetare implementation details of the MCP info page. If the addon's UI changes, these tests will break. Consider using data-testid attributes if you control the addon's HTML output, or document this coupling.
181-193: Tool count assertion may be brittle.The assertion
expect(response.result.tools).toHaveLength(4)will fail if tools are added or removed from the addon. Consider whether this should verify a minimum count or just check that expected tools are present.🔧 Alternative approach
expect(response.result).toHaveProperty('tools'); - // Dev and docs tools should be present (4 total) - expect(response.result.tools).toHaveLength(4); + // Dev and docs tools should be present (at least 4) + expect(response.result.tools.length).toBeGreaterThanOrEqual(4);Or remove the length check entirely since the subsequent tool name assertions already verify the expected tools exist.
196-217: Assertion for get-story-urls response could be stronger.The test only verifies that
content[0]has atextproperty (line 215), but doesn't validate the actual content. Since the comment on line 214 mentions it "Should contain either a valid URL or an error message", consider asserting on the expected format.🔧 Suggested improvement
// Should contain either a valid URL or an error message about the story expect(response.result.content[0]).toHaveProperty('text'); + // Verify the response contains a URL or meaningful content + const text = response.result.content[0].text; + expect(typeof text).toBe('string'); + expect(text.length).toBeGreaterThan(0); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/e2e-tests/addon-mcp.spec.tscode/lib/cli-storybook/src/sandbox-templates.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{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/lib/cli-storybook/src/sandbox-templates.tscode/e2e-tests/addon-mcp.spec.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/lib/cli-storybook/src/sandbox-templates.tscode/e2e-tests/addon-mcp.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/lib/cli-storybook/src/sandbox-templates.tscode/e2e-tests/addon-mcp.spec.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/lib/cli-storybook/src/sandbox-templates.tscode/e2e-tests/addon-mcp.spec.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/lib/cli-storybook/src/sandbox-templates.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/e2e-tests/addon-mcp.spec.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/e2e-tests/addon-mcp.spec.ts
🧠 Learnings (7)
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/lib/cli-storybook/src/sandbox-templates.ts
📚 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/lib/cli-storybook/src/sandbox-templates.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/lib/cli-storybook/src/sandbox-templates.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/lib/cli-storybook/src/sandbox-templates.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/e2e-tests/addon-mcp.spec.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/e2e-tests/addon-mcp.spec.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} : Cover all branches, conditions, edge cases, error paths, and different input variations in unit tests
Applied to files:
code/e2e-tests/addon-mcp.spec.ts
⏰ 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). (5)
- GitHub Check: normal
- GitHub Check: nx
- GitHub Check: nx
- GitHub Check: nx
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (3)
code/lib/cli-storybook/src/sandbox-templates.ts (1)
372-378: LGTM! Template configuration for MCP addon looks correct.The pattern of adding both the dependency in
extraDependenciesand registering viaeditAddonsis consistent with howinternal/react18-webpack-babelhandles addon modifications. TheexperimentalComponentsManifestfeature flag is required for the component manifest endpoint that the e2e tests will validate.code/e2e-tests/addon-mcp.spec.ts (2)
1-10: Setup and configuration looks good.The environment variable handling with sensible defaults allows the tests to run both locally and in CI with different configurations.
252-271: Good coverage of MCP tool functionality.The
get-documentationtest validates the tool returns component information including name, id, and stories. The assertions are specific enough to catch regressions while allowing for format flexibility.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 49 | 49 | 0 |
| Self size | 20.30 MB | 20.27 MB | 🎉 -27 KB 🎉 |
| Dependency size | 16.52 MB | 16.52 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 183 | 183 | 0 |
| Self size | 775 KB | 775 KB | 🚨 +221 B 🚨 |
| Dependency size | 67.38 MB | 67.35 MB | 🎉 -27 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 176 | 176 | 0 |
| Self size | 30 KB | 30 KB | 🚨 +36 B 🚨 |
| Dependency size | 65.95 MB | 65.92 MB | 🎉 -27 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 999 KB | 999 KB | 🎉 -179 B 🎉 |
| Dependency size | 36.82 MB | 36.79 MB | 🎉 -27 KB 🎉 |
| Bundle Size Analyzer | node | node |
2 participants
What I did
This PR installs
addon-mcpto thereact-vite/default-tssandbox, and adds E2E tests for it:/manifests/components.jsonworks/manifests/docs.jsonworks/mcpshows correct status of enabled toolsetsThese tests already exists in the mcp repo both in unit and e2e form, however we have had some occurences now of MCP functionality breaking as a result of core changes, and this should catch that going forward.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
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
Chores
✏️ Tip: You can customize this high-level summary in your review settings.