Preserve media embedded in forecast discussions and avalanche problems - #1228
Merged
Conversation
The forecast sanitizer allowed `img` but not `iframe`, so a YouTube or Facebook video a forecaster embedded in a discussion was deleted with no trace, and the `data-video-id` that marks an embedded-video figure was stripped along with it. On a life-safety page, dropping a forecaster's video silently is the wrong failure. Allow `iframe` behind an explicit provider allowlist: YouTube and Vimeo through the existing `videoEmbed` helpers, plus Facebook's `/plugins/video.php` and `/plugins/post.php` over https. `allowedIframeHostnames` is not enough on its own — it deletes the `src` and leaves an empty frame — so the check lives in `transformTags`, which renames anything that fails to a tag outside the allowlist. Surviving frames get a fixed attribute set and are sized by aspect ratio rather than by the authored pixel height, which letterboxes once the column is narrower than the author's screen. An embed from a provider we won't frame becomes a link labelled with its hostname rather than disappearing. Also strip inline `height` from every forecast image. The declared width is honored up to the column width, and a surviving pixel height then stretches the image; the legacy widget forced `height: auto !important` on all of them for the same reason. `MediaSlide` now builds its YouTube URL with `getVideoEmbedUrl` too, so the same video frames the same way inline and in the lightbox. That moves the lightbox to youtube-nocookie.com with `rel=0`, matching the Gallery block. Fixtures in the tests are verbatim markup from the live v2 API. Refs #1214 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Photos a forecaster embeds in a discussion rendered as static images: no expand affordance, not zoomable, and a video figure — a poster image carrying a `data-video-id` — was indistinguishable from a photo with no way to watch it. The legacy widget put both into its lightbox. `collectEmbeddedMedia` finds those figures in the rendered DOM and builds the same `MediaItem` shape the widget built, so they can go through the existing `MediaLightbox`. Reading the DOM rather than splitting the HTML string is what the widget did, and the corpus says why it matters: of 2,261 real figures, 13 are nested inside a wrapper or another figure's `figcaption`, and one center wraps its whole discussion in a `div`, so nothing there is ever top-level. Querying the tree finds all 2,261. `DiscussionBody` still renders the discussion in one `dangerouslySetInnerHTML`, so server rendering and the no-JS rendering are unchanged, then marks up the figures after mount and portals in the legacy affordances — an expand chip, and a play button on videos. The rendered HTML sits behind `memo` because React compares the `dangerouslySetInnerHTML` wrapper by identity: re-rendering it, as opening the lightbox does, would rewrite the subtree and strand every overlay in a detached node. Only the small chip is a button. The rest of the overlay is `pointer-events: none`, as the legacy icons were, so an image keeps its own context menu; mouse users open the lightbox by clicking anywhere on the figure. Being a button is also what makes the figure keyboard-reachable — the legacy widget was click-only. Refs #1214 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`getVideoEmbedUrl` rebuilds the URL from the video id alone, which is fine for a plain video and wrong for a playlist: `/embed/videoseries?list=…` loses its list and, because `videoseries` is itself eleven characters, sails through the video-id check and asks YouTube for a video by that name — a broken player, with no sign anything went wrong. A start offset and a Vimeo private-video hash disappear the same way, more quietly. Carry `list`, `index`, `start`, `end`, `t` and `h` onto the rebuilt URL. The `si` share token still goes. Nothing in the corpus embeds a playlist today; this is about the next one. Refs #1214 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A browser never renders an `<iframe>`'s children — they are fallback for clients that can't frame — but htmlparser2 reads them as live markup. A blocked embed was being renamed to a real `<a>`, and `nonTextTags` only governs tags the sanitizer disallows, so those children survived: `<iframe src="https://evil/x"><figure class="afp-photoswipe">…` came out the other side as a figure, which `collectEmbeddedMedia` then queries for and puts in the lightbox. Content the legacy widget would never have shown, reaching a life-safety page. Keep the blocked embed on the dropped tag, which `nonTextTags` does govern, and carry the note as that transform's `text` — sanitize-html emits a discarded tag's text while discarding its children. The note is now inert rather than a link. The URL just failed the allowlist; the reader should know something was there without being one click from it. Refs #1214 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`problemPhoto` returned null for anything that wasn't an image or a photo, so a video attached to an avalanche problem rendered as nothing at all — no poster, no caption, no indication a forecaster had put one there. The legacy widget shows the poster frame with a play glyph over it and opens it in the lightbox. Across 4,545 sampled products, 1,836 problems carry media and 111 of those are videos, including recent CNFAIC, SNFAC and NWAC forecasts. All of them are YouTube: 107 as an object with a `video_id` and its own poster frame, and 4 — SNFAC, 2020 — where `url` is a bare string that is itself the YouTube id. Handle both, and route the media through the lightbox, so a problem's example photo is zoomable too rather than a static image. `getYouTubeVideoId` now reads the bare-string shape, matching the legacy widget's `image.url?.video_id ? … : "…/embed/" + image.url`. That shape reached `resolveMediaSlide` as "Unsupported media type" before, so this also fixes those four in the forecast media lightbox. A video with no poster of its own falls back to YouTube's; the legacy widget has a helper for that but never calls it, and renders a broken image instead. The expand/play affordance moves to a shared MediaOverlay, now that the discussion and the problem card both draw it. The jsdom stubs both of their tests need move to a shared client setup file rather than being copied per suite. Refs #1214 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`/embed/live_stream?channel=…` fails exactly like the playlist case: `live_stream` is eleven characters, so it passes for a video id and the rebuilt URL drops the channel it was meant to play. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Preview deployment: https://busbykxembedded-media.preview.avy-fx.org |
Contributor
|
Here's a visual recap of what changed: Open the full interactive recap |
This was referenced Aug 26, 2026
busbyk
added a commit
that referenced
this pull request
Aug 26, 2026
#1228 changed where a forecast page goes for a video. `getVideoEmbedUrl` frames YouTube at `youtube-nocookie.com` rather than `youtube.com`, a video poster now comes from `i.ytimg.com`, and the sanitizer lets a forecaster's own `iframe` through for Vimeo and Facebook as well. `stubExternalAssets` still routed only `www.youtube.com`. Nothing fails when a host is missing from that list — the suite just quietly stops being hermetic and starts loading the real provider, which is the failure the whole harness is built to rule out. The lightbox's YouTube assertion moved for the same reason: it matched `youtube.com/embed/`, which the nocookie host no longer contains. It is skipped pending a fixture, so it would have gone stale unnoticed until the capture landed and then failed for a reason unrelated to the capture. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
busbyk
added a commit
that referenced
this pull request
Aug 26, 2026
#1228 restored four things the native forecast page was dropping: an embedded photo is zoomable, an embedded video plays, a pasted provider iframe frames inline, and an embed we will not frame leaves a note naming the provider. None of it is reachable from the corpus — no golden's authored HTML carries a `figure` or an `iframe` at all — so the specs are written and skip themselves with the Case they wait on, as the rest of the blocked suite does. That capture is a different kind of gap from the ones already listed, and the doc now says so: the others are wire-level, so any product of the right shape will do, while this one is about markup a forecaster typed, which no schema requires. It belongs with the shape-variant work in #1209. One test here is not about embedding. `DiscussionBody` delegates clicks from the whole discussion subtree, so a link inside it is the one thing that has to escape the handler rather than open the lightbox. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
busbyk
added a commit
that referenced
this pull request
Aug 26, 2026
CI already runs this suite on every PR, so a change that breaks an existing spec fails on its own. What CI cannot say is that a surface added yesterday has no spec at all — which is how #1228 shipped four new reader-facing behaviours, each well covered by unit tests, with nothing here. Two failure modes are worth naming because neither shows up red: a third-party host missing from `stubExternalAssets` loads for real and the suite still passes, and an assertion inside a skipped spec is still asserting whatever was true the day it was written. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Description
Forecaster-authored HTML can embed media, and the native forecast page was dropping it:
sanitizeHtmlallowedimgbut notiframe, so a pasted YouTube or Facebook video was deleted with no trace, and thedata-video-idmarking an embedded-video figure went with it — leaving a poster indistinguishable from a photo. A video attached to an avalanche problem rendered as nothing at all, becauseproblemPhotoreturnednullforMediaType.Video. This restores what the legacy widget does, on AvyWeb's design system.I sampled 6,745 v2 products across all 29 centers' full history rather than guessing at the markup: 2,259
afp-photoswipefigures, 40 iframes (36 YouTube, 4 Facebook), 2afp-video-modalfigures, and — separately — 111 problem-level videos among the 1,836 problems that carry media, all of them YouTube.Related Issues
Closes #1214 — Preserve discussion-embedded media (videos and zoomable images). Inventory row D3.
Also fixes the same defect on a problem's example media, which had no issue of its own.
Key Changes
iframeallowed behind an explicit provider allowlist (YouTube/Vimeo via the existingvideoEmbedhelpers, plus Facebook'svideo.php/post.phpover https), sized by aspect ratio rather than the authored pixel height.allowedIframeHostnamesisn't enough on its own — it only deletes thesrcand leaves an empty frame — so the check lives intransformTags.nonTextTagsdiscards its children: an<iframe>'s children are fallback a browser never renders, but the parser reads them as live markup.heightis stripped from every forecast image, matching the widget's.tinymce-img { height: auto !important }.collectEmbeddedMediafinds embedded figures in the rendered DOM and feeds the existingMediaLightbox. Reading the DOM rather than splitting the HTML matters — 13 of 2,261 real figures are nested, and one center wraps its whole discussion in adiv, so nothing there is ever top-level.getYouTubeVideoIdnow also reads the bare-string shape, which four SNFAC forecasts use and which previously showed "Unsupported media type".How to test
pnpm testcovers the sanitizer (28 cases, fixtures are verbatim markup from the live v2 API), the DOM collector, and the rendering. By hand, on a center with the native forecast flag on: an embedded photo shows an expand chip and opens zoomable; an embedded video shows a play button and plays in the lightbox; a pasted YouTube/Facebook iframe plays inline; right-clicking a poster still opens the image's own context menu, since only the small chip is a button.Screenshots / Demo video
All four are real markup from the live v2 API, rendered on a local tenant.
A discussion with embedded figures. The chart carries an expand chip in its bottom-left corner; the video below it shows a play button. Before this change both rendered as plain static images, and the video was indistinguishable from a photo — its
data-video-idwas stripped by the sanitizer, so there was no way to tell it was a video or to watch it. Sources: BTAC 184960 (chart), SNFAC 109444 (video).A pasted YouTube iframe, playing inline. The sanitizer allowed
imgbut notiframe, so this was deleted outright with no trace. Below it, an embed from a provider we won't frame now leaves an inert note naming the provider rather than vanishing. Source: TAC 181191.The same discussion video in the shared lightbox, with its caption, matching what the legacy widget does on click. Source: SNFAC 109444.
A video attached to an avalanche problem. This rendered as nothing at all before — no poster, no caption, no sign a forecaster had attached one. 111 of the 1,836 problems that carry media are videos, including recent CNFAIC, SNFAC and NWAC forecasts. Source: BAC 127432.
No console errors in any of these. They're viewport captures rather than full-page ones because Chrome blanks cross-origin iframes in full-page screenshots.
Migration Explanation
None — no schema change.
Future enhancements / Questions
Deliberate divergences from the widget, worth a look:
AvalancheProblemCard, which would pullsanitize-htmlinto the client bundle.youtube-nocookie.comwithrel=0, which is whatgetVideoEmbedUrlproduces and what the Gallery block already ships. This also changesMediaSlide; two existing assertions were updated.allowpolicy replaces whatever was authored (32 of the 40 real iframes authored none). Nosandbox: thesrcis one of a handful of vetted provider URLs, and a wrong flag fails as a blank frame.Left alone, each worth its own issue:
align-centeron a figure does nothing here (22 real figures; the widget centers them), andWarningBannerrenders forecaster HTML that the widget escapes with{{ }}— pre-existing, now slightly wider.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.