Repository navigation
Show inline video embeds (YouTube, Vimeo) in the reader - #80
Merged
Merged
Conversation
Readability keeps a page's video <iframe>s, but the reader's DOMPurify call stripped every iframe, so a saved article silently lost its player while the images survived. Allow iframes only for the embed hosts Readability itself preserves (YouTube, youtube-nocookie, Vimeo, Twitch, Dailymotion, v.qq, archive.org/wikimedia), and keep the player attributes (allow, allowfullscreen, frameborder). Arbitrary, srcdoc, and javascript: iframes stay blocked. Extract the policy into src/utils/article/sanitize.ts with unit tests covering allowed and rejected embeds.
✅ Deploy Preview for savrlist ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
jonocodes
added a commit
that referenced
this pull request
Oct 4, 2026
Conflicts: - ArticleComponent.tsx: #80 moved sanitising into sanitizeArticleHtml (video-embed iframe allow-list) but still passed a fresh `{ __html }` literal each render, so the DOM-rebuild bug remained on main. Keep #80's sanitiser and this branch's memoization: useMemo(() => ({ __html: sanitizeArticleHtml(html) }), [html]). - CHANGELOG.md: keep both; this branch's entries move under #80's 2026-10-03 heading since they land after it. With embeds now rendered the rebuild also reloaded every player on each save: on main's code 3 embeds were loaded 18 times over 3 reading pauses. Added a scroll-stability test pinning one load per embed (fails on main's version of the line, passes with the memoization). Adjusted for #79, which swaps failed images for a local placeholder: - The failed-download dimensions test now expects the placeholder and data-orig-src, and still asserts no width/height is claimed. - The shared jest mock of ~/utils/article/tools gains a FetchError class. ingestion.ts now does `e instanceof FetchError` on a failed image, and without it the check throws; no existing test ran a failed download through ingestHtml, so the gap was latent. - Comment and changelog wording: only articles saved before #79 still hold remote URLs for failed images. Co-Authored-By: Claude Opus 5.5 <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.
Problem
Saved articles showed their images but silently lost any embedded video — not even a link. Readability deliberately keeps a page's video
<iframe>s (itsREGEXPS.videos), so the player survived scraping, but the reader'sDOMPurify.sanitize(...)call only allowedlink, and DOMPurify drops every iframe by default.Fix
src/utils/article/sanitize.ts, which allows<iframe>only for the hosts Readability itself preserves: YouTube, youtube-nocookie, Vimeo (player.vimeo.com), Twitch, Dailymotion,v.qq.com, archive.org / upload.wikimedia.org. Arbitrary,srcdoc, andjavascript:iframes stay blocked.allow,allowfullscreen,frameborder,scrolling,referrerpolicy).ArticleComponentnow renders throughsanitizeArticleHtml; script stripping is unchanged.Because sanitizing happens at render time, already-saved articles start showing their videos too — their stored
content.htmlalready contains the iframe.Tests
src/utils/article/sanitize.test.ts(21 tests): allows YouTube/Vimeo/Twitch/Dailymotion/archive.org, rejects non-video,srcdoc,javascript:, and empty sources, and keeps script stripping + stylesheet links intact.npx jest→ 348 passed ·npx tsc --noEmitclean · lint 0 errors.Notes
Video needs the network, so embeds won't play offline (text/images still do).