ai-app/docs/REVIEW-2026-09-06.md

Review: iris changes since 0e46293

Scope: git diff 0e46293..HEAD -- iris/ client-core/ (58 files, +5224/-226). Read-only review; no source changed. Ordered likely-bug, then invariant guards, then rules, then tests/docs.

Likely bugs

1. iris/transcript-ui/src/lib.rs:152-160 (RowDiff::Rebuild arm of TranscriptScreen::apply) never unregisters the rows it drops from Selection, so a stale WeakWidget<TextEdit> outlives the widget it points to and the next touch on any row panics. Selection::rows: BTreeMap<RowKey, WeakWidget<TextEdit>> documents its own contract at selection.rs:69-71: "every addition here needs its removal ... called when List evicts the row." The ReplaceLast arm above it honours this (lib.rs:143-145, self.selection.borrow_mut() .unregister(old_key) when the key changes). The Rebuild arm calls (self.list)(rsc).clear() and rebuilds every row from new_rows, but never touches self.selection — any key present in old_rows and absent from new_rows (exactly what group_tool_runs regrouping two separate tool-call rows into one produces — see diff_tests:: a_tool_run_closing_and_joining_an_earlier_call_is_a_regroup_fallback, which tests the diff decision but not apply itself) is left in self.rows pointing at a widget List::clear() just freed. TextEditable::edit (iris/src/widget/text/edit.rs:582-587) resolves that handle with ui.widgets.get_mut(self).unwrap() — an unconditional panic on the freed slot. Selection::begin (selection.rs:88-101) iterates every registered row (w.edit(ui).deselect()) on an ordinary fresh press, so the crash fires on the next tap anywhere in the transcript after a regroup, not only on a tap targeting the orphaned row. Fix: give Selection a way to reconcile against the row set that survived a rebuild (e.g. Selection::retain(&self, keys: &BTreeSet<RowKey>) removing everything else, called from the Rebuild arm before rebuilding), or simplest — call self.selection.borrow_mut() cleared the same way List::clear() clears the list, then let the rebuild's push_row calls re-register everything as they already do.

Guarded invariants missing

2. iris/src/widget/list.rs:751 (List::place) indexes/expects on slot with no assertion that it exists. slot_widget (:563-575) panics via .expect(...) for a sentinel with no widget set, and does an unchecked &self.items[s as usize] for a real index — a bare "index out of bounds" with no context if place is ever reached with a stale slot. Every current caller happens to derive slot from repair_anchor/prev_slot/next_slot, which already check existence, but that invariant is enforced by convention across three call sites, not by the function that depends on it. Add debug_assert!(self.slot_exists(slot), "place() called with a slot that doesn't exist: {slot:?}"); at the top of place.
3. iris/src/widget/list.rs:426 (List::fling) and sense.rs's FlingCalculator::distance/duration/position_at never check that the incoming velocity is finite. A NaN/inf velocity (a VelocityTracker::velocity() divide-by-near-zero span, or a caller passing a raw device value straight through) propagates through deceleration_for's .ln() silently — the fling either never settles (settled_on_schedule compares against a NaN duration(), which is always false) or jumps to NaN positions with nothing on screen saying why. Add debug_assert!(velocity_px_per_s.is_finite()) in List::fling and FlingCalculator::new/distance.
4. iris/src/sense.rs:592-604 (VelocityTracker::velocity) has no assertion that samples are chronological. add_sample trusts its caller's Instant ordering; a caller that samples out of order (a restored/replayed gesture, a test) would silently produce a negative span handled only by the span <= 0.0 => 0.0 catch-all, masking the bug that produced it rather than surfacing it. Add debug_assert!(self.samples.back().is_none_or(|&(last, _)| at >= last)) in add_sample.
5. iris/core/src/render/frame_report.rs:247-252 (mark_phase) has no assertion that phases are pushed in non-decreasing start_index order. phase_stats's slicing (:274, idx >= phase.start_index && idx < end_index) silently produces an empty or nonsensical slice for an out-of-order phase rather than surfacing the misuse — cheap to add given self.phases.last() is already in scope: debug_assert!(self.phases.last().is_none_or(|p| self.total_frames >= p.start_index));

Rules

6. Two mechanisms answer "what row selection points at, still valid?" Selection relies on callers remembering to unregister (finding 1); List relies on callers deriving slots only from already-checked sources (finding 2). Both are the same class of problem — a derived handle that silently outlives what it points to — solved ad hoc twice rather than once. Not asking for a shared abstraction here, but the two should at minimum cross-reference each other's doc comment so the next caller who adds a third handle-into-List-rows type (the code rules' "a rule that governs a set belongs to the set") finds both existing examples.
7. iris/android-app/src/bench_client.rs:224-225 (battery_line) calls .min().unwrap()/.max().unwrap() on samples guarded three lines above by if samples.is_empty(), which is fine — but the guard and the two unwraps are two statements apart with a let mean = ... in between reading the same slice; a future edit reordering those lines loses the guard's protection silently. Low severity (this is the bench tool, not the app), but worth a one-line comment tying the unwraps back to the guard, or restructuring as let (Some(min), Some(max)) = (samples.iter().min(), samples.iter().max()) pattern so the empty case can't be separated from the check by a future edit.

Tests

