fix: admin provider video preview - #2463
Conversation
🦋 Changeset detectedLatest commit: 8979400 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| // Main Media component for EmDash media. Unlike `Image`, this resolves the | ||
| // media provider via `getEmbed()`, so it can render video/audio whose URLs | ||
| // are not a flat `src` — e.g. Cloudflare Stream, where playback lives in | ||
| // `meta.playback` as HLS/DASH and `Image` would emit a broken <img>. | ||
| EmDashMedia as Media, |
There was a problem hiding this comment.
let me know if this is not intended to be exported, need it for dynamic cloudflare stream playback in app client
90e402a to
47cbb6a
Compare
helio-cf
changed the title
|
All contributors have signed the CLA ✍️ ✅ |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is the right fix for a real, narrow problem: Cloudflare Stream items carry a poster in previewUrl and HLS/DASH playback in meta.playback, while the admin UI assumed every video had a single fetchable src. The approach—introducing a small metaPlayback helper, using previewUrl for thumbnails/posters, and re-exporting Media from emdash/ui so frontend consumers can resolve provider embeds—is well-scoped and additive.
I checked the changed files against AGENTS.md conventions and the existing component patterns. The implementation looks sound, but there are a few AGENTS.md hygiene issues and one inconsistency that should be addressed before merge:
- The changeset reads like a PR description: it includes internal mechanics (
meta.thumbnail,getEmbed, browser version notes) and file-level narrative. A changeset is user-facing release notes, not a summary of the diff. - Several new inline comments are change summaries aimed at the reviewer rather than at future readers of the code. AGENTS.md asks us to avoid PR narrative and justification in comments.
- The
meta.sizefallback added inproviderItemToMediaItemdoes not reach the provider list-row size display, so the list view still shows"—"for items that only report size inmeta. - The library grid/list thumbnail fix is not covered by a test; the new tests are all for the detail panel and the Cloudflare embed, with nothing guarding the core "non-image provider items should render a poster" behavior.
No security, data-integrity, or query-count issues were introduced. I’m not blocking merge, but the changeset and comments need cleanup.
Findings
-
[needs fixing]
.changeset/stream-provider-video-preview.md:7-16The changeset is written as a PR description for reviewers, not as user-facing release notes. It lists internal mechanics (
getEmbed,meta.thumbnail,EmDashMedia/barrel re-export plumbing, browser version notes) and justifies implementation choices. AGENTS.md says a changeset is “release notes a user reads while upgrading,” should lead with a present-tense verb describing the observable effect, and should leave out internal mechanics, file names, and implementation narrative.--- "@emdash-cms/admin": patch "@emdash-cms/cloudflare": patch "emdash": patch --- Fix media previews for streaming providers such as Cloudflare Stream. Streaming items expose playback URLs via `meta.playback` and a poster via `previewUrl`. The media library now shows posters for these items, and the detail panel plays the HLS/DASH sources instead of treating the poster as the video file. `Media` is now exported from `emdash/ui` for frontend rendering. -
[suggestion]
packages/admin/src/components/MediaLibrary.tsx:770-771This JSX comment is a change summary aimed at the reviewer, not at a future reader of the code. The condition
{item.previewUrl ? <img … /> : <icon />}already makes the behavior clear. Per AGENTS.md, comments should not narrate the PR or explain what was changed.<div className="aspect-square"> {item.previewUrl ? ( <img src={item.previewUrl} alt={item.alt || item.filename} className="h-full w-full object-cover" onLoad={handleImageLoad} /> ) : ( -
[suggestion]
packages/admin/src/components/MediaLibrary.tsx:870Same as the grid-item comment: this is a pointer to the PR change rather than everlasting context for code readers. The guard has been removed, so the comment can go.
{item.previewUrl ? ( <img src={item.previewUrl} alt={item.alt || item.filename} className="h-full w-full object-cover" onLoad={handleImageLoad} /> -
[suggestion]
packages/admin/src/components/MediaDetailPanel.tsx:255-257This comment restates the branch condition and the source list that follows, and it includes implementation justification (“Browsers demux HLS natively; DASH is a fallback”) that belongs in the changeset/PR description. The code already expresses that streaming items use
sources. Remove the comment and rely on themetaPlaybackname and the<source>elements.) : isVideo && playback ? ( <video poster={item.url || undefined} controls preload="metadata" className="max-h-full max-w-full" > -
[suggestion]
packages/cloudflare/src/media/stream-runtime.ts:289-292This block comment explains why the implementation changed (
list()/get()don’t setmeta.thumbnail) rather than giving a non-obvious invariant a future reader needs. The expressionvalue.previewUrl ?? toString(value.meta?.thumbnail)is self-explanatory about precedence. Drop the narrative comment.poster: value.previewUrl ?? toString(value.meta?.thumbnail), -
[suggestion]
packages/core/src/ui.ts:48-51The comment at this public re-export is mostly a comparison to
Imageand a warning about whatImagedoes wrong. That is PR justification, not a docstring. Keep a short, forward-looking description of whatMediadoes.// Unified media component that delegates rendering to the provider's getEmbed(). EmDashMedia as Media, -
[needs fixing]
packages/admin/src/components/MediaLibrary.tsx:888The changeset says provider items may report size in
meta, andproviderItemToMediaItemnow falls back tometa.sizefor the detail panel. However, the provider list row still only reads the top-levelitem.sizeand shows"—"when it is missing, even ifmeta.sizeexists. This makes the list and detail inconsistent for the same item. Use the samemetaNumberfallback here (and import it from../lib/media-utils).import { providerItemToMediaItem, getFileIcon, formatFileSize, getMediaThumbnailUrl, fallbackToOriginalThumbnail, MEDIA_THUMBNAIL_WIDTH, metaNumber, } from "../lib/media-utils"; function ProviderListItem({ item, selected, onClick, onDimensionsLoaded }: ProviderListItemProps) { const { t } = useLingui(); const size = item.size ?? metaNumber(item.meta, "size"); const handleImageLoad = (e: React.SyntheticEvent<HTMLImageElement>) => { const img = e.currentTarget; if (onDimensionsLoaded && (!item.width || !item.height)) { onDimensionsLoaded(img.naturalWidth, img.naturalHeight); } }; return ( <tr className={cn( "cursor-pointer transition-colors", selected ? "bg-kumo-brand/10" : "hover:bg-kumo-tint/25", )} onClick={onClick} > <td className="px-4 py-3"> <div className="h-10 w-10 overflow-hidden rounded"> {item.previewUrl ? ( <img src={item.previewUrl} alt={item.alt || item.filename} className="h-full w-full object-cover" onLoad={handleImageLoad} /> ) : ( <div className="flex h-full w-full items-center justify-center bg-kumo-tint text-xl"> {getFileIcon(item.mimeType)} </div> )} </div> </td> <td className="px-4 py-3 text-base font-medium leading-5">{item.filename}</td> <td className="px-4 py-3 text-sm text-kumo-subtle">{item.mimeType}</td> <td className="px-4 py-3 text-sm text-kumo-subtle tabular-nums"> {size ? formatFileSize(size) : "—"} </td> -
[suggestion]
packages/admin/src/components/MediaLibrary.tsx:770The grid/list thumbnail fix is not covered by tests. This is the core behavior being changed—provider-backed video/audio items with a
previewUrlshould render a poster in the library—and it is currently unguarded against a future regression that re-adds animage/*gate or otherwise drops the poster. Add a focused test forProviderGridItem/ProviderListItem(or forMediaLibrarywith mocked provider items) asserting that a non-image item withpreviewUrlrenders the expected<img>.
| function isRecord(value: unknown): value is Record<string, unknown> { | ||
| return typeof value === "object" && value !== null; | ||
| } |
There was a problem hiding this comment.
let me know if we have something for this already ie zod
There was a problem hiding this comment.
small enough tho, im fine with this approach
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
github-actions
Bot
added
cla: signed
review/needs-rereview
There was a problem hiding this comment.
Re-review of PR #2463: fix/admin-provider-video-preview.
All of my prior findings have been addressed:
- Changeset — rewritten as concise, user-facing release notes that describe the observable fix rather than the implementation mechanics.
- Narrative comments — the PR-summary-style JSX comments in
MediaLibraryare gone; the remaining comments inMediaDetailPanelandstream-runtime.tsnow state non-obvious invariants (item.urlis the poster for streaming items,previewUrlis preferred over the legacymeta.thumbnailfallback) and are acceptable under AGENTS.md. - Provider list size —
ProviderListItemnow readsitem.size ?? metaNumber(item.meta, "size"), so the list and detail panel are consistent. - Grid/list poster tests —
MediaLibrary.test.tsxnow asserts that a non-image provider item withpreviewUrlrenders the poster and thatmeta.sizesurfaces in the list view.
I re-read the changed files against AGENTS.md. The implementation remains the right approach: metaPlayback cleanly separates streaming sources from direct file URLs, the detail panel branches correctly between streaming and local video, the Cloudflare Stream getEmbed poster falls back correctly, and Media is re-exported from emdash/ui without any barrel-file side effects. No new security, data-integrity, logged-out query-count, i18n, RTL, or SQL-safety issues were introduced.
The diff is clean and the PR is ready to merge.
github-actions
Bot
added
review/approved
Streaming providers such as Cloudflare Stream have no directly playable
file URL: the poster is reported as `previewUrl` and playback lives in
`meta.playback` as HLS/DASH. Consumers assumed a flat, fetchable `src`,
so provider-backed video rendered no thumbnail in the library and a
permanently-0:00 player in the detail panel.
- Stop gating library thumbnails on an `image/*` mime type; `previewUrl`
is a poster that video items have too.
- Play a streaming item's HLS/DASH sources in the detail panel, using
its thumbnail as the poster. Locally stored video is unchanged.
- Read `size` from `meta` when it is not a first-class field, so
streaming assets report a real file size.
- Prefer `previewUrl` for the poster in the Stream provider's
`getEmbed()`. `list()`/`get()` never set `meta.thumbnail`, so posters
were dropped for every value the media picker produced.
- Export `EmDashMedia` from `emdash/ui` as `Media`. It was exported from
the internal components barrel but never re-exported from the public
entrypoint, and there is no `./components` subpath, so user code had
no way to render provider-backed video. `Image` is not a substitute:
it only handles `embed.type === "image"`.
No player library is needed. Chromium and Safari both report
`canPlayType("application/x-mpegURL") === "maybe"` and demux a Stream
manifest natively, verified on Chrome 150 and Chromium 148 (readyState
4, correct duration, currentTime advancing). The DASH source is a
declarative fallback only, as Chromium reports no native support for
`application/dash+xml`.
The fix had no coverage: existing media tests only exercised images and provider deletion. Eight tests, scoped to behaviour this change introduced rather than the surrounding surface. - Detail panel emits HLS/DASH `<source>` elements with the thumbnail as `poster`, and specifically does not use the poster URL as `src` -- that was the defect behind the 0:00 player. A locally stored video keeps asserting the plain `src` path so the new branch cannot regress it. - `metaPlayback` reads the shape Stream reports, and returns undefined for a plain uploaded file, which is what keeps local video on the direct-file path. - `providerItemToMediaItem` falls back to `meta.size`, the reason Stream assets used to display "0 B", and carries `meta` through so the panel can find playback at all. - The Stream provider's `getEmbed()` takes its poster from `previewUrl`, still honouring a legacy `meta.thumbnail` when that is absent. Each assertion was checked against the pre-fix sources: the detail panel cases report the poster URL where null was expected and find no <source> elements, and `getEmbed()` returns no poster at all.
A provider list row showed a dash for any item whose byte size arrives under `meta` rather than as a first-class field -- every Cloudflare Stream asset. The detail panel already fell back to `meta`, so the same asset reported a real size in the panel and none in the list. - Fall back to `meta.size` in provider list rows, matching the panel. - Cover the library poster and the list-row size with tests; the grid and list poster change had none. Both assertions were checked against the pre-fix sources. - Cut the explanatory comments down to the facts a reader needs, and rewrite the changeset as user-facing release notes.
5c82cba to
ddaa948
Compare
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
This is a focused, appropriate fix for a real bug: streaming-provider items have a poster in previewUrl/ url and playable sources in meta.playback, so using item.url as a video src produced a stalled player. The implementation correctly distinguishes streaming from direct-file video, falls back meta.size → meta.size, and re-uses the existing EmDashMedia component by exporting it as Media from emdash/ui.
I re-read the full diff and the changed files against the prior review context. All of the previous round’s findings remain addressed:
- Changeset is concise, user-facing release notes.
- Inline comments in
MediaDetailPanelandstream-runtime.tsdescribe non-obvious invariants and are narrowly scoped. - Provider list size now falls back to
meta.sizeconsistently. - Grid/list poster tests assert the observable rendered poster and size, not just implementation details.
I also checked the new code for AGENTS.md conventions: no new hard-coded UI strings, no physical Tailwind directional classes, no SQL/route/auth/db changes, no logged-out query additions, and no stale narrative comments. EmDashMedia is already exported from packages/core/src/components/index.ts, so adding the re-export in packages/core/src/ui.ts does not pull in any new runtime cost.
No new static issues were introduced. The PR is ready to merge.
github-actions
Bot
added
review/approved
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
4 participants
What does this PR do?
Fix media previews for streaming providers such as Cloudflare Stream, whose items have no directly playable file URL
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure