fix: adjust how ComponentErrorBoundary wraps components#1236
Conversation
|
Warning: Component files have been updated but no migrations have been added. See https://github.com/yext/visual-editor/blob/main/packages/visual-editor/src/components/migrations/README.md for more information. |
auto-screenshot-update: true
|
Warning Review limit reached
More reviews will be available in 13 minutes and 13 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR refactors error-boundary placement from individual page section components to a centralized config-wrapping approach. It introduces Sequence DiagramsequenceDiagram
participant ConfigInput as Config Input
participant wrapConfig as wrapConfigWithComponentErrorBoundary
participant ComponentErrorBoundary as CEB
participant VisualEditorRender
participant PuckRender
ConfigInput->>wrapConfig: pass original config
wrapConfig->>wrapConfig: iterate config.components
wrapConfig->>ComponentErrorBoundary: wrap component.render
wrapConfig-->>VisualEditorRender: return wrapped config
VisualEditorRender->>PuckRender: render(wrappedConfig, data)
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
asanehisa
requested review from
benlife5,
briantstephan,
jwartofsky-yext and
mkilpatrick
and removed request for
briantstephan and
mkilpatrick
… into component-error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/visual-editor/src/internal/utils/wrapConfigWithComponentErrorBoundary.test.tsx (1)
60-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate stale memoization expectations in this test.
Line 72-75 asserts identity memoization, but
wrapConfigWithComponentErrorBoundarynow returns a fresh wrapped config for non-emptycomponents. This test will fail against the current implementation contract.Suggested test update
- it("memoizes wrapped configs by input identity", () => { + it("returns a newly wrapped config on each call", () => { @@ - expect(secondWrappedConfig).toBe(firstWrappedConfig); - expect(secondWrappedConfig.components.Banner.render).toBe( - firstWrappedConfig.components.Banner.render - ); + expect(secondWrappedConfig).not.toBe(firstWrappedConfig); + expect(secondWrappedConfig.components.Banner.render).not.toBe( + firstWrappedConfig.components.Banner.render + ); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/visual-editor/src/internal/utils/wrapConfigWithComponentErrorBoundary.test.tsx` around lines 60 - 75, The test incorrectly asserts identity memoization for wrapConfigWithComponentErrorBoundary when config.components is non-empty; update the test to reflect that a fresh wrapped config is returned for configs with components: replace the identity assertions (expect(...).toBe(...)) with checks that the returned config is not the same object as the input but that component renders are wrapped (e.g., component key exists and its render is a function distinct from the original render or that calling it returns expected JSX/error-boundary behavior). Locate wrapConfigWithComponentErrorBoundary and the test case in wrapConfigWithComponentErrorBoundary.test.tsx and modify assertions accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@packages/visual-editor/src/internal/utils/wrapConfigWithComponentErrorBoundary.test.tsx`:
- Around line 60-75: The test incorrectly asserts identity memoization for
wrapConfigWithComponentErrorBoundary when config.components is non-empty; update
the test to reflect that a fresh wrapped config is returned for configs with
components: replace the identity assertions (expect(...).toBe(...)) with checks
that the returned config is not the same object as the input but that component
renders are wrapped (e.g., component key exists and its render is a function
distinct from the original render or that calling it returns expected
JSX/error-boundary behavior). Locate wrapConfigWithComponentErrorBoundary and
the test case in wrapConfigWithComponentErrorBoundary.test.tsx and modify
assertions accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 00ccabfd-e7bd-4aa6-bf8c-5d77f76d0197
⛔ Files ignored due to path filters (1)
packages/visual-editor/src/components/testing/screenshots/PhotoGallerySection/[desktop] version 59 with showSectionHeading false.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (29)
packages/visual-editor/locales/platform/cs/visual-editor.jsonpackages/visual-editor/locales/platform/da/visual-editor.jsonpackages/visual-editor/locales/platform/de/visual-editor.jsonpackages/visual-editor/locales/platform/en-GB/visual-editor.jsonpackages/visual-editor/locales/platform/en/visual-editor.jsonpackages/visual-editor/locales/platform/es/visual-editor.jsonpackages/visual-editor/locales/platform/et/visual-editor.jsonpackages/visual-editor/locales/platform/fi/visual-editor.jsonpackages/visual-editor/locales/platform/fr/visual-editor.jsonpackages/visual-editor/locales/platform/hr/visual-editor.jsonpackages/visual-editor/locales/platform/hu/visual-editor.jsonpackages/visual-editor/locales/platform/it/visual-editor.jsonpackages/visual-editor/locales/platform/ja/visual-editor.jsonpackages/visual-editor/locales/platform/lt/visual-editor.jsonpackages/visual-editor/locales/platform/lv/visual-editor.jsonpackages/visual-editor/locales/platform/nb/visual-editor.jsonpackages/visual-editor/locales/platform/nl/visual-editor.jsonpackages/visual-editor/locales/platform/pl/visual-editor.jsonpackages/visual-editor/locales/platform/pt/visual-editor.jsonpackages/visual-editor/locales/platform/ro/visual-editor.jsonpackages/visual-editor/locales/platform/sk/visual-editor.jsonpackages/visual-editor/locales/platform/sv/visual-editor.jsonpackages/visual-editor/locales/platform/tr/visual-editor.jsonpackages/visual-editor/locales/platform/zh-TW/visual-editor.jsonpackages/visual-editor/locales/platform/zh/visual-editor.jsonpackages/visual-editor/src/editor/VisualEditorRender.tsxpackages/visual-editor/src/internal/components/ComponentErrorBoundary.tsxpackages/visual-editor/src/internal/utils/wrapConfigWithComponentErrorBoundary.test.tsxpackages/visual-editor/src/internal/utils/wrapConfigWithComponentErrorBoundary.tsx
✅ Files skipped from review due to trivial changes (15)
- packages/visual-editor/locales/platform/en/visual-editor.json
- packages/visual-editor/locales/platform/es/visual-editor.json
- packages/visual-editor/locales/platform/de/visual-editor.json
- packages/visual-editor/locales/platform/da/visual-editor.json
- packages/visual-editor/locales/platform/cs/visual-editor.json
- packages/visual-editor/locales/platform/lv/visual-editor.json
- packages/visual-editor/locales/platform/fr/visual-editor.json
- packages/visual-editor/locales/platform/et/visual-editor.json
- packages/visual-editor/locales/platform/it/visual-editor.json
- packages/visual-editor/locales/platform/zh-TW/visual-editor.json
- packages/visual-editor/locales/platform/zh/visual-editor.json
- packages/visual-editor/locales/platform/sv/visual-editor.json
- packages/visual-editor/locales/platform/sk/visual-editor.json
- packages/visual-editor/src/internal/components/ComponentErrorBoundary.tsx
- packages/visual-editor/locales/platform/ro/visual-editor.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/visual-editor/src/editor/VisualEditorRender.tsx
auto-screenshot-update: true
4 participants
Instead of wrapping each component in the component code, wrap any components passed through our Edit or Render.
Tested by temporarily forcing the Banner to error
https://github.com/user-attachments/assets/da0c1595-b5b3-4dee-af4f-17548b9bde82