Skip to content

Add a VideoPreview renderer for the video FileDef family - #6180

Open
lukemelia wants to merge 3 commits into
mainfrom
cs-12590-video-preview-component
Open

lukemelia wants to merge 3 commits into
mainfrom
cs-12590-video-preview-component

Conversation

@lukemelia

Copy link
Copy Markdown
Contributor

What

VideoDef and its Mp4Def / MovDef / WebmDef subclasses declared no static previewComponent, so the video family was the last one falling through FilePreviewStage to the generic file pane instead of a native player. This adds the missing renderer.

  • file-formats/video-preview.gtsVideoPreview, modeled on AudioPreview. Reading formats mount a native player via the FileVideo primitive (which loads protected realm bytes for Safari-safe playback and keeps transport events from bubbling into an enclosing card), letterboxed on a matte so an ultrawide/tall source keeps its shape. A fitted cell mounts no player — the fitted shell prefers the captured poster thumbnail, and with no poster this falls back to the family glyph + running time.
  • video-file-def.gtsstatic previewComponent = VideoPreview on VideoDef; the mp4/mov/webm subclasses inherit it.
  • file-formats/index.ts — export VideoPreview alongside MarkdownPreview / ImagePreview / AudioPreview.
  • content-only preview test — added barrel-export, kind-dispatch, and render + fitted coverage for VideoPreview.

This closes the last gap: every FileDef family in packages/base is now on the previewComponent architecture.

Verification

