Conversation
Signed-off-by: Marcos Candeia <marrcooos@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds per-request dirty tracking (proxied request/headers and Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant Runtime
participant CacheLogic
participant Renderer
Client->>Middleware: HTTP request
Middleware->>Runtime: prepareState(req) (uses proxied request/headers)
Runtime->>Runtime: headers.get/has('cookie') -> set state.dirty = true, push trace
Runtime-->>Middleware: response + state
Middleware->>CacheLogic: evaluate caching (state.dirty, Set-Cookie, PAGE_CACHE_DRY_RUN)
alt dirty or Set-Cookie
CacheLogic-->>Middleware: mark not-cacheable / no-store
else
alt PAGE_CACHE_DRY_RUN enabled
CacheLogic-->>Middleware: log cacheable (dry-run)
else
CacheLogic-->>Middleware: set Cache-Control (public, max-age=120)
end
end
Middleware->>Renderer: render page (may call singleFlight.do(key) if enabled)
Renderer-->>Middleware: rendered HTML
Middleware->>Client: send response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Tagging OptionsShould a new tag be published when this PR is merged?
|
Signed-off-by: Marcos Candeia <marrcooos@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
runtime/mod.ts (1)
171-237: Preserve existingdirtywhen a base state is supplied.
Line 236 unconditionally resetsdirtyeven ifcontext.basealready had it set. Consider a nullish default to avoid clobbering upstream state.♻️ Suggested change
- state.dirty = false; + state.dirty ??= false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/mod.ts` around lines 171 - 237, The prepareState function currently overwrites state.dirty unconditionally, clobbering any existing dirty value from a provided context.base; update the assignment for state.dirty so it preserves an existing value when present (e.g., use a nullish default or conditional set) instead of always setting to false—locate the state.dirty assignment in prepareState and change it to something like state.dirty = state.dirty ?? false or only set when state.dirty is undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@runtime/middleware.ts`:
- Around line 447-464: The code branch that checks ctx.var.dirty doesn't set
Cache-Control when dirty and PAGE_CACHE_DRY_RUN is false, allowing CDNs to cache
responses; update the branch handling dirty responses (alongside getSetCookies,
newHeaders, PAGE_CACHE_DRY_RUN, and url.pathname) so that when ctx.var.dirty is
true and PAGE_CACHE_DRY_RUN is false you explicitly set
newHeaders.set("Cache-Control", "no-store, no-cache, must-revalidate") to ensure
dirty responses are not cached, keeping the existing console.warn behavior only
for dry-run mode.
In `@runtime/routes/render.tsx`:
- Around line 94-116: The single-flight key uses stableStringify(props) but
props.url is a URL object (stableStringify turns URL into {}), so different URLs
collapse; update the key generation to use a string URL by creating a
key-specific object (e.g., derive propsForKey or replace props.url) where url is
String(ctx.var.url) or ctx.var.url.toString() before calling stableStringify,
then pass that string to AsyncRenderSF.do(propsString, renderFn) while keeping
the original props (with the URL object) for render; reference props,
propsString, stableStringify, renderFn, AsyncRenderSF.do, and
SHOULD_USE_ASYNC_RENDER_SINGLE_FLIGHT.
---
Nitpick comments:
In `@runtime/mod.ts`:
- Around line 171-237: The prepareState function currently overwrites
state.dirty unconditionally, clobbering any existing dirty value from a provided
context.base; update the assignment for state.dirty so it preserves an existing
value when present (e.g., use a nullish default or conditional set) instead of
always setting to false—locate the state.dirty assignment in prepareState and
change it to something like state.dirty = state.dirty ?? false or only set when
state.dirty is undefined.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="runtime/routes/render.tsx">
<violation number="1" location="runtime/routes/render.tsx:113">
P1: `stableStringify` serializes `URL` objects as `{}`, so the single-flight key will be identical for requests that differ only in URL (e.g., different query parameters). This can cause concurrent renders for different pages to be collapsed into one, serving the wrong response. Include the URL string explicitly in the key, e.g., `stableStringify({ ...props, url: ctx.var.url.href })`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Signed-off-by: Marcos Candeia <marrcooos@gmail.com>
Signed-off-by: Marcos Candeia <marrcooos@gmail.com>
Signed-off-by: Marcos Candeia <marrcooos@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
runtime/middleware.ts (1)
429-445:⚠️ Potential issue | 🟠 Major
no-storeis never set whendirty=trueandPAGE_CACHE_DRY_RUN=false.The three-branch structure leaves a gap: when cookies are accessed (
dirty=true) but dry-run is disabled, noCache-Controlheader is emitted. Any upstream CDN can then apply its own default caching, silently serving stale, cookie-personalized content to other users. The PR description explicitly requiresno-store, no-cache, must-revalidatefor dirty responses.🛡️ Proposed fix
} else if (PAGE_CACHE_DRY_RUN) { console.warn( `[page-cache] not cacheable (cookies accessed): ${url.pathname}`, ); for (const trace of ctx.var.dirtyTraces ?? []) { console.warn(`[page-cache] trace:\n${trace}`); } + } else { + newHeaders.set("Cache-Control", "no-store, no-cache, must-revalidate"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/middleware.ts` around lines 429 - 445, The three-branch header logic in the middleware leaves dirty responses without a Cache-Control when ctx.var.dirty is true and PAGE_CACHE_DRY_RUN is false; update the branch handling so that whenever ctx.var.dirty is true (regardless of PAGE_CACHE_DRY_RUN) you set newHeaders.set("Cache-Control", "no-store, no-cache, must-revalidate") after checking getSetCookies(newHeaders), and keep the existing dry-run logging behavior (using PAGE_CACHE_DRY_RUN and ctx.var.dirtyTraces) for visibility when dry-run is enabled; refer to getSetCookies, newHeaders, ctx.var.dirty, PAGE_CACHE_DRY_RUN, and ctx.var.dirtyTraces to locate and modify the logic.
🧹 Nitpick comments (1)
runtime/mod.ts (1)
179-221: Proxy closures referencestatebefore it is declared and beforestate.dirtyTracesis initialised — latent TDZ andTypeErrorrisk.Two hazards exist with the current ordering:
constTDZ:proxiedHeaders' get-trap closes overstate, which is declared on line 222. Any code path that accessesreq.headers.get("cookie")before line 222 will throwReferenceError: Cannot access 'state' before initialization.Undefined
dirtyTraces:state.dirtyTraces!.push(...)(line 194) uses a non-null assertion, butstate.dirtyTracesis only initialised to[]on line 276. Between lines 222–275,state.dirtyTracesisundefined; calling.push()on it would throw aTypeErrorat runtime despite the!silencing TypeScript.Neither hazard is triggered by the current code path (the resolver only accesses headers after line 276), but the fragility will bite on any future refactor that accesses
req.headersduring intermediate state setup.♻️ Recommended fix: move proxy definitions after `state` and `state.dirtyTraces` are initialised
async prepareState<TConfig = any>(...): Promise<State<TAppManifest, TConfig>> { const _req = context.req.raw; - // Proxy the request headers to detect cookie access... - const proxiedHeaders = new Proxy(_req.headers, { ... }); - const req = new Proxy(_req, { ... }); - const state = (context.base ?? {}) as State<TAppManifest, TConfig>; state.deco = this; // ... all state field initialisation ... state.dirty = false; state.dirtyTraces = []; + + // Proxy the request headers to detect cookie access during resolution. + const proxiedHeaders = new Proxy(_req.headers, { + get(target, prop, receiver) { + const value = Reflect.get(target, prop, receiver); + if (typeof value === "function") { + return function (this: Headers, ...args: any[]) { + if ( + (prop === "get" || prop === "has") && + typeof args[0] === "string" && + args[0].toLowerCase() === "cookie" + ) { + state.dirty = true; + state.dirtyTraces.push( + new Error( + `cookie header accessed via headers.${String(prop)}("cookie")`, + ).stack ?? "", + ); + } + return value.apply(target, args); + }; + } + return value; + }, + }); + const req = new Proxy(_req, { + get(target, prop, receiver) { + if (prop === "headers") return proxiedHeaders; + const value = Reflect.get(target, prop, receiver); + return typeof value === "function" ? value.bind(target) : value; + }, + });With this ordering, both proxy closures close over an already-initialised
stateandstate.dirtyTraces, eliminating the TDZ and the undefined-pushrisk. The!non-null assertion onstate.dirtyTracescan also be dropped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/mod.ts` around lines 179 - 221, The proxies proxiedHeaders and req close over state and call state.dirtyTraces!.push(...) before state and state.dirtyTraces are initialized, risking TDZ/TypeError; fix by moving the creation of proxiedHeaders and req to after state is declared and ensure state.dirtyTraces is initialized to an array (e.g., []) before the proxies are created, and remove the non-null assertion on state.dirtyTraces in the proxiedHeaders get-trap so the push is safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@runtime/middleware.ts`:
- Around line 354-356: The code uses
Object.entries(allowCorsFor(ctx.req.raw)).map(...) purely for side-effects which
creates a discarded array; replace the map call with forEach to avoid allocating
that array: iterate over
Object.entries(allowCorsFor(ctx.req.raw)).forEach(([name, value]) => {
newHeaders.set(name, value); }) so change the invocation on the result of
Object.entries and keep the callback using newHeaders.set and the same
ctx.req.raw input.
---
Duplicate comments:
In `@runtime/middleware.ts`:
- Around line 429-445: The three-branch header logic in the middleware leaves
dirty responses without a Cache-Control when ctx.var.dirty is true and
PAGE_CACHE_DRY_RUN is false; update the branch handling so that whenever
ctx.var.dirty is true (regardless of PAGE_CACHE_DRY_RUN) you set
newHeaders.set("Cache-Control", "no-store, no-cache, must-revalidate") after
checking getSetCookies(newHeaders), and keep the existing dry-run logging
behavior (using PAGE_CACHE_DRY_RUN and ctx.var.dirtyTraces) for visibility when
dry-run is enabled; refer to getSetCookies, newHeaders, ctx.var.dirty,
PAGE_CACHE_DRY_RUN, and ctx.var.dirtyTraces to locate and modify the logic.
---
Nitpick comments:
In `@runtime/mod.ts`:
- Around line 179-221: The proxies proxiedHeaders and req close over state and
call state.dirtyTraces!.push(...) before state and state.dirtyTraces are
initialized, risking TDZ/TypeError; fix by moving the creation of proxiedHeaders
and req to after state is declared and ensure state.dirtyTraces is initialized
to an array (e.g., []) before the proxies are created, and remove the non-null
assertion on state.dirtyTraces in the proxiedHeaders get-trap so the push is
safe.
| Object.entries(allowCorsFor(ctx.req.raw)).map(([name, value]) => { | ||
| newHeaders.set(name, value); | ||
| }); |
There was a problem hiding this comment.
Use forEach instead of map for side-effect-only iteration.
map() builds and immediately discards an array of undefined values. Biome correctly flags this as a suspicious pattern.
♻️ Proposed fix
- Object.entries(allowCorsFor(ctx.req.raw)).map(([name, value]) => {
- newHeaders.set(name, value);
- });
+ for (const [name, value] of Object.entries(allowCorsFor(ctx.req.raw))) {
+ newHeaders.set(name, value);
+ }📝 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.
| Object.entries(allowCorsFor(ctx.req.raw)).map(([name, value]) => { | |
| newHeaders.set(name, value); | |
| }); | |
| for (const [name, value] of Object.entries(allowCorsFor(ctx.req.raw))) { | |
| newHeaders.set(name, value); | |
| } |
🧰 Tools
🪛 Biome (2.4.4)
[error] 354-354: This callback passed to map() iterable method should always return a value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@runtime/middleware.ts` around lines 354 - 356, The code uses
Object.entries(allowCorsFor(ctx.req.raw)).map(...) purely for side-effects which
creates a discarded array; replace the map call with forEach to avoid allocating
that array: iterate over
Object.entries(allowCorsFor(ctx.req.raw)).forEach(([name, value]) => {
newHeaders.set(name, value); }) so change the invocation on the result of
Object.entries and keep the callback using newHeaders.set and the same
ctx.req.raw input.
2 participants
Summary by cubic
Add automatic page-level caching for cookie-free requests and optional single-flight to dedupe concurrent renders. Dry-run mode logs cookie access traces, and cookie detection is now reliable via proxied headers; single-flight key now includes URL to avoid collisions.
New Features
Migration
Written for commit b966d0d. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Public API