Skip to content

Core: Redesign and refactor Viewports tool#33290

Merged
ghengeveld merged 32 commits into
nextfrom
viewport-viewer
Jan 9, 2026
Merged

Core: Redesign and refactor Viewports tool#33290
ghengeveld merged 32 commits into
nextfrom
viewport-viewer

Conversation

@ghengeveld
Copy link
Copy Markdown
Member

@ghengeveld ghengeveld commented Dec 5, 2025

Contributes to #33245

What I did

  • Refactored viewports handling into a hook
  • Move dimension inputs from toolbar to canvas to reduce clutter
  • Updated toolbar Select component to use ActionList components for visual consistency
  • Added drag handles to resize the viewport, along with realtime size indicators
  • Persist custom size to the URL for sharing
Screen.Recording.2026-01-08.at.11.19.56.mov

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

  • Use the Viewports tool in the toolbar to switch between viewport presets.
  • Try using up/down/enter/esc keys to navigate the menu.
  • Try changing the width and height values through the inputs above the iframe.
  • Try changing width, height, and both together through the drag handles.
  • Try switching orientation through the "swap" button.
  • Try resetting the custom size back to the preset through the "undo" button.
  • Verify that refreshing the browser will keep the selected viewport.
  • Check the other toolbar menus to see if they look okay.

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Make 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/core team 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

    • Right-side "aside" content for select options; new SizeInput control; resizable Viewport with drag-to-resize, numeric inputs, rotate and restore actions; added Viewport stories and watch device icon.
  • Refactor

    • Viewport tool rebuilt on a centralized hook/state; Select options now render via ActionList; ActionList gains listbox-friendly layouts and two-line title/description items.
  • Style

    • Improved visuals, focus, hover and active/disabled interaction styling across controls.

✏️ Tip: You can customize this high-level summary in your review settings.

@ghengeveld ghengeveld self-assigned this Dec 5, 2025
@nx-cloud
Copy link
Copy Markdown

nx-cloud Bot commented Dec 5, 2025

View your CI Pipeline Execution ↗ for commit ccbcc9a

Command Status Duration Result
nx run-many -t compile,check,knip,test,pretty-d... ✅ Succeeded 10m 46s View ↗

☁️ Nx Cloud last updated this comment at 2026-01-09 13:14:10 UTC

- Updated SizeInput to correctly parse units, allowing for percentages and other units.
- Refactored Viewport component to improve layout and styling, including adjustments to padding and border handling.
- Removed deprecated shortcuts functionality and streamlined viewport tool integration.
- Enhanced viewport state management to support custom viewport formats.
@ghengeveld ghengeveld marked this pull request as ready for review December 8, 2025 14:04
- Updated SizeInput to accept inputs even when invalid.
- Refactored Viewport to improve drag handling and ensure minimum dimensions for width and height.
- Added focus-visible styles for SizeInput to enhance user experience during resizing.
@github-actions github-actions Bot added the Stale label Dec 28, 2025
@Sidnioulz Sidnioulz self-requested a review January 6, 2026 16:07
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 7, 2026

📝 Walkthrough

Walkthrough

Adds a new interactive Viewport component and useViewport hook with resize/rotate/restore controls; refactors ActionList styling and listbox support; composes SelectOption with ActionList and adds an aside prop; replaces FramesRenderer IFrame usage with Viewport; introduces SizeInput and reorganizes viewport types/icons.

Changes

Cohort / File(s) Summary
Viewport component & hook
code/core/src/manager/components/preview/Viewport.tsx, code/core/src/viewport/useViewport.ts
New Viewport component with drag-to-resize handles, rotate/restore actions, and live resizing logic; new useViewport hook providing state parsing, resize/select/rotate APIs, VIEWPORT_MIN_* constants, and shortcut registration.
Viewport stories & SizeInput
code/core/src/manager/components/preview/Viewport.stories.tsx, code/core/src/manager/components/preview/SizeInput.tsx
Added Storybook stories for Viewport scenarios and a new SizeInput component (label, unit handling, arrow-key increment/decrement, px normalization).
Preview rendering & iframe tweaks
code/core/src/manager/components/preview/FramesRenderer.tsx, code/core/src/manager/components/preview/Iframe.tsx, code/core/src/manager/components/preview/utils/components.ts
FramesRenderer now renders Viewport instead of the legacy IFrame; minor iframe CSS border change and CanvasWrap sizing relaxed to min-width/min-height.
ActionList styles & stories
code/core/src/components/components/ActionList/ActionList.tsx, code/core/src/components/components/ActionList/ActionList.stories.tsx
Extended ActionList item styling for listbox/option role, active/selected/disabled states and CSS-variable based active colors; ActionList.Text switched to vertical layout; StyledButton gap sizing; added Listbox story and updated stories to use ActionList.Icon wrapper.
Select & SelectOption composition
code/core/src/components/components/Select/Select.tsx, code/core/src/components/components/Select/SelectOption.tsx, code/core/src/components/components/Select/helpers.tsx
Option type gained aside?: React.ReactNode; SelectOption now composes ActionList.Item/ActionList.Icon/ActionList.Text, rendering optional aside to the right; setActiveOption can be called without changing selection (changeSelection flag).
Viewport tool & registration changes
code/core/src/viewport/components/Tool.tsx, code/core/src/viewport/manager.tsx
ViewportTool refactored to use useViewport (parameterless) and show dimensions per option; addons.register callback no longer receives api, render uses <ViewportTool />.
Types, icons & utils reorganization
code/core/src/viewport/types.ts, code/core/src/viewport/viewportIcons.tsx, code/core/src/viewport/utils.tsx
Introduced ViewportType and broadened accepted global value formats; moved/added iconsMap to viewportIcons.tsx and removed older ActiveViewportSize/ActiveViewportLabel and previous icons mapping.
Removed shortcuts module
code/core/src/viewport/shortcuts.ts
Deleted legacy viewport shortcuts registration (next/previous/reset helpers).
Misc: Select aside propagation & small tweaks
code/core/src/components/components/Select/Select.tsx, code/core/src/components/components/Select/SelectOption.tsx, code/core/src/manager/components/sidebar/ChecklistWidget.tsx, code/addons/a11y/src/components/VisionSimulator.tsx, code/core/src/manager/components/layout/useDragging.ts
Propagated aside to SelectOption rendering; minor style adjustments (skipped checklist alignSelf, numeric sizing for ColorIcon), and iframe DOM query id changed for dragging pointer-events handling.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant User
participant Viewport
participant useViewport
participant ManagerAPI

User->>Viewport: pointerdown on drag handle
Viewport->>Viewport: capture initial size & handle side
User->>Viewport: pointermove (drag)
Viewport->>Viewport: update visual dimensions (disable iframe pointer events)
User->>Viewport: pointerup
Viewport->>useViewport: resize(newWidth, newHeight)
useViewport->>useViewport: normalize units, enforce min dims
useViewport->>ManagerAPI: updateGlobals({ viewport: urlValue })
ManagerAPI->>useViewport: globals updated
useViewport->>Viewport: propagate updated state
Viewport->>User: render updated frame

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 04ed550 and ccbcc9a.

📒 Files selected for processing (1)
  • code/core/src/components/components/Select/Select.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • code/core/src/components/components/Select/Select.tsx
⏰ 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: normal
  • GitHub Check: Core Unit Tests, windows-latest
  • GitHub Check: nx

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

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/components/components/ActionList/ActionList.tsx (1)

