Manager: URL-based tag filter state + filter-aware initial story selection#34283
Merged
Conversation
|
View your CI Pipeline Execution ↗ for commit 17b4191
☁️ Nx Cloud last updated this comment at |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds URL-backed tag-filter persistence and query-parameter-aware navigation: new tag parse/serialize utilities, URL persistence handler registration, nullable/deletable query-param semantics, initial tag filters derived from the URL, and navigation updated to preserve and validate against URL tag filters. Changes
Sequence DiagramsequenceDiagram
participant Browser as Browser Location
participant Stories as Stories Module
participant URL as URL Module
participant Store as Store/Persistence
participant Navigation as Navigator
Browser->>Stories: App load (with ?tags=...)
Stories->>URL: queryFromLocation(location)
URL-->>Stories: parsed query (tags param)
Stories->>Stories: parseTagsParam(tags)
Stories->>Store: init state with parsed tag filters
Note over Stories,Store: user updates tag filters
Stories->>Store: setState(patch, persistence: 'url')
Store->>Store: invoke registered 'url' handler with delta + serialize
Store->>URL: handler.applyQueryParams(serialized)
URL->>Navigation: navigate(updated URL with tags)
Navigation-->>Browser: URL updated
Note over Stories: user selects a story
Stories->>Stories: navigateWithQueryParams(path)
Stories->>URL: merge current customQueryParams into path
URL->>Navigation: navigate(path + preserved params)
Navigation-->>Browser: URL includes story route + tags
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.
Actionable comments posted: 4
🤖 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 1179-1183: The URL-parsed tag filters (computed via
queryFromLocation and parseTagsParam into initialIncluded/initialExcluded when
hasTagsParam is true) are being overridden by legacy persisted keys merged by
Store.getInitialState (store.local / store.session) so the active filters stay
out-of-sync; modify startup behavior so when hasTagsParam is true you ignore or
scrub legacy persisted includedTagFilters/excludedTagFilters instead of letting
Store.getInitialState merge them: either (A) update Store.getInitialState to
skip merging those two keys from store.local/session when a tags param is
present, or (B) add a one-time migration in the stories module startup to
delete/clear store.local.storeKeys for includedTagFilters/excludedTagFilters
before state merge, or (C) ensure the module’s initial state explicitly sets
includedTagFilters/excludedTagFilters to parsedTags.included/parsedTags.excluded
after Store.getInitialState runs when hasTagsParam is true so URL-derived values
win.
- Around line 967-975: The fallback redirect uses viewMode blindly which can
misroute a docs entry; change the fallback to derive the route from
filteredIndex[firstFiltered]. Retrieve const entry =
filteredIndex[firstFiltered]; determine routeMode = entry && entry.type ===
'docs' ? 'docs' : 'story' (or only allow 'story' if you prefer restricting),
then call navigateWithQueryParams(`/${routeMode}/${firstFiltered}`) instead of
using viewMode; ensure you guard for entry existence before navigating.
In `@code/core/src/manager-api/modules/tags.ts`:
- Around line 39-42: The serialization currently returns undefined when both
included and excluded are empty which collapses an explicit “no tags” selection
into an absent tags param; change serializeTagsParam (in
code/core/src/manager-api/modules/tags.ts) to return an explicit empty string
('') when included.length===0 && excluded.length===0 so the URL-writing path can
preserve the empty value instead of dropping the param, and ensure this aligns
with parseTagsParam so parseTagsParam('') continues to mean “no filters” while
undefined remains “no tags param / use presets” (verify any callers in
stories.ts that write the tags param do not treat '' as missing).
In `@code/core/src/manager-api/tests/url.test.js`:
- Around line 126-143: The test seeds store.state with only customQueryParams
causing applyQueryParams (api.applyQueryParams) to call store.getState() and
access location.path and location.hash on an undefined location; fix by
initializing the store with the full module state shape (including location: {
path, search, hash }) before calling initURL/api.applyQueryParams so getState()
returns a location object—update the test's store or initial state passed to
initURL to include location.path and location.hash alongside customQueryParams.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d4be47d-677e-4e55-b652-7d025048af77
📒 Files selected for processing (7)
code/core/src/manager-api/modules/stories.tscode/core/src/manager-api/modules/tags.tscode/core/src/manager-api/modules/url.tscode/core/src/manager-api/store.tscode/core/src/manager-api/tests/tags.test.jscode/core/src/manager-api/tests/url.test.jscode/core/src/router/utils.ts
14 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/core/src/manager-api/store.ts (1)
142-150: Consider logging when URL persistence handler is missing.If the
'url'handler isn't registered (e.g., due to initialization order issues), the persistence operation silently no-ops. This could make debugging difficult. A warning log would surface misconfiguration early.💡 Optional: Add a warning when handler is missing
if (persistence === 'url') { const handler = this.persistenceHandlers.get('url'); if (handler) { await handler(delta, (options as Options | undefined)?.serialize); + } else if (process.env.NODE_ENV !== 'production') { + console.warn('URL persistence handler not registered'); } } else {Note: Per coding guidelines, consider using
@storybook/internal/client-loggerinstead ofconsole.warnif readily available in this module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager-api/store.ts` around lines 142 - 150, When persistence === 'url' and this.persistenceHandlers.get('url') returns undefined, add a warning log so the missing handler doesn't silently no-op; update the block around persistenceHandlers/get('url') (the handler variable and its conditional) to log a warning that the 'url' persistence handler is not registered and include context (e.g., persistence value and any relevant options). Use `@storybook/internal/client-logger.warn` if that module is available in this file, otherwise fall back to console.warn; do not change the existing behavior of calling handler(delta, serialize) when present.
🤖 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/store.ts`:
- Around line 81-94: The migration removes tag-filter keys after taking
snapshots (local and session), so the returned initial state still includes the
old includedTagFilters/excludedTagFilters; update the logic in the store
initialization to either (A) re-read storage after the cleanup loop (call
get(store.local) and get(store.session) again and use those for the returned
merge) or (B) explicitly strip includedTagFilters/excludedTagFilters from the
previously-captured local and session objects before returning; reference the
existing symbols local, session, store.local, store.session, get, set, and the
final return that merges base, local, session to locate where to apply the fix.
---
Nitpick comments:
In `@code/core/src/manager-api/store.ts`:
- Around line 142-150: When persistence === 'url' and
this.persistenceHandlers.get('url') returns undefined, add a warning log so the
missing handler doesn't silently no-op; update the block around
persistenceHandlers/get('url') (the handler variable and its conditional) to log
a warning that the 'url' persistence handler is not registered and include
context (e.g., persistence value and any relevant options). Use
`@storybook/internal/client-logger.warn` if that module is available in this file,
otherwise fall back to console.warn; do not change the existing behavior of
calling handler(delta, serialize) when present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5dd03e6e-802b-48c6-8699-7f72beecafcf
📒 Files selected for processing (5)
code/core/src/manager-api/modules/stories.tscode/core/src/manager-api/modules/tags.tscode/core/src/manager-api/modules/url.tscode/core/src/manager-api/store.tscode/core/src/manager-api/tests/tags.test.js
🚧 Files skipped from review as they are similar to previous changes (4)
- code/core/src/manager-api/tests/tags.test.js
- code/core/src/manager-api/modules/tags.ts
- code/core/src/manager-api/modules/url.ts
- code/core/src/manager-api/modules/stories.ts
valentinpalkovic
force-pushed
the
valentin/url-based-tag-state
branch
from
0d52e76 to
2f80eb0
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
code/core/src/manager-api/modules/stories.ts (1)
967-980:⚠️ Potential issue | 🟠 MajorUse the matched entry type for the filtered fallback.
This block finds
firstFilteredby ID but then navigates using the preview-emittedviewMode. IffirstFilteredis a docs entry butviewModeis'story', this produces an incorrect route like/story/<docsId>instead of/docs/<docsId>.Derive the route from the matched entry's type instead of using
viewMode.Proposed fix
- const firstFiltered = filteredIndex - ? Object.keys(filteredIndex).find((id) => { - const entry = filteredIndex[id]; - return entry.type === 'story' || entry.type === 'docs'; - }) + const firstFilteredEntry = filteredIndex + ? Object.values(filteredIndex).find( + (entry) => entry.type === 'story' || entry.type === 'docs' + ) : undefined; - if (firstFiltered) { - navigateWithQueryParams(`/${viewMode}/${firstFiltered}`); + if (firstFilteredEntry) { + navigateWithQueryParams(`/${firstFilteredEntry.type}/${firstFilteredEntry.id}`); }🤖 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 967 - 980, The fallback navigation uses viewMode instead of the matched entry's type, causing routes like `/story/<docsId>`; change the logic that computes firstFiltered to also capture the matched entry (e.g., find both id and entry via filteredIndex) and derive the routeType from entry.type (map entry.type 'docs' => 'docs', otherwise 'story'), then call navigateWithQueryParams(`/${routeType}/${firstFiltered}`) instead of using viewMode; keep the existing early return and preserve filteredIndex checks around this change.
🧹 Nitpick comments (4)
code/core/src/manager-api/modules/stories.ts (2)
447-455: Clarify theserializecallback parameter type.The
serializefunction parametersuses a type assertion viaReturnType<typeof store.getState>. Consider adding explicit typing to thepersistFiltersfunction signature for better readability and maintainability.🤖 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 447 - 455, The serialize callback currently types its parameter `s` via ReturnType<typeof store.getState>, which is unclear; update the persistFilters signature to accept an explicit state type (e.g., define or import the store state interface and annotate persistFilters or the serialize param) so that the `serialize` function parameter is strongly typed instead of using ReturnType<typeof store.getState>; locate the persistFilters function, the store.setState call, and the serialize callback (and types referenced: store.getState, serializeTagsParam, includedTagFilters, excludedTagFilters) and replace the assertion with the explicit state type in the function signature or serialize param.
437-445: Consider extracting URL building logic to reduce duplication.
navigateWithQueryParamsduplicates the query string building logic fromnavigateToinurl.ts(lines 227-231). Consider refactoring to share this logic, perhaps by exposing a utility function or having the stories module call into the URL module's API.Alternative approach
If the URL module's
api.applyQueryParamsis available, you could potentially use it instead. However, I understand this may be a deliberate design choice to avoid circular dependencies or timing issues during module initialization.🤖 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 437 - 445, navigateWithQueryParams duplicates query-string construction already implemented in navigateTo (url.ts) — refactor to reuse that logic: extract the query-building logic into a shared utility (e.g., buildQueryString or reuse api.applyQueryParams) and have navigateWithQueryParams call that helper instead of rebuilding params; update navigateWithQueryParams to accept the path and options, call the shared buildQueryString([...]) or api.applyQueryParams(path, customQueryParams) to produce the final URL, then call navigate(to, options). Ensure the helper exports are used to avoid circular imports (or move the helper to a neutral util module) and keep the existing function signature of navigateWithQueryParams and the behavior filtering null/undefined and sorting keys.code/core/src/manager-api/modules/url.ts (1)
321-326: Consider the order of operations inapplyQueryParams.The function navigates first (line 324) then updates query params state (line 325). If
setQueryParamstriggers any side effects, they would happen after navigation. This seems intentional, but verify that this ordering is correct for the URL persistence use case where the persistence handler callsapplyQueryParams.Also, line 324 casts
{ ...queryParams, ...input }toany, which loses type safety. Consider using a more precise type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager-api/modules/url.ts` around lines 321 - 326, applyQueryParams currently calls navigateTo with merged params then calls api.setQueryParams, which can invert expected side-effect ordering for URL persistence handlers; decide and enforce the correct order: if persistence/handlers must run before navigation, call api.setQueryParams(input) first, otherwise leave order but document intent. Also remove the unsafe cast to any on `{ ...queryParams, ...input }` and replace it with a precise type (e.g., the same type as api.getUrlState().queryParams or a defined QueryParams/Record<string, string|undefined> type) so navigateTo receives a correctly typed object; update references in applyQueryParams, navigateTo call site, and types returned by api.getUrlState() accordingly.code/core/src/manager-api/tests/stories.test.ts (1)
959-992: Add test coverage for filter-aware STORY_SPECIFIED behavior.The
STORY_SPECIFIEDhandler now includes logic for filter-aware navigation (lines 960-972 in stories.ts):
- Navigates to the first filtered story/docs when the specified story doesn't pass active tag filters
- Suppresses navigation when no entries pass the current filters
- Selects docs entries as fallback when stories are unavailable
The existing tests in this block (lines 709-746) cover basic navigation only and don't test these filter-aware scenarios. Adding tests for these cases would improve confidence in the tag filter functionality.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager-api/tests/stories.test.ts` around lines 959 - 992, Add unit tests around the STORY_SPECIFIED handler in stories.test.ts (the tests near the existing navigation block that use api.setIndex and api.jumpToStory) to cover filter-aware behavior: (1) when the specified storyId is filtered out by active tag filters, assert that navigate/jumpToStory goes to the first filtered story or docs entry (depending on available entries); (2) when no entries pass the current filters, assert that navigation is suppressed (navigate not called); and (3) when stories are filtered out but docs entries exist, assert the docs entry is selected as a fallback. Use the same helpers (createMockModuleArgs, initStories, navigationEntries, api.setIndex, api.jumpToStory and the mocked navigate) and the STORY_SPECIFIED handler in stories.ts to locate where to add these cases.
🤖 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-api/modules/stories.ts`:
- Around line 967-980: The fallback navigation uses viewMode instead of the
matched entry's type, causing routes like `/story/<docsId>`; change the logic
that computes firstFiltered to also capture the matched entry (e.g., find both
id and entry via filteredIndex) and derive the routeType from entry.type (map
entry.type 'docs' => 'docs', otherwise 'story'), then call
navigateWithQueryParams(`/${routeType}/${firstFiltered}`) instead of using
viewMode; keep the existing early return and preserve filteredIndex checks
around this change.
---
Nitpick comments:
In `@code/core/src/manager-api/modules/stories.ts`:
- Around line 447-455: The serialize callback currently types its parameter `s`
via ReturnType<typeof store.getState>, which is unclear; update the
persistFilters signature to accept an explicit state type (e.g., define or
import the store state interface and annotate persistFilters or the serialize
param) so that the `serialize` function parameter is strongly typed instead of
using ReturnType<typeof store.getState>; locate the persistFilters function, the
store.setState call, and the serialize callback (and types referenced:
store.getState, serializeTagsParam, includedTagFilters, excludedTagFilters) and
replace the assertion with the explicit state type in the function signature or
serialize param.
- Around line 437-445: navigateWithQueryParams duplicates query-string
construction already implemented in navigateTo (url.ts) — refactor to reuse that
logic: extract the query-building logic into a shared utility (e.g.,
buildQueryString or reuse api.applyQueryParams) and have navigateWithQueryParams
call that helper instead of rebuilding params; update navigateWithQueryParams to
accept the path and options, call the shared buildQueryString([...]) or
api.applyQueryParams(path, customQueryParams) to produce the final URL, then
call navigate(to, options). Ensure the helper exports are used to avoid circular
imports (or move the helper to a neutral util module) and keep the existing
function signature of navigateWithQueryParams and the behavior filtering
null/undefined and sorting keys.
In `@code/core/src/manager-api/modules/url.ts`:
- Around line 321-326: applyQueryParams currently calls navigateTo with merged
params then calls api.setQueryParams, which can invert expected side-effect
ordering for URL persistence handlers; decide and enforce the correct order: if
persistence/handlers must run before navigation, call api.setQueryParams(input)
first, otherwise leave order but document intent. Also remove the unsafe cast to
any on `{ ...queryParams, ...input }` and replace it with a precise type (e.g.,
the same type as api.getUrlState().queryParams or a defined
QueryParams/Record<string, string|undefined> type) so navigateTo receives a
correctly typed object; update references in applyQueryParams, navigateTo call
site, and types returned by api.getUrlState() accordingly.
In `@code/core/src/manager-api/tests/stories.test.ts`:
- Around line 959-992: Add unit tests around the STORY_SPECIFIED handler in
stories.test.ts (the tests near the existing navigation block that use
api.setIndex and api.jumpToStory) to cover filter-aware behavior: (1) when the
specified storyId is filtered out by active tag filters, assert that
navigate/jumpToStory goes to the first filtered story or docs entry (depending
on available entries); (2) when no entries pass the current filters, assert that
navigation is suppressed (navigate not called); and (3) when stories are
filtered out but docs entries exist, assert the docs entry is selected as a
fallback. Use the same helpers (createMockModuleArgs, initStories,
navigationEntries, api.setIndex, api.jumpToStory and the mocked navigate) and
the STORY_SPECIFIED handler in stories.ts to locate where to add these cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd63282c-9d33-4fe6-b65e-1276259c0679
📒 Files selected for processing (3)
code/core/src/manager-api/modules/stories.tscode/core/src/manager-api/modules/url.tscode/core/src/manager-api/tests/stories.test.ts
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/tests/store.test.js`:
- Around line 24-30: Update the test to use vi.mock(..., { spy: true }) for the
store2 mock, and move the inline mockReturnValueOnce setup into a beforeEach
where you reset mocks (e.g., vi.resetAllMocks or vi.clearAllMocks) and then
configure behaviors via vi.mocked(...) wrappers; specifically replace direct
calls to store2.session.get.mockReturnValueOnce and
store2.local.get.mockReturnValueOnce with
vi.mocked(store2.session.get).mockReturnValueOnce(...) and
vi.mocked(store2.local.get).mockReturnValueOnce(...), doing both the
migration-check and merge returns inside beforeEach so tests use the typed
vi.mocked accessors and mocks are reset between tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c70e553f-563b-41e5-941f-e7e95d794d91
📒 Files selected for processing (1)
code/core/src/manager-api/tests/store.test.js
ghengeveld
approved these changes
Mar 25, 2026
valentinpalkovic
requested review from
jonniebigodes and
kylegach
as code owners
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
10 tasks
8 tasks
12 tasks
Closed
8 tasks
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #34252
What I did
Replaces localStorage persistence for tag filters with URL-based state. All filter state is now encoded in a single
tagsURL parameter (e.g.?tags=a11y;$changed;!experimental). Also fixes the bug where initial story selection ignored active tag filters when navigating to Storybook without a story path.Bildschirmaufnahme.2026-03-23.um.16.59.50.mov
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!
yarn task --task sandbox --start-from auto --template react-vite/default-tsplay)?tags=$play(or the appropriate tag encoding)http://localhost:6006/?tags=playdirectly — verify only stories taggedplayare shown and the first story/docs is auto-selectedtagsparam is removed from the URLtagsparam is preserved in the URL throughoutDocumentation
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
Tests