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 <noreply@anthropic.com>
This commit is contained in:
1 parent
e63e923d44
commit
155d899e55
2 files changed
+85
-9
No files matched your search
@@ -590,9 +590,10 @@ mod apply_tests {
|
|||||||
out
|
out
|
||||||
}
|
}
|
||||||
|
|
||||||
/// `Widget::draw` calls caused by one streamed delta landing in the
|
/// `(Widget::draw` calls, text layouts) caused by one streamed delta
|
||||||
/// last paragraph of a reply that already has `paragraphs` of them.
|
/// landing in the last paragraph of a reply that already has
|
||||||
fn draws_for_one_delta(paragraphs: usize) -> u64 {
|
/// `paragraphs` of them.
|
||||||
|
fn cost_of_one_delta(paragraphs: usize) -> (u64, u64) {
|
||||||
let mut rsc = TestRsc {
|
let mut rsc = TestRsc {
|
||||||
ui: UiData::default(),
|
ui: UiData::default(),
|
||||||
events: EventManager::default(),
|
events: EventManager::default(),
|
||||||
@@ -614,7 +615,7 @@ mod apply_tests {
|
|||||||
screen.apply(&mut rsc, &old_items, &new_items);
|
screen.apply(&mut rsc, &old_items, &new_items);
|
||||||
render.update(&tree, &mut rsc);
|
render.update(&tree, &mut rsc);
|
||||||
assert_eq!(screen.take_rebuilds(), 0, "the delta path must be taken");
|
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
|
/// 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,
|
reply(100, "").len() > 3_000,
|
||||||
"the long case must actually be a long message"
|
"the long case must actually be a long message"
|
||||||
);
|
);
|
||||||
let short = draws_for_one_delta(1);
|
let (short_draws, _) = cost_of_one_delta(1);
|
||||||
let long = draws_for_one_delta(100);
|
let (long_draws, _) = cost_of_one_delta(100);
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
short, long,
|
short_draws, long_draws,
|
||||||
"a delta into a 100-paragraph reply redrew {long} widgets against {short} for a \
|
"a delta into a 100-paragraph reply redrew {long_draws} widgets against \
|
||||||
one-paragraph reply -- the earlier blocks are not being kept"
|
{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),
|
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),
|
||||||
|
);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
@@ -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<Item = u32> + '_ {
|
||||||
|
self.rows
|
||||||
|
.keys()
|
||||||
|
.filter(move |(k, _)| *k == row)
|
||||||
|
.map(|&(_, b)| b)
|
||||||
|
}
|
||||||
|
|
||||||
/// Which registered block is under `pos_window`, with the position
|
/// Which registered block is under `pos_window`, with the position
|
||||||
/// and size that block's own `TextEdit` wants (block-local, the way
|
/// and size that block's own `TextEdit` wants (block-local, the way
|
||||||
/// `begin`/`extend` are given them by a block's own pointer handler).
|
/// `begin`/`extend` are given them by a block's own pointer handler).
|
||||||
|
|||||||
Reference in new issue
Block a user