Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Navigator can remain empty after a change that introduces an error #6314

Closed
Rheeseyb opened this issue Sep 4, 2024 · 0 comments · Fixed by #6326
Closed

Navigator can remain empty after a change that introduces an error #6314

Rheeseyb opened this issue Sep 4, 2024 · 0 comments · Fixed by #6326
Assignees

Comments

@Rheeseyb
Copy link
Contributor

Rheeseyb commented Sep 4, 2024

When editing the sample store project, if you are making a code change that introduces an error temporarily, it is possible to end up with an empty Navigator, even though metadata exists for the project, and the file is successfully parsed:

Image

Image

Image

This regression might have been introduced by #6107

@gbalint gbalint self-assigned this Sep 4, 2024
@gbalint gbalint closed this as completed in 536b9c0 Sep 6, 2024
liady pushed a commit that referenced this issue Dec 13, 2024
**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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants