UI: Improve sidebar empty state #32548
Conversation
WalkthroughSidebar now computes a boolean Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Sidebar
participant Explorer
participant Ref
participant EmptyBlock
participant NoResults
User->>Sidebar: render with indexJson
Sidebar->>Sidebar: compute hasEntries = entries present
Sidebar->>Explorer: render(props, hasEntries)
Explorer->>Ref: render(props, hasEntries)
Ref->>EmptyBlock: render(isMain, hasEntries)
alt hasEntries == true
EmptyBlock->>NoResults: render "No stories found"
NoResults-->>User: show empty-search result view
else hasEntries == false
EmptyBlock-->>User: show guidance about missing stories / empty composed Storybook
end
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (2)
Comment |
|
View your CI Pipeline Execution ↗ for commit 18a30d8
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
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/RefBlocks.tsx (1)
139-184: Bug: scope hasEntries per ref — empty composed refs mislabelledSidebar computes hasEntries from the main index (indexJson) and Explorer forwards that same boolean to every Ref, so non‑main refs can show the wrong "No stories found" message when the main has entries.
Action:
- Keep current behavior for isMain.
- For composed refs, compute a ref‑scoped boolean and pass that to EmptyBlock. In code/core/src/manager/components/sidebar/Refs.tsx replace:
{state === 'empty' && }
with:
{state === 'empty' && 0} />}
(or const hasRefEntries = length > 0; then pass hasEntries={hasRefEntries})
🧹 Nitpick comments (9)
code/core/src/manager/container/Sidebar.tsx (1)
16-16: Fix internal component name typo ("Sideber" → "Sidebar")Minor but improves clarity (affects React DevTools displayName).
-const Sidebar = React.memo(function Sideber({ onMenuClick }: SidebarProps) { +const Sidebar = React.memo(function Sidebar({ onMenuClick }: SidebarProps) {code/core/src/manager/components/sidebar/Refs.tsx (1)
151-152: Empty-state logic may mislabel composed refsPassing the global hasEntries to every Ref means a composed ref with zero stories may show “No stories found” (filter case) even when that ref truly has no stories at all. Consider using a per-ref flag (e.g., hasRefEntries based on the ref’s unfiltered index) to decide between “filters matched nothing” and “this ref is empty.”
If feasible, compute and pass hasRefEntries from Explorer for each ref. See the related suggestion on Explorer.tsx for a concrete diff.
code/core/src/manager/components/sidebar/Explorer.tsx (1)
12-12: Propagate per-ref “has entries” when possibleWiring hasEntries through is correct. If the ref object exposes its unfiltered index, prefer a per-ref value to avoid misleading empty states on composed refs.
- {dataset.entries.map(([refId, ref]) => ( + {dataset.entries.map(([refId, ref]) => { + // Prefer per-ref entries when available, else fall back to global + const hasRefEntries = + // @ts-expect-error (non strict / depends on RefType shape) + (ref.index && Object.keys(ref.index).length > 0) || + (ref.filteredIndex && Object.keys(ref.filteredIndex).length > 0) || + hasEntries; + return ( <Ref {...ref} key={refId} isLoading={isLoading} isBrowsing={isBrowsing} - hasEntries={hasEntries} + hasEntries={hasRefEntries} selectedStoryId={selected?.refId === ref.id ? selected.storyId : null} highlightedRef={highlightedRef} setHighlighted={setHighlighted} /> - ))} + )})}Also applies to: 18-19, 48-49
code/core/src/manager/components/sidebar/Sidebar.stories.tsx (1)
165-173: Great addition: explicit EmptyIndex storyCovers the “no stories exist” branch driven by an empty indexJson.
Consider adding Chromatic params (e.g., a distinct mode) to snapshot both Empty and EmptyIndex variants for visual regression.
code/core/src/manager/components/sidebar/Sidebar.tsx (1)
151-152: Compute and pass hasEntries from indexJsonSimple and effective for main Storybook empty-state differentiation.
If you later decide to support per-ref differentiation, pass a function or compute ref-local entries in Explorer (see Explorer.tsx comment).
Also applies to: 229-235
code/core/src/manager/components/sidebar/SearchResults.tsx (1)
18-18: Unify “No results” copy across sidebarHere it says “No components found,” while RefBlocks uses “No stories found.” Consider aligning the phrasing for consistency.
- <strong>No components found</strong> + <strong>No stories found</strong> <small>Find components by name or path.</small>Or update RefBlocks to use “components” consistently.
code/core/src/manager/components/sidebar/NoResults.tsx (1)
1-17: Add robust text wrapping fallbackstextWrap: 'balance' is not universally supported. Add safe fallbacks to maintain layout.
export const NoResults = styled.div(({ theme }) => ({ display: 'flex', flexDirection: 'column', textAlign: 'center', - textWrap: 'balance', + // Prefer balanced wrapping where available, with safe fallbacks + textWrap: 'balance', + overflowWrap: 'anywhere', + wordBreak: 'break-word', gap: 4, padding: '10px 0', lineHeight: `18px`, fontSize: `${theme.typography.size.s2}px`, color: theme.color.defaultText, small: { color: theme.textMutedColor, fontSize: `${theme.typography.size.s1}px`, }, }));Please confirm your target browser support matrix accepts textWrap; otherwise the fallbacks will protect spacing.
code/core/src/manager/components/sidebar/Refs.stories.tsx (1)
175-182: Reduce story prop repetitionMany stories repeat hasEntries, isLoading, isBrowsing, selectedStoryId, highlightedRef, setHighlighted. Extract a Template with default args to DRY this up.
const defaultRefProps = { hasEntries: true, isLoading: false, isBrowsing: true, selectedStoryId: '', highlightedRef: { current: null }, setHighlighted: () => {}, }; // Then in each story: <Ref {...refs.optimized} {...defaultRefProps} />Also applies to: 195-204, 206-215, 217-226, 228-237, 239-248, 250-259, 261-270, 272-281, 283-292, 294-304
code/core/src/manager/components/sidebar/RefBlocks.tsx (1)
176-180: Use NoResults for the non‑main empty message for visual consistencyThe non‑main empty message renders with Text while the filter‑empty case uses NoResults. Consider using NoResults here as well to keep spacing/typography consistent across empty states.
- <Text> - This composed Storybook is empty. Perhaps no stories match your selected filters. - </Text> + <NoResults> + <strong>This composed Storybook is empty</strong> + <small>Perhaps no stories match your selected filters.</small> + </NoResults>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
code/core/src/manager/components/sidebar/Explorer.tsx(2 hunks)code/core/src/manager/components/sidebar/NoResults.tsx(1 hunks)code/core/src/manager/components/sidebar/RefBlocks.tsx(2 hunks)code/core/src/manager/components/sidebar/Refs.stories.tsx(11 hunks)code/core/src/manager/components/sidebar/Refs.tsx(3 hunks)code/core/src/manager/components/sidebar/SearchResults.tsx(1 hunks)code/core/src/manager/components/sidebar/Sidebar.stories.tsx(1 hunks)code/core/src/manager/components/sidebar/Sidebar.tsx(2 hunks)code/core/src/manager/container/Sidebar.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
code/core/src/manager/components/sidebar/Refs.stories.tsx (1)
code/core/src/manager/components/sidebar/Refs.tsx (1)
Ref(77-172)
code/core/src/manager/components/sidebar/RefBlocks.tsx (1)
code/core/src/manager/components/sidebar/NoResults.tsx (1)
NoResults(3-17)
code/core/src/manager/components/sidebar/Refs.tsx (1)
code/core/src/manager/components/sidebar/RefBlocks.tsx (1)
EmptyBlock(139-184)
⏰ 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: Danger JS
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (5)
code/core/src/manager/container/Sidebar.tsx (1)
1-1: Import cleanup LGTMRemoving unused imports keeps the container lean.
code/core/src/manager/components/sidebar/Refs.stories.tsx (1)
184-193: Nice: explicit NoEntries storyExercises the “no stories exist” path for a ref.
code/core/src/manager/components/sidebar/Refs.tsx (1)
16-23: Cannot verify — sandbox script failed; confirm every <Ref …> call site provides hasEntriesThe verification run errored with "tee: command not found", so I couldn't confirm that every <Ref …> usage passes hasEntries. Run this from the repo root and attach the output:
rg -nP --type=tsx "<Ref\\b[^>]*>" code -S | while IFS= read -r L; do file=$(printf "%s" "$L" | cut -d: -f1) lineno=$(printf "%s" "$L" | cut -d: -f2) tag=$(printf "%s" "$L" | cut -d: -f3-) printf "%s:%s %s\n" "$file" "$lineno" "$tag" if printf "%s" "$tag" | rg -q "hasEntries\\s*="; then printf " -> hasEntries present\n"; else printf " -> MISSING hasEntries\n"; fi donecode/core/src/manager/components/sidebar/RefBlocks.tsx (2)
13-13: Good reuse of shared stylingImporting and using the shared NoResults component keeps empty states visually consistent across the sidebar.
154-158: Add rel for target=_blank links (noopener, noreferrer)For security and performance, links opened in a new tab should set rel="noopener noreferrer". If the internal Link component doesn’t do this automatically, pass rel explicitly.
Example:
- <Link + <Link + rel="noopener noreferrer" href="https://storybook.js.org/docs/api/main-config/main-config-stories?ref=ui" cancel={false} target="_blank" >Repeat for the “writing-stories” link.
Also applies to: 166-169
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
code/core/src/manager/components/sidebar/Explorer.stories.tsx (2)
85-85: Derive hasEntries from dataset to avoid driftHard-coding
truecan get out of sync with the dataset. Derive it from the entries length.Apply this minimal change:
- hasEntries={true} + hasEntries={Object.entries(simple).length > 0}Optionally, factor entries once:
const simpleEntries = Object.entries(simple); // ... <Explorer dataset={{ hash: simple, entries: simpleEntries }} // ... hasEntries={simpleEntries.length > 0} />
98-98: Same here: compute hasEntries from withRefsKeep
hasEntriesconsistent with the provided dataset.- hasEntries={true} + hasEntries={Object.entries(withRefs).length > 0}Optionally:
const withRefsEntries = Object.entries(withRefs); // ... <Explorer dataset={{ hash: withRefs, entries: withRefsEntries }} // ... hasEntries={withRefsEntries.length > 0} />Also consider adding a story variant with an empty dataset (
entries: []) andhasEntries={false}to showcase the “no stories exist” state, if not already covered elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/manager/components/sidebar/Explorer.stories.tsx(2 hunks)
⏰ 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). (1)
- GitHub Check: Core Unit Tests, windows-latest
Closes #
What I did
Split up the empty state for "nothing matches filters" and "no stories exist" situations:
The latter now has links to the docs to resolve the problem.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
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 canary-release-pr.yml --field pr=<PR_NUMBER>Summary by CodeRabbit