fix(cli): keep scaffolded video sources playable - #4839
Open
AKnassa wants to merge 2 commits into
Open
Conversation
`stripTemplateAssetRefs` swapped every `/template-assets/*` reference for the inline image placeholder, whatever the file type. Scaffolding the LightboxVideo block therefore produced `src: 'data:image/svg+xml,...'` sitting next to `type: 'video'` — SVG image data handed to a <video> element, which can never play. Only the copy path was affected; `astryx template LightboxVideo` and the docsite example read the raw source and stayed valid. Capture the extension and choose the placeholder per reference: `.mp4`, `.webm`, `.mov`, `.m4v`, and `.ogv` take a new inline video placeholder, everything else keeps the image one. Both remain self-contained, so a scaffolded project still needs no `/template-assets/` dir and no network. The video placeholder is 640x360, 2 seconds, no audio track, H.264 constrained baseline in MP4 (2,269 bytes) drawing the same mark as the image placeholder with a play glyph. placeholder-video.svg is committed beside placeholder.svg as the reviewable still of what those bytes encode. Tests cover the five video extensions, case-insensitive matching, per-reference selection in a file that mixes image and video, and an end-to-end scaffold of LightboxVideo. The blob is opaque to review, so one test also walks its top-level MP4 boxes and asserts they tile the buffer exactly — a single dropped byte fails it. Fixes facebook#4780
|
@AKnassa is attempting to deploy a commit to the Meta Open Source Team on Vercel. A member of the Team first needs to authorize it. |
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
community
Contributor
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR No new or modified components detected. Bundle Size SummaryNo component packages changed. Accessibility AuditStatus: No accessibility violations detected. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
Self-review of the first commit found the placeholder was not what it said it was, and that the classifier had a latent hole. The blob was a single frame declaring 33ms, while the JSDoc, the changeset, and the PR body all promised two seconds. Its duration headers also contradicted each other: mvhd said 33ms, mdhd said 1.1ms, and the sample table said 33.3ms. Re-encoded to a real clip — 3 frames, 1.625s — and corrected mdhd, which the encoder had written in the movie timescale instead of the media timescale. Docs and changeset now state the measured values. Every documented property is now asserted against the decoded bytes: dimensions, frame count, duration, one video track with no audio, the codec profile, and agreement between mvhd, tkhd, mdhd and the sample table. The old structural test could not fail on any of this — a one-frame movie satisfied it. The box walk is lazy so a broken transform fails these tests instead of collapsing the whole file at collection time. The extension is now read from the last dotted segment of the filename. `clip.min.mp4` was matched as `clip.min`, classified from `min` as an image, and left a stray `.mp4` welded to the data URI — the same image-behind-video bug this PR exists to fix. `@` joins the name class because the demo-media dir ships 16 `@2x` variants that fell through the pattern entirely and would 404 in a scaffolded project. Both shapes are latent: replaying the old and new patterns over all 1328 shipped template sources produces byte-identical output. Adds a layout-expand test, since splicing runs block sources through the same transform and had no media coverage. Verified: the five video extensions, case-insensitivity, per-reference selection, multi-dot, `@2x`, and the whole placeholder property set survive mutation testing — swapping the placeholders fails 23 tests, forcing the image branch fails 18, dropping the lowercase fails 1, removing an extension fails 1, restoring the old pattern fails 3. Scaffolding through the real CLI and decoding the result gives a 2,788-byte, 3-frame, 1.625s clip that AVFoundation renders as the placeholder frame.
AKnassa
marked this pull request as ready for review
August 9, 2026 18:06
AKnassa
requested review from
cixzhang,
imdreamrunner and
josephfarina
as code owners
August 9, 2026 18:06
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Scaffolding a template that shows a video now gives you a video that actually plays. Before, the copied file pointed the video at a picture, so the player came up empty.
Why
astryx template LightboxVideo ./out --type blockis meant to hand you a working starting point, and it handed you a broken one. The copied file kepttype: 'video'but its source was swapped for the image placeholder, so the Lightbox asked a<video>element to play SVG picture data. Nothing plays, and it is not obvious why.Only the copy path was affected.
astryx template LightboxVideoon its own, and the docsite example, read the original file and were always fine.What changed
.mp4,.webm,.mov,.m4v,.ogv) get a video placeholder; everything else keeps the image placeholder it has always had.placeholder-video.svgalongside the existingplaceholder.svgso the still frame is viewable in the diff rather than hidden inside an encoded string.clip.min.mp4used to be judged onminand treated as a picture, which is the same bug in a different shape, and@2xfilenames slipped through untouched and would go missing in a scaffolded project. Both are latent rather than live: replaying the old and new rules over all 1328 template files gives byte-identical results.How it was checked
@2xnames, and a full copy of the LightboxVideo block. The layoutexpandpath is covered too, since it runs the same swap.How to see it
Open
out/LightboxVideo.tsx. Thesrcnow begins withdata:video/mp4;base64,instead ofdata:image/svg+xml,. Paste that whole value into a browser address bar to play it, or render the block and open the Lightbox.The frame it shows is in the diff at
apps/docsite/public/template-assets/placeholder-video.svg.Known and deliberately left alone
clip.mp4?v=2) would leave the query glued to the end of the data URI. No template does this, and widening the pattern to swallow trailing characters is riskier than the bug.astryx template <name>(and its--jsonform) still prints the raw/template-assets/...path, because it shows the template as authored rather than as scaffolded. That is existing behaviour and a separate question from this fix.Fixes #4780