Offline article-image caching — Codex review vehicle#1
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acba1639da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| d["body"] = body | ||
| if AppDefaults.shared.cacheImagesForOffline { | ||
| // Tell main.js to route article images through the offline cache (see rewriteImagesForOfflineCache). | ||
| d["body"] = "<script>window.nnwCacheImagesForOffline=true;</script>" + body |
There was a problem hiding this comment.
When the user has disabled article JavaScript, WebViewConfiguration.webpagePreferences sets allowsContentJavaScript from that preference, so this inline <script> in the article body will not run. In that configuration main.js still checks window.nnwCacheImagesForOffline, sees it unset, and never rewrites images to nnwimage://..., making the new offline-image setting ineffective for users who keep content JavaScript off.
Useful? React with 👍 / 👎.
| let homePagesWithNoIconURL = tempDir.appendingPathComponent("HomePagesWithNoIconURLCache.plist") | ||
|
|
||
| for tempItem in [faviconsFolderURL, imagesFolderURL, feedURLToIconURL, homePageToIconURL, homePagesWithNoIconURL] { | ||
| for tempItem in [faviconsFolderURL, imagesFolderURL, articleImagesFolderURL, feedURLToIconURL, homePageToIconURL, homePagesWithNoIconURL] { |
There was a problem hiding this comment.
Including ArticleImages in the existing three-day image-cache purge means a connected launch can delete all previously prefetched article images. Since the new prefetcher only observes newly downloaded articles, older saved/unread articles are not repopulated after this purge unless each is opened while online, so those articles can unexpectedly lose offline images despite the offline-cache setting being enabled.
Useful? React with 👍 / 👎.
c762b1c to
e4a6df7
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 227d7159f2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| document.querySelectorAll("img").forEach(element => { | ||
| var src = element.src; | ||
| if (/^https?:\/\//i.test(src)) { | ||
| element.src = "nnwimage://cache/?u=" + encodeURIComponent(src); |
There was a problem hiding this comment.
When offline caching is enabled, this replaces every HTTP(S) img.src with an nnwimage URL. On iOS the tap-to-zoom path posts this.img.src from iOS/Resources/main_ios.js, and WebViewController.imageWasClicked passes that URL to Downloader.shared.download, which skips non-http(s) URLs, so tapping any rewritten article image silently fails instead of opening the full-screen viewer. Store/send the original URL, or teach the viewer to load through the cache scheme, before rewriting src.
Useful? React with 👍 / 👎.
| guard let html = article.contentHTML, !html.isEmpty else { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The prefetch inputs are built only from article.contentHTML, but the renderer displays article.body, which falls back to contentText and then summary when contentHTML is nil. For feeds/accounts that store rendered markup in the summary, refresh-time prefetching never enqueues their <img> URLs, so those newly downloaded articles still have broken images offline unless they were opened online first. Use the same body fallback that rendering uses when extracting prefetch URLs.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1cbf18cba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| document.querySelectorAll("img").forEach(element => { | ||
| var src = element.src; | ||
| if (/^https?:\/\//i.test(src)) { | ||
| element.src = "nnwimage://cache/?u=" + encodeURIComponent(src); |
There was a problem hiding this comment.
When an article image uses srcset (common for responsive images), this rewrites only src and leaves the srcset candidates pointing at the original https URLs. WebKit source selection can continue choosing those candidates, so those images bypass the nnwimage scheme and still fail when the article is opened offline despite the offline-image setting. Clear or rewrite srcset alongside src so every selected image URL goes through the cache.
Useful? React with 👍 / 👎.
| let homePagesWithNoIconURL = tempDir.appendingPathComponent("HomePagesWithNoIconURLCache.plist") | ||
|
|
||
| for tempItem in [faviconsFolderURL, imagesFolderURL, feedURLToIconURL, homePageToIconURL, homePagesWithNoIconURL] { | ||
| for tempItem in [faviconsFolderURL, imagesFolderURL, articleImagesFolderURL, feedURLToIconURL, homePageToIconURL, homePagesWithNoIconURL] { |
There was a problem hiding this comment.
When a user has cached images for offline reading, launching the app after the three-day flush interval while connected deletes the entire ArticleImages folder. The automatic prefetch path only handles newly downloaded articles, so existing unread articles that had been cached for offline use lose their images until the user manually runs “Cache All Images Now” again or opens each article online. Treat this user-facing offline store differently from the transient favicon/image caches, or recache unread articles after purging.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e74c2d75d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for index in start..<end { | ||
| let url = urlStrings[index] | ||
| tasks.append(Task { @MainActor in | ||
| _ = await ArticleImageDownloader.shared.data(for: url, allowNetwork: true) |
There was a problem hiding this comment.
When a user starts “Cache All Images Now” and then turns “Cache Images for Offline” off before it finishes, this loop keeps launching later batches with allowNetwork: true because runCacheAll() never re-checks the setting. That continues using network and writing article images to disk after the user has opted out; normal refresh prefetching has an isPrefetchingEnabled gate, but this cache-all path bypasses it.
Useful? React with 👍 / 👎.
- Ranchero-Software#4 (unbounded cache): the ArticleImages store is kept out of the routine 3-day flush while offline caching is on, so give it its own bound — a 500 MB total-size cap that evicts the oldest cached images. Enforced at startup, when a refresh prefetch burst drains, and after a "Cache All Images Now" run. - #1 (redirect bypass): the content blocker only saw the initial URL, but URLSession auto-follows redirects, so an allowed URL could 302 to a blocked tracker. Add an optional redirect policy to the shared RSWeb.Downloader (a URLSession redirect delegate) and wire it to ArticleImageContentBlocker so a redirect to a blocked domain isn't followed. Applies to all one-shot Downloader traffic (favicons/feed icons too) — a net privacy win; feed refresh uses a separate session and is unaffected. ArticleImageContentBlocker.isBlocked is now nonisolated (immutable, thread-safe regex) so the off-main redirect delegate can consult it.
5b2e51c to
bff01e2
Compare
- Ranchero-Software#4 (unbounded cache): the ArticleImages store is kept out of the routine 3-day flush while offline caching is on, so give it its own bound — a 500 MB total-size cap that evicts the oldest cached images. Enforced at startup, when a refresh prefetch burst drains, and after a "Cache All Images Now" run. - #1 (redirect bypass): the content blocker only saw the initial URL, but URLSession auto-follows redirects, so an allowed URL could 302 to a blocked tracker. Add an optional redirect policy to the shared RSWeb.Downloader (a URLSession redirect delegate) and wire it to ArticleImageContentBlocker so a redirect to a blocked domain isn't followed. Applies to all one-shot Downloader traffic (favicons/feed icons too) — a net privacy win; feed refresh uses a separate session and is unaffected. ArticleImageContentBlocker.isBlocked is now nonisolated (immutable, thread-safe regex) so the off-main redirect delegate can consult it.
bff01e2 to
4748ec9
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4748ec9b18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if forHTML { | ||
| out.append(lt) | ||
| out.append(contentsOf: utf8[tagStart..<tagEnd]) |
There was a problem hiding this comment.
When forHTML is true this now copies the entire original tag body for any allowed tag once only the leading name matches. Feed titles are untrusted and this sanitized value is inserted directly into the article-title HTML (Shared/Article Rendering/template.html:43); with article content JavaScript enabled by default, a title like <abbr onclick="...">x</abbr> preserves the event handler and can run it on interaction. Before this change, allowed tags carrying attributes were escaped because the whole tag body did not match the allowlist, so please strip or whitelist attributes (for example only abbr's title) before preserving the tag.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4748ec9b18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| d["body"] = body | ||
| if AppDefaults.shared.cacheImagesForOffline { | ||
| // Tell main.js to route article images through the offline cache (see rewriteImagesForOfflineCache). | ||
| d["body"] = "<script>window.nnwCacheImagesForOffline=true;</script>" + body |
There was a problem hiding this comment.
When the user has disabled article JavaScript, this inline <script> is treated as page/content JavaScript and will not run because the web view sets allowsContentJavaScript from AppDefaults.shared.isArticleContentJavascriptEnabled in WebViewConfiguration. In that configuration, main.js still runs as the app's user script but never sees window.nnwCacheImagesForOffline, so enabling offline image caching has no effect for users who keep article JS off. Pass this flag through an app/user-script path instead of embedding it in the article HTML.
Useful? React with 👍 / 👎.
| document.querySelectorAll("img").forEach(element => { | ||
| var src = element.src; | ||
| if (/^https?:\/\//i.test(src)) { | ||
| element.src = "nnwimage://cache/?u=" + encodeURIComponent(src); |
There was a problem hiding this comment.
For articles that use <picture><source srcset=...><img src=...>, this only rewrites the fallback img while leaving the matching source candidates pointed at their original https URLs. WebKit will select the source URL instead of the rewritten nnwimage fallback, so those images bypass the offline cache and remain broken offline even though the fallback img was cached. The rewrite needs to disable or rewrite the relevant source/srcset entries when the fallback image is routed through the cache.
Useful? React with 👍 / 👎.
Optional, off-by-default caching of the images embedded in article HTML: prefetched during refresh and served from a local cache at render time, so articles can be read with their images while offline. Addresses Ranchero-Software#1873; a step toward the unified image handling in Ranchero-Software#3460. - ArticleImagePrefetcher observes .AccountDidDownloadArticles and, for each new article, extracts its <img> URLs (off the main thread, via the unit-tested ArticleImageURLExtractor) and fetches them through ArticleImageDownloader into a disk cache, throttled rather than in one unbounded burst. - With the setting on, main.js / main_ios.js rewrite http(s) <img src> to nnwimage://cache/…; ArticleImageSchemeHandler resolves those memory → disk → network. The img's srcset — and, inside a <picture>, its <source> candidates — are dropped on rewrite so WebKit can't select an original URL that bypasses the cache offline. The enable flag is passed to main.js as a hidden DOM marker (not an inline <script>) so it still applies when the reader has article JavaScript turned off. - ArticleImageDownloader mirrors ImageDownloader/FaviconDownloader (full-size bytes in a separate ArticleImages folder, ~50MB NSCache, 20MB disk-write cap with image-type sniffing). App-side fetches reuse the web view's content blocker via ArticleImageContentBlocker, so tracker/ad URLs stay blocked — including on cross-host redirects. The folder is swept by the 3-day CacheCleaner and purged outright when the setting is off. - Prefetch only catches future refreshes, so switching the setting on offers to backfill images for the current unread articles right away (a "Cache Images Now? / Not Now" prompt). There's no standalone button — toggle off and on to backfill again later. A progress line and the current cache size appear under the setting, and in the iOS account-stats screen. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cu69KNiXXmKjfmgcnS8RZU
resizeWindow(toFitView:) sized the (non-resizable) Preferences window to each pane's fixed storyboard frame, so a pane whose content outgrew that frame — like General after gaining the offline-images rows — was clipped off the bottom. Measure the pane's real Auto Layout fitting height at the fixed window width instead — before the view is added to the window hierarchy, so its height is free — and size the window to that, letting panes grow to fit rather than squishing or clipping their last control. A <2000pt clamp guards against the old unbounded-width measurement ballooning the multiline label. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cu69KNiXXmKjfmgcnS8RZU
4748ec9 to
fbc8c46
Compare
1 participant
Internal PR on the fork solely to run
@codex review(the Codex GitHub App can't be installed on the upstream repo). The real proposal is Ranchero-Software#5352 — same diff, same head commit. Safe to close after review.