Core: Agentic observability for vitest and ghost stories#34537
Merged
yannbf merged 27 commits intoproject/sb-agentic-setupfrom Apr 15, 2026
Merged
Core: Agentic observability for vitest and ghost stories#34537yannbf merged 27 commits intoproject/sb-agentic-setupfrom
yannbf merged 27 commits intoproject/sb-agentic-setupfrom
Conversation
|
View your CI Pipeline Execution ↗ for commit e81a5db
☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit 0834932 ☁️ Nx Cloud last updated this comment at |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces shared story-test types and analysis utilities, adds Vitest AgentTelemetryReporter that collects per-story results and emits telemetry, renames/extends the ghost-story runner to runStoryTests with ghostRun option, broadens error categorization, and wires session-aware render-analysis gating and telemetry helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant Vitest as Vitest Runner
participant Reporter as AgentTelemetryReporter
participant Analyzer as analyzeTestResults
participant Telemetry as Telemetry Service
participant Files as Test Modules / FS
Vitest->>Reporter: onInit(ctx)
Reporter->>Reporter: store context & startTime
Vitest->>Reporter: onTestCaseResult(testCase)
Reporter->>Reporter: toStoryTestResult(testCase) -> append StoryTestResult
Vitest->>Reporter: onTestRunEnd(testModules, unhandledErrors)
Reporter->>Files: gather module errors
Reporter->>Analyzer: analyzeTestResults(testResults)
Analyzer-->>Reporter: TestRunAnalysis
Reporter->>Telemetry: telemetry('ai-setup-self-healing-scoring', { agent, analysis, counts, duration, watch })
Telemetry-->>Reporter: ack
Reporter->>Reporter: reset internal buffers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
code/addons/vitest/src/vitest-plugin/agent-telemetry-reporter.test.ts (2)
5-13: Consider usingspy: truefor module mocks per coding guidelines.The coding guidelines recommend using
vi.mock()withspy: trueoption. While this mock intentionally replaces the module, consider if a spy-based approach would work:🔧 Alternative approach using spy: true
-vi.mock('storybook/internal/telemetry', () => ({ - telemetry: vi.fn(), - isExampleStoryId: vi.fn( - (id: string) => - id.startsWith('example-button--') || - id.startsWith('example-header--') || - id.startsWith('example-page--') - ), -})); +vi.mock('storybook/internal/telemetry', { spy: true });Then configure implementations in
beforeEach:beforeEach(() => { vi.clearAllMocks(); vi.mocked(telemetry).mockImplementation(vi.fn()); vi.mocked(isExampleStoryId).mockImplementation( (id: string) => id.startsWith('example-button--') || id.startsWith('example-header--') || id.startsWith('example-page--') ); // ... });As per coding guidelines: "Use
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/addons/vitest/src/vitest-plugin/agent-telemetry-reporter.test.ts` around lines 5 - 13, The test currently replaces the entire 'storybook/internal/telemetry' module via vi.mock(...) with concrete implementations for telemetry and isExampleStoryId; per guidelines switch to using vi.mock(..., { spy: true }) and move concrete implementations into the test setup (e.g., beforeEach) by clearing mocks (vi.clearAllMocks()) and assigning behavior with vi.mocked(telemetry).mockImplementation(...) and vi.mocked(isExampleStoryId).mockImplementation(...); update the existing mocked symbols telemetry and isExampleStoryId accordingly so the module is spied instead of fully replaced.
118-142: Consider adding explicit assertions toonTestCaseResulttests.These tests call
onTestCaseResultbut have no assertions, only verifying the method doesn't throw. The actual collection behavior is implicitly tested viaonTestRunEndtests, but explicit assertions would improve clarity.For example, you could verify collected results count after each call if the reporter exposes internal state, or add comments explaining these are smoke tests for no-throw behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/addons/vitest/src/vitest-plugin/agent-telemetry-reporter.test.ts` around lines 118 - 142, The tests for onTestCaseResult currently only call reporter.onTestCaseResult(...) without assertions; update each it block to assert the reporter's collection behavior by checking an exposed result store (e.g., call reporter.getCollectedResults() or inspect reporter._results) after the call to verify the expected count or contents for the given storyId, and for skipped cases assert that the store is unchanged or empty; if the reporter does not expose state, either add a test-only accessor method to the reporter or assert behavior via the next step (e.g., spy/stub the telemetry send function or verify onTestRunEnd results) and otherwise add a brief comment clarifying the test is intentionally a smoke/no-throw check.code/core/src/shared/utils/analyze-test-results.ts (1)
40-50: Consider using a more specific type instead ofRecord<string, any>.The
categorizedErrorsreduction usesRecord<string, any>which loses type information. The structure is well-defined and could benefit from explicit typing.🔧 Suggested type improvement
- const categorizedErrors = Array.from(map.entries()).reduce<Record<string, any>>( + const categorizedErrors = Array.from(map.entries()).reduce< + Record<string, { uniqueCount: number; count: number; matchedDependencies: string[] }> + >( (acc, [category, data]) => { acc[category] = { uniqueCount: data.uniqueErrors.size, count: data.count, matchedDependencies: Array.from(data.matchedDependencies).sort(), }; return acc; }, {} );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/shared/utils/analyze-test-results.ts` around lines 40 - 50, The reduction that builds categorizedErrors should use an explicit interface instead of Record<string, any>; define a CategorySummary type/interface (e.g., { uniqueCount: number; count: number; matchedDependencies: string[] }) and change the reducer's generic from Record<string, any> to Record<string, CategorySummary> (or to a TypedMap alias) so categorizedErrors, the reduce initial type, and the mapped properties (uniqueCount, count, matchedDependencies) are strongly typed; add the new type near the top of analyze-test-results.ts and update the reduce call signature to use it.code/core/src/shared/utils/analyze-test-results.test.ts (1)
6-6: Add explicit.tsextension to the mock path.Per coding guidelines, relative imports should use explicit file extensions.
🔧 Suggested fix
-vi.mock('./categorize-render-errors', { spy: true }); +vi.mock('./categorize-render-errors.ts', { spy: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/shared/utils/analyze-test-results.test.ts` at line 6, Replace the mock import path used in the test so it includes the explicit TypeScript extension: update the vi.mock call that references './categorize-render-errors' in analyze-test-results.test.ts to use './categorize-render-errors.ts' (keep the existing options like { spy: true } unchanged) to follow the project's relative-import extension guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/addons/a11y/src/preview.tsx`:
- Around line 23-26: The current check treats any globals.renderAnalysis object
as internal mode (isInternalRenderAnalysis = !!globals.renderAnalysis), which
wrongly suppresses a11y when renderAnalysis exists but is disabled; change
isInternalRenderAnalysis to check the enabled flag (e.g., use
!!globals.renderAnalysis?.enabled) and update the dependent condition
(shouldRunEnvironmentIndependent) to rely on that so a11y is only gated when
renderAnalysis.enabled is true; locate and update the symbols
isInternalRenderAnalysis, globals.renderAnalysis, and
shouldRunEnvironmentIndependent in preview.tsx.
In `@code/addons/vitest/src/vitest-plugin/agent-telemetry-reporter.ts`:
- Around line 57-58: Move the test-duration timer start from the end of runs to
the beginning: remove or stop resetting this.startTime in onTestRunEnd() and
instead set this.startTime = Date.now() inside onTestRunStart() so each run
measures only execution time (reference symbols: this.startTime,
onTestRunStart(), onTestRunEnd()).
In `@code/core/src/telemetry/event-cache.ts`:
- Around line 121-127: The current selection logic using
eventTypes.map(...).find(...) finds the first available event by array order
rather than the chronologically latest; update the logic that computes
lastRelevantEvent to choose the event with the greatest timestamp from
lastEvents for the given eventTypes (mirror the approach used in lastEvent()
which sorts by timestamp descending or compute a max by timestamp).
Specifically, replace the map+find sequence with code that collects
lastEvents[type] for each type in eventTypes, filters out undefined, then picks
the item with the largest timestamp (or sorts descending and takes [0]) so
lastRelevantEvent truly represents the most recent occurrence.
---
Nitpick comments:
In `@code/addons/vitest/src/vitest-plugin/agent-telemetry-reporter.test.ts`:
- Around line 5-13: The test currently replaces the entire
'storybook/internal/telemetry' module via vi.mock(...) with concrete
implementations for telemetry and isExampleStoryId; per guidelines switch to
using vi.mock(..., { spy: true }) and move concrete implementations into the
test setup (e.g., beforeEach) by clearing mocks (vi.clearAllMocks()) and
assigning behavior with vi.mocked(telemetry).mockImplementation(...) and
vi.mocked(isExampleStoryId).mockImplementation(...); update the existing mocked
symbols telemetry and isExampleStoryId accordingly so the module is spied
instead of fully replaced.
- Around line 118-142: The tests for onTestCaseResult currently only call
reporter.onTestCaseResult(...) without assertions; update each it block to
assert the reporter's collection behavior by checking an exposed result store
(e.g., call reporter.getCollectedResults() or inspect reporter._results) after
the call to verify the expected count or contents for the given storyId, and for
skipped cases assert that the store is unchanged or empty; if the reporter does
not expose state, either add a test-only accessor method to the reporter or
assert behavior via the next step (e.g., spy/stub the telemetry send function or
verify onTestRunEnd results) and otherwise add a brief comment clarifying the
test is intentionally a smoke/no-throw check.
In `@code/core/src/shared/utils/analyze-test-results.test.ts`:
- Line 6: Replace the mock import path used in the test so it includes the
explicit TypeScript extension: update the vi.mock call that references
'./categorize-render-errors' in analyze-test-results.test.ts to use
'./categorize-render-errors.ts' (keep the existing options like { spy: true }
unchanged) to follow the project's relative-import extension guideline.
In `@code/core/src/shared/utils/analyze-test-results.ts`:
- Around line 40-50: The reduction that builds categorizedErrors should use an
explicit interface instead of Record<string, any>; define a CategorySummary
type/interface (e.g., { uniqueCount: number; count: number; matchedDependencies:
string[] }) and change the reducer's generic from Record<string, any> to
Record<string, CategorySummary> (or to a TypedMap alias) so categorizedErrors,
the reduce initial type, and the mapped properties (uniqueCount, count,
matchedDependencies) are strongly typed; add the new type near the top of
analyze-test-results.ts and update the reduce call signature to use it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 915c3a4b-11d9-4ac3-85a7-4774f037f77e
📒 Files selected for processing (18)
code/addons/a11y/src/preview.tsxcode/addons/vitest/src/vitest-plugin/agent-telemetry-reporter.test.tscode/addons/vitest/src/vitest-plugin/agent-telemetry-reporter.tscode/addons/vitest/src/vitest-plugin/index.tscode/core/src/core-server/index.tscode/core/src/core-server/utils/ghost-stories/parse-vitest-report.test.tscode/core/src/core-server/utils/ghost-stories/parse-vitest-report.tscode/core/src/core-server/utils/ghost-stories/test-annotations.tscode/core/src/core-server/utils/ghost-stories/types.tscode/core/src/shared/utils/analyze-test-results.test.tscode/core/src/shared/utils/analyze-test-results.tscode/core/src/shared/utils/categorize-render-errors.test.tscode/core/src/shared/utils/categorize-render-errors.tscode/core/src/shared/utils/test-result-types.tscode/core/src/telemetry/event-cache.tscode/core/src/telemetry/index.tscode/core/src/telemetry/types.tsscripts/tsconfig.json
💤 Files with no reviewable changes (1)
- scripts/tsconfig.json
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
… into sidnioulz/agentic-telemetry-ws2
Base automatically changed from
sidnioulz/agentic-telemetry-ws1
to
project/sb-agentic-setup
April 14, 2026 14:04
yannbf
commented
Apr 14, 2026
yannbf
commented
Apr 14, 2026
yannbf
commented
Apr 14, 2026
Sidnioulz
reviewed
Apr 15, 2026
Sidnioulz
reviewed
Apr 15, 2026
Sidnioulz
reviewed
Apr 15, 2026
Sidnioulz
reviewed
Apr 15, 2026
Sidnioulz
approved these changes
Apr 15, 2026
Member
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
We need to update the disabled telemetry detection, and see comment on error categorisation. All the rest LGTM!
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/event-log-collector.ts (1)
75-85:⚠️ Potential issue | 🟠 Major
--include/--excludecurrently filter display only, not collection.Docs at Line 13-Line 14 say “Only collect / Skip events”, but Line 73 and Line 81-Line 85 still store/write every event regardless of filter. This makes the option behavior inconsistent with its documented contract.
Suggested fix (if “collect” semantics are intended)
const eventType = data.eventType || 'unknown'; const entry = { receivedAt: new Date().toISOString(), ...data }; - events.push(entry); - - if (matchesFilter(eventType)) { + if (matchesFilter(eventType)) { + events.push(entry); console.log(`\n\x1b[1;32m[telemetry] ${eventType}\x1b[0m`); const logged = hideMetadata ? { ...data, metadata: undefined } : data; console.log(JSON.stringify(logged, null, 2)); - } - - await writeFile( - resolve(LOG_DIR, `events-${new Date().toISOString().slice(0, 10)}.jsonl`), - JSON.stringify(entry) + '\n', - { flag: 'a' } - ); + await writeFile( + resolve(LOG_DIR, `events-${new Date().toISOString().slice(0, 10)}.jsonl`), + JSON.stringify(entry) + '\n', + { flag: 'a' } + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/event-log-collector.ts` around lines 75 - 85, The code currently calls writeFile for every incoming event (using writeFile(resolve(LOG_DIR, `events-${new Date().toISOString().slice(0, 10)}.jsonl`), JSON.stringify(entry) + '\n', { flag: 'a' })) regardless of the include/exclude filter; change the logic so persistence follows the same filter as display by wrapping the writeFile call in the same matchesFilter(eventType) check (or apply the inverse when using an exclude mode), and when hideMetadata is enabled ensure the persisted object matches the displayed logged variable (use the same logged value instead of entry) so only filtered events are stored and stored payloads respect hideMetadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/eval/lib/grade.ts`:
- Line 264: Call runStoryTests with the ghostRun flag set to true when grading
component candidates so STORYBOOK_COMPONENT_PATHS gets injected; change the
invocation where result is assigned (the call to runStoryTests(candidates, {
cwd: projectPath })) to include ghostRun: true in the options object so the
component-transform plugin runs for candidate component files and avoids "No
tests found" failures.
In `@scripts/event-log-collector.ts`:
- Around line 35-39: The getFlag function's current logic uses args.indexOf(arg)
which can return the wrong index when flags repeat and doesn't check for a
missing value; change getFlag to iterate with an index (e.g., for (let i = 0; i
< args.length; i++)) so you can use the current index i to read the next token
safely, and for the exact-match case ensure i + 1 < args.length and that args[i
+ 1] is not another flag token (e.g., doesn't start with '-' or with the flag
prefix) before returning it; keep the existing startsWith(`${flag}=`) branch
unchanged and return undefined if no valid value is found.
- Around line 45-46: The RegExp constructors for includeRegex and excludeRegex
can throw on malformed patterns; wrap the creation of includeRegex (from
includePattern) and excludeRegex (from excludePattern) in try/catch blocks,
validate the pattern strings before constructing, and on error log a clear
user-facing message including the invalid pattern and the regex error, then exit
non-zero (or return) so the script fails cleanly; ensure you still set
includeRegex/excludeRegex to null when the corresponding pattern is falsy or
when you decide to continue.
---
Outside diff comments:
In `@scripts/event-log-collector.ts`:
- Around line 75-85: The code currently calls writeFile for every incoming event
(using writeFile(resolve(LOG_DIR, `events-${new Date().toISOString().slice(0,
10)}.jsonl`), JSON.stringify(entry) + '\n', { flag: 'a' })) regardless of the
include/exclude filter; change the logic so persistence follows the same filter
as display by wrapping the writeFile call in the same matchesFilter(eventType)
check (or apply the inverse when using an exclude mode), and when hideMetadata
is enabled ensure the persisted object matches the displayed logged variable
(use the same logged value instead of entry) so only filtered events are stored
and stored payloads respect hideMetadata.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9cf16072-4062-43e1-bdd0-615d03bcbeb1
📒 Files selected for processing (16)
code/addons/vitest/src/vitest-plugin/agent-telemetry-reporter.test.tscode/addons/vitest/src/vitest-plugin/agent-telemetry-reporter.tscode/core/src/core-server/index.tscode/core/src/core-server/server-channel/ai-setup-channel.tscode/core/src/core-server/server-channel/ghost-stories-channel.test.tscode/core/src/core-server/server-channel/ghost-stories-channel.tscode/core/src/core-server/utils/ghost-stories/parse-vitest-report.tscode/core/src/core-server/utils/ghost-stories/run-story-tests.tscode/core/src/core-server/withTelemetry.tscode/core/src/shared/utils/categorize-render-errors.test.tscode/core/src/shared/utils/to-story-test-result.test.tscode/core/src/shared/utils/to-story-test-result.tscode/core/src/telemetry/ai-setup-utils.tscode/core/src/telemetry/types.tsscripts/eval/lib/grade.tsscripts/event-log-collector.ts
✅ Files skipped from review due to trivial changes (2)
- code/core/src/core-server/withTelemetry.ts
- code/core/src/telemetry/types.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- code/core/src/shared/utils/categorize-render-errors.test.ts
- code/addons/vitest/src/vitest-plugin/agent-telemetry-reporter.test.ts
- code/core/src/core-server/index.ts
- code/core/src/core-server/utils/ghost-stories/parse-vitest-report.ts
| logger.logStep(`Found ${candidates.length} candidate component(s) for ${label}`); | ||
|
|
||
| const result = await runGhostStories(candidates, { cwd: projectPath }); | ||
| const result = await runStoryTests(candidates, { cwd: projectPath }); |
Contributor
There was a problem hiding this comment.
Pass ghostRun: true for component-candidate grading.
Line 264 currently runs runStoryTests without ghostRun. That skips STORYBOOK_COMPONENT_PATHS injection (see code/core/src/core-server/utils/ghost-stories/run-story-tests.ts, Line 50-Line 52), so the Vitest component-transform plugin won’t run for candidate component files and this path can degrade into "No tests found" results.
💡 Proposed fix
- const result = await runStoryTests(candidates, { cwd: projectPath });
+ const result = await runStoryTests(candidates, { cwd: projectPath, ghostRun: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/eval/lib/grade.ts` at line 264, Call runStoryTests with the ghostRun
flag set to true when grading component candidates so STORYBOOK_COMPONENT_PATHS
gets injected; change the invocation where result is assigned (the call to
runStoryTests(candidates, { cwd: projectPath })) to include ghostRun: true in
the options object so the component-transform plugin runs for candidate
component files and avoids "No tests found" failures.
Comment on lines
+35
to
+39
| const getFlag = (flag: string): string | undefined => { | ||
| for (const arg of args) { | ||
| if (arg === flag) return args[args.indexOf(arg) + 1]; | ||
| if (arg.startsWith(`${flag}=`)) return arg.slice(flag.length + 1); | ||
| } |
Contributor
There was a problem hiding this comment.
Guard flag-value parsing against repeated flags and missing values.
Line 37 uses args.indexOf(arg), which can pick the first occurrence when a flag is repeated and can also treat another flag token as a value.
Suggested fix
const args = process.argv.slice(2);
const getFlag = (flag: string): string | undefined => {
- for (const arg of args) {
- if (arg === flag) return args[args.indexOf(arg) + 1];
+ for (let i = 0; i < args.length; i += 1) {
+ const arg = args[i];
+ if (arg === flag) {
+ const next = args[i + 1];
+ return next && !next.startsWith('--') ? next : undefined;
+ }
if (arg.startsWith(`${flag}=`)) return arg.slice(flag.length + 1);
}
return undefined;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const getFlag = (flag: string): string | undefined => { | |
| for (const arg of args) { | |
| if (arg === flag) return args[args.indexOf(arg) + 1]; | |
| if (arg.startsWith(`${flag}=`)) return arg.slice(flag.length + 1); | |
| } | |
| const getFlag = (flag: string): string | undefined => { | |
| for (let i = 0; i < args.length; i += 1) { | |
| const arg = args[i]; | |
| if (arg === flag) { | |
| const next = args[i + 1]; | |
| return next && !next.startsWith('--') ? next : undefined; | |
| } | |
| if (arg.startsWith(`${flag}=`)) return arg.slice(flag.length + 1); | |
| } | |
| return undefined; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/event-log-collector.ts` around lines 35 - 39, The getFlag function's
current logic uses args.indexOf(arg) which can return the wrong index when flags
repeat and doesn't check for a missing value; change getFlag to iterate with an
index (e.g., for (let i = 0; i < args.length; i++)) so you can use the current
index i to read the next token safely, and for the exact-match case ensure i + 1
< args.length and that args[i + 1] is not another flag token (e.g., doesn't
start with '-' or with the flag prefix) before returning it; keep the existing
startsWith(`${flag}=`) branch unchanged and return undefined if no valid value
is found.
Comment on lines
+45
to
+46
| const includeRegex = includePattern ? new RegExp(includePattern) : null; | ||
| const excludeRegex = excludePattern ? new RegExp(excludePattern) : null; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify unguarded dynamic RegExp construction in this script
rg -n "new RegExp\\(" scripts/event-log-collector.ts
rg -n -C2 "includePattern|excludePattern" scripts/event-log-collector.tsRepository: storybookjs/storybook
Length of output: 943
🏁 Script executed:
wc -l scripts/event-log-collector.tsRepository: storybookjs/storybook
Length of output: 99
🏁 Script executed:
cat -n scripts/event-log-collector.tsRepository: storybookjs/storybook
Length of output: 5530
Wrap RegExp construction in error handling to provide clear feedback on invalid patterns.
Lines 45-46 throw synchronously on malformed regex patterns (e.g., unmatched (), preventing the script from starting. The current code offers no user-friendly error message when this occurs.
Suggested fix
const includePattern = getFlag('--include');
const excludePattern = getFlag('--exclude');
-const includeRegex = includePattern ? new RegExp(includePattern) : null;
-const excludeRegex = excludePattern ? new RegExp(excludePattern) : null;
+const toRegex = (pattern: string | undefined, name: string): RegExp | null => {
+ if (!pattern) return null;
+ try {
+ return new RegExp(pattern);
+ } catch (error) {
+ throw new Error(`Invalid ${name} regex "${pattern}": ${(error as Error).message}`);
+ }
+};
+const includeRegex = toRegex(includePattern, '--include');
+const excludeRegex = toRegex(excludePattern, '--exclude');
const hideMetadata = args.includes('--no-metadata');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const includeRegex = includePattern ? new RegExp(includePattern) : null; | |
| const excludeRegex = excludePattern ? new RegExp(excludePattern) : null; | |
| const includePattern = getFlag('--include'); | |
| const excludePattern = getFlag('--exclude'); | |
| const toRegex = (pattern: string | undefined, name: string): RegExp | null => { | |
| if (!pattern) return null; | |
| try { | |
| return new RegExp(pattern); | |
| } catch (error) { | |
| throw new Error(`Invalid ${name} regex "${pattern}": ${(error as Error).message}`); | |
| } | |
| }; | |
| const includeRegex = toRegex(includePattern, '--include'); | |
| const excludeRegex = toRegex(excludePattern, '--exclude'); | |
| const hideMetadata = args.includes('--no-metadata'); |
🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 45-45: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(excludePattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/event-log-collector.ts` around lines 45 - 46, The RegExp constructors
for includeRegex and excludeRegex can throw on malformed patterns; wrap the
creation of includeRegex (from includePattern) and excludeRegex (from
excludePattern) in try/catch blocks, validate the pattern strings before
constructing, and on error log a clear user-facing message including the invalid
pattern and the regex error, then exit non-zero (or return) so the script fails
cleanly; ensure you still set includeRegex/excludeRegex to null when the
corresponding pattern is falsy or when you decide to continue.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/core/src/telemetry/ai-setup-utils.test.ts (1)
254-259: Consider extracting a helper for payload-factory retrieval/assertion.The same
mock.calls[0][1]extraction andresolves.toMatchObjectpattern is repeated three times; a small helper would keep the tests tighter and easier to maintain.Also applies to: 276-279, 318-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/telemetry/ai-setup-utils.test.ts` around lines 254 - 259, Extract a small test helper (e.g., getAndAssertPayloadFactory or assertTelemetryPayloadFactory) that accepts the telemetry mock, expected object, and optional call index; inside it, retrieve the factory via vi.mocked(telemetry).mock.calls[callIndex ?? 0][1] as () => Promise<unknown>, invoke await expect(factory()).resolves.toMatchObject(expected), and replace the duplicated extraction/assertion lines at the three locations (the current uses of telemetry.mock.calls[0][1] and similar at lines ~254, ~276, ~318) with calls to this helper to DRY the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/telemetry/ai-setup-utils.test.ts`:
- Around line 73-78: The shared mock currently executes payload factories inside
vi.mocked(telemetry).mockImplementation which causes collectAiSetupEvidence
tests to run payload-building twice; change the mock so it does not call
functions but instead returns payloadOrFactory as-is (i.e., if payloadOrFactory
is a function, return the function rather than invoking it) so tests can capture
and invoke the factory themselves; update the mockImplementation in
ai-setup-utils.test.ts (the vi.mocked(telemetry).mockImplementation) accordingly
and ensure tests that call collectAiSetupEvidence still assert by invoking the
captured factory.
---
Nitpick comments:
In `@code/core/src/telemetry/ai-setup-utils.test.ts`:
- Around line 254-259: Extract a small test helper (e.g.,
getAndAssertPayloadFactory or assertTelemetryPayloadFactory) that accepts the
telemetry mock, expected object, and optional call index; inside it, retrieve
the factory via vi.mocked(telemetry).mock.calls[callIndex ?? 0][1] as () =>
Promise<unknown>, invoke await
expect(factory()).resolves.toMatchObject(expected), and replace the duplicated
extraction/assertion lines at the three locations (the current uses of
telemetry.mock.calls[0][1] and similar at lines ~254, ~276, ~318) with calls to
this helper to DRY the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 374c8ca7-e3d6-4614-bd66-7a366ec73966
📒 Files selected for processing (1)
code/core/src/telemetry/ai-setup-utils.test.ts
Comment on lines
+73
to
+78
| vi.mocked(telemetry).mockImplementation(async (_eventType, payloadOrFactory) => { | ||
| if (typeof payloadOrFactory === 'function') { | ||
| return payloadOrFactory(); | ||
| } | ||
| return payloadOrFactory; | ||
| }); |
Contributor
There was a problem hiding this comment.
Avoid executing the telemetry payload factory inside the shared mock.
This runs payload-building logic during collectAiSetupEvidence() and then again when the test manually invokes the captured factory, which makes these tests more brittle for non-idempotent payloads.
Suggested change
beforeEach(() => {
vi.resetAllMocks();
- vi.mocked(telemetry).mockImplementation(async (_eventType, payloadOrFactory) => {
- if (typeof payloadOrFactory === 'function') {
- return payloadOrFactory();
- }
- return payloadOrFactory;
- });
+ vi.mocked(telemetry).mockResolvedValue(undefined);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/telemetry/ai-setup-utils.test.ts` around lines 73 - 78, The
shared mock currently executes payload factories inside
vi.mocked(telemetry).mockImplementation which causes collectAiSetupEvidence
tests to run payload-building twice; change the mock so it does not call
functions but instead returns payloadOrFactory as-is (i.e., if payloadOrFactory
is a function, return the function rather than invoking it) so tests can capture
and invoke the factory themselves; update the mockImplementation in
ai-setup-utils.test.ts (the vi.mocked(telemetry).mockImplementation) accordingly
and ensure tests that call collectAiSetupEvidence still assert by invoking the
captured factory.
This was referenced Apr 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #
What I did
This PR does the following:
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This pull request has been released as version
0.0.0-pr-34537-sha-cd66a6a9. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-34537-sha-cd66a6a9 sandboxor in an existing project withnpx storybook@0.0.0-pr-34537-sha-cd66a6a9 upgrade.More information
0.0.0-pr-34537-sha-cd66a6a9sidnioulz/agentic-telemetry-ws2cd66a6a91776173670)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=34537Summary by CodeRabbit
New Features
Improvements
Tests