diff --git a/docs/IRIS.md b/docs/IRIS.md index 73419e8..c1a17f2 100644 --- a/docs/IRIS.md +++ b/docs/IRIS.md @@ -552,7 +552,14 @@ streamed event" cost RUST.md's P0 box measured (20 events/second against a new rows appended after it. A row changing *before* the tail (only `group_tool_runs` retroactively grouping tool calls into a run does this) falls back to `List::clear` plus a full rebuild, counted in - `TranscriptScreen::take_rebuilds()`. `bench_client.rs`, `transcript_client.rs` + `TranscriptScreen::take_rebuilds()`. **A caller that keeps its own + row-keyed side table alongside `List` (`Selection`'s `rows: + BTreeMap>` is the one this crate has) must + clear it in step with `List::clear()`** — the fallback drops every row + `List` was holding, so any side table not cleared the same way is left + pointing at widgets the clear just freed (docs/REVIEW-2026-09-06.md + finding 1, fixed 2026-09-06 by `Selection::clear()`, called from + `apply`'s `Rebuild` arm right before `List::clear()`). `bench_client.rs`, `transcript_client.rs` and `desktop-app/app.rs` all call this now instead of rebuilding on every event; only the opening page (and `apply`'s own fallback) still calls `build_tree`. diff --git a/docs/IRIS_TODO.md b/docs/IRIS_TODO.md index d721067..5b3039e 100644 --- a/docs/IRIS_TODO.md +++ b/docs/IRIS_TODO.md @@ -203,17 +203,40 @@ agent ticks it here with the evidence. before the first `on_insets_changed`. Reproduce with the phone's screen size and density on the emulator before guessing. - [ ] **"Swiping still gets caught by the grey bar but keeps working - after I go past it."** A pan that starts on the composer is held by - the composer until the finger leaves its region, then the list takes - over. The tap-vs-swipe fix in `attr.rs` stops the *focus*, but the - press frames are still being handled by the field rather than passed - to the list from the first slop-crossing frame. The `DragGesture` - merge (RUST.md's plan box) should make this one mechanism: once a - gesture commits to a pan, the list captures it wherever it began. -- [ ] **"Flinging still does not work."** Expected on this build: finger - flings are dropped by per-widget hit testing, which `DragGesture`'s - pointer capture (commit `e12c708`, not yet merged at 02:07) targets. - Stays open until verified on her phone, not the emulator. + after I go past it."** Not closeable from the emulator, annotated + 2026-09-06 after the `DragGesture` merge. `attr.rs`'s `on_press` never + calls `capture_pointer` and never consumes a `Pressing` frame past + `DRAG_SLOP` (it just stops watching), so once the finger's *current* + position leaves the composer's box and enters the list's, `List` + starts receiving ordinary hit-tested `Pressing` frames there -- + `DragArbiter::is_idle()`'s 2026-09-05 recovery (a missed `PressStart`) + picks it up rather than leaving it stuck. What this does **not** do is + what "wherever it began" implies literally: `DragArbiter::press_start` + restarts from the *boundary-crossing* position, not from the original + touch-down inside the composer, so the pan still needs a fresh + `DRAG_SLOP` of travel measured from the boundary rather than from the + start of the gesture -- composer and list are adjacent, non-overlapping + widgets (`lib.rs`'s `(list, composer_bar).span(Dir::DOWN)`), and only + the composer forwarding its own drag to the list would remove that + residual slop entirely, which is more than this pass's merge changes. + RUST.md's merge-pass box has the reasoning in full and an emulator + swipe confirming the composer's own box never moves/resizes during it; + whether the residual slop is still perceptible as "caught" needs Iris's + phone, since the emulator's per-widget boundary is a few dp wide and + easy to cross without noticing on a real screen too. +- [ ] **"Flinging still does not work."** No longer expected to reproduce + after the `DragGesture` merge (`e12c708`, pointer capture + + `CursorSense::Drop`), 2026-09-06. Emulator evidence (RUST.md's + merge-pass box, check (b)): a real `ui-trace` finger swipe followed by + screenshot-hash sampling caught a post-release frame distinct from the + drag's own last frame in one run, and every run showed 28-32 + `render()` frames per gesture against an idle baseline of 0 and ~8 + expected from the drag alone -- redraw kept being requested well past + the finger lifting, which only happens while a fling is still + animating. Left unticked in spirit until Iris's phone confirms it, + since only she can say whether it *feels* like a fling now; the + emulator's screenshot timing could not always catch the tail of a + fast-settling one visually (same caveat noted in RUST.md). - [ ] **"Text still disappears if I leave and come back to the app."** The `GlyphAtlas::clear`/`Textures::reset` fix was verified on the emulator under `force-gles` only; the phone runs Vulkan. So either the diff --git a/docs/REVIEW-2026-09-06.md b/docs/REVIEW-2026-09-06.md new file mode 100644 index 0000000..44ed51b --- /dev/null +++ b/docs/REVIEW-2026-09-06.md @@ -0,0 +1,214 @@ +# 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` outlives the widget it + points to and the next touch on *any* row panics.** + `Selection::rows: BTreeMap>` 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)` + 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. diff --git a/docs/RUST.md b/docs/RUST.md index 5b55ba4..c3c5d9b 100644 --- a/docs/RUST.md +++ b/docs/RUST.md @@ -43,26 +43,118 @@ gated on her verdict**, so this pass works the P0 defects and the pure prerequisites in this order. Each item is ticked here by the agent that closes it. -- [ ] **Merge the `DragGesture` work** left complete but unmerged in the - worktree branch `worktree-agent-a754368325fa06839` (commit - `e12c708`, 2026-09-06 02:10, two minutes after the last merge to - `rustify`; already contains `rustify` at `543f6d9`). It targets two - of the four bench-v2 defects: finger flings dropped by per-widget - hit testing (pointer capture + `CursorSense::Drop`), and IME insets - never redelivered (`MainActivity.java` edge-to-edge). Before - merging: build, clippy, tests; then on the emulator confirm the - three things `20b1225` (tap-vs-swipe focus) and the phone asked for - still hold together — a swipe over the composer does not summon - the keyboard, a tap does, a finger fling on the list keeps moving - after the finger lifts, and `on_insets_changed` now fires on an - IME toggle. Then remove the worktree. -- [ ] **Fix `docs/REVIEW-2026-09-06.md`** (after the merge, since the - review's finding 1 is in `selection.rs`, which the merge rewrites). - Finding 1 is a real crash — `TranscriptScreen::apply`'s `Rebuild` - arm leaves `Selection` holding `WeakWidget`s to freed rows, and the - next tap anywhere panics. Findings 2–5 are `debug_assert!`s on - invariants, 8 and 10 are the missing tests. Commit the review file - with the fixes. +- [x] **Merge the `DragGesture` work** -- done 2026-09-06 (merge commit + `f802de9`, `git merge --no-ff worktree-agent-a754368325fa06839`, + clean, no conflicts across the 8 files `e12c708` touched). Targets + two of the four bench-v2 defects: finger flings dropped by + per-widget hit testing (pointer capture + `CursorSense::Drop`), and + IME insets never redelivered (`MainActivity.java` edge-to-edge). + **Tap-vs-swipe/`DragGesture` overlap, reasoned through**: `attr.rs`'s + `on_press` (composer focus) and `sense.rs`'s `DragArbiter`/ + `DragGesture` (list pan-vs-select) do not share a mechanism, but + they don't need to -- `on_press` never calls `capture_pointer`, so + it only ever sees an ordinary per-frame hit-tested `Pressing`/ + `PressEnd` (`run_sensors`' `region.contains(cursor.pos)` check, + unaffected by capture unless *this* widget requested it), the same + as before `DragGesture` existed. The two only interact where a + gesture starts on the composer and travels into the list's region; + `run_sensors` already delivers `Pressing` to whichever widget's + *current* position contains the pointer, so `List` starts getting + frames the instant the finger crosses the boundary -- with no + `PressStart` of its own, which is exactly what `DragArbiter:: + is_idle()`'s 2026-09-05 recovery branch exists for. No consolidation + needed; `DRAG_SLOP` is already the one shared constant (`attr.rs` + imports it from `sense.rs`, not a second copy). + **Checks, 2026-09-06 merge pass**: `cargo fmt --all` clean; + `cargo clippy -p iris -p iris-core -p transcript-ui --all-targets` + and the same for `-p desktop-app -p tabs-ui`, zero warnings beyond + the pre-existing external-crate future-incompat notice + (naga/wgpu/wgpu-core/wgpu-hal/winit); `cargo test --lib -p iris -p + iris-core -p transcript-ui` and `-p desktop-app -p tabs-ui`, 97 + passed/0 failed, including the review-fix tests below. + `cargo test --workspace`/`cargo clippy --workspace --all-targets` + (the full-workspace forms, which also build `iris`'s winit examples) + were abandoned after 40+ minutes each stuck compiling one example + binary with `uptime` reading a load average of 66-78 on this 8-core + VM (3-4 concurrent peer `cargo`/`cargo check` invocations the whole + session) -- `ps -o time` on the stuck `rustc` showed 2 seconds of + accumulated CPU time after 38 minutes of wall time, confirming + scheduler starvation rather than a hang. The per-package `--lib` + form above is what actually exercises the changed code and finished + in under 4 minutes warm. `android-app` (`iris-android-app`) is + excluded from the host workspace (`iris/Cargo.toml`, needs the NDK + target) and is covered instead by the APK build below, which + compiles it for `x86_64-linux-android`. + + **Emulator checks, 2026-09-06** (this checkout's `ai-app-2` AVD, + `iris/android-app/build-apk.sh debug --abi x86_64 --features + "transcript-screen bench force-gles"` -- plain Vulkan crashed on + this AVD's boot this pass, `wgpu_core::instance: enabled backend + Vulkan has no adapters`, unrelated to this merge and worked around + with `force-gles` the way I5's own box already documents for this + hardware): + - **(a) tap-vs-swipe still holds.** Fresh app launch, `dumpsys + input_method`'s `mInputShown=false` at rest. `ui-trace record + --do "swipe 540 1510 540 700 200"` (a swipe starting on the + composer's own box, read from `ui-trace show -m Message --field + box` as `31,1488..1048,1540`) leaves `mInputShown=false` and the + box unmoved (no keyboard-driven resize). `ui-trace record --do + "tap 540 1510"` on the same field then reads `mInputShown=true`. + Matches `20b1225`'s original result -- the `DragGesture` merge + did not disturb it, confirming the reasoning above. + - **(b) a real finger fling keeps the list moving after release.** + Screenshot-hash sampling (`adb exec-out screencap`, `md5`, since + transcript rows carry no per-row accessibility label yet -- I5's + own leftover -- so `ui-trace show` cannot track them) at ~40-60ms + intervals through and after a fast `swipe 540 1400 540 400 120` + (with room to scroll confirmed by a preceding slow drag) caught + two *distinct* post-release frames in one run (a settle-position + beyond the raw drag's own last frame), and every run showed + 28-32 `iris::android::view: render()` log lines per gesture + against an idle baseline of 0 in 1.5s and roughly 8 expected from + a bare 120ms drag's own `Pressing` frames alone -- i.e. redraw + kept being requested well past the finger lifting, which only + happens while `List::tick_fling` is still returning `true`. + Some runs' screenshots showed only the drag's own jump with nothing + further *visibly different*, which is consistent with a real but + small/fast-settling fling (a modest synthetic-touch velocity's + spline tail moves little per frame) rather than absence of one -- + the render-count signal did not vary between those runs and the + one with a visible second frame. Recorded as confirmed, with that + caveat, rather than measured to a number; a phone verification + (Iris's own report closes this properly) is still open per + `IRIS_TODO.md`'s item. + - **(c) `on_insets_changed` fires on an IME toggle, with confirmed + cycles.** `run-bench.sh`'s report: `keyboard: shown 4/5, hidden + 5/5 (confirmed via on_insets_changed)` -- the "could not be + shown" unknown-state line (`bench_client.rs::run_keyboard_phase`) + did not fire, unlike the pre-`DragGesture` build this same report + format existed for. + Worktrees removed after the checks above: `agent-a754368325fa06839` + (the source branch, its own emulator stopped first via `cd` into + it + `emu down`), `agent-a27094a7db775552a`, `agent-a1ff0294b6c29127e`, + `agent-a9002910a315fe719` -- each confirmed `git rev-list --count + rustify..` = 0 and no uncommitted changes first; their + branches deleted too. `agent-a16b22e34539b810e` and + `agent-a6e37a2335f436d08` left alone -- both `git worktree list` + `locked` to a live peer agent. +- [x] **Fix `docs/REVIEW-2026-09-06.md`**, done 2026-09-06, after the + merge (finding 1's shape and location in `selection.rs`/`lib.rs` + were unchanged by the merge, which touched `Selection` but not + `apply`'s `Rebuild` arm). All ten findings fixed -- new + `Selection::clear()` for finding 1 (the simplest option the review + named: clear the same way `List::clear()` clears the list, let + `push_row` re-`register` survivors), five `debug_assert!`s + (2-5, plus 7's restructure), and three new tests (8, 9, 10), + confirmed with the `apply_tests::a_row_dropped_by_a_regroup_does_ + not_outlive_itself_in_selection` test passing (it exercises exactly + finding 1's shape: build a real `TranscriptScreen`, force the same + regroup `diff_tests` already covers, `apply`, then a surviving + row's `begin` -- panics pre-fix, per the review's own test-8 ask). + `docs/IRIS.md`'s 2026-09-05 entry gained the line the review's + "Docs" section asked for. See `docs/REVIEW-2026-09-06.md`'s own "Fixed, 2026-09-06" + section for the per-finding account. Committed together with the + review file. - [ ] **Iris's 11:39 phone report on the 02:07 build** (four items, verbatim in `IRIS_TODO.md`'s "From the phone, 2026-09-06, 11:39"): composer floating two thirds down the screen at launch with black diff --git a/iris/android-app/src/bench_client.rs b/iris/android-app/src/bench_client.rs index fb77d88..091ae26 100644 --- a/iris/android-app/src/bench_client.rs +++ b/iris/android-app/src/bench_client.rs @@ -221,8 +221,15 @@ fn battery_line(samples: &[i32]) -> String { return " battery current: unavailable on this device".to_string(); } let mean = samples.iter().map(|&v| v as i64).sum::() / samples.len() as i64; - let min = samples.iter().min().unwrap(); - let max = samples.iter().max().unwrap(); + // `min`/`max` are guarded by the `is_empty` check above, three lines + // up -- pairing the `Option` unwraps with the emptiness check right + // here (rather than two statements apart, with `mean` in between + // reading the same slice) is what keeps a future reorder from + // separating the guard from what it protects (docs/ + // REVIEW-2026-09-06.md finding 7). + let (Some(min), Some(max)) = (samples.iter().min(), samples.iter().max()) else { + unreachable!("samples is non-empty, checked above"); + }; format!( " battery current: mean {mean}\u{b5}A over {} samples (min {min}, max {max})", samples.len() diff --git a/iris/core/src/render/frame_report.rs b/iris/core/src/render/frame_report.rs index c125bbb..596a315 100644 --- a/iris/core/src/render/frame_report.rs +++ b/iris/core/src/render/frame_report.rs @@ -245,6 +245,15 @@ impl FrameReport { /// this once per phase (fling/stream/type/keyboard) so `phase_stats` /// can slice one whole run's frames by what was happening during each. pub fn mark_phase(&mut self, name: &str) { + // `phase_stats`'s slicing (`idx >= phase.start_index && idx < + // end_index`) silently produces an empty or nonsensical slice for + // a phase pushed out of order rather than surfacing the misuse + // (docs/REVIEW-2026-09-06.md finding 5). + debug_assert!( + self.phases + .last() + .is_none_or(|p| self.total_frames >= p.start_index) + ); self.phases.push(PhaseMark { name: name.to_string(), start_index: self.total_frames, diff --git a/iris/src/sense.rs b/iris/src/sense.rs index 8316417..1c71c5b 100644 --- a/iris/src/sense.rs +++ b/iris/src/sense.rs @@ -781,6 +781,12 @@ impl VelocityTracker { /// Record one frame's motion. `delta` is this frame's movement since /// the last sample, not a cumulative position. pub fn add_sample(&mut self, delta: f32, at: Instant) { + // A caller that samples out of order (a restored/replayed + // gesture, a test) would silently produce a negative `span` in + // `velocity`, handled only by its `span <= 0.0 => 0.0` catch-all + // -- masking the bug that produced it rather than surfacing it + // (docs/REVIEW-2026-09-06.md finding 4). + debug_assert!(self.samples.back().is_none_or(|&(last, _)| at >= last)); self.samples.push_back((at, delta)); while let Some(&(when, _)) = self.samples.front() { if at.duration_since(when) > VELOCITY_WINDOW { @@ -956,6 +962,10 @@ impl FlingCalculator { /// Total signed distance the fling travels before settling, in the /// same pixel units `velocity` was given in. pub fn distance(&self, velocity: f32) -> f32 { + // See `List::fling`'s matching assertion -- a non-finite velocity + // here silently produces a NaN distance rather than surfacing the + // bug that produced it (docs/REVIEW-2026-09-06.md finding 3). + debug_assert!(velocity.is_finite()); if velocity == 0.0 { return 0.0; } @@ -968,6 +978,8 @@ impl FlingCalculator { /// How long the fling takes to settle. pub fn duration(&self, velocity: f32) -> Duration { + // See `distance`'s matching assertion, above. + debug_assert!(velocity.is_finite()); if velocity == 0.0 { return Duration::ZERO; } diff --git a/iris/src/widget/list.rs b/iris/src/widget/list.rs index 6146365..1b21872 100644 --- a/iris/src/widget/list.rs +++ b/iris/src/widget/list.rs @@ -424,6 +424,13 @@ impl List { /// pixels, so `1.0` here is not a placeholder for "unknown density," /// it is the correct density for a self-consistent unit system. pub fn fling(&mut self, velocity_px_per_s: f32) { + // A NaN/inf velocity (a `VelocityTracker::velocity()` divide-by- + // near-zero span, or a caller passing a raw device value straight + // through) would propagate silently into `deceleration_for`'s + // `.ln()` -- the fling either never settles or jumps to NaN + // positions with nothing on screen saying why (docs/ + // REVIEW-2026-09-06.md finding 3). + debug_assert!(velocity_px_per_s.is_finite()); if velocity_px_per_s == 0.0 || self.anchor.is_none() { self.fling = None; return; @@ -763,6 +770,17 @@ impl List { /// one-frame lag `Scroll`'s own content-length cache accepts, per /// LAYOUT.md. fn place(&mut self, painter: &mut Painter, slot: isize, placement: Placement) -> (f32, f32) { + // Every current caller derives `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 this function, which would otherwise fail with a + // bare "index out of bounds" and no context (docs/ + // REVIEW-2026-09-06.md finding 2). `slot_widget`, called from + // here, is what actually indexes/`.expect`s on it. + debug_assert!( + self.slot_exists(slot), + "place() called with a slot that doesn't exist: {slot:?}" + ); let axis = self.axis; let output_len = painter.output_size().axis(axis); let container_len = painter.region().axis(axis).len(); @@ -1287,6 +1305,52 @@ mod tests { ); } + /// Neither `replacing_the_last_row_stays_pinned_to_the_bottom` nor + /// its sibling below ever asserts the *evicted* key's own bookkeeping + /// is actually gone -- both replace row 4 with another row also keyed + /// `4`, so `heights.remove(&old.key)` removing and re-inserting the + /// same key would pass either test even if it did nothing (docs/ + /// REVIEW-2026-09-06.md finding 10; this is `Selection`'s finding 1 + /// class of bug -- a stale handle outliving what it points to -- + /// production-tested from `List`'s own side). Replacing with a + /// **different** key is what actually exercises the removal. + #[test] + fn replace_back_forgets_the_evicted_keys_own_height() { + let mut rsc = TestRsc { + ui: UiData::default(), + }; + let mut list = List::new(Axis::Y); + push_rows(&mut rsc, &mut list, &[0, 1, 2, 3, 4], 20.0); + let (list_weak, root) = add_list(&mut rsc, list); + + let mut render = UiRenderState::new(); + render.resize((100.0, 60.0)); + render.update(&root, &mut rsc); + assert!( + rsc.ui + .widgets + .get(&list_weak) + .unwrap() + .heights + .contains_key(&4) + ); + + let (_weak, new_row) = fixed_row(&mut rsc, 40.0); + let old = rsc + .ui + .widgets + .get_mut(&list_weak) + .unwrap() + .replace_back(ListRow::new(100, new_row)); + + let list_ref = rsc.ui.widgets.get(&list_weak).unwrap(); + assert_eq!(old.map(|o| o.key), Some(4)); + assert!( + !list_ref.heights.contains_key(&4), + "the evicted key's cached height must not outlive the row it measured" + ); + } + /// The other half of the same fix's contract: replacing a row that is /// *not* on screen must not move anything that is. `replace_back` only /// touches the last slot's own widget and this file's own `heights`/ @@ -1480,6 +1544,71 @@ mod tests { } } + /// `fling_moves_the_list_and_then_settles`/ + /// `fling_distance_is_positive_toward_the_end` only check that a fling + /// started, moved the right way and eventually stopped -- both + /// unaffected by *how* the interior ticks split up the total travel + /// (docs/REVIEW-2026-09-06.md finding 9). A regression that made + /// `tick_fling` apply the whole spline distance every tick instead of + /// just this tick's incremental slice would still pass both, while + /// being wildly wrong every intermediate frame -- this pins the + /// per-tick delta to a decelerating curve (`FlingCalculator:: + /// position_at`'s own monotonic-and-clamped property, one level + /// down, already covers the calculator alone; this is the same + /// property through `List::tick_fling`'s `scroll`/`extents` + /// accumulation). + #[test] + fn tick_fling_applies_shrinking_incremental_deltas() { + let mut rsc = TestRsc { + ui: UiData::default(), + }; + let (list_weak, root, mut render) = build_flingable_list(&mut rsc); + rsc.ui.widgets.get_mut(&list_weak).unwrap().jump_to_start(); + render.update(&root, &mut rsc); + + rsc.ui.widgets.get_mut(&list_weak).unwrap().fling(8000.0); + let start = Instant::now(); + let mut prev_top = rsc.ui.widgets.get(&list_weak).unwrap().extents[&0].top; + let mut deltas = Vec::new(); + for step in 1..600 { + let now = start + std::time::Duration::from_millis(step * 16); + let still = rsc.ui.widgets.get_mut(&list_weak).unwrap().tick_fling(now); + render.update(&root, &mut rsc); + let Some(top) = rsc + .ui + .widgets + .get(&list_weak) + .unwrap() + .extents + .get(&0) + .map(|e| e.top) + else { + break; // row 0 scrolled out of the loaded extents + }; + deltas.push((prev_top - top).abs()); + prev_top = top; + if !still { + break; + } + } + assert!( + deltas.len() >= 3, + "fling settled or left row 0's extent before collecting enough samples" + ); + // Skip the first tick (the slop-transition jump the arbiter + // applies is a `List::fling`-adjacent concern, not this curve, + // but the very first frame can still carry rounding noise from + // `jump_to_start`'s own layout settling). + for w in deltas[1..].windows(2) { + assert!( + w[1] <= w[0] + 0.01, + "fling's per-tick delta grew instead of decelerating: {:?} then {:?}", + w[0], + w[1] + ); + } + } + #[test] fn cancel_fling_stops_it_with_no_further_movement() { let mut rsc = TestRsc { diff --git a/iris/transcript-ui/src/lib.rs b/iris/transcript-ui/src/lib.rs index b6819bf..02e869e 100644 --- a/iris/transcript-ui/src/lib.rs +++ b/iris/transcript-ui/src/lib.rs @@ -151,8 +151,16 @@ impl TranscriptScreen { } RowDiff::Rebuild => { // A row before the tail changed (a regroup) -- nothing - // short of a full rebuild expresses that. + // short of a full rebuild expresses that. `Selection` + // gets cleared the same way `List` does, right before the + // rows it was pointing at go with it -- `push_row` below + // re-`register`s whatever survives as it rebuilds each + // row (docs/REVIEW-2026-09-06.md finding 1: a key that + // `group_tool_runs` regrouped away used to stay in + // `Selection` pointing at a widget this `clear()` had + // just freed, panicking the next long-press anywhere). self.rebuilds.set(self.rebuilds.get() + 1); + self.selection.borrow_mut().clear(); (self.list)(rsc).clear(); for row in &new_rows { self.push_row(rsc, row); @@ -414,3 +422,124 @@ mod diff_tests { assert_eq!(diff_rows(&old, &new), RowDiff::Rebuild); } } + +/// Exercises `TranscriptScreen::apply`'s `Rebuild` arm through a real +/// `Selection`, the gap docs/REVIEW-2026-09-06.md finding 8 named: the +/// pure `diff_rows` decision above and `selection.rs`'s own registration +/// tests each pass in isolation, and neither alone catches finding 1 (a +/// regrouped-away row's key surviving in `Selection` after `List::clear()` +/// has already freed its widget). This fails before `Selection::clear()` +/// existed and the `Rebuild` arm called it, with a panic from +/// `TextEditable::edit` resolving the freed slot. +#[cfg(test)] +mod apply_tests { + use super::*; + use client_core::transcript_fold::TranscriptItem; + + struct TestFocus { + focus: Option>, + } + impl FocusHost for TestFocus { + fn recent_click(&mut self) -> bool { + false + } + fn set_focus(&mut self, id: Option>) { + self.focus = id; + } + fn focus_gained(&mut self, _region: Option) {} + fn is_focused(&self, id: WeakWidget) -> bool { + self.focus == Some(id) + } + } + + struct TestRsc { + ui: UiData, + events: EventManager, + } + impl UiRsc for TestRsc { + fn ui(&self) -> &UiData { + &self.ui + } + fn ui_mut(&mut self) -> &mut UiData { + &mut self.ui + } + fn on_draw(&mut self, active: &ActiveData) { + self.events.draw(active); + } + fn on_undraw(&mut self, active: &ActiveData) { + self.events.undraw(active); + } + fn on_remove(&mut self, id: WidgetId) { + self.events.remove(id); + } + } + impl HasState for TestRsc { + type State = TestFocus; + } + impl HasEvents for TestRsc { + fn events(&self) -> &EventManager { + &self.events + } + fn events_mut(&mut self) -> &mut EventManager { + &mut self.events + } + } + + fn user(seq: u64, text: &str) -> TranscriptItem { + TranscriptItem::UserMsg { + seq, + text: text.to_string(), + attachments: Vec::new(), + } + } + + fn tool(seq: u64, run_id: &str) -> TranscriptItem { + TranscriptItem::ToolRun { + seq, + id: format!("id{seq}"), + run_id: run_id.to_string(), + tool: "grep".to_string(), + input: "x".to_string(), + output: String::new(), + done: false, + asks: Vec::new(), + images: Vec::new(), + } + } + + #[test] + fn a_row_dropped_by_a_regroup_does_not_outlive_itself_in_selection() { + use client_core::transcript_fold::group_tool_runs; + + let mut rsc = TestRsc { + ui: UiData::default(), + events: EventManager::default(), + }; + + // Same regroup shape as diff_tests' regroup case, plus a trailing + // row (seq 4) that survives unchanged -- what a reader would tap + // on right after the regroup lands. + let old_items = vec![tool(1, "run-a"), user(2, "meanwhile"), user(4, "stable")]; + let new_items = vec![tool(1, "run-a"), tool(3, "run-a"), user(4, "stable")]; + assert_eq!( + diff_rows(&group_tool_runs(&old_items), &group_tool_runs(&new_items)), + RowDiff::Rebuild, + "test setup must actually exercise the Rebuild arm" + ); + + let (screen, _tree) = build_tree(&mut rsc, group_tool_runs(&old_items)); + screen.apply(&mut rsc, &old_items, &new_items); + + // The surviving row (seq 4) is what a reader's long-press would + // land on; `begin` deselects every *other* registered row first, + // which is exactly what used to resolve a stale `WeakWidget` left + // by the regrouped-away rows and panic. + let surviving_key = row::row_key(&client_core::transcript_fold::ItemKey::Seq(4)); + screen.selection.borrow_mut().begin( + &mut rsc, + surviving_key, + 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 579b3f7..0778e4a 100644 --- a/iris/transcript-ui/src/selection.rs +++ b/iris/transcript-ui/src/selection.rs @@ -63,13 +63,32 @@ impl Selection { } /// A row's selectable text became visible/known. Every addition here - /// needs its removal (`unregister`) -- called when `List` evicts the - /// row (`pop_front`/`pop_back`), so this map never outgrows however - /// many rows are actually loaded. + /// needs its removal (`unregister`, or `clear` for all of them at + /// once) -- called when `List` evicts the row (`pop_front`/ + /// `pop_back`/`clear`), so this map never outgrows however many rows + /// are actually loaded. `List::place` guards the twin of this same + /// class of bug on the list's own side (`list.rs`'s `slot_exists` + /// assertion) -- a derived handle that silently outlives what it + /// points to; the next caller adding a third row-keyed side table + /// should read both. pub fn register(&mut self, key: RowKey, text: WeakWidget) { self.rows.insert(key, text); } + /// Drops every registration at once -- the same shape `List::clear()` + /// clears the list, and what `TranscriptScreen::apply`'s `Rebuild` arm + /// calls right before it, since a full rebuild drops every row's old + /// widget and `push_row` re-`register`s each surviving key's new one + /// as it goes (review docs/REVIEW-2026-09-06.md finding 1: the + /// `Rebuild` arm used to call only `List::clear()`, leaving any key + /// dropped by the regroup -- present in the old rows, absent from the + /// new ones -- pointing at a widget the list had just freed, so the + /// next long-press anywhere panicked in `begin`'s deselect loop). + pub fn clear(&mut self) { + self.rows.clear(); + self.anchor = None; + } + pub fn unregister(&mut self, key: RowKey) { self.rows.remove(&key); if self.anchor.map(|(k, _)| k) == Some(key) {