diff --git a/docs/branch-review-records/e5aed82f89e308e20fae2728a7601d6f36d8d946da7b21e5adf1163a061a530a.record.md b/docs/branch-review-records/e5aed82f89e308e20fae2728a7601d6f36d8d946da7b21e5adf1163a061a530a.record.md new file mode 100644 index 0000000000..28bb732687 --- /dev/null +++ b/docs/branch-review-records/e5aed82f89e308e20fae2728a7601d6f36d8d946da7b21e5adf1163a061a530a.record.md @@ -0,0 +1 @@ +| 2026-08-20 | PR #2199 | 175c641321e58089d452ed76718972b8d7f705ca | PR #2199 on-demand document search — Codex P2 review-comment resolution | Fixed: the document-viewer visual-baseline target still declared .document-viewer-composer as a mask, but the composer is no longer rendered until Search document is pressed, and assertMaskSelectors fails loudly on a mask matching zero nodes — which classify-visual-baseline-outcome.mjs scores as a NON-drift failure, turning the advisory job hard red on every main push. Mask reduced to the sticky header, with the reasoning recorded in the target comment. Reviewed the rest of the diff: stale-response protection holds (closeDocumentSearch clears sourceSearch, whose effect cleanup aborts the in-flight controller before the reset run), focus restoration and Escape are covered by the Chromium journeys. | npm run verify:pr-local: check:runtime, check:installed-lock-parity, format:changed, lint, typecheck all passed; npm run test 692/693 files, 7592 passed, 4 skipped, 1 failed — tests/guard-push.test.ts 'inFlightCiGuard formats actionable blocked message', byte-identical to origin/main and failing only because gh is absent in this container so inFlightCiGuard fails open before the injected runFetcher is used; CI Unit coverage passed on this head. Browser proof delegated to CI Production UI: local Playwright pins chromium 1234, image ships 1194 (#255). | diff --git a/src/components/DocumentViewer.tsx b/src/components/DocumentViewer.tsx index 6b5ed4e146..b296a7f319 100644 --- a/src/components/DocumentViewer.tsx +++ b/src/components/DocumentViewer.tsx @@ -8,11 +8,11 @@ import { Download, Ellipsis, ExternalLink, - FilePlus2, FileText, Loader2, Search, Sparkles, + X, } from "lucide-react"; import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { documentDisplayTitle } from "@/components/DocumentOrganizationBadges"; @@ -149,7 +149,10 @@ export function DocumentViewer({ // change / successful reload so only an unrecoverable URL exhausts the budget. const signedUrlRefreshCountRef = useRef(0); const sourceSearchInputRef = useRef(null); + const documentActionsTriggerRef = useRef(null); + const documentSearchReturnFocusRef = useRef(null); const viewerRootRef = useRef(null); + const [documentSearchOpen, setDocumentSearchOpen] = useState(false); const [sourceSearch, setSourceSearch] = useState(""); const [documentSearchState, setDocumentSearchState] = useState<{ query: string; @@ -212,6 +215,7 @@ export function DocumentViewer({ mobileActionsOpen || sectionSheetOpen, composerChromeFocused, ); + const composerVisible = documentSearchOpen && !composerScrollHidden; const activeScrollOwner = useActiveScrollOwner(shellScrollContainer, documentId); // DocumentFrame owns every viewing control for the canvas PDF — there is one // reader, so there is one toolbar. Reset viewing chrome when the document @@ -228,6 +232,12 @@ export function DocumentViewer({ const [pdfPageCount, setPdfPageCount] = useState(null); if (pdfViewingDocumentId !== documentId) { setPdfViewingDocumentId(documentId); + setDocumentSearchOpen(false); + setSourceSearch(""); + setDocumentSearchState({ query: "", results: [] }); + setSearchingDocument(false); + setDocumentSearchError(null); + setInspectRevealKey(null); setPdfFitWidth(true); setPdfZoom(VIEWER_DEFAULT_ZOOM); setPdfViewingAid(false); @@ -319,6 +329,7 @@ export function DocumentViewer({ setIndexHealth(null); setSummary(null); setSummaryError(null); + setDocumentSearchOpen(false); setSourceSearch(""); setDocumentSearchState({ query: "", results: [] }); setDocumentSearchError(null); @@ -899,9 +910,6 @@ export function DocumentViewer({ ? "Document" : "Source unavailable"; const documentHomeHref = "/?mode=documents"; - const scopedDocumentHref = readyDocument - ? `/?mode=documents&q=${encodeURIComponent(documentDisplayTitle(readyDocument))}&documentId=${encodeURIComponent(documentId)}` - : documentHomeHref; const usefulPageHref = (page: number) => documentPageHref(documentId, page); const canSummarizeDocument = viewerState === "ready" && !loadingSummary && canUsePrivateApis; const summarizeTitle = !canUsePrivateApis @@ -1051,6 +1059,28 @@ export function DocumentViewer({ behavior: resolveScrollBehavior(), }); }; + const openDocumentSearch = (returnFocusTarget?: HTMLElement | null) => { + documentSearchReturnFocusRef.current = returnFocusTarget ?? documentActionsTriggerRef.current; + setMobileActionsOpen(false); + setDocumentSearchOpen(true); + window.requestAnimationFrame(() => { + window.requestAnimationFrame(() => sourceSearchInputRef.current?.focus()); + }); + }; + const closeDocumentSearch = () => { + const returnFocusTarget = documentSearchReturnFocusRef.current; + setDocumentSearchOpen(false); + setComposerChromeFocused(false); + setSourceSearch(""); + setDocumentSearchState({ query: "", results: [] }); + setSearchingDocument(false); + setDocumentSearchError(null); + setInspectRevealKey(null); + window.requestAnimationFrame(() => { + const focusTarget = returnFocusTarget?.isConnected ? returnFocusTarget : documentActionsTriggerRef.current; + focusTarget?.focus(); + }); + }; async function reviewTableFact(fact: TableFactRow, reviewClass: string) { setReviewingTableFactId(fact.id); try { @@ -1147,6 +1177,7 @@ export function DocumentViewer({
-
{canUseAdministrativeApis ? (
@@ -1310,14 +1325,14 @@ export function DocumentViewer({ - {readyDocument ? ( + {readyDocument && documentSearchOpen ? (
@@ -171,11 +176,13 @@ export function DocumentOverviewLanding({ {downloading ? "Preparing" : "Download"} - Add to scope + Search document { expect(heading).toBeVisible(); // Open the document actions sheet and verify the exact filename is visible. - // There are two "Open document actions" buttons (header and floating composer); - // click the first one (header button) to open the actions sheet. - const actionsButtons = screen.getAllByRole("button", { name: "Open document actions" }); - expect(actionsButtons).toHaveLength(2); - expect(actionsButtons[0]).toHaveAttribute("aria-expanded", "false"); - expect(screen.queryByRole("link", { name: "Add this document to scope" })).toBeNull(); - fireEvent.click(actionsButtons[0]); + const actionsButton = screen.getByRole("button", { name: "Open document actions" }); + expect(actionsButton).toHaveAttribute("aria-expanded", "false"); + fireEvent.click(actionsButton); expect(await screen.findByText("clozapine-titration.pdf")).toBeVisible(); - expect(actionsButtons[0]).toHaveAttribute("aria-expanded", "true"); + expect(actionsButton).toHaveAttribute("aria-expanded", "true"); + expect(screen.queryByRole("button", { name: "Add to scope" })).toBeNull(); // Close the sheet before teardown so focus-restore timers settle while jsdom // is still alive (avoids an unhandled post-test `document` ReferenceError @@ -212,7 +209,7 @@ describe("DocumentViewer — shell states", () => { expect(scrolledIds).not.toContain("pdf-preview-section"); }); - it("requires two characters and ignores an aborted search response after the query changes", async () => { + it("opens document search on demand, ignores stale responses, and clears it on close", async () => { const pendingSearches: Array<{ url: string; resolve: (response: Response) => void; @@ -229,7 +226,19 @@ describe("DocumentViewer — shell states", () => { ); render(); + expect(screen.queryByRole("textbox", { name: "Search within this document" })).toBeNull(); + expect(screen.queryByRole("button", { name: "Add to scope" })).toBeNull(); + + const searchTriggers = await screen.findAllByRole("button", { name: "Search document" }); + expect(searchTriggers).toHaveLength(2); + const searchTrigger = searchTriggers[1]!; + expect(searchTrigger).toHaveAttribute("aria-expanded", "false"); + expect(searchTrigger).not.toHaveAttribute("aria-controls"); + fireEvent.click(searchTrigger); const composerSearch = await screen.findByRole("textbox", { name: "Search within this document" }); + await waitFor(() => expect(composerSearch).toHaveFocus()); + expect(searchTrigger).toHaveAttribute("aria-expanded", "true"); + expect(searchTrigger).toHaveAttribute("aria-controls", "document-viewer-search"); fireEvent.change(composerSearch, { target: { value: "r" } }); expect(await screen.findByText("Enter at least 2 characters to search all indexed passages.")).toBeVisible(); @@ -281,6 +290,40 @@ describe("DocumentViewer — shell states", () => { expect(indexedTextPanel).not.toHaveTextContent("First query stale result"); expect(indexedTextPanel).toHaveTextContent("Second query current result"); }); + + fireEvent.change(composerSearch, { target: { value: "closing query" } }); + await waitFor(() => expect(pendingSearches).toHaveLength(3)); + fireEvent.click(screen.getByRole("button", { name: "Close document search" })); + expect(screen.queryByRole("textbox", { name: "Search within this document" })).toBeNull(); + expect(indexedTextPanel).not.toHaveTextContent("Second query current result"); + expect(searchTrigger).toHaveAttribute("aria-expanded", "false"); + await waitFor(() => expect(searchTrigger).toHaveFocus()); + + pendingSearches[2]?.resolve( + Response.json({ + query: "closing query", + results: [ + { + id: "closed-hit", + page_number: 3, + chunk_index: 2, + section_heading: "Closed result", + snippet: "A closed search must ignore this result", + matched_terms: ["closing", "query"], + image_ids: [], + score: 1, + }, + ], + }), + ); + await waitFor(() => expect(indexedTextPanel).not.toHaveTextContent("A closed search must ignore this result")); + + fireEvent.click(searchTrigger); + const reopenedSearch = await screen.findByRole("textbox", { name: "Search within this document" }); + expect(reopenedSearch).toHaveValue(""); + fireEvent.keyDown(reopenedSearch, { key: "Escape" }); + expect(screen.queryByRole("textbox", { name: "Search within this document" })).toBeNull(); + await waitFor(() => expect(searchTrigger).toHaveFocus()); }); // A private document's signed URL is a bearer link. The viewer holds the diff --git a/tests/helpers/phone-scroll.ts b/tests/helpers/phone-scroll.ts index f656ce9e77..813997b7f5 100644 --- a/tests/helpers/phone-scroll.ts +++ b/tests/helpers/phone-scroll.ts @@ -129,6 +129,7 @@ export const standalonePageOwnedFooterRoutes = [ name: "document composer", route: "/documents/11111111-1111-4111-8111-111111111111?page=1", selector: "form.document-viewer-composer", + openViaDocumentActions: true, focusSelector: 'input[placeholder="Search within this document..."]', reserveSelector: '[data-testid="document-viewer-content"]', flushBottom: false, @@ -137,6 +138,7 @@ export const standalonePageOwnedFooterRoutes = [ name: "differential comparison actions", route: "/differentials/presentations/acute-confusion-encephalopathy", selector: '[data-testid="differential-presentation-phone-footer"]', + openViaDocumentActions: false, focusSelector: null, reserveSelector: null, flushBottom: true, diff --git a/tests/ui-phone-scroll-page-owned.spec.ts b/tests/ui-phone-scroll-page-owned.spec.ts index 52fd965a49..f8a47c3ada 100644 --- a/tests/ui-phone-scroll-page-owned.spec.ts +++ b/tests/ui-phone-scroll-page-owned.spec.ts @@ -114,6 +114,11 @@ for (const phoneOwner of ["browser document", "standalone PWA main"] as const) { const content = page.getByTestId("document-viewer-content"); const sectionTrigger = page.getByTestId("document-section-trigger"); + await expect(composer).toHaveCount(0); + await expect(content).toHaveAttribute("data-phone-footer-owner", "none"); + await expect(content).toHaveAttribute("data-phone-composer-reserve", "0.75rem"); + await page.getByRole("button", { name: "Open document actions" }).click(); + await page.getByRole("dialog", { name: "This document" }).getByRole("button", { name: "Search document" }).click(); await expect(composer).toBeVisible({ timeout: 20_000 }); await expect(content).toHaveAttribute("data-phone-scroll-owner", expectedOwner); await expect(content).toHaveAttribute("data-phone-footer-owner", "document-viewer"); @@ -419,6 +424,13 @@ for (const footerCase of standalonePageOwnedFooterRoutes) { await page.setViewportSize(phoneViewport); await gotoPhoneSurface(page, footerCase.route, 112); + if (footerCase.openViaDocumentActions) { + await page.getByRole("button", { name: "Open document actions" }).click(); + await page + .getByRole("dialog", { name: "This document" }) + .getByRole("button", { name: "Search document" }) + .click(); + } const footer = page.locator(footerCase.selector); await expect(footer).toBeVisible({ timeout: 20_000 }); expect( diff --git a/tests/ui-smoke.spec.ts b/tests/ui-smoke.spec.ts index 16991c28a8..4c466b0333 100644 --- a/tests/ui-smoke.spec.ts +++ b/tests/ui-smoke.spec.ts @@ -4418,6 +4418,7 @@ test.describe("Clinical KB UI smoke coverage", () => { // The fixed document composer is the single search owner; the indexed-text // disclosure must not duplicate a large search field inside its content. + await page.getByRole("button", { name: "Search document" }).click(); const sourceSearch = page.getByRole("textbox", { name: "Search within this document" }); await expect(page.getByLabel("Search within indexed source text")).toHaveCount(0); await waitForReactEventHandler(sourceSearch, "onChange"); @@ -4441,6 +4442,9 @@ test.describe("Clinical KB UI smoke coverage", () => { const nextActiveHit = desktopTextPanel.locator('details[data-source-active-hit="true"]'); await expect(nextActiveHit).toHaveJSProperty("open", true); await expect(initialActiveDisclosure).toHaveJSProperty("open", false); + await page.getByRole("button", { name: "Close document search" }).click(); + await expect(sourceSearch).toHaveCount(0); + await expect(desktopTextPanel.getByText("Hit 2 of 2")).toHaveCount(0); await expectNoPageHorizontalOverflow(page); }); @@ -4712,8 +4716,12 @@ test.describe("Clinical KB UI smoke coverage", () => { const documentActions = page.getByRole("dialog", { name: "This document" }); await expect(documentActions).toBeVisible(); await expect(openDocumentActions).toHaveAttribute("aria-expanded", "true"); - await expect(documentActions.getByRole("button", { name: "Add to scope" })).toBeVisible(); + await expect(documentActions.getByRole("button", { name: "Add to scope" })).toHaveCount(0); + await documentActions.getByRole("button", { name: "Search document" }).click(); const composer = page.locator("form.document-viewer-composer"); + await expect(composer).toBeVisible(); + await openDocumentActions.click(); + await expect(documentActions).toBeVisible(); const composerBox = await composer.boundingBox(); expect(composerBox).not.toBeNull(); const sheetOwnsComposerPoint = await documentActions.evaluate( @@ -5168,7 +5176,10 @@ test.describe("Clinical KB UI smoke coverage", () => { await expect(page.getByRole("heading", { level: 1, name: "Synthetic lithium monitoring protocol" })).toBeVisible(); const composer = page.locator("form.document-viewer-composer"); + await page.getByRole("button", { name: "Open document actions" }).click(); + await page.getByRole("dialog", { name: "This document" }).getByRole("button", { name: "Search document" }).click(); await expect(composer).toBeVisible(); + await composer.locator("input").evaluate((element) => element.blur()); // The chunk deep link intentionally scrolls the highlighted passage into // view, which can initially hide the phone composer. Returning to the top // must restore it before the explicit hide-on-scroll checks below. @@ -5232,6 +5243,16 @@ test.describe("Clinical KB UI smoke coverage", () => { ) .toBeGreaterThan(250); await expect.poll(async () => readMobileComposerReservePx(main)).toBeLessThanOrEqual(13); + + await scrollPrimarySurface(page, 0); + await composer.getByRole("button", { name: "Close document search" }).click(); + await expect(composer).toHaveCount(0); + await expect(viewerContent).toHaveAttribute("data-phone-footer-owner", "none"); + await expect + .poll(async () => + viewerContent.evaluate((node) => Number.parseFloat(window.getComputedStyle(node).paddingBottom)), + ) + .toBeLessThanOrEqual(13); }); test("document search stays separate from the shared answer stream and summary action", async ({ page }) => { @@ -5258,12 +5279,18 @@ test.describe("Clinical KB UI smoke coverage", () => { ); const composer = page.locator("form.document-viewer-composer"); + await page.getByRole("button", { name: "Open document actions" }).click(); + await page.getByRole("dialog", { name: "This document" }).getByRole("button", { name: "Search document" }).click(); await composer.getByRole("textbox", { name: "Search within this document" }).fill("safety plan include"); await activateFocusedControl(page, composer.getByRole("button", { name: "Search within this document" })); await expect(page.getByTestId("source-chunk-indexed-text-panel").getByText("Hit 1 of 2").first()).toBeVisible(); expect(answerRequests).toEqual([]); - await composer.getByRole("button", { name: "Open document actions" }).click(); + const openDocumentActions = page.getByRole("button", { name: "Open document actions" }); + await composer.getByRole("button", { name: "Close document search" }).click(); + await expect(composer).toHaveCount(0); + await expect(openDocumentActions).toBeFocused(); + await openDocumentActions.click(); const documentActions = page.getByRole("dialog", { name: "This document" }); await documentActions.getByRole("button", { name: "Answer from this", exact: true }).click(); diff --git a/tests/ui-visual-baseline.spec.ts b/tests/ui-visual-baseline.spec.ts index 7eb8df3ddf..24680a2646 100644 --- a/tests/ui-visual-baseline.spec.ts +++ b/tests/ui-visual-baseline.spec.ts @@ -87,25 +87,37 @@ const targets: readonly BaselineTarget[] = [ selector: "#main-content", viewport: desktop, /** - * The document header and the search composer are viewport-pinned - * (`sm:sticky sm:top-0` and `sm:fixed` in `DocumentViewer.tsx`), and this - * target clips a ~2900px region against a 900px viewport. Playwright stitches - * an oversized element capture, so both land partway DOWN the image, overlap - * whatever content sits behind them at that offset, and move whenever the - * content above them changes height. That made an unrelated edit anywhere on - * the page redraw two bands of the golden and inflated every diff (#278). + * The document header is viewport-pinned (`sm:sticky sm:top-0` in + * `DocumentViewer.tsx`), and this target clips a ~2900px region against a + * 900px viewport. Playwright stitches an oversized element capture, so the + * header lands partway DOWN the image, overlaps whatever content sits behind + * it at that offset, and moves whenever the content above it changes height. + * That made an unrelated edit anywhere on the page redraw a band of the + * golden and inflated every diff (#278). * - * Masked rather than clipped away: both are real chrome that belongs in the + * Masked rather than clipped away: it is real chrome that belongs in the * frame, and narrowing the selector would drop the rail panels this target - * exists to watch. A mask is a hole in the gate, so it is limited to the two - * pinned elements — their own geometry is covered by the phone chrome - * contracts in `docs/search-chrome-behaviour.md`, not by this pixel gate. - * The header selector is the document-specific `data-document-sticky-header` - * attribute, not `.edge-glass-header`, because the universal search header - * also carries that class and would keep the fail-loud mask guard green after - * a DocumentViewer rename. + * exists to watch. A mask is a hole in the gate, so it is limited to that one + * pinned element — its own geometry is covered by the phone chrome contracts + * in `docs/search-chrome-behaviour.md`, not by this pixel gate. The selector + * is the document-specific `data-document-sticky-header` attribute, not + * `.edge-glass-header`, because the universal search header also carries that + * class and would keep the fail-loud mask guard green after a DocumentViewer + * rename. + * + * `.document-viewer-composer` was masked here for the same reason until + * document search became on-demand: the composer is now rendered only after + * "Search document" is pressed, so the default state this target captures has + * no composer at all. `assertMaskSelectors` fails loudly on a mask that + * matches nothing — deliberately, so a rename cannot silently stop masking — + * which would have turned this advisory job red on every run for a reason + * that has nothing to do with pixels. Nothing is lost: a masked region was + * never compared. The closed composer is now inside the golden, which is the + * state a reader actually lands on. If a target ever needs the open composer + * in frame, open it in that target's own `prepare` and re-declare the mask + * there. */ - mask: ["[data-document-sticky-header]", ".document-viewer-composer"], + mask: ["[data-document-sticky-header]"], prepare: async (page) => { const sectionIndex = page.getByTestId("document-section-index"); const sourceText = sectionIndex.getByRole("button", { name: /Indexed source text/ });