Improve paint/repaint behavior (#10449)

Makes several small changes to graphics related code to improve interruption and better handle caching edge cases

packages/e2e-tests/helpers/screenshot.ts

@@ -84,6 +84,11 @@ export async function getGraphicsExecutionPoint(page: Page): Promise<string | nu
   return await element.getAttribute("data-execution-point");
 }
 
+export async function getGraphicsScreenshotType(page: Page): Promise<string | null> {
+  const element = getVideoElement(page);
+  return await element.getAttribute("data-screenshot-type");
+}
+
 export async function getGraphicsStatus(page: Page): Promise<string | null> {
   const element = getVideoElement(page);
   return await element.getAttribute("data-status");
@@ -135,3 +140,11 @@ export async function waitForGraphicsToLoad(page: Page) {
     await expect(await getGraphicsStatus(page)).toBe("loaded");
   });
 }
+
+export async function waitForRepaintGraphicsToLoad(page: Page) {
+  await debugPrint(page, `Waiting for repaint graphics to load...`, "waitForRepaintGraphicsToLoad");
+
+  await waitFor(async () => {
+    await expect(await getGraphicsScreenshotType(page)).toBe("repaint");
+  });
+}

packages/e2e-tests/tests/repaint-06.test.ts

@@ -0,0 +1,96 @@
+import { openDevToolsTab, startTest } from "../helpers";
+import { findConsoleMessage, seekToConsoleMessage } from "../helpers/console-panel";
+import { stepOver } from "../helpers/pause-information-panel";
+import { getGraphicsDataUrl, waitForRepaintGraphicsToLoad } from "../helpers/screenshot";
+import { debugPrint } from "../helpers/utils";
+import { expect, test } from "../testFixture";
+
+test.use({ exampleKey: "doc_control_flow.html" });
+
+test("repaint-06: repaints the screen screen when stepping over code that modifies the DOM", async ({
+  pageWithMeta: { page, recordingId },
+  exampleKey,
+}) => {
+  await startTest(page, recordingId);
+  await openDevToolsTab(page);
+
+  // TODO [FE-2363] Some of these points fail repaint, so they're disabled until RUN-3397 has been fixed
+  const printedStrings = [
+    // "catch",
+    // "afterCatch",
+    "finally",
+    // "yield 1",
+    "generated 1",
+    "yield 2",
+    "generated 2",
+    "after timer 1",
+    "after timer 2",
+    "after throw timer 1",
+    "after throw timer 2",
+    "within iterator",
+  ];
+
+  const textToBase64DataMap: {
+    [text: string]: string | null;
+  } = {};
+
+  {
+    await debugPrint(page, "Testing repaint graphics");
+
+    let previousGraphicsDataUrl: string | null = null;
+
+    for (let index = 0; index < printedStrings.length; index++) {
+      const string = printedStrings[index];
+      const text = `updateText("${string}")`;
+
+      const locator = await findConsoleMessage(page, text, "console-log");
+      await seekToConsoleMessage(page, locator);
+
+      // Step from "updateText(...)" to "Element text after..."
+      await stepOver(page);
+      await stepOver(page);
+      await stepOver(page);
+      await stepOver(page);
+
+      await waitForRepaintGraphicsToLoad(page);
+
+      const graphicsDataUrl = await getGraphicsDataUrl(page);
+
+      expect(
+        graphicsDataUrl,
+        `Expected graphics to have been updated to reflect the text "${string}"`
+      ).not.toEqual(previousGraphicsDataUrl);
+
+      previousGraphicsDataUrl = graphicsDataUrl;
+      textToBase64DataMap[string] = graphicsDataUrl;
+    }
+  }
+
+  {
+    await debugPrint(page, "Testing screenshot caching");
+
+    for (let index = 0; index < printedStrings.length; index++) {
+      const string = printedStrings[index];
+      const text = `updateText("${string}")`;
+
+      const locator = await findConsoleMessage(page, text, "console-log");
+      await seekToConsoleMessage(page, locator);
+
+      // Step from "updateText(...)" to "Element text after..."
+      await stepOver(page);
+      await stepOver(page);
+      await stepOver(page);
+      await stepOver(page);
+
+      await waitForRepaintGraphicsToLoad(page);
+
+      const expectedGraphicsDataUrl = textToBase64DataMap[string];
+      const actualGraphicsDataUrl = await getGraphicsDataUrl(page);
+
+      expect(
+        actualGraphicsDataUrl,
+        `Expected graphics for the text "${string}" to match previously cached graphics`
+      ).toEqual(expectedGraphicsDataUrl);
+    }
+  }
+});

packages/protocol/RepaintGraphicsCache.ts

@@ -1,6 +1,7 @@
 import { PauseId, repaintGraphicsResult } from "@replayio/protocol";
 import { Cache, createCache } from "suspense";
 
+import { paintHashCache } from "replay-next/src/suspense/ScreenshotCache";
 import { ReplayClientInterface } from "shared/client/types";
 
 export const RepaintGraphicsCache: Cache<
@@ -11,6 +12,20 @@ export const RepaintGraphicsCache: Cache<
   debugLabel: "RepaintGraphicsCache",
   getKey: ([replayClient, pauseId]) => pauseId,
   load: async ([replayClient, pauseId]) => {
-    return replayClient.repaintGraphics(pauseId);
+    const result = await replayClient.repaintGraphics(pauseId);
+
+    let { description, screenShot } = result;
+
+    // The backend won't return a screenshot for a given hash more than once; see FE-2357
+    if (screenShot) {
+      paintHashCache.cacheValue(screenShot, description.hash);
+    } else {
+      screenShot = paintHashCache.getValueIfCached(description.hash);
+    }
+
+    return {
+      description,
+      screenShot,
+    };
   },
 });

