Skip to content

Commit 668a82c

Browse files
authored
Fix race condition with HTML plots loading in auxiliary windows (#10586)
Addresses #10512 again, following up to #10562 I was going to do this next week, but I think I found where this was going wrong! Like @jmcphers hypothesized, we had a race condition where the webview layout was sometimes calculated before the DOM element had its final dimensions. The layout `useEffect` ran on every render without proper dependencies, calling `layoutWebviewOverElement()` before the element was fully laid out in the DOM. This was especially problematic in auxiliary windows where the rendering pipeline can be slightly delayed or out of sync. I replaced the eager layout call with a `ResizeObserver` that waits for the element to have non-zero dimensions before calculating position. Hopefully this means the webview is always positioned correctly on first render. 🤞 I was only seeing this in about 1 out of 20 loads of the auxiliary window, so it will definitely be good to run this a BUNCH of times to see if you ever see the bad behavior now. @:plots ### Release Notes <!-- Optionally, replace `N/A` with text to be included in the next release notes. The `N/A` bullets are ignored. If you refer to one or more Positron issues, these issues are used to collect information about the feature or bugfix, such as the relevant language pack as determined by Github labels of type `lang: `. The note will automatically be tagged with the language. These notes are typically filled by the Positron team. If you are an external contributor, you may ignore this section. --> #### New Features - N/A #### Bug Fixes - N/A ### QA Notes I'm going to copy from #10562: ----- Some R plots: ```r # Interactive ------------------------------------------------------------ library(highcharter) hchart(mtcars, "scatter", hcaes(wt, mpg, z = drat, color = hp)) |> hc_title(text = "Scatter chart with size and color") # Static ----------------------------------------------------------------- plot(faithful) # Static ----------------------------------------------------------------- plot(sample(1:100, 20)) # Interactive ------------------------------------------------------------ library(ggplot2) library(plotly) p <- ggplot(data = diamonds, aes(x = cut, fill = clarity)) + geom_bar(position = "dodge") ggplotly(p) ``` Some Python plots: ```python import numpy as np import pandas as pd df = pd.DataFrame(np.random.randint(1, 7, 6000), columns=["one"]) df["two"] = df["one"] + np.random.randint(1, 7, 6000) # make one plot df.plot.hist(bins=12, alpha=0.5) # make a new plot with fewer bins df.plot.hist(bins=8, alpha=0.5) ``` - You should be able to create some of these plots (perhaps intermixed R and Python) and then pop out the new auxiliary window, with the interactive/HTML plots correctly showing up in the new auxiliary window. - You should also be able to make new interactive and static plots and have them show up in that window, if it is open. - You should be able to close the auxiliary window and have everything come back to the main window plots pane.
1 parent 64b2e26 commit 668a82c

File tree

1 file changed

+21
-7
lines changed

1 file changed

+21
-7
lines changed

src/vs/workbench/contrib/positronPlots/browser/components/webviewPlotInstance.tsx

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
*--------------------------------------------------------------------------------------------*/
55

66
// React.
7-
import React, { useEffect } from 'react';
7+
import React, { useEffect, useLayoutEffect } from 'react';
88

99
// Other dependencies.
1010
import { WebviewPlotClient } from '../webviewPlotClient.js';
@@ -48,21 +48,35 @@ export const WebviewPlotInstance = (props: WebviewPlotInstanceProps) => {
4848
};
4949
}, [props.plotClient, props.visible]);
5050

51-
useEffect(() => {
51+
useLayoutEffect(() => {
5252
// If the client is not claimed, do nothing.
5353
// This is to avoid activating the client when it isn't claimed, which could happen
5454
// if the previous effect is cleaned up before this one runs.
55-
if (!clientIsClaimed) {
55+
if (!clientIsClaimed || !webviewRef.current) {
5656
return;
5757
}
5858

5959
const client = props.plotClient;
60-
client.activate().then(() => {
61-
if (webviewRef.current) {
62-
client.layoutWebviewOverElement(webviewRef.current);
60+
const element = webviewRef.current;
61+
62+
// Use ResizeObserver to detect when the element gets dimensions.
63+
// This is especially important in auxiliary windows where the element
64+
// might not have its final dimensions immediately after mounting.
65+
const resizeObserver = new ResizeObserver((entries) => {
66+
for (const entry of entries) {
67+
// Only layout if the element has non-zero dimensions
68+
if (entry.contentRect.width > 0 && entry.contentRect.height > 0) {
69+
client.layoutWebviewOverElement(element);
70+
}
6371
}
6472
});
65-
});
73+
74+
resizeObserver.observe(element);
75+
76+
return () => {
77+
resizeObserver.disconnect();
78+
};
79+
}, [clientIsClaimed, props.plotClient]);
6680

6781
const style = {
6882
width: `${props.width}px`,

0 commit comments

Comments
 (0)