UI: Prevent crash when tag filters contain undefined entries#33931
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change updates the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
code/core/src/manager/components/sidebar/TagsFilterPanel.tsx (1)
151-151:⚠️ Potential issue | 🟠 MajorIncomplete fix:
hasUserTagsis still vulnerable to the same undefined-entry crash.The guard added inside
groupByTypeprotects line 140, but line 151 uses the sameObject.values(filtersById)and immediately destructures{ type }inside.some(...). An undefined entry infiltersByIdwill still throw"Cannot destructure property 'type' of undefined"here, reproducing the original crash via a different path.🐛 Proposed fix
- const hasUserTags = Object.values(filtersById).some(({ type }) => type === 'tag'); + const hasUserTags = Object.values(filtersById).some((filter) => filter?.type === 'tag');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager/components/sidebar/TagsFilterPanel.tsx` at line 151, The hasUserTags computation is still vulnerable because Object.values(filtersById).some(({ type }) => ...) will throw if an entry is undefined; update the check used to compute hasUserTags (the hasUserTags constant in TagsFilterPanel) to guard against undefined entries—e.g., iterate safely by filtering out falsy values or by using a predicate like item => item && item.type === 'tag'—so it mirrors the defensive logic added in groupByType and avoids destructuring undefined.
🧹 Nitpick comments (1)
code/core/src/manager/components/sidebar/TagsFilterPanel.tsx (1)
20-30: Tighten the type signature to reflect that undefined entries are the actual concern.
filters: Filter[]declares a non-nullable array, so TypeScript's strict mode sees.filter(Boolean)as a no-op type-wise and gives no narrowing. The runtime guard is correct, but the declared type doesn't communicate the intent. Consider widening the parameter type and using a type predicate for proper narrowing:♻️ Proposed type-safe refactor
-export const groupByType = (filters: Filter[]) => - filters - .filter(Boolean) - .reduce( +export const groupByType = (filters: (Filter | undefined | null)[]) => + filters + .filter((f): f is Filter => f != null) + .reduce( (acc, filter) => { acc[filter.type] ??= []; acc[filter.type].push(filter); return acc; }, {} as Record<string, Filter[]> );The call site at line 140 (
Object.values(filtersById)) would need no change — TypeScript will accept it once the parameter type is widened.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager/components/sidebar/TagsFilterPanel.tsx` around lines 20 - 30, The parameter type for groupByType is too narrow—change the signature from filters: Filter[] to filters: (Filter | undefined)[] (or Array<Filter | undefined>) and update the filter call to use a type predicate so TypeScript knows you're removing undefined values; keep the runtime .filter(Boolean) logic but replace its type with a proper predicate or declare the function as (f): f is Filter => Boolean(f) so the reducer sees only Filter items; reference groupByType and the filters parameter when making this change (call sites like Object.values(filtersById) need no modification).
🤖 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/components/sidebar/TagsFilterPanel.tsx`:
- Line 151: The hasUserTags computation is still vulnerable because
Object.values(filtersById).some(({ type }) => ...) will throw if an entry is
undefined; update the check used to compute hasUserTags (the hasUserTags
constant in TagsFilterPanel) to guard against undefined entries—e.g., iterate
safely by filtering out falsy values or by using a predicate like item => item
&& item.type === 'tag'—so it mirrors the defensive logic added in groupByType
and avoids destructuring undefined.
---
Nitpick comments:
In `@code/core/src/manager/components/sidebar/TagsFilterPanel.tsx`:
- Around line 20-30: The parameter type for groupByType is too narrow—change the
signature from filters: Filter[] to filters: (Filter | undefined)[] (or
Array<Filter | undefined>) and update the filter call to use a type predicate so
TypeScript knows you're removing undefined values; keep the runtime
.filter(Boolean) logic but replace its type with a proper predicate or declare
the function as (f): f is Filter => Boolean(f) so the reducer sees only Filter
items; reference groupByType and the filters parameter when making this change
(call sites like Object.values(filtersById) need no modification).
yannbf
changed the title
abhaysinh1000
force-pushed
the
fix/tags-filter-undefined
branch
from
833049e to
a7fab86
Compare
|
CI seems to be failing in a flaky mobile E2E test This does not appear related to the tag filter change. |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 188 | 188 | 0 |
| Self size | 76 KB | 76 KB | 🚨 +48 B 🚨 |
| Dependency size | 32.21 MB | 32.22 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/ember
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 192 | 192 | 0 |
| Self size | 15 KB | 15 KB | 🎉 -18 B 🎉 |
| Dependency size | 28.92 MB | 28.93 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 535 | 535 | 0 |
| Self size | 648 KB | 648 KB | 0 B |
| Dependency size | 59.87 MB | 59.89 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 274 | 274 | 0 |
| Self size | 24 KB | 24 KB | 0 B |
| Dependency size | 44.54 MB | 44.55 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/server-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 200 | 200 | 0 |
| Self size | 16 KB | 16 KB | 🚨 +12 B 🚨 |
| Dependency size | 33.46 MB | 33.47 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-react-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 170 | 170 | 0 |
| Self size | 18 KB | 18 KB | 🚨 +24 B 🚨 |
| Dependency size | 31.41 MB | 31.42 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
UI: Prevent crash when tag filters contain undefined entries (cherry picked from commit 5a9c6ec)
github-actions
Bot
added
the
patch:done
2 participants
Closes #33803
What does this change?
Guards against undefined entries in the tag filters used by the manager sidebar.
Why is this needed?
When tags are configured globally but not yet used in any story, the filters
array can temporarily contain
undefinedentries during hot reload. Thesidebar assumed all filters were defined, which could cause a runtime crash:
Cannot read properties of undefined (reading 'type')
How was this tested?
Summary by CodeRabbit