Skip to content

Commit

Permalink
Fix subscribing mutation/resize observers during a build error (#6344)
Browse files Browse the repository at this point in the history
**Problem:**
Followup to #6326
In a Remix project it can happen that in the dispatch flow we don't have
the results of the rendering in the dom yet. It can even happen, that
after a build error is fixed, we still don't have the canvas root
container in the dom.
That is a critical issue, because we attach the mutation/resize
observers to the canvas root container element, so when we don't have
that we can subscribe them, so the dom sample is not going to run when
the rendering is finished.

**Fix:**
I needed an element, which is an ancestor of the canvas root container
and
1 is always rendered
2 does not contain canvas controls or other content
I choose canvas-container-outer, but I needed to modify its ancestors so
it is always rendered, even when there is a build error.
To make sure we don't observe unnecessary elements, I filtered the query
for the resize observer to only include elements with the `data-uid`
attribute.

I needed to update the tests to make sure the observers from the
previous domwalkermutablestate don't continue to fire when there is a
change, because the outer canvas container can stay persistent between
test runs.

**Manual Tests:**
I hereby swear that:

- [x] I opened a hydrogen project and it loaded
- [x] I could navigate to various routes in Preview mode

Fixes #6345
  • Loading branch information
gbalint authored Sep 11, 2024
1 parent 73b4df7 commit a2d6ac7
Show file tree
Hide file tree
Showing 5 changed files with 39 additions and 25 deletions.
13 changes: 9 additions & 4 deletions editor/src/components/canvas/canvas-component-entry.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,11 @@ import type {
} from './ui-jsx-canvas'
import { DomWalkerInvalidatePathsCtxAtom, UiJsxCanvas, pickUiJsxCanvasProps } from './ui-jsx-canvas'

interface CanvasComponentEntryProps {}
export const CanvasContainerOuterId = 'canvas-container-outer'

interface CanvasComponentEntryProps {
shouldRenderCanvas: boolean
}

export const CanvasComponentEntry = React.memo((props: CanvasComponentEntryProps) => {
const canvasStore = React.useContext(CanvasStateContext)
Expand Down Expand Up @@ -85,22 +89,23 @@ const CanvasComponentEntryInner = React.memo((props: CanvasComponentEntryProps)
<>
{when(canvasProps == null, <CanvasLoadingScreen />)}
<div
id='canvas-container-outer'
id={CanvasContainerOuterId}
ref={containerRef}
style={{
position: 'absolute',
transition: canvasScrollAnimation ? 'transform 0.3s ease-in-out' : 'initial',
transform: 'translate3d(0px, 0px, 0px)',
}}
>
{canvasProps == null ? null : (
{props.shouldRenderCanvas && canvasProps != null ? (
<CanvasInner
canvasProps={canvasProps}
onRuntimeError={onRuntimeError}
localClearRuntimeErrors={localClearRuntimeErrors}
/>
)}
) : null}
</div>
,
</>
)
})
Expand Down
21 changes: 10 additions & 11 deletions editor/src/components/canvas/canvas-wrapper-component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -85,17 +85,16 @@ export const CanvasWrapperComponent = React.memo(() => {
// ^ prevents Monaco from pushing the inspector out
}}
>
{fatalErrors.length === 0 && !safeMode ? (
<EditorCanvas
userState={userState}
editor={editorState}
derived={derivedState}
model={createCanvasModelKILLME(editorState, derivedState)}
builtinDependencies={builtinDependencies}
updateCanvasSize={updateCanvasSize}
dispatch={dispatch}
/>
) : null}
<EditorCanvas
userState={userState}
editor={editorState}
derived={derivedState}
model={createCanvasModelKILLME(editorState, derivedState)}
builtinDependencies={builtinDependencies}
updateCanvasSize={updateCanvasSize}
dispatch={dispatch}
shouldRenderCanvas={fatalErrors.length === 0 && !safeMode}
/>

<FlexRow
style={{
Expand Down
9 changes: 5 additions & 4 deletions editor/src/components/canvas/dom-walker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,7 @@ import {
} from '../inspector/common/css-utils'
import { camelCaseToDashed } from '../../core/shared/string-utils'
import type { UtopiaStoreAPI } from '../editor/store/store-hook'
import { UTOPIA_SCENE_ID_KEY } from '../../core/model/utopia-constants'
import { CanvasContainerID } from './canvas-types'
import { UTOPIA_SCENE_ID_KEY, UTOPIA_UID_KEY } from '../../core/model/utopia-constants'
import { emptySet } from '../../core/shared/set-utils'
import type { PathWithString } from '../../core/shared/uid-utils'
import { getDeepestPathOnDomElement, getPathStringsOnDomElement } from '../../core/shared/uid-utils'
Expand All @@ -72,6 +71,7 @@ import { pick } from '../../core/shared/object-utils'
import { getFlexAlignment, getFlexJustifyContent, MaxContent } from '../inspector/inspector-common'
import type { EditorDispatch } from '../editor/action-types'
import { runDOMWalker } from '../editor/actions/action-creators'
import { CanvasContainerOuterId } from './canvas-component-entry'

export const ResizeObserver =
window.ResizeObserver ?? ResizeObserverSyntheticDefault.default ?? ResizeObserverSyntheticDefault
Expand Down Expand Up @@ -291,14 +291,15 @@ export function resubscribeObservers(domWalkerMutableState: {
mutationObserver: MutationObserver
resizeObserver: ResizeObserver
}) {
const canvasRootContainer = document.getElementById(CanvasContainerID)
const canvasRootContainer = document.getElementById(CanvasContainerOuterId)

if (
ObserversAvailable &&
canvasRootContainer != null &&
domWalkerMutableState.resizeObserver != null &&
domWalkerMutableState.mutationObserver != null
) {
document.querySelectorAll(`#${CanvasContainerID} *`).forEach((elem) => {
document.querySelectorAll(`#${CanvasContainerOuterId} [${UTOPIA_UID_KEY}]`).forEach((elem) => {
domWalkerMutableState.resizeObserver.observe(elem)
})
domWalkerMutableState.mutationObserver.observe(canvasRootContainer, MutationObserverConfig)
Expand Down
5 changes: 5 additions & 0 deletions editor/src/components/canvas/ui-jsx.test-utils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,8 @@ export function optOutFromCheckFileTimestamps() {
})
}

let prevDomWalkerMutableState: DomWalkerMutableStateData | null = null

export async function renderTestEditorWithModel(
rawModel: PersistentModel,
awaitFirstDomReport: 'await-first-dom-report' | 'dont-await-first-dom-report',
Expand Down Expand Up @@ -600,7 +602,10 @@ export async function renderTestEditorWithModel(
patchedStoreFromFullStore(initialEditorStore, 'canvas-store'),
)

prevDomWalkerMutableState?.resizeObserver.disconnect()
prevDomWalkerMutableState?.mutationObserver.disconnect()
const domWalkerMutableState = createDomWalkerMutableState(canvasStoreHook, asyncTestDispatch)
prevDomWalkerMutableState = domWalkerMutableState

const lowPriorityStoreHook: UtopiaStoreAPI = createStoresAndState(
patchedStoreFromFullStore(initialEditorStore, 'low-priority-store'),
Expand Down
16 changes: 10 additions & 6 deletions editor/src/templates/editor-canvas.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -695,6 +695,7 @@ interface EditorCanvasProps {
builtinDependencies: BuiltInDependencies
dispatch: EditorDispatch
updateCanvasSize: (newValueOrUpdater: Size | ((oldValue: Size) => Size)) => void
shouldRenderCanvas: boolean
}

export class EditorCanvas extends React.Component<EditorCanvasProps> {
Expand Down Expand Up @@ -855,6 +856,10 @@ export class EditorCanvas extends React.Component<EditorCanvasProps> {
// we round the offset here, so all layers, the canvas, and controls use the same rounded value for positioning
// instead of letting Chrome do it, because that results in funky artifacts (e.g. while scrolling, the layers don't "jump" pixels at the same time)

if (!this.props.shouldRenderCanvas) {
return <CanvasComponentEntry shouldRenderCanvas={false} />
}

const nodeConnectorsDiv = createNodeConnectorsDiv(
this.props.model.canvasOffset,
this.props.model.scale,
Expand All @@ -871,10 +876,9 @@ export class EditorCanvas extends React.Component<EditorCanvasProps> {

const canvasIsLive = isLiveMode(this.props.editor.mode)

const canvasControls = React.createElement(NewCanvasControls, {
windowToCanvasPosition: this.getPosition,
cursor,
})
const canvasControls = (
<NewCanvasControls windowToCanvasPosition={this.getPosition} cursor={cursor} />
)

const canvasLiveEditingStyle = canvasIsLive
? UtopiaStyles.canvas.live
Expand Down Expand Up @@ -1070,9 +1074,9 @@ export class EditorCanvas extends React.Component<EditorCanvasProps> {
},
},
nodeConnectorsDiv,
React.createElement(CanvasComponentEntry, {}),
<CanvasComponentEntry shouldRenderCanvas={true} />,
canvasControls,
React.createElement(CursorComponent, {}),
<CursorComponent />,
<EditorCommon mouseDown={this.handleMouseDown} />,
)
}
Expand Down

0 comments on commit a2d6ac7

Please sign in to comment.