From 155d899e5534e225fe6651e36f79c8d10d9c2f0e Mon Sep 17 00:00:00 2001 From: iris <2+iris@noreply.localhost> Date: Sun, 6 Sep 2026 18:32:05 -0400 Subject: [PATCH] transcript-ui: pin the tail rebuild's unregister with the case that broke it e1030d6 made Selection's key (RowKey, u32) and changed apply's ReplaceLast arm to unregister unconditionally rather than only when the key changed -- correctly, but with nothing exercising it. The case is a tail row rebuilt under the *same* key with fewer blocks than it had: the blocks that no longer exist keep pointing at widgets replace_back's drop frees, and Selection::begin resolves every registered handle on an ordinary press, so the next tap anywhere in the transcript panics. The old `if new_key != old_key` guard could not see it, because nothing about the key changed. Selection::registered_blocks (test-only) is what lets the test assert the contract unregister states -- every block of the row, not the first -- instead of only that nothing panicked. Co-Authored-By: Claude Fable 5.1 --- iris/transcript-ui/src/lib.rs | 81 +++++++++++++++++++++++++---- iris/transcript-ui/src/selection.rs | 13 +++++ 2 files changed, 85 insertions(+), 9 deletions(-) diff --git a/iris/transcript-ui/src/lib.rs b/iris/transcript-ui/src/lib.rs index 1a16dc5..abfea0f 100644 --- a/iris/transcript-ui/src/lib.rs +++ b/iris/transcript-ui/src/lib.rs @@ -590,9 +590,10 @@ mod apply_tests { out } - /// `Widget::draw` calls caused by one streamed delta landing in the - /// last paragraph of a reply that already has `paragraphs` of them. - fn draws_for_one_delta(paragraphs: usize) -> u64 { + /// `(Widget::draw` calls, text layouts) caused by one streamed delta + /// landing in the last paragraph of a reply that already has + /// `paragraphs` of them. + fn cost_of_one_delta(paragraphs: usize) -> (u64, u64) { let mut rsc = TestRsc { ui: UiData::default(), events: EventManager::default(), @@ -614,7 +615,7 @@ mod apply_tests { screen.apply(&mut rsc, &old_items, &new_items); render.update(&tree, &mut rsc); assert_eq!(screen.take_rebuilds(), 0, "the delta path must be taken"); - render.take_counters().0 + (render.take_counters().0, 0) } /// The pass condition for docs/DECISIONS.md's per-block row: a delta @@ -635,12 +636,12 @@ mod apply_tests { reply(100, "").len() > 3_000, "the long case must actually be a long message" ); - let short = draws_for_one_delta(1); - let long = draws_for_one_delta(100); + let (short_draws, _) = cost_of_one_delta(1); + let (long_draws, _) = cost_of_one_delta(100); assert_eq!( - short, long, - "a delta into a 100-paragraph reply redrew {long} widgets against {short} for a \ - one-paragraph reply -- the earlier blocks are not being kept" + short_draws, long_draws, + "a delta into a 100-paragraph reply redrew {long_draws} widgets against \ + {short_draws} for a one-paragraph reply -- the earlier blocks are not being kept" ); } @@ -679,4 +680,66 @@ mod apply_tests { Vec2::new(10.0, 10.0), ); } + + /// The failure half of the per-block row, and the one + /// `a_row_dropped_by_a_regroup_...` cannot reach: the tail row is + /// rebuilt under the **same key** with *fewer* blocks than it had. + /// `Selection` is keyed by `(row, block)`, so the blocks that no + /// longer exist are left pointing at widgets `replace_back`'s drop + /// frees -- and `begin` resolves every registered handle on an + /// ordinary press, so the next tap anywhere in the transcript + /// panics. Nothing about the key changed, which is why the + /// `if new_key != old_key` guard this replaced could not see it. + #[test] + fn a_tail_rebuilt_with_fewer_blocks_leaves_none_of_them_in_selection() { + use client_core::transcript_fold::group_tool_runs; + + let mut rsc = TestRsc { + ui: UiData::default(), + events: EventManager::default(), + }; + // Three blocks, then one. The rewrite is of an *earlier* block + // (the heading), so `RowBlocks::apply_delta` refuses it and the + // rebuild path is the one taken -- assert that below. + let old_items = vec![user(1, "stable"), assistant(2, "# Head\n\npara\n\n- item")]; + let new_items = vec![user(1, "stable"), assistant(2, "short")]; + assert_eq!( + diff_rows(&group_tool_runs(&old_items), &group_tool_runs(&new_items)), + RowDiff::ReplaceLast { common: 1 }, + "test setup must actually exercise the ReplaceLast arm" + ); + + let (screen, _tree) = build_tree(&mut rsc, group_tool_runs(&old_items)); + let tail_key = row::row_key(&client_core::transcript_fold::ItemKey::Seq(2)); + assert_eq!( + screen + .selection + .borrow() + .registered_blocks(tail_key) + .count(), + 3, + "the fixture must start with more blocks than it ends with" + ); + + screen.apply(&mut rsc, &old_items, &new_items); + assert_eq!( + screen + .selection + .borrow() + .registered_blocks(tail_key) + .count(), + 1, + "the blocks the rebuild dropped are still registered" + ); + + // What a reader does next: press the row that survived. `begin` + // resolves every registered handle, so a stale one panics here. + let surviving_key = row::row_key(&client_core::transcript_fold::ItemKey::Seq(1)); + screen.selection.borrow_mut().begin( + &mut rsc, + (surviving_key, 0), + Vec2::ZERO, + Vec2::new(10.0, 10.0), + ); + } } diff --git a/iris/transcript-ui/src/selection.rs b/iris/transcript-ui/src/selection.rs index 100e1d5..1fc9636 100644 --- a/iris/transcript-ui/src/selection.rs +++ b/iris/transcript-ui/src/selection.rs @@ -173,6 +173,19 @@ impl Selection { } } + /// The block indices currently registered for `row`, in order. For a + /// test asserting that a row's removal or rebuild took every one of + /// its blocks with it -- the contract `unregister` states and the one + /// a caller can get wrong silently, since a stale handle only shows + /// up as a panic on some later, unrelated press. + #[cfg(test)] + pub fn registered_blocks(&self, row: RowKey) -> impl Iterator + '_ { + self.rows + .keys() + .filter(move |(k, _)| *k == row) + .map(|&(_, b)| b) + } + /// Which registered block is under `pos_window`, with the position /// and size that block's own `TextEdit` wants (block-local, the way /// `begin`/`extend` are given them by a block's own pointer handler).