UI: Make TagsFilter state persistent#33374
Conversation
Sidnioulz
force-pushed
the
sidnioulz/issue-32986
branch
from
084b811 to
93c4829
Compare
|
View your CI Pipeline Execution ↗ for commit 1fdeddf
☁️ Nx Cloud last updated this comment at |
|
@JReinhold @ghengeveld I'd love some feedback from you guys on the approach and how far you think I should take module mocking in the short term. The first few stories in TagsFilter.stories.ts use the mocked module, though with limitations (see #32986 (comment)) |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 49 | 49 | 0 |
| Self size | 20.43 MB | 20.45 MB | 🚨 +27 KB 🚨 |
| Dependency size | 16.54 MB | 16.54 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 183 | 183 | 0 |
| Self size | 780 KB | 780 KB | 0 B |
| Dependency size | 67.62 MB | 67.64 MB | 🚨 +27 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 176 | 176 | 0 |
| Self size | 32 KB | 32 KB | 🚨 +36 B 🚨 |
| Dependency size | 66.14 MB | 66.17 MB | 🚨 +27 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 1.04 MB | 1.04 MB | 0 B |
| Dependency size | 36.97 MB | 37.00 MB | 🚨 +27 KB 🚨 |
| Bundle Size Analyzer | node | node |
Sidnioulz
force-pushed
the
sidnioulz/issue-32986
branch
from
93c4829 to
d727e6c
Compare
Sidnioulz
force-pushed
the
sidnioulz/issue-32986
branch
from
d727e6c to
13557a1
Compare
|
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:
📝 WalkthroughWalkthroughCentralizes tag-filtering into the manager layout API with dynamic defaults and recompute wiring; refactors sidebar filter components and stories to consume API-driven filters and indexJson; adds persistence gating and a non-persistent testing store factory; adds a small Storybook preview mock. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as TagsFilter / TagsFilterPanel
participant API as Layout API
participant Store
User->>UI: open tags panel / request counts
UI->>API: getIncludedTagFilters() / getExcludedTagFilters()
API->>Store: read layout state (included/excluded, tagPresets)
Store-->>API: return layout arrays
API-->>UI: return filter arrays and defaults
User->>UI: toggle tag / select all / reset
UI->>API: addTagFilters / removeTagFilters / setAllTagFilters / resetTagFilters
API->>Store: persist layout changes (if upstreamPersistence true)
Store->>API: emit state change (e.g., STORY_INDEX_INVALIDATED)
API->>API: recompute filter functions and defaults
API-->>UI: updated getters reflect new state
UI-->>User: UI updates selection and counts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
code/core/src/manager/components/sidebar/TagsFilter.stories.tsx (1)
93-95: Consider adding componentWillUnmount cleanup.The
mountedflag is set incomponentDidMount, but there's no cleanup incomponentWillUnmount. If the store'sonChangecallback fires during unmount, it could cause issues.🔎 Suggested addition
componentDidMount() { this.mounted = true; } + + componentWillUnmount() { + this.mounted = false; + }code/core/src/manager/components/sidebar/TagsFilterPanel.tsx (1)
102-127: Duplicate icon definitions between builtInFilters and builtInFilterIcons.The icons for built-in filters are defined twice:
- Inside the
filtersByIduseMemo (lines 107, 114, 121) as part ofbuiltInFilters- In the separate
builtInFilterIconsuseMemo (lines 157-164)The
iconproperty added tobuiltInFiltersobjects (lines 107, 114, 121) doesn't match theFiltertype definition (lines 22-27) which doesn't include aniconproperty. This property is unused sincerenderLinkaccesses icons viabuiltInFilterIcons.🔎 Remove unused icon property from builtInFilters
const builtInFilters = { _docs: { id: '_docs', type: 'built-in', title: 'Documentation', - icon: <DocumentIcon color={color.gold} />, count: getBuiltInCount(api.getFilterFunction('_docs')), }, _play: { id: '_play', type: 'built-in', title: 'Play', - icon: <PlayHollowIcon color={color.seafoam} />, count: getBuiltInCount(api.getFilterFunction('_play')), }, _test: { id: '_test', type: 'built-in', title: 'Testing', - icon: <BeakerIcon color={color.green} />, count: getBuiltInCount(api.getFilterFunction('_test')), }, };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
code/.storybook/preview.tsxcode/core/src/manager-api/modules/layout.tscode/core/src/manager-api/modules/url.tscode/core/src/manager-api/store.tscode/core/src/manager-api/tests/layout.test.tscode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/TagsFilter.stories.tsxcode/core/src/manager/components/sidebar/TagsFilter.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.stories.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.tsxcode/core/src/types/modules/api.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
code/core/src/manager-api/modules/url.tscode/core/src/types/modules/api.tscode/core/src/manager-api/store.tscode/core/src/manager/components/sidebar/TagsFilter.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.stories.tsxcode/core/src/manager-api/tests/layout.test.tscode/core/src/manager/components/sidebar/TagsFilter.stories.tsxcode/core/src/manager-api/modules/layout.tscode/core/src/manager/components/sidebar/TagsFilterPanel.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/manager-api/modules/url.tscode/core/src/types/modules/api.tscode/core/src/manager-api/store.tscode/core/src/manager/components/sidebar/TagsFilter.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.stories.tsxcode/core/src/manager-api/tests/layout.test.tscode/core/src/manager/components/sidebar/TagsFilter.stories.tsxcode/core/src/manager-api/modules/layout.tscode/core/src/manager/components/sidebar/TagsFilterPanel.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/manager-api/modules/url.tscode/core/src/types/modules/api.tscode/core/src/manager-api/store.tscode/core/src/manager/components/sidebar/TagsFilter.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.stories.tsxcode/core/src/manager-api/tests/layout.test.tscode/core/src/manager/components/sidebar/TagsFilter.stories.tsxcode/core/src/manager-api/modules/layout.tscode/core/src/manager/components/sidebar/TagsFilterPanel.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager-api/modules/url.tscode/core/src/types/modules/api.tscode/core/src/manager-api/store.tscode/core/src/manager/components/sidebar/TagsFilter.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.stories.tsxcode/core/src/manager-api/tests/layout.test.tscode/core/src/manager/components/sidebar/TagsFilter.stories.tsxcode/core/src/manager-api/modules/layout.tscode/core/src/manager/components/sidebar/TagsFilterPanel.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/manager-api/modules/url.tscode/core/src/types/modules/api.tscode/core/src/manager-api/store.tscode/core/src/manager/components/sidebar/TagsFilter.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.stories.tsxcode/core/src/manager-api/tests/layout.test.tscode/core/src/manager/components/sidebar/TagsFilter.stories.tsxcode/core/src/manager-api/modules/layout.tscode/core/src/manager/components/sidebar/TagsFilterPanel.tsx
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{test,spec}.{ts,tsx}: Test files should follow the naming pattern*.test.ts,*.test.tsx,*.spec.ts, or*.spec.tsx
Follow the spy mocking rules defined in.cursor/rules/spy-mocking.mdcfor consistent mocking patterns with Vitest
Files:
code/core/src/manager-api/tests/layout.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Usevi.mock()with thespy: trueoption for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Usevi.mocked()to type and access the mocked functions in Vitest tests
Implement mock behaviors inbeforeEachblocks in Vitest tests
Mock all required dependencies that the test subject uses
Each mock implementation should return a Promise for async functions in Vitest
Mock implementations should match the expected return type of the original function
Mock all required properties and methods that the test subject uses in Vitest tests
Avoid direct function mocking withoutvi.mocked()in Vitest tests
Avoid mock implementations outside ofbeforeEachblocks in Vitest tests
Avoid mocking without thespy: trueoption in Vitest tests
Avoid inline mock implementations within test cases in Vitest tests
Avoid mocking only a subset of required dependencies in Vitest tests
Mock at the highest level of abstraction needed in Vitest tests
Keep mock implementations simple and focused in Vitest tests
Use type-safe mocking withvi.mocked()in Vitest tests
Document complex mock behaviors in Vitest tests
Group related mocks together in Vitest tests
Files:
code/core/src/manager-api/tests/layout.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{test,spec}.{ts,tsx,js,jsx}: Write meaningful unit tests that actually import and call the functions being tested, not just verify syntax patterns
Achieve high test coverage of business logic, aiming for 75%+ coverage of statements/lines
Cover all branches, conditions, edge cases, error paths, and different input variations in unit tests
Usevi.mock()to mock file system, loggers, and other external dependencies in tests
Files:
code/core/src/manager-api/tests/layout.test.ts
🧠 Learnings (22)
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.mock()` with the `spy: true` option for all package and file mocks in Vitest tests
Applied to files:
code/.storybook/preview.tsx
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 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/.storybook/preview.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.stories.tsxcode/core/src/manager/components/sidebar/TagsFilter.stories.tsxcode/core/src/manager-api/modules/layout.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking without the `spy: true` option in Vitest tests
Applied to files:
code/.storybook/preview.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required dependencies that the test subject uses
Applied to files:
code/.storybook/preview.tsxcode/core/src/manager/components/sidebar/TagsFilter.stories.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of `beforeEach` blocks in Vitest tests
Applied to files:
code/.storybook/preview.tsx
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Use `vi.mock()` to mock file system, loggers, and other external dependencies in tests
Applied to files:
code/.storybook/preview.tsx
📚 Learning: 2025-11-24T17:49:31.838Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-24T17:49:31.838Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Follow the spy mocking rules defined in `.cursor/rules/spy-mocking.mdc` for consistent mocking patterns with Vitest
Applied to files:
code/.storybook/preview.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in `beforeEach` blocks in Vitest tests
Applied to files:
code/.storybook/preview.tsxcode/core/src/manager/components/sidebar/TagsFilter.stories.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies in Vitest tests
Applied to files:
code/.storybook/preview.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Place all mocks at the top of the test file before any test cases
Applied to files:
code/.storybook/preview.tsx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/.storybook/preview.tsxcode/core/src/manager/components/sidebar/TagsFilter.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.stories.tsxcode/core/src/manager/components/sidebar/TagsFilter.stories.tsxcode/core/src/manager-api/modules/layout.tscode/core/src/manager/components/sidebar/TagsFilterPanel.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors in Vitest tests
Applied to files:
code/.storybook/preview.tsxcode/core/src/manager/components/sidebar/TagsFilter.stories.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses in Vitest tests
Applied to files:
code/.storybook/preview.tsxcode/core/src/manager/components/sidebar/TagsFilter.stories.tsx
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to code/{renderers}/**/*.{ts,tsx,js,jsx} : Use `logger` from `storybook/internal/client-logger` for client-side logging in browser code
Applied to files:
code/.storybook/preview.tsx
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/.storybook/preview.tsxcode/core/src/manager/components/sidebar/TagsFilter.tsxcode/core/src/manager/components/sidebar/TagsFilter.stories.tsxcode/core/src/manager-api/modules/layout.ts
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/src/manager/components/sidebar/TagsFilter.tsx
📚 Learning: 2025-11-25T11:09:33.798Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 33140
File: code/core/src/manager/components/sidebar/TagsFilter.tsx:247-259
Timestamp: 2025-11-25T11:09:33.798Z
Learning: In the storybookjs/storybook repository, PopoverProvider creates popovers with a dialog role, so using aria-haspopup="dialog" on buttons that trigger PopoverProvider is semantically correct.
Applied to files:
code/core/src/manager/components/sidebar/TagsFilter.tsx
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: The useGlobals hook from storybook/manager-api returns a tuple where the third element (storyGlobals) is typed as Globals, not Globals | undefined. This means TypeScript guarantees it's always defined, making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager-api/modules/layout.ts
📚 Learning: 2025-10-01T15:24:01.060Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 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/core/src/manager/components/sidebar/TagsFilter.stories.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock at the highest level of abstraction needed in Vitest tests
Applied to files:
code/core/src/manager/components/sidebar/TagsFilter.stories.tsx
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx} : Use `logger` from `storybook/internal/node-logger` for server-side logging in Node.js code
Applied to files:
code/core/src/manager-api/modules/layout.ts
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/manager-api/modules/layout.ts
🧬 Code graph analysis (8)
code/core/src/manager-api/modules/url.ts (1)
code/core/src/manager-api/modules/layout.ts (3)
DEFAULT_NAV_SIZE(180-180)DEFAULT_BOTTOM_PANEL_HEIGHT(181-181)DEFAULT_RIGHT_PANEL_WIDTH(182-182)
code/core/src/types/modules/api.ts (1)
code/core/src/types/modules/core-common.ts (1)
TagsOptions(351-351)
code/core/src/manager/components/sidebar/TagsFilter.tsx (2)
code/core/src/manager/components/sidebar/TagsFilterPanel.tsx (1)
TagsFilterPanel(72-302)code/renderers/react/src/componentManifest/fixtures.ts (1)
indexJson(112-191)
code/core/src/manager/components/sidebar/Sidebar.tsx (2)
code/renderers/react/src/componentManifest/fixtures.ts (1)
indexJson(112-191)code/core/src/manager/components/sidebar/TagsFilter.tsx (1)
TagsFilter(46-88)
code/core/src/manager/components/sidebar/TagsFilterPanel.stories.tsx (2)
code/core/src/manager/components/sidebar/TagsFilterPanel.tsx (1)
TagsFilterPanel(72-302)code/core/src/manager/components/sidebar/TagsFilter.stories.tsx (1)
MockAPIDecorator(122-131)
code/core/src/manager-api/tests/layout.test.ts (1)
code/core/src/manager-api/modules/layout.ts (1)
getDefaultLayoutState(184-217)
code/core/src/manager/components/sidebar/TagsFilter.stories.tsx (3)
code/core/src/manager-api/modules/layout.ts (3)
SubAPI(62-162)SubState(36-42)init(288-741)code/core/src/manager-api/modules/url.ts (3)
SubAPI(115-167)SubState(21-23)init(169-284)code/core/src/manager-api/store.ts (1)
createTestingStore(134-159)
code/core/src/manager/components/sidebar/TagsFilterPanel.tsx (3)
code/core/src/csf/story.ts (1)
Tag(15-15)code/core/src/types/modules/api.ts (1)
FilterFunction(77-77)code/core/src/components/index.ts (2)
Link(31-31)Link(87-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: nx
🔇 Additional comments (38)
code/.storybook/preview.tsx (1)
37-38: LGTM!The mock for
@storybook/globalfollows the established pattern in this file and correctly usesspy: trueas per coding guidelines.code/core/src/manager-api/store.ts (4)
30-34: LGTM!The
allowPersistenceoptional property is a clean extension of theUpstreaminterface, maintaining backward compatibility with existing consumers.
50-58: LGTM!The
upstreamPersistenceproperty is correctly initialized with a sensible default (true), ensuring backward compatibility for existing code that doesn't explicitly passallowPersistence.
113-116: LGTM!The updated persistence check correctly gates storage writes on both the
persistenceoption and the upstream persistence flag. This ensures the testing store can disable persistence without modifying other behavior.
134-159: Well-documented testing utility.The
createTestingStorefactory is well-documented and provides a clean API for testing. The implementation correctly:
- Disables persistence via
allowPersistence: false- Handles both function and object patches
- Invokes the
onChangecallback after state updatesOne minor consideration: the
onChangecallback is called synchronously after thecallbackparameter. This matches the expected flow but worth noting for test authors.code/core/src/types/modules/api.ts (2)
77-78: LGTM!The
FilterFunctiontype signature is well-defined and aligns with the built-in and user filter implementations in the layout module.
96-102: LGTM!The new
API_Layoutproperties are well-documented and align with the PR objective of making tag filter state persistent. ThetagPresetsstores initial configuration whileincludedTagFiltersandexcludedTagFiltershold the persisted user selections.code/core/src/manager-api/modules/url.ts (2)
17-17: LGTM!The import change from
defaultLayoutStateto individual constants (DEFAULT_NAV_SIZE,DEFAULT_BOTTOM_PANEL_HEIGHT,DEFAULT_RIGHT_PANEL_WIDTH) aligns with the layout module refactor and keeps the url module decoupled from the full layout state structure.
71-78: LGTM!The usage of the new constants is equivalent to the previous
defaultLayoutState.layout.*access pattern. The behavior remains unchanged while improving clarity.code/core/src/manager-api/tests/layout.test.ts (2)
12-12: LGTM!The import correctly references the new
getDefaultLayoutStatefunction exported from the layout module.
25-31: LGTM!Using
getDefaultLayoutState()inbeforeEachensures each test starts with a fresh default state. This is appropriate since the function derives defaults fromglobal.TAGS_OPTIONSwhich could theoretically vary between test scenarios.code/core/src/manager/components/sidebar/Sidebar.tsx (2)
4-5: LGTM!The removal of
TagsOptionsfrom imports aligns with the architectural change of moving tag filter state to the layout module.
185-185: LGTM!Simplifying the
TagsFilterusage by removing thetagPresetsprop aligns with the PR objective. The component now retrieves tag filter state from the API, making the state persistent across sidebar show/hide cycles.code/core/src/manager/components/sidebar/TagsFilter.tsx (3)
41-44: LGTM!The simplified
TagsFilterPropsinterface correctly removestagPresetssince the component now retrieves filter state from the API.
46-51: LGTM!The component now delegates to the API for tag filter state (
getIncludedTagFilters(),getExcludedTagFilters()), achieving the PR objective of making filter state persistent across sidebar show/hide cycles.
67-84: LGTM!The rendering logic is clean and the accessibility labels properly reflect the active filter count with correct pluralization.
code/core/src/manager-api/modules/layout.ts (8)
44-60: LGTM!The built-in filter functions handle the
excludedparameter correctly:
- When
excluded=true, they returntruefor entries that should NOT be filtered out- When
excluded=false, they returntruefor entries that SHOULD be includedThe
USER_TAG_FILTERfactory follows the same pattern consistently.
166-178: LGTM!The
getDefaultTagsFromPresetfunction is correctly memoized (cache size 1 is appropriate sinceglobal.TAGS_OPTIONSis effectively static) and properly extracts default filter selections from the presets.
180-217: LGTM!The
getDefaultLayoutStatefunction properly constructs the initial layout state with:
- Tag presets from global configuration
- Default included/excluded filters derived from presets
- Standard dimension constants
- Sensible defaults for all layout properties
254-286: Verify filter composition semantics.The filter logic at lines 279-284 implements:
- Included filters: Every group (built-in/user) must have at least one matching filter (OR within group, AND between groups)
- Excluded filters: Every group must have all filters pass (AND within group, AND between groups)
This means if a user includes both
_docsand a custom tagmyTag, an entry must match at least one built-in filter AND at least one user filter. Please verify this AND-between-groups behavior is the intended UX.
641-656: LGTM!The
resetTagFiltersimplementation correctly:
- Derives defaults from the stored
tagPresets- Awaits the state update before recomputing filters
- Uses
'permanent'persistence as appropriate for user preferences
672-697: LGTM!The
addTagFiltersimplementation correctly ensures mutual exclusivity - when adding a tag to one list, it removes it from the other. This prevents invalid states where a tag is both included and excluded.
699-713: LGTM!The
removeTagFiltersimplementation cleanly removes tags from both lists, ensuring consistent state cleanup.
730-739: LGTM!The event handler registration and initialization are correctly wired:
STORY_INDEX_INVALIDATEDtriggers filter recomputation to handle story changes- The
initfunction performs initial filter computationThis ensures filters are always in sync with the story index.
code/core/src/manager/components/sidebar/TagsFilterPanel.stories.tsx (3)
10-85: LGTM! Well-structured test data generator.The
getEntrieshelper provides comprehensive coverage of tag combinations including user tags, built-in tags (autodocs, dev, play-fn, test-fn), and edge cases like long tag names. The conditional filtering whenincludeUserTagsis false correctly retains only built-in tags for testing the BuiltInOnly scenarios.
87-99: Good use of MockAPIDecorator pattern.The meta configuration correctly applies the decorator and provides the necessary args structure. The
api: {} as APIplaceholder with the comment clarifying it will be overridden is a clear approach.
158-191: Clean global state mocking with proper cleanup.The
beforeEachpattern for temporarily overridingglobal.TAGS_OPTIONSwith a cleanup function is the correct approach for isolating story state. The DefaultSelectionModified story correctly combines both the global override and initial layout state.code/core/src/manager/components/sidebar/TagsFilter.stories.tsx (5)
122-131: Well-designed decorator export.The MockAPIDecorator correctly extracts
initialStoryStatefrom parameters and passes it to MockAPIWrapper. This pattern allows individual stories to configure their initial state through parameters.
181-198: Comment clarifies limitation with TAGS_OPTIONS mocking.The comment on lines 181-184 documents a known limitation where resetting uses the global rather than mocked values. This is helpful for maintainers understanding why certain reset scenarios can't be fully tested.
186-198: Robust play function with appropriate timeouts.The play function properly uses
findByRolewith a timeout for async rendering andwaitForfor state change verification. The assertion chain (find button → click → find clear button → expect → click → wait for disappearance) provides good coverage.
200-220: ResetToDefaults story validates reset button behavior.Good test coverage for the reset flow: starts with excluded filters, clicks reset, and verifies the button becomes disabled (indicating filters are now at defaults).
97-119: The pattern of passing modified args viaReact.cloneElementto a Story component is unconventional and potentially fragile.While Storybook's render system does spread args as props to the actual component (TagsFilter), cloning the Story wrapper element with
{ args: {...} }bypasses Storybook's standard arg-passing mechanisms. The Story component is managed internally by Storybook and doesn't expect args as a JSX prop. This test utility may work in practice, but consider using Storybook's standard decorator patterns or parameter injection instead.code/core/src/manager/components/sidebar/TagsFilterPanel.tsx (6)
57-65: BUILT_IN_TAGS set is well-defined.The set correctly contains all the standard Storybook tags that should be treated as built-in and not shown as user tags.
67-70: Clever equality check with documented assumption.The comment clearly explains the assumption that arrays have no duplicates. The implementation using Set size comparison is efficient for this use case.
129-140: Toggle logic is correct and well-structured.The
toggleFiltercallback correctly handles three cases:
- Explicit exclude flag → add to excluded
- Currently selected and toggling → add to included
- Deselecting → remove from filters
142-147: setAllFilters correctly clears excluded filters when selecting all.When
selectedis true, all filter IDs are set as included and excluded is emptied. When false, both arrays are cleared. This matches the expected "Select all" and "Clear filters" behavior.
166-215: renderLink function handles all filter states correctly.The function properly:
- Shows DeleteIcon when excluded, no icon when included, or built-in icon when neutral
- Hides built-in filters with zero count
- Provides appropriate accessibility labels
257-268: Reset button correctly wired to API.The reset button calls
api.resetTagFilters()and is properly disabled when the current selection matches the default. ThehasDefaultSelectioncheck ensures the button only appears when there are defaults configured.
code/core/src/manager/components/sidebar/TagsFilter.stories.tsx
Outdated
Show resolved
Hide resolved
Sidnioulz
force-pushed
the
sidnioulz/issue-32986
branch
from
13557a1 to
c64b660
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
code/core/src/manager/components/sidebar/TagsFilter.stories.tsx (1)
22-91: Class component approach is reasonable for coordinating store and React state.The MockAPIWrapper correctly bridges the testing store with React's component lifecycle. The
mountedflag (line 45, 47-49, 93-95) properly preventssetStatecalls before the component is mounted, avoiding React warnings.However, the assignment
this.state = this.store.getState()on line 90 is confusing since React class components usethis.statefor managed state. Consider renaming to avoid confusion.🔎 Suggested rename to avoid confusion
- this.state = this.store.getState(); + // Store reference not needed as state is managed by the store + // and re-renders are triggered via the onChange callbackOr if you need the reference:
- this.state = this.store.getState(); + this.storeSnapshot = this.store.getState();
🧹 Nitpick comments (1)
code/core/src/manager/components/sidebar/TagsFilterPanel.tsx (1)
102-127: Duplicate icon definitions.Icons are defined within
builtInFilters(lines 107, 114, 121) and again inbuiltInFilterIcons(lines 157-164). This creates React elements twice unnecessarily.🔎 Consider consolidating icon definitions
Define icons once and reuse:
+const BUILT_IN_FILTER_ICONS = { + _docs: <DocumentIcon color={color.gold} />, + _play: <PlayHollowIcon color={color.seafoam} />, + _test: <BeakerIcon color={color.green} />, +} as const; + export const TagsFilterPanel = ({ api, indexJson }: TagsFilterPanelProps) => { // ... const filtersById = useMemo<{ [id: string]: Filter }>(() => { // ... const builtInFilters = { _docs: { id: '_docs', type: 'built-in', title: 'Documentation', - icon: <DocumentIcon color={color.gold} />, + icon: BUILT_IN_FILTER_ICONS._docs, count: getBuiltInCount(api.getFilterFunction('_docs')), }, // ... similar for _play and _test }; }, [api, indexJson.entries]); - const builtInFilterIcons = useMemo( - () => ({ - _docs: <DocumentIcon color={color.gold} />, - _play: <PlayHollowIcon color={color.seafoam} />, - _test: <BeakerIcon color={color.green} />, - }), - [] - ); + // Use BUILT_IN_FILTER_ICONS directly in renderLink
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
code/.storybook/preview.tsxcode/core/src/manager-api/modules/layout.tscode/core/src/manager-api/modules/url.tscode/core/src/manager-api/store.tscode/core/src/manager-api/tests/layout.test.tscode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/TagsFilter.stories.tsxcode/core/src/manager/components/sidebar/TagsFilter.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.stories.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.tsxcode/core/src/types/modules/api.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- code/core/src/types/modules/api.ts
- code/core/src/manager/components/sidebar/Sidebar.tsx
- code/core/src/manager-api/tests/layout.test.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
code/core/src/manager-api/store.tscode/core/src/manager-api/modules/layout.tscode/core/src/manager/components/sidebar/TagsFilter.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.stories.tsxcode/core/src/manager/components/sidebar/TagsFilter.stories.tsxcode/core/src/manager-api/modules/url.tscode/core/src/manager/components/sidebar/TagsFilterPanel.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/manager-api/store.tscode/core/src/manager-api/modules/layout.tscode/core/src/manager/components/sidebar/TagsFilter.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.stories.tsxcode/core/src/manager/components/sidebar/TagsFilter.stories.tsxcode/core/src/manager-api/modules/url.tscode/core/src/manager/components/sidebar/TagsFilterPanel.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/manager-api/store.tscode/core/src/manager-api/modules/layout.tscode/core/src/manager/components/sidebar/TagsFilter.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.stories.tsxcode/core/src/manager/components/sidebar/TagsFilter.stories.tsxcode/core/src/manager-api/modules/url.tscode/core/src/manager/components/sidebar/TagsFilterPanel.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager-api/store.tscode/core/src/manager-api/modules/layout.tscode/core/src/manager/components/sidebar/TagsFilter.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.stories.tsxcode/core/src/manager/components/sidebar/TagsFilter.stories.tsxcode/core/src/manager-api/modules/url.tscode/core/src/manager/components/sidebar/TagsFilterPanel.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/manager-api/store.tscode/core/src/manager-api/modules/layout.tscode/core/src/manager/components/sidebar/TagsFilter.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.stories.tsxcode/core/src/manager/components/sidebar/TagsFilter.stories.tsxcode/core/src/manager-api/modules/url.tscode/core/src/manager/components/sidebar/TagsFilterPanel.tsx
🧠 Learnings (24)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 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/.storybook/preview.tsxcode/core/src/manager-api/modules/layout.tscode/core/src/manager/components/sidebar/TagsFilterPanel.stories.tsxcode/core/src/manager/components/sidebar/TagsFilter.stories.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.mock()` with the `spy: true` option for all package and file mocks in Vitest tests
Applied to files:
code/.storybook/preview.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required dependencies that the test subject uses
Applied to files:
code/.storybook/preview.tsxcode/core/src/manager/components/sidebar/TagsFilter.stories.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking without the `spy: true` option in Vitest tests
Applied to files:
code/.storybook/preview.tsx
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Use `vi.mock()` to mock file system, loggers, and other external dependencies in tests
Applied to files:
code/.storybook/preview.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of `beforeEach` blocks in Vitest tests
Applied to files:
code/.storybook/preview.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Place all mocks at the top of the test file before any test cases
Applied to files:
code/.storybook/preview.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies in Vitest tests
Applied to files:
code/.storybook/preview.tsx
📚 Learning: 2025-11-24T17:49:31.838Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-24T17:49:31.838Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Follow the spy mocking rules defined in `.cursor/rules/spy-mocking.mdc` for consistent mocking patterns with Vitest
Applied to files:
code/.storybook/preview.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid direct function mocking without `vi.mocked()` in Vitest tests
Applied to files:
code/.storybook/preview.tsx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/.storybook/preview.tsxcode/core/src/manager-api/modules/layout.tscode/core/src/manager/components/sidebar/TagsFilter.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.stories.tsxcode/core/src/manager/components/sidebar/TagsFilter.stories.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors in Vitest tests
Applied to files:
code/.storybook/preview.tsxcode/core/src/manager/components/sidebar/TagsFilter.stories.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in `beforeEach` blocks in Vitest tests
Applied to files:
code/.storybook/preview.tsxcode/core/src/manager/components/sidebar/TagsFilter.stories.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses in Vitest tests
Applied to files:
code/.storybook/preview.tsxcode/core/src/manager/components/sidebar/TagsFilter.stories.tsx
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to code/{renderers}/**/*.{ts,tsx,js,jsx} : Use `logger` from `storybook/internal/client-logger` for client-side logging in browser code
Applied to files:
code/.storybook/preview.tsx
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/.storybook/preview.tsxcode/core/src/manager-api/modules/layout.tscode/core/src/manager/components/sidebar/TagsFilter.tsxcode/core/src/manager/components/sidebar/TagsFilter.stories.tsx
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: The useGlobals hook from storybook/manager-api returns a tuple where the third element (storyGlobals) is typed as Globals, not Globals | undefined. This means TypeScript guarantees it's always defined, making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/manager-api/modules/layout.ts
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx} : Use `logger` from `storybook/internal/node-logger` for server-side logging in Node.js code
Applied to files:
code/core/src/manager-api/modules/layout.ts
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/manager-api/modules/layout.ts
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/src/manager/components/sidebar/TagsFilter.tsx
📚 Learning: 2025-11-25T11:09:33.798Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 33140
File: code/core/src/manager/components/sidebar/TagsFilter.tsx:247-259
Timestamp: 2025-11-25T11:09:33.798Z
Learning: In the storybookjs/storybook repository, PopoverProvider creates popovers with a dialog role, so using aria-haspopup="dialog" on buttons that trigger PopoverProvider is semantically correct.
Applied to files:
code/core/src/manager/components/sidebar/TagsFilter.tsx
📚 Learning: 2025-10-01T15:24:01.060Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 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/core/src/manager/components/sidebar/TagsFilter.stories.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock at the highest level of abstraction needed in Vitest tests
Applied to files:
code/core/src/manager/components/sidebar/TagsFilter.stories.tsx
🧬 Code graph analysis (5)
code/core/src/manager-api/store.ts (1)
code/core/src/shared/checklist-store/checklistData.state.ts (1)
initialState(3-29)
code/core/src/manager-api/modules/layout.ts (2)
code/core/src/csf/story.ts (1)
Tag(15-15)code/core/src/types/modules/api.ts (1)
FilterFunction(77-77)
code/core/src/manager/components/sidebar/TagsFilter.stories.tsx (3)
code/core/src/manager-api/modules/url.ts (2)
SubAPI(115-167)init(169-284)code/core/src/manager-api/store.ts (1)
createTestingStore(134-159)code/core/src/manager/components/sidebar/TagsFilter.tsx (1)
TagsFilter(46-88)
code/core/src/manager-api/modules/url.ts (1)
code/core/src/manager-api/modules/layout.ts (3)
DEFAULT_NAV_SIZE(180-180)DEFAULT_BOTTOM_PANEL_HEIGHT(181-181)DEFAULT_RIGHT_PANEL_WIDTH(182-182)
code/core/src/manager/components/sidebar/TagsFilterPanel.tsx (6)
code/renderers/react/src/componentManifest/fixtures.ts (1)
indexJson(112-191)code/core/src/csf/story.ts (1)
Tag(15-15)code/core/src/node-logger/logger/console.ts (1)
count(108-113)code/core/src/types/modules/api.ts (1)
FilterFunction(77-77)code/core/src/components/index.ts (2)
Link(31-31)Link(87-87)code/core/src/core-server/presets/common-preset.ts (1)
title(138-139)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: nx
🔇 Additional comments (21)
code/.storybook/preview.tsx (1)
37-38: Verify necessity of mocking@storybook/global.The
@storybook/globalmodule is already imported on line 5 and used on line 49. Adding this mock may be needed for specific story testing scenarios, but ensure this doesn't cause issues with the direct import usage in this file.code/core/src/manager-api/modules/url.ts (2)
17-17: LGTM!Clean import of layout dimension constants, replacing the previous dependency on
defaultLayoutStateobject properties.
70-78: LGTM!The refactor to use explicit constants (
DEFAULT_NAV_SIZE,DEFAULT_BOTTOM_PANEL_HEIGHT,DEFAULT_RIGHT_PANEL_WIDTH) improves clarity and maintains consistency with the new dynamicgetDefaultLayoutState()approach in the layout module.code/core/src/manager-api/store.ts (3)
50-58: LGTM!The
upstreamPersistenceflag provides a clean mechanism to disable persistence for testing scenarios while maintaining backward compatibility with the defaulttruevalue.
113-116: LGTM!Correctly gates persistence writes with the new
upstreamPersistenceflag, ensuring testing stores don't write to localStorage/sessionStorage.
134-159: Well-documented testing utility.The
createTestingStorefactory provides a clean way to create in-memory stores for testing. The implementation correctly:
- Disables persistence via
allowPersistence: false- Supports both object and function patches
- Invokes callbacks in proper order
code/core/src/manager/components/sidebar/TagsFilter.tsx (2)
46-51: LGTM!Clean refactor moving filter state management to the API. The component now correctly derives
activeFilterCountfrom the API-provided filter arrays.
71-75: LGTM!The aria-label correctly handles singular/plural grammar ("filter" vs "filters"), improving accessibility.
code/core/src/manager/components/sidebar/TagsFilterPanel.stories.tsx (2)
10-85: LGTM!The
getEntrieshelper provides comprehensive test data covering various tag combinations, entry types (story, docs, test), and edge cases like long tag names. The conditional filtering for built-in vs user tags is well-implemented.
159-169: LGTM!Good use of
beforeEachwith a cleanup function to temporarily overrideglobal.TAGS_OPTIONSfor testing default tag selections, ensuring proper test isolation.code/core/src/manager-api/modules/layout.ts (6)
44-60: LGTM!Well-structured built-in filter definitions with clear semantics:
_docs: filters by entry type_play: filters by story type withplay-fntag_test: filters by story subtypeThe
excludedparameter correctly inverts the filter logic.
166-178: LGTM!Good use of
memoizerific(1)to cache the derived default tags from presets, avoiding redundant computation when presets haven't changed.
184-217: LGTM!The dynamic
getDefaultLayoutState()correctly:
- Reads from
global.TAGS_OPTIONSat runtime- Derives default included/excluded tags from presets
- Returns a complete SubState with all layout defaults
254-286: Review the filter combination logic.The
recomputeFiltersfunction applies OR logic within groups (built-in vs user) and AND logic across groups. This means:
- Within built-in filters: item matches if ANY filter passes
- Within user filters: item matches if ANY filter passes
- Across groups: item matches only if ALL groups pass
Verify this is the intended behavior. For excluded filters, the logic requires ALL filters in EACH group to pass (lines 282-283), which is correct for exclusion semantics.
672-696: LGTM!The
addTagFiltersimplementation correctly:
- Uses Sets to handle duplicates
- Removes tag from opposite filter list when adding
- Persists changes permanently
- Triggers filter recomputation
730-739: LGTM!Properly wires filter recomputation to:
STORY_INDEX_INVALIDATEDevent (when index changes)- Module initialization (on startup)
This ensures filters are always in sync with the current state.
code/core/src/manager/components/sidebar/TagsFilter.stories.tsx (2)
122-131: LGTM!Clean decorator pattern that wraps stories with the MockAPIWrapper, providing a consistent mock API environment for all TagsFilter stories.
181-184: Good documentation of testing limitation.The comment clearly explains why testing the reset-to-default behavior is limited - the global.TAGS_OPTIONS mocking doesn't propagate to the API's resetTagFilters function. This transparency helps future maintainers understand the test coverage gaps.
code/core/src/manager/components/sidebar/TagsFilterPanel.tsx (3)
69-70: Theequalhelper relies on arrays having no duplicates.The comment on lines 67-68 correctly documents this assumption. Since the tag filter arrays are managed by the layout module which uses Sets internally (see
addTagFiltersin layout.ts lines 675-690), this assumption should hold. However, consider adding a brief note about why this is safe.
129-147: LGTM!The
toggleFilterandsetAllFilterscallbacks correctly delegate to the API methods, maintaining a clean separation between UI logic and state management.
149-155: LGTM!
isDefaultSelectionandhasDefaultSelectionare correctly derived from API defaults, enabling proper Reset button state management.
|
@Sidnioulz can you resolve the merge conflicts? 🙏 |
Sidnioulz
force-pushed
the
sidnioulz/issue-32986
branch
from
c64b660 to
45d3eec
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@code/core/src/manager-api/modules/layout.ts`:
- Around line 260-268: The code uses the loose check "tag in BUILT_IN_FILTERS"
(seen in computeFilterFunctions and the other occurrence) which will match
prototype keys; change these to a safe own-property check such as
Object.prototype.hasOwnProperty.call(BUILT_IN_FILTERS, tag) so only actual
built-in keys are treated as built-ins; update both occurrences (the block in
computeFilterFunctions and the similar check around USER_TAG_FILTER usage) to
use this hasOwnProperty call and leave the existing push logic unchanged.
In `@code/core/src/manager-api/store.ts`:
- Around line 126-159: createTestingStore currently disables writes via
upstream.allowPersistence=false but Store.getInitialState still merges from
local/session storage; update the contract so getInitialState honors the
upstream persistence flag: modify Store.getInitialState (or the Store
constructor initialization) to check upstream.allowPersistence (or
upstreamPersistence) and skip reading/merging from localStorage/sessionStorage
when that flag is falsy, or alternatively have createTestingStore pass an
explicit upstream property (e.g., allowPersistence: false or a no-op read
function) that signals Store to avoid persisting reads so the testing store
remains purely in-memory.
♻️ Duplicate comments (1)
code/core/src/manager/components/sidebar/TagsFilter.stories.tsx (1)
22-91: Thethis.stateassignment on line 90 is confusing.As noted in a previous review, assigning to
this.statein the constructor conflicts with React's internal state management conventions. While this works because you're usingsetStatein the store callback for re-renders, it's semantically confusing.
🧹 Nitpick comments (1)
code/core/src/manager/components/sidebar/TagsFilterPanel.tsx (1)
102-124: Consider deduplicating built-in filter icons.The built-in filter icons are defined twice: once within the
filtersByIdmemo (lines 107, 114, 121) and again inbuiltInFilterIcons(lines 157-164). Consider consolidating to reduce duplication.♻️ Suggested refactor
+const builtInFilterIcons = { + _docs: <DocumentIcon color={color.gold} />, + _play: <PlayHollowIcon color={color.seafoam} />, + _test: <BeakerIcon color={color.green} />, +}; + export const TagsFilterPanel = ({ api, indexJson }: TagsFilterPanelProps) => { // ... existing code ... const filtersById = useMemo<{ [id: string]: Filter }>(() => { // ... userFilters code ... const builtInFilters = { _docs: { id: '_docs', type: 'built-in', title: 'Documentation', - icon: <DocumentIcon color={color.gold} />, count: getBuiltInCount(api.getFilterFunction('_docs')), }, // ... similar for _play and _test }; return { ...userFilters, ...builtInFilters }; }, [api, indexJson.entries]); - const builtInFilterIcons = useMemo( - () => ({ - _docs: <DocumentIcon color={color.gold} />, - _play: <PlayHollowIcon color={color.seafoam} />, - _test: <BeakerIcon color={color.green} />, - }), - [] - );Then use the module-level
builtInFilterIconsconstant inrenderLink.Also applies to: 157-164
Sidnioulz
force-pushed
the
sidnioulz/issue-32986
branch
from
45d3eec to
8f67532
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@code/core/src/manager/components/sidebar/TagsFilterPanel.stories.tsx`:
- Around line 158-183: The stories DefaultSelection and DefaultSelectionModified
reference global.TAGS_OPTIONS but never import global; add an import for global
from '@storybook/global' at the top of the file so both beforeEach hooks can use
global.TAGS_OPTIONS safely (locate the file symbols DefaultSelection,
DefaultSelectionModified and the usage of global.TAGS_OPTIONS to add the
import).
In `@code/core/src/manager/components/sidebar/TagsFilterPanel.tsx`:
- Around line 142-146: setAllFilters currently passes all tag ids from
Object.keys(filtersById) to api.setAllTagFilters, which erroneously includes
hidden built‑in filters (those with count === 0); update setAllFilters so when
selected is true it builds the id list by iterating filtersById and excluding
filters where filter.builtIn (or builtin) is true AND filter.count === 0, then
pass that filtered id array to api.setAllTagFilters (leave the else branch as
[]). Reference: setAllFilters, filtersById, api.setAllTagFilters, and the filter
objects' count and built‑in flag.
♻️ Duplicate comments (3)
code/core/src/manager-api/store.ts (1)
134-158: Prevent testing stores from reading persisted state.
createTestingStoredisables writes, butStore.getInitialStatestill merges local/session storage, so stories/tests can pick up stale browser state. Consider honoringupstreamPersistenceon reads too (see Line 62).🛠️ Proposed fix
getInitialState(base: State) { + if (!this.upstreamPersistence) { + return { ...base }; + } return { ...base, ...get(store.local), ...get(store.session) }; }code/core/src/manager-api/modules/layout.ts (1)
264-268: Use own‑property check for built‑in tag lookup.Same concern as earlier review:
tag in BUILT_IN_FILTERSmatches prototype keys; switch tohasOwnProperty.Also applies to: 716-720
code/core/src/manager/components/sidebar/TagsFilter.stories.tsx (1)
88-91: Renamethis.stateto avoid confusion with React's managed state.The assignment
this.state = this.store.getState()on line 90 conflates React's internal state management with the testing store's state. While React class components do initializethis.statein the constructor, this value is never read in the component—setStateis only used to trigger re-renders.🔧 Suggested fix
this.api = api as API; - this.state = this.store.getState(); + // Initial store state is managed by the store itself; setState is used only to trigger re-renders }Alternatively, if you need a reference for debugging, use a non-conflicting property name like
this.storeState.
🧹 Nitpick comments (3)
code/core/src/manager-api/modules/layout.ts (1)
255-285: Precompute tag-filter groups once per recompute.
computeFilterFunctionsruns for every entry during filtering. You can compute included/excluded groups once insiderecomputeFiltersand reuse them to avoid repeated work on large indices.♻️ Proposed refactor
- fullAPI.experimental_setFilter?.(TAGS_FILTER, (item: API_PreparedIndexEntry) => { - const included = computeFilterFunctions(includedTagFilters); - const excluded = computeFilterFunctions(excludedTagFilters); + const included = computeFilterFunctions(includedTagFilters); + const excluded = computeFilterFunctions(excludedTagFilters); + + fullAPI.experimental_setFilter?.(TAGS_FILTER, (item: API_PreparedIndexEntry) => { return ( (!included.length || included.every((group) => group.some((filterFn) => filterFn(item, false)))) && (!excluded.length || excluded.every((group) => group.every((filterFn) => filterFn(item, true)))) ); });code/core/src/manager/components/sidebar/TagsFilter.stories.tsx (2)
186-198: Consider using a more specific button selector for robustness.Line 189 uses
canvas.findByRole('button', {}, { timeout: 3000 })which finds the first button. If the component structure changes to include multiple buttons in the initial state, this could break.♻️ Optional: Add accessible name or test ID for clarity
If the trigger button has accessible text, consider:
- const button = await canvas.findByRole('button', {}, { timeout: 3000 }); + const button = await canvas.findByRole('button', { name: /filter/i }, { timeout: 3000 });Alternatively, ensure the TagsFilter trigger button has a distinguishing accessible name or
data-testid.
275-281: Consider whetherEmptyProductionis still needed.The comment states "Production is equal to development now" and the story has identical args to
Empty. If this distinction is no longer relevant, consider removing the duplicate story to reduce maintenance burden. If kept intentionally for regression testing, the comment adequately explains the rationale.
code/core/src/manager/components/sidebar/TagsFilterPanel.stories.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@code/core/src/manager/components/sidebar/TagsFilter.stories.tsx`:
- Around line 37-52: The constructor sets this.mounted and creates this.store
but never initializes this.state, and componentDidMount flips this.mounted true
without any componentWillUnmount to reset it; initialize the component state in
the constructor (e.g., this.state = this.props.initialStoryState || {} /
appropriate State shape) so setState calls are valid, and add a
componentWillUnmount lifecycle that sets this.mounted = false (and if
createTestingStore returns an unsubscribe, call it there) to prevent setState
after unmount; update the methods referenced (constructor, componentDidMount,
componentWillUnmount, mounted, store, setState) accordingly.
🧹 Nitpick comments (3)
code/core/src/manager-api/modules/layout.ts (1)
255-287: Consider memoizing or hoistingcomputeFilterFunctionsresults outside the filter callback.The
computeFilterFunctionshelper is called twice on everyitempassed to the filter (lines 277-278). SinceincludedTagFiltersandexcludedTagFiltersdon't change during filtering, computing these arrays once outside the callback would avoid redundant work for large indices.♻️ Suggested refactor
const recomputeFilters = (fullAPI: Parameters<ModuleFn>[0]['fullAPI'], store: Store) => { const { layout: { includedTagFilters, excludedTagFilters }, } = store.getState(); const computeFilterFunctions = (set: Tag[]): FilterFunction[][] => { return Object.values( set.reduce( (acc, tag) => { if (Object.hasOwn(BUILT_IN_FILTERS, tag)) { acc['built-in'].push(BUILT_IN_FILTERS[tag as keyof typeof BUILT_IN_FILTERS]); } else { acc.user.push(USER_TAG_FILTER(tag)); } return acc; }, { 'built-in': [], user: [] } as { 'built-in': FilterFunction[]; user: FilterFunction[] } ) ).filter((group) => group.length > 0); }; + const included = computeFilterFunctions(includedTagFilters); + const excluded = computeFilterFunctions(excludedTagFilters); + fullAPI.experimental_setFilter?.(TAGS_FILTER, (item: API_PreparedIndexEntry) => { - const included = computeFilterFunctions(includedTagFilters); - const excluded = computeFilterFunctions(excludedTagFilters); return ( (!included.length || included.every((group) => group.some((filterFn) => filterFn(item, false)))) && (!excluded.length || excluded.every((group) => group.every((filterFn) => filterFn(item, true)))) ); }); };code/core/src/manager/components/sidebar/TagsFilter.stories.tsx (2)
146-146: Double type assertion suggests incomplete mock data.The
as unknown as DocsIndexEntrydouble cast indicates the mock object doesn't satisfy theDocsIndexEntryinterface. Consider adding the required properties to avoid the intermediateunknowncast, which bypasses type safety.
271-277: Consider removing redundant story or documenting the distinction.The comment states "Production is equal to development now", and
EmptyProductionappears identical toEmptyexcept it explicitly spreadsEmpty.args. If there's no meaningful difference, consider removing this story to reduce maintenance burden, or document why it's kept (e.g., for regression testing a historical distinction).
code/core/src/manager/components/sidebar/TagsFilter.stories.tsx
Outdated
Show resolved
Hide resolved
|
@ndelangen branch is updated and CI green, it should be good to review |
Sidnioulz
force-pushed
the
sidnioulz/issue-32986
branch
from
cae1b4e to
8b98617
Compare
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@code/core/src/manager/components/sidebar/TagsFilterPanel.stories.tsx`:
- Around line 158-183: The beforeEach hooks in the Story objects
DefaultSelection and DefaultSelectionModified reference global.TAGS_OPTIONS but
do not import global, causing runtime errors; fix by adding the appropriate
import (e.g. import global from 'global' or import { global } from 'global'
depending on your codebase) at the top of TagsFilterPanel.stories.tsx so global
is defined before using TAGS_OPTIONS, and keep the existing beforeEach logic
that saves and restores originalTagsOptions.
In `@code/core/src/manager/components/sidebar/TagsFilterPanel.tsx`:
- Around line 142-146: The select-all currently uses Object.keys(filtersById)
which includes hidden built-in filters (those with count === 0), so change
setAllFilters to build the selected ids from Object.values(filtersById) while
filtering out built-in filters with count === 0 (e.g., .filter(f => !(f.builtIn
&& f.count === 0))). Then pass the resulting ids to api.setAllTagFilters when
selected is true and keep [] when false; update the call in setAllFilters (the
function name) that currently calls api.setAllTagFilters(selected ?
Object.keys(filtersById) : [], []) to use the filtered id list instead.
8b98617 to
1815fe7
Compare
4 participants
Closes #32986
What I did
layoutmodulelayoutmodule for TagsFilter* storiesChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
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 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
Refactor
Tests
Chores