Fix search-active scroll lock (issue #19)

docs/LESSONS.md

@@ -1025,3 +1025,26 @@ Smooth ≤ ~3k events; borderline ~5–10k; laggy ~20k; near-unusable at 100k. A
 **Why `_if_absent` existed:** the prior author intended "pin the first sighting so click targets are stable." Stable but stale. The right trade-off here is "always fresh," especially since the disable-virtualization fix means every paint records every heading anyway — cost is negligible.
 
 **Files:** `crates/egui_commonmark/egui_commonmark/src/parsers/pulldown.rs` (`TagEnd::Heading` handler — line ~1587)
+
+### `record_active_search_y_viewport` keeps firing for off-screen matches once virtualization is disabled
+**Context:** Issue #19 — with the find bar open, wheel-scrolling away from the active match snapped back to it every frame. Esc to close the find bar restored scrolling. The corrective scroll block in `render_tab_content` (the one that uses `cache.active_search_y()` to snap precisely to the active match) was re-firing every frame.
+
+**Root cause:** Two facts compounded.
+1. `record_active_search_y_viewport` is called by the renderer for every Active highlight segment it walks past (`crates/egui_commonmark/.../pulldown.rs:1372`). egui's clip rect culls *painting*, not widget layout — the renderer's event loop runs `record_*` for off-screen matches just like on-screen ones. So `cache.active_search_y()` returns a fresh, accurate value every frame for as long as the active match exists.
+2. With virtualization disabled (commit `21d43c5`), the renderer walks the entire event stream every paint. The pre-virtualization version only walked the viewport slice, so off-screen matches didn't re-record — `active_search_y` went stale once you scrolled past, and the corrective block effectively self-disabled.
+3. The corrective block in `render_tab_content` had no guard for "user just scrolled" — it only checked `if let Some(actual_y) = tab.cache.active_search_y()`. Permanent snap-back loop as soon as the user's wheel moved the match out of viewport.
+
+**Fix:** one-shot `correct_active_search_pending: bool` on `Tab`. Set by `scroll_to_active_match` (called from `jump_match` / `maybe_rebuild_search` / search-open). Gates the corrective block, which clears the flag after running once. Two-stage scroll still converges in 1–2 frames after a jump; subsequent frames have flag=false so the block no-ops on user wheel.
+
+**Pre-existing pattern this mirrors:** the outline-click corrective uses `tab.pending_header_click_key.take()` — the `Option<String>` is consumed on first read for the same one-shot semantics. The search-corrective could have used `Option<()>` but a named `bool` reads better.
+
+**Empirical evidence** (Xvfb + `xdotool` 500 wheel-down events on `/tmp/search-repro.md`):
+
+| Build | Net scroll (500 wheel-downs) | FIRING events |
+|---|---|---|
+| Pre-fix (`active_search_y` always fresh) | 16 px | 213 |
+| Fix (one-shot guard) | ~2 800 px | 0 (after initial paint) |
+
+**General lesson:** when a "side path" caches values mid-render for downstream consumers (here, `active_search_y` for the corrective scroll), audit ALL the code paths that *populate* the cache when you change rendering behavior. The disable-virtualization fix in `21d43c5` was correctness-first for paint, but it silently changed the semantics of `active_search_y` from "valid while visible" to "valid forever," and the corrective block was implicitly assuming the old semantics.
+
+**Files:** `src/main.rs` (`Tab.correct_active_search_pending`, `scroll_to_active_match`, `render_tab_content` corrective block), `docs/devlog/031-search-scroll-lock.md`

docs/devlog/031-search-scroll-lock.md

@@ -0,0 +1,110 @@
+# Fix: Search-active scroll lock (issue #19)
+
+**Status:** ✅ Complete
+**Branch:** `fix/search-scroll-lock`
+**Date:** 2026-05-23
+**Lines Changed:** +41 / -15 in `src/main.rs`
+**Issue:** #19
+
+## Summary
+
+With the find bar open, wheel-scrolling away from the active match snapped the view back to the match every frame, leaving the user "locked" near the active result. Closing the bar (Esc) restored scrolling. The fix is a one-shot guard on the post-render corrective scroll: it now fires once per `scroll_to_active_match` call and stops fighting subsequent user input.
+
+## Root Cause
+
+The post-render corrective scroll block at `render_tab_content` (around `src/main.rs:2554`) was designed as stage 2 of a two-stage scroll: `scroll_to_active_match` sets `pending_scroll_offset` from a line-ratio estimate (gets the view roughly in the right area), then the next frame the renderer records the active match's real content-y via `cache.record_active_search_y_viewport`, and the corrective block uses that precise y to snap. Worked when the system was originally designed.
+
+Two interacting facts made it perpetual after commit `21d43c5` ("Disable show_scrollable virtualization"):
+
+1. **`active_search_y` never expires.** It's set whenever the renderer paints an Active highlight segment (`crates/egui_commonmark/.../pulldown.rs:1372`) and cleared only when the active range changes or search closes. With virtualization disabled, the renderer walks the full event stream every frame — egui's clip rect skips painting but not widget layout, so `record_active_search_y_viewport` fires every frame, keeping `active_search_y` perpetually fresh and accurate even when the match is off-screen.
+
+2. **The corrective block had no guard for "user just scrolled."** It only checked `if let Some(actual_y) = tab.cache.active_search_y()`. Once the user scrolled the match out of viewport, the block snapped back to it next frame, undoing the user's wheel input. Loop forever.
+
+Pre-`21d43c5` (virtualization enabled), `active_search_y` only got recorded when the active match was in the rendered viewport slice. Scrolling past it left a stale value but didn't matter because the LESSONS entry "Outline scroll-to: virtualization breaks the corrective y-record loop" already noted off-screen blocks didn't paint. Disabling virtualization shipped this regression as a side effect — `record_active_search_y_viewport` started firing every frame for any active match.
+
+## Repro
+
+`/tmp/search-repro.md` — 3 sections with `findme` on each page.
+
+```bash
+md-viewer /tmp/search-repro.md
+# Ctrl+F → findme → try to wheel-scroll past page 1
+```
+
+For automated repro on Xvfb, an internal `--debug-search QUERY` CLI flag was added (then stripped from the final commit) that opens the find bar at startup. With it, 500 `xdotool` wheel-down events produced 16 px of net scroll before the fix; 213 of those frames had the corrective block set `pending_scroll_offset = Some(0.0)` (snap-back to match 1). After the fix, the same 500-event test scrolled ~2800 px (reached page 3); 0 frames fired the corrective.
+
+## Fix
+
+One-shot `bool` flag on `Tab`:
+
+```rust
+struct Tab {
+    // ...
+    correct_active_search_pending: bool,
+    // ...
+}
+```
+
+Set by `scroll_to_active_match` (called from `jump_match` and `maybe_rebuild_search`):
+
+```rust
+tab.pending_scroll_offset = Some((estimated_y - margin).max(0.0));
+tab.correct_active_search_pending = true;  // grant one frame of corrective permission
+```
+
+Gates the corrective block, which clears the flag after running once (whichever branch):
+
+```rust
+if tab.correct_active_search_pending {
+    if let Some(actual_y) = tab.cache.active_search_y() {
+        // ... compute needs_correction, set pending_scroll_offset if needed ...
+        tab.correct_active_search_pending = false;  // one-shot consumed
+    }
+}
+```
+
+The two-stage scroll still works because the flag stays `true` until the corrective block has had its chance:
+
+- Frame N (`jump_match` or query change): `scroll_to_active_match` sets `pending_scroll_offset` + flag.
+- Frame N+1: ScrollArea applies pending → renderer paints, records `active_search_y` → corrective block reads it, either snaps (`needs_correction=true`, delta > 2 px) or no-ops; flag cleared.
+- Frame N+2+: flag is `false`; user wheel input is no longer overridden.
+
+## Key Discoveries
+
+### `record_active_search_y_viewport` fires even when the match is off-screen
+
+egui's clip rect culls *painting*, not widget layout. The renderer's `event_text_with_highlights` walks every event and calls `record_active_search_y_viewport` for each Active segment regardless of viewport intersection. Once virtualization was disabled, that means it fires every paint for the active match — making `active_search_y()` perpetually fresh, perpetually accurate, and perpetually load-bearing for the corrective-scroll loop.
+
+### Pre-existing one-shot pattern: `pending_header_click_key.take()`
+
+The outline-click corrective scroll already uses `.take()` on an `Option<String>` for the same one-shot semantics. The search-corrective block could have followed that pattern (`Option<()>` taken), but a `bool` matches the existing `correct_*_pending` naming convention better and reads less weird than `Option<()>` at a setter site.
+
+## Architecture
+
+### Modified Struct
+
+```rust
+struct Tab {
+    // ...
+    correct_active_search_pending: bool,  // new — one-shot for corrective scroll
+    // ...
+}
+```
+
+### Modified Functions
+
+| Function | Change |
+|----------|--------|
+| `scroll_to_active_match` | Sets `correct_active_search_pending = true` after setting `pending_scroll_offset` |
+| `render_tab_content` (corrective block) | Gates the existing `if let Some(actual_y) = ...` on the flag; clears the flag inside the gate |
+
+## Testing Notes
+
+Manual: confirmed by user on real display with `/tmp/search-repro.md`. Reported "working."
+
+Automated repro (pre- and post-fix): `Xvfb :99` + `xdotool mousemove ... click --repeat 500 5` over the content area, with a temporary `--debug-search findme` CLI flag and `eprintln!` traces in the corrective block. Pre-fix: 213 FIRING events, max `cur` of 406 before snap-back, screenshot still showed page 1. Post-fix: 0 FIRING events after the initial paint, screenshot showed page 3 ("End of page 3" visible). Tracing and the CLI flag were stripped before the final commit.
+
+## Future Improvements
+
+- **`MCP keystroke injection`**: the only way to repro this through MCP was a temporary CLI debug flag plus `xdotool` for the wheel. An `egui_keystroke` MCP primitive would make this and any other keyboard-shortcut-only feature testable end-to-end without scaffolding. Tracked cross-repo in `~/dev/mcp/egui-mcp/`.
+- **Consolidate the two corrective-scroll blocks**: the outline-click block (`pending_header_click_key.take()`) and the search block (`correct_active_search_pending` bool) share a structure. A small helper that takes a `cache.get_recorded_y()` closure plus the source flag would deduplicate, but the two diverge in `inset` (35% of viewport vs. fixed −50 px) and in whether the flag is `Option<String>` (the key) or `bool` (just a permission). Not worth a refactor yet.

src/main.rs

@@ -621,6 +621,14 @@ struct Tab {
     /// for this key. Cleared once the post-render corrective step has
     /// snapped the viewport to the recorded position.
     pending_header_click_key: Option<String>,
+    /// One-shot permission for the post-render corrective scroll that snaps
+    /// the active search match into view. Set by `scroll_to_active_match`
+    /// (jump_match / search-open / tab-switch / query-rebuild) and cleared
+    /// after the corrective block runs once. Without this gate, the block
+    /// would re-fire every frame while `active_search_y` remains populated,
+    /// fighting the user's wheel input and locking the view to the active
+    /// match — see issue #19 / docs/devlog/031-search-scroll-lock.md.
+    correct_active_search_pending: bool,
     last_content_height: f32,
     last_viewport_height: f32,
     content_lines: usize,
@@ -670,6 +678,7 @@ impl Tab {
             scroll_offset: 0.0,
             pending_scroll_offset: None,
             pending_header_click_key: None,
+            correct_active_search_pending: false,
             last_content_height: 0.0,
             last_viewport_height: 0.0,
             content_lines,
@@ -706,6 +715,7 @@ impl Tab {
             scroll_offset: 0.0,
             pending_scroll_offset: None,
             pending_header_click_key: None,
+            correct_active_search_pending: false,
             last_content_height: 0.0,
             last_viewport_height: 0.0,
             content_lines,
@@ -2055,6 +2065,11 @@ impl MarkdownApp {
             100.0
         };
         tab.pending_scroll_offset = Some((estimated_y - margin).max(0.0));
+        // Grant the post-render corrective block one frame of permission to
+        // snap to the renderer-recorded `active_search_y`. The block clears
+        // the flag after it runs, so subsequent frames (e.g. user wheeling
+        // away from the match) won't re-trigger snap-back. See Tab field doc.
+        tab.correct_active_search_pending = true;
     }
 
     /// Rebuild the active tab's search matches if the query or active tab has changed.
@@ -2539,22 +2554,33 @@ impl MarkdownApp {
                 // whether the current scroll position keeps it visible. If not,
                 // schedule a corrective scroll using the recorded y — this fixes
                 // line-ratio overshoot/undershoot in image-heavy documents.
-                if let Some(actual_y) = tab.cache.active_search_y() {
-                    let current_scroll = scroll_output.state.offset.y;
-                    let viewport_top = current_scroll;
-                    let viewport_bottom = current_scroll + tab.last_viewport_height;
-                    // Consider "not visible" if outside the viewport with a small margin
-                    let margin_outside = 20.0_f32;
-                    let needs_correction = actual_y < viewport_top + margin_outside
-                        || actual_y > viewport_bottom - margin_outside;
-                    if needs_correction && tab.last_viewport_height > 0.0 {
-                        // Place the active match ~35% from the top of the viewport
-                        let inset = tab.last_viewport_height * 0.35;
-                        let want_scroll = (actual_y - inset).max(0.0);
-                        // Avoid stomping if we're already at the target (within a frame)
-                        if (want_scroll - current_scroll).abs() > 2.0 {
-                            tab.pending_scroll_offset = Some(want_scroll);
+                // Corrective scroll for search-match jump. Gated by the
+                // one-shot `correct_active_search_pending` flag (set by
+                // `scroll_to_active_match`) so it fires at most once per
+                // jump — without this gate the block would re-trigger every
+                // frame the active match is off-screen, fighting the user's
+                // wheel input and locking the view (issue #19).
+                if tab.correct_active_search_pending {
+                    if let Some(actual_y) = tab.cache.active_search_y() {
+                        let current_scroll = scroll_output.state.offset.y;
+                        let viewport_top = current_scroll;
+                        let viewport_bottom = current_scroll + tab.last_viewport_height;
+                        // Consider "not visible" if outside the viewport with a small margin
+                        let margin_outside = 20.0_f32;
+                        let needs_correction = actual_y < viewport_top + margin_outside
+                            || actual_y > viewport_bottom - margin_outside;
+                        if needs_correction && tab.last_viewport_height > 0.0 {
+                            // Place the active match ~35% from the top of the viewport
+                            let inset = tab.last_viewport_height * 0.35;
+                            let want_scroll = (actual_y - inset).max(0.0);
+                            // Avoid stomping if we're already at the target (within a frame)
+                            if (want_scroll - current_scroll).abs() > 2.0 {
+                                tab.pending_scroll_offset = Some(want_scroll);
+                            }
                         }
+                        // Clear the one-shot regardless of which branch ran;
+                        // the corrective block has now had its chance to snap.
+                        tab.correct_active_search_pending = false;
                     }
                 }