From 9ec022e91963a1989122d32b9842f5613ce045f6 Mon Sep 17 00:00:00 2001 From: Nathan Date: Thu, 14 May 2026 21:22:58 +0800 Subject: [PATCH 1/6] fix: type image-load errors and retry patiently with backoff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Users report that images uploaded from desktop sometimes don't appear on web until the page is refreshed multiple times. Two web-side causes: 1. `checkImage` for AppFlowy URLs falls back to `validateImageLoad` (an unauthenticated `` load) when the authenticated fetch fails. AppFlowy storage *requires* a Bearer token, so this fallback is guaranteed to 401/403 and the browser caches that failure under the URL. The polling loop then burns its retry attempts on a request that physically cannot succeed. 2. `Img.tsx` polls only 5× over 30 seconds with a single error state. If the upload pipeline takes longer than that (which it often does for first-render right after desktop uploads), polling exhausts and the user sees a permanent "Image not found" until they refresh. Changes: - `image.ts`: drop the no-auth fallback for AppFlowy URLs; return a typed `CheckImageErrorKind` (`no-auth`, `auth-rejected`, `forbidden`, `not-ready`, `not-found`, `server-error`, `network`, `format`) so the caller can pick a backoff per category. Add `cache: 'no-store'` on the fetch as defense-in-depth against stale CDN/browser entries. - `Img.tsx`: replace 5×6s polling with a typed-error-driven exponential backoff (~5 min wall-clock budget, ±20% jitter), three-state UI (loading → pending after 10 s → failed after budget), and a visibilitychange/focus listener that re-checks when the user returns to the tab. Skip retries entirely when the URL is empty (YJS sync hasn't delivered it yet). Manual retry button when finally failed. Together with the server-side cache-control fix (no-store on 4xx, 425 on metadata/S3 race), this turns "refresh five times" into "wait two seconds, image appears" for the common slow-upload case. --- .../editor/components/blocks/image/Img.tsx | 300 ++++++++++++------ src/utils/image.ts | 168 ++++++---- 2 files changed, 310 insertions(+), 158 deletions(-) diff --git a/src/components/editor/components/blocks/image/Img.tsx b/src/components/editor/components/blocks/image/Img.tsx index 0a70d52da..abc0f6ac5 100644 --- a/src/components/editor/components/blocks/image/Img.tsx +++ b/src/components/editor/components/blocks/image/Img.tsx @@ -3,7 +3,45 @@ import { useTranslation } from 'react-i18next'; import { ReactComponent as ErrorOutline } from '@/assets/icons/error.svg'; import LoadingDots from '@/components/_shared/LoadingDots'; -import { checkImage } from '@/utils/image'; +import { checkImage, CheckImageErrorKind, CheckImageResult } from '@/utils/image'; +import { Log } from '@/utils/log'; + +type LoadPhase = 'loading' | 'pending' | 'failed'; + +// Backoff schedule (ms). The polling stops once we either succeed or hit the +// last entry. Total wall-clock budget is ~5 minutes — long enough to outlast +// a slow upload pipeline, short enough that the user notices when something +// is actually broken. +const FAST_BACKOFF_MS = [500, 1000, 2000, 4000, 8000]; +const SLOW_BACKOFF_MS = [2000, 5000, 10000, 20000, 40000, 80000]; +// After this elapsed time without success, switch the UI from "loading" to +// "pending" — a softer state that says "we're still waiting" rather than +// "this failed." This prevents users from interpreting a slow optimistic +// upload as a broken image. +const PENDING_AFTER_MS = 10_000; + +function backoffSchedule(kind: CheckImageErrorKind | undefined): number[] { + switch (kind) { + case 'not-ready': + case 'no-auth': + case 'auth-rejected': + return FAST_BACKOFF_MS; + case 'forbidden': + return []; // terminal — don't retry + case 'not-found': + case 'server-error': + case 'network': + case 'format': + default: + return SLOW_BACKOFF_MS; + } +} + +function jitter(ms: number): number { + // ±20% jitter so a hundred clients don't synchronize their retries on the + // same edge cache miss / origin warm-up. + return Math.round(ms * (0.8 + Math.random() * 0.4)); +} function Img({ onLoad, @@ -17,135 +55,172 @@ function Img({ width: number | string; }) { const { t } = useTranslation(); - const [loading, setLoading] = useState(true); + const [phase, setPhase] = useState('loading'); const [localUrl, setLocalUrl] = useState(''); - const [imgError, setImgError] = useState<{ - ok: boolean; - status: number; - statusText: string; - } | null>(null); + const [lastError, setLastError] = useState(null); + const previousBlobUrlRef = useRef(''); const isMountedRef = useRef(true); + // Bumped on every restart to invalidate in-flight retry chains (focus + // re-checks, manual retries, URL changes). Any pending setTimeout callback + // that observes a stale generation just bails. + const runGenerationRef = useRef(0); - const handleCheckImage = useCallback(async (url: string) => { - setLoading(true); + const revokePreviousBlob = useCallback(() => { + if (previousBlobUrlRef.current && previousBlobUrlRef.current.startsWith('blob:')) { + URL.revokeObjectURL(previousBlobUrlRef.current); + previousBlobUrlRef.current = ''; + } + }, []); - // Configuration for polling - const maxAttempts = 5; // Maximum number of polling attempts - const pollingInterval = 6000; // Time between attempts in milliseconds (6 seconds) - const timeoutDuration = 30000; // Maximum time to poll in milliseconds (30 seconds) + const runCheck = useCallback( + async (targetUrl: string) => { + if (!targetUrl) { + // Empty URL means the block hasn't received a `data.url` yet (YJS + // sync in flight, or new image block). Don't waste retry attempts; + // just sit in `loading` until a real URL arrives and re-triggers + // this effect. + setPhase('loading'); + setLastError(null); + return; + } - let attempts = 0; - const startTime = Date.now(); + const generation = ++runGenerationRef.current; + const startedAt = Date.now(); + let attempt = 0; - const attemptCheck: () => Promise = async () => { - // Don't proceed if component is unmounted - if (!isMountedRef.current) { - return false; - } + setPhase('loading'); + setLastError(null); - try { - const result = await checkImage(url); + // Promote loading → pending if we cross the threshold without success. + const pendingTimer = window.setTimeout(() => { + if (!isMountedRef.current) return; + if (runGenerationRef.current !== generation) return; + setPhase((p) => (p === 'loading' ? 'pending' : p)); + }, PENDING_AFTER_MS); + + const cleanup = () => window.clearTimeout(pendingTimer); + + // Loop until success, until we exhaust the schedule, or until cancelled. + // eslint-disable-next-line no-constant-condition + while (true) { + if (!isMountedRef.current || runGenerationRef.current !== generation) { + cleanup(); + return; + } - // Don't update state if component is unmounted - if (!isMountedRef.current) { - // Revoke blob URL if component unmounted during fetch - if (result.ok && result.validatedUrl && result.validatedUrl.startsWith('blob:')) { + let result: CheckImageResult; + + try { + result = await checkImage(targetUrl); + } catch (err) { + Log.warn('[Img] checkImage threw', err); + result = { + ok: false, + status: 0, + statusText: 'Exception', + errorKind: 'network', + }; + } + + if (!isMountedRef.current || runGenerationRef.current !== generation) { + // We were cancelled mid-fetch; revoke any blob URL that arrived too late. + if (result.ok && result.validatedUrl?.startsWith('blob:')) { URL.revokeObjectURL(result.validatedUrl); } - return false; + cleanup(); + return; } - // Success case if (result.ok) { - /** - * Revoke previous blob URL to prevent memory leaks. - * - * When checkImage handles AppFlowy file storage URLs, it creates blob URLs via - * URL.createObjectURL(). These blob URLs must be explicitly revoked using - * URL.revokeObjectURL() to free memory, otherwise they persist until page reload. - * - * We only revoke if: - * - A previous blob URL exists - * - It's different from the new one (to avoid revoking the URL we're about to use) - * - It's actually a blob URL (not a regular HTTP URL) - * - * This prevents memory leaks when images change or during polling retries. - */ + const newUrl = result.validatedUrl || ''; + if ( previousBlobUrlRef.current && - previousBlobUrlRef.current !== result.validatedUrl && + previousBlobUrlRef.current !== newUrl && previousBlobUrlRef.current.startsWith('blob:') ) { URL.revokeObjectURL(previousBlobUrlRef.current); } - setImgError(null); - setLoading(false); - const newUrl = result.validatedUrl || ''; - - setLocalUrl(newUrl); previousBlobUrlRef.current = newUrl; - setTimeout(() => { - if (onLoad && isMountedRef.current) { - onLoad(); - } - }, 200); + setLocalUrl(newUrl); + setLastError(null); + setPhase('loading'); // 's onLoad will resolve this fully - return true; + cleanup(); + // Defer the onLoad callback so the consumer's effects observe the + // src change first, mirroring the previous behavior. + window.setTimeout(() => { + if (isMountedRef.current && runGenerationRef.current === generation) { + onLoad?.(); + } + }, 100); + return; } - // Error case but continue polling if within limits - setImgError(result); + setLastError(result); - // Check if we've exceeded our timeout or max attempts - attempts++; - const elapsedTime = Date.now() - startTime; + const schedule = backoffSchedule(result.errorKind); + const exhausted = attempt >= schedule.length; + const elapsed = Date.now() - startedAt; - if (attempts >= maxAttempts || elapsedTime >= timeoutDuration) { - setLoading(false); // Stop loading after max attempts or timeout - setImgError({ ok: false, status: 404, statusText: 'Image Not Found' }); - return false; + // Hard wall-clock cap — even if backoff would happily continue. + if (exhausted || elapsed > 5 * 60 * 1000) { + setPhase('failed'); + cleanup(); + return; } - await new Promise((resolve) => setTimeout(resolve, pollingInterval)); - return await attemptCheck(); - // eslint-disable-next-line - } catch (e) { - setImgError({ ok: false, status: 404, statusText: 'Image Not Found' }); - // Check if we should stop trying - attempts++; - const elapsedTime = Date.now() - startTime; - - if (attempts >= maxAttempts || elapsedTime >= timeoutDuration) { - setLoading(false); - return false; - } + const delayMs = jitter(schedule[attempt]); - // Continue polling after interval - await new Promise((resolve) => setTimeout(resolve, pollingInterval)); - return await attemptCheck(); - } - }; + attempt += 1; - void attemptCheck(); - // eslint-disable-next-line - }, []); + await new Promise((resolve) => window.setTimeout(resolve, delayMs)); + } + }, + [onLoad] + ); useEffect(() => { isMountedRef.current = true; - void handleCheckImage(url); + void runCheck(url); - // Cleanup: revoke blob URL when component unmounts or URL changes return () => { isMountedRef.current = false; - if (previousBlobUrlRef.current && previousBlobUrlRef.current.startsWith('blob:')) { - URL.revokeObjectURL(previousBlobUrlRef.current); - previousBlobUrlRef.current = ''; + // Invalidate any in-flight retry loop bound to this URL. + runGenerationRef.current += 1; + revokePreviousBlob(); + }; + }, [url, runCheck, revokePreviousBlob]); + + // Re-check when the tab regains focus and we're currently failed/pending. + // Matches the user's mental model: "I came back to the tab and it appears." + useEffect(() => { + const onVisible = () => { + if (document.visibilityState !== 'visible') return; + if (phase === 'failed' || phase === 'pending') { + void runCheck(url); } }; - }, [handleCheckImage, url]); + + document.addEventListener('visibilitychange', onVisible); + window.addEventListener('focus', onVisible); + return () => { + document.removeEventListener('visibilitychange', onVisible); + window.removeEventListener('focus', onVisible); + }; + }, [phase, url, runCheck]); + + const handleManualRetry = useCallback(() => { + void runCheck(url); + }, [runCheck, url]); + + const showLoading = phase === 'loading'; + const showPending = phase === 'pending'; + const showFailed = phase === 'failed'; + const hideImage = showLoading || showPending || showFailed; return ( <> @@ -154,30 +229,55 @@ function Img({ src={localUrl} alt={''} onLoad={() => { - setLoading(false); - setImgError(null); + if (localUrl) { + setPhase('loading'); // ensures hideImage flips off + setLastError(null); + } }} draggable={false} style={{ - visibility: imgError ? 'hidden' : 'visible', + visibility: hideImage ? 'hidden' : 'visible', width, }} className={'h-full bg-cover bg-center object-cover'} /> - {loading ? ( -
+ {showLoading && ( +
- ) : imgError ? ( + )} + {showPending && (
- -
{t('editor.imageLoadFailed')}
+ +
{t('editor.imageStillUploading', 'Waiting for upload to finish…')}
- ) : null} + )} + {showFailed && ( + + )} ); } diff --git a/src/utils/image.ts b/src/utils/image.ts index b78a4da14..70c4968bd 100644 --- a/src/utils/image.ts +++ b/src/utils/image.ts @@ -76,27 +76,56 @@ const resolveImageUrl = (url: string): string => { }; -interface CheckImageResult { +/** + * Categorized failure mode for image loads. The polling/retry policy in + * `Img.tsx` keys off this — different categories deserve different backoff. + */ +export type CheckImageErrorKind = + | 'no-auth' // Local: no token yet. Wait briefly; token is hydrating. + | 'auth-rejected' // 401 from server. Token expired / invalid. Refresh + retry once. + | 'forbidden' // 403 from server. Permission denied. Terminal. + | 'not-ready' // 425/503 from server. Upload pipeline still in flight. Fast retry. + | 'not-found' // 404 from server. Could still be a slow optimistic upload — slow retry. + | 'server-error' // 5xx. Normal retry. + | 'network' // fetch threw / timed out / opaque onerror. + | 'format'; // Successfully fetched but not a usable image blob. + +export interface CheckImageResult { ok: boolean; status: number; statusText: string; error?: string; + errorKind?: CheckImageErrorKind; validatedUrl?: string; } -// Helper function to check image using Image() approach +const errorResult = ( + status: number, + statusText: string, + errorKind: CheckImageErrorKind, + error?: string +): CheckImageResult => ({ ok: false, status, statusText, errorKind, error }); + +const classifyHttpStatus = (status: number): CheckImageErrorKind => { + if (status === 401) return 'auth-rejected'; + if (status === 403) return 'forbidden'; + if (status === 404) return 'not-found'; + if (status === 425 || status === 503 || status === 408) return 'not-ready'; + if (status >= 500) return 'server-error'; + return 'network'; +}; + +// Probe a non-AppFlowy URL by attempting to load it via . We can't read +// the HTTP status from a cross-origin , so failures collapse into a +// generic 'network' error — that's fine for the retry policy because it +// doesn't try to distinguish 404 vs 5xx for external hosts anyway. const validateImageLoad = (imageUrl: string): Promise => { return new Promise((resolve) => { const img = new Image(); // Set a timeout to handle very slow loads const timeoutId = setTimeout(() => { - resolve({ - ok: false, - status: 408, - statusText: 'Request Timeout', - error: 'Image loading timed out', - }); + resolve(errorResult(408, 'Request Timeout', 'not-ready', 'Image loading timed out')); }, 10000); // 10 second timeout img.onload = () => { @@ -111,12 +140,7 @@ const validateImageLoad = (imageUrl: string): Promise => { img.onerror = () => { clearTimeout(timeoutId); - resolve({ - ok: false, - status: 404, - statusText: 'Image Not Found', - error: 'Failed to load image', - }); + resolve(errorResult(0, 'Image load failed', 'network', 'Failed to load image')); }; img.src = imageUrl; @@ -152,58 +176,86 @@ const validateImageBlob = async (blob: Blob, url?: string): Promise }; export const checkImage = async (url: string): Promise => { - // If it's an AppFlowy file storage URL, try authenticated fetch first if (isAppFlowyFileStorageUrl(url)) { - const token = getTokenParsed(); - const fullUrl = resolveImageUrl(url); + return checkAppFlowyImage(url); + } - Log.debug('[checkImage] fullUrl', fullUrl); - - if (token) { - try { - const response = await fetch(fullUrl, { - headers: { - Authorization: `Bearer ${token.access_token}`, - 'x-platform': 'web-app', - }, - }); - - if (response.ok) { - const blob = await response.blob(); - const validatedBlob = await validateImageBlob(blob, url); - - if (!validatedBlob) { - return { - ok: false, - status: 406, // Not Acceptable - statusText: 'Not Acceptable', - error: 'Image fetch returned JSON instead of image', - }; - } + // External URL — let the browser do its thing. + return validateImageLoad(url); +}; - const blobUrl = URL.createObjectURL(validatedBlob); +/** + * Fetch an AppFlowy-storage image with auth and turn it into a blob URL the + * can render. + * + * Why not fall back to a plain `` on failure (as we used to): + * - AppFlowy storage requires a Bearer token; an unauthenticated + * request is guaranteed to fail (401/403). The browser would then cache + * that failure under the URL, so subsequent legitimate retries get the + * cached error without ever hitting the server. + * - The polling loop in Img.tsx burns retry attempts on guaranteed-failure + * requests instead of waiting for a real condition to change (token + * becoming available, upload pipeline finishing). + * + * Instead, return a typed error so the caller can apply a sensible backoff. + */ +async function checkAppFlowyImage(url: string): Promise { + const fullUrl = resolveImageUrl(url); - return { - ok: true, - status: 200, - statusText: 'OK', - validatedUrl: blobUrl, - }; - } + Log.debug('[checkImage] AppFlowy', fullUrl); - console.error('Authenticated image fetch failed', response.status, response.statusText); - } catch (error) { - console.error('Failed to fetch authenticated image', error); - } - } + const token = getTokenParsed(); - // Fallback for no token or failed fetch - return validateImageLoad(fullUrl); + if (!token) { + // Token may still be hydrating from storage. Caller retries shortly. + return errorResult(401, 'No auth token', 'no-auth'); } - // For non-AppFlowy URLs, use the original Image() approach - return validateImageLoad(url); -}; + let response: Response; + + try { + response = await fetch(fullUrl, { + headers: { + Authorization: `Bearer ${token.access_token}`, + 'x-platform': 'web-app', + }, + // Don't let the browser cache transient failures (404/425/5xx) — the + // server now sends `Cache-Control: no-store` on those, but defending + // against an older server / misbehaving proxy is cheap. + cache: 'no-store', + }); + } catch (err) { + Log.warn('[checkImage] auth fetch network error', err); + return errorResult(0, 'Network error', 'network', String(err)); + } + + if (!response.ok) { + return errorResult( + response.status, + response.statusText, + classifyHttpStatus(response.status) + ); + } + + const blob = await response.blob(); + const validatedBlob = await validateImageBlob(blob, url); + + if (!validatedBlob) { + return errorResult( + 406, + 'Not Acceptable', + 'format', + 'Image fetch returned JSON instead of image' + ); + } + + return { + ok: true, + status: 200, + statusText: 'OK', + validatedUrl: URL.createObjectURL(validatedBlob), + }; +} export const fetchImageBlob = async (url: string): Promise => { if (isAppFlowyFileStorageUrl(url)) { From e863377456e4b06c5baa06a5d5174c8c5c4d85dd Mon Sep 17 00:00:00 2001 From: Nathan Date: Thu, 14 May 2026 21:41:48 +0800 Subject: [PATCH 2/6] fix: honor server Cache-Control on first fetch, bypass only on retry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, `checkAppFlowyImage` set `cache: 'no-store'` on every fetch as defense-in-depth against stale negative cache entries. But that disables the browser HTTP cache for *all* responses — including the 200s that the server now tags `Cache-Control: public, immutable, max-age=31536000`. Result: every component mount re-downloaded the image even though the server told the browser it could keep the bytes for a year. Switch to a per-attempt cache policy: - First attempt: `cache: 'default'` — honor server headers. Successful fetches enter the HTTP cache; the next mount of the same Img is free. Failures (no-store from the server) still skip the cache, so no poisoning risk. - Retry attempts: `cache: 'reload'` — force a server round-trip to defeat any stale cached response from a misbehaving proxy. Unlike `no-store`, `reload` still lets the *new* response enter the cache, so once we finally succeed we stop paying network for re-renders. Adds a `CheckImageOptions { retry?: boolean }` parameter to `checkImage` and threads it from the polling loop in Img.tsx (`{ retry: attempt > 0 }`). --- .../editor/components/blocks/image/Img.tsx | 6 ++- src/utils/image.ts | 37 +++++++++++++++---- 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/src/components/editor/components/blocks/image/Img.tsx b/src/components/editor/components/blocks/image/Img.tsx index abc0f6ac5..e80723971 100644 --- a/src/components/editor/components/blocks/image/Img.tsx +++ b/src/components/editor/components/blocks/image/Img.tsx @@ -112,7 +112,11 @@ function Img({ let result: CheckImageResult; try { - result = await checkImage(targetUrl); + // First attempt uses the browser HTTP cache (so a previously + // fetched immutable image is served from disk without hitting + // the network). Subsequent attempts bypass the cache in case + // a stale failure response is somehow lingering. + result = await checkImage(targetUrl, { retry: attempt > 0 }); } catch (err) { Log.warn('[Img] checkImage threw', err); result = { diff --git a/src/utils/image.ts b/src/utils/image.ts index 70c4968bd..2c81f61e4 100644 --- a/src/utils/image.ts +++ b/src/utils/image.ts @@ -175,9 +175,24 @@ const validateImageBlob = async (blob: Blob, url?: string): Promise return transcodeIfUnsupported(normalizedBlob, url); }; -export const checkImage = async (url: string): Promise => { +export interface CheckImageOptions { + /** + * True when this call is a retry after a previous failure. Retries skip + * the browser HTTP cache so we always re-ask the origin — defends against + * a misbehaving proxy that cached a transient failure. First attempts use + * the default cache mode so successful responses (which the server tags + * `Cache-Control: public, immutable, max-age=31536000`) can be reused + * across mounts without a network round-trip. + */ + retry?: boolean; +} + +export const checkImage = async ( + url: string, + options: CheckImageOptions = {} +): Promise => { if (isAppFlowyFileStorageUrl(url)) { - return checkAppFlowyImage(url); + return checkAppFlowyImage(url, options); } // External URL — let the browser do its thing. @@ -199,7 +214,10 @@ export const checkImage = async (url: string): Promise => { * * Instead, return a typed error so the caller can apply a sensible backoff. */ -async function checkAppFlowyImage(url: string): Promise { +async function checkAppFlowyImage( + url: string, + options: CheckImageOptions +): Promise { const fullUrl = resolveImageUrl(url); Log.debug('[checkImage] AppFlowy', fullUrl); @@ -219,10 +237,15 @@ async function checkAppFlowyImage(url: string): Promise { Authorization: `Bearer ${token.access_token}`, 'x-platform': 'web-app', }, - // Don't let the browser cache transient failures (404/425/5xx) — the - // server now sends `Cache-Control: no-store` on those, but defending - // against an older server / misbehaving proxy is cheap. - cache: 'no-store', + // First attempt: honor the server's Cache-Control (immutable, 1 yr + // on 200; no-store on 4xx/5xx). This is what lets the browser cache + // a once-fetched image forever and skip the network on later mounts. + // + // Retry: force a server round-trip via `reload`, in case some proxy + // or older server version cached a stale failure. We deliberately + // don't use `no-store` here — `reload` still allows the response we + // receive *now* to enter the cache normally. + cache: options.retry ? 'reload' : 'default', }); } catch (err) { Log.warn('[checkImage] auth fetch network error', err); From 41780437cbbbcb7612028137696932fe11918c95 Mon Sep 17 00:00:00 2001 From: Nathan Date: Thu, 14 May 2026 21:56:50 +0800 Subject: [PATCH 3/6] fix: address review issues from React best-practices pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six fixes to Img.tsx + image.ts, in order of severity: 1. Image was hidden forever after successful load. `phase === 'loading'` left `visibility: hidden` on the , and the 's own onLoad handler also wrote 'loading'. Split into two state machines: `phase` tracks the fetch lifecycle (loading|pending|failed), `isImageReady` tracks the 's decode lifecycle (set by native onLoad). Visibility keys on `isImageReady` only. 2. Inline `onLoad={() => ...}` props from parent re-mounted on every render churned `runCheck`'s identity, which made the URL-effect cancel the in-flight retry chain and start fresh — so every parent re-render re-fetched the image. Now `onLoad` is read through `onLoadRef` and `runCheck` has a stable identity (deps: [cancelAllInflight, scheduleTimer]). 3. The visibilitychange/focus listener effect re-subscribed every time phase or url changed. Now attached once; reads via `phaseRef`, `urlRef`. 4. The auth fetch in `image.ts::checkAppFlowyImage` ignored cancellation — the polling loop checked refs between awaits, but the fetch itself ran to completion. Added `signal?: AbortSignal` to `CheckImageOptions` and plumbed an `AbortController` per `runCheck` invocation. AbortError is classified separately so it doesn't log as a network error. 5. Untracked `setTimeout`s (pendingTimer, retry-delay) now go through a `timersRef` Set. On cancellation `cancelAllInflight()` aborts every controller and clears every pending timer in one call. Retry-delay `sleep` resolves early when the abort signal fires. 6. Free perf wins on the rendered : - `loading="lazy"` — defer offscreen images until they near viewport - `decoding="async"` — let the browser decode off-thread - Added `onError` handler that flips to 'failed' on decode failures (rare but previously silent). Skipped from the review: - Better `alt` text (would require parent prop plumbing; pre-existing). - Extracting state machine into a `useImageWithRetry` hook (worthwhile but a structural change; tracked for a follow-up). --- .../editor/components/blocks/image/Img.tsx | 213 ++++++++++++------ src/utils/image.ts | 13 ++ 2 files changed, 151 insertions(+), 75 deletions(-) diff --git a/src/components/editor/components/blocks/image/Img.tsx b/src/components/editor/components/blocks/image/Img.tsx index e80723971..5a3bbd17f 100644 --- a/src/components/editor/components/blocks/image/Img.tsx +++ b/src/components/editor/components/blocks/image/Img.tsx @@ -6,18 +6,22 @@ import LoadingDots from '@/components/_shared/LoadingDots'; import { checkImage, CheckImageErrorKind, CheckImageResult } from '@/utils/image'; import { Log } from '@/utils/log'; +// Polling state machine. Note: this tracks the *fetch* lifecycle, not the +// rendered ``'s decode lifecycle — the ``'s native onLoad is what +// flips `isImageReady` independently. type LoadPhase = 'loading' | 'pending' | 'failed'; -// Backoff schedule (ms). The polling stops once we either succeed or hit the -// last entry. Total wall-clock budget is ~5 minutes — long enough to outlast -// a slow upload pipeline, short enough that the user notices when something -// is actually broken. +// Backoff schedules (ms). Total wall-clock budget is ~5 minutes — long +// enough to outlast a slow upload pipeline, short enough that the user +// notices when something is actually broken. const FAST_BACKOFF_MS = [500, 1000, 2000, 4000, 8000]; const SLOW_BACKOFF_MS = [2000, 5000, 10000, 20000, 40000, 80000]; +const MAX_BUDGET_MS = 5 * 60 * 1000; + // After this elapsed time without success, switch the UI from "loading" to // "pending" — a softer state that says "we're still waiting" rather than -// "this failed." This prevents users from interpreting a slow optimistic -// upload as a broken image. +// "this failed." Prevents users from interpreting a slow optimistic upload +// as a broken image. const PENDING_AFTER_MS = 10_000; function backoffSchedule(kind: CheckImageErrorKind | undefined): number[] { @@ -38,8 +42,8 @@ function backoffSchedule(kind: CheckImageErrorKind | undefined): number[] { } function jitter(ms: number): number { - // ±20% jitter so a hundred clients don't synchronize their retries on the - // same edge cache miss / origin warm-up. + // ±20% jitter so a hundred clients don't synchronize retries on the same + // edge cache miss / origin warm-up. return Math.round(ms * (0.8 + Math.random() * 0.4)); } @@ -58,13 +62,50 @@ function Img({ const [phase, setPhase] = useState('loading'); const [localUrl, setLocalUrl] = useState(''); const [lastError, setLastError] = useState(null); - + // ``-decode state. Independent of `phase` (which is about *fetching*). + // Visibility is gated on this so the spinner stays up until the browser + // has actually painted the image. + const [isImageReady, setIsImageReady] = useState(false); + + // Refs to keep `runCheck`'s identity stable and to let listeners read the + // latest values without subscribing. See review bugs #2 and #3. + const onLoadRef = useRef(onLoad); + const urlRef = useRef(url); + const phaseRef = useRef(phase); const previousBlobUrlRef = useRef(''); - const isMountedRef = useRef(true); - // Bumped on every restart to invalidate in-flight retry chains (focus - // re-checks, manual retries, URL changes). Any pending setTimeout callback - // that observes a stale generation just bails. - const runGenerationRef = useRef(0); + // In-flight cleanup tracking so we can cancel imperatively on unmount, + // URL change, or manual retry. See review bugs #4 and #5. + const controllersRef = useRef>(new Set()); + const timersRef = useRef>(new Set()); + + // Mirror props/state into refs every commit. Cheap and lets event handlers + // and async loops read the current value without re-creating closures. + useEffect(() => { + onLoadRef.current = onLoad; + }); + useEffect(() => { + urlRef.current = url; + }); + useEffect(() => { + phaseRef.current = phase; + }); + + const scheduleTimer = useCallback((cb: () => void, ms: number) => { + const id = window.setTimeout(() => { + timersRef.current.delete(id); + cb(); + }, ms); + + timersRef.current.add(id); + return id; + }, []); + + const cancelAllInflight = useCallback(() => { + controllersRef.current.forEach((c) => c.abort()); + controllersRef.current.clear(); + timersRef.current.forEach((id) => window.clearTimeout(id)); + timersRef.current.clear(); + }, []); const revokePreviousBlob = useCallback(() => { if (previousBlobUrlRef.current && previousBlobUrlRef.current.startsWith('blob:')) { @@ -73,19 +114,27 @@ function Img({ } }, []); + // Stable. Reads everything it needs through refs so it never has to be + // re-created when props change. const runCheck = useCallback( async (targetUrl: string) => { + // Anything left over from a previous run is now stale — abort and + // clear timers up front. + cancelAllInflight(); + setIsImageReady(false); + if (!targetUrl) { - // Empty URL means the block hasn't received a `data.url` yet (YJS - // sync in flight, or new image block). Don't waste retry attempts; - // just sit in `loading` until a real URL arrives and re-triggers - // this effect. + // No URL yet (YJS sync still in flight). Sit in `loading` until a + // real URL arrives and re-triggers the URL-change effect. setPhase('loading'); setLastError(null); return; } - const generation = ++runGenerationRef.current; + const controller = new AbortController(); + + controllersRef.current.add(controller); + const startedAt = Date.now(); let attempt = 0; @@ -93,30 +142,39 @@ function Img({ setLastError(null); // Promote loading → pending if we cross the threshold without success. - const pendingTimer = window.setTimeout(() => { - if (!isMountedRef.current) return; - if (runGenerationRef.current !== generation) return; + scheduleTimer(() => { + if (controller.signal.aborted) return; setPhase((p) => (p === 'loading' ? 'pending' : p)); }, PENDING_AFTER_MS); - const cleanup = () => window.clearTimeout(pendingTimer); + // Cancellable delay between retries. Resolves early if aborted. + const sleep = (ms: number) => + new Promise((resolve) => { + const onAbort = () => { + window.clearTimeout(id); + timersRef.current.delete(id); + resolve(); + }; + + const id = scheduleTimer(() => { + controller.signal.removeEventListener('abort', onAbort); + resolve(); + }, ms); + + controller.signal.addEventListener('abort', onAbort, { once: true }); + }); - // Loop until success, until we exhaust the schedule, or until cancelled. // eslint-disable-next-line no-constant-condition while (true) { - if (!isMountedRef.current || runGenerationRef.current !== generation) { - cleanup(); - return; - } + if (controller.signal.aborted) return; let result: CheckImageResult; try { - // First attempt uses the browser HTTP cache (so a previously - // fetched immutable image is served from disk without hitting - // the network). Subsequent attempts bypass the cache in case - // a stale failure response is somehow lingering. - result = await checkImage(targetUrl, { retry: attempt > 0 }); + result = await checkImage(targetUrl, { + retry: attempt > 0, + signal: controller.signal, + }); } catch (err) { Log.warn('[Img] checkImage threw', err); result = { @@ -127,13 +185,12 @@ function Img({ }; } - if (!isMountedRef.current || runGenerationRef.current !== generation) { - // We were cancelled mid-fetch; revoke any blob URL that arrived too late. + if (controller.signal.aborted) { + // Revoke any blob URL that arrived after cancellation. if (result.ok && result.validatedUrl?.startsWith('blob:')) { URL.revokeObjectURL(result.validatedUrl); } - cleanup(); return; } @@ -151,16 +208,11 @@ function Img({ previousBlobUrlRef.current = newUrl; setLocalUrl(newUrl); setLastError(null); - setPhase('loading'); // 's onLoad will resolve this fully - - cleanup(); - // Defer the onLoad callback so the consumer's effects observe the - // src change first, mirroring the previous behavior. - window.setTimeout(() => { - if (isMountedRef.current && runGenerationRef.current === generation) { - onLoad?.(); - } - }, 100); + // Stay in 'loading' until fires onLoad and we flip + // isImageReady. That keeps the spinner up across the decode. + setPhase('loading'); + + controllersRef.current.delete(controller); return; } @@ -170,42 +222,35 @@ function Img({ const exhausted = attempt >= schedule.length; const elapsed = Date.now() - startedAt; - // Hard wall-clock cap — even if backoff would happily continue. - if (exhausted || elapsed > 5 * 60 * 1000) { + if (exhausted || elapsed > MAX_BUDGET_MS) { setPhase('failed'); - cleanup(); + controllersRef.current.delete(controller); return; } - const delayMs = jitter(schedule[attempt]); - + await sleep(jitter(schedule[attempt])); attempt += 1; - - await new Promise((resolve) => window.setTimeout(resolve, delayMs)); } }, - [onLoad] + [cancelAllInflight, scheduleTimer] ); + // Kick off (or restart) the check whenever the URL changes. useEffect(() => { - isMountedRef.current = true; void runCheck(url); return () => { - isMountedRef.current = false; - // Invalidate any in-flight retry loop bound to this URL. - runGenerationRef.current += 1; + cancelAllInflight(); revokePreviousBlob(); }; - }, [url, runCheck, revokePreviousBlob]); + }, [url, runCheck, cancelAllInflight, revokePreviousBlob]); - // Re-check when the tab regains focus and we're currently failed/pending. - // Matches the user's mental model: "I came back to the tab and it appears." + // Re-check on tab focus / visibility return — listeners attached ONCE. useEffect(() => { const onVisible = () => { if (document.visibilityState !== 'visible') return; - if (phase === 'failed' || phase === 'pending') { - void runCheck(url); + if (phaseRef.current === 'failed' || phaseRef.current === 'pending') { + void runCheck(urlRef.current); } }; @@ -215,16 +260,36 @@ function Img({ document.removeEventListener('visibilitychange', onVisible); window.removeEventListener('focus', onVisible); }; - }, [phase, url, runCheck]); + }, [runCheck]); const handleManualRetry = useCallback(() => { - void runCheck(url); - }, [runCheck, url]); + void runCheck(urlRef.current); + }, [runCheck]); + + const handleImageLoaded = useCallback(() => { + if (!previousBlobUrlRef.current) return; + setIsImageReady(true); + setLastError(null); + // Notify the parent once the image is actually painted, not just fetched. + onLoadRef.current?.(); + }, []); - const showLoading = phase === 'loading'; + const handleImageError = useCallback(() => { + // The browser couldn't decode the bytes we gave it (rare — corrupted + // blob, format mismatch). Treat as a format error and stop retrying. + setIsImageReady(false); + setLastError({ + ok: false, + status: 0, + statusText: 'Decode failed', + errorKind: 'format', + }); + setPhase('failed'); + }, []); + + const showLoading = phase === 'loading' && !isImageReady; const showPending = phase === 'pending'; const showFailed = phase === 'failed'; - const hideImage = showLoading || showPending || showFailed; return ( <> @@ -232,15 +297,13 @@ function Img({ ref={imgRef} src={localUrl} alt={''} - onLoad={() => { - if (localUrl) { - setPhase('loading'); // ensures hideImage flips off - setLastError(null); - } - }} + onLoad={handleImageLoaded} + onError={handleImageError} + loading={'lazy'} + decoding={'async'} draggable={false} style={{ - visibility: hideImage ? 'hidden' : 'visible', + visibility: isImageReady ? 'visible' : 'hidden', width, }} className={'h-full bg-cover bg-center object-cover'} diff --git a/src/utils/image.ts b/src/utils/image.ts index 2c81f61e4..84d041bfe 100644 --- a/src/utils/image.ts +++ b/src/utils/image.ts @@ -185,6 +185,12 @@ export interface CheckImageOptions { * across mounts without a network round-trip. */ retry?: boolean; + /** + * Lets the caller cancel an in-flight fetch (and the retry chain that + * follows it) on unmount / URL change. Without this, orphan fetches can + * keep running after the component goes away. + */ + signal?: AbortSignal; } export const checkImage = async ( @@ -246,8 +252,15 @@ async function checkAppFlowyImage( // don't use `no-store` here — `reload` still allows the response we // receive *now* to enter the cache normally. cache: options.retry ? 'reload' : 'default', + signal: options.signal, }); } catch (err) { + if ((err as { name?: string })?.name === 'AbortError') { + // Caller aborted (unmount, URL change). Surface as a non-actionable + // error so the caller can detect it without logging noise. + return errorResult(0, 'Aborted', 'network', 'aborted'); + } + Log.warn('[checkImage] auth fetch network error', err); return errorResult(0, 'Network error', 'network', String(err)); } From 219da733752055699676219efddebf283a44a8fd Mon Sep 17 00:00:00 2001 From: Nathan Date: Thu, 14 May 2026 22:15:58 +0800 Subject: [PATCH 4/6] refactor: extract image-load state machine + add accessibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups to the React best-practices review (PR comments #7, #8): **#8 — Extract `useImageWithRetry` hook.** The fetch state machine (backoff schedules, abort controllers, timer tracking, ref-stable listener attachment) was ~200 lines of imperative async logic awkwardly inlined in a presentation component. Moved to `src/components/editor/components/blocks/image/useImageWithRetry.ts`, which returns `{ src, phase, isImageReady, lastError, retry, onImageLoaded, onImageError }`. `Img.tsx` is now ~100 lines of pure rendering. The hook is unit-testable in isolation with a fetch mock, and the JSX no longer hides correctness concerns behind a wall of useCallback/useEffect plumbing. **#7 — Accessibility.** Pre-existing issue: every image rendered with `alt={''}`, which tells assistive tech "this is decorative" — wrong for content images in a document editor. `Img` now accepts an `alt` prop. When omitted (current callers), it derives a label from the URL filename — strips extension, percent-encoding, AppFlowy upload prefix, collapses separators. Falls back to "Embedded image" if nothing sensible can be extracted. Future work: thread an explicit `alt` from ImageBlockData once the data model gains a caption field. Also tightened the failure/loading overlays with `role="status"`, `aria-live="polite"`, `aria-label`, and `aria-hidden` on the decorative error icon — screen readers now announce loading state transitions instead of going silent. Behavior unchanged; lint + type-check clean. Image E2E tests not re-run in this commit (local docker stack down — port 8000 not listening); they passed against the previous commit on this branch and the refactor preserves all observable behavior. --- .../components/blocks/image/ImageRender.tsx | 3 + .../editor/components/blocks/image/Img.tsx | 341 +++--------------- .../blocks/image/useImageWithRetry.ts | 321 +++++++++++++++++ 3 files changed, 380 insertions(+), 285 deletions(-) create mode 100644 src/components/editor/components/blocks/image/useImageWithRetry.ts diff --git a/src/components/editor/components/blocks/image/ImageRender.tsx b/src/components/editor/components/blocks/image/ImageRender.tsx index 8f48eb854..3d6820fa4 100644 --- a/src/components/editor/components/blocks/image/ImageRender.tsx +++ b/src/components/editor/components/blocks/image/ImageRender.tsx @@ -71,6 +71,9 @@ function ImageRender({ width={rendered ? (newWidth ?? '100%') : 0} imgRef={imgRef} url={url} + // `ImageBlockData` has no caption field today, so let `Img` derive + // an alt from the URL filename. This avoids screen readers reading + // the raw blob URL or skipping the image entirely. onLoad={() => { setRendered(true); }} diff --git a/src/components/editor/components/blocks/image/Img.tsx b/src/components/editor/components/blocks/image/Img.tsx index 5a3bbd17f..aec2c3a0e 100644 --- a/src/components/editor/components/blocks/image/Img.tsx +++ b/src/components/editor/components/blocks/image/Img.tsx @@ -1,304 +1,66 @@ -import React, { useCallback, useEffect, useRef, useState } from 'react'; +import React from 'react'; import { useTranslation } from 'react-i18next'; import { ReactComponent as ErrorOutline } from '@/assets/icons/error.svg'; import LoadingDots from '@/components/_shared/LoadingDots'; -import { checkImage, CheckImageErrorKind, CheckImageResult } from '@/utils/image'; -import { Log } from '@/utils/log'; -// Polling state machine. Note: this tracks the *fetch* lifecycle, not the -// rendered ``'s decode lifecycle — the ``'s native onLoad is what -// flips `isImageReady` independently. -type LoadPhase = 'loading' | 'pending' | 'failed'; +import { useImageWithRetry } from './useImageWithRetry'; -// Backoff schedules (ms). Total wall-clock budget is ~5 minutes — long -// enough to outlast a slow upload pipeline, short enough that the user -// notices when something is actually broken. -const FAST_BACKOFF_MS = [500, 1000, 2000, 4000, 8000]; -const SLOW_BACKOFF_MS = [2000, 5000, 10000, 20000, 40000, 80000]; -const MAX_BUDGET_MS = 5 * 60 * 1000; - -// After this elapsed time without success, switch the UI from "loading" to -// "pending" — a softer state that says "we're still waiting" rather than -// "this failed." Prevents users from interpreting a slow optimistic upload -// as a broken image. -const PENDING_AFTER_MS = 10_000; - -function backoffSchedule(kind: CheckImageErrorKind | undefined): number[] { - switch (kind) { - case 'not-ready': - case 'no-auth': - case 'auth-rejected': - return FAST_BACKOFF_MS; - case 'forbidden': - return []; // terminal — don't retry - case 'not-found': - case 'server-error': - case 'network': - case 'format': - default: - return SLOW_BACKOFF_MS; - } -} - -function jitter(ms: number): number { - // ±20% jitter so a hundred clients don't synchronize retries on the same - // edge cache miss / origin warm-up. - return Math.round(ms * (0.8 + Math.random() * 0.4)); -} - -function Img({ - onLoad, - imgRef, - url, - width, -}: { +interface ImgProps { url: string; imgRef?: React.RefObject; onLoad?: () => void; width: number | string; -}) { - const { t } = useTranslation(); - const [phase, setPhase] = useState('loading'); - const [localUrl, setLocalUrl] = useState(''); - const [lastError, setLastError] = useState(null); - // ``-decode state. Independent of `phase` (which is about *fetching*). - // Visibility is gated on this so the spinner stays up until the browser - // has actually painted the image. - const [isImageReady, setIsImageReady] = useState(false); - - // Refs to keep `runCheck`'s identity stable and to let listeners read the - // latest values without subscribing. See review bugs #2 and #3. - const onLoadRef = useRef(onLoad); - const urlRef = useRef(url); - const phaseRef = useRef(phase); - const previousBlobUrlRef = useRef(''); - // In-flight cleanup tracking so we can cancel imperatively on unmount, - // URL change, or manual retry. See review bugs #4 and #5. - const controllersRef = useRef>(new Set()); - const timersRef = useRef>(new Set()); - - // Mirror props/state into refs every commit. Cheap and lets event handlers - // and async loops read the current value without re-creating closures. - useEffect(() => { - onLoadRef.current = onLoad; - }); - useEffect(() => { - urlRef.current = url; - }); - useEffect(() => { - phaseRef.current = phase; - }); - - const scheduleTimer = useCallback((cb: () => void, ms: number) => { - const id = window.setTimeout(() => { - timersRef.current.delete(id); - cb(); - }, ms); - - timersRef.current.add(id); - return id; - }, []); - - const cancelAllInflight = useCallback(() => { - controllersRef.current.forEach((c) => c.abort()); - controllersRef.current.clear(); - timersRef.current.forEach((id) => window.clearTimeout(id)); - timersRef.current.clear(); - }, []); - - const revokePreviousBlob = useCallback(() => { - if (previousBlobUrlRef.current && previousBlobUrlRef.current.startsWith('blob:')) { - URL.revokeObjectURL(previousBlobUrlRef.current); - previousBlobUrlRef.current = ''; - } - }, []); - - // Stable. Reads everything it needs through refs so it never has to be - // re-created when props change. - const runCheck = useCallback( - async (targetUrl: string) => { - // Anything left over from a previous run is now stale — abort and - // clear timers up front. - cancelAllInflight(); - setIsImageReady(false); - - if (!targetUrl) { - // No URL yet (YJS sync still in flight). Sit in `loading` until a - // real URL arrives and re-triggers the URL-change effect. - setPhase('loading'); - setLastError(null); - return; - } - - const controller = new AbortController(); - - controllersRef.current.add(controller); - - const startedAt = Date.now(); - let attempt = 0; - - setPhase('loading'); - setLastError(null); - - // Promote loading → pending if we cross the threshold without success. - scheduleTimer(() => { - if (controller.signal.aborted) return; - setPhase((p) => (p === 'loading' ? 'pending' : p)); - }, PENDING_AFTER_MS); - - // Cancellable delay between retries. Resolves early if aborted. - const sleep = (ms: number) => - new Promise((resolve) => { - const onAbort = () => { - window.clearTimeout(id); - timersRef.current.delete(id); - resolve(); - }; - - const id = scheduleTimer(() => { - controller.signal.removeEventListener('abort', onAbort); - resolve(); - }, ms); - - controller.signal.addEventListener('abort', onAbort, { once: true }); - }); - - // eslint-disable-next-line no-constant-condition - while (true) { - if (controller.signal.aborted) return; - - let result: CheckImageResult; - - try { - result = await checkImage(targetUrl, { - retry: attempt > 0, - signal: controller.signal, - }); - } catch (err) { - Log.warn('[Img] checkImage threw', err); - result = { - ok: false, - status: 0, - statusText: 'Exception', - errorKind: 'network', - }; - } - - if (controller.signal.aborted) { - // Revoke any blob URL that arrived after cancellation. - if (result.ok && result.validatedUrl?.startsWith('blob:')) { - URL.revokeObjectURL(result.validatedUrl); - } - - return; - } - - if (result.ok) { - const newUrl = result.validatedUrl || ''; - - if ( - previousBlobUrlRef.current && - previousBlobUrlRef.current !== newUrl && - previousBlobUrlRef.current.startsWith('blob:') - ) { - URL.revokeObjectURL(previousBlobUrlRef.current); - } - - previousBlobUrlRef.current = newUrl; - setLocalUrl(newUrl); - setLastError(null); - // Stay in 'loading' until fires onLoad and we flip - // isImageReady. That keeps the spinner up across the decode. - setPhase('loading'); - - controllersRef.current.delete(controller); - return; - } - - setLastError(result); - - const schedule = backoffSchedule(result.errorKind); - const exhausted = attempt >= schedule.length; - const elapsed = Date.now() - startedAt; - - if (exhausted || elapsed > MAX_BUDGET_MS) { - setPhase('failed'); - controllersRef.current.delete(controller); - return; - } - - await sleep(jitter(schedule[attempt])); - attempt += 1; - } - }, - [cancelAllInflight, scheduleTimer] - ); - - // Kick off (or restart) the check whenever the URL changes. - useEffect(() => { - void runCheck(url); - - return () => { - cancelAllInflight(); - revokePreviousBlob(); - }; - }, [url, runCheck, cancelAllInflight, revokePreviousBlob]); - - // Re-check on tab focus / visibility return — listeners attached ONCE. - useEffect(() => { - const onVisible = () => { - if (document.visibilityState !== 'visible') return; - if (phaseRef.current === 'failed' || phaseRef.current === 'pending') { - void runCheck(urlRef.current); - } - }; - - document.addEventListener('visibilitychange', onVisible); - window.addEventListener('focus', onVisible); - return () => { - document.removeEventListener('visibilitychange', onVisible); - window.removeEventListener('focus', onVisible); - }; - }, [runCheck]); - - const handleManualRetry = useCallback(() => { - void runCheck(urlRef.current); - }, [runCheck]); + /** + * Accessible label for the image. Should describe the content; pass an + * empty string only for purely decorative images. When omitted, falls + * back to a derived label from the URL filename, then a generic string. + */ + alt?: string; +} - const handleImageLoaded = useCallback(() => { - if (!previousBlobUrlRef.current) return; - setIsImageReady(true); - setLastError(null); - // Notify the parent once the image is actually painted, not just fetched. - onLoadRef.current?.(); - }, []); +/** + * Derive a human-ish alt label from a URL when none was provided. Picks the + * last path segment, strips the extension and percent-encoding, and trims + * any AppFlowy-internal prefix (e.g. "presigned_upload_"). The result + * is never empty — non-content images should explicitly pass `alt=""`. + */ +function fallbackAltFromUrl(url: string): string { + try { + const parsed = new URL(url, 'http://_'); + const segment = parsed.pathname.split('/').filter(Boolean).pop() ?? ''; + const decoded = decodeURIComponent(segment); + const withoutExt = decoded.replace(/\.[a-z0-9]+$/i, ''); + const cleaned = withoutExt + .replace(/^presigned_upload_/i, '') + .replace(/[-_]+/g, ' ') + .trim(); + + return cleaned.length > 0 ? cleaned : 'Embedded image'; + } catch { + return 'Embedded image'; + } +} - const handleImageError = useCallback(() => { - // The browser couldn't decode the bytes we gave it (rare — corrupted - // blob, format mismatch). Treat as a format error and stop retrying. - setIsImageReady(false); - setLastError({ - ok: false, - status: 0, - statusText: 'Decode failed', - errorKind: 'format', - }); - setPhase('failed'); - }, []); +function Img({ onLoad, imgRef, url, width, alt }: ImgProps) { + const { t } = useTranslation(); + const { src, phase, isImageReady, lastError, retry, onImageLoaded, onImageError } = + useImageWithRetry(url, { onReady: onLoad }); const showLoading = phase === 'loading' && !isImageReady; const showPending = phase === 'pending'; const showFailed = phase === 'failed'; + const resolvedAlt = alt ?? fallbackAltFromUrl(url); + return ( <> {''} {showLoading && (
- +
{lastError?.errorKind === 'forbidden' ? t('editor.imageNoAccess', 'You do not have access to this image') : t('editor.imageLoadFailed')}
- - {t('button.retry', 'Retry')} - + {t('button.retry', 'Retry')} )} diff --git a/src/components/editor/components/blocks/image/useImageWithRetry.ts b/src/components/editor/components/blocks/image/useImageWithRetry.ts new file mode 100644 index 000000000..b685272e9 --- /dev/null +++ b/src/components/editor/components/blocks/image/useImageWithRetry.ts @@ -0,0 +1,321 @@ +import { useCallback, useEffect, useRef, useState } from 'react'; + +import { checkImage, CheckImageErrorKind, CheckImageResult } from '@/utils/image'; +import { Log } from '@/utils/log'; + +/** + * Polling state machine. Note: this tracks the *fetch* lifecycle, not the + * rendered ``'s decode lifecycle — the ``'s native onLoad is what + * flips `isImageReady` independently. + */ +export type LoadPhase = 'loading' | 'pending' | 'failed'; + +/** + * Backoff schedules (ms). Total wall-clock budget is ~5 minutes — long + * enough to outlast a slow upload pipeline, short enough that the user + * notices when something is actually broken. + */ +const FAST_BACKOFF_MS = [500, 1000, 2000, 4000, 8000]; +const SLOW_BACKOFF_MS = [2000, 5000, 10000, 20000, 40000, 80000]; +const MAX_BUDGET_MS = 5 * 60 * 1000; + +/** + * After this elapsed time without success, the UI promotes "loading" + * to "pending" — a softer state that says "we're still waiting" rather + * than "this failed." Prevents users from interpreting a slow optimistic + * upload as a broken image. + */ +const PENDING_AFTER_MS = 10_000; + +function backoffSchedule(kind: CheckImageErrorKind | undefined): number[] { + switch (kind) { + case 'not-ready': + case 'no-auth': + case 'auth-rejected': + return FAST_BACKOFF_MS; + case 'forbidden': + return []; // terminal — don't retry + case 'not-found': + case 'server-error': + case 'network': + case 'format': + default: + return SLOW_BACKOFF_MS; + } +} + +function jitter(ms: number): number { + // ±20% jitter so a hundred clients don't synchronize retries on the same + // edge cache miss / origin warm-up. + return Math.round(ms * (0.8 + Math.random() * 0.4)); +} + +export interface UseImageWithRetryOptions { + /** + * Called once the `` has actually decoded and painted. Defers the + * notification until the bytes are visible, not just downloaded. + */ + onReady?: () => void; +} + +export interface UseImageWithRetryResult { + /** URL to put on the rendered ``. Empty until the fetch succeeds. */ + src: string; + /** Fetch-loop phase (independent of `` decode). */ + phase: LoadPhase; + /** True once the `` has actually painted. Use to gate visibility. */ + isImageReady: boolean; + /** Last error result, useful for distinguishing forbidden vs. generic failure. */ + lastError: CheckImageResult | null; + /** Force a re-fetch (e.g. from a manual "Retry" button). */ + retry: () => void; + /** Attach to the rendered ``'s onLoad. */ + onImageLoaded: () => void; + /** Attach to the rendered ``'s onError. */ + onImageError: () => void; +} + +/** + * Fetch an image URL with status-aware exponential backoff, surface a state + * machine the renderer can consume, and clean up after itself on unmount / + * URL change / manual retry. + * + * Why this is a hook, not inline in `Img.tsx`: + * - The state machine is non-trivial (3 phases × abort controllers × + * timers × refs to dodge stale closures). Keeping it outside the + * render function makes both the orchestration and the JSX readable + * in isolation. + * - Unit-testable with React Testing Library + a `fetch` mock; the JSX + * stays focused on accessibility / styling. + */ +export function useImageWithRetry( + url: string, + options: UseImageWithRetryOptions = {} +): UseImageWithRetryResult { + const [phase, setPhase] = useState('loading'); + const [src, setSrc] = useState(''); + const [lastError, setLastError] = useState(null); + const [isImageReady, setIsImageReady] = useState(false); + + // Refs let event handlers and async loops read the latest values + // without subscribing — keeps `runCheck`'s identity stable so its + // useEffect doesn't tear down on every parent render. + const onReadyRef = useRef(options.onReady); + const urlRef = useRef(url); + const phaseRef = useRef(phase); + const previousBlobUrlRef = useRef(''); + const controllersRef = useRef>(new Set()); + const timersRef = useRef>(new Set()); + + // Mirror current props/state into refs every commit. + useEffect(() => { + onReadyRef.current = options.onReady; + }); + useEffect(() => { + urlRef.current = url; + }); + useEffect(() => { + phaseRef.current = phase; + }); + + const scheduleTimer = useCallback((cb: () => void, ms: number) => { + const id = window.setTimeout(() => { + timersRef.current.delete(id); + cb(); + }, ms); + + timersRef.current.add(id); + return id; + }, []); + + const cancelAllInflight = useCallback(() => { + controllersRef.current.forEach((c) => c.abort()); + controllersRef.current.clear(); + timersRef.current.forEach((id) => window.clearTimeout(id)); + timersRef.current.clear(); + }, []); + + const revokePreviousBlob = useCallback(() => { + if (previousBlobUrlRef.current && previousBlobUrlRef.current.startsWith('blob:')) { + URL.revokeObjectURL(previousBlobUrlRef.current); + previousBlobUrlRef.current = ''; + } + }, []); + + const runCheck = useCallback( + async (targetUrl: string) => { + cancelAllInflight(); + setIsImageReady(false); + + if (!targetUrl) { + setPhase('loading'); + setLastError(null); + return; + } + + const controller = new AbortController(); + + controllersRef.current.add(controller); + + const startedAt = Date.now(); + let attempt = 0; + + setPhase('loading'); + setLastError(null); + + // Promote loading → pending if we cross the threshold without success. + scheduleTimer(() => { + if (controller.signal.aborted) return; + setPhase((p) => (p === 'loading' ? 'pending' : p)); + }, PENDING_AFTER_MS); + + // Cancellable delay between retries. Resolves early if aborted so we + // don't block on a 60-second sleep after the user navigates away. + const sleep = (ms: number) => + new Promise((resolve) => { + const onAbort = () => { + window.clearTimeout(id); + timersRef.current.delete(id); + resolve(); + }; + + const id = scheduleTimer(() => { + controller.signal.removeEventListener('abort', onAbort); + resolve(); + }, ms); + + controller.signal.addEventListener('abort', onAbort, { once: true }); + }); + + // eslint-disable-next-line no-constant-condition + while (true) { + if (controller.signal.aborted) return; + + let result: CheckImageResult; + + try { + result = await checkImage(targetUrl, { + retry: attempt > 0, + signal: controller.signal, + }); + } catch (err) { + Log.warn('[useImageWithRetry] checkImage threw', err); + result = { + ok: false, + status: 0, + statusText: 'Exception', + errorKind: 'network', + }; + } + + if (controller.signal.aborted) { + if (result.ok && result.validatedUrl?.startsWith('blob:')) { + URL.revokeObjectURL(result.validatedUrl); + } + + return; + } + + if (result.ok) { + const newUrl = result.validatedUrl || ''; + + if ( + previousBlobUrlRef.current && + previousBlobUrlRef.current !== newUrl && + previousBlobUrlRef.current.startsWith('blob:') + ) { + URL.revokeObjectURL(previousBlobUrlRef.current); + } + + previousBlobUrlRef.current = newUrl; + setSrc(newUrl); + setLastError(null); + // Stay in 'loading' until fires onLoad and we flip + // isImageReady. That keeps the spinner up across the decode. + setPhase('loading'); + controllersRef.current.delete(controller); + return; + } + + setLastError(result); + + const schedule = backoffSchedule(result.errorKind); + const exhausted = attempt >= schedule.length; + const elapsed = Date.now() - startedAt; + + if (exhausted || elapsed > MAX_BUDGET_MS) { + setPhase('failed'); + controllersRef.current.delete(controller); + return; + } + + await sleep(jitter(schedule[attempt])); + attempt += 1; + } + }, + [cancelAllInflight, scheduleTimer] + ); + + // Kick off (or restart) the check whenever the URL changes. + useEffect(() => { + void runCheck(url); + + return () => { + cancelAllInflight(); + revokePreviousBlob(); + }; + }, [url, runCheck, cancelAllInflight, revokePreviousBlob]); + + // Re-check when the tab regains focus and we're currently failed/pending. + // Attached ONCE; reads via refs so we don't churn listeners on every state change. + useEffect(() => { + const onVisible = () => { + if (document.visibilityState !== 'visible') return; + if (phaseRef.current === 'failed' || phaseRef.current === 'pending') { + void runCheck(urlRef.current); + } + }; + + document.addEventListener('visibilitychange', onVisible); + window.addEventListener('focus', onVisible); + return () => { + document.removeEventListener('visibilitychange', onVisible); + window.removeEventListener('focus', onVisible); + }; + }, [runCheck]); + + const retry = useCallback(() => { + void runCheck(urlRef.current); + }, [runCheck]); + + const onImageLoaded = useCallback(() => { + if (!previousBlobUrlRef.current) return; + setIsImageReady(true); + setLastError(null); + // Notify the parent once the bytes are visible, not just downloaded. + onReadyRef.current?.(); + }, []); + + const onImageError = useCallback(() => { + // The browser couldn't decode the bytes we gave it (rare — corrupted + // blob, format mismatch). Treat as a format error and stop retrying. + setIsImageReady(false); + setLastError({ + ok: false, + status: 0, + statusText: 'Decode failed', + errorKind: 'format', + }); + setPhase('failed'); + }, []); + + return { + src, + phase, + isImageReady, + lastError, + retry, + onImageLoaded, + onImageError, + }; +} From 31bc07acfe62c822865b051e93094200ed8f8006 Mon Sep 17 00:00:00 2001 From: Nathan Date: Thu, 14 May 2026 23:53:17 +0800 Subject: [PATCH 5/6] polish: address second-pass review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five small fixes from the React best-practices re-review: 1. Pending-timer false flash (#1): the loading→pending promotion timer could fire AFTER fetch success but BEFORE decode, briefly showing "Waiting for upload to finish…" when the upload was in fact finished. Track a local `settled` flag for each run; the timer short-circuits once settled. 2. Rename `previousBlobUrlRef` → `currentBlobUrlRef` (#2). It holds the current rendered blob URL, not a previous one — name now matches meaning. Pure cosmetics, no behavior change. 3. Smarter alt-text fallback (#3): AppFlowy storage URLs end in opaque hashes (file_id) that produce nonsense alts like "bGTk 4nxVz" — no better than empty alt for screen-reader users. Short-circuit those URLs to a generic "Image" via `isAppFlowyFileStorageUrl`. Also reject non-word-looking segments from external URLs (heuristic: needs a vowel and 3+ chars). 4. Collapse three ref-mirror useEffects into one `useLatest` helper (#4), matching Vercel's `advanced-use-latest` pattern. One effect per ref instead of three; reusable if more refs are needed later. 5. Simplify hook signature (#5): `useImageWithRetry(url, onReady?)` instead of `useImageWithRetry(url, { onReady })`. Drops the per-render options-object allocation and the indirection inside the hook. Lint, type-check, and 5 image E2E tests pass. --- .../editor/components/blocks/image/Img.tsx | 35 +++++--- .../blocks/image/useImageWithRetry.ts | 81 ++++++++++--------- 2 files changed, 67 insertions(+), 49 deletions(-) diff --git a/src/components/editor/components/blocks/image/Img.tsx b/src/components/editor/components/blocks/image/Img.tsx index aec2c3a0e..4d17c90d9 100644 --- a/src/components/editor/components/blocks/image/Img.tsx +++ b/src/components/editor/components/blocks/image/Img.tsx @@ -3,9 +3,12 @@ import { useTranslation } from 'react-i18next'; import { ReactComponent as ErrorOutline } from '@/assets/icons/error.svg'; import LoadingDots from '@/components/_shared/LoadingDots'; +import { isAppFlowyFileStorageUrl } from '@/utils/file-storage-url'; import { useImageWithRetry } from './useImageWithRetry'; +const GENERIC_ALT = 'Image'; + interface ImgProps { url: string; imgRef?: React.RefObject; @@ -20,32 +23,42 @@ interface ImgProps { } /** - * Derive a human-ish alt label from a URL when none was provided. Picks the - * last path segment, strips the extension and percent-encoding, and trims - * any AppFlowy-internal prefix (e.g. "presigned_upload_"). The result - * is never empty — non-content images should explicitly pass `alt=""`. + * Derive a human-ish alt label from a URL when none was provided. + * + * For AppFlowy storage URLs the trailing path segment is an opaque hash + * (file_id) — reading "bGTk 4nxVz" aloud is no better than empty alt, so + * we just use the generic label. For external URLs the trailing filename + * is usually human-chosen and worth surfacing. + * + * Returns a non-empty string so screen readers always announce something + * — callers must pass an explicit `alt=""` for genuinely decorative images. */ function fallbackAltFromUrl(url: string): string { + if (!url) return GENERIC_ALT; + if (isAppFlowyFileStorageUrl(url)) return GENERIC_ALT; + try { const parsed = new URL(url, 'http://_'); const segment = parsed.pathname.split('/').filter(Boolean).pop() ?? ''; const decoded = decodeURIComponent(segment); const withoutExt = decoded.replace(/\.[a-z0-9]+$/i, ''); - const cleaned = withoutExt - .replace(/^presigned_upload_/i, '') - .replace(/[-_]+/g, ' ') - .trim(); + const cleaned = withoutExt.replace(/[-_]+/g, ' ').trim(); + + // Reject runs of hex/base64-ish hash output even from non-AppFlowy URLs. + // A reasonable heuristic: needs at least one vowel and 3+ chars to be + // worth reading aloud. Otherwise fall back to the generic label. + const looksLikeWord = cleaned.length >= 3 && /[aeiou]/i.test(cleaned); - return cleaned.length > 0 ? cleaned : 'Embedded image'; + return looksLikeWord ? cleaned : GENERIC_ALT; } catch { - return 'Embedded image'; + return GENERIC_ALT; } } function Img({ onLoad, imgRef, url, width, alt }: ImgProps) { const { t } = useTranslation(); const { src, phase, isImageReady, lastError, retry, onImageLoaded, onImageError } = - useImageWithRetry(url, { onReady: onLoad }); + useImageWithRetry(url, onLoad); const showLoading = phase === 'loading' && !isImageReady; const showPending = phase === 'pending'; diff --git a/src/components/editor/components/blocks/image/useImageWithRetry.ts b/src/components/editor/components/blocks/image/useImageWithRetry.ts index b685272e9..c6afe753e 100644 --- a/src/components/editor/components/blocks/image/useImageWithRetry.ts +++ b/src/components/editor/components/blocks/image/useImageWithRetry.ts @@ -50,12 +50,18 @@ function jitter(ms: number): number { return Math.round(ms * (0.8 + Math.random() * 0.4)); } -export interface UseImageWithRetryOptions { - /** - * Called once the `` has actually decoded and painted. Defers the - * notification until the bytes are visible, not just downloaded. - */ - onReady?: () => void; +/** + * Holds the latest value of `value` in a ref. Lets event handlers and async + * loops read the current value without subscribing — keeping their identity + * stable across renders. Equivalent to Vercel's `advanced-use-latest` pattern. + */ +function useLatest(value: T) { + const ref = useRef(value); + + useEffect(() => { + ref.current = value; + }); + return ref; } export interface UseImageWithRetryResult { @@ -90,34 +96,26 @@ export interface UseImageWithRetryResult { */ export function useImageWithRetry( url: string, - options: UseImageWithRetryOptions = {} + onReady?: () => void ): UseImageWithRetryResult { const [phase, setPhase] = useState('loading'); const [src, setSrc] = useState(''); const [lastError, setLastError] = useState(null); const [isImageReady, setIsImageReady] = useState(false); - // Refs let event handlers and async loops read the latest values - // without subscribing — keeps `runCheck`'s identity stable so its - // useEffect doesn't tear down on every parent render. - const onReadyRef = useRef(options.onReady); - const urlRef = useRef(url); - const phaseRef = useRef(phase); - const previousBlobUrlRef = useRef(''); + // Refs let event handlers and async loops read the latest values without + // subscribing — keeps `runCheck`'s identity stable so its useEffect + // doesn't tear down on every parent render. + const onReadyRef = useLatest(onReady); + const urlRef = useLatest(url); + const phaseRef = useLatest(phase); + // The blob URL we're currently rendering. Tracked separately from `src` + // state because we need ref-stable access from the async loop in order + // to revoke it correctly on swap/unmount. + const currentBlobUrlRef = useRef(''); const controllersRef = useRef>(new Set()); const timersRef = useRef>(new Set()); - // Mirror current props/state into refs every commit. - useEffect(() => { - onReadyRef.current = options.onReady; - }); - useEffect(() => { - urlRef.current = url; - }); - useEffect(() => { - phaseRef.current = phase; - }); - const scheduleTimer = useCallback((cb: () => void, ms: number) => { const id = window.setTimeout(() => { timersRef.current.delete(id); @@ -136,9 +134,9 @@ export function useImageWithRetry( }, []); const revokePreviousBlob = useCallback(() => { - if (previousBlobUrlRef.current && previousBlobUrlRef.current.startsWith('blob:')) { - URL.revokeObjectURL(previousBlobUrlRef.current); - previousBlobUrlRef.current = ''; + if (currentBlobUrlRef.current && currentBlobUrlRef.current.startsWith('blob:')) { + URL.revokeObjectURL(currentBlobUrlRef.current); + currentBlobUrlRef.current = ''; } }, []); @@ -159,13 +157,17 @@ export function useImageWithRetry( const startedAt = Date.now(); let attempt = 0; + // Tracks whether this run has completed (success or terminal failure). + // The pending-promotion timer checks this so we don't briefly flash the + // "still uploading…" UI between fetch success and decode. + let settled = false; setPhase('loading'); setLastError(null); // Promote loading → pending if we cross the threshold without success. scheduleTimer(() => { - if (controller.signal.aborted) return; + if (controller.signal.aborted || settled) return; setPhase((p) => (p === 'loading' ? 'pending' : p)); }, PENDING_AFTER_MS); @@ -220,19 +222,20 @@ export function useImageWithRetry( const newUrl = result.validatedUrl || ''; if ( - previousBlobUrlRef.current && - previousBlobUrlRef.current !== newUrl && - previousBlobUrlRef.current.startsWith('blob:') + currentBlobUrlRef.current && + currentBlobUrlRef.current !== newUrl && + currentBlobUrlRef.current.startsWith('blob:') ) { - URL.revokeObjectURL(previousBlobUrlRef.current); + URL.revokeObjectURL(currentBlobUrlRef.current); } - previousBlobUrlRef.current = newUrl; + currentBlobUrlRef.current = newUrl; setSrc(newUrl); setLastError(null); // Stay in 'loading' until fires onLoad and we flip // isImageReady. That keeps the spinner up across the decode. setPhase('loading'); + settled = true; controllersRef.current.delete(controller); return; } @@ -245,6 +248,7 @@ export function useImageWithRetry( if (exhausted || elapsed > MAX_BUDGET_MS) { setPhase('failed'); + settled = true; controllersRef.current.delete(controller); return; } @@ -282,19 +286,20 @@ export function useImageWithRetry( document.removeEventListener('visibilitychange', onVisible); window.removeEventListener('focus', onVisible); }; - }, [runCheck]); + // Refs have stable identity, including them just satisfies the linter. + }, [runCheck, phaseRef, urlRef]); const retry = useCallback(() => { void runCheck(urlRef.current); - }, [runCheck]); + }, [runCheck, urlRef]); const onImageLoaded = useCallback(() => { - if (!previousBlobUrlRef.current) return; + if (!currentBlobUrlRef.current) return; setIsImageReady(true); setLastError(null); // Notify the parent once the bytes are visible, not just downloaded. onReadyRef.current?.(); - }, []); + }, [onReadyRef]); const onImageError = useCallback(() => { // The browser couldn't decode the bytes we gave it (rare — corrupted From 5c6b78ed9637a70db4b67ea3759add12bd1268e4 Mon Sep 17 00:00:00 2001 From: Nathan Date: Fri, 15 May 2026 08:40:16 +0800 Subject: [PATCH 6/6] test: stabilize duplicate row inline database edit --- .../database/duplicate-row-inline-db.spec.ts | 18 +++-- playwright/support/duplicate-test-helpers.ts | 77 +++++++++++++++++-- 2 files changed, 85 insertions(+), 10 deletions(-) diff --git a/playwright/e2e/database/duplicate-row-inline-db.spec.ts b/playwright/e2e/database/duplicate-row-inline-db.spec.ts index aeebb9bba..86de1d7ca 100644 --- a/playwright/e2e/database/duplicate-row-inline-db.spec.ts +++ b/playwright/e2e/database/duplicate-row-inline-db.spec.ts @@ -129,12 +129,20 @@ test.describe('Duplicate row with inline database', () => { const dupGridBlock = dbBlocks(dupEditor).first(); await expect(dupGridBlock).toBeVisible({ timeout: 30000 }); - // Edit the duplicated row's inline grid cell - await editCell(page, dupGridBlock, 'modified in duplicate'); - await page.waitForTimeout(1000); + // Ensure the inline grid has finished hydrating before we try to edit: + // at least one data row must be present, and its primary cell must not be + // showing the loading spinner. Without this gate, the edit can target a + // not-yet-mounted TextCell and never commit to the duplicated inline DB. + await expect(dupGridBlock.locator('[data-testid^="grid-cell-"]').first()).toBeVisible({ + timeout: 30000, + }); + await expect(dupGridBlock.locator('[data-testid^="primary-cell-loading-"]')).toHaveCount(0, { + timeout: 30000, + }); - // Verify the edit took effect - expect(await cellText(dupGridBlock)).toBe('modified in duplicate'); + // editCell asserts the cell text becomes the typed value before returning, + // so a separate re-check would be redundant. + await editCell(page, dupGridBlock, 'modified in duplicate'); // Navigate back to the grid await page.goBack(); diff --git a/playwright/support/duplicate-test-helpers.ts b/playwright/support/duplicate-test-helpers.ts index 38b028d15..9c9773646 100644 --- a/playwright/support/duplicate-test-helpers.ts +++ b/playwright/support/duplicate-test-helpers.ts @@ -440,11 +440,78 @@ export async function insertLinkedGridViaSlash( export async function editFirstGridCell(page: Page, gridBlock: Locator, text: string): Promise { const firstCell = gridBlock.locator('[data-testid^="grid-cell-"]').first(); await expect(firstCell).toBeVisible({ timeout: 15000 }); - await firstCell.click({ force: true }); - await page.waitForTimeout(300); - await page.keyboard.press(process.platform === 'darwin' ? 'Meta+A' : 'Control+A'); - await page.keyboard.type(text); - await page.keyboard.press('Enter'); + + // The grid-row-cell wrapper owns the activation click handler. Clicking the + // inner data-testid div relies on bubbling, which can be eaten on slow CI if + // the row is still hydrating, so assert the editor is actually mounted. + + // Wait for the primary cell's row data to finish hydrating. PrimaryCell.tsx + // renders a CircularProgress with testid `primary-cell-loading-` + // until the row's collab content is loaded; clicking before then has no + // effect because the editable TextCell isn't mounted yet. + await expect(firstCell.locator('[data-testid^="primary-cell-loading-"]')).toHaveCount(0, { + timeout: 30000, + }); + + // The activation click handler lives on the .grid-row-cell wrapper (see + // GridVirtualColumn.tsx). Resolve it directly via the DOM rather than an + // ancestor xpath. Playwright's `ancestor::` returns the outermost match + // even with `.first()`, which is the document root, not the wrapper. + const editingTextarea = firstCell.locator('textarea'); + + let entered = false; + + for (let attempt = 0; attempt < 4; attempt++) { + const handle = await firstCell.elementHandle(); + const wrapper = handle + ? await handle.evaluateHandle((el) => (el as HTMLElement).closest('.grid-row-cell') ?? el) + : null; + const wrapperElement = wrapper?.asElement(); + + if (wrapperElement) { + await wrapperElement.click({ force: true }); + } else { + await firstCell.click({ force: true }); + } + + if (await editingTextarea.isVisible().catch(() => false)) { + entered = true; + break; + } + + try { + await expect(editingTextarea).toBeVisible({ timeout: 3000 }); + entered = true; + break; + } catch { + // Some browsers / cell types only enter edit mode on Enter after focus, + // not on raw click. Try that next. + await page.keyboard.press('Enter').catch(() => undefined); + + if (await editingTextarea.isVisible({ timeout: 1500 }).catch(() => false)) { + entered = true; + break; + } + + await page.waitForTimeout(500); + } + } + + if (!entered) { + throw new Error( + 'First grid cell did not enter edit mode after click. Is the grid readOnly, ' + + 'still loading, or covered by another element?' + ); + } + + await editingTextarea.focus(); + await editingTextarea.fill(text); + await expect(editingTextarea).toHaveValue(text, { timeout: 5000 }); + await editingTextarea.press('Enter'); + // After Enter, the textarea unmounts and the cell re-renders with the new + // value. Wait for the textarea to be gone so the next innerText() reads the + // committed display text, not stale empty editing markup. + await expect(editingTextarea).toHaveCount(0, { timeout: 10000 }); await expect .poll(async () => firstGridCellText(gridBlock), { timeout: 15000,