M14: the comments panel lays out only the cards in view (#5488) (#5583)

* M14: the comments panel lays out only the cards in view (#5488)

With more than 100 comments, a card or page header out of view that
was measured at the current width only takes up its height instead of
being laid out every frame. The selected card is always drawn, so it
still scrolls itself into view. Shorter lists are laid out whole and
keep every card in the accessibility tree.

The 500-comment panel in tests/perf.rs drops from about 23 ms to 5 ms
per frame locally, well inside the 40 ms budget it overran on CI.

* M14: the comments panel measures a card again when it changes (#5488)

A card's kept height is now per document and per what the card shows (its
thread's text, author, date, status, colour and lock, and whether it is the
selected card), so another document at the same width, an edit, an undo or a
redo never lays the cards below it out at a stale height (CodeRabbit on #5583).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GbuSKAS2k5248LGFEapMUR

* M14: a comment card with an editor open is never skipped (#5488)

A card with an inline editor open on it or on one of its replies is laid out
wherever it is, and its kept height is apart from the one without the editor,
so the cards below make room for the editor (CodeRabbit on #5583).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GbuSKAS2k5248LGFEapMUR

---------

Co-authored-by: Claude <noreply@anthropic.com>

crates/ui-egui/src/panels/comments_panel.rs

@@ -241,6 +241,10 @@ pub(crate) fn show(
     }
     let mut page = usize::MAX;
     let mut group: Option<String> = None;
+    // A card (or page header) out of view that was measured at this width only takes up its
+    // height: laying out hundreds of them every frame put the panel over the frame budget
+    // (#5488). Shorter lists are laid out whole, so every card stays in the accessibility tree.
+    let virtual_list = roots.len() > LAY_OUT_WHOLE;
     for a in roots {
         if sort != SortBy::Page {
             let g = group_of(a);
@@ -252,48 +256,118 @@ pub(crate) fn show(
             }
         } else if a.page != page {
             page = a.page;
-            let n = page_counts.get(&page).copied().unwrap_or_default();
-            let open = !view.comments.collapsed_pages.contains(&page);
-            ui.add_space(6.0);
-            let clicked = ui
-                .horizontal(|ui| {
-                    let chevron = if open { "chevron-down" } else { "chevron-right" };
-                    let toggle = icons::button(ui, chevron, 18.0, false, if open { tl!("Collapse") } else { tl!("Expand") }).clicked();
-                    let label = ui.add(
-                        egui::Label::new(
-                            egui::RichText::new(crate::i18n::fmt(tl!("Page {label}"), &[("label", &info.pages[page].label)]))
-                                .font(theme::semibold(12.5))
-                                .color(t.text_muted),
-                        )
-                        .sense(Sense::click()),
-                    );
-                    ui.with_layout(Layout::right_to_left(Align::Center), |ui| {
-                        ui.label(egui::RichText::new(n.to_string()).color(t.text_faint).small())
-                    });
-                    toggle || label.clicked()
-                })
-                .inner;
-            if clicked {
-                if open {
-                    view.comments.collapsed_pages.push(page);
-                } else {
-                    view.comments.collapsed_pages.retain(|&c| c != page);
+            // Per document: two documents shown at the same width don't share heights.
+            let header = ui.scope_id().with(("comment-page-header", view.id, page));
+            if !(virtual_list && skip_if_hidden(ui, header)) {
+                let top = ui.cursor().min.y;
+                page_header(ui, t, info, page, page_counts.get(&page).copied().unwrap_or_default(), &mut view.comments.collapsed_pages);
+                if virtual_list {
+                    remember_height(ui, header, top);
                 }
             }
-            ui.add_space(2.0);
         }
         if sort == SortBy::Page && view.comments.collapsed_pages.contains(&a.page) {
             continue;
         }
+        let key = (a.page, a.index);
         let thread = replies_of(a);
+        let selected = view.comments.selected == Some(key);
+        // A height is kept for what the card showed when it was measured: its document, what the
+        // thread says, whether it was the selected card (which draws its menu and editor) and
+        // whether an editor was open on it. An
+        // edit, undo or redo that changes the card measures it afresh; other cards keep theirs.
+        // A card with an editor open on it or on a reply is always drawn too (CodeRabbit on
+        // #5583): a kept height would show it at its size before the editor opened.
+        let editing = view.comments.editing.as_ref().is_some_and(|(p, i, _)| (*p, *i) == key || thread.iter().any(|r| (r.page, r.index) == (*p, *i)));
+        let id = ui.scope_id().with(("comment-card", view.id, key, card_signature(a, thread), selected, editing));
+        // The selected card is always drawn, so it can scroll itself into view and keep its menu
+        // and editor.
+        if virtual_list && !selected && !editing && skip_if_hidden(ui, id) {
+            continue;
+        }
         let rotation = info.pages.get(a.page).map_or(0, |p| p.rotation);
+        let top = ui.cursor().min.y;
         if let Some(e) = card(ui, view, CardContext { theme: t, annotation: a, rotation, thread, doc, prefs, allowed, nav }) {
             edit = Some(e);
         }
+        if virtual_list {
+            remember_height(ui, id, top);
+        }
     }
     edit
 }
 
+/// The "Page N" group header with its fold chevron and comment count `n`; a click folds or
+/// unfolds the page.
+fn page_header(ui: &mut egui::Ui, t: &Tokens, info: &DocInfo, page: usize, n: usize, collapsed: &mut Vec<usize>) {
+    let open = !collapsed.contains(&page);
+    ui.add_space(6.0);
+    let clicked = ui
+        .horizontal(|ui| {
+            let chevron = if open { "chevron-down" } else { "chevron-right" };
+            let toggle = icons::button(ui, chevron, 18.0, false, if open { tl!("Collapse") } else { tl!("Expand") }).clicked();
+            let label = ui.add(
+                egui::Label::new(
+                    egui::RichText::new(crate::i18n::fmt(tl!("Page {label}"), &[("label", info.pages.get(page).map_or("?", |p| p.label.as_str()))]))
+                        .font(theme::semibold(12.5))
+                        .color(t.text_muted),
+                )
+                .sense(Sense::click()),
+            );
+            ui.with_layout(Layout::right_to_left(Align::Center), |ui| ui.label(egui::RichText::new(n.to_string()).color(t.text_faint).small()));
+            toggle || label.clicked()
+        })
+        .inner;
+    ui.add_space(2.0);
+    if clicked {
+        if open {
+            collapsed.push(page);
+        } else {
+            collapsed.retain(|&c| c != page);
+        }
+    }
+}
+
+/// What a card's height depends on in the document: the comment and its replies as shown (their
+/// kind, author, text, date, status, colour and lock). Two cards with the same signature lay out
+/// alike at one width.
+fn card_signature(a: &Annotation, thread: &[&Annotation]) -> u64 {
+    use std::hash::{Hash, Hasher};
+    let mut h = std::collections::hash_map::DefaultHasher::new();
+    for x in std::iter::once(a).chain(thread.iter().copied()) {
+        (&x.subtype, &x.author, &x.contents, &x.modified, &x.state, x.locked).hash(&mut h);
+        x.color.map(|c| c.map(f32::to_bits)).hash(&mut h);
+    }
+    h.finish()
+}
+
+/// Comment lists up to this long are laid out whole; longer ones lay out only what is in view.
+const LAY_OUT_WHOLE: usize = 100;
+
+/// For a part of the list (a card or a page header) that was measured at this width and would
+/// fall out of view: takes up its height without laying it out, and returns `true`.
+fn skip_if_hidden(ui: &mut egui::Ui, id: egui::Id) -> bool {
+    let width = ui.available_width();
+    let Some(height) = ui.data(|d| d.get_temp::<(f32, f32)>(id)).filter(|(w, _)| (w - width).abs() < 0.5).map(|(_, h)| h) else {
+        return false;
+    };
+    if ui.is_rect_visible(egui::Rect::from_min_size(ui.cursor().min, vec2(width, height))) {
+        return false;
+    }
+    ui.allocate_space(vec2(width, height));
+    true
+}
+
+/// Records the height part `id` took since the cursor was at `top`, for [`skip_if_hidden`]: less
+/// the spacing `allocate_space` adds back after it.
+fn remember_height(ui: &egui::Ui, id: egui::Id, top: f32) {
+    let height = ui.cursor().min.y - top - ui.spacing().item_spacing.y;
+    if height.is_finite() && height > 0.0 {
+        let width = ui.available_width();
+        ui.data_mut(|d| d.insert_temp(id, (width, height)));
+    }
+}
+
 /// Draw one comment thread card, including author, body or inline editor, replies, status and per-comment
 /// actions; returns an edit to apply.
 struct CardContext<'a> {

crates/ui-egui/tests/it/comments.rs

@@ -3022,3 +3022,73 @@ fn an_upright_comment_on_a_turned_page_is_drawn_where_it_goes_before_release() {
     h.drop_at(from + delta);
     h.run_steps(3);
 }
+
+/// `n` pages with one comment each, "Card 1" to "Card n".
+fn one_comment_per_page(n: usize) -> Vec<u8> {
+    let mut objs = vec!["<< /Type /Catalog /Pages 2 0 R >>".to_string()];
+    let kids: Vec<String> = (0..n).map(|i| format!("{} 0 R", 3 + 2 * i)).collect();
+    objs.push(format!("<< /Type /Pages /Kids [{}] /Count {n} /MediaBox [0 0 300 200] >>", kids.join(" ")));
+    for i in 0..n {
+        objs.push(format!("<< /Type /Page /Parent 2 0 R /Annots [{} 0 R] >>", 4 + 2 * i));
+        objs.push(format!("<< /Type /Annot /Subtype /Square /Rect [20 20 80 80] /C [1 0 0] /T (Ada) /Contents (Card {}) >>", i + 1));
+    }
+    let mut out = b"%PDF-1.7\n".to_vec();
+    for (i, o) in objs.iter().enumerate() {
+        out.extend_from_slice(format!("{} 0 obj\n{o}\nendobj\n", i + 1).as_bytes());
+    }
+    out.extend_from_slice(b"trailer << /Root 1 0 R >>\n%%EOF\n");
+    out
+}
+
+/// #5488: the panel laid out every card on every frame, which put a few hundred comments over the
+/// frame budget. Cards out of view now only take up their height, and the selected one is still
+/// drawn wherever it is, so it can scroll into view.
+#[test]
+fn the_comments_panel_lays_out_only_the_cards_in_view() {
+    let mut h = harness_with_bytes(one_comment_per_page(120), |app| app.set_option("panel", "comments").unwrap());
+    h.run_steps(4);
+    let drawn = h.query_all_by_label_contains("Card ").count();
+    assert!((1..40).contains(&drawn), "{drawn} of 120 cards laid out");
+    h.state_mut().views[0].comments.selected = Some((100, 0));
+    h.state_mut().views[0].comments.reveal = true;
+    h.run_steps(4);
+    assert!(h.query_by_label("Card 101").is_some(), "the selected card is drawn and scrolled to");
+    assert!(h.query_by_label("Card 1").is_none(), "the top of the list scrolled out of view");
+}
+
+/// CodeRabbit on #5583: a card's measured height belongs to what it showed. Card 1, far above the
+/// cards in view, grows by two replies; the cards in view move down by its growth, not stay where
+/// card 1's old height would put them.
+#[test]
+fn a_card_out_of_view_that_grows_moves_the_cards_below_it() {
+    let mut h = harness_with_bytes(one_comment_per_page(120), |app| app.set_option("panel", "comments").unwrap());
+    h.run_steps(4);
+    h.state_mut().views[0].comments.selected = Some((60, 0));
+    h.state_mut().views[0].comments.reveal = true;
+    h.run_steps(4);
+    assert!(h.query_by_label("Card 1").is_none(), "card 1 is out of view");
+    let before = h.get_by_label("Card 61").rect().top();
+    for text in ["A first reply", "A second reply"] {
+        h.state_mut().apply_edit(pdfcraft_engine::Edit::ReplyToAnnotation { page: 0, index: 0, text: text.into(), author: "Bo".into() });
+    }
+    h.run_steps(4);
+    let after = h.get_by_label("Card 61").rect().top();
+    assert!(after > before + 40.0, "the cards below a grown card move down: {before} → {after}");
+}
+
+/// CodeRabbit on #5583: a card with an editor open on it is drawn wherever it is, so the cards
+/// below it make room for the editor rather than keep the card's height from before it opened.
+#[test]
+fn a_card_with_an_editor_open_out_of_view_is_drawn_at_its_size() {
+    let mut h = harness_with_bytes(one_comment_per_page(120), |app| app.set_option("panel", "comments").unwrap());
+    h.run_steps(4);
+    h.state_mut().views[0].comments.selected = Some((60, 0));
+    h.state_mut().views[0].comments.reveal = true;
+    h.run_steps(4);
+    assert!(h.query_by_label("Card 1").is_none(), "card 1 is out of view");
+    let before = h.get_by_label("Card 61").rect().top();
+    h.state_mut().views[0].comments.editing = Some((0, 0, "Card 1".into()));
+    h.run_steps(4);
+    let after = h.get_by_label("Card 61").rect().top();
+    assert!(after > before + 20.0, "the cards below make room for card 1's editor: {before} → {after}");
+}