Conversation
Array.includes() inside a .filter() is O(N*M); a Set makes each membership check O(1). Bounded today by the 100-card cap, but free and zero-risk. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Pure readability change, no behavior difference — normalizeCards() already handles relationships, timestamps, fee normalization, and URL handling, so pulling picture selection into its own function keeps the main per-animal loop easier to follow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
newtab.js already defines const \$ = (id) => document.getElementById(id) and uses it throughout; options.js had 5 separate getElementById calls instead. Purely additive, no behavior change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
showNotice() already supported an extensible {linkText, linkAction}
pattern (used for "open-settings"); adds a "report-issue" action that
opens the GitHub issues page. Wired into the non-ZIP branch of
_start()'s refresh-failure handler only — a ZIP-validation failure
already carries enough detail to know it's a user issue, so it keeps
the existing settings-shortcut link instead (per GH issue #26).
Also exports isInvalidZipError() from error-messages.js so the caller
can make that branch decision without re-deriving classifyRefreshError's
internal classification.
Test harness fix: newtab.test.js/options.test.js inline error-messages.js
into a plain <script> tag via string replacement of "export function" —
now that the file has two exports, switched both to a global regex so
the second export doesn't survive as invalid syntax inside the inlined
script.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Both were pure, well-understood algorithms (CRC32, DOS date/time bit-packing) inside the release pipeline's hand-rolled zip writer, with no direct test coverage. Exported crc32/dosDateTime (previously module-private) so they can be tested directly against known vectors, rather than reverse-parsing byte offsets out of createZipBuffer's output to get the same coverage — purely additive, no existing caller affected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Only exercised indirectly via options.js/newtab.js flows before. Uses the same navigator.geolocation stubbing approach as options.test.js's "Use my location" tests, applied directly to the function itself. Node's own `navigator` global is a non-configurable-looking getter in recent Node versions, so a plain `global.navigator = ...` assignment throws — used Object.defineProperty to replace/restore it instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Confirmed empirically: request.destroy(error) on an IncomingMessage
destroys the underlying socket whenever the body is still incomplete
(readableEnded/complete false) at that moment -- which is exactly the
case on a genuine request timeout. The old timeout handler called
request.destroy(new Error("Request timeout")) from an async timer
callback, which killed the connection before the handler's own catch
block could ever write the intended 408 response back -- the client
just saw a bare connection reset with zero bytes, never an actual 408.
Replaced the for-await/destroy-based body reader with an event-based
one (readRequestBody) that never calls destroy() on the request itself;
an early bail-out (timeout, oversized payload) just stops listening.
The now-abandoned body can't safely be reused for a keep-alive
connection, so 408/413 responses now also send "Connection: close" so
Node closes the socket cleanly after flushing that response, instead
of leaving a half-read connection open.
The 5000ms timeout is now configurable via REQUEST_BODY_TIMEOUT_MS
(read live per-request, same pattern as ALERT_WEBHOOK_URL) so tests
don't have to wait out the real default.
Added a raw-socket test that deliberately never finishes sending the
body and asserts a real 408 arrives with "Connection: close" -- this
path had zero coverage before and, per the above, didn't actually work
as documented. Also asserts the existing 413 path now closes the
connection too.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The existing "closes settings when close button clicked" test never supplied a tab with an id from chrome.tabs.query, so it only exercised the window.close() fallback branch -- the primary chrome.tabs.create/ chrome.tabs.remove path (what runs on almost every real close) had no dedicated assertions on the URL or tab id involved. Note: this path wasn't entirely uncovered before -- "auto-navigates back a brief moment after a successful save" already invoked it indirectly via the post-save timeout callback, but only checked booleans, not the actual arguments passed to chrome.tabs.create/remove. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The response cache only helps repeat requests for the same location/ page; a script varying postal codes or coordinates costs a real RescueGroups API call per request, unbounded. Adds an in-memory fixed-window counter (30 requests / 5 minutes per IP), architecturally consistent with the existing response cache -- same single-process deployment assumption, same Map-based bounding against unbounded growth. Threshold (30/5min) was a product judgment call, confirmed with Brandon rather than picked silently. Client IP is read from X-Forwarded-For when present, falling back to the raw socket address for local dev -- Northflank (and similar platforms) terminates the real connection and forwards, so the socket address alone would collapse every real visitor into one shared bucket. Documented the platform assumption in the README. A rate-limited (or otherwise aborted) request's body is never read, so -- same reasoning as the 408/413 fix in bodyOf() -- the connection gets "Connection: close" rather than being left keep-alive in a half-read state. Verified end-to-end against a real running server (30 requests OK, 31st+ return 429 with Retry-After and Connection: close) in addition to the new unit tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Portrait photos were classified purely by aspect ratio into two tiers, both getting a fixed object-position: top -- which assumes the subject is near the top of the frame. Issue #24 documents a real failure (a 500x1071 photo, cat dead-center) where that assumption cuts the cat out of frame entirely. Client side (extension/newtab.js): on load, a portrait-tier photo is analyzed via a row-wise edge/contrast-energy profile -- real subjects (fur, faces) tend to carry more local contrast than a plain floor or wall. A sliding band (~35% of the photo's height) finds the highest- energy region, which becomes the object-position anchor in place of the hardcoded "top". A confidence check (winning band's energy density vs. the photo's own average) guards against acting on a flat or noisy image with no real localized signal -- below threshold, or on any failure (network, decode, timeout), the CSS default is left untouched, so the worst case is exactly today's behavior. Threshold (1.1) was calibrated against real photos pulled live from the API, including issue #24's own cited example (scored 1.12) -- every other sampled photo scored 1.19+, so this catches the hard case with a small margin while still requiring a non-trivial signal. Server side (server/index.js): the heuristic needs to read pixel data, but RescueGroups' CDN sends no CORS headers on its images (confirmed by inspecting the response directly) -- a canvas drawn from one via <img> is unconditionally tainted, getImageData() throws, no client- side workaround exists. Added GET /api/photo-thumb, a narrow proxy that fetches a small (~100px-wide) analysis-only thumbnail from cdn.rescuegroups.org specifically (hostname-locked, width forced server-side regardless of what's requested, so it can't become a general-purpose open proxy) and re-serves it with this server's own CORS headers. The real, full-size photo shown to the user still loads directly from RescueGroups' CDN, unchanged -- only this small analysis fetch is proxied. Verified end-to-end against the real running server and issue #24's own cited photo URL (see test/photo-thumb.test.js and the content-aware crop describe block in test/newtab.test.js for the unit-level coverage of both). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
.shell caps at 620px regardless of viewport, so the previous 1280x800 screenshots showed the real card centered with ~330px of empty margin on each side -- close to half the image blank, making the actual product imagery smaller and less noticeable in the store listing. Added screenshot-frame.source.html / screenshot-frame-options.source.html, following the same pattern as promo-marquee.source.html / promo-small-tile.source.html: standalone capture harnesses that load the real newtab.css/newtab.js/options.js unmodified and seed chrome.storage.local with canned sample data, then scale the rendered card up via CSS transform (never touching the real responsive layout). The card's own aspect ratio is close to square, so it can't be scaled to fill 80%+ of a 1280x800 canvas' width while every line of on-screen text also fits inside its height -- those two goals are in real tension on a card this shape, not just a tuning choice. Screenshots 1 and 5 trade a bit of width (~63-75%) to keep their relevant text (name; status chips + fee) fully visible rather than truncated; screenshots 2-4 reach ~75-90% width with nothing cropped at all. All still a large improvement over the original ~48%. See the harness file's own header comment for the exact per-screenshot capture URLs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…otice
The "no results" notice's link rendered as a separate flex item with a
fixed gap from the message text (`.notice { display:flex; gap:6px }`
treated the trailing text node and the button as sibling row items),
so it looked like a disconnected chip rather than part of the sentence.
The message text also duplicated "zip code" once as plain text and
once as the link label.
Wrap the message and any links in one `.notice-body` span so they flow
as normal wrapped text, and let a message with multiple links (a
"{token}" placeholder per link) splice each button in at its exact
spot instead of only ever appending after the message. The two
"no cats found" messages now also offer an "explore another city"
link alongside "zip code", reusing the existing explore flow via a new
shared startExplore() helper instead of duplicating it.
Fixes #29.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds a "Share {name}" button to the card that calls navigator.share()
with the cat's details, a Tabby tagline + Chrome Web Store link, and
its RescueGroups profile URL. The photo is attached as a real file
when the platform supports it (confirmed live on Chrome desktop from
inside the actual newtab.html origin: canShare({ files }) === true).
RescueGroups' CDN has no CORS headers, so the photo can't be fetched
client-side any more than it could be drawn to canvas (see the
content-aware-crop comment in server/index.js) -- a second
hostname-locked proxy, GET /api/photo-share, fetches it server-side at
a real, presentable 640px width (vs. the existing /api/photo-thumb's
100px analysis-only size, too small to actually look like a photo in
a share sheet) and streams it back with CORS headers. Both proxies now
share one buildPhotoProxyUrl/sendPhotoProxy implementation.
Degrades in two steps when the ideal path isn't available: link+text
share without the photo if canShare rejects it or the proxy fetch
fails, then a clipboard-copy fallback if navigator.share itself fails
or isn't present at all (the button doesn't render in that last case).
Closes #27.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds three GitHub Actions workflows to close the gaps found in a pre-release audit: no CI gate before merge, no git tags for any shipped version, and no automated confirmation that a Northflank deploy actually landed before submitting to CWS/EWS. - ci.yml: npm test on push/PR to main and dev, pinned to Node 22 to match the Dockerfile. - tag-release.yml: tags main `v<version>` (from manifest.json) on every push, idempotent. - deploy-verify.yml: on a push to main touching server/**/Dockerfile, polls the live /healthz until it reports the pushed commit's sha, then smoke-tests /api/nearby-cats, /api/photo-thumb, and /api/photo-share against production. /healthz now reports Northflank's auto-injected NF_DEPLOYMENT_SHA runtime env var (confirmed present on the live service) so deploy-verify.yml can tell "the new code is live" from "a process is answering," rather than relying on a version bump that might not happen for every server change. Also, from the same audit: - Dockerfile: copies package.json into the image so ESM import/export in server/*.js resolves via the standard package.json "type" field instead of relying on Node 22's module-syntax auto-detection, which only works because no package.json was present at all -- a base image downgrade would have silently broken this. - README: corrected a stale claim that NODE_ENV would select between a "production" and "dev/sandbox" RescueGroups endpoint -- no such distinction exists in server/rescuegroups.js, there's only one BASE_URL used everywhere. Documented the new workflows and the /healthz sha field. - extension/newtab.js: the "Share this cat" promo link now checks for Edge's user agent and can point at a separate Edge Add-ons URL instead of always linking to the Chrome Web Store (which Edge blocks one-click installs from by default). Currently still falls back to the CWS link either way -- the Edge listing isn't public yet, so there's no real URL to point at (see the TODO in the code). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…book The Edge Add-ons listing (https://microsoftedge.microsoft.com/addons/detail/fieeoalehgckgnkohkdblljmgaemaiho) is live -- confirmed by fetching it -- so the Share feature's Edge-vs-Chrome link now actually differs instead of always falling back to the Chrome Web Store URL. RELEASE.md is the versioned source of truth for the dev-to-main release steps (kept in the repo so it stays in sync with the actual scripts/workflows via normal PRs, rather than drifting in a separately-hosted doc); README links to it from the CI/CD section. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Closed
This was
linked to
issues
Sep 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Consolidates this development cycle's work (13 items from the original task brief plus 2 follow-up GitHub issues and a pre-release process audit) into
main. Full diff: 29 files, +1819/-88.User-facing:
/api/photo-thumbproxy/api/photo-shareproxy, and an Edge-vs-Chrome-aware store linkServer hardening:
/api/nearby-cats(30 req / 5 min per IP)/healthznow reports the live deployment's git SHA (NF_DEPLOYMENT_SHA), enabling automated post-deploy verificationCode quality / tests:
getSeenIdsO(N×M) fix,getBestPicture/DOM-lookup helper extractions, new test coverage forscripts/zip.js,locationFromBrowser,showAnotherExploreCard, the timeout path, the options "close" path, andexploreArea.Release process (this cycle's own audit):
Dockerfilenow copiespackage.jsonso ESM resolution doesn't depend on Node-version-specific auto-detectionci.yml(test gate),tag-release.yml(auto-tagsmainon push),deploy-verify.yml(confirms the live deploy matches the pushed commit + smoke-tests the API before store submission)RELEASE.mdrunbook addedTest plan
npm test— 202/202 passing on this branch's HEADnpm run test:live— 3/3 passing against the real RescueGroups API/api/photo-thumband/api/photo-shareagainst the real CDNscripts/release.jsin an isolated copy — version sync and zip contents verifiedci.yml'stestcheck must pass here before this can merge (required by themainruleset)deploy-verify.ymlwill run automatically after merge — must go green before submitting to CWS/EWS🤖 Generated with Claude Code