feat(cli): capture falls back to the well-known icon paths when a page declares none - #3599
Open
miguel-heygen wants to merge 2 commits into
Open
feat(cli): capture falls back to the well-known icon paths when a page declares none#3599miguel-heygen wants to merge 2 commits into
miguel-heygen wants to merge 2 commits into
Conversation
Capture drops assets for four reasons and reported none of them, so a folder
with thirty images and a folder truncated to thirty images were the same
object. Every drop site was a bare `continue`, `break`, `return null` or an
empty `catch`, and the only signals downstream were two hand-written warning
strings that fired when the budget was already gone before a download pass
started, which is the one case where the pass could not say how much it lost.
`downloadAssets` and `downloadAndRewriteFonts` now return an `AssetDropCounts`
tally beside their result, incremented at the single line that performs each
drop. `capture --json` carries it as `dropped`; the human summary prints a
`Dropped:` line when it is non-zero.
Four reasons, three decisions and one failure:
size-floor fetched, then judged too small to be a real asset
budget-exhausted the post-navigation clock ran out before this one
cap-reached 30 inline SVGs, 30 fonts, or 6 faces per family
unavailable the request or the write failed
A break now counts everything it did not reach rather than the one it stopped
on, because "how many did we lose" is the question and one is never the answer.
The two budget warnings are gone. Both existed only to cover the case where
the budget ran out before a pass was called, so both passes are now called
unconditionally: a zero budget makes each loop break on its first item and
record `budget-exhausted` for the rest, which costs no network and produces a
real number instead of the word "some". The single remaining warning is derived
from the tally, so the prose and the count cannot disagree.
Measured on a live capture of a large marketing site:
default budget 232 kept, 91 dropped (39 size-floor, 20 cap-reached,
32 unavailable)
15s budget 30 kept, 299 dropped (279 budget-exhausted, 20 cap-reached)
Same page, same command, and until now both runs described themselves the same
way.
…e declares none `capture` built its favicon candidate list only from `link[rel*="icon"]` and `link[rel="apple-touch-icon"]` in the document, so a page that declares neither produced no favicon AND no drop reason: the download loop had an empty list, so it attempted nothing and therefore counted nothing. A folder with no favicon because the site has none looked exactly like a folder with no favicon because nobody asked. Plenty of large sites are in the second group. apple.com declares none of the ~150 `<link>` tags in its head as an icon and still serves `/favicon.ico` (200, image/x-icon) and `/apple-touch-icon.png` (200). Every browser gets an icon there; capture did not. With no declared icon link, the candidates are now the site root's well-known paths in the browser's own preference order, resolved against the page's FINAL url so a redirect lands on the right origin. The first one that answers wins, and `fetchBuffer`'s existing refusal of an HTML or XML body under a 200 is what keeps a no-icon site's app shell from being written to `assets/favicon.ico`. A guess that answers with nothing usable is counted under `unavailable`, so the miss is on the record rather than invisible. Declared links keep priority and their path is untouched: when the page vouches for an icon, the well-known paths are never requested.
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
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.
Stacked on #3598 (open) — that PR adds the per-reason drop counts this one reuses. Review the top commit only.
The gap
capturebuilt its favicon candidate list from the document's ownlink[rel*="icon"]/link[rel="apple-touch-icon"]tags and nothing else(
packages/cli/src/capture/index.ts). A page that declares neither handed thedownloader an empty list, so the favicon loop in
assetDownloader.tsran zeroiterations: no fetch was attempted, and — even after #3598 — no drop reason was
emitted. A folder with no favicon because the site has none was byte-identical
to a folder with no favicon because nobody asked.
https://www.apple.comis in the second group: it declares none of the ~150<link>tags in its head as an icon, and serves/favicon.ico(200,image/x-icon, 22 KB) and/apple-touch-icon.png(200) anyway. Every browsergets an icon there. Capture did not.
The change
When the page declares no icon link, the candidates become the site root's
well-known paths in the browser's own preference order —
/favicon.ico, then/apple-touch-icon.png— resolved against the page's final URL so aredirect lands on the right origin. The first that answers wins.
well-known paths are never requested. The control test asserts the fetch call
list, not just the result.
unavailabledrop reason, so thedroppedtally from feat(cli): capture reports why a referenced asset is not in the folder #3598 shows it.fetchBufferalready refuses an HTML or XMLbody served under a 200, which is exactly how a site with no icon answers
/favicon.ico. That is what keeps an app shell from being written toassets/favicon.ico.What I measured
Same command, same page,
main-side build vs this branch:One more asset, and it is the right one.
unavailablestays at 0 because thefirst well-known path answered, so the second was never requested.
Unit run:
vitest run src/capture/assetDownloader.test.ts→ 27 passed.The three new tests were proved non-vacuous by reverting the one-line candidate
change: 2 of 3 fail (the declared-link control passes either way, which is the
point of a control).
What I did NOT exercise
That path (both guesses miss,
unavailablereaches 2) is covered only by theunit test with a stubbed 200-HTML response, not by a live capture.
isPrivateUrlblocks loopback bydesign (SSRF denylist, F-003), so a fixture server on
127.0.0.1can never befetched by
downloadAssets. Every existing drop-count test in this file stubsfetchfor the same reason, and these follow that convention.page.url()returns thepost-redirect URL and that is what the paths resolve against, but I did not
capture a site that redirects across origins to confirm it end to end.
--skip-visionon both runs. The vision pass does not touch favicons, butthe before/after numbers are from that configuration, not the default one.
vitest run src/capturegives 153 passed with3 collection failures from an unbuilt workspace package (
@hyperframes/parsershas no dist in this worktree) — pre-existing, unrelated to this diff, and
present on the base commit too.