Viewport: Prioritize story viewport globals and avoid user-global pollution#33849
Conversation
…ort pollution - prioritize story viewport globals when story viewport globals exist - remove story-to-user viewport global synchronization - reset invalid viewport options only when no story viewport global exists
📝 WalkthroughWalkthroughReworks viewport value resolution in the useViewport hook: introduces storyHasViewport plus primary/secondary global precedence, removes the unconditional reset on storyGlobals updates, and changes invalid-option handling to reset to defaults only when no story-level viewport exists. Changes
Sequence Diagram(s)(omitted — changes are localized and do not introduce a new multi-component sequential flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing Touches
Comment |
valentinpalkovic
moved this from Empathy Queue (prioritized)
to In Progress
in Core Team Projects
|
Hi @ia319, Thank you for your contribution! The story |
…story-global-precedence
…als to restore isRotated fallback
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/useViewport.ts (1)
84-95:⚠️ Potential issue | 🟡 MinorUse defined-value check to distinguish between unset and explicitly undefined viewports.
The
inoperator returns true even when a property is explicitly set toundefined. If a story setsglobals: { viewport: undefined }to clear a viewport override,PARAM_KEY in storyGlobalswill still evaluate true, treating it as if a viewport is set. This causes inconsistent behavior: the viewport locks even though the story hasn't meaningfully configured one.Check for defined values instead:
storyGlobals[PARAM_KEY] !== undefined. This aligns with the pattern already used in the codebase (e.g., VisionSimulator addon) and ensures undefined properties are treated as absent.🛠️ Suggested changes
- const storyHasViewport = PARAM_KEY in storyGlobals; + const storyHasViewport = storyGlobals[PARAM_KEY] !== undefined;- const isLocked = disable || PARAM_KEY in storyGlobals || !keys.length; + const isLocked = disable || storyHasViewport || !keys.length;- if (!(PARAM_KEY in storyGlobals)) { + if (storyGlobals[PARAM_KEY] === undefined) {Also applies to: 198-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/viewport/useViewport.ts` around lines 84 - 95, The code uses the in operator to detect a story-level viewport via "PARAM_KEY in storyGlobals", which treats explicit undefined as present; change these checks to explicit defined-value checks (e.g., storyGlobals[PARAM_KEY] !== undefined) wherever you currently use "in" for viewport detection (notably around the computation of storyHasViewport/primaryGlobal/secondaryGlobal/isLocked and the similar block at the later 198-206 region) so that explicitly undefined globals are treated as absent and the viewport lock logic (isLocked and primary/secondary selection) behaves correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@code/core/src/viewport/useViewport.ts`:
- Around line 84-95: The code uses the in operator to detect a story-level
viewport via "PARAM_KEY in storyGlobals", which treats explicit undefined as
present; change these checks to explicit defined-value checks (e.g.,
storyGlobals[PARAM_KEY] !== undefined) wherever you currently use "in" for
viewport detection (notably around the computation of
storyHasViewport/primaryGlobal/secondaryGlobal/isLocked and the similar block at
the later 198-206 region) so that explicitly undefined globals are treated as
absent and the viewport lock logic (isLocked and primary/secondary selection)
behaves correctly.
|
@valentinpalkovic Now it should be fine. I removed the incorrect default |
valentinpalkovic
added
the
patch:yes
github-project-automation
Bot
moved this from In Progress
to Done
in Core Team Projects
yannbf
removed
the
patch:yes
valentinpalkovic
added
the
needs qa
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 18 | 18 | 0 |
| Self size | 1.66 MB | 1.64 MB | 🎉 -17 KB 🎉 |
| Dependency size | 9.25 MB | 9.25 MB | 🎉 -219 B 🎉 |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 49 | 49 | 0 |
| Self size | 20.21 MB | 20.42 MB | 🚨 +207 KB 🚨 |
| Dependency size | 16.52 MB | 16.52 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 183 | 183 | 0 |
| Self size | 779 KB | 779 KB | 🎉 -13 B 🎉 |
| Dependency size | 67.35 MB | 67.56 MB | 🚨 +208 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 176 | 176 | 0 |
| Self size | 32 KB | 32 KB | 🚨 +36 B 🚨 |
| Dependency size | 65.88 MB | 66.08 MB | 🚨 +207 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 1.04 MB | 1.04 MB | 🚨 +905 B 🚨 |
| Dependency size | 36.73 MB | 36.94 MB | 🚨 +207 KB 🚨 |
| Bundle Size Analyzer | node | node |
Sidnioulz
removed
the
needs qa
5 participants
Closes #33831
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Copy the
previewandstoriescode from the issue reproduction link into the sandbox.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