Skip to content

fix(shader-transitions): pin cloned scene roots to composition dimensions#2254

Merged
vanceingalls merged 2 commits into
mainfrom
07-11-fix_shader-transitions_pin_cloned_scene_roots_to_composition_dimensions
Jul 11, 2026
Merged

fix(shader-transitions): pin cloned scene roots to composition dimensions#2254
vanceingalls merged 2 commits into
mainfrom
07-11-fix_shader-transitions_pin_cloned_scene_roots_to_composition_dimensions

Conversation

@vanceingalls

@vanceingalls vanceingalls commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

What

Pins cloned scene roots in the page-side shader compositor (engineModePageComposite.ts) to the exact box each measures via getBoundingClientRect() while still live in the document — not the composition's full pixel size. New pure exported helper clonePinStyleFor(rect) computes the style values from that measured rect.

Why

Wild report: page-side shader clones collapsed to 0x0 inside the canvas layoutsubtree when scenes relied only on position:absolute; inset:0 sizing (no containing-block dimensions exist there).

The live document already resolves inset:0 (and any authored explicit width/height) correctly against the real ancestor chain — measuring the source element's rect there and reapplying that exact box to the clone fixes the collapse without ever overriding an author's own sizing, unlike the original fix which unconditionally hardcoded every clone to the composition's full width/height regardless of the source scene's own dimensions.

Validation

New unit tests for clonePinStyleFor: proves a 0x0-collapse-prone inset:0 scene measures/reproduces the full frame box, AND proves an authored explicit width/height/offset (e.g. a picture-in-picture panel) survives verbatim rather than being overridden. 9/9 tests pass. shader-transitions package build clean.

Review history

Addressed review feedback: the previous version clobbered authored explicit dimensions. Now measures and reapplies the live-document box instead of hardcoding composition size.

🤖 Generated with Claude Code

@vanceingalls vanceingalls force-pushed the 07-11-fix_shader-transitions_pin_cloned_scene_roots_to_composition_dimensions branch from a7caaab to 1d2aca6 Compare July 11, 2026 20:15
@vanceingalls vanceingalls force-pushed the 07-11-fix_cli_fold_canvas_pixel_hashes_into_the_frozen-sweep_fingerprint branch from 2e69883 to 17db454 Compare July 11, 2026 20:15

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2254 — pin cloned scene roots to composition dimensions

Verdict: LGTM 🟢

11 lines. Cloned scene roots in the page-side shader compositor get explicit dimensions (width: ${width}px; height: ${height}px) so scenes sized only by position:absolute; inset:0 don't resolve to 0x0 inside the staging canvas's layout subtree. Scenes with explicit dimensions are unaffected (the explicit values win).

Minimal, correct fix for a measured failure (blank transition textures).

Review by Miga

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker: the new clone pinning unconditionally assigns clone.style.width/height = composition dimensions (engineModePageComposite.ts). That overrides authored scene-root layout, contrary to the PR contract and its comment that explicit dimensions are unaffected. Please preserve authored dimensions (only pin when the root relies on auto/zero sizing, or otherwise establish the containing block without overwriting explicit author styles) and add a regression test proving an authored width/height survives.

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 1d2aca6.

Right diagnosis: position:absolute; inset:0 needs a containing-block with dimensions, and the staging canvas's layout subtree doesn't provide one. Pinning to the composition's pixel size unconditionally fixes the collapse in both transitions.

One small clarification worth adding to the docstring:

Nit — "scenes that already carry explicit dimensions are unaffected". The block at packages/shader-transitions/src/engineModePageComposite.ts:250-256 overwrites clone.style.width/height/position/left/top unconditionally, so a scene with style="width: 500px; height: 500px" inline WILL be pinned to composition dims. In practice shader-transition scene roots are full-canvas so the visual output is unchanged for correctly-authored scenes, but the docstring wording implies pin-then-preserve semantics that the code doesn't actually implement. Either soften to "scenes that already match the composition dimensions render identically" or add a if (!clone.style.width) … guard if the intent was actually preservation.

