From ce518e4a2b7f5fbe8d335515066193fb4f27a738 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 18 Mar 2024 14:32:43 -0400 Subject: [PATCH] Improved paint/repaint behavior --- packages/e2e-tests/helpers/screenshot.ts | 13 ++ packages/e2e-tests/tests/repaint-06.test.ts | 126 ++++++++++++++++++ packages/protocol/RepaintGraphicsCache.ts | 17 ++- .../src/suspense/ScreenshotCache.ts | 17 ++- src/ui/components/Video/Video.tsx | 1 + .../Video/imperative/MutableGraphicsState.tsx | 19 +++ .../imperative/subscribeToMutableSources.ts | 8 +- .../Video/imperative/updateGraphics.ts | 65 +++++++-- 8 files changed, 244 insertions(+), 22 deletions(-) create mode 100644 packages/e2e-tests/tests/repaint-06.test.ts diff --git a/packages/e2e-tests/helpers/screenshot.ts b/packages/e2e-tests/helpers/screenshot.ts index d00c70f871f..5261a4c7119 100644 --- a/packages/e2e-tests/helpers/screenshot.ts +++ b/packages/e2e-tests/helpers/screenshot.ts @@ -84,6 +84,11 @@ export async function getGraphicsExecutionPoint(page: Page): Promise { + const element = getVideoElement(page); + return await element.getAttribute("data-screenshot-type"); +} + export async function getGraphicsStatus(page: Page): Promise { 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"); + }); +} diff --git a/packages/e2e-tests/tests/repaint-06.test.ts b/packages/e2e-tests/tests/repaint-06.test.ts new file mode 100644 index 00000000000..9b93147ec76 --- /dev/null +++ b/packages/e2e-tests/tests/repaint-06.test.ts @@ -0,0 +1,126 @@ +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.skip("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); + + 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 paints: { + [key: string]: { + before: string | null; + after: string | null; + }; + } = {}; + + { + await debugPrint(page, "Testing repaint graphics"); + + let graphicsDataUrl: string | null = null; + 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 to "Element text before..." + await stepOver(page); + await stepOver(page); + + await waitForRepaintGraphicsToLoad(page); + + graphicsDataUrl = await getGraphicsDataUrl(page); + expect( + graphicsDataUrl, + `Expected graphics before "${string}" not to match previous after graphics` + ).not.toEqual(previousGraphicsDataUrl); + previousGraphicsDataUrl = graphicsDataUrl; + + const cachedPaints = { + before: graphicsDataUrl, + after: "" as string | null, + }; + + // Step to "Element text after..." + await stepOver(page); + await stepOver(page); + + await waitForRepaintGraphicsToLoad(page); + + graphicsDataUrl = await getGraphicsDataUrl(page); + expect( + graphicsDataUrl, + `Expected graphics after "${string}" not to match the graphics before` + ).not.toEqual(previousGraphicsDataUrl); + previousGraphicsDataUrl = graphicsDataUrl; + + cachedPaints.after = graphicsDataUrl; + + paints[string] = cachedPaints; + } + } + + { + await debugPrint(page, "Testing screenshot caching"); + + let graphicsDataUrl: 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 to "Element text before..." + await stepOver(page); + await stepOver(page); + + await waitForRepaintGraphicsToLoad(page); + + graphicsDataUrl = await getGraphicsDataUrl(page); + expect( + graphicsDataUrl, + `Expected graphics before "${string}" to match previous cached graphics` + ).toEqual(paints[string]?.before); + + // Step to "Element text after..." + await stepOver(page); + await stepOver(page); + + await waitForRepaintGraphicsToLoad(page); + + graphicsDataUrl = await getGraphicsDataUrl(page); + expect( + graphicsDataUrl, + `Expected graphics after "${string}" to match previous cached graphics` + ).toEqual(paints[string]?.after); + } + } +}); diff --git a/packages/protocol/RepaintGraphicsCache.ts b/packages/protocol/RepaintGraphicsCache.ts index 57e9d6ec681..ec60a3b42d5 100644 --- a/packages/protocol/RepaintGraphicsCache.ts +++ b/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, + }; }, }); diff --git a/packages/replay-next/src/suspense/ScreenshotCache.ts b/packages/replay-next/src/suspense/ScreenshotCache.ts index b73ecc709c9..367959c3efd 100644 --- a/packages/replay-next/src/suspense/ScreenshotCache.ts +++ b/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, }); diff --git a/src/ui/components/Video/Video.tsx b/src/ui/components/Video/Video.tsx index 6ad0f28efd5..107678a1946 100644 --- a/src/ui/components/Video/Video.tsx +++ b/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()); diff --git a/src/ui/components/Video/imperative/MutableGraphicsState.tsx b/src/ui/components/Video/imperative/MutableGraphicsState.tsx index 2cf1c839811..0aa38a285a3 100644 --- a/src/ui/components/Video/imperative/MutableGraphicsState.tsx +++ b/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,25 +50,33 @@ export const state = createState({ localScale: 1, recordingScale: 1, screenShot: undefined, + screenShotType: undefined, status: "loading", }); +let lock = new Object(); + export async function updateState( containerElement: HTMLElement, options: Partial<{ didResize?: boolean; executionPoint: ExecutionPoint | null; screenShot: ScreenShot | null; + screenShotType: ScreenShotType | null; status: Status; time: number; }> = {} ) { + const localLock = new Object(); + lock = localLock; + const prevState = state.read(); let { didResize, executionPoint = prevState.currentExecutionPoint, screenShot = prevState.screenShot, + screenShotType = prevState.screenShotType, status = prevState.status, time = prevState.currentTime, } = options; @@ -76,6 +86,10 @@ export async function updateState( 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; @@ -86,6 +100,10 @@ export async function updateState( imageHeight: naturalHeight, imageWidth: naturalWidth, }); + if (lock !== localLock) { + return; + } + const clientHeight = scaledDimensions.height; const clientWidth = scaledDimensions.width; @@ -107,6 +125,7 @@ export async function updateState( localScale, recordingScale, screenShot: screenShot || undefined, + screenShotType: screenShotType || undefined, status, }; diff --git a/src/ui/components/Video/imperative/subscribeToMutableSources.ts b/src/ui/components/Video/imperative/subscribeToMutableSources.ts index bc712f4bbe2..e390fd6fc2c 100644 --- a/src/ui/components/Video/imperative/subscribeToMutableSources.ts +++ b/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(); diff --git a/src/ui/components/Video/imperative/updateGraphics.ts b/src/ui/components/Video/imperative/updateGraphics.ts index e7ec0e4f0d9..8193261ba73 100644 --- a/src/ui/components/Video/imperative/updateGraphics.ts +++ b/src/ui/components/Video/imperative/updateGraphics.ts @@ -31,27 +31,30 @@ export async function updateGraphics({ return; } + const promises: Promise[] = []; + // If the current time is before the first paint, we should show nothing const paintPoint = findMostRecentPaint(time); - if (!paintPoint || !paintPoint.paintHash) { + const isBeforeFirstCachedPaint = !paintPoint || !paintPoint.paintHash; + if (isBeforeFirstCachedPaint) { updateState(containerElement, { executionPoint, screenShot: null, - status: "loaded", + screenShotType: null, + status: executionPoint ? "loading" : "loaded", time, }); - return; + } else { + promises.push( + fetchPaintContents({ + abortSignal, + containerElement, + replayClient, + time, + }) + ); } - const promises: Promise[] = [ - fetchPaintContents({ - abortSignal, - containerElement, - replayClient, - time, - }), - ]; - let repaintGraphicsScreenShot: ScreenShot | undefined = undefined; if (executionPoint) { const promise = fetchRepaintGraphics({ @@ -67,6 +70,12 @@ export async function updateGraphics({ promises.push(promise); } + if (promises.length === 0) { + // If we are before the first paint and have no execution point to request a repaint, + // then we should clear out the currently visible graphics and bail out + return; + } + // Fetch paint contents and repaint graphics in parallel const screenShot = await Promise.race(promises); if (abortSignal.aborted) { @@ -78,6 +87,7 @@ export async function updateGraphics({ updateState(containerElement, { executionPoint, screenShot, + screenShotType: repaintGraphicsScreenShot != null ? "repaint" : "cached-paint", status: "loaded", time, }); @@ -101,11 +111,24 @@ export async function updateGraphics({ updateState(containerElement, { executionPoint, screenShot: repaintGraphicsScreenShot, + screenShotType: "repaint", status: "loaded", time, }); + + return; } } + + if (screenShot == null) { + // If we couldn't load either a cached screenshot or a repaint, update the DOM to reflect that + updateState(containerElement, { + executionPoint, + screenShotType: null, + status: isBeforeFirstCachedPaint ? "loaded" : "failed", + time, + }); + } } async function fetchPaintContents({ @@ -149,11 +172,25 @@ async function fetchRepaintGraphics({ }): Promise { const pauseId = await pauseIdCache.readAsync(replayClient, executionPoint, time); + // Until repaints are more reliable, only wait a few seconds before giving up + // this prevents the UI from getting stuck in a visible loading state + const timeoutPromise = new Promise(resolve => { + setTimeout(() => { + resolve(); + }, 5_000); + }); + + let screenShot: ScreenShot | undefined = undefined; + try { - const result = await RepaintGraphicsCache.readAsync(replayClient, pauseId); + const repaintPromise = RepaintGraphicsCache.readAsync(replayClient, pauseId); - return result?.screenShot; + await Promise.race([repaintPromise, timeoutPromise]).then(result => { + screenShot = result?.screenShot; + }); } catch (error) { // Repaint graphics are currently expected to fail } + + return screenShot; }