fix(studio): capture storyboard tiles at review density - #3371
Conversation
miguel-heygen
left a comment
There was a problem hiding this comment.
The blur diagnosis is right, and the scope is mostly disciplined, but I would not ship unbounded output=source for every contact-sheet tile.
What holds
FramePoster.tsx:53-62is the only production caller changed to request source output. GenericCompositionThumbnail/ timeline callers still omitoutput, so the bounded timeline path is preserved.FramePoster.tsx:72keeps the intended fit split: tile=object-cover, hero=object-contain.FramePoster.test.tsx:40-58pins both that split and the default tile surface.- The JPEG route already supports source output and keeps source/preview in separate cache keys. Focused local verification passed all 22 component/URL/route tests.
Blocker — contact-sheet cost is now unbounded by both composition size and frame count
FramePoster.tsx:61 sends output=source for every tile, and thumbnail.ts:160-165 maps that directly to the authored dimensions with no ceiling. StoryboardGrid.tsx:38-52 maps the full manifest and the storyboard schema has no frame-count cap. loading="lazy" and the server's concurrency-1 coordinator soften the burst, but they do not bound it: HyperFrames supports 7680×4320 compositions, where one decoded tile is ~126.6 MiB; six near-viewport tiles can approach ~760 MiB of decoded image memory, plus six serialized 8K captures. The existing hero risk was one image at a time—this multiplies it across the board.
The measured 1080×1080 Framey case is reasonable, but “source” is the wrong invariant for a ~387 CSS-pixel tile. Add a storyboard-specific bounded high-density mode (for example, preserve authored aspect/layout while capping output near the tile's maximum DPR-2 physical size or a fixed ~1080 longest side). Keep source for the single focus hero if desired. Then pin a 4K/8K source fixture to the cap. This fixes the blur without making arbitrary source resolution the contact sheet's resource budget.
Important — the new route test does not test the square authored-size contract that motivated the PR
thumbnail.test.ts:71-89 creates no composition HTML, so it only proves that output=source returns the route defaults, 1920×1080. A mutation that hard-codes 1920×1080 for source JPEGs still passes it. The motivating regression is a 1080×1080 composition becoming 135×135. Write a square data-width="1080" data-height="1080" fixture and assert source JPEG output is 1080×1080; ideally assert the same fixture remains 135×135 on output=preview. That pins both sides of the contract rather than the default case.
CI is green except the still-running Windows test at review time; the PR is a draft. The code and tests I ran locally are otherwise clean.
Verdict: REQUEST CHANGES
Reasoning: The visual fix is correct and well scoped, but it replaces a severe quality problem with an unbounded multi-tile capture/decode cost, and the route regression test does not exercise the square authored-dimension case it claims to protect.
— Magi
|
Addressed the requested changes in |
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 045cccb40865a808dd6df94f061912469a0d2e1c.
Both prior blockers are closed.
FramePoster.tsx:53-62now routes tiles to a distinctstoryboarddensity while the single focus hero retainssource; genericCompositionThumbnailcallers still omit the parameter and stay on preview output.thumbnail.ts:160-173bounds storyboard output to a 1080px longest side, preserves aspect ratio, and never upscales sources already below the cap. The cache key includes the output mode and resolved dimensions, so preview/storyboard/source artifacts cannot collide.- The route coverage now exercises the actual motivating shape:
thumbnail.test.ts:78-120writes a 1080×1080 composition and pins preview=135×135, storyboard=1080×1080, source=1080×1080.:122-142writes 7680×4320 and pins the cap at 1080×608. A hard-coded default-size source branch no longer survives the suite. FramePoster.test.tsx:40-59pins tile=storyboard, hero=source, default surface=tile, tile=object-cover, and hero=object-contain.
Focused local verification: 23/23 component, URL-builder, and route tests pass. Required GitHub CI is green at this reviewed head.
Verdict: APPROVE
Reasoning: The blur fix now has a bounded resource contract for multi-tile boards, preserves the single-hero/source and timeline/preview paths, and the regression suite directly protects both the square bug and 8K cap.
— Magi
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 045cccb40865a808dd6df94f061912469a0d2e1c.
Both prior blockers are closed.
FramePoster.tsx:53-62now routes tiles to a distinctstoryboarddensity while the single focus hero retainssource; genericCompositionThumbnailcallers still omit the parameter and stay on preview output.thumbnail.ts:160-173bounds storyboard output to a 1080px longest side, preserves aspect ratio, and never upscales sources already below the cap. The cache key includes the output mode and resolved dimensions, so preview/storyboard/source artifacts cannot collide.- The route coverage now exercises the actual motivating shape:
thumbnail.test.ts:78-120writes a 1080×1080 composition and pins preview=135×135, storyboard=1080×1080, source=1080×1080.:122-142writes 7680×4320 and pins the cap at 1080×608. A hard-coded default-size source branch no longer survives the suite. FramePoster.test.tsx:40-59pins tile=storyboard, hero=source, default surface=tile, tile=object-cover, and hero=object-contain.
Focused local verification: 23/23 component, URL-builder, and route tests pass. Required GitHub CI is green at this reviewed head.
Verdict: APPROVE
Reasoning: The blur fix now has a bounded resource contract for multi-tile boards, preserves the single-hero/source and timeline/preview paths, and the regression suite directly protects both the square bug and 8K cap.
— Magi
What
object-coverfor tiles andobject-containfor the focus hero.Why
Storyboard tiles currently omit
output=source, so the thumbnail route caps them at 240x135. A square 1080x1080 composition therefore arrives as only 135x135 and is stretched across a much larger card. On high-density displays this makes typography, thin lines, and sprite detail visibly blurry, which undermines the storyboard review surface.How
FramePosternow requestsoutput: "storyboard"for tiles and retainsoutput: "source"for the single focus hero. The storyboard mode preserves authored aspect ratio while capping the longest side at 1080px. Generic timeline thumbnails remain unchanged and continue to omit an output parameter.The tests pin the complete contract: a 1080x1080 composition maps to 135x135 preview and 1080x1080 storyboard/source output, while an 8K composition is capped to 1080x608 in storyboard mode.
Test plan
Commands run:
bun run --cwd packages/studio test src/components/storyboard/FramePoster.test.tsx src/player/components/CompositionThumbnail.test.tsbun run --cwd packages/studio-server test src/routes/thumbnail.test.tsbun run --cwd packages/studio typecheckbun run --cwd packages/studio-server typecheckbunx oxfmt --checkon all four changed filesbunx oxlinton all four changed filesManual verification used the same six 1080x1080 Framey storyboard compositions at a 1280x720, DPR-2 viewport. The same 387x217 CSS tile changed from a 135x135 poster response to 1080x1080, an 8x increase per dimension and 64x increase in pixel area. The follow-up route coverage verifies that storyboard output remains bounded for 8K sources.