From a629835fec101d8eba17d90455840945f59cc00e Mon Sep 17 00:00:00 2001 From: alehel Date: Thu, 22 Oct 2026 13:42:17 +0200 Subject: [PATCH] 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 --- ROADMAP.md | 2 +- crates/automation/src/comments.rs | 9 +++-- crates/automation/src/tools.rs | 5 ++- crates/automation/tests/it/automation.rs | 43 ++++++++++++++++++++++++ parity/acrobat-features.toml | 2 +- 5 files changed, 56 insertions(+), 5 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index fe208d3ca..20613dc46 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -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. diff --git a/crates/automation/src/comments.rs b/crates/automation/src/comments.rs index 538b45e33..87e6ae4f9 100644 --- a/crates/automation/src/comments.rs +++ b/crates/automation/src/comments.rs @@ -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 { diff --git a/crates/automation/src/tools.rs b/crates/automation/src/tools.rs index 1e009c117..9af4eb5b7 100644 --- a/crates/automation/src/tools.rs +++ b/crates/automation/src/tools.rs @@ -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(), diff --git a/crates/automation/tests/it/automation.rs b/crates/automation/tests/it/automation.rs index 9fd5c4269..d9c004b13 100644 --- a/crates/automation/tests/it/automation.rs +++ b/crates/automation/tests/it/automation.rs @@ -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)> { + let list = ok(a, "comment_list", json!({ "doc": doc })); + list["comments"] + .as_array() + .unwrap() + .iter() + .map(|c| { + let mut texts: Vec = + 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"); diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index 69c02f04e..a29da7898 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -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]]