Manager: Fix layout.showPanel config#34777
Merged
Sidnioulz merged 8 commits intoMay 29, 2026
Merged
Conversation
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:
📝 WalkthroughWalkthroughAdds required layout flags and addon typing; implements applyLayoutOptions to merge layout options, deprecate legacy top-level keys, and map showNav/showPanel to size fields (zeroing/restoring); wires helper into getInitialOptions and api.setOptions; expands tests for visibility, preservation, and deprecation precedence. ChangesLayout Options Type and Application
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
valentinpalkovic
moved this from Empathy Queue (prioritized)
to In Progress
in Core Team Projects
Add jsdoc explaining the capture-after-merge ordering of recentVisibleSizes, plus inline notes on the unknown-key safety net and the singleStory nav guard. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/manager-api/modules/layout.ts (1)
196-204: 💤 Low valueConsider documenting the
show*: truebehavior for completeness.The JSDoc clearly explains the
show*: falsecase (capturing sizes after merge). For completeness, you might document that whenshow* = true, sizes are restored fromrecentVisibleSizesregardless of any explicit size values passed in the same call.📝 Optional documentation enhancement
* Numeric sizes from `options` are merged in first, so `recentVisibleSizes` is * captured *after* that merge — meaning if a caller passes both a size and * `show*: false` in the same payload, the new size is what we remember for * later restoration via `togglePanel(true)` / `toggleNav(true)`. + * + * When `show*: true`, sizes are always restored from `recentVisibleSizes`, + * ignoring any explicit size values passed in the same call. */🤖 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-api/modules/layout.ts` around lines 196 - 204, Update the JSDoc above the layout merge routine to explicitly document the "show*: true" behavior: state that when `showSidebar` or `showPanel` is set to true the implementation restores sizes from `recentVisibleSizes` (so those restored sizes take precedence even if numeric size fields are provided in the same options payload), and reference the related restore helpers like `togglePanel(true)` / `toggleNav(true)` and the `recentVisibleSizes` field to make the behavior clear; keep the note about numeric sizes being merged first and the `show*: false` behavior as-is.
🤖 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-api/modules/layout.ts`:
- Around line 196-204: Update the JSDoc above the layout merge routine to
explicitly document the "show*: true" behavior: state that when `showSidebar` or
`showPanel` is set to true the implementation restores sizes from
`recentVisibleSizes` (so those restored sizes take precedence even if numeric
size fields are provided in the same options payload), and reference the related
restore helpers like `togglePanel(true)` / `toggleNav(true)` and the
`recentVisibleSizes` field to make the behavior clear; keep the note about
numeric sizes being merged first and the `show*: false` behavior as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cab6ab84-84ae-435a-8f65-03fb0f86eb8a
📒 Files selected for processing (1)
code/core/src/manager-api/modules/layout.ts
- Fold the `singleStory` nav guard into the `showSidebar === false` branch so the function ends on a single return; equivalent behavior, easier to read. - Add the missing `showPanel: undefined` entry to the default `layoutCustomisations` so it matches `API_LayoutCustomisations`. - Omit `recentVisibleSizes` from `API_LayoutOptions` — it is internal restoration bookkeeping and not something callers should set via `addons.setConfig`. - Note in `API_LayoutOptions` that `showToolbar` already comes from `Partial<API_Layout>` and is intentionally not redeclared. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Live verification in internal Storybook UII applied this PR's changes onto a fresh checkout, recompiled Test matrix
What this confirms
What I did not cover here
Looks good to me. 👍 |
The `Omit<Partial<API_Layout>, 'recentVisibleSizes'>` introduced in the previous commit breaks the rollup-plugin-dts build of core because `applyLayoutOptions` calls `pick(layoutOptions, layoutKeys)` where `layoutKeys` is typed as `(keyof API_Layout)[]` (includes `recentVisibleSizes`) but the destructured `layoutOptions` no longer accepts it. Reverting to `extends Partial<API_Layout>` restores the type compatibility. The `recentVisibleSizes` field is internal restoration bookkeeping and practically no caller sets it via `addons.setConfig`, so the type-tightening was nice-to-have and not worth the breakage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
valentinpalkovic
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/manager-api/tests/layout.test.ts (1)
540-553: ⚡ Quick winMocking pattern does not follow coding guidelines.
The inline
vi.spyOn().mockImplementation()pattern violates the guidelines which require:
- Using
vi.mock()withspy: trueat the top of the file- Implementing mock behaviors in
beforeEachblocks- Accessing mocks via
vi.mocked()As per coding guidelines: "Use
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests", "Implement mock behaviors inbeforeEachblocks in Vitest tests", and "Avoid inline mock implementations within test cases in Vitest tests".♻️ Suggested refactor to follow mocking guidelines
+vi.mock('storybook/internal/client-logger', { spy: true }); + describe('layout API', () => { let layoutApi: SubAPI; let store: Store; let provider: API_Provider<API>; let currentState: SubState & { selectedPanel: AddonsSubState['selectedPanel']; singleStory?: boolean; }; beforeEach(() => { + vi.mocked(clientLogger.deprecate).mockReset(); currentState = { ...getDefaultLayoutState(),Then in tests:
it('should prioritize options.layout over top-level layout keys', () => { - const deprecateSpy = vi.spyOn(clientLogger, 'deprecate').mockImplementation(() => {}); - layoutApi.setOptions({ showNav: true, showPanel: true, layout: { showNav: false, showPanel: false }, }); expect(currentState.layout.navSize).toBe(0); expect(currentState.layout.bottomPanelHeight).toBe(0); expect(currentState.layout.rightPanelWidth).toBe(0); - expect(deprecateSpy).toHaveBeenCalled(); + expect(vi.mocked(clientLogger.deprecate)).toHaveBeenCalled(); });🤖 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-api/tests/layout.test.ts` around lines 540 - 553, The test uses an inline vi.spyOn(...).mockImplementation() which violates the project's mocking guidelines; replace the inline spy with a top-level vi.mock(...) that enables spying (spy: true) and move the mock implementation into the file's beforeEach so tests remain declarative. Specifically, remove the inline vi.spyOn(clientLogger, 'deprecate').mockImplementation(...) in the test, configure vi.mock for the module that exports clientLogger with spy: true at the top of the test file, and in beforeEach call vi.mocked(clientLogger).deprecate.mockImplementation(() => {}) (or reset/restore as appropriate) so the test can call layoutApi.setOptions(...) and assert vi.mocked(clientLogger).deprecate was called without inline spies.
🤖 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-api/tests/layout.test.ts`:
- Around line 540-553: The test uses an inline
vi.spyOn(...).mockImplementation() which violates the project's mocking
guidelines; replace the inline spy with a top-level vi.mock(...) that enables
spying (spy: true) and move the mock implementation into the file's beforeEach
so tests remain declarative. Specifically, remove the inline
vi.spyOn(clientLogger, 'deprecate').mockImplementation(...) in the test,
configure vi.mock for the module that exports clientLogger with spy: true at the
top of the test file, and in beforeEach call
vi.mocked(clientLogger).deprecate.mockImplementation(() => {}) (or reset/restore
as appropriate) so the test can call layoutApi.setOptions(...) and assert
vi.mocked(clientLogger).deprecate was called without inline spies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8ee79f6-b88d-4a5b-a18a-4ca5d2e4832d
📒 Files selected for processing (4)
code/core/src/manager-api/modules/layout.tscode/core/src/manager-api/tests/layout.test.tscode/core/src/types/modules/addons.tscode/core/src/types/modules/api.ts
Sidnioulz
approved these changes
May 28, 2026
valentinpalkovic
approved these changes
May 29, 2026
github-project-automation
Bot
moved this from In Progress
to Done
in Core Team Projects
42 tasks
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.
Closes #32062
What I did
Fixed
layout.showPanelso manager config can set the default addon panel visibility.layout.showPanelandlayout.showSidebartyping for manager config layout optionslayout.showPanel: falseby collapsing the stored addon panel sizes during initial config and runtimesetOptionslayoutCustomisations.showPanelas the stronger per-context override pathsetOptionsChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
EDIT by @Sidnioulz:
Between each of these tests, clear local storage and restart Storybook.
First, set this in
code/.storybook/manager.tsx:You should see no sidebar, toolbar or panel, and no keyboard shortcut should work (make sure to test keyboard shortcuts while focused in the SB ui and not in the preview).
Next, set this in
code/.storybook/manager.tsx:You should see the same thing but with deprecation warnings in the console.
Next, set this in
code/.storybook/manager.tsx:You should see the whole UI and have working keyboard shortcuts, alongside deprecation warnings, because the well-placed config keys take precedence over top-level keys.
Validated locally with:
git diff --checkoxfmt --check code/core/src/types/modules/api.ts code/core/src/types/modules/addons.ts code/core/src/manager-api/modules/layout.ts code/core/src/manager-api/tests/layout.test.tsI was not able to run the targeted local Vitest suite because this sparse checkout could not resolve built internal Storybook workspace packages such as
@storybook/addon-vitest. I also could not run the targeted ESLint command because this checkout could not resolve the internaleslint-plugin-storybookpackage fromcode.Documentation
🦋 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
Tests