fix: contain HttpError within sections instead of crashing the entire page#1152
fix: contain HttpError within sections instead of crashing the entire page#1152nicacioliveira wants to merge 1 commit intomainfrom
Conversation
… page When a section's loader throws an HttpError (e.g., 429, 500 from upstream APIs), the error was re-thrown at three levels — wrapCaughtErrors, section resolver catch, and ErrorBoundary — causing the entire page to return the error status code instead of containing the failure within the affected section. This caused production incidents where a single ProductShelf section failing with a 429 from VTEX would make the entire home page return 429, triggering Cloudflare healthcheck failures (marked as Unhealthy/404). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tagging OptionsShould a new tag be published when this PR is merged?
|
📝 WalkthroughWalkthroughThe changes remove special-case error handling for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
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/loader.ts (1)
97-138:⚠️ Potential issue | 🔴 CriticalVerify client-side redirect handling in render.tsx still works.
The
wrapCaughtErrorsfunction wraps errors (includingHttpError) in a proxy when not in invoke context. TheisInvokeCtxcheck at line 103 correctly preservesHttpErrorpropagation for direct invoke calls. However, render.tsx does not use invoke context.When render.tsx calls
state.deco.render(), the underlyingstate.resolve()has noisInvokeflag set. This means loaders that throwHttpErrorare wrapped bywrapCaughtErrorsand returned as proxy values rather than thrown as exceptions. The try/catch block at runtime/features/render.tsx:94-102 never catches these wrapped errors since they are returned as values, not thrown. Consequently, theinstanceof HttpErrorcheck at line 113 becomes unreachable, breaking client-side redirect handling.This is a critical issue: HttpError-based redirects from loaders will silently fail to trigger error handling logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blocks/loader.ts` around lines 97 - 138, wrapCaughtErrors currently converts thrown HttpError into a proxy when not in an invoke context, causing render.tsx (which calls state.deco.render() / state.resolve()) to receive a value instead of an exception and thus miss HttpError-based redirects; update wrapCaughtErrors (the catch block in wrapCaughtErrors) to detect HttpError (the HttpError class/type used by your loaders) and rethrow it immediately instead of returning a proxy so downstream callers like render.tsx can catch instanceof HttpError, e.g. add an early if (err instanceof HttpError) throw err before creating/returning the Proxy.
🧹 Nitpick comments (2)
components/section.tsx (1)
18-18: Potential formatting issue - verify withdeno fmt.Similar to
blocks/section.ts, this blank line may have formatting issues. Rundeno fmtto ensure consistency and avoid CI failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/section.tsx` at line 18, There’s a likely formatting/blank-line inconsistency in components/section.tsx (mirrors blocks/section.ts) that can fail CI; run deno fmt and fix any whitespace/blank-line differences so the file matches project formatting conventions (remove the stray blank line or adjust surrounding whitespace), then re-run the formatter to ensure components/section.tsx is consistent with blocks/section.ts.blocks/loader.ts (1)
9-9: Potential formatting issue - verify withdeno fmt.This blank line may have formatting issues similar to
blocks/section.ts. Ensure consistent formatting across all changed files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blocks/loader.ts` at line 9, There’s a potential formatting/whitespace issue in the loader.ts module: run deno fmt and remove any stray blank line or trailing whitespace around the module header (the blank line near the top of the loader.ts file) so the file matches the project's formatting rules; ensure the file ends with a single newline and no extra blank lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@blocks/section.ts`:
- Line 22: Run the formatter (deno fmt) on the section module to fix the CI
failing style check; remove any trailing whitespace or incorrect blank line
around the exported Section symbol (or the module's default export) so
formatting matches the project's style, then commit the formatted file.
---
Outside diff comments:
In `@blocks/loader.ts`:
- Around line 97-138: wrapCaughtErrors currently converts thrown HttpError into
a proxy when not in an invoke context, causing render.tsx (which calls
state.deco.render() / state.resolve()) to receive a value instead of an
exception and thus miss HttpError-based redirects; update wrapCaughtErrors (the
catch block in wrapCaughtErrors) to detect HttpError (the HttpError class/type
used by your loaders) and rethrow it immediately instead of returning a proxy so
downstream callers like render.tsx can catch instanceof HttpError, e.g. add an
early if (err instanceof HttpError) throw err before creating/returning the
Proxy.
---
Nitpick comments:
In `@blocks/loader.ts`:
- Line 9: There’s a potential formatting/whitespace issue in the loader.ts
module: run deno fmt and remove any stray blank line or trailing whitespace
around the module header (the blank line near the top of the loader.ts file) so
the file matches the project's formatting rules; ensure the file ends with a
single newline and no extra blank lines.
In `@components/section.tsx`:
- Line 18: There’s a likely formatting/blank-line inconsistency in
components/section.tsx (mirrors blocks/section.ts) that can fail CI; run deno
fmt and fix any whitespace/blank-line differences so the file matches project
formatting conventions (remove the stray blank line or adjust surrounding
whitespace), then re-run the formatter to ensure components/section.tsx is
consistent with blocks/section.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c77a5093-9c29-4583-9eab-55ccc6a7e4d4
📒 Files selected for processing (3)
blocks/loader.tsblocks/section.tscomponents/section.tsx
| } from "../engine/block.ts"; | ||
| import type { Resolver } from "../engine/core/resolver.ts"; | ||
| import { HttpError } from "../engine/errors.ts"; | ||
|
|
There was a problem hiding this comment.
Fix formatting to pass CI.
The pipeline reports a deno fmt --check failure at this location. Run deno fmt to fix the formatting issue (likely a trailing whitespace or incorrect blank line).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@blocks/section.ts` at line 22, Run the formatter (deno fmt) on the section
module to fix the CI failing style check; remove any trailing whitespace or
incorrect blank line around the exported Section symbol (or the module's default
export) so formatting matches the project's style, then commit the formatted
file.
Problem
When a section's loader throws an
HttpError(e.g.,429 Too Many Requestsor500 Internal Server Errorfrom upstream APIs like VTEX), the error propagates through three layers and crashes the entire page instead of being contained within the affected section:blocks/loader.ts—wrapCaughtErrors: catches errors but re-throwsHttpErrorblocks/section.ts— section resolver.catch(): catches errors but re-throwsHttpErrorcomponents/section.tsx—ErrorBoundary.getDerivedStateFromError: catches errors but re-throwsHttpErrorThis means a single failing section (e.g., a
ProductShelfshowing related products) makes the entire page return the upstream error status code.Real-world impact (production incidents)
We observed this across multiple production sites:
lojavirtual.lumini.com.br)HttpError 429: Too Many RequestsrelatedProductsLoaderSyntaxError: Unterminated string in JSONstaleWhileRevalidatewww.hectoralbertazzi.com.br)HttpError 429: Too Many RequestsrelatedProductsLoaderwww.petfarma.com)TypeError: Cannot destructure property 'products'/offlinepageCloudflare healthchecks hit the home page, receive a 404/429 (from a single section's loader failure), and mark the site as Unhealthy — even though the pod is running fine and the page would render correctly if the failing section was contained.
Error flow (before this fix)
Error flow (after this fix)
Changes
blocks/loader.ts: RemoveHttpErrorre-throw inwrapCaughtErrors— treat all errors the same way (wrap in error Proxy)blocks/section.ts: RemoveHttpErrorre-throw in section resolver.catch()— let all errors go through theErrorBoundarypathcomponents/section.tsx: RemoveHttpErrorre-throw inErrorBoundary.getDerivedStateFromError— contain all errors and renderErrorFallbackTest plan
HttpError(429, 500) still return 200 and render theErrorFallbackfor the affected sectioninvokecalls still propagateHttpErrorcorrectly (theisInvokeCtxcheck inwrapCaughtErrorsis preserved)🤖 Generated with Claude Code
Summary by cubic
Contain
HttpErrors thrown by section loaders so only the affected section shows its fallback instead of crashing the entire page. Pages now return 200; directinvokecalls still propagateHttpErroras before.HttpErrorre-throws inblocks/loader.ts(wrapCaughtErrors),blocks/section.ts(section resolver.catch), andcomponents/section.tsx(ErrorBoundary.getDerivedStateFromError) to route errors into the section boundary.ErrorFallback; page status no longer mirrors upstream 429/500, preventing false healthcheck failures.Written for commit 15cb1c0. Summary will update on new commits.
Summary by CodeRabbit