React: Create a components manifest html page #32882
Conversation
|
View your CI Pipeline Execution ↗ for commit 813ff59
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughAdds an HTML renderer and dev-server endpoint for ComponentsManifest, writes Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant DevServer
participant Presets
participant StoryIndexGen
participant Generator
participant ImportResolver
participant Renderer
Browser->>DevServer: GET /manifests/components.html
DevServer->>Presets: resolve('componentManifestGenerator') [experimental]
Presets-->>DevServer: (generator?)
DevServer->>StoryIndexGen: ensure initialized / get storyIndex
alt missing generator or index
DevServer-->>Browser: 400 HTML (error page)
else
DevServer->>Generator: generate(storyIndex)
Generator->>ImportResolver: matchPath + resolveImport(componentPath)
ImportResolver-->>Generator: resolved component paths
Generator-->>DevServer: ComponentsManifest
DevServer->>Renderer: renderManifestComponentsPage(ComponentsManifest)
Renderer-->>DevServer: HTML page
DevServer-->>Browser: 200 HTML (manifest page)
end
opt generation/render error
DevServer-->>Browser: 500 HTML (error message)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
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: 2
🧹 Nitpick comments (7)
code/renderers/react/src/componentManifest/generator.ts (2)
89-90: Clearer read-error copy — nice.Including the concrete path and hint to prefer relative imports improves DX. Consider rendering a path relative to process.cwd() for brevity in logs. Example:
- message: `Could not read the component file located at ${entry.componentPath}. Prefer relative imports.`, + message: `Could not read the component file at ${path.relative(process.cwd(), entry.componentPath!)}. Prefer relative imports.`,
100-104: Prop‑parse error message improvement looks good.Message is explicit and actionable. Optionally include the component name to aid triage in multi-package repos. E.g., append
(component: ${name}).code/core/src/core-server/dev-server.ts (2)
145-168: Minor: make JSON endpoint GET-only and set content type for error paths.Use app.get for clarity and add consistent content types on 400/500 responses.
- app.use('/manifests/components.json', async (req, res) => { + app.get('/manifests/components.json', async (req, res) => { try { … - res.statusCode = 400; - res.end('No component manifest generator configured.'); + res.statusCode = 400; + res.setHeader('Content-Type', 'text/plain; charset=utf-8'); + res.end('No component manifest generator configured.'); return; } catch (e) { logger.error(e instanceof Error ? e : String(e)); res.statusCode = 500; - res.end(e instanceof Error ? e.toString() : String(e)); + res.setHeader('Content-Type', 'text/plain; charset=utf-8'); + res.end(e instanceof Error ? e.toString() : String(e)); return; } });
170-188: Tiny: dedupe generator/index lookup between .json and .html handlers.Factor a small helper to fetch both or return a 400. Reduces drift between endpoints.
code/core/src/core-server/manifest.ts (3)
110-121: Keyboard a11y for toggle badges (labels) — add focus style.Labels are clickable but not focus-styled; keyboard users get no visual cue. Add a focus ring:
.as-toggle{cursor:pointer} - /* 1px ring on active toggle */ + /* 1px ring on active toggle */ .tg-err:checked + label.as-toggle, .tg-warn:checked + label.as-toggle, .tg-ex:checked + label.as-toggle { box-shadow:0 0 0 var(--active-ring) currentColor; border-color:currentColor; } + /* Visible focus for keyboard users */ + label.as-toggle:focus { outline: none; box-shadow: 0 0 0 var(--active-ring) currentColor; border-color: currentColor; }Also applies to: 151-161
85-93: Restore focus indicator for top filter pills.Current CSS removes box-shadow on :focus. Provide visible focus to meet a11y basics.
- .filter-pill:focus,.filter-pill:active{outline:none;box-shadow:none} + .filter-pill:focus { outline: none; box-shadow: 0 0 0 var(--active-ring) currentColor; border-color: currentColor; } + .filter-pill:active{outline:none}Also applies to: 95-99
187-190: Minor semantics: prefer real list markup.div[role="list"]/article[role="listitem"] is acceptable, but ul/li improves semantics and user agents support. Optional.
- <div class="grid" role="list"> + <ul class="grid" role="list"> ${grid || `<div class="card"><div class="head"><div class="hint">No components.</div></div></div>`} - </div> + </ul>And wrap each card in
instead of or nest inside . Also applies to: 285-301
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/core/src/core-server/dev-server.ts(2 hunks)code/core/src/core-server/manifest.ts(1 hunks)code/renderers/react/src/componentManifest/generator.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/src/core-server/manifest.tscode/core/src/core-server/dev-server.tscode/renderers/react/src/componentManifest/generator.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/core-server/manifest.tscode/core/src/core-server/dev-server.tscode/renderers/react/src/componentManifest/generator.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/core-server/manifest.tscode/core/src/core-server/dev-server.tscode/renderers/react/src/componentManifest/generator.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/core-server/manifest.tscode/core/src/core-server/dev-server.tscode/renderers/react/src/componentManifest/generator.ts
🧬 Code graph analysis (2)
code/core/src/core-server/manifest.ts (1)
code/core/src/types/modules/core-common.ts (2)
ComponentsManifest(360-363)ComponentManifest(348-358)
code/core/src/core-server/dev-server.ts (4)
code/renderers/react/src/componentManifest/generator.ts (1)
componentManifestGenerator(20-135)code/renderers/react/src/preset.ts (1)
componentManifestGenerator(11-11)code/core/src/types/modules/core-common.ts (2)
ComponentManifestGenerator(365-367)ComponentsManifest(360-363)code/core/src/core-server/manifest.ts (1)
renderManifestComponentsPage(5-194)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/core/src/core-server/manifest.ts (1)
196-201: Good HTML escaping throughout.esc() covers &, <, >, ", '. Error and snippet blocks are correctly escaped before insertion.
Also applies to: 312-314, 339-341
| // logger?.error?.(e instanceof Error ? e : String(e)); | ||
| res.statusCode = 500; | ||
| res.setHeader('Content-Type', 'text/html; charset=utf-8'); | ||
| res.end(`<pre>${e instanceof Error ? e.toString() : String(e)}</pre>`); | ||
| } |
There was a problem hiding this comment.
Escape HTML in error responses to avoid XSS; log the error.
Interpolating raw error text into HTML is unsafe. Also, keep server logs. Two options:
- Safer HTML: escape the string and keep text/html.
- Or return text/plain without HTML.
Suggested patch (HTML + escape + logging):
- } catch (e) {
- // logger?.error?.(e instanceof Error ? e : String(e));
- res.statusCode = 500;
- res.setHeader('Content-Type', 'text/html; charset=utf-8');
- res.end(`<pre>${e instanceof Error ? e.toString() : String(e)}</pre>`);
- }
+ } catch (e) {
+ logger.error(e instanceof Error ? e : String(e));
+ const esc = (s: unknown) =>
+ String(s ?? '').replace(/[&<>"']/g, (c) => ({'&':'&','<':'<','>':'>','"':'"',"'":'''}[c] as string));
+ res.statusCode = 500;
+ res.setHeader('Content-Type', 'text/html; charset=utf-8');
+ const msg = e instanceof Error ? e.toString() : String(e);
+ res.end(`<pre><code>${esc(msg)}</code></pre>`);
+ }📝 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.
| // logger?.error?.(e instanceof Error ? e : String(e)); | |
| res.statusCode = 500; | |
| res.setHeader('Content-Type', 'text/html; charset=utf-8'); | |
| res.end(`<pre>${e instanceof Error ? e.toString() : String(e)}</pre>`); | |
| } | |
| } catch (e) { | |
| logger.error(e instanceof Error ? e : String(e)); | |
| const esc = (s: unknown) => | |
| String(s ?? '').replace(/[&<>"']/g, (c) => ({'&':'&','<':'<','>':'>','"':'"',"'":'''}[c] as string)); | |
| res.statusCode = 500; | |
| res.setHeader('Content-Type', 'text/html; charset=utf-8'); | |
| const msg = e instanceof Error ? e.toString() : String(e); | |
| res.end(`<pre><code>${esc(msg)}</code></pre>`); | |
| } |
🤖 Prompt for AI Agents
In code/core/src/core-server/dev-server.ts around lines 191 to 195, the error
response currently interpolates raw error text into HTML and doesn't log the
error; replace that with a safe approach: log the error via the server logger
(e.g., logger?.error(e)) and then either (a) escape the error string for HTML
(implement or use an escape utility to replace &,<,>,",') and return as
text/html with the escaped content, or (b) simpler—set Content-Type to
text/plain; charset=utf-8 and return the raw string there; ensure res.statusCode
remains 500 and use e instanceof Error ? e.toString() : String(e) as the source
for logging/escaping.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 43 | 43 | 0 |
| Self size | 22.93 MB | 23.01 MB | 🚨 +77 KB 🚨 |
| Dependency size | 17.36 MB | 17.36 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 187 | 187 | 0 |
| Self size | 928 KB | 928 KB | 🚨 +84 B 🚨 |
| Dependency size | 72.89 MB | 72.96 MB | 🚨 +77 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 169 | 169 | 0 |
| Self size | 35 KB | 35 KB | 0 B |
| Dependency size | 69.31 MB | 69.39 MB | 🚨 +77 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 44 | 44 | 0 |
| Self size | 1.55 MB | 1.55 MB | 0 B |
| Dependency size | 40.29 MB | 40.37 MB | 🚨 +77 KB 🚨 |
| Bundle Size Analyzer | node | node |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/core-server/utils/StoryIndexGenerator.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/src/core-server/utils/StoryIndexGenerator.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/core-server/utils/StoryIndexGenerator.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/core-server/utils/StoryIndexGenerator.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/core-server/utils/StoryIndexGenerator.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). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/renderers/react/src/componentManifest/generator.ts (1)
35-37: Add error handling for story file operations.The
readFileandloadCsfoperations can fail but have no error handling, which will cause the entire component manifest generation to fail. Additionally, the non-null assertion onentry.title.split('/').at(-1)!assumes the title contains a '/' and has parts after splitting, which may not always be true.Consider wrapping these operations in a try-catch and returning a graceful error entry:
+ try { const storyFile = await readFile(path.join(process.cwd(), entry.importPath), 'utf-8'); const csf = loadCsf(storyFile, { makeTitle: (title) => title ?? 'No title' }).parse(); - const name = csf._meta?.component ?? entry.title.split('/').at(-1)!; + const name = csf._meta?.component ?? entry.title.split('/').at(-1) ?? entry.title; const id = entry.id.split('--')[0]; const importPath = entry.importPath; + } catch (e) { + invariant(e instanceof Error); + return { + id: entry.id.split('--')[0], + name: entry.title.split('/').at(-1) ?? entry.title, + path: entry.importPath, + examples: [], + jsDocTags: {}, + error: { + message: `Could not read or parse the story file: ${e.message}`, + }, + }; + }
🧹 Nitpick comments (1)
code/renderers/react/src/componentManifest/generator.ts (1)
96-99: Consider adding error handling for docgen parsing.The
parseWithReactDocgencall has no error handling. If it throws (e.g., due to invalid syntax or unsupported features), the entire component entry will fail instead of returning an error entry like other error cases in this function.Consider wrapping this in a try-catch for consistency:
+ let docgens; + let docgen; + try { - const docgens = await parseWithReactDocgen({ + docgens = await parseWithReactDocgen({ code: componentFile, filename: path.join(process.cwd(), entry.componentPath), }); - const docgen = getMatchingDocgen(docgens, csf); + docgen = getMatchingDocgen(docgens, csf); + } catch (e) { + invariant(e instanceof Error); + return { + ...base, + name, + examples, + error: { + message: `Failed to parse component file: ${e.message}`, + }, + }; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/core-server/utils/StoryIndexGenerator.ts(1 hunks)code/renderers/react/src/componentManifest/generator.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/core-server/utils/StoryIndexGenerator.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/renderers/react/src/componentManifest/generator.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/renderers/react/src/componentManifest/generator.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/renderers/react/src/componentManifest/generator.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/renderers/react/src/componentManifest/generator.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). (1)
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (3)
code/renderers/react/src/componentManifest/generator.ts (3)
74-76: LGTM: Improved error context.The enhanced error message now includes code frame context from the CSF meta statement when available, making it easier to locate the issue in the source file.
90-92: LGTM: Actionable error guidance.The enhanced error message provides clear, actionable guidance about using relative imports when the component file cannot be read.
104-104: LGTM: Helpful guidance about barrel files.The enhanced error message provides actionable guidance about avoiding barrel files, which is a common cause of React docgen parsing failures.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
code/core/src/core-server/manifest.ts (1)
238-250: Toggle badges are not keyboard-accessible (duplicate concern).As noted in the previous review, the label elements used as toggle badges are not keyboard-focusable. Users navigating with keyboard cannot activate these toggles.
The previous review suggested adding
tabindex="0"androle="button"to make these labels keyboard-focusable:const componentErrorBadge = a.hasComponentError - ? `<label for="${slug}-err" class="badge err as-toggle">component error</label>` + ? `<label for="${slug}-err" class="badge err as-toggle" tabindex="0" role="button">component error</label>` : ''; const warningsBadge = a.hasWarns - ? `<label for="${slug}-warn" class="badge warn as-toggle">${a.warns.length} ${plural(a.warns.length, 'warning')}</label>` + ? `<label for="${slug}-warn" class="badge warn as-toggle" tabindex="0" role="button">${a.warns.length} ${plural(a.warns.length, 'warning')}</label>` : ''; const examplesBadge = a.exampleErrors > 0 - ? `<label for="${slug}-ex" class="badge err as-toggle">${a.exampleErrors}/${a.totalExamples} example errors</label>` + ? `<label for="${slug}-ex" class="badge err as-toggle" tabindex="0" role="button">${a.exampleErrors}/${a.totalExamples} example errors</label>` : `<span class="badge ok">${a.totalExamples} examples ok</span>`;Additionally, consider adding keyboard event handling via CSS if using the
:focus-visiblepseudo-class for better visual feedback.
🧹 Nitpick comments (1)
code/core/src/core-server/manifest.ts (1)
220-226: Consider making the API safer by escaping within this function.The
bodyHTMLparameter is inserted without escaping (line 224), relying on callers to pre-escape content. While current usage is safe, this pattern is fragile and could lead to XSS vulnerabilities if future callers forget to escape.Consider one of these approaches:
Option 1: Escape within the function and rename the parameter
-function note(title: string, bodyHTML: string, kind: 'warn' | 'err') { +function note(title: string, body: string, kind: 'warn' | 'err') { return dedent` <div class="note ${kind}"> <div class="note-title">${esc(title)}</div> - <div class="note-body">${bodyHTML}</div> + <div class="note-body">${esc(body)}</div> </div>`; }Then update callers to pass raw strings instead of pre-escaped HTML.
Option 2: Make the intent explicit with better naming
-function note(title: string, bodyHTML: string, kind: 'warn' | 'err') { +function note(title: string, unsafePreEscapedHTML: string, kind: 'warn' | 'err') { return dedent` <div class="note ${kind}"> <div class="note-title">${esc(title)}</div> - <div class="note-body">${bodyHTML}</div> + <div class="note-body">${unsafePreEscapedHTML}</div> </div>`; }This signals to callers that they must handle escaping.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/core-server/manifest.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/src/core-server/manifest.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/core-server/manifest.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/core-server/manifest.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/core-server/manifest.ts
🧠 Learnings (1)
📚 Learning: 2025-10-03T07:55:42.639Z
Learnt from: Sidnioulz
PR: storybookjs/storybook#32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Applied to files:
code/core/src/core-server/manifest.ts
🧬 Code graph analysis (1)
code/core/src/core-server/manifest.ts (1)
code/core/src/types/modules/core-common.ts (2)
ComponentsManifest(360-363)ComponentManifest(348-358)
⏰ 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). (1)
- GitHub Check: normal
🔇 Additional comments (4)
code/core/src/core-server/manifest.ts (4)
1-10: LGTM! Clean imports and safe data handling.The function is properly exported, and the sorting logic safely handles missing component names with appropriate fallbacks.
182-187: LGTM! Proper HTML escaping utilities.The escaping function correctly handles the essential HTML entities and safely processes null/undefined values.
189-218: LGTM! Comprehensive component analysis.The function correctly identifies component errors, generates helpful warnings for missing documentation, and accurately calculates error metrics.
262-327: LGTM! Well-structured card rendering with proper escaping.The card HTML is correctly structured with:
- Proper escaping of all user-provided content
- Valid hidden checkbox pattern for CSS-only toggles
- Good accessibility with
role="listitem"andaria-label- Conditional rendering of error/warning panels
| const slug = `c-${i}-${(c.id || key) | ||
| .toLowerCase() | ||
| .replace(/[^a-z0-9]+/g, '-') | ||
| .replace(/^-+|-+$/g, '')}`; |
There was a problem hiding this comment.
Potential ID collision risk in slug generation.
The slug generation normalizes component IDs to create unique HTML element IDs. While the index prefix (c-${i}-) reduces collision risk, two components with names that normalize identically (e.g., "MyComponent" and "My-Component") could still produce colliding IDs if they appear at the same index in different renders.
Consider incorporating more uniqueness into the slug:
- const slug = `c-${i}-${(c.id || key)
+ const slug = `c-${i}-${(c.id || key).replace(/\//g, '-')}`.replace(/[^a-z0-9-]/gi, '-').replace(/-+/g, '-')
.toLowerCase()
.replace(/[^a-z0-9]+/g, '-')
.replace(/^-+|-+$/g, '')}`;Or use a hash of the full key to guarantee uniqueness:
+ // At top: import { createHash } from 'crypto';
- const slug = `c-${i}-${(c.id || key)
+ const slug = `c-${createHash('md5').update(key).digest('hex').slice(0, 8)}-${(c.id || c.name || key)
.toLowerCase()
.replace(/[^a-z0-9]+/g, '-')
.replace(/^-+|-+$/g, '')}`;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In code/core/src/core-server/manifest.ts around lines 233 to 236, the slug
generation can produce collisions when different keys normalize to the same
string; change the slug to include a short deterministic unique suffix (for
example a 6–8 character hex hash of the full key or id) appended after the
normalized string (still lowercased and sanitized), so the slug becomes
c-{i}-{normalized}-{hash}; compute the hash from the original key/id (not the
normalized value) to ensure uniqueness across similar names and keep the
sanitization step for the final slug output.
| } | ||
| }); | ||
|
|
||
| app.get('/manifests/components.html', async (req, res) => { |
There was a problem hiding this comment.
I think it would be nice to also support '/manifests/components', without the html extension. just call get() two times with the same handler.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code/core/src/common/utils/interpret-files.ts(1 hunks)code/core/src/core-server/utils/StoryIndexGenerator.ts(2 hunks)code/renderers/react/src/componentManifest/generator.ts(3 hunks)code/renderers/react/src/componentManifest/reactDocgen.ts(3 hunks)code/renderers/react/src/componentManifest/reactDocgen/docgenResolver.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/renderers/react/src/componentManifest/generator.tscode/renderers/react/src/componentManifest/reactDocgen/docgenResolver.tscode/renderers/react/src/componentManifest/reactDocgen.tscode/core/src/common/utils/interpret-files.tscode/core/src/core-server/utils/StoryIndexGenerator.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/renderers/react/src/componentManifest/generator.tscode/renderers/react/src/componentManifest/reactDocgen/docgenResolver.tscode/renderers/react/src/componentManifest/reactDocgen.tscode/core/src/common/utils/interpret-files.tscode/core/src/core-server/utils/StoryIndexGenerator.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/renderers/react/src/componentManifest/generator.tscode/renderers/react/src/componentManifest/reactDocgen/docgenResolver.tscode/renderers/react/src/componentManifest/reactDocgen.tscode/core/src/common/utils/interpret-files.tscode/core/src/core-server/utils/StoryIndexGenerator.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/renderers/react/src/componentManifest/generator.tscode/renderers/react/src/componentManifest/reactDocgen/docgenResolver.tscode/renderers/react/src/componentManifest/reactDocgen.tscode/core/src/common/utils/interpret-files.tscode/core/src/core-server/utils/StoryIndexGenerator.ts
🧠 Learnings (4)
📚 Learning: 2025-10-01T15:24:01.060Z
Learnt from: Sidnioulz
PR: storybookjs/storybook#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/renderers/react/src/componentManifest/reactDocgen/docgenResolver.tscode/core/src/core-server/utils/StoryIndexGenerator.ts
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to **/*.{ts,tsx,js,jsx,mjs} : Export functions from modules when they need to be unit-tested
Applied to files:
code/renderers/react/src/componentManifest/reactDocgen/docgenResolver.tscode/core/src/common/utils/interpret-files.ts
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to test-storybooks/** : Maintain test Storybook configurations under `test-storybooks/` for E2E and visual testing scenarios
Applied to files:
code/renderers/react/src/componentManifest/reactDocgen.tscode/core/src/core-server/utils/StoryIndexGenerator.ts
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to code/.storybook/** : Place internal UI Storybook configuration in `code/.storybook/` and maintain it there
Applied to files:
code/renderers/react/src/componentManifest/reactDocgen.ts
🧬 Code graph analysis (3)
code/renderers/react/src/componentManifest/reactDocgen/docgenResolver.ts (1)
code/core/src/common/utils/interpret-files.ts (1)
supportedExtensions(6-17)
code/renderers/react/src/componentManifest/reactDocgen.ts (1)
code/core/src/common/utils/interpret-files.ts (2)
supportedExtensions(6-17)resolveImport(25-58)
code/core/src/core-server/utils/StoryIndexGenerator.ts (2)
code/core/src/common/utils/interpret-files.ts (2)
supportedExtensions(6-17)resolveImport(25-58)code/core/src/common/utils/paths.ts (1)
normalizeStoryPath(82-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
code/core/src/core-server/manifest.ts (2)
288-291: Address the slug collision risk mentioned in past review.As noted in the previous review, the slug generation can produce collisions when different keys normalize to the same string. While the index prefix
c-${i}-reduces risk, components with similar names (e.g., "MyComponent" and "My-Component") could still collide in different renders or contexts.The past review suggested incorporating a hash or ensuring the full key uniqueness. Please address this concern to guarantee unique HTML element IDs.
293-305: Address keyboard accessibility for toggle badges mentioned in past review.As noted in the previous review, the label elements used as toggle badges are not keyboard-focusable. Users navigating with a keyboard cannot reach these badges to toggle the associated panels.
The past review recommended adding
tabindex="0"androle="button"to make these labels focusable and indicate their interactive nature. Please address this accessibility concern.
🧹 Nitpick comments (2)
code/core/src/common/utils/utils.ts (1)
1-15: Consider clarifying the comment.The comment describes this as an "Object.groupBy polyfill," but the implementation doesn't actually polyfill
Object.groupByon the global object—it's a standalone utility. Consider updating the comment to reflect this:-// Object.groupBy polyfill +// Utility function similar to Object.groupBy (ES2024) export const groupBy = <K extends PropertyKey, T>(The implementation itself is clean and correct.
code/core/src/core-server/manifest.ts (1)
7-8: Consider removing or rewording the "AI generated" comment.The comment "AI generated manifests/components.html page" may cause confusion in the codebase. If this code is now maintained by humans, consider removing the comment or rewording it to describe what the code does rather than how it was created.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code/core/src/common/index.ts(1 hunks)code/core/src/common/utils/utils.ts(1 hunks)code/core/src/core-server/manifest.ts(1 hunks)code/core/src/types/modules/core-common.ts(1 hunks)code/renderers/react/src/componentManifest/generator.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/renderers/react/src/componentManifest/generator.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/src/common/index.tscode/core/src/common/utils/utils.tscode/core/src/types/modules/core-common.tscode/core/src/core-server/manifest.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/common/index.tscode/core/src/common/utils/utils.tscode/core/src/types/modules/core-common.tscode/core/src/core-server/manifest.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/common/index.tscode/core/src/common/utils/utils.tscode/core/src/types/modules/core-common.tscode/core/src/core-server/manifest.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/common/index.tscode/core/src/common/utils/utils.tscode/core/src/types/modules/core-common.tscode/core/src/core-server/manifest.ts
🧠 Learnings (2)
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to **/*.{ts,tsx,js,jsx,mjs} : Export functions from modules when they need to be unit-tested
Applied to files:
code/core/src/common/index.ts
📚 Learning: 2025-10-03T07:55:42.639Z
Learnt from: Sidnioulz
PR: storybookjs/storybook#32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Applied to files:
code/core/src/core-server/manifest.ts
🧬 Code graph analysis (1)
code/core/src/core-server/manifest.ts (2)
code/core/src/types/modules/core-common.ts (2)
ComponentsManifest(360-363)ComponentManifest(348-358)code/core/src/common/utils/utils.ts (1)
groupBy(2-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (7)
code/core/src/common/index.ts (1)
48-48: LGTM!The re-export follows the established pattern in this file and correctly exposes the new utilities from
./utils/utils.code/core/src/common/utils/utils.ts (1)
17-26: LGTM!The
invariantimplementation correctly supports lazy message evaluation and uses TypeScript'sassertsfor proper type narrowing.code/core/src/core-server/manifest.ts (4)
10-38: LGTM!The component sorting, analysis, and filter pill generation logic is clean and handles pluralization correctly.
68-233: LGTM with one note.The HTML template is well-structured with semantic markup, ARIA labels, and CSS-only interactions. The progressive enhancement with
@supportsfor:has()is appropriate.Note: The past review comments about keyboard accessibility for toggle badges (lines 293-305) and slug collision (lines 288-291) still apply to the implementation below.
235-273: LGTM!The helper functions (
esc,plural,analyzeComponent) are well-implemented. The warning messages provide actionable guidance for missing descriptions and import statements.
275-281: LGTM!The
notefunction provides consistent rendering for warnings and errors. ThebodyHTMLparameter assumes pre-escaped content, which is appropriate given the controlled usage in this file.code/core/src/types/modules/core-common.ts (1)
355-357: All error creation sites are properly populating thenamefield—no issues found.The verification confirms that in code/renderers/react/src/componentManifest/generator.ts, all three locations where ComponentManifest and example error objects are created (lines 52–55, 85–88, and 103–106) consistently populate both the
nameandmessagefields. The code is ready as-is.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
code/core/src/core-server/manifest.ts (3)
45-51: Fix potential ID collision and variable shadowing.Two issues:
ID collision risk: Line 47 generates IDs by normalizing error names. Different error names that normalize identically (e.g., "TypeError" vs "Type Error") will produce duplicate IDs, breaking toggle functionality.
Variable shadowing: Line 50's map callback parameter
idshadows the outeridvariable from line 47, which reduces clarity.Apply this diff to add uniqueness and fix shadowing:
const errorGroupsHTML = errorGroups - .map(([error, grouped]) => { - const id = error.toLowerCase().replace(/[^a-z0-9]+/g, '-'); + .map(([error, grouped], groupIndex) => { + const id = `err-grp-${groupIndex}-${error.toLowerCase().replace(/[^a-z0-9]+/g, '-')}`; const headerText = `${esc(error)}`; const cards = grouped - .map((manifest, id) => renderComponentCard(manifest.id, manifest, id)) + .map((manifest, idx) => renderComponentCard(manifest.id, manifest, idx)) .join('');
286-289: Strengthen slug generation to prevent ID collisions.The current slug generation can produce collisions when different component IDs/keys normalize to the same string (e.g., "MyComponent" and "My-Component" both become "mycomponent").
Apply this diff to add a hash-based unique suffix:
+ // At top of file: import { createHash } from 'crypto'; + const slug = `c-${i}-${(c.id || key) - .toLowerCase() - .replace(/[^a-z0-9]+/g, '-') - .replace(/^-+|-+$/g, '')}`; + .toLowerCase() + .replace(/[^a-z0-9]+/g, '-') + .replace(/^-+|-+$/g, '')}-${createHash('md5').update(key).digest('hex').slice(0, 8)}`;
291-303: Make toggle badges keyboard-accessible.The label elements used as toggle badges are not keyboard-focusable by default. Users navigating via keyboard cannot reach or activate these toggles.
Apply this diff to add keyboard accessibility:
const componentErrorBadge = a.hasComponentError - ? `<label for="${slug}-err" class="badge err as-toggle">component error</label>` + ? `<label for="${slug}-err" class="badge err as-toggle" tabindex="0" role="button">component error</label>` : ''; const warningsBadge = a.hasWarns - ? `<label for="${slug}-warn" class="badge warn as-toggle">${a.warns.length} ${plural(a.warns.length, 'warning')}</label>` + ? `<label for="${slug}-warn" class="badge warn as-toggle" tabindex="0" role="button">${a.warns.length} ${plural(a.warns.length, 'warning')}</label>` : ''; const examplesBadge = a.exampleErrors > 0 - ? `<label for="${slug}-ex" class="badge err as-toggle">${a.exampleErrors}/${a.totalExamples} example errors</label>` + ? `<label for="${slug}-ex" class="badge err as-toggle" tabindex="0" role="button">${a.exampleErrors}/${a.totalExamples} example errors</label>` : `<span class="badge ok">${a.totalExamples} examples ok</span>`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/core-server/manifest.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/src/core-server/manifest.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/core-server/manifest.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/core-server/manifest.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/core-server/manifest.ts
🧠 Learnings (1)
📚 Learning: 2025-10-03T07:55:42.639Z
Learnt from: Sidnioulz
PR: storybookjs/storybook#32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Applied to files:
code/core/src/core-server/manifest.ts
🧬 Code graph analysis (1)
code/core/src/core-server/manifest.ts (2)
code/core/src/types/modules/core-common.ts (2)
ComponentsManifest(360-363)ComponentManifest(348-358)code/core/src/common/utils/utils.ts (1)
groupBy(2-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (5)
code/core/src/core-server/manifest.ts (5)
1-11: LGTM!The imports are appropriate, and the sorting logic safely handles missing component names by falling back to the key.
12-36: LGTM!The analysis and aggregation logic is correct, and the conditional rendering of filter pills appropriately handles zero-count scenarios.
66-230: LGTM with note on label accessibility.The HTML structure is semantic and includes appropriate ARIA roles. The CSS-only filtering implementation using
:targetis clever and accessible for keyboard navigation.Note: Label keyboard accessibility for toggle badges will be addressed in the review of
renderComponentCard.
233-279: LGTM!The helper functions are well-implemented:
esccorrectly escapes HTML entitiespluralhandles pluralization appropriatelyanalyzeComponentprovides clear, actionable warningsnotefunction is safe given that all callers escape user content
316-381: LGTM!The card HTML template is well-structured with proper escaping of all user-supplied content, preventing XSS risks. The CSS-only toggle pattern using hidden checkboxes is appropriate for this use case.
| .join('') | ||
| : ''; | ||
|
|
||
| esc(c.error?.message || 'Unknown error'); |
There was a problem hiding this comment.
Remove dead code.
Line 315 calls esc() but does not assign or use the result. This appears to be leftover debugging or incomplete code.
Apply this diff:
- esc(c.error?.message || 'Unknown error');
return `📝 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.
| esc(c.error?.message || 'Unknown error'); | |
| return ` |
🤖 Prompt for AI Agents
In code/core/src/core-server/manifest.ts around line 315, there is a dead call
to esc(c.error?.message || 'Unknown error') whose return value is not used;
remove this unused statement (delete the line) so there is no no-op escape call
left in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
code/core/src/core-server/manifest.ts (3)
45-64: ID collision risk in error group toggles.Line 47 generates toggle IDs by normalizing error names. If two error names normalize identically (e.g., "TypeError" and "Type Error"), duplicate IDs will break HTML validity and toggle functionality.
Consider adding a unique suffix (e.g., array index):
- const errorGroupsHTML = errorGroups - .map(([error, grouped]) => { - const id = error.toLowerCase().replace(/[^a-z0-9]+/g, '-'); + const errorGroupsHTML = errorGroups + .map(([error, grouped], idx) => { + const id = `err-grp-${idx}-${error.toLowerCase().replace(/[^a-z0-9]+/g, '-')}`;
291-302: Make toggle badges keyboard-focusable.Labels aren't keyboard-focusable by default, preventing keyboard users from accessing the toggle controls. Add
tabindex="0"to make them tabbable (Enter will toggle the associated checkbox via native HTML behavior).const componentErrorBadge = a.hasComponentError - ? `<label for="${slug}-err" class="badge err as-toggle">component error</label>` + ? `<label for="${slug}-err" class="badge err as-toggle" tabindex="0" role="button">component error</label>` : ''; const warningsBadge = a.hasWarns - ? `<label for="${slug}-warn" class="badge warn as-toggle">${a.warns.length} ${plural(a.warns.length, 'warning')}</label>` + ? `<label for="${slug}-warn" class="badge warn as-toggle" tabindex="0" role="button">${a.warns.length} ${plural(a.warns.length, 'warning')}</label>` : ''; const examplesBadge = a.exampleErrors > 0 - ? `<label for="${slug}-ex" class="badge err as-toggle">${a.exampleErrors}/${a.totalExamples} example errors</label>` + ? `<label for="${slug}-ex" class="badge err as-toggle" tabindex="0" role="button">${a.exampleErrors}/${a.totalExamples} example errors</label>` : `<span class="badge ok">${a.totalExamples} examples ok</span>`;
315-315: Remove dead code.This
esc()call has no effect—its result isn't used.- esc(c.error?.message || 'Unknown error'); return `
🧹 Nitpick comments (1)
code/core/src/core-server/manifest.ts (1)
50-50: Preferidxoveridfor iterator variable.Using
idas the map iterator variable is confusing sinceidcommonly refers to element identifiers.- .map((manifest, id) => renderComponentCard(manifest.id, manifest, `error-${id}`)) + .map((manifest, idx) => renderComponentCard(manifest.id, manifest, `error-${idx}`))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/common/utils/interpret-files.ts(1 hunks)code/core/src/core-server/manifest.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/common/utils/interpret-files.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/src/core-server/manifest.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/core-server/manifest.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/core-server/manifest.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/core-server/manifest.ts
🧠 Learnings (1)
📚 Learning: 2025-10-03T07:55:42.639Z
Learnt from: Sidnioulz
PR: storybookjs/storybook#32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Applied to files:
code/core/src/core-server/manifest.ts
🧬 Code graph analysis (1)
code/core/src/core-server/manifest.ts (2)
code/core/src/types/modules/core-common.ts (2)
ComponentsManifest(360-363)ComponentManifest(348-358)code/core/src/common/utils/utils.ts (1)
groupBy(2-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/core-server/build-static.ts (1)
184-187: LGTM: HTML manifest generation properly implemented.The HTML manifest is correctly generated using the same pattern as the JSON manifest. The code properly awaits the async operation and is covered by the existing error handling block.
Consider updating the error message on line 189 to be more generic since it now covers both JSON and HTML generation:
- logger.error('Failed to generate manifests/components.json'); + logger.error('Failed to generate component manifests');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/core-server/build-static.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/src/core-server/build-static.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/core-server/build-static.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/core-server/build-static.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/core-server/build-static.ts
🧠 Learnings (4)
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to scripts/**/*.{ts,js,mjs} : In Node.js scripts, use `storybook/internal/node-logger` instead of `console.*`
Applied to files:
code/core/src/core-server/build-static.ts
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to code/**/*.{ts,tsx,js,jsx,mjs} : In application code, use Storybook loggers instead of `console.*` (client code: `storybook/internal/client-logger`; server code: `storybook/internal/node-logger`)
Applied to files:
code/core/src/core-server/build-static.ts
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
PR: storybookjs/storybook#32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
Applied to files:
code/core/src/core-server/build-static.ts
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to test-storybooks/** : Maintain test Storybook configurations under `test-storybooks/` for E2E and visual testing scenarios
Applied to files:
code/core/src/core-server/build-static.ts
🧬 Code graph analysis (1)
code/core/src/core-server/build-static.ts (1)
code/core/src/core-server/manifest.ts (1)
renderManifestComponentsPage(7-231)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/core/src/core-server/build-static.ts (1)
15-15: LGTM: Imports are correctly added.The
ComponentsManifesttype import andrenderManifestComponentsPagefunction import are both necessary and properly structured for the new HTML manifest generation feature.Also applies to: 22-22
|
|
||
| return rawComponentPath; | ||
| const relativePath = relative(this.options.workingDir, resolved); | ||
| return slash(normalizeStoryPath(relativePath)); |
There was a problem hiding this comment.
we need to QA this on Windows soon.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
code/core/src/core-server/manifest.ts (3)
641-641: Remove dead code.This line calls
esc()but does not use the result. It appears to be leftover debugging code and should be removed.Apply this diff:
- esc(c.error?.message || 'Unknown error'); return `
617-628: Make toggle badges keyboard-accessible.The label elements used as toggle badges cannot be focused via keyboard navigation. Add
tabindex="0"androle="button"to make them keyboard-accessible.Apply this diff:
const componentErrorBadge = a.hasComponentError - ? `<label for="${slug}-err" class="badge err as-toggle">component error</label>` + ? `<label for="${slug}-err" class="badge err as-toggle" tabindex="0" role="button">component error</label>` : ''; const warningsBadge = a.hasWarns - ? `<label for="${slug}-warn" class="badge warn as-toggle">${a.warns.length} ${plural(a.warns.length, 'warning')}</label>` + ? `<label for="${slug}-warn" class="badge warn as-toggle" tabindex="0" role="button">${a.warns.length} ${plural(a.warns.length, 'warning')}</label>` : ''; const examplesBadge = a.exampleErrors > 0 - ? `<label for="${slug}-ex" class="badge err as-toggle">${a.exampleErrors}/${a.totalExamples} example errors</label>` + ? `<label for="${slug}-ex" class="badge err as-toggle" tabindex="0" role="button">${a.exampleErrors}/${a.totalExamples} example errors</label>` : `<span class="badge ok">${a.totalExamples} examples ok</span>`;
72-526: Format CSS with Prettier.The embedded CSS should be formatted with Prettier to ensure consistent formatting.
As per coding guidelines
Please run Prettier on this style block. You can:
- Extract the CSS to a temporary
.cssfile- Run Prettier on it
- Paste the formatted result back into the template string
Based on learnings
🧹 Nitpick comments (2)
code/core/src/core-server/manifest.ts (2)
45-64: Consider making group header labels keyboard-focusable.The group header labels (line 55) are interactive but not keyboard-focusable. While associated labels can be clicked, they cannot be tabbed to without explicit
tabindex="0".Apply this diff to improve keyboard navigation:
<label for="${id}-toggle" class="group-header"> + <label for="${id}-toggle" class="group-header" tabindex="0" role="button"> <span class="caret">▸</span>
612-615: Consider renaming theidparameter to avoid confusion.The parameter name
idshadows thec.idproperty, which could be confusing. The parameter actually represents an index or unique key for the card.Apply this diff to improve clarity:
-function renderComponentCard(key: string, c: ComponentManifest, id: string) { +function renderComponentCard(key: string, c: ComponentManifest, uniqueKey: string) { const a = analyzeComponent(c); const statusDot = a.hasAnyError ? 'dot-err' : 'dot-ok'; const errorExamples = (c.examples ?? []).filter((ex) => !!ex?.error); - const slug = `c-${id}-${(c.id || key) + const slug = `c-${uniqueKey}-${(c.id || key) .toLowerCase() .replace(/[^a-z0-9]+/g, '-') .replace(/^-+|-+$/g, '')}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
code/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
code/core/package.json(1 hunks)code/core/src/core-server/manifest.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/package.jsoncode/core/src/core-server/manifest.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/core-server/manifest.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/core-server/manifest.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/core-server/manifest.ts
🧠 Learnings (1)
📚 Learning: 2025-10-03T07:55:42.639Z
Learnt from: Sidnioulz
PR: storybookjs/storybook#32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Applied to files:
code/core/src/core-server/manifest.ts
🧬 Code graph analysis (1)
code/core/src/core-server/manifest.ts (2)
code/core/src/types/modules/core-common.ts (2)
ComponentsManifest(360-363)ComponentManifest(348-358)code/core/src/common/utils/utils.ts (1)
groupBy(2-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (5)
code/core/src/core-server/manifest.ts (4)
566-597: LGTM: Clean component analysis logic.The
analyzeComponentfunction correctly computes error states, warnings for missing metadata, and example statistics. The warning messages provide actionable guidance to developers.
559-564: LGTM: Proper HTML escaping and string utilities.The
escfunction correctly escapes all necessary HTML entities to prevent XSS, and thepluralfunction provides clean singular/plural handling.
599-605: LGTM: Clean note rendering with proper escaping.The
notefunction correctly escapes the title and allows pre-escaped HTML body content. All call sites properly escape content before passing.
7-64: LGTM: Clean separation of data processing and presentation.The main function logic is well-structured: it sorts entries, analyzes components, computes aggregate statistics, and builds the UI in a logical sequence. The use of
groupByfor error grouping is appropriate.code/core/package.json (1)
317-317: Dependency version verified as stable and secure.The
resolvepackage version 1.22.11 is the latest stable release, and no security vulnerabilities were detected.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
code/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
code/core/package.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
| "react-textarea-autosize": "^8.3.0", | ||
| "react-transition-group": "^4.4.5", | ||
| "require-from-string": "^2.0.2", | ||
| "resolve": "^1.22.11", |
There was a problem hiding this comment.
🧩 Analysis chain
Good: stable, well-known module resolution library added.
The "resolve" package is a mature, well-established module resolution utility. The caret version constraint (^1.22.11) is sensible, allowing patch and minor updates while maintaining API stability.
Verify that the version is current and free from known vulnerabilities, and confirm it's actually used in the new utilities (e.g., the resolveImport function mentioned in the PR).
🌐 Web query:
What is the latest version of resolve npm package and are there any known security vulnerabilities?
💡 Result:
Latest published version: 1.22.10 (published ~9 months ago). [1]
Known security status: there are no high‑profile npm security advisories explicitly for the resolve package in the npm advisory database; however third‑party scanners (e.g., ReversingLabs / Snyk feed) flag some indirect/patch‑mandated issues tied to versions/dependencies. I recommend running npm audit / Snyk scan on your project to see any dependency paths and remediations. [1][2]
Sources:
- npm package page for resolve (version and publish info). [1]
- ReversingLabs/Security scan summary showing flagged issues for resolve. [2]
Correct the resolve version: 1.22.11 does not exist as a published release.
The latest published version of resolve is 1.22.10, but the PR specifies ^1.22.11. Update line 317 to use "resolve": "^1.22.10" instead.
No high-profile npm security advisories exist for resolve, though third-party scanners flag some indirect/patch-mandated issues. Run npm audit after correcting the version to verify dependency security.
🤖 Prompt for AI Agents
In code/core/package.json at line 317 the dependency version for resolve is
incorrect (references non-existent 1.22.11); change the entry to reference the
latest published 1.22.10 (e.g. "resolve": "^1.22.10") and then run npm install
and npm audit to verify dependencies and security.
kasperpeulen
merged commit 0338640
into
10.1-with-canary-release
What I did
Created a
manifests/components.htmlpage to provide a structured overview of components.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!
manifests/components.html.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 pull request has been released as version
0.0.0-pr-32882-sha-d65d0e7f. 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-32882-sha-d65d0e7fkasper/manifests.htmld65d0e7f1761838852)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=32882Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Utilities
Types
Tests