packages/replay-next/src/suspense/ScreenshotCache.ts

@@ -1,5 +1,5 @@
 import { ExecutionPoint, ScreenShot } from "@replayio/protocol";
-import { createCache } from "suspense";
+import { createCache, createExternallyManagedCache } from "suspense";
 
 import { ReplayClientInterface } from "shared/client/types";
 
@@ -10,5 +10,18 @@ export const screenshotCache = createCache<
   config: { immutable: true },
   debugLabel: "ScreenshotCache",
   getKey: ([client, point, paintHash]) => paintHash,
-  load: ([client, point]) => client.getScreenshot(point),
+  load: async ([client, point]) => {
+    const screenShot = await client.getScreenshot(point);
+
+    paintHashCache.cacheValue(screenShot, screenShot.hash);
+
+    return screenShot;
+  },
+});
+
+// The backend won't return a screenshot for a given hash more than once; see FE-2357
+export const paintHashCache = createExternallyManagedCache<[paintHash: string], ScreenShot>({
+  config: { immutable: true },
+  debugLabel: "paintHashCache",
+  getKey: ([paintHash]) => paintHash,
 });

src/ui/components/Video/Video.tsx

@@ -71,6 +71,7 @@ export default function Video() {
 
       // The data attributes are sed for e2e tests
       containerElement.setAttribute("data-execution-point", nextState.currentExecutionPoint ?? "");
+      containerElement.setAttribute("data-screenshot-type", "" + nextState.screenShotType);
       containerElement.setAttribute("data-status", "" + nextState.status);
       containerElement.setAttribute("data-time", "" + nextState.currentTime);
       graphicsElement.setAttribute("data-scale", nextState.localScale.toString());

src/ui/components/Video/imperative/MutableGraphicsState.tsx

@@ -25,6 +25,7 @@ interface Rect {
 }
 
 export type Status = "failed" | "loaded" | "loading";
+export type ScreenShotType = "cached-paint" | "repaint";
 
 export interface State {
   currentExecutionPoint: ExecutionPoint | null;
@@ -33,6 +34,7 @@ export interface State {
   localScale: number;
   recordingScale: number;
   screenShot: ScreenShot | undefined;
+  screenShotType: ScreenShotType | undefined;
   status: Status;
 }
 
@@ -48,15 +50,19 @@ export const state = createState<State>({
   localScale: 1,
   recordingScale: 1,
   screenShot: undefined,
+  screenShotType: undefined,
   status: "loading",
 });
 
+let lock: Object | null = null;
+
 export async function updateState(
   containerElement: HTMLElement,
   options: Partial<{
     didResize?: boolean;
     executionPoint: ExecutionPoint | null;
     screenShot: ScreenShot | null;
+    screenShotType: ScreenShotType | null;
     status: Status;
     time: number;
   }> = {}
@@ -67,25 +73,42 @@ export async function updateState(
     didResize,
     executionPoint = prevState.currentExecutionPoint,
     screenShot = prevState.screenShot,
+    screenShotType = prevState.screenShotType,
     status = prevState.status,
     time = prevState.currentTime,
   } = options;
 
+  if (shallowEqual(options, { didResize })) {
+    if (lock !== null) {
+      // Don't let an event from the ResizeObserver interrupt an in-progress update;
+      // this should override new SnapShot data
+      return;
+    }
+  }
+
+  const localLock = new Object();
+  lock = localLock;
+
   let graphicsRect = prevState.graphicsRect;
   let localScale = prevState.localScale;
   let recordingScale = prevState.recordingScale;
   if (screenShot && (screenShot != prevState.screenShot || didResize)) {
     const naturalDimensions = await getDimensions(screenShot.data, screenShot.mimeType);
+    if (lock !== localLock) {
+      return;
+    }
+
     const naturalHeight = naturalDimensions.height;
     const naturalWidth = naturalDimensions.width;
 
     const containerRect = containerElement.getBoundingClientRect();
-    const scaledDimensions = await fitImageToContainer({
+    const scaledDimensions = fitImageToContainer({
       containerHeight: containerRect.height,
       containerWidth: containerRect.width,
       imageHeight: naturalHeight,
       imageWidth: naturalWidth,
     });
+
     const clientHeight = scaledDimensions.height;
     const clientWidth = scaledDimensions.width;
 
@@ -107,10 +130,13 @@ export async function updateState(
     localScale,
     recordingScale,
     screenShot: screenShot || undefined,
+    screenShotType: screenShotType || undefined,
     status,
   };
 
   if (!shallowEqual(prevState, nextState)) {
     state.update(nextState);
   }
+
+  lock = null;
 }

src/ui/components/Video/imperative/subscribeToMutableSources.ts

@@ -86,11 +86,9 @@ export function subscribeToMutableSources({
         });
       }
     } else {
-      if (didStopPlaying) {
-        if (abortController != null) {
-          abortController.abort();
-          abortController = null;
-        }
+      if (abortController != null) {
+        abortController.abort();
+        abortController = null;
       }
 
       abortController = new AbortController();