fix: hide components with missing entity data#1159
Open
briantstephan wants to merge 17 commits intomainfrom
Open
fix: hide components with missing entity data#1159briantstephan wants to merge 17 commits intomainfrom
briantstephan wants to merge 17 commits intomainfrom
Conversation
Contributor
|
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. |
Contributor
|
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:
WalkthroughThis pull request introduces a new feature to handle empty-state display for entity-mapped page sections. It adds a new Sequence DiagramsequenceDiagram
participant Section as Section Component
participant ResolveData as resolveData
participant CardWrapper as CardWrapperComponent
participant Observer as useMappedEntitySectionEmptyState
participant Render as render()
participant DOM as DOM
Section->>ResolveData: compute conditionalRender state
ResolveData->>CardWrapper: determine if mapped content exists
CardWrapper-->>ResolveData: hasMappedContent: true/false
ResolveData-->>Section: return updated props with conditionalRender
Section->>Observer: call hook with enabled flag
Observer->>DOM: setWrapperRef(element)
Observer->>Observer: setup MutationObserver on wrapper
Section->>Render: render with conditionalRender state
alt hasMappedContent is false AND not editing
Render-->>DOM: render nothing/empty marker
Observer->>DOM: query for [data-empty-state="true"]
Observer->>Observer: set isMappedContentEmpty = true
else hasMappedContent exists OR is editing
Render-->>DOM: render normal content
Observer->>DOM: query for [data-empty-state="true"]
Observer->>Observer: set isMappedContentEmpty = false
end
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
commit: |
auto-screenshot-update: true
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/visual-editor/src/components/pageSections/useMappedEntitySectionEmptyState.ts (1)
16-45: Potential timing issue with ref and effect dependencies.The
useEffectonly depends onenabled, butwrapperRef.currentis read inside the effect. If the wrapper element is set after the effect runs (or changes withoutenabledchanging), the observer won't be attached to the correct element.This may be safe in practice because:
- The ref callback runs synchronously during render
- The effect runs after render, when
wrapperRef.currentis already set- The parent components likely don't dynamically swap the wrapper element
However, if you want to be defensive, consider adding the ref value to dependencies or using a state-based approach:
♻️ Optional: More robust ref tracking
export const useMappedEntitySectionEmptyState = ({ enabled, }: { enabled: boolean; }) => { - const wrapperRef = React.useRef<HTMLDivElement | null>(null); + const [wrapperElement, setWrapperElement] = React.useState<HTMLDivElement | null>(null); const [isMappedContentEmpty, setIsMappedContentEmpty] = React.useState(false); - const setWrapperRef = React.useCallback((element: HTMLDivElement | null) => { - wrapperRef.current = element; - }, []); + const setWrapperRef = React.useCallback((element: HTMLDivElement | null) => { + setWrapperElement(element); + }, []); React.useEffect(() => { if (!enabled) { setIsMappedContentEmpty(false); return; } - const element = wrapperRef.current; + const element = wrapperElement; if (!element) { return; } // ... rest unchanged - }, [enabled]); + }, [enabled, wrapperElement]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/pageSections/useMappedEntitySectionEmptyState.ts` around lines 16 - 45, The effect that sets up the MutationObserver reads wrapperRef.current but only lists enabled in its dependency array, so if the ref element is assigned/changed after the effect ran the observer won't attach to the correct element; update the effect to also depend on the ref value (wrapperRef.current) or its resolved element, e.g. read const element = wrapperRef.current into a stable variable and include that in the dependency array, and ensure you clean up & reconnect the observer in useEffect when the element changes; adjust references to wrapperRef, useEffect, updateEmptyState, EMPTY_STATE_MARKER_SELECTOR, and setIsMappedContentEmpty accordingly so the observer always attaches to the current element.packages/visual-editor/src/components/pageSections/EntityMappedSectionEmptyState.test.tsx (1)
241-243: Assert DOM absence here, not just empty text.
textContent?.trim() === ""still passes if the section leaves a hidden wrapper or empty marker mounted. Since this PR is validating that the section is actually hidden, these checks should assert that nothing was rendered (for example,container.childElementCount === 0).✅ Stronger assertion
- await waitFor(() => { - expect(result.container.textContent?.trim()).toBe(""); - }); + await waitFor(() => { + expect(result.container.childElementCount).toBe(0); + });Also applies to: 502-504
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/pageSections/EntityMappedSectionEmptyState.test.tsx` around lines 241 - 243, The current assertion uses result.container.textContent?.trim() to check emptiness but can miss hidden wrappers; update the test in EntityMappedSectionEmptyState.test.tsx (inside the await waitFor block that currently expects empty text) to assert DOM absence instead, e.g. wait for result.container.childElementCount === 0 or assert that a queryByTestId for the section returns null; replace the textContent assertion at the problematic waitFor (and the similar assertions at the other occurrences around lines 502-504) with this stronger DOM absence check so the section is truly not rendered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/visual-editor/src/components/pageSections/PhotoGallerySection/photoGalleryUtils.ts`:
- Around line 53-54: The resolved localized asset image stored in the local
variable image (from isLocalizedAssetImage/rawImage.assetImage +
resolveLocalizedAssetImage) is not being used for dimensions; update the
branches where image is set to read height and width from image (the resolved
AssetImageType) instead of rawImage.height/rawImage.width so that entries with
assetImage use the asset's actual dimensions; apply the same change to the other
branch range mentioned (lines 83-113) wherever rawImage.assetImage is resolved
to image.
---
Nitpick comments:
In
`@packages/visual-editor/src/components/pageSections/EntityMappedSectionEmptyState.test.tsx`:
- Around line 241-243: The current assertion uses
result.container.textContent?.trim() to check emptiness but can miss hidden
wrappers; update the test in EntityMappedSectionEmptyState.test.tsx (inside the
await waitFor block that currently expects empty text) to assert DOM absence
instead, e.g. wait for result.container.childElementCount === 0 or assert that a
queryByTestId for the section returns null; replace the textContent assertion at
the problematic waitFor (and the similar assertions at the other occurrences
around lines 502-504) with this stronger DOM absence check so the section is
truly not rendered.
In
`@packages/visual-editor/src/components/pageSections/useMappedEntitySectionEmptyState.ts`:
- Around line 16-45: The effect that sets up the MutationObserver reads
wrapperRef.current but only lists enabled in its dependency array, so if the ref
element is assigned/changed after the effect ran the observer won't attach to
the correct element; update the effect to also depend on the ref value
(wrapperRef.current) or its resolved element, e.g. read const element =
wrapperRef.current into a stable variable and include that in the dependency
array, and ensure you clean up & reconnect the observer in useEffect when the
element changes; adjust references to wrapperRef, useEffect, updateEmptyState,
EMPTY_STATE_MARKER_SELECTOR, and setIsMappedContentEmpty accordingly so the
observer always attaches to the current element.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9610805e-bef3-4695-afc0-9c729edd05fe
📒 Files selected for processing (20)
packages/visual-editor/src/components/pageSections/EntityFieldSectionEmptyState.tsxpackages/visual-editor/src/components/pageSections/EntityMappedSectionEmptyState.test.tsxpackages/visual-editor/src/components/pageSections/EventSection/EventCardsWrapper.tsxpackages/visual-editor/src/components/pageSections/EventSection/EventSection.tsxpackages/visual-editor/src/components/pageSections/FAQsSection/FAQsSection.tsxpackages/visual-editor/src/components/pageSections/InsightSection/InsightCardsWrapper.tsxpackages/visual-editor/src/components/pageSections/InsightSection/InsightSection.tsxpackages/visual-editor/src/components/pageSections/PhotoGallerySection/PhotoGallerySection.tsxpackages/visual-editor/src/components/pageSections/PhotoGallerySection/PhotoGalleryWrapper.tsxpackages/visual-editor/src/components/pageSections/PhotoGallerySection/photoGalleryUtils.test.tspackages/visual-editor/src/components/pageSections/PhotoGallerySection/photoGalleryUtils.tspackages/visual-editor/src/components/pageSections/ProductSection/ProductCardsWrapper.tsxpackages/visual-editor/src/components/pageSections/ProductSection/ProductSection.tsxpackages/visual-editor/src/components/pageSections/TeamSection/TeamCardsWrapper.tsxpackages/visual-editor/src/components/pageSections/TeamSection/TeamSection.tsxpackages/visual-editor/src/components/pageSections/TestimonialSection/TestimonialCardsWrapper.tsxpackages/visual-editor/src/components/pageSections/TestimonialSection/TestimonialSection.tsxpackages/visual-editor/src/components/pageSections/entityFieldSectionUtils.tspackages/visual-editor/src/components/pageSections/useMappedEntitySectionEmptyState.tspackages/visual-editor/src/docs/ai/components.d.ts
auto-screenshot-update: true
…l-editor into hide-empty-components
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/visual-editor/src/components/pageSections/ProductSection/ProductSection.tsx`:
- Around line 290-295: The empty-state is being decided only after mount via
useMappedEntitySectionEmptyState (setWrapperRef, isMappedContentEmpty), causing
a flash; compute an initial isMappedContentEmpty synchronously from available
props (e.g., props.mappedEntities / props.children / props.conditionalRender
data) or expose an initial value to the hook so you can decide before first
render, then use that precomputed boolean to conditionally skip rendering the
section when watchForMappedContentEmptyState is true; update the ProductSection
usage (and the identical wrapper-backed sections) to consult this sync check
rather than relying solely on the post-mount DOM observer.
- Around line 312-329: The hidden wrapper currently unmounts in live mode
because the JSX returns an empty fragment when props.puck.isEditing is false,
causing setWrapperRef(null) and leaving isMappedContentEmpty stuck; instead
always render the hidden wrapper div with ref={setWrapperRef} and the
cardsWrapperSlot (className="hidden" aria-hidden="true") regardless of
props.puck.isEditing, and only conditionally render the
EntityFieldSectionEmptyState when watchForMappedContentEmptyState &&
isMappedContentEmpty && props.puck.isEditing; update the JSX around
watchForMappedContentEmptyState/isMappedContentEmpty/props.puck.isEditing to
keep the wrapper mounted while gating only the empty-state UI.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2a560e1e-120f-4868-8ac7-87d283fd6a63
⛔ Files ignored due to path filters (6)
packages/visual-editor/src/components/testing/screenshots/EventSection/[desktop] version 26 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/EventSection/[desktop] version 51 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/InsightSection/[mobile] version 33 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/InsightSection/[tablet] version 33 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] latest version multi-pageset default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] latest version default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (11)
packages/visual-editor/src/components/pageSections/EntityFieldSectionEmptyState.tsxpackages/visual-editor/src/components/pageSections/EntityMappedSectionEmptyState.test.tsxpackages/visual-editor/src/components/pageSections/EventSection/EventSection.tsxpackages/visual-editor/src/components/pageSections/FAQsSection/FAQsSection.tsxpackages/visual-editor/src/components/pageSections/InsightSection/InsightSection.tsxpackages/visual-editor/src/components/pageSections/PhotoGallerySection/PhotoGallerySection.tsxpackages/visual-editor/src/components/pageSections/ProductSection/ProductSection.tsxpackages/visual-editor/src/components/pageSections/TeamSection/TeamSection.tsxpackages/visual-editor/src/components/pageSections/TestimonialSection/TestimonialSection.tsxpackages/visual-editor/src/components/pageSections/entityFieldSectionUtils.tspackages/visual-editor/src/components/pageSections/useSelectEditorItem.ts
✅ Files skipped from review due to trivial changes (1)
- packages/visual-editor/src/components/pageSections/EntityMappedSectionEmptyState.test.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/visual-editor/src/components/pageSections/EventSection/EventSection.tsx
- packages/visual-editor/src/components/pageSections/EntityFieldSectionEmptyState.tsx
- packages/visual-editor/src/components/pageSections/PhotoGallerySection/PhotoGallerySection.tsx
- packages/visual-editor/src/components/pageSections/FAQsSection/FAQsSection.tsx
- packages/visual-editor/src/components/pageSections/entityFieldSectionUtils.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/visual-editor/src/components/pageSections/EntityFieldSectionEmptyState.tsx (1)
33-44: Consolidate duplicate click handlers to reduce indirection.
handleClickandhandleClickCaptureboth only forward tohandleSelectTarget, so you can remove the wrappers and bindhandleSelectTargetdirectly.♻️ Proposed simplification
- const handleClick = React.useCallback( - (event: React.MouseEvent<HTMLDivElement>) => { - handleSelectTarget(event); - }, - [handleSelectTarget] - ); - const handleClickCapture = React.useCallback( - (event: React.MouseEvent<HTMLDivElement>) => { - handleSelectTarget(event); - }, - [handleSelectTarget] - ); @@ - onClick={isInteractive ? handleClick : undefined} - onClickCapture={isInteractive ? handleClickCapture : undefined} + onClick={isInteractive ? handleSelectTarget : undefined} + onClickCapture={isInteractive ? handleSelectTarget : undefined}Also applies to: 61-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/pageSections/EntityFieldSectionEmptyState.tsx` around lines 33 - 44, handleClick and handleClickCapture are redundant wrappers that only call handleSelectTarget; replace their usages by binding handleSelectTarget directly (remove the React.useCallback wrappers for handleClick and handleClickCapture and use handleSelectTarget where those handlers are attached). Update any JSX props or event bindings that reference handleClick or handleClickCapture to reference handleSelectTarget (and remove the now-unused handleClick/handleClickCapture declarations) to eliminate unnecessary indirection.packages/visual-editor/src/components/pageSections/EntityMappedSectionEmptyState.test.tsx (1)
581-611: Tighten click-selection assertions with exact call counts.These tests currently pass even if selection fires multiple times. Add call-count assertions to catch duplicate dispatch regressions.
✅ Suggested assertion hardening
it("selects the FAQ section when the empty state is clicked", async () => { @@ fireEvent.click(emptyState); + expect(mockSelectEditorItem).toHaveBeenCalledTimes(1); expect(mockSelectEditorItem).toHaveBeenCalledWith("FAQSection-test"); }); it("selects the photo gallery wrapper when the empty state is clicked", async () => { @@ fireEvent.click(emptyState); + expect(mockSelectEditorItem).toHaveBeenCalledTimes(1); expect(mockSelectEditorItem).toHaveBeenCalledWith( "PhotoGalleryWrapper-test" ); }); it("selects the cards wrapper for wrapper-backed sections", async () => { @@ fireEvent.click(emptyState); + expect(mockSelectEditorItem).toHaveBeenCalledTimes(1); expect(mockSelectEditorItem).toHaveBeenCalledWith( "ProductCardsWrapper-test" ); }); @@ it("prevents parent native click handlers from overriding the empty-state selection", () => { @@ fireEvent.click(result.getByRole("button")); expect(parentClickHandler).not.toHaveBeenCalled(); + expect(mockSelectEditorItem).toHaveBeenCalledTimes(1); expect(mockSelectEditorItem).toHaveBeenCalledWith("FAQSection-test"); });Also applies to: 613-630, 647-682
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/pageSections/EntityMappedSectionEmptyState.test.tsx` around lines 581 - 611, The tests in EntityMappedSectionEmptyState.test.tsx call mockSelectEditorItem but don't assert exact call counts, allowing duplicate dispatches; for each test that clicks the empty state (e.g., the tests using renderSection with FAQSection and PhotoGallerySection and the other blocks at lines noted), add an assertion that mockSelectEditorItem was called exactly once (use toHaveBeenCalledTimes(1)) immediately after the existing toHaveBeenCalledWith(...) check and ensure the mock is reset/cleared between tests (beforeEach/afterEach) so the count assertion is reliable; apply this same single-call assertion pattern to the other test blocks referenced (lines 613-630 and 647-682).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/visual-editor/src/components/pageSections/EntityFieldSectionEmptyState.tsx`:
- Around line 33-44: handleClick and handleClickCapture are redundant wrappers
that only call handleSelectTarget; replace their usages by binding
handleSelectTarget directly (remove the React.useCallback wrappers for
handleClick and handleClickCapture and use handleSelectTarget where those
handlers are attached). Update any JSX props or event bindings that reference
handleClick or handleClickCapture to reference handleSelectTarget (and remove
the now-unused handleClick/handleClickCapture declarations) to eliminate
unnecessary indirection.
In
`@packages/visual-editor/src/components/pageSections/EntityMappedSectionEmptyState.test.tsx`:
- Around line 581-611: The tests in EntityMappedSectionEmptyState.test.tsx call
mockSelectEditorItem but don't assert exact call counts, allowing duplicate
dispatches; for each test that clicks the empty state (e.g., the tests using
renderSection with FAQSection and PhotoGallerySection and the other blocks at
lines noted), add an assertion that mockSelectEditorItem was called exactly once
(use toHaveBeenCalledTimes(1)) immediately after the existing
toHaveBeenCalledWith(...) check and ensure the mock is reset/cleared between
tests (beforeEach/afterEach) so the count assertion is reliable; apply this same
single-call assertion pattern to the other test blocks referenced (lines 613-630
and 647-682).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 061a3161-0a87-4b5b-b581-91b055d42b6f
📒 Files selected for processing (3)
packages/visual-editor/src/components/pageSections/EntityFieldSectionEmptyState.tsxpackages/visual-editor/src/components/pageSections/EntityMappedSectionEmptyState.test.tsxpackages/visual-editor/src/components/pageSections/entityFieldSectionUtils.ts
✅ Files skipped from review due to trivial changes (1)
- packages/visual-editor/src/components/pageSections/entityFieldSectionUtils.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/visual-editor/src/components/pageSections/ProductSection/ProductSection.tsx (1)
269-279:⚠️ Potential issue | 🟠 MajorThis still hides the section too late, then drops the observed wrapper.
resolveData()only tells the section to start watching. The hook starts withisMappedContentEmpty = false, so a mapped-empty section can paint once on the live page before the effect flips it hidden. After that, the<></>branch unmounts the observed wrapper entirely, so there is nothing left to watch if mapped content later appears. Compute the initial empty state synchronously and keep the hidden wrapper mounted while only gating the editor-only empty-state UI.Also applies to: 282-309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/pageSections/ProductSection/ProductSection.tsx` around lines 269 - 279, resolveData() currently only signals the hook to start watching, causing the hook's initial isMappedContentEmpty to be false and letting a mapped-empty section render once before the effect hides it and unmounts the observed wrapper; change resolveData() (and the similar block handling slots around lines 282-309) to synchronously compute the initial mapped-empty boolean by calling isMappedCardWrapperSelected(updatedData.props.slots.CardsWrapperSlot?.[0]) and set that value into updatedData.props.conditionalRender (e.g., initialMappedEmpty or set conditionalRender.watchForMappedContentEmptyState to the computed boolean) so the hook can initialize correctly, and modify the render logic to keep the observed wrapper mounted at all times while only gating the editor-only empty-state UI (hide editor overlay/components instead of returning <></>) so the observer remains attached and can unhide when mapped content appears.
🧹 Nitpick comments (1)
packages/visual-editor/src/components/pageSections/EntityMappedSectionEmptyState.test.tsx (1)
500-515: These live-mode assertions won't catch the first-paint flash.Because this test only checks emptiness inside
waitFor, it still passes when a wrapper-backed section renders once and only disappears after the observer/effect updates state. Add an immediate assertion right afterrenderSection(...)so the test fails on any initial live-page paint.Suggested test tightening
const result = renderSection({ sectionConfig, props: createCardsSectionRenderProps(sectionConfig, { isEditing: false, watchForMappedContentEmptyState: true, cardsWrapperContent: ( <EntityFieldSectionEmptyStateBox showEmptyStateMarker /> ), }), }); + expect(result.container.childElementCount).toBe(0); + await waitFor(() => { expect(result.container.childElementCount).toBe(0); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/pageSections/EntityMappedSectionEmptyState.test.tsx` around lines 500 - 515, The test currently only asserts emptiness inside waitFor, which misses an initial render flash; immediately after calling renderSection (the call that returns result), add an immediate assertion (e.g., expect(result.container.childElementCount).toBe(0)) to catch any first-paint content, then keep the existing waitFor assertion to ensure it stays empty; reference the render helper renderSection, the props factory createCardsSectionRenderProps, and the EntityFieldSectionEmptyStateBox used for cardsWrapperContent when adding the immediate assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/visual-editor/src/components/pageSections/EventSection/EventSection.tsx`:
- Around line 196-206: The rendered empty-state decision is currently made
post-mount via an observer effect, causing a transient render and unmounting
that prevents recovery; modify EventSection so
conditionalRender.watchForMappedContentEmptyState is derived synchronously from
the incoming props/slots (use isMappedCardWrapperSelected on
props.slots.CardsWrapperSlot?.[0] before render) and ensure the non-editing
branch (the puck check using props.puck.isEditing) does not return an immediate
<> that unmounts the observed wrapper—instead render a persistent
placeholder/container that keeps the observer mounted so mapped data can later
populate; update references: isMappedCardWrapperSelected, conditionalRender,
props.puck.isEditing, slots.CardsWrapperSlot to compute and drive render
behavior pre-mount.
---
Duplicate comments:
In
`@packages/visual-editor/src/components/pageSections/ProductSection/ProductSection.tsx`:
- Around line 269-279: resolveData() currently only signals the hook to start
watching, causing the hook's initial isMappedContentEmpty to be false and
letting a mapped-empty section render once before the effect hides it and
unmounts the observed wrapper; change resolveData() (and the similar block
handling slots around lines 282-309) to synchronously compute the initial
mapped-empty boolean by calling
isMappedCardWrapperSelected(updatedData.props.slots.CardsWrapperSlot?.[0]) and
set that value into updatedData.props.conditionalRender (e.g.,
initialMappedEmpty or set conditionalRender.watchForMappedContentEmptyState to
the computed boolean) so the hook can initialize correctly, and modify the
render logic to keep the observed wrapper mounted at all times while only gating
the editor-only empty-state UI (hide editor overlay/components instead of
returning <></>) so the observer remains attached and can unhide when mapped
content appears.
---
Nitpick comments:
In
`@packages/visual-editor/src/components/pageSections/EntityMappedSectionEmptyState.test.tsx`:
- Around line 500-515: The test currently only asserts emptiness inside waitFor,
which misses an initial render flash; immediately after calling renderSection
(the call that returns result), add an immediate assertion (e.g.,
expect(result.container.childElementCount).toBe(0)) to catch any first-paint
content, then keep the existing waitFor assertion to ensure it stays empty;
reference the render helper renderSection, the props factory
createCardsSectionRenderProps, and the EntityFieldSectionEmptyStateBox used for
cardsWrapperContent when adding the immediate assertion.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: eac9d50b-2054-461b-bfbc-964c8cd637cf
📒 Files selected for processing (16)
packages/visual-editor/src/components/pageSections/EntityFieldSectionEmptyState.tsxpackages/visual-editor/src/components/pageSections/EntityMappedSectionEmptyState.test.tsxpackages/visual-editor/src/components/pageSections/EventSection/EventCardsWrapper.tsxpackages/visual-editor/src/components/pageSections/EventSection/EventSection.tsxpackages/visual-editor/src/components/pageSections/FAQsSection/FAQsSection.tsxpackages/visual-editor/src/components/pageSections/InsightSection/InsightCardsWrapper.tsxpackages/visual-editor/src/components/pageSections/InsightSection/InsightSection.tsxpackages/visual-editor/src/components/pageSections/PhotoGallerySection/PhotoGallerySection.tsxpackages/visual-editor/src/components/pageSections/PhotoGallerySection/PhotoGalleryWrapper.tsxpackages/visual-editor/src/components/pageSections/ProductSection/ProductCardsWrapper.tsxpackages/visual-editor/src/components/pageSections/ProductSection/ProductSection.tsxpackages/visual-editor/src/components/pageSections/TeamSection/TeamCardsWrapper.tsxpackages/visual-editor/src/components/pageSections/TeamSection/TeamSection.tsxpackages/visual-editor/src/components/pageSections/TestimonialSection/TestimonialCardsWrapper.tsxpackages/visual-editor/src/components/pageSections/TestimonialSection/TestimonialSection.tsxpackages/visual-editor/src/components/pageSections/entityFieldSectionUtils.ts
✅ Files skipped from review due to trivial changes (1)
- packages/visual-editor/src/components/pageSections/entityFieldSectionUtils.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/visual-editor/src/components/pageSections/ProductSection/ProductCardsWrapper.tsx
- packages/visual-editor/src/components/pageSections/InsightSection/InsightCardsWrapper.tsx
- packages/visual-editor/src/components/pageSections/EntityFieldSectionEmptyState.tsx
- packages/visual-editor/src/components/pageSections/FAQsSection/FAQsSection.tsx
- packages/visual-editor/src/components/pageSections/TeamSection/TeamSection.tsx
- packages/visual-editor/src/components/pageSections/InsightSection/InsightSection.tsx
- packages/visual-editor/src/components/pageSections/TestimonialSection/TestimonialSection.tsx
- packages/visual-editor/src/components/pageSections/EventSection/EventCardsWrapper.tsx
briantstephan
requested review from
asanehisa,
benlife5,
jwartofsky-yext and
mkilpatrick
auto-screenshot-update: true
briantstephan
force-pushed
the
hide-empty-components
branch
from
fd5b890 to
d3c1869
Compare
auto-screenshot-update: true
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This ensures that components that map to a specific entity field are hidden when there is no data.
Feel free to test on this site:
https://dev.yext.com/s/1000167375/yextsites/67077/pagesets
The entity with 137 in the address has the entity fields populated, whereas the entity with 250 in the address does not, and shows the new empty state.