fix: reduce memory usage in Directory PageSets#1121
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. |
commit: |
WalkthroughThis PR refactors content block data resolution across multiple components (Address, HeadingText, HoursStatus, Phone) to safely handle optional parent data fields using optional chaining and nullish coalescing. It also introduces a reference-based system for directory child management, replacing direct profile object passing with Sequence DiagramsequenceDiagram
participant DW as DirectoryWrapper
participant DC as DirectoryCard
participant DCP as DirectoryChildrenProvider
participant DCRF as resolveDirectoryChildFromReference
participant MM as mergeMeta
participant TPC as TemplatePropsContext.Provider
participant Slots as Child Slots
DW->>DW: getSortedDirectoryChildren(dm_directoryChildren)
DW->>DCP: Render with sortedDirectoryChildren
DCP->>DCP: Store in DirectoryChildrenContext
DW->>DC: Pass childRef in parentData
DC->>DCRF: Resolve child from childRef
DCRF->>DCRF: Match childIndex + optional childId
DCRF-->>DC: Return resolved DirectoryChild
DC->>MM: mergeMeta(resolvedChild, streamDocument)
MM-->>DC: Merged document with child context
DC->>TPC: Wrap rendering with merged context
TPC->>Slots: Provide child-scoped document
Slots->>Slots: Resolve data using child context (name, address, mainPhone, hours)
Slots-->>DC: Rendered slot content
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/visual-editor/src/utils/directory/resolveDirectoryCardReferences.test.ts (1)
22-49: Consider adding edge case coverage.The test fixture includes children with varying data completeness (second child lacks
hours). Consider adding assertions to verify conditional slot rendering behavior when fields are missing, or add a separate test case forchildId-based resolution when the child list order changes between resolution and render.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/utils/directory/resolveDirectoryCardReferences.test.ts` around lines 22 - 49, Add test coverage for edge cases in resolveDirectoryCardReferences.test.ts: create a test that asserts conditional slot rendering when a child lacks optional fields (e.g., second child missing hours) so the component/function under test (resolveDirectoryCardReferences or the render helper used in these tests) does not attempt to render hours for that child, and add a separate test that verifies childId-based resolution when the order of dm_directoryChildren changes between resolution and render (i.e., ensure resolveDirectoryCardReferences correctly matches entries by childId rather than array index). Locate tests referencing dm_directoryChildren in resolveDirectoryCardReferences.test.ts and add assertions for absence/presence of slots for hours and for correct lookup by childId.
🤖 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/utils/directory/resolveDirectoryCardReferences.test.ts`:
- Around line 22-49: Add test coverage for edge cases in
resolveDirectoryCardReferences.test.ts: create a test that asserts conditional
slot rendering when a child lacks optional fields (e.g., second child missing
hours) so the component/function under test (resolveDirectoryCardReferences or
the render helper used in these tests) does not attempt to render hours for that
child, and add a separate test that verifies childId-based resolution when the
order of dm_directoryChildren changes between resolution and render (i.e.,
ensure resolveDirectoryCardReferences correctly matches entries by childId
rather than array index). Locate tests referencing dm_directoryChildren in
resolveDirectoryCardReferences.test.ts and add assertions for absence/presence
of slots for hours and for correct lookup by childId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d3aa3749-0986-431c-b263-714e4da95d3d
📒 Files selected for processing (8)
packages/visual-editor/src/components/contentBlocks/Address.tsxpackages/visual-editor/src/components/contentBlocks/HeadingText.tsxpackages/visual-editor/src/components/contentBlocks/HoursStatus.tsxpackages/visual-editor/src/components/contentBlocks/Phone.tsxpackages/visual-editor/src/components/directory/DirectoryCard.tsxpackages/visual-editor/src/components/directory/DirectoryWrapper.tsxpackages/visual-editor/src/components/directory/directoryChildReference.tsxpackages/visual-editor/src/utils/directory/resolveDirectoryCardReferences.test.ts
asanehisa
requested review from
benlife5,
briantstephan,
jwartofsky-yext and
mkilpatrick
benlife5
left a comment
benlife5
left a comment
There was a problem hiding this comment.
Nice!! It's great to finally get to the bottom of this 👑
| ), | ||
| [parentData?.childRef, sortedDirectoryChildren] | ||
| ); | ||
| const childDocumentContext = React.useMemo( |
There was a problem hiding this comment.
Can you explain this block?
There was a problem hiding this comment.
am adding a comment but lmk if doesnt make sense!
packages/visual-editor/src/components/directory/directoryChildReference.tsx
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/components/directory/directoryChildReference.test.tsx
Show resolved
Hide resolved
packages/visual-editor/src/components/directory/DirectoryWrapper.tsx
Outdated
Show resolved
Hide resolved
| parentData?: { | ||
| field: string; | ||
| address: AddressType; | ||
| address?: AddressType; |
There was a problem hiding this comment.
Is this something we should think about doing as a larger refactor for all the slots?
There was a problem hiding this comment.
I think it is something to consider / see if it makes a diff! I started with Directory as it used a lotttt more memory than main templates or locators.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/visual-editor/src/components/directory/directoryChildReference.test.tsx (1)
106-123: Make child-reference assertions order-agnostic to reduce test brittleness.The current check depends on
cards[0]beingchildIndex: 0. If card ordering changes (while behavior remains correct), this test can fail unnecessarily.Proposed refactor
- const firstCard = cards[0]; + const childIndices = cards + .map((card) => card?.props?.parentData?.childRef?.childIndex) + .sort(); + const firstCard = cards.find( + (card) => card?.props?.parentData?.childRef?.childIndex === 0 + ); expect(cards).toHaveLength(2); - expect(firstCard?.props?.parentData).toEqual({ - childRef: { childIndex: 0 }, - }); + expect(childIndices).toEqual([0, 1]); + expect(firstCard?.props?.parentData).toEqual({ childRef: { childIndex: 0 } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/directory/directoryChildReference.test.tsx` around lines 106 - 123, The test assumes the first entry in cards is childIndex 0 which is brittle; instead locate the card by its childRef and assert on that object. Replace uses of firstCard with a lookup like find over cards to get the element whose props.parentData.childRef.childIndex === 0 (e.g., cardWithIndex0) and then assert its parentData, profile, and HeadingSlot values; keep the check that cards has length 2 but make the child-ref assertions order-agnostic by referencing the found card rather than cards[0].
🤖 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/directory/directoryChildReference.test.tsx`:
- Around line 106-123: The test assumes the first entry in cards is childIndex 0
which is brittle; instead locate the card by its childRef and assert on that
object. Replace uses of firstCard with a lookup like find over cards to get the
element whose props.parentData.childRef.childIndex === 0 (e.g., cardWithIndex0)
and then assert its parentData, profile, and HeadingSlot values; keep the check
that cards has length 2 but make the child-ref assertions order-agnostic by
referencing the found card rather than cards[0].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 832a8567-e3c6-4f7f-8e6c-567a879bd8cc
📒 Files selected for processing (4)
packages/visual-editor/src/components/directory/DirectoryCard.tsxpackages/visual-editor/src/components/directory/DirectoryWrapper.tsxpackages/visual-editor/src/components/directory/directoryChildReference.test.tsxpackages/visual-editor/src/components/directory/directoryChildReference.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/visual-editor/src/components/directory/DirectoryWrapper.tsx
- packages/visual-editor/src/components/directory/directoryChildReference.tsx
- packages/visual-editor/src/components/directory/DirectoryCard.tsx
2 participants
Resolved DirectoryCard nodes no longer embed the full child payload in props.parentData.profile. Instead, DirectoryGrid.resolveData stores a lightweight child reference on each card, and DirectoryCard resolves the referenced child at render time. Nested slot content then reads from the resolved child document context rather than carrying duplicated child-derived values in each slot’s parentData.
Directory memory use was scaling poorly because:
This refactor removes that duplicated child payload from the resolved Puck tree while preserving existing rendering behavior. Before a 100 child directory city page would easily have a 20mb heap compared to a 4 child page with 3.8mb heap.
I used a local diagnostic tool between main and this branch and saw:
You can run the diagnostic tool locally like:
live site: https://www.yext.com/s/4412098/yextsites/164953/pagesets