From 732efe4512c3e87182d3d363274f5f61faa15210 Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Thu, 13 Aug 2026 16:28:57 -0400 Subject: [PATCH] fix(capture): ask one question about a finished popup, and ask it at the shutter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three waits before a screenshot — images, then aria-busy, then a hold — do not hold, because each one creates the next. The account lookup is what gives `#owner-avatar` its `src`, so the images check ran over a popup that had no avatar yet, passed, and aria-busy then cleared at the instant that lookup landed. The shot went out 200ms later with a grey circle where the account's picture goes, which is what the reverted popup-publication regeneration showed. `popupSettled` asks all of it at once — nothing aria-busy, no placeholder standing, every image with a src finished — and `settle` asks it again after the hold, since the shutter is the far side of a wait and several round trips. `popupStalled` is the opposite, so the loading capture now fails if it caught nothing loading rather than shooting an ordinary popup. --- docs/status-states.md | 19 ++++ scripts/capture-status-docs.mjs | 135 ++++++++++++++++++++++----- scripts/capture-status-docs.test.mjs | 108 +++++++++++++++++++++ 3 files changed, 237 insertions(+), 25 deletions(-) create mode 100644 scripts/capture-status-docs.test.mjs diff --git a/docs/status-states.md b/docs/status-states.md index cfce3c1..fe888b7 100644 --- a/docs/status-states.md +++ b/docs/status-states.md @@ -128,3 +128,22 @@ are authoritative, the tile framing is illustrative. A publication that moves or goes away fails the capture loudly (the script requires each `REAL` page to verify) rather than quietly changing the docs. + +## What counts as a finished popup + +Every capture but `popup-loading` waits for the whole of this, as one question +asked of the popup at one moment: no lookup still out (the card drops +`aria-busy`), no placeholder still standing, and every `` the popup has +given a `src` finished. `popup-loading` waits for the opposite, since the +placeholders are what it is a picture of. + +The three used to be waited for one after another, which does not hold, because +each of them *creates* the next: the account lookup is what gives the avatar its +`src`, so an image check that ran before that lookup landed was a check of a +popup that had no avatar yet. It passed, `aria-busy` cleared at the instant the +lookup landed, and the shot went out a fifth of a second later with a grey +circle where the account's picture goes. + +The question is also asked again after the hold, immediately before the shutter. +Everything the capture waits for is a sample of a popup that is still changing, +and the screenshot is the far side of a wait and several round trips. diff --git a/scripts/capture-status-docs.mjs b/scripts/capture-status-docs.mjs index bef73b9..4d2d0bc 100644 --- a/scripts/capture-status-docs.mjs +++ b/scripts/capture-status-docs.mjs @@ -273,6 +273,91 @@ async function poll(fn, what, timeoutMs = 8000) { } } +// --- when a popup is done ----------------------------------------------------- +// +// These run in the popup, not here: they are stringified into a +// Runtime.evaluate (see popupExpression), which is why they take `document` +// rather than closing over it, and why the one helper they share is put back +// beside them there. Kept as functions so the rule the captures depend on can +// be unit tested (capture-status-docs.test.mjs). + +/** Every image the popup has actually asked for has finished, one way or another. */ +export function imagesLanded(document) { + // `complete` is true for an image that failed as well as one that loaded, + // which is what this wants: a publication whose icon 404s is a real state, + // and the popup draws its monogram fallback for it. An `img` with no `src` + // is one nothing has been asked of yet — `complete` is true for it too, and + // that is the trap this whole file exists to avoid. + return [...document.images].every((img) => !img.getAttribute('src') || img.complete) +} + +/** + * A popup with nothing left to wait for: no lookup out, no placeholder + * standing, every image landed. + * + * One question rather than three waits in a row, because each of these + * *creates* the next. The account lookup is what gives `#owner-avatar` its + * `src`, so an images check that ran before that lookup landed was a check of + * a popup that had no avatar yet — it passed, and the shot went out with a + * grey circle where the account's picture goes. Asked as one thing, it cannot + * be satisfied by three answers that were never true at the same moment. + */ +export function popupSettled(document) { + // The card element itself, so a build without it fails here rather than + // passing a check that found nothing to object to. + if (!document.getElementById('pub')) return false + // Every region that reports its own work: the publication card while a card + // lookup is out, the status region while the worker has not answered about + // the page at all. Nothing else in the popup sets it. + if (document.querySelector('[aria-busy]')) return false + if (document.querySelector('.loading')) return false + return imagesLanded(document) +} + +/** + * The opposite, for the one capture that is *of* the placeholders: its lookups + * are held open and never land, so what it waits for is the card saying it is + * busy with a placeholder actually standing (src/popup/cards/loading.ts). + * Stated rather than assumed, so a capture of the loading state that caught + * nothing loading fails instead of shooting an ordinary popup. + */ +export function popupStalled(document) { + const pub = document.getElementById('pub') + if (!pub || !pub.hasAttribute('aria-busy')) return false + if (!document.querySelector('.loading')) return false + return imagesLanded(document) +} + +/** + * One of the predicates above as an expression the popup can evaluate. The + * helper is declared alongside it: a stringified function arrives with nothing + * it referred to, so what it called by name has to be in scope where it lands. + */ +export function popupExpression(predicate) { + return `(() => { + const imagesLanded = ${imagesLanded} + return (${predicate})(document) + })()` +} + +/** + * Wait for the popup to reach `predicate`, hold, and ask again — the shot is + * taken from a state that survived the hold rather than from one that was true + * some hundreds of milliseconds and several round trips before the shutter. + */ +async function settle(cdp, sessionId, predicate, holdMs, what) { + const expr = popupExpression(predicate) + await poll( + async () => { + if (!(await evaluate(cdp, sessionId, expr))) return false + await new Promise((r) => setTimeout(r, holdMs)) + return await evaluate(cdp, sessionId, expr) + }, + what, + SETTLE_MS, + ) +} + // --- fixture servers --------------------------------------------------------- /** Plain page; its /.well-known probe 404s, so detection finds nothing. */ @@ -895,8 +980,8 @@ async function screenshotPage(cdp, url, { width, height }) { * (stubLabelersByHost); queryLabels is served from these for this popup * @property {string[]} [stall] url substrings whose requests this popup never * gets an answer to, for capturing what it shows while it waits - * @property {number} [hold] ms to wait before the shot, over the settle - * (default 200); a capture of a transient state waits out its own delays + * @property {number} [hold] ms the popup must stay settled for before the + * shot (default 200); a capture of a transient state waits out its own delays * @property {{sel: string, settles?: string}[]} [clicks] clicked in order once * the popup settles, each waiting for its own `settles` expression (or for * the popup to reach expectPills) before the next one @@ -1059,26 +1144,22 @@ async function captureScenario(cdp, inWorker, popupUrl, s) { throw new Error(`${err.message}\n popup: ${await popupShape(cdp, sessionId)}`) } } - // The publication icon is a real blob fetch off the publisher's PDS; wait - // for every image to land or fail, or the card gets shot mid-load. - await poll( - () => evaluate(cdp, sessionId, '[...document.images].every((i) => i.complete)'), - `${s.name} images to load`, - SETTLE_MS, + // Every capture but one is of a popup that has finished: no lookup still + // out, no placeholder still standing, and every image — the publication + // icon, the account avatar, the subscriber faces, all real blob fetches + // off somebody's PDS — landed. The exception is the capture that is *of* + // the placeholders, whose lookups are held open and never land. + // + // The hold is inside this: the popup goes on changing after every wait + // here, and the shutter is the far side of the hold and a few more round + // trips, so what the gate saw has to still be true when it fires. + await settle( + cdp, + sessionId, + s.stall ? popupStalled : popupSettled, + s.hold ?? 200, + s.stall ? `${s.name} to be caught mid-load` : `${s.name} to finish loading`, ) - // Every capture but one is of a popup that has finished: the card marks - // itself busy while any of its lookups is still out, and a shot taken - // before they land is a shot of the placeholders those lookups stand - // behind (src/popup/cards/loading.ts). The exception is the capture that - // is *of* the placeholders, whose lookups are stalled and never land. - if (!s.stall) { - await poll( - () => evaluate(cdp, sessionId, `!document.getElementById('pub').hasAttribute('aria-busy')`), - `${s.name} lookups to land`, - SETTLE_MS, - ) - } - await new Promise((r) => setTimeout(r, s.hold ?? 200)) if (asked) { // Every stub labeler must have been asked, and every stubbed label must @@ -1183,7 +1264,11 @@ async function captureScenario(cdp, inWorker, popupUrl, s) { } } -main().catch((err) => { - console.error(err) - process.exit(1) -}) +// Importable for its own tests (capture-status-docs.test.mjs); only a direct +// run shoots anything. +if (process.argv[1] === import.meta.filename) { + main().catch((err) => { + console.error(err) + process.exit(1) + }) +} diff --git a/scripts/capture-status-docs.test.mjs b/scripts/capture-status-docs.test.mjs new file mode 100644 index 0000000..01c8e57 --- /dev/null +++ b/scripts/capture-status-docs.test.mjs @@ -0,0 +1,108 @@ +// What the capture calls a finished popup. +// +// These ran as three waits in a row and shot the store listing image mid-load +// anyway, so the rule is worth stating twice: once where the capture asks it, +// and once here, against the states that used to slip through it. + +import { describe, expect, it } from 'vitest' +import { + imagesLanded, + popupExpression, + popupSettled, + popupStalled, +} from './capture-status-docs.mjs' + +/** An `` as these predicates read one: a src it was given, and whether it landed. */ +const img = (src, complete) => ({ getAttribute: () => src, complete }) + +/** + * Enough of a document for the predicates: whether the popup has its `#pub` + * card, whether anything in it is `aria-busy` (the card while a lookup is out, + * the status region while the worker has not answered), whether a `.loading` + * placeholder is standing, and the images the popup has so far. + */ +function fakeDocument({ card = true, busy = false, loading = false, images = [] } = {}) { + return { + getElementById: (id) => (id === 'pub' && card ? { hasAttribute: () => busy } : null), + querySelector: (sel) => + (sel === '[aria-busy]' && busy) || (sel === '.loading' && loading) ? {} : null, + images, + } +} + +describe('imagesLanded', () => { + it('is true for a popup with no images at all', () => { + expect(imagesLanded(fakeDocument())).toBe(true) + }) + + it('waits for an image that is still being fetched', () => { + expect(imagesLanded(fakeDocument({ images: [img('https://pds/blob', false)] }))).toBe(false) + }) + + it('accepts an image that failed, which is a state the popup draws', () => { + // `complete` is true either way; the popup answers a failed icon with its + // monogram fallback, and a capture of that must not hang. + expect(imagesLanded(fakeDocument({ images: [img('https://pds/gone', true)] }))).toBe(true) + }) + + it('ignores an img nothing has been asked of yet', () => { + // The whole bug: `#owner-avatar` has no src until the account lookup + // lands, and an empty img reports itself complete. + expect(imagesLanded(fakeDocument({ images: [img(null, true)] }))).toBe(true) + }) +}) + +describe('popupSettled', () => { + it('accepts a card with no lookup out, no placeholder and every image landed', () => { + expect( + popupSettled(fakeDocument({ images: [img('https://pds/icon', true)] })), + ).toBe(true) + }) + + it('refuses a card that still says it is busy', () => { + expect(popupSettled(fakeDocument({ busy: true }))).toBe(false) + }) + + it('refuses a placeholder still standing', () => { + expect(popupSettled(fakeDocument({ loading: true }))).toBe(false) + }) + + it('refuses an avatar the landed lookup has only just asked for', () => { + // The state that shipped: the account card is drawn, so nothing is busy + // and no placeholder is up, but its avatar is still coming off the PDS. + expect(popupSettled(fakeDocument({ images: [img('https://pds/avatar', false)] }))).toBe(false) + }) + + it('refuses a popup that has no card element, so a wrong build cannot pass', () => { + expect(popupSettled(fakeDocument({ card: false }))).toBe(false) + }) +}) + +describe('popupExpression', () => { + /** Run the expression the way the popup does: as text, over a document. */ + const evaluateInPage = (predicate, doc) => + new Function('document', `return ${popupExpression(predicate)}`)(doc) + + // The predicates go over the wire as text, so a helper one of them calls has + // to travel with it. Left out, this is a ReferenceError inside the popup that + // nothing but a real capture run would find. + it('carries the helper the predicate calls into the page', () => { + const stillFetching = fakeDocument({ images: [img('https://pds/avatar', false)] }) + expect(evaluateInPage(popupSettled, stillFetching)).toBe(false) + expect(evaluateInPage(popupSettled, fakeDocument())).toBe(true) + expect(evaluateInPage(popupStalled, fakeDocument({ busy: true, loading: true }))).toBe( + true, + ) + }) +}) + +describe('popupStalled', () => { + it('accepts a busy card with a placeholder standing', () => { + expect(popupStalled(fakeDocument({ busy: true, loading: true }))).toBe(true) + }) + + it('refuses a popup that finished loading, which is not what it is a capture of', () => { + expect(popupStalled(fakeDocument({ loading: true }))).toBe(false) + expect(popupStalled(fakeDocument({ busy: true }))).toBe(false) + }) +}) -- 2.51.2