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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Nov 13, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f7f250f
fix: Re-running ui code initially rendering the old document
mattrunyon Nov 13, 2024
70bb5a8
Remove unintentional code
mattrunyon Nov 13, 2024
4657e6a
Address review comments
mattrunyon Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions plugins/ui/src/js/src/DashboardPlugin.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,7 @@ export function DashboardPlugin(
>(new Map());

const handleWidgetOpen = useCallback(
({
widgetId = nanoid(),
widget,
}: {
widgetId: string;
widget: WidgetDescriptor;
}) => {
({ widgetId, widget }: { widgetId: string; widget: WidgetDescriptor }) => {
log.debug('Opening widget with ID', widgetId, widget);
setWidgetMap(prevWidgetMap => {
const newWidgetMap = new Map(prevWidgetMap);
Expand Down
23 changes: 17 additions & 6 deletions plugins/ui/src/js/src/layout/ReactPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,13 @@ import {
useLayoutManager,
useListener,
} from '@deephaven/dashboard';
import { View, ViewProps, Flex, FlexProps } from '@deephaven/components';
import {
View,
ViewProps,
Flex,
FlexProps,
LoadingOverlay,
} from '@deephaven/components';
import Log from '@deephaven/log';
import PortalPanel from './PortalPanel';
import { ReactPanelControl, useReactPanel } from './ReactPanelManager';
Expand Down Expand Up @@ -186,6 +192,15 @@ function ReactPanel({
);
const widgetStatus = useWidgetStatus();

let renderedChildren: React.ReactNode;
if (widgetStatus.status === 'loading') {
renderedChildren = <LoadingOverlay />;
} else if (widgetStatus.status === 'error') {
renderedChildren = <WidgetErrorView error={widgetStatus.error} />;
} else {
renderedChildren = children;
}

return portal
? ReactDOM.createPortal(
<ReactPanelContext.Provider value={panelId}>
Expand Down Expand Up @@ -224,11 +239,7 @@ function ReactPanel({
* Don't render the children if there's an error with the widget. If there's an error with the widget, we can assume the children won't render properly,
* but we still want the panels to appear so things don't disappear/jump around.
*/}
{widgetStatus.status === 'error' ? (
<WidgetErrorView error={widgetStatus.error} />
) : (
children
)}
{renderedChildren}
</ReactPanelErrorBoundary>
</Flex>
</View>
Expand Down
14 changes: 13 additions & 1 deletion plugins/ui/src/js/src/widget/WidgetHandler.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,14 @@ function WidgetHandler({
initialData: initialDataProp,
}: WidgetHandlerProps): JSX.Element | null {
const { widget, error: widgetError } = useWidget(widgetDescriptor);
const [isLoading, setIsLoading] = useState(true);
const [prevWidget, setPrevWidget] = useState(widget);
// Cannot use usePrevious to change setIsLoading
// Since usePrevious runs in an effect, the value never gets updated if setIsLoading is called during render
if (widget !== prevWidget) {
setPrevWidget(widget);
setIsLoading(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just call this in initializeWidget?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

const [document, setDocument] = useState<ReactNode>();

Expand Down Expand Up @@ -226,6 +234,7 @@ function WidgetHandler({
const newDocument = parseDocument(documentParam);
setInternalError(undefined);
setDocument(newDocument);
setIsLoading(false); // Must go after setDocument since setters are not batched in effects
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated to use unstable_batchedUpdates

if (stateParam != null) {
try {
const newState = JSON.parse(stateParam);
Expand Down Expand Up @@ -339,11 +348,14 @@ function WidgetHandler({
if (error != null) {
return { status: 'error', descriptor: widgetDescriptor, error };
}
if (isLoading) {
return { status: 'loading', descriptor: widgetDescriptor };
}
if (renderedDocument != null) {
return { status: 'ready', descriptor: widgetDescriptor };
}
return { status: 'loading', descriptor: widgetDescriptor };
}, [error, renderedDocument, widgetDescriptor]);
}, [error, renderedDocument, widgetDescriptor, isLoading]);

return useMemo(
() =>
Expand Down
Loading