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-`register`s 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.