Telemetry: Add sidebar filter telemetry for change detection#34533
Merged
Conversation
In mobile mode, clicking a story calls setMobileMenuOpen(false), which starts a 300ms exit transition on the Modal. After it completes, children are unmounted (unmountOnExit: true). On slow sandboxes like Angular webpack, by the time waitForURL resolves the tree is already unmounted, so the storyLink locator finds nothing and toHaveAttribute fails with "element(s) not found". URL verification already confirms correct navigation, so we safely skip the attribute check when the element is gone. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…_CHANGED event Adds a new channel event emitted when users toggle tag/status filters in the sidebar (interaction) or load Storybook with filters pre-set via URL params (url). The telemetry channel forwards the payload to the sidebar-filter event type. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ookup - stories.ts: extract addFilters/removeFilters helpers to deduplicate the identical set-manipulation logic in addTagFilters, addStatusFilters, removeTagFilters, and removeStatusFilters - FilterPanel.tsx: collapse the duplicated just-changed filter lookup in emitFilterTelemetry into a single ternary + shared assignment Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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:
📝 WalkthroughWalkthroughAdds a new core event SIDEBAR_FILTER_CHANGED and telemetry event 'sidebar-filter'; emits it both on URL-derived init and on user sidebar interactions, wires server telemetry channel to forward the payload, introduces status-count helpers and shared filter add/remove logic, and adds tests and a globals export. Changes
Sequence DiagramsequenceDiagram
participant User
participant FilterPanel
participant StoriesModule
participant TelemetryChannel
participant Telemetry
rect rgba(100, 150, 200, 0.5)
Note over StoriesModule,Telemetry: URL-based initialization flow
StoriesModule->>StoriesModule: init() completes fetchIndex()
StoriesModule->>StoriesModule: detect built-in URL filters & compute storyCounts
StoriesModule->>TelemetryChannel: emit(SIDEBAR_FILTER_CHANGED, {trigger: "url", ...})
TelemetryChannel->>Telemetry: emit('sidebar-filter', payload)
end
rect rgba(150, 200, 100, 0.5)
Note over User,Telemetry: User-driven filter interaction
User->>FilterPanel: click checkbox / invert
FilterPanel->>FilterPanel: computeFilterTelemetryPayload(...)
FilterPanel->>TelemetryChannel: emit(SIDEBAR_FILTER_CHANGED, {trigger: "interaction", changed, ...})
TelemetryChannel->>Telemetry: emit('sidebar-filter', payload)
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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/core-server/server-channel/telemetry-channel.test.ts (1)
7-11: Consider usingspy: trueoption for mocks.Per coding guidelines, Vitest mocks should use the
spy: trueoption. While the current mock implementation works correctly, aligning with the project's mocking conventions would improve consistency.♻️ Suggested refactor
-vi.mock('storybook/internal/telemetry', () => ({ - telemetry: vi.fn(), - getLastEvents: vi.fn().mockResolvedValue({}), - getSessionId: vi.fn().mockResolvedValue('test-session-id'), -})); +vi.mock('storybook/internal/telemetry', { spy: true });Then set up mock implementations in a
beforeEachblock:beforeEach(() => { vi.mocked(telemetry).mockImplementation(vi.fn()); vi.mocked(getLastEvents).mockResolvedValue({}); vi.mocked(getSessionId).mockResolvedValue('test-session-id'); });As per coding guidelines: "Use
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests" and "Implement mock behaviors inbeforeEachblocks in Vitest tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/core-server/server-channel/telemetry-channel.test.ts` around lines 7 - 11, Update the Vitest mocks to use the spy option and move mock behavior setup into a beforeEach: change the top-level vi.mock('storybook/internal/telemetry', ...) call to vi.mock('storybook/internal/telemetry', () => ({ telemetry: vi.fn(), getLastEvents: vi.fn(), getSessionId: vi.fn() }), { spy: true }), then in the test file add a beforeEach that uses vi.mocked(telemetry).mockImplementation(vi.fn()), vi.mocked(getLastEvents).mockResolvedValue({}), and vi.mocked(getSessionId).mockResolvedValue('test-session-id') so the mocked behavior is configured per-test; reference the telemetry, getLastEvents, and getSessionId symbols when applying these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/core-server/server-channel/telemetry-channel.test.ts`:
- Around line 7-11: Update the Vitest mocks to use the spy option and move mock
behavior setup into a beforeEach: change the top-level
vi.mock('storybook/internal/telemetry', ...) call to
vi.mock('storybook/internal/telemetry', () => ({ telemetry: vi.fn(),
getLastEvents: vi.fn(), getSessionId: vi.fn() }), { spy: true }), then in the
test file add a beforeEach that uses
vi.mocked(telemetry).mockImplementation(vi.fn()),
vi.mocked(getLastEvents).mockResolvedValue({}), and
vi.mocked(getSessionId).mockResolvedValue('test-session-id') so the mocked
behavior is configured per-test; reference the telemetry, getLastEvents, and
getSessionId symbols when applying these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69ff6dd0-f974-451b-a522-cd49e80193e0
📒 Files selected for processing (8)
code/core/src/core-events/index.tscode/core/src/core-server/server-channel/telemetry-channel.test.tscode/core/src/core-server/server-channel/telemetry-channel.tscode/core/src/manager-api/modules/stories.tscode/core/src/manager/components/sidebar/FilterPanel.tsxcode/core/src/manager/globals/exports.tscode/core/src/telemetry/types.tscode/e2e-tests/util.ts
|
View your CI Pipeline Execution ↗ for commit ab03ec5
☁️ Nx Cloud last updated this comment at |
…ounting loops Add a canonical countStatusesByValue(allStatuses) helper to the shared status-store module, expose it as fullStatusStore.countByValue(), and replace the three independent status-counting loops in useStatusFilterEntries, stories.ts telemetry init, and FilterPanel.utils.ts with calls to this single implementation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/manager-api/modules/stories.ts`:
- Around line 1272-1277: The loop over targetStatuses should emit a count of 0
for active status filters that have no matches; update the logic in the block
that reads targetStatuses, statusCounts (from fullStatusStore.countByValue())
and storyCounts so that for each statusValue you assign storyCounts[statusValue]
= statusCounts[statusValue] ?? 0 (or equivalent check for undefined) instead of
skipping when undefined, ensuring every active filter in targetStatuses is
present in storyCounts.
🪄 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: b9f4282f-0b7e-4d74-b0c4-69cdd7bfbf73
📒 Files selected for processing (5)
code/core/src/manager-api/modules/stories.tscode/core/src/manager/components/sidebar/FilterPanel.utils.tscode/core/src/manager/components/sidebar/useFilterData.tsxcode/core/src/shared/status-store/index.test.tscode/core/src/shared/status-store/index.ts
✅ Files skipped from review due to trivial changes (1)
- code/core/src/manager/components/sidebar/FilterPanel.utils.ts
…lemetry Extract computeFilterTelemetryPayload() as a pure helper that applies the pending action to the current filter arrays before emitting, so the SIDEBAR_FILTER_CHANGED payload always reflects the state after the toggle rather than the stale pre-change React state. Also removes the partial workaround that only fixed storyCounts but left activeTagFilters/activeStatusFilters stale, and drops the unnecessary async keyword from the telemetry-channel handler. Adds 19 unit tests covering all action/filterType combinations. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/core-server/server-channel/telemetry-channel.test.ts`:
- Around line 7-16: The mock of 'storybook/internal/telemetry' should use
Vitest's spy-based mocking and move behavior setup into beforeEach: change
vi.mock(...) to include the { spy: true } option, remove inline mock
implementations for telemetry, getLastEvents, getSessionId,
isTelemetryModuleEnabled, and setTelemetryEnabled, and in beforeEach import the
module with await import('storybook/internal/telemetry') then wrap it with
vi.mocked(..., true) and assign mock implementations there (e.g.,
mocked.getLastEvents.mockResolvedValue({}),
mocked.getSessionId.mockResolvedValue('test-session-id'),
mocked.isTelemetryModuleEnabled.mockReturnValue(true), mocked.telemetry =
vi.fn(), mocked.setTelemetryEnabled = vi.fn()) so tests use spy-mocks and follow
repository conventions.
🪄 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: 02afdf30-4b4e-4a77-b23a-821f6479414e
📒 Files selected for processing (4)
code/core/src/core-server/server-channel/telemetry-channel.test.tscode/core/src/core-server/server-channel/telemetry-channel.tscode/core/src/manager/components/sidebar/useFilterData.tsxcode/core/src/telemetry/types.ts
✅ Files skipped from review due to trivial changes (1)
- code/core/src/telemetry/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/manager/components/sidebar/useFilterData.tsx
…and remount - FilterPanel reads post-commit state via api.getState() and awaits filter mutations before emitting, so successive rapid toggles report the true active filter set instead of the previous render's stale props. - URL telemetry in stories.ts is guarded by a module-level flag so manager remounts/HMR don't re-fire the event for the same page load. - URL telemetry omits status filters when features.changeDetection is off, matching the sidebar gating in useFilterData. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
code/core/src/manager-api/modules/stories.ts (1)
1287-1291:⚠️ Potential issue | 🟠 MajorKeep active status filters in
storyCountseven when they match nothing.If an active URL status filter has zero matches,
countByValue()can omit that key and this loop drops the filter from the payload entirely. Emit0instead so every active status filter is represented consistently.Suggested fix
const statusCounts = fullStatusStore.countByValue(); for (const statusValue of targetStatuses) { - if (statusCounts[statusValue] !== undefined) { - storyCounts[statusValue] = statusCounts[statusValue]; - } + storyCounts[statusValue] = statusCounts[statusValue] ?? 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager-api/modules/stories.ts` around lines 1287 - 1291, The loop that builds storyCounts uses fullStatusStore.countByValue() (statusCounts) and currently skips keys not present, causing active filters in targetStatuses to be omitted when zero; update the loop over targetStatuses so that for each statusValue you set storyCounts[statusValue] = statusCounts[statusValue] ?? 0 (or otherwise assign 0 when statusCounts has no entry) to guarantee every active status filter from targetStatuses appears in storyCounts even if there are zero matches.
🤖 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/manager-api/modules/stories.ts`:
- Around line 1264-1273: The telemetry currently counts built-in tag defaults as
URL-triggered because initialIncluded/initialExcluded fall back to defaultTags
when tags is absent; change detection should only mark filters as trigger: 'url'
when the corresponding query param actually exists. Update the logic around
initialIncluded/initialExcluded and
includedStatusesForTelemetry/excludedStatusesForTelemetry to check the presence
of the query params (e.g. a truthy tags variable for tags and the statuses query
param for statuses) before using them for telemetry; keep using
changeDetectionEnabled but gate
includedStatusesForTelemetry/excludedStatusesForTelemetry and the builtIn tag
detection on the actual query-param presence so defaults from defaultTags or
presets are not counted as URL-driven filters.
---
Duplicate comments:
In `@code/core/src/manager-api/modules/stories.ts`:
- Around line 1287-1291: The loop that builds storyCounts uses
fullStatusStore.countByValue() (statusCounts) and currently skips keys not
present, causing active filters in targetStatuses to be omitted when zero;
update the loop over targetStatuses so that for each statusValue you set
storyCounts[statusValue] = statusCounts[statusValue] ?? 0 (or otherwise assign 0
when statusCounts has no entry) to guarantee every active status filter from
targetStatuses appears in storyCounts even if there are zero matches.
🪄 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: 7fd35daf-6275-41db-b1b7-f3c69bd09eaf
📒 Files selected for processing (2)
code/core/src/manager-api/modules/stories.tscode/core/src/manager/components/sidebar/FilterPanel.tsx
Move SIDEBAR_FILTER_CHANGED emission out of FilterPanel.tsx and into the manager-api stories module so the API owns the post-mutation state. Drop the projection helper, the 220-line FilterPanel utility test, and the standalone countByValue store method. Collapse the addFilters/removeFilters key plumbing into a FILTER_KEYS map. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
emitFilterTelemetry crashed in stories.test.ts when state.includedTagFilters was undefined; restore the `?? []` defensive read present in the previous FilterPanel-side implementation. Also tighten the mobile data-selected check in the e2e helper: isVisible() could pass before the drawer unmounted, leaving toHaveAttribute to fail on a missing element. Switch to a short-timeout assertion wrapped in try/catch so navigation success (already confirmed via waitForURL) is the source of truth on mobile. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ghengeveld
approved these changes
Apr 28, 2026
Contributor
Author
|
@copilot Fix requested comments |
…port and extract BUILT_IN_TAG_IDS constant Agent-Logs-Url: https://github.com/storybookjs/storybook/sessions/43a8888e-777f-4c0c-b3b3-c92ff976c9b1 Co-authored-by: valentinpalkovic <5889929+valentinpalkovic@users.noreply.github.com>
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 184 | 184 | 0 |
| Self size | 782 KB | 782 KB | 0 B |
| Dependency size | 68.20 MB | 68.22 MB | 🚨 +18 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 177 | 177 | 0 |
| Self size | 32 KB | 32 KB | 0 B |
| Dependency size | 66.73 MB | 66.74 MB | 🚨 +18 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
16 tasks
This was referenced Apr 29, 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
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
Added a
sidebar-filtertelemetry event to track usage of built-in tag filters (_docs,_play,_test) and change detection status filters (new,modified,affected) in the sidebar.The event fires in two scenarios:
include/exclude/remove)Each event includes the full active filter state and per-filter story counts. User-defined tag filters are explicitly excluded from telemetry.
Telemetry emission is owned by the
manager-apistories module —FilterPanel.tsxonly calls the public API, and the API emits the canonical post-mutation payload. A singleemitFilterTelemetry(trigger, changed?)helper services both the URL-init path and the interaction path.addFilters/removeFiltersuse aFILTER_KEYSconst map so tag and status share one code path.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Unit tests cover:
code/core/src/shared/status-store/index.test.ts—countStatusesByValueaggregation behaviourcode/core/src/core-server/server-channel/telemetry-channel.test.ts—SIDEBAR_FILTER_CHANGEDchannel listener forwards totelemetry('sidebar-filter', payload)Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Already verified end-to-end
Tested live in the internal Storybook UI (
yarn storybook:ui) withSTORYBOOK_TELEMETRY_DEBUG=1anddisableTelemetry: false, observing the actual telemetry payloads emitted from the server channel:DocumentationON via checkboxtrigger: "interaction",changed.action: "include",_docsinactiveTagFilters.included,storyCounts._docs: 60DocumentationOFF via checkboxchanged.action: "remove", no_docsin active filters,_docsnot instoryCountsDocumentationchanged.action: "exclude",_docsinactiveTagFilters.excludedNewstatus filter ONchanged.filterType: "status",changed.filterId: "status-value:new",changed.action: "include"?tags=$playtrigger: "url",_playinactiveTagFilters.included,storyCounts._play: 339vitest)Reproduction steps
code/.storybook/main.ts:features: { changeDetection: true }(already set in this branch)disableTelemetry: falseincode/.storybook/main.tsand run withSTORYBOOK_TELEMETRY_DEBUG=1 yarn storybook:uihttp://localhost:6006, open the Tag filters dialog from the sidebar searchDocumentation) → terminal logs[telemetry]block witheventType: "sidebar-filter"andtrigger: "interaction"action: "exclude"New) → event withfilterType: "status"http://localhost:6006/?tags=$play&path=/story/<any>→ URL-triggered event fires once withtrigger: "url"Documentation
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests