Fix navigation through DOM views with pages

[apedroferreira]

Closes #1551

• Loads first page in the URL, even if it's not the first page or a page.
• Does not get rid of query params in URL on first page load.
• Always use DOM view state as the source of truth for the current view. Load initial view from URL. Update URL as a side-effect.

Preview: https://toolpad-pr-1565.onrender.com/

[apedroferreira]

Should I move this function to some other file? Not sure where to move it to.

[oliviertassinari]

Do we know what created this regression?

[Janpot]

I'm not sure about this, it feels a bit brittle. How should the undoable views interact with the browser back button?

I don't believe it would currently be possible to keep the browser history in sync with the undo stack. I feel like as soon as we're in the editor flow, the undoable view changes shouldn't create new browser history entries. And if that is the case then:

1. Should we be not using react-router for the undoable views, instead have the route defined by the editor state? (i.e. not use <Route>, but switch routes based on editor state)
2. Initialize the state using the current url
3. in an effect synchronize all state changes back to the url

Do we know what created this regression?

@oliviertassinari This was introduced when taking the view switching and selection into account for undo/redo. I think the initial implementation gets into a chicken/egg situation between application state (undo stack) and the browser history. I believe it would be best to make one the single source of truth (application state) and have the other simply follow suit. For that we'll have to break out of react-router for this part.

[apedroferreira]

I tried to make the source of truth be the DOM state, but yeah I did not anticipate this issue coming from the URLs and I tried out this solution but it's not the simplest...
Picking a single source of truth and not letting the other influence anything sounds like a way to do it - so either always use URLs (could be tough) or always using the DOM state without relying on URLs.
I can probably simplify this instead of trying to make it work so universally - will give it a try.

[apedroferreira]

I'm not sure about this, it feels a bit brittle. How should the undoable views interact with the browser back button?

I don't believe it would currently be possible to keep the browser history in sync with the undo stack. I feel like as soon as we're in the editor flow, the undoable view changes shouldn't create new browser history entries. And if that is the case then:

1. Should we be not using react-router for the undoable views, instead have the route defined by the editor state? (i.e. not use <Route>, but switch routes based on editor state)
2. Initialize the state using the current url
3. in an effect synchronize all state changes back to the url

Do we know what created this regression?

@oliviertassinari This was introduced when taking the view switching and selection into account for undo/redo. I think the initial implementation gets into a chicken/egg situation between application state (undo stack) and the browser history. I believe it would be best to make one the single source of truth (application state) and have the other simply follow suit. For that we'll have to break out of react-router for this part.

@Janpot how about this? I followed the approach you suggested, seems like a good solution!

[Janpot]

I believe this could be a bit DRYer if written as:

function getPathnameForView (appId, view) {
  switch (view.kind) {
    case 'page': return `/app/${appId}/pages/${view.nodeId}`;
    // ...
  }
}

React.useEffect(() => {
  const desiredPath = getPathnameForView(currentView)
  if (desiredPath !== location.pathname) {
    navigate({  pathname: desiredPath }, { replace: true });
  }
}, [appId, currentView, location.pathname]

[Janpot]

The intent of this isn't very clear from structuring the code this way. (The intent being "only one of these needs to be rendered"). Moreover, the last element needs to always be kept in sync with the lines above. Ideally this would be written as a switch. Infortunately there's still no way to write switch expressions or do pattern matching in javascript, but perhaps this could be written as:

{(() => {
  switch (currentView.kind) {
    case 'page': return <PageEditor appId={appId} nodeId={currentView.nodeId} />;
    // ...
  }
})()}

It's a bit ugly with all those brackets. Alternatively this could be written in a useMemo, or extracted as a function.

[Janpot]

This looks like the inverse of the getPathnameFromView. Perhaps we could put the two functions physically together and name them similarly?

getPathnameFromView and getViewFromPathname?

[Janpot]

how about this? I followed the approach you suggested, seems like a good solution!

I like this implementation a lot, hopefully it works well in practice. Just a few stylistic comments

[apedroferreira]

I like this implementation a lot, hopefully it works well in practice. Just a few stylistic comments

Great suggestions again, there was definitely some unneeded repetition.
I've added the suggestions now.

[Janpot]

Should there be any view for which no pathame exists? Perhaps this should throw?

Suggested change

	return null;
	throw new Error(`Unknown view "${(view as DomView).kind}".`)

[apedroferreira]

Sounds good! I've added it in a new commit to include small type adjustments since the result can't be null

[Janpot]

👌 Very nice!

[oliviertassinari]

Great, the DX is much better with this fix. Also the e2e test was much needed, great to see it 👌.