sort comments sidebar in visual reading order (#6439) (#6514)

This commit is contained in:
Anthony Stirling
2026-06-04 17:58:55 +01:00
committed by GitHub
parent 22dacbed01
commit 353b5c807c
5 changed files with 533 additions and 3 deletions
@@ -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;
}
@@ -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<typeof entry>;
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<number, ReturnType<typeof entry>[]> = {
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<number, string> = {};
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",
});
});
});
@@ -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);
}
@@ -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<HTMLElement>("[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<HTMLElement>("[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<number, string[]> = {};
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"],
});
});
});
@@ -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/[+f8<JTCd[L^oq9HYV@35sY;ZS=WF!6VJQ_5ac^fR&Z)fc6WqHQo8:H>tCS[9WW$NBF!'qbR)!j@'#^38Q_"QI*7quN4(FLQ.GdUWCN)r43J.(@;D;^Xe(<A1$Dd`Bh2V5hZ/[TM[fF!p8pg)-skh$`ZZ=bp)N~>
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/[+f8<JTCd[L^oq9HYV@35sY;ZS=WF!6VJQ_5ac^fR&Z)fc6WqHQo8:H>tCS[9WW$NBF!'p&Ik"^_0&I-o!H#fM'Mp]E;/k\f2lV9&f&2nGEs;/VRhVG;T/Wa7sfV3.NC93%2=D6_;Vk>'SkZnYb_],0<?>V]=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<Y&$KbR[G+hd,ZP4.1!&GFN@589262J3(_E!;-:(js/V]ci`8[*L%Pacr!ifLe4,XPdH(FbqLH^VHllT<B7q$%ZV7#JWds)r7XNZ8-\*rC/`p36EY?AEMsNWLjQ3^GL-+@&[muU+M]ll.j[t]/E-ZJ+phW!aaY^rr~>
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 [ <f0cdd00ba65dc098aa8b749db481bf6a> <f0cdd00ba65dc098aa8b749db481bf6a> ]
>>
startxref
3862
%%EOF