8. No test exercises TranscriptScreen::apply's Rebuild arm through Selection. lib.rs's diff_tests module (:284-379) tests only the pure diff_rows decision function, never apply itself wired to a real Selection; selection.rs's own tests (a_missed_press_start_ recovers_on_the_next_pressing_frame, unregister_forgets_the_row_and_ clears_a_matching_anchor) never go through apply/List::clear either. This is exactly the gap that let finding 1 through: the two pieces (apply's fallback, Selection's registration contract) are each tested in isolation and never together. Add: build a TranscriptScreen, force a RowDiff::Rebuild (two adjacent tool-call rows regrouping, per the existing diff_tests case), then call selected_text/simulate a fresh press on a surviving row and assert no panic.
9. iris/src/widget/list.rs's fling tests check total distance and the start/end clamp but not the speed profile in between. fling_moves_the_list_and_then_settles/fling_distance_is_positive_ toward_the_end only assert the fling started, moved in the right direction, and eventually stopped — none checks that tick_fling's per-tick delta is monotonically decreasing once past the fling's peak (the property fling_calculator_tests::position_at_ is_monotonic_and_clamped_past_the_end already checks one level down, for FlingCalculator alone, but never through List::tick_fling's own scroll/anchor.offset accumulation). A regression that made tick_fling apply the total distance every tick instead of the incremental one, for instance, would still pass both existing tests (final position and direction are unaffected by how the interior ticks split it up) while being wildly wrong every intermediate frame.
10. iris/src/widget/list.rs::replacing_the_last_row_stays_pinned_to_ the_bottom and its sibling test replace_back's effect on the displayed row, never that the row it evicted is actually gone from heights/extents. Both tests assert the new row's position; neither asserts old.key is absent from list_ref.heights/extents after the replace (the "stale primitive" class finding 1 is a production instance of). A cheap addition: assert !list_ref.heights.contains_key(&old.key) after replace_back in the existing test, since old.key is already returned to the test as evicted... (lib.rs calls it that way; the list.rs test would need to capture the key from old similarly.)

Docs

No missing IRIS.md entry found for a public API change in this diff — List::fling/VelocityTracker/FlingCalculator, List:: anchor_position_display, FrameReport::mark_phase/phase_stats/ late_at_hz, UiRenderNode::new's Result change, Len::dp, and List::replace_back/clear/TranscriptScreen::apply all have entries. The List::replace_back/clear/TranscriptScreen::apply entry (docs/IRIS.md:526) predates this review's finding 1 and does not mention Selection's registration contract at all — once finding 1 is fixed, that entry should gain a line noting what the fix requires of a caller that keeps its own row-keyed side table (the same shape Selection is), so the next such table doesn't reproduce the same gap.

Fixed, 2026-09-06

All ten findings addressed after the DragGesture merge (selection.rs was rewritten by that merge, but finding 1's shape and location were unchanged — TranscriptScreen::apply's Rebuild arm, iris/transcript-ui/ src/lib.rs).

1. Fixed. Selection::clear() (selection.rs) drops rows and anchor, called from apply's Rebuild arm right before List::clear() — push_row re-registers whatever survives as it rebuilds each row, the "simplest" fix option the finding named.
2. Fixed. debug_assert!(self.slot_exists(slot), ...) at the top of List::place (iris/src/widget/list.rs).
3. Fixed. debug_assert!(velocity_px_per_s.is_finite()) in List::fling, and debug_assert!(velocity.is_finite()) in FlingCalculator::distance/duration (iris/src/sense.rs). position_at calls both, so it inherits the guard rather than needing its own.
4. Fixed. debug_assert! on chronological sample order in VelocityTracker::add_sample (iris/src/sense.rs).
5. Fixed. debug_assert! on non-decreasing start_index in FrameReport::mark_phase (iris/core/src/render/frame_report.rs).
6. Fixed (doc cross-reference only, as asked). Selection::register's doc now points at List::place's slot_exists assertion and vice versa isn't needed since finding 2's fix already cites this file in its own comment; both are grep-able on "docs/REVIEW-2026-09-06.md" and on each other's type names.
7. Fixed. bench_client.rs::battery_line restructured to let (Some(min), Some(max)) = (samples.iter().min(), samples.iter().max()), so the empty-guard and the two lookups can no longer be separated by a future edit.
8. Fixed. transcript-ui's new apply_tests:: a_row_dropped_by_a_regroup_does_not_outlive_itself_in_selection (lib.rs) builds a real TranscriptScreen, forces the same regroup shape diff_tests already covers at the pure-diff level, calls apply, and then Selection::begin on a surviving row — which panicked before fix 1, resolving a WeakWidget List::clear() had just freed.
9. Fixed. list.rs's new tick_fling_applies_shrinking_incremental_ deltas flings toward the end from jump_to_start and asserts each tick's extents[&0] delta is no larger than the previous one — would fail against a tick_fling that applied the total spline distance every tick instead of the incremental slice, which the two pre-existing fling tests cannot catch.
10. Fixed. list.rs's new replace_back_forgets_the_evicted_keys_own_ height replaces row 4 with a row keyed 100 (the two existing replace_back tests always reuse the same key, so neither actually exercises the removal) and asserts heights no longer contains the evicted key.

Docs: docs/IRIS.md's 2026-09-05 List::replace_back/clear/ TranscriptScreen::apply entry now has a line on what the fix requires of a caller with its own row-keyed side table, naming Selection as the example and dating the fix.

Verification run alongside the rest of this pass's checks: cargo fmt --all, cargo clippy --workspace --all-targets, cargo test --workspace from iris/ — see docs/RUST.md's plan box for the pass/fail and any caveats from this same session.