UI: Allow direct kb/mouse actions on zoom tool button#33496
Conversation
|
View your CI Pipeline Execution ↗ for commit 498b913
☁️ Nx Cloud last updated this comment at |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a programmatic incremental zoom API Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (keyboard / wheel)
participant ZoomUI as Zoom component
participant Wrapper as ZoomWrapper (state + zoomBy)
participant State as internal setter
User->>ZoomUI: keydown / wheel event
ZoomUI->>Wrapper: call zoomIn / zoomOut / zoomTo / zoomBy(delta)
Wrapper->>State: set(Math.max(0.01, value + delta)) / set(newValue)
State-->>Wrapper: value updated
Wrapper-->>ZoomUI: re-render with new value
ZoomUI-->>User: updated zoom label shown
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @code/core/src/manager/components/preview/tools/zoom.tsx:
- Around line 206-211: zoomBy currently clamps only the minimum and can exceed
the maximum zoom; update the zoomBy callback to clamp the new value between 0.01
and the max zoom level (use ZOOM_LEVELS.at(-1) or the equivalent MAX_ZOOM)
before calling set, so it matches the bounds enforced by zoomIn/zoomOut (refer
to the zoomBy function, set, value, and ZOOM_LEVELS symbols).
- Around line 163-168: The Home/End handling is reversed: in the keydown handler
where it checks e.key for 'Home' and 'End' (calling zoomTo with ZOOM_LEVELS
indices), swap the targets so Home calls zoomTo(ZOOM_LEVELS[0]) and End calls
zoomTo(ZOOM_LEVELS[ZOOM_LEVELS.length - 1]); keep e.preventDefault() in both
branches and update the branches around the zoomTo calls in the same keydown
handler to maintain accessibility per WAI-ARIA.
🧹 Nitpick comments (1)
code/core/src/manager/components/preview/tools/zoom.stories.tsx (1)
86-144: Inconsistent use ofawaitonfocus()calls.Some tests use
zoom.focus()(lines 90, 110, 120, 130, 140) while one usesawait zoom.focus()(line 100). Whilefocus()is synchronous, consistency improves readability.♻️ Suggested fix for consistency (choose one style)
Either remove the
awaiton line 100:- await zoom.focus(); + zoom.focus();Or add
awaitto all other focus calls for consistency if there's a linting preference.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/manager/components/preview/tools/zoom.stories.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/tools/zoom.tsxcode/core/src/manager/components/preview/tools/zoom.stories.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.tsxcode/core/src/manager/components/preview/tools/zoom.stories.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.tsxcode/core/src/manager/components/preview/tools/zoom.stories.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.tsxcode/core/src/manager/components/preview/tools/zoom.stories.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.tsxcode/core/src/manager/components/preview/tools/zoom.stories.tsx
🧠 Learnings (13)
📚 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.stories.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.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.stories.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.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/tools/zoom.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} : Implement mock behaviors in `beforeEach` blocks in Vitest tests
Applied to files:
code/core/src/manager/components/preview/tools/zoom.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} : Avoid mock implementations outside of `beforeEach` blocks in Vitest tests
Applied to files:
code/core/src/manager/components/preview/tools/zoom.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} : Mock all required properties and methods that the test subject uses in Vitest tests
Applied to files:
code/core/src/manager/components/preview/tools/zoom.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} : Mock at the highest level of abstraction needed in Vitest tests
Applied to files:
code/core/src/manager/components/preview/tools/zoom.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} : Avoid mocking only a subset of required dependencies in Vitest tests
Applied to files:
code/core/src/manager/components/preview/tools/zoom.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} : Avoid inline mock implementations within test cases in Vitest tests
Applied to files:
code/core/src/manager/components/preview/tools/zoom.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} : Group related mocks together in Vitest tests
Applied to files:
code/core/src/manager/components/preview/tools/zoom.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} : Use `vi.mocked()` to type and access the mocked functions in Vitest tests
Applied to files:
code/core/src/manager/components/preview/tools/zoom.stories.tsx
🧬 Code graph analysis (1)
code/core/src/manager/components/preview/tools/zoom.tsx (1)
code/core/src/preview-api/index.ts (1)
useCallback(6-6)
⏰ 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). (4)
- GitHub Check: normal
- GitHub Check: nx
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: get-parameters
🔇 Additional comments (5)
code/core/src/manager/components/preview/tools/zoom.tsx (2)
150-178: LGTM on keyboard and wheel event handling structure.The event handler logic is well-structured with appropriate
preventDefault()calls to stop default browser behavior. The wheel direction mapping (negative deltaY = zoom in, positive = zoom out) follows the natural scroll metaphor.
54-60: LGTM on thezoomByprop addition.The new
zoomByprop is properly typed and threaded through fromZoomWrappertoZoom. TheuseCallbackdependency array is correct.Also applies to: 241-241
code/core/src/manager/components/preview/tools/zoom.stories.tsx (3)
126-144: Update HomeKey/EndKey test expectations after accessibility fix.If the Home/End key mapping is corrected in
zoom.tsxper accessibility standards, these test expectations will need to be swapped:
HomeKeyshould expect25%(minimum)EndKeyshould expect400%(maximum)
146-166: Good choice usingSimulate.wheelfor wheel events.Using
react-dom/test-utilsSimulate is appropriate here sinceuserEventfrom@testing-library/user-eventdoesn't fully support wheel events. ThewaitForwrapper ensures the async state update is captured.
25-37: LGTM on the render function withzoomBywiring.The
zoomByimplementation correctly mirrors the lower-bound clamping logic from the main component. Note that the story uses simplified+/- 0.5forzoomIn/zoomOutwhich differs from the actualZOOM_LEVELS-based implementation—this is acceptable for component-level isolation testing.
Sidnioulz
force-pushed
the
zoom-tool-kb-extras
branch
from
7ad1e99 to
0917ebf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @code/core/src/manager/components/preview/tools/zoom.stories.tsx:
- Around line 5-6: Remove the deprecated Simulate import and replace its usage
with fireEvent: delete the "Simulate" import from the top import statement and
add "fireEvent" to the existing import that currently has "expect, fn, screen,
waitFor, within"; then replace all calls to Simulate.wheel inside the file (the
two occurrences) with fireEvent.wheel so the wheel events use the modern test
utility.
In @code/core/src/manager/components/preview/tools/zoom.tsx:
- Around line 153-173: The Home/End key handling in the onKeyDown handler of the
zoom control is reversed: change the Home branch (currently calling
zoomTo(ZOOM_LEVELS[ZOOM_LEVELS.length - 1])) to call zoomTo(ZOOM_LEVELS[0]) so
Home goes to minimum zoom, and change the End branch (currently calling
zoomTo(ZOOM_LEVELS[0])) to call zoomTo(ZOOM_LEVELS[ZOOM_LEVELS.length - 1]) so
End goes to maximum zoom; update the two else-if branches in the onKeyDown
anonymous function accordingly and keep the e.preventDefault() calls.
🧹 Nitpick comments (1)
code/core/src/manager/components/preview/tools/zoom.tsx (1)
174-181: Consider handling horizontal wheel events.The current implementation triggers zoom for any wheel event with non-zero
deltaY. While this works for vertical scrolling, it doesn't guard against horizontal scrolling whendeltaY === 0, which would still callpreventDefault()but not perform any zoom action.♻️ Optional enhancement to skip no-op cases
onWheel={(e) => { if (e.deltaY < 0) { zoomIn(); + e.preventDefault(); } else if (e.deltaY > 0) { zoomOut(); + e.preventDefault(); } - e.preventDefault(); }}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/manager/components/preview/tools/zoom.stories.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/tools/zoom.tsxcode/core/src/manager/components/preview/tools/zoom.stories.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.tsxcode/core/src/manager/components/preview/tools/zoom.stories.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.tsxcode/core/src/manager/components/preview/tools/zoom.stories.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.tsxcode/core/src/manager/components/preview/tools/zoom.stories.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.tsxcode/core/src/manager/components/preview/tools/zoom.stories.tsx
🧠 Learnings (14)
📚 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.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.stories.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.stories.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.stories.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.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/tools/zoom.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} : Implement mock behaviors in `beforeEach` blocks in Vitest tests
Applied to files:
code/core/src/manager/components/preview/tools/zoom.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} : Avoid mock implementations outside of `beforeEach` blocks in Vitest tests
Applied to files:
code/core/src/manager/components/preview/tools/zoom.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} : Mock all required properties and methods that the test subject uses in Vitest tests
Applied to files:
code/core/src/manager/components/preview/tools/zoom.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} : Mock at the highest level of abstraction needed in Vitest tests
Applied to files:
code/core/src/manager/components/preview/tools/zoom.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} : Avoid mocking only a subset of required dependencies in Vitest tests
Applied to files:
code/core/src/manager/components/preview/tools/zoom.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} : Avoid inline mock implementations within test cases in Vitest tests
Applied to files:
code/core/src/manager/components/preview/tools/zoom.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} : Group related mocks together in Vitest tests
Applied to files:
code/core/src/manager/components/preview/tools/zoom.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} : Use `vi.mocked()` to type and access the mocked functions in Vitest tests
Applied to files:
code/core/src/manager/components/preview/tools/zoom.stories.tsx
🧬 Code graph analysis (1)
code/core/src/manager/components/preview/tools/zoom.tsx (1)
code/core/src/preview-api/index.ts (1)
useCallback(6-6)
⏰ 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 (6)
code/core/src/manager/components/preview/tools/zoom.tsx (3)
54-60: LGTM: zoomBy prop correctly added to Zoom component.The new
zoomByprop is properly typed and destructured for use in the component.
209-214: LGTM: zoomBy implementation is correct.The function properly clamps the minimum value to 0.01 while allowing unlimited maximum zoom, which is consistent with the ZoomInput's
maxValue={1000}configuration at line 97.
223-244: LGTM: useEffect dependencies are correct.The
zoomByfunction is correctly excluded from the effect dependencies since it's not used within the effect. The effect only registers shortcuts forzoomIn,zoomOut, andzoomTo.code/core/src/manager/components/preview/tools/zoom.stories.tsx (3)
25-38: LGTM: zoomBy implementation in stories matches component behavior.The
zoomBycallback correctly clamps the minimum value to 0.01, consistent with the implementation inzoom.tsx.
126-144: Update test expectations if Home/End semantics are corrected.The
HomeKeyandEndKeystories expect 400% and 25% respectively, matching the current implementation. However, if the Home/End key semantics are reversed to match UI conventions (as suggested inzoom.tsx), these test expectations will need to be swapped.
146-166: LGTM: Wheel event tests correctly use waitFor for async state updates.The wheel tests appropriately use
waitForto account for asynchronous state updates, ensuring stable assertions.
Sidnioulz
changed the title
Sidnioulz
changed the title
Sidnioulz
force-pushed
the
zoom-tool-kb-extras
branch
from
0917ebf to
27c6350
Compare
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
Sidnioulz
force-pushed
the
zoom-tool-kb-extras
branch
from
27c6350 to
3bec845
Compare
Sidnioulz
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
code/core/src/manager/components/preview/tools/zoom.tsx (2)
166-171:⚠️ Potential issue | 🟠 MajorSwap
Home/Endtargets to match expected keyboard semantics.
Homeshould jump to minimum zoom andEndto maximum zoom; the current branches are reversed. This also impacts the Home/End story assertions downstream.🔧 Proposed fix
- } else if (e.key === 'Home') { - zoomTo(ZOOM_LEVELS[ZOOM_LEVELS.length - 1]); + } else if (e.key === 'Home') { + zoomTo(ZOOM_LEVELS[0]); e.preventDefault(); } else if (e.key === 'End') { - zoomTo(ZOOM_LEVELS[0]); + zoomTo(ZOOM_LEVELS[ZOOM_LEVELS.length - 1]); e.preventDefault(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager/components/preview/tools/zoom.tsx` around lines 166 - 171, The Home/End handling is reversed: when e.key === 'Home' call zoomTo with the minimum zoom (ZOOM_LEVELS[0]) and when e.key === 'End' call zoomTo with the maximum zoom (ZOOM_LEVELS[ZOOM_LEVELS.length - 1]); update the branches in the keyboard handler where zoomTo is used (referencing ZOOM_LEVELS and zoomTo) so Home jumps to min and End to max, and adjust any impacted Home/End story assertions if present.
209-214:⚠️ Potential issue | 🟠 MajorClamp
zoomByto the same upper bound as other zoom controls.
zoomBycurrently enforces only a minimum. Repeated ArrowUp can push zoom beyond the supported max level.🔧 Proposed fix
const zoomBy = useCallback( (delta: number) => { - set(Math.max(0.01, value + delta)); + const min = 0.01; + const max = ZOOM_LEVELS[ZOOM_LEVELS.length - 1]; + set(Math.min(max, Math.max(min, value + delta))); }, [set, value] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager/components/preview/tools/zoom.tsx` around lines 209 - 214, zoomBy currently only enforces a minimum and can exceed the same max used elsewhere; modify zoomBy to clamp the computed new zoom to the same upper bound as the other zoom controls (e.g., use the same MAX_ZOOM constant or helper used by zoomIn/zoomOut/zoomTo). In the zoomBy callback (function zoomBy using set and value), compute newValue = clamp(value + delta, MIN_ZOOM, MAX_ZOOM) (or Math.min(MAX_ZOOM, Math.max(MIN_ZOOM, value + delta))) and pass newValue to set so zoomBy respects the same max limit as the other controls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/manager/components/preview/tools/zoom.stories.tsx`:
- Around line 89-93: The tests use findByRole('button', { name: 'Change zoom
level' }) but the component's helper openDialog and the actual control use role
'switch', causing lookup failures; update the play functions that query the zoom
control (e.g., where you assign const zoom = await canvas.findByRole(...)) to
use findByRole('switch', { name: 'Change zoom level' }) consistently (same
change for all occurrences noted around the zoom keyboard/wheel stories), keep
the subsequent zoom.focus(), userEvent.keyboard/wheel calls and assertions
unchanged other than the role string so the interactions target the real
control.
---
Duplicate comments:
In `@code/core/src/manager/components/preview/tools/zoom.tsx`:
- Around line 166-171: The Home/End handling is reversed: when e.key === 'Home'
call zoomTo with the minimum zoom (ZOOM_LEVELS[0]) and when e.key === 'End' call
zoomTo with the maximum zoom (ZOOM_LEVELS[ZOOM_LEVELS.length - 1]); update the
branches in the keyboard handler where zoomTo is used (referencing ZOOM_LEVELS
and zoomTo) so Home jumps to min and End to max, and adjust any impacted
Home/End story assertions if present.
- Around line 209-214: zoomBy currently only enforces a minimum and can exceed
the same max used elsewhere; modify zoomBy to clamp the computed new zoom to the
same upper bound as the other zoom controls (e.g., use the same MAX_ZOOM
constant or helper used by zoomIn/zoomOut/zoomTo). In the zoomBy callback
(function zoomBy using set and value), compute newValue = clamp(value + delta,
MIN_ZOOM, MAX_ZOOM) (or Math.min(MAX_ZOOM, Math.max(MIN_ZOOM, value + delta)))
and pass newValue to set so zoomBy respects the same max limit as the other
controls.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/manager/components/preview/tools/zoom.stories.tsxcode/core/src/manager/components/preview/tools/zoom.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/manager/components/preview/tools/zoom.stories.tsx (1)
90-90: Inconsistentawaitbeforefocus()calls.Line 100 uses
await zoom.focus()while all other keyboard stories omitawait. Sincefocus()is synchronous, theawaitis unnecessary—consider removing it from line 100 for consistency, or adding it everywhere if you prefer uniformity.Also applies to: 100-100, 110-110, 120-120, 130-130, 140-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager/components/preview/tools/zoom.stories.tsx` at line 90, The focus() calls on the zoom element are synchronously invoked (zoom.focus()) but one or more stories use an unnecessary await; make them consistent by removing the await operator from all zoom.focus() calls in the keyboard stories (e.g., the zoom variable's focus() invocations used in the keyboard story functions), so every story uses plain synchronous zoom.focus(), and run tests/Storybook to verify no behavior changed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/manager/components/preview/tools/zoom.stories.tsx`:
- Line 90: The focus() calls on the zoom element are synchronously invoked
(zoom.focus()) but one or more stories use an unnecessary await; make them
consistent by removing the await operator from all zoom.focus() calls in the
keyboard stories (e.g., the zoom variable's focus() invocations used in the
keyboard story functions), so every story uses plain synchronous zoom.focus(),
and run tests/Storybook to verify no behavior changed.
3 participants
What I did
Supports #33375.
I allowed for more direct keyboard and scroll operation, directly on the button. This helps keyboard users who expect ArrowDown to do something on other toolbar tools with a popup, and provides a more direct shortcut for mouse users (scroll the zoom button) that is commonly found in graphics apps with a canvas and zoom controls.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Documentation
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
Tests