feat(cli): capture reports why a referenced asset is not in the folder - #3598
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.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
| } else if (buffer.length <= 5000) { | ||
| drops["size-floor"]++; | ||
| } else { | ||
| writeFileSync(join(outputDir, localPath), buffer); |
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Approved at 460dba3117e. Verified first-hand against the head:
- Every drop site in
downloadAssetsanddownloadAndRewriteFontsincrements exactly one reason at the line that performs the drop: SVG cap (pre-loop arithmetic) and size floor; favicon budget (remaining count), fetch failure and write failure; image batch budget (remaining count), null fetch, size floor, rejected promise, write failure; OG image budget / null / size; font budget and total cap (remaining counts), per-family cap, fetch and write failure. The fulfilled-nullpath is counted inside the mapper and deliberately not again in the results loop, so no double count. - Exits that are not counted are not drops of a distinct referenced asset: URL dedup (
downloadedUrls), a favicon link with nohref, the first-favicon-winsbreak, and the catalog's junk filter (pixel/beacon/analytics//favicon). Nit, non-blocking: a tracking pixel caught by that URL heuristic is uncounted while the same pixel caught by byte size issize-floor; if the heuristic ever needs to be visible,size-flooris the honest bucket. - Both passes now run with a spent budget so their loops can count
budget-exhaustedper item; the inline-SVG pass does no network, so a zero-budget run still writes up to 30 inline SVGs where before it wrote none. Intended per the comment, and cheap. --jsonis additive:droppedalways present with all four keys (zeros when clean), text line printed only when non-zero, reason strings and shape matchdocs/packages/cli.mdxexactly.CaptureResult.droppedis required, and Typecheck/Build are green so every constructor supplies it.- Tests reference
noDrops/drop reasons that do not exist onmain, so each fails there. - CodeQL
js/http-to-file-accessatassetDownloader.ts:291is the OG-image write that existed onmain(the block was restructured, not introduced); same class as the open alerts 806/599 on this file. Pre-existing by design for a downloader.
Stamp only; merge is yours (repo needs last-push approval).
jrusso1020
left a comment
There was a problem hiding this comment.
Read assetDownloader.ts in full at 460dba3117e1c83079bf5c7662d3f38de97e5afa. The premise is right and the live numbers make it - default budget 232 kept / 91 dropped against 15s budget 30 kept / 299 dropped, two captures that described themselves identically until now, is exactly the case a bare continue cannot answer.
The taxonomy is the good part: three decisions and one failure, which is the split a reader actually needs. Deleting the two hand-written budget warnings and deriving the remaining one from the tally is the right follow-through - a warning computed separately from the thing it warns about is the next disagreement.
The counting is careful where it is easy to get wrong. Splitting rejected from a fulfilled null in the batch loop, with the comment saying why counting both would double it, is the specific bug this shape invites and it is handled. drops["budget-exhausted"] += toDownload.length - i on the batch break is right, and so is the ogImage rewrite from a collapsed boolean expression into three named outcomes.
The one count that is wrong: the favicon block is a search, not a download list
for (const [index, icon] of icons.entries()) {
const remainingMs = options.remainingMs?.() ?? 10_000;
if (remainingMs <= 0) { drops["budget-exhausted"] += icons.length - index; break; }
if (!icon.href) continue;
try {
const buffer = await fetchBuffer(icon.href, Math.min(10_000, remainingMs));
if (buffer) { writeFileSync(...); assets.push(...); break; }
drops.unavailable++;
} catch { drops.unavailable++; }
}This loop breaks on the FIRST success, so the entries are alternate candidates for one favicon, not a list of assets to fetch. Both counters read them as the latter:
- A page declaring
favicon.ico,apple-touch-icon.pngandicon-192.pngwhere the first two 404 and the third works reportsunavailable: 2with the favicon successfully captured. Nothing was dropped; two candidates were tried. - The budget break counts every remaining candidate as a separate loss, when at most one favicon was ever going to land.
The docblock's rule - "every member is counted at the single line that performs the drop, so a count can never disagree with the branch it describes" - is what makes this stand out: that line is not performing a drop, it is advancing a search, and it is the one place in the file where the rule does not hold. On a header that exists to say how much is missing, an over-count is the same class of wrong as the silence it replaces, just quieter.
The shape that matches the rest of the file is to decide the favicon's outcome once after the loop: if one landed, no drop; if none did, exactly one drop, attributed to why the last candidate failed (or budget-exhausted if the clock is what ended it).
Smaller, same block: if (!icon.href) continue; is the one drop site in the function that counts nothing at all. Harmless under the fix above, since a hrefless candidate stops being an asset the moment the favicon is one outcome rather than N.
One judgement call worth naming rather than changing
drops["cap-reached"] += Math.max(0, tokens.svgs.length - MAX_INLINE_SVGS) attributes the whole overflow to the cap, including svgs that would have failed the 50-character floor had they been examined. That is the right attribution - the cap is genuinely why they were never looked at - and the alternative would mean examining what the cap exists to avoid examining. Worth a half-line in the comment so a later reader does not "fix" it into a floor count.
Approving. The mechanism is sound everywhere else, the tally is a real improvement over four kinds of silent continue, and the favicon count is a contained fix rather than a rethink - but it is in the feature's own subject matter, so I would take it before merging rather than after.
Review by Rames
|
Non-blocking, after the stamp: the docs define — Jerrai |
|
Concur with Rames, and this supersedes my note above: the favicon loop counts each failed candidate as — Jerrai |
The problem
capturedrops assets for four reasons and reports none of them.Every drop site is a bare
continue,break,return null, or an emptycatch. The result isthat a capture of a spare page and a capture that a limit truncated produce the same summary:
There is no way, from the output, to tell whether the page had thirty images or three hundred. The
only existing signals were two hand-written warning strings that fire when the budget is already
gone before a download pass is called — which is the one case where the pass itself could not
have said how much it lost.
What changed
downloadAssetsanddownloadAndRewriteFontsreturn anAssetDropCountstally beside theirresult, incremented at the single line that performs each drop. Nothing re-derives a count from
anything else.
capture --jsoncarries it asdropped. The human summary prints aDropped:line when it isnon-zero:
Four reasons — three of them decisions this code made, one a failure it hit, which is the split a
reader actually needs:
size-floorbudget-exhaustedcap-reachedunavailablefetchBuffermiss and everycatcharound a writeA break counts everything it did not reach, not the one it stopped on. "How many did we lose"
is the question, and one is never the answer.
The two warnings are gone, not duplicated
Both existed only to cover the case where the budget ran out before a pass was called. So both
passes are now called unconditionally: with a zero budget each loop breaks on its first item and
records
budget-exhaustedfor the rest. That costs no network — the break happens before anyfetch — and produces a real number where the string could only say "some".
One warning remains, and it is derived from the tally rather than written alongside it, so the
prose and the count cannot drift apart the way two separately-authored strings could.
What I measured
A live capture of a large marketing site, same URL, same command, two budgets:
size-floor, 20cap-reached, 32unavailable--capture-budget 15000budget-exhausted, 20cap-reachedUntil now both runs described themselves identically. The 20
cap-reachedin both is the samefact seen twice: that page carries 50 inline SVGs and the per-run cap is 30.
Tests: 7 new cases through the public API of the two exported functions, expected values read off
the fixture (35 declared with a cap of 30 gives 5; 10 rules in one family with a per-family cap of
6 gives 4), never recomputed the way the code computes them. One of them is a control that asserts
an all-zero tally on a page whose fonts all downloaded, because that is the case the whole
field exists to make readable.
Each counter was proven non-vacuous by mutation: neutering the increments fails 5 of the 7, and
neutering the per-family increment alone fails exactly the per-family test.
bun run lint,oxfmt --check, and the pre-commit typecheck are all green.What I did NOT exercise
downloadAssets(type !== Image, thepixel/beacon/analyticssubstring skip,!hasGoodContext). Those reject things that are not assets, so counting them would inflate thenumber the field exists to make trustworthy. Say the word if you want them under a fifth reason.
src/capture/fail to collect in a fresh worktree until@hyperframes/parsersis built. That is unrelated to this change and reproduces onorigin/main(143 passing there, 150 here, same three files failing to collect both times).