AddonViewport: Stricter types #32324
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
jonniebigodes
left a comment
jonniebigodes
left a comment
There was a problem hiding this comment.
@hpohlmeyer thanks for taking the time to address the changes in the documentation. We appreciate it 🙏 ! From my end all is good. But i'll defer to @ndelangen for the code changes.
Have a great day.
Stay safe
|
View your CI Pipeline Execution ↗ for commit c0914ec
☁️ Nx Cloud last updated this comment at |
ndelangen
changed the title
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
|
@hpohlmeyer can you look at the CI failures? looks like the type strictness is causing some breakages, might be an easy fix? |
|
@jonniebigodes Can you please take a look! |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughExports INITIAL_VIEWPORTS (replacing the previous INTERNAL INITIAL_VIEWPORTS_DATA) and updates InitialViewportKeys to derive from it. Applies stricter typing using Sequence Diagram(s)sequenceDiagram
autonumber
participant Defaults as defaults.ts
participant Vitest as vitest-plugin/viewports.ts
participant Tool as Tool.tsx
Note over Defaults: INITIAL_VIEWPORTS exported\nMINIMAL_VIEWPORTS exported\n(as const satisfies ViewportMap)
Defaults->>Vitest: import { MINIMAL_VIEWPORTS, INITIAL_VIEWPORTS }
Defaults->>Tool: import { MINIMAL_VIEWPORTS, INITIAL_VIEWPORTS }
Vitest->>Vitest: options = { ...MINIMAL_VIEWPORTS, ...param.viewports, ...param.options }
Vitest->>Vitest: if options[defaultViewport] then extract styles (width/height)
Vitest-->>Defaults: read types/keys from INITIAL_VIEWPORTS
Tool->>Tool: viewportName: keyof ViewportMap\nisRotated: boolean | undefined
Tool->>Defaults: index options/INITIAL_VIEWPORTS as ViewportMap[viewportName]
Note over Vitest,Tool: Both now consume the exported, readonly ViewportMap
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-18T20:51:06.618ZApplied to files:
🔇 Additional comments (4)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/essentials/viewport.mdx (1)
171-173: Replace import path with '@storybook/addon-viewport'docs/essentials/viewport.mdx:171-173 — change
import { INITIAL_VIEWPORTS, MINIMAL_VIEWPORTS } from 'storybook/viewport';
to
import { INITIAL_VIEWPORTS, MINIMAL_VIEWPORTS } from '@storybook/addon-viewport';
🧹 Nitpick comments (1)
docs/essentials/viewport.mdx (1)
59-61: Docs/code mismatch: Nexus 5X height (660 vs 668).defaults.ts has 412 × 660; docs show 412 × 668. Align docs to code (or vice versa) to avoid confusion.
Apply this diff in the docs to match current code:
-| `nexus5x` | Nexus 5X | 412 × 668 | +| `nexus5x` | Nexus 5X | 412 × 660 |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/viewport/defaults.ts(3 hunks)docs/essentials/viewport.mdx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: normal
🔇 Additional comments (6)
docs/essentials/viewport.mdx (2)
33-33: Desktop added to MINIMAL_VIEWPORTS looks correct.Matches code (width × height: 1280 × 1024 shown as 1024 × 1280 in docs). Good.
64-64: Fixed iPad Pro 10.5-in height to 1112 — good catch.Aligned with defaults.ts.
code/core/src/viewport/defaults.ts (4)
3-3: Good use of “as const satisfies” for precise, readonly types.This tightens types without runtime impact and preserves literal keys/values. Nice.
Also applies to: 228-228
230-230: Deriving InitialViewportKeys from INITIAL_VIEWPORTS is correct.Keeps keys in sync with the exported map.
234-267: MINIMAL_VIEWPORTS addition of “desktop” matches docs and types.Dimensions and type align with the documentation changes.
196-201: Nexus 5X height mismatch — confirm and updateCode: code/core/src/viewport/defaults.ts (nexus5x) sets height '660px' (lines 196–200). Docs: docs/essentials/viewport.mdx lists
nexus5xas 412 × 668 (line 59). Update docs to 412 × 660 or change code height to '668px' so they match.
|
@hpohlmeyer, it seems there are some issues with this pull request regarding type strictness. Can you check on your end and run some tests to verify what's causing the breakages, adjust accordingly and let us know? |
… consistent type definitions for options and viewports
… to improve type safety
|
Problems should be resolved now |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/viewport/components/Tool.tsx (1)
120-133: Fix ReferenceError:lengthis undefined in Pure
lengthis declared inViewportTool, not inPure. This will throw at runtime. Compute fromviewportMaplocally.const Pure = React.memo(function PureTool(props: PureProps) { const { item, viewportMap, viewportName, isRotated, updateGlobals, setIsTooltipVisible, isLocked, isActive, width, height, } = props; const update = useCallback( (input: GlobalStateUpdate | undefined) => updateGlobals({ [PARAM_KEY]: input }), [updateGlobals] ); + + const length = Object.keys(viewportMap).length;
🧹 Nitpick comments (1)
code/core/src/viewport/components/Tool.tsx (1)
81-82: Type mismatch: pass a ViewportMap to Pure
PureProps.viewportMapisViewportMap, butoptionsmay be a looser type fromuseParameter. Cast at the callsite to satisfy the stricter prop.- viewportMap: options, + viewportMap: options as ViewportMap,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/viewport/components/Tool.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
PR: storybookjs/storybook#32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/viewport/components/Tool.tsx
⏰ 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). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
| setIsTooltipVisible: React.Dispatch<React.SetStateAction<boolean>>; | ||
| viewportMap: ViewportMap; | ||
| viewportName: any; | ||
| viewportName: keyof ViewportMap; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
viewportName is too strict; allow 'responsive' and undefined
viewportName can be unset or equal to the sentinel 'responsive'. Typing it as keyof ViewportMap introduces type errors and doesn't reflect runtime behavior.
Apply this diff:
- viewportName: keyof ViewportMap;
+ viewportName: keyof ViewportMap | 'responsive' | undefined;📝 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.
| viewportName: keyof ViewportMap; | |
| viewportName: keyof ViewportMap | 'responsive' | undefined; |
🤖 Prompt for AI Agents
In code/core/src/viewport/components/Tool.tsx around line 28, the prop type
viewportName is too strict as keyof ViewportMap; change its type to allow the
sentinel 'responsive' and undefined (e.g. viewportName?: keyof ViewportMap |
'responsive' or keyof ViewportMap | 'responsive' | undefined) and update any
local usage/guards to handle the 'responsive' and undefined cases (null-checks
or switch/if branches) so the runtime behavior and types align.
| const viewportName = typeof data === 'string' ? data : data.value; | ||
| const isRotated: boolean = typeof data === 'string' ? false : data.isRotated; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize globals shape; handle legacy string/empty object safely
globals?.[PARAM_KEY] || {} can produce {}, so data.value/data.isRotated may be undefined. Also, legacy string values may be 'responsive'. Normalize both.
- const viewportName = typeof data === 'string' ? data : data.value;
- const isRotated: boolean = typeof data === 'string' ? false : data.isRotated;
+ type ViewportKey = keyof ViewportMap | 'responsive' | undefined;
+ const viewportName: ViewportKey =
+ typeof data === 'string' ? (data as ViewportKey) : (data?.value as ViewportKey);
+ const isRotated = typeof data === 'string' ? false : Boolean(data?.isRotated);📝 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 viewportName = typeof data === 'string' ? data : data.value; | |
| const isRotated: boolean = typeof data === 'string' ? false : data.isRotated; | |
| type ViewportKey = keyof ViewportMap | 'responsive' | undefined; | |
| const viewportName: ViewportKey = | |
| typeof data === 'string' ? (data as ViewportKey) : (data?.value as ViewportKey); | |
| const isRotated = typeof data === 'string' ? false : Boolean(data?.isRotated); |
🤖 Prompt for AI Agents
In code/core/src/viewport/components/Tool.tsx around lines 45-46, normalize the
incoming globals shape and handle legacy string/empty-object cases: treat data
that may be a string (including legacy 'responsive') or an object (possibly {}),
coerce it into a safe object with explicit defaults for value and isRotated,
then read viewportName and isRotated from that normalized object; e.g. if typeof
data === 'string' create an object { value: data, isRotated: false }, if data is
null/{} use sensible defaults (value: undefined or a fallback string and
isRotated: false) so accessing .value and .isRotated never yields undefined.
| const item = (options as ViewportMap)[viewportName] || responsiveViewport; | ||
| const isActive = isTooltipVisible || item !== responsiveViewport; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard map access when key may be undefined/'responsive'
Indexing with an unvalidated key causes typing friction and possible surprises. Guard and fall back to responsiveViewport.
- const item = (options as ViewportMap)[viewportName] || responsiveViewport;
+ const map = options as ViewportMap;
+ const item =
+ viewportName && viewportName !== 'responsive'
+ ? map[viewportName as keyof ViewportMap] ?? responsiveViewport
+ : responsiveViewport;📝 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 item = (options as ViewportMap)[viewportName] || responsiveViewport; | |
| const isActive = isTooltipVisible || item !== responsiveViewport; | |
| const map = options as ViewportMap; | |
| const item = | |
| viewportName && viewportName !== 'responsive' | |
| ? map[viewportName as keyof ViewportMap] ?? responsiveViewport | |
| : responsiveViewport; | |
| const isActive = isTooltipVisible || item !== responsiveViewport; |
🤖 Prompt for AI Agents
In code/core/src/viewport/components/Tool.tsx around lines 48-49, the current
indexing (options as ViewportMap)[viewportName] may access the map with an
undefined or special key ('responsive'); change the logic to guard the lookup
and fall back to responsiveViewport: first test that viewportName is a valid
string key and exists on options (or use a safe optional lookup), then use
options[viewportName] only when that check passes, otherwise set item =
responsiveViewport; adjust types or narrow viewportName as needed so the
compiler knows the key is valid.
|
Sorry, I was swamped with work, thanks for getting it over the finish line. |
4 participants
What I did
This PR contains two changes:
Export default constants with
as constThe motivation for this, is that this ensures better type safety. A good example would be to pick a subset of the viewports.
It also makes the
InitialViewportKeysobsolete, because you can easily create it yourself. It would also allow to get the keys for theMINIMAL_VIEWPORTS, which currently is not possible. I’ve kept theInitialViewportKeysexport though, to avoid introducing breaking changes.Fix the listed viewports in the documentation
While looking at the exports, I’ve noticed that the documentation is not properly reflecting the export:
desktopviewport is not listed in the list for theMINIMAL_VIEWPORTSINITIAL_VIEWPORTSlist includes the entries from theMINIMAL_VIEWPORTS, but they are not part of the exportipad10pdimensions where not matching the dimensions from the exportI’ve adjusted the docs to be in-sync with the exports.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
The changes are jut type-changes, there are no runtime changes.
MINIMAL_VIEWPORTSandINITIAL_VIEWPORTSfromstorybook/viewportsDocumentation
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 canary-release-pr.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Bug Fixes
Documentation