fix: state/history bugs #935
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 introduces observability improvements and optimizations across the visual editor's layout handling. Changes include removing explicit spotlight variant alignment from HeroSection (now using default alignment), optimizing InsightCard's useEffect dependency array to reduce unnecessary effect triggers, adding deep equality checks in InternalLayoutEditor to prevent redundant data updates on document changes, and enhancing clipboard paste operations for CardsWrapper components to preserve child IDs. New logging prefixes are added to track resolved layout data and pasted data during these operations. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ 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: 0
🧹 Nitpick comments (1)
packages/visual-editor/src/internal/components/InternalLayoutEditor.tsx (1)
489-511: Good fix for the CardsWrapper copy-paste ID sync issue.This correctly propagates the regenerated CardSlot IDs into the
constantValuearray, ensuring constant values persist after paste.Minor note: The type check
item.type.includes("CardsWrapper")is string-based. If component naming conventions change or other components include "CardsWrapper" in their name with different structures, this could match unintentionally. Consider an exact match against known CardsWrapper types if the set is stable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/visual-editor/src/components/testing/screenshots/HeroSection/[desktop] [spotlight] version 17 props using constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/HeroSection/[mobile] [spotlight] version 17 props using constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/HeroSection/[tablet] [spotlight] version 17 props using constant values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (5)
packages/visual-editor/src/components/pageSections/HeroSection.tsx(0 hunks)packages/visual-editor/src/components/pageSections/InsightSection/InsightCard.tsx(1 hunks)packages/visual-editor/src/internal/components/InternalLayoutEditor.tsx(4 hunks)packages/visual-editor/src/internal/puck/components/LayoutHeader.tsx(2 hunks)packages/visual-editor/src/utils/devLogger.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/visual-editor/src/components/pageSections/HeroSection.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/visual-editor/src/internal/puck/components/LayoutHeader.tsx (1)
packages/visual-editor/src/utils/devLogger.ts (1)
DevLogger(17-62)
⏰ 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: call_unit_test / unit_tests (20.x)
🔇 Additional comments (7)
packages/visual-editor/src/utils/devLogger.ts (1)
7-8: LGTM!The new logging prefixes align well with the observability improvements added in
InternalLayoutEditor.tsxandLayoutHeader.tsx.packages/visual-editor/src/internal/puck/components/LayoutHeader.tsx (2)
23-26: LGTM!Module-scope instantiation of
DevLoggeris consistent with the pattern used inInternalLayoutEditor.tsx.
166-166: Good addition for debugging paste flows.Logging the migrated data before dispatch provides visibility into the paste operation, which will help diagnose layout paste issues.
packages/visual-editor/src/components/pageSections/InsightSection/InsightCard.tsx (1)
322-322: Verify intentional narrowing of dependency array.Narrowing the dependencies to only
[sharedCardProps]omits several values used within the effect (styles,slotStyles,getPuck,props.id,slotsData,props). This appears intentional to prevent update cycles between the two effects, but could cause stale closure issues if those values change independently.If this is intentional, consider adding a comment explaining the rationale and an eslint-disable directive if the exhaustive-deps rule is active.
packages/visual-editor/src/internal/components/InternalLayoutEditor.tsx (3)
35-35: LGTM!Import supports the new deep equality optimization.
271-288: Good optimization to reduce unnecessary state updates.The deep equality check prevents redundant
setDatadispatches when resolved data hasn't changed.Note: There's a potential TOCTOU gap where
appState.datais captured before the asyncresolveAllDatacall but compared after. If state changes during the await, the comparison uses the stale reference. However, since this is an optimization and the worst case is an extra dispatch (same behavior as before this change), it's acceptable.
522-522: Good addition for debugging component paste flows.
benlife5
requested review from
asanehisa,
briantstephan,
colton-demetriou and
mkilpatrick
#935 added a deepEqual check for re-resolving all data which seemed to have helped a bit. This PR adds additional save state management, which helps particularly when clearing local changes. Reducing the placeholder image size seems to make the re-rendering a little faster. There still is a delay when publishing and then switching tabs. I'm not entirely sure what is triggering it but the slowness seems to be on Puck's side. Puck has some slot performance improvements coming up, so I think we should take a look again after those are done. --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
3 participants
Was actually two separate issues:
a. This meant that once you copy-pasted a component, your constant values would never save because they would get overwritten when resolveData ran on any subsequent load
Unrelatedly, found a case we can skip some "setData" actions. Might help with lag.
Finally, added additional dev logging to some newer features