UI: Allow hiding tags from sidebar filters#35397
Conversation
mrginglymus
force-pushed
the
bill/hidden-tags
branch
from
c4eba7b to
8820409
Compare
📝 WalkthroughWalkthroughThis PR adds an optional ChangesTag filtering options update
Estimated code review effort: 2 (Simple) | ~12 minutes ✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
code/addons/docs/src/preview.ts (1)
3-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
excludeTagsconstruction across files.The same
Object.fromEntries(Object.entries<Partial<TagOptions>>(...).filter(...).map(...))pattern appears here and incode/core/src/manager-api/modules/tags.ts(Lines 60-64). Extracting a small shared helper (e.g.buildExcludeTagsMap(tagsOptions, flagKey)) would remove duplication and reduce the risk of the two copies drifting apart on which flag they key off — exactly the kind of divergence flagged above.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/addons/docs/src/preview.ts` around lines 3 - 7, The excludeTags निर्माण logic is duplicated here and in the tags manager, so extract the shared Object.fromEntries/Object.entries/filter/map pattern into a reusable helper such as buildExcludeTagsMap that accepts the tag options source and flag key. Update preview.ts to call that helper instead of rebuilding the map inline, and reuse the same helper from tags.ts so both paths stay consistent on which property they key off.code/core/src/manager/components/sidebar/useFilterData.tsx (1)
51-59: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
tagOptionsread from a mutable global but omitted fromuseMemodeps.
global.TAGS_OPTIONSis read inside theuseMemobody but the dependency array (Line 86) only includesindexJson.entries. IfTAGS_OPTIONSchanges at runtime (e.g. tag config hot-reloaded) withoutindexJson.entriesalso changing,excludeFromFilterPanelexclusions and tag counts would silently use stale config until some other prop change forces recomputation.♻️ Proposed fix
- const tagOptions = global.TAGS_OPTIONS ?? {}; + const tagOptions = global.TAGS_OPTIONS ?? {}; ... - }, [indexJson.entries]); + }, [indexJson.entries, global.TAGS_OPTIONS]);Please confirm whether
global.TAGS_OPTIONScan change without a full manager reload during a live session (e.g. via HMR ofmain.tstag config); if not, this is low priority.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/manager/components/sidebar/useFilterData.tsx` around lines 51 - 59, The useMemo in useTagFilterEntries depends on mutable tag configuration from global.TAGS_OPTIONS but only tracks indexJson.entries, so stale tag filtering can occur when the global changes at runtime. Update the memoization in useTagFilterEntries to include the tag config source in its dependencies, or otherwise derive a stable dependency from the tag options used for excludeFromFilterPanel and userTagsCounts so recomputation happens when TAGS_OPTIONS changes.code/core/src/manager/components/sidebar/Filter.stories.tsx (1)
253-273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
userEventinstead of native.click()in theplayfunction.
button.click()(Line 256) andclearButton.click()(Line 265) call the native DOM API directly rather than simulating a real user interaction. As per path instructions for**/*.stories.tsx, play functions should cover behaviors usingexpect,userEvent, andwithinfromstorybook/test. Native.click()skips the pointer/focus event sequenceuserEvent.click()fires, which can mask real interaction issues (e.g. handlers bound toonPointerDown/onMouseDown).✅ Proposed fix
const button = await canvas.findByRole('button', { name: /1 active tag filter/i }); - button.click(); + await userEvent.click(button); const visibleTag = await screen.findByRole('checkbox', { name: /visible/i }); await expect(visibleTag).toBeInTheDocument(); await expect(screen.queryByRole('checkbox', { name: /hidden/i })).not.toBeInTheDocument(); const clearButton = await screen.findByRole('button', { name: 'Clear filters', }); - clearButton.click(); + await userEvent.click(clearButton);As per coding guidelines, "When writing tests for components, add or update
<Component>.stories.tsxand cover each behavior withplayfunctions usingexpect,userEvent, andwithinfromstorybook/test." Also, as per coding guidelines, please verify the assertions withvitest --config code/vitest.config.storybook.ts <story-file>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/manager/components/sidebar/Filter.stories.tsx` around lines 253 - 273, The Filter.stories.tsx play function is using native DOM clicks instead of user-level interactions. Update the play handler in the Filter story to use userEvent for both the tag filter button and the Clear filters button, and keep the assertions with expect/queries consistent with Storybook test patterns. Make the change in the play function alongside the existing canvas and screen queries, and verify the story behavior with the Storybook vitest config for this story file.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@code/addons/docs/src/preview.ts`:
- Around line 3-7: The excludeTags निर्माण logic is duplicated here and in the
tags manager, so extract the shared Object.fromEntries/Object.entries/filter/map
pattern into a reusable helper such as buildExcludeTagsMap that accepts the tag
options source and flag key. Update preview.ts to call that helper instead of
rebuilding the map inline, and reuse the same helper from tags.ts so both paths
stay consistent on which property they key off.
In `@code/core/src/manager/components/sidebar/Filter.stories.tsx`:
- Around line 253-273: The Filter.stories.tsx play function is using native DOM
clicks instead of user-level interactions. Update the play handler in the Filter
story to use userEvent for both the tag filter button and the Clear filters
button, and keep the assertions with expect/queries consistent with Storybook
test patterns. Make the change in the play function alongside the existing
canvas and screen queries, and verify the story behavior with the Storybook
vitest config for this story file.
In `@code/core/src/manager/components/sidebar/useFilterData.tsx`:
- Around line 51-59: The useMemo in useTagFilterEntries depends on mutable tag
configuration from global.TAGS_OPTIONS but only tracks indexJson.entries, so
stale tag filtering can occur when the global changes at runtime. Update the
memoization in useTagFilterEntries to include the tag config source in its
dependencies, or otherwise derive a stable dependency from the tag options used
for excludeFromFilterPanel and userTagsCounts so recomputation happens when
TAGS_OPTIONS changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12c54c52-bd6f-4ccb-bc8f-816a20273171
📒 Files selected for processing (6)
code/addons/docs/src/preview.tscode/core/src/manager-api/modules/tags.tscode/core/src/manager/components/sidebar/Filter.stories.tsxcode/core/src/manager/components/sidebar/useFilterData.tsxcode/core/src/types/modules/core-common.tsdocs/api/main-config/main-config-tags.mdx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/addons/docs/src/preview.ts (1)
3-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLGTM, minor style nit optional.
Logic is correct and matches the prior
true-flagging behavior viaTagOptions.excludeFromDocsStories. Thefilterthenmapre-extracts the same destructured field, which is slightly redundant.♻️ Optional simplification
const excludeTags = Object.fromEntries( Object.entries<Partial<TagOptions>>(globalThis.TAGS_OPTIONS ?? {}) - .filter(([, { excludeFromDocsStories }]) => excludeFromDocsStories) - .map(([tag, { excludeFromDocsStories }]) => [tag, excludeFromDocsStories]) + .map(([tag, { excludeFromDocsStories }]) => [tag, excludeFromDocsStories] as const) + .filter(([, excludeFromDocsStories]) => excludeFromDocsStories) );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/addons/docs/src/preview.ts` around lines 3 - 7, The logic in the TAGS_OPTIONS transformation is correct, but the `filter` followed by `map` in `excludeTags` redundantly re-destructures `excludeFromDocsStories`. Simplify the `Object.entries(...).filter(...).map(...)` chain in `preview.ts` by keeping the same behavior while avoiding the repeated extraction of the same field, so the `excludeTags` construction is cleaner and easier to read.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@code/addons/docs/src/preview.ts`:
- Around line 3-7: The logic in the TAGS_OPTIONS transformation is correct, but
the `filter` followed by `map` in `excludeTags` redundantly re-destructures
`excludeFromDocsStories`. Simplify the
`Object.entries(...).filter(...).map(...)` chain in `preview.ts` by keeping the
same behavior while avoiding the repeated extraction of the same field, so the
`excludeTags` construction is cleaner and easier to read.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7fcf1435-51ef-4040-b55f-33d8dd570749
📒 Files selected for processing (6)
code/addons/docs/src/preview.tscode/core/src/manager-api/modules/tags.tscode/core/src/manager/components/sidebar/Filter.stories.tsxcode/core/src/manager/components/sidebar/useFilterData.tsxcode/core/src/types/modules/core-common.tsdocs/api/main-config/main-config-tags.mdx
✅ Files skipped from review due to trivial changes (1)
- docs/api/main-config/main-config-tags.mdx
🚧 Files skipped from review as they are similar to previous changes (4)
- code/core/src/manager/components/sidebar/useFilterData.tsx
- code/core/src/manager-api/modules/tags.ts
- code/core/src/manager/components/sidebar/Filter.stories.tsx
- code/core/src/types/modules/core-common.ts
Sidnioulz
added
feature request
tags
qa:needed
2 participants
Closes #34186
What I did
Follow-up from #35113 with some minor modifications - only user tags are included (rather than system tags), and a note is added to the docs to discourage using hidden tags within the built-in tag filter.
This is because this will present an inconsistent and confusing UI to the user:
Here we see a notification that there is an active filter, but no evidence of it (as the tag in question is hidden).
Whilst it may be possible to dig through the various code paths and add further exclusions (including url persistence) it is hard to see a use case for hiding a tag and using it in the built-in tag filters.
It is more likely that the hidden tag will be used for, for example, static filters like
excludeFromSidebar,excludeFromDocsStoriesor a user-provided custom filter function.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
The new story added describes an undesirable use case of a hidden tag being used as a filter, but exercises the UI logic.
To give it a more thorough spin:
excludeFromFilterPanelsetexcludeFromSidebarandexcludeFromDocsStoriesDocumentation
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.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake 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
excludeFromFilterPanelto tag options to hide specific tags from the sidebar tag filter UI while keeping them usable for internal tag-based filtering.excludeFromFilterPanelin the main configuration reference, including guidance on recommended usage.