Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe changes add early handling for server errors (statusCode >= 500) in the isomorphic handler that sends the status code before SEO and page rendering logic. Integration tests are simplified to assert only HTTP status codes instead of parsing response bodies, with minor formatting and cache header updates applied consistently across test files. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(1 hunks)server/handlers/isomorphic-handler.js(2 hunks)test/integration/isomorphic-handler-test.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/integration/isomorphic-handler-test.js (1)
test/integration/isomorphic-data-load-test.js (17)
supertest(5-5)app(20-20)app(44-53)app(70-72)app(86-88)app(102-104)app(118-120)app(133-141)app(154-156)app(171-177)app(191-198)app(214-216)app(224-226)app(257-268)app(287-294)app(313-324)app(343-355)
🔇 Additional comments (4)
package.json (1)
3-3: LGTM!The pre-release version format is appropriate for this bug fix branch.
server/handlers/isomorphic-handler.js (1)
488-494: Verify the intended behavior for 5xx error responses.This change significantly alters error handling - 5xx errors now return a minimal status response via
sendStatus()(e.g., "500 Internal Server Error") instead of rendering a full error page with SEO tags, store state, and content. Additionally, no cache headers are set for these responses.Please confirm this behavior aligns with the fix described in issue #4117 and won't break downstream consumers that expect rendered error pages.
test/integration/isomorphic-handler-test.js (2)
157-172: LGTM!The test simplification appropriately aligns with the new 500 error handling behavior that returns a minimal status response instead of a rendered page.
174-188: LGTM!The test simplification appropriately aligns with the new error handling behavior where both
loadDataandloadErrorDatafailures result in a minimal 500 status response.
| return res.redirect(301, result.data.location); | ||
| } | ||
|
|
||
| // For server errors (>=500),send statuss |
There was a problem hiding this comment.
Fix typo in comment.
The comment contains a typo: "send statuss" should be "send status".
Apply this diff:
- // For server errors (>=500),send statuss
+ // For server errors (>=500), send status📝 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 server errors (>=500),send statuss | |
| // For server errors (>=500), send status |
🤖 Prompt for AI Agents
In server/handlers/isomorphic-handler.js around line 489, the inline comment
contains a typo "send statuss"; update that comment to read "send status" so it
correctly documents behavior for server errors (>=500).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/handlers/isomorphic-handler.js (1)
489-489: Fix typo in comment (already flagged in previous review).The comment contains "send statuss" (should be "send status") and is missing a space after the comma.
Apply this diff:
- // For server errors (>=500),send statuss + // For server errors (>=500), send status
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
constants.js(1 hunks)server/handlers/cdn-caching.js(1 hunks)server/handlers/isomorphic-handler.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/handlers/cdn-caching.js (2)
test/integration/sketches-proxy-test.js (6)
maxAge(233-233)maxAge(245-245)maxAge(256-256)sMaxAge(160-160)sMaxAge(172-172)sMaxAge(183-183)constants.js (1)
STALE_IF_ERROR_CACHE_DURATION(1-1)
🔇 Additional comments (3)
server/handlers/isomorphic-handler.js (2)
128-128: Semicolon added for consistency.The semicolon after
return nullimproves code style consistency.
490-492: Confirm intentionality of skipping error page rendering for 5xx responses.The early return at line 490-492 is intentional (indicated by the inline comment), but it creates asymmetric error handling: 4xx errors like 404 render through the full pipeline with custom error pages (lines 403-427), while 5xx errors skip all rendering and return plain-text status codes.
This behavior prevents rendering when
result.httpStatusCode >= 500, including exceptions caught inloadDatawhich return{ httpStatusCode: 500, pageType: "error" }. No error-specific render components were found in the codebase.Verify: Is this plain-text fallback intentional for safety (to avoid cascading failures), or should 5xx errors also render custom error pages like 404s do?
server/handlers/cdn-caching.js (1)
45-45: The original review mischaracterizes this change as a risky reduction when it actually corrects the cache strategy toward best practice.RFC 5861 recommends stale-while-revalidate be set to small windows (seconds to low minutes), and industry best practice typically uses 10–120 seconds for very dynamic content. The value of 120 seconds (2 minutes) falls squarely within this guidance. The prior value of 1000 seconds was the anomaly—8× the recommended range for content with a 15-second max-age.
The header now correctly pairs a 15-second max-age with a 120-second revalidation window, where the stale window acts as a grace period during background cache refresh. This aligns with best practice of tuning stale-while-revalidate relative to max-age. The change improves cache strategy alignment, not undermines it.
No action required; this change is sound.
Likely an incorrect or invalid review comment.
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 (2)
test/integration/custom-route-handler-test.js (1)
115-275: Add test coverage for 500 status code handling.The PR is titled "Handle 500 issue" and references fixing a 500 error scenario, but this test file contains no test cases that verify 500 status code handling. The existing tests cover 301, 302, 200, and 404 status codes, but there's a critical gap for server error scenarios (5xx status codes).
Without test coverage, the fix cannot be verified to work correctly. Please add test cases that:
- Simulate a scenario where the API returns a 500+ status code
- Verify the isomorphic handler correctly detects and handles the error
- Confirm the response status, headers, and body are as expected
Would you like me to help generate test cases for 500 error handling? I can provide example test code that follows the patterns in this file.
server/handlers/cdn-caching.js (1)
36-47: Inconsistent caching strategy betweennetworkOnlyand non-networkOnlypaths.The
networkOnlypath (lines 36-41) still uses dynamic values (${sMaxAge}and${STALE_IF_ERROR_CACHE_DURATION}), while the non-networkOnlypath now uses hardcoded 120-second values. This creates inconsistent behavior within the same function.If the goal is to handle 500 errors more gracefully, the reduced
stale-if-errorduration should likely apply to both paths. Consider unifying the caching strategy or documenting why different paths need different behaviors.
🧹 Nitpick comments (3)
test/integration/custom-route-handler-test.js (1)
51-51: Formatting changes are cosmetic and unrelated to the PR objective.These multi-line to single-line conversions have no functional impact but appear unrelated to the stated PR objective of handling 500 errors. Consider keeping formatting changes separate from functional bug fixes for clearer change tracking.
Also applies to: 62-62, 73-73, 104-104, 183-183, 216-216
server/handlers/cdn-caching.js (2)
45-45: Three inconsistent caching strategies in the same function.The function now has three different cache duration strategies:
- Line 45 (non-networkOnly with cacheKeys):
s-maxage=120, stale-while-revalidate=120, stale-if-error=120- Line 37 (networkOnly):
s-maxage=${sMaxAge}, stale-if-error=${STALE_IF_ERROR_CACHE_DURATION}- Line 74 (no cacheKeys):
s-maxage=60, stale-while-revalidate=150, stale-if-error=3600Consider consolidating these strategies or at least documenting the rationale for each path to improve maintainability.
Also applies to: 74-74
45-45: Document the rationale for the 120-second cache duration.The hardcoded value
120appears without explanation. Adding a comment or extracting it to a named constant would help future maintainers understand why this specific duration was chosen and how it relates to the 500 error handling requirements.Example:
+// Reduced cache duration to minimize stale content during server errors (issue #4117) +const REDUCED_CACHE_DURATION = 120; + res.setHeader( "Cache-Control", - `public,max-age=${maxAge},s-maxage=120,stale-while-revalidate=120,stale-if-error=120` + `public,max-age=${maxAge},s-maxage=${REDUCED_CACHE_DURATION},stale-while-revalidate=${REDUCED_CACHE_DURATION},stale-if-error=${REDUCED_CACHE_DURATION}` );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/handlers/cdn-caching.js(1 hunks)test/integration/custom-route-handler-test.js(12 hunks)test/integration/isomorphic-handler-test.js(8 hunks)test/integration/url-redirect-test.js(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/integration/isomorphic-handler-test.js
🧰 Additional context used
🧬 Code graph analysis (2)
server/handlers/cdn-caching.js (1)
test/integration/sketches-proxy-test.js (3)
maxAge(233-233)maxAge(245-245)maxAge(256-256)
test/integration/url-redirect-test.js (3)
test/integration/custom-route-handler-test.js (16)
supertest(6-6)app(91-91)app(117-120)app(132-135)app(147-151)app(170-173)app(189-192)app(197-201)app(214-218)app(232-235)app(247-250)app(262-265)assert(1-1)response(161-161)response(207-207)response(224-224)test/integration/isomorphic-handler-test.js (14)
supertest(6-6)app(57-60)app(77-83)app(97-100)app(115-121)app(132-142)assert(1-1)response(67-67)response(90-90)response(107-107)response(149-149)response(379-379)response(944-944)response(980-980)test/integration/isomorphic-data-load-test.js (8)
supertest(5-5)assert(1-1)response(60-60)response(79-79)response(95-95)response(111-111)response(126-126)response(147-147)
🔇 Additional comments (4)
test/integration/custom-route-handler-test.js (1)
124-124: Verify that the cache duration reduction is intentional and understand its implications.All test expectations now assert 120-second cache durations (s-maxage, stale-while-revalidate, stale-if-error). This represents a significant reduction in CDN/proxy caching that could:
- Increase origin server load
- Reduce CDN cache hit rates
- Impact overall site performance
Additionally, this change appears unrelated to the PR's stated objective of "handling 500 errors." Please confirm:
- The cache reduction is intentional and necessary for the 500 error handling fix
- The performance impact has been evaluated
- Whether this should be a separate change
Also applies to: 139-139, 155-155, 177-177, 239-239, 253-253, 268-268
test/integration/url-redirect-test.js (2)
511-511: LGTM! Formatting improvements enhance readability.The supertest chains have been compressed to single lines, making the test assertions more concise without affecting functionality.
Also applies to: 528-528, 566-566, 575-575, 584-584, 593-593, 602-602, 611-611
552-552: LGTM! Inline assertion improves conciseness.The assertion has been condensed to a single line without changing the expected value or test behavior.
server/handlers/cdn-caching.js (1)
45-45: Based on my verification of the codebase, the original review comment is accurate and well-founded. Here are the confirmed findings:Verified facts:
- The
sMaxAgeparameter (default 900s) is defined in the function signature and IS passed by multiple callers across 3 files (isomorphic-handler.js, story-redirect.js, custom-route-handler.js)- Callers use conditional logic to set different
sMaxAgevalues based on content type (e.g.,sMaxAge: result.pageType === "tag-page" ? 600 : sMaxAge)- In the
networkOnly=truepath (lines 37-40),${sMaxAge}is correctly used- In the
networkOnly=falsepath (line 45-46),sMaxAgeis hardcoded to 120, completely ignoring the parameter- This creates a breaking change: caller-specific cache configurations are now ignored
STALE_IF_ERROR_CACHE_DURATIONis 14400 (4 hours), being reduced to 120 (2 minutes) - a 99% reduction- No code comments explain the rationale for the hardcoded values
Conclusion:
The original review comment correctly identifies a breaking change with significant performance implications. The concerns about ignoring thesMaxAgeparameter, the dramatic cache reduction, and the lack of documentation are all valid. The review accurately points out the inconsistency between code paths and appropriately raises questions about whether this aggressive caching reduction is justified solely for handling 500 errors.
| "Cache-Control", | ||
| "public,max-age=15,s-maxage=900,stale-while-revalidate=1000,stale-if-error=14400" | ||
| ) | ||
| .expect("Cache-Control", "public,max-age=15,s-maxage=120,stale-while-revalidate=120,stale-if-error=120") |
There was a problem hiding this comment.
🧩 Analysis chain
Verify cache duration changes are implemented consistently.
The Cache-Control header expectations have been updated to use uniform 120-second durations. Notably, stale-if-error is reduced from 4 hours to 2 minutes, which should help prevent long-lived error responses (aligning with the PR's 500 error handling objective).
Run the following script to verify that the implementation code has been updated to match these test expectations:
🏁 Script executed:
#!/bin/bash
# Description: Verify that cache-control implementation matches the updated test expectations.
# Expected: Find the actual cache-control logic that sets these values to 120.
# Search for cache-control header setting in the codebase
rg -n -A3 -B3 "s-maxage.*120|stale-while-revalidate.*120|stale-if-error.*120" --type=js -g '!test/**'
# Also search for the old values to ensure they've been updated everywhere
rg -n "s-maxage.*900|stale-while-revalidate.*1000|stale-if-error.*14400" --type=jsLength of output: 2643
🏁 Script executed:
#!/bin/bash
# Inspect cdn-caching.js to understand the conditional logic around cache-control headers
sed -n '30,55p' server/handlers/cdn-caching.js
# Also check other handlers that still have old values
echo "=== json-manifest-handlers.js ==="
sed -n '8,18p' server/handlers/json-manifest-handlers.js
echo "=== generate-service-worker.js ==="
sed -n '40,50p' server/handlers/generate-service-worker.jsLength of output: 1978
🏁 Script executed:
#!/bin/bash
# Understand the networkOnly flag and its purpose
echo "=== Context around networkOnly flag in cdn-caching.js ==="
sed -n '1,50p' server/handlers/cdn-caching.js | head -30
# Check if networkOnly is set in the test that's being verified
echo "=== Check url-redirect-test.js for networkOnly parameter ==="
sed -n '610,630p' test/integration/url-redirect-test.js
# Search for commits or comments explaining the differentiation
echo "=== Search for networkOnly usage across codebase ==="
rg -n "networkOnly" --type=js -B2 -A2 | head -40Length of output: 3567
🏁 Script executed:
#!/bin/bash
# Find comments or documentation about networkOnly
echo "=== Search for networkOnly context and comments ==="
rg -n "networkOnly" --type=js -B5 -A1 server/handlers/cdn-caching.js | head -60
# Check if there's a constant for the old durations
echo "=== Check constants file for STALE_IF_ERROR_CACHE_DURATION ==="
rg -n "STALE_IF_ERROR_CACHE_DURATION|stale.*while.*revalidate|1000" constants.js | head -10
# Check git log for recent changes to understand intent
echo "=== Look for PR context or related changes ==="
git log --oneline -20 | head -20Length of output: 1546
Implementation incomplete: Cache durations not updated across all handlers and code paths.
The test expectations at line 622 are correct—the non-networkOnly path in cdn-caching.js (line 45) properly uses the new 120-second durations. However, the implementation is inconsistent:
cdn-caching.jsline 37-40 (networkOnly=truepath): Still uses old durations withstale-while-revalidate=1000andstale-if-error=${STALE_IF_ERROR_CACHE_DURATION}(14400s)json-manifest-handlers.js(line 13): Not updated—still usesstale-while-revalidate=1000generate-service-worker.js(line 44): Not updated—still usesstale-while-revalidate=1000
The commit message ("changed all cache windows to 2 min") suggests broader intent, but the actual changes are incomplete. Verify whether the networkOnly path and other handlers should also use uniform 120-second durations or if differentiated cache policies are intentional.
🤖 Prompt for AI Agents
In test/integration/url-redirect-test.js around line 622 and the related source
files cdn-caching.js (lines ~37-40 and ~45), json-manifest-handlers.js (line
~13), and generate-service-worker.js (line ~44): the test expects Cache-Control
values with stale-while-revalidate=120 and stale-if-error=120 but several code
paths still use the old values (stale-while-revalidate=1000 and
stale-if-error=${STALE_IF_ERROR_CACHE_DURATION} / 14400). Update the networkOnly
branch in cdn-caching.js and the cache header strings in
json-manifest-handlers.js and generate-service-worker.js to use
stale-while-revalidate=120 and stale-if-error=120 (or update the shared
cache-duration constants so all references resolve to 120), ensuring all
handlers and code paths produce the uniform 120-second durations, then run the
integration tests to verify the updated headers.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/integration/url-redirect-test.js (1)
622-622: Cache-Control duration inconsistency already flagged.The Cache-Control header values on this line are part of the broader inconsistency issue already documented in the past review comments.
🧹 Nitpick comments (2)
test/integration/url-redirect-test.js (1)
511-611: LGTM! Test formatting improvements enhance readability.The refactoring from multi-line to single-line chained SuperTest calls is a good stylistic improvement that reduces verbosity without affecting test behavior. This makes the tests more concise and easier to scan.
test/integration/custom-route-handler-test.js (1)
50-216: LGTM! Formatting refactors improve test consistency.The changes collapsing multi-line strings, object literals, and function bodies to single-line expressions are purely stylistic improvements with no functional impact. This enhances readability and maintains consistency across the test suite.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/handlers/isomorphic-handler.js(2 hunks)test/integration/custom-route-handler-test.js(12 hunks)test/integration/isomorphic-handler-test.js(2 hunks)test/integration/url-redirect-test.js(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/handlers/isomorphic-handler.js
⏰ 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). (1)
- GitHub Check: default
🔇 Additional comments (3)
test/integration/isomorphic-handler-test.js (2)
174-188: LGTM! Test simplification is appropriate for cascading failures.When both
loadDataandloadErrorDatacrash, returning only the 500 status code without a body is the correct fallback behavior. The simplified assertion properly validates this.
157-172: No issues found. The test correctly reflects the 500 error handling behavior.The verification confirms that:
- For statusCode >= 500, the implementation calls
res.sendStatus(statusCode)and returns, bypassing all error page rendering- The test's assertion of only the 500 status code (without body validation) is correct, since
res.sendStatus()sends no custom response bodyloadErrorDataremains necessary in the test setup—it's called in the error handler to extract thehttpStatusCodevalue that determines whether rendering is skipped- The test simplification is appropriate and accurately reflects the new behavior
test/integration/custom-route-handler-test.js (1)
124-268: Verify Cache-Control duration consistency across handlers.Multiple tests in this file expect Cache-Control headers with
s-maxage=900, stale-while-revalidate=1000, stale-if-error=14400. This matches the values inurl-redirect-test.jsthat were flagged in a previous review as potentially inconsistent with the implementation.Ensure that:
- These expected values match the actual implementation for custom route handlers
- If the broader cache duration update to 120 seconds applies to these endpoints, update these test expectations accordingly
- Cache policies are intentionally differentiated or consistently applied across all handlers
Based on learnings (past review comments on url-redirect-test.js).
2 participants
Description
Please include the summary of the change made. Please include the required context here. Please include the issue reference for the fix and the dependencies reference for the change.
Fixes # (issue-reference)
https://github.com/quintype/page-builder/issues/4117
Type of change
Please delete options that are not relevant.
Checklist:
Summary by CodeRabbit
Bug Fixes
Tests