fix(automation): refuse a comment id that several comments share

The comment tools resolved `id` (the comment's /NM, as comment_list
reports it) to the first annotation with that name. /NM need not be
unique: in a PyMuPDF file every page's first comment is `fitz-A0`, so
comment_reply, comment_set_status and the rest acted on page 1's
comment whatever page the caller meant. A shared id is now refused
with a message pointing at `page` + `index`, which always name one
comment. A new end-to-end test also checks that comment_list threads
such a file by reference.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
alehel
2026-10-23 17:30:08 +02:00
co-authored by Claude Opus 5.5
parent 21814f60f5
commit a629835fec
5 changed files with 56 additions and 5 deletions
+1 -1
View File
@@ -199,7 +199,7 @@ Ranked; detail and the full milestone table in [docs/roadmap.md](docs/roadmap.md
- **2026-10-20 (M3, #3535):** Escape with a menu open closes the menu and keeps the text selection; the next Escape clears it, as before. One keypress used to do both. A synthetic UI regression checks it. Estimates unchanged.
- **2026-10-16 (M3, close prompt keyboard):** The "Save changes?" prompt opens with Save chosen (Enter saves, as before) and the arrow keys and Tab / Shift+Tab move the choice to Cancel or Don't save, shown with a focus ring; Enter or Space press the chosen button and Escape still cancels. Before, Enter always saved whatever was Tab-focused. Estimates unchanged.
- **2026-10-21 (M3, #3808):** Page thumbnails in the Inspector can be resized independently from the document and Organize, with a slider, buttons and `thumbnail-zoom` control option. Selection, page following and reload are covered by regressions. Estimates unchanged.
- **2026-10-22 (M5, comment threads):** Replies join the comment their `/IRT` references, not every comment sharing its `/NM`; PyMuPDF names each page's first comment `fitz-A0`, so all replies used to land under page 1. The Comments header counts comments without their replies (28, as Acrobat says, for a file it used to call 44) with the replies in small text under it ("16 replies", in every catalogue), and a reply's thread line runs from the avatar above it to its own. A reply to a reply shows in its thread in the panel too (it was listed nowhere), an `/IRT` that leads to no comment (itself, a loop, a pop-up, an unlisted object) leaves its annotation a comment of its own instead of hiding it, and a Replace Text caret isn't counted as a reply. UI, engine and automation regressions cover it. Estimates unchanged.
- **2026-10-22 (M5, comment threads):** Replies join the comment their `/IRT` references, not every comment sharing its `/NM`; PyMuPDF names each page's first comment `fitz-A0`, so all replies used to land under page 1. The Comments header counts comments without their replies (28, as Acrobat says, for a file it used to call 44) with the replies in small text under it ("16 replies", in every catalogue), and a reply's thread line runs from the avatar above it to its own. A reply to a reply shows in its thread in the panel too (it was listed nowhere), an `/IRT` that leads to no comment (itself, a loop, a pop-up, an unlisted object) leaves its annotation a comment of its own instead of hiding it, and a Replace Text caret isn't counted as a reply. The comment tools refuse an `id` several comments share instead of acting on the first (page 1's), and take `page` + `index` for those. UI, engine and automation regressions cover it. Estimates unchanged.
- **2026-10-22 (M2, #3107 / PR #3438):** Inline-image parsing falls back to the existing EI heuristic when missing declared samples, headers or terminators exceed the content-window budget, instead of waiting for an impossible allocation. Exact-length parsing for available samples and ordinary large streamed images is retained. Estimates unchanged.
- **2026-10-21 (M14, Brazilian Portuguese):** Completed the `pt-br` catalog against the current 2,674-key union, including signature validation and accessibility messages; no build or compilation run. Localization remains partial because help and unpredictable engine/OS error details are not translated.
- **2026-10-21 (M14, #3557):** Preferences: the OK button stays below the scrolling settings instead of being the last thing in them, so it is in reach in a short window.
+7 -2
View File
@@ -158,8 +158,13 @@ impl Automation {
fn comment_target(&self, a: &Args) -> Result<(usize, usize)> {
let all = self.comments(a)?;
if let Some(id) = a.opt_str("id")? {
let c =
all.iter().find(|c| c.name.as_deref() == Some(id)).ok_or_else(|| failed(format!("no comment with id {id:?} (see comment_list)")))?;
// `/NM` need not be unique (PyMuPDF names the first comment on every page `fitz-A0`):
// the first match could be another page's comment, so a shared id is refused.
let mut named = all.iter().filter(|c| c.name.as_deref() == Some(id));
let c = named.next().ok_or_else(|| failed(format!("no comment with id {id:?} (see comment_list)")))?;
if named.next().is_some() {
return Err(failed(format!("several comments have the id {id:?}: pass page and index instead (see comment_list)")));
}
return Ok((c.page, c.index));
}
let (Some(page), Some(index)) = (a.opt_int("page")?, a.opt_int("index")?) else {
+4 -1
View File
@@ -60,7 +60,10 @@ fn color() -> Value {
fn comment_ref(mut extra: Value) -> Value {
if let Some(props) = extra.as_object_mut() {
props.insert("doc".into(), doc());
props.insert("id".into(), json!({ "type": "string", "description": "The comment's id (from comment_list)." }));
props.insert(
"id".into(),
json!({ "type": "string", "description": "The comment's id (from comment_list). An id several comments share is refused: pass page and index." }),
);
props.insert("page".into(), json!({ "type": "integer", "minimum": 1, "description": "With `index`, instead of `id`." }));
props.insert(
"index".into(),
+43
View File
@@ -2078,6 +2078,49 @@ fn nested_replies_stay_in_the_listing() {
assert!(replies.contains(&"DIRECT_REPLY") && replies.contains(&"NESTED_REPLY"), "the nested reply is listed: {replies:?}");
}
/// Replies thread by their `/IRT` reference, not `/NM`: PyMuPDF names the first comment on every
/// page `fitz-A0`. A shared id is refused rather than taken to mean page 1's comment.
#[test]
fn replies_thread_by_reference_when_ids_repeat_across_pages() {
let dir = workdir("repeated-ids");
// Contributor-original PDF, shaped like PyMuPDF output.
std::fs::write(
dir.join("fitz.pdf"),
b"%PDF-1.7
1 0 obj << /Type /Catalog /Pages 2 0 R >> endobj
2 0 obj << /Type /Pages /Kids [3 0 R 4 0 R] /Count 2 >> endobj
3 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 300 200] /Annots [5 0 R] >> endobj
4 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 300 200] /Annots [6 0 R 7 0 R] >> endobj
5 0 obj << /Type /Annot /Subtype /Text /Rect [20 140 40 160] /NM (fitz-A0) /T (Alpha) /Contents (COVER) >> endobj
6 0 obj << /Type /Annot /Subtype /Text /Rect [20 140 40 160] /NM (fitz-A0) /T (Beta) /Contents (SECOND) >> endobj
7 0 obj << /Type /Annot /Subtype /Text /Rect [20 100 40 120] /NM (fitz-A1) /IRT 6 0 R /T (Gamma) /Contents (ANSWER) >> endobj
trailer << /Root 1 0 R >>
%%EOF",
)
.unwrap();
let mut a = auto(&dir);
let doc = ok(&mut a, "doc_open", json!({ "path": "fitz.pdf" }))["doc"].as_u64().unwrap();
let replies = |a: &mut Automation| -> Vec<(String, Vec<String>)> {
let list = ok(a, "comment_list", json!({ "doc": doc }));
list["comments"]
.as_array()
.unwrap()
.iter()
.map(|c| {
let mut texts: Vec<String> =
c["replies"].as_array().unwrap().iter().filter_map(|r| r["contents"].as_str().map(String::from)).collect();
texts.sort();
(c["contents"].as_str().unwrap().to_string(), texts)
})
.collect()
};
assert_eq!(replies(&mut a), [("COVER".into(), vec![]), ("SECOND".into(), vec!["ANSWER".to_string()])]);
let refused = err(&mut a, "comment_reply", json!({ "doc": doc, "id": "fitz-A0", "text": "LATER" }));
assert!(refused.to_string().contains("several comments"), "{refused}");
ok(&mut a, "comment_reply", json!({ "doc": doc, "page": 2, "index": 1, "text": "LATER" }));
assert_eq!(replies(&mut a), [("COVER".into(), vec![]), ("SECOND".into(), vec!["ANSWER".to_string(), "LATER".to_string()])]);
}
#[test]
fn protecting_through_tools() {
let dir = workdir("protect");
+1 -1
View File
@@ -3560,7 +3560,7 @@ milestone = "M3"
status = "shipped"
commands = ["comment.list"]
tools = ["comment_list"]
evidence = ["crates/ui-egui/tests/it/ui.rs::opening_a_pdf_shows_comments_and_bookmarks", "crates/ui-egui/tests/it/comments.rs::the_panel_posts_comments_and_replies", "crates/ui-egui/tests/it/comments.rs::page_groups_collapse_and_expand", "crates/ui-egui/tests/it/comments.rs::replies_follow_irt_references_when_names_repeat_on_every_page", "crates/engine/src/tests.rs::replies_thread_by_reference_when_names_repeat_across_pages"]
evidence = ["crates/ui-egui/tests/it/ui.rs::opening_a_pdf_shows_comments_and_bookmarks", "crates/ui-egui/tests/it/comments.rs::the_panel_posts_comments_and_replies", "crates/ui-egui/tests/it/comments.rs::page_groups_collapse_and_expand", "crates/ui-egui/tests/it/comments.rs::replies_follow_irt_references_when_names_repeat_on_every_page", "crates/engine/src/tests.rs::replies_thread_by_reference_when_names_repeat_across_pages", "crates/automation/tests/it/automation.rs::replies_thread_by_reference_when_ids_repeat_across_pages"]
notes = "cards grouped by page with replies (threaded by the `/IRT` reference, so files whose `/NM` repeats per page, as PyMuPDF writes them, thread correctly; the header counts comments without replies, as Acrobat does), status badges, selection, reply box and an Add a comment box; page groups fold and unfold from their chevron or with Collapse/Expand all pages in the panel's \"…\" menu (a comment selected on a folded page opens its group). Groups by author, type or colour don't fold yet"
[[feature]]