Repository navigation
Review: Dynamic thumbnail scaling based on content dimensions - #35282
Conversation
When embed=true, measure story content and post iframe.resize messages to the parent frame. Wire a final measurement into the story freezer via onFrozen for stable thumbnail dimensions after freeze completes.
Review thumbnails opt into content-dimension broadcast via embed=true, emitted before freeze=finished when both params are requested.
Listen for embed preview postMessages in CollectionGrid and derive per-cell scale and aspect ratio from measured content via computeThumbnailLayout.
Request embed=true alongside freeze=finished when building review thumbnail preview URLs and update story mocks to match.
|
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:
📝 WalkthroughWalkthroughThis PR adds ChangesEmbedded preview resize flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
code/core/src/preview-api/modules/preview-web/setupContentResizeBroadcast.test.ts (1)
22-40: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse Vitest global stubs instead of mutating
windowdirectly.This test overwrites
window.parentmanually and installs theResizeObservermock inside the test body. That makes cleanup depend on custom restoration logic and is exactly the kind of global state pollution the repo rules are trying to avoid. Prefervi.stubGlobal()inbeforeEach()andvi.unstubAllGlobals()inafterEach().As per coding guidelines, "Never assign
globalThis.*directly in tests ... Use Vitest'svi.stubGlobal()...vi.unstubAllGlobals()inafterEach()" and "Implement mock behaviors inbeforeEachblocks in Vitest tests."Also applies to: 62-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/preview-api/modules/preview-web/setupContentResizeBroadcast.test.ts` around lines 22 - 40, The test is mutating window globals directly, including window.parent and the ResizeObserver mock setup, which should be managed through Vitest stubs instead. Update setupContentResizeBroadcast.test.ts to use vi.stubGlobal() in beforeEach for window.parent and any global mocks, and call vi.unstubAllGlobals() in afterEach to restore state cleanly. Keep the mock behavior setup near the existing beforeEach/afterEach test hooks so the test stays isolated and avoids manual Object.defineProperty cleanup.Source: Coding guidelines
code/core/src/manager-api/tests/url.test.js (1)
641-655: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert params, not query-string ordering.
This test is locking in
embedbeforefreeze, but the behavior that matters is that both params are present onpreviewHref. Parsing the query here would make the test survive harmless URL-builder refactors.Suggested test shape
- it('appends embed before freeze when both are set', () => { + it('includes both embed and freeze when both are set', () => { const { api, state } = initURL({ store, provider: { channel: new EventEmitter() }, state: { location: { pathname: '/', search: '' } }, navigate: vi.fn(), fullAPI: { getCurrentStoryData: () => ({ id: 'test--story' }) }, }); store.setState(state); const { previewHref } = api.getStoryHrefs('test--story', { embed: true, freeze: true }); - expect(previewHref).toEqual( - '/iframe.html?id=test--story&viewMode=story&embed=true&freeze=finished' - ); + const url = new URL(previewHref, 'http://localhost'); + expect(url.pathname).toBe('/iframe.html'); + expect(url.searchParams.get('id')).toBe('test--story'); + expect(url.searchParams.get('viewMode')).toBe('story'); + expect(url.searchParams.get('embed')).toBe('true'); + expect(url.searchParams.get('freeze')).toBe('finished'); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/manager-api/tests/url.test.js` around lines 641 - 655, This test is asserting a specific query-string order in getStoryHrefs, but it should only verify that previewHref contains both embed and freeze parameters. Update the test around initURL/getStoryHrefs so it parses previewHref and asserts the presence and values of the params instead of comparing the full URL string, keeping the coverage focused on the API behavior rather than ordering.code/core/src/manager/components/review/components/computeThumbnailLayout.test.ts (1)
1-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct coverage for
parseIframeResizeMessage.The new listener path depends on malformed-message rejection, but this suite only locks down the layout buckets. Add a few cases for string/object payloads, wrong
context, bad JSON, and invalid dimensions so the resize message contract is tested directly. As per coding guidelines,**/*.test.ts?(x): "For unit tests of utilities, hooks, and non-React modules, export functions that need testing, test real behavior (not syntax patterns)."Minimal expansion
-import { computeThumbnailLayout } from './computeThumbnailLayout.ts'; +import { + computeThumbnailLayout, + IFRAME_RESIZE_CONTEXT, + parseIframeResizeMessage, +} from './computeThumbnailLayout.ts'; + +describe('parseIframeResizeMessage', () => { + it('accepts valid resize payloads', () => { + expect( + parseIframeResizeMessage( + JSON.stringify({ context: IFRAME_RESIZE_CONTEXT, width: 320, height: 240 }) + ) + ).toEqual({ width: 320, height: 240 }); + }); + + it('rejects malformed payloads', () => { + expect(parseIframeResizeMessage('{')).toBeNull(); + expect(parseIframeResizeMessage({ context: 'other', width: 320, height: 240 })).toBeNull(); + expect(parseIframeResizeMessage({ context: IFRAME_RESIZE_CONTEXT, width: -1, height: 240 })).toBeNull(); + }); +});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/manager/components/review/components/computeThumbnailLayout.test.ts` around lines 1 - 48, The current test suite only covers computeThumbnailLayout, but the new listener behavior relies on parseIframeResizeMessage rejecting malformed resize payloads. Add direct unit coverage for parseIframeResizeMessage with valid string/object inputs and invalid cases like wrong context, bad JSON, and non-positive or missing dimensions, so the message contract is tested explicitly. Use the existing computeThumbnailLayout and parseIframeResizeMessage symbols to locate the related utility and keep the tests focused on real behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/core/src/manager-api/modules/url.ts`:
- Around line 297-302: The URL builder in manager-api/modules/url.ts is mixing
preview-only flags into the generic query string, which can duplicate
embed/freeze and leak them into managerHref. Update the logic around the URL
assembly in the return object so that queryParams are filtered to exclude embed
and freeze, and only the explicit embedParam/freezeParam pieces are appended to
previewHref. Use the existing symbols managerHref, previewHref, embedParam,
freezeParam, and queryParams handling to keep the fix localized.
In
`@code/core/src/manager/components/review/components/computeThumbnailLayout.ts`:
- Around line 38-45: Reject invalid resize payload dimensions in
computeThumbnailLayout by tightening the parsed width/height checks: after
JSON.parse and the IFRAME_RESIZE_CONTEXT guard, ensure both parsed.width and
parsed.height are finite and greater than zero before returning the layout
object. Update the validation in computeThumbnailLayout so malformed
iframe.resize messages with NaN, Infinity, zero, or negative values are treated
as null instead of being passed through to thumbnail sizing.
In `@code/core/src/preview-api/modules/preview-web/setupStoryFreezer.test.ts`:
- Around line 266-269: The test for setupStoryFreezer only waits for a
microtask, so it passes only when the queueMicrotask path is used. Update the
assertion flow after channel.emit(STORY_RENDER_PHASE_CHANGED, ...) to wait in a
way that covers both scheduling branches, so the expectation on onFrozen in
setupStoryFreezer.test remains valid whether setupStoryFreezer uses
queueMicrotask or setTimeout(..., 0).
In `@code/core/src/preview-api/modules/preview-web/setupStoryFreezer.ts`:
- Around line 300-309: The STORY_RENDER_PHASE_CHANGED handler in
setupStoryFreezer currently calls options.onFrozen every time newPhase becomes
finished, even after freezer.freeze() has already no-op’d. Update the runFreeze
flow so onFrozen is only invoked on the first successful freeze, and keep the
guard close to the existing freezer.freeze() logic so repeated finished events
do not trigger duplicate callbacks or resize notifications.
---
Nitpick comments:
In `@code/core/src/manager-api/tests/url.test.js`:
- Around line 641-655: This test is asserting a specific query-string order in
getStoryHrefs, but it should only verify that previewHref contains both embed
and freeze parameters. Update the test around initURL/getStoryHrefs so it parses
previewHref and asserts the presence and values of the params instead of
comparing the full URL string, keeping the coverage focused on the API behavior
rather than ordering.
In
`@code/core/src/manager/components/review/components/computeThumbnailLayout.test.ts`:
- Around line 1-48: The current test suite only covers computeThumbnailLayout,
but the new listener behavior relies on parseIframeResizeMessage rejecting
malformed resize payloads. Add direct unit coverage for parseIframeResizeMessage
with valid string/object inputs and invalid cases like wrong context, bad JSON,
and non-positive or missing dimensions, so the message contract is tested
explicitly. Use the existing computeThumbnailLayout and parseIframeResizeMessage
symbols to locate the related utility and keep the tests focused on real
behavior.
In
`@code/core/src/preview-api/modules/preview-web/setupContentResizeBroadcast.test.ts`:
- Around line 22-40: The test is mutating window globals directly, including
window.parent and the ResizeObserver mock setup, which should be managed through
Vitest stubs instead. Update setupContentResizeBroadcast.test.ts to use
vi.stubGlobal() in beforeEach for window.parent and any global mocks, and call
vi.unstubAllGlobals() in afterEach to restore state cleanly. Keep the mock
behavior setup near the existing beforeEach/afterEach test hooks so the test
stays isolated and avoids manual Object.defineProperty cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd7bd0dd-30df-473e-b141-5e9a778c6659
📒 Files selected for processing (15)
code/core/src/manager-api/modules/url.tscode/core/src/manager-api/tests/url.test.jscode/core/src/manager/components/review/ReviewPage.stories.tsxcode/core/src/manager/components/review/ReviewProvider.tsxcode/core/src/manager/components/review/ReviewToolbarHeader.stories.tsxcode/core/src/manager/components/review/components/CollectionGrid.stories.tsxcode/core/src/manager/components/review/components/CollectionGrid.tsxcode/core/src/manager/components/review/components/computeThumbnailLayout.test.tscode/core/src/manager/components/review/components/computeThumbnailLayout.tscode/core/src/manager/components/review/screens/SummaryScreen.stories.tsxcode/core/src/preview-api/modules/preview-web/PreviewWithSelection.tsxcode/core/src/preview-api/modules/preview-web/setupContentResizeBroadcast.test.tscode/core/src/preview-api/modules/preview-web/setupContentResizeBroadcast.tscode/core/src/preview-api/modules/preview-web/setupStoryFreezer.test.tscode/core/src/preview-api/modules/preview-web/setupStoryFreezer.ts
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 72 | 72 | 0 |
| Self size | 21.90 MB | 21.91 MB | 🚨 +12 KB 🚨 |
| Dependency size | 36.44 MB | 36.44 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 204 | 204 | 0 |
| Self size | 821 KB | 821 KB | 🚨 +84 B 🚨 |
| Dependency size | 91.68 MB | 91.70 MB | 🚨 +12 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 197 | 197 | 0 |
| Self size | 32 KB | 32 KB | 🚨 +36 B 🚨 |
| Dependency size | 90.17 MB | 90.18 MB | 🚨 +12 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 73 | 73 | 0 |
| Self size | 1.09 MB | 1.09 MB | 🚨 +66 B 🚨 |
| Dependency size | 58.34 MB | 58.35 MB | 🚨 +12 KB 🚨 |
| Bundle Size Analyzer | node | node |
Measure content bounds with symmetric runtime body padding, skip pass-through wrappers and viewport underlays, and size CollectionGrid frames from iframe.resize via CSS variables and subgrid alignment.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@code/core/src/manager/components/review/components/CollectionGrid.stories.tsx`:
- Around line 170-177: The CollectionGrid story test is capturing neighbor
heights too early, so the waitFor loop keeps checking a stale snapshot instead
of fresh layout values. Update the logic around rowNeighborHeights to read the
cell heights inside the waitFor callback, then compute or re-read the final
heights after layout settles before asserting they match reviewAllHeight.
In
`@code/core/src/preview-api/modules/preview-web/setupContentResizeBroadcast.test.ts`:
- Around line 83-86: The tests in setupContentResizeBroadcast.test.ts are
directly mutating global window properties instead of using Vitest’s global
stubbing helpers. Update the affected setup blocks and cases that touch
innerWidth, innerHeight, parent, and ResizeObserver to use vi.stubGlobal(...) so
the test environment is isolated, and add vi.unstubAllGlobals() in afterEach()
to restore globals consistently between cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3cfaef2-77d1-4359-9813-98117f0cfad0
📒 Files selected for processing (9)
code/core/src/manager/components/review/ReviewProvider.tsxcode/core/src/manager/components/review/ReviewToolbarHeader.stories.tsxcode/core/src/manager/components/review/components/CollectionGrid.stories.tsxcode/core/src/manager/components/review/components/CollectionGrid.tsxcode/core/src/manager/components/review/components/computeThumbnailLayout.tscode/core/src/manager/components/review/components/iframeResizeMessage.tscode/core/src/manager/components/review/screens/SummaryScreen.stories.tsxcode/core/src/preview-api/modules/preview-web/setupContentResizeBroadcast.test.tscode/core/src/preview-api/modules/preview-web/setupContentResizeBroadcast.ts
✅ Files skipped from review due to trivial changes (1)
- code/core/src/manager/components/review/components/iframeResizeMessage.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- code/core/src/manager/components/review/ReviewProvider.tsx
- code/core/src/manager/components/review/ReviewToolbarHeader.stories.tsx
Compute a pixel min-height from reported iframe dimensions so subgrid rows expand to the correct 3/n aspect bucket instead of clipping tall stories.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
code/core/src/manager/components/review/components/CollectionGrid.tsx (1)
490-499: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSkip no-op resize updates.
The preview now emits a final resize after freeze, so identical
iframe.resizepayloads will still allocate a new object and rerender this cell. Guarding the setter by width/height avoids unnecessary grid churn when many embeds settle at once.Proposed change
- setRememberedDimensions(dimensions); + setRememberedDimensions((current) => + current?.width === dimensions.width && current?.height === dimensions.height + ? current + : dimensions + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/manager/components/review/components/CollectionGrid.tsx` around lines 490 - 499, Skip no-op resize updates in the CollectionGrid message handler by comparing the parsed iframe dimensions before calling setRememberedDimensions. In the handleMessage logic that uses parseIframeResizeMessage and setRememberedDimensions, add a width/height equality guard so identical iframe.resize payloads do not create a new object and trigger unnecessary rerenders.code/core/src/manager/components/review/components/iframeResizeMessage.test.ts (1)
3-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd parser contract coverage here too.
These cases lock down the sizing math, but not
parseIframeResizeMessage, which is the gateCollectionGriddepends on before any layout update happens. A few valid/invalid payload cases here would catch silent breakage in the manager/preview message format.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/manager/components/review/components/iframeResizeMessage.test.ts` around lines 3 - 27, The current test file covers the sizing helpers but misses parser contract coverage for parseIframeResizeMessage, which is the entry point CollectionGrid relies on before applying layout updates. Add tests in iframeResizeMessage.test.ts for parseIframeResizeMessage that exercise a few valid and invalid payload shapes, including the expected manager/preview message format, so regressions in message parsing are caught alongside the existing computeThumbnailScale, computeThumbnailThirds, and computeThumbnailMinHeight cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@code/core/src/manager/components/review/components/CollectionGrid.tsx`:
- Around line 490-499: Skip no-op resize updates in the CollectionGrid message
handler by comparing the parsed iframe dimensions before calling
setRememberedDimensions. In the handleMessage logic that uses
parseIframeResizeMessage and setRememberedDimensions, add a width/height
equality guard so identical iframe.resize payloads do not create a new object
and trigger unnecessary rerenders.
In
`@code/core/src/manager/components/review/components/iframeResizeMessage.test.ts`:
- Around line 3-27: The current test file covers the sizing helpers but misses
parser contract coverage for parseIframeResizeMessage, which is the entry point
CollectionGrid relies on before applying layout updates. Add tests in
iframeResizeMessage.test.ts for parseIframeResizeMessage that exercise a few
valid and invalid payload shapes, including the expected manager/preview message
format, so regressions in message parsing are caught alongside the existing
computeThumbnailScale, computeThumbnailThirds, and computeThumbnailMinHeight
cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75f33aea-bc01-4df5-98fd-835297384ec1
📒 Files selected for processing (3)
code/core/src/manager/components/review/components/CollectionGrid.tsxcode/core/src/manager/components/review/components/iframeResizeMessage.test.tscode/core/src/manager/components/review/components/iframeResizeMessage.ts
Extract preview scheduler, thumbnail hook, and shared types; use a single resize message listener and JS-only minHeight for subgrid row growth while CSS handles visual scaling.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/core/src/manager/components/review/components/usePreviewThumbnail.ts`:
- Around line 211-215: The thumbnail URL is being cached in usePreviewThumbnail,
so changes to getPreviewHref() do not refresh the iframe once src is already
set. Update the start handler in the PreviewTask so it can replace the existing
src when the computed previewHref changes, rather than only setting it when
current is null. Use the usePreviewThumbnail and PreviewTask.start symbols to
locate the logic and make the iframe reload on href changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c2a6f96b-7951-4cb6-b05e-cdb2d30f3345
📒 Files selected for processing (14)
code/core/src/manager/components/review/ReviewCollectionPicker.tsxcode/core/src/manager/components/review/ReviewProvider.tsxcode/core/src/manager/components/review/components/CollectionGrid.stories.tsxcode/core/src/manager/components/review/components/CollectionGrid.tsxcode/core/src/manager/components/review/components/iframeResizeMessage.tscode/core/src/manager/components/review/components/previewScheduler.tscode/core/src/manager/components/review/components/usePreviewThumbnail.tscode/core/src/manager/components/review/review-store.tscode/core/src/manager/components/review/review-types.tscode/core/src/manager/components/review/screens/SummaryScreen.stories.tsxcode/core/src/manager/components/review/screens/SummaryScreen.tsxcode/core/src/preview-api/modules/preview-web/setupContentResizeBroadcast.test.tscode/core/src/preview-api/modules/preview-web/setupContentResizeBroadcast.tscode/core/src/shared/constants/iframe-resize.ts
✅ Files skipped from review due to trivial changes (4)
- code/core/src/manager/components/review/review-types.ts
- code/core/src/manager/components/review/review-store.ts
- code/core/src/shared/constants/iframe-resize.ts
- code/core/src/manager/components/review/screens/SummaryScreen.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- code/core/src/manager/components/review/screens/SummaryScreen.stories.tsx
- code/core/src/manager/components/review/ReviewProvider.tsx
- code/core/src/preview-api/modules/preview-web/setupContentResizeBroadcast.test.ts
- code/core/src/manager/components/review/components/CollectionGrid.stories.tsx
Drop dynamic aspect-ratio buckets and subgrid minHeight resizing; preview frames stay 3/2 while scale fits measured content width and height.
Re-read cell heights inside waitFor so layout assertions track live DOM values instead of a pre-layout snapshot.
Always set iframe src from the latest preview URL and skip no-op resize dimension updates to avoid stale thumbnails and unnecessary rerenders.
Guard repeated finished-phase events and wait with vi.waitFor so the freezer callback test covers both microtask and timeout scheduling paths.
Filter preview-only flags from manager and preview query strings and assert param presence in tests instead of URL ordering.
Require finite positive width and height before applying thumbnail sizing and add parser contract tests.
Replace direct window mutations with vi.stubGlobal and vi.unstubAllGlobals for consistent test isolation.
Summary
setupContentResizeBroadcastin preview: whenembed=true, measure story content bounds (skipping pass-through wrappers and viewport underlays) and postiframe.resizemessages to the parent frame, with a final measurement after freeze viaonFrozenembedoption togetStoryHrefs(preview-only; filtered from generic query params); review thumbnails requestembed=true&freeze=finishedusePreviewThumbnailandpreviewSchedulerfromCollectionGrid; a single global resize listener routesiframe.resizemessages to the matching iframe--scalefits content width (≤300px → 100%, wider → stepped down to 50%); tall stories are cropped rather than shrunk to fit frame heightreview-types.tsTest plan
🦋 Canary release
This pull request has been released as version
0.0.0-pr-35282-sha-ac39c7d8. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-35282-sha-ac39c7d8 sandboxor in an existing project withnpx storybook@0.0.0-pr-35282-sha-ac39c7d8 upgrade.More information
0.0.0-pr-35282-sha-ac39c7d8ghengeveld/review-thumbnail-content-resizeac39c7d81782486018)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=35282Summary by CodeRabbit