159-193: Unused muted prop in ActionListText.

The muted prop is declared in the component's type signature but never used in the styles. Either apply conditional styling based on this prop or remove it.

🔧 Suggested fix to apply the muted prop
-const ActionListText = styled.div<{ muted?: boolean }>(({ theme }) => ({
+const ActionListText = styled.div<{ muted?: boolean }>(({ theme, muted }) => ({
   display: 'flex',
   flexDirection: 'column',
   justifyContent: 'center',
   flexGrow: 1,
   minWidth: 0,
   padding: '8px 0',
   lineHeight: '16px',
+  color: muted ? theme.textMutedColor : 'inherit',

   '& > *': {
🤖 Fix all issues with AI agents
In @code/core/src/manager/components/preview/Viewport.tsx:
- Around line 164-197: The useEffect that attaches mousedown handlers (via
handles = [dragRefX.current, dragRefY.current, dragRefXY.current]) registers
window mousemove/mouseup listeners inside onStart but only removes them inside
onEnd, so if the component unmounts mid-drag those window listeners remain;
update the effect cleanup to also remove any window listeners
(window.removeEventListener('mousemove', onDrag) and
window.removeEventListener('mouseup', onEnd)) and ensure drag state is cleared
(call setDragging('none') and reset dragStart.current) so on unmount you clean
up the listeners added in onStart and prevent a memory leak involving
onDrag/onEnd.

In @code/core/src/viewport/useViewport.ts:
- Around line 144-157: The resize callback uses a hardcoded minimum dimension
40; replace that magic number with the defined constants VIEWPORT_MIN_WIDTH and
VIEWPORT_MIN_HEIGHT. Inside the resize function (which builds value, matches
URL_VALUE_PATTERN, and calls update), change the checks that use Number(vx) >=
40 and Number(vy) >= 40 to use VIEWPORT_MIN_WIDTH and VIEWPORT_MIN_HEIGHT
respectively, keeping existing logic for percentage units (ux/uy) and the
isRotated/update flow unchanged.
- Around line 23-36: The cycle function can return undefined when viewports is
empty; add a defensive check at the top of cycle (the function named cycle in
useViewport.ts that accepts viewports: ViewportMap, current, direction) to
handle an empty keys array and return a safe string (e.g., an empty string '')
or throw a clear error; specifically, compute const keys =
Object.keys(viewports); if (keys.length === 0) return ''; then proceed with
existing logic to compute currentIndex/nextIndex so callers always receive a
string.
🧹 Nitpick comments (3)
code/core/src/manager/components/preview/SizeInput.tsx (1)

66-86: Consider simplifying the unit extraction regex.

The regex pattern on line 75 /[0-9]{1,4}(%|[a-z]{0,4})?$/ only matches 1-4 trailing digits. For values with more than 4 digits (e.g., "10000"), this might not behave as expected. A simpler approach would be to match just the unit suffix:

♻️ Suggested simplification
-        const unit = inputValue.match(/[0-9]{1,4}(%|[a-z]{0,4})?$/)?.[1] || 'px';
+        const unit = inputValue.match(/(%|[a-z]+)$/)?.[1] || 'px';
code/core/src/viewport/components/Tool.tsx (1)

27-40: Handle optional type gracefully instead of using non-null assertion.

Line 30 uses iconsMap[value.type!] with a non-null assertion, but Viewport.type is optional per the type definition. If a viewport config omits type, this would return undefined for the icon. Consider providing a fallback:

♻️ Suggested fix
       Object.entries(viewportMap).map(([k, value]) => ({
         value: k,
         title: value.name,
-        icon: iconsMap[value.type!],
+        icon: iconsMap[value.type ?? 'other'],
         right: (
code/core/src/manager/components/preview/Viewport.stories.tsx (1)

8-21: Consider adding proper typing for the mock context.

Using any type bypasses TypeScript's strict mode benefits. While mocking is often pragmatic, consider creating a partial type or using as unknown as ManagerContextType pattern to maintain some type safety while still allowing the mock.

♻️ Suggested improvement
+import type { Combo } from 'storybook/manager-api';
+
-const managerContext: any = {
+const managerContext = {
   state: {},
   api: {
     getCurrentParameter: fn(),
     getGlobals: fn(() => ({})),
     getStoryGlobals: fn(() => ({})),
     getUserGlobals: fn(() => ({})),
     updateGlobals: fn(),
     setAddonShortcut: fn(),
     on: fn(),
     off: fn(),
     emit: fn(),
   },
-};
+} as unknown as Combo;
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 13b7cff and 4352437.

📒 Files selected for processing (18)
  • code/core/src/components/components/ActionList/ActionList.stories.tsx
  • code/core/src/components/components/ActionList/ActionList.tsx
  • code/core/src/components/components/Select/Select.tsx
  • code/core/src/components/components/Select/SelectOption.tsx
  • code/core/src/components/components/Select/helpers.tsx
  • code/core/src/manager/components/preview/FramesRenderer.tsx
  • code/core/src/manager/components/preview/Iframe.tsx
  • code/core/src/manager/components/preview/SizeInput.tsx
  • code/core/src/manager/components/preview/Viewport.stories.tsx
  • code/core/src/manager/components/preview/Viewport.tsx
  • code/core/src/manager/components/preview/utils/components.ts
  • code/core/src/viewport/components/Tool.tsx
  • code/core/src/viewport/manager.tsx
  • code/core/src/viewport/shortcuts.ts
  • code/core/src/viewport/types.ts
  • code/core/src/viewport/useViewport.ts
  • code/core/src/viewport/utils.tsx
  • code/core/src/viewport/viewportIcons.tsx
💤 Files with no reviewable changes (2)
  • code/core/src/viewport/utils.tsx
  • code/core/src/viewport/shortcuts.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Format code using Prettier with yarn prettier --write <file>

Files:

  • code/core/src/manager/components/preview/SizeInput.tsx
  • code/core/src/components/components/Select/Select.tsx
  • code/core/src/manager/components/preview/Iframe.tsx
  • code/core/src/components/components/Select/SelectOption.tsx
  • code/core/src/manager/components/preview/utils/components.ts
  • code/core/src/manager/components/preview/Viewport.tsx
  • code/core/src/components/components/Select/helpers.tsx
  • code/core/src/viewport/viewportIcons.tsx
  • code/core/src/manager/components/preview/Viewport.stories.tsx
  • code/core/src/manager/components/preview/FramesRenderer.tsx
  • code/core/src/viewport/components/Tool.tsx
  • code/core/src/components/components/ActionList/ActionList.stories.tsx
  • code/core/src/viewport/types.ts
  • code/core/src/viewport/manager.tsx
  • code/core/src/components/components/ActionList/ActionList.tsx
  • code/core/src/viewport/useViewport.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Run ESLint checks using yarn lint:js:cmd <file> or the full command cross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directives to fix linting errors before committing

Files:

  • code/core/src/manager/components/preview/SizeInput.tsx
  • code/core/src/components/components/Select/Select.tsx
  • code/core/src/manager/components/preview/Iframe.tsx
  • code/core/src/components/components/Select/SelectOption.tsx
  • code/core/src/manager/components/preview/utils/components.ts
  • code/core/src/manager/components/preview/Viewport.tsx
  • code/core/src/components/components/Select/helpers.tsx
  • code/core/src/viewport/viewportIcons.tsx
  • code/core/src/manager/components/preview/Viewport.stories.tsx
  • code/core/src/manager/components/preview/FramesRenderer.tsx
  • code/core/src/viewport/components/Tool.tsx
  • code/core/src/components/components/ActionList/ActionList.stories.tsx
  • code/core/src/viewport/types.ts
  • code/core/src/viewport/manager.tsx
  • code/core/src/components/components/ActionList/ActionList.tsx
  • code/core/src/viewport/useViewport.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Enable TypeScript strict mode across all packages

Files:

  • code/core/src/manager/components/preview/SizeInput.tsx
  • code/core/src/components/components/Select/Select.tsx
  • code/core/src/manager/components/preview/Iframe.tsx
  • code/core/src/components/components/Select/SelectOption.tsx
  • code/core/src/manager/components/preview/utils/components.ts
  • code/core/src/manager/components/preview/Viewport.tsx
  • code/core/src/components/components/Select/helpers.tsx
  • code/core/src/viewport/viewportIcons.tsx
  • code/core/src/manager/components/preview/Viewport.stories.tsx
  • code/core/src/manager/components/preview/FramesRenderer.tsx
  • code/core/src/viewport/components/Tool.tsx
  • code/core/src/components/components/ActionList/ActionList.stories.tsx
  • code/core/src/viewport/types.ts
  • code/core/src/viewport/manager.tsx
  • code/core/src/components/components/ActionList/ActionList.tsx
  • code/core/src/viewport/useViewport.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size

Files:

  • code/core/src/manager/components/preview/SizeInput.tsx
  • code/core/src/components/components/Select/Select.tsx
  • code/core/src/manager/components/preview/Iframe.tsx
  • code/core/src/components/components/Select/SelectOption.tsx
  • code/core/src/manager/components/preview/utils/components.ts
  • code/core/src/manager/components/preview/Viewport.tsx
  • code/core/src/components/components/Select/helpers.tsx
  • code/core/src/viewport/viewportIcons.tsx
  • code/core/src/manager/components/preview/Viewport.stories.tsx
  • code/core/src/manager/components/preview/FramesRenderer.tsx
  • code/core/src/viewport/components/Tool.tsx
  • code/core/src/components/components/ActionList/ActionList.stories.tsx
  • code/core/src/viewport/types.ts
  • code/core/src/viewport/manager.tsx
  • code/core/src/components/components/ActionList/ActionList.tsx
  • code/core/src/viewport/useViewport.ts
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use logger from storybook/internal/node-logger for server-side logging in Node.js code

Files:

  • code/core/src/manager/components/preview/SizeInput.tsx
  • code/core/src/components/components/Select/Select.tsx
  • code/core/src/manager/components/preview/Iframe.tsx
  • code/core/src/components/components/Select/SelectOption.tsx
  • code/core/src/manager/components/preview/utils/components.ts
  • code/core/src/manager/components/preview/Viewport.tsx
  • code/core/src/components/components/Select/helpers.tsx
  • code/core/src/viewport/viewportIcons.tsx
  • code/core/src/manager/components/preview/Viewport.stories.tsx
  • code/core/src/manager/components/preview/FramesRenderer.tsx
  • code/core/src/viewport/components/Tool.tsx
  • code/core/src/components/components/ActionList/ActionList.stories.tsx
  • code/core/src/viewport/types.ts
  • code/core/src/viewport/manager.tsx
  • code/core/src/components/components/ActionList/ActionList.tsx
  • code/core/src/viewport/useViewport.ts
🧠 Learnings (6)
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.

Applied to files:

  • code/core/src/manager/components/preview/SizeInput.tsx
  • code/core/src/components/components/Select/SelectOption.tsx
  • code/core/src/components/components/ActionList/ActionList.stories.tsx
  • code/core/src/components/components/ActionList/ActionList.tsx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.

Applied to files:

  • code/core/src/manager/components/preview/SizeInput.tsx
  • code/core/src/components/components/Select/SelectOption.tsx
  • code/core/src/manager/components/preview/FramesRenderer.tsx
  • code/core/src/viewport/components/Tool.tsx
  • code/core/src/components/components/ActionList/ActionList.tsx
  • code/core/src/viewport/useViewport.ts
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.

Applied to files:

  • code/core/src/manager/components/preview/SizeInput.tsx
  • code/core/src/manager/components/preview/Viewport.stories.tsx
  • code/core/src/viewport/components/Tool.tsx
  • code/core/src/components/components/ActionList/ActionList.tsx
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.

Applied to files:

  • code/core/src/manager/components/preview/Viewport.stories.tsx
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.

Applied to files:

  • code/core/src/manager/components/preview/Viewport.stories.tsx
  • code/core/src/manager/components/preview/FramesRenderer.tsx
  • code/core/src/viewport/components/Tool.tsx
  • code/core/src/viewport/types.ts
  • code/core/src/viewport/useViewport.ts
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: The useGlobals hook from storybook/manager-api returns a tuple where the third element (storyGlobals) is typed as Globals, not Globals | undefined. This means TypeScript guarantees it's always defined, making the `in` operator safe to use without additional null checks.

Applied to files:

  • code/core/src/manager/components/preview/FramesRenderer.tsx
  • code/core/src/viewport/useViewport.ts
🧬 Code graph analysis (8)
code/core/src/manager/components/preview/SizeInput.tsx (2)
code/core/src/components/index.ts (1)
  • Form (62-62)
code/core/src/preview-api/index.ts (4)
  • useRef (13-13)
  • useState (14-14)
  • useEffect (8-8)
  • useCallback (6-6)
code/core/src/components/components/Select/SelectOption.tsx (1)
code/core/src/components/components/ActionList/ActionList.tsx (1)
  • ActionList (205-226)
code/core/src/manager/components/preview/Viewport.tsx (5)
code/core/src/viewport/useViewport.ts (2)
  • VIEWPORT_MIN_WIDTH (20-20)
  • useViewport (128-235)
code/core/src/viewport/types.ts (1)
  • Viewport (1-5)
code/core/src/manager/components/preview/SizeInput.tsx (1)
  • SizeInput (41-94)
code/core/src/components/components/ActionList/ActionList.tsx (1)
  • ActionList (205-226)
code/core/src/manager/components/preview/Iframe.tsx (1)
  • IFrame (29-47)
code/core/src/viewport/viewportIcons.tsx (1)
code/core/src/viewport/types.ts (1)
  • Viewport (1-5)
code/core/src/manager/components/preview/Viewport.stories.tsx (3)
code/core/src/viewport/types.ts (2)
  • ViewportMap (14-14)
  • Viewport (1-5)
code/core/src/manager/components/preview/Viewport.tsx (1)
  • Viewport (142-262)
code/core/src/manager-api/root.tsx (1)
  • ManagerContext (79-79)
code/core/src/components/components/ActionList/ActionList.stories.tsx (3)
code/core/src/components/components/ActionList/ActionList.tsx (1)
  • ActionList (205-226)
code/core/src/components/index.ts (1)
  • ActionList (44-44)
code/core/src/manager/container/Menu.tsx (1)
  • Shortcut (56-62)
code/core/src/viewport/manager.tsx (1)
code/core/src/viewport/components/Tool.tsx (1)
  • ViewportTool (22-59)
code/core/src/viewport/useViewport.ts (3)
code/core/src/viewport/types.ts (5)
  • ViewportMap (14-14)
  • GlobalState (16-29)
  • ViewportType (7-7)
  • ViewportParameters (33-54)
  • GlobalStateUpdate (31-31)
code/core/src/csf/story.ts (1)
  • Globals (164-166)
code/core/src/manager-api/root.tsx (1)
  • useStorybookApi (296-299)
⏰ 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
🔇 Additional comments (21)
code/core/src/manager/components/preview/Iframe.tsx (1)

14-14: LGTM! CSS simplification.

The border value change from '0 none' to 'none' is a good simplification that follows CSS best practices.

code/core/src/manager/components/preview/utils/components.ts (1)

30-31: LGTM! Enables flexible canvas sizing.

Changing from fixed width/height to minWidth/minHeight allows the canvas to grow beyond 100% when needed, which is appropriate for supporting custom viewport sizes and the new Viewport-based rendering system.

code/core/src/components/components/Select/helpers.tsx (1)

17-17: LGTM! Backward-compatible API extension.

The addition of the optional right property to the Option interface is a clean, backward-compatible extension that enables right-aligned content in select options.

code/core/src/components/components/Select/Select.tsx (1)

562-562: LGTM! Correctly propagates the right prop.

The prop is properly passed from the option to the SelectOption component, completing the integration of the right-aligned content feature.

code/core/src/viewport/types.ts (2)

4-7: LGTM! Good type extraction with expanded viewport coverage.

Extracting ViewportType as a named, exported type improves reusability and maintainability. The addition of 'watch' as a viewport type provides better support for smartwatch and wearable device viewports.


19-20: LGTM! Improved documentation clarity.

The documentation update clarifies that custom viewport values can include units (e.g., '100vw', '100pct'), which is helpful for developers using the custom size feature.

code/core/src/viewport/viewportIcons.tsx (1)

1-13: LGTM!

Clean, well-typed mapping of viewport types to their corresponding icons. The use of NonNullable<Viewport['type']> ensures type safety by requiring all valid type values to have an icon mapping.

code/core/src/components/components/Select/SelectOption.tsx (1)

64-87: LGTM!

The migration to ActionList-based composition is well-structured. The new right prop provides flexibility for right-aligned content, and the rendering logic properly handles both custom children and the default composition pattern. Accessibility attributes are correctly preserved.

code/core/src/manager/components/preview/SizeInput.tsx (1)

88-93: LGTM!

The accessible label using sb-sr-only properly labels the input for screen readers while keeping the UI clean. The prefix visual indicator and proper ref forwarding are well implemented.

code/core/src/components/components/ActionList/ActionList.tsx (1)

24-39: LGTM on the CSS variable approach!

The use of CSS custom properties for active/selected colors enables clean inheritance and consistent theming across nested elements. The aria-disabled and aria-selected attribute selectors properly align visual states with accessibility semantics.

code/core/src/components/components/ActionList/ActionList.stories.tsx (1)

63-109: LGTM!

Good coverage of the new ActionList features. The stories effectively demonstrate:

  • ActionList.Icon wrapper usage
  • aria-selected and aria-disabled states
  • Multi-line text composition with title and description
  • Active state with icons and wrapped content
code/core/src/viewport/components/Tool.tsx (1)

31-37: Dimension display assumes px unit.

The display strips px from width/height, but viewports could potentially use other units (%, em, etc.). This is likely fine for the predefined viewport options, but worth noting if custom viewports with non-px units are supported.

code/core/src/manager/components/preview/Viewport.tsx (2)

199-200: Unnecessary key prop on ViewportWrapper.

Using key={id} on ViewportWrapper will cause the entire component subtree to remount when id changes, which may not be intended. If the goal is to reset state when the viewport changes, this is fine, but otherwise consider removing it.


241-259: LGTM on the drag handle implementation!

The drag handles are well-positioned with appropriate cursor styles. The data-side attribute pattern cleanly differentiates between horizontal, vertical, and corner resize handles. The iframe pointer-events toggle during drag prevents interaction issues.

code/core/src/manager/components/preview/FramesRenderer.tsx (1)

107-109: Clean refactor to the new Viewport component.

The transition from directly rendering IFrame to using the new Viewport component is well-structured. The props align correctly with the Viewport component's expected interface (active, id, src, scale), and the key prop ensures proper reconciliation.

code/core/src/viewport/manager.tsx (1)

8-14: Well-aligned with the hook-based architecture.

The removal of the api parameter from the registration callback and prop passing to ViewportTool is correct. The ViewportTool component now internally uses the useViewport hook, which in turn uses useStorybookApi() to access the API, making the explicit prop passing unnecessary.

code/core/src/manager/components/preview/Viewport.stories.tsx (1)

102-110: Verify intentionality of empty renders.

The Short and Narrow stories render empty fragments (<></>), which means they won't render the actual Viewport component being tested. If the intent is to test only the viewport sizing behavior without content, consider adding a brief comment explaining this. Otherwise, these stories may not be testing the component as expected.

Also applies to: 112-120

code/core/src/viewport/useViewport.ts (4)

38-41: LGTM - Clean normalization utility.

The function correctly handles both string and object inputs with appropriate optional chaining and nullish coalescing.


98-112: Verify fallback behavior for invalid viewport option.

On line 99, if value doesn't exist in options, the code falls back to options[keys[0]]. This silent fallback might mask configuration errors where a story references a viewport that doesn't exist. Consider whether a warning should be logged in this case.


200-234: Well-structured memoized API.

The hook exposes a clean API with proper memoization. The dependency array is comprehensive, and using lastSelectedOption.current directly (without adding it to deps) is correct since ref changes don't trigger re-renders.


179-198: No changes required. The shortcut registration pattern is correct.

The setAddonShortcut implementation uses a composite key (${addon}-${actionName}) to store shortcuts, ensuring that calling it with the same addon and actionName idempotently replaces the previous entry in the addonsShortcuts object. Each of the three shortcuts in the effect uses a distinct actionName ('next', 'previous', 'reset'), creating unique keys that do not conflict. When the effect re-runs due to dependency changes, the new action closures (which capture the current options and isRotated values) replace the old ones in the shared storage. This is the correct behavior—handlers must use the current state, not stale closures. No cleanup function is needed since the storage is keyed and replaced, not additive.

Likely an incorrect or invalid review comment.

Comment thread code/core/src/manager/components/preview/Viewport.tsx
Comment thread code/core/src/viewport/useViewport.ts
Comment thread code/core/src/viewport/useViewport.ts
@storybook-app-bot
Copy link
Copy Markdown

storybook-app-bot Bot commented Jan 7, 2026

Package Benchmarks

Commit: ccbcc9a, ran on 9 January 2026 at 13:13:46 UTC

The following packages have significant changes to their size or dependencies:

storybook

Before After Difference
Dependency count 49 49 0
Self size 20.19 MB 20.20 MB 🚨 +12 KB 🚨
Dependency size 16.52 MB 16.52 MB 0 B
Bundle Size Analyzer Link Link

@storybook/nextjs

Before After Difference
Dependency count 534 538 🚨 +4 🚨
Self size 646 KB 646 KB 0 B
Dependency size 59.16 MB 59.19 MB 🚨 +27 KB 🚨
Bundle Size Analyzer Link Link

@storybook/react-webpack5

Before After Difference
Dependency count 274 278 🚨 +4 🚨
Self size 24 KB 24 KB 🚨 +12 B 🚨
Dependency size 44.09 MB 44.12 MB 🚨 +28 KB 🚨
Bundle Size Analyzer Link Link

@storybook/cli

Before After Difference
Dependency count 183 183 0
Self size 775 KB 775 KB 🎉 -236 B 🎉
Dependency size 67.26 MB 67.28 MB 🚨 +12 KB 🚨
Bundle Size Analyzer Link Link

@storybook/codemod

Before After Difference
Dependency count 176 176 0
Self size 30 KB 30 KB 0 B
Dependency size 65.84 MB 65.85 MB 🚨 +12 KB 🚨
Bundle Size Analyzer Link Link

create-storybook

Before After Difference
Dependency count 50 50 0
Self size 999 KB 999 KB 🎉 -42 B 🎉
Dependency size 36.71 MB 36.72 MB 🚨 +12 KB 🚨
Bundle Size Analyzer node node

@storybook/preset-create-react-app

Before After Difference
Dependency count 64 68 🚨 +4 🚨
Self size 32 KB 32 KB 🚨 +24 B 🚨
Dependency size 5.95 MB 5.98 MB 🚨 +28 KB 🚨
Bundle Size Analyzer Link Link

@storybook/preset-react-webpack

Before After Difference
Dependency count 166 170 🚨 +4 🚨
Self size 18 KB 18 KB 🚨 +52 B 🚨
Dependency size 31.23 MB 31.26 MB 🚨 +28 KB 🚨
Bundle Size Analyzer Link Link

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
code/core/src/viewport/components/Tool.tsx (1)

30-30: Provide a fallback for undefined viewport type.

The non-null assertion on value.type! could result in no icon being rendered if a viewport option doesn't specify a type. Consider using a fallback to ensure a consistent icon is always displayed.

♻️ Suggested fix
-        icon: iconsMap[value.type!],
+        icon: iconsMap[value.type ?? 'other'],
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f2bd41f and 920de71.

📒 Files selected for processing (1)
  • code/core/src/viewport/components/Tool.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Format code using Prettier with yarn prettier --write <file>

Files:

  • code/core/src/viewport/components/Tool.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Run ESLint checks using yarn lint:js:cmd <file> or the full command cross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directives to fix linting errors before committing

Files:

  • code/core/src/viewport/components/Tool.tsx
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Enable TypeScript strict mode across all packages

Files:

  • code/core/src/viewport/components/Tool.tsx
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size

Files:

  • code/core/src/viewport/components/Tool.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use logger from storybook/internal/node-logger for server-side logging in Node.js code

Files:

  • code/core/src/viewport/components/Tool.tsx
🧠 Learnings (3)
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.

Applied to files:

  • code/core/src/viewport/components/Tool.tsx
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.

Applied to files:

  • code/core/src/viewport/components/Tool.tsx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.

Applied to files:

  • code/core/src/viewport/components/Tool.tsx
🧬 Code graph analysis (1)
code/core/src/viewport/components/Tool.tsx (3)
code/core/src/viewport/useViewport.ts (1)
  • useViewport (128-235)
code/core/src/viewport/viewportIcons.tsx (1)
  • iconsMap (7-13)
code/core/src/components/components/Select/Select.tsx (1)
  • Select (194-607)
⏰ 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: normal
  • GitHub Check: nx
  • GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (5)
code/core/src/viewport/components/Tool.tsx (5)

1-10: LGTM!

Imports are clean and all appear to be used in the component.


12-20: LGTM!

The Dimensions styled component correctly leverages theme tokens for typography and color consistency.


31-37: Verify handling of non-pixel units in dimensions display.

The code uses .replace('px', '') to strip units, but based on the useViewport hook's resize function which handles percentage values (converting % to pct), viewport styles might contain non-pixel values. If percentages or other units are possible, this replacement would leave them unchanged (e.g., 100% would display as 100% rather than just 100).

If this is intentional, the current behavior is fine. Otherwise, consider a more robust unit stripping approach:

-            <span>{value.styles.width.replace('px', '')}</span>
+            <span>{value.styles.width.replace(/px|%$/, '')}</span>

42-58: LGTM on the overall Select implementation.

The refactored hook-based approach cleanly separates state management from rendering. The conditional rendering for the default viewport state (isDefault ? null : name) and the locked state handling via disabled and dynamic ariaLabel/tooltip are well implemented.


49-49: No changes needed. The ariaDescription prop is properly handled through the component hierarchy: it passes via ...props spread from Select to the underlying StyledButton (Button component), which explicitly handles it through the useAriaDescription() hook. This hook correctly converts the description string into an aria-describedby attribute paired with a hidden element, following the standard ARIA pattern. The prop is functional and accessible.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In @code/core/src/viewport/components/Tool.tsx:
- Line 30: The code uses an unsafe non-null assertion on value.type when looking
up iconsMap (icon: iconsMap[value.type!]); update the Tool component to handle a
missing type by either validating value.type before use or falling back to a
default ViewportType (e.g., a DEFAULT_VIEWPORT_TYPE or 'default') and use that
for the iconsMap lookup; ensure references to value.type, iconsMap, and the Tool
rendering logic are updated so no non-null assertion is used and a safe icon is
always selected.
- Around line 31-37: The dimension display in Tool.tsx currently uses
value.styles.width.replace('px','') and value.styles.height.replace('px','')
which fails for non-pixel units (e.g., '100%', '2em'); update the rendering to
strip any trailing CSS unit instead (use a unit-stripping helper such as a regex
that removes trailing letters/percent or use parseFloat) when rendering inside
the Dimensions component so responsiveViewport and other unit types show the
numeric value correctly; update references to value.styles.width and
value.styles.height and add/inline the stripUnit helper (or use parseFloat)
where those spans are produced.
🧹 Nitpick comments (1)
code/core/src/viewport/components/Tool.tsx (1)

12-20: Consider using explicit units for the gap property.

The gap: 2 on line 14 will likely be interpreted as pixels, but explicitly specifying gap: 2px or using a theme spacing value would improve clarity and maintainability.

♻️ Suggested improvement
 const Dimensions = styled.div(({ theme }) => ({
   display: 'flex',
-  gap: 2,
+  gap: '2px',
   marginLeft: 20,
   fontFamily: theme.typography.fonts.mono,
   fontSize: theme.typography.size.s1 - 1,
   fontWeight: theme.typography.weight.regular,
   color: theme.textMutedColor,
 }));
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 602ddd7 and 94c22bf.

📒 Files selected for processing (1)
  • code/core/src/viewport/components/Tool.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Format code using Prettier with yarn prettier --write <file>

Files:

  • code/core/src/viewport/components/Tool.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Run ESLint checks using yarn lint:js:cmd <file> or the full command cross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directives to fix linting errors before committing

Files:

  • code/core/src/viewport/components/Tool.tsx
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Enable TypeScript strict mode across all packages

Files:

  • code/core/src/viewport/components/Tool.tsx
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size

Files:

  • code/core/src/viewport/components/Tool.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use logger from storybook/internal/node-logger for server-side logging in Node.js code

Files:

  • code/core/src/viewport/components/Tool.tsx
🧠 Learnings (3)
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.

Applied to files:

  • code/core/src/viewport/components/Tool.tsx
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.

Applied to files:

  • code/core/src/viewport/components/Tool.tsx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.

Applied to files:

  • code/core/src/viewport/components/Tool.tsx
⏰ 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: normal
  • GitHub Check: Core Unit Tests, windows-latest
  • GitHub Check: nx
🔇 Additional comments (2)
code/core/src/viewport/components/Tool.tsx (2)

22-58: Well-structured refactor with clean hook-based architecture.

The migration to a hook-based approach using useViewport() significantly simplifies the component:

  • Clear separation of concerns with state management in the hook
  • Appropriate use of useMemo for options computation
  • Excellent accessibility support with contextual labels and descriptions for locked/unlocked states
  • Clean conditional rendering based on isDefault

The parameterless component signature makes it easier to use and test.


53-53: The type assertion selected as string is necessary and appropriate here. The Select component's onSelect callback has a signature of (option: Value) => void where Value is string | number | null | boolean | undefined. However, the select function from useViewport (line 217 of useViewport.ts) expects specifically (value: string). Since the options array is created with string keys only (Object.entries(viewportMap).map(([k, value]) => ({ value: k, ... }))), the assertion is justified—all option values passed to the callback will be strings, matching the select function's expectation.

Object.entries(viewportMap).map(([k, value]) => ({
value: k,
title: value.name,
icon: iconsMap[value.type!],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, let's find the Tool.tsx file and understand its context
head -50 code/core/src/viewport/components/Tool.tsx

Repository: storybookjs/storybook

Length of output: 186


🏁 Script executed:

# Find the useViewport hook definition
rg -nP --type=ts 'useViewport|export.*useViewport' -A 10 code/core/src/viewport

Repository: storybookjs/storybook

Length of output: 2829


🏁 Script executed:

# Search for viewport type definitions across the codebase
rg -nP --type=ts 'interface.*Viewport|type.*Viewport\s*=|viewport.*type' code/core/src/viewport --max-count=20

Repository: storybookjs/storybook

Length of output: 405


🏁 Script executed:

# Look for where viewport values are created or validated
rg -nP --type=ts 'viewportMap|viewports.*=|viewport.*:' code/core/src/viewport -B2 -A2

Repository: storybookjs/storybook

Length of output: 2019


🏁 Script executed:

# Get the full Viewport interface definition
cat -n code/core/src/viewport/types.ts | head -65

Repository: storybookjs/storybook

Length of output: 2248


🏁 Script executed:

# Get the ViewportMap type definition
rg -nP --type=ts 'ViewportMap|type ViewportMap' code/core/src/viewport -B2 -A5

Repository: storybookjs/storybook

Length of output: 3210


🏁 Script executed:

# Check the parseGlobals function to see how value.type is set
rg -nP --type=ts 'parseGlobals' code/core/src/viewport -B2 -A30

Repository: storybookjs/storybook

Length of output: 5242


🏁 Script executed:

# Check if there's any validation or default type assignment for custom viewports
rg -nP --type=ts 'type.*=' code/core/src/viewport/useViewport.ts -B3 -A3 | grep -A 10 -B 10 'type.*||'

Repository: storybookjs/storybook

Length of output: 47


🏁 Script executed:

# Check the iconsMap to understand what happens with undefined type
rg -nP --type=ts 'iconsMap' code/core/src/viewport -B2 -A10

Repository: storybookjs/storybook

Length of output: 2621


🏁 Script executed:

# Check the parseGlobals return value more carefully - does it ensure type is defined?
sed -n '43,120p' code/core/src/viewport/useViewport.ts

Repository: storybookjs/storybook

Length of output: 2513


🏁 Script executed:

# Check for any viewport creation or merging logic that might add defaults
rg -nP --type=ts 'type.*:.*other|type.*=.*other' code/core/src/viewport -B2 -A2

Repository: storybookjs/storybook

Length of output: 1084


🏁 Script executed:

# Check the useViewport hook return type for the options/viewportMap
sed -n '128,200p' code/core/src/viewport/useViewport.ts

Repository: storybookjs/storybook

Length of output: 2788


🏁 Script executed:

# Check what useViewport returns for the options property
sed -n '198,240p' code/core/src/viewport/useViewport.ts

Repository: storybookjs/storybook

Length of output: 771


🏁 Script executed:

# Check MINIMAL_VIEWPORTS to see if all viewports have type defined
rg -nP --type=ts 'MINIMAL_VIEWPORTS' code/core/src/viewport -B2 -A20

Repository: storybookjs/storybook

Length of output: 4879


Remove unsafe non-null assertion on value.type.

The value.type! assertion is unsafe because the Viewport interface defines type as optional (type?: ViewportType). User-provided viewport configurations can omit this property entirely. Without validation, accessing iconsMap[undefined] would fail at runtime. Either validate that type exists before rendering, or provide a default type for viewports missing this property.

🤖 Prompt for AI Agents
In @code/core/src/viewport/components/Tool.tsx at line 30, The code uses an
unsafe non-null assertion on value.type when looking up iconsMap (icon:
iconsMap[value.type!]); update the Tool component to handle a missing type by
either validating value.type before use or falling back to a default
ViewportType (e.g., a DEFAULT_VIEWPORT_TYPE or 'default') and use that for the
iconsMap lookup; ensure references to value.type, iconsMap, and the Tool
rendering logic are updated so no non-null assertion is used and a safe icon is
always selected.

Comment thread code/core/src/viewport/components/Tool.tsx
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @code/core/src/components/components/ActionList/ActionList.tsx:
- Around line 176-210: ActionListText declares a muted?: boolean prop but never
uses it; either remove the unused prop from the styled component signature or
wire it into the styling so consumers can toggle muted appearance. To fix,
update the styled definition for ActionListText to accept ({ theme, muted }) and
conditionally apply the muted color (e.g., set color: theme.textMutedColor or
adjust '& small' when muted is true), or simply drop the muted?: boolean type
from ActionListText and remove any external usages that expect it.
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e83efbf and 72b4543.

📒 Files selected for processing (2)
  • code/core/src/components/components/ActionList/ActionList.stories.tsx
  • code/core/src/components/components/ActionList/ActionList.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • code/core/src/components/components/ActionList/ActionList.stories.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Format code using Prettier with yarn prettier --write <file>

Files:

  • code/core/src/components/components/ActionList/ActionList.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Run ESLint checks using yarn lint:js:cmd <file> or the full command cross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directives to fix linting errors before committing

Files:

  • code/core/src/components/components/ActionList/ActionList.tsx
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Enable TypeScript strict mode across all packages

Files:

  • code/core/src/components/components/ActionList/ActionList.tsx
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size

Files:

  • code/core/src/components/components/ActionList/ActionList.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use logger from storybook/internal/node-logger for server-side logging in Node.js code

Files:

  • code/core/src/components/components/ActionList/ActionList.tsx
🧠 Learnings (3)
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.

Applied to files:

  • code/core/src/components/components/ActionList/ActionList.tsx
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.

Applied to files:

  • code/core/src/components/components/ActionList/ActionList.tsx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.

Applied to files:

  • code/core/src/components/components/ActionList/ActionList.tsx
🧬 Code graph analysis (1)
code/core/src/components/components/ActionList/ActionList.tsx (1)
code/core/src/components/index.ts (1)
  • Button (56-56)
⏰ 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: normal
  • GitHub Check: Core Unit Tests, windows-latest
  • GitHub Check: nx
🔇 Additional comments (5)
code/core/src/components/components/ActionList/ActionList.tsx (5)

20-60: LGTM! Well-structured ARIA state styling and interactive feedback.

The CSS variable approach for active/muted colors is clean and maintainable. The ARIA state styles (aria-disabled, aria-selected) follow good accessibility patterns, and the role="option" styling provides appropriate interactive feedback with hover and focus-visible states.


62-67: Good refinement of transition properties.

Explicitly listing transitionProperty instead of using a catch-all transition is a performance improvement, as it limits animations to only the necessary properties.


152-154: Good accessibility consideration.

The minHeight: 32 ensures a minimum touch/click target size, which aligns with accessibility guidelines. The matching value with ActionListItem (line 20) maintains visual consistency.


212-220: Good use of CSS variable for icon color.

The ActionListIcon correctly references --listbox-item-muted-color which is defined in ActionListItem, ensuring consistent theming across the component hierarchy.


118-125: Size-based gap logic is correct and appropriate.

The Button component only supports size?: 'small' | 'medium', so the ternary gap: size === 'small' ? 6 : 8 correctly maps both defined sizes with appropriate spacing values.

Comment thread code/core/src/components/components/ActionList/ActionList.tsx Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @code/core/src/manager/components/layout/useDragging.ts:
- Line 37: The selector in useDragging.ts now queries
'#storybook-preview-iframe' but the app still renders
'#storybook-preview-wrapper' and skip links in Tree.tsx, Sidebar.tsx, and
Heading.stories.tsx point to the old id; either revert useDragging.ts to query
'#storybook-preview-wrapper' (restore the previous selector in the const
previewIframe declaration) until the viewport redesign is finished, or complete
migration by updating Wrappers.tsx to stop rendering the wrapper and instead
render the iframe with id='storybook-preview-iframe' and then update all skip
link targets in Tree.tsx, Sidebar.tsx, and Heading.stories.tsx to use
'#storybook-preview-iframe'.
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 72b4543 and e84ab86.

📒 Files selected for processing (4)
  • code/addons/a11y/src/components/VisionSimulator.tsx
  • code/core/src/components/components/ActionList/ActionList.tsx
  • code/core/src/manager/components/layout/useDragging.ts
  • code/core/src/manager/components/preview/Viewport.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • code/core/src/manager/components/preview/Viewport.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Format code using Prettier with yarn prettier --write <file>

Files:

  • code/core/src/manager/components/layout/useDragging.ts
  • code/addons/a11y/src/components/VisionSimulator.tsx
  • code/core/src/components/components/ActionList/ActionList.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Run ESLint checks using yarn lint:js:cmd <file> or the full command cross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directives to fix linting errors before committing

Files:

  • code/core/src/manager/components/layout/useDragging.ts
  • code/addons/a11y/src/components/VisionSimulator.tsx
  • code/core/src/components/components/ActionList/ActionList.tsx
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Enable TypeScript strict mode across all packages

Files:

  • code/core/src/manager/components/layout/useDragging.ts
  • code/addons/a11y/src/components/VisionSimulator.tsx
  • code/core/src/components/components/ActionList/ActionList.tsx
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size

Files:

  • code/core/src/manager/components/layout/useDragging.ts
  • code/addons/a11y/src/components/VisionSimulator.tsx
  • code/core/src/components/components/ActionList/ActionList.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use logger from storybook/internal/node-logger for server-side logging in Node.js code

Files:

  • code/core/src/manager/components/layout/useDragging.ts
  • code/addons/a11y/src/components/VisionSimulator.tsx
  • code/core/src/components/components/ActionList/ActionList.tsx
🧠 Learnings (4)
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.

Applied to files:

  • code/core/src/manager/components/layout/useDragging.ts
  • code/core/src/components/components/ActionList/ActionList.tsx
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.

Applied to files:

  • code/core/src/manager/components/layout/useDragging.ts
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.

Applied to files:

  • code/addons/a11y/src/components/VisionSimulator.tsx
  • code/core/src/components/components/ActionList/ActionList.tsx
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.

Applied to files:

  • code/core/src/components/components/ActionList/ActionList.tsx
🧬 Code graph analysis (2)
code/core/src/manager/components/layout/useDragging.ts (1)
code/e2e-tests/util.ts (1)
  • previewIframe (163-165)
code/core/src/components/components/ActionList/ActionList.tsx (1)
code/core/src/components/index.ts (1)
  • Button (56-56)
⏰ 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: normal
  • GitHub Check: nx
  • GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (8)
code/addons/a11y/src/components/VisionSimulator.tsx (1)

21-36: LGTM! Good icon sizing refinements.

The addition of flexShrink: 0 is a best practice for fixed-size icons in flex containers, preventing unwanted shrinking. The size adjustments from 1rem to 14 pixels align with the commit message "Fixed icons for the vision simulator" and appear intentional for the viewport redesign.

code/core/src/components/components/ActionList/ActionList.tsx (7)

3-3: LGTM!

The darken and transparentize imports from polished are correctly added and used for color calculations in the component styling.


20-40: Good accessibility improvements.

The addition of aria-disabled and aria-selected attribute selectors properly handles accessibility states. The CSS custom properties (--listbox-item-active-color, --listbox-item-muted-color) provide a clean way to manage color inheritance for nested elements.


47-60: Well-implemented listbox option styling.

The role="option" selector appropriately styles listbox items with proper hover feedback and focus-visible outline for keyboard navigation. The outlineOffset: -2 is consistent with other focus styles in this component.


62-67: LGTM!

The explicit transitionProperty list is more performant than animating all properties, and the CSS variable for duration provides flexibility for consumers to customize animation timing.


118-125: LGTM!

The size-aware gap calculation provides appropriate spacing for different button sizes.


152-154: LGTM!

The height: 'auto' with minHeight: 32 allows flexible content while maintaining consistent minimum heights across list items.


176-210: Well-structured text layout.

The column layout with centered content supports multi-line items (title + description). The distinction between whiteSpace: 'normal' for general children and whiteSpace: 'nowrap' for spans with ellipsis overflow is intentional and correctly handles different content types. The extended selectors for [role="option"] parent context properly support the new listbox functionality.

Comment thread code/core/src/manager/components/layout/useDragging.ts
@ghengeveld ghengeveld mentioned this pull request Jan 8, 2026
8 tasks
Copy link
Copy Markdown
Member

@Sidnioulz Sidnioulz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! I love that the select works well with your new ActionList UI!

There are some UX edge cases around viewport customisation, and a bug with viewports & the docs viewMode. LGTM otherwise.

Comment thread code/core/src/components/components/Select/SelectOption.tsx
Comment thread code/core/src/manager/components/preview/SizeInput.tsx Outdated
Comment thread code/core/src/manager/components/preview/Viewport.tsx Outdated
Comment thread code/core/src/manager/components/preview/Viewport.tsx
Comment thread code/core/src/manager/components/preview/Viewport.tsx
Comment thread code/core/src/manager/components/preview/Viewport.tsx
Comment thread code/core/src/components/components/Select/helpers.tsx Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @code/core/src/manager/components/preview/Viewport.tsx:
- Around line 217-235: The drag handlers onDrag and onEnd currently use non-null
assertions for targetRef.current, dragRefX.current, dragRefY.current and
dragStart.current; add defensive null checks at the start of onDrag and onEnd to
bail out if any required ref is missing (e.g., if !dragStart.current ||
!targetRef.current return), and when reading clientWidth/clientHeight or setting
dataset/value use optional chaining or guarded access and provide sensible
fallbacks when calling resize; update references to targetRef, dragRefX,
dragRefY, dragStart, onDrag, onEnd, setDragging, and resize accordingly so no
runtime errors occur if refs are null during unmount/re-render.
🧹 Nitpick comments (1)
code/core/src/viewport/useViewport.ts (1)

205-224: Shortcut registration is functional but re-runs frequently.

The shortcuts are re-registered whenever isRotated changes. While functional, this could be optimized by moving the rotation state handling inside the action callbacks rather than depending on it in the effect.

⚡ Optional optimization to reduce effect re-runs

Consider restructuring to avoid re-registering shortcuts on every rotation:

  useEffect(() => {
    api.setAddonShortcut(ADDON_ID, {
      label: 'Next viewport',
      defaultShortcut: ['alt', 'V'],
      actionName: 'next',
-     action: () => update({ value: cycle(options, lastSelectedOption.current), isRotated }),
+     action: () => {
+       const { isRotated: currentRotation } = parseGlobals(globals, storyGlobals, userGlobals, options, lastSelectedOption.current, false, viewMode);
+       update({ value: cycle(options, lastSelectedOption.current), isRotated: currentRotation });
+     },
    });
    // Similar for other shortcuts...
- }, [api, update, options, isRotated]);
+ }, [api, update, options]);

Note: This would require additional parameters to be available in the action callback scope.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e84ab86 and c115fef.

📒 Files selected for processing (4)
  • code/core/src/manager/components/preview/SizeInput.tsx
  • code/core/src/manager/components/preview/Viewport.stories.tsx
  • code/core/src/manager/components/preview/Viewport.tsx
  • code/core/src/viewport/useViewport.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • code/core/src/manager/components/preview/Viewport.stories.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Format code using Prettier with yarn prettier --write <file>

Files:

  • code/core/src/viewport/useViewport.ts
  • code/core/src/manager/components/preview/Viewport.tsx
  • code/core/src/manager/components/preview/SizeInput.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Run ESLint checks using yarn lint:js:cmd <file> or the full command cross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directives to fix linting errors before committing

Files:

  • code/core/src/viewport/useViewport.ts
  • code/core/src/manager/components/preview/Viewport.tsx
  • code/core/src/manager/components/preview/SizeInput.tsx
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Enable TypeScript strict mode across all packages

Files:

  • code/core/src/viewport/useViewport.ts
  • code/core/src/manager/components/preview/Viewport.tsx
  • code/core/src/manager/components/preview/SizeInput.tsx
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size

Files:

  • code/core/src/viewport/useViewport.ts
  • code/core/src/manager/components/preview/Viewport.tsx
  • code/core/src/manager/components/preview/SizeInput.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use logger from storybook/internal/node-logger for server-side logging in Node.js code

Files:

  • code/core/src/viewport/useViewport.ts
  • code/core/src/manager/components/preview/Viewport.tsx
  • code/core/src/manager/components/preview/SizeInput.tsx
🧠 Learnings (6)
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.

Applied to files:

  • code/core/src/viewport/useViewport.ts
  • code/core/src/manager/components/preview/Viewport.tsx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.

Applied to files:

  • code/core/src/viewport/useViewport.ts
  • code/core/src/manager/components/preview/SizeInput.tsx
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: The useGlobals hook from storybook/manager-api returns a tuple where the third element (storyGlobals) is typed as Globals, not Globals | undefined. This means TypeScript guarantees it's always defined, making the `in` operator safe to use without additional null checks.

Applied to files:

  • code/core/src/viewport/useViewport.ts
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.

Applied to files:

  • code/core/src/manager/components/preview/SizeInput.tsx
📚 Learning: 2025-10-03T07:55:42.639Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.

Applied to files:

  • code/core/src/manager/components/preview/SizeInput.tsx
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.

Applied to files:

  • code/core/src/manager/components/preview/SizeInput.tsx
🧬 Code graph analysis (1)
code/core/src/viewport/useViewport.ts (2)
code/core/src/viewport/types.ts (5)
  • ViewportMap (14-14)
  • GlobalState (16-29)
  • ViewportType (7-7)
  • ViewportParameters (33-54)
  • GlobalStateUpdate (31-31)
code/core/src/csf/story.ts (1)
  • Globals (164-166)
⏰ 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: normal
  • GitHub Check: nx
  • GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (11)
code/core/src/manager/components/preview/SizeInput.tsx (3)

6-6: Good choice for React 17 compatibility.

Using useId from @react-aria/utils is appropriate given the requirement to maintain React 17 compatibility.


69-89: Keyboard increment/decrement logic looks solid.

The keyboard handling correctly preserves units when incrementing/decrementing values. The regex pattern and fallback to 'px' handles various CSS units appropriately.


91-101: Accessibility implementation is correct.

The screen-reader-only label is properly associated with the input via htmlFor and id, while the visual prefix provides context for sighted users. This follows good accessibility practices.

code/core/src/viewport/useViewport.ts (5)

18-18: Unit pattern is permissive but functional.

The regex pattern [a-z]{0,4} allows 0-4 lowercase letters for units, which covers all standard CSS units (px, em, rem, vh, vw, %, etc.). While it could theoretically accept invalid units, the pattern is simple and functional for the use case.


88-97: Clamping logic correctly handles different units.

The clamping logic only enforces minimum dimensions for pixel values, allowing percentage and other units to pass through unclamped. This is the correct behavior.


170-183: Resize validation correctly enforces minimum dimensions.

The resize callback properly validates that pixel values meet the minimum threshold (40px) while allowing non-pixel units through. The unit normalization (converting '%' to 'pct') maintains consistency with the URL format.


185-203: State synchronization effects are correctly implemented.

The effects properly handle viewport state synchronization without risk of infinite loops. The dependency arrays are complete and the guard conditions prevent unnecessary updates.


226-260: Memoization is correctly implemented.

The return value is properly memoized with a complete dependency array. The inline callbacks correctly capture their dependencies from the memoization scope.

code/core/src/manager/components/preview/Viewport.tsx (3)

259-292: Control buttons and inputs are properly accessible.

The SizeInput components have appropriate labels, and the ActionList.Button components use ariaLabel correctly for icon-only buttons. The data attributes for CSS targeting are a reasonable approach for managing focus styling.


302-310: IFrame implementation is correct.

The IFrame component has appropriate props including accessibility attributes (id, title) and the key prop ensures proper React reconciliation when switching between stories.


318-330: Drag handle value display handles multiple units correctly.

The data-value attributes strip 'px' for cleaner display while preserving other units like '%' or 'vw'. This intentional behavior provides appropriate visual feedback during resize operations.

Comment thread code/core/src/manager/components/preview/Viewport.tsx
…use it could change the selection when opening the Select menu
@ghengeveld ghengeveld requested a review from Sidnioulz January 9, 2026 13:02
@ghengeveld ghengeveld merged commit 17efa74 into next Jan 9, 2026
69 checks passed
@ghengeveld ghengeveld deleted the viewport-viewer branch January 9, 2026 13:24
@github-actions github-actions Bot mentioned this pull request Jan 9, 2026
19 tasks
@ghengeveld ghengeveld added the needs qa Indicates that this needs manual QA during the upcoming minor/major release label Jan 14, 2026
@ndelangen ndelangen removed the needs qa Indicates that this needs manual QA during the upcoming minor/major release label Jan 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants