Add unified cross-runtime logger implementation #131
Conversation
WalkthroughAdds a cross‑runtime structured logger ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Requestor
participant App as Application
participant TRPC as tRPC
participant Context as createContext
participant LoggerAPI as logger(...)
participant Backend as Runtime Backend
Client->>App: HTTP request
App->>TRPC: route to tRPC handler
TRPC->>Context: createContext(req)
Context->>LoggerAPI: createRequestLogger({ method,path,requestId,vercelId,ip })
LoggerAPI->>Backend: choose Pino (node) or Console (edge/browser)
Backend-->>LoggerAPI: Logger instance
LoggerAPI-->>Context: request-scoped log (ctx.log)
TRPC->>TRPC: withLogging middleware logs start
TRPC->>App: handler runs (uses ctx.log/module logs)
alt success
TRPC->>TRPC: withLogging logs ok (durationMs)
else error
TRPC->>TRPC: withLogging logs error (durationMs) and onError logs unhandled
end
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (9)
🧰 Additional context used📓 Path-based instructions (5)server/**📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,css,json}📄 CodeRabbit inference engine (AGENTS.md)
Files:
app/{page.tsx,api/**}📄 CodeRabbit inference engine (AGENTS.md)
Files:
lib/**📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (9)server/services/headers.ts (2)
server/services/hosting.ts (2)
server/services/dns.ts (2)
app/api/cron/due-drain/route.ts (1)
instrumentation.ts (2)
server/services/certificates.ts (2)
app/api/cron/blob-prune/route.ts (2)
server/services/registration.ts (2)
lib/cache.ts (2)
🔇 Additional comments (24)
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
lib/storage.ts (2)
118-121: Preserve the root cause on failureAttach
causeso upstream handlers and aggregators keep the original stack.- throw new Error( - `Upload failed after ${maxAttempts} attempts: ${lastError?.message ?? "unknown error"}`, - ); + throw new Error( + `Upload failed after ${maxAttempts} attempts: ${lastError?.message ?? "unknown error"}`, + { cause: lastError ?? undefined }, + );
74-79: Consider lowering key cardinality in logsObject keys are high-cardinality. Add a derived field (e.g., digest prefix) and rely on it for aggregation.
- log.debug("storage.upload.attempt", { - key, + log.debug("storage.upload.attempt", { + key, + key_prefix: key.split("/")[0], @@ - log.info("storage.upload.ok", { - key, + log.info("storage.upload.ok", { + key, + key_prefix: key.split("/")[0],Also applies to: 87-91
server/services/pricing.ts (1)
31-33: Optional: log cache hits at debug levelLightweight visibility when Redis already had a fresh payload.
- let payload = await redis.get<DomainPricingResponse>(resultKey); + let payload = await redis.get<DomainPricingResponse>(resultKey); + if (payload) log.debug("pricing.fetch.ok", { cached: true });server/services/ip.ts (2)
23-24: Remove ChatGPT link in code commentsExternal conversational links don’t belong in source; drop to keep comments focused and durable.
- // https://chatgpt.com/s/t_68ed0de1e01881919c2545fe40ffc7ac
19-21: Optional: add a timeout to the fetchAlign with other services using
fetchWithTimeoutto avoid hanging requests.- const res = await fetch(`https://ipwho.is/${encodeURIComponent(ip)}`); + // Consider fetchWithTimeout(...) with a modest timeout and 0-1 retries. + const res = await fetch(`https://ipwho.is/${encodeURIComponent(ip)}`);server/services/registration.ts (1)
14-24: Scope your logger with static bindingsBind a section once to cut repetition and improve query-ability.
-import { logger } from "@/lib/logger"; +import { logger } from "@/lib/logger"; @@ -const log = logger(); +const log = logger({ section: "registration" });Optionally, at function entry do
const reqLog = log.with({ domain });and usereqLogbelow.server/services/seo.ts (3)
13-27: Pre-bind a section for SEO logsBind static context once for consistency and less boilerplate.
-import { logger } from "@/lib/logger"; +import { logger } from "@/lib/logger"; @@ -const log = logger(); +const log = logger({ section: "seo" });
299-317: Harden against SSRF when fetching external images from meta tagsimageUrl is user-controlled. Block non-http(s), IP literals, localhost, and .local hosts before fetching.
- try { - const res = await fetchWithTimeout( + try { + // Basic SSRF guard + let u: URL; + try { + u = new URL(imageUrl); + } catch { + return { url: null }; + } + if (!["http:", "https:"].includes(u.protocol)) return { url: null }; + if (isIpHostname(u.hostname) || u.hostname === "localhost" || u.hostname.endsWith(".local")) { + return { url: null }; + } + + const res = await fetchWithTimeout( imageUrl, { method: "GET", headers: {Add helper in this file:
function isIpHostname(hostname: string): boolean { // IPv4 literal if (/^\d{1,3}(\.\d{1,3}){3}$/.test(hostname)) return true; // IPv6 literal like [::1] if (/^\[.*\]$/.test(hostname)) return true; return false; }
242-248: Consider emitting a cache-hit log for symmetry (optional)A lightweight
seo.cache.hitwhen DB path returns fresh data helps trace behavior.lib/ratelimit.ts (1)
49-56: Avoid logging raw IPs; log a masked form insteadReduce PII exposure while keeping usefulness. Mask last IPv4 octet / compress IPv6.
- log.warn("ratelimit.blocked", { - service, - ip, - limit: res.limit, - remaining: res.remaining, - reset: res.reset, - retryAfterSec, - }); + log.warn("ratelimit.blocked", { + service, + ipMasked: maskIp(ip), + limit: res.limit, + remaining: res.remaining, + reset: res.reset, + retryAfterSec, + });Add helper in this module:
function maskIp(ip: string): string { // IPv4: a.b.c.d -> a.b.c.x if (/^\d{1,3}(\.\d{1,3}){3}$/.test(ip)) { const parts = ip.split("."); parts[3] = "x"; return parts.join("."); } // IPv6: keep first 4 hextets if (ip.includes(":")) { const parts = ip.split(":"); return parts.slice(0, 4).join(":") + "::"; } return "unknown"; }lib/cache.ts (1)
167-208: Consider logging errors in silent catch blocks.The empty catch blocks at lines 167, 202, and 208 silently swallow errors when:
- Reading from cache (line 167)
- Writing to cache/purge queue (line 202)
- Releasing the lock (line 208)
While a fail-safe approach makes sense for availability, silent failures can hide infrastructure issues. Consider adding at least debug-level logging to these blocks for observability.
Apply this diff to add minimal logging:
- } catch {} + } catch (err) { + log.debug("redis.index.read.failed", { indexKey, err }); + } // ... later in the code ... - } catch {} + } catch (err) { + log.warn("redis.cache.store.failed", { indexKey, purgeQueue, err }); + } // ... later in the code ... - } catch {} + } catch (err) { + log.debug("redis.lock.release.failed", { lockKey, err }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
app/api/cron/blob-prune/route.ts(3 hunks)app/api/cron/due-drain/route.ts(2 hunks)app/api/trpc/[trpc]/route.ts(2 hunks)instrumentation.ts(3 hunks)lib/cache.test.ts(0 hunks)lib/cache.ts(5 hunks)lib/logger.ts(1 hunks)lib/r2.ts(2 hunks)lib/ratelimit.ts(2 hunks)lib/schedule.ts(0 hunks)lib/storage.ts(5 hunks)package.json(2 hunks)server/services/certificates.ts(2 hunks)server/services/dns.ts(7 hunks)server/services/favicon.ts(0 hunks)server/services/headers.ts(3 hunks)server/services/hosting.ts(4 hunks)server/services/ip.ts(4 hunks)server/services/pricing.ts(2 hunks)server/services/registration.ts(5 hunks)server/services/screenshot.ts(0 hunks)server/services/seo.ts(3 hunks)trpc/init.ts(3 hunks)
💤 Files with no reviewable changes (4)
- server/services/favicon.ts
- lib/cache.test.ts
- server/services/screenshot.ts
- lib/schedule.ts
🧰 Additional context used
📓 Path-based instructions (6)
lib/**
📄 CodeRabbit inference engine (AGENTS.md)
lib/ holds domain utilities and caching (lib/cache); import these via @/... aliases
Files:
lib/storage.tslib/cache.tslib/logger.tslib/ratelimit.tslib/r2.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: TypeScript only; keep modules small, pure, and roughly ≤300 LOC
Consolidate imports using @/... path aliases
Files:
lib/storage.tsapp/api/trpc/[trpc]/route.tsserver/services/pricing.tsserver/services/registration.tslib/cache.tsserver/services/ip.tsapp/api/cron/blob-prune/route.tsserver/services/seo.tsserver/services/certificates.tsinstrumentation.tsserver/services/dns.tsserver/services/headers.tstrpc/init.tslib/logger.tsserver/services/hosting.tslib/ratelimit.tsapp/api/cron/due-drain/route.tslib/r2.ts
**/*.{ts,tsx,css,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indentation across the codebase
Files:
lib/storage.tsapp/api/trpc/[trpc]/route.tsserver/services/pricing.tsserver/services/registration.tslib/cache.tsserver/services/ip.tsapp/api/cron/blob-prune/route.tsserver/services/seo.tspackage.jsonserver/services/certificates.tsinstrumentation.tsserver/services/dns.tsserver/services/headers.tstrpc/init.tslib/logger.tsserver/services/hosting.tslib/ratelimit.tsapp/api/cron/due-drain/route.tslib/r2.ts
app/{page.tsx,api/**}
📄 CodeRabbit inference engine (AGENTS.md)
Keep app/page.tsx and all app/api/* handlers thin; delegate logic to server/ or lib/
Files:
app/api/trpc/[trpc]/route.tsapp/api/cron/blob-prune/route.tsapp/api/cron/due-drain/route.ts
server/**
📄 CodeRabbit inference engine (AGENTS.md)
server/ hosts backend integrations and tRPC routers; isolate DNS, RDAP/WHOIS, TLS, and header probing services here
Files:
server/services/pricing.tsserver/services/registration.tsserver/services/ip.tsserver/services/seo.tsserver/services/certificates.tsserver/services/dns.tsserver/services/headers.tsserver/services/hosting.ts
package.json
📄 CodeRabbit inference engine (AGENTS.md)
Require Node.js >= 22 via package.json engines
Files:
package.json
🧠 Learnings (1)
📚 Learning: 2025-10-20T02:11:42.179Z
Learnt from: CR
PR: jakejarvis/domainstack.io#0
File: AGENTS.md:0-0
Timestamp: 2025-10-20T02:11:42.179Z
Learning: Applies to server/trpc.ts : Review server/trpc.ts when extending procedures to ensure auth and context remain intact
Applied to files:
trpc/init.ts
🧬 Code graph analysis (17)
lib/storage.ts (1)
lib/logger.ts (1)
logger(112-116)
app/api/trpc/[trpc]/route.ts (1)
lib/logger.ts (1)
logger(112-116)
server/services/pricing.ts (1)
lib/logger.ts (1)
logger(112-116)
server/services/registration.ts (1)
lib/logger.ts (1)
logger(112-116)
lib/cache.ts (2)
lib/logger.ts (1)
logger(112-116)lib/redis.ts (1)
redis(4-4)
server/services/ip.ts (1)
lib/logger.ts (1)
logger(112-116)
app/api/cron/blob-prune/route.ts (1)
lib/logger.ts (1)
logger(112-116)
server/services/seo.ts (2)
lib/logger.ts (1)
logger(112-116)lib/schemas/domain/seo.ts (1)
SeoResponse(94-94)
server/services/certificates.ts (1)
lib/logger.ts (1)
logger(112-116)
instrumentation.ts (1)
lib/logger.ts (1)
logger(112-116)
server/services/dns.ts (2)
lib/logger.ts (1)
logger(112-116)lib/schemas/domain/dns.ts (1)
DnsResolveResult(24-24)
server/services/headers.ts (1)
lib/logger.ts (1)
logger(112-116)
trpc/init.ts (1)
lib/logger.ts (1)
createRequestLogger(118-134)
server/services/hosting.ts (1)
lib/logger.ts (1)
logger(112-116)
lib/ratelimit.ts (1)
lib/logger.ts (1)
logger(112-116)
app/api/cron/due-drain/route.ts (1)
lib/logger.ts (1)
logger(112-116)
lib/r2.ts (1)
lib/logger.ts (1)
logger(112-116)
🔇 Additional comments (25)
package.json (2)
118-120: Engines constraint matches guidelineNode >= 22 is set; aligns with repo guidance.
69-71: No issues found — Pino is correctly gated to Node runtime onlyThe code in
lib/logger.tsalready implements the exact pattern the review requests:
- Line 59: Pino imported dynamically inside
getPinoRoot()(not top-level)- Line 111: Public
logger()function gates viaRUNTIME === "node"check- Edge/Browser path: Uses
makeConsoleLogger(console-based, zero Pino dependency)No top-level Pino imports exist. Bundlers can safely tree-shake the library from Edge/Browser builds.
server/services/dns.ts (1)
59-59: LGTM on start logConsistent event naming; good entry point signal.
lib/storage.ts (1)
74-79: LGTM on structured retry telemetryVerified: the logger uses
pino.default.stdSerializers.err(line 80 of lib/logger.ts), which is Pino's standard error serializer that includes stack traces. Error serialization is correctly configured for production.app/api/trpc/[trpc]/route.ts (1)
37-38: LGTM! Error logging is correctly implemented.The structured logging with event name and error payload follows the centralized logging pattern introduced in this PR.
app/api/cron/due-drain/route.ts (2)
7-7: LGTM! Logger instance correctly initialized at module level.Creating the logger instance at module scope ensures it's shared across invocations efficiently.
57-66: LGTM! Structured logging with appropriate levels.The logging correctly distinguishes between warnings (cleanup failures) and info (successful completion) with relevant contextual data.
lib/r2.ts (2)
11-11: LGTM! Logger instance correctly initialized at module level.The module-level logger instance enables efficient reuse across function calls.
174-177: LGTM! Error logging with appropriate context.The structured error log includes both the failed keys and the error object, providing sufficient context for debugging S3 deletion failures.
instrumentation.ts (2)
4-13: LGTM! Process-level error hooks correctly implemented.The conditional check ensures these handlers only register in Node.js environments, preventing issues in Edge runtime. The scoped logger instance is appropriate for these global error handlers.
44-59: LGTM! Graceful error handling with structured logging.The error logging correctly handles both PostHog cookie parsing failures and overall instrumentation failures without throwing, ensuring request processing continues.
server/services/certificates.ts (2)
16-19: LGTM! Logger initialization and debug logging.The module-level logger instance and debug logging at function entry provide good observability for certificate fetching operations.
158-174: LGTM! Appropriate logging levels for different outcomes.The logging correctly uses:
warnfor non-fatal schedule failures (line 158)infofor successful certificate fetch (line 165)warnfor TLS errors with graceful degradation (line 171)The consistent use of the
errkey in payloads maintains structured logging standards.app/api/cron/blob-prune/route.ts (2)
5-5: LGTM! Logger instance correctly initialized at module level.The module-level logger enables efficient reuse across cron invocations.
32-42: LGTM! Differentiated logging for partial vs. complete success.The conditional logging correctly distinguishes between operations that completed with errors vs. clean success, providing appropriate operational visibility.
trpc/init.ts (3)
16-28: Excellent request-scoped logger implementation.The request-scoped logger captures essential request metadata (IP, method, path, requestId, vercelId) and makes it available throughout the request lifecycle via context. This enables correlation of logs across middleware and procedures.
70-90: LGTM! Well-structured logging middleware.The middleware correctly:
- Logs RPC start at debug level
- Captures duration for both success and error paths
- Logs success at info level with metrics
- Logs errors without swallowing them
- Uses consistent event naming (rpc.start, rpc.ok, rpc.error)
92-92: All procedures are compatible with the updated context.Verification confirms that
publicProcedureis used exclusively inserver/routers/domain.tsacross 9 procedures (registration, pricing, dns, hosting, certificates, headers, seo, favicon, screenshot). The context object returned bycreateContextincludes thelogproperty initialized viacreateRequestLogger(), and all middleware in the chain—including the newwithLoggingmiddleware—access only existing context properties (ctx.log,ctx.ip). Procedure handlers receive only theinputparameter and don't directly interact with context, ensuring compatibility across the board.server/services/hosting.ts (3)
26-29: LGTM! Logger initialization and debug logging.The module-level logger instance and debug logging at function entry provide observability for hosting detection operations.
122-127: LGTM! Cache hit logging with relevant context.The info-level log for cache hits includes essential provider information (hosting, email, DNS) for operational visibility.
250-255: LGTM! Success logging with provider details.The completion log captures all detected providers, providing good observability for hosting detection operations.
lib/cache.ts (4)
1-5: LGTM! Logger integration follows the new pattern.The import and initialization align with the codebase's shift to structured logging. The logger is initialized without bindings, which is appropriate for a general-purpose cache utility.
53-66: LGTM! Structured logging implementation is clean and consistent.The log calls use descriptive message keys and follow common conventions (snake_case fields like
wait_msanderr). The structured approach will improve observability.Also applies to: 81-86, 110-114, 120-125
130-147: Breaking change verified as complete.The removal of
eventNameandbaseMetricsfromCachedAssetOptionswas properly handled. Both production call sites (server/services/screenshot.tsandserver/services/favicon.ts) have been updated to use the new API signature, and no references to the removed fields remain in the codebase.
93-108: Add test coverage for lock disappearance scenario.The behavior when a lock disappears during polling is intentional (documented in comments) and the NX flag prevents actual race conditions—only one process acquires the lock per attempt. However, this critical failure-recovery scenario lacks test coverage.
Add a test to
lib/cache.test.tsthat verifies the waiter can take over when the lock holder fails:it("reacquires lock when it disappears during polling", async () => { const indexKey = ns("test", "asset5"); const lockKey = ns("lock", "test", "asset5"); const { redis } = await import("@/lib/redis"); // Simulate worker acquiring lock but failing await redis.set(lockKey, "1"); // Simulate lock disappearing (worker crashed, lock expired) await redis.del(lockKey); const result = await getOrCreateCachedAsset<{ source: string }>({ indexKey, lockKey, ttlSeconds: 60, produceAndUpload: async () => ({ url: "https://cdn/recovery.webp", key: "k", metrics: { source: "recovery" }, }), }); expect(result.url).toBe("https://cdn/recovery.webp"); });
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (7)
server/services/ip.ts (1)
63-63: Avoid logging the full ipwho.is JSON (PII and high cardinality).The full JSON response is still being logged, which may expose PII and creates high-cardinality data. This concern was previously raised and remains unresolved.
Consider logging only essential, non-sensitive fields:
- log.debug("ipwhois.result", { ip, json: j }); + log.debug("ipwhois.result", { + ip, + country_code: j.country_code, + city: j.city, + org: j.connection?.org, + isp: j.connection?.isp, + tz: j.timezone?.id, + });server/services/pricing.ts (1)
57-62: Include TLD and provider in logs for actionability.The logs lack contextual fields that would help diagnose issues. The
tldvariable (line 25) and provider name are in scope but not included.Apply this diff to enrich log context:
payload = (await res.json()) as DomainPricingResponse; await redis.set(resultKey, payload, { ex: 7 * 24 * 60 * 60 }); - log.info("fetch.ok", { cached: false }); + log.info("fetch.ok", { cached: false, tld, provider: "porkbun" }); } else { - log.error("upstream.error", { status: res.status }); + log.error("upstream.error", { status: res.status, tld, provider: "porkbun" }); } } catch (err) { - log.error("fetch.error", { err }); + log.error("fetch.error", { err, tld, provider: "porkbun" }); }server/services/dns.ts (3)
241-245: Include provider duration in partial refresh logs.The
durationByProvideris computed (line 161) but not logged, which would be valuable for SLO monitoring and regression detection.Apply this diff:
log.info("ok.partial", { domain: registrable ?? domain, counts, resolver: pinnedProvider.key, + duration_ms: durationByProvider[pinnedProvider.key], });
337-341: Include all provider durations in full refresh logs.The
durationByProviderobject (line 271) contains timing for all attempted providers and should be logged for observability.Apply this diff:
log.info("ok", { domain: registrable ?? domain, counts, resolver: resolverUsed, + durations_ms: durationByProvider, });
251-255: Normalize error objects for consistent stack traces.Unknown errors should be wrapped to ensure logs always contain a
messageandstack.Apply this diff to the three catch blocks:
} catch (err) { log.warn("partial.refresh.failed", { domain: registrable ?? domain, provider: pinnedProvider.key, - err, + err: err instanceof Error ? err : new Error(String(err)), }); // Fall through to full provider loop below }} catch (err) { log.warn("provider.attempt.failed", { domain: registrable ?? domain, provider: provider.key, - err, + err: err instanceof Error ? err : new Error(String(err)), });log.error("all.providers.failed", { domain: registrable ?? domain, providers: providers.map((p) => p.key), - err: lastError, + err: lastError instanceof Error ? lastError : new Error(String(lastError)), });Also applies to: 344-348, 356-360
server/services/registration.ts (2)
130-132: Avoid logging full RDAP payload; redact contacts/emails to reduce PII risk.As noted in a previous review, RDAP records can include registrant/admin contact data (emails, names, addresses). Even if publicly available, logging this data may create compliance obligations under GDPR/CCPA. Consider implementing a
redactRdaphelper that logs only a whitelisted subset of fields (domain, isRegistered, expirationDate, registrar.name/url, limited statuses/nameservers) and drops contacts, emails, and handles.Based on learnings
204-207: Include a short error message alongside the Error for searchability.As mentioned in a previous review, while logging the
errobject captures the stack trace, adding a briefreasonfield improves searchability in log aggregation tools.Apply this diff:
log.warn("schedule.failed", { domain: registrable ?? domain, + reason: "scheduleSectionIfEarlier threw", err, });Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
app/api/cron/blob-prune/route.ts(3 hunks)app/api/cron/due-drain/route.ts(2 hunks)app/api/trpc/[trpc]/route.ts(2 hunks)instrumentation.ts(3 hunks)lib/cache.ts(6 hunks)lib/logger.ts(1 hunks)lib/r2.ts(2 hunks)lib/ratelimit.ts(2 hunks)lib/storage.ts(5 hunks)package.json(2 hunks)server/services/certificates.ts(2 hunks)server/services/dns.ts(7 hunks)server/services/headers.ts(3 hunks)server/services/hosting.ts(4 hunks)server/services/ip.ts(4 hunks)server/services/pricing.ts(2 hunks)server/services/registration.ts(5 hunks)server/services/seo.ts(3 hunks)trpc/init.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
lib/**
📄 CodeRabbit inference engine (AGENTS.md)
lib/ holds domain utilities and caching (lib/cache); import these via @/... aliases
Files:
lib/storage.tslib/ratelimit.tslib/cache.tslib/r2.tslib/logger.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: TypeScript only; keep modules small, pure, and roughly ≤300 LOC
Consolidate imports using @/... path aliases
Files:
lib/storage.tsapp/api/trpc/[trpc]/route.tstrpc/init.tsserver/services/seo.tsapp/api/cron/blob-prune/route.tsinstrumentation.tslib/ratelimit.tsserver/services/hosting.tsserver/services/dns.tsapp/api/cron/due-drain/route.tslib/cache.tsserver/services/registration.tsserver/services/pricing.tslib/r2.tslib/logger.tsserver/services/certificates.tsserver/services/ip.tsserver/services/headers.ts
**/*.{ts,tsx,css,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indentation across the codebase
Files:
lib/storage.tsapp/api/trpc/[trpc]/route.tstrpc/init.tsserver/services/seo.tspackage.jsonapp/api/cron/blob-prune/route.tsinstrumentation.tslib/ratelimit.tsserver/services/hosting.tsserver/services/dns.tsapp/api/cron/due-drain/route.tslib/cache.tsserver/services/registration.tsserver/services/pricing.tslib/r2.tslib/logger.tsserver/services/certificates.tsserver/services/ip.tsserver/services/headers.ts
app/{page.tsx,api/**}
📄 CodeRabbit inference engine (AGENTS.md)
Keep app/page.tsx and all app/api/* handlers thin; delegate logic to server/ or lib/
Files:
app/api/trpc/[trpc]/route.tsapp/api/cron/blob-prune/route.tsapp/api/cron/due-drain/route.ts
server/**
📄 CodeRabbit inference engine (AGENTS.md)
server/ hosts backend integrations and tRPC routers; isolate DNS, RDAP/WHOIS, TLS, and header probing services here
Files:
server/services/seo.tsserver/services/hosting.tsserver/services/dns.tsserver/services/registration.tsserver/services/pricing.tsserver/services/certificates.tsserver/services/ip.tsserver/services/headers.ts
package.json
📄 CodeRabbit inference engine (AGENTS.md)
Require Node.js >= 22 via package.json engines
Files:
package.json
🧠 Learnings (1)
📚 Learning: 2025-10-20T02:11:42.179Z
Learnt from: CR
PR: jakejarvis/domainstack.io#0
File: AGENTS.md:0-0
Timestamp: 2025-10-20T02:11:42.179Z
Learning: Applies to server/trpc.ts : Review server/trpc.ts when extending procedures to ensure auth and context remain intact
Applied to files:
trpc/init.ts
🧬 Code graph analysis (17)
lib/storage.ts (1)
lib/logger.ts (1)
logger(120-124)
app/api/trpc/[trpc]/route.ts (1)
lib/logger.ts (1)
logger(120-124)
trpc/init.ts (1)
lib/logger.ts (1)
createRequestLogger(126-142)
server/services/seo.ts (2)
lib/logger.ts (1)
logger(120-124)lib/schemas/domain/seo.ts (1)
SeoResponse(94-94)
app/api/cron/blob-prune/route.ts (1)
lib/logger.ts (1)
logger(120-124)
instrumentation.ts (1)
lib/logger.ts (1)
logger(120-124)
lib/ratelimit.ts (1)
lib/logger.ts (1)
logger(120-124)
server/services/hosting.ts (1)
lib/logger.ts (1)
logger(120-124)
server/services/dns.ts (2)
lib/logger.ts (1)
logger(120-124)lib/schemas/domain/dns.ts (1)
DnsResolveResult(24-24)
app/api/cron/due-drain/route.ts (1)
lib/logger.ts (1)
logger(120-124)
lib/cache.ts (2)
lib/logger.ts (1)
logger(120-124)lib/redis.ts (1)
redis(4-4)
server/services/registration.ts (2)
lib/logger.ts (1)
logger(120-124)lib/schemas/domain/registration.ts (1)
Registration(97-97)
server/services/pricing.ts (1)
lib/logger.ts (1)
logger(120-124)
lib/r2.ts (1)
lib/logger.ts (1)
logger(120-124)
server/services/certificates.ts (2)
lib/logger.ts (1)
logger(120-124)lib/schemas/domain/certificates.ts (1)
Certificate(15-15)
server/services/ip.ts (1)
lib/logger.ts (1)
logger(120-124)
server/services/headers.ts (2)
lib/logger.ts (1)
logger(120-124)lib/schemas/domain/http.ts (1)
HttpHeader(10-10)
🔇 Additional comments (33)
package.json (1)
69-69: LGTM! Pino dependencies are correctly placed.The pino logger is correctly added to production dependencies, and pino-pretty is appropriately placed in devDependencies for development-only pretty printing. This aligns with the unified logging strategy introduced across the codebase.
Also applies to: 108-108
app/api/trpc/[trpc]/route.ts (1)
2-2: LGTM! Centralized error logging with proper context.The logger is correctly initialized with a module identifier, and the
onErrorhandler now logs unhandled errors with structured data including the path and error object. This aligns with the unified logging strategy across the codebase.Also applies to: 37-38
lib/storage.ts (3)
4-4: LGTM! Module-scoped logger correctly implemented.The logger is properly initialized with module context at the file scope, addressing the past review concern. All log calls now automatically include the
module: "storage"binding, making filtering and observability straightforward.Also applies to: 8-8
74-78: LGTM! Structured logging follows best practices.The logging implementation correctly uses:
log.debugfor retry attempts with relevant contextlog.infofor successful operationslog.warnfor transient failures with error details- Consistent field naming (
delay_ms,err)Also applies to: 87-90, 96-100, 109-112
118-120: LGTM! Error chaining with cause property.Using the
causeproperty to chain errors is a best practice that preserves the original error context for debugging and tracing.server/services/ip.ts (2)
1-3: LGTM! Module-scoped logger correctly initialized.The logger is properly initialized with
module: "ip"context, resolving the past review concern about missing module binding.
85-86: LGTM! Error details now captured in logs.The catch block now captures and logs the error object, resolving the past review concern about swallowed exceptions.
lib/r2.ts (2)
9-11: LGTM! Module-scoped logger correctly initialized.The logger is properly initialized with
module: "r2"context, following the consistent pattern across the codebase.
172-177: LGTM! Error logging with appropriate context.The error handler correctly logs the failed batch (slice) and the error object, providing sufficient context for debugging while avoiding excessive logging on the success path.
app/api/cron/due-drain/route.ts (2)
3-7: LGTM! Module-scoped logger with clear naming.The logger is correctly initialized with
module: "cron:due-drain", using a clear naming convention that identifies both the category and specific cron job.
57-66: LGTM! Comprehensive operational logging.The logging strategy correctly captures:
- Transient cleanup failures with
log.warn- Success metrics (emitted events, groups, duration)
- Critical cron failures with
log.errorAll error logs include the error object for debugging.
Also applies to: 75-75
app/api/cron/blob-prune/route.ts (2)
2-5: LGTM! Module-scoped logger with clear naming.The logger is correctly initialized with
module: "cron:blob-prune", maintaining consistency with the cron naming convention.
32-42: LGTM! Intelligent operational logging.The logging strategy correctly differentiates between:
- Partial success (completed with errors) using
log.warn- Clean success using
log.info- Critical failures using
log.errorAll paths include relevant metrics (deletedCount, errorCount, duration_ms).
Also applies to: 51-51
server/services/hosting.ts (2)
14-26: LGTM! Module-scoped logger correctly initialized.The logger is properly initialized with
module: "hosting"context, following the consistent pattern across the codebase.
29-29: LGTM! Clean structured logging for hosting detection.The logging strategy appropriately captures:
- Detection start with
log.debug- Cache hits with provider details using
log.info- Successful detection with provider details using
log.infoThe removal of duration tracking and analytics side-effects aligns with the unified logging strategy.
Also applies to: 122-127, 250-255
lib/ratelimit.ts (1)
10-10: LGTM—clean logger integration.The rate-limit logging is well-structured with all relevant fields (service, IP, limits, retry timing) and properly scoped with module binding.
Also applies to: 49-56
server/services/seo.ts (1)
26-26: LGTM—comprehensive SEO logging.The logger integration is clean with appropriate module binding and rich context (status, metadata presence, errors). The removal of analytics timing in favor of structured logging aligns with the PR objectives.
Also applies to: 32-32, 242-248
trpc/init.ts (1)
28-28: All context consumers are compatible with the newlogfield.Verification confirms that the context augmentation is fully backward-compatible:
- The
withLoggingmiddleware (line 70) correctly accesses the newctx.logfield via.debug(),.info(), and.error()methods.- The
rateLimitMiddleware(lib/ratelimit.ts:79) accesses the existingctx.ipfield (line 83:!ctx.ip, line 84:ctx.ipusage), which remains unchanged in the new context shape.- All 9 procedures in
server/routers/domain.tsuse the middleware chain (publicProcedure→rateLimitMiddleware) and don't directly destructurectx—they receive only{ input }.- No other context property accesses found that would be affected by the structural change.
server/services/registration.ts (1)
14-14: LGTM: Logger initialization follows the established pattern.The import and module-scoped logger creation align with the unified logging strategy across the codebase.
Also applies to: 23-23
lib/cache.ts (7)
1-1: LGTM: Logger initialization consistent with codebase patterns.Also applies to: 4-4
53-53: LGTM: Lock acquisition logging is well-structured.The log levels are appropriate (debug for success/waiting, warn for failures), and all relevant context is captured.
Also applies to: 57-66
81-86: LGTM: Cache hit logging with appropriate timing metrics.The use of
wait_msfollows structured logging conventions (snake_case for field names), improving compatibility with log aggregation tools.
130-147: LGTM: Type signature simplified by removing analytics-related fields.The removal of
eventNameandbaseMetricsaligns with the migration from analytics capture to structured logging.
167-169: LGTM: Index read failure logged at appropriate debug level.Since this failure has a safe fallback (acquiring the lock and regenerating), debug level is appropriate.
204-206: LGTM: Cache store failure appropriately logged.The warn level is correct—this degrades performance but doesn't affect correctness, as the produced result is still returned.
212-214: LGTM: Lock release failure logged defensively.Debug level is appropriate since locks have TTL and will expire automatically. This captures the error without alarming.
lib/logger.ts (7)
1-15: LGTM: Well-designed logger interface with clear types.The synchronous facade (
voidreturns) is pragmatic for Node's async Pino, and thewith()method enables elegant binding composition.
17-22: LGTM: Runtime detection logic is robust.The detection order (browser → edge → node) correctly handles all target environments.
24-28: LGTM: Safe environment access compatible with Edge runtime.The use of
globalThis?.process?.env ?? {}safely handles environments whereprocessis undefined, addressing the previous Edge runtime concern.Based on learnings
31-45: LGTM: Console fallback logger provides consistent structured output.The implementation correctly merges bindings, safely handles missing console methods, and maintains the same API as the Node logger.
89-117: LGTM: Child logger caching implemented correctly.The
cachedChildvariable prevents repeated allocations on each log call, addressing the previous performance concern. Each logger instance has its own cache, andwith()correctly creates fresh instances.Based on learnings
119-124: LGTM: Public API provides unified interface across runtimes.
126-142: LGTM: Request logger helper provides consistent request context.The conversion of
nulltoundefinedforvercelIdpreventsnullfrom appearing in logs, maintaining cleaner output.
…rror handling and improve dynamic imports for logging
…prove duration metrics
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
server/services/headers.ts (1)
77-85: Don't swallow scheduling errors; log them.Silent catch drops useful signals. Emit a warn with a short reason plus err.
Apply this diff:
- try { + try { const dueAtMs = ttlForHeaders(now).getTime(); await scheduleSectionIfEarlier( "headers", registrable ?? domain, dueAtMs, ); - } catch {} + } catch (err) { + log.warn("schedule.failed", { + domain: registrable ?? domain, + reason: "scheduleSectionIfEarlier threw", + err: err instanceof Error ? err : new Error(String(err)), + }); + }app/api/cron/blob-prune/route.ts (2)
16-25: Add logs for misconfiguration and unauthorized attempts.Emit structured logs for these branches to aid ops visibility.
Apply this diff:
if (!expectedAuth) { + log.error("cron.misconfigured", { reason: "CRON_SECRET missing" }); return NextResponse.json( { error: "CRON_SECRET not configured" }, { status: 500 }, ); } if (authHeader !== expectedAuth) { + log.warn("cron.unauthorized", { provided: Boolean(authHeader) }); return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); }
28-41: Compute duration once; reuse for logs and response.Removes repetition and ensures consistent values.
Apply this diff:
try { const startedAt = Date.now(); const result = await pruneDueBlobsOnce(startedAt); + const durationMs = Date.now() - startedAt; if (result.errorCount > 0) { log.warn("completed.with.errors", { deletedCount: result.deletedCount, errorCount: result.errorCount, - durationMs: Date.now() - startedAt, + durationMs, }); } else { log.info("completed", { deletedCount: result.deletedCount, - durationMs: Date.now() - startedAt, + durationMs, }); } return NextResponse.json({ success: true, deletedCount: result.deletedCount, errorCount: result.errorCount, - durationMs: Date.now() - startedAt, + durationMs, });Also applies to: 44-49
instrumentation.ts (1)
39-56: Fix Fetch Request usage: use headers.get("cookie") and URL pathname.Request.headers.cookie and Request.path are undefined in the Web Fetch API; this breaks cookie extraction and path logging.
Apply this diff:
- let distinctId = null; - if (request.headers.cookie) { - const cookieString = request.headers.cookie; + let distinctId: string | null = null; + const cookieString = request.headers.get("cookie"); + if (cookieString) { const postHogCookieMatch = - typeof cookieString === "string" - ? cookieString.match(/ph_phc_.*?_posthog=([^;]+)/) - : null; + cookieString.match(/ph_phc_.*?_posthog=([^;]+)/); @@ - phClient.captureException(err, distinctId || undefined, { - path: request.path, + phClient.captureException(err, distinctId || undefined, { + path: (() => { + try { return new URL(request.url).pathname; } catch { return undefined; } + })(), method: request.method, });Also applies to: 58-61
♻️ Duplicate comments (10)
server/services/headers.ts (2)
45-47: Good fix: always log a non-null domain.Using registrable ?? domain avoids nulls in logs and improves queryability.
93-96: Error level + normalized Error looks good.Logging at error and normalizing err ensures consistent serialization and stacks.
server/services/registration.ts (2)
131-135: Redact RDAP payload to minimize PII in logs.RDAP may include contacts/emails. Log a whitelisted subset instead.
Apply this diff:
- log.debug("rdapper.result", { - ...record, - }); + log.debug("rdapper.result", redactRdap(record));Add within this module (e.g., below getRegistration):
function redactRdap(record: Record<string, any>) { return { domain: record.domain, isRegistered: record.isRegistered, expirationDate: record.expirationDate ?? null, registrar: { name: record.registrar?.name ?? null, url: record.registrar?.url ?? null, }, statuses: Array.isArray(record.statuses) ? record.statuses.slice(0, 5) : undefined, nameservers: Array.isArray(record.nameservers) ? record.nameservers.slice(0, 5).map((n: any) => ({ host: n.host })) : undefined, }; }
206-209: Add a short reason string to improve log queries.Keep err, but include a concise reason for searchability.
Apply this diff:
- log.warn("schedule.failed", { - domain: registrable ?? domain, - err, - }); + log.warn("schedule.failed", { + domain: registrable ?? domain, + reason: "scheduleSectionIfEarlier threw", + err, + });lib/logger.ts (3)
70-77: Defensively populate base context fields.Avoid undefined values; add sensible defaults or omit keys.
Apply this diff:
nodeRoot = pino.default({ level: defaultLevel, base: { app: "domainstack", - env: env.NODE_ENV, - commit: env.VERCEL_GIT_COMMIT_SHA, - region: env.VERCEL_REGION, + env: env.NODE_ENV ?? "development", + ...(env.VERCEL_GIT_COMMIT_SHA ? { commit: env.VERCEL_GIT_COMMIT_SHA } : {}), + ...(env.VERCEL_REGION ? { region: env.VERCEL_REGION } : {}), },
119-124: Optional: pre-warm Pino import in Node.Eliminates first-log import latency.
Apply this diff:
export function logger(bindings?: LogFields): Logger { return RUNTIME === "node" ? makeNodeLogger(bindings) : makeConsoleLogger(bindings); } + +// Pre-warm in Node so the first log doesn't await a dynamic import. +if (RUNTIME === "node") { + void getPinoRoot(); +}
24-29: Type-safe env access for Edge/TS.globalThis?.process?.env without a cast can fail TS checks; cast globalThis to any and type env.
Apply this diff:
-// Safe env access for Edge compatibility -const env = globalThis?.process?.env ?? {}; +// Safe env access for Edge compatibility (typed) +const env = ((globalThis as any)?.process?.env ?? + {}) as Record<string, string | undefined>; const isProd = env.NODE_ENV === "production"; const defaultLevel = (env.LOG_LEVEL as Level | undefined) ?? (isProd ? "info" : "debug");lib/storage.ts (1)
4-9: Adopts module-scoped logger + consistent retry logging — looks good.Nice use of cause and normalized error; this addresses prior feedback.
Also applies to: 74-79, 87-91, 96-101, 109-113, 118-121
server/services/certificates.ts (1)
11-17: Logger integration and error normalization — good upgrade.Per-module binding and normalized err values look correct.
Also applies to: 158-161, 171-174
trpc/init.ts (1)
70-90: Middleware logging looks correct and resilient.Start/ok/error with durationMs and normalized errors — solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
app/api/cron/blob-prune/route.ts(2 hunks)app/api/cron/due-drain/route.ts(2 hunks)instrumentation.ts(3 hunks)lib/cache.ts(6 hunks)lib/logger.ts(1 hunks)lib/storage.ts(6 hunks)server/services/certificates.ts(2 hunks)server/services/headers.ts(3 hunks)server/services/registration.ts(5 hunks)trpc/init.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
server/**
📄 CodeRabbit inference engine (AGENTS.md)
server/ hosts backend integrations and tRPC routers; isolate DNS, RDAP/WHOIS, TLS, and header probing services here
Files:
server/services/certificates.tsserver/services/headers.tsserver/services/registration.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: TypeScript only; keep modules small, pure, and roughly ≤300 LOC
Consolidate imports using @/... path aliases
Files:
server/services/certificates.tsserver/services/headers.tslib/cache.tslib/storage.tsapp/api/cron/due-drain/route.tslib/logger.tsinstrumentation.tsapp/api/cron/blob-prune/route.tstrpc/init.tsserver/services/registration.ts
**/*.{ts,tsx,css,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indentation across the codebase
Files:
server/services/certificates.tsserver/services/headers.tslib/cache.tslib/storage.tsapp/api/cron/due-drain/route.tslib/logger.tsinstrumentation.tsapp/api/cron/blob-prune/route.tstrpc/init.tsserver/services/registration.ts
lib/**
📄 CodeRabbit inference engine (AGENTS.md)
lib/ holds domain utilities and caching (lib/cache); import these via @/... aliases
Files:
lib/cache.tslib/storage.tslib/logger.ts
app/{page.tsx,api/**}
📄 CodeRabbit inference engine (AGENTS.md)
Keep app/page.tsx and all app/api/* handlers thin; delegate logic to server/ or lib/
Files:
app/api/cron/due-drain/route.tsapp/api/cron/blob-prune/route.ts
🧠 Learnings (1)
📚 Learning: 2025-10-20T02:11:42.179Z
Learnt from: CR
PR: jakejarvis/domainstack.io#0
File: AGENTS.md:0-0
Timestamp: 2025-10-20T02:11:42.179Z
Learning: Applies to server/trpc.ts : Review server/trpc.ts when extending procedures to ensure auth and context remain intact
Applied to files:
trpc/init.ts
🧬 Code graph analysis (9)
server/services/certificates.ts (2)
lib/logger.ts (1)
logger(120-124)lib/schemas/domain/certificates.ts (1)
Certificate(15-15)
server/services/headers.ts (1)
lib/logger.ts (1)
logger(120-124)
lib/cache.ts (2)
lib/logger.ts (1)
logger(120-124)lib/redis.ts (1)
redis(4-4)
lib/storage.ts (1)
lib/logger.ts (1)
logger(120-124)
app/api/cron/due-drain/route.ts (1)
lib/logger.ts (1)
logger(120-124)
instrumentation.ts (1)
lib/logger.ts (1)
logger(120-124)
app/api/cron/blob-prune/route.ts (1)
lib/logger.ts (1)
logger(120-124)
trpc/init.ts (1)
lib/logger.ts (1)
createRequestLogger(126-140)
server/services/registration.ts (2)
lib/logger.ts (1)
logger(120-124)lib/schemas/domain/registration.ts (1)
Registration(97-97)
🔇 Additional comments (9)
server/services/headers.ts (1)
14-14: LGTM on pre-bound module logger.Using logger({ module: "headers" }) ensures consistent context across events.
instrumentation.ts (1)
11-18: Good: process-level handlers with normalized rejection reason.Normalization guarantees stack traces; handlers are Node-guarded.
server/services/registration.ts (1)
120-129: Correct error level and thrown Error with domain context.Good upgrade: structured error logging and a domain-specific message.
lib/logger.ts (1)
89-117: Good: cache Pino child per instance.Prevents per-call child creation; reduces allocations and lock contention.
app/api/cron/due-drain/route.ts (2)
7-8: LGTM on structured, module-scoped logging.Good module binding and clear event names (cron.ok/failed).
Also applies to: 62-66
68-73: This review comment is incorrect; ignore the suggestion.The recent commit (a8199b0) already standardized
duration_ms→durationMsthroughout the codebase as part of logging normalization. Since these cron endpoints are internal infrastructure with no external consumers, there is no breaking change concern. Adding a deprecated snake_case field contradicts the standardization goal.Likely an incorrect or invalid review comment.
trpc/init.ts (2)
16-26: LGTM on request-scoped logger fields.Capturing path/method/ip/requestId/vercelId up front is clean.
20-29: Review comment does not apply to current code — protectedProcedure does not exist.The review raises concerns about protecting procedures, but
protectedProcedureis not defined anywhere in the codebase. OnlypublicProcedureexists (line 92) and it correctly chains thewithLoggingmiddleware. The context (including the log created at lines 20-29) is properly preserved through the middleware chain vianext(). The underlying requirement—that auth and context remain intact—is already satisfied by the current implementation.Likely an incorrect or invalid review comment.
lib/cache.ts (1)
4-5: Good observability upgrade.Module-scoped logger and clearer event names improve traceability.
Also applies to: 152-154
…ing consistent error messages and improved duration metrics.
Summary by CodeRabbit
New Features
Bug Fixes
Chores