fix(editor): Safari preview performance, unified block CSS, and editor scale#3726
Open
bfintal wants to merge 7 commits into
Open
fix(editor): Safari preview performance, unified block CSS, and editor scale#3726bfintal wants to merge 7 commits into
bfintal wants to merge 7 commits into
Conversation
Pool editor DOM class observers into a shared onClassChange utility with rAF batching to avoid MutationObserver feedback loops when switching device preview. Replace per-image ResizeObservers with CSS container queries for narrow resizer tooltip hiding. Fixes #2269 Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change centralizes class observation and generated block CSS, improves editor DOM and responsive preview handling, replaces image resizer width state with a container query, updates typography editing behavior, and adjusts style dependency tracking and Stylelint rules. ChangesShared class-change observation
Container-based image tooltip
Centralized editor block CSS
Editor preview readiness
Typography editing updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BlockStyleGenerator
participant EditorBlockCssStore
participant EditorBlockCssLoader
participant BlockStyleSheets
participant EditorDocument
BlockStyleGenerator->>EditorBlockCssStore: Set generated block CSS
EditorBlockCssStore->>EditorBlockCssLoader: Publish SET change
EditorBlockCssLoader->>BlockStyleSheets: Batch stylesheet update
BlockStyleSheets->>EditorDocument: Adopt stylesheet or sync style element
EditorBlockCssStore->>EditorBlockCssLoader: Publish REMOVE change
EditorBlockCssLoader->>BlockStyleSheets: Remove stylesheet
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🤖 Pull request artifacts
|
|
Size Change: +2.07 kB (+0.08%) Total Size: 2.63 MB 📦 View Changed
ℹ️ View Unchanged
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/util/on-class-change.js (1)
52-64: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
attributeFilter: ['class']to reduce unnecessary observer invocationsThe observer currently fires for all attribute changes and filters in the callback. The editor element has many attributes that change during editing (
data-type,style, etc.). UsingattributeFilterprevents firing for non-class changes entirely, eliminating unnecessary mutation record creation and callback overhead — directly aligned with this PR's Safari performance objective.⚡ Proposed fix
Apply at both
observe()call sites (lines 37 and 64):- state.mutationObserver.observe( node, { attributes: true } ) + state.mutationObserver.observe( node, { attributes: true, attributeFilter: [ 'class' ] } )- state.mutationObserver.observe( node, { attributes: true } ) + state.mutationObserver.observe( node, { attributes: true, attributeFilter: [ 'class' ] } )With
attributeFilterin place, theitem.attributeName === 'class'check in the MutationObserver callback (line 54) becomes redundant and can be simplified.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/util/on-class-change.js` around lines 52 - 64, Configure both MutationObserver.observe call sites in the class-change handling logic to use { attributes: true, attributeFilter: ['class'] }, preventing callbacks for unrelated attribute updates. Update the MutationObserver callback around scheduleCallbacks to remove the now-redundant item.attributeName === 'class' check while preserving class-change detection and callback scheduling.src/block-components/image/editor.scss (1)
87-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueContainer query approach looks solid.
The
.stk-img-resizer→@container→.stk-resizer-tooltipchain is correct: the relevant code insrc/components/resizer-tooltip/index.jsconfirms.stk-resizer-tooltipis a descendant of.stk-img-resizer.container-type: inline-sizeis safe here since the resizable box has explicit inline dimensions.One minor note: the
139pxthreshold is a magic number. Consider adding a brief comment explaining why this specific value was chosen (e.g., "Below this width the tooltip overflows the resizer bounds").🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/block-components/image/editor.scss` around lines 87 - 97, Add a brief comment next to the 139px container-query threshold explaining that widths below it cause the resizer tooltip to overflow the resizer bounds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/util/on-class-change.js`:
- Around line 30-37: Wrap the callback loop in the class-change observer handler
with a try-finally block so state.mutationObserver.observe(node, { attributes:
true }) always executes even when a callback throws. Preserve callback error
propagation while guaranteeing reconnection after iterating state.callbacks.
---
Nitpick comments:
In `@src/block-components/image/editor.scss`:
- Around line 87-97: Add a brief comment next to the 139px container-query
threshold explaining that widths below it cause the resizer tooltip to overflow
the resizer bounds.
In `@src/util/on-class-change.js`:
- Around line 52-64: Configure both MutationObserver.observe call sites in the
class-change handling logic to use { attributes: true, attributeFilter:
['class'] }, preventing callbacks for unrelated attribute updates. Update the
MutationObserver callback around scheduleCallbacks to remove the now-redundant
item.attributeName === 'class' check while preserving class-change detection and
callback scheduling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f3266dff-1e33-402c-948b-db3a4d9f37d2
📒 Files selected for processing (10)
.stylelintrc.jssrc/block-components/image/editor.scsssrc/block-components/image/image.jssrc/plugins/editor-device-preview-class/index.jssrc/plugins/global-settings/color-schemes/editor-loader.jssrc/plugins/global-settings/utils/block-layout-utils.jssrc/plugins/global-settings/utils/use-block-layout-editor-loader.jssrc/plugins/theme-block-style-inheritance/index.jssrc/util/index.jssrc/util/on-class-change.js
💤 Files with no reviewable changes (2)
- src/plugins/global-settings/utils/block-layout-utils.js
- src/block-components/image/image.js
Comment on lines
+30
to
+37
| state.mutationObserver.disconnect() | ||
|
|
||
| for ( const callback of state.callbacks ) { | ||
| callback() | ||
| } | ||
|
|
||
| state.lastClassString = node.classList.toString() | ||
| state.mutationObserver.observe( node, { attributes: true } ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wrap callback execution in try-finally to prevent permanent observer disconnection
If a callback throws, the observer is never re-connected (line 37 is unreachable), permanently breaking class-change detection for all subscribers on that node. Since multiple plugins share the same editorEl observer, one failing callback silences every consumer.
🔒 Proposed fix
state.mutationObserver.disconnect()
- for ( const callback of state.callbacks ) {
- callback()
- }
+ try {
+ for ( const callback of state.callbacks ) {
+ callback()
+ }
+ } finally {
+ state.lastClassString = node.classList.toString()
+ state.mutationObserver.observe( node, { attributes: true } )
+ }
- state.lastClassString = node.classList.toString()
- state.mutationObserver.observe( node, { attributes: true } )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| state.mutationObserver.disconnect() | |
| for ( const callback of state.callbacks ) { | |
| callback() | |
| } | |
| state.lastClassString = node.classList.toString() | |
| state.mutationObserver.observe( node, { attributes: true } ) | |
| state.mutationObserver.disconnect() | |
| try { | |
| for ( const callback of state.callbacks ) { | |
| callback() | |
| } | |
| } finally { | |
| state.lastClassString = node.classList.toString() | |
| state.mutationObserver.observe( node, { attributes: true } ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/util/on-class-change.js` around lines 30 - 37, Wrap the callback loop in
the class-change observer handler with a try-finally block so
state.mutationObserver.observe(node, { attributes: true }) always executes even
when a callback throws. Preserve callback error propagation while guaranteeing
reconnection after iterating state.callbacks.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/plugins/editor-block-css/index.js (1)
73-83: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist the constructable-support check out of the per-key loop.
shouldUseConstructableStyleSheets( editorDom )is evaluated for every key inkeysToUpdate(Line 74) and again at Line 81. Each call runsgetEditorCanvasDocument(), which does twodocument.querySelectorlookups. On pages with many Stackable blocks (the exact scenario this PR targets) this is O(N) DOM queries per animation frame. Compute it once per frame.♻️ Proposed refactor
const keysToUpdate = [ ...pendingKeys ] pendingKeys.clear() + const useConstructable = shouldUseConstructableStyleSheets( editorDom ) keysToUpdate.forEach( key => { - if ( shouldUseConstructableStyleSheets( editorDom ) ) { + if ( useConstructable ) { replaceBlockSheetCss( key, blockStyles[ key ], editorDom ) } else { syncBlockStyleSheet( key, blockStyles[ key ], editorDom ) } } ) - if ( shouldUseConstructableStyleSheets( editorDom ) && ( keysToUpdate.length || hadRemovals ) ) { + if ( useConstructable && ( keysToUpdate.length || hadRemovals ) ) { adoptBlockStyleSheets( editorDom ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/editor-block-css/index.js` around lines 73 - 83, Compute shouldUseConstructableStyleSheets(editorDom) once before iterating over keysToUpdate, store the result, and reuse it for both the per-key replaceBlockSheetCss/syncBlockStyleSheet branch and the final adoptBlockStyleSheets condition. This removes repeated getEditorCanvasDocument() DOM lookups while preserving the existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugins/editor-block-css/block-style-sheets.js`:
- Around line 206-220: Update removeBlockStyleSheet and the stale-key cleanup in
readoptAllBlockStyleSheets to remove deleted constructable stylesheets from
doc.adoptedStyleSheets before deleting their documentStyleSheets entries or
calling mergeAdoptedStyleSheets. Preserve foreign sheets, then rebuild the
adopted sheet list so removed block sheets are not reclassified as foreign.
---
Nitpick comments:
In `@src/plugins/editor-block-css/index.js`:
- Around line 73-83: Compute shouldUseConstructableStyleSheets(editorDom) once
before iterating over keysToUpdate, store the result, and reuse it for both the
per-key replaceBlockSheetCss/syncBlockStyleSheet branch and the final
adoptBlockStyleSheets condition. This removes repeated getEditorCanvasDocument()
DOM lookups while preserving the existing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e76ae1c-39c2-456a-a7df-2e94e41e0852
📒 Files selected for processing (5)
src/components/block-css/use-block-style-generator.jssrc/plugins/editor-block-css/block-style-sheets.jssrc/plugins/editor-block-css/index.jssrc/plugins/editor-block-css/store.jssrc/plugins/index.js
✅ Files skipped from review due to trivial changes (2)
- src/plugins/index.js
- src/plugins/editor-block-css/store.js
Comment on lines
+206
to
+220
| export const removeBlockStyleSheet = ( key, editorDom ) => { | ||
| const doc = getTargetEditorDocument( editorDom ) | ||
|
|
||
| getDocumentStyleSheets( doc ).delete( key ) | ||
|
|
||
| const fallbackEl = getDocumentFallbackStyles( doc ).get( key ) | ||
| if ( fallbackEl ) { | ||
| fallbackEl.remove() | ||
| getDocumentFallbackStyles( doc ).delete( key ) | ||
| } | ||
|
|
||
| if ( shouldUseConstructableStyleSheets( editorDom ) && doc?.adoptedStyleSheets ) { | ||
| mergeAdoptedStyleSheets( doc ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate all call sites that mutate adoptedStyleSheets and the sheet lifecycle
rg -nP --type=js -C3 'adoptedStyleSheets|documentStyleSheets|getBlockStyleSheet' src/plugins/editor-block-css/block-style-sheets.jsRepository: gambitph/Stackable
Length of output: 3227
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/plugins/editor-block-css/block-style-sheets.js"
echo "=== lines 120-240 ==="
sed -n '120,240p' "$file" | cat -n
echo
echo "=== symbol outline ==="
ast-grep outline "$file" --view expandedRepository: gambitph/Stackable
Length of output: 4531
Drop deleted sheets from adoptedStyleSheets before merging.
mergeAdoptedStyleSheets() rebuilds the array from documentStyleSheets values, so deleting the map entry first leaves the old sheet in doc.adoptedStyleSheets and it gets treated as foreign. The same applies to the stale-key cleanup in readoptAllBlockStyleSheets.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugins/editor-block-css/block-style-sheets.js` around lines 206 - 220,
Update removeBlockStyleSheet and the stale-key cleanup in
readoptAllBlockStyleSheets to remove deleted constructable stylesheets from
doc.adoptedStyleSheets before deleting their documentStyleSheets entries or
calling mergeAdoptedStyleSheets. Preserve foreign sheets, then rebuild the
adopted sheet list so removed block sheets are not reclassified as foreign.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugins/editor-block-css/index.js`:
- Around line 88-96: In the lastChange handling logic, clear the removed block’s
key from pendingKeys before calling removeBlockStyleSheet in the REMOVE branch.
Update the code associated with the pendingKeys collection and
removeBlockStyleSheet so queued SET updates cannot recreate styles for deleted
blocks.
In `@src/plugins/get-editor-dom/index.js`:
- Around line 54-82: Update the editor-DOM resolution flow around
commitEditorDom, tryResolve, and the MutationObserver to disconnect the observer
immediately after the first successful resolution, regardless of whether it
succeeds via the initial check, iframe load, or mutation callback; retain
cleanup safety and the iframe reload behavior as described. Also deduplicate
commitEditorDom by comparing the resolved DOM element with the current store
value and dispatching stackable/editor-dom only when the value has changed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0fa948a2-bb33-4546-b8d9-84731b970317
📒 Files selected for processing (5)
src/components/block-css/use-block-style-generator.jssrc/higher-order/with-block-wrapper/index.jssrc/plugins/editor-block-css/index.jssrc/plugins/editor-device-preview-class/index.jssrc/plugins/get-editor-dom/index.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/block-css/use-block-style-generator.js
Comment on lines
+88
to
+96
| if ( lastChange.type === 'REMOVE' ) { | ||
| const editorDom = select( 'stackable/editor-dom' ).getEditorDom() | ||
| removeBlockStyleSheet( lastChange.key, editorDom ) | ||
| return | ||
| } | ||
|
|
||
| if ( lastChange.type === 'SET' ) { | ||
| pendingKeys.add( lastChange.key ) | ||
| scheduleStyleUpdates() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C5 'export const replaceBlockSheetCss' src/plugins/editor-block-css/block-style-sheets.jsRepository: gambitph/Stackable
Length of output: 507
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== index.js outline ==\n'
ast-grep outline src/plugins/editor-block-css/index.js --view expanded || true
printf '\n== block-style-sheets.js outline ==\n'
ast-grep outline src/plugins/editor-block-css/block-style-sheets.js --view expanded || true
printf '\n== pendingKeys usages ==\n'
rg -n "pendingKeys|replaceSheetCss|syncBlockStyleSheet|removeBlockStyleSheet" src/plugins/editor-block-css
printf '\n== relevant slices ==\n'
sed -n '1,220p' src/plugins/editor-block-css/index.js
printf '\n--- block-style-sheets.js ---\n'
sed -n '1,260p' src/plugins/editor-block-css/block-style-sheets.jsRepository: gambitph/Stackable
Length of output: 12843
Clear pendingKeys on REMOVE.
A queued SET can leave its key behind after the block is removed, so the next frame recreates an empty stylesheet/style tag for a block that’s already gone. Drop the key before removeBlockStyleSheet(...).
Proposed fix
if ( lastChange.type === 'REMOVE' ) {
const editorDom = select( 'stackable/editor-dom' ).getEditorDom()
+ pendingKeys.delete( lastChange.key )
removeBlockStyleSheet( lastChange.key, editorDom )
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ( lastChange.type === 'REMOVE' ) { | |
| const editorDom = select( 'stackable/editor-dom' ).getEditorDom() | |
| removeBlockStyleSheet( lastChange.key, editorDom ) | |
| return | |
| } | |
| if ( lastChange.type === 'SET' ) { | |
| pendingKeys.add( lastChange.key ) | |
| scheduleStyleUpdates() | |
| if ( lastChange.type === 'REMOVE' ) { | |
| const editorDom = select( 'stackable/editor-dom' ).getEditorDom() | |
| pendingKeys.delete( lastChange.key ) | |
| removeBlockStyleSheet( lastChange.key, editorDom ) | |
| return | |
| } | |
| if ( lastChange.type === 'SET' ) { | |
| pendingKeys.add( lastChange.key ) | |
| scheduleStyleUpdates() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugins/editor-block-css/index.js` around lines 88 - 96, In the
lastChange handling logic, clear the removed block’s key from pendingKeys before
calling removeBlockStyleSheet in the REMOVE branch. Update the code associated
with the pendingKeys collection and removeBlockStyleSheet so queued SET updates
cannot recreate styles for deleted blocks.
Comment on lines
+54
to
+82
| const commitEditorDom = dom => { | ||
| if ( cancelledRef.current || ! dom ) { | ||
| return false | ||
| } | ||
|
|
||
| dispatch( 'stackable/editor-dom' ).updateEditorDom( dom ) | ||
| return true | ||
| } | ||
|
|
||
| if ( deviceType === 'Desktop' ) { | ||
| // We have to wait for the editor area to load (e.g. FSE iframe takes a long time to load) | ||
| interval.current = setInterval( () => { | ||
| const editorEl = document.querySelector( `.editor-styles-wrapper, iframe[name="editor-canvas"]` ) | ||
| if ( editorEl ) { | ||
| clearInterval( interval.current ) | ||
| clearTimeout( timeout.current ) | ||
| setIframeForceUpdate( v => v + 1 ) | ||
| // There's a chance that the editor STILL isn't ready, try again. | ||
| setTimeout( () => { | ||
| setIframeForceUpdate( v => v + 1 ) | ||
| }, 200 ) | ||
| } | ||
| }, 200 ) | ||
| } else { // Tablet or Mobile. | ||
| const iframeEl = document.querySelector( `iframe[name="editor-canvas"]` ) | ||
| if ( iframeEl ) { | ||
| const body = iframeEl.contentDocument.body | ||
| if ( body && body.querySelector( '.block-editor-block-list__layout' ) ) { | ||
| clearInterval( interval.current ) | ||
| setIframeForceUpdate( v => v + 1 ) | ||
| } else { | ||
| clearTimeout( timeout.current ) | ||
| timeout.current = setTimeout( () => { | ||
| const body = iframeEl.contentDocument.body | ||
| if ( body && body.querySelector( '.block-editor-block-list__layout' ) ) { | ||
| setIframeForceUpdate( v => v + 1 ) | ||
| } | ||
| }, 200 ) | ||
| iframeEl.onload = () => { | ||
| clearTimeout( timeout.current ) | ||
| const body = iframeEl.contentDocument.body | ||
| if ( body && body.querySelector( '.block-editor-block-list__layout' ) ) { | ||
| setIframeForceUpdate( v => v + 1 ) | ||
| } else { | ||
| setTimeout( () => { | ||
| setIframeForceUpdate( v => v + 1 ) | ||
| }, 200 ) | ||
| } | ||
| } | ||
| } | ||
| if ( commitEditorDom( resolveEditorDom() ) ) { | ||
| return () => { | ||
| cancelledRef.current = true | ||
| } | ||
| } | ||
|
|
||
| return () => { | ||
| clearInterval( interval.current ) | ||
| clearTimeout( timeout.current ) | ||
| const tryResolve = () => { | ||
| return commitEditorDom( resolveEditorDom() ) | ||
| } | ||
| }, [ deviceType, editorMode, editedSitePostId, currentPage ] ) | ||
|
|
||
| useMemo( () => { | ||
| const iframeEl = document.querySelector( `iframe[name="editor-canvas"]` ) | ||
| if ( iframeEl ) { | ||
| setEditorDom( iframeEl.contentDocument.body ) | ||
| } else { | ||
| setEditorDom( document.querySelector( `.editor-styles-wrapper` ) ) | ||
| const iframeEl = document.querySelector( 'iframe[name="editor-canvas"]' ) | ||
| const onIframeLoad = () => { | ||
| tryResolve() | ||
| } | ||
| iframeEl?.addEventListener( 'load', onIframeLoad ) | ||
|
|
||
| const observer = new MutationObserver( () => { | ||
| tryResolve() | ||
| } ) | ||
| observer.observe( document.body, { childList: true, subtree: true } ) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
MutationObserver remains active after successful resolution, causing per-mutation overhead
The MutationObserver on document.body with subtree: true is never disconnected after tryResolve() succeeds. Every subsequent DOM mutation triggers resolveEditorDom() (2–3 querySelector calls) and a store dispatch—even though the resolved DOM hasn't changed. In a Gutenberg editor with frequent DOM mutations (typing, selection, hover), this creates continuous unnecessary work that counteracts this PR's Safari performance goals.
Additionally, commitEditorDom dispatches unconditionally, even when the DOM element is identical to the current store value. The reducer always produces a new state object, so every dispatch triggers subscriber notifications.
Suggested fixes: disconnect the observer after first successful resolution (regardless of which callback triggered it), and skip the dispatch when the DOM hasn't changed.
⚡ Proposed fix: disconnect observer on success + deduplicate dispatch
import { dispatch, useSelect } from '`@wordpress/data`'
+import { dispatch, select, useSelect } from '`@wordpress/data`' const commitEditorDom = dom => {
if ( cancelledRef.current || ! dom ) {
return false
}
+ if ( select( 'stackable/editor-dom' ).getEditorDom() === dom ) {
+ return true
+ }
dispatch( 'stackable/editor-dom' ).updateEditorDom( dom )
return true
}+ let observer
+
const tryResolve = () => {
- return commitEditorDom( resolveEditorDom() )
+ const success = commitEditorDom( resolveEditorDom() )
+ if ( success && observer ) {
+ observer.disconnect()
+ }
+ return success
}- const observer = new MutationObserver( () => {
+ observer = new MutationObserver( () => {
tryResolve()
} )
observer.observe( document.body, { childList: true, subtree: true } )The cleanup function's observer.disconnect() remains safe—calling disconnect on an already-disconnected observer is a no-op. The iframe load listener stays active to handle iframe reloads within the same session, and the effect dependencies (deviceType, editorMode, etc.) ensure the observer is re-created when the editor context changes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const commitEditorDom = dom => { | |
| if ( cancelledRef.current || ! dom ) { | |
| return false | |
| } | |
| dispatch( 'stackable/editor-dom' ).updateEditorDom( dom ) | |
| return true | |
| } | |
| if ( deviceType === 'Desktop' ) { | |
| // We have to wait for the editor area to load (e.g. FSE iframe takes a long time to load) | |
| interval.current = setInterval( () => { | |
| const editorEl = document.querySelector( `.editor-styles-wrapper, iframe[name="editor-canvas"]` ) | |
| if ( editorEl ) { | |
| clearInterval( interval.current ) | |
| clearTimeout( timeout.current ) | |
| setIframeForceUpdate( v => v + 1 ) | |
| // There's a chance that the editor STILL isn't ready, try again. | |
| setTimeout( () => { | |
| setIframeForceUpdate( v => v + 1 ) | |
| }, 200 ) | |
| } | |
| }, 200 ) | |
| } else { // Tablet or Mobile. | |
| const iframeEl = document.querySelector( `iframe[name="editor-canvas"]` ) | |
| if ( iframeEl ) { | |
| const body = iframeEl.contentDocument.body | |
| if ( body && body.querySelector( '.block-editor-block-list__layout' ) ) { | |
| clearInterval( interval.current ) | |
| setIframeForceUpdate( v => v + 1 ) | |
| } else { | |
| clearTimeout( timeout.current ) | |
| timeout.current = setTimeout( () => { | |
| const body = iframeEl.contentDocument.body | |
| if ( body && body.querySelector( '.block-editor-block-list__layout' ) ) { | |
| setIframeForceUpdate( v => v + 1 ) | |
| } | |
| }, 200 ) | |
| iframeEl.onload = () => { | |
| clearTimeout( timeout.current ) | |
| const body = iframeEl.contentDocument.body | |
| if ( body && body.querySelector( '.block-editor-block-list__layout' ) ) { | |
| setIframeForceUpdate( v => v + 1 ) | |
| } else { | |
| setTimeout( () => { | |
| setIframeForceUpdate( v => v + 1 ) | |
| }, 200 ) | |
| } | |
| } | |
| } | |
| if ( commitEditorDom( resolveEditorDom() ) ) { | |
| return () => { | |
| cancelledRef.current = true | |
| } | |
| } | |
| return () => { | |
| clearInterval( interval.current ) | |
| clearTimeout( timeout.current ) | |
| const tryResolve = () => { | |
| return commitEditorDom( resolveEditorDom() ) | |
| } | |
| }, [ deviceType, editorMode, editedSitePostId, currentPage ] ) | |
| useMemo( () => { | |
| const iframeEl = document.querySelector( `iframe[name="editor-canvas"]` ) | |
| if ( iframeEl ) { | |
| setEditorDom( iframeEl.contentDocument.body ) | |
| } else { | |
| setEditorDom( document.querySelector( `.editor-styles-wrapper` ) ) | |
| const iframeEl = document.querySelector( 'iframe[name="editor-canvas"]' ) | |
| const onIframeLoad = () => { | |
| tryResolve() | |
| } | |
| iframeEl?.addEventListener( 'load', onIframeLoad ) | |
| const observer = new MutationObserver( () => { | |
| tryResolve() | |
| } ) | |
| observer.observe( document.body, { childList: true, subtree: true } ) | |
| const commitEditorDom = dom => { | |
| if ( cancelledRef.current || ! dom ) { | |
| return false | |
| } | |
| if ( select( 'stackable/editor-dom' ).getEditorDom() === dom ) { | |
| return true | |
| } | |
| dispatch( 'stackable/editor-dom' ).updateEditorDom( dom ) | |
| return true | |
| } | |
| let observer | |
| const tryResolve = () => { | |
| const success = commitEditorDom( resolveEditorDom() ) | |
| if ( success && observer ) { | |
| observer.disconnect() | |
| } | |
| return success | |
| } | |
| if ( commitEditorDom( resolveEditorDom() ) ) { | |
| return () => { | |
| cancelledRef.current = true | |
| } | |
| } | |
| const iframeEl = document.querySelector( 'iframe[name="editor-canvas"]' ) | |
| const onIframeLoad = () => { | |
| tryResolve() | |
| } | |
| iframeEl?.addEventListener( 'load', onIframeLoad ) | |
| observer = new MutationObserver( () => { | |
| tryResolve() | |
| } ) | |
| observer.observe( document.body, { childList: true, subtree: true } ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugins/get-editor-dom/index.js` around lines 54 - 82, Update the
editor-DOM resolution flow around commitEditorDom, tryResolve, and the
MutationObserver to disconnect the observer immediately after the first
successful resolution, regardless of whether it succeeds via the initial check,
iframe load, or mutation callback; retain cleanup safety and the iframe reload
behavior as described. Also deduplicate commitEditorDom by comparing the
resolved DOM element with the current store value and dispatching
stackable/editor-dom only when the value has changed.
bfintal
changed the title
Resolve conflict in use-block-style-generator.js by combining PR #3716's setAttributes fix with styleFingerprint-based save CSS generation. Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2269 — Safari (and other browsers) freezing or spiking CPU when switching Gutenberg responsive preview (Desktop → Tablet/Mobile) on pages with many Stackable blocks.
This PR is a layered editor performance pass: reduce work on preview remount, avoid redundant CSS regeneration, and cut per-block observer/DOM overhead at scale.
Preview switch & editor DOM
onClassChangeutility — single pooledMutationObserverper DOM node, rAF-batched callbacks, observer disconnected during callback execution (prevents class-mutation feedback loops).onClassChangeimplementations across device preview class, theme block style inheritance, color schemes, and design-system loaders.get-editor-dom—useLayoutEffect, rAF polling +iframe.onload+ scopedMutationObserver(replaces 200ms intervals); sync dispatch to store.editor-device-preview-class— device/hover/theme classes applied inuseLayoutEffect(before paint).useDevicePreviewOptimization— restored root-block mount stagger after preview remount (was 300ms ondevelop). Now 50ms base + random 0–99ms spread (~50–149ms per unselected root block) so mounts spread across frames instead of bunching in one commit. Selected root blocks and all nested blocks still mount immediately. Safe with unified CSS because block styles persist instackable/editor-block-cssand are re-adopted on preview switch.Unified editor block CSS (
stackable/editor-block-css)useBlockCssGenerator; no per-block<style>in the React tree.CSSStyleSheetviaadoptedStyleSheetson the main document (classic editor).<style id="stk-block-css-*">fallback inside the editor-canvas iframe (FSE / tablet / mobile preview) — constructable sheets cannot be shared across documents.removeBlockCssonly when the block is deleted from the editor.SET); synchronousreadoptAllon preview switch.CSS regeneration at scale
useBlockCssGeneratoronly regenerates CSS when style-relevant attributes change (not on text/content-only edits).renderConditionattrs,valuePreCallbackattrs,enabledCallbackattrs (parsed from callback source), and dynamic conditional styles (addBlockStyleConditionallydependency list — e.g. ColumnscolumnArrangement).Text editing
Typography(canvas + inspector). InstantsetAttributeson keystroke; fingerprint prevents CSS regen on text-only edits.enableDebounce={ true }remains available for edge cases.Image block
ResizeObserverin the editor; container queries hide the resizer tooltip when the image is narrow (<140px).container-type/@container.Related issues
Related: #2225, #3394
How to test whether this PR is good
Use a stress page (50–100+ Stackable blocks, nested columns, images, styled cards/headings) in Safari first, then Chrome. Open Activity Monitor / Task Manager and watch CPU while running the checks below.
For React Profiler: compare commit count and longest single commit — staggered root mounts should avoid one huge spike; also check time-to-interactive (can you select a block right after switch?).
1. Preview switch (primary fix — #2269)
2. Block CSS still works
enabledCallbackfingerprint).className-gated callbacks).3. Text editing (no debounce regression)
4. Global settings & classes
stk-preview-device-*, hover state classes) still apply after preview switch.5. Image resizer tooltip
6. Regression smoke
Pass criteria
Known follow-ups (out of scope)
{ blockCss && <style> }in ~46 blockedit.jsfiles (harmless; plugin injects CSS).SETwithout equality check — may still dispatch on remount at very large block counts.<style>tags (chunking could help 200+ blocks further).Summary by CodeRabbit
New Features
Bug Fixes
Performance