Repository navigation
Count events_on_page_1 from page 1, not from the merged rows - #19
Merged
Merged
Conversation
The audit's last recommendation asks for a freshness and completeness report so a consumer sees the date, the walk type, the number of events and the gaps. The sidecar already carries all four — finished_at, mode and pagination, total_events and events_on_page_1, and pages_failed beside pages_without_rows — so checking that claim meant reading a real sidecar rather than building anything. Which is how this turned up. events_on_page_1 was counted over the MERGED rows with `r.page == 1`, and that asks a different question once the walk has merged: a listing row is replaced in place by the deeper read from its event page, and the replacement carries that event page's number. So the field counted the events whose rows happened NOT to be upgraded. Measured on a real ten-page walk: 3 reported where page 1 had named 11, printed beside total_events: 23475 — the comparison a reader makes to judge how much of the catalogue a run saw. Right in --mode markets, where nothing is upgraded, which is why it looked fine. Counted from page 1's own rows now, in all three engines. Verified live after the change: events_on_page_1 = 12, matching both the log's "12 event(s) the page renders" and the 12 distinct event slugs in the file, where the old expression would have said 7. The check is controlled against each engine separately, because a fix landing in one of three is the failure this repo has already met. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Merged
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.
The audit's last recommendation asks for a freshness and completeness report — so a consumer can see the date, the walk type, the number of events and the gaps.
The sidecar already carries all four, and checking that claim meant reading a real one rather than building anything:
finished_atmode,paginationtotal_events,events_on_page_1pages_failed,pages_without_rows,pages_completed/pages_requestedReading one is how this turned up.
events_on_page_1was counting the wrong thingon a ten-page walk where page 1 had named 11 events.
It was counted over the merged rows with
r.page == 1, and that asks a different question once the walk has merged:--mode eventsreplaces a listing row in place with the deeper read from its event page, and the replacement carries that event page's number. So the field counted the events whose rows happened not to be upgraded — which is an artefact of how far the walk got, not a fact about the listing.It matters because of what sits beside it.
total_eventsagainstevents_on_page_1is exactly the comparison a reader makes to judge how much of the catalogue a run saw, and 3-against-23,475 tells them something very wrong about a run that read 11 events in full.It is correct in
--mode markets, where nothing is upgraded, which is why it looked fine.Verified live after the change
matching the engine's own log line for that run: "this run took 160 market(s) from the 12 event(s) the page renders".
Verification
Fixed in all three engines and controlled against each one separately, because a fix landing in one of three is a failure this repo has already met:
playwright_scrapercounts over the merged rows again...from page 1's own outcome, not the merged rowsselenium_scraperlikewisepuppeteer_scraperlikewiseThe check reads the source rather than calling a function, because the sidecar is assembled inline in each engine; that is also why it is worth asserting in all three rather than once.
No new sink, queue or delivery layer
The audit's final item suggests a service layer — queue, schedules, storage, API, webhooks — if the goal is to compete with managed platforms. That is a product decision rather than a defect, and it is not taken here: this repo is a scraper with a file output contract, and the README already says so. Anything of that shape belongs behind one small sink interface in the shared core rather than grown into
product_parser.pyor the row model, and it would have to preserve two things this contract already guarantees — a failed run writes nothing, and the sidecar reaches the same place as its rows or not at all.🤖 Generated with Claude Code