fix: prevent user from exiting interactive mode while in Theme mode and hide sidebar breadcrumbs#1073
fix: prevent user from exiting interactive mode while in Theme mode and hide sidebar breadcrumbs#1073jwartofsky-yext wants to merge 15 commits intomainfrom
Conversation
The user is not supposed to select comopnents, so this always just displays "Page" There is a bug that makes JSON appear in this section when the user is able to select components in the theme editor. There will be a separate PR to fix this issue, but this prevents it from being visible.
|
Warning Rate limit exceeded
⌛ 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. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThemeHeader was extended with multiple props (theme histories/config, modal state, counts, localDev, headDeployStatus, puckInitialHistory) and now uses a createUsePuck-based usePuck to read appState.ui.previewMode and set UI state. On mount it initializes puck history, forces interactive preview mode when needed, injects a SIDEBAR_HIDE_STYLE_ID CSS element to hide right-panel breadcrumbs/titles, and attaches per-iframe handlers (via MutationObserver) to block and sanitize link navigation inside the preview iframe. On unmount it removes the injected style and detaches/restores iframe listeners/attributes. Sequence Diagram(s)sequenceDiagram
participant TH as "ThemeHeader"
participant PS as "Puck Store"
participant IF as "Preview Iframe"
participant DOM as "Document (style element)"
participant MO as "MutationObserver"
TH->>DOM: inject SIDEBAR_HIDE_STYLE_ID (hide breadcrumbs/titles)
TH->>PS: read appState.ui.previewMode
alt previewMode != "interactive"
TH->>PS: dispatch setUi(previewMode="interactive")
PS-->>TH: updated previewMode
end
TH->>MO: attach observer for preview iframe content
MO->>IF: observe mutations
MO->>IF: strip/disable link href/target attributes
IF-->>TH: navigation events blocked / suppressed
Note right of TH: Component active
TH->>MO: disconnect observer (on unmount)
TH->>DOM: remove SIDEBAR_HIDE_STYLE_ID (on unmount)
TH->>IF: restore original link attributes (on teardown)
🚥 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 (2)
packages/visual-editor/src/internal/puck/components/ThemeHeader.tsx (2)
97-107: Consider addinggetPuckto the dependency array.The
getPuckfunction is used inside the effect but not listed in the dependency array. WhileuseGetPuck()likely returns a stable reference, the exhaustive-deps ESLint rule would flag this. If the reference is guaranteed stable, consider adding a comment or suppressing the lint warning explicitly.♻️ Suggested change
useEffect(() => { // Keep theme mode in interactive preview so links/buttons are clickable // and Puck block selection is disabled. if (previewMode !== "interactive") { const { dispatch } = getPuck(); dispatch({ type: "setUi", ui: { previewMode: "interactive" }, }); } - }, [previewMode]); + }, [previewMode, getPuck]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/internal/puck/components/ThemeHeader.tsx` around lines 97 - 107, The useEffect in ThemeHeader uses getPuck() but doesn't include it in the dependency array, which will trigger exhaustive-deps linting; either add getPuck to the dependency array of the useEffect or, if getPuck is provably stable (from useGetPuck), add an explicit eslint-disable-next-line comment (or // eslint-disable-next-line react-hooks/exhaustive-deps) with a brief justification; update the effect around the dispatch call (the useEffect that references previewMode and getPuck) accordingly to satisfy the linter while preserving the existing behavior.
109-131: Same dependency consideration forgetPuck.Similar to the previous effect,
getPuckis used inside the event handler but not in the dependency array. IfgetPuckreference can change, this would cause a stale closure. For consistency with the other effect, consider adding it to the dependencies.♻️ Suggested change
useEffect(() => { // Prevent Puck's built-in Cmd/Ctrl+I toggle from switching to edit mode. const onKeyDown = (event: KeyboardEvent) => { const isPreviewToggle = (event.metaKey || event.ctrlKey) && event.key.toLowerCase() === "i"; if (!isPreviewToggle) { return; } event.preventDefault(); event.stopPropagation(); const { dispatch } = getPuck(); dispatch({ type: "setUi", ui: { previewMode: "interactive" }, }); }; window.addEventListener("keydown", onKeyDown, true); return () => { window.removeEventListener("keydown", onKeyDown, true); }; - }, []); + }, [getPuck]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/internal/puck/components/ThemeHeader.tsx` around lines 109 - 131, The effect registers an onKeyDown handler that calls getPuck() but the hook's dependency array is empty, risking a stale closure if getPuck can change; update the useEffect to include getPuck in its dependency array (or derive a stable ref to getPuck and use that) so the handler always calls the latest getPuck, and ensure the cleanup still removes the correct listener (referencing useEffect, onKeyDown, and getPuck).
🤖 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/internal/puck/components/ThemeHeader.tsx`:
- Around line 97-107: The useEffect in ThemeHeader uses getPuck() but doesn't
include it in the dependency array, which will trigger exhaustive-deps linting;
either add getPuck to the dependency array of the useEffect or, if getPuck is
provably stable (from useGetPuck), add an explicit eslint-disable-next-line
comment (or // eslint-disable-next-line react-hooks/exhaustive-deps) with a
brief justification; update the effect around the dispatch call (the useEffect
that references previewMode and getPuck) accordingly to satisfy the linter while
preserving the existing behavior.
- Around line 109-131: The effect registers an onKeyDown handler that calls
getPuck() but the hook's dependency array is empty, risking a stale closure if
getPuck can change; update the useEffect to include getPuck in its dependency
array (or derive a stable ref to getPuck and use that) so the handler always
calls the latest getPuck, and ensure the cleanup still removes the correct
listener (referencing useEffect, onKeyDown, and getPuck).
jwartofsky-yext
changed the title
jwartofsky-yext
requested review from
asanehisa,
benlife5,
briantstephan and
mkilpatrick
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/visual-editor/src/internal/puck/components/ThemeHeader.tsx (2)
97-107: MissinggetPuckin dependency array.
getPuckis called inside this effect but is not listed in the dependency array. IfgetPuckreference changes between renders, the effect would use a stale closure.The same pattern appears in the effects at lines 61-74 and 143-149. If
useGetPuckis guaranteed to return a stable reference (like Redux'suseDispatch), this is safe but should be documented or added to the deps for clarity.♻️ Suggested fix
useEffect(() => { // Keep theme mode in interactive preview so links/buttons are clickable // and Puck component selection is disabled. if (previewMode !== "interactive") { const { dispatch } = getPuck(); dispatch({ type: "setUi", ui: { previewMode: "interactive" }, }); } - }, [previewMode]); + }, [previewMode, getPuck]);Apply the same pattern to the other effects using
getPuckat lines 61-74 and 143-149.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/internal/puck/components/ThemeHeader.tsx` around lines 97 - 107, The useEffect hooks that call getPuck (the blocks at/around ThemeHeader where previewMode is set and the other effects at lines 61-74 and 143-149) are missing getPuck in their dependency arrays; add getPuck to each effect's dependency list or, if useGetPuck (the hook providing getPuck) guarantees a stable reference, explicitly document that guarantee and wrap getPuck in useCallback/useRef in the ThemeHeader component to ensure stability. Locate the effects that reference getPuck (the ones that dispatch setUi/other actions) and either include getPuck in the deps or stabilize it via useCallback/useRef so the effects won’t close over a stale getPuck reference.
76-95: CSS attribute selectors are fragile and may break on library updates.The
[class*='SidebarSection-breadcrumbs']and[class*='SidebarSection-title']selectors rely on internal CSS class naming from@puckeditor/core. These could silently break if the library updates its class naming convention.Consider adding a comment noting the Puck version this targets, or check if Puck exposes a more stable API to hide these elements.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/internal/puck/components/ThemeHeader.tsx` around lines 76 - 95, The injected CSS in ThemeHeader.tsx inside the useEffect uses fragile attribute selectors ([class*='SidebarSection-breadcrumbs'] and [class*='SidebarSection-title']) tied to `@puckeditor/core` internals; update this by first trying to use any stable Puck API to hide breadcrumbs/titles (if available) and only fall back to the style injection if no API exists, and add a clear comment stating the exact Puck version this fallback targets; reference the useEffect block and the SIDEBAR_HIDE_STYLE_ID constant so reviewers can find and update the fallback later.
🤖 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/internal/puck/components/ThemeHeader.tsx`:
- Around line 97-107: The useEffect hooks that call getPuck (the blocks
at/around ThemeHeader where previewMode is set and the other effects at lines
61-74 and 143-149) are missing getPuck in their dependency arrays; add getPuck
to each effect's dependency list or, if useGetPuck (the hook providing getPuck)
guarantees a stable reference, explicitly document that guarantee and wrap
getPuck in useCallback/useRef in the ThemeHeader component to ensure stability.
Locate the effects that reference getPuck (the ones that dispatch setUi/other
actions) and either include getPuck in the deps or stabilize it via
useCallback/useRef so the effects won’t close over a stale getPuck reference.
- Around line 76-95: The injected CSS in ThemeHeader.tsx inside the useEffect
uses fragile attribute selectors ([class*='SidebarSection-breadcrumbs'] and
[class*='SidebarSection-title']) tied to `@puckeditor/core` internals; update this
by first trying to use any stable Puck API to hide breadcrumbs/titles (if
available) and only fall back to the style injection if no API exists, and add a
clear comment stating the exact Puck version this fallback targets; reference
the useEffect block and the SIDEBAR_HIDE_STYLE_ID constant so reviewers can find
and update the fallback later.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] latest version default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] latest version 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 (1)
packages/visual-editor/src/internal/puck/components/ThemeHeader.tsx
packages/visual-editor/src/internal/puck/components/ThemeHeader.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/visual-editor/src/internal/puck/components/ThemeHeader.tsx (1)
99-119:⚠️ Potential issue | 🟠 MajorAdd cleanup and retry logic to pointer-blocking effect.
The effect runs once on mount. If
#preview-frameisn't ready, pointer blocking is never applied. Additionally, the style added to the iframe is never removed on unmount, causing behavior to leak after the component unmounts.The first
useEffectin this component (lines 78–93) demonstrates the cleanup pattern; this effect should follow the same pattern.Suggested fix
useEffect(() => { - const puckPreview = - document.querySelector<HTMLIFrameElement>("#preview-frame"); - if ( - puckPreview?.contentDocument?.head && - !puckPreview?.contentDocument.getElementById( - PREVIEW_DISABLE_POINTER_STYLE_ID - ) - ) { - // add this style to preview iFrame to prevent clicking or hover effects. - const style = puckPreview.contentDocument.createElement("style"); - style.id = PREVIEW_DISABLE_POINTER_STYLE_ID; - style.innerHTML = ` - * { - cursor: default !important; - pointer-events: none !important; - } - `; - puckPreview.contentDocument.head.appendChild(style); - } + const applyPointerBlock = () => { + const puckPreview = + document.querySelector<HTMLIFrameElement>("#preview-frame"); + if ( + !puckPreview?.contentDocument?.head || + puckPreview.contentDocument.getElementById( + PREVIEW_DISABLE_POINTER_STYLE_ID + ) + ) { + return; + } + + const style = puckPreview.contentDocument.createElement("style"); + style.id = PREVIEW_DISABLE_POINTER_STYLE_ID; + style.textContent = ` + * { + cursor: default !important; + pointer-events: none !important; + } + `; + puckPreview.contentDocument.head.appendChild(style); + }; + + applyPointerBlock(); + window.addEventListener("load", applyPointerBlock); + + return () => { + window.removeEventListener("load", applyPointerBlock); + document + .querySelector<HTMLIFrameElement>("#preview-frame") + ?.contentDocument?.getElementById(PREVIEW_DISABLE_POINTER_STYLE_ID) + ?.remove(); + }; }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/internal/puck/components/ThemeHeader.tsx` around lines 99 - 119, The pointer-blocking effect in ThemeHeader (the useEffect that queries "#preview-frame" and creates a style with id PREVIEW_DISABLE_POINTER_STYLE_ID) needs retry and cleanup: change the effect to attempt attaching the style repeatedly until the iframe document is available (e.g., setInterval retry with short delay), ensure you only append one style (check getElementById), store references to the created <style> element and the interval timer, and on cleanup clear the interval and remove the injected style from puckPreview.contentDocument (or from the element with PREVIEW_DISABLE_POINTER_STYLE_ID) so pointer-blocking is removed when the component unmounts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/visual-editor/src/internal/puck/components/ThemeHeader.tsx`:
- Around line 99-119: The pointer-blocking effect in ThemeHeader (the useEffect
that queries "#preview-frame" and creates a style with id
PREVIEW_DISABLE_POINTER_STYLE_ID) needs retry and cleanup: change the effect to
attempt attaching the style repeatedly until the iframe document is available
(e.g., setInterval retry with short delay), ensure you only append one style
(check getElementById), store references to the created <style> element and the
interval timer, and on cleanup clear the interval and remove the injected style
from puckPreview.contentDocument (or from the element with
PREVIEW_DISABLE_POINTER_STYLE_ID) so pointer-blocking is removed when the
component unmounts.
commit: |
| }; | ||
| }, []); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Am cleaning this up, don't dig too deep yet
It works though
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/visual-editor/src/internal/puck/components/ThemeHeader.tsx (1)
261-265: Consider scoping the MutationObserver to a narrower parent if possible.Observing the entire
documentwithchildList: true, subtree: truecan be expensive in complex DOMs. If the preview iframe is always inserted into a known container, observing that container instead would reduce overhead.That said, this is a reasonable approach given the need to handle dynamic iframe insertion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/internal/puck/components/ThemeHeader.tsx` around lines 261 - 265, The MutationObserver is currently observing the whole document (iframeObserver.observe(document, ...)), which is potentially expensive; change it to observe a narrower known container element (e.g., the preview iframe parent/container node) by locating that element first (query selector or existing ref) and calling iframeObserver.observe(container, { childList: true, subtree: true }) and fallback to observing document only if the container is not found; update the logic around syncPreviewFrame/iframeObserver to locate the container (use the container's unique selector or ref), and ensure you disconnect iframeObserver in the component cleanup/unmount to avoid leaks.
🤖 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/internal/puck/components/ThemeHeader.tsx`:
- Around line 261-265: The MutationObserver is currently observing the whole
document (iframeObserver.observe(document, ...)), which is potentially
expensive; change it to observe a narrower known container element (e.g., the
preview iframe parent/container node) by locating that element first (query
selector or existing ref) and calling iframeObserver.observe(container, {
childList: true, subtree: true }) and fallback to observing document only if the
container is not found; update the logic around syncPreviewFrame/iframeObserver
to locate the container (use the container's unique selector or ref), and ensure
you disconnect iframeObserver in the component cleanup/unmount to avoid leaks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1484da44-830c-4894-b6d9-1c85b2b59262
📒 Files selected for processing (1)
packages/visual-editor/src/internal/puck/components/ThemeHeader.tsx
3 participants
Hides the page title in the theme editor sidebar since we shouldn't ever select anything besides "Page"
Prevents the user from leaving Interactive mode while in the theme editor
If they leave interactive mode, they can click on components and enter a strange state