Core: Zoom tool reimplementation #33375
Conversation
|
View your CI Pipeline Execution ↗ for commit 59eee1c
☁️ Nx Cloud last updated this comment at |
This comment was marked as spam.
This comment was marked as spam.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 534 | 538 | 🚨 +4 🚨 |
| Self size | 646 KB | 646 KB | 0 B |
| Dependency size | 59.16 MB | 59.19 MB | 🚨 +28 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 274 | 278 | 🚨 +4 🚨 |
| Self size | 24 KB | 24 KB | 0 B |
| Dependency size | 44.09 MB | 44.12 MB | 🚨 +28 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-create-react-app
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 64 | 68 | 🚨 +4 🚨 |
| Self size | 32 KB | 32 KB | 🚨 +36 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @code/core/src/manager/components/preview/Viewport.tsx:
- Around line 274-286: frameStyles can produce invalid CSS when scale is zero or
negative (resulting in Infinity% or negative sizes); in the frameStyles useMemo
clamp the incoming scale to a safe minimum (e.g., minScale = 0.01 or the same
lower bound used by the zoom controls) before computing width/height so
parseNumber(width/height) times clampedScale is always finite and positive;
reference the parseNumber function and the frameStyles useMemo and apply the
clamp to the local scale variable derived from the scale prop (or also add
equivalent validation in the manual zoom handler in zoom.tsx to reject/normalize
<=0 values).
🧹 Nitpick comments (1)
code/core/src/manager/components/preview/Viewport.tsx (1)
366-366: Consider consistent unit handling for data-value attributes.The
data-valueattributes currently use.replace('px', '')which only strips pixel units. If dimensions use other units (%, em, rem), they would remain in the displayed value, creating inconsistency with the numeric-only values shown during drag.♻️ Proposed refactor for consistent unit stripping
+const stripUnit = (value: string) => parseNumber(value)?.number.toString() ?? value; + <DragHandle ref={dragRefX} isDefault={isDefault} data-side="right" - data-value={width.replace('px', '')} + data-value={stripUnit(width)} /> <DragHandle ref={dragRefY} isDefault={isDefault} data-side="bottom" - data-value={height.replace('px', '')} + data-value={stripUnit(height)} />Also applies to: 372-372
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code/.storybook/bench/bench.stories.tsxcode/core/src/components/components/ActionList/ActionList.stories.tsxcode/core/src/manager/components/preview/Viewport.tsxcode/core/src/manager/components/preview/tools/share.tsxcode/core/src/manager/components/sidebar/ContextMenu.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- code/core/src/components/components/ActionList/ActionList.stories.tsx
- code/.storybook/bench/bench.stories.tsx
- code/core/src/manager/components/sidebar/ContextMenu.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/preview/tools/share.tsxcode/core/src/manager/components/preview/Viewport.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 commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/manager/components/preview/tools/share.tsxcode/core/src/manager/components/preview/Viewport.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/manager/components/preview/tools/share.tsxcode/core/src/manager/components/preview/Viewport.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 useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/components/preview/tools/share.tsxcode/core/src/manager/components/preview/Viewport.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/manager/components/preview/tools/share.tsxcode/core/src/manager/components/preview/Viewport.tsx
🧠 Learnings (4)
📓 Common learnings
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.
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.
📚 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/tools/share.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/tools/share.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/Viewport.tsx
🧬 Code graph analysis (1)
code/core/src/manager/components/preview/Viewport.tsx (2)
code/core/src/manager/components/preview/NumericInput.tsx (1)
NumericInput(66-152)code/core/src/manager/components/preview/Iframe.tsx (1)
IFrame(29-47)
⏰ 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/core/src/manager/components/preview/tools/share.tsx (1)
15-15: LGTM! Import path updated to use centralized Shortcut component.The import path change is correct and aligns with the refactoring effort to extract Shortcut into a standalone, shared component.
code/core/src/manager/components/preview/Viewport.tsx (7)
1-1: LGTM: Import updates support the refactoring.The addition of
useMemoand the switch fromSizeInputtoNumericInputalign well with the performance optimizations and UI component updates in this PR.Also applies to: 15-15
117-173: LGTM: DragHandle refactor adds helpful dimension feedback.The conditional rendering based on
isDefaultand the addition ofdata-valuedisplay via::afterpseudo-elements provide good visual feedback during resizing. The cursor types and positioning are appropriate for each drag direction.
175-192: LGTM: ScrollEdge provides smooth drag experience.The ScrollEdge components are cleverly positioned outside the frame to serve as scroll anchors, ensuring drag handles remain visible during resize operations. The
pointerEvents: 'none'ensures they don't interfere with user interaction.
194-198: LGTM: Clean component composition.The
SizeInputstyled wrapper provides consistent dimensions for the numeric input controls while keeping the underlyingNumericInputcomponent generic and reusable.
223-272: LGTM: Enhanced drag behavior with scroll-into-view.The drag logic correctly updates dimension feedback via
data-valueattributes and usesscrollIntoViewto keep handles visible during resize. TheonEndhandler properly compensates for the scale factor by dividingclientWidth/clientHeightby scale before callingresize.
296-300: LGTM: Consistent UI pattern with ActionList.Using
ActionList.Actionfor the W/H prefixes aligns with the toolbar's component patterns. Thearia-hiddenattribute is appropriate since theSizeInputcomponents have their own labels for accessibility.Also applies to: 316-320
341-361: LGTM: CSS transform-based scaling implementation.The scaling approach is sound: the
FrameWrapperis sized to the scaled dimensions, the innerdivis inversely sized and then scaled up via CSStransform, and theIFramereceivesscale={1}since scaling is handled by the wrapper. This allows the viewport content to scale while keeping UI elements (handles, edges) properly positioned.The logic is complex but necessary to support the zoom feature while maintaining interactive resize handles.
…ions - Updated ZoomIFrame to correctly apply scaling transformations. - Enhanced ZoomProvider to manage zoom levels with predefined constants. - Refactored Zoom component to utilize ActionList for zoom controls and added keyboard shortcuts for zoom actions. - Improved ZoomWrapper to dynamically adjust zoom levels based on user interactions.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @code/core/src/components/components/Zoom/ZoomIFrame.tsx:
- Around line 42-44: ZoomIFrame currently uses @ts-expect-error to silence
strict TypeScript issues for the iframe property and related refs and DOM
access; replace those suppressions with proper typing and null checks: type the
component's iframe prop and iFrameRef as HTMLIFrameElement | null (or
React.RefObject<HTMLIFrameElement>), guard accesses to iFrameRef.current and
iFrameRef.current.contentDocument with nullish checks before assigning or
reading body.style, and narrow types when setting style (e.g., ensure values are
strings like `${(1/scale)*100}%`); also add unit tests for ZoomIFrame rendering
at scales 0.5, 1, and 2 to verify computed width/height and transform values.
In @code/core/src/manager/components/preview/NumericInput.tsx:
- Around line 91-99: The cursor positioning in setInputSelection incorrectly
skips when index === 0; change the condition so it triggers for any non-negative
index (e.g., use index >= 0 or index !== -1) and still guard input exists before
calling input.setSelectionRange(index, index). Update the useCallback block that
references inputRef.current and the computed index (const index =
input?.value.search(/[^\d.]/) ?? -1) to check for index >= 0 (or index !== -1)
instead of index > 0 so the cursor is set at the start of the field as well.
- Around line 66-77: The ref type declared in forwardRef for NumericInput ({
select: () => void }) does not match what useImperativeHandle returns
(inputRef.current, an HTMLInputElement); fix by making useImperativeHandle
expose an object that matches the declared shape: call
useImperativeHandle(forwardedRef, () => ({ select: () =>
inputRef.current?.select?.() })), ensuring you reference inputRef and
forwardedRef; alternatively, if you prefer exposing the raw element, change the
forwardRef generic from { select: () => void } to HTMLInputElement so the types
align.
In @code/core/src/manager/components/preview/tools/zoom.tsx:
- Around line 168-173: The zoomOut callback uses Array.prototype.findLast
(ES2023) which breaks ES2020 targets; replace it with an ES2020-compatible
search over ZOOM_LEVELS (e.g., iterate from the end and pick the first level <
value) inside the zoomOut function and then call set(lowerZoomLevel) when found;
update references to zoomOut, ZOOM_LEVELS, set, and value accordingly so
behavior remains the same without using findLast.
In @code/core/src/manager/components/preview/Viewport.tsx:
- Around line 345-351: Compute a safeScale and use it instead of raw scale to
avoid Infinity/negative CSS values: e.g. derive safeScale = (typeof scale ===
'number' && isFinite(scale) && scale > 0) ? scale : 1 (or clamp with
Math.max(scale, 0.01) if you prefer a minimum zoom), then replace the direct
uses of scale in the inline style block (height, width, transform) and in
frameStyles with safeScale so layout never receives 0, negative, NaN or Infinity
values.
🧹 Nitpick comments (4)
code/core/src/manager/components/preview/Viewport.tsx (2)
274-277: Consider handling edge cases in parseNumber.The regex doesn't match negative values or values with whitespace (e.g.,
"100 px"). If callers ever pass such values,parseNumberreturnsundefined, falling back to{ number: 0, unit: 'px' }, which silently produces zero-sized dimensions.If negative or whitespace-padded inputs are possible, consider:
♻️ More robust regex (optional)
const parseNumber = (value: string) => { - const [match, number, unit] = value.match(/^(\d+(?:\.\d+)?)(\%|[a-z]{0,4})?$/) || []; + const [match, number, unit] = value.trim().match(/^(-?\d+(?:\.\d+)?)(\%|[a-z]{0,4})?$/) || []; return match ? { number: Number(number), unit } : undefined; };
362-373: Consider using parseNumber for consistent data-value extraction.The
.replace('px', '')only handlespxunits. If width/height uses%,em, or other units, the data-value will include the unit string. For consistency, consider reusingparseNumber:♻️ Consistent unit handling
<DragHandle ref={dragRefX} isDefault={isDefault} data-side="right" - data-value={width.replace('px', '')} + data-value={parseNumber(width)?.number ?? width} /> <DragHandle ref={dragRefY} isDefault={isDefault} data-side="bottom" - data-value={height.replace('px', '')} + data-value={parseNumber(height)?.number ?? height} />code/core/src/manager/components/preview/NumericInput.tsx (1)
112-134: Clarify unit handling logic in arrow key handler.The arrow key handler has inconsistent unit handling:
- Line 123:
setInputValuestripsbaseUnitfrom the display value- Line 124:
setValueincludes the unit in the external valueThis seems intentional (display without baseUnit, store with baseUnit), but the logic is subtle. Consider adding a comment explaining why
baseUnitis stripped for display but included in the stored value.📝 Suggested documentation
if (!Number.isNaN(num) && update >= 0) { const inputUnit = inputValue.match(/(\d+(?:\.\d+)?)(\%|[a-z]{0,4})?$/)?.[2] || unit || baseUnit || ''; + // Strip baseUnit from display (user sees "100") but include in stored value ("100px") setInputValue(`${update}${inputUnit === baseUnit ? '' : inputUnit}`); setValue(`${update}${inputUnit}`); setInputSelection(); }code/core/src/manager/components/preview/tools/zoom.tsx (1)
182-201: Consider optimizing shortcut re-registration.The shortcuts are re-registered on every
valuechange becausezoomInandzoomOutare recreated whenvaluechanges. While functional, this could be optimized by using refs or restructuring the callbacks.Potential optimization approach
Consider using a ref to hold the current value and create stable callbacks:
const zoomIn = useCallback(() => { - const higherZoomLevel = ZOOM_LEVELS.find((level) => level > value); + const higherZoomLevel = ZOOM_LEVELS.find((level) => level > valueRef.current); if (higherZoomLevel) { set(higherZoomLevel); } - }, [set, value]); + }, [set]);This would prevent re-registration of shortcuts on every zoom change.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
code/.storybook/bench/bench.stories.tsxcode/core/src/components/components/ActionList/ActionList.stories.tsxcode/core/src/components/components/Zoom/ZoomIFrame.tsxcode/core/src/manager-api/modules/shortcuts.tscode/core/src/manager/components/Shortcut.tsxcode/core/src/manager/components/preview/NumericInput.stories.tsxcode/core/src/manager/components/preview/NumericInput.tsxcode/core/src/manager/components/preview/SizeInput.tsxcode/core/src/manager/components/preview/Viewport.tsxcode/core/src/manager/components/preview/tools/share.tsxcode/core/src/manager/components/preview/tools/zoom.stories.tsxcode/core/src/manager/components/preview/tools/zoom.tsxcode/core/src/manager/components/sidebar/ContextMenu.tsxcode/core/src/manager/container/Menu.stories.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/manager/container/Preview.tsx
💤 Files with no reviewable changes (1)
- code/core/src/manager/components/preview/SizeInput.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- code/core/src/manager/components/sidebar/ContextMenu.tsx
- code/.storybook/bench/bench.stories.tsx
- code/core/src/manager/components/preview/tools/zoom.stories.tsx
- code/core/src/manager/components/preview/NumericInput.stories.tsx
- code/core/src/manager/container/Menu.stories.tsx
- code/core/src/manager/container/Preview.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-api/modules/shortcuts.tscode/core/src/manager/components/preview/tools/zoom.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/manager/components/preview/NumericInput.tsxcode/core/src/manager/components/preview/Viewport.tsxcode/core/src/components/components/ActionList/ActionList.stories.tsxcode/core/src/manager/components/Shortcut.tsxcode/core/src/components/components/Zoom/ZoomIFrame.tsxcode/core/src/manager/components/preview/tools/share.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 commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/manager-api/modules/shortcuts.tscode/core/src/manager/components/preview/tools/zoom.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/manager/components/preview/NumericInput.tsxcode/core/src/manager/components/preview/Viewport.tsxcode/core/src/components/components/ActionList/ActionList.stories.tsxcode/core/src/manager/components/Shortcut.tsxcode/core/src/components/components/Zoom/ZoomIFrame.tsxcode/core/src/manager/components/preview/tools/share.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/manager-api/modules/shortcuts.tscode/core/src/manager/components/preview/tools/zoom.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/manager/components/preview/NumericInput.tsxcode/core/src/manager/components/preview/Viewport.tsxcode/core/src/components/components/ActionList/ActionList.stories.tsxcode/core/src/manager/components/Shortcut.tsxcode/core/src/components/components/Zoom/ZoomIFrame.tsxcode/core/src/manager/components/preview/tools/share.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 useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager-api/modules/shortcuts.tscode/core/src/manager/components/preview/tools/zoom.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/manager/components/preview/NumericInput.tsxcode/core/src/manager/components/preview/Viewport.tsxcode/core/src/components/components/ActionList/ActionList.stories.tsxcode/core/src/manager/components/Shortcut.tsxcode/core/src/components/components/Zoom/ZoomIFrame.tsxcode/core/src/manager/components/preview/tools/share.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/manager-api/modules/shortcuts.tscode/core/src/manager/components/preview/tools/zoom.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/manager/components/preview/NumericInput.tsxcode/core/src/manager/components/preview/Viewport.tsxcode/core/src/components/components/ActionList/ActionList.stories.tsxcode/core/src/manager/components/Shortcut.tsxcode/core/src/components/components/Zoom/ZoomIFrame.tsxcode/core/src/manager/components/preview/tools/share.tsx
🧠 Learnings (4)
📚 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/tools/zoom.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/manager/components/preview/tools/share.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/tools/zoom.tsxcode/core/src/manager/container/Menu.tsxcode/core/src/manager/components/preview/Viewport.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/container/Menu.tsxcode/core/src/components/components/ActionList/ActionList.stories.tsxcode/core/src/manager/components/preview/tools/share.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/tools/share.tsx
🧬 Code graph analysis (3)
code/core/src/manager/components/preview/tools/zoom.tsx (2)
code/core/src/manager/components/preview/NumericInput.tsx (1)
NumericInput(66-152)code/core/src/manager-api/root.tsx (1)
useStorybookApi(296-299)
code/core/src/manager/components/preview/NumericInput.tsx (2)
code/core/src/components/index.ts (1)
Form(62-62)code/core/src/preview-api/index.ts (5)
useMemo(10-10)useRef(13-13)useState(14-14)useCallback(6-6)useEffect(8-8)
code/core/src/manager/components/Shortcut.tsx (1)
code/core/src/manager-api/modules/shortcuts.ts (1)
keys(23-25)
⏰ 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 (11)
code/core/src/manager/components/preview/Viewport.tsx (1)
222-271: Event handler implementation looks solid.The drag handling logic properly:
- Cleans up event listeners in the effect cleanup and
onEnd- Uses refs to track drag state across renders
- Applies the
|| 1fallback for scale inonEnd(line 252)- Scrolls handles into view during drag for better UX
The non-null assertions on refs are safe here since the handlers only fire after mount.
code/core/src/components/components/ActionList/ActionList.stories.tsx (1)
5-5: LGTM! Import path correctly updated.The import path has been updated to reference the newly extracted Shortcut component. This aligns with the PR's refactoring effort to centralize the Shortcut component for reuse across the codebase.
code/core/src/manager-api/modules/shortcuts.ts (2)
48-50: Excellent functional update pattern.The updated signature allows
setShortcutsto accept either a value or an updater function, following React'ssetStatepattern. This prevents race conditions when multiple shortcut updates occur in quick succession and ensures updates are always based on the latest state.Also applies to: 210-216
221-221: Consistent usage of updater pattern.Both
setShortcutandsetAddonShortcutcorrectly use the new functional update pattern, ensuring immutable state updates and consistency across the API.Also applies to: 225-228
code/core/src/manager/components/preview/tools/share.tsx (1)
15-15: LGTM! Import path correctly updated.The import path has been updated to reference the centralized Shortcut component, consistent with the refactoring across the codebase.
code/core/src/manager/components/Shortcut.tsx (1)
1-36: LGTM! Clean component extraction.The Shortcut component is well-implemented and successfully centralizes shortcut rendering. The styling is consistent with Storybook's design system, and the logic is straightforward.
code/core/src/manager/container/Menu.tsx (1)
20-20: LGTM! Successful migration to centralized Shortcut component.The import change correctly references the extracted Shortcut component, and all usages throughout the file remain compatible.
code/core/src/manager/components/preview/tools/zoom.tsx (4)
1-16: LGTM! Well-defined zoom levels.The predefined ZOOM_LEVELS array provides good coverage from 25% to 400%, and the constants are clearly named.
32-52: LGTM! Correct context provider implementation.The ZoomProvider properly manages zoom state and conditionally applies scaling based on the
shouldScaleprop, which aligns with the PR's goal of making the canvas scale with zoom level.
54-153: LGTM! Comprehensive zoom UI with good UX.The popover-based UI provides intuitive controls with manual input, preset options, and keyboard shortcuts. The focus management (lines 65-69) enhances usability by auto-selecting the input when the popover opens.
206-212: LGTM! Proper addon registration.The zoom tool is correctly registered with appropriate match conditions and uses the context consumer pattern to access zoom state.
| const zoomOut = useCallback(() => { | ||
| const lowerZoomLevel = ZOOM_LEVELS.findLast((level) => level < value); | ||
| if (lowerZoomLevel) { | ||
| set(lowerZoomLevel); | ||
| } | ||
| }, [set, value]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check all usages of findLast in the codebase
rg -n "\.findLast\(" --type ts --type tsx -C 2Repository: storybookjs/storybook
Length of output: 92
🏁 Script executed:
# Check the tsconfig and build configuration to understand target browser support
find . -name "tsconfig*.json" -o -name "*.config.js" -o -name "*.config.ts" | grep -E "(tsconfig|webpack|babel|vite)" | head -10Repository: storybookjs/storybook
Length of output: 441
🏁 Script executed:
# Check the specific file context and surrounding code
head -180 code/core/src/manager/components/preview/tools/zoom.tsx | tail -20Repository: storybookjs/storybook
Length of output: 543
🏁 Script executed:
# Search for any polyfill configurations or ES2023 support mentions
rg -i "findlast|polyfill|es2023|es2022" --type json --type ts --type tsx | head -20Repository: storybookjs/storybook
Length of output: 92
🏁 Script executed:
# Check package.json for browserslist or browser support target
cat package.json | grep -A 10 -B 2 "browserslist\|engines\|target"Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Search for findLast usage with corrected syntax
rg "findLast" --type ts -C 2Repository: storybookjs/storybook
Length of output: 2167
🏁 Script executed:
# Check the main tsconfig to understand target browser support
cat code/tsconfig.base.json 2>/dev/null || cat tsconfig.json 2>/dev/null || cat code/core/tsconfig.jsonRepository: storybookjs/storybook
Length of output: 338
🏁 Script executed:
# Look for browserslist or engine configuration
cat package.json | jq '.browserslist, .engines'Repository: storybookjs/storybook
Length of output: 74
🏁 Script executed:
# Search for any polyfill or ES2023 compatibility mentions in config files
rg "target|lib|ES2023|ES2022" code/tsconfig.base.json code/core/tsconfig.json --type json 2>/dev/null || find code -name "tsconfig.json" -exec grep -H "target\|lib" {} \; | head -20Repository: storybookjs/storybook
Length of output: 2685
🏁 Script executed:
# Count how many times findLast appears in the codebase
git grep "findLast" | wc -lRepository: storybookjs/storybook
Length of output: 66
🏁 Script executed:
# Check the root tsconfig
cat tsconfig.jsonRepository: storybookjs/storybook
Length of output: 110
🏁 Script executed:
# Check if there's a specific target mentioned in any build or webpack config
find . -maxdepth 2 -name "*.config.*" -type f | head -20Repository: storybookjs/storybook
Length of output: 309
🏁 Script executed:
# Check the package.json for any relevant metadata about browser support
cat package.json | grep -A 5 -B 5 "browserslist\|target\|esnext" | head -30Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Look at the actual project structure for where zoom.tsx is compiled
cat code/core/tsconfig.jsonRepository: storybookjs/storybook
Length of output: 338
🏁 Script executed:
# Check the parent tsconfig that code/core/tsconfig.json extends
cat code/tsconfig.jsonRepository: storybookjs/storybook
Length of output: 723
🏁 Script executed:
# Look for any build configuration that might transpile ES2023 features
rg "es2023|es2022|findLast" code --type json -C 2Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Check if there's any .babelrc or similar transpilation config
find code -maxdepth 3 -name ".babelrc*" -o -name "babel.config.*" -o -name "webpack.config.*" | head -10Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Check the actual browser support target in package.json more comprehensively
head -50 package.jsonRepository: storybookjs/storybook
Length of output: 2477
🏁 Script executed:
# Check if there's any webpack or build configuration that might handle transpilation
find code/core -name "*.config.*" -o -name "webpack*" -o -name "vite*" | head -20Repository: storybookjs/storybook
Length of output: 190
🏁 Script executed:
# Check the git log for zoom.tsx to see when findLast was introduced
git log --oneline -n 20 code/core/src/manager/components/preview/tools/zoom.tsxRepository: storybookjs/storybook
Length of output: 85
🏁 Script executed:
# Check if there are any build-time transformations mentioned in package.json scripts
cat package.json | grep -A 50 '"scripts"' | grep -E "build|compile|transpile"Repository: storybookjs/storybook
Length of output: 276
🏁 Script executed:
# Verify what browsers/runtime Storybook officially targets
rg "browser|target|support" code/core/package.json --type json -A 2 -B 2Repository: storybookjs/storybook
Length of output: 365
🏁 Script executed:
# Check if there's a polyfill or shim being applied at runtime
rg "findLast|Array.prototype" code/core/src --type ts -B 3 -A 3Repository: storybookjs/storybook
Length of output: 649
🏁 Script executed:
# Check the core/package.json specifically for build configuration and dependencies
cat code/core/package.json | head -100Repository: storybookjs/storybook
Length of output: 2947
🏁 Script executed:
# Look for any mention of ES compatibility or version support in docs or config
rg "ES2023|ES2022|browser.*support" code --type md -C 2 | head -40Repository: storybookjs/storybook
Length of output: 47
Use a fallback implementation compatible with ES2020.
The findLast method is an ES2023 feature. Since Storybook targets ES2020, this will fail in browsers without ES2023 support.
Suggested fix
const zoomOut = useCallback(() => {
- const lowerZoomLevel = ZOOM_LEVELS.findLast((level) => level < value);
+ const lowerZoomLevel = [...ZOOM_LEVELS].reverse().find((level) => level < value);
if (lowerZoomLevel) {
set(lowerZoomLevel);
}
}, [set, value]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const zoomOut = useCallback(() => { | |
| const lowerZoomLevel = ZOOM_LEVELS.findLast((level) => level < value); | |
| if (lowerZoomLevel) { | |
| set(lowerZoomLevel); | |
| } | |
| }, [set, value]); | |
| const zoomOut = useCallback(() => { | |
| const lowerZoomLevel = [...ZOOM_LEVELS].reverse().find((level) => level < value); | |
| if (lowerZoomLevel) { | |
| set(lowerZoomLevel); | |
| } | |
| }, [set, value]); |
🤖 Prompt for AI Agents
In @code/core/src/manager/components/preview/tools/zoom.tsx around lines 168 -
173, The zoomOut callback uses Array.prototype.findLast (ES2023) which breaks
ES2020 targets; replace it with an ES2020-compatible search over ZOOM_LEVELS
(e.g., iterate from the end and pick the first level < value) inside the zoomOut
function and then call set(lowerZoomLevel) when found; update references to
zoomOut, ZOOM_LEVELS, set, and value accordingly so behavior remains the same
without using findLast.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/manager-api/tests/shortcuts.test.js (1)
9-12: Add test coverage for the functional update branch.The mock now supports functional updates (line 10), but no test exercises this new capability. While the shortcuts module may use functional updates internally, the mock's conditional logic remains untested.
🧪 Suggested test case
Consider adding a test that directly verifies the functional update pattern:
it('supports functional updates in setState', () => { const store = createMockStore(); store.setState({ shortcuts: { fullScreen: ['alt', 'F'] } }); const result = store.setState((currentState) => ({ shortcuts: { ...currentState.shortcuts, fullScreen: ['X'] } })); expect(result.shortcuts.fullScreen).toEqual(['X']); expect(store.getState().shortcuts.fullScreen).toEqual(['X']); });As per coding guidelines, tests should cover all branches and variations, including the new
typeof update === 'function'path.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/manager-api/tests/shortcuts.test.js
🧰 Additional context used
📓 Path-based instructions (6)
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Usevi.mock()with thespy: trueoption for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Usevi.mocked()to type and access the mocked functions in Vitest tests
Implement mock behaviors inbeforeEachblocks in Vitest tests
Mock all required dependencies that the test subject uses
Each mock implementation should return a Promise for async functions in Vitest
Mock implementations should match the expected return type of the original function
Mock all required properties and methods that the test subject uses in Vitest tests
Avoid direct function mocking withoutvi.mocked()in Vitest tests
Avoid mock implementations outside ofbeforeEachblocks in Vitest tests
Avoid mocking without thespy: trueoption in Vitest tests
Avoid inline mock implementations within test cases in Vitest tests
Avoid mocking only a subset of required dependencies in Vitest tests
Mock at the highest level of abstraction needed in Vitest tests
Keep mock implementations simple and focused in Vitest tests
Use type-safe mocking withvi.mocked()in Vitest tests
Document complex mock behaviors in Vitest tests
Group related mocks together in Vitest tests
Files:
code/core/src/manager-api/tests/shortcuts.test.js
**/*.{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-api/tests/shortcuts.test.js
**/*.{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 commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/manager-api/tests/shortcuts.test.js
**/*.{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 useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager-api/tests/shortcuts.test.js
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{test,spec}.{ts,tsx,js,jsx}: Write meaningful unit tests that actually import and call the functions being tested, not just verify syntax patterns
Achieve high test coverage of business logic, aiming for 75%+ coverage of statements/lines
Cover all branches, conditions, edge cases, error paths, and different input variations in unit tests
Usevi.mock()to mock file system, loggers, and other external dependencies in tests
Files:
code/core/src/manager-api/tests/shortcuts.test.js
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/manager-api/tests/shortcuts.test.js
🧠 Learnings (10)
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Use `vi.mock()` to mock file system, loggers, and other external dependencies in tests
Applied to files:
code/core/src/manager-api/tests/shortcuts.test.js
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.mocked()` to type and access the mocked functions in Vitest tests
Applied to files:
code/core/src/manager-api/tests/shortcuts.test.js
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.mock()` with the `spy: true` option for all package and file mocks in Vitest tests
Applied to files:
code/core/src/manager-api/tests/shortcuts.test.js
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use type-safe mocking with `vi.mocked()` in Vitest tests
Applied to files:
code/core/src/manager-api/tests/shortcuts.test.js
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses in Vitest tests
Applied to files:
code/core/src/manager-api/tests/shortcuts.test.js
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Each mock implementation should return a Promise for async functions in Vitest
Applied to files:
code/core/src/manager-api/tests/shortcuts.test.js
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid direct function mocking without `vi.mocked()` in Vitest tests
Applied to files:
code/core/src/manager-api/tests/shortcuts.test.js
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors in Vitest tests
Applied to files:
code/core/src/manager-api/tests/shortcuts.test.js
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in `beforeEach` blocks in Vitest tests
Applied to files:
code/core/src/manager-api/tests/shortcuts.test.js
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused in Vitest tests
Applied to files:
code/core/src/manager-api/tests/shortcuts.test.js
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/manager/components/preview/tools/zoom.tsx (1)
136-138: Inconsistent ActionList.Text wrapping.The 200% action text is not wrapped in
ActionList.Text, unlike the other action items (lines 110, 120, 126, 131). For consistency, wrap it inActionList.Text.♻️ Proposed fix
<ActionList.Action onClick={() => zoomTo(2)} ariaLabel="Zoom to 200%"> - 200% + <ActionList.Text>200%</ActionList.Text> </ActionList.Action>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/manager/components/preview/Viewport.tsxcode/core/src/manager/components/preview/tools/zoom.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/preview/tools/zoom.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 commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/manager/components/preview/tools/zoom.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/manager/components/preview/tools/zoom.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 useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/components/preview/tools/zoom.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/manager/components/preview/tools/zoom.tsx
🧠 Learnings (2)
📚 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/tools/zoom.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/tools/zoom.tsx
🧬 Code graph analysis (1)
code/core/src/manager/components/preview/tools/zoom.tsx (4)
code/core/src/components/components/Button/Button.tsx (1)
Button(53-157)code/core/src/manager/components/preview/NumericInput.tsx (1)
NumericInput(66-152)code/core/src/preview-api/index.ts (3)
useRef(13-13)useCallback(6-6)useEffect(8-8)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). (3)
- GitHub Check: normal
- GitHub Check: nx
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (2)
code/core/src/manager/components/preview/tools/zoom.tsx (2)
183-202: Verify consistency between displayed and registered keyboard shortcuts.Line 111 displays the zoom in shortcut as
['alt', '+'], but line 192 registers it as['alt', '=']. While these typically map to the same physical key (with=being the unshifted variant), ensure this inconsistency doesn't confuse users or cause the shortcut to fail. Consider aligning the display with the registered key or documenting this intentional difference.#!/bin/bash # Search for other shortcut registrations and displays to check the pattern used in the codebase echo "=== Checking setAddonShortcut usage patterns ===" rg -n "setAddonShortcut" -A 5 -B 2 echo "" echo "=== Checking Shortcut component usage patterns ===" rg -n "<Shortcut keys=" -B 2 -A 2
144-152: Remove deprecatedactiveprop fromZoomButton.The
activeprop onButtonis deprecated and will be removed in Storybook 11. Replace withToggleButtonor adjust the styling/state management approach to indicate the non-default zoom state without relying on theactiveprop.⛔ Skipped due to learnings
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.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.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.
code/core/src/manager/components/preview/NumericInput.stories.tsx
Outdated
Show resolved
Hide resolved
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
LGTM, all comments minor.
If you're ok with it, I'd like to try something on your branch so that ArrowUp + ArrowDown on the zoom tool cycle through the zoom levels without needing to open the dialog. Many other buttons in the toolbar trigger on ArrowUp + ArrowDown so I wanna allow for users' muscle memory to pay off.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
code/core/src/manager/components/preview/NumericInput.tsx (1)
114-141: Consider stricter unit validation in the regex pattern.The regex on line 129 (
/(\d+(?:\.\d+)?)(\%|[a-z]{0,4})?$/) is quite permissive, accepting any 0-4 letter suffix. While this provides flexibility, it also allows invalid units like "abcd". If the component should only support specific CSS units (px, em, rem, vw, vh, etc.), consider using a more restrictive pattern.♻️ Optional: Stricter unit validation
- const inputUnit = - inputValue.match(/(\d+(?:\.\d+)?)(\%|[a-z]{0,4})?$/)?.[2] || unit || baseUnit || ''; + const inputUnit = + inputValue.match(/(\d+(?:\.\d+)?)(\%|px|em|rem|vw|vh|vmin|vmax)?$/)?.[2] || unit || baseUnit || '';This restricts units to common CSS values. Adjust the list based on your requirements.
code/core/src/manager/components/preview/Viewport.tsx (1)
200-203: Consider stricter unit validation (same pattern as NumericInput).The
parseNumberregex uses the same permissive pattern/^(\d+(?:\.\d+)?)(\%|[a-z]{0,4})?$/that accepts any 0-4 letter suffix. For consistency and type safety, consider using the same stricter unit validation suggested for NumericInput if you decide to implement it.Note: This is the same pattern used in NumericInput.tsx line 129, so any changes should be coordinated across both files to maintain consistency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/manager/components/preview/NumericInput.tsxcode/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/preview/NumericInput.tsxcode/core/src/manager/components/preview/Viewport.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 commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/manager/components/preview/NumericInput.tsxcode/core/src/manager/components/preview/Viewport.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/manager/components/preview/NumericInput.tsxcode/core/src/manager/components/preview/Viewport.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 useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/components/preview/NumericInput.tsxcode/core/src/manager/components/preview/Viewport.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/manager/components/preview/NumericInput.tsxcode/core/src/manager/components/preview/Viewport.tsx
🧠 Learnings (1)
📚 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/NumericInput.tsxcode/core/src/manager/components/preview/Viewport.tsx
🧬 Code graph analysis (1)
code/core/src/manager/components/preview/NumericInput.tsx (2)
code/core/src/components/index.ts (1)
Form(62-62)code/core/src/preview-api/index.ts (5)
useMemo(10-10)useRef(13-13)useState(14-14)useCallback(6-6)useEffect(8-8)
⏰ 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 (7)
code/core/src/manager/components/preview/NumericInput.tsx (4)
14-14: LGTM: Using React Aria's useId for React 17 compatibility.The
useIdfrom@react-aria/utilsprovides a compatibility layer that works with React 17, unlikeReact.useIdwhich requires React 18+. This aligns with the project's need to support React 17 until Storybook 11 is released.Based on learnings, this is the recommended approach for generating IDs while maintaining React 17 compatibility.
93-101: LGTM: Cursor positioning logic is correct.The cursor positioning places the selection just before the unit suffix (e.g., between "100.5" and "px"), which provides good UX when users focus the input. The use of
requestAnimationFrameensures the DOM has updated before manipulating selection.
143-163: LGTM: Component structure is well-designed.The component correctly handles:
- Screen-reader accessibility with the
sb-sr-onlylabel- ID generation with fallback to
useId- Proper focus/blur handling to sync internal and external state
- Optional before/after adornments for flexible composition
81-91: The onChange handler intentionally allows non-numeric values to support intermediate editing states (e.g., when a user is typing "100"). Both consumers of this component already validate these values before using them:Viewport.tsxvalidates through regex pattern matching in theresize()function, andzoom.tsxvalidates with an explicit!Number.isNaN()check before calling downstream functions. No action needed.code/core/src/manager/components/preview/Viewport.tsx (3)
279-286: LGTM: Scaling implementation is mathematically sound.The wrapper-based scaling approach elegantly handles zoom:
FrameWrapperis sized tooriginalDimensions * scaleviaframeStyles- Inner wrapper is sized to
(1 / scale) * 100%to contain the original dimensions- CSS
transform: scale(scale)applies the visual zoom- IFrame always renders at
scale={1}, avoiding nested scaling issuesThis ensures the iframe content remains at 1:1 pixel ratio while achieving the desired zoom effect, which should prevent rendering artifacts and maintain proper event coordinate handling.
Also applies to: 343-363
228-230: LGTM: Drag behavior correctly handles scaling.Key improvements:
- Auto-scroll (lines 241-249): Uses
scrollIntoViewto keep drag handles visible during resize, improving UX when handles are near viewport edges.- Scale compensation (lines 257-258): Correctly divides
clientWidthandclientHeightby scale when callingresize(), ensuring the stored dimensions represent the actual (unscaled) size rather than the visual size.- Visual feedback (lines 235, 239): Updates
data-valueattributes on drag handles for real-time dimension display.The scale-aware resize calculation properly handles the wrapper-based scaling approach.
Also applies to: 234-249, 256-258
294-301: ThereadOnlyprop is valid and supported byActionList.Action.
ActionList.ActionextendsActionListButton, which extendsButton, and theButtoncomponent explicitly accepts and defaultsreadOnlyto false. The prop correctly propagates through the component hierarchy. No action needed on this point.Regarding the
aria-labelandlabelprops onSizeInput(lines 294-296, 315-317): while both are provided, this is acceptable. Thelabelprop creates a visually-hidden<label>element, andaria-labelis passed through to the input itself. Both can coexist on the same input;aria-labeltakes precedence for screen reader announcements. If reducing verbosity is desired, consider removing one of them, though both being present is not incorrect.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @code/core/src/manager/components/preview/NumericInput.tsx:
- Around line 14-15: The code constructs a RegExp from user-controlled baseUnit
in NumericInput (new RegExp(`${baseUnit}$`)) which allows regex injection;
escape baseUnit before building the RegExp (or use a safe alternative like
RegExp.escape helper) so metacharacters are treated literally, then replace the
existing RegExp construction in NumericInput (the baseUnit variable and the
RegExp usage around the suffix strip logic) with the escaped value.
🧹 Nitpick comments (6)
code/core/src/manager/components/Shortcut.tsx (1)
30-35: Avoid potential React key collisions inkeys.map(...).Using
key={key}(Line 33) can collide if the array contains duplicates; use an index-based key (or${key}-${i}) to keep rendering stable.Proposed change
export const Shortcut = ({ keys }: { keys: string[] }) => ( <Wrapper> - {keys.map((key) => ( - <Key key={key}>{shortcutToHumanString([key])}</Key> + {keys.map((key, i) => ( + <Key key={`${key}-${i}`}>{shortcutToHumanString([key])}</Key> ))} </Wrapper> );code/core/src/manager/components/preview/NumericInput.tsx (1)
145-164: PreferonKeyDownover a nativeaddEventListenerfor ArrowUp/ArrowDown.The current native listener (Line 161) is harder to compose with consumer handlers and is re-bound whenever
inputValuechanges. Consider moving this logic into a ReactonKeyDownand chaining withprops.onKeyDownif provided.code/core/src/manager/components/preview/NumericInput.stories.tsx (1)
16-105: Nice coverage for keyboard stepping; consider adding a “manual typing” story for parsing edge cases.Current stories validate ArrowUp/Down well; adding one story that types values (decimals, negative, invalid suffix, empty) would protect
parseValue()behavior from regressions.code/core/src/manager/components/preview/Viewport.tsx (1)
227-250: Consider throttlingscrollIntoViewduring drag.Calling
scrollIntoViewon everymousemove(Lines 241-249) can cause avoidable reflow/jank; usingrequestAnimationFramethrottling (or only scrolling when the handle is near/outside the viewport) would be safer.code/core/src/manager/components/preview/tools/zoom.tsx (2)
15-30: AvoidContextdefault no-op setter (fail fast if provider missing).Right now, calling
set()outside aZoomProviderwill silently do nothing, which is painful to diagnose—especially sinceZoomConsumer/ZoomProviderare exported.Proposed change
-const Context = createContext({ value: INITIAL_ZOOM_LEVEL, set: (v: number) => {} }); +const Context = createContext({ + value: INITIAL_ZOOM_LEVEL, + set: (_v: number) => { + throw new Error('ZoomProvider is missing'); + }, +});
63-155: Numeric input: allow decimals + clamp invalid/unsafe zoom values; make the leading icon non-interactive.
parseIntdrops decimals (e.g.12.5%→12%), which undermines “custom zoom” expectations.- No guard against
zoomTo(0)/ negative values (can create broken scaling downstream).- The leading
ActionList.Buttonis decorative but still a button element—ensure it’s not focusable/announced. (As per repo convention, preferariaLabel={false}when a Button’s children already provide the name; here it’s decorative so making it disabled + aria-hidden is safest.) Based on learnings, ButtonariaLabelusage is intentionally standardized.Proposed change
popover={ <> <ActionList> <ActionList.Item> <ZoomInput aria-label="Zoom percentage" ref={inputRef} unit="%" before={ - <ActionList.Button size="small" padding="small" readOnly aria-hidden> + <ActionList.Button + size="small" + padding="small" + disabled + aria-hidden + ariaLabel={false} + > <ZoomIcon /> </ActionList.Button> } after={ <ActionList.Button size="small" padding="small" onClick={() => zoomTo(1)} ariaLabel="Reset zoom" > <UndoIcon /> </ActionList.Button> } value={`${Math.round(value * 100)}%`} minValue={1} maxValue={1000} setValue={(value: string) => { - const zoomLevel = parseInt(value, 10) / 100; - if (!Number.isNaN(zoomLevel)) { - zoomTo(zoomLevel); - } + const percent = Number(String(value).replace('%', '')); + const zoomLevel = percent / 100; + if (Number.isFinite(zoomLevel) && zoomLevel > 0) zoomTo(zoomLevel); }} /> </ActionList.Item> </ActionList>onVisibleChange={(isVisible) => { if (isVisible) { - requestAnimationFrame(() => inputRef.current?.select()); + requestAnimationFrame(() => { + inputRef.current?.focus(); + inputRef.current?.select(); + }); } }}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code/core/src/manager/components/Shortcut.tsxcode/core/src/manager/components/preview/NumericInput.stories.tsxcode/core/src/manager/components/preview/NumericInput.tsxcode/core/src/manager/components/preview/Viewport.tsxcode/core/src/manager/components/preview/tools/zoom.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/preview/NumericInput.stories.tsxcode/core/src/manager/components/preview/tools/zoom.tsxcode/core/src/manager/components/Shortcut.tsxcode/core/src/manager/components/preview/NumericInput.tsxcode/core/src/manager/components/preview/Viewport.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 commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/manager/components/preview/NumericInput.stories.tsxcode/core/src/manager/components/preview/tools/zoom.tsxcode/core/src/manager/components/Shortcut.tsxcode/core/src/manager/components/preview/NumericInput.tsxcode/core/src/manager/components/preview/Viewport.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/manager/components/preview/NumericInput.stories.tsxcode/core/src/manager/components/preview/tools/zoom.tsxcode/core/src/manager/components/Shortcut.tsxcode/core/src/manager/components/preview/NumericInput.tsxcode/core/src/manager/components/preview/Viewport.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 useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/components/preview/NumericInput.stories.tsxcode/core/src/manager/components/preview/tools/zoom.tsxcode/core/src/manager/components/Shortcut.tsxcode/core/src/manager/components/preview/NumericInput.tsxcode/core/src/manager/components/preview/Viewport.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/manager/components/preview/NumericInput.stories.tsxcode/core/src/manager/components/preview/tools/zoom.tsxcode/core/src/manager/components/Shortcut.tsxcode/core/src/manager/components/preview/NumericInput.tsxcode/core/src/manager/components/preview/Viewport.tsx
🧠 Learnings (10)
📚 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/NumericInput.stories.tsx
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors in Vitest tests
Applied to files:
code/core/src/manager/components/preview/NumericInput.stories.tsx
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Cover all branches, conditions, edge cases, error paths, and different input variations in unit tests
Applied to files:
code/core/src/manager/components/preview/NumericInput.stories.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/tools/zoom.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/tools/zoom.tsxcode/core/src/manager/components/preview/NumericInput.tsxcode/core/src/manager/components/preview/Viewport.tsx
📚 Learning: 2025-11-25T11:09:33.798Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 33140
File: code/core/src/manager/components/sidebar/TagsFilter.tsx:247-259
Timestamp: 2025-11-25T11:09:33.798Z
Learning: In the storybookjs/storybook repository, PopoverProvider creates popovers with a dialog role, so using aria-haspopup="dialog" on buttons that trigger PopoverProvider is semantically correct.
Applied to files:
code/core/src/manager/components/preview/tools/zoom.tsx
📚 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/tools/zoom.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/tools/zoom.tsx
📚 Learning: 2025-10-01T15:24:01.060Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32594
File: code/core/src/components/components/Popover/WithPopover.tsx:7-9
Timestamp: 2025-10-01T15:24:01.060Z
Learning: In the Storybook repository, "react-aria-components/patched-dist/*" (e.g., "react-aria-components/patched-dist/Dialog", "react-aria-components/patched-dist/Popover", "react-aria-components/patched-dist/Tooltip") are valid import paths created by a patch applied to the react-aria-components package. These imports should not be flagged as broken or invalid until a maintainer explicitly states they are no longer acceptable.
Applied to files:
code/core/src/manager/components/preview/tools/zoom.tsx
📚 Learning: 2025-09-18T20:51:52.320Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:32-37
Timestamp: 2025-09-18T20:51:52.320Z
Learning: NVDA screen reader announces the selected value in Select components even when aria-label is used, possibly due to special treatment of the role or aria-expanded attribute. Real-world screen reader testing may reveal that theoretical ARIA concerns don't always apply in practice.
Applied to files:
code/core/src/manager/components/preview/tools/zoom.tsx
🧬 Code graph analysis (1)
code/core/src/manager/components/Shortcut.tsx (1)
code/core/src/manager-api/modules/shortcuts.ts (1)
keys(23-25)
⏰ 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 (2)
code/core/src/manager/components/preview/tools/zoom.tsx (2)
32-51:shouldScale=falsestill updates internal zoom state—confirm that’s intended.
set()always updates provider state even whenshouldScaleforces the exposed context value back toINITIAL_ZOOM_LEVEL. That’s fine if you want zoom “remembered” when scaling is re-enabled; otherwise consider ignoringset()whileshouldScaleis false.
214-215: Render wiring looks fine; just confirmZoomProvideralways wraps this tool.If
zoomToolcan render without an ancestorZoomProviderin some manager surfaces, the fail-fastContextchange above will throw (which is desirable, but should be validated).
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @code/core/src/manager/components/preview/tools/zoom.tsx:
- Line 114: The UI display uses Shortcut keys={['alt','+']} but the hotkey is
registered as ['alt','=']; update the Shortcut props to keys={['alt','=']} so
the visual label matches the actual binding (adjust the Shortcut component
instance and any other displayed instances), or alternatively change the hotkey
registration to ['alt','+'] if you intend the plus symbol to be the canonical
binding; ensure the Shortcut component and the hotkey registration use the same
array literal (refer to the Shortcut usage and the hotkey registration that
currently uses ['alt','=']).
- Line 173: The use of Array.prototype.findLast on ZOOM_LEVELS (assigning
lowerZoomLevel) is an ES2023 feature incompatible with the project's ES2020
target; replace it with an ES2020-compatible reverse-search: iterate ZOOM_LEVELS
from the last index down (or use a reversed shallow copy and .find) to locate
the first level < value and assign that to lowerZoomLevel, keeping the variable
name and semantics unchanged.
- Line 111: The usage of Array.prototype.at() (e.g., in the expression
disabled={value >= ZOOM_LEVELS.at(-1)!}) is an ES2022 feature while the project
targets ES2020; replace .at() usages in zoom.tsx (and the other occurrence
around the same block) with standard bracket indexing (use
ZOOM_LEVELS[ZOOM_LEVELS.length - 1] for .at(-1) and ZOOM_LEVELS[0] for .at(0)
where applicable) or ensure the build pipeline polyfills/transpiles .at()
appropriately so the code compiles under ES2020.
🧹 Nitpick comments (1)
code/core/src/manager/components/preview/tools/zoom.tsx (1)
138-142: Use consistent text wrapper for action items.Unlike the other preset items (lines 129, 134), this 200% preset doesn't wrap its text in
ActionList.Text. For consistency, consider:♻️ Consistent text wrapping
<ActionList.Item active={value === 2}> <ActionList.Action onClick={() => zoomTo(2)} ariaLabel="Zoom to 200%"> - 200% + <ActionList.Text>200%</ActionList.Text> </ActionList.Action> </ActionList.Item>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/manager/components/preview/tools/zoom.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/preview/tools/zoom.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 commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/manager/components/preview/tools/zoom.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/manager/components/preview/tools/zoom.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 useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/components/preview/tools/zoom.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/manager/components/preview/tools/zoom.tsx
🧠 Learnings (8)
📚 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/tools/zoom.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/tools/zoom.tsx
📚 Learning: 2025-11-25T11:09:33.798Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 33140
File: code/core/src/manager/components/sidebar/TagsFilter.tsx:247-259
Timestamp: 2025-11-25T11:09:33.798Z
Learning: In the storybookjs/storybook repository, PopoverProvider creates popovers with a dialog role, so using aria-haspopup="dialog" on buttons that trigger PopoverProvider is semantically correct.
Applied to files:
code/core/src/manager/components/preview/tools/zoom.tsx
📚 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/tools/zoom.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/tools/zoom.tsx
📚 Learning: 2025-10-01T15:24:01.060Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32594
File: code/core/src/components/components/Popover/WithPopover.tsx:7-9
Timestamp: 2025-10-01T15:24:01.060Z
Learning: In the Storybook repository, "react-aria-components/patched-dist/*" (e.g., "react-aria-components/patched-dist/Dialog", "react-aria-components/patched-dist/Popover", "react-aria-components/patched-dist/Tooltip") are valid import paths created by a patch applied to the react-aria-components package. These imports should not be flagged as broken or invalid until a maintainer explicitly states they are no longer acceptable.
Applied to files:
code/core/src/manager/components/preview/tools/zoom.tsx
📚 Learning: 2025-09-18T20:51:52.320Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:32-37
Timestamp: 2025-09-18T20:51:52.320Z
Learning: NVDA screen reader announces the selected value in Select components even when aria-label is used, possibly due to special treatment of the role or aria-expanded attribute. Real-world screen reader testing may reveal that theoretical ARIA concerns don't always apply in practice.
Applied to files:
code/core/src/manager/components/preview/tools/zoom.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/tools/zoom.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 (10)
code/core/src/manager/components/preview/tools/zoom.tsx (10)
1-16: LGTM: Clean imports and well-defined zoom levels.The predefined zoom levels align with the PR's goal of providing predictability, and the initial zoom level of 100% is appropriate.
18-28: LGTM: Styled components are appropriately defined.The fixed dimensions ensure consistent UI sizing.
22-22: LGTM: Context initialization follows standard React patterns.The default no-op setter is appropriate for the initial context value.
Also applies to: 30-30
32-52: LGTM: ZoomProvider correctly manages zoom state.The conditional zoom value based on
shouldScaleprop enables integration with the viewports feature as mentioned in the PR objectives.
54-69: LGTM: Popover configuration with smart auto-selection.The use of
requestAnimationFramebefore selecting the input text is a good pattern to ensure the DOM is ready.
95-96: LGTM: Zoom range supports custom values as documented.The 1-1000% range exceeds the previous 25-400% limitation, fulfilling the PR objective of supporting custom zoom percentages.
147-156: LGTM: Trigger button with clear accessibility label.The active state visualization helps users identify when zoom is not at default.
210-216: LGTM: Tool registration correctly wired through context.The render pattern properly connects the context consumer to the wrapper component.
186-205: No cleanup needed for shortcut registrations.
setAddonShortcutonly updates state and stores shortcuts in configuration objects; it does not register listeners or create subscriptions. Calling it multiple times with the sameactionNamesimply overwrites the previous value, so no cleanup is required.Likely an incorrect or invalid review comment.
79-79: ThereadOnlyprop is a valid and fully supported prop on the Button component in this codebase. No changes needed — the implementation is correct.Likely an incorrect or invalid review comment.
Contributes to #33245
Telescopes on #33290
What I did
This is an almost complete rewrite of the Zoom tool. Notable changes:
⌥+,⌥-and⌥0have been added for quick access.Screen.Recording.2026-01-08.at.11.27.01.mov
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Refactor
Tests
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.