Sidebar: Add status-based filtering with refactored status architecture#34339
Conversation
…to valentin/status-based-filtering
…omponents - Added Filter component to manage tag and status filters in the sidebar. - Created FilterPanel component to display and manage filter options. - Integrated filter functionality into the Sidebar component, replacing the previous TagsFilter. - Enhanced status handling in various components, ensuring consistent status icon and color usage. - Updated SearchResults and Tree components to utilize new status handling logic. - Added stories for FilterPanel to demonstrate various filter states and interactions.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
View your CI Pipeline Execution ↗ for commit 793070b
☁️ Nx Cloud last updated this comment at |
|
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 end-to-end status-based filtering: URL parse/serialize, status filter predicate and manager-api wiring, status-aware sidebar UI (Filter/FilterPanel, hooks, links), status mapping refactor to named fields, tests, and small UI selector and rendering tweaks. Changes
Sequence DiagramsequenceDiagram
participant User
participant FilterPanel as Filter Panel UI
participant API as Manager API
participant Store as Status Store
participant Transform as StoryIndex Transformer
participant Navigation as Navigation/Selector
User->>FilterPanel: toggle status filter (include/exclude)
FilterPanel->>API: addStatusFilters / removeStatusFilters
API->>Store: update included/excluded status lists
API->>API: computeStatusFilterFn(included, excluded)
API->>Transform: transformStoryIndexToStoriesHash(..., statusFilterKey)
Transform->>Transform: apply statusFilterFn to entries (check descendants if rejected)
Transform-->>API: return filteredIndex
API->>Navigation: update selection using filteredIndex
Navigation->>User: show filtered story list / navigate
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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: 2
🧹 Nitpick comments (8)
code/core/src/manager-api/lib/stories.ts (1)
200-208: Consider documenting the direct-children-only behavior for hierarchy filtering.The child check at line 201 only examines direct children (
item.parent === entry.id). For deeply nested structures, an entry will be hidden if it fails the status filter even when a grandchild passes. This appears intentional but differs from the bottom-up propagation typical in tree filtering. If grandchild-based visibility is expected, recursive descendant checking would be needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager-api/lib/stories.ts` around lines 200 - 208, The status-filter logic only checks direct children (uses indexEntries.filter(item => 'parent' in item && item.parent === entry.id)) so entries with passing grandchildren will still be hidden; add a clear comment above this block in stories.ts explaining the intentional "direct-children-only" behavior (mention statusFilterFn, indexEntries, children, allStatuses, and entry.id) and, if you want grandchild visibility instead, replace the direct-children check with a small recursive helper (e.g., hasPassingDescendant(entryId)) that traverses indexEntries by parent links and calls statusFilterFn({ ...descendant, statuses: allStatuses[descendant.id] ?? {} }) until a match is found.code/core/src/manager/components/sidebar/Filter.stories.tsx (1)
60-113: Add mocks for status filter APIs to enable complete test coverage.The decorator mocks tag filter APIs (
addTagFilters,removeTagFilters, etc.), but the PR introduces status filter APIs (addStatusFilters,removeStatusFilters,resetStatusFilters). SinceFilterPanelexercises these APIs, add corresponding mocks to the story to enable story-based testing of status filtering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager/components/sidebar/Filter.stories.tsx` around lines 60 - 113, The story's mocked API inside the useMemo (the api object) only implements tag filter methods; add corresponding status filter mocks—implement addStatusFilters(excluded flag), removeStatusFilters(tags), resetStatusFilters(), and setAllStatusFilters(included, excluded) alongside the existing tag methods—by updating the same setState logic but operating on includedStatusFilters, excludedStatusFilters and their default counterparts (defaultIncludedStatusFilters, defaultExcludedStatusFilters) so FilterPanel’s status-filter interactions are exercised in the story.code/core/src/manager/components/sidebar/Filter.tsx (2)
94-94: Update aria label to reflect broader filtering scope.The
ariaLabelstill says "Tag filters" but the panel now includes status-based filtering as well. Consider updating to a more inclusive label like "Filters" or "Tag and status filters".<PopoverProvider - ariaLabel="Tag filters" + ariaLabel="Filters" placement="bottom"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager/components/sidebar/Filter.tsx` at line 94, The ariaLabel on the filter panel (in Filter.tsx) is currently "Tag filters" but the panel includes status-based filters too; update the ariaLabel prop value on the component (the element/property that currently reads ariaLabel="Tag filters") to a more inclusive label such as "Filters" or "Tag and status filters" so screen readers accurately reflect the panel's scope.
115-118: Accessibility label also mentions only "tag" filters.The
ariaLabelon the button references "tag filter(s)" but now includes status filters in the count. Consider updating the label to be more accurate.ariaLabel={ activeFilterCount - ? `${activeFilterCount} active tag ${activeFilterCount !== 1 ? 'filters' : 'filter'}` - : 'Tag filters' + ? `${activeFilterCount} active ${activeFilterCount !== 1 ? 'filters' : 'filter'}` + : 'Filters' }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager/components/sidebar/Filter.tsx` around lines 115 - 118, The ariaLabel currently built in Filter.tsx for the button uses "tag filter(s)" but activeFilterCount includes status filters too; update the ariaLabel construction in the component (where ariaLabel and activeFilterCount are used) to use a generic, accurate phrase like "filters" (or differentiate counts if you have separate tag/status counts) so the label no longer misleadingly references only "tag" filters while representing all active filters.code/core/src/manager/components/sidebar/FilterPanel.tsx (1)
149-160: Reset button only resets tag filters, not status filters.The "Reset to default selection" button calls
api.resetTagFilters()but notapi.resetStatusFilters(). If status filters should also be reset to their defaults when the user clicks reset, this should be updated.If this is intentional because there are no default status filters (status filters always default to empty), consider documenting this behavior or evaluating if resetting all filters makes more sense for UX consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager/components/sidebar/FilterPanel.tsx` around lines 149 - 160, The Reset button in FilterPanel currently only calls api.resetTagFilters() (see ActionList.Button with id "reset-filters") so status filters remain unchanged; update the onClick handler to also call api.resetStatusFilters() (or call a combined reset method if you prefer) so both tag and status filters return to defaults when pressed, and ensure the handler uses the FilterPanel's api reference consistently and the button's disabled state (isDefaultSelection) still reflects the combined-default condition.code/core/src/manager-api/tests/stories.test.ts (1)
1552-1554: Movevi.mockcalls to top level of test file.Per coding guidelines,
vi.mock()should be placed at the top of the test file before any test cases, not within individual test bodies. The mocks at lines 1553, 1896, 1909, 1944, and 1979 should be consolidated at the top level.+vi.mock('../stores/status'); + describe('stories API', () => {Then remove the inline
vi.mock('../stores/status');calls from within the test cases.As per coding guidelines: "Place all mocks at the top of the test file before any test cases"
🤖 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 1552 - 1554, Move all vi.mock('../stores/status') calls out of individual test bodies (e.g., the 'can filter on status' it block) and place a single vi.mock('../stores/status') at the top-level of the test file before any describe/it declarations; remove the inline vi.mock calls from the tests (including the ones referenced around the file) so tests like the one using createMockModuleArgs({}) no longer call vi.mock inside the it blocks.code/core/src/manager-api/modules/stories.ts (1)
141-164: Status filter function callsfullStatusStore.getAll()on every evaluation.The
computeStatusFilterFnclosure capturesincludedStatusFiltersandexcludedStatusFiltersbut callsfullStatusStore.getAll()inside the returned filter function. This meansgetAll()is called for each story entry during filtering.If
getAll()is expensive, consider:
- Capturing the status data when the filter function is created (but then it won't reflect changes)
- Memoizing at a higher level
However, since
onAllStatusChangetriggersrecomputeStatusFilter()which creates a new filter function, andgetAll()likely just returns a cached object, this is probably acceptable.🤖 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 141 - 164, computeStatusFilterFn currently calls fullStatusStore.getAll() inside the returned predicate so getAll() runs for every entry; move the call out of the inner function by reading const allStatuses = fullStatusStore.getAll() at the top of computeStatusFilterFn (before returning the (entry) => { ... } closure) so the filter uses the captured snapshot instead of calling getAll() per entry; ensure you keep the same uses of allStatuses and leave recomputeStatusFilter() behavior unchanged so updates still create a new snapshot when needed.code/core/src/manager/components/sidebar/FilterPanel.utils.ts (1)
49-55: Return type could be more precise.Based on the implementation,
getFilterFunctioncan never returnnullsinceUSER_TAG_FILTERalways returns a function (pershared/constants/tags.ts:50-51). The return typeFilterFunction | nullis overly permissive.However, keeping
| nullprovides defensive typing if the behavior ofBUILT_IN_FILTERSorUSER_TAG_FILTERchanges in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager/components/sidebar/FilterPanel.utils.ts` around lines 49 - 55, getFilterFunction's declared return type is overly permissive (FilterFunction | null) even though BUILT_IN_FILTERS[tag] or USER_TAG_FILTER(tag) always produce a function; change the signature of getFilterFunction to return FilterFunction (remove | null) and update any call sites that currently handle null to assume a function (or add explicit guards where appropriate); reference the function name getFilterFunction and the constants BUILT_IN_FILTERS and USER_TAG_FILTER when locating and updating the implementation and its usages.
🤖 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/components/sidebar/FilterPanel.stories.tsx`:
- Around line 321-333: The story OnlyAffectedStatus has an incorrect play()
assertion that claims 'Affected status should not have a color or icon' but the
status 'status-value:affected' (created via makeStatuses) actually defines an
icon (UseSymbol type="affected") and iconColor (theme.fgColor.accent) in the
status definitions (see status.tsx around the affected case); update the
OnlyAffectedStatus story by either removing the play() function or replacing its
assertion to check for the expected icon and color (i.e., verify the rendered
item uses the affected UseSymbol icon and the iconColor equals
theme.fgColor.accent and textColor is null) so the story reflects the real
implementation.
In `@code/core/src/manager/utils/status.tsx`:
- Around line 93-101: The test failure comes from a mismatch between the
component implementation that defines 'status-value:affected' with an icon and
iconColor (see the 'status-value:affected' entry using <UseSymbol
type="affected" /> and theme.fgColor.accent) and the OnlyAffectedStatus story in
FilterPanel.stories.tsx which expects no color or icon; fix this by either
updating the OnlyAffectedStatus story to reflect the current implementation
(remove the assertion that "Affected status should not have a color or icon" and
include the icon/iconColor properties for 'status-value:affected') or revert the
implementation by removing the icon and iconColor from the
'status-value:affected' entry so the story/test expectation remains valid.
---
Nitpick comments:
In `@code/core/src/manager-api/lib/stories.ts`:
- Around line 200-208: The status-filter logic only checks direct children (uses
indexEntries.filter(item => 'parent' in item && item.parent === entry.id)) so
entries with passing grandchildren will still be hidden; add a clear comment
above this block in stories.ts explaining the intentional "direct-children-only"
behavior (mention statusFilterFn, indexEntries, children, allStatuses, and
entry.id) and, if you want grandchild visibility instead, replace the
direct-children check with a small recursive helper (e.g.,
hasPassingDescendant(entryId)) that traverses indexEntries by parent links and
calls statusFilterFn({ ...descendant, statuses: allStatuses[descendant.id] ?? {}
}) until a match is found.
In `@code/core/src/manager-api/modules/stories.ts`:
- Around line 141-164: computeStatusFilterFn currently calls
fullStatusStore.getAll() inside the returned predicate so getAll() runs for
every entry; move the call out of the inner function by reading const
allStatuses = fullStatusStore.getAll() at the top of computeStatusFilterFn
(before returning the (entry) => { ... } closure) so the filter uses the
captured snapshot instead of calling getAll() per entry; ensure you keep the
same uses of allStatuses and leave recomputeStatusFilter() behavior unchanged so
updates still create a new snapshot when needed.
In `@code/core/src/manager-api/tests/stories.test.ts`:
- Around line 1552-1554: Move all vi.mock('../stores/status') calls out of
individual test bodies (e.g., the 'can filter on status' it block) and place a
single vi.mock('../stores/status') at the top-level of the test file before any
describe/it declarations; remove the inline vi.mock calls from the tests
(including the ones referenced around the file) so tests like the one using
createMockModuleArgs({}) no longer call vi.mock inside the it blocks.
In `@code/core/src/manager/components/sidebar/Filter.stories.tsx`:
- Around line 60-113: The story's mocked API inside the useMemo (the api object)
only implements tag filter methods; add corresponding status filter
mocks—implement addStatusFilters(excluded flag), removeStatusFilters(tags),
resetStatusFilters(), and setAllStatusFilters(included, excluded) alongside the
existing tag methods—by updating the same setState logic but operating on
includedStatusFilters, excludedStatusFilters and their default counterparts
(defaultIncludedStatusFilters, defaultExcludedStatusFilters) so FilterPanel’s
status-filter interactions are exercised in the story.
In `@code/core/src/manager/components/sidebar/Filter.tsx`:
- Line 94: The ariaLabel on the filter panel (in Filter.tsx) is currently "Tag
filters" but the panel includes status-based filters too; update the ariaLabel
prop value on the component (the element/property that currently reads
ariaLabel="Tag filters") to a more inclusive label such as "Filters" or "Tag and
status filters" so screen readers accurately reflect the panel's scope.
- Around line 115-118: The ariaLabel currently built in Filter.tsx for the
button uses "tag filter(s)" but activeFilterCount includes status filters too;
update the ariaLabel construction in the component (where ariaLabel and
activeFilterCount are used) to use a generic, accurate phrase like "filters" (or
differentiate counts if you have separate tag/status counts) so the label no
longer misleadingly references only "tag" filters while representing all active
filters.
In `@code/core/src/manager/components/sidebar/FilterPanel.tsx`:
- Around line 149-160: The Reset button in FilterPanel currently only calls
api.resetTagFilters() (see ActionList.Button with id "reset-filters") so status
filters remain unchanged; update the onClick handler to also call
api.resetStatusFilters() (or call a combined reset method if you prefer) so both
tag and status filters return to defaults when pressed, and ensure the handler
uses the FilterPanel's api reference consistently and the button's disabled
state (isDefaultSelection) still reflects the combined-default condition.
In `@code/core/src/manager/components/sidebar/FilterPanel.utils.ts`:
- Around line 49-55: getFilterFunction's declared return type is overly
permissive (FilterFunction | null) even though BUILT_IN_FILTERS[tag] or
USER_TAG_FILTER(tag) always produce a function; change the signature of
getFilterFunction to return FilterFunction (remove | null) and update any call
sites that currently handle null to assume a function (or add explicit guards
where appropriate); reference the function name getFilterFunction and the
constants BUILT_IN_FILTERS and USER_TAG_FILTER when locating and updating the
implementation and its usages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b324fa4d-f782-4e98-80a8-acd7bd124977
📒 Files selected for processing (18)
code/core/src/components/components/ActionList/ActionList.tsxcode/core/src/manager-api/lib/stories.tscode/core/src/manager-api/modules/stories.tscode/core/src/manager-api/tests/stories.test.tscode/core/src/manager/components/sidebar/Filter.stories.tsxcode/core/src/manager/components/sidebar/Filter.story-helpers.tsxcode/core/src/manager/components/sidebar/Filter.tsxcode/core/src/manager/components/sidebar/FilterPanel.stories.tsxcode/core/src/manager/components/sidebar/FilterPanel.tsxcode/core/src/manager/components/sidebar/FilterPanel.utils.tscode/core/src/manager/components/sidebar/FilterPanelItem.tsxcode/core/src/manager/components/sidebar/SearchResults.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/StatusButton.tsxcode/core/src/manager/components/sidebar/TagsFilterPanel.tsxcode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/components/sidebar/useFilterData.tsxcode/core/src/manager/utils/status.tsx
💤 Files with no reviewable changes (1)
- code/core/src/manager/components/sidebar/TagsFilterPanel.tsx
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
code/core/src/manager-api/modules/stories.ts (1)
128-151: Performance:fullStatusStore.getAll()called per entry during filtering.The
computeStatusFilterFncallsfullStatusStore.getAll()inside the returned filter function (line 137). Since this filter runs for every index entry, the store lookup happens O(n) times during filtering.Consider capturing the statuses outside the inner function to avoid repeated lookups:
const computeStatusFilterFn = ( includedStatusFilters: StatusValue[], excludedStatusFilters: StatusValue[] ): API_FilterFunction => { + const allStatuses = fullStatusStore.getAll() ?? {}; return (entry: API_PreparedIndexEntry) => { if (!includedStatusFilters.length && !excludedStatusFilters.length) { return true; } - const allStatuses = fullStatusStore.getAll() ?? {}; const storyStatuses = allStatuses[entry.id];However, this changes the semantics - the current implementation gets fresh status data on each call, while the suggested change captures statuses at filter creation time. If status data can change between filter creations, the current approach may be intentional.
🤖 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 128 - 151, computeStatusFilterFn currently calls fullStatusStore.getAll() for every entry inside the returned filter, causing repeated lookups; capture the result once at the start of computeStatusFilterFn (e.g., const allStatuses = fullStatusStore.getAll() ?? {}) and use that captured allStatuses within the returned API_FilterFunction applied to API_PreparedIndexEntry, so the store is not queried per-entry (if you must preserve “fresh” semantics, instead add an explicit refresh step or pass a snapshot parameter to computeStatusFilterFn).code/core/src/manager/components/sidebar/FilterPanelLink.tsx (1)
45-53: Consider showing the filter icon when included.The current icon logic shows
nullwhenisIncludedis true, meaning no icon appears for actively included filters. This may be intentional, but it could be confusing since excluded filters showDeleteIconwhile included filters show nothing.Consider whether showing the original
iconfor included filters would provide better visual feedback:<ActionList.Icon> - {isExcluded ? <DeleteIcon /> : isIncluded ? null : icon} + {isExcluded ? <DeleteIcon /> : icon} <Form.Checkbox🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager/components/sidebar/FilterPanelLink.tsx` around lines 45 - 53, The ActionList.Icon currently renders DeleteIcon when isExcluded, null when isIncluded, and icon otherwise; change this to render the original icon for included filters so active includes have visible feedback—update the JSX in FilterPanelLink (the ActionList.Icon block) to return {isExcluded ? <DeleteIcon /> : icon} and ensure the rest of the checkbox handlers (isChecked, onCheckboxChange, data-tag, aria-label) remain unchanged so visual behavior is consistent.code/core/src/manager/components/sidebar/FilterPanel.utils.ts (1)
51-57: Return type| nullis unreachable.
getFilterFunctiondeclares a return type ofFilterFunction | null, butUSER_TAG_FILTER(tag)always returns a function (as shown in the relevant code snippet atcode/core/src/shared/constants/tags.ts:50-52). Thenullcase is never reached.Consider removing the
nullfrom the return type for accuracy:-export const getFilterFunction = (tag: Tag): FilterFunction | null => { +export const getFilterFunction = (tag: Tag): FilterFunction => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager/components/sidebar/FilterPanel.utils.ts` around lines 51 - 57, The return type on getFilterFunction is overly broad because USER_TAG_FILTER(tag) always returns a FilterFunction, so remove the unreachable `| null` from the signature of getFilterFunction and update its type to return just `FilterFunction`; ensure the function body still returns values from BUILT_IN_FILTERS or USER_TAG_FILTER unchanged and adjust any type annotations or callers that assumed a nullable result (references: getFilterFunction, BUILT_IN_FILTERS, USER_TAG_FILTER).code/core/src/manager/components/sidebar/FilterPanel.tsx (1)
147-158:isDefaultSelectiondoesn't account for status filters.The
isDefaultSelectioncheck only compares tag filters to their defaults, butisNothingSelectedYetincludes status filters. This means the "Reset filters" button's disabled state doesn't consider status filter state, which could lead to the button being disabled even when status filters are active.If "Reset" should restore everything to defaults (including clearing status filters), update the disabled logic:
const isDefaultSelection = areFiltersEqual(includedFilters, defaultIncludedFilters) && - areFiltersEqual(excludedFilters, defaultExcludedFilters); + areFiltersEqual(excludedFilters, defaultExcludedFilters) && + includedStatusFilters.length === 0 && + excludedStatusFilters.length === 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager/components/sidebar/FilterPanel.tsx` around lines 147 - 158, isDefaultSelection currently only compares tag filters via areFiltersEqual(includedFilters, defaultIncludedFilters) and areFiltersEqual(excludedFilters, defaultExcludedFilters) but ignores status filters, causing the Reset button disabled logic to be incorrect; update isDefaultSelection to also compare includedStatusFilters and excludedStatusFilters to their defaults (or include analogous defaultStatus arrays) using the same equality helper (or a new areStatusFiltersEqual) so the Reset button reflects the full filter state (tags + status) alongside isNothingSelectedYet and hasDefaultSelection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/manager-api/modules/stories.ts`:
- Around line 128-151: computeStatusFilterFn currently calls
fullStatusStore.getAll() for every entry inside the returned filter, causing
repeated lookups; capture the result once at the start of computeStatusFilterFn
(e.g., const allStatuses = fullStatusStore.getAll() ?? {}) and use that captured
allStatuses within the returned API_FilterFunction applied to
API_PreparedIndexEntry, so the store is not queried per-entry (if you must
preserve “fresh” semantics, instead add an explicit refresh step or pass a
snapshot parameter to computeStatusFilterFn).
In `@code/core/src/manager/components/sidebar/FilterPanel.tsx`:
- Around line 147-158: isDefaultSelection currently only compares tag filters
via areFiltersEqual(includedFilters, defaultIncludedFilters) and
areFiltersEqual(excludedFilters, defaultExcludedFilters) but ignores status
filters, causing the Reset button disabled logic to be incorrect; update
isDefaultSelection to also compare includedStatusFilters and
excludedStatusFilters to their defaults (or include analogous defaultStatus
arrays) using the same equality helper (or a new areStatusFiltersEqual) so the
Reset button reflects the full filter state (tags + status) alongside
isNothingSelectedYet and hasDefaultSelection.
In `@code/core/src/manager/components/sidebar/FilterPanel.utils.ts`:
- Around line 51-57: The return type on getFilterFunction is overly broad
because USER_TAG_FILTER(tag) always returns a FilterFunction, so remove the
unreachable `| null` from the signature of getFilterFunction and update its type
to return just `FilterFunction`; ensure the function body still returns values
from BUILT_IN_FILTERS or USER_TAG_FILTER unchanged and adjust any type
annotations or callers that assumed a nullable result (references:
getFilterFunction, BUILT_IN_FILTERS, USER_TAG_FILTER).
In `@code/core/src/manager/components/sidebar/FilterPanelLink.tsx`:
- Around line 45-53: The ActionList.Icon currently renders DeleteIcon when
isExcluded, null when isIncluded, and icon otherwise; change this to render the
original icon for included filters so active includes have visible
feedback—update the JSX in FilterPanelLink (the ActionList.Icon block) to return
{isExcluded ? <DeleteIcon /> : icon} and ensure the rest of the checkbox
handlers (isChecked, onCheckboxChange, data-tag, aria-label) remain unchanged so
visual behavior is consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 346cff13-8785-447e-af89-ad3cc5e2f96f
📒 Files selected for processing (8)
code/core/src/manager-api/modules/stories.tscode/core/src/manager/components/sidebar/FilterPanel.stories.tsxcode/core/src/manager/components/sidebar/FilterPanel.tsxcode/core/src/manager/components/sidebar/FilterPanel.utils.tscode/core/src/manager/components/sidebar/FilterPanelLink.tsxcode/core/src/manager/components/sidebar/useFilterData.tsxcode/core/src/manager/utils/status.tsxcode/core/src/shared/status-store/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- code/core/src/manager/utils/status.tsx
- code/core/src/manager/components/sidebar/FilterPanel.stories.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
code/core/src/manager-api/modules/stories.ts (2)
1188-1198:⚠️ Potential issue | 🟠 MajorRe-run first-story selection after late status hydration.
If
?statuses=is restored from the URL beforefullStatusStorehas entries, the initialSTORY_SPECIFIEDflow can legitimately suppress navigation against an emptyfilteredIndex. This listener rebuilds the index once statuses arrive, but it never re-selects anything, so a refresh can stay on/even after matches now exist. Afterapi.setIndex(index), if status filters are active and there is still no current selection, callapi.selectFirstStory()(or the same fallback used inSTORY_SPECIFIED).🤖 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 1188 - 1198, The listener registered on fullStatusStore (inside fullStatusStore.onAllStatusChange) re-applies filters and calls recomputeStatusFilter(), then awaits api.setIndex(index) but does not re-run the first-story selection; update this handler so that after await api.setIndex(index) you check whether status filters are active and there is no current selection and then call api.selectFirstStory() (or the same fallback used in the STORY_SPECIFIED flow) to ensure a first story is selected when late status hydration creates matches; reference recomputeStatusFilter(), store.getState().internal_index, api.setIndex(index), and api.selectFirstStory() when making the change.
1188-1203:⚠️ Potential issue | 🟠 MajorAvoid the double rebuild on every status-store update.
computeStatusFilterFnalready readsfullStatusStoreduring predicate evaluation, sorecomputeStatusFilter()does not change the effective filter here. It does callexperimental_setFilter(), which already rebuilds the index and refreshes refs; this listener then immediately does both again, and the secondsetRef()loop drops the returned promises.♻️ Suggested simplification
fullStatusStore.onAllStatusChange(async () => { - // re-apply the filters when the statuses change - recomputeStatusFilter(); - const { internal_index: index } = store.getState(); if (!index) { return; } // apply new filters by setting the index again await api.setIndex(index); const refs = await fullAPI.getRefs(); - Object.entries(refs).forEach(([refId, { internal_index, ...ref }]) => { - fullAPI.setRef(refId, { ...ref, storyIndex: internal_index }, true); - }); + for (const [refId, { internal_index, ...ref }] of Object.entries(refs)) { + await fullAPI.setRef(refId, { ...ref, storyIndex: internal_index }, true); + } });🤖 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 1188 - 1203, The onAllStatusChange listener is causing a duplicate rebuild: recomputeStatusFilter() already calls experimental_setFilter() which rebuilds the index and refreshes refs, so the subsequent await api.setIndex(index) and the fullAPI.getRefs()/fullAPI.setRef(...) loop cause a second rebuild and drop promises; remove the recomputeStatusFilter() call and the explicit api.setIndex + refs loop from the fullStatusStore.onAllStatusChange callback (leave the early return on !index) so only the experimental_setFilter-driven rebuild runs and no duplicate setRef promises are lost.code/core/src/manager-api/tests/stories.test.ts (1)
1550-1550:⚠️ Potential issue | 🟡 MinorRemove the per-test
vi.mock('../stores/status')calls inside test bodies.These calls are placed inside
it()blocks afterfullStatusStoreis already imported at the module level. In Vitest,vi.mock()must be called at the top level (hoisted) before imports to control the module instance. Calls within test bodies have no effect on the already-loaded module. Since the tests use the realfullStatusStore.set()method directly, these inline mocks are ineffective and should be removed.If mocking is actually needed, hoist a single
vi.mock('../stores/status', { spy: true })to the top of the file and access it viavi.mocked(fullStatusStore). Otherwise, remove these calls and keep using the real store.Also applies to: 1893, 1906, 1941, 1976
Per coding guidelines:
Use vi.mock() with the spy: true option for all package and file mocks in Vitest testsandPlace all mocks at the top of the test file before any test cases.🤖 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` at line 1550, Remove the per-test vi.mock('../stores/status') calls inside individual it() blocks because fullStatusStore is imported at module scope and those inline mocks are ineffective; either delete these in-test mocks and keep using the real fullStatusStore.set() calls, or hoist a single top-level mock like vi.mock('../stores/status', { spy: true }) at the top of the file and access the mocked API via vi.mocked(fullStatusStore) in tests; apply this change for the occurrences around fullStatusStore referenced at lines noted (also at the other listed spots) to satisfy the "spy: true" and top-level mocking guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@code/core/src/manager-api/modules/stories.ts`:
- Around line 1188-1198: The listener registered on fullStatusStore (inside
fullStatusStore.onAllStatusChange) re-applies filters and calls
recomputeStatusFilter(), then awaits api.setIndex(index) but does not re-run the
first-story selection; update this handler so that after await
api.setIndex(index) you check whether status filters are active and there is no
current selection and then call api.selectFirstStory() (or the same fallback
used in the STORY_SPECIFIED flow) to ensure a first story is selected when late
status hydration creates matches; reference recomputeStatusFilter(),
store.getState().internal_index, api.setIndex(index), and api.selectFirstStory()
when making the change.
- Around line 1188-1203: The onAllStatusChange listener is causing a duplicate
rebuild: recomputeStatusFilter() already calls experimental_setFilter() which
rebuilds the index and refreshes refs, so the subsequent await
api.setIndex(index) and the fullAPI.getRefs()/fullAPI.setRef(...) loop cause a
second rebuild and drop promises; remove the recomputeStatusFilter() call and
the explicit api.setIndex + refs loop from the fullStatusStore.onAllStatusChange
callback (leave the early return on !index) so only the
experimental_setFilter-driven rebuild runs and no duplicate setRef promises are
lost.
In `@code/core/src/manager-api/tests/stories.test.ts`:
- Line 1550: Remove the per-test vi.mock('../stores/status') calls inside
individual it() blocks because fullStatusStore is imported at module scope and
those inline mocks are ineffective; either delete these in-test mocks and keep
using the real fullStatusStore.set() calls, or hoist a single top-level mock
like vi.mock('../stores/status', { spy: true }) at the top of the file and
access the mocked API via vi.mocked(fullStatusStore) in tests; apply this change
for the occurrences around fullStatusStore referenced at lines noted (also at
the other listed spots) to satisfy the "spy: true" and top-level mocking
guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 691f5b21-56e9-44c1-a6f7-3fdf36b0e900
📒 Files selected for processing (4)
code/core/src/manager-api/modules/statuses.tscode/core/src/manager-api/modules/stories.tscode/core/src/manager-api/modules/tags.tscode/core/src/manager-api/tests/stories.test.ts
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/components/sidebar/FilterPanel.stories.tsx`:
- Around line 213-225: The current makeStatuses construction using
Object.fromEntries over values rebuilds each storyId entry and overwrites
previous typeId maps when duplicate storyId appears; change makeStatuses to
accumulate/merge entries instead (e.g., use values.reduce or a Map) so for each
item (referencing storyId, typeId, statusValue, title) you check if an entry for
that storyId already exists and then add/merge the new typeId object into the
existing map rather than replacing it, ensuring description/title/statusValue
are set for the new typeId while preserving any previously added typeIds for
that same storyId.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44373526-2536-4216-b6fe-ca15408e8d4c
📒 Files selected for processing (1)
code/core/src/manager/components/sidebar/FilterPanel.stories.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/manager/components/sidebar/Filter.stories.tsx (1)
109-147: Add at least one story that actually renders the new Statuses group.This decorator now supports status-filter mutations, but every exported story in this file still seeds only tag data. Since the sidebar hides the status section when all status counts are
0, the stories module still doesn't exercise the new status UI, ordering, or include/exclude flows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager/components/sidebar/Filter.stories.tsx` around lines 109 - 147, Add a new story that seeds the decorator with non-zero status counts so the Statuses group is rendered and its include/exclude flows can be exercised; specifically, create a story that uses the same decorator used by existing stories but initializes state to include non-empty includedStatusFilters/excludedStatusFilters and/or calls the decorator's addStatusFilters/removeStatusFilters/resetStatusFilters methods so counts are >0 and the UI shows the Statuses section, then export that story alongside the tag-only stories to validate ordering and include/exclude behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/manager/components/sidebar/Filter.stories.tsx`:
- Around line 109-147: Add a new story that seeds the decorator with non-zero
status counts so the Statuses group is rendered and its include/exclude flows
can be exercised; specifically, create a story that uses the same decorator used
by existing stories but initializes state to include non-empty
includedStatusFilters/excludedStatusFilters and/or calls the decorator's
addStatusFilters/removeStatusFilters/resetStatusFilters methods so counts are >0
and the UI shows the Statuses section, then export that story alongside the
tag-only stories to validate ordering and include/exclude behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f31009e8-a622-440e-ad12-97a2975aa350
📒 Files selected for processing (1)
code/core/src/manager/components/sidebar/Filter.stories.tsx
2 participants
Closes #34302
What I did
getStatusinstatus.tsxfrom returning tuples to returning named objects ({ icon, iconColor, textColor }), making it the single source of truth for both icon colors and text colorsStatusButton.tsx'swithStatusColornow delegates togetStatusinstead of maintaining a separate color mappingFilterPanelfor better separation of concerns:FilterPanel.utils.ts— constants, types, and pure utility functionsFilterPanelItem.tsx— individual filter row renderinguseFilterData.tsx— data hooks (useTagFilterItems,useStatusFilterItems)FilterPanel.tsx— thin orchestrator componentTagsFilter→FilterandTagsFilterPanel→FilterPanelto reflect the broader filtering scopeIconSymbols.tsxaddStatusFilters,removeStatusFilters,resetStatusFilters, andsetAllTagFiltersmethodsChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
cd code && yarn storybook:uiSidebar/FilterPanelDocumentation
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
Bug Fixes
Refactor
Docs / Stories
Tests