ember-template-lint clean on the new component and the test. The new tests could not be run locally this session — the base realm server (:4201) was down (unrelated crash), so every test in the host suite fails on base-module fetch (https://localhost:4201/base/string → Failed to fetch), not on this change. They'll run in CI where the base realm is served. A visual eyeball of a video card in a running realm is still worth doing (embedded player + fitted poster/glyph fallback).

The realm-indexing-test.gts module-manifest gotcha does not apply — no fixture there pulls in the video family or the full file-formats barrel.

Linear: CS-12590

VideoDef and its mp4/mov/webm subclasses declared no static previewComponent, so the video family fell through FilePreviewStage to the generic file pane instead of a player. Add VideoPreview (built on the FileVideo primitive, which loads protected realm bytes for Safari-safe playback and contains its transport events) and wire it as VideoDef.previewComponent; the subclasses inherit it. Export it from the file-formats/index barrel alongside the other family renderers.

Reading formats mount a native player letterboxed on a matte; a fitted cell shows the poster thumbnail (the shell's job) or, with no poster, the family glyph and running time — never a grid of live players. Adds barrel-export, kind-dispatch, and render/fitted coverage to the content-only preview test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_014GsYGsuCqHJz9GAti4jheG

@lukemelia lukemelia left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Reviewed the new renderer against its siblings — AudioPreview, the FileVideo primitive, the fileViewModel projection, and the FilePreviewStage dispatch. I did not run the host suite (base-realm dependency), so this is a read of the mechanism, not an executed test pass.

Bottom line: one thing to decide before merge — the component credits FileVideo with "Safari-safe protected-byte playback" but never switches that path on (inline comment). Everything else lands: filePreviewComponentFor dispatches VideoDef (and its inherited subclasses) to VideoPreview, the projection routes mediaUrl/posterUrl correctly for the video family, the fitted branch falls back to glyph + clock without mounting a player, the atom shell mounts no renderer at all, and transport-event containment is real.

Recommendations:

  1. Reconcile the @loadAsBlob wiring with the protected-bytes claim — see the inline comment on the <FileVideo> invocation in video-preview.gts.

No other blocking issues.

Comment on lines +81 to +86
<FileVideo
class='video-player'
@src={{this.mediaUrl}}
@poster={{this.posterUrl}}
@preload='metadata'
data-test-video-player

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] The "loads protected realm bytes for Safari-safe playback" behavior this component (and the PR description) credit to FileVideo is inert on this call path. That path lives entirely inside loadProtectedMediaBlob, gated on @loadAsBlob: if (!enabled || !resourceURL) return. This invocation passes only @src/@poster/@preload, so @loadAsBlob is undefined, the modifier returns immediately, and elementSrc falls back to setting src directly — the plain native request that leans on the auth service worker, which is exactly the case loadProtectedMediaBlob's own comment says such browsers "decline to route media element requests through."

So on the browser the claim names, protected-realm video plays via the path the primitive was built to bypass. Two ways to resolve:

  • pass @loadAsBlob={{true}} so the blob fetch actually runs. It degrades gracefully — the modifier's catch restores the canonical src on fetch failure — so this is the low-risk default if you're unsure whether Safari needs it; or
  • drop the "loads protected realm bytes for Safari-safe playback" clause from both the header comment and the PR description, and rest on the two behaviors this path does get from FileVideo (event containment + seek-intent preservation, both unconditional).

The transport-containment half of the header comment is fine — containNativeMediaEvents always applies. Only the protected-bytes/Safari half is unwired. Worth settling since Safari video playback is the headline behavior. (Note AudioPreview uses a raw <audio src> with no blob path either, so if the service worker is in fact sufficient in Safari, correcting the wording is the honest fix rather than adding the arg.)

Class: regression — a new claim this PR's own call path doesn't deliver. Whether it blocks turns on whether Safari actually needs the blob path, which I can't exercise here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Resolved by passing @loadAsBlob={{true}} on the <FileVideo> invocation (8618265). loadProtectedMediaBlob now runs the authenticated blob fetch and only falls back to the canonical src on failure, so the header comment's "Safari-safe protected-byte playback" claim is now actually delivered rather than describing an inert capability. Went with the arg over dropping the wording since making the player work in Safari is the point; the graceful catch degradation keeps it low-risk in browsers where the direct native request already works.

The two render tests still hold — FileVideo renders its <video data-test-video-player> unconditionally, and the blob fetch is fire-and-forget behind the existing AbortController, so the synchronous existence assertions are unaffected (same pattern as the FileAudio blob-fallback primitive test).

@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Preview deployments

Host Test Results

    1 files  ±0      1 suites  ±0   2h 14m 1s ⏱️ - 1m 16s
4 887 tests ±0  4 873 ✅ ±0  14 💤 ±0  0 ❌ ±0 
4 902 runs  ±0  4 888 ✅ ±0  14 💤 ±0  0 ❌ ±0 

Results for commit 50911db. ± Comparison against earlier commit 8618265.

Realm Server Test Results

    1 files  ±0    241 suites  ±0   1h 29m 34s ⏱️ + 2m 46s
3 508 tests ±0  3 508 ✅ ±0  0 💤 ±0  0 ❌ ±0 
3 556 runs  ±0  3 556 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 50911db. ± Comparison against earlier commit 8618265.

VideoPreview mounted <FileVideo> without @loadAsBlob, so loadProtectedMediaBlob
returned immediately and the element's src was set directly — the plain native
request that leans on the auth service worker, the exact case the primitive's
blob path exists to cover for browsers (Safari) that decline to route media
element requests through the worker. Pass @loadAsBlob={{true}} so the component
actually delivers the Safari-safe protected-byte playback its header comment
credits to FileVideo; it degrades gracefully, restoring the canonical src on
fetch failure.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_014GsYGsuCqHJz9GAti4jheG
@lukemelia
lukemelia requested review from a team and backspace September 17, 2026 19:46
The UI guidelines' token contract says a contract token never carries a
literal fallback, a plain color is set through background-color, and a
family token outside the contract resolves once on a component root rather
than restating a literal at every use.

The matte pair resolves once and degrades to `--tooltip`, the one inverted
surface the theme guarantees a foreground for — the same mapping the office
badge takes. The clock chip's scrim becomes a color-mix over black, which
is what the guidance asks for when neither `--overlay` nor `--hover` fits,
and the letterbox bars take the same matte as the stage they sit in rather
than a second, differently-dark literal. Spacing, radius and the two
tracked-out mono labels take the theme's own scales and the eyebrow role.

Both roots now spread `...attributes`, so the `Element: HTMLElement` the
signature already declares is honest and an embedder can style the player
through a class instead of reaching into its markup.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lukemelia

Copy link
Copy Markdown
Contributor Author

[Claude Code 🤖] Skills pass over video-preview.gts, matching the one on the office templates — 50911db.

  • The matte pair (--fd-slate / --fd-paper) is outside the theme contract, so it resolves once on the root and is read bare below, degrading to --tooltip / --tooltip-foreground — the one inverted surface the theme guarantees a foreground for.
  • No literal fallbacks on contract tokens (var(--card, #f7f7f5) is dead weight — the token is always defined), background-color instead of the background shorthand for plain colors.
  • The clock chip's rgb(0 0 0 / 42%) becomes color-mix(in oklch, transparent, black 42%). Black stays deliberate there and is commented: a readability scrim has to darken the frame behind it in both schemes, where a token would flip.
  • The player's #000 letterbox bars take the same matte as the stage they sit in, rather than a second, differently-dark literal.
  • Spacing, radius and the two tracked-out mono labels take --boxel-sp-*, --boxel-border-radius-2xs and the eyebrow role group.
  • Both roots spread ...attributes. ContentPreviewSignature already declares Element: HTMLElement, so without it a caller's class or data-test- was silently dropped — and a class is the lever the guidance prefers over an embedder reaching into the markup.

Verified against a dev stack: both VideoPreview cases in content-only file preview components pass (18 assertions), 91/92 in the module. The one failure is PdfViewer falls back to the plain URL when its fetch cannot get the bytes, which fails identically with this change reverted — it wants a failing fetch and gets a live dev stack. packages/base lint clean.

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.

2 participants