diff --git a/data/repo-awareness-snapshot.json b/data/repo-awareness-snapshot.json index b3c7d773a..f946d7e65 100644 --- a/data/repo-awareness-snapshot.json +++ b/data/repo-awareness-snapshot.json @@ -1,8 +1,8 @@ { "version": "repo-awareness-snapshot-v1", "captured_revision": { - "sha": "38fb26025fe7dfb4d007182c319769dd42ed861d", - "committed_at": "2026-08-26T08:43:32+00:00" + "sha": "ea58e0fcc734a79db3af625f800953333203f5bd", + "committed_at": "2026-08-26T22:38:53+08:00" }, "routes": { "modes": [ @@ -3819,6 +3819,22 @@ "outcome": "Whole-branch review: approved with findings, 0 P0, 0 P1, 7 P2 — all seven fixed and mutation-tested. Four earlier automated findings: three fixed, one rejected with reasons.", "checks": "npm run test (3 pre-existing failures, identical on clean origin/main); npm run typecheck; npm run lint (eslint cache cleared); prettier --check on all changed files; chromium-mockups ward-management + ward-coordinator + ward-discharges; screenshots at 390/820/1440 on five screens, looked at" }, + { + "date": "2026-08-26", + "ref": "codex/chat-image-preview-reliability-image-preview-reliability", + "head": "09ff8ca36489411f26408d6b557888459151bb4a", + "scope": "authenticated document cover selection, retry lifecycle, optional preview failure, and source evidence preservation", + "outcome": "All substantiated P2 findings fixed; no remaining P0-P2 findings", + "checks": "lint pass; typecheck pass; focused preview 79/79; production build pass; full suite 10489 pass with six unrelated Windows Claude-cloud failures after the one preview failure was fixed; offline fixture contracts pass; snapshot and medication checks pass" + }, + { + "date": "2026-08-26", + "ref": "codex/chat-image-preview-reliability-image-preview-reliability", + "head": "57ab01cbe28977ed3e0db830c45a00749977e351", + "scope": "PR #2391", + "outcome": "fixes-applied", + "checks": "PR policy body canonicalized; cover in-flight keyed by credential; vitest use-document-cover 9/9; snapshot regenerated; threads resolved; CI watching" + }, { "date": "2026-08-26", "ref": "codex/medication-risk-highlights", @@ -24789,8 +24805,8 @@ } ], "counts": { - "records": 2624, - "refs": 1603 + "records": 2626, + "refs": 1604 } } } diff --git a/docs/branch-review-records/04098ccb23c407505308b9e046181f8b978d85a4122db061e5b56a5f3cc0d988.record.md b/docs/branch-review-records/04098ccb23c407505308b9e046181f8b978d85a4122db061e5b56a5f3cc0d988.record.md new file mode 100644 index 000000000..80d6968ae --- /dev/null +++ b/docs/branch-review-records/04098ccb23c407505308b9e046181f8b978d85a4122db061e5b56a5f3cc0d988.record.md @@ -0,0 +1 @@ +| 2026-08-26 | codex/chat-image-preview-reliability-image-preview-reliability | 09ff8ca36489411f26408d6b557888459151bb4a | authenticated document cover selection, retry lifecycle, optional preview failure, and source evidence preservation | All substantiated P2 findings fixed; no remaining P0-P2 findings | lint pass; typecheck pass; focused preview 79/79; production build pass; full suite 10489 pass with six unrelated Windows Claude-cloud failures after the one preview failure was fixed; offline fixture contracts pass; snapshot and medication checks pass | diff --git a/docs/branch-review-records/a3ef569d0937ef0d619ee26c491cf0bac4c4d808d1597a7d1969c1cd5fe49762.record.md b/docs/branch-review-records/a3ef569d0937ef0d619ee26c491cf0bac4c4d808d1597a7d1969c1cd5fe49762.record.md new file mode 100644 index 000000000..e874dfcea --- /dev/null +++ b/docs/branch-review-records/a3ef569d0937ef0d619ee26c491cf0bac4c4d808d1597a7d1969c1cd5fe49762.record.md @@ -0,0 +1 @@ +| 2026-08-26 | codex/chat-image-preview-reliability-image-preview-reliability | 57ab01cbe28977ed3e0db830c45a00749977e351 | PR #2391 | fixes-applied | PR policy body canonicalized; cover in-flight keyed by credential; vitest use-document-cover 9/9; snapshot regenerated; threads resolved; CI watching | diff --git a/src/app/api/documents/[id]/cover/route.ts b/src/app/api/documents/[id]/cover/route.ts index 3d2f54c26..f722e16d1 100644 --- a/src/app/api/documents/[id]/cover/route.ts +++ b/src/app/api/documents/[id]/cover/route.ts @@ -5,7 +5,6 @@ import { rateLimitJsonResponse } from "@/lib/api-rate-limit"; import { demoImages } from "@/lib/demo-data"; import { isDemoMode } from "@/lib/env"; import { jsonError, PublicApiError, publicErrorResponse } from "@/lib/http"; -import { fetchDocumentCoverImageIds } from "@/lib/document-enrichment"; import { committedIndexGeneration, isCommittedGenerationMetadata } from "@/lib/reindex-pipeline"; import { parseRouteParams } from "@/lib/validation/params"; import { enforceDocumentReadRateLimit, withOwnerReadScope } from "@/lib/public-api-access"; @@ -16,6 +15,7 @@ export const runtime = "nodejs"; const coverRouteParamsSchema = z.object({ id: z.string().uuid() }); const coverImageIdSchema = z.string().uuid(); +const maxLegacyCoverCandidates = 64; /** * The document's first-page cover thumbnail id, for surfaces that show what a @@ -105,12 +105,32 @@ export async function GET(request: Request, { params }: { params: Promise<{ id: return NextResponse.json({ coverImageId: cover.id }); } - // Documents indexed before the pointer existed carry no such key. Fall back - // to the scan rather than drop their thumbnail: it is the same resolution - // the document search cards already use, and the signed-url route still - // re-checks ownership and committed generation before it hands anything out. - const covers = await fetchDocumentCoverImageIds(supabase, [id], request.signal); - return NextResponse.json({ coverImageId: covers.get(id) ?? null }); + // Documents indexed before the selected pointer existed carry no such key. + // Resolve their cover locally so the route never hands a staged/stale row + // to the signed-url route. The database predicates enforce document + kind; + // the defensive checks below preserve that invariant even if query data is + // malformed. A bounded ambiguous legacy result fails closed. + const committedGeneration = committedIndexGeneration(metadata); + const { data: candidates, error: candidatesError } = await supabase + .from("document_images") + .select("id,document_id,source_kind,metadata") + .eq("document_id", id) + .eq("source_kind", "cover_page") + .abortSignal(request.signal) + .limit(maxLegacyCoverCandidates + 1); + if (candidatesError) throw new Error(candidatesError.message); + + const boundedCandidates = candidates ?? []; + if (boundedCandidates.length > maxLegacyCoverCandidates) { + return NextResponse.json({ coverImageId: null }); + } + const committedCandidates = boundedCandidates.filter((candidate) => { + if (candidate.document_id !== id || candidate.source_kind !== "cover_page") return false; + if (!coverImageIdSchema.safeParse(candidate.id).success) return false; + const candidateGeneration = committedIndexGeneration(candidate.metadata); + return committedGeneration ? candidateGeneration === committedGeneration : candidateGeneration === null; + }); + return NextResponse.json({ coverImageId: committedCandidates.length === 1 ? committedCandidates[0].id : null }); } catch (error) { if (error instanceof AuthenticationError) return unauthorizedResponse(); if (error instanceof PublicApiError) return jsonError(error); diff --git a/src/components/clinical-dashboard/answer-source-drawer.tsx b/src/components/clinical-dashboard/answer-source-drawer.tsx index f0e25daa2..5835b0791 100644 --- a/src/components/clinical-dashboard/answer-source-drawer.tsx +++ b/src/components/clinical-dashboard/answer-source-drawer.tsx @@ -142,7 +142,7 @@ export function AnswerSourceDrawer({ const stale = source ? sourceRowIsStale(source) : false; // Hooks cannot be conditional, so this asks for the open source's cover on // every render and resolves to null while the drawer is closed. - const coverImageId = useDocumentCoverImageId(source?.documentId); + const { coverImageId, markCoverUnavailable } = useDocumentCoverImageId(source?.documentId); const numbered = sources.length <= NUMBERED_PAGER_LIMIT; return ( @@ -280,6 +280,8 @@ export function AnswerSourceDrawer({ className="rounded-lg border border-t-[3px] border-[color:var(--border-lux)] border-t-[color:var(--clinical-accent)] bg-[color:var(--surface)] shadow-[var(--shadow-inset)]" rootMargin="0px" priority + failurePresentation="hidden" + onSettledFailure={() => markCoverUnavailable(coverImageId)} />
Front page diff --git a/src/components/clinical-dashboard/signed-image.tsx b/src/components/clinical-dashboard/signed-image.tsx index 51c9c8bd5..724fefb13 100644 --- a/src/components/clinical-dashboard/signed-image.tsx +++ b/src/components/clinical-dashboard/signed-image.tsx @@ -89,6 +89,8 @@ export const SignedImage = memo(function SignedImage({ aspectRatio, priority = false, expandLabel, + failurePresentation = "message", + onSettledFailure, }: { /** Signed-URL API route, e.g. `/api/images/{id}/signed-url`. */ endpoint: string; @@ -128,6 +130,10 @@ export const SignedImage = memo(function SignedImage({ * unchanged. */ expandLabel?: string; + /** Settled failures remain visible by default; optional decoration may hide them. */ + failurePresentation?: "message" | "hidden"; + /** Called once when a failure remains after the bounded automatic retry cycle. */ + onSettledFailure?: (failure: SignedImageFailure) => void; }) { const [shouldLoad, setShouldLoad] = useState(() => priority || Boolean(getCachedSignedUrl(endpoint))); const [loaded, setLoaded] = useState(false); @@ -136,15 +142,18 @@ export const SignedImage = memo(function SignedImage({ const triggerRef = useRef(null); const [retryDisabled, setRetryDisabled] = useState(false); const [automaticRetryCount, setAutomaticRetryCount] = useState(0); + const notifiedSettledFailureRef = useRef(null); const [seenEndpoint, setSeenEndpoint] = useState(endpoint); if (endpoint !== seenEndpoint) { setSeenEndpoint(endpoint); setAutomaticRetryCount(0); setLoaded(false); + notifiedSettledFailureRef.current = null; } const { url, failed, failure, retry, markFailed } = useSignedImageUrl(endpoint, shouldLoad); const nextAutomaticRetryDelay = automaticRetryDelay(failure, automaticRetryCount); const automaticRetryPending = nextAutomaticRetryDelay !== null; + const settledFailure = failed && !automaticRetryPending ? failure : null; // Defer the request until the frame is near the viewport. A cached URL seeds // `shouldLoad` synchronously, so already-fetched images skip the observer. @@ -185,10 +194,17 @@ export const SignedImage = memo(function SignedImage({ }); }, [endpoint, nextAutomaticRetryDelay, retry]); + useEffect(() => { + if (!settledFailure || notifiedSettledFailureRef.current === settledFailure) return; + notifiedSettledFailureRef.current = settledFailure; + onSettledFailure?.(settledFailure); + }, [onSettledFailure, settledFailure]); + function retryImage() { if (retryDisabled) return; setRetryDisabled(true); setAutomaticRetryCount(0); + notifiedSettledFailureRef.current = null; setLoaded(false); setShouldLoad(true); retry(); @@ -200,7 +216,8 @@ export const SignedImage = memo(function SignedImage({ markFailed(); } - if (failed && !automaticRetryPending) { + if (settledFailure) { + if (failurePresentation === "hidden") return null; return (
{ setLoaded(true); setAutomaticRetryCount(0); + notifiedSettledFailureRef.current = null; }} onError={handleImageError} className={cn( diff --git a/src/components/clinical-dashboard/use-document-cover.ts b/src/components/clinical-dashboard/use-document-cover.ts index 5c5ea3034..9dfa98926 100644 --- a/src/components/clinical-dashboard/use-document-cover.ts +++ b/src/components/clinical-dashboard/use-document-cover.ts @@ -1,6 +1,8 @@ "use client"; -import { useEffect, useState } from "react"; +import { useCallback, useEffect, useState } from "react"; + +import { useAuthSession } from "@/lib/supabase/client"; /** * The first-page cover thumbnail id for a document, fetched on demand. @@ -10,10 +12,10 @@ import { useEffect, useState } from "react"; * editing retrieval hydration — a protected RAG surface, and far more blast * radius than a thumbnail earns. So the drawer asks for it when a source opens. * - * Cached per document for the page's lifetime, including the authoritative - * misses. A document with no cover is the common case for a text-only upload, - * and re-asking on every drawer open would spend a document-read rate-limit - * token each time to learn the same `null`. + * Cached per authenticated identity and document for the page's lifetime, + * including authoritative misses. A document with no cover is the common case + * for a text-only upload, and re-asking on every drawer open would spend a + * document-read rate-limit token each time to learn the same `null`. * * An authoritative miss is not the same as a failed lookup, and the first cut * of this cached both as `null`. A 429, a 5xx or an offline blip then pinned @@ -25,26 +27,49 @@ import { useEffect, useState } from "react"; const coverImageIds = new Map(); const inFlight = new Map>(); +function coverCacheKey(documentId: string, authIdentity: string | null) { + return JSON.stringify([authIdentity, documentId]); +} + +function coverInFlightKey( + documentId: string, + authIdentity: string | null, + authorizationHeader: Record, +) { + // Resolved answers stay keyed by user + document so a healthy cover survives + // token refresh. In-flight work must also include the credential: the same + // user can rotate a header while a request is pending, and reusing that + // promise would let a 401 settle as a miss after the new header is in use. + return JSON.stringify([authIdentity, documentId, authorizationHeader.Authorization ?? ""]); +} + /** `string`/`null` are answers and get cached; `undefined` is a transient failure. */ -async function loadCoverImageId(documentId: string): Promise { - const cached = coverImageIds.get(documentId); +async function loadCoverImageId( + key: string, + documentId: string, + authIdentity: string | null, + authorizationHeader: Record, +): Promise { + const cached = coverImageIds.get(key); if (cached !== undefined) return cached; - const pending = inFlight.get(documentId); + const requestKey = coverInFlightKey(documentId, authIdentity, authorizationHeader); + const pending = inFlight.get(requestKey); if (pending) return pending; const request = (async () => { try { - const response = await fetch(`/api/documents/${encodeURIComponent(documentId)}/cover`); + const response = await fetch(`/api/documents/${encodeURIComponent(documentId)}/cover`, { + headers: authorizationHeader, + }); // 404 is an answer: the document is gone or not ours to read. Anything // else non-ok (429, 5xx) is the server declining for now, not saying no. if (response.status === 404) return null; if (!response.ok) return undefined; const payload: unknown = await response.json(); - const value = - payload && typeof payload === "object" && "coverImageId" in payload - ? (payload as { coverImageId: unknown }).coverImageId - : null; - return typeof value === "string" && value.length > 0 ? value : null; + if (!payload || typeof payload !== "object" || !("coverImageId" in payload)) return undefined; + const value = (payload as { coverImageId: unknown }).coverImageId; + if (value === null) return null; + return typeof value === "string" && value.length > 0 ? value : undefined; } catch { // Offline, aborted, or unparseable. A cover is decoration for a citation, // never the citation itself, so this renders no thumbnail and changes @@ -53,44 +78,74 @@ async function loadCoverImageId(documentId: string): Promise void; +} { + const { authorizationHeader, session } = useAuthSession(); const id = documentId ?? null; + const authIdentity = session?.user?.id ?? null; + const key = id ? coverCacheKey(id, authIdentity) : null; /** * Reset happens during render, not in an effect. Clearing the previous - * document's answer from inside an effect renders one frame with the wrong - * cover attached to the new source — a picture of the last document beside - * this document's passage — and `react-hooks/set-state-in-effect` rejects the - * synchronous set that would cause it. The effect below only ever sets state - * from the resolved promise. + * identity/document answer from inside an effect renders one frame with the + * wrong cover attached to the new source — a picture from another document + * or account beside this document's passage. The effect below only ever sets + * state from the resolved promise. */ - const [renderedId, setRenderedId] = useState(id); + const [renderedKey, setRenderedKey] = useState(key); const [fetched, setFetched] = useState(null); - if (renderedId !== id) { - setRenderedId(id); + const [, notifyCacheChanged] = useState(0); + if (renderedKey !== key) { + setRenderedKey(key); setFetched(null); } useEffect(() => { - if (!id || coverImageIds.get(id) !== undefined) return; + if (!id || !key || coverImageIds.get(key) !== undefined) return; let active = true; - void loadCoverImageId(id).then((resolved) => { + void loadCoverImageId(key, id, authIdentity, authorizationHeader).then((resolved) => { if (active) setFetched(resolved ?? null); }); return () => { active = false; }; - }, [id]); + }, [authIdentity, authorizationHeader, id, key]); + + const markCoverUnavailable = useCallback( + (imageId: string) => { + // A settled image failure can report after the drawer has paged to a new + // document/account. The captured identity+document key and exact image + // guard ensure it can only invalidate the result that actually failed; + // a newer result for that key and every other account remain untouched. + if (!key || coverImageIds.get(key) !== imageId) return; + // This is not an authoritative "no cover" answer. The signed-url or + // object download may have failed transiently, or a reindex may have + // selected a replacement since this id was resolved. Hide the optional + // thumbnail for this mounted drawer, but discard the stale lookup so the + // next open can resolve the current cover instead of pinning a negative + // result for the rest of the page lifetime. + coverImageIds.delete(key); + setFetched(null); + // `coverImageIds` is external mutable state. `fetched` can already be + // null, so setting it to null is a React no-op; explicitly notify this + // hook so the deleted cached id is observed without refetching until a + // later mount/open runs the effect again. + notifyCacheChanged((revision) => revision + 1); + }, + [key], + ); - if (!id) return null; - const cached = coverImageIds.get(id); - return cached !== undefined ? cached : fetched; + if (!id || !key) return { coverImageId: null, markCoverUnavailable }; + const cached = coverImageIds.get(key); + return { coverImageId: cached !== undefined ? cached : fetched, markCoverUnavailable }; } /** Test-only reset for the process-local cover cache. */ diff --git a/tests/answer-source-marks.dom.test.tsx b/tests/answer-source-marks.dom.test.tsx index bc3039a19..d35c7e5d1 100644 --- a/tests/answer-source-marks.dom.test.tsx +++ b/tests/answer-source-marks.dom.test.tsx @@ -8,6 +8,13 @@ vi.mock("@/components/clinical-dashboard/signed-image", () => ({ SignedImage: ({ caption }: { caption?: string }) =>

{caption}

, })); +vi.mock("@/lib/supabase/client", () => ({ + useAuthSession: () => ({ + authorizationHeader: { Authorization: "Bearer cover-test" }, + session: { user: { id: "cover-test-user" } }, + }), +})); + import { AnswerSourceDrawer } from "@/components/clinical-dashboard/answer-source-drawer"; import { NaturalLanguageAnswer, primaryAnswerDisplayText } from "@/components/clinical-dashboard/answer-content"; import { type AnswerSourceRow } from "@/components/clinical-dashboard/answer-source-rows"; diff --git a/tests/answer-source-rail.dom.test.tsx b/tests/answer-source-rail.dom.test.tsx index 1cf8a5c41..d8c019065 100644 --- a/tests/answer-source-rail.dom.test.tsx +++ b/tests/answer-source-rail.dom.test.tsx @@ -4,8 +4,48 @@ import { render, screen, within } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +const coverAuth = vi.hoisted(() => ({ + authorizationHeader: { Authorization: "Bearer cover-test" }, + session: { user: { id: "cover-test-user" } }, +})); + +vi.mock("@/lib/supabase/client", () => ({ + // The real provider memoizes this value until credentials change. Keeping + // the mock stable prevents an ordinary state update from masquerading as a + // token refresh and starting a new cover lookup in the same open drawer. + useAuthSession: () => coverAuth, +})); + vi.mock("@/components/clinical-dashboard/signed-image", () => ({ - SignedImage: ({ caption, alt }: { caption?: string; alt?: string }) =>

{caption ?? alt}

, + SignedImage: ({ + caption, + alt, + failurePresentation, + onSettledFailure, + }: { + caption?: string; + alt?: string; + failurePresentation?: "message" | "hidden"; + onSettledFailure?: (failure: { + source: "response"; + status: number; + retryable: boolean; + retryAfterMs: null; + }) => void; + }) => ( + <> +

{caption ?? alt}

+ {failurePresentation === "hidden" && onSettledFailure ? ( + + ) : null} + + ), })); import { AnswerSupportSummaryCard } from "@/components/clinical-dashboard/evidence-panels"; @@ -607,4 +647,26 @@ describe("answer source drawer cover", () => { // passage down with it. expect(screen.getByTestId("answer-source-drawer-passage")).toBeInTheDocument(); }); + + it("removes only a failed optional cover while preserving source evidence and actions", async () => { + fetchMock.mockResolvedValue({ ok: true, json: async () => ({ coverImageId: "cover-1" }) }); + const user = userEvent.setup(); + render( + , + ); + await user.click(screen.getAllByTestId("answer-source-rail-row")[0]); + + const cover = await screen.findByTestId("answer-source-drawer-cover"); + await user.click(within(cover).getByTestId("settle-hidden-signed-image")); + + expect(screen.queryByTestId("answer-source-drawer-cover")).not.toBeInTheDocument(); + expect(screen.getByTestId("answer-source-drawer-passage")).toBeInTheDocument(); + expect(screen.getByTestId("answer-source-drawer-pager")).toBeInTheDocument(); + expect(screen.getByTestId("answer-source-drawer-menu-trigger")).toBeInTheDocument(); + expect(screen.getByRole("link", { name: "View original PDF" })).toBeInTheDocument(); + expect(screen.getByText("Ordinary evidence")).toBeInTheDocument(); + }); }); diff --git a/tests/document-cover-route.test.ts b/tests/document-cover-route.test.ts index e82108edb..46e856fce 100644 --- a/tests/document-cover-route.test.ts +++ b/tests/document-cover-route.test.ts @@ -3,12 +3,14 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; const documentId = "11111111-1111-4111-8111-111111111111"; const selectedCoverId = "22222222-2222-4222-8222-222222222222"; const stagedCoverId = "33333333-3333-4333-8333-333333333333"; +const committedCoverId = "66666666-6666-4666-8666-666666666666"; const committedGeneration = "44444444-4444-4444-8444-444444444444"; const stagedGeneration = "55555555-5555-4555-8555-555555555555"; type QueryCall = { table: string; selected?: string; + limit?: number; filters: Array<{ column: string; value: unknown }>; }; type QueryResult = { data: unknown; error: { message: string } | null }; @@ -35,6 +37,18 @@ class QueryBuilder { return this; } + limit(value: number) { + this.call.limit = value; + return this; + } + + then( + onfulfilled?: ((value: QueryResult) => TResult1 | PromiseLike) | null, + onrejected?: ((reason: unknown) => TResult2 | PromiseLike) | null, + ): Promise { + return Promise.resolve(this.resolve(this.call)).then(onfulfilled, onrejected); + } + maybeSingle() { return Promise.resolve(this.resolve(this.call)); } @@ -103,6 +117,155 @@ beforeEach(() => { }); describe("GET /api/documents/[id]/cover", () => { + it("selects the committed cover from legacy candidates instead of handing off a stale row", async () => { + const calls = setRouteData((call) => { + if (call.table === "documents") { + return { + data: { id: documentId, metadata: { index_generation_id: committedGeneration } }, + error: null, + }; + } + if (call.table === "document_images") { + return { + data: [ + { + id: stagedCoverId, + document_id: documentId, + source_kind: "cover_page", + metadata: { index_generation_id: stagedGeneration }, + }, + { + id: committedCoverId, + document_id: documentId, + source_kind: "cover_page", + metadata: { index_generation_id: committedGeneration }, + }, + { + id: selectedCoverId, + document_id: "99999999-9999-4999-8999-999999999999", + source_kind: "cover_page", + metadata: { index_generation_id: committedGeneration }, + }, + { + id: "77777777-7777-4777-8777-777777777777", + document_id: documentId, + source_kind: "figure", + metadata: { index_generation_id: committedGeneration }, + }, + ], + error: null, + }; + } + return { data: null, error: null }; + }); + + const response = await GET(request(), routeParams()); + + await expect(response.json()).resolves.toEqual({ coverImageId: committedCoverId }); + expect(calls.find((call) => call.table === "document_images")).toMatchObject({ + selected: "id,document_id,source_kind,metadata", + limit: 65, + filters: expect.arrayContaining([ + { column: "document_id", value: documentId }, + { column: "source_kind", value: "cover_page" }, + ]), + }); + expect(mocks.fetchDocumentCoverImageIds).not.toHaveBeenCalled(); + }); + + it("returns null for a legacy document when no committed cover candidate exists", async () => { + setRouteData((call) => { + if (call.table === "documents") { + return { + data: { id: documentId, metadata: { index_generation_id: committedGeneration } }, + error: null, + }; + } + if (call.table === "document_images") { + return { + data: [ + { + id: stagedCoverId, + document_id: documentId, + source_kind: "cover_page", + metadata: { index_generation_id: stagedGeneration }, + }, + ], + error: null, + }; + } + return { data: null, error: null }; + }); + + const response = await GET(request(), routeParams()); + + await expect(response.json()).resolves.toEqual({ coverImageId: null }); + }); + + it("fails closed when the bounded legacy query returns its 65-row overflow sentinel", async () => { + setRouteData((call) => { + if (call.table === "documents") { + return { + data: { id: documentId, metadata: { index_generation_id: committedGeneration } }, + error: null, + }; + } + if (call.table === "document_images") { + return { + data: Array.from({ length: 65 }, (_, index) => ({ + id: `88888888-8888-4888-8888-${index.toString(16).padStart(12, "0")}`, + document_id: documentId, + source_kind: "cover_page", + metadata: { index_generation_id: committedGeneration }, + })), + error: null, + }; + } + return { data: null, error: null }; + }); + + const response = await GET(request(), routeParams()); + + await expect(response.json()).resolves.toEqual({ coverImageId: null }); + expect(mocks.fetchDocumentCoverImageIds).not.toHaveBeenCalled(); + }); + + it("fails closed when two legacy candidates belong to the committed generation", async () => { + setRouteData((call) => { + if (call.table === "documents") { + return { + data: { id: documentId, metadata: { index_generation_id: committedGeneration } }, + error: null, + }; + } + if (call.table === "document_images") { + return { + data: [ + { + id: committedCoverId, + document_id: documentId, + source_kind: "cover_page", + metadata: { index_generation_id: committedGeneration }, + }, + { + id: selectedCoverId, + document_id: documentId, + source_kind: "cover_page", + metadata: { index_generation_id: committedGeneration }, + }, + ], + error: null, + }; + } + return { data: null, error: null }; + }); + + const response = await GET(request(), routeParams()); + + await expect(response.json()).resolves.toEqual({ coverImageId: null }); + expect(mocks.fetchDocumentCoverImageIds).not.toHaveBeenCalled(); + }); + it("returns only the selected cover after validating its document, kind, and committed generation", async () => { const calls = setRouteData((call) => { if (call.table === "documents") { diff --git a/tests/signed-image.dom.test.tsx b/tests/signed-image.dom.test.tsx index 0ddf7b35a..78281c0fb 100644 --- a/tests/signed-image.dom.test.tsx +++ b/tests/signed-image.dom.test.tsx @@ -81,6 +81,102 @@ describe("SignedImage failure/retry (jsdom)", () => { expect(fetchMock).toHaveBeenCalledTimes(1); }); + it("keeps the default settled fallback visible and reports it once", async () => { + const onSettledFailure = vi.fn(); + vi.stubGlobal( + "fetch", + vi.fn().mockResolvedValue({ ok: false, status: 404, json: async () => ({ error: "not found" }) }), + ); + + const { rerender } = render( + , + ); + + expect(await screen.findByText("Image preview failed.")).toBeInTheDocument(); + await waitFor(() => expect(onSettledFailure).toHaveBeenCalledTimes(1)); + expect(onSettledFailure).toHaveBeenCalledWith( + expect.objectContaining({ source: "response", status: 404, retryable: false }), + ); + + const replacementCallback = vi.fn(); + rerender( + , + ); + expect(onSettledFailure).toHaveBeenCalledTimes(1); + expect(replacementCallback).not.toHaveBeenCalled(); + }); + + it("hides the fallback and reports only after automatic retries are exhausted", async () => { + const onSettledFailure = vi.fn(); + const fetchMock = vi.fn().mockResolvedValue({ ok: false, status: 500, json: async () => ({ error: "boom" }) }); + vi.stubGlobal("fetch", fetchMock); + + render( + , + ); + + await waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(1)); + expect(onSettledFailure).not.toHaveBeenCalled(); + await waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(3), { timeout: 3_000 }); + await waitFor(() => expect(onSettledFailure).toHaveBeenCalledTimes(1)); + expect(screen.queryByText("Cover failed.")).not.toBeInTheDocument(); + expect(screen.queryByRole("status")).not.toBeInTheDocument(); + }); + + it("starts a new settled-failure callback cycle after manual retry", async () => { + const user = userEvent.setup(); + const onSettledFailure = vi.fn(); + const fetchMock = vi.fn().mockResolvedValue({ ok: false, status: 404, json: async () => ({ error: "not found" }) }); + vi.stubGlobal("fetch", fetchMock); + + render( + , + ); + + await screen.findByText("Image preview failed."); + await waitFor(() => expect(onSettledFailure).toHaveBeenCalledTimes(1)); + await user.click(screen.getByRole("button", { name: "Retry" })); + await waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(2)); + await waitFor(() => expect(onSettledFailure).toHaveBeenCalledTimes(2)); + }); + + it("starts a new settled-failure callback cycle when the endpoint changes", async () => { + const onSettledFailure = vi.fn(); + const fetchMock = vi.fn().mockResolvedValue({ ok: false, status: 404, json: async () => ({ error: "not found" }) }); + vi.stubGlobal("fetch", fetchMock); + + const { rerender } = render( + , + ); + await waitFor(() => expect(onSettledFailure).toHaveBeenCalledTimes(1)); + + rerender(); + await waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(2)); + await waitFor(() => expect(onSettledFailure).toHaveBeenCalledTimes(2)); + }); + it("honours Retry-After before automatically retrying a rate limit", async () => { const fetchMock = vi .fn() diff --git a/tests/use-document-cover.dom.test.tsx b/tests/use-document-cover.dom.test.tsx new file mode 100644 index 000000000..4745b34b1 --- /dev/null +++ b/tests/use-document-cover.dom.test.tsx @@ -0,0 +1,242 @@ +import { act, cleanup, renderHook, waitFor } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const auth = vi.hoisted(() => ({ + value: { + authorizationHeader: { Authorization: "Bearer token-a" } as Record, + session: { user: { id: "user-a" } } as { user: { id: string } } | null, + }, +})); + +vi.mock("@/lib/supabase/client", () => ({ + useAuthSession: () => auth.value, +})); + +import { + resetDocumentCoverCacheForTests, + useDocumentCoverImageId, +} from "@/components/clinical-dashboard/use-document-cover"; + +const DOCUMENT_ID = "11111111-1111-4111-8111-111111111111"; + +function coverResponse(coverImageId: string | null) { + return { ok: true, status: 200, json: async () => ({ coverImageId }) }; +} + +function deferred() { + let resolve!: (value: T) => void; + let reject!: (reason?: unknown) => void; + const promise = new Promise((accept, decline) => { + resolve = accept; + reject = decline; + }); + return { promise, reject, resolve }; +} + +function setAuth(userId: string | null, token: string) { + auth.value = { + authorizationHeader: { Authorization: `Bearer ${token}` }, + session: userId ? { user: { id: userId } } : null, + }; +} + +beforeEach(() => { + resetDocumentCoverCacheForTests(); + setAuth("user-a", "token-a"); +}); + +afterEach(() => { + cleanup(); + vi.unstubAllGlobals(); + resetDocumentCoverCacheForTests(); +}); + +describe("useDocumentCoverImageId", () => { + it("forwards the active authorization header", async () => { + const fetchMock = vi.fn().mockResolvedValue(coverResponse("cover-a")); + vi.stubGlobal("fetch", fetchMock); + + const { result } = renderHook(() => useDocumentCoverImageId(DOCUMENT_ID)); + + await waitFor(() => expect(result.current.coverImageId).toBe("cover-a")); + expect(fetchMock).toHaveBeenCalledWith(`/api/documents/${DOCUMENT_ID}/cover`, { + headers: { Authorization: "Bearer token-a" }, + }); + }); + + it("binds in-flight work and resolved values to the authenticated identity", async () => { + const first = deferred>(); + const second = deferred>(); + const fetchMock = vi.fn().mockReturnValueOnce(first.promise).mockReturnValueOnce(second.promise); + vi.stubGlobal("fetch", fetchMock); + + const { result, rerender } = renderHook(() => useDocumentCoverImageId(DOCUMENT_ID)); + await waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(1)); + + setAuth("user-b", "token-b"); + rerender(); + expect(result.current.coverImageId).toBeNull(); + await waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(2)); + + await act(async () => first.resolve(coverResponse("cover-a"))); + expect(result.current.coverImageId).toBeNull(); + + await act(async () => second.resolve(coverResponse("cover-b"))); + await waitFor(() => expect(result.current.coverImageId).toBe("cover-b")); + + setAuth("user-a", "token-a-refreshed"); + rerender(); + expect(result.current.coverImageId).toBe("cover-a"); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it("keeps a healthy cover through same-user token refresh without refetching", async () => { + const fetchMock = vi.fn().mockResolvedValue(coverResponse("cover-a")); + vi.stubGlobal("fetch", fetchMock); + const { result, rerender } = renderHook(() => useDocumentCoverImageId(DOCUMENT_ID)); + await waitFor(() => expect(result.current.coverImageId).toBe("cover-a")); + + setAuth("user-a", "token-a-refreshed"); + rerender(); + + expect(result.current.coverImageId).toBe("cover-a"); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("does not reuse an in-flight cover request after the same user rotates credentials", async () => { + const first = deferred<{ ok: boolean; status: number; json: () => Promise }>(); + const second = deferred>(); + const fetchMock = vi.fn().mockReturnValueOnce(first.promise).mockReturnValueOnce(second.promise); + vi.stubGlobal("fetch", fetchMock); + + const { result, rerender } = renderHook(() => useDocumentCoverImageId(DOCUMENT_ID)); + await waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(1)); + + setAuth("user-a", "token-a-refreshed"); + rerender(); + await waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(2)); + expect(fetchMock).toHaveBeenLastCalledWith(`/api/documents/${DOCUMENT_ID}/cover`, { + headers: { Authorization: "Bearer token-a-refreshed" }, + }); + + await act(async () => first.resolve({ ok: false, status: 401, json: async () => ({}) })); + expect(result.current.coverImageId).toBeNull(); + + await act(async () => second.resolve(coverResponse("cover-after-rotation"))); + await waitFor(() => expect(result.current.coverImageId).toBe("cover-after-rotation")); + }); + + it("caches an authoritative null for the current identity and document", async () => { + const payload = deferred<{ coverImageId: null }>(); + const json = vi.fn().mockReturnValue(payload.promise); + const fetchMock = vi.fn().mockResolvedValue({ ok: true, status: 200, json }); + vi.stubGlobal("fetch", fetchMock); + const first = renderHook(() => useDocumentCoverImageId(DOCUMENT_ID)); + await waitFor(() => expect(json).toHaveBeenCalledTimes(1)); + await act(async () => payload.resolve({ coverImageId: null })); + first.unmount(); + + const second = renderHook(() => useDocumentCoverImageId(DOCUMENT_ID)); + + expect(second.result.current.coverImageId).toBeNull(); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("does not cache a transient failure and retries on the next open", async () => { + const failedRequest = deferred>(); + const fetchMock = vi + .fn() + .mockReturnValueOnce(failedRequest.promise) + .mockResolvedValueOnce(coverResponse("cover-after-retry")); + vi.stubGlobal("fetch", fetchMock); + const { result, rerender } = renderHook( + ({ documentId }: { documentId: string | null }) => useDocumentCoverImageId(documentId), + { initialProps: { documentId: DOCUMENT_ID as string | null } }, + ); + await waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(1)); + await act(async () => failedRequest.reject(new Error("offline"))); + + rerender({ documentId: null }); + rerender({ documentId: DOCUMENT_ID }); + + await waitFor(() => expect(result.current.coverImageId).toBe("cover-after-retry")); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it("does not cache a malformed successful payload and retries on the next open", async () => { + const malformedPayload = deferred<{ unexpected: true }>(); + const malformedJson = vi.fn().mockReturnValue(malformedPayload.promise); + const fetchMock = vi + .fn() + .mockResolvedValueOnce({ ok: true, status: 200, json: malformedJson }) + .mockResolvedValueOnce(coverResponse("cover-after-malformed")); + vi.stubGlobal("fetch", fetchMock); + const { result, rerender } = renderHook( + ({ documentId }: { documentId: string | null }) => useDocumentCoverImageId(documentId), + { initialProps: { documentId: DOCUMENT_ID as string | null } }, + ); + await waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(1)); + await waitFor(() => expect(malformedJson).toHaveBeenCalledTimes(1)); + await act(async () => malformedPayload.resolve({ unexpected: true })); + + rerender({ documentId: null }); + rerender({ documentId: DOCUMENT_ID }); + + await waitFor(() => expect(result.current.coverImageId).toBe("cover-after-malformed")); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it("ignores stale image callbacks and callbacks from a previous identity", async () => { + const fetchMock = vi + .fn() + .mockResolvedValueOnce(coverResponse("cover-a")) + .mockResolvedValueOnce(coverResponse("cover-b")) + .mockResolvedValueOnce(coverResponse("cover-a-recovered")); + vi.stubGlobal("fetch", fetchMock); + const { result, rerender } = renderHook(() => useDocumentCoverImageId(DOCUMENT_ID)); + await waitFor(() => expect(result.current.coverImageId).toBe("cover-a")); + const markForUserA = result.current.markCoverUnavailable; + + act(() => result.current.markCoverUnavailable("another-cover")); + expect(result.current.coverImageId).toBe("cover-a"); + + setAuth("user-b", "token-b"); + rerender(); + await waitFor(() => expect(result.current.coverImageId).toBe("cover-b")); + act(() => markForUserA("cover-a")); + expect(result.current.coverImageId).toBe("cover-b"); + + setAuth("user-a", "token-a-refreshed"); + rerender(); + expect(result.current.coverImageId).toBeNull(); + await waitFor(() => expect(result.current.coverImageId).toBe("cover-a-recovered")); + + act(() => result.current.markCoverUnavailable("cover-a-recovered")); + expect(result.current.coverImageId).toBeNull(); + }); + + it("re-resolves a failed cover on the next open instead of pinning a negative cache entry", async () => { + const fetchMock = vi + .fn() + .mockResolvedValueOnce(coverResponse("stale-cover")) + .mockResolvedValueOnce(coverResponse("replacement-cover")); + vi.stubGlobal("fetch", fetchMock); + + const { result, rerender } = renderHook( + ({ documentId }: { documentId: string | null }) => useDocumentCoverImageId(documentId), + { initialProps: { documentId: DOCUMENT_ID as string | null } }, + ); + await waitFor(() => expect(result.current.coverImageId).toBe("stale-cover")); + + act(() => result.current.markCoverUnavailable("stale-cover")); + expect(result.current.coverImageId).toBeNull(); + // Invalidating the external cache must not create an immediate request + // loop while the failed optional image is still mounted. + expect(fetchMock).toHaveBeenCalledTimes(1); + + rerender({ documentId: null }); + rerender({ documentId: DOCUMENT_ID }); + await waitFor(() => expect(result.current.coverImageId).toBe("replacement-cover")); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); +});