fix (design library): design preview scale adjustment #3598
Conversation
|
Warning Rate limit exceeded@kaeizen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughusePreviewRenderer now requires a hostRef and no longer exposes adjustScale; scaling uses host width and RAF scheduling. DesignLibraryListItem delays rendering until shadowRoot exists and loading is false and stops passing adjustScale. DesignPreview drops the adjustScale prop. IntersectionObserver replaces scroll-based lazy rendering. Two SCSS tweaks added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant List as DesignLibraryList
participant Item as DesignLibraryListItem
participant Hook as usePreviewRenderer
participant Host as hostRef
participant Card as ref
participant Shadow as shadowRoot
participant Preview as DesignPreview
User->>List: opens modal / scrolls
List->>Item: mount (attach wrapperRef)
Item->>List: IntersectionObserver reports visibility
alt not visible and not selected
Item-->>User: render placeholder
else visible or selected
Item->>Hook: init(..., ref, hostRef, shadowRoot, setIsLoading)
Hook->>Host: read hostRect (hostRef.current)
Hook->>Card: read cardRect (ref.current)
Hook->>Hook: schedule adjustScale(force=true) via RAF
Hook-->>Item: update isLoading / size states
alt no shadowRoot or isLoading
Item-->>User: height = 0 (no Preview)
else ready
Item->>Preview: render with blocks, shadowRoot, selectedTab, onMouseDown
end
end
Note over Hook: on blocks/content/tab changes -> cancel prior RAF and schedule adjustScale(false)\nCleanup: cancel RAF on unmount
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
🤖 Pull request artifacts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/components/design-library-list/use-preview-renderer.js (2)
73-91: Scale guard and width equality check are brittle.
- Using
cardWidth === hostWidthis fragile due to sub‑pixel rounding; minor resizes may never trigger or may thrash.- Consider a tolerance or a more direct signal (ResizeObserver on host).
Minimal change:
- if ( ! force && cardWidth === hostWidth ) { + if ( ! force && Math.abs(cardWidth - hostWidth) < 0.5 ) { return }Or switch to ResizeObserver on
hostRef.currentto driveadjustScale.
321-326: Unmount cleanup — add null check and clear handle.Minor safety nit: guard
cancelAnimationFrameand null the ref (you already do the latter in your proposed fix above).- cancelAnimationFrame( adjustAnimateFrameRef.current ) - adjustAnimateFrameRef.current = null + if (adjustAnimateFrameRef.current != null) { + cancelAnimationFrame( adjustAnimateFrameRef.current ) + adjustAnimateFrameRef.current = null + }src/components/design-library-list/design-library-list-item.js (2)
61-64: Zero height during load — OK; prevents layout flicker.Ensure the spinner container provides sufficient min-height so the card doesn’t collapse visually.
106-110: Default scale to 1 to avoid invalid CSS during init.
previewSize?.scalecan beundefinedbefore the first adjust;scale(undefined)is invalid. Fallback to 1.- transform: `scale(${ selectedNum && selectedData ? selectedData.selectedPreviewSize.scale : previewSize?.scale })`, + transform: `scale(${ selectedNum && selectedData ? selectedData.selectedPreviewSize.scale : (previewSize?.scale ?? 1) })`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/design-library-list/design-library-list-item.js(2 hunks)src/components/design-library-list/design-preview.js(0 hunks)src/components/design-library-list/use-preview-renderer.js(7 hunks)src/components/modal-design-library/editor.scss(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/design-library-list/design-preview.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/design-library-list/design-library-list-item.js (5)
src/components/design-library-list/use-preview-renderer.js (3)
usePreviewRenderer(37-349)usePreviewRenderer(37-349)shadowBodySizeRef(63-63)src/components/design-library-list/index.js (2)
previewProps(109-114)previewSize(106-106)src/components/design-library-list/design-preview.js (3)
ref(18-18)DesignPreview(12-91)DesignPreview(12-91)src/components/design-library-list/use-shadow-root.js (2)
hostRef(9-9)shadowRoot(10-10)src/components/design-library-list/use-auto-scroll.js (2)
useAutoScroll(8-147)useAutoScroll(8-147)
src/components/design-library-list/use-preview-renderer.js (2)
src/components/design-library-list/use-shadow-root.js (3)
hostRef(9-9)shadowRoot(10-10)useSelect(12-12)src/components/design-library-list/index.js (2)
previewSize(106-106)cardHeight(105-105)
⏰ 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). (6)
- GitHub Check: PHP 8.2 and WP 6.5.5
- GitHub Check: PHP 8.2 and WP latest
- GitHub Check: PHP 8.2 and WP 6.7.2
- GitHub Check: PHP 7.3 and WP 6.5.5
- GitHub Check: PHP 7.3 and WP latest
- GitHub Check: PHP 8.2 and WP 6.6.2
🔇 Additional comments (5)
src/components/design-library-list/use-preview-renderer.js (3)
40-41: Signature change to include hostRef and setIsLoading — OK.Call sites appear updated; no further concerns.
65-65: Initialize prevEnableBackgroundRef to null — OK.This ensures the first post‑parse adjustment triggers.
344-348: Return shape update — OK.Removing
adjustScalefrom the public API is consistent with the new scheduling.src/components/design-library-list/design-library-list-item.js (2)
49-54: Hook result/use invocation updates — OK.Dropping
adjustScaleand passinghostRefmatch the hook changes.
113-118: Gate preview rendering on readiness — OK.Rendering only when
shadowRoot && !isLoadingis correct and avoids unnecessary work.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/components/design-library-list/use-preview-renderer.js (2)
138-140: Stop the infinite rAF loop; only schedule a follow‑up when explicitly forced.Unconditionally scheduling
requestAnimationFrame(() => adjustScale(false))creates a perpetual loop (even whenshouldAdjustis false). Make scheduling conditional onforce, guard cancellations, and clear the handle.Apply:
- cancelAnimationFrame( adjustAnimateFrameRef.current ) - adjustAnimateFrameRef.current = requestAnimationFrame( () => adjustScale( false ) ) + if ( force ) { + if ( adjustAnimateFrameRef.current != null ) { + cancelAnimationFrame( adjustAnimateFrameRef.current ) + } + adjustAnimateFrameRef.current = requestAnimationFrame( () => { + adjustAnimateFrameRef.current = null + adjustScale( false ) + } ) + }
295-297: Normalize rAF handling: guard cancels, store handles, and clear on run/cleanup.
- Guard
cancelAnimationFramecalls to avoid passingnull.- In the
[content]effect you don’t store the returned handle, so you can’t cancel it later.- Clear handles to
nullafter use and during unmount.Apply:
- cancelAnimationFrame( adjustAnimateFrameRef.current ) - adjustAnimateFrameRef.current = requestAnimationFrame( adjustScale ) + if ( adjustAnimateFrameRef.current != null ) cancelAnimationFrame( adjustAnimateFrameRef.current ) + adjustAnimateFrameRef.current = requestAnimationFrame( adjustScale )- cancelAnimationFrame( adjustAnimateFrameRef.current ) - adjustAnimateFrameRef.current = requestAnimationFrame( adjustScale ) + if ( adjustAnimateFrameRef.current != null ) cancelAnimationFrame( adjustAnimateFrameRef.current ) + adjustAnimateFrameRef.current = requestAnimationFrame( adjustScale )- cancelAnimationFrame( adjustAnimateFrameRef.current ) - requestAnimationFrame( () => { - adjustScale() - prevSelectedTabRef.current = selectedTab - } ) + if ( adjustAnimateFrameRef.current != null ) cancelAnimationFrame( adjustAnimateFrameRef.current ) + adjustAnimateFrameRef.current = requestAnimationFrame( () => { + adjustAnimateFrameRef.current = null + adjustScale() + prevSelectedTabRef.current = selectedTab + } )- cancelAnimationFrame( adjustAnimateFrameRef.current ) - adjustAnimateFrameRef.current = null + if ( adjustAnimateFrameRef.current != null ) { + cancelAnimationFrame( adjustAnimateFrameRef.current ) + adjustAnimateFrameRef.current = null + }Also applies to: 307-309, 318-323, 325-332
🧹 Nitpick comments (5)
src/components/design-library-list/use-preview-renderer.js (2)
135-135: Replace 500ms timeout with rAF (and clear on cleanup).The fixed 500ms delay is brittle and can update after unmount. Use rAF and keep a dedicated ref for it.
Apply:
- setTimeout( () => setCardHeight( newCardHeight ), 500 ) + if ( cardHeightRafRef.current != null ) cancelAnimationFrame( cardHeightRafRef.current ) + cardHeightRafRef.current = requestAnimationFrame( () => { + setCardHeight( newCardHeight ) + cardHeightRafRef.current = null + } )Also add the ref and cleanup:
- const adjustAnimateFrameRef = useRef( null ) + const adjustAnimateFrameRef = useRef( null ) + const cardHeightRafRef = useRef( null )return () => { - cancelAnimationFrame( adjustAnimateFrameRef.current ) - adjustAnimateFrameRef.current = null + if ( adjustAnimateFrameRef.current != null ) { + cancelAnimationFrame( adjustAnimateFrameRef.current ) + adjustAnimateFrameRef.current = null + } + if ( cardHeightRafRef.current != null ) { + cancelAnimationFrame( cardHeightRafRef.current ) + cardHeightRafRef.current = null + } }
61-62: Remove unused state.
hasBackgroundTargetRefis written but never read in this file. Drop it to reduce noise.src/components/design-library-list/index.js (1)
112-127: Stabilize lazy rendering and avoid global query.
- Prefer scoping the observer root via
closest()to the container instead ofdocument.querySelector.- Don’t unrender once rendered to prevent thrash and state loss; flip
shouldRenderto sticky true.- const rootEl = document.querySelector( '.ugb-modal-design-library__designs' ) + const rootEl = wrapperRef.current?.closest?.( '.ugb-modal-design-library__designs' ) - const observer = new IntersectionObserver( ( [ entry ] ) => { - setShouldRender( entry.isIntersecting ) - }, { + const observer = new IntersectionObserver( ( [ entry ] ) => { + setShouldRender( prev => prev || entry.isIntersecting ) + }, {src/components/design-library-list/design-library-list-item.js (2)
61-67: Avoid layout jank while loading; keep a stable height.Returning
0collapses the container during load. Fall back to the last measured card height (or a sane default).- if ( ! shadowRoot || isLoading ) { - return 0 - } + if ( ! shadowRoot || isLoading ) { + const key = enableBackground ? 'background' : 'noBackground' + return cardHeight?.[ key ] ?? 250 + }
107-107: Default scale to 1 to avoid invalid CSS.When
previewSize.scaleis undefined,scale(undefined)is invalid. Use a neutral fallback.- transform: `scale(${ selectedNum && selectedData ? selectedData.selectedPreviewSize.scale : previewSize?.scale })`, + transform: `scale(${ ( selectedNum && selectedData ? selectedData.selectedPreviewSize.scale : previewSize?.scale ) ?? 1 })`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/design-library-list/design-library-list-item.js(3 hunks)src/components/design-library-list/editor.scss(1 hunks)src/components/design-library-list/index.js(2 hunks)src/components/design-library-list/use-preview-renderer.js(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/components/design-library-list/index.js (2)
src/components/design-library-list/design-library-list-item.js (2)
props(27-36)DesignLibraryListItem(26-158)src/components/design-library-list/use-preview-renderer.js (1)
props(42-53)
src/components/design-library-list/use-preview-renderer.js (3)
src/components/design-library-list/design-preview.js (1)
ref(18-18)src/components/design-library-list/use-shadow-root.js (3)
hostRef(9-9)shadowRoot(10-10)useSelect(12-12)src/components/design-library-list/index.js (2)
previewSize(101-101)cardHeight(100-100)
src/components/design-library-list/design-library-list-item.js (5)
src/components/design-library-list/use-preview-renderer.js (3)
usePreviewRenderer(37-354)usePreviewRenderer(37-354)shadowBodySizeRef(63-63)src/components/design-library-list/index.js (2)
previewProps(104-109)previewSize(101-101)src/components/design-library-list/design-preview.js (3)
ref(18-18)DesignPreview(12-91)DesignPreview(12-91)src/components/design-library-list/use-shadow-root.js (2)
hostRef(9-9)shadowRoot(10-10)src/components/design-library-list/use-auto-scroll.js (2)
useAutoScroll(8-147)useAutoScroll(8-147)
⏰ 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). (6)
- GitHub Check: PHP 8.2 and WP latest
- GitHub Check: PHP 8.2 and WP 6.6.2
- GitHub Check: PHP 7.3 and WP latest
- GitHub Check: PHP 8.2 and WP 6.5.5
- GitHub Check: PHP 7.3 and WP 6.5.5
- GitHub Check: PHP 8.2 and WP 6.7.2
🔇 Additional comments (4)
src/components/design-library-list/use-preview-renderer.js (1)
73-96: Early‑exit check looks good; confirm host/card pairing.The height/width equality gate should cut redundant work. Please confirm
hostRefalways points to the transformed element whose rect reflects the applied scale; otherwise the equality test may misfire.src/components/design-library-list/editor.scss (1)
30-30: LGTM: make item span full grid width.
width: 100%on the grid item is appropriate and shouldn’t disrupt the existing 1fr columns.Please sanity‑check on narrow breakpoints to ensure no unintended horizontal scroll appears.
src/components/design-library-list/index.js (1)
37-39: Guard scroll reset.Avoid potential null deref when
containerRef.currentisn’t set yet.
[raise_nitpick_refactor]- containerRef.current.scrollTop = 0 + if ( containerRef.current ) containerRef.current.scrollTop = 0src/components/design-library-list/design-library-list-item.js (1)
113-118: Conditional render looks good.Rendering
DesignPreviewonly whenshadowRootexists and loading is done avoids work and errors.
2 participants
Summary by CodeRabbit
Bug Fixes
New Features
Style
Refactor