Telemetry: Add sidebar filter telemetry for change detection#34533
Open
valentinpalkovic wants to merge 5 commits intonextfrom
Open
Telemetry: Add sidebar filter telemetry for change detection#34533valentinpalkovic wants to merge 5 commits intonextfrom
valentinpalkovic wants to merge 5 commits intonextfrom
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new core event SIDEBAR_FILTER_CHANGED and telemetry event 'sidebar-filter'; emits that event during URL-based init and user interactions in the sidebar, wires the server telemetry channel to forward the payload, introduces status-count helpers, refactors 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 filters from URL & 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: Compute activeTagFilters, activeStatusFilters, storyCounts
FilterPanel->>TelemetryChannel: emit(SIDEBAR_FILTER_CHANGED, {action, filterType, ...})
TelemetryChannel->>Telemetry: emit('sidebar-filter', payload)
end
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.
🧹 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
Comment on lines
+1272
to
+1277
| const targetStatuses = new Set([...initialIncludedStatuses, ...initialExcludedStatuses]); | ||
| const statusCounts = fullStatusStore.countByValue(); | ||
| for (const statusValue of targetStatuses) { | ||
| if (statusCounts[statusValue] !== undefined) { | ||
| storyCounts[statusValue] = statusCounts[statusValue]; | ||
| } |
Contributor
There was a problem hiding this comment.
Emit 0 for active status filters with no matches.
fullStatusStore.countByValue() only includes statuses that currently exist in the store. With the current guard, an active URL status filter that matches zero stories disappears from storyCounts entirely, so the emitted payload no longer contains a per-filter count for every active status filter.
Suggested patch
- for (const statusValue of targetStatuses) {
- if (statusCounts[statusValue] !== undefined) {
- storyCounts[statusValue] = statusCounts[statusValue];
- }
- }
+ for (const statusValue of targetStatuses) {
+ storyCounts[statusValue] = statusCounts[statusValue] ?? 0;
+ }📝 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 targetStatuses = new Set([...initialIncludedStatuses, ...initialExcludedStatuses]); | |
| const statusCounts = fullStatusStore.countByValue(); | |
| for (const statusValue of targetStatuses) { | |
| if (statusCounts[statusValue] !== undefined) { | |
| storyCounts[statusValue] = statusCounts[statusValue]; | |
| } | |
| const targetStatuses = new Set([...initialIncludedStatuses, ...initialExcludedStatuses]); | |
| const statusCounts = fullStatusStore.countByValue(); | |
| for (const statusValue of targetStatuses) { | |
| 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 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.
…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>
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
1 participant
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:
Each event includes the full active filter state and per-filter story counts. User-defined tag filters are explicitly excluded from telemetry.
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!
.storybook/main.ts:features: { changeDetection: true }yarn task --task sandbox --start-from auto --template react-vite/default-tssidebarFilterChangedevent is emitted on the channel?statuses=status-value%3AnewURL param → verify a URL-triggered telemetry event fires once on loadDocumentation
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