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, 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 0a70d52da..4d17c90d9 100644 --- a/src/components/editor/components/blocks/image/Img.tsx +++ b/src/components/editor/components/blocks/image/Img.tsx @@ -1,183 +1,134 @@ -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 } from '@/utils/image'; +import { isAppFlowyFileStorageUrl } from '@/utils/file-storage-url'; -function Img({ - onLoad, - imgRef, - url, - width, -}: { +import { useImageWithRetry } from './useImageWithRetry'; + +const GENERIC_ALT = 'Image'; + +interface ImgProps { url: string; imgRef?: React.RefObject; onLoad?: () => void; width: number | string; -}) { - const { t } = useTranslation(); - const [loading, setLoading] = useState(true); - const [localUrl, setLocalUrl] = useState(''); - const [imgError, setImgError] = useState<{ - ok: boolean; - status: number; - statusText: string; - } | null>(null); - const previousBlobUrlRef = useRef(''); - const isMountedRef = useRef(true); - - const handleCheckImage = useCallback(async (url: string) => { - setLoading(true); - - // 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) - - let attempts = 0; - const startTime = Date.now(); - - const attemptCheck: () => Promise = async () => { - // Don't proceed if component is unmounted - if (!isMountedRef.current) { - return false; - } - - try { - const result = await checkImage(url); - - // 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:')) { - URL.revokeObjectURL(result.validatedUrl); - } - - return false; - } - - // 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. - */ - if ( - previousBlobUrlRef.current && - previousBlobUrlRef.current !== result.validatedUrl && - 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); - - return true; - } - - // Error case but continue polling if within limits - setImgError(result); - - // Check if we've exceeded our timeout or max attempts - attempts++; - const elapsedTime = Date.now() - startTime; - - 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; - } - - 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; - } + /** + * 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; +} - // Continue polling after interval - await new Promise((resolve) => setTimeout(resolve, pollingInterval)); - return await attemptCheck(); - } - }; +/** + * 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(/[-_]+/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 looksLikeWord ? cleaned : GENERIC_ALT; + } catch { + return GENERIC_ALT; + } +} - void attemptCheck(); - // eslint-disable-next-line - }, []); +function Img({ onLoad, imgRef, url, width, alt }: ImgProps) { + const { t } = useTranslation(); + const { src, phase, isImageReady, lastError, retry, onImageLoaded, onImageError } = + useImageWithRetry(url, onLoad); - useEffect(() => { - isMountedRef.current = true; - void handleCheckImage(url); + const showLoading = phase === 'loading' && !isImageReady; + const showPending = phase === 'pending'; + const showFailed = phase === 'failed'; - // 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 = ''; - } - }; - }, [handleCheckImage, url]); + const resolvedAlt = alt ?? fallbackAltFromUrl(url); return ( <> {''} { - setLoading(false); - setImgError(null); - }} + src={src} + alt={resolvedAlt} + onLoad={onImageLoaded} + onError={onImageError} + loading={'lazy'} + decoding={'async'} draggable={false} style={{ - visibility: imgError ? 'hidden' : 'visible', + visibility: isImageReady ? 'visible' : 'hidden', 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/components/editor/components/blocks/image/useImageWithRetry.ts b/src/components/editor/components/blocks/image/useImageWithRetry.ts new file mode 100644 index 000000000..c6afe753e --- /dev/null +++ b/src/components/editor/components/blocks/image/useImageWithRetry.ts @@ -0,0 +1,326 @@ +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)); +} + +/** + * 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 { + /** 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, + 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 = 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()); + + 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 (currentBlobUrlRef.current && currentBlobUrlRef.current.startsWith('blob:')) { + URL.revokeObjectURL(currentBlobUrlRef.current); + currentBlobUrlRef.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; + // 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 || settled) 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 ( + currentBlobUrlRef.current && + currentBlobUrlRef.current !== newUrl && + currentBlobUrlRef.current.startsWith('blob:') + ) { + URL.revokeObjectURL(currentBlobUrlRef.current); + } + + 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; + } + + 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'); + settled = true; + 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); + }; + // Refs have stable identity, including them just satisfies the linter. + }, [runCheck, phaseRef, urlRef]); + + const retry = useCallback(() => { + void runCheck(urlRef.current); + }, [runCheck, urlRef]); + + const onImageLoaded = useCallback(() => { + 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 + // 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, + }; +} diff --git a/src/utils/image.ts b/src/utils/image.ts index b78a4da14..84d041bfe 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; @@ -151,59 +175,123 @@ const validateImageBlob = async (blob: Blob, url?: string): Promise return transcodeIfUnsupported(normalizedBlob, url); }; -export const checkImage = async (url: string): Promise => { - // If it's an AppFlowy file storage URL, try authenticated fetch first +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; + /** + * 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 ( + url: string, + options: CheckImageOptions = {} +): Promise => { if (isAppFlowyFileStorageUrl(url)) { - const token = getTokenParsed(); - const fullUrl = resolveImageUrl(url); + return checkAppFlowyImage(url, options); + } - 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, + options: CheckImageOptions +): 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(); + + if (!token) { + // Token may still be hydrating from storage. Caller retries shortly. + return errorResult(401, 'No auth token', 'no-auth'); + } + + let response: Response; + + try { + response = await fetch(fullUrl, { + headers: { + Authorization: `Bearer ${token.access_token}`, + 'x-platform': 'web-app', + }, + // 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', + 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'); } - // Fallback for no token or failed fetch - return validateImageLoad(fullUrl); + Log.warn('[checkImage] auth fetch network error', err); + return errorResult(0, 'Network error', 'network', String(err)); } - // For non-AppFlowy URLs, use the original Image() approach - return validateImageLoad(url); -}; + 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)) {