Core: Add sitemap.xml handler #33243
Conversation
Sidnioulz
added
feature request
ci:normal
experiment
|
View your CI Pipeline Execution ↗ for commit 48d1d32
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughAdds sitemap generation and serving: new runtime dependency, build-time sitemap extraction, a dev-server Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant DevServer as Storybook Server
participant IndexGen as StoryIndexGenerator
participant SitemapGen as SitemapStream+Gzip
participant Cache as cachedSitemap
Client->>DevServer: GET /sitemap.xml
alt cached sitemap exists
DevServer->>Cache: read cached buffer
Cache-->>DevServer: gzipped buffer
DevServer-->>Client: 200 + application/xml + content-encoding:gzip + buffer
else cached sitemap missing
DevServer->>IndexGen: request story entries
IndexGen-->>DevServer: story list
DevServer->>SitemapGen: build XML & gzip from entries (+ static pages)
SitemapGen-->>DevServer: gzipped buffer
DevServer->>Cache: store gzipped buffer
DevServer-->>Client: 200 + application/xml + content-encoding:gzip + buffer
end
Note over DevServer,IndexGen: File watcher / index updates invalidate cache -> triggers regeneration on next request
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
✨ Finishing touches
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: 1
🧹 Nitpick comments (2)
code/core/src/core-server/utils/stories-json.ts (2)
32-69: Sitemap cache integration with file/config watchers looks solidIntroducing
cachedSitemapin theuseStoriesJsonclosure and clearing it from bothwatchStorySpecifiersandwatchConfigcallbacks keeps the sitemap in sync with story and preview changes without recomputing on every request. If there are other code paths that can invalidate theStoryIndexoutside these watchers, consider also clearingcachedSitemapin those paths (e.g., on a genericSTORY_INDEX_INVALIDATEDlistener) to avoid any chance of stale sitemap content.
83-132: /sitemap.xml implementation correctly streams, compresses, and caches sitemap entriesThe
/sitemap.xmlhandler usesSitemapStream+createGzip()with an in-memory cached buffer, sets appropriateContent-Type/Content-Encodingheaders, includes both static settings URLs and dynamic docs/story URLs (skippingsubtype === 'test'), and safely reuses the cached gzip across requests. As a minor enhancement, you could optionally respectAccept-Encodingand fall back to an uncompressed response for clients that don’t advertise gzip support, though for search engine crawlers this is unlikely to matter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
code/core/package.json(1 hunks)code/core/src/core-server/utils/getStoryIndexGenerator.ts(1 hunks)code/core/src/core-server/utils/stories-json.test.ts(8 hunks)code/core/src/core-server/utils/stories-json.ts(3 hunks)code/e2e-tests/sitemap.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/package.jsoncode/core/src/core-server/utils/getStoryIndexGenerator.tscode/core/src/core-server/utils/stories-json.test.tscode/e2e-tests/sitemap.spec.tscode/core/src/core-server/utils/stories-json.ts
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/package.jsoncode/core/src/core-server/utils/getStoryIndexGenerator.tscode/core/src/core-server/utils/stories-json.test.tscode/e2e-tests/sitemap.spec.tscode/core/src/core-server/utils/stories-json.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/core-server/utils/getStoryIndexGenerator.tscode/core/src/core-server/utils/stories-json.test.tscode/e2e-tests/sitemap.spec.tscode/core/src/core-server/utils/stories-json.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
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/core-server/utils/getStoryIndexGenerator.tscode/core/src/core-server/utils/stories-json.test.tscode/e2e-tests/sitemap.spec.tscode/core/src/core-server/utils/stories-json.ts
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/core-server/utils/getStoryIndexGenerator.tscode/core/src/core-server/utils/stories-json.test.tscode/e2e-tests/sitemap.spec.tscode/core/src/core-server/utils/stories-json.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/core-server/utils/stories-json.test.tscode/e2e-tests/sitemap.spec.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/core-server/utils/stories-json.test.ts
code/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{test,spec}.{ts,tsx,js,jsx}: Write meaningful unit tests that actually import and call the functions being tested
Mock external dependencies using vi.mock() for file system, loggers, and other external dependencies in tests
Aim for high test coverage of business logic (75%+ for statements/lines) using coverage reports
Files:
code/core/src/core-server/utils/stories-json.test.tscode/e2e-tests/sitemap.spec.ts
🧠 Learnings (17)
📚 Learning: 2025-11-28T14:50:24.872Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.872Z
Learning: Applies to code/**/*.{test,spec}.{ts,tsx,js,jsx} : Mock external dependencies using vi.mock() for file system, loggers, and other external dependencies in tests
Applied to files:
code/core/src/core-server/utils/stories-json.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/core-server/utils/stories-json.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 only a subset of required dependencies in Vitest tests
Applied to files:
code/core/src/core-server/utils/stories-json.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/core-server/utils/stories-json.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/core-server/utils/stories-json.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/core-server/utils/stories-json.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/core-server/utils/stories-json.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/core-server/utils/stories-json.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/core-server/utils/stories-json.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/core-server/utils/stories-json.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/core-server/utils/stories-json.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/core-server/utils/stories-json.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/core-server/utils/stories-json.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/core-server/utils/stories-json.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/core-server/utils/stories-json.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/core-server/utils/stories-json.test.ts
📚 Learning: 2025-11-28T14:50:24.872Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.872Z
Learning: Applies to code/**/*.{ts,tsx,js,jsx,mjs} : Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Applied to files:
code/core/src/core-server/utils/stories-json.ts
🧬 Code graph analysis (1)
code/core/src/core-server/utils/stories-json.test.ts (1)
code/core/src/core-server/utils/stories-json.ts (1)
useStoriesJson(32-133)
⏰ 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: Core Unit Tests, windows-latest
- GitHub Check: nx
🔇 Additional comments (4)
code/core/package.json (1)
321-321: sitemap dependency addition aligns with new /sitemap.xml endpoint
stories-json.tsimportsSitemapStreamandstreamToPromisefromsitemap, so adding"sitemap": "^9.0.0"is consistent and in the right place in the manifest. Please confirm this version is compatible with your supported Node/bundler targets and passes your usual dependency/security checks.code/core/src/core-server/utils/getStoryIndexGenerator.ts (1)
35-43: ForwardingoptionsintouseStoriesJsonkeeps server configuration consistentPassing the same
optionsobject used to build theStoryIndexGeneratorintouseStoriesJsonensures the sitemap route can reliably derive host/port/protocol from a single source of truth without additional plumbing.code/core/src/core-server/utils/stories-json.test.ts (1)
2-3: Test setup changes correctly exercise the new sitemap-awareuseStoriesJsonAPIImporting
gunzipSync, introducing a shareddefaultOptions, and passingoptionsinto alluseStoriesJsoncalls keeps the tests explicit about server host/port/protocol and in sync with the added/sitemap.xmlroute (reflected in the updatedusecall counts). This is a clean way to adapt the existing suites to the new API surface.Also applies to: 83-89, 102-117, 481-506, 515-636
code/e2e-tests/sitemap.spec.ts (1)
1-46: End-to-end sitemap.xml test provides good coverage of headers and contentThe Playwright spec verifies status, XML + gzip headers, decompresses the response, and asserts both structural markers (
<urlset>,</urlset>) and representative doc/story/settings URLs along with<lastmod>and<priority>. This gives solid end-to-end confidence that the new/sitemap.xmlendpoint behaves as intended in a real browser environment.
| describe('Sitemap endpoint', () => { | ||
| beforeEach(() => { | ||
| use.mockClear(); | ||
| end.mockClear(); | ||
| }); | ||
|
|
||
| it('generates sitemap with http://localhost', async () => { | ||
| const mockServerChannel = { emit: vi.fn() } as any as ServerChannel; | ||
| useStoriesJson({ | ||
| app, | ||
| serverChannel: mockServerChannel, | ||
| workingDir, | ||
| normalizedStories, | ||
| initializedStoryIndexGenerator: getInitializedStoryIndexGenerator(), | ||
| options: defaultOptions, | ||
| }); | ||
|
|
||
| expect(use).toHaveBeenCalledTimes(2); | ||
| const sitemapRoute = use.mock.calls[1][1]; | ||
|
|
||
| await sitemapRoute(request, response); | ||
|
|
||
| expect(end).toHaveBeenCalledTimes(1); | ||
| expect(response.setHeader).toHaveBeenCalledWith('Content-Type', 'application/xml'); | ||
| expect(response.setHeader).toHaveBeenCalledWith('Content-Encoding', 'gzip'); | ||
|
|
||
| const gzippedBuffer = end.mock.calls[0][0]; | ||
| const decompressed = gunzipSync(gzippedBuffer).toString('utf-8'); | ||
|
|
||
| expect(decompressed).toContain('<?xml version="1.0" encoding="UTF-8"?>'); | ||
| expect(decompressed).toContain('<urlset'); | ||
| expect(decompressed).toContain('http://localhost:6006'); | ||
| expect(decompressed).toContain('/?path=/docs/a--metaof'); | ||
| expect(decompressed).toContain('/?path=/story/a--story-one'); | ||
| expect(decompressed).toContain('/?path=/settings/about'); | ||
| expect(decompressed).toContain('/?path=/settings/whats-new'); | ||
| expect(decompressed).toContain('/?path=/settings/guide'); | ||
| expect(decompressed).toContain('/?path=/settings/shortcuts'); | ||
| }); | ||
|
|
||
| it('generates sitemap with https protocol', async () => { | ||
| const mockServerChannel = { emit: vi.fn() } as any as ServerChannel; | ||
| const httpsOptions = { ...defaultOptions, https: true }; | ||
|
|
||
| useStoriesJson({ | ||
| app, | ||
| serverChannel: mockServerChannel, | ||
| workingDir, | ||
| normalizedStories, | ||
| initializedStoryIndexGenerator: getInitializedStoryIndexGenerator(), | ||
| options: httpsOptions, | ||
| }); | ||
|
|
||
| const sitemapRoute = use.mock.calls[1][1]; | ||
| await sitemapRoute(request, response); | ||
|
|
||
| const gzippedBuffer = end.mock.calls[0][0]; | ||
| const decompressed = gunzipSync(gzippedBuffer).toString('utf-8'); | ||
|
|
||
| expect(decompressed).toContain('https://localhost:6006'); | ||
| expect(decompressed).not.toContain('http://localhost:6006'); | ||
| }); | ||
|
|
||
| it('generates sitemap for production website (example.com)', async () => { | ||
| const mockServerChannel = { emit: vi.fn() } as any as ServerChannel; | ||
| const productionOptions = { | ||
| host: 'example.com', | ||
| port: 443, | ||
| https: true, | ||
| initialPath: '', | ||
| } as any; | ||
|
|
||
| useStoriesJson({ | ||
| app, | ||
| serverChannel: mockServerChannel, | ||
| workingDir, | ||
| normalizedStories, | ||
| initializedStoryIndexGenerator: getInitializedStoryIndexGenerator(), | ||
| options: productionOptions, | ||
| }); | ||
|
|
||
| const sitemapRoute = use.mock.calls[1][1]; | ||
| await sitemapRoute(request, response); | ||
|
|
||
| const gzippedBuffer = end.mock.calls[0][0]; | ||
| const decompressed = gunzipSync(gzippedBuffer).toString('utf-8'); | ||
|
|
||
| expect(decompressed).toContain('https://example.com'); | ||
| expect(decompressed).toContain('/?path=/docs/a--metaof'); | ||
| expect(decompressed).toContain('/?path=/story/a--story-one'); | ||
| }); | ||
|
|
||
| it('uses cached sitemap on subsequent requests', async () => { | ||
| const mockServerChannel = { emit: vi.fn() } as any as ServerChannel; | ||
| useStoriesJson({ | ||
| app, | ||
| serverChannel: mockServerChannel, | ||
| workingDir, | ||
| normalizedStories, | ||
| initializedStoryIndexGenerator: getInitializedStoryIndexGenerator(), | ||
| options: defaultOptions, | ||
| }); | ||
|
|
||
| const sitemapRoute = use.mock.calls[1][1]; | ||
|
|
||
| // First request | ||
| await sitemapRoute(request, response); | ||
| const firstBuffer = end.mock.calls[0][0] as Buffer; | ||
|
|
||
| // Second request | ||
| const secondResponse = { | ||
| ...response, | ||
| end: vi.fn(), | ||
| setHeader: vi.fn(), | ||
| on: vi.fn(), | ||
| }; | ||
| await sitemapRoute(request, secondResponse); | ||
| const secondBuffer = secondResponse.end.mock.calls[0][0] as Buffer; | ||
|
|
||
| // Should return the same cached buffer | ||
| expect(firstBuffer).toBe(secondBuffer); | ||
| }); | ||
|
|
||
| it('excludes test stories from sitemap', async () => { | ||
| const mockServerChannel = { emit: vi.fn() } as any as ServerChannel; | ||
| useStoriesJson({ | ||
| app, | ||
| serverChannel: mockServerChannel, | ||
| workingDir, | ||
| normalizedStories, | ||
| initializedStoryIndexGenerator: getInitializedStoryIndexGenerator(), | ||
| options: defaultOptions, | ||
| }); | ||
|
|
||
| const sitemapRoute = use.mock.calls[1][1]; | ||
| await sitemapRoute(request, response); | ||
|
|
||
| const gzippedBuffer = end.mock.calls[0][0]; | ||
| const decompressed = gunzipSync(gzippedBuffer).toString('utf-8'); | ||
|
|
||
| // Verify test stories are not included | ||
| // Note: If test stories exist in mock data with subtype: 'test', they should not appear | ||
| expect(decompressed).toContain('/?path=/story/a--story-one'); | ||
| expect(decompressed).toContain('/?path=/docs/a--metaof'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Sitemap endpoint tests are strong overall; tighten the “excludes test stories” assertion
The new sitemap tests nicely validate hostname/protocol handling, presence of key paths, and cache reuse. However, in it('excludes test stories from sitemap') you only assert that known non-test doc/story URLs are present; you never assert that any “test” stories are actually missing, and the current mock index may not include entries with subtype: 'test'. To make this test meaningful, either (a) add a fixture entry with subtype: 'test' and assert that its /story/ URL is not present in the decompressed sitemap, or (b) rename the test to reflect that it’s only checking inclusion of specific non-test entries.
🤖 Prompt for AI Agents
In code/core/src/core-server/utils/stories-json.test.ts around lines 639 to 784,
the "excludes test stories from sitemap" test never verifies that a story with
subtype: 'test' is actually excluded; update the test to either add a mock story
entry with subtype: 'test' to normalizedStories (so it would generate a
/?path=/story/... URL if not excluded) and assert that the decompressed sitemap
does NOT contain that test-story path, or change the test name to reflect it
only asserts presence of known non-test entries; preferred fix: add a test-only
fixture story with subtype: 'test' in the normalizedStories passed into
useStoriesJson and add an
expect(decompressed).not.toContain('/?path=/story/<test-story-id>') assertion to
prove exclusion.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 39 | 39 | 0 |
| Self size | 20.52 MB | 20.59 MB | 🚨 +65 KB 🚨 |
| Dependency size | 16.41 MB | 16.41 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 173 | 173 | 0 |
| Self size | 774 KB | 774 KB | 🎉 -68 B 🎉 |
| Dependency size | 67.62 MB | 67.69 MB | 🚨 +64 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 166 | 166 | 0 |
| Self size | 30 KB | 30 KB | 0 B |
| Dependency size | 66.19 MB | 66.26 MB | 🚨 +65 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 40 | 40 | 0 |
| Self size | 1000 KB | 1000 KB | 🎉 -67 B 🎉 |
| Dependency size | 36.93 MB | 36.99 MB | 🚨 +65 KB 🚨 |
| Bundle Size Analyzer | node | node |
|
|
||
| const now = new Date(); | ||
|
|
||
| // static settings pages; those with backlinks to storybook.js.org are the most important. |
|
|
||
| export interface TestBuildConfig { | ||
| /** URL of the deployed website (needed for sitemap computation). */ | ||
| siteUrl?: string; |
There was a problem hiding this comment.
- Find out how to get those build options merged with the SB cli build command
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/core-server/utils/stories-json.ts (1)
101-116: Unusedoptionsparameter inuseStoriesJson.The
options: Optionsparameter was added to the function signature (line 115) but is never used within the function body. Either remove it or use it as intended.export function useStoriesJson({ app, initializedStoryIndexGenerator, workingDir = process.cwd(), configDir, serverChannel, normalizedStories, - options, }: { app: Polka; initializedStoryIndexGenerator: Promise<StoryIndexGenerator>; serverChannel: ServerChannel; workingDir?: string; configDir?: string; normalizedStories: NormalizedStoriesSpecifier[]; - options: Options; }) {
🧹 Nitpick comments (4)
code/core/src/cli/build.ts (1)
12-12: Minor: Redundant fallback toundefined.The
|| undefinedis unnecessary sincecliOptions.siteUrlwill already beundefinedif not provided, and falsy values like empty strings would also becomeundefined(which may or may not be intended behavior).- siteUrl: cliOptions.siteUrl || undefined, + siteUrl: cliOptions.siteUrl,code/core/src/core-server/utils/stories-json.ts (3)
24-60: Consider renamingisProductionparameter for clarity.The parameter name
isProductionis misleading—it actually controls whether to skip gzip compression (true = no gzip for static file output, false = gzip for dev server streaming). A name likeskipGziporforStaticFilewould better convey the intent.async function generateSitemap( initializedStoryIndexGenerator: Promise<StoryIndexGenerator>, networkAddress: string, - isProduction: boolean + skipGzip: boolean ): Promise<Buffer | undefined> { const generator = await initializedStoryIndexGenerator; const index = await generator.getIndex(); const smStream = new SitemapStream({ hostname: networkAddress }); - // const smStream = new SitemapStream(); - const pipeline = isProduction ? smStream : smStream.pipe(createGzip()); + const pipeline = skipGzip ? smStream : smStream.pipe(createGzip());
33-33: Remove commented-out code.Line 33 contains commented-out code that should be removed.
const smStream = new SitemapStream({ hostname: networkAddress }); - // const smStream = new SitemapStream(); const pipeline = isProduction ? smStream : smStream.pipe(createGzip());
22-22: Consider adding a reset function for test isolation.The
cachedSitemapvariable is module-level mutable state. While the cache includes invalidation logic via file watchers (triggered when stories or preview config changes), there is no explicit mechanism for tests to reset the cache between test runs. In test environments with parallel execution or multiple test files, this could lead to stale cache state being shared across tests. Consider exporting a function like__resetCacheForTesting()to allow tests to clear the cache explicitly, or document this cache behavior and its invalidation triggers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
code/core/src/bin/core.ts(2 hunks)code/core/src/cli/build.ts(1 hunks)code/core/src/core-server/build-static.ts(2 hunks)code/core/src/core-server/dev-server.ts(2 hunks)code/core/src/core-server/utils/stories-json.ts(4 hunks)code/core/src/types/modules/core-common.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/src/types/modules/core-common.tscode/core/src/bin/core.tscode/core/src/core-server/dev-server.tscode/core/src/cli/build.tscode/core/src/core-server/build-static.tscode/core/src/core-server/utils/stories-json.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/types/modules/core-common.tscode/core/src/bin/core.tscode/core/src/core-server/dev-server.tscode/core/src/cli/build.tscode/core/src/core-server/build-static.tscode/core/src/core-server/utils/stories-json.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
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/types/modules/core-common.tscode/core/src/bin/core.tscode/core/src/core-server/dev-server.tscode/core/src/cli/build.tscode/core/src/core-server/build-static.tscode/core/src/core-server/utils/stories-json.ts
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/types/modules/core-common.tscode/core/src/bin/core.tscode/core/src/core-server/dev-server.tscode/core/src/cli/build.tscode/core/src/core-server/build-static.tscode/core/src/core-server/utils/stories-json.ts
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/src/types/modules/core-common.tscode/core/src/bin/core.tscode/core/src/core-server/dev-server.tscode/core/src/cli/build.tscode/core/src/core-server/build-static.tscode/core/src/core-server/utils/stories-json.ts
🧠 Learnings (3)
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{ts,tsx,js,jsx,mjs} : Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Applied to files:
code/core/src/core-server/dev-server.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/core/src/core-server/dev-server.ts
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{ts,tsx,js,jsx,mjs} : Use client-side logger from 'storybook/internal/client-logger' for browser code
Applied to files:
code/core/src/core-server/dev-server.ts
🧬 Code graph analysis (1)
code/core/src/core-server/dev-server.ts (2)
code/core/src/core-server/utils/stories-json.ts (1)
useSitemap(150-183)code/core/src/core-server/index.ts (1)
StoryIndexGenerator(11-11)
⏰ 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: nx
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (8)
code/core/src/types/modules/core-common.ts (1)
336-341: LGTM!The
siteUrlproperty addition toTestBuildConfigis well-documented and correctly typed as optional.code/core/src/core-server/dev-server.ts (1)
142-143: LGTM with a note on the type assertion.The placement after the
indexErrorcheck is correct. Since errors fromgetStoryIndexGeneratorsetindexErrorand returnundefined, and we throw onindexError, the generator should be defined at this point. The type assertion is justified.code/core/src/bin/core.ts (2)
156-159: LGTM!The
--site-urlCLI option is clearly documented and follows the existing pattern for build command options.
176-176: LGTM!The environment variable mapping follows the established
SBCONFIG_*naming convention.code/core/src/core-server/build-static.ts (3)
28-28: LGTM!Import addition aligns with the new sitemap extraction capability.
31-36: LGTM!The type extension properly exposes
siteUrlas an optional property for sitemap generation.
169-175: LGTM!Sitemap extraction is correctly added to the effects array for parallel execution. The
extractSitemapfunction handles the missingsiteUrlcase gracefully by logging and returning early.code/core/src/core-server/utils/stories-json.ts (1)
150-183: LGTM!The
useSitemapfunction correctly:
- Sets appropriate headers for gzipped XML
- Serves cached sitemap when available
- Generates and caches the sitemap on first request
- Handles errors with 500 status code
| .option( | ||
| '--site-url <string>', | ||
| 'The URL where the built site will be deployed, for sitemap generation' | ||
| ) |
There was a problem hiding this comment.
@Sidnioulz, minor item here. Don't forget to add the appropriate documentation to reflect this CLI flag, to avoid potential questions/issues, and ensure parity between what's available and what's documented.
There was a problem hiding this comment.
Of course 👍
I will copy my TODO list for this feature into the description to dispel any concern :)
|
Failed to publish canary version of this pull request, triggered by @Sidnioulz. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/20018620346 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
code/core/src/core-server/utils/stories-json.ts (2)
22-22: Module-level mutable state creates potential issues in concurrent/multi-instance scenarios.The
cachedSitemapvariable is module-level mutable state. If multiple Storybook instances run in the same process or if tests run in parallel, they would share this cache, potentially serving stale or incorrect sitemaps.Consider encapsulating the cache within the
useSitemapclosure or passing it as part of the options/context to avoid shared state issues.
33-34: Remove commented-out code and clarify gzip logic.Line 33 contains dead commented-out code. The
isProductionparameter name is misleading—it actually controls whether gzip is disabled (true = no gzip for file output, false = gzip for HTTP response). Consider renaming toskipGziporforFileOutputfor clarity.- // const smStream = new SitemapStream(); - const pipeline = isProduction ? smStream : smStream.pipe(createGzip()); + const pipeline = skipGzip ? smStream : smStream.pipe(createGzip());code/core/src/common/utils/cli.ts (1)
74-80: Variable naming is misleading.The variable
envVarValueactually holds the name of the environment variable, not its value. The value is retrieved on line 80. Consider renaming for clarity.- const envVarValue = envVarNames.find((envVarName) => process.env[envVarName]); - if (envVarValue) { - program[fieldName] = process.env[envVarValue]; + const matchingEnvVar = envVarNames.find((name) => process.env[name]); + if (matchingEnvVar) { + program[fieldName] = process.env[matchingEnvVar]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/core/src/bin/core.ts(2 hunks)code/core/src/common/utils/cli.ts(1 hunks)code/core/src/core-server/utils/stories-json.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/bin/core.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/src/common/utils/cli.tscode/core/src/core-server/utils/stories-json.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/common/utils/cli.tscode/core/src/core-server/utils/stories-json.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
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/common/utils/cli.tscode/core/src/core-server/utils/stories-json.ts
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/common/utils/cli.tscode/core/src/core-server/utils/stories-json.ts
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/src/common/utils/cli.tscode/core/src/core-server/utils/stories-json.ts
🧠 Learnings (1)
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{ts,tsx,js,jsx,mjs} : Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Applied to files:
code/core/src/core-server/utils/stories-json.ts
🧬 Code graph analysis (1)
code/core/src/core-server/utils/stories-json.ts (2)
code/core/src/core-server/build-static.ts (1)
BuildStaticStandaloneOptions(31-36)code/core/src/core-server/utils/watch-story-specifiers.ts (1)
watchStorySpecifiers(42-134)
⏰ 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 (3)
code/core/src/core-server/utils/stories-json.ts (2)
72-108: LGTM!The URL normalization logic correctly handles trailing slashes and protocol detection, including the Vercel edge case. The function appropriately generates uncompressed XML for file output.
175-175: Theportdefault of80is never used in practice. Indev-server.ts,useSitemapis called only after an explicitinvariant(port, 'expected options to have a port')check, which guarantees thatportis always present in theoptionsobject passed to the function. The default value exists as a defensive fallback but poses no actual risk since the function is only called from one location with a validatedportvalue.Likely an incorrect or invalid review comment.
code/core/src/common/utils/cli.ts (1)
69-83: LGTM!The updated
getEnvConfigfunction correctly handles both single environment variable names and arrays, enabling flexible multi-provider environment variable resolution (e.g., for site URL across Vercel, Netlify, etc.).
| workingDir?: string; | ||
| configDir?: string; | ||
| normalizedStories: NormalizedStoriesSpecifier[]; | ||
| options: Options; |
There was a problem hiding this comment.
Unused options parameter in function signature.
The options: Options parameter is declared in the destructured parameters but is never used within the useStoriesJson function body. Either remove the unused parameter or use it if intended.
}: {
app: Polka;
initializedStoryIndexGenerator: Promise<StoryIndexGenerator>;
serverChannel: ServerChannel;
workingDir?: string;
configDir?: string;
normalizedStories: NormalizedStoriesSpecifier[];
- options: Options;
}) {📝 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.
| options: Options; | |
| }: { | |
| app: Polka; | |
| initializedStoryIndexGenerator: Promise<StoryIndexGenerator>; | |
| serverChannel: ServerChannel; | |
| workingDir?: string; | |
| configDir?: string; | |
| normalizedStories: NormalizedStoriesSpecifier[]; | |
| }) { |
🤖 Prompt for AI Agents
In code/core/src/core-server/utils/stories-json.ts around line 124, the
destructured function signature includes an unused parameter `options: Options`;
remove `options` from the parameter list (and any related type annotation) if it
is not needed, or if it was intended to influence behavior, thread its usage
into the function body (e.g., apply options to configure file parsing/filters)
and update the implementation and tests accordingly so the parameter is actually
used.
| const sitemapBuffer = await generateSitemap( | ||
| initializedStoryIndexGenerator, | ||
| networkAddress, | ||
| false | ||
| ); | ||
| cachedSitemap = sitemapBuffer; | ||
| res.end(sitemapBuffer); |
There was a problem hiding this comment.
Handle undefined sitemap buffer to prevent malformed gzip response.
If generateSitemap returns undefined, the response will have Content-Encoding: gzip header but send an empty body, causing client decompression errors. Add a check before caching and sending.
const sitemapBuffer = await generateSitemap(
initializedStoryIndexGenerator,
networkAddress,
false
);
+ if (!sitemapBuffer) {
+ res.statusCode = 500;
+ res.end('Failed to generate sitemap');
+ return;
+ }
cachedSitemap = sitemapBuffer;
res.end(sitemapBuffer);🤖 Prompt for AI Agents
In code/core/src/core-server/utils/stories-json.ts around lines 180 to 186,
guard against generateSitemap returning undefined: check if sitemapBuffer is
undefined before assigning cachedSitemap and res.end; if undefined, log or
report the error, clear or avoid setting the gzip Content-Encoding header and
respond with a 204 No Content (or 500 if preferred) without caching, otherwise
proceed to cache and send the buffer as before.
What I did
Adds a sitemap.xml to help with the indexing of Storybook instances.
Part of an experiment on SEO improvements through the About page.
TODO list
Now
Later
main.ts'sbuild.siteUrlnot being usedgetEnvConfiginbin/coreto read Netlify/Vercel/GH pages URL env varsChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Documentation
Caution
Documentation is missing for now.
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 pull request has been released as version
0.0.0-pr-33243-sha-278c6240. Try it out in a new sandbox by runningnpx [email protected] sandboxor in an existing project withnpx [email protected] upgrade.More information
0.0.0-pr-33243-sha-278c6240sidnioulz/growth-exp-sitemap278c62401765174976)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=33243Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.