fix (design library): preview of selected design when switching tabs #3594
Conversation
WalkthroughThe hook adds a ref to track the previously selected tab, gates re-renders based on tab changes and selected design state, defers some scale adjustments via setTimeout, and updates the timing of when scale recalculation occurs after content updates. Comments note a 1300px reference width without changing the scale formula. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as User/UI
participant Hook as usePreviewRenderer
participant Scale as adjustScale
participant DOM as Layout/DOM
User->>Hook: Select tab / update content
Note over Hook: Track previous tab via prevSelectedTabRef
alt No design selected OR tab changed
Hook->>Scale: Invoke adjustScale()
Scale->>DOM: Read sizes, compute scale (ref=1300)
DOM-->>Hook: Layout metrics
Hook-->>User: Render scaled preview
else Design selected AND tab unchanged
Hook-->>User: Skip re-render
end
Note over Hook: On content update
Hook->>Hook: setTimeout(adjustScale, 100)
Hook->>Hook: After run, set prevSelectedTabRef = selectedTab
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 0
🧹 Nitpick comments (4)
src/components/design-library-list/use-preview-renderer.js (4)
73-76: Tighten guard logic; avoid falsy traps and detached nodes.Use explicit “not selected” semantics and ensure the node is connected.
-const shouldAdjust = ref.current && shadowRoot && - ( ! selectedNum || // adjust if design is not selected - prevSelectedTabRef.current !== selectedTab ) // adjust if selected tab changed even if design is selected +const shouldAdjust = ref.current?.isConnected && shadowRoot && + ( selectedNum === 0 || // adjust if design is not selected + prevSelectedTabRef.current !== selectedTab ) // adjust if tab changed even if a design is selected
85-85: Replace magic number (1300) with a named constant.Improves readability and single‑source of truth.
-const scaleFactor = cardWidth > 0 ? cardWidth / 1300 : 1 // Divide by 1300, which is the width of preview in the shadow DOM +const scaleFactor = cardWidth > 0 ? cardWidth / SHADOW_PREVIEW_WIDTH : 1 // Width of preview in the shadow DOMAdd once near the top (outside this hunk):
const SHADOW_PREVIEW_WIDTH = 1300
275-278: Timeout has no cleanup; prevent setState on unmounted and rapid re-schedules.Store/clear the timer.
- setTimeout( adjustScale, 100 ) + if ( unselectAdjustTimeoutRef.current ) { + clearTimeout( unselectAdjustTimeoutRef.current ) + } + unselectAdjustTimeoutRef.current = setTimeout( adjustScale, 100 )Add outside this hunk:
// at top-level with other refs const unselectAdjustTimeoutRef = useRef(null) // one-time cleanup useEffect(() => () => { if (unselectAdjustTimeoutRef.current) clearTimeout(unselectAdjustTimeoutRef.current) }, [])
297-301: Same: defer with cleanup; consider rAF for layout-timing.Prevents duplicate queued work in StrictMode and fast tab switches.
- setTimeout( () => { - adjustScale() - prevSelectedTabRef.current = selectedTab - }, 100 ) + if ( contentAdjustTimeoutRef.current ) { + clearTimeout( contentAdjustTimeoutRef.current ) + } + contentAdjustTimeoutRef.current = setTimeout( () => { + adjustScale() + prevSelectedTabRef.current = selectedTab + }, 100 )Add outside this hunk:
const contentAdjustTimeoutRef = useRef(null) useEffect(() => () => { if (contentAdjustTimeoutRef.current) clearTimeout(contentAdjustTimeoutRef.current) }, []) // Optional: swap setTimeout for rAF for layout coherence // contentAdjustTimeoutRef.current = requestAnimationFrame(() => { ... }) // and cancel via cancelAnimationFrame(...)Also consider applying the same cleanup pattern to the
setTimeout(() => setCardHeight(...), 500)insideadjustScale.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/design-library-list/use-preview-renderer.js(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/design-library-list/use-preview-renderer.js (2)
src/components/modal-design-library/modal.js (1)
selectedTab(62-62)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). (7)
- GitHub Check: PHP 8.2 and WP 6.6.2
- GitHub Check: PHP 8.2 and WP 6.5.5
- GitHub Check: PHP 7.3 and WP latest
- GitHub Check: PHP 8.2 and WP 6.7.2
- GitHub Check: PHP 8.2 and WP latest
- GitHub Check: PHP 7.3 and WP 6.5.5
- GitHub Check: build
🔇 Additional comments (2)
src/components/design-library-list/use-preview-renderer.js (2)
65-65: Good: track previous tab with a ref.This enables clean gating tied to tab transitions.
262-266: Effect gating could miss tab-only changes; consider adding selectedTab to deps.If a tab switch doesn’t change
template/content, this effect won’t run. The guard already prevents unnecessary work.-}, [ content, containerScheme, backgroundScheme, enableBackground ] ) +}, [ content, containerScheme, backgroundScheme, enableBackground, selectedTab ] )Would you confirm whether
templatealways changes on tab switch in all entry points? If not, the above is safer.
bfintal
deleted the
fix/design-library-selected-design-switching-tabs
branch
Summary by CodeRabbit