Skip to content

Commit

Permalink
Fix disappearing navigator after code editing in remix projects (#6326)
Browse files Browse the repository at this point in the history
**Problem:**
See #6314

**Root cause:**
This bug is connected to the new dom sampler, and only affects Remix
projects.
This is what is happening:
1. You fix the error in vscode
2. After an UPDATE_FROM_WORKER action the project changes and the dom
sampler runs. However, rendering of the Remix project can be async, and
only the content outside of Remix is rendering immediately, so the spy
collector only has data for those elements.
3. After this we can see that some valid navigator content is blinking
up for a second.
4. Remix content is rendering and that triggers the mutation/resize
observers which call a runDomWalker action (see this PR:
#5838 , up-to-date
metadata in remix projects rely on these observers, because there is no
dispatch which runs after all remix rendering is ready).
5. Dom sampler is running again _from scratch_, but now only the Remix
content is rendering, and nothing else outside of it! So there is no spy
data for the storyboard, for the Outlet, etc, for anything outside of
the Remix content.
6. The navigator will not render anything if it can not build a
navigator item tree from the root element, and the metadata now misses
that (and only includes elements from the Remix content).

**Fix:**
I believe the problem is that
1. The dom sampler starts from scratch in `rerender-all-elements` mode,
so does not keep any of the previous metadata
2. We only create metadata for elements which have spy data
3. We don't necessarily have spy data for all valid elements when the
dom walker was triggered by a remix render

Solution:
Let's keep the previous metadata in `rerender-all-elements` mode too.
However, in this case we have to validate if the elements in the
previous metadata are still valid. Those ones which are not in the new
spyPaths have to be checked whether they are still in the dom.
Note, that it is not enough to remove those elements which are not in
the valid path. (OR to be more precise their static path is not in the
valid paths). The valid path list is created in a fully static way from
the project contents. E.g. in case of a conditional both branches are in
the valid path list, even though only one of them is actually rendered.
And when you e.g. override a condition, it is important to remove
non-rendered elements from the metadata.

**TODO:**
- [ ] There is another very similar bug which results in a similar
symptom (empty navigator) after editing. In that case the observers are
not triggered, so the dom sampler does not run again
- [ ] it is quite confusing that we still use the dom-walker
terminology, e.g. `runDomWalker` action, `shouldRunDomWalker` etc,
rename these
- [ ] while working on this I realized that `buildTree` is dependent on
the key order of metadata, investigate that

**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 #6314
  • Loading branch information
gbalint authored and liady committed Dec 13, 2024
1 parent 193e3dd commit ba04fa5
Showing 1 changed file with 58 additions and 32 deletions.
90 changes: 58 additions & 32 deletions editor/src/components/canvas/dom-sampler.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ export function runDomSampler(options: {
let result: { metadata: ElementInstanceMetadataMap; tree: ElementPathTrees }
if (elementsToCollect == 'rerender-all-elements') {
result = collectMetadataForPaths({
metadataToUpdate_MUTATE: {},
metadataToUpdate_MUTATE: { ...options.metadataToUpdate }, // shallow cloning this object so we can mutate it
canvasRootContainer: canvasRootContainer,
pathsToCollect: validPaths,
validPaths: validPaths,
Expand All @@ -91,6 +91,7 @@ export function runDomSampler(options: {
spyCollector: options.spyCollector,
spyPaths: spyPaths,
elementCanvasRectangleCache: elementCanvasRectangleCache,
checkExistingMetadata: 'check-existing',
})
} else {
result = collectMetadataForPaths({
Expand All @@ -108,6 +109,7 @@ export function runDomSampler(options: {
spyCollector: options.spyCollector,
spyPaths: spyPaths,
elementCanvasRectangleCache: elementCanvasRectangleCache,
checkExistingMetadata: 'keep-existing',
})
}

Expand All @@ -124,6 +126,7 @@ function collectMetadataForPaths({
spyCollector,
spyPaths,
elementCanvasRectangleCache,
checkExistingMetadata,
}: {
canvasRootContainer: HTMLElement
pathsToCollect: Array<ElementPath>
Expand All @@ -134,6 +137,7 @@ function collectMetadataForPaths({
spyCollector: UiJsxCanvasContextData
spyPaths: Array<string>
elementCanvasRectangleCache: ElementCanvasRectangleCache
checkExistingMetadata: 'check-existing' | 'keep-existing'
}): {
metadata: ElementInstanceMetadataMap
tree: ElementPathTrees
Expand All @@ -146,52 +150,74 @@ function collectMetadataForPaths({
elementCanvasRectangleCache,
)

pathsToCollect.forEach((staticPath) => {
const dynamicPaths = spyPaths.filter((spyPath) =>
const dynamicPathsToCollect = pathsToCollect.flatMap((staticPath) => {
return spyPaths.filter((spyPath) =>
EP.pathsEqual(EP.makeLastPartOfPathStatic(EP.fromString(spyPath)), staticPath),
)
})

if (checkExistingMetadata === 'check-existing') {
// delete all metadata which should be collected now and those which are not in the dom anymore
Object.keys(metadataToUpdate_MUTATE).forEach((p) => {
if (dynamicPathsToCollect.includes(p)) {
delete metadataToUpdate_MUTATE[p]
}

dynamicPaths.forEach((pathString) => {
const path = EP.fromString(pathString)
const domMetadata = collectMetadataForElementPath(
path,
EP.fromString(p),
validPaths,
selectedViews,
scale,
containerRect,
elementCanvasRectangleCache,
)

const spyMetadata = spyCollector.current.spyValues.metadata[EP.toString(path)]
if (spyMetadata == null) {
// if the element is missing from the spyMetadata, we bail out. this is the same behavior as the old reconstructJSXMetadata implementation
return
}

if (domMetadata == null) {
metadataToUpdate_MUTATE[EP.toString(path)] = {
...spyMetadata,
}
return
delete metadataToUpdate_MUTATE[p]
}
})
}

let jsxElement = alternativeEither(spyMetadata.element, domMetadata.element)

// TODO avoid temporary object creation
const elementInstanceMetadata: ElementInstanceMetadata = {
...domMetadata,
element: jsxElement,
elementPath: spyMetadata.elementPath,
componentInstance: spyMetadata.componentInstance,
isEmotionOrStyledComponent: spyMetadata.isEmotionOrStyledComponent,
label: spyMetadata.label,
importInfo: spyMetadata.importInfo,
assignedToProp: spyMetadata.assignedToProp,
conditionValue: spyMetadata.conditionValue,
earlyReturn: spyMetadata.earlyReturn,
dynamicPathsToCollect.forEach((pathString) => {
const path = EP.fromString(pathString)
const domMetadata = collectMetadataForElementPath(
path,
validPaths,
selectedViews,
scale,
containerRect,
elementCanvasRectangleCache,
)

const spyMetadata = spyCollector.current.spyValues.metadata[EP.toString(path)]
if (spyMetadata == null) {
// if the element is missing from the spyMetadata, we bail out. this is the same behavior as the old reconstructJSXMetadata implementation
return
}

if (domMetadata == null) {
metadataToUpdate_MUTATE[EP.toString(path)] = {
...spyMetadata,
}
metadataToUpdate_MUTATE[EP.toString(spyMetadata.elementPath)] = elementInstanceMetadata
})
return
}

let jsxElement = alternativeEither(spyMetadata.element, domMetadata.element)

// TODO avoid temporary object creation
const elementInstanceMetadata: ElementInstanceMetadata = {
...domMetadata,
element: jsxElement,
elementPath: spyMetadata.elementPath,
componentInstance: spyMetadata.componentInstance,
isEmotionOrStyledComponent: spyMetadata.isEmotionOrStyledComponent,
label: spyMetadata.label,
importInfo: spyMetadata.importInfo,
assignedToProp: spyMetadata.assignedToProp,
conditionValue: spyMetadata.conditionValue,
earlyReturn: spyMetadata.earlyReturn,
}
metadataToUpdate_MUTATE[EP.toString(spyMetadata.elementPath)] = elementInstanceMetadata
})

const finalMetadata = [
Expand Down

0 comments on commit ba04fa5

Please sign in to comment.