From 42c49f1ed2e28fe99f61cbddc0d68110dd887cd6 Mon Sep 17 00:00:00 2001 From: Gert Hengeveld Date: Thu, 2 Jul 2026 13:07:40 +0200 Subject: [PATCH 01/11] Move iframe.resize parser next to its payload type and fold frame styles into CollectionGrid The review thumbnail message layer had a renaming re-export module (iframeResizeMessage.ts) that gave the shared iframe.resize wire protocol a second vocabulary. The parser now lives in shared/constants/iframe-resize.ts next to the payload type it validates, and the frame-style helpers live in CollectionGrid next to the Frame styled component that consumes their CSS variables. Also drops the tautological THUMBNAIL_BOOTSTRAP_SCALE constant test. Co-Authored-By: Claude Fable 5 --- .../review/components/CollectionGrid.tsx | 46 ++++++-- .../components/iframeResizeMessage.test.ts | 93 ---------------- .../review/components/iframeResizeMessage.ts | 100 ------------------ .../review/components/usePreviewThumbnail.ts | 22 ++-- .../shared/constants/iframe-resize.test.ts | 66 +++++++++++- .../src/shared/constants/iframe-resize.ts | 61 +++++++++++ 6 files changed, 175 insertions(+), 213 deletions(-) delete mode 100644 code/core/src/manager/components/review/components/iframeResizeMessage.test.ts delete mode 100644 code/core/src/manager/components/review/components/iframeResizeMessage.ts diff --git a/code/core/src/manager/components/review/components/CollectionGrid.tsx b/code/core/src/manager/components/review/components/CollectionGrid.tsx index 8156fba702b1..ddf5f0d5b574 100644 --- a/code/core/src/manager/components/review/components/CollectionGrid.tsx +++ b/code/core/src/manager/components/review/components/CollectionGrid.tsx @@ -1,14 +1,13 @@ -import React, { type FC } from 'react'; +import React, { type CSSProperties, type FC } from 'react'; import { Badge, Button, Loader } from 'storybook/internal/components'; import { styled } from 'storybook/theming'; -import { fallbackStoryInfo, type StoryInfo } from '../review-types.ts'; import { - DEFAULT_CONTENT_WIDTH, - getPreviewFrameStyle, - hasFixedViewportAspect, -} from './iframeResizeMessage.ts'; + hasFixedViewportDimensions, + type IframeResizeDimensions, +} from '../../../../shared/constants/iframe-resize.ts'; +import { fallbackStoryInfo, type StoryInfo } from '../review-types.ts'; import { usePreviewThumbnail } from './usePreviewThumbnail.ts'; // Per-breakpoint grid: `cols` columns (each cell clamped to 400px) capped at @@ -60,6 +59,37 @@ const FrameShell = styled.div({ position: 'relative', }); +/** Default `--content-w` before the embed iframe reports its size. */ +const DEFAULT_CONTENT_WIDTH = 300; + +/** Pre-measurement scale so the embed iframe viewport is 2× the frame width (100% / 0.5). */ +const THUMBNAIL_BOOTSTRAP_SCALE = 0.5; + +// Feeds the CSS variables consumed by `Frame` below (`--scale`, `--content-w`, +// `--vp-w`/`--vp-h`); `viewportFill` toggles the `data-viewport-fill` branch. +const getPreviewFrameLayout = ( + dimensions: IframeResizeDimensions | null +): { style: CSSProperties; viewportFill: boolean } => { + if (!dimensions) { + return { + style: { '--scale': THUMBNAIL_BOOTSTRAP_SCALE } as CSSProperties, + viewportFill: false, + }; + } + + if (hasFixedViewportDimensions(dimensions.viewport)) { + return { + style: { + '--vp-w': dimensions.viewport.width, + '--vp-h': dimensions.viewport.height, + } as CSSProperties, + viewportFill: true, + }; + } + + return { style: { '--content-w': dimensions.width } as CSSProperties, viewportFill: false }; +}; + const Frame = styled.a(({ theme }) => ({ position: 'absolute', inset: 0, @@ -215,9 +245,7 @@ const StoryPreviewCell: FC<{ } = usePreviewThumbnail({ storyId, getPreviewHref, previewsPaused }); const { component, name } = deriveStoryInfo(info); - const viewport = rememberedDimensions?.viewport; - const viewportFill = hasFixedViewportAspect(viewport); - const frameStyle = getPreviewFrameStyle(rememberedDimensions); + const { style: frameStyle, viewportFill } = getPreviewFrameLayout(rememberedDimensions); const preview = src ? (
diff --git a/code/core/src/manager/components/review/components/iframeResizeMessage.test.ts b/code/core/src/manager/components/review/components/iframeResizeMessage.test.ts deleted file mode 100644 index 353d8260f077..000000000000 --- a/code/core/src/manager/components/review/components/iframeResizeMessage.test.ts +++ /dev/null @@ -1,93 +0,0 @@ -import { describe, expect, it } from 'vitest'; - -import { IFRAME_RESIZE_CONTEXT } from '../../../../shared/constants/iframe-resize.ts'; -import { - getPreviewFrameStyle, - parseIframeResizeMessage, - THUMBNAIL_BOOTSTRAP_SCALE, -} from './iframeResizeMessage.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 }); - expect( - parseIframeResizeMessage({ context: IFRAME_RESIZE_CONTEXT, width: 320, height: 240 }) - ).toEqual({ width: 320, height: 240 }); - expect( - parseIframeResizeMessage({ - context: IFRAME_RESIZE_CONTEXT, - width: 320, - height: 240, - viewport: { name: 'Small mobile', value: 'mobile1', width: 320, height: 568 }, - }) - ).toEqual({ - width: 320, - height: 240, - viewport: { name: 'Small mobile', value: 'mobile1', width: 320, height: 568 }, - }); - }); - - 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(); - expect( - parseIframeResizeMessage({ context: IFRAME_RESIZE_CONTEXT, width: 0, height: 240 }) - ).toBeNull(); - expect( - parseIframeResizeMessage({ context: IFRAME_RESIZE_CONTEXT, width: NaN, height: 240 }) - ).toBeNull(); - expect( - parseIframeResizeMessage({ - context: IFRAME_RESIZE_CONTEXT, - width: 320, - height: 240, - viewport: { name: 'Small mobile', value: 'mobile1' }, - }) - ).toEqual({ - width: 320, - height: 240, - viewport: { name: 'Small mobile', value: 'mobile1' }, - }); - expect( - parseIframeResizeMessage({ - context: IFRAME_RESIZE_CONTEXT, - width: 320, - height: 240, - viewport: { value: 'mobile1', width: 320, height: 568 }, - }) - ).toBeNull(); - }); -}); - -describe('getPreviewFrameStyle', () => { - it('bootstraps before resize', () => { - expect(getPreviewFrameStyle(null)).toEqual({ '--scale': THUMBNAIL_BOOTSTRAP_SCALE }); - }); - - it('uses viewport dimensions for fixed viewports', () => { - expect( - getPreviewFrameStyle({ - width: 120, - height: 48, - viewport: { name: 'Small mobile', value: 'mobile1', width: 320, height: 568 }, - }) - ).toEqual({ '--vp-w': 320, '--vp-h': 568 }); - }); - - it('uses content width for responsive viewports', () => { - expect(getPreviewFrameStyle({ width: 320, height: 240 })).toEqual({ '--content-w': 320 }); - }); -}); - -describe('THUMBNAIL_BOOTSTRAP_SCALE', () => { - it('widens the embed viewport to 2× the frame width before measurement', () => { - expect(THUMBNAIL_BOOTSTRAP_SCALE).toBe(0.5); - }); -}); diff --git a/code/core/src/manager/components/review/components/iframeResizeMessage.ts b/code/core/src/manager/components/review/components/iframeResizeMessage.ts deleted file mode 100644 index 64488e30c58f..000000000000 --- a/code/core/src/manager/components/review/components/iframeResizeMessage.ts +++ /dev/null @@ -1,100 +0,0 @@ -import type { CSSProperties } from 'react'; - -import { - IFRAME_RESIZE_CONTEXT, - hasFixedViewportDimensions, - iframeResizeDimensionsEqual, - iframeResizeViewportsEqual, - type IframeResizeDimensions, - type IframeResizeViewport, -} from '../../../../shared/constants/iframe-resize.ts'; - -/** Default `--content-w` before the embed iframe reports its size. */ -export const DEFAULT_CONTENT_WIDTH = 300; - -export type ContentDimensions = IframeResizeDimensions; - -export { - iframeResizeDimensionsEqual as contentDimensionsEqual, - hasFixedViewportDimensions as hasFixedViewportAspect, - iframeResizeViewportsEqual, - type IframeResizeViewport, -}; - -const isPositiveFinite = (value: unknown): value is number => - typeof value === 'number' && Number.isFinite(value) && value > 0; - -const parseViewport = (value: unknown): IframeResizeViewport | undefined => { - if (!value || typeof value !== 'object') { - return undefined; - } - - const viewport = value as Partial; - if (typeof viewport.name !== 'string' || typeof viewport.value !== 'string') { - return undefined; - } - - const parsed: IframeResizeViewport = { - name: viewport.name, - value: viewport.value, - }; - - if (viewport.width !== undefined) { - if (!isPositiveFinite(viewport.width)) { - return undefined; - } - parsed.width = viewport.width; - } - - if (viewport.height !== undefined) { - if (!isPositiveFinite(viewport.height)) { - return undefined; - } - parsed.height = viewport.height; - } - - return parsed; -}; - -export const parseIframeResizeMessage = (data: unknown): ContentDimensions | null => { - try { - const parsed = typeof data === 'string' ? JSON.parse(data) : data; - if (parsed?.context !== IFRAME_RESIZE_CONTEXT) { - return null; - } - if (!isPositiveFinite(parsed.width) || !isPositiveFinite(parsed.height)) { - return null; - } - - const viewport = parseViewport(parsed.viewport); - if (parsed.viewport !== undefined && viewport === undefined) { - return null; - } - - return { - width: parsed.width, - height: parsed.height, - ...(viewport ? { viewport } : {}), - }; - } catch { - return null; - } -}; - -/** Pre-measurement scale so the embed iframe viewport is 2× the frame width (100% / 0.5). */ -export const THUMBNAIL_BOOTSTRAP_SCALE = 0.5; - -export const getPreviewFrameStyle = (dimensions: ContentDimensions | null): CSSProperties => { - if (!dimensions) { - return { '--scale': THUMBNAIL_BOOTSTRAP_SCALE } as CSSProperties; - } - - if (hasFixedViewportDimensions(dimensions.viewport)) { - return { - '--vp-w': dimensions.viewport.width, - '--vp-h': dimensions.viewport.height, - } as CSSProperties; - } - - return { '--content-w': dimensions.width } as CSSProperties; -}; diff --git a/code/core/src/manager/components/review/components/usePreviewThumbnail.ts b/code/core/src/manager/components/review/components/usePreviewThumbnail.ts index b2789048c792..dd07c797c58a 100644 --- a/code/core/src/manager/components/review/components/usePreviewThumbnail.ts +++ b/code/core/src/manager/components/review/components/usePreviewThumbnail.ts @@ -8,12 +8,12 @@ import { type RefObject, } from 'react'; -import { IFRAME_RESIZE_REQUEST_CONTEXT } from '../../../../shared/constants/iframe-resize.ts'; import { - contentDimensionsEqual, + IFRAME_RESIZE_REQUEST_CONTEXT, + iframeResizeDimensionsEqual, parseIframeResizeMessage, - type ContentDimensions, -} from './iframeResizeMessage.ts'; + type IframeResizeDimensions, +} from '../../../../shared/constants/iframe-resize.ts'; import { PREVIEW_SETTLE_TIMEOUT_MS, enqueuePreview, @@ -30,7 +30,7 @@ const PREVIEW_SCALE_SETTLE_FALLBACK_MS = PREVIEW_SETTLE_TIMEOUT_MS * 3; const PREVIEW_MOUNT_ROOT_MARGIN = '50% 0px'; const PREVIEW_EVICT_ROOT_MARGIN = '150% 0px'; -const resizeHandlers = new Map void>(); +const resizeHandlers = new Map void>(); let resizeMessageListening = false; const ensureResizeMessageListener = (): void => { @@ -57,7 +57,7 @@ const ensureResizeMessageListener = (): void => { const registerResizeHandler = ( contentWindow: Window, - handler: (dimensions: ContentDimensions) => void + handler: (dimensions: IframeResizeDimensions) => void ): (() => void) => { ensureResizeMessageListener(); resizeHandlers.set(contentWindow, handler); @@ -128,7 +128,7 @@ export type UsePreviewThumbnailResult = { src: string | undefined; /** True until iframe.resize arrives and the measured scale has painted. */ isPreviewLoading: boolean; - rememberedDimensions: ContentDimensions | null; + rememberedDimensions: IframeResizeDimensions | null; forceStartCurrent: () => void; finishCurrent: () => void; }; @@ -147,7 +147,9 @@ export const usePreviewThumbnail = ({ const previewsPausedRef = useRef(previewsPaused); previewsPausedRef.current = previewsPaused; const [isInView, setIsInView] = useState(false); - const [rememberedDimensions, setRememberedDimensions] = useState(null); + const [rememberedDimensions, setRememberedDimensions] = useState( + null + ); const [isPreviewLoading, setIsPreviewLoading] = useState(false); const [src, setSrc] = useState(undefined); const taskRef = useRef(null); @@ -205,10 +207,10 @@ export const usePreviewThumbnail = ({ }, [clearSettleTimers, finishLoadingForGeneration]); const handleResize = useCallback( - (dimensions: ContentDimensions) => { + (dimensions: IframeResizeDimensions) => { const generation = loadGenerationRef.current; setRememberedDimensions((current) => - contentDimensionsEqual(current, dimensions) ? current : dimensions + iframeResizeDimensionsEqual(current, dimensions) ? current : dimensions ); scheduleLoadingClearAfterPaint(generation); }, diff --git a/code/core/src/shared/constants/iframe-resize.test.ts b/code/core/src/shared/constants/iframe-resize.test.ts index c27c06c4ceba..ededdd73b273 100644 --- a/code/core/src/shared/constants/iframe-resize.test.ts +++ b/code/core/src/shared/constants/iframe-resize.test.ts @@ -1,7 +1,71 @@ import { describe, expect, it } from 'vitest'; import { RESPONSIVE_VIEWPORT_VALUE } from '../../viewport/constants.ts'; -import { hasFixedViewportDimensions, iframeResizeDimensionsEqual } from './iframe-resize.ts'; +import { + hasFixedViewportDimensions, + IFRAME_RESIZE_CONTEXT, + iframeResizeDimensionsEqual, + parseIframeResizeMessage, +} from './iframe-resize.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 }); + expect( + parseIframeResizeMessage({ context: IFRAME_RESIZE_CONTEXT, width: 320, height: 240 }) + ).toEqual({ width: 320, height: 240 }); + expect( + parseIframeResizeMessage({ + context: IFRAME_RESIZE_CONTEXT, + width: 320, + height: 240, + viewport: { name: 'Small mobile', value: 'mobile1', width: 320, height: 568 }, + }) + ).toEqual({ + width: 320, + height: 240, + viewport: { name: 'Small mobile', value: 'mobile1', width: 320, height: 568 }, + }); + }); + + 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(); + expect( + parseIframeResizeMessage({ context: IFRAME_RESIZE_CONTEXT, width: 0, height: 240 }) + ).toBeNull(); + expect( + parseIframeResizeMessage({ context: IFRAME_RESIZE_CONTEXT, width: NaN, height: 240 }) + ).toBeNull(); + expect( + parseIframeResizeMessage({ + context: IFRAME_RESIZE_CONTEXT, + width: 320, + height: 240, + viewport: { name: 'Small mobile', value: 'mobile1' }, + }) + ).toEqual({ + width: 320, + height: 240, + viewport: { name: 'Small mobile', value: 'mobile1' }, + }); + expect( + parseIframeResizeMessage({ + context: IFRAME_RESIZE_CONTEXT, + width: 320, + height: 240, + viewport: { value: 'mobile1', width: 320, height: 568 }, + }) + ).toBeNull(); + }); +}); describe('hasFixedViewportDimensions', () => { it('accepts named viewports with pixel dimensions', () => { diff --git a/code/core/src/shared/constants/iframe-resize.ts b/code/core/src/shared/constants/iframe-resize.ts index 2ab449b0f545..6db473569005 100644 --- a/code/core/src/shared/constants/iframe-resize.ts +++ b/code/core/src/shared/constants/iframe-resize.ts @@ -23,6 +23,67 @@ export type IframeResizePayload = { export type IframeResizeDimensions = Pick; +const isPositiveFinite = (value: unknown): value is number => + typeof value === 'number' && Number.isFinite(value) && value > 0; + +const parseViewport = (value: unknown): IframeResizeViewport | undefined => { + if (!value || typeof value !== 'object') { + return undefined; + } + + const viewport = value as Partial; + if (typeof viewport.name !== 'string' || typeof viewport.value !== 'string') { + return undefined; + } + + const parsed: IframeResizeViewport = { + name: viewport.name, + value: viewport.value, + }; + + if (viewport.width !== undefined) { + if (!isPositiveFinite(viewport.width)) { + return undefined; + } + parsed.width = viewport.width; + } + + if (viewport.height !== undefined) { + if (!isPositiveFinite(viewport.height)) { + return undefined; + } + parsed.height = viewport.height; + } + + return parsed; +}; + +/** Validates an `iframe.resize` postMessage payload from an embed preview. */ +export const parseIframeResizeMessage = (data: unknown): IframeResizeDimensions | null => { + try { + const parsed = typeof data === 'string' ? JSON.parse(data) : data; + if (parsed?.context !== IFRAME_RESIZE_CONTEXT) { + return null; + } + if (!isPositiveFinite(parsed.width) || !isPositiveFinite(parsed.height)) { + return null; + } + + const viewport = parseViewport(parsed.viewport); + if (parsed.viewport !== undefined && viewport === undefined) { + return null; + } + + return { + width: parsed.width, + height: parsed.height, + ...(viewport ? { viewport } : {}), + }; + } catch { + return null; + } +}; + export const hasFixedViewportDimensions = ( viewport: IframeResizeViewport | undefined ): viewport is IframeResizeViewport & { From 9c005c91625b4fb10309e00c7a37a2515c320607 Mon Sep 17 00:00:00 2001 From: Gert Hengeveld Date: Thu, 2 Jul 2026 13:08:58 +0200 Subject: [PATCH 02/11] Move preview slot-release ownership into the previewScheduler Slot accounting for the review thumbnail concurrency cap was split across the scheduler (start/finish), the hook (a settle timer after src assignment), and CollectionGrid (iframe load/error). The scheduler now owns the deadline: it starts a slot timer when it starts a task and auto-releases when it fires. Callers get an opaque {forceStart, release} handle instead of a task object with public mutable booleans, so the accounting-drift clamp is no longer needed. Also fixes the header comment that pointed at a CollectionGrid explanation which no longer exists. Co-Authored-By: Claude Fable 5 --- .../review/components/previewScheduler.ts | 97 ++++++++++--------- .../review/components/usePreviewThumbnail.ts | 39 ++------ 2 files changed, 61 insertions(+), 75 deletions(-) diff --git a/code/core/src/manager/components/review/components/previewScheduler.ts b/code/core/src/manager/components/review/components/previewScheduler.ts index ad1824cc98e4..0af1623cc347 100644 --- a/code/core/src/manager/components/review/components/previewScheduler.ts +++ b/code/core/src/manager/components/review/components/previewScheduler.ts @@ -1,68 +1,73 @@ +// Caps concurrent preview iframe boots across the review grid: booting many +// embed iframes at once can fail with `net::ERR_INSUFFICIENT_RESOURCES`. +const MAX_CONCURRENT_PREVIEWS = 3; + /** - * Caps concurrent preview iframe boots across the review grid. See CollectionGrid - * header comment for why this exists (`net::ERR_INSUFFICIENT_RESOURCES`). + * How long a started preview may hold its concurrency slot. The iframe's load/error events release + * the slot earlier; this deadline keeps the queue draining when neither fires. */ export const PREVIEW_SETTLE_TIMEOUT_MS = 1500; -const MAX_CONCURRENT_PREVIEWS = 3; +/** Handle for a scheduled preview boot, returned by `enqueuePreview`. */ +export interface PreviewHandle { + /** Hover/focus: start a still-queued preview right away, bypassing the cap. */ + forceStart: () => void; + /** Release the concurrency slot (iframe loaded/errored/unmounted). Idempotent. */ + release: () => void; +} -export interface PreviewTask { - /** Assigns the iframe src, kicking off the actual load. */ +interface Task { start: () => void; - started: boolean; - finished: boolean; + state: 'queued' | 'started' | 'released'; + deadline?: ReturnType; } let activePreviewLoads = 0; -const previewQueue: PreviewTask[] = []; +const previewQueue: Task[] = []; -function startQueuedPreviews(): void { - while (activePreviewLoads < MAX_CONCURRENT_PREVIEWS) { - const task = previewQueue.shift(); - if (!task) { - return; - } - if (task.started || task.finished) { - continue; - } - task.started = true; - activePreviewLoads += 1; - task.start(); - } +function startTask(task: Task): void { + task.state = 'started'; + activePreviewLoads += 1; + task.deadline = setTimeout(() => releaseTask(task), PREVIEW_SETTLE_TIMEOUT_MS); + task.start(); } -export function enqueuePreview(task: PreviewTask): void { - previewQueue.push(task); - startQueuedPreviews(); +function startQueuedPreviews(): void { + while (activePreviewLoads < MAX_CONCURRENT_PREVIEWS && previewQueue.length > 0) { + startTask(previewQueue.shift()!); + } } -/** Mark a task done (load/error/settle/unmount) and let the next one start. */ -export function finishPreview(task: PreviewTask): void { - if (task.finished) { +function releaseTask(task: Task): void { + if (task.state === 'released') { return; } - task.finished = true; - if (task.started) { - activePreviewLoads = Math.max(0, activePreviewLoads - 1); + if (task.state === 'started') { + clearTimeout(task.deadline); + activePreviewLoads -= 1; } else { - const index = previewQueue.indexOf(task); - if (index !== -1) { - previewQueue.splice(index, 1); - } + previewQueue.splice(previewQueue.indexOf(task), 1); } + task.state = 'released'; startQueuedPreviews(); } -/** Hover/focus: start a still-queued preview right away, bypassing the cap. */ -export function forceStartPreview(task: PreviewTask): void { - if (task.started || task.finished) { - return; - } - const index = previewQueue.indexOf(task); - if (index !== -1) { - previewQueue.splice(index, 1); - } - task.started = true; - activePreviewLoads += 1; - task.start(); +/** + * Queue a preview boot. `start` runs (synchronously or later) once a concurrency slot frees up. The + * slot is held until `release()` or the settle deadline, whichever comes first. + */ +export function enqueuePreview(start: () => void): PreviewHandle { + const task: Task = { start, state: 'queued' }; + previewQueue.push(task); + startQueuedPreviews(); + return { + forceStart: () => { + if (task.state !== 'queued') { + return; + } + previewQueue.splice(previewQueue.indexOf(task), 1); + startTask(task); + }, + release: () => releaseTask(task), + }; } diff --git a/code/core/src/manager/components/review/components/usePreviewThumbnail.ts b/code/core/src/manager/components/review/components/usePreviewThumbnail.ts index dd07c797c58a..ded666668d02 100644 --- a/code/core/src/manager/components/review/components/usePreviewThumbnail.ts +++ b/code/core/src/manager/components/review/components/usePreviewThumbnail.ts @@ -17,9 +17,7 @@ import { import { PREVIEW_SETTLE_TIMEOUT_MS, enqueuePreview, - finishPreview, - forceStartPreview, - type PreviewTask, + type PreviewHandle, } from './previewScheduler.ts'; /** If iframe.resize never arrives, don't block the thumbnail forever. */ @@ -152,7 +150,7 @@ export const usePreviewThumbnail = ({ ); const [isPreviewLoading, setIsPreviewLoading] = useState(false); const [src, setSrc] = useState(undefined); - const taskRef = useRef(null); + const handleRef = useRef(null); const loadGenerationRef = useRef(0); const fallbackTimerRef = useRef>(); const settleRafRef = useRef({ raf1: 0, raf2: 0 }); @@ -279,34 +277,25 @@ export const usePreviewThumbnail = ({ return undefined; } const previewHref = getPreviewHref(storyId); - const task: PreviewTask = { - start: () => { - setSrc(previewHref); - }, - started: false, - finished: false, - }; - taskRef.current = task; // Spinner while queued for a concurrency slot, not only after src is assigned. setIsPreviewLoading(true); - enqueuePreview(task); + const handle = enqueuePreview(() => { + setSrc(previewHref); + }); + handleRef.current = handle; return () => { - finishPreview(task); - taskRef.current = null; + handle.release(); + handleRef.current = null; resetLoad(); }; }, [isInView, storyId, getPreviewHref, resetLoad]); const finishCurrent = useCallback(() => { - if (taskRef.current) { - finishPreview(taskRef.current); - } + handleRef.current?.release(); }, []); const forceStartCurrent = useCallback(() => { - if (taskRef.current) { - forceStartPreview(taskRef.current); - } + handleRef.current?.forceStart(); }, []); useEffect(() => { @@ -318,14 +307,6 @@ export const usePreviewThumbnail = ({ return clearSettleTimers; }, [src, beginLoad, resetLoad, clearSettleTimers]); - useEffect(() => { - if (!src) { - return undefined; - } - const timer = setTimeout(finishCurrent, PREVIEW_SETTLE_TIMEOUT_MS); - return () => clearTimeout(timer); - }, [src, finishCurrent]); - useLayoutEffect(() => { if (!src) { return undefined; From b01cf98da43d1e88aba1d3ed810b6c242c685e87 Mon Sep 17 00:00:00 2001 From: Gert Hengeveld Date: Thu, 2 Jul 2026 13:10:49 +0200 Subject: [PATCH 03/11] Promote review routes to a first-class layout policy in manager-api Add 'review' to the API_ViewMode union and introduce a single nav-availability policy (api.getNavAvailability) in the manager-api layout module. Layout, the sidebar menu tool, the fullscreen tool, keyboard shortcuts, and the review summary portal now consume that policy instead of each re-deriving sidebar suppression from isReviewManagerRoute. Also unify the isPagesShown derivation between Layout and Sidebar via isPagesViewMode, and share the review-collection story predicate as isReviewCollectionStoryRoute. Co-Authored-By: Claude Fable 5 --- code/core/src/manager-api/modules/layout.ts | 24 ++++++++++++++++++ .../core/src/manager-api/modules/shortcuts.ts | 3 +-- .../core/src/manager-api/tests/layout.test.ts | 25 +++++++++++++++++++ .../components/layout/Layout.stories.tsx | 3 ++- .../src/manager/components/layout/Layout.tsx | 14 ++++------- .../manager/components/preview/Preview.tsx | 6 ++--- .../manager/components/preview/Toolbar.tsx | 7 +++--- .../manager/components/preview/tools/menu.tsx | 9 +++---- .../components/preview/utils/types.tsx | 1 + .../components/review/ReviewPage.stories.tsx | 1 + .../review/screens/ReviewSummaryPortal.tsx | 7 ++---- .../manager/components/sidebar/Sidebar.tsx | 4 +-- code/core/src/manager/container/Preview.tsx | 1 + code/core/src/shared/review/index.ts | 1 + code/core/src/shared/review/routes.test.ts | 15 ++++++++++- code/core/src/shared/review/routes.ts | 12 +++++++-- code/core/src/types/modules/api.ts | 2 +- 17 files changed, 99 insertions(+), 36 deletions(-) diff --git a/code/core/src/manager-api/modules/layout.ts b/code/core/src/manager-api/modules/layout.ts index 5fdaf61d99b3..11e071094aa9 100644 --- a/code/core/src/manager-api/modules/layout.ts +++ b/code/core/src/manager-api/modules/layout.ts @@ -4,6 +4,7 @@ import type { API_LayoutCustomisations, API_PanelPositions, API_UI, + API_ViewMode, } from 'storybook/internal/types'; import { global } from '@storybook/global'; @@ -14,6 +15,7 @@ import type { ThemeVars } from 'storybook/theming'; import { deprecate } from 'storybook/internal/client-logger'; import { create } from 'storybook/theming/create'; +import { isReviewManagerRoute } from '../../shared/review/routes.ts'; import merge from '../lib/merge.ts'; import type { ModuleFn } from '../lib/types.tsx'; import type { State } from '../root.tsx'; @@ -36,6 +38,16 @@ export interface SubState { theme: ThemeVars; } +/** + * Availability of the sidebar/nav: 'unavailable' means the current route suppresses the nav + * entirely (review routes), otherwise it is 'shown' or 'hidden' based on the layout state. + */ +export type NavAvailability = 'shown' | 'hidden' | 'unavailable'; + +/** True when the route renders a full-screen page (e.g. settings) instead of the preview canvas. */ +export const isPagesViewMode = (viewMode: API_ViewMode): boolean => + viewMode !== undefined && viewMode !== 'story' && viewMode !== 'docs' && viewMode !== 'review'; + export interface SubAPI { /** * Toggles the fullscreen mode of the Storybook UI. @@ -88,6 +100,11 @@ export interface SubAPI { getIsPanelShown: () => boolean; /** GetIsNavShown - Returns the current visibility of the navigation bar in the Storybook UI. */ getIsNavShown: () => boolean; + /** + * GetNavAvailability - Returns whether the sidebar/nav is shown, hidden (but can be shown by the + * user), or unavailable because the current route suppresses it entirely (review routes). + */ + getNavAvailability: () => NavAvailability; /** * GetShowToolbarWithCustomisations - Returns the current visibility of the toolbar, taking into * account customisations requested by the end user via a layoutCustomisations function. @@ -545,6 +562,13 @@ export const init: ModuleFn = ({ store, provider, singleStory getIsNavShown() { return getIsNavShown(store.getState()); }, + getNavAvailability(): NavAvailability { + const state = store.getState(); + if (isReviewManagerRoute(state.path, state.customQueryParams)) { + return 'unavailable'; + } + return getIsNavShown(state) ? 'shown' : 'hidden'; + }, getShowToolbarWithCustomisations(showToolbar: boolean) { const state = store.getState(); diff --git a/code/core/src/manager-api/modules/shortcuts.ts b/code/core/src/manager-api/modules/shortcuts.ts index c44c098c9041..5652afa38baa 100644 --- a/code/core/src/manager-api/modules/shortcuts.ts +++ b/code/core/src/manager-api/modules/shortcuts.ts @@ -9,7 +9,6 @@ import { global } from '@storybook/global'; import copy from 'copy-to-clipboard'; -import { isReviewManagerRoute } from '../../shared/review/routes.ts'; import type { KeyboardEventLike } from '../lib/shortcut.ts'; import { eventToShortcut, shortcutMatchesShortcut } from '../lib/shortcut.ts'; import type { ModuleFn } from '../lib/types.tsx'; @@ -269,7 +268,7 @@ export const init: ModuleFn = ({ store, fullAPI, provider }) => { return; } - const isSidebarShortcutBlocked = isReviewManagerRoute(state.path, state.customQueryParams); + const isSidebarShortcutBlocked = fullAPI.getNavAvailability() === 'unavailable'; const isSidebarShortcutFeature = ['focusNav', 'search', 'toggleNav'].includes(feature); if (isSidebarShortcutBlocked && isSidebarShortcutFeature) { return; diff --git a/code/core/src/manager-api/tests/layout.test.ts b/code/core/src/manager-api/tests/layout.test.ts index f8c8c0b45e56..29fee74e2a17 100644 --- a/code/core/src/manager-api/tests/layout.test.ts +++ b/code/core/src/manager-api/tests/layout.test.ts @@ -700,6 +700,31 @@ describe('layout API', () => { expect(layoutApi.getIsNavShown()).toBe(true); }); + it('should get nav availability with getNavAvailability', () => { + expect(layoutApi.getNavAvailability()).toBe('shown'); + + layoutApi.toggleNav(); + + expect(layoutApi.getNavAvailability()).toBe('hidden'); + + layoutApi.toggleNav(); + + Object.assign(currentState, { path: '/review/', customQueryParams: {} }); + + expect(layoutApi.getNavAvailability()).toBe('unavailable'); + + Object.assign(currentState, { + path: '/story/foo--bar', + customQueryParams: { collection: '0' }, + }); + + expect(layoutApi.getNavAvailability()).toBe('unavailable'); + + Object.assign(currentState, { path: '/story/foo--bar', customQueryParams: {} }); + + expect(layoutApi.getNavAvailability()).toBe('shown'); + }); + it('should get panelShwon with getIsPanelShown', () => { expect(layoutApi.getIsPanelShown()).toBe(true); diff --git a/code/core/src/manager/components/layout/Layout.stories.tsx b/code/core/src/manager/components/layout/Layout.stories.tsx index a20a9a61738e..59163072ab3c 100644 --- a/code/core/src/manager/components/layout/Layout.stories.tsx +++ b/code/core/src/manager/components/layout/Layout.stories.tsx @@ -99,6 +99,7 @@ const mockManagerStore: any = { getCurrentStoryData: fn(() => { return mockManagerStore.state.index.someStoryId; }), + getNavAvailability: fn(() => 'shown'), }, }; @@ -527,7 +528,7 @@ export const MobileReview: Story = { }, api: { ...mockManagerStore.api, - getIsNavShown: fn(() => true), + getNavAvailability: fn(() => 'unavailable'), }, }} > diff --git a/code/core/src/manager/components/layout/Layout.tsx b/code/core/src/manager/components/layout/Layout.tsx index d69c7659fd59..dc30e471e781 100644 --- a/code/core/src/manager/components/layout/Layout.tsx +++ b/code/core/src/manager/components/layout/Layout.tsx @@ -6,7 +6,7 @@ import type { API_Layout, API_ViewMode } from 'storybook/internal/types'; import { useStorybookApi, useStorybookState, type API } from 'storybook/manager-api'; import { styled } from 'storybook/theming'; -import { isReviewManagerRoute } from '../../../shared/review/routes.ts'; +import { isPagesViewMode } from '../../../manager-api/modules/layout.ts'; import { MEDIA_DESKTOP_BREAKPOINT, MINIMUM_CONTENT_WIDTH_PX } from '../../constants.ts'; import { Notifications } from '../../container/Notifications.tsx'; @@ -108,11 +108,7 @@ const useLayoutSyncingState = ({ // eslint-disable-next-line react-hooks/exhaustive-deps }, [internalDraggingSizeState, setManagerLayoutState]); - const isPagesShown = - managerLayoutState.viewMode !== undefined && - managerLayoutState.viewMode !== 'story' && - managerLayoutState.viewMode !== 'docs' && - managerLayoutState.viewMode !== 'review'; + const isPagesShown = isPagesViewMode(managerLayoutState.viewMode); const isPanelShown = managerLayoutState.viewMode === 'story' && !hasTab; const { navSize, rightPanelWidth, bottomPanelHeight } = internalDraggingSizeState.isDragging @@ -154,9 +150,9 @@ const OrderedMobileNavigation = styled(MobileNavigation)({ export const Layout = ({ managerLayoutState, setManagerLayoutState, hasTab, ...slots }: Props) => { const { isDesktop, isMobile } = useLayout(); const api = useStorybookApi(); - const { path, customQueryParams } = useStorybookState(); - const showSidebar = - (api.getIsNavShown?.() ?? true) && !isReviewManagerRoute(path, customQueryParams); + // Subscribe to manager state so nav availability re-evaluates on route and layout changes. + useStorybookState(); + const showSidebar = api.getNavAvailability() === 'shown'; const { navSize, diff --git a/code/core/src/manager/components/preview/Preview.tsx b/code/core/src/manager/components/preview/Preview.tsx index 6f88b53e34d9..55f2499007a5 100644 --- a/code/core/src/manager/components/preview/Preview.tsx +++ b/code/core/src/manager/components/preview/Preview.tsx @@ -13,7 +13,7 @@ import { Helmet } from 'react-helmet-async'; import { Consumer, addons, merge, types, type Combo } from 'storybook/manager-api'; import { useLandmark } from '../../hooks/useLandmark.ts'; -import { REVIEW_COLLECTION_QUERY_PARAM } from '../../../shared/review/routes.ts'; +import { isReviewCollectionStoryRoute } from '../../../shared/review/routes.ts'; import { ReviewToolbarHeader } from '../review/components/ReviewToolbarHeader.tsx'; import { FramesRenderer } from './FramesRenderer.tsx'; import { ToolbarComp } from './Toolbar.tsx'; @@ -60,6 +60,7 @@ const Preview = React.memo(function Preview(props) { tabs, wrappers, tabId, + path, queryParams, } = props; @@ -92,8 +93,7 @@ const Preview = React.memo(function Preview(props) { const shouldScale = viewMode === 'story'; const { showToolbar } = options; const customisedShowToolbar = api.getShowToolbarWithCustomisations(showToolbar); - const isReviewCollectionStory = - viewMode === 'story' && queryParams?.[REVIEW_COLLECTION_QUERY_PARAM] !== undefined; + const isReviewCollectionStory = isReviewCollectionStoryRoute(path, queryParams); const previousStoryId = useRef(storyId); diff --git a/code/core/src/manager/components/preview/Toolbar.tsx b/code/core/src/manager/components/preview/Toolbar.tsx index 8981ed94ead4..c4024a635741 100644 --- a/code/core/src/manager/components/preview/Toolbar.tsx +++ b/code/core/src/manager/components/preview/Toolbar.tsx @@ -18,7 +18,6 @@ import { } from 'storybook/manager-api'; import { styled } from 'storybook/theming'; -import { isReviewManagerRoute } from '../../../shared/review/routes.ts'; import { useLandmark } from '../../hooks/useLandmark.ts'; import { useLayout } from '../layout/LayoutProvider.tsx'; import type { PreviewProps } from './utils/types.tsx'; @@ -33,7 +32,7 @@ const fullScreenMapper = ({ api, state }: Combo) => { shortcut: api.getShortcutKeys().fullScreen, hasPanel: Object.keys(api.getElements(Addon_TypesEnum.PANEL)).length > 0, singleStory: state.singleStory, - isReviewRoute: isReviewManagerRoute(state.path, state.customQueryParams), + isNavUnavailable: api.getNavAvailability() === 'unavailable', }; }; @@ -52,8 +51,8 @@ export const fullScreenTool: Addon_BaseType = { return ( - {({ toggle, isFullscreen, shortcut, hasPanel, singleStory, isReviewRoute }) => - !isReviewRoute && + {({ toggle, isFullscreen, shortcut, hasPanel, singleStory, isNavUnavailable }) => + !isNavUnavailable && (!singleStory || (singleStory && hasPanel)) && (