fix(web+appview): address code review feedback on theme caching (ATB-56)

+2

apps/appview/src/routes/__tests__/themes.test.ts

··· 139 139 140 140 const res = await app.request("/themes"); 141 141 expect(res.status).toBe(503); 142 + // Error responses must NOT be cached by CDNs/proxies 143 + expect(res.headers.get("Cache-Control")).toBeNull(); 142 144 }); 143 145 144 146 it("sets Cache-Control: public, max-age=300 on successful response", async () => {

+3 -1

apps/appview/src/routes/themes.ts

··· 37 37 return new Hono() 38 38 .get("/", async (c) => { 39 39 try { 40 - c.header("Cache-Control", "public, max-age=300"); 41 40 // Step 1: Get available theme URIs from this forum's policy 42 41 const availableRows = await ctx.db 43 42 .select({ themeUri: themePolicyAvailableThemes.themeUri }) ··· 49 48 .where(eq(themePolicies.did, ctx.config.forumDid)); 50 49 51 50 if (availableRows.length === 0) { 51 + c.header("Cache-Control", "public, max-age=300"); 52 52 return c.json({ themes: [] }); 53 53 } 54 54 ··· 58 58 .filter((rkey): rkey is string => !!rkey); 59 59 60 60 if (rkeys.length === 0) { 61 + c.header("Cache-Control", "public, max-age=300"); 61 62 return c.json({ themes: [] }); 62 63 } 63 64 ··· 73 74 ) 74 75 .limit(100); 75 76 77 + c.header("Cache-Control", "public, max-age=300"); 76 78 return c.json({ themes: themeList.map(serializeThemeSummary) }); 77 79 } catch (error) { 78 80 return handleRouteError(c, error, "Failed to retrieve themes", {

+17

apps/web/src/lib/__tests__/theme-cache.test.ts

··· 124 124 expect(cache.getTheme(darkUri, "light")).toBeNull(); 125 125 }); 126 126 127 + it("deleteTheme removes a specific entry before TTL", () => { 128 + const uri = "at://did/col/theme"; 129 + cache.setTheme(uri, "light", MOCK_THEME); 130 + cache.deleteTheme(uri, "light"); 131 + expect(cache.getTheme(uri, "light")).toBeNull(); 132 + }); 133 + 134 + it("deleteTheme only removes the targeted uri:colorScheme entry", () => { 135 + const uri = "at://did/col/theme"; 136 + const darkTheme: CachedTheme = { cid: "bafydark", tokens: { "color-bg": "#111" }, cssOverrides: null, fontUrls: null }; 137 + cache.setTheme(uri, "light", MOCK_THEME); 138 + cache.setTheme(uri, "dark", darkTheme); 139 + cache.deleteTheme(uri, "light"); 140 + expect(cache.getTheme(uri, "light")).toBeNull(); 141 + expect(cache.getTheme(uri, "dark")).toEqual(darkTheme); 142 + }); 143 + 127 144 it("evicts stale entry from the map on expired access", () => { 128 145 const uri = "at://did/col/theme"; 129 146 cache.setTheme(uri, "light", MOCK_THEME);

+67 -15

apps/web/src/lib/__tests__/theme-resolution.test.ts

··· 402 402 const cache = new ThemeCache(TTL_MS); 403 403 mockFetch 404 404 .mockResolvedValueOnce(policyResponse()) 405 - .mockResolvedValueOnce(themeResponse("light", "bafylight")) 406 - // second call — only theme fetch needed (policy is cached) 407 405 .mockResolvedValueOnce(themeResponse("light", "bafylight")); 408 406 409 407 await resolveTheme(APPVIEW, undefined, undefined, cache); 410 408 await resolveTheme(APPVIEW, undefined, undefined, cache); 411 409 412 - // 3 fetches: 1 policy + 2 theme (theme cache not populated for second call 413 - // because first call already cached it — actually theme should also be cached) 414 - // Both policy AND theme should be cached after first call 415 - expect(mockFetch).toHaveBeenCalledTimes(2); // policy + theme (1 each) 410 + // Both policy and theme are cached after the first call — second call makes no fetches 411 + expect(mockFetch).toHaveBeenCalledTimes(2); // policy (1) + theme (1), both from first call 416 412 }); 417 413 418 414 it("theme cache hit skips theme fetch on second call", async () => { ··· 461 457 expect(mockFetch).toHaveBeenCalledTimes(3); 462 458 }); 463 459 464 - it("stale cache CID triggers fresh fetch and logs warning", async () => { 460 + it("stale cache CID triggers eviction, fresh fetch, and logs warning", async () => { 465 461 const cache = new ThemeCache(TTL_MS); 466 - // First request: policy has cid=bafylight, theme fetched with cid=bafylight 467 462 mockFetch 468 463 .mockResolvedValueOnce(policyResponse()) 469 464 .mockResolvedValueOnce(themeResponse("light", "bafylight")); 470 465 await resolveTheme(APPVIEW, undefined, undefined, cache); 471 466 472 - // Simulate policy TTL expiry + policy updated with new CID 473 - // We manually update the cache's policy to have a new CID 467 + // Update cached policy to reflect a new CID (simulates admin updating the theme) 474 468 cache.setPolicy({ 475 469 defaultLightThemeUri: "at://did:plc:forum/space.atbb.forum.theme/3lbllight", 476 470 defaultDarkThemeUri: "at://did:plc:forum/space.atbb.forum.theme/3lbldark", 477 471 allowUserChoice: true, 478 472 availableThemes: [ 479 - { uri: "at://did:plc:forum/space.atbb.forum.theme/3lbllight", cid: "bafynew" }, // CID changed 473 + { uri: "at://did:plc:forum/space.atbb.forum.theme/3lbllight", cid: "bafynew" }, 480 474 { uri: "at://did:plc:forum/space.atbb.forum.theme/3lbldark", cid: "bafydark" }, 481 475 ], 482 476 }); 483 477 484 - // Next request: policy is re-cached with new CID; theme cache has old CID 485 - // → stale cache detected → fresh fetch triggered 486 478 mockFetch.mockResolvedValueOnce(themeResponse("light", "bafynew")); 487 479 488 480 const mockLogger = vi.mocked(logger); 489 481 const result = await resolveTheme(APPVIEW, undefined, undefined, cache); 490 482 491 483 expect(mockLogger.warn).toHaveBeenCalledWith( 492 - expect.stringContaining("Cached theme has stale CID"), 484 + expect.stringContaining("stale CID"), 493 485 expect.objectContaining({ expectedCid: "bafynew", cachedCid: "bafylight" }) 494 486 ); 495 - expect(result.tokens["color-bg"]).toBe("#fff"); // fresh theme served 487 + expect(result.tokens["color-bg"]).toBe("#fff"); 496 488 expect(mockFetch).toHaveBeenCalledTimes(3); // initial policy+theme + 1 fresh theme 489 + }); 490 + 491 + it("stale CID + failed fresh fetch falls back and evicts so next request retries", async () => { 492 + const cache = new ThemeCache(TTL_MS); 493 + mockFetch 494 + .mockResolvedValueOnce(policyResponse()) 495 + .mockResolvedValueOnce(themeResponse("light", "bafylight")); 496 + await resolveTheme(APPVIEW, undefined, undefined, cache); 497 + 498 + // Update policy to reflect a new CID 499 + cache.setPolicy({ 500 + defaultLightThemeUri: "at://did:plc:forum/space.atbb.forum.theme/3lbllight", 501 + defaultDarkThemeUri: "at://did:plc:forum/space.atbb.forum.theme/3lbldark", 502 + allowUserChoice: true, 503 + availableThemes: [ 504 + { uri: "at://did:plc:forum/space.atbb.forum.theme/3lbllight", cid: "bafynew" }, 505 + { uri: "at://did:plc:forum/space.atbb.forum.theme/3lbldark", cid: "bafydark" }, 506 + ], 507 + }); 508 + 509 + // Fresh fetch fails (AppView outage) 510 + mockFetch.mockResolvedValueOnce({ ok: false, status: 503 }); 511 + const fallbackResult = await resolveTheme(APPVIEW, undefined, undefined, cache); 512 + 513 + // Falls back to FALLBACK_THEME — stale data is not served 514 + expect(fallbackResult.tokens).toEqual(FALLBACK_THEME.tokens); 515 + 516 + // On the NEXT request: stale entry was evicted, so a fresh fetch is attempted again 517 + // (rather than re-detecting stale CID and looping forever) 518 + mockFetch.mockResolvedValueOnce(themeResponse("light", "bafynew")); 519 + const recoveredResult = await resolveTheme(APPVIEW, undefined, undefined, cache); 520 + 521 + expect(recoveredResult.tokens["color-bg"]).toBe("#fff"); 522 + expect(mockFetch).toHaveBeenCalledTimes(4); // initial 2 + failed fetch + recovered fetch 523 + }); 524 + 525 + it("cache repopulated after stale-CID fresh fetch — third call makes no fetches", async () => { 526 + const cache = new ThemeCache(TTL_MS); 527 + mockFetch 528 + .mockResolvedValueOnce(policyResponse()) 529 + .mockResolvedValueOnce(themeResponse("light", "bafylight")); 530 + await resolveTheme(APPVIEW, undefined, undefined, cache); 531 + 532 + cache.setPolicy({ 533 + defaultLightThemeUri: "at://did:plc:forum/space.atbb.forum.theme/3lbllight", 534 + defaultDarkThemeUri: "at://did:plc:forum/space.atbb.forum.theme/3lbldark", 535 + allowUserChoice: true, 536 + availableThemes: [ 537 + { uri: "at://did:plc:forum/space.atbb.forum.theme/3lbllight", cid: "bafynew" }, 538 + { uri: "at://did:plc:forum/space.atbb.forum.theme/3lbldark", cid: "bafydark" }, 539 + ], 540 + }); 541 + 542 + mockFetch.mockResolvedValueOnce(themeResponse("light", "bafynew")); 543 + await resolveTheme(APPVIEW, undefined, undefined, cache); // triggers fresh fetch, repopulates cache 544 + 545 + mockFetch.mockClear(); 546 + await resolveTheme(APPVIEW, undefined, undefined, cache); // should be a full cache hit 547 + 548 + expect(mockFetch).not.toHaveBeenCalled(); 497 549 }); 498 550 });

+4 -1

apps/web/src/lib/config.ts

··· 13 13 port: parseInt(process.env.WEB_PORT ?? "3001", 10), 14 14 appviewUrl: process.env.APPVIEW_URL ?? "http://localhost:3000", 15 15 logLevel: (process.env.LOG_LEVEL as LogLevel) ?? "info", 16 - themeCacheTtlMs: parseInt(process.env.THEME_CACHE_TTL_MS ?? "300000", 10), 16 + themeCacheTtlMs: (() => { 17 + const parsed = parseInt(process.env.THEME_CACHE_TTL_MS ?? "300000", 10); 18 + return Number.isNaN(parsed) ? 300_000 : parsed; 19 + })(), 17 20 }; 18 21 }

+18 -4

apps/web/src/lib/theme-cache.ts

··· 2 2 defaultLightThemeUri: string | null; 3 3 defaultDarkThemeUri: string | null; 4 4 allowUserChoice: boolean; 5 - availableThemes: Array<{ uri: string; cid?: string }>; 5 + availableThemes: Array<{ 6 + uri: string; 7 + /** 8 + * Present for pinned refs (locked to an exact version). 9 + * Absent for live refs (e.g. canonical atbb.space presets) — those 10 + * resolve to the current record at the URI without CID verification. 11 + */ 12 + cid?: string; 13 + }>; 6 14 } 7 15 8 16 export interface CachedTheme { 9 17 cid: string; 10 - tokens: Record<string, unknown>; 18 + /** All token values are CSS strings at the cache boundary. */ 19 + tokens: Record<string, string>; 11 20 cssOverrides: string | null; 12 21 fontUrls: string[] | null; 13 22 } ··· 34 43 35 44 constructor(readonly ttlMs: number) {} 36 45 37 - getPolicy(): CachedPolicy | null { 46 + getPolicy(): Readonly<CachedPolicy> | null { 38 47 if (!this.policy || Date.now() > this.policy.expiresAt) { 39 48 this.policy = null; 40 49 return null; ··· 46 55 this.policy = { data, expiresAt: Date.now() + this.ttlMs }; 47 56 } 48 57 49 - getTheme(uri: string, colorScheme: "light" | "dark"): CachedTheme | null { 58 + getTheme(uri: string, colorScheme: "light" | "dark"): Readonly<CachedTheme> | null { 50 59 const key = `${uri}:${colorScheme}`; 51 60 const entry = this.themes.get(key); 52 61 if (!entry || Date.now() > entry.expiresAt) { ··· 59 68 setTheme(uri: string, colorScheme: "light" | "dark", data: CachedTheme): void { 60 69 const key = `${uri}:${colorScheme}`; 61 70 this.themes.set(key, { data, expiresAt: Date.now() + this.ttlMs }); 71 + } 72 + 73 + /** Evict a stale theme entry so the next request fetches fresh data. */ 74 + deleteTheme(uri: string, colorScheme: "light" | "dark"): void { 75 + this.themes.delete(`${uri}:${colorScheme}`); 62 76 } 63 77 }

+6 -5

apps/web/src/lib/theme-resolution.ts

··· 51 51 return parts[4] ?? null; 52 52 } 53 53 54 - // Re-export for consumers that only need the type shapes 55 - export type { CachedPolicy, CachedTheme, ThemeCache }; 56 54 57 55 /** 58 56 * Resolves which theme to render for a request using the waterfall: ··· 138 136 // CID check on the cached entry — guards against stale cache when the policy 139 137 // refreshed (TTL expired) with a new CID while the theme entry is still live. 140 138 if (expectedCid && cachedTheme.cid !== expectedCid) { 141 - logger.warn("Cached theme has stale CID — fetching fresh from AppView", { 139 + logger.warn("Cached theme has stale CID — evicting and fetching fresh from AppView", { 142 140 operation: "resolveTheme", 143 141 expectedCid, 144 142 cachedCid: cachedTheme.cid, 145 143 themeUri: defaultUri, 146 144 }); 145 + // Evict the stale entry so that if the fresh fetch also fails, 146 + // the next request doesn't re-enter this loop indefinitely. 147 + cache?.deleteTheme(defaultUri, colorScheme); 147 148 // Fall through to fresh fetch below 148 149 } else { 149 150 // Cache hit — CID matches (or live ref with no expected CID) 150 151 return { 151 - tokens: cachedTheme.tokens as Record<string, string>, 152 + tokens: cachedTheme.tokens, 152 153 cssOverrides: cachedTheme.cssOverrides ?? null, 153 154 fontUrls: cachedTheme.fontUrls ?? null, 154 155 colorScheme, ··· 207 208 cache?.setTheme(defaultUri, colorScheme, theme); 208 209 209 210 return { 210 - tokens: theme.tokens as Record<string, string>, 211 + tokens: theme.tokens, 211 212 cssOverrides: theme.cssOverrides ?? null, 212 213 fontUrls: theme.fontUrls ?? null, 213 214 colorScheme,

+3

apps/web/src/middleware/theme.ts

··· 10 10 appviewUrl: string, 11 11 cacheTtlMs: number = DEFAULT_CACHE_TTL_MS 12 12 ): MiddlewareHandler<WebAppEnv> { 13 + // Intentionally outside the request handler: one shared instance per server 14 + // startup. Moving this inside the handler would create a new cache per 15 + // request, silently defeating the feature. 13 16 const cache = new ThemeCache(cacheTtlMs); 14 17 return async (c, next) => { 15 18 const cookieHeader = c.req.header("Cookie");

+1 -1

turbo.json

··· 19 19 }, 20 20 "test": { 21 21 "dependsOn": ["^build"], 22 - "env": ["DATABASE_URL", "LOG_LEVEL", "BACKFILL_RATE_LIMIT", "BACKFILL_CONCURRENCY", "BACKFILL_CURSOR_MAX_AGE_HOURS"] 22 + "env": ["DATABASE_URL", "LOG_LEVEL", "BACKFILL_RATE_LIMIT", "BACKFILL_CONCURRENCY", "BACKFILL_CURSOR_MAX_AGE_HOURS", "THEME_CACHE_TTL_MS"] 23 23 }, 24 24 "clean": { 25 25 "cache": false

Configure Feed

Configure Feed