From fef2b430729a8dfa9a65a75d7158a5a3f1db3dde Mon Sep 17 00:00:00 2001 From: sua yoo Date: Tue, 30 Apr 2024 18:16:12 -0700 Subject: [PATCH] Fix QA tab loading state (#1772) - Prevents misleading "no text" message shown when switching tabs - Reset data and show loading indicator when switching pages - Fix replay tab height and visually differentiates empty replay window from empty tab - Refactors console.logs to debugs --- .../org/archived-item-qa/archived-item-qa.ts | 41 ++++++++++++------- .../pages/org/archived-item-qa/ui/replay.ts | 4 +- .../org/archived-item-qa/ui/resources.ts | 18 ++++---- .../org/archived-item-qa/ui/screenshots.ts | 32 +++++++++++---- .../src/pages/org/archived-item-qa/ui/text.ts | 2 +- 5 files changed, 61 insertions(+), 36 deletions(-) diff --git a/frontend/src/pages/org/archived-item-qa/archived-item-qa.ts b/frontend/src/pages/org/archived-item-qa/archived-item-qa.ts index 5180788f..3d7fd178 100644 --- a/frontend/src/pages/org/archived-item-qa/archived-item-qa.ts +++ b/frontend/src/pages/org/archived-item-qa/archived-item-qa.ts @@ -176,7 +176,7 @@ export class ArchivedItemQA extends TailwindElement { // Receive messages from replay-web-page windows void this.replaySwReg.then((reg) => { if (!reg) { - console.log("[debug] no reg, listening to messages"); + console.debug("no reg, listening to messages"); // window.addEventListener("message", this.onWindowMessage); } }); @@ -207,17 +207,17 @@ export class ArchivedItemQA extends TailwindElement { * if the sw is not present. */ private async handleRwpMessage(sourceLoc: string) { - console.log("[debug] handleRwpMessage", sourceLoc); + console.debug("handleRwpMessage", sourceLoc); // check if has /qa/ in path, then QA if (sourceLoc.indexOf("%2Fqa%2F") >= 0 && !this.qaDataRegistered) { this.qaDataRegistered = true; - console.log("[debug] onWindowMessage qa", this.qaData); + console.debug("onWindowMessage qa", this.qaData); await this.fetchContentForTab({ qa: true }); await this.updateComplete; // otherwise main crawl replay } else if (!this.crawlDataRegistered) { this.crawlDataRegistered = true; - console.log("[debug] onWindowMessage crawl", this.crawlData); + console.debug("onWindowMessage crawl", this.crawlData); await this.fetchContentForTab(); await this.updateComplete; } @@ -248,15 +248,23 @@ export class ArchivedItemQA extends TailwindElement { // Re-fetch when tab, archived item page, or QA run ID changes // from an existing one, probably due to user interaction if (changedProperties.get("tab") || changedProperties.get("page")) { - if (this.tab === "screenshots") { + if (changedProperties.get("page")) { if (this.crawlData?.blobUrl) URL.revokeObjectURL(this.crawlData.blobUrl); if (this.qaData?.blobUrl) URL.revokeObjectURL(this.qaData.blobUrl); + + // FIXME Set to null to render loading state, should be refactored + // to handle loading state separately in https://github.com/webrecorder/browsertrix/issues/1716 + this.crawlData = null; + this.qaData = null; } // TODO prefetch content for other tabs? void this.fetchContentForTab(); void this.fetchContentForTab({ qa: true }); } else if (changedProperties.get("qaRunId")) { + // FIXME Set to null to render loading state, should be refactored + // to handle loading state separately in https://github.com/webrecorder/browsertrix/issues/1716 + this.qaData = null; void this.fetchContentForTab({ qa: true }); } } @@ -867,7 +875,7 @@ export class ArchivedItemQA extends TailwindElement { const replaySource = `/api/orgs/${this.orgId}/crawls/${this.itemId}${qa ? `/qa/${rwpId}` : ""}/replay.json`; const headers = this.authState?.headers; const config = JSON.stringify({ headers }); - console.log("[debug] rendering rwp", rwpId); + console.debug("rendering rwp", rwpId); return guard( [rwpId, this.page, this.authState], () => html` @@ -1107,8 +1115,8 @@ export class ArchivedItemQA extends TailwindElement { const frameWindow = this.replayFrame?.contentWindow; if (!page || !sourceId || !frameWindow) { - console.log( - "[debug] no page replaId or frameWindow", + console.debug( + "no page replaId or frameWindow", page, sourceId, frameWindow, @@ -1129,9 +1137,8 @@ export class ArchivedItemQA extends TailwindElement { const url = `/replay/w/${sourceId}/${urlPart}`; // TODO check status code - if (tab === "replay") { - return { replayUrl: url }; - } + console.log("replay?"); + const resp = await frameWindow.fetch(url); //console.log("resp:", resp); @@ -1140,14 +1147,19 @@ export class ArchivedItemQA extends TailwindElement { throw resp.status; } + if (tab === "replay") { + return { replayUrl: url }; + } if (tab === "screenshots") { const blob = await resp.blob(); const blobUrl = URL.createObjectURL(blob) || ""; return { blobUrl }; - } else if (tab === "text") { + } + if (tab === "text") { const text = await resp.text(); return { text }; - } else { + } + { // tab === "resources" const json = (await resp.json()) as { @@ -1197,7 +1209,6 @@ export class ArchivedItemQA extends TailwindElement { return { resources: Object.fromEntries(typeMap.entries()) }; } - return { text: "" }; }; try { @@ -1217,7 +1228,7 @@ export class ArchivedItemQA extends TailwindElement { this.crawlDataRegistered = true; } } catch (e: unknown) { - console.log("[debug] error:", e); + console.debug("error:", e); // check if this endpoint is registered, if not, ensure re-render if (e === 404) { diff --git a/frontend/src/pages/org/archived-item-qa/ui/replay.ts b/frontend/src/pages/org/archived-item-qa/ui/replay.ts index 0b01b39b..d3343a0f 100644 --- a/frontend/src/pages/org/archived-item-qa/ui/replay.ts +++ b/frontend/src/pages/org/archived-item-qa/ui/replay.ts @@ -11,7 +11,7 @@ import { tw } from "@/utils/tailwind"; export function renderReplay(crawlData: ReplayData) { return html`
${guard([crawlData], () => when( @@ -19,7 +19,7 @@ export function renderReplay(crawlData: ReplayData) { (replayUrl) => html``, renderSpinner, ), diff --git a/frontend/src/pages/org/archived-item-qa/ui/resources.ts b/frontend/src/pages/org/archived-item-qa/ui/resources.ts index df6377e9..0c35e105 100644 --- a/frontend/src/pages/org/archived-item-qa/ui/resources.ts +++ b/frontend/src/pages/org/archived-item-qa/ui/resources.ts @@ -92,20 +92,18 @@ function renderDiff( } export function renderResources(crawlData: ReplayData, qaData: ReplayData) { - const noData = html`
- - ${msg("Resources data not available")} -
`; + // const noData = html`
+ // + // ${msg("Resources data not available")} + //
`; return html`
- ${crawlData && qaData - ? crawlData.resources && qaData.resources - ? renderDiff(crawlData.resources, qaData.resources) - : noData + ${crawlData?.resources && qaData?.resources + ? renderDiff(crawlData.resources, qaData.resources) : renderSpinner()}
diff --git a/frontend/src/pages/org/archived-item-qa/ui/screenshots.ts b/frontend/src/pages/org/archived-item-qa/ui/screenshots.ts index 7d9e0873..50234a73 100644 --- a/frontend/src/pages/org/archived-item-qa/ui/screenshots.ts +++ b/frontend/src/pages/org/archived-item-qa/ui/screenshots.ts @@ -4,12 +4,12 @@ import { html } from "lit"; import { guard } from "lit/directives/guard.js"; import { when } from "lit/directives/when.js"; -import type { ReplayData } from "../types"; +import type { BlobPayload, ReplayData } from "../types"; import { renderSpinner } from "./spinner"; -function image(data: ReplayData) { - if (!data?.blobUrl) { +function image(blobUrl: BlobPayload["blobUrl"]) { + if (!blobUrl) { return html`
@@ -23,7 +23,7 @@ function image(data: ReplayData) { width="1920" height="1080" alt="" - src=${data.blobUrl} + src=${blobUrl} /> `; } @@ -60,13 +60,21 @@ export function renderScreenshots( class="aspect-video flex-1 overflow-hidden rounded-lg border bg-slate-50" aria-labelledby="crawlScreenshotHeading" > - ${when(crawlData, image, renderSpinner)} + ${when( + crawlData?.blobUrl !== undefined && crawlData.blobUrl, + image, + renderSpinner, + )}
- ${when(qaData, image, renderSpinner)} + ${when( + qaData?.blobUrl !== undefined && qaData.blobUrl, + image, + renderSpinner, + )}
` : html` @@ -75,10 +83,18 @@ export function renderScreenshots( >
- ${when(crawlData, image, renderSpinner)} + ${when( + crawlData?.blobUrl !== undefined && crawlData.blobUrl, + image, + renderSpinner, + )}
- ${when(qaData, image, renderSpinner)} + ${when( + qaData?.blobUrl !== undefined && qaData.blobUrl, + image, + renderSpinner, + )}
diff --git a/frontend/src/pages/org/archived-item-qa/ui/text.ts b/frontend/src/pages/org/archived-item-qa/ui/text.ts index b31f44b2..fb68e19d 100644 --- a/frontend/src/pages/org/archived-item-qa/ui/text.ts +++ b/frontend/src/pages/org/archived-item-qa/ui/text.ts @@ -86,7 +86,7 @@ export function renderText(crawlData: ReplayData, qaData: ReplayData) { > ${guard([crawlData, qaData], () => when( - crawlData && qaData, + crawlData?.text !== undefined && qaData?.text !== undefined, () => html`