Skip to content

fix: revalidate a stale cache and skip broken photos (#79) - #81

Open
BrandonML wants to merge 3 commits into
mainfrom
fix/stale-cache-revalidation
Open

BrandonML wants to merge 3 commits into
mainfrom
fix/stale-cache-revalidation

Conversation

@BrandonML

@BrandonML BrandonML commented Sep 29, 2026 •

Copy link
Copy Markdown
Owner

Fixes #79.

Problem

A user's cache was ~5 weeks old, so some cached listings had been adopted or removed. Their cards still rendered, with broken photos (the image URL 404s). There was no age ceiling: the cache refreshes only once it is 5+ minutes old and 85% seen, and a light user can take weeks to get there. Refresh also never rechecks the unseen cards it keeps (it fetches page + 1, a different window), and it resets fetchedAt, so a cache can look fresh while holding old cards.

The old time-based cutoff (STALE_MS) was removed in b829eed because it advanced page and walked users outward through the server's radius ladder. So a plain "refresh after N days" is not the fix; it would reintroduce that.

Approach

Three commits on a stack (one merge to main):

1. fix(newtab): skip to another cat when a photo fails to load (fb0a1ef)

Covers the issue's note about broken images. On a photo error the card moves on to another unseen card (normal mode from feedCache, explore mode from the in-memory batch), up to 3 in a row, then falls back to the existing "no longer available" notice.

  • The failed card is already marked seen when picked, so the only write is marking the replacement seen. Cards are never evicted, so a transient CDN error can't shrink the pool.
  • Never recycles seen cards, does nothing while offline (issue Add Logic to Handle No Internet #69's notice stays), waits for an in-flight refresh before touching feedCache, and stands down if that photo's card was already replaced.
  • nextCard() now delegates to a new nextUnseenCard(); behavior unchanged.

2. fix(cache): revalidate a week-old cache against RescueGroups (88a6601)

The main fix. Once a pool's validatedAt (falling back to fetchedAt) is 7+ days old, the next new tab asks the backend which cached ids are still available and drops the rest.

Server

  • New POST /api/validate-cats { ids } -> { availableIds }. One RescueGroups request per call: animals.id equal with an array criteria on the existing available/cats/haspic endpoint. I confirmed against the live API that this acts as an "in" filter and silently omits unknown ids (99 of 100 real ids returned, the fake one dropped).
  • Ids validated as 1-100 numeric. Uncached. Shares /api/nearby-cats's per-IP rate limit, error mapping and upstream-failure alerting. Purely additive.
  • searchRadius and the new findAvailableIds share one request/error helper.

Extension

  • Dead cards and their seenIds are dropped. Every still-available unseen card is kept, in order. page, fetchedAt, location and radius are untouched; validatedAt is stamped.
  • Chunks of 100 ids, 2s overall timeout. Only ids that were actually checked can be dropped, so a card another tab added meanwhile survives.
  • refresh() carries validatedAt forward when it keeps unseen cards (its fetchedAt reset would otherwise hide their age).
  • If the whole pool is dead: refresh from page 1 with nothing kept. Nothing is written unless that fetch succeeds.
  • Fails open: any error serves the cache exactly as before and sets a 1-hour retry cooldown so a down server can't delay every tab. Offline skips validation. A cache with no usable timestamp is left alone.

Docs: README behavior and rate-limit notes. PRIVACY.md said "nothing else is sent"; it now discloses that cached public listing ids are sent to the backend (and on to RescueGroups) at most about weekly, with no location attached, and the "Last updated" date is bumped. WEBSTORE.md and the FAQ stay accurate as written.

3. test(release): close the release safety-net gaps around /api/validate-cats (dae6822)

The new route fails open in the extension, so a broken route in production would show up as nothing at all: stale cards would just keep being served. Yet no pre-merge, CI or post-deploy check covered it. This closes that, and makes the same class of gap fail the build in future.

  • deploy-verify.yml: the production smoke test now calls /api/validate-cats with ids /api/nearby-cats just listed plus one that cannot exist. The impossible id must never come back and at least one real one must. It also checks invalid input gets 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 (jsdom) against the real server over a real socket, with only RescueGroups faked. The client and server tests each mock the other side, so this is the only thing that checks the two agree (route, response field, id format, filter operation). Mutations of each were caught.
  • test-live/stale-cache.live.js: the same flow against the real RescueGroups API, with the server started in-process (nothing to run first). This is the end-to-end script I used to verify the fix, promoted to a repeatable test. npm run test:live now runs every live file. The live tests also switched their "impossible" id to 12 digits, since an 8-digit one could plausibly be assigned someday.
  • test/deploy-coverage.test.js: fails the build if a server route has no smoke check in deploy-verify.yml or no mention in README/RELEASE.md, if a test-live/ file isn't wired into test:live, or if one fails to load without a key.
  • test-support/newtab-harness.js: the shared page harness. ESLint and .dockerignore cover the new directory.
  • 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 the new smoke check. README: documents the above, plus a "Manual QA: simulating an old cache" recipe, and corrects the CI section (ci.yml has always run lint too).

Options considered

  • Age limit then full cache replace: loses unseen active cats and seen history. Rejected.
  • Age limit then the existing refresh(): never validates kept cards and walks the radius outward. Rejected.
  • Refetch the same page and diff by distance coverage: approximate, can wrongly drop active cats, and can't verify cards outside the window. Rejected.
  • Reusing /api/nearby-cats with an ids field: saves a route but adds branching to the hottest handler. Rejected in favor of a separate route.

Risk and compatibility

  • Server change is additive. New extension against an old server gets a 404 and fails open; old extension against the new server is unaffected. The deploy pipeline already ships the server first.
  • New stored field validatedAt (plus a transient validationRetryAfter) is additive; an older extension ignores both.
  • No manifest or permission changes, and no version bump.
  • The main hot-path change is one awaited step in _start, gated to once a week, capped at 2s, wrapped in try/catch.

Verification Report

Changes Made

  • Photo-error fallback in extension/newtab.js.
  • POST /api/validate-cats (server/index.js, server/rescuegroups.js) and client revalidation (extension/newtab.js).
  • README and PRIVACY.md updates; unit and live tests.

Tests Executed

  • npm run lint: clean.
  • npm test (full suite, Tier 3): 339 / 339 pass (268 on main), two consecutive runs.
    • New: 18 server and rescuegroups tests, about 31 client revalidation tests, 8 new photo-fallback tests, 5 client-to-server integration tests, 9 release-coverage guard tests.
    • Updated: the two photo load failure (issue #69) tests (the handler is now async) and one start test's title and comment.
  • npm run test:live against the real RescueGroups API: 7 / 7 pass (4 existing/by-id contract tests, 3 end-to-end stale-cache tests).
    • The end-to-end tests show an 8-day-old pool with 5 dead ids dropping exactly those and keeping all 97 live cats, with 0 search requests and page unchanged; a 35-day-old all-dead pool refreshing from page 1; and a 2-day-old pool making no requests.
  • Mutation checks: 7 deliberate breakages of the client logic, 4 of the client/server contract, and 2 of the release-coverage guard were each caught by the tests.
  • The new deploy-verify.yml assertion snippet, run locally for the pass case and both failure modes. The workflow itself only runs on a push to main that touches server/**, so its first real run is post-merge, and that run is the real check of this step. (A red result there means the check or the deploy is broken, and per RELEASE.md, do not proceed to store submission.)
  • Manual QA in Chrome (load unpacked): not yet done, to be run by the author before merge.

Existing Functionality Confirmed Intact

  • All pre-existing tests pass unchanged apart from the edits listed above (refresh merge/dedupe, pagination reset, seen-ratio thresholds, explore mode, saved cats, share flows, server cache/rate-limit/CORS/alerting).

Before release

  • Full manual QA checklist in AGENTS.md (load unpacked, cards render, settings flow, no console errors), including simulating an old cache by setting fetchedAt/validatedAt back 8 days in devtools and testing with the server stopped.
  • If store listings link to a separately hosted copy of the privacy policy, update it to match PRIVACY.md.

🤖 Generated with Claude Code

BrandonML and others added 3 commits September 29, 2026 12:40
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 <noreply@anthropic.com>
…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 <noreply@anthropic.com>
…-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 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Safety/logic to handle old/stale cache to prevent listings that are no longer active from showing

1 participant