-
Notifications
You must be signed in to change notification settings - Fork 55
Next #1192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Next #1192
Changes from all commits
5c2e46f
7372876
83d84ed
7c95832
610d392
c40aff8
a634468
a7aecde
063f2c5
5c986f1
0347fbf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -390,27 +390,24 @@ export const middlewareFor = <TAppManifest extends AppManifest = AppManifest>( | |
| ); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: Treating all Prompt for AI agents |
||
| const segment = tryOrDefault(() => JSON.parse(cookieSegment), {}); | ||
|
|
||
| const active = new Set(segment.active || []); | ||
| const inactiveDrawn = new Set(segment.inactiveDrawn || []); | ||
| // Track only the `active` set. The previous `inactiveDrawn` set was | ||
| // write-only state — never read by any matcher logic — and its growth | ||
| // across pages was the main reason `deco_segment` got rewritten on | ||
| // almost every request, tripping the Set-Cookie kill-switch below and | ||
| // making HTML uncacheable. Removing it lets cohort assignment stick. | ||
| const previousActive = [...new Set<string>(segment.active || [])].sort(); | ||
| const active = new Set<string>(previousActive); | ||
| for (const flag of ctx.var.flags) { | ||
| if (flag.isSegment) { | ||
| if (flag.value) { | ||
| active.add(flag.name); | ||
| inactiveDrawn.delete(flag.name); | ||
| } else { | ||
| active.delete(flag.name); | ||
| inactiveDrawn.add(flag.name); | ||
| } | ||
| } | ||
| if (!flag.isSegment) continue; | ||
| if (flag.value) active.add(flag.name); | ||
| else active.delete(flag.name); | ||
| } | ||
| const newSegment = { | ||
| active: [...active].sort(), | ||
| inactiveDrawn: [...inactiveDrawn].sort(), | ||
| }; | ||
| const value = JSON.stringify(newSegment); | ||
| const hasFlags = active.size > 0 || inactiveDrawn.size > 0; | ||
|
|
||
| if (hasFlags && cookieSegment !== value) { | ||
| const newActive = [...active].sort(); | ||
| const activeChanged = | ||
| JSON.stringify(previousActive) !== JSON.stringify(newActive); | ||
|
|
||
| if (active.size > 0 && activeChanged) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: When all active segment flags are removed, the cookie is never updated/cleared, leaving stale Prompt for AI agents |
||
| const value = JSON.stringify({ active: newActive }); | ||
| const date = new Date(); | ||
| date.setTime(date.getTime() + 30 * 24 * 60 * 60 * 1000); // 1 month | ||
| setCookie( | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -21,7 +21,8 @@ export const handler = createHandler(async (ctx) => { | |||||||||||||||||||||||
| return new Response(JSON.stringify(value), { | ||||||||||||||||||||||||
| headers: { | ||||||||||||||||||||||||
| "Content-Type": "application/json", | ||||||||||||||||||||||||
| "cache-control": "must-revalidate", | ||||||||||||||||||||||||
| "cache-control": | ||||||||||||||||||||||||
| "public, max-age=60, s-maxage=60, stale-while-revalidate=300, must-revalidate", | ||||||||||||||||||||||||
|
Comment on lines
+24
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: The Cache-Control directives must-revalidate and stale-while-revalidate serve fundamentally different purposes and are generally considered incompatible when used together, as they impose conflicting requirements on how a cache must handle stale content [1][2][3]. must-revalidate The must-revalidate directive is a strict instruction that prohibits a cache from serving a stale response without first successfully revalidating it with the origin server [1][2][3]. If the cache is unable to reach the origin (e.g., due to network disconnection), it must return an error (typically a 504 Gateway Timeout) rather than serving the stale content [2][3][4]. This directive is intended for scenarios where serving stale data could lead to incorrect operations, such as financial transactions [2][3][4]. stale-while-revalidate The stale-while-revalidate directive is designed to improve performance by allowing a cache to serve a stale response immediately while simultaneously revalidating it in the background [1][5][6]. This directive explicitly permits the use of stale content to avoid blocking the user, prioritizing availability and latency over strict freshness [5][6]. Compatibility and Interaction When both directives are present in the same Cache-Control header, they create a conflict: - must-revalidate mandates that stale content must not be served until revalidation is complete [2][3]. - stale-while-revalidate permits stale content to be served immediately while revalidation happens in the background [5][6]. In practice, because must-revalidate is a strict requirement that caches are mandated not to ignore [2][3][4], it effectively overrides the permissive behavior of stale-while-revalidate. A compliant cache will prioritize the must-revalidate constraint, forcing the client to wait for the revalidation to complete before receiving a response, thereby negating the performance benefits of stale-while-revalidate [1][2][3]. Consequently, these directives should not be used together if the goal is to leverage the background revalidation capabilities of stale-while-revalidate [1]. Citations:
🏁 Script executed: cat -n runtime/routes/_meta.ts | sed -n '20,30p'Repository: deco-cx/deco Length of output: 414 Remove The cache-control header combines
When both are present, If the intent is to allow stale responses during revalidation (performance optimization), remove Proposed fix (assuming stale-while-revalidate is intended) "cache-control":
- "public, max-age=60, s-maxage=60, stale-while-revalidate=300, must-revalidate",
+ "public, max-age=60, s-maxage=60, stale-while-revalidate=300",📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Comment on lines
+24
to
+25
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: This response is now publicly cacheable while CORS headers vary by request origin, but Prompt for AI agents
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Prompt for AI agents
Suggested change
|
||||||||||||||||||||||||
| etag, | ||||||||||||||||||||||||
| ...allowCorsFor(ctx.req.raw), | ||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,6 +1,6 @@ | ||||||
| { | ||||||
| "name": "@deco/scripts", | ||||||
| "version": "1.197.0", | ||||||
| "version": "1.197.1-next.2", | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Version downgraded from 1.200.1-next.1 to 1.197.1-next.2, which contradicts the PR's stated goal of bumping to 1.200.1-next.1. This may be a merge/revert artifact that left scripts/deno.json at an older version than intended. Prompt for AI agents
Suggested change
|
||||||
| "exports": { | ||||||
| "./release": "./release.ts", | ||||||
| "./update": "./update.run.ts", | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: Version regression in
dev/deno.json: changed from1.200.1-next.1to1.197.1-next.2, but the PR intent is to bump to1.200.1-next.1. This downgrade will cause@deco/devto be published at an older version than intended, creating inconsistency with sibling packages.Prompt for AI agents