From 353b5c807cf8d9c14d78aeadbe210d6e34ab71ae Mon Sep 17 00:00:00 2001 From: Anthony Stirling <77850077+Frooodle@users.noreply.github.com> Date: Thu, 4 Jun 2026 17:58:55 +0100 Subject: [PATCH] sort comments sidebar in visual reading order (#6439) (#6514) --- .../components/viewer/CommentsSidebar.tsx | 7 +- .../viewer/commentsSidebarOrder.test.ts | 101 +++++++ .../components/viewer/commentsSidebarOrder.ts | 37 +++ .../stubbed/comments-sidebar-order.spec.ts | 106 +++++++ .../annotations_out_of_order.pdf | 285 ++++++++++++++++++ 5 files changed, 533 insertions(+), 3 deletions(-) create mode 100644 frontend/editor/src/core/components/viewer/commentsSidebarOrder.test.ts create mode 100644 frontend/editor/src/core/components/viewer/commentsSidebarOrder.ts create mode 100644 frontend/editor/src/core/tests/stubbed/comments-sidebar-order.spec.ts create mode 100644 frontend/editor/src/core/tests/test-fixtures/annotations_out_of_order.pdf diff --git a/frontend/editor/src/core/components/viewer/CommentsSidebar.tsx b/frontend/editor/src/core/components/viewer/CommentsSidebar.tsx index 0c7cb6dfb..82e6d91b0 100644 --- a/frontend/editor/src/core/components/viewer/CommentsSidebar.tsx +++ b/frontend/editor/src/core/components/viewer/CommentsSidebar.tsx @@ -33,6 +33,7 @@ import { useViewer } from "@app/contexts/ViewerContext"; import { useToolWorkflow } from "@app/contexts/ToolWorkflowContext"; import { useAnnotation as useAnnotationContext } from "@app/contexts/AnnotationContext"; import LocalIcon from "@app/components/shared/LocalIcon"; +import { compareEntriesByVisualOrder } from "@app/components/viewer/commentsSidebarOrder"; const SIDEBAR_WIDTH = "18rem"; @@ -362,9 +363,9 @@ export function CommentsSidebar({ const all = getSidebarAnnotationsWithRepliesGroupedByPage(state) ?? {}; const filtered: typeof all = {}; for (const [page, entries] of Object.entries(all)) { - const commentEntries = entries.filter((e) => - isCommentAnnotation(e.annotation.object), - ); + const commentEntries = entries + .filter((e) => isCommentAnnotation(e.annotation.object)) + .sort(compareEntriesByVisualOrder); if (commentEntries.length > 0) { filtered[Number(page)] = commentEntries; } diff --git a/frontend/editor/src/core/components/viewer/commentsSidebarOrder.test.ts b/frontend/editor/src/core/components/viewer/commentsSidebarOrder.test.ts new file mode 100644 index 000000000..98d55d9a9 --- /dev/null +++ b/frontend/editor/src/core/components/viewer/commentsSidebarOrder.test.ts @@ -0,0 +1,101 @@ +import { describe, expect, it } from "vitest"; + +import { compareEntriesByVisualOrder } from "@app/components/viewer/commentsSidebarOrder"; + +function entry(id: string, x: number, y: number) { + return { + annotation: { + object: { + id, + rect: { origin: { x, y }, size: { width: 50, height: 12 } }, + }, + }, + }; +} + +function ids(list: Array<{ annotation: { object: { id: string } } }>): string { + return list.map((e) => e.annotation.object.id).join(","); +} + +describe("compareEntriesByVisualOrder", () => { + it("sorts a scrambled page back into top-to-bottom / left-to-right reading order", () => { + // EmbedPDF rects are viewport (top-left origin, y grows downward). + // Reading order on the page: A (top-left) → B (top-right, same row) + // → C (middle) → D (bottom). Storage order is D, C, A, B. + const A = entry("A", 100, 100); + const B = entry("B", 400, 100); + const C = entry("C", 100, 250); + const D = entry("D", 100, 400); + const out = [D, C, A, B].sort(compareEntriesByVisualOrder); + expect(ids(out)).toBe("A,B,C,D"); + }); + + it("sorts entries on the same row by x ascending", () => { + const left = entry("L", 100, 200); + const mid = entry("M", 250, 200); + const right = entry("R", 400, 200); + const out = [right, left, mid].sort(compareEntriesByVisualOrder); + expect(ids(out)).toBe("L,M,R"); + }); + + it("treats sub-pixel y differences as the same row", () => { + // Two annotations whose y differs by less than the SAME_ROW_EPSILON_PX + // tolerance should tie-break by x, not by the noisy y. + const a = entry("a", 100, 100.0); + const b = entry("b", 400, 100.3); + const out = [b, a].sort(compareEntriesByVisualOrder); + expect(ids(out)).toBe("a,b"); + }); + + it("places entries without a rect at the end and preserves their order", () => { + const A = entry("A", 100, 100); + const D = entry("D", 100, 400); + const noRectX = { annotation: { object: { id: "X" } } }; + const noRectY = { annotation: { object: { id: "Y" } } }; + const out = [noRectX, A, noRectY, D].sort(compareEntriesByVisualOrder); + expect(ids(out)).toBe("A,D,X,Y"); + }); + + it("returns 0 when both entries are missing a rect", () => { + const noRectX = { annotation: { object: { id: "X" } } }; + const noRectY = { annotation: { object: { id: "Y" } } }; + expect(compareEntriesByVisualOrder(noRectX, noRectY)).toBe(0); + expect(compareEntriesByVisualOrder(noRectY, noRectX)).toBe(0); + }); + + it("treats a missing rect origin as { x: 0, y: 0 }", () => { + const noOrigin = { + annotation: { object: { id: "noOrigin", rect: {} } }, + } as unknown as ReturnType; + const farDown = entry("farDown", 0, 999); + const out = [farDown, noOrigin].sort(compareEntriesByVisualOrder); + expect(ids(out)).toBe("noOrigin,farDown"); + }); + + it("handles the multi-page case by sorting each page independently", () => { + // Simulates the byPage useMemo: each page is sorted on its own array. + const pages: Record[]> = { + 1: [ + entry("p1-D", 100, 400), + entry("p1-B", 400, 100), + entry("p1-C", 100, 250), + entry("p1-A", 100, 100), + ], + 2: [ + entry("p2-B", 400, 100), + entry("p2-A", 100, 100), + entry("p2-C", 100, 250), + ], + 3: [entry("p3-A", 100, 100)], + }; + const sorted: Record = {}; + for (const [page, entries] of Object.entries(pages)) { + sorted[Number(page)] = ids(entries.sort(compareEntriesByVisualOrder)); + } + expect(sorted).toEqual({ + 1: "p1-A,p1-B,p1-C,p1-D", + 2: "p2-A,p2-B,p2-C", + 3: "p3-A", + }); + }); +}); diff --git a/frontend/editor/src/core/components/viewer/commentsSidebarOrder.ts b/frontend/editor/src/core/components/viewer/commentsSidebarOrder.ts new file mode 100644 index 000000000..9ecd614a2 --- /dev/null +++ b/frontend/editor/src/core/components/viewer/commentsSidebarOrder.ts @@ -0,0 +1,37 @@ +/** + * Compare two sidebar entries by visual position so the list matches the + * top-to-bottom / left-to-right reading order on the page. + * + * EmbedPDF exposes annotation rects in viewport coordinates (top-left origin, + * y grows downward), so the smaller `origin.y` comes first. A small epsilon on + * the y comparison treats items on the same row as a tie so they fall back to + * x-order. Entries without a rect sort to the end while preserving relative + * order (Array.prototype.sort is stable in modern engines). + */ + +export interface CommentEntryLike { + annotation: { + object: { + id?: string; + rect?: { + origin?: { x?: number; y?: number }; + }; + }; + }; +} + +const SAME_ROW_EPSILON_PX = 0.5; + +export function compareEntriesByVisualOrder( + a: CommentEntryLike, + b: CommentEntryLike, +): number { + const ra = a.annotation?.object?.rect; + const rb = b.annotation?.object?.rect; + if (!ra && !rb) return 0; + if (!ra) return 1; + if (!rb) return -1; + const dy = (ra.origin?.y ?? 0) - (rb.origin?.y ?? 0); + if (Math.abs(dy) > SAME_ROW_EPSILON_PX) return dy; + return (ra.origin?.x ?? 0) - (rb.origin?.x ?? 0); +} diff --git a/frontend/editor/src/core/tests/stubbed/comments-sidebar-order.spec.ts b/frontend/editor/src/core/tests/stubbed/comments-sidebar-order.spec.ts new file mode 100644 index 000000000..097b162c8 --- /dev/null +++ b/frontend/editor/src/core/tests/stubbed/comments-sidebar-order.spec.ts @@ -0,0 +1,106 @@ +import { test, expect } from "@app/tests/helpers/stub-test-base"; +import path from "path"; + +const ANNOTATED_PDF = path.join( + __dirname, + "../test-fixtures/annotations_out_of_order.pdf", +); + +/** + * Regression test for https://github.com/Stirling-Tools/Stirling-PDF/issues/6439 + * + * The fixture PDF (3 pages) has 4 text annotations per page stored in + * deliberately scrambled object order (D, B, C, A). With the sort applied, + * the Comments sidebar must list them in top-to-bottom / left-to-right + * reading order on every page: A, B, C, D. + */ +test.describe("Comments sidebar - annotation reading order", () => { + test("annotations on each page are listed in visual reading order", async ({ + page, + }) => { + await page.goto("/read"); + await page.waitForLoadState("domcontentloaded"); + + // Upload the test fixture via the hidden file input (avoids the native + // file picker dialog). + await page + .locator('[data-testid="file-input"]') + .first() + .setInputFiles(ANNOTATED_PDF); + + // Sanity-check that the page indicator reflects a 3-page document. + await expect(page.getByText(/\/\s*3/)).toBeVisible({ timeout: 30_000 }); + + // Open the Comments sidebar via the WorkbenchBar button. + const commentsBtn = page + .getByRole("button", { name: /^Comments$/i }) + .first(); + if ( + !(await commentsBtn.isVisible({ timeout: 10_000 }).catch(() => false)) + ) { + test.skip(true, "Comments button not visible on this build"); + return; + } + await commentsBtn.click(); + + // Pull the rendered order out of the sidebar. Each comment card is + // identified by `data-comment-card="${pageIndex}_${annotationId}"`. + // The annotation `contents` text (e.g. "Annotation A on page 1") is + // rendered inside the card and is the most reliable label to read. + await expect + .poll( + async () => { + return await page.evaluate(() => { + const cards = Array.from( + document.querySelectorAll("[data-comment-card]"), + ); + return cards + .map((c) => { + const key = c.getAttribute("data-comment-card") || ""; + const pageIndex = Number(key.split("_")[0]); + const text = (c.innerText || "").replace(/\s+/g, " "); + const m = text.match( + /Annotation\s+([ABCD])\s+on\s+page\s+(\d+)/i, + ); + return m + ? { pageIndex, label: m[1].toUpperCase(), text } + : { pageIndex, label: "?", text: text.slice(0, 80) }; + }) + .filter((e) => e.label !== "?"); + }); + }, + { timeout: 30_000, message: "waiting for 12 comment cards" }, + ) + .toHaveLength(12); + + const entries = await page.evaluate(() => { + const cards = Array.from( + document.querySelectorAll("[data-comment-card]"), + ); + return cards + .map((c) => { + const key = c.getAttribute("data-comment-card") || ""; + const pageIndex = Number(key.split("_")[0]); + const text = (c.innerText || "").replace(/\s+/g, " "); + const m = text.match(/Annotation\s+([ABCD])\s+on\s+page\s+(\d+)/i); + return m && pageIndex === Number(m[2]) - 1 + ? { pageIndex, label: m[1].toUpperCase() } + : null; + }) + .filter((e): e is { pageIndex: number; label: string } => e !== null); + }); + + // Group labels by page in the order they appear in the sidebar. + const byPage: Record = {}; + for (const { pageIndex, label } of entries) { + (byPage[pageIndex] ||= []).push(label); + } + + // Each of the 3 pages must show A, B, C, D in that order. + expect(byPage).toEqual({ + 0: ["A", "B", "C", "D"], + 1: ["A", "B", "C", "D"], + 2: ["A", "B", "C", "D"], + }); + }); +}); diff --git a/frontend/editor/src/core/tests/test-fixtures/annotations_out_of_order.pdf b/frontend/editor/src/core/tests/test-fixtures/annotations_out_of_order.pdf new file mode 100644 index 000000000..8abeb18f7 --- /dev/null +++ b/frontend/editor/src/core/tests/test-fixtures/annotations_out_of_order.pdf @@ -0,0 +1,285 @@ +%PDF-1.3 +%âãÏÓ +1 0 obj +<< +/PageMode /UseNone +/Pages 2 0 R +/Type /Catalog +>> +endobj +2 0 obj +<< +/Count 3 +/Kids [ 3 0 R 8 0 R 10 0 R ] +/Type /Pages +>> +endobj +3 0 obj +<< +/Contents 4 0 R +/MediaBox [ 0 0 612 792 ] +/Parent 2 0 R +/Resources << +/Font 5 0 R +/ProcSet [ /PDF /Text /ImageB /ImageC /ImageI ] +>> +/Rotate 0 +/Trans << +>> +/Type /Page +/Annots [ 13 0 R 14 0 R 15 0 R 16 0 R ] +>> +endobj +4 0 obj +<< +/Filter [ /ASCII85Decode /FlateDecode ] +/Length 202 +>> +stream +Gasc=3tHos'F!Ec?ZAOh&UQ#o*6#tZi,D1%Q)C=AR/[+f8tCS[9WW$NBF!'qbR)!j@'#^38Q_"QI*7quN4(FLQ.GdUWCN)r43J.(@;D;^Xe( +endstream +endobj +5 0 obj +<< +/F1 6 0 R +/F2 7 0 R +>> +endobj +6 0 obj +<< +/BaseFont /Helvetica +/Encoding /WinAnsiEncoding +/Name /F1 +/Subtype /Type1 +/Type /Font +>> +endobj +7 0 obj +<< +/BaseFont /Helvetica-Bold +/Encoding /WinAnsiEncoding +/Name /F2 +/Subtype /Type1 +/Type /Font +>> +endobj +8 0 obj +<< +/Contents 9 0 R +/MediaBox [ 0 0 612 792 ] +/Parent 2 0 R +/Resources << +/Font 5 0 R +/ProcSet [ /PDF /Text /ImageB /ImageC /ImageI ] +>> +/Rotate 0 +/Trans << +>> +/Type /Page +/Annots [ 17 0 R 18 0 R 19 0 R 20 0 R ] +>> +endobj +9 0 obj +<< +/Filter [ /ASCII85Decode /FlateDecode ] +/Length 202 +>> +stream +Gasc=3tHos'F!Ec?ZAOh&UQ#o*6#tZi,D1%Q)C=AR/[+f8tCS[9WW$NBF!'p&Ik"^_0&I-o!H#fM'Mp]E;/k\f2lV9&f&2nGEs;/VRhVG;T/Wa7sfV3.NC93%2=D6_;Vk>'SkZnYb_],0V]=U~> +endstream +endobj +10 0 obj +<< +/Contents 11 0 R +/MediaBox [ 0 0 612 792 ] +/Parent 2 0 R +/Resources << +/Font 5 0 R +/ProcSet [ /PDF /Text /ImageB /ImageC /ImageI ] +>> +/Rotate 0 +/Trans << +>> +/Type /Page +/Annots [ 21 0 R 22 0 R 23 0 R 24 0 R ] +>> +endobj +11 0 obj +<< +/Filter [ /ASCII85Decode /FlateDecode ] +/Length 204 +>> +stream +Gasc=3tHos'F!Ec?ZAOh&UOmO*6#tZi,D1%Q%p2kM +endstream +endobj +12 0 obj +<< +/Author (anonymous) +/CreationDate (D\07220260601133935\05301\04700\047) +/Creator (ReportLab PDF Library \055 www\056reportlab\056com) +/Keywords () +/ModDate (D\07220260601133935\05301\04700\047) +/Producer (ReportLab PDF Library \055 www\056reportlab\056com) +/Subject (unspecified) +/Title (untitled) +/Trapped /False +>> +endobj +13 0 obj +<< +/Subtype /Text +/Rect [ 100 286 116 308 ] +/Contents (Annotation D on page 1) +/Open false +/Flags 0 +/P 3 0 R +>> +endobj +14 0 obj +<< +/Subtype /Text +/Rect [ 380 686 396 708 ] +/Contents (Annotation B on page 1) +/Open false +/Flags 0 +/P 3 0 R +>> +endobj +15 0 obj +<< +/Subtype /Text +/Rect [ 100 486 116 508 ] +/Contents (Annotation C on page 1) +/Open false +/Flags 0 +/P 3 0 R +>> +endobj +16 0 obj +<< +/Subtype /Text +/Rect [ 100 686 116 708 ] +/Contents (Annotation A on page 1) +/Open false +/Flags 0 +/P 3 0 R +>> +endobj +17 0 obj +<< +/Subtype /Text +/Rect [ 100 286 116 308 ] +/Contents (Annotation D on page 2) +/Open false +/Flags 0 +/P 8 0 R +>> +endobj +18 0 obj +<< +/Subtype /Text +/Rect [ 380 686 396 708 ] +/Contents (Annotation B on page 2) +/Open false +/Flags 0 +/P 8 0 R +>> +endobj +19 0 obj +<< +/Subtype /Text +/Rect [ 100 486 116 508 ] +/Contents (Annotation C on page 2) +/Open false +/Flags 0 +/P 8 0 R +>> +endobj +20 0 obj +<< +/Subtype /Text +/Rect [ 100 686 116 708 ] +/Contents (Annotation A on page 2) +/Open false +/Flags 0 +/P 8 0 R +>> +endobj +21 0 obj +<< +/Subtype /Text +/Rect [ 100 286 116 308 ] +/Contents (Annotation D on page 3) +/Open false +/Flags 0 +/P 10 0 R +>> +endobj +22 0 obj +<< +/Subtype /Text +/Rect [ 380 686 396 708 ] +/Contents (Annotation B on page 3) +/Open false +/Flags 0 +/P 10 0 R +>> +endobj +23 0 obj +<< +/Subtype /Text +/Rect [ 100 486 116 508 ] +/Contents (Annotation C on page 3) +/Open false +/Flags 0 +/P 10 0 R +>> +endobj +24 0 obj +<< +/Subtype /Text +/Rect [ 100 686 116 708 ] +/Contents (Annotation A on page 3) +/Open false +/Flags 0 +/P 10 0 R +>> +endobj +xref +0 25 +0000000000 65535 f +0000000015 00000 n +0000000083 00000 n +0000000155 00000 n +0000000384 00000 n +0000000677 00000 n +0000000718 00000 n +0000000825 00000 n +0000000937 00000 n +0000001166 00000 n +0000001459 00000 n +0000001690 00000 n +0000001986 00000 n +0000002322 00000 n +0000002450 00000 n +0000002578 00000 n +0000002706 00000 n +0000002834 00000 n +0000002962 00000 n +0000003090 00000 n +0000003218 00000 n +0000003346 00000 n +0000003475 00000 n +0000003604 00000 n +0000003733 00000 n +trailer +<< +/Size 25 +/Root 1 0 R +/Info 12 0 R +/ID [ ] +>> +startxref +3862 +%%EOF