feat: add favicons and screenshots tables with caching and persistence logic #182
Conversation
…e logic - Introduced new SQL tables for favicons and screenshots, including necessary constraints and indexes. - Implemented caching strategy for favicons and screenshots using Redis and Postgres, allowing for efficient retrieval and storage. - Added functions to upsert and fetch favicons and screenshots by domain ID, ensuring data integrity and expiration handling. - Enhanced service logic to persist generated assets to the database after creation.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded@jakejarvis has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds Postgres favicons/screenshots tables and related schema/zod/TTL helpers, repository functions and tests, integrates an optional DB L2 cache into the asset cache and services (favicon/screenshot), replaces Redis access cron with debounced post-response DB writes via after(), and defers multiple post-response side-effects using Next.js after(). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Cache as getOrCreateCachedAsset
participant Redis as Redis (L1)
participant DB as Postgres (L2)
participant Producer as Asset Producer
Client->>Cache: request(key, options)
Cache->>Redis: get(key)
alt Redis hit
Redis-->>Cache: asset
Cache-->>Client: asset
else Redis miss
Cache->>DB: fetchFromDb()
alt DB hit
DB-->>Cache: {url, key?, notFound?}
Cache->>Redis: set(key, result) (fire-and-forget via after)
Cache-->>Client: {url}
else DB miss
Cache->>Producer: produce()
Producer-->>Cache: {url, key?, metrics}
Cache->>DB: persistToDb(result) (fire-and-forget via after)
Cache->>Redis: set(key, result)
Cache-->>Client: {url}
end
end
sequenceDiagram
participant Requestor
participant Service as favicon/screenshot service
participant DB as Postgres
participant DomainRepo as ensureDomainRecord
participant Storage as Image Storage
Requestor->>Service: generate(domain)
Service->>Service: normalize -> registrableDomain
Service->>DB: get...ByDomainId(registrableDomain)
alt DB hit
DB-->>Service: cached asset
Service-->>Requestor: cached asset
else DB miss
Service->>Storage: storeImage/generated asset
Storage-->>Service: stored url, pathname
Service->>DomainRepo: ensureDomainRecord(registrableDomain)
DomainRepo-->>DB: upsert domain -> domainId
Service->>DB: upsert... (domainId, metadata, expiresAt) (fire-and-forget via after)
Service-->>Requestor: generated asset
end
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #182 +/- ##
==========================================
+ Coverage 69.38% 69.48% +0.09%
==========================================
Files 103 104 +1
Lines 2904 3002 +98
Branches 889 920 +31
==========================================
+ Hits 2015 2086 +71
- Misses 551 566 +15
- Partials 338 350 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/services/screenshot.ts (1)
139-145: Persisting null URLs and not wiringsourcemay not match intended semantics
persistToDbruns unconditionally after generation, even whenproduceAndUploadreturns{ url: null }(full failure). That results in:
- an upserted
screenshotsrow withurl: null,pathname: null, andnotFound: false, and- L2 cache hits that immediately return
{ url: null }for the entire TTL window without retrying generation.- If the intent is to treat screenshot generation failures as transient, you might want to:
- skip persistence (and maybe Redis writes) when
result.urlisnull, or- explicitly mark
notFound: trueonly for cases you consider “permanent” and leave transient failures uncached.- Also,
sourceis always persisted asnullhere, and the comment “Will be set from metrics if available” is currently misleading—metricsare not passed throughpersistToDb, so thesourcecolumn from this path will never be populated.A minimal change would be:
- persistToDb: async (result) => { + persistToDb: async (result) => { + if (!result.url && !result.notFound) { + // Skip persisting purely transient failures + return; + } const domainRecord = await ensureDomainRecord(registrable); const now = new Date(); const expiresAt = ttlForScreenshot(now); await upsertScreenshot({ domainId: domainRecord.id, url: result.url, pathname: result.key ?? null, width: VIEWPORT_WIDTH, height: VIEWPORT_HEIGHT, - source: null, // Will be set from metrics if available + // TODO: wire through a real source value once metrics are plumbed into persistToDb + source: null, notFound: result.notFound ?? false, fetchedAt: now, expiresAt, }); },This avoids caching opaque failures while keeping the door open to populate
sourcelater.Also applies to: 179-196
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
drizzle/0001_perpetual_wallow.sql(1 hunks)drizzle/meta/0001_snapshot.json(1 hunks)drizzle/meta/_journal.json(1 hunks)lib/cache.test.ts(1 hunks)lib/cache.ts(3 hunks)lib/db/pglite.ts(2 hunks)lib/db/repos/domain-helpers.ts(1 hunks)lib/db/repos/favicons.test.ts(1 hunks)lib/db/repos/favicons.ts(1 hunks)lib/db/repos/screenshots.test.ts(1 hunks)lib/db/repos/screenshots.ts(1 hunks)lib/db/schema.ts(1 hunks)lib/db/ttl.ts(2 hunks)lib/db/zod.ts(2 hunks)server/services/favicon.test.ts(3 hunks)server/services/favicon.ts(4 hunks)server/services/screenshot.test.ts(2 hunks)server/services/screenshot.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (14)
lib/db/repos/domain-helpers.ts (1)
lib/db/repos/domains.ts (1)
upsertDomain(17-30)
lib/db/zod.ts (1)
lib/db/schema.ts (2)
favicons(296-313)screenshots(316-332)
server/services/screenshot.test.ts (1)
lib/db/pglite.ts (2)
makePGliteDb(14-45)resetPGliteDb(48-74)
lib/db/repos/favicons.test.ts (6)
lib/db/pglite.ts (1)
makePGliteDb(14-45)lib/db/repos/domains.ts (1)
upsertDomain(17-30)lib/db/client.ts (1)
db(10-10)lib/db/schema.ts (1)
favicons(296-313)lib/db/ttl.ts (1)
ttlForFavicon(78-80)lib/db/repos/favicons.ts (2)
upsertFavicon(10-16)getFaviconByDomainId(21-29)
server/services/favicon.test.ts (1)
lib/db/pglite.ts (2)
makePGliteDb(14-45)resetPGliteDb(48-74)
lib/db/ttl.ts (1)
lib/constants.ts (2)
TTL_FAVICON(43-43)TTL_SCREENSHOT(44-44)
server/services/screenshot.ts (6)
lib/domain-server.ts (1)
toRegistrableDomain(6-16)lib/cache.ts (1)
getOrCreateCachedAsset(58-181)lib/db/repos/domains.ts (1)
findDomainByName(36-43)lib/db/repos/screenshots.ts (2)
getScreenshotByDomainId(21-31)upsertScreenshot(10-16)lib/db/repos/domain-helpers.ts (1)
ensureDomainRecord(13-28)lib/db/ttl.ts (1)
ttlForScreenshot(82-84)
lib/db/repos/screenshots.ts (3)
lib/db/zod.ts (1)
ScreenshotInsert(71-71)lib/db/schema.ts (1)
screenshots(316-332)lib/db/client.ts (1)
db(10-10)
server/services/favicon.ts (8)
lib/domain-server.ts (1)
toRegistrableDomain(6-16)lib/redis.ts (1)
ns(12-14)lib/constants.ts (1)
TTL_FAVICON(43-43)lib/cache.ts (1)
getOrCreateCachedAsset(58-181)lib/db/repos/domains.ts (1)
findDomainByName(36-43)lib/db/repos/favicons.ts (2)
getFaviconByDomainId(21-29)upsertFavicon(10-16)lib/db/repos/domain-helpers.ts (1)
ensureDomainRecord(13-28)lib/db/ttl.ts (1)
ttlForFavicon(78-80)
lib/cache.ts (1)
lib/redis.ts (1)
redis(4-7)
lib/db/pglite.ts (2)
lib/db/client.ts (1)
db(10-10)lib/db/schema.ts (2)
favicons(296-313)screenshots(316-332)
lib/db/repos/favicons.ts (3)
lib/db/zod.ts (1)
FaviconInsert(66-66)lib/db/schema.ts (1)
favicons(296-313)lib/db/client.ts (1)
db(10-10)
lib/db/repos/screenshots.test.ts (6)
lib/db/pglite.ts (1)
makePGliteDb(14-45)lib/db/repos/domains.ts (1)
upsertDomain(17-30)lib/db/client.ts (1)
db(10-10)lib/db/schema.ts (1)
screenshots(316-332)lib/db/ttl.ts (1)
ttlForScreenshot(82-84)lib/db/repos/screenshots.ts (2)
upsertScreenshot(10-16)getScreenshotByDomainId(21-31)
lib/cache.test.ts (2)
lib/redis.ts (2)
ns(12-14)redis(4-7)lib/cache.ts (1)
getOrCreateCachedAsset(58-181)
🔇 Additional comments (22)
drizzle/meta/_journal.json (1)
11-18: LGTM!The new migration journal entry follows the standard Drizzle format and correctly sequences the second migration after the initial schema.
lib/db/ttl.ts (1)
78-84: LGTM!The new TTL helpers follow the established pattern and correctly delegate to
addSecondswith the appropriate constants. The implementation is consistent with existing TTL calculation functions in this module.server/services/screenshot.test.ts (2)
54-61: LGTM!The test setup correctly initializes an isolated PGlite in-memory database and mocks the DB client, enabling tests to exercise the new DB-backed caching paths for screenshots.
70-72: LGTM!The teardown properly resets the PGlite database state between tests, ensuring test isolation for the new DB-backed screenshot functionality.
lib/db/pglite.ts (1)
59-72: LGTM!The extended cleanup correctly includes the new
faviconsandscreenshotstables. The deletion order properly respects foreign key constraints by deleting child tables before the parentdomainstable.server/services/favicon.test.ts (3)
4-20: LGTM!The mock for
toRegistrableDomainappropriately handles RFC-reserved test TLDs (.invalidand.example) while preserving the real implementation for production domains. This enables safe testing without relying on actual domain resolution.
45-51: LGTM!The test setup correctly initializes an isolated PGlite in-memory database and mocks the DB client, consistent with the screenshot test setup and enabling DB-backed caching tests.
62-64: LGTM!The teardown properly resets the PGlite database state between tests, ensuring test isolation for the new DB-backed favicon functionality.
lib/db/zod.ts (1)
64-72: LGTM!The new Zod schemas for favicons and screenshots follow the established pattern in this module, properly using
zReadfor strict select schemas andzWritefor insert/update schemas with date coercion. The implementation is consistent with existing schema definitions.drizzle/0001_perpetual_wallow.sql (2)
26-29: LGTM!The foreign key constraints with
ON DELETE CASCADEare appropriate—favicon and screenshot assets should be removed when their parent domain is deleted. The indexes onexpires_atwill enable efficient expiration-based cleanup queries.
1-12: The schema constraint concern is unfounded.The test evidence shows that
size,width, andheightare intentionally stored with numeric values even whennotFound: true(e.g.,favicons.test.ts:117-119setssize: 32withnotFound: true). The schema design is correct—these fields remainNOT NULLbecause they always receive values.Likely an incorrect or invalid review comment.
lib/db/repos/domain-helpers.ts (1)
13-27: LGTM!The function correctly ensures a domain record exists by extracting the TLD and upserting the domain. The implementation properly handles the
getDomainTldfallback and leverages the existingupsertDomainfunction.drizzle/meta/0001_snapshot.json (1)
1-1361: Generated Drizzle snapshot matches new favicons/screenshots schemaThe snapshot reflects the new
faviconsandscreenshotstables (PK ondomain_id,expires_atindexes, FKs todomains) and is consistent with the TypeScript schema definitions. Given this file is generated, manual edits should be avoided.server/services/screenshot.ts (1)
52-71: Input validation already guards the behavior changeThe
DomainInputSchemain the router validatestoRegistrableDomain(domain) !== nullbefore any request reachesgetOrCreateScreenshotBlobUrl, so the new throw behavior is unreachable under normal operation. This matches the established pattern ingetOrCreateFaviconBlobUrl(favicon.ts:26-33), which has identical error handling. TRPC automatically propagates any errors as validation failures to the client.server/services/favicon.ts (2)
29-35: Good practice: Domain normalization for cache consistency.Normalizing to the registrable domain ensures consistent cache keys and database records across subdomains (e.g.,
www.example.comandapi.example.comboth normalize toexample.com). The error handling is appropriate.
41-54: LGTM: DB cache lookup implementation.The L2 cache lookup properly handles the domain-not-found and favicon-not-found cases, and relies on
getFaviconByDomainIdto filter expired records.lib/cache.test.ts (1)
165-299: Excellent test coverage for DB cache integration.The new tests comprehensively cover:
- L2 cache hits with Redis backfill
- Cache misses triggering asset generation
- DB persistence after generation
- Graceful degradation on DB failures
- Optional DB callback behavior
The test structure is clear, and the fire-and-forget assertions correctly use
setTimeoutto wait for async operations.lib/db/repos/favicons.ts (1)
21-28: LGTM: Efficient retrieval with expiry filtering.The function correctly filters out expired records using a composite condition and benefits from the index on
expiresAtdefined in the schema.lib/db/schema.ts (2)
295-313: LGTM: Well-designed favicons table schema.The schema correctly models favicon caching with:
- One favicon per domain (via primary key on
domainId)- Nullable
url/pathnamefornotFound=truecases- Index on
expiresAtfor efficient expiry queries- Cascade delete maintaining referential integrity
315-332: LGTM: Consistent screenshots table schema.The schema mirrors the favicons design with appropriate adjustments for screenshots (width/height instead of size). The structure supports the same caching patterns with expiry and notFound tracking.
lib/cache.ts (2)
95-131: LGTM: Robust L2 cache implementation with graceful degradation.The database cache check properly:
- Short-circuits on DB hits and backfills Redis
- Gracefully handles DB failures without breaking the flow
- Logs appropriate warnings for debugging
The fire-and-forget Redis write is appropriate for performance.
138-151: LGTM: Fire-and-forget persistence with error handling.The DB persistence is correctly implemented as a fire-and-forget operation with error logging, ensuring that persistence failures don't impact the user-facing response.
Note: The
persistToDbsignature currently doesn't includemetrics, which prevents services from persisting metadata collected during asset generation. See the comment on server/services/favicon.ts lines 122-140 for the suggested fix.
lib/db/repos/domain-helpers.ts
Outdated
| // For unicode handling, we'd need to use toUnicode from node:url or a library, | ||
| // but for now we'll use the ASCII version as the unicode name if they match | ||
| // This is safe because rdapper already normalizes to ASCII/punycode when needed | ||
| const unicodeName = domain; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify unicode handling comment.
The comment suggests that setting unicodeName = domain is safe because "rdapper already normalizes to ASCII/punycode," but this doesn't actually convert punycode back to Unicode. If the input domain is already in punycode (e.g., xn--...), then unicodeName will also be punycode, not Unicode.
Consider either:
- Updating the comment to clarify that unicode conversion is deferred/not implemented yet
- Using Node's
url.domainToUnicode()to properly populateunicodeName
Example fix for the comment:
- // For unicode handling, we'd need to use toUnicode from node:url or a library,
- // but for now we'll use the ASCII version as the unicode name if they match
- // This is safe because rdapper already normalizes to ASCII/punycode when needed
+ // TODO: For proper unicode handling, use toUnicode from node:url to convert
+ // punycode domains back to unicode. For now, both name and unicodeName store
+ // the ASCII/punycode version.
const unicodeName = domain;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // For unicode handling, we'd need to use toUnicode from node:url or a library, | |
| // but for now we'll use the ASCII version as the unicode name if they match | |
| // This is safe because rdapper already normalizes to ASCII/punycode when needed | |
| const unicodeName = domain; | |
| // TODO: For proper unicode handling, use toUnicode from node:url to convert | |
| // punycode domains back to unicode. For now, both name and unicodeName store | |
| // the ASCII/punycode version. | |
| const unicodeName = domain; |
🤖 Prompt for AI Agents
In lib/db/repos/domain-helpers.ts around lines 16 to 19, the current comment and
assignment const unicodeName = domain wrongly imply punycode is converted back
to Unicode; update the comment to explicitly state that Unicode conversion is
not performed here (or deferred) and that if the input is punycode it will
remain punycode, or replace the assignment with a proper conversion using Node's
url.domainToUnicode(domain) to populate unicodeName; ensure you import
domainToUnicode from 'url' if you choose conversion and add a brief comment
explaining why/when conversion is applied.
| /* @vitest-environment node */ | ||
| import { | ||
| afterAll, | ||
| beforeAll, | ||
| beforeEach, | ||
| describe, | ||
| expect, | ||
| it, | ||
| vi, | ||
| } from "vitest"; | ||
|
|
||
| // Mock the DB client before importing anything else | ||
| vi.mock("@/lib/db/client", async () => { | ||
| const { makePGliteDb } = await import("@/lib/db/pglite"); | ||
| const { db } = await makePGliteDb(); | ||
| return { db }; | ||
| }); | ||
|
|
||
| import { db } from "@/lib/db/client"; | ||
| import { favicons } from "@/lib/db/schema"; | ||
| import { ttlForFavicon } from "@/lib/db/ttl"; | ||
| import { getFaviconByDomainId, upsertFavicon } from "./favicons"; | ||
|
|
||
| let testDomainId: string; | ||
|
|
||
| beforeAll(async () => { | ||
| // Create a test domain | ||
| const { upsertDomain } = await import("./domains"); | ||
| const domain = await upsertDomain({ | ||
| name: "test-favicon.com", | ||
| tld: "com", | ||
| unicodeName: "test-favicon.com", | ||
| }); | ||
| testDomainId = domain.id; | ||
| }); | ||
|
|
||
| afterAll(async () => { | ||
| // PGlite cleanup handled automatically | ||
| }); | ||
|
|
||
| beforeEach(async () => { | ||
| // Clear favicons table before each test | ||
| await db.delete(favicons); | ||
| }); | ||
|
|
||
| describe("upsertFavicon", () => { | ||
| it("inserts a new favicon record", async () => { | ||
| const now = new Date(); | ||
| const expiresAt = ttlForFavicon(now); | ||
|
|
||
| await upsertFavicon({ | ||
| domainId: testDomainId, | ||
| url: "https://example.com/favicon.webp", | ||
| pathname: "abc123/32x32.webp", | ||
| size: 32, | ||
| source: "duckduckgo", | ||
| notFound: false, | ||
| upstreamStatus: 200, | ||
| upstreamContentType: "image/x-icon", | ||
| fetchedAt: now, | ||
| expiresAt, | ||
| }); | ||
|
|
||
| const rows = await db.select().from(favicons); | ||
| expect(rows).toHaveLength(1); | ||
| expect(rows[0]?.url).toBe("https://example.com/favicon.webp"); | ||
| expect(rows[0]?.source).toBe("duckduckgo"); | ||
| }); | ||
|
|
||
| it("updates an existing favicon record", async () => { | ||
| const now = new Date(); | ||
| const expiresAt = ttlForFavicon(now); | ||
|
|
||
| // Insert first | ||
| await upsertFavicon({ | ||
| domainId: testDomainId, | ||
| url: "https://example.com/favicon-old.webp", | ||
| pathname: "old123/32x32.webp", | ||
| size: 32, | ||
| source: "google", | ||
| notFound: false, | ||
| upstreamStatus: 200, | ||
| upstreamContentType: "image/x-icon", | ||
| fetchedAt: now, | ||
| expiresAt, | ||
| }); | ||
|
|
||
| // Update with new data | ||
| const laterDate = new Date(now.getTime() + 1000); | ||
| await upsertFavicon({ | ||
| domainId: testDomainId, | ||
| url: "https://example.com/favicon-new.webp", | ||
| pathname: "new123/32x32.webp", | ||
| size: 32, | ||
| source: "duckduckgo", | ||
| notFound: false, | ||
| upstreamStatus: 200, | ||
| upstreamContentType: "image/webp", | ||
| fetchedAt: laterDate, | ||
| expiresAt, | ||
| }); | ||
|
|
||
| const rows = await db.select().from(favicons); | ||
| expect(rows).toHaveLength(1); | ||
| expect(rows[0]?.url).toBe("https://example.com/favicon-new.webp"); | ||
| expect(rows[0]?.source).toBe("duckduckgo"); | ||
| }); | ||
|
|
||
| it("handles notFound flag", async () => { | ||
| const now = new Date(); | ||
| const expiresAt = ttlForFavicon(now); | ||
|
|
||
| await upsertFavicon({ | ||
| domainId: testDomainId, | ||
| url: null, | ||
| pathname: null, | ||
| size: 32, | ||
| source: null, | ||
| notFound: true, | ||
| upstreamStatus: null, | ||
| upstreamContentType: null, | ||
| fetchedAt: now, | ||
| expiresAt, | ||
| }); | ||
|
|
||
| const rows = await db.select().from(favicons); | ||
| expect(rows).toHaveLength(1); | ||
| expect(rows[0]?.notFound).toBe(true); | ||
| expect(rows[0]?.url).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("getFaviconByDomainId", () => { | ||
| it("returns null when domain has no favicon", async () => { | ||
| const result = await getFaviconByDomainId(testDomainId); | ||
| expect(result).toBeNull(); | ||
| }); | ||
|
|
||
| it("returns favicon when not expired", async () => { | ||
| const now = new Date(); | ||
| const expiresAt = new Date(now.getTime() + 3600 * 1000); // 1 hour from now | ||
|
|
||
| await upsertFavicon({ | ||
| domainId: testDomainId, | ||
| url: "https://example.com/favicon.webp", | ||
| pathname: "abc123/32x32.webp", | ||
| size: 32, | ||
| source: "duckduckgo", | ||
| notFound: false, | ||
| upstreamStatus: 200, | ||
| upstreamContentType: "image/x-icon", | ||
| fetchedAt: now, | ||
| expiresAt, | ||
| }); | ||
|
|
||
| const result = await getFaviconByDomainId(testDomainId); | ||
| expect(result).not.toBeNull(); | ||
| expect(result?.url).toBe("https://example.com/favicon.webp"); | ||
| }); | ||
|
|
||
| it("returns null when favicon is expired", async () => { | ||
| const now = new Date(); | ||
| const expiresAt = new Date(now.getTime() - 1000); // 1 second ago | ||
|
|
||
| await upsertFavicon({ | ||
| domainId: testDomainId, | ||
| url: "https://example.com/favicon.webp", | ||
| pathname: "abc123/32x32.webp", | ||
| size: 32, | ||
| source: "duckduckgo", | ||
| notFound: false, | ||
| upstreamStatus: 200, | ||
| upstreamContentType: "image/x-icon", | ||
| fetchedAt: now, | ||
| expiresAt, | ||
| }); | ||
|
|
||
| const result = await getFaviconByDomainId(testDomainId); | ||
| expect(result).toBeNull(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Favicon repo tests comprehensively validate upsert + expiry semantics
- DB mocking and domain setup are correct and consistent with the screenshots tests.
- Tests assert single-row behavior on upsert, correct field updates,
notFoundhandling (includingurl: null), and time-based behavior forgetFaviconByDomainId. - This gives good confidence in both repository methods.
Given the near-identical structure with screenshots.test.ts, consider extracting shared test helpers for domain setup and table cleanup to reduce duplication.
🤖 Prompt for AI Agents
In lib/db/repos/favicons.test.ts around lines 1 to 181 the test file duplicates
domain setup and table cleanup logic that's nearly identical to
screenshots.test.ts; extract shared helpers (e.g., createTestDomain() that
performs the upsertDomain and returns domain.id, and clearTables(...tables) that
runs db.delete for given tables) into a test utilities module (e.g.,
test/helpers/db.ts), import those helpers here and in screenshots.test.ts,
replace the beforeAll/afterAll/beforeEach boilerplate with calls to the shared
helpers to reduce duplication and keep tests DRY.
| /* @vitest-environment node */ | ||
| import { | ||
| afterAll, | ||
| beforeAll, | ||
| beforeEach, | ||
| describe, | ||
| expect, | ||
| it, | ||
| vi, | ||
| } from "vitest"; | ||
|
|
||
| // Mock the DB client before importing anything else | ||
| vi.mock("@/lib/db/client", async () => { | ||
| const { makePGliteDb } = await import("@/lib/db/pglite"); | ||
| const { db } = await makePGliteDb(); | ||
| return { db }; | ||
| }); | ||
|
|
||
| import { db } from "@/lib/db/client"; | ||
| import { screenshots } from "@/lib/db/schema"; | ||
| import { ttlForScreenshot } from "@/lib/db/ttl"; | ||
| import { getScreenshotByDomainId, upsertScreenshot } from "./screenshots"; | ||
|
|
||
| let testDomainId: string; | ||
|
|
||
| beforeAll(async () => { | ||
| // Create a test domain | ||
| const { upsertDomain } = await import("./domains"); | ||
| const domain = await upsertDomain({ | ||
| name: "test-screenshot.com", | ||
| tld: "com", | ||
| unicodeName: "test-screenshot.com", | ||
| }); | ||
| testDomainId = domain.id; | ||
| }); | ||
|
|
||
| afterAll(async () => { | ||
| // PGlite cleanup handled automatically | ||
| }); | ||
|
|
||
| beforeEach(async () => { | ||
| // Clear screenshots table before each test | ||
| await db.delete(screenshots); | ||
| }); | ||
|
|
||
| describe("upsertScreenshot", () => { | ||
| it("inserts a new screenshot record", async () => { | ||
| const now = new Date(); | ||
| const expiresAt = ttlForScreenshot(now); | ||
|
|
||
| await upsertScreenshot({ | ||
| domainId: testDomainId, | ||
| url: "https://example.com/screenshot.webp", | ||
| pathname: "abc123/1200x630.webp", | ||
| width: 1200, | ||
| height: 630, | ||
| source: "direct_https", | ||
| notFound: false, | ||
| fetchedAt: now, | ||
| expiresAt, | ||
| }); | ||
|
|
||
| const rows = await db.select().from(screenshots); | ||
| expect(rows).toHaveLength(1); | ||
| expect(rows[0]?.url).toBe("https://example.com/screenshot.webp"); | ||
| expect(rows[0]?.width).toBe(1200); | ||
| expect(rows[0]?.height).toBe(630); | ||
| }); | ||
|
|
||
| it("updates an existing screenshot record", async () => { | ||
| const now = new Date(); | ||
| const expiresAt = ttlForScreenshot(now); | ||
|
|
||
| // Insert first | ||
| await upsertScreenshot({ | ||
| domainId: testDomainId, | ||
| url: "https://example.com/screenshot-old.webp", | ||
| pathname: "old123/1200x630.webp", | ||
| width: 1200, | ||
| height: 630, | ||
| source: "direct_http", | ||
| notFound: false, | ||
| fetchedAt: now, | ||
| expiresAt, | ||
| }); | ||
|
|
||
| // Update with new data | ||
| const laterDate = new Date(now.getTime() + 1000); | ||
| await upsertScreenshot({ | ||
| domainId: testDomainId, | ||
| url: "https://example.com/screenshot-new.webp", | ||
| pathname: "new123/1200x630.webp", | ||
| width: 1200, | ||
| height: 630, | ||
| source: "direct_https", | ||
| notFound: false, | ||
| fetchedAt: laterDate, | ||
| expiresAt, | ||
| }); | ||
|
|
||
| const rows = await db.select().from(screenshots); | ||
| expect(rows).toHaveLength(1); | ||
| expect(rows[0]?.url).toBe("https://example.com/screenshot-new.webp"); | ||
| expect(rows[0]?.source).toBe("direct_https"); | ||
| }); | ||
|
|
||
| it("handles notFound flag", async () => { | ||
| const now = new Date(); | ||
| const expiresAt = ttlForScreenshot(now); | ||
|
|
||
| await upsertScreenshot({ | ||
| domainId: testDomainId, | ||
| url: null, | ||
| pathname: null, | ||
| width: 1200, | ||
| height: 630, | ||
| source: null, | ||
| notFound: true, | ||
| fetchedAt: now, | ||
| expiresAt, | ||
| }); | ||
|
|
||
| const rows = await db.select().from(screenshots); | ||
| expect(rows).toHaveLength(1); | ||
| expect(rows[0]?.notFound).toBe(true); | ||
| expect(rows[0]?.url).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("getScreenshotByDomainId", () => { | ||
| it("returns null when domain has no screenshot", async () => { | ||
| const result = await getScreenshotByDomainId(testDomainId); | ||
| expect(result).toBeNull(); | ||
| }); | ||
|
|
||
| it("returns screenshot when not expired", async () => { | ||
| const now = new Date(); | ||
| const expiresAt = new Date(now.getTime() + 3600 * 1000); // 1 hour from now | ||
|
|
||
| await upsertScreenshot({ | ||
| domainId: testDomainId, | ||
| url: "https://example.com/screenshot.webp", | ||
| pathname: "abc123/1200x630.webp", | ||
| width: 1200, | ||
| height: 630, | ||
| source: "direct_https", | ||
| notFound: false, | ||
| fetchedAt: now, | ||
| expiresAt, | ||
| }); | ||
|
|
||
| const result = await getScreenshotByDomainId(testDomainId); | ||
| expect(result).not.toBeNull(); | ||
| expect(result?.url).toBe("https://example.com/screenshot.webp"); | ||
| }); | ||
|
|
||
| it("returns null when screenshot is expired", async () => { | ||
| const now = new Date(); | ||
| const expiresAt = new Date(now.getTime() - 1000); // 1 second ago | ||
|
|
||
| await upsertScreenshot({ | ||
| domainId: testDomainId, | ||
| url: "https://example.com/screenshot.webp", | ||
| pathname: "abc123/1200x630.webp", | ||
| width: 1200, | ||
| height: 630, | ||
| source: "direct_https", | ||
| notFound: false, | ||
| fetchedAt: now, | ||
| expiresAt, | ||
| }); | ||
|
|
||
| const result = await getScreenshotByDomainId(testDomainId); | ||
| expect(result).toBeNull(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Screenshots repo tests give solid coverage of core behaviors
- Properly mocks the DB client before importing repo code, so everything points at the in-memory PGlite instance.
- Covers insert, update (single-row invariant),
notFoundhandling, and expiry semantics (nullfor missing/expired, non-nullfor valid). beforeEachtruncation keeps tests isolated while sharing the same domain row frombeforeAll.
You could deduplicate some of the now/expiresAt setup with small helpers, but it’s fine as-is.
🤖 Prompt for AI Agents
In lib/db/repos/screenshots.test.ts lines 1 to 176, tests repeat creating `now`
and `expiresAt` values across cases; extract a small helper (e.g.,
makeNowAndExpiry(deltaMs?) or makeTimestamps) near the top of the file and
replace duplicated setups with calls to that helper to reduce repetition while
keeping behavior identical; ensure helper returns a Date for fetchedAt and an
expiresAt Date computed from passed delta (default 0) and update all tests to
use it.
…Record to domains.ts - Removed domain-helpers.ts and migrated the ensureDomainRecord function to domains.ts for better organization. - Updated service files to import ensureDomainRecord from the new location, maintaining functionality for favicon and screenshot services.
- Added metrics parameter to CachedAssetOptions for better tracking of asset performance. - Updated getOrCreateCachedAsset to utilize metrics when persisting cached assets. - Modified favicon and screenshot services to extract relevant metrics from results, improving data handling and source tracking.
…turn inserted rows - Updated upsertFavicon and upsertScreenshot functions to return the inserted row or null, improving data handling. - Added InferSelectModel type for favicons and screenshots to support the new return type.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
lib/cache.ts(3 hunks)lib/db/repos/domains.ts(2 hunks)lib/db/repos/favicons.ts(1 hunks)lib/db/repos/screenshots.ts(1 hunks)server/services/favicon.ts(4 hunks)server/services/screenshot.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
server/services/favicon.ts (7)
lib/domain-server.ts (1)
toRegistrableDomain(6-16)lib/redis.ts (1)
ns(12-14)lib/constants.ts (1)
TTL_FAVICON(43-43)lib/cache.ts (1)
getOrCreateCachedAsset(59-183)lib/db/repos/domains.ts (2)
findDomainByName(37-44)ensureDomainRecord(54-69)lib/db/repos/favicons.ts (2)
getFaviconByDomainId(29-37)upsertFavicon(11-24)lib/db/ttl.ts (1)
ttlForFavicon(78-80)
lib/db/repos/favicons.ts (3)
lib/db/zod.ts (1)
FaviconInsert(66-66)lib/db/schema.ts (1)
favicons(296-313)lib/db/client.ts (1)
db(10-10)
lib/db/repos/screenshots.ts (3)
lib/db/zod.ts (1)
ScreenshotInsert(71-71)lib/db/schema.ts (1)
screenshots(316-332)lib/db/client.ts (1)
db(10-10)
lib/cache.ts (1)
lib/redis.ts (1)
redis(4-7)
server/services/screenshot.ts (5)
lib/domain-server.ts (1)
toRegistrableDomain(6-16)lib/cache.ts (1)
getOrCreateCachedAsset(59-183)lib/db/repos/domains.ts (2)
findDomainByName(37-44)ensureDomainRecord(54-69)lib/db/repos/screenshots.ts (2)
getScreenshotByDomainId(29-39)upsertScreenshot(11-24)lib/db/ttl.ts (1)
ttlForScreenshot(82-84)
🔇 Additional comments (6)
lib/db/repos/screenshots.ts (1)
11-24: Screenshot repo upsert + TTL lookup look solidInput validation via
ScreenshotInsertSchema.parse, theonConflictDoUpdateondomainIdwith.returning()and theexpiresAt > nowfilter ingetScreenshotByDomainIdgive you the expected “one row per domain, non‑expired only” semantics and mirror the favicon repo nicely. I don’t see any issues here.Also applies to: 29-39
lib/db/repos/favicons.ts (1)
11-24: Favicon repo upsert + expiry lookup are well-structuredValidating via
FaviconInsertSchema.parsebefore the upsert, updating all fields on conflict bydomainId, and returningrows[0] ?? nullgives clear, predictable behavior. TheexpiresAt > nowfilter ingetFaviconByDomainIdmatches the screenshot repo and the intended TTL semantics.Also applies to: 29-37
server/services/favicon.ts (1)
29-35: Favicon caching behavior is working as designed; optional refactor suggestion should be removedThe registrable-domain normalization, DB persistence, and metadata handling are all confirmed solid. However, the negative caching test suite (favicon.test.ts, lines 130–141) explicitly validates that calls to the same domain return
{ url: null }from cache on subsequent requests without retrying—this is intentional behavior, not a bug. The optional refactor suggestion to skip records whereurlis null andnotFoundis false contradicts the tested semantics and should be removed.server/services/screenshot.ts (1)
51-56: Input validation for registrable domains is already enforced upstream—no caller risk.The production caller at
server/routers/domain.ts:100receives input throughDomainInputSchema(lines 29–35), which includes a.refine()check that validatestoRegistrableDomain(domain) !== nullbefore passing the domain togetOrCreateScreenshotBlobUrl. This means the throw atscreenshot.ts:51–56is effectively unreachable from the TRPC endpoint, and no caller can pass non-registrable hosts like IPs or localhost to the function.The implementation is sound—callers are already protected by schema validation at the router level.
lib/db/repos/domains.ts (1)
46-69: Implementation is correct; no changes needed.The assumptions in the JSDoc are validated:
- Both call sites (screenshot.ts, favicon.ts) pass
registrableobtained viatoRegistrableDomain()with guard checks, ensuring the domain is normalized before reachingensureDomainRecord.- The empty string TLD sentinel (
getDomainTld(domain) ?? "") follows the established pattern throughout the codebase (pricing.ts, registration.ts) and is handled uniformly byupsertDomain.The implementation aligns with existing patterns and no downstream issues with the empty TLD value were found.
lib/cache.ts (1)
44-50: Updated caching strategy docs accurately describe the new flowThe expanded comment clearly documents the L1 Redis → L2 Postgres → generation → persist → Redis write pipeline and matches the actual control flow below. This is helpful for future maintainers and keeps the behavior of
getOrCreateCachedAssetdiscoverable.Also applies to: 70-70
lib/cache.ts
Outdated
| // 4) Persist to Postgres if callback provided (fire-and-forget) | ||
| if (persistToDb) { | ||
| persistToDb({ | ||
| url: produced.url, | ||
| key: produced.key, | ||
| notFound: produced.notFound, | ||
| metrics: produced.metrics, | ||
| }).catch((err) => { | ||
| console.error( | ||
| "[cache] db persist error", | ||
| { indexKey }, | ||
| err instanceof Error ? err : new Error(String(err)), | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| // 5) Cache in Redis for next time (fire-and-forget) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Fire‑and‑forget DB persistence is reasonable; consider observability hooks
The persistToDb callback is invoked fire‑and‑forget with a defensive .catch that logs any failure, which keeps latency of getOrCreateCachedAsset low and avoids impacting callers on persistence errors.
If DB persistence is important for analytics or correctness, you might want to:
- Add a structured metric or counter for
[cache] db persist errorin addition to the log, and/or - Optionally allow callers to opt into awaiting
persistToDb(e.g., via a flag) for flows where durability is more important than latency.
Not strictly required for correctness, but would improve operational visibility.
🤖 Prompt for AI Agents
In lib/cache.ts around lines 139 to 155, the current fire-and-forget persistToDb
call only logs errors to console which limits observability; update the
implementation to (1) emit a structured metric/counter (e.g.,
metrics.increment('cache.db_persist_error')) alongside the existing error log
inside the .catch so failures are visible to monitoring, and (2) add an optional
flag on getOrCreateCachedAsset (or its options) that when set will await
persistToDb and surface or handle errors deterministically for callers that need
durability; keep the default behavior fire-and-forget but ensure the error
handler logs structured context (indexKey, url, key) and increments the metric.
| // Persist to Postgres after generation | ||
| persistToDb: async (result) => { | ||
| const domainRecord = await ensureDomainRecord(registrable); | ||
| const now = new Date(); | ||
| const expiresAt = ttlForScreenshot(now); | ||
|
|
||
| await upsertScreenshot({ | ||
| domainId: domainRecord.id, | ||
| url: result.url, | ||
| pathname: result.key ?? null, | ||
| width: VIEWPORT_WIDTH, | ||
| height: VIEWPORT_HEIGHT, | ||
| source: result.metrics?.source ?? null, | ||
| notFound: result.notFound ?? false, | ||
| fetchedAt: now, | ||
| expiresAt, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
DB persistence of null-URL screenshots effectively negative-caches failures
With persistToDb always upserting, even when result.url is null, the DB row will be treated as a “hit” by fetchFromDb and will cause getOrCreateCachedAsset to return { url: null } without re-running Puppeteer while the row’s expiresAt is in the future. That’s a reasonable way to protect expensive screenshot generation after repeated failures.
If you’d rather keep retrying on every request when no screenshot was produced (while still persisting metrics/timestamps), consider having fetchFromDb treat records with url === null as a miss:
const screenshotRecord = await getScreenshotByDomainId(existingDomain.id);
if (!screenshotRecord || screenshotRecord.url == null) return null;This keeps the current schema but changes the negative-cache semantics to “DB only short-circuits when we actually have a saved URL”.
🤖 Prompt for AI Agents
In server/services/screenshot.ts around lines 178-195, the current persistToDb
upserts records even when result.url is null, which lets fetchFromDb treat those
rows as cached hits and prevents retries; modify the DB-read path (fetchFromDb
or getOrCreateCachedAsset) so that after loading a screenshot record you treat
url === null as a cache miss (i.e., return null / continue to recreate) instead
of a hit; ensure you still persist metrics/timestamps if you want negative-cache
behavior elsewhere, but change the fetch logic to explicitly check for
screenshotRecord.url == null and act as if no record exists.
- Deleted the access-sync cron job from vercel.json and removed the corresponding route implementation. - Updated the recordDomainAccess function to debounce database writes, ensuring updates occur only if more than 5 minutes have passed since the last attempt. - Introduced a new updateLastAccessed function to handle individual domain updates, reducing unnecessary writes to the database.
…ions - Replaced fire-and-forget pattern with 'after' for caching operations in cache.ts, pricing.ts, and registration.ts, ensuring that Redis writes do not block the main execution flow. - Improved error handling for Redis operations by maintaining the existing logging structure. - Enhanced code readability and maintainability by centralizing asynchronous cache updates.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
lib/cache.ts (1)
23-40: DB callbacks API looks good, butproduceAndUploadmay be overly strict for L2-only flowsThe addition of
fetchFromDb/persistToDbis type-consistent with the producer result shape andTProduceMeta, and the optionality on these callbacks is appropriate.However,
produceAndUploadremains required even for potential L2-only (DB-only) use cases where you might want to read from Postgres and never generate. If those flows are expected, consider makingproduceAndUploadoptional and guarding at runtime (throw a clear error if both caches miss and no producer is provided) instead of forcing callers to pass a dummy producer. This mirrors an earlier review note on the same API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
app/api/cron/access-sync/route.ts(0 hunks)lib/access.test.ts(1 hunks)lib/access.ts(2 hunks)lib/cache.ts(4 hunks)lib/db/repos/domains.ts(2 hunks)server/services/pricing.ts(2 hunks)server/services/registration.ts(3 hunks)vercel.json(0 hunks)
💤 Files with no reviewable changes (2)
- app/api/cron/access-sync/route.ts
- vercel.json
🧰 Additional context used
🧬 Code graph analysis (5)
lib/db/repos/domains.ts (2)
lib/db/client.ts (1)
db(10-10)lib/db/schema.ts (1)
domains(79-99)
lib/cache.ts (1)
lib/redis.ts (1)
redis(4-7)
server/services/pricing.ts (1)
lib/redis.ts (1)
redis(4-7)
server/services/registration.ts (1)
lib/db/repos/registrations.ts (1)
setRegistrationStatusInCache(79-107)
lib/access.ts (1)
lib/db/repos/domains.ts (1)
updateLastAccessed(80-99)
🪛 GitHub Actions: Run tests and upload coverage
lib/cache.ts
[error] 161-161: after was called outside a request scope. Read more: https://nextjs.org/docs/messages/next-dynamic-api-wrong-context
server/services/pricing.ts
[error] 58-58: after was called outside a request scope. Read more: https://nextjs.org/docs/messages/next-dynamic-api-wrong-context
server/services/registration.ts
[error] 145-145: after was called outside a request scope. Read more: https://nextjs.org/docs/messages/next-dynamic-api-wrong-context
🪛 GitHub Check: build-and-test
lib/cache.ts
[failure] 144-144: lib/cache.test.ts > cached assets > handles DB persist errors gracefully
Error: after was called outside a request scope. Read more: https://nextjs.org/docs/messages/next-dynamic-api-wrong-context
❯ after node_modules/.pnpm/next@16.0.3_@babel+core@7.28.5_@opentelemetry+api@1.9.0_babel-plugin-react-compiler@19._30ec04ef55a575873a8393e7b75d6d7f/node_modules/next/dist/server/after/after.js:16:37
❯ getOrCreateCachedAsset lib/cache.ts:144:7
❯ lib/cache.test.ts:276:20
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { __NEXT_ERROR_CODE: 'E468' }
[failure] 161-161: lib/cache.test.ts > cached assets > handles DB fetch errors gracefully
Error: after was called outside a request scope. Read more: https://nextjs.org/docs/messages/next-dynamic-api-wrong-context
❯ after node_modules/.pnpm/next@16.0.3_@babel+core@7.28.5_@opentelemetry+api@1.9.0_babel-plugin-react-compiler@19._30ec04ef55a575873a8393e7b75d6d7f/node_modules/next/dist/server/after/after.js:16:37
❯ getOrCreateCachedAsset lib/cache.ts:161:5
❯ lib/cache.test.ts:251:20
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { __NEXT_ERROR_CODE: 'E468' }
[failure] 161-161: lib/cache.test.ts > cached assets > skips DB callbacks when not provided
Error: after was called outside a request scope. Read more: https://nextjs.org/docs/messages/next-dynamic-api-wrong-context
❯ after node_modules/.pnpm/next@16.0.3_@babel+core@7.28.5_@opentelemetry+api@1.9.0_babel-plugin-react-compiler@19._30ec04ef55a575873a8393e7b75d6d7f/node_modules/next/dist/server/after/after.js:16:37
❯ getOrCreateCachedAsset lib/cache.ts:161:5
❯ lib/cache.test.ts:233:20
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { __NEXT_ERROR_CODE: 'E468' }
[failure] 144-144: lib/cache.test.ts > cached assets > generates asset when both Redis and DB miss
Error: after was called outside a request scope. Read more: https://nextjs.org/docs/messages/next-dynamic-api-wrong-context
❯ after node_modules/.pnpm/next@16.0.3_@babel+core@7.28.5_@opentelemetry+api@1.9.0_babel-plugin-react-compiler@19._30ec04ef55a575873a8393e7b75d6d7f/node_modules/next/dist/server/after/after.js:16:37
❯ getOrCreateCachedAsset lib/cache.ts:144:7
❯ lib/cache.test.ts:204:20
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { __NEXT_ERROR_CODE: 'E468' }
[failure] 161-161: lib/cache.test.ts > cached assets > checks DB cache (L2) on Redis miss
Error: after was called outside a request scope. Read more: https://nextjs.org/docs/messages/next-dynamic-api-wrong-context
❯ after node_modules/.pnpm/next@16.0.3_@babel+core@7.28.5_@opentelemetry+api@1.9.0_babel-plugin-react-compiler@19._30ec04ef55a575873a8393e7b75d6d7f/node_modules/next/dist/server/after/after.js:16:37
❯ getOrCreateCachedAsset lib/cache.ts:161:5
❯ lib/cache.test.ts:173:20
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { __NEXT_ERROR_CODE: 'E468' }
[failure] 161-161: lib/cache.test.ts > cached assets > caches notFound flag when returned by producer
Error: after was called outside a request scope. Read more: https://nextjs.org/docs/messages/next-dynamic-api-wrong-context
❯ after node_modules/.pnpm/next@16.0.3_@babel+core@7.28.5_@opentelemetry+api@1.9.0_babel-plugin-react-compiler@19._30ec04ef55a575873a8393e7b75d6d7f/node_modules/next/dist/server/after/after.js:16:37
❯ getOrCreateCachedAsset lib/cache.ts:161:5
❯ lib/cache.test.ts:145:20
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { __NEXT_ERROR_CODE: 'E468' }
[failure] 161-161: lib/cache.test.ts > cached assets > retries transient failures (null without notFound flag)
Error: after was called outside a request scope. Read more: https://nextjs.org/docs/messages/next-dynamic-api-wrong-context
❯ after node_modules/.pnpm/next@16.0.3_@babel+core@7.28.5_@opentelemetry+api@1.9.0_babel-plugin-react-compiler@19._30ec04ef55a575873a8393e7b75d6d7f/node_modules/next/dist/server/after/after.js:16:37
❯ getOrCreateCachedAsset lib/cache.ts:161:5
❯ lib/cache.test.ts:102:20
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { __NEXT_ERROR_CODE: 'E468' }
[failure] 161-161: lib/cache.test.ts > cached assets > produces, stores, and returns new asset
Error: after was called outside a request scope. Read more: https://nextjs.org/docs/messages/next-dynamic-api-wrong-context
❯ after node_modules/.pnpm/next@16.0.3_@babel+core@7.28.5_@opentelemetry+api@1.9.0_babel-plugin-react-compiler@19._30ec04ef55a575873a8393e7b75d6d7f/node_modules/next/dist/server/after/after.js:16:37
❯ getOrCreateCachedAsset lib/cache.ts:161:5
❯ lib/cache.test.ts:68:20
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { __NEXT_ERROR_CODE: 'E468' }
[failure] 161-161: lib/cache.test.ts > cached assets > generates asset when cache miss
Error: after was called outside a request scope. Read more: https://nextjs.org/docs/messages/next-dynamic-api-wrong-context
❯ after node_modules/.pnpm/next@16.0.3_@babel+core@7.28.5_@opentelemetry+api@1.9.0_babel-plugin-react-compiler@19._30ec04ef55a575873a8393e7b75d6d7f/node_modules/next/dist/server/after/after.js:16:37
❯ getOrCreateCachedAsset lib/cache.ts:161:5
❯ lib/cache.test.ts:56:20
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { __NEXT_ERROR_CODE: 'E468' }
🔇 Additional comments (6)
lib/access.test.ts (1)
11-15: MockingupdateLastAccessedis appropriate to isolate tests from DBThe added
vi.mockcleanly stubsupdateLastAccessedbefore./accessis imported, removing the DB dependency from these tests while keeping the behavior focused on the decay helpers. Sincelib/access.tsonly usesupdateLastAccessed, the partial mock is sufficient.lib/access.ts (1)
3-3: The review comment references a non-existentsafeAfterutility and patternThe suggested refactor to
@/lib/safe-aftercannot be applied because:
safeAfterdoes not exist — no such utility is defined or exported anywhere in the codebase.- The referenced pattern doesn't match reality — pricing and caching code (server/services/pricing.ts, lib/cache.ts) use
after()directly without any wrapper helper. Only lib/analytics/server.ts has a try/catch defensive pattern, and it doesn't use a named utility either.recordDomainAccessis only called in real request contexts — it's invoked from trpc/init.ts within tRPC middleware, and test files deliberately avoid testing it (per the comment: "only testing pure calculation functions, not recordDomainAccess").The debounce logic itself is sound, but the suggested refactor cannot proceed.
Likely an incorrect or invalid review comment.
server/services/registration.ts (1)
2-2: The review comment references a non-existentsafeAfterhelper that cannot be importedThe recommended fix imports
safeAfterfrom"@/lib/safe-after", but this file does not exist in the codebase. Additionally, the "pricing/caching pattern" cited as the model for alignment (server/services/pricing.ts) also callsafter()directly without any wrapper—it uses the raw Next.jsafter()function in lines 46–52 and 57–63, not a safe wrapper.An existing workaround pattern does exist in
lib/analytics/server.ts(lines 85–90 and 122–127), which uses try-catch to gracefully handle the E468 error whenafter()is unavailable outside request contexts. However, the review's recommendation cannot be implemented as written.Likely an incorrect or invalid review comment.
lib/cache.ts (1)
1-1: The review comment references asafeAfterhelper that does not exist in the codebaseThe review incorrectly claims that a
safeAfterhelper is "used in the services," but verification shows:
- No
safeAfterexists anywhere in the codebase (confirmed via multiple searches)- All service files (registration.ts, pricing.ts) import and use
afterdirectly from "next/server" without any wrapper- The suggested file
@/lib/safe-afterdoes not exist- The latest commit adding
after()usage does not include this helperThe review cannot be applied as written because the referenced solution doesn't exist. Either:
- The safeAfter implementation needs to be provided as part of this change, OR
- This review is based on code from a different branch/PR that hasn't been merged yet
Likely an incorrect or invalid review comment.
lib/db/repos/domains.ts (2)
3-4: LGTM: Imports are necessary and correctly used.The new imports support the domain TLD extraction via rdapper and the conditional SQL logic in
updateLastAccessed.
90-92: SQL condition correctly implements debounced updates.The WHERE clause properly handles both NULL and stale
lastAccessedAtvalues, ensuring updates only occur when needed. The use of indexed fields should provide good query performance.
| // For unicode handling, we'd need to use toUnicode from node:url or a library, | ||
| // but for now we'll use the ASCII version as the unicode name if they match | ||
| // This is safe because rdapper already normalizes to ASCII/punycode when needed | ||
| const unicodeName = domain; |
There was a problem hiding this comment.
Incomplete Unicode domain name handling.
The current implementation always sets unicodeName to the ASCII/punycode version (same as domain), which defeats the purpose of having separate unicode and ASCII name fields. The comment acknowledges this limitation but doesn't track it as technical debt.
Consider using Node's url.domainToUnicode() for proper IDN handling, or create a tracking issue for this limitation.
+ // TODO: Implement proper Unicode handling
+ // Convert punycode domains like xn--... to their Unicode representation
+ // For now, both fields store the ASCII version
const unicodeName = domain;Would you like me to open an issue to track proper Unicode domain name support?
Committable suggestion skipped: line range outside the PR's diff.
…rvices - Replaced try-catch blocks with 'after' for scheduling revalidation in certificates, dns, headers, hosting, registration, and seo services, allowing for immediate execution of callbacks without blocking. - Improved error handling by ensuring that errors are logged similarly to the previous implementation. - Enhanced code readability and maintainability by centralizing asynchronous scheduling logic.
…ve error handling - Made the produceAndUpload function optional in CachedAssetOptions, throwing an error if not provided during asset generation. - Enhanced cache hit logic to only treat results as definitive if the URL is present or marked as permanently not found, aligning with Redis L1 semantics. - Improved error handling for Redis write operations, ensuring consistent logging for failures.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/cache.ts (1)
162-177: DB persistence observability remains limited to logging.The fire-and-forget
persistToDbpattern with defensive error handling is appropriate for keeping response latency low. However, as noted in previous reviews, the observability concern persists: onlyconsole.erroris emitted on failures, with no structured metrics or counters for monitoring.For production visibility, consider emitting a metric/counter (e.g.,
metrics.increment('cache.db_persist_error')) alongside the log, especially if DB persistence is important for analytics or correctness guarantees.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
lib/cache.ts(4 hunks)lib/db/repos/domains.ts(2 hunks)lib/schedule.test.ts(1 hunks)server/services/certificates.ts(2 hunks)server/services/dns.ts(4 hunks)server/services/headers.test.ts(1 hunks)server/services/headers.ts(2 hunks)server/services/hosting.ts(2 hunks)server/services/registration.ts(4 hunks)server/services/seo.ts(2 hunks)vitest.setup.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
server/services/hosting.ts (1)
lib/schedule.ts (1)
scheduleRevalidation(53-125)
server/services/headers.ts (1)
lib/schedule.ts (1)
scheduleRevalidation(53-125)
server/services/certificates.ts (1)
lib/schedule.ts (1)
scheduleRevalidation(53-125)
server/services/registration.ts (2)
lib/db/repos/registrations.ts (1)
setRegistrationStatusInCache(79-107)lib/schedule.ts (1)
scheduleRevalidation(53-125)
server/services/dns.ts (1)
lib/schedule.ts (1)
scheduleRevalidation(53-125)
lib/cache.ts (1)
lib/redis.ts (1)
redis(4-7)
server/services/seo.ts (1)
lib/schedule.ts (1)
scheduleRevalidation(53-125)
lib/db/repos/domains.ts (2)
lib/db/client.ts (1)
db(10-10)lib/db/schema.ts (1)
domains(79-99)
🔇 Additional comments (23)
lib/schedule.test.ts (1)
12-15: LGTM!The mock correctly isolates the test from database dependencies. The placement and syntax are appropriate for Vitest, and this aligns with the new
updateLastAccessedfunction added to the domains repository.lib/db/repos/domains.ts (3)
3-4: LGTM!The new imports support the added functionality:
sqlfor the conditional WHERE clause andgetDomainTldfor TLD extraction.
8-13: LGTM!The debounce constant is properly extracted and documented, addressing the previous review feedback.
53-81: LGTM!The function properly validates TLD extraction and throws an error for invalid domains, addressing previous review feedback. The Unicode handling limitation is clearly documented, which is acceptable given the technical constraints noted in the comments.
server/services/headers.ts (1)
3-3: LGTM: Proper use of Next.js 15'safter()API.The refactoring correctly defers the scheduling side-effect to execute after the response is sent. Error handling is preserved via
.catch(), and the non-blocking behavior is appropriate for this use case.Also applies to: 100-112
server/services/hosting.ts (1)
3-3: LGTM: Consistent implementation of deferred scheduling.The change mirrors the pattern in headers.ts, correctly using
after()to defer scheduling without blocking the response.Also applies to: 260-272
server/services/headers.test.ts (1)
21-24: LGTM: Appropriate test mock to prevent external API calls.The mock correctly prevents Inngest API calls during tests and aligns with the test environment setup in vitest.setup.ts.
vitest.setup.ts (1)
25-39: LGTM: Test mock correctly simulatesafter()behavior.The mock executes callbacks immediately for deterministic testing while swallowing errors to match production behavior. The combination with the
scheduleRevalidationmock in test files ensures no external side effects during tests.server/services/dns.ts (2)
2-2: LGTM: Deferred scheduling for partial DNS refresh.Correctly wraps the scheduling call in
after()for the partial refresh path, with appropriate error handling.Also applies to: 426-446
555-574: LGTM: Deferred scheduling for full DNS fetch.Mirrors the pattern used in the partial refresh path, maintaining consistency across both code paths.
server/services/seo.ts (1)
2-2: LGTM: Consistent deferred scheduling implementation.The refactoring follows the established pattern across all service files, correctly using
after()for non-blocking scheduling.Also applies to: 240-252
server/services/certificates.ts (1)
3-3: LGTM: Deferred scheduling with proper certificate expiry handling.The refactoring correctly defers scheduling while maintaining the certificate expiry-based scheduling logic.
Also applies to: 170-183
server/services/registration.ts (4)
2-2: LGTM: Deferred Redis cache warming in fast path.Correctly uses
after()to warm the Redis cache without blocking the response for cached registration data.Also applies to: 145-156
159-171: LGTM: Deferred scheduling in fast path.Properly defers the scheduling call, maintaining the access-tracking behavior while not blocking the response.
227-236: LGTM: Deferred cache update after RDAP lookup.Correctly defers the Redis cache update in the slow path, allowing the response to return immediately after the lookup completes.
352-364: LGTM: Deferred scheduling after domain persistence.Mirrors the pattern used in the fast path, correctly deferring the scheduling call after persisting the domain to Postgres.
lib/cache.ts (7)
1-1: LGTM: Correct usage of Next.js 15'safter()API.The import and usage of
after()throughout the file correctly implements Next.js 15's pattern for scheduling post-response work, which is ideal for fire-and-forget cache writes.
15-40: Well-structured type definitions with consistent callback signatures.The optional
produceAndUpload,fetchFromDb, andpersistToDbcallbacks use consistent result shapes, which makes the API coherent and flexible for various caching strategies. The decision to makeproduceAndUploadoptional (with a runtime guard below) correctly addresses the previous review feedback.
43-60: Clear and accurate documentation of the caching strategy.The updated comments correctly describe the multi-tier caching flow and provide good rationale for the fail-open design decisions.
98-148: Excellent L2 cache implementation with correct hit semantics.The DB cache layer correctly distinguishes between:
- Definitive hits (url is present or permanently not found)
- "Not yet generated" rows that should fall through to generation
The
isDefinitiveResultcheck at lines 108-109 properly addresses the previous review concern about treating any non-nulldbResultas a hit. The fail-open error handling and Redis backfill usingafter()are both implemented correctly.
150-156: Clear guard for missing producer correctly addresses previous feedback.The runtime check for
produceAndUploadis well-placed and throws a descriptive error when both caches miss but no producer is available. This properly completes the refactor to makeproduceAndUploadoptional.
180-199: Redis caching implementation is correct and consistent.The fire-and-forget Redis write using
after()correctly caches the generated asset with appropriate TTL. TheexpiresAtMsfield in the cached object (while not used by Redis itself) provides application-level timestamp information that may be useful for debugging or staleness checks.
202-208: Appropriate error handling for asset generation failures.Production errors are logged with context and correctly re-thrown to allow callers to handle failures as needed. This maintains the fail-fast behavior for generation while keeping cache operations fail-open.
- Enhanced the updateLastAccessed function to utilize logical operators for better readability and maintainability. - Replaced raw SQL conditions with Drizzle ORM's query builder methods for improved clarity and type safety.
Summary by CodeRabbit
New Features
Behavior Changes
Tests