From 408938bdba11897f6d58b6888a98d66e3a8aee61 Mon Sep 17 00:00:00 2001 From: Dotta Date: Thu, 14 May 2026 23:06:48 -0500 Subject: [PATCH] Polish document annotation layer updates Commit the Phase 4 review follow-up that replaces interval polling with observer-driven highlight refresh and adds regression coverage for the annotation panel/cache contract. Co-Authored-By: Paperclip --- ui/src/components/DocumentAnnotationLayer.tsx | 59 +++-- .../IssueDocumentAnnotations.test.tsx | 202 +++++++++++++++++- 2 files changed, 248 insertions(+), 13 deletions(-) diff --git a/ui/src/components/DocumentAnnotationLayer.tsx b/ui/src/components/DocumentAnnotationLayer.tsx index 5c32a4cfac8..b22a8d4137e 100644 --- a/ui/src/components/DocumentAnnotationLayer.tsx +++ b/ui/src/components/DocumentAnnotationLayer.tsx @@ -67,8 +67,6 @@ interface ToolbarPosition { left: number; } -const POSITION_REFRESH_INTERVAL_MS = 250; - export function DocumentAnnotationLayer({ containerRef, markdown, @@ -137,22 +135,59 @@ export function DocumentAnnotationLayer({ useEffect(() => { if (typeof window === "undefined") return; + const container = containerRef.current; + const overlay = overlayRef.current; let cancelled = false; + let frame: number | null = null; + const schedule = () => { - if (cancelled) return; - computeHighlightRects(); + if (cancelled || frame !== null) return; + frame = window.requestAnimationFrame(() => { + frame = null; + if (!cancelled) computeHighlightRects(); + }); }; - const interval = window.setInterval(schedule, POSITION_REFRESH_INTERVAL_MS); - const handleResize = () => schedule(); - window.addEventListener("resize", handleResize); - window.addEventListener("scroll", handleResize, true); + + const handleResizeOrScroll = () => schedule(); + window.addEventListener("resize", handleResizeOrScroll); + window.addEventListener("scroll", handleResizeOrScroll, true); + + const resizeObserver = typeof window.ResizeObserver === "function" + ? new window.ResizeObserver(schedule) + : null; + if (resizeObserver && container) resizeObserver.observe(container); + if (resizeObserver && overlay) resizeObserver.observe(overlay); + + const mutationObserver = typeof window.MutationObserver === "function" && container + ? new window.MutationObserver((mutations) => { + const layer = layerContainerRef.current; + const onlyLayerMutations = layer + ? mutations.every((mutation) => layer.contains(mutation.target)) + : false; + if (!onlyLayerMutations) schedule(); + }) + : null; + if (mutationObserver && container) { + mutationObserver.observe(container, { + childList: true, + characterData: true, + subtree: true, + attributes: true, + attributeFilter: ["class", "style", "data-state", "open", "hidden", "aria-expanded"], + }); + } + + schedule(); + return () => { cancelled = true; - window.clearInterval(interval); - window.removeEventListener("resize", handleResize); - window.removeEventListener("scroll", handleResize, true); + if (frame !== null) window.cancelAnimationFrame(frame); + resizeObserver?.disconnect(); + mutationObserver?.disconnect(); + window.removeEventListener("resize", handleResizeOrScroll); + window.removeEventListener("scroll", handleResizeOrScroll, true); }; - }, [computeHighlightRects]); + }, [computeHighlightRects, containerRef]); const captureSelection = useCallback((): PendingAnchor | null => { const container = containerRef.current; diff --git a/ui/src/components/IssueDocumentAnnotations.test.tsx b/ui/src/components/IssueDocumentAnnotations.test.tsx index 2f7d60024ce..87929a3fe1f 100644 --- a/ui/src/components/IssueDocumentAnnotations.test.tsx +++ b/ui/src/components/IssueDocumentAnnotations.test.tsx @@ -21,6 +21,14 @@ const mockAnnotationsApi = vi.hoisted(() => ({ updateStatus: vi.fn(), })); +const mockPendingAnchor = vi.hoisted(() => ({ + selector: { + quote: { exact: "should keep the editor", prefix: "We ", suffix: "." }, + position: { normalizedStart: 10, normalizedEnd: 32, markdownStart: 10, markdownEnd: 32 }, + }, + selectedText: "should keep the editor", +})); + vi.mock("@/api/document-annotations", () => ({ documentAnnotationsApi: mockAnnotationsApi, })); @@ -29,8 +37,45 @@ vi.mock("./MarkdownBody", () => ({ MarkdownBody: ({ children }: { children: string }) =>
{children}
, })); +vi.mock("@/components/ui/sheet", () => ({ + Sheet: ({ open, children }: { open: boolean; children: React.ReactNode }) => + open ?
{children}
: null, + SheetContent: ({ + children, + className, + side, + }: { + children: React.ReactNode; + className?: string; + side?: string; + }) => ( +
+ {children} +
+ ), + SheetTitle: ({ children, className }: { children: React.ReactNode; className?: string }) => ( +
{children}
+ ), +})); + vi.mock("./DocumentAnnotationLayer", () => ({ - DocumentAnnotationLayer: () => null, + DocumentAnnotationLayer: (props: { + newCommentDisabled?: boolean; + onPendingAnchorChange: (anchor: typeof mockPendingAnchor | null) => void; + onRequestComment: (anchor: typeof mockPendingAnchor) => void; + }) => ( + + ), })); // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -42,6 +87,12 @@ async function flush() { }); } +function setTextareaValue(textarea: HTMLTextAreaElement, value: string) { + const setter = Object.getOwnPropertyDescriptor(HTMLTextAreaElement.prototype, "value")?.set; + setter?.call(textarea, value); + textarea.dispatchEvent(new Event("input", { bubbles: true })); +} + function makeQueryClient() { return new QueryClient({ defaultOptions: { @@ -200,6 +251,7 @@ describe("IssueDocumentAnnotations", () => { const chip = container.querySelector('[data-testid="document-annotation-count-plan"]'); expect(chip).not.toBeNull(); expect(chip!.textContent).toContain("1"); + expect(mockAnnotationsApi.list).toHaveBeenCalledTimes(1); await act(async () => { (chip as HTMLButtonElement).click(); @@ -395,4 +447,152 @@ describe("IssueDocumentAnnotations", () => { expect(cta!.textContent).toMatch(/New comment on selection/i); expect(cta!.textContent).toMatch(/⌘⇧M/); }); + + it("creates a thread from a captured selection and refreshes the shared annotations query", async () => { + mockAnnotationsApi.list.mockResolvedValue([]); + mockAnnotationsApi.create.mockResolvedValue(makeThread({ id: "created-1" })); + const root = createRoot(container); + const queryClient = makeQueryClient(); + const doc = makeDoc(); + + await act(async () => { + root.render( + + + , + ); + }); + await flush(); + await flush(); + expect(mockAnnotationsApi.list).toHaveBeenCalledTimes(1); + + const selectButton = container.querySelector('[data-testid="mock-annotation-selection"]') as HTMLButtonElement | null; + expect(selectButton).not.toBeNull(); + await act(async () => { + selectButton!.click(); + }); + await flush(); + + const composer = container.querySelector('[data-testid="document-annotation-composer"]') as HTMLTextAreaElement | null; + expect(composer).not.toBeNull(); + await act(async () => { + setTextareaValue(composer!, "New anchored comment"); + }); + await flush(); + + const submit = Array.from(container.querySelectorAll("button")).find( + (button) => button.textContent === "Comment", + ); + expect(submit).not.toBeUndefined(); + await act(async () => { + submit!.click(); + }); + await flush(); + await flush(); + + expect(mockAnnotationsApi.create).toHaveBeenCalledWith("issue-1", "plan", { + baseRevisionId: "rev-4", + baseRevisionNumber: 4, + selector: mockPendingAnchor.selector, + body: "New anchored comment", + }); + expect(mockAnnotationsApi.list.mock.calls.length).toBeGreaterThan(1); + }); + + it("shows resolve and reopen actions and updates thread status", async () => { + mockAnnotationsApi.list.mockResolvedValue([ + makeThread({ id: "open-1" }), + makeThread({ id: "resolved-1", status: "resolved" }), + ]); + mockAnnotationsApi.updateStatus.mockResolvedValue(makeThread({ id: "open-1", status: "resolved" })); + const root = createRoot(container); + const queryClient = makeQueryClient(); + const doc = makeDoc(); + + await act(async () => { + root.render( + + + , + ); + }); + await flush(); + await flush(); + + const openThread = container.querySelector('[data-thread-id="open-1"]') as HTMLElement | null; + expect(openThread).not.toBeNull(); + await act(async () => openThread!.click()); + await flush(); + + const resolveButton = Array.from(container.querySelectorAll("button")).find( + (button) => /\bResolve\b/.test(button.textContent ?? ""), + ); + expect(resolveButton).not.toBeUndefined(); + await act(async () => resolveButton!.click()); + await flush(); + expect(mockAnnotationsApi.updateStatus).toHaveBeenCalledWith("issue-1", "plan", "open-1", "resolved"); + + const resolvedTab = Array.from(container.querySelectorAll("button")).find( + (button) => button.textContent?.startsWith("Resolved"), + ); + expect(resolvedTab).not.toBeUndefined(); + await act(async () => resolvedTab!.click()); + await flush(); + + const resolvedThread = container.querySelector('[data-thread-id="resolved-1"]') as HTMLElement | null; + expect(resolvedThread).not.toBeNull(); + await act(async () => resolvedThread!.click()); + await flush(); + + const reopenButton = Array.from(container.querySelectorAll("button")).find( + (button) => button.textContent?.includes("Reopen"), + ); + expect(reopenButton).not.toBeUndefined(); + await act(async () => reopenButton!.click()); + await flush(); + expect(mockAnnotationsApi.updateStatus).toHaveBeenCalledWith("issue-1", "plan", "resolved-1", "open"); + }); + + it("renders the mobile annotation panel through the sheet path", async () => { + const originalMatchMedia = window.matchMedia; + Object.defineProperty(window, "matchMedia", { + configurable: true, + value: vi.fn().mockImplementation((query: string) => ({ + matches: true, + media: query, + onchange: null, + addEventListener: vi.fn(), + removeEventListener: vi.fn(), + addListener: vi.fn(), + removeListener: vi.fn(), + dispatchEvent: vi.fn(), + })), + }); + mockAnnotationsApi.list.mockResolvedValue([makeThread()]); + const root = createRoot(container); + const queryClient = makeQueryClient(); + const doc = makeDoc(); + + try { + await act(async () => { + root.render( + + + , + ); + }); + await flush(); + await flush(); + + const sheet = container.querySelector('[data-slot="sheet-content"]'); + expect(sheet).not.toBeNull(); + expect(sheet?.getAttribute("data-side")).toBe("bottom"); + expect(sheet?.className).toContain("paperclip-doc-annotation-sheet"); + } finally { + Object.defineProperty(window, "matchMedia", { + configurable: true, + value: originalMatchMedia, + }); + } + }); });