Not blocking — happy to leave the code and just tweak the comment.

LGTM from my side.

Review by Rames D Jusso

@vanceingalls vanceingalls force-pushed the 07-11-fix_cli_fold_canvas_pixel_hashes_into_the_frozen-sweep_fingerprint branch from 17db454 to a9d320d Compare July 11, 2026 20:46
@vanceingalls vanceingalls force-pushed the 07-11-fix_shader-transitions_pin_cloned_scene_roots_to_composition_dimensions branch from 1d2aca6 to 32228cf Compare July 11, 2026 20:46
@vanceingalls vanceingalls force-pushed the 07-11-fix_shader-transitions_pin_cloned_scene_roots_to_composition_dimensions branch from 32228cf to d9e6f7d Compare July 11, 2026 21:06
@vanceingalls vanceingalls force-pushed the 07-11-fix_cli_fold_canvas_pixel_hashes_into_the_frozen-sweep_fingerprint branch 2 times, most recently from 7f47b36 to 2935a46 Compare July 11, 2026 21:20
vanceingalls added a commit that referenced this pull request Jul 11, 2026
…omposition size

Addresses review feedback on #2254: the previous fix hardcoded every clone
to the full composition width/height regardless of the source scene's own
sizing, silently overriding any authored explicit width/height. Measuring
each scene's getBoundingClientRect() while still live in the document (its
inset:0 already resolves correctly against the real ancestor chain there)
and reapplying that exact box to the clone fixes the 0x0 collapse without
ever clobbering an author's own layout.
@vanceingalls vanceingalls force-pushed the 07-11-fix_shader-transitions_pin_cloned_scene_roots_to_composition_dimensions branch from d9e6f7d to 01350bf Compare July 11, 2026 21:20
miguel-heygen
miguel-heygen previously approved these changes Jul 11, 2026

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed current head 01350bf. The fix now measures each live scene root and pins the clone to that measured box; authored dimensions/offsets are preserved and regression tests cover both collapsed inset roots and explicit sizing. No required-check failures are present (Graphite mergeability pending only). APPROVE.

jrusso1020
jrusso1020 previously approved these changes Jul 11, 2026

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving per the channel stamp-flow: Magi re-reviewed the fix and approved at 01350bf (authored-dim blocker resolved via measured-rect clone + regression tests), Rames-D had cleared it, CI clean.

…omposition size

Addresses review feedback on #2254: the previous fix hardcoded every clone
to the full composition width/height regardless of the source scene's own
sizing, silently overriding any authored explicit width/height. Measuring
each scene's getBoundingClientRect() while still live in the document (its
inset:0 already resolves correctly against the real ancestor chain there)
and reapplying that exact box to the clone fixes the 0x0 collapse without
ever clobbering an author's own layout.
@vanceingalls vanceingalls force-pushed the 07-11-fix_cli_fold_canvas_pixel_hashes_into_the_frozen-sweep_fingerprint branch from 2935a46 to 3338a6f Compare July 11, 2026 21:49
@vanceingalls vanceingalls force-pushed the 07-11-fix_shader-transitions_pin_cloned_scene_roots_to_composition_dimensions branch from 01350bf to b3d851d Compare July 11, 2026 21:49
Base automatically changed from 07-11-fix_cli_fold_canvas_pixel_hashes_into_the_frozen-sweep_fingerprint to main July 11, 2026 22:24
@vanceingalls vanceingalls dismissed jrusso1020’s stale review July 11, 2026 22:24

The base branch was changed.

@vanceingalls vanceingalls dismissed miguel-heygen’s stale review July 11, 2026 22:24

The base branch was changed.

@vanceingalls vanceingalls merged commit 76f4d2e into main Jul 11, 2026
20 checks passed
@vanceingalls vanceingalls deleted the 07-11-fix_shader-transitions_pin_cloned_scene_roots_to_composition_dimensions branch July 11, 2026 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants