From fb0a1ef481bcbee555515b818c8cf3c0e2ad2357 Mon Sep 17 00:00:00 2001 From: BrandonML Date: Tue, 29 Sep 2026 12:40:44 -0400 Subject: [PATCH 1/3] fix(newtab): skip to another cat when a photo fails to load A card whose photo 404s (a listing removed since it was cached, issue #79) used to stay on screen with a broken-image icon and a notice. It now moves on to another unseen card from the pool -- normal mode from feedCache, explore mode from the in-memory batch -- up to three times in a row before falling back to the existing "no longer available" notice. - The failed card is already marked seen when picked, so the only storage write is marking the replacement seen; cards are never evicted, so a transient CDN error can't permanently shrink the pool. - Never recycles seen cards (nextUnseenCard returns null instead), never runs while offline (every photo would fail; issue #69's notice stays), waits for an in-flight refresh so it can't clobber feedCache, and stands down if the failing photo's card has already been replaced. - nextCard() now delegates to nextUnseenCard(); behavior is unchanged. Co-Authored-By: Claude Sonnet 5.5 --- extension/newtab.js | 72 ++++++++++++++++++--- test/newtab.test.js | 153 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 208 insertions(+), 17 deletions(-) diff --git a/extension/newtab.js b/extension/newtab.js index 1fa4692..74ebbdc 100644 --- a/extension/newtab.js +++ b/extension/newtab.js @@ -50,6 +50,9 @@ const PHOTO_SHARE_TIMEOUT_MS = 6000; // gets its own message; a false "online" reading just falls through to the // existing per-context error handling unchanged. const OFFLINE_MESSAGE = "You're offline. Reconnect and refresh to keep browsing cats."; +// Issue #79: how many replacement cards a run of broken photos may skip +// through before giving up and showing the "no longer available" notice. +const MAX_PHOTO_FALLBACKS = 3; const TABBY_CWS_URL = "https://chromewebstore.google.com/detail/tabby-new-tab-for-adoptab/elfpnkoboidkgahmoggodpnmekfodcig"; const TABBY_EDGE_URL = "https://microsoftedge.microsoft.com/addons/detail/fieeoalehgckgnkohkdblljmgaemaiho"; const TABBY_TAGLINE = "Meet an adoptable cat every time you open a new tab."; @@ -397,19 +400,56 @@ function buildSaveButton(card) { return button; } -function nextCard(cards, seenIds = []) { +// Returns null (rather than recycling a seen card) when nothing unseen is left. +function nextUnseenCard(cards, seenIds = []) { const seenSet = new Set(seenIds); const unseenCards = cards.filter((card) => !seenSet.has(card.id)); - if (unseenCards.length) { - const selected = randomCard(unseenCards); - seenSet.add(selected.id); - return { selected, nextSeenIds: [...seenSet] }; - } + if (!unseenCards.length) return null; + const selected = randomCard(unseenCards); + seenSet.add(selected.id); + return { selected, nextSeenIds: [...seenSet] }; +} + +function nextCard(cards, seenIds = []) { + const unseenPick = nextUnseenCard(cards, seenIds); + if (unseenPick) return unseenPick; const selected = randomCard(cards); return { selected, nextSeenIds: [selected.id] }; } -function renderCard(card, { stale = false, exploreLabel = null, locationLabel = null } = {}) { +// Issue #79: a broken photo used to leave a card on screen with a broken-image +// icon and a notice. The card was already marked seen when it was picked, so +// moving on needs no storage change beyond marking the replacement seen too. +// Returns true when there is nothing more for the caller to do (a replacement +// was rendered, or this photo's card is no longer the one on screen), false +// when no unseen replacement exists and the notice should show instead. +async function showNextCardAfterPhotoError(img, { exploreLabel, locationLabel, fallbacksLeft }) { + // A refresh may be rewriting feedCache right now; let it finish so this + // read-modify-write can't clobber its result. (If it replaces the card on + // screen in the meantime, the check below notices and stands down.) + if (inFlight) await inFlight.catch(() => {}); + if (!img.isConnected) return true; + + if (exploreLabel) { + if (!exploreBatch || exploreBatch.label !== exploreLabel) return false; + const pick = nextUnseenCard(exploreBatch.cards, exploreBatch.seenIds); + if (!pick) return false; + exploreBatch = { ...exploreBatch, seenIds: pick.nextSeenIds }; + renderCard(pick.selected, { exploreLabel, locationLabel, fallbacksLeft }); + return true; + } + + const { feedCache } = await storageGet(["feedCache"]); + if (!feedCache?.cards?.length) return false; + const pick = nextUnseenCard(feedCache.cards, getSeenIds(feedCache)); + if (!pick) return false; + await storageSet({ feedCache: { ...feedCache, seenIds: pick.nextSeenIds } }); + if (!img.isConnected) return true; + renderCard(pick.selected, { locationLabel, fallbacksLeft }); + return true; +} + +function renderCard(card, { stale = false, exploreLabel = null, locationLabel = null, fallbacksLeft = MAX_PHOTO_FALLBACKS } = {}) { closeShareMenu(); // a card rebuild (e.g. "Show another cat") orphans any open menu -- close it first const meta = [card.breed, card.age, card.sex].filter(Boolean).join(" · "); // While exploring, distanceMiles is measured from the explored city, not @@ -447,9 +487,21 @@ function renderCard(card, { stale = false, exploreLabel = null, locationLabel = img.src = card.imageUrl; img.alt = card.name; img.referrerPolicy = "no-referrer"; - img.addEventListener("error", () => { - const message = navigator.onLine === false ? OFFLINE_MESSAGE : "That photo is no longer available. Refresh to try another cat."; - showNotice(message, { type: "error" }); + img.addEventListener("error", async () => { + // Offline means every photo would fail -- skipping ahead would just burn + // through the pool, so it keeps its own notice (issue #69). + if (navigator.onLine === false) { + showNotice(OFFLINE_MESSAGE, { type: "error" }); + return; + } + if (fallbacksLeft > 0) { + try { + if (await showNextCardAfterPhotoError(img, { exploreLabel, locationLabel, fallbacksLeft: fallbacksLeft - 1 })) return; + } catch (error) { + console.error("[tabby]", error); + } + } + showNotice("That photo is no longer available. Refresh to try another cat.", { type: "error" }); }); img.addEventListener("load", () => { // A portrait-oriented photo (taller than wide) can't fill the card's diff --git a/test/newtab.test.js b/test/newtab.test.js index 62f9be4..699a14b 100644 --- a/test/newtab.test.js +++ b/test/newtab.test.js @@ -232,26 +232,165 @@ describe('newtab.js DOM manipulation', () => { }); }); - describe('photo load failure (issue #69)', () => { - it('shows the generic "photo unavailable" notice when the image fails to load while online', () => { - window.renderCard({ name: "Milo", imageUrl: "https://image.org/cat.jpg" }); + describe('photo load failure (issues #69, #79)', () => { + const tick = () => new Promise((resolve) => setTimeout(resolve, 10)); + const failPhoto = async () => { + document.querySelector('.photo').dispatchEvent(new window.Event('error')); + await tick(); + }; + const shownName = () => document.querySelector('#card h1').textContent; + const catPool = (n) => Array.from({ length: n }, (_, i) => ({ id: `c${i}`, name: `Cat${i}`, imageUrl: `https://image.org/${i}.jpg` })); + + beforeEach(() => { Object.defineProperty(window.navigator, 'onLine', { value: true, configurable: true }); + }); - document.querySelector('.photo').dispatchEvent(new window.Event('error')); + it('shows the generic "photo unavailable" notice when the image fails and there is no other card to show', async () => { + window.renderCard({ name: "Milo", imageUrl: "https://image.org/cat.jpg" }); + + await failPhoto(); assert.ok(document.getElementById('notice').textContent.includes('no longer available')); }); - it('shows an offline-specific notice when the image fails to load while offline', () => { - window.renderCard({ name: "Milo", imageUrl: "https://image.org/cat.jpg" }); + it('shows an offline-specific notice when the image fails to load while offline', async () => { Object.defineProperty(window.navigator, 'onLine', { value: false, configurable: true }); + window.renderCard({ name: "Milo", imageUrl: "https://image.org/cat.jpg" }); - document.querySelector('.photo').dispatchEvent(new window.Event('error')); + await failPhoto(); const noticeText = document.getElementById('notice').textContent; assert.ok(noticeText.includes("offline"), `expected an offline-specific notice, got: ${noticeText}`); assert.ok(!noticeText.includes('no longer available'), 'should not show the generic missing-photo message while offline'); }); + + it('does not skip ahead through the pool while offline', async () => { + const cards = catPool(3); + let poolRead = false; + window.chrome.storage.local.get = async (keys) => { + if (keys.includes('feedCache')) poolRead = true; + return { feedCache: { cards, seenIds: ['c0'], location: { postalcode: '12345' } } }; + }; + Object.defineProperty(window.navigator, 'onLine', { value: false, configurable: true }); + window.renderCard(cards[0]); + + await failPhoto(); + + assert.equal(poolRead, false, 'offline failures should not touch the pool'); + assert.equal(shownName(), 'Cat0'); + }); + + it('moves on to another unseen card, and marks it seen, when a photo fails', async () => { + const cards = catPool(3); + let savedCache; + window.chrome.storage.local.get = async () => ({ feedCache: { cards, seenIds: ['c0'], location: { postalcode: '12345' }, page: 2, fetchedAt: 123 } }); + window.chrome.storage.local.set = async (val) => { if (val.feedCache) savedCache = val.feedCache; }; + window.renderCard(cards[0], { locationLabel: 'from 12345' }); + + await failPhoto(); + + assert.ok(['Cat1', 'Cat2'].includes(shownName()), `expected an unseen replacement, got ${shownName()}`); + assert.equal(document.getElementById('notice').textContent, '', 'a successful skip should not show the notice'); + assert.equal(savedCache.seenIds.length, 2); + assert.ok(savedCache.seenIds.includes('c0')); + assert.equal(savedCache.page, 2, 'only seenIds should change'); + assert.equal(savedCache.fetchedAt, 123); + assert.equal(savedCache.cards.length, 3, 'the failed card is not evicted, just already-seen'); + }); + + it('keeps the distance basis label on the replacement card', async () => { + const cards = [{ id: 'a', name: 'A', imageUrl: 'https://image.org/a.jpg', distanceMiles: 1 }, { id: 'b', name: 'B', imageUrl: 'https://image.org/b.jpg', distanceMiles: 2 }]; + window.chrome.storage.local.get = async () => ({ feedCache: { cards, seenIds: ['a'] } }); + window.renderCard(cards[0], { locationLabel: 'from 97703' }); + + await failPhoto(); + + assert.equal(shownName(), 'B'); + assert.equal(document.querySelector('.distance').textContent, '2.0 mi away from 97703'); + }); + + it('shows the notice instead of recycling a seen card when nothing unseen is left', async () => { + const cards = catPool(2); + window.chrome.storage.local.get = async () => ({ feedCache: { cards, seenIds: ['c0', 'c1'] } }); + window.renderCard(cards[0]); + + await failPhoto(); + + assert.equal(shownName(), 'Cat0'); + assert.ok(document.getElementById('notice').textContent.includes('no longer available')); + }); + + it('gives up with the notice after three consecutive broken photos', async () => { + const cards = catPool(8); + let seenIds = ['c0']; + window.chrome.storage.local.get = async () => ({ feedCache: { cards, seenIds } }); + window.chrome.storage.local.set = async (val) => { if (val.feedCache) seenIds = val.feedCache.seenIds; }; + window.renderCard(cards[0]); + + for (let i = 0; i < 3; i++) { + await failPhoto(); + assert.equal(document.getElementById('notice').textContent, '', `skip ${i + 1} should be silent`); + } + assert.equal(seenIds.length, 4, 'original + three replacements shown'); + + await failPhoto(); // the fourth broken photo in a row + + assert.ok(document.getElementById('notice').textContent.includes('no longer available')); + assert.equal(seenIds.length, 4, 'no fifth card should have been tried'); + }); + + it('does not act on a stale photo whose card has already been replaced', async () => { + const cards = catPool(3); + let setCalls = 0; + window.chrome.storage.local.get = async () => ({ feedCache: { cards, seenIds: ['c0'] } }); + window.chrome.storage.local.set = async () => { setCalls++; }; + window.renderCard(cards[0]); + const staleImg = document.querySelector('.photo'); + window.renderCard(cards[1]); // e.g. a refresh landed first + + staleImg.dispatchEvent(new window.Event('error')); + await tick(); + + assert.equal(shownName(), 'Cat1'); + assert.equal(setCalls, 0); + assert.equal(document.getElementById('notice').textContent, ''); + }); + + it('falls back to the notice if reading the pool throws', async () => { + const originalError = console.error; + console.error = () => {}; + try { + window.chrome.storage.local.get = async (keys) => { + if (keys.includes('feedCache')) throw new Error('storage unavailable'); + return {}; + }; + window.renderCard({ id: 'c0', name: 'Cat0', imageUrl: 'https://image.org/0.jpg' }); + + await failPhoto(); + } finally { + console.error = originalError; + } + + assert.ok(document.getElementById('notice').textContent.includes('no longer available')); + }); + + it('cycles within the explore batch, without a new fetch or touching feedCache', async () => { + const cards = catPool(3); + let fetchCount = 0; + let feedCacheWrites = 0; + window.fetch = async () => { fetchCount++; return { ok: true, json: async () => ({ cards, radiusMiles: 25 }) }; }; + window.chrome.storage.local.set = async (val) => { if (val.feedCache) feedCacheWrites++; }; + await window.exploreArea(); + assert.equal(fetchCount, 1); + const firstShown = shownName(); + + await failPhoto(); + + assert.notEqual(shownName(), firstShown, 'moved on to another explore card'); + assert.equal(fetchCount, 1); + assert.equal(feedCacheWrites, 0); + assert.equal(document.getElementById('explore-banner').hidden, false); + }); }); describe('Save cats (issue #43)', () => { From 88a6601891b7c756db55a51855c32efb261746b7 Mon Sep 17 00:00:00 2001 From: BrandonML Date: Tue, 29 Sep 2026 12:52:06 -0400 Subject: [PATCH 2/3] fix(cache): revalidate a week-old cache against RescueGroups instead of serving dead listings Refs #79. A light user can take weeks to hit the 85% seen ratio, so a cached pool could sit long enough for some listings to be adopted or removed -- the card still rendered, with a broken photo. There was no age ceiling because the old time-based cutoff (STALE_MS) was removed in b829eed: it advanced `page` and walked users outward through the radius ladder. This adds an age check that does neither. Server: new POST /api/validate-cats { ids } -> { availableIds }. One RescueGroups request (animals.id `equal` + array criteria on the existing available/cats/haspic endpoint, confirmed live to act as an "in" filter that silently omits unknown ids). Ids are validated (1-100 numeric), the route is uncached, and it shares /api/nearby-cats's per-IP rate limit, error mapping and upstream-failure alerting. Purely additive. searchRadius and the new findAvailableIds now share one request/error helper. Extension: once feedCache.validatedAt (falling back to fetchedAt) is 7+ days old, _start validates the pool's ids before rendering (2s timeout, chunks of 100). Dead cards and their seenIds are dropped; every still-available unseen card is kept in order; page/fetchedAt/location are untouched; validatedAt is stamped. refresh() now carries validatedAt forward when it keeps unseen cards (fetchedAt resets on refresh, which would otherwise hide their age). If the whole pool is dead it refreshes from page 1 with nothing kept, writing only if that fetch succeeds. Any failure fails open (cache served as before) and sets a 1h cooldown so a down server can't delay every tab; offline skips validation entirely. A cache with no usable timestamp is left alone. Docs: README behavior/rate-limit notes; PRIVACY.md now discloses that cached public listing ids are sent to the backend (and on to RescueGroups) at most about weekly, with no location attached. Tests (Tier 3, full suite: 325 pass, lint clean): server route + request builder + findAvailableIds; 31 client tests covering when it runs, pruning, seenIds, page/fetchedAt preservation, chunking, cross-tab safety, refresh interplay, all-dead pool, and every failure mode; a live by-id contract test. Mutation-checked seven ways. Also verified end-to-end (real newtab.js against the real local server and RescueGroups API). Co-Authored-By: Claude Sonnet 5.5 --- PRIVACY.md | 8 +- README.md | 4 +- extension/newtab.js | 120 ++++++++- server/index.js | 21 +- server/rescuegroups.js | 42 +++- test-live/rescuegroups.live.js | 29 ++- test/newtab.test.js | 433 ++++++++++++++++++++++++++++++++- test/rescuegroups.test.js | 76 +++++- test/server.test.js | 119 +++++++++ 9 files changed, 825 insertions(+), 27 deletions(-) diff --git a/PRIVACY.md b/PRIVACY.md index 2b31807..d9771d4 100644 --- a/PRIVACY.md +++ b/PRIVACY.md @@ -1,6 +1,6 @@ # Privacy Policy for Tabby -_Last updated: 2026-09-22_ +_Last updated: 2026-09-29_ Tabby ("the extension") replaces your new tab page with one real, adoptable cat sourced from [RescueGroups.org](https://rescuegroups.org). It's the same extension package on both the Chrome Web Store and Edge Add-ons, and this policy applies to both. This policy explains what data Tabby collects, how it's used, and how it's stored. @@ -14,17 +14,17 @@ Tabby does not collect your name, email address, browsing history, or any other ## How Data Is Used -Your location or ZIP code is sent to Tabby's own backend server, which uses it to search RescueGroups.org for adoptable cats near that location. Nothing else is sent — no browsing history, no device identifiers, no data from other tabs or sites. +Your location or ZIP code is sent to Tabby's own backend server, which uses it to search RescueGroups.org for adoptable cats near that location. Separately, at most about once a week, Tabby sends the public listing IDs of the cats it has cached on your device (not your location, and nothing that identifies you) to the same backend, which passes them to RescueGroups.org to check whether those listings are still available so removed ones can be dropped. The backend does not store them. Nothing else is sent — no browsing history, no device identifiers, no data from other tabs or sites. ## How Data Is Stored -Your location/ZIP code and the most recently fetched batch of cat listings are stored locally on your device, using the browser's local extension storage (`chrome.storage.local` — the same API on both Chrome and Edge, since Edge is Chromium-based). This data is **not** synced to Google's, Microsoft's, or any other cloud service, and never leaves your device except for the single search request described above. +Your location/ZIP code and the most recently fetched batch of cat listings are stored locally on your device, using the browser's local extension storage (`chrome.storage.local` — the same API on both Chrome and Edge, since Edge is Chromium-based). This data is **not** synced to Google's, Microsoft's, or any other cloud service, and never leaves your device except for the search request and the periodic availability check described above. On the server side, Tabby's backend keeps a short-lived (a few minutes) cache of search results, keyed only by a rounded location and page number — never by anything that identifies you personally, such as an IP address, account, or device ID. This cache exists purely to avoid making duplicate requests to RescueGroups.org and is not linked to you as an individual. ## Third-Party Services -Tabby's backend queries the [RescueGroups.org](https://rescuegroups.org) public API to find adoptable cats. Your search location (ZIP code or coordinates) is sent to RescueGroups.org as part of that search — this is the only third party that ever receives your location, and only for the purpose of returning matching adoptable-cat listings. See [RescueGroups.org's own privacy policy](https://rescuegroups.org/privacy-policy/) for how they handle that request. +Tabby's backend queries the [RescueGroups.org](https://rescuegroups.org) public API to find adoptable cats. Your search location (ZIP code or coordinates) is sent to RescueGroups.org as part of that search — this is the only third party that ever receives your location, and only for the purpose of returning matching adoptable-cat listings. It also receives the public listing IDs described above, with no location attached, to confirm those listings are still available. See [RescueGroups.org's own privacy policy](https://rescuegroups.org/privacy-policy/) for how they handle that request. Tabby also uses Chrome Web Store's built-in GA4 analytics, as described above — this collects only basic, aggregate usage metrics, not your location or any other data described in this policy. Tabby does not use any advertising or crash-reporting service. No data is sold, rented, or shared with any party other than RescueGroups.org and Google/Chrome Web Store as described in this policy. diff --git a/README.md b/README.md index 0e66e1e..e8ab40c 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ Tabby is a Manifest V3 extension (Chrome and Edge) that replaces the new tab pag ## How Tabby works -- New-tab UI with instant cached-card rendering and stale-while-revalidate refresh: the last-fetched batch renders immediately from `chrome.storage.local`, and a background refresh only fires once the cache is at least 5 minutes old **and** the user has seen at least 85% of the cached cards — so a batch the user hasn't finished browsing isn't discarded early. +- New-tab UI with instant cached-card rendering and stale-while-revalidate refresh: the last-fetched batch renders immediately from `chrome.storage.local`, and a background refresh only fires once the cache is at least 5 minutes old **and** the user has seen at least 85% of the cached cards — so a batch the user hasn't finished browsing isn't discarded early. A batch that's sat unfinished can still go stale, though (listings get adopted or removed), so once its cards haven't been checked in 7 days, the next new tab first asks the backend (`POST /api/validate-cats`, one RescueGroups request per up-to-100 ids) which of them are still available and drops the rest: every still-available unseen card is kept, `page` never advances, and any failure just serves the cache as before (issue #79). If a photo fails to load anyway, Tabby skips ahead to another unseen cat instead of leaving a broken image on screen. - Browser-coordinate lookup with native postal-code fallback. - Server-side 25 -> 75 -> 150 -> 250 mile radius ladder: widens the radius, deduplicating by cat ID across steps, until at least 40 unique cats are accumulated or the 250-mile step is reached, whichever comes first — 40 is a floor, not a target. Whatever's accumulated is then capped at 100 cats (closest-first) before being sent to the extension. - RescueGroups `available/cats/haspic` query — only cats in "available" status with at least one photo are ever shown — plus nearest-first sorting, picture validation, organization join, and safe profile-url fallback. @@ -47,7 +47,7 @@ Deploying the server is a separate step from packaging the extension; whatever h The in-memory cache is correct as-is for the intended deployment target: a single persistent Node process (for example Render, Railway, Fly.io, or Northflank). It would need to be replaced with a shared cache (for example KV/Redis) only if the server is ever scaled to multiple concurrent instances, or moved to a serverless/edge platform (Vercel functions, Cloudflare Workers) where in-process state isn't reliably shared or persistent between requests — those platforms would also require restructuring `server/index.js` away from its current `node:http` `createServer` model. -`/api/nearby-cats` is also rate-limited per client IP (30 requests / 5 minutes, in-memory, same deployment assumption as the cache above) — a cache miss costs a real RescueGroups API call, so this bounds how much a script varying postal codes/coordinates can cost regardless of the response cache. The client IP is taken from `X-Forwarded-For` when present (Northflank and similar platforms terminate the real connection and forward, so `request.socket.remoteAddress` alone would otherwise be the platform's internal proxy address for every request), falling back to the raw socket address only when that header is absent, as in local dev. If a future host doesn't set `X-Forwarded-For` in front of this server, every request would be seen as one shared IP. +`/api/nearby-cats` and `/api/validate-cats` (never cached: each call is one real RescueGroups request) are also rate-limited per client IP, sharing one budget (30 requests / 5 minutes, in-memory, same deployment assumption as the cache above) — a cache miss costs a real RescueGroups API call, so this bounds how much a script varying postal codes/coordinates can cost regardless of the response cache. The client IP is taken from `X-Forwarded-For` when present (Northflank and similar platforms terminate the real connection and forward, so `request.socket.remoteAddress` alone would otherwise be the platform's internal proxy address for every request), falling back to the raw socket address only when that header is absent, as in local dev. If a future host doesn't set `X-Forwarded-For` in front of this server, every request would be seen as one shared IP. - `ALERT_WEBHOOK_URL` — optional. When set, the server posts a Discord-compatible webhook message (a JSON body with a `content` field) whenever upstream RescueGroups failures spike: 5+ failures within a rolling 10-minute window, with a 30-minute cooldown between alerts so a sustained outage doesn't spam the channel. Left unset, alerting is a no-op — this exists because a real RescueGroups connectivity incident once went undetected for hours with errors only reaching server logs. diff --git a/extension/newtab.js b/extension/newtab.js index 74ebbdc..4c0e5e0 100644 --- a/extension/newtab.js +++ b/extension/newtab.js @@ -5,9 +5,24 @@ import { locationFromBrowser } from "./location.js"; const FRESH_MS = 5 * 60 * 1000; // A refresh replaces only the *seen* cards in the pool (kept unseen ones // survive), so waiting until most of the pool has been shown just means -// fewer, chunkier fetches — there's no accuracy cost to waiting, unlike the -// old time-based cutoff this replaced (see git history on STALE_MS). +// fewer, chunkier fetches — and, unlike the old time-based cutoff this +// replaced (see git history on STALE_MS), a refresh never fires off elapsed +// time alone: that cutoff advanced `page` and walked users outward through +// the server's radius ladder regardless of how much they'd actually browsed. const SEEN_REFRESH_RATIO = 0.85; +// Waiting has one cost, though (issue #79): a light user can take weeks to +// reach the seen ratio, and listings get adopted or removed in the meantime. +// So a pool that hasn't been checked for MAX_POOL_AGE_MS gets its ids +// revalidated against RescueGroups in a cheap request per 100 ids (see revalidatePool) +// -- dead cards are dropped, every still-available unseen card is kept, and +// `page` never advances. It fails open: any error just serves the cache as +// before, and VALIDATE_RETRY_COOLDOWN_MS keeps a down server from delaying +// every new tab (each attempt can block the first render for up to +// VALIDATE_TIMEOUT_MS). +const MAX_POOL_AGE_MS = 7 * 24 * 60 * 60 * 1000; +const VALIDATE_TIMEOUT_MS = 2000; +const VALIDATE_RETRY_COOLDOWN_MS = 60 * 60 * 1000; +const VALIDATE_BATCH_SIZE = 100; // the server's per-request id cap // RescueGroups' photo height/width ratio is a continuous spread, not two // clusters (sampled live across 5 metros: ~52% land in 0.9-1.1, but the // portrait side alone stretches from 1.1 to 2.3+ with no natural gap) — a @@ -897,9 +912,84 @@ function mergeCards(keptCards, incomingCards, excludeIds = []) { return [...keptCards, ...freshCards]; } -async function refresh(location, locationLabel) { +// When a pool's cards were last known to be available. Falls back to +// fetchedAt for caches written before validatedAt existed. Null if neither is +// a usable timestamp (nothing sensible to compare against). +function poolValidatedAt(feedCache) { + const stamp = feedCache?.validatedAt ?? feedCache?.fetchedAt; + return Number.isFinite(stamp) ? stamp : null; +} + +function poolNeedsRevalidation(feedCache) { + if (!feedCache?.cards?.length) return false; + const validatedAt = poolValidatedAt(feedCache); + if (validatedAt === null) return false; + const now = Date.now(); + return now - validatedAt >= MAX_POOL_AGE_MS && now >= (Number(feedCache.validationRetryAfter) || 0); +} + +async function fetchAvailableIds(ids, signal) { + const backendUrl = BACKEND_URL.replace(/\/$/, ""); + const response = await fetch(`${backendUrl}/api/validate-cats`, { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ ids }), signal }); + if (!response.ok) throw new Error((await response.json().catch(() => ({}))).error || "Could not validate cats."); + const { availableIds } = await response.json(); + if (!Array.isArray(availableIds)) throw new Error("Malformed validation response."); + return availableIds.map(String); +} + +// The cache as it is in storage right now, falling back to the copy this +// tab read earlier -- another tab may have refreshed it while a validation +// request was in flight, and writing back a stale copy would undo that. +async function latestPool(fallback) { const { feedCache } = await storageGet(["feedCache"]); - const isRepeatLocation = sameLocation(feedCache?.location, location) && Boolean(feedCache?.cards?.length); + return feedCache?.cards?.length ? feedCache : fallback; +} + +// Drops the cached cards RescueGroups no longer lists as available. Returns +// `{ feedCache, allGone }` and never throws. `allGone` means every card in the +// pool is dead: nothing is written (the caller should refresh from scratch +// instead, which replaces the pool only if that fetch succeeds). Zero +// survivors is trusted rather than treated as a glitch, because the +// alternative -- serving a pool known to be dead -- is the bug being fixed. +async function revalidatePool(feedCache) { + if (navigator.onLine === false) return { feedCache, allGone: false }; + try { + const checkedIds = [...new Set(feedCache.cards.map((card) => card.id))]; + const signal = AbortSignal.timeout(VALIDATE_TIMEOUT_MS); + const availableIds = new Set(); + for (let i = 0; i < checkedIds.length; i += VALIDATE_BATCH_SIZE) { + (await fetchAvailableIds(checkedIds.slice(i, i + VALIDATE_BATCH_SIZE), signal)).forEach((id) => availableIds.add(id)); + } + const goneIds = new Set(checkedIds.filter((id) => !availableIds.has(id))); + + // Only ids that were actually checked can be dropped, so a card another + // tab added in the meantime is left alone. + const base = await latestPool(feedCache); + const survivors = base.cards.filter((card) => !goneIds.has(card.id)); + if (!survivors.length) return { feedCache: base, allGone: true }; + + const pruned = { ...base, cards: survivors, seenIds: getSeenIds(base).filter((id) => !goneIds.has(id)), validatedAt: Date.now() }; + delete pruned.validationRetryAfter; + await storageSet({ feedCache: pruned }); + return { feedCache: pruned, allGone: false }; + } catch (error) { + console.error("[tabby]", error); + try { + const failed = { ...(await latestPool(feedCache)), validationRetryAfter: Date.now() + VALIDATE_RETRY_COOLDOWN_MS }; + await storageSet({ feedCache: failed }); + return { feedCache: failed, allGone: false }; + } catch { + return { feedCache, allGone: false }; + } + } +} + +// `discardPool` is for a pool revalidation found entirely dead: it refreshes +// as if there were no prior cache (page 1, nothing kept), and since nothing is +// written until the fetch succeeds, a failed attempt leaves the old cache as-is. +async function refresh(location, locationLabel, { discardPool = false } = {}) { + const { feedCache } = await storageGet(["feedCache"]); + const isRepeatLocation = !discardPool && sameLocation(feedCache?.location, location) && Boolean(feedCache?.cards?.length); const priorSeenIds = isRepeatLocation ? getSeenIds(feedCache) : []; // Only the *seen* cards are dropped on a refresh — whatever the user // hasn't looked at yet survives and is topped up with new cards below, @@ -925,7 +1015,12 @@ async function refresh(location, locationLabel) { mergedCards = mergeCards(keptUnseenCards, feed.cards || []); } - const nextCache = { cards: mergedCards, fetchedAt: Date.now(), radiusMiles: feed.radiusMiles || feedCache?.radiusMiles || 0, location, page, seenIds: [] }; + // Kept unseen cards are older than this fetch, so the pool as a whole is + // only as recently validated as they were -- resetting validatedAt to now + // here (as fetchedAt is) would hide their age from poolNeedsRevalidation(). + const fetchedAt = Date.now(); + const validatedAt = keptUnseenCards.length ? (poolValidatedAt(feedCache) ?? fetchedAt) : fetchedAt; + const nextCache = { cards: mergedCards, fetchedAt, validatedAt, radiusMiles: feed.radiusMiles || feedCache?.radiusMiles || 0, location, page, seenIds: [] }; await storageSet({ feedCache: nextCache }); if (!mergedCards.length) { setCardVisible(false); @@ -946,7 +1041,14 @@ async function refresh(location, locationLabel) { async function _start({ requestLocation = false } = {}) { setCardVisible(false); - const { settings = { postalcode: "", location: null }, feedCache } = await storageGet(["settings", "feedCache"]); + const { settings = { postalcode: "", location: null }, feedCache: storedCache } = await storageGet(["settings", "feedCache"]); + let feedCache = storedCache; + let poolIsDead = false; + if (poolNeedsRevalidation(feedCache)) { + const revalidated = await revalidatePool(feedCache); + feedCache = revalidated.feedCache; + poolIsDead = revalidated.allGone; + } const resolvedSettings = { postalcode: settings.postalcode || "", location: settings.location || null @@ -954,8 +1056,8 @@ async function _start({ requestLocation = false } = {}) { const locationLabel = resolvedSettings.postalcode ? `from ${resolvedSettings.postalcode}` : "from you"; const age = feedCache ? Date.now() - feedCache.fetchedAt : Infinity; const seenRatio = feedCache?.cards?.length ? getSeenIds(feedCache).length / feedCache.cards.length : 1; - const shouldRefresh = !feedCache?.cards?.length || (age >= FRESH_MS && seenRatio >= SEEN_REFRESH_RATIO); - if (feedCache?.cards?.length) { + const shouldRefresh = poolIsDead || !feedCache?.cards?.length || (age >= FRESH_MS && seenRatio >= SEEN_REFRESH_RATIO); + if (feedCache?.cards?.length && !poolIsDead) { const { selected, nextSeenIds } = nextCard(feedCache.cards, getSeenIds(feedCache)); await storageSet({ feedCache: { ...feedCache, seenIds: nextSeenIds } }); renderCard(selected, { stale: shouldRefresh, locationLabel }); @@ -972,7 +1074,7 @@ async function _start({ requestLocation = false } = {}) { } $("location-panel").hidden = true; if (shouldRefresh) { - try { await refresh(location, locationLabel); } catch (error) { + try { await refresh(location, locationLabel, { discardPool: poolIsDead }); } catch (error) { console.error("[tabby]", error); if (navigator.onLine === false) { showNotice(OFFLINE_MESSAGE, { type: "error" }); diff --git a/server/index.js b/server/index.js index 723f111..257c235 100644 --- a/server/index.js +++ b/server/index.js @@ -1,6 +1,6 @@ import { createServer } from "node:http"; import { fileURLToPath } from "node:url"; -import { findNearbyCats, validateLocation } from "./rescuegroups.js"; +import { findAvailableIds, findNearbyCats, validateLocation } from "./rescuegroups.js"; const port = Number(process.env.PORT || 8787); // Comma-separated list — one origin per store build (Chrome, Edge, ...), @@ -303,7 +303,8 @@ export const server = createServer(async (request, response) => { const requestUrl = new URL(request.url, "http://internal"); return sendPhotoProxy(response, origin, requestUrl.searchParams.get("url") || "", buildPhotoShareUrl, PHOTO_SHARE_MAX_BYTES); } - if (request.url !== "/api/nearby-cats") return send(response, 404, { error: "Not found" }, origin); + const isValidateRoute = request.url === "/api/validate-cats"; + if (request.url !== "/api/nearby-cats" && !isValidateRoute) return send(response, 404, { error: "Not found" }, origin); if (request.method !== "POST") return send(response, 405, { error: "Method Not Allowed" }, origin, { "Allow": "POST" }); const retryAfterSeconds = checkRateLimit(clientIp(request)); @@ -314,7 +315,19 @@ export const server = createServer(async (request, response) => { } try { - const { location, page } = await bodyOf(request); + const body = await bodyOf(request); + + // Issue #79: tells the extension which of its cached listings are still + // available, so a long-idle cache can drop the ones that aren't. One + // RescueGroups request per call (see findAvailableIds), deliberately + // uncached -- the answer is only useful fresh -- and behind the same + // per-IP limit as /api/nearby-cats. + if (isValidateRoute) { + const availableIds = await findAvailableIds(body?.ids, { apiKey: process.env.RG_API_KEY }); + return send(response, 200, { availableIds }, origin); + } + + const { location, page } = body; const safeLocation = validateLocation(location); const requestedPage = safePage(page); const key = cacheKey(safeLocation, requestedPage); @@ -338,7 +351,7 @@ export const server = createServer(async (request, response) => { let status = 502; if (error.message === "Payload too large") status = 413; else if (error.message === "Request timeout") status = 408; - else if (error instanceof SyntaxError || /Provide a five-digit|location is required/.test(error.message)) status = 400; + else if (error instanceof SyntaxError || /Provide a five-digit|location is required|Provide 1 to \d+ numeric animal ids/.test(error.message)) status = 400; else if (/not a recognized postalcode/i.test(error.message)) status = 400; console.error("[tabby-server]", { diff --git a/server/rescuegroups.js b/server/rescuegroups.js index 4c0a7f5..511db09 100644 --- a/server/rescuegroups.js +++ b/server/rescuegroups.js @@ -97,6 +97,29 @@ export function buildSearchRequest(location, miles, page = 1) { }; } +// Issue #79: revalidates specific cached listings without re-paging the +// search. `animals.id` + `equal` with an array criteria acts as an "in" +// filter (confirmed live: 99 of 100 real ids returned in one request, an +// unknown id silently dropped), and the request hits the same +// .../search/available/... endpoint as every other query here -- so an id +// that doesn't come back is no longer an available listing. +export function validateAnimalIds(input) { + const message = `Provide 1 to ${MAX_LIMIT} numeric animal ids.`; + if (!Array.isArray(input) || input.length === 0 || input.length > MAX_LIMIT) throw new Error(message); + const ids = input.map((id) => (typeof id === "number" ? String(id) : id)); + if (!ids.every((id) => typeof id === "string" && /^\d{1,12}$/.test(id))) throw new Error(message); + return [...new Set(ids)]; +} + +export function buildAvailabilityRequest(ids) { + const safeIds = validateAnimalIds(ids); + const query = new URLSearchParams({ limit: String(MAX_LIMIT), page: "1", "fields[animals]": "name" }); + return { + url: `${BASE_URL}/public/animals/search/available/cats/haspic/?${query}`, + body: { data: { filters: [{ fieldName: "animals.id", operation: "equal", criteria: safeIds }] } } + }; +} + function includedIndex(included = []) { return new Map(included.map((resource) => [`${resource.type}:${resource.id}`, resource])); } @@ -201,9 +224,7 @@ export function normalizeCards(payload) { }).filter(Boolean); } -export async function searchRadius(location, miles, { apiKey, fetchImpl = fetch, page = 1 } = {}) { - if (!apiKey) throw new Error("RG_API_KEY is not configured."); - const { url, body } = buildSearchRequest(location, miles, page); +async function requestPayload({ url, body }, { apiKey, fetchImpl }) { const response = await fetchImpl(url, { method: "POST", headers: { Authorization: apiKey, "Content-Type": CONTENT_TYPE, Accept: CONTENT_TYPE }, @@ -222,9 +243,24 @@ export async function searchRadius(location, miles, { apiKey, fetchImpl = fetch, if (!payload || typeof payload !== "object") { throw new Error(`RescueGroups HTTP ${response.status}: empty or malformed response body`); } + return payload; +} + +export async function searchRadius(location, miles, { apiKey, fetchImpl = fetch, page = 1 } = {}) { + if (!apiKey) throw new Error("RG_API_KEY is not configured."); + const payload = await requestPayload(buildSearchRequest(location, miles, page), { apiKey, fetchImpl }); return normalizeCards(payload); } +// Returns the subset of `ids` that are still available listings. +export async function findAvailableIds(ids, { apiKey, fetchImpl = fetch } = {}) { + const request = buildAvailabilityRequest(ids); // bad input is a 400 regardless of server config + if (!apiKey) throw new Error("RG_API_KEY is not configured."); + const requested = new Set(request.body.data.filters[0].criteria); + const payload = await requestPayload(request, { apiKey, fetchImpl }); + return (payload.data || []).map((animal) => String(animal.id)).filter((id) => requested.has(id)); +} + export async function findNearbyCats(location, { apiKey, target = 40, fetchImpl = fetch, page = 1 } = {}) { // Every step re-requests the same `page`, and RescueGroups sorts // nearest-first, so a wider radius's results substantially overlap the diff --git a/test-live/rescuegroups.live.js b/test-live/rescuegroups.live.js index 28c6f63..7a9a922 100644 --- a/test-live/rescuegroups.live.js +++ b/test-live/rescuegroups.live.js @@ -6,7 +6,7 @@ // logs a message and exits 0 rather than failing. import { describe, it } from "node:test"; import assert from "node:assert"; -import { buildSearchRequest } from "../server/rescuegroups.js"; +import { buildSearchRequest, findAvailableIds } from "../server/rescuegroups.js"; if (!process.env.RG_API_KEY) { console.log("Skipping live RescueGroups test — set RG_API_KEY to run"); @@ -117,3 +117,30 @@ describe("RescueGroups live pagination contract", () => { ); }); }); + +// Issue #79: the extension revalidates a long-idle cache by asking which of +// its cached ids are still available. This pins the two behaviors that +// approach depends on, against the real API: an `animals.id` `equal` filter +// with an array criteria works as an "in" filter on the available-cats +// endpoint, and an id that isn't a currently-available listing is silently +// left out (not an error). If either regresses, revalidation would either +// fail open forever (harmless but useless) or -- worse -- drop live cats. +describe("RescueGroups live by-id availability contract", () => { + it("returns exactly the requested ids that are available, silently omitting unknown ones", async () => { + const { url, body } = buildSearchRequest(LOCATION, 25); + const seed = await fetch(url, { + method: "POST", + headers: { Authorization: API_KEY, "Content-Type": CONTENT_TYPE, Accept: CONTENT_TYPE }, + body: JSON.stringify(body), + signal: AbortSignal.timeout(8_000) + }).then((response) => response.json()); + const realIds = idsOf(seed).slice(0, 20); + assert.ok(realIds.length >= 5, "need a handful of real available ids to test with"); + const fakeIds = ["99999991", "99999992"]; + + const availableIds = await findAvailableIds([...realIds, ...fakeIds], { apiKey: API_KEY }); + + console.log("[live] asked about", realIds.length + fakeIds.length, "ids, got back", availableIds.length); + assert.deepEqual([...availableIds].sort(), [...realIds].sort(), "every real id should come back, and neither fake one"); + }); +}); diff --git a/test/newtab.test.js b/test/newtab.test.js index 699a14b..9baee9c 100644 --- a/test/newtab.test.js +++ b/test/newtab.test.js @@ -1665,13 +1665,16 @@ describe('newtab.js DOM manipulation', () => { assert.equal(card.querySelector('h1').textContent, 'CatFresh'); }); - it('does not refresh when under the seen-ratio threshold, no matter how old the cache is', async () => { + it('does not refresh when under the seen-ratio threshold, however old the cache is', async () => { window.chrome.storage.local.get = async () => ({ settings: { postalcode: '12345', location: { lat: 1, lon: 2 } }, feedCache: { cards: [{ id: '1', name: 'CatA' }, { id: '2', name: 'CatB' }], - // Very old — there's no hard time-based cutoff any more (removed - // along with STALE_MS; a refresh now only fires off the seen ratio). + // Old, but inside the 7-day revalidation window. A refresh (a new + // page of cats) never fires off elapsed time alone -- STALE_MS was + // removed for that reason; only the seen ratio triggers one. Age past + // 7 days triggers a separate, page-free revalidation instead (see + // 'stale pool revalidation' below). fetchedAt: Date.now() - (1000 * 60 * 60 * 24), seenIds: [] // 0 of 2 seen — well under the 85% refresh threshold } @@ -1912,6 +1915,430 @@ describe('newtab.js DOM manipulation', () => { }); }); + describe('stale pool revalidation (issue #79)', () => { + const DAY = 24 * 60 * 60 * 1000; + const LOCATION = { lat: 1, lon: 2 }; + const settings = { postalcode: '12345', location: LOCATION }; + const cardsFor = (ids) => ids.map((id) => ({ id, name: `Cat${id}`, imageUrl: `https://image.org/${id}.jpg` })); + const poolCache = (overrides = {}) => ({ + cards: cardsFor(['1', '2', '3']), + fetchedAt: Date.now() - 8 * DAY, + location: LOCATION, + page: 3, + radiusMiles: 25, + seenIds: [], + ...overrides + }); + + let store; + let calls; // { validate: [ids...], nearby: [body...] } + let availability; // ids the mocked server reports as still available, or a function/throwing behavior + let nearbyResponse; + + beforeEach(() => { + Object.defineProperty(window.navigator, 'onLine', { value: true, configurable: true }); + store = { settings }; + calls = { validate: [], nearby: [] }; + availability = () => []; // set per test + nearbyResponse = { cards: cardsFor(['fresh1', 'fresh2']), radiusMiles: 25 }; + window.chrome.storage.local.get = async (keys) => Object.fromEntries((Array.isArray(keys) ? keys : [keys]).map((key) => [key, store[key]])); + window.chrome.storage.local.set = async (value) => { Object.assign(store, value); }; + window.fetch = async (url, options) => { + const body = JSON.parse(options.body); + if (url.endsWith('/api/validate-cats')) { + calls.validate.push(body.ids); + const result = await availability(body.ids); + return result; + } + calls.nearby.push(body); + return { ok: true, json: async () => nearbyResponse }; + }; + }); + + const serverSays = (availableIds) => async () => ({ ok: true, json: async () => ({ availableIds }) }); + const shownName = () => document.querySelector('#card h1')?.textContent; + const silenceConsoleError = async (fn) => { + const original = console.error; + console.error = () => {}; + try { await fn(); } finally { console.error = original; } + }; + + describe('when it runs', () => { + it('does not validate a pool that was fetched within the last 7 days', async () => { + store.feedCache = poolCache({ fetchedAt: Date.now() - 6 * DAY }); + + await window.start(); + + assert.equal(calls.validate.length, 0); + assert.equal(calls.nearby.length, 0); + assert.ok(['Cat1', 'Cat2', 'Cat3'].includes(shownName())); + }); + + it('validates a pool whose fetchedAt is 7 or more days old and has no validatedAt (a cache from before this field existed)', async () => { + store.feedCache = poolCache({ fetchedAt: Date.now() - 7 * DAY - 1000 }); + availability = serverSays(['1', '2', '3']); + + await window.start(); + + assert.equal(calls.validate.length, 1); + }); + + it('goes by validatedAt when it is set, even though fetchedAt is much older', async () => { + store.feedCache = poolCache({ fetchedAt: Date.now() - 30 * DAY, validatedAt: Date.now() - 1 * DAY }); + + await window.start(); + + assert.equal(calls.validate.length, 0, 'validated yesterday, so nothing to do'); + }); + + it('validates when validatedAt is old even though fetchedAt is recent (kept-unseen cards carried across a refresh)', async () => { + store.feedCache = poolCache({ fetchedAt: Date.now() - 1 * DAY, validatedAt: Date.now() - 9 * DAY }); + availability = serverSays(['1', '2', '3']); + + await window.start(); + + assert.equal(calls.validate.length, 1); + }); + + it('does nothing when the cache carries no usable timestamp at all', async () => { + store.feedCache = poolCache({ fetchedAt: undefined }); + + await window.start(); + + assert.equal(calls.validate.length, 0); + }); + + it('does nothing when there is no cache, or an empty one', async () => { + store.feedCache = null; + await window.start(); + store.feedCache = poolCache({ cards: [] }); + await window.start(); + + assert.equal(calls.validate.length, 0); + }); + + it('skips validation while offline, without recording a failure cooldown', async () => { + Object.defineProperty(window.navigator, 'onLine', { value: false, configurable: true }); + store.feedCache = poolCache(); + + await window.start(); + + assert.equal(calls.validate.length, 0); + assert.equal(store.feedCache.validationRetryAfter, undefined); + assert.ok(['Cat1', 'Cat2', 'Cat3'].includes(shownName()), 'the cache is still served'); + }); + }); + + describe('what it does to the pool', () => { + it('sends every cached id in one request and keeps only the ids still available, in their original order', async () => { + store.feedCache = poolCache({ cards: cardsFor(['1', '2', '3', '4', '5']) }); + availability = serverSays(['5', '3', '1']); // response order is irrelevant + + await window.start(); + + assert.deepEqual(calls.validate, [['1', '2', '3', '4', '5']]); + assert.deepEqual(store.feedCache.cards.map((c) => c.id), ['1', '3', '5']); + }); + + it('never shows a card that was found to be gone', async () => { + store.feedCache = poolCache({ cards: cardsFor(['dead1', 'dead2', 'alive']) }); + availability = serverSays(['alive']); + + await window.start(); + + assert.equal(shownName(), 'Catalive'); + }); + + it('keeps every still-available unseen card, and drops gone cards from seenIds too so the seen-ratio math stays honest', async () => { + store.feedCache = poolCache({ + cards: cardsFor(['1', '2', '3', '4', '5', '6']), + seenIds: ['1', '2'] // 1 alive+seen, 2 dead+seen; 3-6 unseen (4 dead) + }); + availability = serverSays(['1', '3', '5', '6']); + + await window.start(); + + // start() then serves one card from the pruned pool and marks it seen. + const ids = store.feedCache.cards.map((c) => c.id); + assert.deepEqual(ids, ['1', '3', '5', '6']); + assert.ok(!store.feedCache.seenIds.includes('2'), 'the dead-but-seen id must not linger as a phantom'); + assert.ok(!store.feedCache.seenIds.includes('4')); + assert.ok(store.feedCache.seenIds.includes('1'), 'a still-available seen card stays seen'); + assert.ok(store.feedCache.seenIds.every((id) => ids.includes(id))); + }); + + it('stamps validatedAt with now and leaves page, location, radius and fetchedAt exactly as they were', async () => { + const before = poolCache({ page: 4, radiusMiles: 75 }); + store.feedCache = before; + availability = serverSays(['1', '2', '3']); + + const start = Date.now(); + await window.start(); + + assert.ok(store.feedCache.validatedAt >= start && store.feedCache.validatedAt <= Date.now()); + assert.equal(store.feedCache.page, 4, 'validation must never advance pagination'); + assert.equal(store.feedCache.radiusMiles, 75); + assert.equal(store.feedCache.fetchedAt, before.fetchedAt); + assert.deepEqual(store.feedCache.location, LOCATION); + }); + + it('does not fetch a new page when validation alone is enough', async () => { + store.feedCache = poolCache(); + availability = serverSays(['1', '2']); + + await window.start(); + + assert.equal(calls.nearby.length, 0, 'plenty unseen and survivors left: no search, no radius escalation'); + }); + + it('does not validate again on the next tab once it has just run', async () => { + store.feedCache = poolCache(); + availability = serverSays(['1', '2', '3']); + + await window.start(); + await window.start(); + + assert.equal(calls.validate.length, 1); + }); + + it('splits a pool larger than 100 ids into 100-id requests and keeps survivors from every chunk', async () => { + const ids = Array.from({ length: 150 }, (_, i) => String(i + 1)); + store.feedCache = poolCache({ cards: cardsFor(ids) }); + availability = (asked) => ({ ok: true, json: async () => ({ availableIds: asked.filter((id) => Number(id) % 2 === 0) }) }); + + await window.start(); + + assert.deepEqual(calls.validate.map((chunk) => chunk.length), [100, 50]); + assert.deepEqual(calls.validate.flat(), ids); + assert.equal(store.feedCache.cards.length, 75); + assert.ok(store.feedCache.cards.every((c) => Number(c.id) % 2 === 0)); + }); + + it('leaves alone a card another tab added while the request was in flight', async () => { + store.feedCache = poolCache({ cards: cardsFor(['1', '2']) }); + availability = async () => { + // Another tab refreshes the cache while this one waits on the server. + store.feedCache = { ...store.feedCache, cards: [...store.feedCache.cards, ...cardsFor(['from-other-tab'])] }; + return { ok: true, json: async () => ({ availableIds: ['1'] }) }; + }; + + await window.start(); + + assert.deepEqual(store.feedCache.cards.map((c) => c.id).sort(), ['1', 'from-other-tab']); + }); + + it('deduplicates ids it asks about', async () => { + store.feedCache = poolCache({ cards: [...cardsFor(['1']), ...cardsFor(['1']), ...cardsFor(['2'])] }); + availability = serverSays(['1', '2']); + + await window.start(); + + assert.deepEqual(calls.validate, [['1', '2']]); + }); + }); + + describe('interaction with the normal refresh', () => { + it('still refreshes afterwards if pruning pushes the seen ratio over the threshold, and advances the page normally', async () => { + store.feedCache = poolCache({ + cards: cardsFor(['1', '2', '3', '4', '5', '6', '7', '8', '9', '10']), + seenIds: ['1', '2', '3', '4', '5', '6'], // 60% seen: no refresh on its own + page: 2 + }); + availability = serverSays(['1', '2', '3', '4', '5', '6']); // 7-10 (all unseen) are gone -> 100% seen + + await window.start(); + + assert.equal(calls.validate.length, 1); + assert.equal(calls.nearby.length, 1); + assert.equal(calls.nearby[0].page, 3, 'an ordinary top-up refresh: page + 1'); + }); + + it('carries validatedAt forward on a refresh that keeps unseen cards, so their age is not hidden', async () => { + const validatedAt = Date.now() - 3 * DAY; + store.feedCache = poolCache({ fetchedAt: Date.now() - 3 * DAY, validatedAt, cards: cardsFor(['1', '2', '3']), seenIds: ['1', '2'] }); + + await window.refresh(LOCATION); + + assert.equal(store.feedCache.validatedAt, validatedAt); + assert.ok(store.feedCache.fetchedAt > validatedAt); + assert.ok(store.feedCache.cards.some((c) => c.id === '3'), 'the unseen card was kept'); + }); + + it('falls back to fetchedAt as the carried-forward validatedAt for a pre-existing cache', async () => { + const fetchedAt = Date.now() - 3 * DAY; + store.feedCache = poolCache({ fetchedAt, cards: cardsFor(['1', '2']), seenIds: ['1'] }); + + await window.refresh(LOCATION); + + assert.equal(store.feedCache.validatedAt, fetchedAt); + }); + + it('resets validatedAt to now when a refresh keeps nothing from the old pool', async () => { + store.feedCache = poolCache({ validatedAt: Date.now() - 20 * DAY, cards: cardsFor(['1', '2']), seenIds: ['1', '2'] }); + + const start = Date.now(); + await window.refresh(LOCATION); + + assert.ok(store.feedCache.validatedAt >= start); + assert.deepEqual(store.feedCache.cards.map((c) => c.id).sort(), ['fresh1', 'fresh2']); + }); + + it('resets validatedAt for a brand-new location, and drops any failure cooldown with the rest of the old cache', async () => { + store.feedCache = poolCache({ location: { lat: 9, lon: 9 }, validatedAt: Date.now() - 20 * DAY, validationRetryAfter: Date.now() + DAY }); + + const start = Date.now(); + await window.refresh(LOCATION); + + assert.ok(store.feedCache.validatedAt >= start); + assert.equal(store.feedCache.validationRetryAfter, undefined); + }); + }); + + describe('when every cached card is gone', () => { + it('refreshes from page 1 with nothing kept, and never shows a dead card', async () => { + store.feedCache = poolCache({ page: 6, seenIds: ['1'] }); + availability = serverSays([]); + const shown = []; + const observer = new window.MutationObserver(() => { const n = shownName(); if (n) shown.push(n); }); + observer.observe(document.getElementById('card'), { childList: true, subtree: true }); + + await window.start(); + observer.disconnect(); + + assert.equal(calls.nearby.length, 1); + assert.equal(calls.nearby[0].page, 1, 'a dead pool restarts pagination instead of walking further out'); + assert.deepEqual(store.feedCache.cards.map((c) => c.id).sort(), ['fresh1', 'fresh2']); + assert.equal(store.feedCache.page, 1); + assert.ok(['Catfresh1', 'Catfresh2'].includes(shownName())); + assert.ok(shown.every((name) => name.startsWith('Catfresh')), `only fresh cats were ever shown, saw: ${shown}`); + }); + + it('leaves the old cache untouched, and shows the failure notice rather than a dead card, if that refresh fails', async () => { + const before = poolCache(); + store.feedCache = before; + availability = serverSays([]); + window.fetch = async (url, options) => { + if (url.endsWith('/api/validate-cats')) return serverSays([])(); + calls.nearby.push(JSON.parse(options.body)); + return { ok: false, json: async () => ({ error: 'Unable to refresh nearby cats right now.' }) }; + }; + + await silenceConsoleError(() => window.start()); + + assert.deepEqual(store.feedCache, before, 'nothing was written, so the cache is exactly as it was'); + assert.equal(document.getElementById('card').hidden, true); + assert.ok(document.getElementById('notice').textContent.length > 0); + }); + + it('refresh({ discardPool }) ignores the prior pool entirely, even for the same location', async () => { + store.feedCache = poolCache({ cards: cardsFor(['1', '2', '3']), seenIds: ['1'] }); + + await window.refresh(LOCATION, 'from you', { discardPool: true }); + + assert.equal(calls.nearby[0].page, 1); + assert.deepEqual(store.feedCache.cards.map((c) => c.id).sort(), ['fresh1', 'fresh2'], 'unseen 2 and 3 were not kept'); + }); + }); + + describe('when validation fails', () => { + const cacheBefore = () => poolCache(); + + it('serves the cache as usual after a server error, and records a 1 hour cooldown', async () => { + store.feedCache = cacheBefore(); + availability = async () => ({ ok: false, json: async () => ({ error: 'Unable to refresh nearby cats right now.' }) }); + + const start = Date.now(); + await silenceConsoleError(() => window.start()); + + assert.ok(['Cat1', 'Cat2', 'Cat3'].includes(shownName())); + assert.equal(store.feedCache.cards.length, 3, 'nothing was dropped'); + assert.ok(store.feedCache.validationRetryAfter >= start + 60 * 60 * 1000 - 1000); + assert.equal(store.feedCache.validatedAt, undefined, 'a failed check must not count as a validation'); + assert.equal(document.getElementById('notice').textContent, '', 'a background check failing is not the user\'s problem'); + assert.equal(calls.nearby.length, 0); + }); + + it('serves the cache when the request throws (network error or timeout)', async () => { + store.feedCache = cacheBefore(); + availability = async () => { throw new Error('network down'); }; + + await silenceConsoleError(() => window.start()); + + assert.ok(['Cat1', 'Cat2', 'Cat3'].includes(shownName())); + assert.equal(store.feedCache.cards.length, 3); + assert.ok(store.feedCache.validationRetryAfter > Date.now()); + }); + + it('serves the cache when an old server answers 404 (extension updated before the server)', async () => { + store.feedCache = cacheBefore(); + availability = async () => ({ ok: false, status: 404, json: async () => ({ error: 'Not found' }) }); + + await silenceConsoleError(() => window.start()); + + assert.ok(['Cat1', 'Cat2', 'Cat3'].includes(shownName())); + assert.equal(store.feedCache.cards.length, 3); + }); + + it('treats a malformed success body as a failure, never as "everything is gone"', async () => { + for (const body of [{}, { availableIds: 'nope' }, { availableIds: null }]) { + store.feedCache = cacheBefore(); + availability = async () => ({ ok: true, json: async () => body }); + + await silenceConsoleError(() => window.start()); + + assert.equal(store.feedCache.cards.length, 3, `body ${JSON.stringify(body)} must not empty the pool`); + assert.equal(calls.nearby.length, 0); + } + }); + + it('does not retry on the next tab within the cooldown, then does once it has passed', async () => { + store.feedCache = cacheBefore(); + availability = async () => { throw new Error('network down'); }; + await silenceConsoleError(() => window.start()); + assert.equal(calls.validate.length, 1); + + await window.start(); + assert.equal(calls.validate.length, 1, 'still cooling down'); + + store.feedCache = { ...store.feedCache, validationRetryAfter: Date.now() - 1000 }; + availability = serverSays(['1', '2', '3']); + await window.start(); + assert.equal(calls.validate.length, 2, 'cooldown over'); + assert.equal(store.feedCache.validationRetryAfter, undefined, 'a success clears the cooldown'); + assert.ok(store.feedCache.validatedAt); + }); + + it('gives up after the first failed chunk and prunes nothing, even if an earlier chunk succeeded', async () => { + const ids = Array.from({ length: 150 }, (_, i) => String(i + 1)); + store.feedCache = poolCache({ cards: cardsFor(ids) }); + let chunkNumber = 0; + availability = async () => { + chunkNumber++; + if (chunkNumber === 2) throw new Error('second chunk failed'); + return { ok: true, json: async () => ({ availableIds: [] }) }; + }; + + await silenceConsoleError(() => window.start()); + + assert.equal(store.feedCache.cards.length, 150, 'a partial answer must never prune'); + }); + + it('still starts even if recording the cooldown itself fails', async () => { + store.feedCache = cacheBefore(); + availability = async () => { throw new Error('network down'); }; + const realSet = window.chrome.storage.local.set; + window.chrome.storage.local.set = async (value) => { + if (value.feedCache && value.feedCache.validationRetryAfter) throw new Error('quota'); + return realSet(value); + }; + + await silenceConsoleError(() => window.start()); + + assert.ok(['Cat1', 'Cat2', 'Cat3'].includes(shownName())); + }); + }); + }); + describe('explore another area', () => { beforeEach(() => { window.chrome.storage.local.get = async () => ({ diff --git a/test/rescuegroups.test.js b/test/rescuegroups.test.js index a41f81f..145d366 100644 --- a/test/rescuegroups.test.js +++ b/test/rescuegroups.test.js @@ -1,6 +1,6 @@ import assert from "node:assert/strict"; import test from "node:test"; -import { buildSearchRequest, normalizeCards, validateLocation, searchRadius, findNearbyCats } from "../server/rescuegroups.js"; +import { buildSearchRequest, normalizeCards, validateLocation, searchRadius, findNearbyCats, validateAnimalIds, buildAvailabilityRequest, findAvailableIds } from "../server/rescuegroups.js"; test("ZIP fallback uses RescueGroups native postalcode radius filter", () => { const request = buildSearchRequest({ postalcode: "33629" }, 25); @@ -519,3 +519,77 @@ test("searchRadius returns normalized cards on successful response", async () => const body = JSON.parse(fetchOptions.body); assert.deepEqual(body, { data: { filterRadius: { postalcode: "33629", miles: 25 } } }); }); + +// Issue #79: revalidating specific cached listings by id. + +test("validateAnimalIds accepts numeric strings and numbers, de-duplicated, as strings", () => { + assert.deepEqual(validateAnimalIds(["101", 202, "101"]), ["101", "202"]); +}); + +test("validateAnimalIds rejects anything that isn't 1-100 numeric ids", () => { + const message = /Provide 1 to 100 numeric animal ids\./; + assert.throws(() => validateAnimalIds(undefined), message); + assert.throws(() => validateAnimalIds("101"), message); + assert.throws(() => validateAnimalIds([]), message); + assert.throws(() => validateAnimalIds(Array.from({ length: 101 }, (_, i) => String(i + 1))), message); + assert.throws(() => validateAnimalIds(["101", "abc"]), message); + assert.throws(() => validateAnimalIds(["101", "12 34"]), message); + assert.throws(() => validateAnimalIds(["1; DROP"]), message); + assert.throws(() => validateAnimalIds(["101", null]), message); + assert.throws(() => validateAnimalIds([{ id: "101" }]), message); + assert.throws(() => validateAnimalIds(["1234567890123"]), message, "an id longer than 12 digits is not a real RescueGroups id"); + assert.equal(validateAnimalIds(Array.from({ length: 100 }, (_, i) => String(i + 1))).length, 100, "exactly 100 is allowed"); +}); + +test("buildAvailabilityRequest filters the available-cats endpoint by animals.id, one page of up to 100", () => { + const request = buildAvailabilityRequest(["101", "202"]); + const url = new URL(request.url); + assert.match(url.pathname, /\/public\/animals\/search\/available\/cats\/haspic\/$/); + assert.equal(url.searchParams.get("limit"), "100"); + assert.equal(url.searchParams.get("page"), "1"); + assert.equal(url.searchParams.get("fields[animals]"), "name", "only the id is needed, so keep the payload minimal"); + assert.deepEqual(request.body, { data: { filters: [{ fieldName: "animals.id", operation: "equal", criteria: ["101", "202"] }] } }); +}); + +test("findAvailableIds returns only the requested ids RescueGroups still lists", async () => { + let sent; + const fetchImpl = async (url, options) => { + sent = { url, options }; + return { ok: true, status: 200, json: async () => ({ data: [{ id: "101" }, { id: 303 }] }) }; + }; + const result = await findAvailableIds(["101", "202", "303"], { apiKey: "test", fetchImpl }); + assert.deepEqual(result, ["101", "303"], "202 wasn't returned, so it's no longer available"); + assert.equal(sent.options.method, "POST"); + assert.equal(sent.options.headers.Authorization, "test"); +}); + +test("findAvailableIds ignores ids in the response that weren't asked about", async () => { + const fetchImpl = async () => ({ ok: true, status: 200, json: async () => ({ data: [{ id: "101" }, { id: "999" }] }) }); + assert.deepEqual(await findAvailableIds(["101"], { apiKey: "test", fetchImpl }), ["101"]); +}); + +test("findAvailableIds returns an empty list when nothing is still available, without treating it as an error", async () => { + const fetchImpl = async () => ({ ok: true, status: 200, json: async () => ({ data: [] }) }); + assert.deepEqual(await findAvailableIds(["101"], { apiKey: "test", fetchImpl }), []); + const noDataFetch = async () => ({ ok: true, status: 200, json: async () => ({}) }); + assert.deepEqual(await findAvailableIds(["101"], { apiKey: "test", fetchImpl: noDataFetch }), []); +}); + +test("findAvailableIds rejects bad ids before touching the network, even with no API key configured", async () => { + let called = false; + const fetchImpl = async () => { called = true; }; + await assert.rejects(findAvailableIds(["nope"], { apiKey: "test", fetchImpl }), /Provide 1 to 100 numeric animal ids/); + await assert.rejects(findAvailableIds(["nope"], { fetchImpl }), /Provide 1 to 100 numeric animal ids/); + assert.equal(called, false); +}); + +test("findAvailableIds requires an API key", async () => { + await assert.rejects(findAvailableIds(["101"], { fetchImpl: async () => ({}) }), /RG_API_KEY is not configured/); +}); + +test("findAvailableIds surfaces RescueGroups HTTP errors and malformed bodies like searchRadius does", async () => { + const httpError = async () => ({ ok: false, status: 429, json: async () => ({ errors: [{ detail: "Rate limited." }] }) }); + await assert.rejects(findAvailableIds(["101"], { apiKey: "test", fetchImpl: httpError }), /RescueGroups HTTP 429: Rate limited\./); + const malformed = async () => ({ ok: true, status: 200, json: async () => null }); + await assert.rejects(findAvailableIds(["101"], { apiKey: "test", fetchImpl: malformed }), /empty or malformed response body/); +}); diff --git a/test/server.test.js b/test/server.test.js index 493d5e2..5414d76 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -477,6 +477,125 @@ describe("server routing and behavior", () => { }); }); +describe("POST /api/validate-cats (issue #79)", () => { + let port; + + beforeEach(async () => { + cache.clear(); + resetRateLimitsForTests(); + resetAlertStateForTests(); + await new Promise((resolve) => server.listen(0, resolve)); + port = server.address().port; + }); + + afterEach(async () => { + await new Promise((resolve) => server.close(resolve)); + mock.restoreAll(); + }); + + const request = (options, body = null) => new Promise((resolve, reject) => { + const req = http.request({ ...options, hostname: "127.0.0.1", port }, (res) => { + let data = ""; + res.on("data", (chunk) => data += chunk); + res.on("end", () => resolve({ res, data })); + }); + req.on("error", reject); + if (body !== null) req.write(body); + req.end(); + }); + const validate = (ids) => request({ path: "/api/validate-cats", method: "POST" }, JSON.stringify({ ids })); + + it("returns the ids RescueGroups still lists as available, with CORS headers", async () => { + process.env.RG_API_KEY = "test-key"; + const fetchSpy = mock.method(global, "fetch", async () => ({ ok: true, status: 200, json: async () => ({ data: [{ id: "101" }] }) })); + + const { res, data } = await validate(["101", "202"]); + + assert.strictEqual(res.statusCode, 200); + assert.deepStrictEqual(JSON.parse(data), { availableIds: ["101"] }); + assert.ok(res.headers["access-control-allow-origin"]); + assert.strictEqual(fetchSpy.mock.callCount(), 1, "one RescueGroups request per call"); + const [url, options] = fetchSpy.mock.calls[0].arguments; + assert.match(url, /available\/cats\/haspic/); + assert.deepStrictEqual(JSON.parse(options.body).data.filters, [{ fieldName: "animals.id", operation: "equal", criteria: ["101", "202"] }]); + }); + + it("is never served from the response cache, and does not populate it", async () => { + process.env.RG_API_KEY = "test-key"; + const fetchSpy = mock.method(global, "fetch", async () => ({ ok: true, status: 200, json: async () => ({ data: [] }) })); + + await validate(["101"]); + await validate(["101"]); + + assert.strictEqual(fetchSpy.mock.callCount(), 2, "availability is only useful fresh"); + assert.strictEqual(cache.size, 0); + }); + + it("returns 405 with an Allow header for a non-POST method", async () => { + const { res } = await request({ path: "/api/validate-cats", method: "GET" }); + assert.strictEqual(res.statusCode, 405); + assert.strictEqual(res.headers.allow, "POST"); + }); + + it("answers the OPTIONS preflight", async () => { + const { res } = await request({ path: "/api/validate-cats", method: "OPTIONS" }); + assert.strictEqual(res.statusCode, 204); + }); + + it("rejects invalid ids with a 400 and never calls RescueGroups", async () => { + process.env.RG_API_KEY = "test-key"; + mock.method(console, "error", () => {}); + const fetchSpy = mock.method(global, "fetch", async () => ({ ok: true, json: async () => ({ data: [] }) })); + + for (const ids of [undefined, "101", [], ["abc"], ["101", null], Array.from({ length: 101 }, (_, i) => String(i + 1))]) { + const { res, data } = await validate(ids); + assert.strictEqual(res.statusCode, 400, `ids=${JSON.stringify(ids)?.slice(0, 40)} should be a 400`); + assert.strictEqual(JSON.parse(data).error, "Provide 1 to 100 numeric animal ids."); + } + assert.strictEqual(fetchSpy.mock.callCount(), 0); + }); + + it("returns 400 for a malformed JSON body, and for a body that is JSON null", async () => { + mock.method(console, "error", () => {}); + const malformed = await request({ path: "/api/validate-cats", method: "POST" }, "{not json"); + assert.strictEqual(malformed.res.statusCode, 400); + const nullBody = await request({ path: "/api/validate-cats", method: "POST" }, "null"); + assert.strictEqual(nullBody.res.statusCode, 400); + }); + + it("returns 502 with the generic message when RescueGroups fails", async () => { + process.env.RG_API_KEY = "test-key"; + mock.method(global, "fetch", async () => { throw new Error("Raw upstream crash"); }); + const errorSpy = mock.method(console, "error", () => {}); + + const { res, data } = await validate(["101"]); + + assert.strictEqual(res.statusCode, 502); + assert.strictEqual(JSON.parse(data).error, "Unable to refresh nearby cats right now."); + assert.strictEqual(errorSpy.mock.calls[0].arguments[1].message, "Raw upstream crash"); + }); + + it("shares the per-IP rate limit with /api/nearby-cats", async () => { + mock.method(console, "error", () => {}); + const badNearby = JSON.stringify({ location: { postalcode: "123" } }); + for (let i = 0; i < 15; i++) await request({ path: "/api/nearby-cats", method: "POST" }, badNearby); + for (let i = 0; i < 15; i++) { + const { res } = await validate(["abc"]); + assert.strictEqual(res.statusCode, 400, `request ${i + 16} of 30 is still within the limit`); + } + + const { res } = await validate(["abc"]); + assert.strictEqual(res.statusCode, 429); + const nearby = await request({ path: "/api/nearby-cats", method: "POST" }, badNearby); + assert.strictEqual(nearby.res.statusCode, 429, "one shared budget, not one per route"); + }); + + it("still 404s a path that only starts with the route name", async () => { + const { res } = await request({ path: "/api/validate-cats/extra", method: "POST" }, "{}"); + assert.strictEqual(res.statusCode, 404); + }); +}); + describe("upstream failure alerting", () => { let port; const webhookUrl = "https://discord.com/api/webhooks/test/token"; From dae6822163b86b46a3c3f2adfee1994840636aa4 Mon Sep 17 00:00:00 2001 From: BrandonML Date: Tue, 29 Sep 2026 13:17:15 -0400 Subject: [PATCH 3/3] test(release): close the release safety-net gaps around /api/validate-cats Follow-up to the #79 fix, on the same PR. The new route fails open in the extension (a broken route just means stale cards keep being served), so nothing user-visible would flag it breaking in production -- yet none of the pre-merge, CI or post-deploy checks covered it. This closes that, and makes the same class of gap fail the build in future. - deploy-verify.yml: the post-deploy production smoke test now calls /api/validate-cats: ids /api/nearby-cats just listed plus one that cannot exist (12 digits) -- the impossible one must never come back and at least one real one must -- and confirms invalid input is rejected with a 400. The assertion logic was exercised locally for the pass and both failure modes. - test/revalidation-integration.test.js (runs in CI): the real newtab.js in jsdom against the real server over a real socket, with only RescueGroups faked. Covers the client<->server contract, which the two halves' own tests only checked against mocks of each other. Contract mutations (renamed route, renamed response field, wrong filter operation, no pruning) each fail it. - test-live/stale-cache.live.js: the same flow against the real RescueGroups API, starting the server in-process (nothing to run first). Promotes the ad hoc end-to-end script used to verify the fix into a repeatable test. Live tests now use an id that can never be listed (12 digits) instead of 8 digits that could plausibly be assigned someday. - test-support/newtab-harness.js: the shared page harness for both suites. - test/deploy-coverage.test.js: fails if a server route lacks a smoke check in deploy-verify.yml or a mention in README/RELEASE.md, if a file in test-live/ isn't wired into `npm run test:live`, or if one fails to load. - package.json: test:live runs every live file. eslint and .dockerignore cover test-support/. - RELEASE.md: pre-merge now requires `npm run test:live` and a real-browser manual QA pass (with the old-cache simulation for cache changes); post-merge lists /api/validate-cats among the smoke-tested routes. - README.md: CI section says ci.yml runs lint too (it always has) and lists the new smoke check; validation section documents the live suites, the integration test, and a "simulating an old cache" manual QA recipe. Tier 3: lint clean; 339/339 (was 325) on two consecutive runs; 7/7 live tests against the real API. Co-Authored-By: Claude Sonnet 5.5 --- .dockerignore | 1 + .github/workflows/deploy-verify.yml | 39 ++++++ README.md | 21 ++- RELEASE.md | 4 +- eslint.config.js | 2 +- package.json | 2 +- test-live/rescuegroups.live.js | 2 +- test-live/stale-cache.live.js | 101 ++++++++++++++ test-support/newtab-harness.js | 85 ++++++++++++ test/deploy-coverage.test.js | 75 +++++++++++ test/revalidation-integration.test.js | 185 ++++++++++++++++++++++++++ 11 files changed, 510 insertions(+), 7 deletions(-) create mode 100644 test-live/stale-cache.live.js create mode 100644 test-support/newtab-harness.js create mode 100644 test/deploy-coverage.test.js create mode 100644 test/revalidation-integration.test.js diff --git a/.dockerignore b/.dockerignore index 87feccf..2eb46f7 100644 --- a/.dockerignore +++ b/.dockerignore @@ -5,6 +5,7 @@ node_modules extension test test-live +test-support benchmark webstore tasks diff --git a/.github/workflows/deploy-verify.yml b/.github/workflows/deploy-verify.yml index 304068e..f09b13d 100644 --- a/.github/workflows/deploy-verify.yml +++ b/.github/workflows/deploy-verify.yml @@ -81,4 +81,43 @@ jobs: curl -fsS -o /dev/null -w "status=%{http_code} size=%{size_download}\n" \ "$PROD_URL/api/photo-share?url=$(node -e "console.log(encodeURIComponent(process.argv[1]))" "$IMAGE_URL")" + # Issue #79: the extension revalidates a long-idle cache through this + # route. It fails open (a broken route just means stale cards keep + # getting served), so nothing else would notice it breaking in + # production. Ask about ids /api/nearby-cats just listed plus one + # that cannot exist (12 digits is the validator's max length): the + # impossible one must never come back, and at least one of the real + # ones must (not all -- a cat can be adopted between the two calls). + echo "-- POST /api/validate-cats --" + VALIDATE_BODY=$(echo "$NEARBY" | jq -c '{ids: ([.cards[0:3][].id] + ["999999999999"])}') + VALIDATED=$(curl -fsS -X POST "$PROD_URL/api/validate-cats" \ + -H "Content-Type: application/json" \ + -d "$VALIDATE_BODY") + echo "$VALIDATED" | jq . + node -e ' + const asked = JSON.parse(process.argv[1]).ids; + const got = JSON.parse(process.argv[2]).availableIds; + const fake = asked.at(-1); + const real = asked.slice(0, -1); + if (got.includes(fake)) { + console.error("::error::/api/validate-cats reported an impossible id as available."); + process.exit(1); + } + if (!real.some((id) => got.includes(id))) { + console.error("::error::/api/validate-cats returned none of the ids /api/nearby-cats had just listed -- the by-id filter may have stopped working."); + process.exit(1); + } + console.log(`ok: ${got.length} of ${asked.length} ids reported available`); + ' "$VALIDATE_BODY" "$VALIDATED" + + echo "-- POST /api/validate-cats rejects invalid ids --" + STATUS=$(curl -s -o /dev/null -w "%{http_code}" -X POST "$PROD_URL/api/validate-cats" \ + -H "Content-Type: application/json" \ + -d '{"ids":["not-an-id"]}') + echo "status=$STATUS" + if [ "$STATUS" != "400" ]; then + echo "::error::/api/validate-cats returned $STATUS for an invalid id (expected 400) -- the route may be missing or its input validation broken." + exit 1 + fi + echo "All smoke checks passed -- safe to proceed with store submission." diff --git a/README.md b/README.md index e8ab40c..ae6229c 100644 --- a/README.md +++ b/README.md @@ -63,9 +63,9 @@ This bumps `manifest.json`/`package.json` to the given version and zips `manifes Three GitHub Actions workflows automate the release process end to end — see [RELEASE.md](RELEASE.md) for the step-by-step runbook: -- **`ci.yml`** — runs `npm test` on every push and pull request to `main`/`dev`, pinned to Node 22 to match the Dockerfile. `main` has a ruleset (Settings → Rules → Rulesets) requiring this check to pass and requiring a pull request before merging — the workflow alone doesn't block anything, only the ruleset does. +- **`ci.yml`** — runs `npm run lint` and `npm test` on every push and pull request to `main`/`dev`, pinned to Node 22 to match the Dockerfile. `main` has a ruleset (Settings → Rules → Rulesets) requiring this check to pass and requiring a pull request before merging — the workflow alone doesn't block anything, only the ruleset does. - **`tag-release.yml`** — on every push to `main`, tags the commit `v` (read from `manifest.json`) if that tag doesn't already exist. Idempotent, so it's safe to fire on every push rather than needing to detect "was this actually a release." -- **`deploy-verify.yml`** — on every push to `main` that touches `server/**` or `Dockerfile` (mirroring the `tabby` service's own Northflank build trigger), polls the live `/healthz` endpoint until its `sha` field (see below) matches the pushed commit, then smoke-tests `/api/nearby-cats`, `/api/photo-thumb`, and `/api/photo-share` against production. A **green run is the signal that it's safe to build and submit the release to CWS/EWS** — since Northflank deploys in minutes and store review takes hours, the server is always live and correct well before any user's browser updates to a new extension version, as long as server changes stay additive/backward-compatible with whatever extension version is still in the wild. +- **`deploy-verify.yml`** — on every push to `main` that touches `server/**` or `Dockerfile` (mirroring the `tabby` service's own Northflank build trigger), polls the live `/healthz` endpoint until its `sha` field (see below) matches the pushed commit, then smoke-tests `/api/nearby-cats`, `/api/validate-cats`, `/api/photo-thumb`, and `/api/photo-share` against production. A **green run is the signal that it's safe to build and submit the release to CWS/EWS** — since Northflank deploys in minutes and store review takes hours, the server is always live and correct well before any user's browser updates to a new extension version, as long as server changes stay additive/backward-compatible with whatever extension version is still in the wild. `/healthz` reports `{ status: "ok", sha }`, where `sha` is Northflank's auto-injected `NF_DEPLOYMENT_SHA` runtime env var (the exact git commit of the running build) — `null` locally, where that variable is never set. This is what lets `deploy-verify.yml` confirm the *new* code is actually live, not just that some process answered the health check. @@ -92,10 +92,25 @@ npm.cmd test Both run in CI as part of the same required `test` check; a lint failure blocks merge exactly like a test failure. Linting is `eslint.config.js`, no separate config file per directory — it enforces `no-eval`/`no-implied-eval`/`no-new-func`/`no-script-url` repo-wide (see AGENTS.md's "Security-First Coding") on top of `eslint:recommended`, deliberately without a formatter (no Prettier) or stylistic rules beyond that. -Run the live RescueGroups integration test once against the real API before merging any change to search radius, pagination, or the RescueGroups query contract, to confirm the pagination contract still holds. It requires a real `RG_API_KEY` (loaded from `.env`, same as `start:server`) and is excluded from `npm test`/CI by design: +Run the live tests against the real API before every release (see [RELEASE.md](RELEASE.md)) and before merging any change to search radius, pagination, or the RescueGroups query contract. `test:live` runs everything in `test-live/`: the pagination/radius contract and the by-id availability contract (`rescuegroups.live.js`), and the stale-cache revalidation flow end to end (`stale-cache.live.js`, which starts the real server in-process and drives the real `newtab.js` against the real API, so nothing needs to be running first). It requires a real `RG_API_KEY` (loaded from `.env`, same as `start:server`) and is excluded from `npm test`/CI by design: ```powershell npm.cmd run test:live ``` +`npm test` also includes `test/revalidation-integration.test.js`, which runs the real `newtab.js` (jsdom) against the real server with only RescueGroups faked, so the extension/server contract is covered in CI and not just each half against a mock of the other; `test-support/` holds the harness it shares with the live suite. `test/deploy-coverage.test.js` fails if a server route is added without a `deploy-verify.yml` smoke check and a mention here and in RELEASE.md, or if a file in `test-live/` isn't wired into `test:live`. + +### Manual QA: simulating an old cache + +The stale-cache check (issue #79) only fires for a cache that's 7+ days old, so seeing it in a real browser means aging one by hand. With the unpacked extension loaded and `npm run start:server` running on this branch, open a new tab, press F12, and in the console (type `allow pasting` first if Chrome asks): + +```js +const { feedCache } = await chrome.storage.local.get("feedCache"); +const old = Date.now() - 8 * 24 * 60 * 60 * 1000; +const fake = { ...feedCache.cards[0], id: "999999999999", name: "FAKE DEAD CAT" }; +await chrome.storage.local.set({ feedCache: { ...feedCache, cards: [fake, ...feedCache.cards], fetchedAt: old, validatedAt: old } }); +``` + +Reload the tab. The Network tab should show one `POST /api/validate-cats`, "FAKE DEAD CAT" must never appear, and re-reading `feedCache` should show it gone with `page` unchanged, `fetchedAt` still 8 days old, and `validatedAt` about now. Also worth trying: stop the server first (a cat should still render with no notice, and `validationRetryAfter` should be set about an hour out), and set a few unseen cards' `imageUrl` to a bogus URL (a broken photo should skip ahead silently). + The project design follows the architecture document in the parent workspace. The API key is intentionally absent from all source files. diff --git a/RELEASE.md b/RELEASE.md index 43a27f7..7cbf21e 100644 --- a/RELEASE.md +++ b/RELEASE.md @@ -12,13 +12,15 @@ Steps to ship a change to `main`, which auto-deploys the server to Northflank, a - [ ] Exception: if several feature branches are ready to ship together, merge them all into a shared `dev` branch first, then open one PR from `dev` into `main`. Use this only when bundling multiple branches — a single feature branch goes straight to `main`. - [ ] Bump `manifest.json`/`package.json` to the new version and add a row to [README.md](README.md)'s Version History table, as a commit on the same branch/PR — this is what `tag-release.yml` reads post-merge, so it needs to land in the same merge as the change it describes rather than as a follow-up. - [ ] Documentation currency check (see AGENTS.md's "Documentation & Asset Currency"): does this release make README.md, this file, or `webstore/WEBSTORE.md` (copy or screenshots/promo assets) inaccurate? Fix small stuff directly in this PR. For a larger asset-generation task (e.g. regenerating screenshots), don't block the release on it — file a tracking issue instead and note it in the PR description, the way [issue #66](https://github.com/BrandonML/tabby/issues/66) tracks the icon-menu change making the screenshot harness stale. +- [ ] Run `npm run test:live` locally with a real `RG_API_KEY` in `.env` and confirm it passes. It's excluded from CI by design (it needs the real key and hits the real RescueGroups API), so nothing else checks that the pagination/radius contract, the by-id availability query, and the stale-cache revalidation flow still work against the real service. +- [ ] Manual QA in a real browser, per AGENTS.md's Manual QA Checklist (load unpacked, cards render, settings save/close, no console errors in the service worker or new-tab page). If the release touches the cache or revalidation logic, also run the old-cache simulation in [README.md](README.md)'s "Manual QA: simulating an old cache" section. - [ ] Wait for the `test` check to go green. If it's red, stop — do not merge. - [ ] Merge the PR. ## 3. Post-merge (fully automated — just watch) - [ ] `.github/workflows/tag-release.yml` tags the commit `v` (read from `manifest.json`). Check the repo's Tags page. -- [ ] `.github/workflows/deploy-verify.yml` polls the live server's `/healthz` until it reports this exact commit's SHA (`NF_DEPLOYMENT_SHA`, injected by Northflank), then smoke-tests `/api/nearby-cats`, `/api/photo-thumb`, and `/api/photo-share` against production. **A green run is the signal that it's safe to proceed to store submission.** If it goes red or times out, stop and check the `tabby` service's build/deploy status directly on Northflank before doing anything else. +- [ ] `.github/workflows/deploy-verify.yml` polls the live server's `/healthz` until it reports this exact commit's SHA (`NF_DEPLOYMENT_SHA`, injected by Northflank), then smoke-tests `/api/nearby-cats`, `/api/validate-cats`, `/api/photo-thumb`, and `/api/photo-share` against production. **A green run is the signal that it's safe to proceed to store submission.** If it goes red or times out, stop and check the `tabby` service's build/deploy status directly on Northflank before doing anything else. ## 4. Store submission (manual — both stores require a human in their dashboard) diff --git a/eslint.config.js b/eslint.config.js index 0f73494..eb24029 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -43,7 +43,7 @@ export default [ rules: securityRules }, { - files: ["test/**/*.js", "test-live/**/*.js"], + files: ["test/**/*.js", "test-live/**/*.js", "test-support/**/*.js"], languageOptions: { ecmaVersion: "latest", sourceType: "module", diff --git a/package.json b/package.json index 399c6f0..c42f1aa 100644 --- a/package.json +++ b/package.json @@ -5,7 +5,7 @@ "type": "module", "scripts": { "test": "node --test", - "test:live": "node --env-file-if-exists=.env --test test-live/rescuegroups.live.js", + "test:live": "node --env-file-if-exists=.env --test test-live/rescuegroups.live.js test-live/stale-cache.live.js", "start:server": "node --env-file-if-exists=.env server/index.js", "release": "node scripts/release.js", "lint": "eslint ." diff --git a/test-live/rescuegroups.live.js b/test-live/rescuegroups.live.js index 7a9a922..bdb7b12 100644 --- a/test-live/rescuegroups.live.js +++ b/test-live/rescuegroups.live.js @@ -136,7 +136,7 @@ describe("RescueGroups live by-id availability contract", () => { }).then((response) => response.json()); const realIds = idsOf(seed).slice(0, 20); assert.ok(realIds.length >= 5, "need a handful of real available ids to test with"); - const fakeIds = ["99999991", "99999992"]; + const fakeIds = ["999999999991", "999999999992"]; const availableIds = await findAvailableIds([...realIds, ...fakeIds], { apiKey: API_KEY }); diff --git a/test-live/stale-cache.live.js b/test-live/stale-cache.live.js new file mode 100644 index 0000000..d27afdf --- /dev/null +++ b/test-live/stale-cache.live.js @@ -0,0 +1,101 @@ +// Live end-to-end check for issue #79 (stale-cache revalidation): the real +// newtab.js in jsdom -> the real server/index.js (started in-process on a free +// port, so nothing needs to be running) -> the REAL RescueGroups API. It's the +// same flow as test/revalidation-integration.test.js, which runs in CI with +// RescueGroups faked; this is the one that proves the by-id availability query +// still behaves against the real service. NOT run by `npm test`/CI -- it needs +// a real RG_API_KEY and makes a handful of real API requests (about 6). Run it +// with `npm run test:live` before every release (see RELEASE.md) and after +// any change to the query contract. Safe to leave in the repo without a key: +// it logs a message and exits 0 rather than failing. +import { describe, it, before, after } from "node:test"; +import assert from "node:assert"; +import { cache, server } from "../server/index.js"; +import { openNewtab } from "../test-support/newtab-harness.js"; + +if (!process.env.RG_API_KEY) { + console.log("Skipping live stale-cache test — set RG_API_KEY to run"); + process.exit(0); +} + +const DAY = 24 * 60 * 60 * 1000; +const ZIP = "10001"; +// 12 digits is the longest id the server accepts, and far past any real +// RescueGroups animal id (currently 8 digits), so these can never be listed. +const deadCard = (template, n) => ({ ...template, id: String(999999999990 + n), name: `DEAD-${n}` }); + +describe("stale-cache revalidation against the real RescueGroups API", () => { + let backendUrl; + let realCards; // a real page of cats, fetched once through the app itself + + const openTab = async (store) => { + const tab = openNewtab({ backendUrl, store }); + await tab.start(); + return tab; + }; + const cacheOf = (cards, { ageDays, page = 1, seenIds = [] }) => ({ + settings: { postalcode: ZIP, location: null }, + feedCache: { cards, fetchedAt: Date.now() - ageDays * DAY, location: { postalcode: ZIP }, page, radiusMiles: 25, seenIds } + }); + + before(async () => { + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + backendUrl = `http://127.0.0.1:${server.address().port}`; + const store = { settings: { postalcode: ZIP, location: null } }; + await openTab(store); // empty cache: exercises the real refresh path end to end + realCards = store.feedCache.cards; + console.log("[live] seeded", realCards.length, "real cats for", ZIP); + assert.ok(realCards.length > 20, "need a real pool of cats to test with"); + }); + + after(async () => { + await new Promise((resolve) => server.close(resolve)); + }); + + it("drops dead ids from an 8-day-old pool, keeps every real cat, and never advances the page", async () => { + const dead = [0, 1, 2, 3, 4].map((n) => deadCard(realCards[0], n)); + const pool = [...realCards.slice(0, 40), ...dead, ...realCards.slice(40)]; + const store = cacheOf(pool, { ageDays: 8, seenIds: [realCards[0].id, dead[0].id] }); + const staleFetchedAt = store.feedCache.fetchedAt; + + const tab = await openTab(store); + + const validateRequests = tab.requestsTo("/api/validate-cats"); + const ids = store.feedCache.cards.map((card) => card.id); + console.log("[live] validate requests:", validateRequests.length, "| pool", pool.length, "->", ids.length); + assert.strictEqual(validateRequests.length, Math.ceil(pool.length / 100), "one request per up-to-100 ids"); + assert.strictEqual(tab.requestsTo("/api/nearby-cats").length, 0, "validation alone is enough: no search, no new page"); + assert.deepStrictEqual(ids.filter((id) => dead.some((card) => card.id === id)), [], "every dead card was dropped"); + const kept = realCards.filter((card) => ids.includes(card.id)).length; + assert.ok(kept >= realCards.length - 2, `real cats should survive (kept ${kept} of ${realCards.length}; a couple may be adopted mid-test)`); + assert.strictEqual(store.feedCache.page, 1); + assert.strictEqual(store.feedCache.fetchedAt, staleFetchedAt); + assert.ok(Date.now() - store.feedCache.validatedAt < 60_000, "validatedAt was stamped"); + assert.ok(!store.feedCache.seenIds.includes(dead[0].id), "a dead card's seen id was dropped too"); + assert.ok(!tab.shownName().startsWith("DEAD-"), "a dead card was never shown"); + }); + + it("restarts from page 1 with a fresh pool when every cached card is dead", async () => { + cache.clear(); // the server's 3-minute response cache: a real week would have expired it + const store = cacheOf([0, 1, 2].map((n) => deadCard(realCards[0], n)), { ageDays: 35, page: 4 }); + + const tab = await openTab(store); + + const searches = tab.requestsTo("/api/nearby-cats"); + console.log("[live] nearby requests:", searches.length, "| pages:", searches.map((s) => s.body.page), "| new pool:", store.feedCache.cards.length); + assert.deepStrictEqual(searches.map((s) => s.body.page), [1], "a dead pool restarts pagination instead of walking further out"); + assert.ok(store.feedCache.cards.length > 20); + assert.ok(store.feedCache.cards.every((card) => !card.id.startsWith("99999999999"))); + assert.strictEqual(store.feedCache.page, 1); + assert.ok(!tab.shownName().startsWith("DEAD-")); + }); + + it("makes no requests at all for a pool that was checked 2 days ago", async () => { + const store = cacheOf(realCards, { ageDays: 2 }); + + const tab = await openTab(store); + + assert.strictEqual(tab.requests.length, 0); + assert.ok(tab.shownName()); + }); +}); diff --git a/test-support/newtab-harness.js b/test-support/newtab-harness.js new file mode 100644 index 0000000..618a7e7 --- /dev/null +++ b/test-support/newtab-harness.js @@ -0,0 +1,85 @@ +// Shared harness for the two end-to-end suites -- test/revalidation-integration.test.js +// (CI: real server + real newtab.js, RescueGroups mocked) and +// test-live/stale-cache.live.js (manual: same, against the real RescueGroups +// API). Lives outside test/ on purpose: Node's test discovery runs every .js +// file under a directory named "test", and this is a helper, not a test. +// +// It loads the real extension/newtab.html + newtab.js into jsdom -- not a +// re-implementation -- pointed at a backend of the caller's choosing, with +// chrome.storage backed by a plain object the caller owns. Reusing the same +// `store` across openNewtab() calls models closing a tab and opening another +// one, which is exactly what the cache-age logic cares about. +import { JSDOM } from "jsdom"; +import fs from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); +// Captured at import time, before any suite mocks globalThis.fetch to fake +// RescueGroups -- requests from the page must still reach the real server. +const nativeFetch = globalThis.fetch; + +function readExtensionFile(name) { + return fs.readFileSync(path.join(ROOT, "extension", name), "utf8"); +} + +// Same inlining trick as test/newtab.test.js: newtab.js is an ES module, but +// the harness runs it as a classic script, so its imports are replaced with +// the imported files' own source. +function buildScript(backendUrl) { + const errorMessages = readExtensionFile("error-messages.js").replace(/export function/g, "function"); + const config = readExtensionFile("config.js").replace("export const", "const").replace("http://localhost:8787", backendUrl); + const location = readExtensionFile("location.js").replace("export async function", "async function"); + const script = readExtensionFile("newtab.js") + .replace(/import \{ classifyRefreshError, isInvalidZipError \} from "\.\/error-messages\.js";/, () => errorMessages) + .replace(/import \{ BACKEND_URL \} from "\.\/config\.js";/, () => config) + .replace(/import \{ locationFromBrowser \} from "\.\/location\.js";/, () => location); + // The file ends by starting itself. The harness starts it explicitly so a + // caller can await it, and must not run it twice -- so the auto-start goes, + // and if it can't be found, fail loudly rather than silently double-start. + const autoStart = /\r?\nstart\(\);\r?\nupdateSavedHeaderIndicator\(\);\s*$/; + if (!autoStart.test(script)) throw new Error("newtab-harness: could not find newtab.js's trailing start() call -- update the harness."); + return script.replace(autoStart, "\n"); +} + +/** + * Opens a new-tab page. `store` is the chrome.storage.local contents (mutated + * in place by the page); `requests` records every fetch the page makes. + * `await tab.start()` runs one full load of the page. + */ +export function openNewtab({ backendUrl, store }) { + const dom = new JSDOM(readExtensionFile("newtab.html"), { runScripts: "dangerously" }); + const { window } = dom; + const requests = []; + + window.chrome = { + storage: { + local: { + get: async (keys) => Object.fromEntries((Array.isArray(keys) ? keys : [keys]).map((key) => [key, structuredClone(store[key])])), + set: async (value) => { Object.assign(store, structuredClone(value)); } + } + }, + runtime: { openOptionsPage() {}, getURL: (file) => file }, + tabs: { query: (_query, callback) => callback([{ id: 1 }]), update() {} } + }; + // Node's fetch rejects jsdom's AbortSignal, so requests go out without one. + window.fetch = async (url, options = {}) => { + requests.push({ url, path: new URL(url).pathname, body: options.body ? JSON.parse(options.body) : null }); + return nativeFetch(url, { ...options, signal: undefined }); + }; + + const scriptEl = window.document.createElement("script"); + scriptEl.textContent = buildScript(backendUrl); + window.document.body.appendChild(scriptEl); + + const document = window.document; + return { + window, + requests, + start: () => window.start(), + requestsTo: (pathname) => requests.filter((request) => request.path === pathname), + shownName: () => document.querySelector("#card h1")?.textContent ?? null, + isCardHidden: () => document.getElementById("card").hidden, + noticeText: () => document.getElementById("notice").textContent + }; +} diff --git a/test/deploy-coverage.test.js b/test/deploy-coverage.test.js new file mode 100644 index 0000000..7078878 --- /dev/null +++ b/test/deploy-coverage.test.js @@ -0,0 +1,75 @@ +// Guards against the release safety net quietly falling behind the code. +// Issue #79 added /api/validate-cats, and the extension fails open when it +// breaks (stale cards just keep getting served) -- so nothing user-visible +// would have flagged it dying in production, and deploy-verify.yml's smoke +// test didn't know the route existed. These checks make that class of gap a +// red build instead of something a reviewer has to remember: every server +// route needs a production smoke check and a mention in the docs that +// describe it, and every live-API test file has to actually be wired into +// `npm run test:live` (CI never runs those, so an orphaned one would rot). +import { describe, it } from "node:test"; +import assert from "node:assert"; +import fs from "node:fs"; +import path from "node:path"; +import { spawnSync } from "node:child_process"; +import { fileURLToPath } from "node:url"; + +const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); +const read = (relativePath) => fs.readFileSync(path.join(ROOT, relativePath), "utf8"); + +// Every route literal in server/index.js: "/healthz" and "/api/". +const serverRoutes = [...new Set([...read("server/index.js").matchAll(/"(\/(?:healthz|api\/[a-z][a-z-]*))"/g)].map((match) => match[1]))].sort(); + +describe("release safety net covers every server route", () => { + it("finds the routes this test is meant to guard (so a regex or refactor can't make it vacuous)", () => { + for (const route of ["/healthz", "/api/nearby-cats", "/api/validate-cats", "/api/photo-thumb", "/api/photo-share"]) { + assert.ok(serverRoutes.includes(route), `expected server/index.js to define ${route}; found ${serverRoutes.join(", ")}`); + } + }); + + it("smoke-tests every route against production in deploy-verify.yml", () => { + const workflow = read(".github/workflows/deploy-verify.yml"); + const smokeStep = workflow.slice(workflow.indexOf("Smoke test the live API")); + assert.ok(smokeStep.length < workflow.length, "deploy-verify.yml should have a 'Smoke test the live API' step"); + for (const route of serverRoutes) { + assert.ok(smokeStep.includes(route), `deploy-verify.yml's smoke test never calls ${route} -- add a check for it (or a production regression there would go unnoticed)`); + } + }); + + for (const doc of ["RELEASE.md", "README.md"]) { + it(`is described in ${doc}`, () => { + const text = read(doc); + for (const route of serverRoutes) { + assert.ok(text.includes(route), `${doc} never mentions ${route} -- update it (AGENTS.md's "Documentation & Asset Currency")`); + } + }); + } +}); + +describe("live-API tests can't be orphaned", () => { + const liveFiles = fs.readdirSync(path.join(ROOT, "test-live")).filter((name) => name.endsWith(".js")); + const script = JSON.parse(read("package.json")).scripts["test:live"]; + + it("finds live test files to check", () => { + assert.ok(liveFiles.length >= 2, `expected the live test files, found: ${liveFiles.join(", ")}`); + }); + + for (const file of liveFiles) { + it(`npm run test:live runs test-live/${file}`, () => { + assert.ok(script.includes(`test-live/${file}`), `package.json's test:live script doesn't list test-live/${file}, so it would never run`); + }); + + // CI never executes these, so an import error or crash on load would only + // surface on a maintainer's machine mid-release. Without a key each one + // must exit 0 with a skip message. + it(`test-live/${file} loads cleanly and skips (exit 0) without an RG_API_KEY`, () => { + const result = spawnSync(process.execPath, [path.join(ROOT, "test-live", file)], { + env: { ...process.env, RG_API_KEY: "" }, + encoding: "utf8", + timeout: 30_000 + }); + assert.strictEqual(result.status, 0, `exit ${result.status}: ${result.stderr}`); + assert.match(result.stdout, /Skipping/); + }); + } +}); diff --git a/test/revalidation-integration.test.js b/test/revalidation-integration.test.js new file mode 100644 index 0000000..e556a7b --- /dev/null +++ b/test/revalidation-integration.test.js @@ -0,0 +1,185 @@ +// Issue #79, client <-> server contract. test/newtab.test.js and +// test/server.test.js each test their own half against a mock of the other, +// which can't catch the two halves drifting apart (a renamed route, a changed +// response field, a different id format). This runs the real newtab.js in +// jsdom against the real server/index.js over a real localhost socket, with +// only RescueGroups itself faked -- so it covers the whole path: page -> +// /api/validate-cats -> RescueGroups by-id query -> pruned cache -> rendered +// card. test-live/stale-cache.live.js is the same flow against the real API. +import { describe, it, beforeEach, afterEach, mock } from "node:test"; +import assert from "node:assert"; +import { cache, server, resetAlertStateForTests, resetRateLimitsForTests } from "../server/index.js"; +import { openNewtab } from "../test-support/newtab-harness.js"; + +const DAY = 24 * 60 * 60 * 1000; +const RG_ORIGIN = "https://api.rescuegroups.org/"; +const nativeFetch = globalThis.fetch; // captured before any test mocks it + +// A stand-in for RescueGroups: `available` is the set of ids it currently +// lists, and every request it receives is logged so tests can assert on what +// the server actually sent upstream. +function makeFakeRescueGroups() { + const rg = { available: [], requests: [], byIdFailureStatus: null }; + + const animal = (id, distance) => ({ + id, + type: "animals", + attributes: { name: `Cat ${id}`, distance, ageString: "Adult", sex: "Female", breedString: "Domestic Short Hair", updatedDate: new Date().toISOString() }, + relationships: { pictures: { data: [{ type: "pictures", id: `pic-${id}` }] }, orgs: { data: [{ type: "orgs", id: "org-1" }] } } + }); + const included = (animals) => [ + { type: "orgs", id: "org-1", attributes: { name: "Test Rescue", url: "https://rescue.example.org" } }, + ...animals.map(({ id }) => ({ type: "pictures", id: `pic-${id}`, attributes: { large: { url: `https://cdn.rescuegroups.org/pic/${id}.jpg` }, original: { url: `https://cdn.rescuegroups.org/pic/${id}-orig.jpg` }, order: 1 } })) + ]; + const respond = (payload) => ({ ok: true, status: 200, statusText: "OK", json: async () => payload }); + + rg.handle = async (url, options) => { + const body = JSON.parse(options.body); + const idFilter = body.data.filters?.find((filter) => filter.fieldName === "animals.id"); + if (idFilter) { + rg.requests.push({ kind: "byId", ids: idFilter.criteria, operation: idFilter.operation }); + if (rg.byIdFailureStatus) return { ok: false, status: rg.byIdFailureStatus, statusText: "Upstream failure", json: async () => ({ errors: [{ detail: "upstream is down" }] }) }; + const wanted = new Set(idFilter.criteria); + return respond({ data: rg.available.filter((id) => wanted.has(id)).map((id) => animal(id, 1)) }); + } + const page = Number(new URL(url).searchParams.get("page")); + rg.requests.push({ kind: "radius", page, miles: body.data.filterRadius.miles }); + const animals = page === 1 ? rg.available.map((id, i) => animal(id, i + 1)) : []; + return respond({ data: animals, included: included(animals) }); + }; + rg.requestsOfKind = (kind) => rg.requests.filter((request) => request.kind === kind); + return rg; +} + +describe("stale cache revalidation, real extension code against the real server (issue #79)", () => { + let backendUrl; + let store; + let rg; + + beforeEach(async () => { + process.env.RG_API_KEY = "test-key"; + cache.clear(); + resetRateLimitsForTests(); + resetAlertStateForTests(); + delete process.env.ALERT_WEBHOOK_URL; + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + backendUrl = `http://127.0.0.1:${server.address().port}`; + store = { settings: { postalcode: "10001", location: null } }; + rg = makeFakeRescueGroups(); + mock.method(globalThis, "fetch", (url, options) => (String(url).startsWith(RG_ORIGIN) ? rg.handle(url, options) : nativeFetch(url, options))); + }); + + afterEach(async () => { + await new Promise((resolve) => server.close(resolve)); + mock.restoreAll(); + }); + + const idRange = (from, count) => Array.from({ length: count }, (_, i) => String(from + i)); + const openTab = async () => { + const tab = openNewtab({ backendUrl, store }); + await tab.start(); + return tab; + }; + // Simulates `days` passing between tab loads: the stored cache ages, and + // the server's short-lived response cache (3 minutes) has long expired. + const timePasses = (days) => { + const at = Date.now() - days * DAY; + store.feedCache = { ...store.feedCache, fetchedAt: at, validatedAt: at }; + cache.clear(); + }; + const seedPool = async (ids) => { + rg.available = ids; + await openTab(); + assert.deepStrictEqual(store.feedCache.cards.map((card) => card.id), ids, "the first load builds the pool through the real refresh path"); + rg.requests.length = 0; + }; + + it("drops adopted cats, keeps every cat still listed, and never advances the page", async () => { + const ids = idRange(1001, 12); + await seedPool(ids); + const adopted = [ids[1], ids[4], ids[9]]; + store.feedCache.seenIds = [ids[0], ids[1], ids[2]]; // ids[1] is seen *and* about to be adopted + timePasses(8); + const staleFetchedAt = store.feedCache.fetchedAt; + rg.available = ids.filter((id) => !adopted.includes(id)); + + const tab = await openTab(); + + const byId = rg.requestsOfKind("byId"); + assert.strictEqual(byId.length, 1, "one RescueGroups by-id request for the whole pool"); + assert.deepStrictEqual(byId[0].ids, ids, "every cached id was asked about"); + assert.strictEqual(byId[0].operation, "equal"); + assert.strictEqual(rg.requestsOfKind("radius").length, 0, "no search, so no radius escalation and no new page"); + assert.strictEqual(tab.requestsTo("/api/validate-cats").length, 1); + assert.strictEqual(tab.requestsTo("/api/nearby-cats").length, 0); + + assert.deepStrictEqual(store.feedCache.cards.map((card) => card.id), rg.available, "survivors kept, in their original order"); + assert.strictEqual(store.feedCache.page, 1); + assert.strictEqual(store.feedCache.fetchedAt, staleFetchedAt); + assert.ok(Date.now() - store.feedCache.validatedAt < 60_000, "validatedAt was stamped"); + assert.ok(!store.feedCache.seenIds.includes(ids[1]), "the adopted-but-seen id is gone from seenIds too"); + assert.ok(store.feedCache.seenIds.includes(ids[0]) && store.feedCache.seenIds.includes(ids[2])); + assert.ok(rg.available.some((id) => tab.shownName() === `Cat ${id}`), `shown card ${tab.shownName()} should be a survivor`); + }); + + it("does nothing at all for a cache that was checked recently", async () => { + await seedPool(idRange(1001, 12)); + timePasses(6); + + const tab = await openTab(); + + assert.strictEqual(rg.requests.length, 0); + assert.strictEqual(tab.requests.length, 0); + assert.ok(tab.shownName()); + }); + + it("serves the cache and backs off when RescueGroups is failing, instead of surfacing an error", async () => { + const ids = idRange(1001, 12); + await seedPool(ids); + timePasses(8); + rg.byIdFailureStatus = 500; + const errorSpy = mock.method(console, "error", () => {}); // both the server and the page log the failure + + const tab = await openTab(); + + assert.strictEqual(tab.requestsTo("/api/validate-cats").length, 1); + assert.strictEqual(rg.requestsOfKind("byId").length, 1); + assert.ok(errorSpy.mock.callCount() > 0, "the failure is logged"); + assert.deepStrictEqual(store.feedCache.cards.map((card) => card.id), ids, "nothing was dropped on a failed check"); + assert.ok(tab.shownName(), "the user still gets a cat"); + assert.strictEqual(tab.noticeText(), "", "a background check failing is not the user's problem"); + assert.ok(store.feedCache.validationRetryAfter > Date.now()); + + const nextTab = await openTab(); + assert.strictEqual(nextTab.requestsTo("/api/validate-cats").length, 0, "backs off for the cooldown instead of retrying on every tab"); + assert.strictEqual(rg.requestsOfKind("byId").length, 1); + }); + + it("restarts from page 1, not page + 1, when every cached cat has been adopted", async () => { + await seedPool(idRange(1001, 12)); + store.feedCache.page = 3; // a user well into the radius ladder + timePasses(8); + rg.available = idRange(2001, 5); // none of the old cats are listed; a new batch is + + const tab = await openTab(); + + assert.strictEqual(rg.requestsOfKind("byId").length, 1); + const searches = rg.requestsOfKind("radius"); + assert.ok(searches.length > 0); + assert.ok(searches.every((search) => search.page === 1), `a dead pool restarts pagination, saw pages ${searches.map((s) => s.page)}`); + assert.deepStrictEqual(store.feedCache.cards.map((card) => card.id), idRange(2001, 5), "nothing from the dead pool was kept"); + assert.strictEqual(store.feedCache.page, 1); + assert.ok(/^Cat 200\d$/.test(tab.shownName()), `only a fresh cat is shown, got ${tab.shownName()}`); + }); + + it("asks about a pool larger than 100 cats in two requests through the real server", async () => { + const ids = idRange(1001, 150); + store.feedCache = { cards: ids.map((id) => ({ id, name: `Cat ${id}`, imageUrl: `https://cdn.rescuegroups.org/pic/${id}.jpg` })), fetchedAt: Date.now() - 8 * DAY, location: { postalcode: "10001" }, page: 1, radiusMiles: 25, seenIds: [] }; + rg.available = ids.filter((id) => Number(id) % 2 === 0); + + await openTab(); + + assert.deepStrictEqual(rg.requestsOfKind("byId").map((request) => request.ids.length), [100, 50]); + assert.deepStrictEqual(store.feedCache.cards.map((card) => card.id), rg.available); + }); +});