fix: Re-running ui code initially rendering the old document

[mattrunyon]

This fixes a bug where re-running code causes the old document to be rendered first. This can cause initial mounting with old props for document trees that don't change between runs which can cause problems with components that rely on an initial prop value only.

This also fixes an issue where the old panels are shown until new panels/document is fetched. Now, it will show loading spinners on those panels instead. If the new document (re-assigned to the same variable in Python) has a new tree structure, then the old panels will be closed and new panels opened. If the document has the same tree, then the new panels will replace the old panels.

This Python example shows a slow loading component. Run the code, then change the props (I commented out the format_ prop) and re-run. The existing panel should turn into a loading spinner and then load without the formatting rules.

from deephaven import ui
import deephaven.plot.express as dx
import time

@ui.component
def my_t():
    time.sleep(2)
    return ui.table(
        dx.data.stocks().update("SymColor=Sym==`FISH` ? `positive` : `salmon`"),
        hidden_columns=["SymColor"],
        format_=[
            ui.TableFormat(value="0.00%"),
            ui.TableFormat(cols="Timestamp", value="E, dd MMM yyyy HH:mm:ss z"),
            ui.TableFormat(cols="Size", color="info", if_="Size < 10"),
            ui.TableFormat(cols="Size", color="notice", if_="Size > 100"),
            ui.TableFormat(cols=["Sym", "Exchange"], alignment="center"),
            ui.TableFormat(
                cols=["Sym", "Exchange"], background_color="negative", if_="Sym=`CAT`"
            ),
            ui.TableFormat(if_="Sym=`DOG`", color="oklab(0.6 -0.3 -0.25)"),
            ui.TableFormat(cols="Sym", color="SymColor"),
        ],
    )

t = my_t()

[mofojed]

Works fine, take a look at my comments.

One thing that's still a downside - we don't show a "loading" panel right away. Ideally we'd show that something is still loading. That was a problem prior to this, and I had a branch that fixed this, but broke dashboards (since the dashboard would open, it would open a panel to show "loading" but then it would try and load the tree and throw because there was already a root in the dashboard or something like that).

Anyway, take a look at my comments, then I'll approve if you think this is good.

[mofojed]

Being pedantic - this comment isn't true. In a useEffect, the setStates would be batched. It's callbacks that are called from outside the event handler that are not (setTimeout, listeners for other events, etc). In React v18 they are automatically batched still no matter what.
We could use unstable_batchedUpdates to batch these updates together and then order wouldn't matter: https://blog.saeloun.com/2021/07/22/react-automatic-batching/
It might actually make sense in this case, as we don't really want to render in a half-rendered state (though it should work in this particular order as well).

[mattrunyon]

Oh ya, this is actually a callback that causes it, not the effect. So I don't know if this would be addressed by React 18

That's interesting and I hadn't seen that API before. Would probably be useful here since there are probably 3 render passes here instead of 1.

[mattrunyon]

I updated to use unstable_batchedUpdates

[mofojed]

Can't we just call this in initializeWidget?

[mattrunyon]

Maybe. Not sure how if it being in an effect would cause issues. It will cause an unnecessary render cycle probably

https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes

[mattrunyon]

I changed this to use widgetDescriptor instead. useWidget async gets the widget from the descriptor, so there's potential latency there (I saw some flashing of the old table before the loader).

Putting this in a useEffect means we get an extra render of the old table. I put some logging in and with this in anything that relies on widget instead of widgetDescriptor, the table unmounts, remounts (old), quickly unmounts (< 100ms), shows loader, then mounts new

[mattrunyon]

If we wanted to show a loading panel immediately, we could modify the document to open a panel or show a toast when loading (if there's no document). We would maybe want to open a loading panel after some short delay so there's no flashing for quick running commands

[mofojed]

If we wanted to show a loading panel immediately, we could modify the document to open a panel or show a toast when loading (if there's no document). We would maybe want to open a loading panel after some short delay so there's no flashing for quick running commands

Showing the loading panel immediately would be fine in that once it loads, it would just fill that panel. However it gets dicey when you have components that have multiple panels.

[mattrunyon]

If we wanted to show a loading panel immediately, we could modify the document to open a panel or show a toast when loading (if there's no document). We would maybe want to open a loading panel after some short delay so there's no flashing for quick running commands

Showing the loading panel immediately would be fine in that once it loads, it would just fill that panel. However it gets dicey when you have components that have multiple panels.

Fair point. I can do this as a separate PR. I think it's fine if there's multiple panels because we have no way of knowing that, so showing 1 w/ an actual loading message instead of just a spinner will work fine I think. And then in the DocumentHandler we should be able to re-use the panelId and replace the loading panel w/ the 1st actual panel.

[mofojed]

Fair point. I can do this as a separate PR. I think it's fine if there's multiple panels because we have no way of knowing that, so showing 1 w/ an actual loading message instead of just a spinner will work fine I think. And then in the DocumentHandler we should be able to re-use the panelId and replace the loading panel w/ the 1st actual panel.

@mattrunyon This is easier said than done with the current setup... because the document structure is different going from "loading" to when the document is loaded (even with one panel, but especially with more than one panel), it means the ReactPanel unmounts, causing it to close a panel, then the new ReactPanel mounts, opening a new panel in a possibly new location.

I had a branch where I was playing around with this at one point. I think fix-panel-state-management changes to WidgetHandler. In that branch I'm ensuring a panel is opened immediately. I'm pretty sure the snag I hit was just with opening dashboards though, and didn't have time to resolve that.