fix: address code review findings from PR #5 - #6
Merged
Merged
Conversation
An --incremental run writes only the URLs it re-fetched, so its date folder is a delta rather than a picture of the site. Scoping a report to "the newest date folder" therefore pushed every page that happened not to change into `stale` and collapsed a 300-page report to the handful of URLs the delta touched — without tripping the `pages.length === 0` guard, so the near-empty report looked valid. Each crawl now records a _crawl-meta.json manifest in its date folder, the merge step stamps `_metadata.crawlMode` onto every record from it, and the snapshot is anchored on the newest *full* crawl with every incremental crawl since layered on top. Date folders with no manifest read as `full`, so existing datasets behave exactly as they did. seo-audit gets the same rule separately because it loads date folders directly rather than the merged JSONL. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Three defects in the load-shaping flags added alongside them: --delay-min=0 disabled pacing entirely. The request-handler guard tested requestDelayMin for truthiness, so a legitimate 0ms lower bound (--delay-min=0 --delay-max=3000) read as "no delay configured" and produced a faster, heavier crawl than the crawler.yml default — the opposite of the flag's intent. The two bounds were never validated. An inverted pair made getRandomDelay() multiply by a negative span and return values below the requested minimum, and a delay longer than requestHandlerTimeoutSecs aborted the handler as a timeout and retried it, adding load rather than shedding it. normalizeDelayRange() now swaps an inverted pair and clamps to half the handler timeout, warning on both. It runs on the effective pair, since overriding only one side inverts the range against the crawler.yml value for the other just as easily as passing both. --block-assets blocked more than assets. crawlee's blockRequests() forwards its patterns to CDP Network.setBlockedURLs, which matches substrings, so the '.js' pattern also swallowed '.json' endpoints and any URL merely containing '.js'. A page fetching its content over JSON was then recorded as an empty 200 and flagged as broken by every downstream SEO check. Replaced with a Playwright route that filters on the browser's own resourceType, so scripts are blocked and XHR/fetch responses are not. Verified against Chromium: .js/.css/.png blocked, .json served. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The snapshot scoping made reports state their crawl date on stdout, but only the 404 report persisted it. report-seo-issues and report-link-graph-issues still stamped their filenames with new Date() — today, not the crawl — and wrote no scope metadata at all, so a saved report built from a months-old crawl was indistinguishable from one describing the site as it is now. Both now embed a crawl_scope block (per domain: crawl date, baseline full crawl, incremental crawls layered on it, mode, pages analysed) and stamp filenames with the crawl date. Aggregate runs span several crawls and have no single date, so those still fall back to today. The 404 report's JSON also changed shape in the previous release without a version marker: it was a bare ReportEntry[] and became an object, silently breaking any consumer reading it as an array. All persisted reports now carry schema_version (2), and the README documents the break from version 1. Wires up metaGeneratedVia, added to both locales but never referenced: the audit header now states the crawl scope it was generated from. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
Follow-up to #5, fixing seven findings from a code review of its two commits (
ca106a7load-shaping CLI flags,c1f1581report snapshot scoping). Three thematic commits, each independently green.1. Incremental crawls collapsed every report —
283f5f8The severe one.
--incrementalwrites only the URLs it re-fetched, so its date folder is a delta, not a picture of the site. Scoping a report to "the newest date folder" pushed every unchanged page intostaleand shrank a 300-page report to the handful of URLs the delta touched — andpages.length === 0never tripped, so the near-empty report looked valid.--all-crawlswas the only escape hatch, undocumented for this case.Each crawl now writes
_crawl-meta.json(mode: full | incremental) into its date folder; the merge step stamps_metadata.crawlModeonto every record; the snapshot anchors on the newest full crawl with later incrementals layered on top:Folders with no manifest read as
full, so existing datasets are unaffected — verified on a real 9-crawl dataset, byte-identical output before and after.2. Delay-range and asset-blocking guards —
0c2c584--delay-min=0disabled all pacing. The guard testedrequestDelayMinfor truthiness, so a legitimate0lower bound read as "unset" and produced a heavier crawl than thecrawler.ymldefault.--delay-min=5000 --delay-max=1000madegetRandomDelay()multiply by a negative span; a delay aboverequestHandlerTimeoutSecsaborted the handler as a timeout and retried — the "more load, not less" failure this PR's own docs warn about.normalizeDelayRange()swaps and clamps, warning on both, and runs on the effective pair (overriding one side inverts the range against the YAML value for the other).--block-assetsblocked.json. crawlee'sblockRequests()forwards patterns to CDPNetwork.setBlockedURLs, which matches substrings, so.jsalso swallowed.jsonendpoints. A page fetching content over JSON was recorded as an empty 200 and flagged broken by every SEO check. Replaced with a Playwright route filtering onresourceType. Verified against Chromium:.js/.css/.pngblocked,.jsonserved, JSON-driven content intact.3. Reports now carry their crawl —
43c8a46report-seo-issuesandreport-link-graph-issuesstamped filenames with today's date and wrote no scope metadata, so a saved report from a months-old crawl looked current. Both now embed acrawl_scopeblock and stamp filenames with the crawl date.ReportEntry[]to an object in fix(reports): default report scripts to the latest crawl, not the historical union #5 with no version marker, silently breaking array consumers. All reports now carryschema_version: 2; README documents the break.metaGeneratedViawas added to both locales but never referenced — now wired into the audit header.Verification
npx tsc --noEmitclean;npm test275 passed (was 258)crawlManifest.test.ts,delayRange.test.ts, +5 inpageRecords.test.tstsc+ targeted tests green at each commit, so history is bisectableKnown limitation, not addressed here
The manifest is written only on successful completion. A crawl killed mid-run, or cut short by
--max-requests, still leaves a partial folder that reads asfull. Same symptom, different cause — happy to take it in a follow-up.🤖 Generated with Claude Code