🐛 Prevent stale TDD image loads

[Robdel12]

Why this change

In TDD mode, baseline/current/diff images are overwritten in place under stable /images/* URLs.

That means the reporter can receive fresh comparison data while the browser still shows stale cached image bytes. The result feels like an SSE issue, but the primary problem is image caching.

What changed

• Added a reporter utility that appends a version query param to local /images/* URLs.
• Added a report-data normalizer so comparisons always have a timestamp fallback for cache-busting (comparison.timestamp ?? reportData.timestamp).
• Wired cache-busted URLs into comparison image render paths (viewer, fullscreen queue thumbnails, screenshot list, screenshot display).
• Added no-cache response headers for image responses in the assets router.
• Added focused unit tests for URL rewriting behavior and report-data timestamp normalization.

Test plan

• node --test tests/server/routers/assets.test.js tests/reporter/utils/image-url.test.js tests/reporter/utils/report-data.test.js
• npm run build:reporter

[claude]

PR Review: 🐛 Prevent stale TDD image loads

Overall this is a clean, well-scoped fix for a real and annoying caching issue. The approach is sound and the test coverage is good. A few observations below.

---

✅ What works well

• Defense-in-depth: Using both no-store HTTP headers and a ?v= query param is the right approach. Either alone could fail (e.g. a proxy might strip headers, or a service worker might intercept), so both together makes this robust.
• Narrow scope: withImageVersion only rewrites /images/* paths, leaving cloud/CDN URLs untouched. Good guard.
• Test coverage: The new tests cover the key cases well—null/undefined URL, non-local URLs, missing version, existing query params, and URL encoding.
• Cache headers: no-store, no-cache, must-revalidate + Pragma: no-cache + Expires: 0 is the canonical belt-and-suspenders combination for HTTP 1.0/1.1 compatibility.

---

🔍 Issues & Questions

1. Double-rewrites on re-renders (minor)

In comparison-viewer.jsx and screenshot-display.jsx, imageUrls is recomputed on every render (no useMemo). The comments say "no memoization needed, object creation is cheap"—which is fine for the object itself, but withImageVersion now also does string work (URL parsing with .includes('?'), encodeURIComponent, string concatenation). Still cheap, but the original rationale is slightly weakened. Not a bug, just worth noting if this component becomes a hotspot.

2. comparison.timestamp is the version signal—is it always present?

withImageVersion gracefully returns the bare URL when version is null/undefined, which means a missing timestamp silently falls back to no cache-busting. Is timestamp guaranteed to be set on every comparison object returned by the TDD server? If it can ever be absent (e.g. for very first baseline writes or legacy data), the cache bug could silently resurface. Worth either:

• Asserting/logging a warning when timestamp is missing on a local image URL, or
• Documenting that timestamp is required on comparison objects.

3. String(version) for numeric 0

if (version === null || version === undefined) {
  return url;
}

This guard correctly passes through 0 (falsy but valid), which is good. Just confirming intentional—v=0 is a valid cache-buster if timestamp starts from epoch 0.

4. withImageVersion not tested with 0 as version

Following from above: 0 as a version value would pass the guard and produce ?v=0. The test suite covers null, undefined, numeric, and string versions but not 0. Low priority, but a one-liner assertion would close the gap:

assert.strictEqual(
  withImageVersion('/images/foo.png', 0),
  '/images/foo.png?v=0'
);

5. Assets router: cache headers applied inside the try block

try {
  const imageData = readFileSync(fullImagePath);
  res.setHeader('Cache-Control', 'no-store, no-cache, must-revalidate');
  // ...
  sendFile(res, imageData, 'image/png');

The headers are set after reading the file. If readFileSync throws, headers are never set (the catch presumably returns a 404 or error response), which is the correct behaviour. But if sendFile were to throw after headers are set, you'd have a partial response. This is an existing pattern in the codebase, not introduced by this PR, so not a blocker—just flagging it for awareness.

6. Pragma: no-cache / Expires: 0 coverage in tests

The second test block (line ~183) only asserts Cache-Control, not Pragma or Expires. The first block checks all three. Inconsistency is minor but worth making uniform so a future refactor doesn't accidentally drop those headers without failing tests.

---

💡 Suggestions (non-blocking)

• Consider exporting LOCAL_IMAGE_PREFIX = '/images/' as a named constant from image-url.js so the prefix isn't hardcoded in two places (the utility and any future consumers).
• The withImageVersion JSDoc comment could mention that it's a no-op for static/cloud URLs—makes it easier to grep and understand at a glance.

---

Summary

This is a good fix. The core logic is correct and the test coverage is solid. The main thing worth a follow-up is confirming whether timestamp can ever be absent on a comparison object returned from the TDD server, since that's the silent fallback path. Everything else is polish.

[vizzly-testing]

Vizzly - Visual Test Results

CLI Reporter - Approved

19 comparisons, no changes detected.

View build

CLI TUI - Approved

5 comparisons, no changes detected.

View build

---

_{fix/tdd-image-cache-bust · 1481ed68}