Repository navigation
Review: Remove sidebar in review mode to avoid complex layout state management - #35329
Closed
ghengeveld wants to merge 16 commits into
Closed
ghengeveld wants to merge 16 commits into
ghengeveld wants to merge 16 commits into
Conversation
Review entry and exit now only snapshot and restore sidebar filters; entering a review no longer collapses or restores nav/panel layout state.
The review route no longer renders the sidebar (desktop or mobile menu), and sidebar keyboard shortcuts are ignored there.
Curated review story URLs suppress sidebar rendering and sidebar shortcuts, hide the toolbar show-sidebar control, and gate the review toolbar header on the collection query param.
The fixed-position summary overlay no longer reserves nav width on review routes, matching the layout grid when the sidebar is hidden.
ghengeveld
added
ci:normal
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
code/core/src/manager/components/mobile/navigation/MobileNavigation.tsx (1)
146-164: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClose the mobile menu when
showMenuturns off.Unmounting
MobileMenuDrawerhere leavesisMobileMenuOpenuntouched. If the drawer is open before entering a review route, navigating back will remount it already open.Possible fix
+ useEffect(() => { + if (!showMenu && isMobileMenuOpen) { + setMobileMenuOpen(false); + } + }, [showMenu, isMobileMenuOpen, setMobileMenuOpen]);🤖 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/mobile/navigation/MobileNavigation.tsx` around lines 146 - 164, The MobileMenuDrawer in MobileNavigation is unmounted when showMenu becomes false, but the open state in useLayout remains true, so it can remount already open later. Update the MobileNavigation component to watch showMenu and call setMobileMenuOpen(false) whenever showMenu turns off, using the existing useLayout state helpers so the drawer state is reset consistently.code/core/src/manager/components/layout/Layout.stories.tsx (1)
514-518: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMock the review route in this story.
Line 515 only changes
managerLayoutState.viewMode, butLayoutnow hides the menu fromuseStorybookState().path/customQueryParams. Without stubbing a/review/...route or a/story/...route with the reviewcollectionquery param, Line 518 is asserting against ambient manager state instead of the new contract. As per coding guidelines, "When writing component stories, mock external context (for exampleManagerContext.Provider) inside story decorators orbeforeEach."🤖 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/layout/Layout.stories.tsx` around lines 514 - 518, The story is only setting managerLayoutState.viewMode, but Layout now derives the menu visibility from useStorybookState().path and customQueryParams, so the assertion is relying on ambient manager state. Update the Layout story to explicitly mock the review route context via a story decorator or beforeEach, using either a /review/... path or a /story/... path with the review collection query param, and keep the play assertion in sync with that mocked route state.Source: Coding guidelines
🤖 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.
Inline comments:
In `@code/core/src/manager-api/modules/shortcuts.ts`:
- Around line 276-280: Blocked sidebar shortcuts should exit before calling
preventDefault() so review-route keypresses are ignored instead of swallowed. In
the shortcuts handling logic around isSidebarShortcutBlocked, move the
blocked-route check ahead of the event.preventDefault() call and return
immediately for focusNav, search, and toggleNav when sidebar shortcuts are
blocked. Apply the same early-return pattern in the other affected shortcut
branches so no preventDefault() runs for blocked sidebar shortcuts.
In `@code/core/src/manager/components/review/review-mode.ts`:
- Around line 54-58: Move the REVIEW_MODE_SESSION_KEY write in enterReviewMode
so it happens only after api.setAllTagFilters and api.setAllStatusFilters both
complete successfully. Keep the existing FILTERS_SNAPSHOT_SESSION_KEY
persistence before the awaited calls if needed, but do not mark review mode
active until the setters resolve. Use enterReviewMode and the
sessionStore.write(REVIEW_MODE_SESSION_KEY, ...) call as the key locations to
update.
---
Outside diff comments:
In `@code/core/src/manager/components/layout/Layout.stories.tsx`:
- Around line 514-518: The story is only setting managerLayoutState.viewMode,
but Layout now derives the menu visibility from useStorybookState().path and
customQueryParams, so the assertion is relying on ambient manager state. Update
the Layout story to explicitly mock the review route context via a story
decorator or beforeEach, using either a /review/... path or a /story/... path
with the review collection query param, and keep the play assertion in sync with
that mocked route state.
In `@code/core/src/manager/components/mobile/navigation/MobileNavigation.tsx`:
- Around line 146-164: The MobileMenuDrawer in MobileNavigation is unmounted
when showMenu becomes false, but the open state in useLayout remains true, so it
can remount already open later. Update the MobileNavigation component to watch
showMenu and call setMobileMenuOpen(false) whenever showMenu turns off, using
the existing useLayout state helpers so the drawer state is reset consistently.
🪄 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: 30702a94-1d60-4555-8eca-ad46b0de5995
📒 Files selected for processing (11)
code/core/src/manager-api/modules/shortcuts.tscode/core/src/manager/components/layout/Layout.stories.tsxcode/core/src/manager/components/layout/Layout.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/preview/Preview.tsxcode/core/src/manager/components/preview/tools/menu.tsxcode/core/src/manager/components/review/review-actions.tscode/core/src/manager/components/review/review-mode.test.tscode/core/src/manager/components/review/review-mode.tscode/core/src/manager/components/review/screens/ReviewSummaryPortal.tsxcode/core/src/manager/components/sidebar/ReviewWidget.stories.tsx
💤 Files with no reviewable changes (1)
- code/core/src/manager/components/sidebar/ReviewWidget.stories.tsx
Contributor
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
ghengeveld
changed the title
Auto-accept now runs before the in-review arrival skip, so opening the summary or a collection story marks the matching createdAt as visited and clears the sidebar toast without a click.
Layout and other callers can render before router state is initialized; treat a missing path as a non-review route.
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReview route matching now accepts undefined paths. Review mode stores filters only, leaving review-dismiss navigation async with visit tracking and exit-session guarding. Sidebar, mobile menu, shortcuts, preview toolbar, and inset behavior now depend on review-route and collection query state. ChangesReview Route and Chrome Visibility
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
code/core/src/manager/components/review/review-mode.ts (1)
54-58: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winActive-flag write still precedes the awaited filter setters.
Line 55 marks review mode active before
setAllTagFilters/setAllStatusFiltersresolve. If either rejects, the early-return guard at Line 50 makes every laterenterReviewMode()a no-op while the transition never completed, leaving filters in a partial state. Defer the flag write until both setters succeed and roll back the snapshot on failure.Suggested fix
- sessionStore.write(FILTERS_SNAPSHOT_SESSION_KEY, JSON.stringify(filters)); - sessionStore.write(REVIEW_MODE_SESSION_KEY, '1'); - - await api.setAllTagFilters([], []); - await api.setAllStatusFilters([REVIEWING_STATUS_VALUE], []); + sessionStore.write(FILTERS_SNAPSHOT_SESSION_KEY, JSON.stringify(filters)); + try { + await api.setAllTagFilters([], []); + await api.setAllStatusFilters([REVIEWING_STATUS_VALUE], []); + sessionStore.write(REVIEW_MODE_SESSION_KEY, '1'); + } catch (error) { + sessionStore.remove(FILTERS_SNAPSHOT_SESSION_KEY); + throw error; + }🤖 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/review/review-mode.ts` around lines 54 - 58, The review mode activation flag is being written before the awaited filter updates complete, so a rejection can leave the session marked active while the transition never finished. In enterReviewMode(), keep the snapshot write first, then await api.setAllTagFilters and api.setAllStatusFilters, and only after both succeed write REVIEW_MODE_SESSION_KEY. If either setter fails, do not set the active flag and restore or clear the saved snapshot state so future enterReviewMode() calls can retry cleanly.
🧹 Nitpick comments (1)
code/core/src/manager/components/preview/tools/menu.tsx (1)
14-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMisleading flag name:
isReviewCollectionStoryactually reflects the review-manager route.This field is computed from
isReviewManagerRoute(...), so it istruefor any review-manager route, not specifically a "collection story". The same nameisReviewCollectionStoryis used inPreview.tsxbut derived fromREVIEW_COLLECTION_QUERY_PARAM, so reusing it here with different semantics is confusing. Consider renaming to something likeisReviewManagerRoute/isReviewRoutefor clarity.♻️ Proposed rename
- isReviewCollectionStory: isReviewManagerRoute(state.path, state.customQueryParams), + isReviewRoute: isReviewManagerRoute(state.path, state.customQueryParams),- {({ isVisible, showSidebar, singleStory, isReviewCollectionStory }) => + {({ isVisible, showSidebar, singleStory, isReviewRoute }) => !singleStory && !isVisible && - !isReviewCollectionStory && ( + !isReviewRoute && (🤖 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/preview/tools/menu.tsx` around lines 14 - 17, The flag name in the preview menu is misleading because it is derived from isReviewManagerRoute(...) rather than a collection-story-specific condition. Rename the local boolean in menu.tsx to match its actual meaning, such as isReviewManagerRoute or isReviewRoute, and update any references in the menu component so the naming is consistent with the route check and does not conflict with the different isReviewCollectionStory usage in Preview.tsx.
🤖 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.
Inline comments:
In `@code/core/src/manager/components/layout/Layout.tsx`:
- Around line 154-155: `Layout` is overriding the user’s existing nav visibility
when computing `showSidebar`, causing the sidebar to reappear on non-review
routes. Update the `showSidebar` derivation in `Layout.tsx` to preserve the
current nav-visibility state and only combine it with the route-based review
condition instead of replacing it. Make sure the same `showSidebar` value
continues to drive the desktop sidebar, mobile menu/drawer, and grid layout
consistently.
In `@code/core/src/manager/components/mobile/navigation/MobileNavigation.tsx`:
- Around line 154-164: Reset the mobile menu state when showMenu turns false in
MobileNavigation so the drawer does not remount open after a route change.
Update the component logic around MobileNavigation and MobileMenuDrawer to close
isMobileMenuOpen whenever showMenu becomes false, either by syncing it in the
same conditional path or via an effect tied to showMenu. Make sure the state is
cleared before the drawer is hidden so reopening a route with showMenu true
starts closed.
In `@code/core/src/manager/components/review/ReviewPage.stories.tsx`:
- Around line 500-514: The ReviewPage story currently leaves
NOTIFIED_REVIEW_CREATED_AT_KEY and VISITED_REVIEW_CREATED_AT_KEY behind in
sessionStorage, making later stories order-dependent. Update the story setup
around the ReviewPage.stories.tsx play flow by using a story beforeEach or
decorator to seed and clean up these sessionStorage keys, or ensure they are
removed in a finally block after the play run. Keep the mock/context setup for
this story inside the story decorator or beforeEach, and reference the play
function and any helper that writes to sessionStorage so the cleanup stays close
to the seeded state.
---
Duplicate comments:
In `@code/core/src/manager/components/review/review-mode.ts`:
- Around line 54-58: The review mode activation flag is being written before the
awaited filter updates complete, so a rejection can leave the session marked
active while the transition never finished. In enterReviewMode(), keep the
snapshot write first, then await api.setAllTagFilters and
api.setAllStatusFilters, and only after both succeed write
REVIEW_MODE_SESSION_KEY. If either setter fails, do not set the active flag and
restore or clear the saved snapshot state so future enterReviewMode() calls can
retry cleanly.
---
Nitpick comments:
In `@code/core/src/manager/components/preview/tools/menu.tsx`:
- Around line 14-17: The flag name in the preview menu is misleading because it
is derived from isReviewManagerRoute(...) rather than a
collection-story-specific condition. Rename the local boolean in menu.tsx to
match its actual meaning, such as isReviewManagerRoute or isReviewRoute, and
update any references in the menu component so the naming is consistent with the
route check and does not conflict with the different isReviewCollectionStory
usage in Preview.tsx.
🪄 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: 590a6d25-914c-40e9-8ea2-fe8e7a304b97
📒 Files selected for processing (16)
code/core/src/manager-api/modules/shortcuts.tscode/core/src/manager/components/layout/Layout.stories.tsxcode/core/src/manager/components/layout/Layout.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/preview/Preview.tsxcode/core/src/manager/components/preview/tools/menu.tsxcode/core/src/manager/components/review/ReviewPage.stories.tsxcode/core/src/manager/components/review/components/ReviewNotification.tsxcode/core/src/manager/components/review/review-actions.tscode/core/src/manager/components/review/review-mode.test.tscode/core/src/manager/components/review/review-mode.tscode/core/src/manager/components/review/review-notification.test.tscode/core/src/manager/components/review/screens/ReviewSummaryPortal.tsxcode/core/src/manager/components/sidebar/ReviewWidget.stories.tsxcode/core/src/shared/review/routes.test.tscode/core/src/shared/review/routes.ts
💤 Files with no reviewable changes (1)
- code/core/src/manager/components/sidebar/ReviewWidget.stories.tsx
Contributor
|
Failed to publish canary version of this pull request, triggered by @ghengeveld. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/28442106040 |
Write REVIEW_MODE_SESSION_KEY only after setAllTagFilters and setAllStatusFilters resolve, and remove the filters snapshot if either setter rejects so a failed entry does not block re-entry.
Combine getIsNavShown with the review-route check so leaving review does not remount the sidebar when the user had previously hidden nav.
Close isMobileMenuOpen via useLayoutEffect when the menu is hidden on review routes so the drawer does not remount open after navigation.
Seed and remove NOTIFIED/VISITED review keys in beforeEach and a play finally block so DismissesNotificationOnReviewVisit does not leak state into later stories.
Await filter restoration before navigating away, mark visited reviews on exit, block summary auto-enter during exit, and reject curated story URLs as return targets.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/core/src/manager/components/review/review-actions.test.ts (1)
59-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove these Vitest mock implementations into
beforeEach.This suite installs
setAllTagFilters,setAllStatusFilters, andnavigatebehaviors directly inside the test body, which makes the setup harder to reuse and audit. Please move that behavior intobeforeEachor a nesteddescribehook and keep the case focused on assertions. As per coding guidelines, "Implement mock behaviors inbeforeEachblocks in Vitest tests" and "Avoid inline mock implementations within test cases in Vitest tests".🤖 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/review/review-actions.test.ts` around lines 59 - 80, The Vitest mocks for setAllTagFilters, setAllStatusFilters, and navigate are being redefined inline in the test body, which should be moved into shared setup. Update the review-actions.test.ts suite by placing these mockImplementation behaviors in a beforeEach (or a nested describe hook) near enterReviewMode and the existing order setup, then keep the test case focused on the assertions and any case-specific expectations only.Source: Coding guidelines
🤖 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.
Inline comments:
In `@code/core/src/manager/components/review/review-actions.ts`:
- Around line 60-81: Move the visit-marking logic in review-actions’ exit flow
so `acceptReviewNotification()` only runs after `exitReviewMode(api)` succeeds,
or otherwise wrap the exit path in error handling that rolls back any
suppression/state changes on failure. Update the `recordVisit` block and the
`exitReviewMode` try/finally flow in `review-actions.ts` so `reviewStore`,
`sessionStore`, and the toast suppression/visit persistence only commit after a
successful review exit, preventing unhandled rejections and premature visited
state.
---
Nitpick comments:
In `@code/core/src/manager/components/review/review-actions.test.ts`:
- Around line 59-80: The Vitest mocks for setAllTagFilters, setAllStatusFilters,
and navigate are being redefined inline in the test body, which should be moved
into shared setup. Update the review-actions.test.ts suite by placing these
mockImplementation behaviors in a beforeEach (or a nested describe hook) near
enterReviewMode and the existing order setup, then keep the test case focused on
the assertions and any case-specific expectations only.
🪄 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: f16bf65a-783d-4432-a478-0cb8c7ab021b
📒 Files selected for processing (13)
code/core/src/manager-api/modules/shortcuts.tscode/core/src/manager/components/layout/Layout.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/review/ReviewPage.stories.tsxcode/core/src/manager/components/review/components/ReviewNotification.tsxcode/core/src/manager/components/review/components/ReviewProvider.tsxcode/core/src/manager/components/review/constants.tscode/core/src/manager/components/review/review-actions.test.tscode/core/src/manager/components/review/review-actions.tscode/core/src/manager/components/review/review-mode.tscode/core/src/manager/components/review/review-navigation.test.tscode/core/src/manager/components/review/review-navigation.tscode/core/src/manager/components/review/useReviewNavigationInterceptor.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- code/core/src/manager-api/modules/shortcuts.ts
- code/core/src/manager/components/review/ReviewPage.stories.tsx
- code/core/src/manager/components/mobile/navigation/MobileNavigation.tsx
- code/core/src/manager/components/review/components/ReviewNotification.tsx
- code/core/src/manager/components/review/review-mode.ts
- code/core/src/manager/components/layout/Layout.tsx
Strip reviewing from restored filter snapshots, remove it when no snapshot exists, and hide the fullscreen toolbar button on review routes.
Record review visits only after exitReviewMode succeeds, and mock the review manager route in Layout MobileReview so the mobile menu assertion matches layout.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/manager/components/review/review-actions.test.ts (1)
134-156: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInline
mockRejectedValueOnceconflicts with spy-mocking guideline.As per coding guidelines for
**/*.test.{ts,tsx,js,jsx}, mock behaviors should be "Implement[ed] ... inbeforeEachblocks" and reviewers should "Avoid inline mock implementations within test cases." Line 140 setssetAllTagFilters.mockRejectedValueOnce(...)directly inside the test body rather than inbeforeEach.The test logic itself is sound: it correctly verifies that a restore failure during
exitReviewModeshort-circuits visit recording and notification clearing, matchingnavigateOutOfReview'stry/finallyflow.🤖 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/review/review-actions.test.ts` around lines 134 - 156, The test in review-actions.test.ts is using an inline mockRejectedValueOnce inside the case, which conflicts with the test mocking guideline. Move the failure setup for setAllTagFilters into the test’s beforeEach/shared setup used by navigateOutOfReview and enterReviewMode, or apply the mock there and parameterize the case so the test body only exercises the behavior. Keep the assertions around VISITED_REVIEW_CREATED_AT_KEY and api.clearNotification unchanged, and reference the setAllTagFilters mock and navigateOutOfReview flow when relocating the mock behavior.Source: Coding guidelines
🤖 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/core/src/manager/components/review/review-actions.test.ts`:
- Around line 134-156: The test in review-actions.test.ts is using an inline
mockRejectedValueOnce inside the case, which conflicts with the test mocking
guideline. Move the failure setup for setAllTagFilters into the test’s
beforeEach/shared setup used by navigateOutOfReview and enterReviewMode, or
apply the mock there and parameterize the case so the test body only exercises
the behavior. Keep the assertions around VISITED_REVIEW_CREATED_AT_KEY and
api.clearNotification unchanged, and reference the setAllTagFilters mock and
navigateOutOfReview flow when relocating the mock behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9bceef80-7463-413b-b25d-b36861091e28
📒 Files selected for processing (3)
code/core/src/manager/components/layout/Layout.stories.tsxcode/core/src/manager/components/review/review-actions.test.tscode/core/src/manager/components/review/review-actions.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/manager/components/review/review-actions.ts
Only override ManagerContext in the story decorator; the meta decorator already supplies router and layout providers.
Sidnioulz
removed
the
qa:needed
Contributor
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
3 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.
What I did
Review mode no longer collapses or restores sidebar/panel visibility when entering or leaving a review; it still snapshots and restores sidebar filters only.
On the review summary page and on story routes with a
collectionquery param, the sidebar is not rendered (including the mobile menu drawer), sidebar keyboard shortcuts (toggleNav,focusNav,search) are ignored, and the toolbar “Show sidebar” control is hidden. The review toolbar header on story canvases only renders whencollectionis present.The fixed-position review summary portal no longer reserves left nav width on review routes, so the summary fills the full width when the sidebar is suppressed.
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!
cd code && yarn storybook:ui).?path=/review/).collectionquery param present): confirm no sidebar, no review toolbar header on normal stories withoutcollection, review header visible withcollection, and no “Show sidebar” toolbar button.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.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 pull request has been released as version
0.0.0-pr-35329-sha-ec096ee6. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-35329-sha-ec096ee6 sandboxor in an existing project withnpx storybook@0.0.0-pr-35329-sha-ec096ee6 upgrade.More information
0.0.0-pr-35329-sha-ec096ee6review-sidebar-layoutec096ee61782818766)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=35329Summary by CodeRabbit