Review 4839

Review 4839 — cli: keep scaffolded video sources playable

#4839 · AKnassa · community Head reviewed a1f1764223f5 (+376/−13, 7 files; opened 9 Aug) Verdict close as superseded by #4863 — posted 2026-08-23, PR closed. This review produced R6e.

Problem

Real, and the author diagnosed it correctly: scaffolding LightboxVideo handed people a <video> pointed at image data — the CLI substituted the image placeholder for a video source.

Solution — and why it is not this one

#4863 landed on 11 Aug, two days after this opened, and fixes the same function a different way: video sources are stripped to an empty src rather than given a substitute clip, so a scaffolded project is honest about needing your own file. The embedded-clip approach this PR takes was declined there.

The leftovers, and the measurement

The diff against the winner left two genuine survivors, both real and both correctly described by the author:

• .m4v missing from VIDEO_EXTENSIONS
• DEMO_ASSET_PATTERN's [\w-]+\.(\w+) reads the extension off the first dot, so clip.min.mp4 is judged on min

Counted on main: 150 /template-assets/ references, zero with a second dot, zero .m4v, one video asset in total. Neither survivor reaches a single person today.

That count is the whole finding. The pull on a superseded close is to hand the winner everything the loser had that it lacks, so nothing is "lost" — which is R28 committed from the other side, a mechanism with no established need, proposed by us, to someone whose PR we are closing. A leftover earns a handover only when you can name who it reaches.

What the leftovers were actually good for

They share a shape: an unrecognised extension falls through to the image branch, so the failure is silent and points toward broken rather than erroring. That is a finding about the guard on main — it belongs in an issue about the guard, never as a reason to hold someone else's PR open.

The close, as posted

Thanks for chasing this down, @AKnassa — you found a real one. Scaffolding LightboxVideo really did hand people a <video> pointed at image data, and the diagnosis in your description is exactly right.

It's since been fixed a different way: #4863 landed on 11 Aug and strips video sources to an empty src rather than substituting a placeholder clip, so a scaffolded project is honest about needing your own file. Same function, same bug — different remedy, and the embedded-clip approach was declined there. So I'm closing this as superseded rather than as anything you got wrong.

The two extras you caught — .m4v missing from the extension set, and DEMO_ASSET_PATTERN reading the extension off the first dot so clip.min.mp4 is judged on min — I checked against main: all 150 /template-assets/ references are single-dot, and none are .m4v, so neither one reaches anyone today. Worth knowing, not worth holding a PR open for.

Reopen any time if you think #4863 misses your case — merge main in first, since this branch has aged. And Discord is the fastest way to reach a person if you'd rather talk it through.

[Reviewed by Robohands]

Why it is written that way

Cindy, on this PR: "The most important piece is to understand the user impact. Why does it matter to get the extra pieces in? How does that improve the product?"

Two minutes of counting turned a confident handover into a clean close — and the close still tells the contributor their eye was good. A contributor who found a real edge case is owed the difference between "we checked, and it reaches nobody today" and silence; the second reads as a brush-off, and they are the only person who can tell us we measured the wrong thing.

No homework on the way out (R13b): the close asks for nothing.

Open follow-up

The silent fall-through to the image branch has no home now this is closed — an issue against the guard on main is owed.