feat(events-crawler): North Vancouver City Library adapter, staged disabled - #92
Conversation
…sabled First of the orgs deferred from the 2026-07-29 round. The deferral assumed the listing carried no dates and would need a fetch per event; re-checking the live feed showed otherwise — date and time are both in the listing, so there is no N+1. NVCL is the second library whose BiblioCommons tenant is a dead end. Unlike the bpl/Boston trap, `nvcl` really is North Vancouver City Library — but the gateway answers 403 "The Events feature is not available", exactly as Burnaby does. Verified before writing a line, and recorded in the adapter header so nobody re-tests it. The Drupal listing is the only readable surface. The listing carries no year. Every row reads "Tuesday, August 4, 10:30 am to 11:00 am" — 0 of 45 sampled rows across three pages carried one. That matters because the paginated list runs chronologically past the year boundary: page 37 of 38 lists April-June, i.e. the following year, which "assume current year" would file eight months in the past. So the year is derived and then *checked*: each candidate year's weekday is compared against the weekday the page printed, and a row matching neither is skipped rather than guessed at. Two more things the feed does not have. There is no location — the venue slot exists in the markup but is empty on every row sampled — so the source supplies defaultLocation, and the adapter refuses to run without one rather than inserting a wrong place into a NOT NULL column. And there are no images, only an icon font, so covers always fall through to the Pexels/Unsplash tiers. Dedupe-by-link is load-bearing here rather than defensive: every page repeats the same five featured rows above its paginated section, so without it a 24-page walk returns those five 24 times. Safe as an identity — across 45 sampled rows no slug appeared with two different dates, so the slug identifies the occurrence and matches events_external_link_key. Carries forward both parse-failure guards from surrey.ts (page 0 with no blocks, page 0 where every title misses), since this is the same class of scraper. KNOWN: with relevanceFilter on, this source currently yields ZERO rows — see the PR for the evidence and the decision it needs. Landing disabled either way. Verified with the real harness: deno check clean; dryrun --source nvcl parses 25 well-formed rows with the filter off (correct dates, end times, dedupe, covers) and 0 with it on. tsc and lint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughAdds a staged North Vancouver City Library Drupal source. The adapter fetches and parses paginated event pages, resolves yearless dates, filters and deduplicates events, resolves covers, and returns event rows with per-row failure isolation. ChangesNVCL event ingestion
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@supabase/functions/events-crawler/adapters/nvcl.ts`:
- Around line 264-271: Update parsePage so maxStartMs is calculated only from
the paginated chronological listings, excluding the five leading featured rows
repeated on every page. Preserve the existing pastWindow stop logic in the
crawler loop, and verify the featured-row dates remain within ctx.windowEndMs
before relying on this source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0543e4b8-7be2-4082-9c02-41732caac88f
📒 Files selected for processing (3)
supabase/functions/events-crawler/adapters/nvcl.tssupabase/functions/events-crawler/lib/sources.tssupabase/functions/events-crawler/lib/types.ts
…aximum Addresses the CodeRabbit finding, which is a real latent truncation bug. parsePage took the maximum start across every block on the page, but each page is "five featured rows, then the chronological section". The featured rows are curated, so one can sit arbitrarily far out — a save-the-date. That maximum would then exceed the window on page 0, set pastWindow on the very first batch, and stop the walk after 4 pages while in-window listings remained. Not reachable today (the featured rows are currently Aug 4-5, well inside the window) but entirely reachable by an editorial choice on the library's side, with no signal that it had happened. Took a different fix from the one sketched. Requiring a whole batch to agree before stopping does not work here: the featured block repeats on EVERY page, so a distant featured row makes every page in the batch report the same past-the-window maximum and the walk still truncates. Excluding "the leading five" would work but hardcodes a count the markup never promises. Using the LAST block in document order sidesteps both. The chronological section always trails the featured block, so the final block is the furthest-out entry the paginated list has actually reached — which is the thing the walk wants to compare against the window — and it needs no guess about how many featured rows exist. Verified against the live feed: with the relevance filter off the walk still ends early and correctly, taking 4 pages, 58 candidates, 38 unique, soonest 25. The 20 dropped duplicates are exactly the 5 featured rows repeated across those 4 pages, which is the repetition this finding is about. deno check clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Applied in Why the batch-level condition doesn't work here. The sketch stops only when a whole batch is past the window. But the featured block repeats on every page — that's the premise of the finding — so a distant featured row makes every page in the batch report the same past-the-window maximum. The batch condition is then satisfied just as readily as the per-page one, and the walk truncates anyway. It changes how many pages it takes to go wrong, not whether it does. Why not exclude the leading five. That works, but hardcodes a count the markup never promises. If the library features four one week and six the next, the guard silently starts trimming real rows off the chronological section. What I did instead: use the LAST block in document order. The chronological section always trails the featured block, so the final block is the furthest-out entry the paginated list has actually reached — precisely the value the walk wants to compare against the window — and it needs no assumption about how many featured rows there are. Verified against the live feed. With the relevance filter off (so the walk actually produces rows), it still ends early and correctly: That run also happens to confirm the finding's premise: 58 candidates → 38 unique is exactly 20 duplicates, i.e. the 5 featured rows repeated across 4 pages. One thing worth confirming explicitly, since you asked: the featured-row dates are currently well inside the window — Aug 4–5 against a Dec 2 edge — so this was latent, not active. It would have become reachable the first time NVCL featured a save-the-date.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 52 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@supabase/functions/events-crawler/adapters/nvcl.ts`:
- Around line 229-286: Update fetchEvents to track candidate uniqueness by link
while pages are processed, using an incremental set or equivalent keyed by each
candidate’s link. Add candidates only when their links have not been seen, and
use the unique count for the MAX_PER_ORG early-stop check instead of
candidates.length; preserve the existing later processing and other stop
conditions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c96db07b-c7fb-4d0e-9408-4471a50985e5
📒 Files selected for processing (3)
supabase/functions/events-crawler/adapters/nvcl.tssupabase/functions/events-crawler/lib/sources.tssupabase/functions/events-crawler/lib/types.ts
…t raw hits Second CodeRabbit finding on this adapter, and valid for the same underlying reason as the first: the featured block repeats on every page, so any running total that ignores it means something other than what it says. The MAX_PER_ORG early-stop compared candidates.length, which counted the featured rows once per page. With the relevance filter on — the shipped configuration — a source whose relevant rows are mostly featured ones would reach 25 in raw count while holding a handful of distinct events, stop there, and return that handful with in-window listings still unread. The observed run hid this (58 raw / 38 unique, both above the cap) but the shape of the bug does not depend on that mix. Deduping during the walk rather than after it fixes the count at its source and removes the separate post-loop pass: byLink.size is now what the stop condition tests, what the page-cap warning reports, and what gets sorted and sliced. A raw counter is kept purely for the log line, where "58 in-window + relevant (38 unique)" is genuinely more informative than either number alone. Verified against the live feed. Filter off: 4 pages, 58 raw, 38 unique, soonest 25 — identical to before, so the happy path is unchanged. Filter on: still 0, now reported as "0 unique candidate(s)". deno check clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Applied in
Worth being precise that the observed run masks this rather than demonstrating it: 58 raw / 38 unique are both above the cap, so the correct 25 came out either way. The bug doesn't depend on that mix being favourable, which is why it's worth fixing rather than filing. Deduped during the walk instead of after it, which fixes the count at its source rather than patching the comparison. Verified against the live feed: Filter-off output is byte-identical to before the change, so the happy path is untouched. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@supabase/functions/events-crawler/adapters/nvcl.ts`:
- Around line 157-227: Update parsePage to track when-line and date-parsing
failures alongside title-link failures, using a dedicated counter or equivalent
aggregate that identifies blocks rejected before candidate creation. Update the
page-0 diagnostic in fetchEvents to report this failure state when all parsed
blocks fail title or date extraction, while preserving the existing zero-block
and relevance-filter behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3cde6e77-a61a-498a-9f15-77970b2cad0d
📒 Files selected for processing (3)
supabase/functions/events-crawler/adapters/nvcl.tssupabase/functions/events-crawler/lib/sources.tssupabase/functions/events-crawler/lib/types.ts
Third CodeRabbit finding, and valid: the page-0 diagnostic only counted title misses, so a block whose anchor still matched but whose when-line or date format had moved was rejected silently. The source would return zero rows and look exactly like a library with nothing on. Closed the whole family rather than the flagged layer, because these fail independently and a per-layer fix invites the same finding again next round. The three structural rejections — no anchor or empty title, no bold when-line paragraph, when-line present but unparseable — are now counted separately, and page 0 reports the breakdown when none of its blocks survive extraction. The per-layer split is not decoration: it says which of the three moved, and the fix differs for each. The relevance and window filters are deliberately excluded from that count. They reject on merit, and rejecting most of a library calendar is precisely their job, so folding them in would make the diagnostic fire on every healthy run — this source legitimately returns zero under the filter today. Verified by breaking each layer in turn against the live feed: when-line regex broken → "title 0, when-line 15, date 0" date parse broken → "title 0, when-line 0, date 15" restored → silent, and still 0 rows from the relevance filter That last line is the one that matters: a legitimate zero stays quiet while a structural zero is loud. Happy path unchanged — filter off still gives 4 pages, 58 raw, 38 unique, soonest 25. deno check clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Applied in Closed the whole family rather than the flagged layer. The three structural rejections fail independently — the anchor can move, the bold when-line paragraph can be renamed, or the date string can change shape (dropping the weekday would defeat the parse on its own) — so fixing only the when-line invites this finding again next round. All three are now counted separately, and page 0 reports the breakdown when nothing survives extraction. The split isn't decoration: it names which layer moved, and the fix differs for each. The relevance and window filters are deliberately excluded from that count. They reject on merit, and rejecting most of a library calendar is their job — this source legitimately returns zero under the filter today. Folding them in would make the diagnostic fire on every healthy run, which is how a warning becomes noise and then gets ignored. Verified by breaking each layer in turn against the live feed: That last line is the one that matters — a legitimate zero stays quiet while a structural zero is loud. Under the previous code the first two cases printed nothing at all, because Happy path unchanged: filter off still gives Worth noting across this review round, since all three findings share a root: NVCL pages are featured block + chronological section, and every bug found here came from logic that treated a page as homogeneous — the page maximum (#1), the raw candidate count (#2), and now single-layer failure detection (#3). I've swept the adapter for the remaining instances of that assumption and don't see a fourth. Carrying the same lens into the Capilano adapter rather than rediscovering it in review. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
1 participant
What & why
First of the three orgs deferred from the 2026-07-29 round. The deferral rationale did not survive re-checking. It assumed the listing carried no dates and would need a fetch per event; the live feed puts date and time in the listing, so there is no N+1.
Ships
enabled: false, same as every Phase-2 source did. No deploy, no enable.🛑 Known: this source currently yields ZERO rows
Not a bug — the adapter is correct, and the same dry-run returns 25 well-formed rows with the relevance filter off. It's the filter, and it needs a decision that doesn't belong in this PR.
NVCL's entire 4-month window is 27 distinct titles, and
lib/relevance.tsmatches none of them:Most of that is genuinely off-mission and should be filtered. But two look like real misses: "MS Office learn and practice: Intro to Excel / Word" is digital-skills-for-employment content, and the digital-literacy group only covers
digital literacy,computer (basics|skills|help),tech (cafe|help|support),device clinic,online safety,internet basics— none of which match.relevance.tsalso already names this exact shape as a known cost: "an opaquely-named but genuinely relevant event (NVDPL's 'Open Door Community Hub Drop-In') is dropped." NVCL runs its own "Open door community hub".Three ways forward, none taken here:
relevance.ts— the path its own header sanctions ("Revisit by adding terms"). But it's shared, so it changes behaviour for all 10 now-live sources, and a naive\bword\bwould catch "Crossword"/"Wordplay". Deserves its own PR and its own dry-run regression.Flagging rather than deciding, since it touches live shared behaviour.
Decisions baked in
The year is derived, then checked. The listing carries none — 0 of 45 sampled rows across three pages. That matters because the paginated list runs chronologically past the year boundary: page 37 of 38 lists April–June, i.e. the following year. "Assume current year" would file those eight months in the past. So each candidate year's weekday is compared against the weekday the page printed, and a row matching neither candidate is skipped rather than guessed at — a wrong date is worse than a missing event.
Second BiblioCommons dead end. Unlike the
bpl/Boston trap,nvclreally is North Vancouver City Library — but the gateway answers 403 "The Events feature is not available", exactly as Burnaby's does. Verified before writing any code and recorded in the adapter header so nobody re-tests it.defaultLocation, and a hard refusal without one. The venue slot exists in the markup but is empty on every row sampled. Sinceevents.locationis NOT NULL, the adapter returns[]with an error rather than run — a wrong place is worse than a dropped source.Dedupe-by-link is load-bearing, not defensive. Every page repeats the same five featured rows above its paginated section, so without it a 24-page walk returns those five 24 times. Verified safe as an identity: across 45 sampled rows no slug appeared with two different dates, so the slug identifies the occurrence and matches
events_external_link_key.No images — the listing renders an icon font, not photos, so covers always fall through to Pexels/Unsplash. No
next.config.tsallowlist needed.Both
surrey.tsparse-failure guards are carried forward (page 0 with no blocks; page 0 where every title misses), since this is the same class of scraper.Changes
adapters/nvcl.ts(new) — the adapter.lib/types.ts—nvcl-drupaladded toSourceKind(theADAPTERSrecord is exhaustive, so a missing implementation is a compile error).lib/sources.ts— import,ADAPTERSentry, and a Phase-3 registry block. Existing ten entries untouched.enabled: false; no migration, cron, Vault secret or DB object touched; not deployed; no writes to shared prod. Enabling needs Savar's sign-off, exactly as the Phase-2 flip did.Verification
Run with the real harness (
npx deno@2, matching CI's--node-modules-dir=auto).deno check(CI's exact invocation)npx tsc --noEmitnpm run lintopengraph-image.tsx)dryrun --source nvcl, filter offdryrun --source nvcl, filter on (shipped)dryrun --listnvcl nvcl-drupal DISABLED, relevance-filtered; other 10 unchangedSample of the 25 rows produced with the filter off:
Reviewer notes
MAX_PAGES = 24(vs Surrey's 20): the pager advertises 38 pages of 15, and the 4-month window is reached well before that, so the cap is a backstop for a listing that grows or stops being chronological — not the expected exit.zonedWallClockToUtcalready returns null for dates that don't exist (Feb 30), and a wrong-year candidate fails the weekday comparison.🤖 Generated with Claude Code
Summary by CodeRabbit