Add page caching support and related headers #992
Conversation
- Introduced DECO_PAGE_CACHE_CONTROL_HEADER and DECO_PAGE_CACHE_ALLOW_HEADER for managing cache behavior. - Updated matcher logic to respect Cache-Control settings, preventing non-device/time based matchers from executing when caching is enabled. - Enhanced middleware to apply Cache-Control headers based on configuration. - Modified manifest generation to skip unnecessary directories during scanning. - Improved render route to utilize the new Cache-Control header for responses.
WalkthroughAdds page-level Cache-Control support and matcher-aware caching: new internal headers and normalization utilities, middleware and render route apply per-page cache directives for safe GETs, matchers can skip evaluation when disallowed by page caching, tests and walker ignores added, and .gitignore expanded. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Middleware
participant Matcher
participant Router
participant Cache
Client->>Middleware: GET /page
Middleware->>Middleware: Inspect ctx/response for x-deco-page-cache-control
Middleware->>Matcher: For each matcher, provide matcher path & page-cache allow list
alt Matcher allowed under page cache
Matcher->>Middleware: Evaluate (may read/write cookies if sticky)
else Matcher disallowed by page cache
Matcher-->>Middleware: Skip evaluation (and skip cookie read/write if sticky)
end
Middleware->>Router: Continue to render
Router->>Router: Read x-deco-page-cache-control from ctx.var.response
Router->>Cache: Set standard Cache-Control header from per-page value (if present)
Router-->>Client: Response with Cache-Control
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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?
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
blocks/matcher.ts (1)
176-231: Don't clobber sticky matcher cookies when page cache disallows them.When the page-level cache disables a matcher, Line 214 forces
resulttofalse, but the sticky branch still drops into the cookie write at Line 224. That overwrites any previously stored segment cookie withfalse, even though the matcher never ran. Once caching conditions change, the original segmentation signal is lost. Guard both evaluation and cookie writes so they are skipped whenever the matcher is disabled by page caching.Apply this diff:
let result = isEnabled; + let matcherDisabledByPageCache = false; // If page Cache-Control is enabled, ignore all matchers that are not device/time based. const pageCachingOn = respHeaders.has(DECO_PAGE_CACHE_CONTROL_HEADER); if (pageCachingOn) { @@ - const allowed = (allowDevice && isDeviceMatcher) || - (allowTime && isTimeMatcher); - if (!allowed) { - result = false; - } + const allowed = (allowDevice && isDeviceMatcher) || + (allowTime && isTimeMatcher); + matcherDisabledByPageCache = !allowed; + if (matcherDisabledByPageCache) { + result = false; + } } // if it is not sticky then we can run the matcher function if (!shouldStickyOnSession) { - if (pageCachingOn && result === false) { - // matcher disabled by page caching, do not evaluate - } else { - result ??= matcherFunc(ctx); - } + if (!matcherDisabledByPageCache) { + result ??= matcherFunc(ctx); + } } else { @@ - if (pageCachingOn && result === false) { - // matcher disabled by page caching, do not evaluate nor set cookie - } else { - result ??= isMatchFromCookie ?? matcherFunc(ctx); - } - if (result !== isMatchFromCookie) { + if (!matcherDisabledByPageCache) { + result ??= isMatchFromCookie ?? matcherFunc(ctx); + } + if (!matcherDisabledByPageCache && result !== isMatchFromCookie) { const date = new Date();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignore(1 hunks)blocks/matcher.ts(5 hunks)engine/manifest/manifestGen.ts(1 hunks)runtime/middleware.ts(1 hunks)runtime/routes/render.tsx(1 hunks)utils/http.ts(1 hunks)utils/mod.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
blocks/matcher.ts (1)
utils/http.ts (2)
DECO_PAGE_CACHE_CONTROL_HEADER(150-150)DECO_PAGE_CACHE_ALLOW_HEADER(155-155)
runtime/middleware.ts (2)
utils/http.ts (1)
DECO_PAGE_CACHE_CONTROL_HEADER(150-150)utils/mod.ts (1)
DECO_PAGE_CACHE_CONTROL_HEADER(22-22)
utils/http.ts (1)
utils/mod.ts (3)
DECO_PAGE_CACHE_CONTROL_HEADER(22-22)DECO_PAGE_CACHE_ALLOW_HEADER(21-21)normalizeCacheControlHeader(23-23)
runtime/routes/render.tsx (2)
utils/http.ts (1)
DECO_PAGE_CACHE_CONTROL_HEADER(150-150)utils/mod.ts (1)
DECO_PAGE_CACHE_CONTROL_HEADER(22-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Setup deno (ubuntu-latest)
- GitHub Check: Setup deno (windows-latest)
| export const normalizeCacheControlHeader = ( | ||
| value?: boolean | string, | ||
| ): string | undefined => { | ||
| if (!value) return undefined; | ||
| if (value === true) { | ||
| return formatCacheControl(DEFAULT_CACHE_CONTROL); | ||
| } | ||
| try { | ||
| const h = new Headers({ "cache-control": value }); | ||
| const parsed = parseCacheControl(h); | ||
| // If nothing parsed, treat as invalid | ||
| const normalized = formatCacheControl(parsed); | ||
| return normalized || formatCacheControl(DEFAULT_CACHE_CONTROL); | ||
| } catch { | ||
| return formatCacheControl(DEFAULT_CACHE_CONTROL); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Fallback when cache-control numbers are invalid.
Line 176 ends up returning strings like max-age=NaN whenever the supplied directive has a non-numeric value (e.g. max-age=foo). In those cases we wanted to fall back to DEFAULT_CACHE_CONTROL, but the current logic formats the NaN instead. That produces an invalid Cache-Control header and breaks downstream caching. Please detect non-finite numeric tokens before formatting and fall back to the default when they appear.
Apply this diff:
try {
const h = new Headers({ "cache-control": value });
const parsed = parseCacheControl(h);
+ const hasInvalidNumbers = Object.values(parsed).some((entry) =>
+ typeof entry === "number" && !Number.isFinite(entry)
+ );
+ if (hasInvalidNumbers) {
+ return formatCacheControl(DEFAULT_CACHE_CONTROL);
+ }
// If nothing parsed, treat as invalid
const normalized = formatCacheControl(parsed);
return normalized || formatCacheControl(DEFAULT_CACHE_CONTROL);📝 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.
| export const normalizeCacheControlHeader = ( | |
| value?: boolean | string, | |
| ): string | undefined => { | |
| if (!value) return undefined; | |
| if (value === true) { | |
| return formatCacheControl(DEFAULT_CACHE_CONTROL); | |
| } | |
| try { | |
| const h = new Headers({ "cache-control": value }); | |
| const parsed = parseCacheControl(h); | |
| // If nothing parsed, treat as invalid | |
| const normalized = formatCacheControl(parsed); | |
| return normalized || formatCacheControl(DEFAULT_CACHE_CONTROL); | |
| } catch { | |
| return formatCacheControl(DEFAULT_CACHE_CONTROL); | |
| } | |
| }; | |
| export const normalizeCacheControlHeader = ( | |
| value?: boolean | string, | |
| ): string | undefined => { | |
| if (!value) return undefined; | |
| if (value === true) { | |
| return formatCacheControl(DEFAULT_CACHE_CONTROL); | |
| } | |
| try { | |
| const h = new Headers({ "cache-control": value }); | |
| const parsed = parseCacheControl(h); | |
| const hasInvalidNumbers = Object.values(parsed).some((entry) => | |
| typeof entry === "number" && !Number.isFinite(entry) | |
| ); | |
| if (hasInvalidNumbers) { | |
| return formatCacheControl(DEFAULT_CACHE_CONTROL); | |
| } | |
| // If nothing parsed, treat as invalid | |
| const normalized = formatCacheControl(parsed); | |
| return normalized || formatCacheControl(DEFAULT_CACHE_CONTROL); | |
| } catch { | |
| return formatCacheControl(DEFAULT_CACHE_CONTROL); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In utils/http.ts around lines 163 to 179, the normalization currently formats
parsed numeric directives (e.g. max-age) even when they parse to NaN (producing
"max-age=NaN"); detect non-finite numeric tokens on the parsed result before
calling formatCacheControl and if any numeric directive is not a finite number,
return formatCacheControl(DEFAULT_CACHE_CONTROL) instead. Concretely: after
parseCacheControl(h) iterate the parsed directives that should be numeric
(max-age, s-maxage, stale-while-revalidate, stale-if-error, etc.), coerce their
values to Number and use Number.isFinite(...) to validate; if any fail, treat as
invalid and fall back to DEFAULT_CACHE_CONTROL, otherwise proceed to format and
return the normalized header.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
blocks/matcher.cache-page.test.ts (2)
8-21: Consider adding type annotations for better maintainability.The stub works for current tests, but adding explicit type info or a comment about which
HttpContextproperties are exercised would help future maintainers understand the minimal contract.
130-134: Type assertionas anyhides interface mismatch.The
STICKY_TRUE_MATCHERusesas anyto satisfy theMatcherModuletype. If the interface changes, this test may silently drift out of sync.Consider aligning with the actual interface:
const STICKY_TRUE_MATCHER: MatcherModule = { sticky: "session", default: () => true, - sessionKey: () => "k", - } as any; + sessionKey: () => "k" as string | null, + };blocks/matcher.ts (2)
184-189: Redundant regex pattern.The pattern
\.tsx?$already matches both.tsand.tsxfiles (thex?makesxoptional). The second regex on line 189 is redundant.const isDeviceMatcher = /\/matchers\/device\.tsx?$/.test( matcherResolver, ); -const isTimeMatcher = /\/matchers\/(date|cron)\.tsx?$/.test( - matcherResolver, -) || /\/matchers\/(date|cron)\.ts$/.test(matcherResolver); +const isTimeMatcher = /\/matchers\/(date|cron)\.tsx?$/.test( + matcherResolver, +);
198-202: Consider simplifying the empty if-block.The intent is clear from the comment, but an empty
ifblock is unusual. A single conditional reads more naturally.-if (pageCachingOn && result === false) { - // matcher disabled by page caching, do not evaluate -} else { - result ??= matcherFunc(ctx); -} +// Only evaluate if page caching hasn't explicitly disabled this matcher +if (!(pageCachingOn && result === false)) { + result ??= matcherFunc(ctx); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
blocks/matcher.cache-page.test.ts(1 hunks)blocks/matcher.ts(5 hunks)utils/http.cache.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
utils/http.cache.test.ts (1)
utils/http.ts (3)
formatCacheControl(86-97)DEFAULT_CACHE_CONTROL(5-11)parseCacheControl(71-84)
blocks/matcher.cache-page.test.ts (2)
blocks/matcher.ts (1)
MatcherModule(85-87)utils/hasher.ts (1)
result(107-127)
🔇 Additional comments (4)
utils/http.cache.test.ts (1)
1-41: LGTM! Good coverage of normalization behavior.The tests cover the main cases well: boolean
true, valid/invalid strings, and falsy values. The round-trip parsing verification (lines 24-27) is a nice touch.blocks/matcher.cache-page.test.ts (1)
28-142: Good test coverage for page caching behavior.The tests comprehensively cover the key scenarios: device/time matcher allowlists, explicit allow-header overrides, cache OFF behavior, and sticky-session cookie suppression.
blocks/matcher.ts (2)
7-10: LGTM!The new header imports align with the page caching feature requirements.
214-229: Sticky operations correctly skipped when page caching disables matcher.The logic ensures that when page caching sets
result = falsefor disallowed matchers, cookie reads and writes are bypassed. This prevents stale/conflicting cookie state and aligns with the test expectations.
| // If page Cache-Control is enabled, ignore all matchers that are not device/time based. | ||
| const pageCachingOn = respHeaders.has(DECO_PAGE_CACHE_CONTROL_HEADER); | ||
| if (pageCachingOn) { | ||
| const allow = respHeaders.get(DECO_PAGE_CACHE_ALLOW_HEADER) ?? | ||
| "device,time"; | ||
| const allowDevice = allow.includes("device"); | ||
| const allowTime = allow.includes("time"); | ||
| const isDeviceMatcher = /\/matchers\/device\.tsx?$/.test( | ||
| matcherResolver, | ||
| ); | ||
| const isTimeMatcher = /\/matchers\/(date|cron)\.tsx?$/.test( | ||
| matcherResolver, | ||
| ) || /\/matchers\/(date|cron)\.ts$/.test(matcherResolver); | ||
| const allowed = (allowDevice && isDeviceMatcher) || | ||
| (allowTime && isTimeMatcher); | ||
| if (!allowed) { | ||
| result = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'DECO_PAGE_CACHE_ALLOW_HEADER' --type ts -A2 -B2Repository: deco-cx/deco
Length of output: 1793
🏁 Script executed:
rg -n 'set.*DECO_PAGE_CACHE_ALLOW_HEADER|\.set\([^,]*x-deco-page-cache-allow' --type ts -A1 -B1Repository: deco-cx/deco
Length of output: 251
🏁 Script executed:
# Also check for any documentation or comments about header format
rg -n 'device.*time|allow.*header' --type ts -i -A2 -B2 | head -50Repository: deco-cx/deco
Length of output: 2453
Consider trimming individual values when parsing the allow-list header.
While the documented format is "device,time" (without spaces), the current .includes() checks are susceptible to malformed headers. For example, if a header is set to "device, time" (with space after comma), allow.includes("time") would fail. Use .split(",").map(s => s.trim()) to make the parsing more robust.
🤖 Prompt for AI Agents
In blocks/matcher.ts around lines 177 to 195, the parsing of the
DECO_PAGE_CACHE_ALLOW_HEADER uses allow.includes(...) which breaks on values
with surrounding whitespace (e.g., "device, time"); split the header on commas,
trim each entry, and check membership against that normalized list (e.g., const
parts = (allow ?? "").split(",").map(s => s.trim()).filter(Boolean); then use
parts.includes("device") and parts.includes("time") to set
allowDevice/allowTime) so malformed spacing no longer prevents matching.
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.