diff --git a/crates/fdemon-app/src/log_view_state.rs b/crates/fdemon-app/src/log_view_state.rs index 07e20265..8e40c074 100644 --- a/crates/fdemon-app/src/log_view_state.rs +++ b/crates/fdemon-app/src/log_view_state.rs @@ -3,7 +3,7 @@ //! This module defines the state types used by both the app handler layer //! (for scroll commands) and the TUI layer (for rendering the log view). -use std::collections::VecDeque; +use std::collections::{HashMap, VecDeque}; use fdemon_core::LogEntry; @@ -334,6 +334,26 @@ pub struct SelectionEdge { // LogViewState // ───────────────────────────────────────────────────────────────────────────── +/// One entry's cached wrap-mode display-row count (issue #75). +/// +/// Render-written by the log-view renderer (`fdemon-tui`) whenever it +/// computes an entry's row count the exact-but-slow way (a cache miss); +/// consulted on every later frame while both the entry's identity +/// (`LogViewState::row_cache` key) and its `expanded` flag stay the same. +/// Lookup-time keyed — no handler-side invalidation wiring is needed: the +/// renderer re-reads `expanded` fresh from `CollapseState` every frame and +/// compares it against the cached value, so a toggle produces a guaranteed +/// miss (recompute + overwrite) rather than a correctness hazard. +/// Registered in docs/REVIEW_FOCUS.md ("Current usage" list of the approved TEA exception). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct CachedRows { + /// The entry's collapse/expand state at the time `rows` was computed. + pub expanded: bool, + /// Terminal rows the entry occupies in wrap mode, at the render's + /// current global key — see [`LogViewState::row_cache_key`]. + pub rows: u16, +} + /// State for log view scrolling with virtualization support #[derive(Debug)] pub struct LogViewState { @@ -380,6 +400,20 @@ pub struct LogViewState { /// Cache key for `selection_text`: the selection it was computed for, so the /// renderer only rebuilds the string when the selection actually changes. pub selection_text_key: Option, + /// Per-entry cached wrap-mode display-row count, keyed by [`LogEntry::id`] + /// (issue #75). Render-written: populated by the log-view renderer on a + /// cache miss, read on every subsequent frame. Safe default: empty (every + /// entry starts as a miss). Wrap mode only — nowrap uses + /// `calculate_entry_lines` (no measured widths) and leaves this untouched. + /// Registered in docs/REVIEW_FOCUS.md ("Current usage" list of the approved TEA exception). + pub row_cache: HashMap, + /// Global cache key for `row_cache`: `(content width, wrap_mode)` at the + /// last render that touched the cache. A mismatch at render start clears + /// `row_cache` wholesale (a resize or the first wrap-mode render + /// invalidates every cached row count). Safe default: `None` (a `None` + /// key never matches, so the first render always "clears" an already- + /// empty map). Registered in docs/REVIEW_FOCUS.md ("Current usage" list of the approved TEA exception). + pub row_cache_key: Option<(u16, bool)>, } impl Default for LogViewState { @@ -410,6 +444,8 @@ impl LogViewState { drag_autoscroll: None, selection_text: None, selection_text_key: None, + row_cache: HashMap::new(), + row_cache_key: None, } } diff --git a/crates/fdemon-tui/src/widgets/log_view/mod.rs b/crates/fdemon-tui/src/widgets/log_view/mod.rs index 62e556b0..576ca251 100644 --- a/crates/fdemon-tui/src/widgets/log_view/mod.rs +++ b/crates/fdemon-tui/src/widgets/log_view/mod.rs @@ -7,7 +7,7 @@ use fdemon_app::config::FlutterMode; use fdemon_app::hyperlinks::LinkHighlightState; use fdemon_app::log_view_state::{ grapheme_cell_widths, wrap_row_starts, wrap_row_starts_cells, wrapped_row_count_widths, - FocusInfo, LogViewState, SelPoint, SelectionEdge, SelectionRow, + CachedRows, FocusInfo, LogViewState, SelPoint, SelectionEdge, SelectionRow, }; use fdemon_core::{ AppPhase, FilterState, LogEntry, LogLevel, LogLevelFilter, LogSource, LogSourceFilter, @@ -38,6 +38,15 @@ pub mod styles; /// Below this width, the bottom metadata bar switches to compact mode. const MIN_FULL_STATUS_WIDTH: u16 = 60; +/// Cell width of the stack-frame indent used by [`LogView::frame_line_widths`]. +/// `styles::INDENT` is 4 ASCII spaces, so `len() == width` (no grapheme +/// measurement needed for a pure-ASCII, single-width-per-char constant). +const FRAME_INDENT_WIDTH: usize = styles::INDENT.len(); + +/// Character length of the async-gap placeholder text, matching the literal +/// used in [`LogView::format_stack_frame_line_with_links`]'s async-gap branch. +const ASYNC_GAP_TEXT_LEN: usize = "".len(); + /// Number of base-10 digits in `n` (`0` counts as one digit). /// /// Allocation-free alternative to `n.to_string().len()` — used to size @@ -734,8 +743,11 @@ impl<'a> LogView<'a> { frame_index: usize, ) -> impl Iterator + 'b { if frame.is_async_gap { - // INDENT(4) + "" (25 ASCII chars). - FrameLineWidths::AsyncGap(std::iter::repeat_n(1, 4 + 25)) + // INDENT + "". + FrameLineWidths::AsyncGap(std::iter::repeat_n( + 1, + FRAME_INDENT_WIDTH + ASYNC_GAP_TEXT_LEN, + )) } else { // Same lookup the formatter uses (mod.rs `format_stack_frame_line_with_links`) // to decide whether a link badge is inserted before the file path. @@ -746,9 +758,10 @@ impl<'a> LogView<'a> { .any(|l| l.entry_index == entry_index && l.frame_index == Some(frame_index)) }); - // INDENT(4) + "#" + left-padded (min width 3) frame number — `{:<3}` is a + // INDENT + "#" + left-padded (min width 3) frame number — `{:<3}` is a // *minimum* width, so numbers >= 1000 widen the field instead of truncating. - let prefix_width = 4 + 1 + 3usize.max(digits(frame.frame_number as u64)); + let prefix_width = + FRAME_INDENT_WIDTH + 1 + 3usize.max(digits(frame.frame_number as u64)); // " (" + optional 3-char link badge "[c]". let paren_width = 2 + if has_badge { 3 } else { 0 }; // ":" + line digits + optional (":" + column digits) + ")". @@ -783,7 +796,7 @@ impl<'a> LogView<'a> { } else { digits(hidden_count as u64) + " more frames...".chars().count() }; - std::iter::repeat_n(1, 4) + std::iter::repeat_n(1, FRAME_INDENT_WIDTH) .chain(grapheme_cell_widths("▶ ").map(|(_, _, w)| w)) .chain(std::iter::repeat_n(1, text_len)) } @@ -798,6 +811,29 @@ impl<'a> LogView<'a> { } } + /// Shared collapse-visibility computation: how many of an entry's stack + /// frames render given its expand/collapse state, and whether a + /// "N more frames..." indicator follows. Used by + /// [`Self::calculate_entry_lines`], [`Self::calculate_entry_display_rows`], + /// and [`Self::measure_entry_exact_rows`] so the collapse rule lives in + /// exactly one place. Returns + /// `(is_expanded, visible_frames, has_indicator, hidden_count)`. + fn collapse_visibility( + &self, + entry: &LogEntry, + frame_count: usize, + ) -> (bool, usize, bool, usize) { + let is_expanded = self.is_entry_expanded(entry); + if is_expanded { + (true, frame_count, false, 0) + } else { + let visible = self.max_collapsed_frames.min(frame_count); + let has_indicator = frame_count > self.max_collapsed_frames; + let hidden_count = frame_count.saturating_sub(self.max_collapsed_frames); + (false, visible, has_indicator, hidden_count) + } + } + /// Calculate lines for a single entry accounting for collapse state fn calculate_entry_lines(&self, entry: &LogEntry) -> usize { let frame_count = entry.stack_trace_frame_count(); @@ -805,16 +841,9 @@ impl<'a> LogView<'a> { return 1; // Just the message line } - let is_expanded = self.is_entry_expanded(entry); - if is_expanded { - // Expanded: message + all frames - 1 + frame_count - } else { - // Collapsed: message + visible frames + indicator (if more) - let visible = self.max_collapsed_frames.min(frame_count); - let has_more = frame_count > self.max_collapsed_frames; - 1 + visible + if has_more { 1 } else { 0 } - } + let (_, visible, has_indicator, _) = self.collapse_visibility(entry, frame_count); + // message + visible frames + indicator (if truncated) + 1 + visible + if has_indicator { 1 } else { 0 } } /// Terminal rows a rendered line occupies when wrapped, using the same @@ -895,13 +924,8 @@ impl<'a> LogView<'a> { // Same collapse rules as `calculate_entry_lines` (mod.rs `calculate_entry_lines`): // expanded -> all frames; collapsed -> first `max_collapsed_frames` (+ indicator // if truncated). - let is_expanded = self.is_entry_expanded(entry); - let (visible_frames, has_indicator) = if is_expanded { - (frame_count, false) - } else { - let visible = self.max_collapsed_frames.min(frame_count); - (visible, frame_count > self.max_collapsed_frames) - }; + let (_, visible_frames, has_indicator, hidden_count) = + self.collapse_visibility(entry, frame_count); let frame_rows: usize = trace.frames[..visible_frames] .iter() @@ -915,7 +939,6 @@ impl<'a> LogView<'a> { .sum(); let indicator_rows = if has_indicator { - let hidden_count = frame_count.saturating_sub(self.max_collapsed_frames); wrapped_row_count_widths( Self::collapsed_indicator_widths(hidden_count), visible_width, @@ -927,6 +950,104 @@ impl<'a> LogView<'a> { msg_rows + frame_rows + indicator_rows } + /// Exact row measurement for a linked entry, bypassing the cache: formats + /// the entry's actual lines the same way the render loop would — message + /// via [`Self::format_entry`] (which inserts the message-level link + /// badge), frames via [`Self::format_stack_frame_line_with_links`], + /// indicator via [`Self::format_collapsed_indicator`], respecting + /// expand/collapse state — and sums [`Self::line_wrapped_row_count`]. + /// Bounded by `MAX_LINK_SHORTCUTS` (≤35 links, all viewport-visible while + /// link mode is active), so the extra allocations here are acceptable. + /// Mirrors `rendered_row_count` in `tests.rs` (the #74 ground truth). + fn measure_entry_exact_rows( + &self, + entry: &LogEntry, + entry_index: usize, + visible_width: usize, + ) -> usize { + let msg_line = self.format_entry(entry, entry_index); + let mut rows = Self::line_wrapped_row_count(&msg_line, visible_width); + + let Some(trace) = &entry.stack_trace else { + return rows; + }; + let frame_count = trace.frames.len(); + if frame_count == 0 { + return rows; + } + + let (_, visible_frames, has_indicator, hidden_count) = + self.collapse_visibility(entry, frame_count); + + for (frame_idx, frame) in trace.frames[..visible_frames].iter().enumerate() { + let line = self.format_stack_frame_line_with_links(frame, entry_index, frame_idx); + rows += Self::line_wrapped_row_count(&line, visible_width); + } + + if has_indicator { + let line = Self::format_collapsed_indicator(hidden_count); + rows += Self::line_wrapped_row_count(&line, visible_width); + } + + rows + } + + /// Cached lookup/compute for one entry's wrap-mode display-row count + /// (issue #75). Shared by both call sites in [`Self::render_inner`] (the + /// `total_lines` loop and the render loop's per-entry `entry_units`) so + /// they can never drift from each other. + /// + /// - **Linked entry** (link-highlight mode active and this entry carries + /// any badge, message- or frame-level): bypasses the cache — measured + /// exactly via [`Self::measure_entry_exact_rows`]. Never written to the + /// cache (link state can change on every scroll). + /// - **Otherwise**: hit iff a cached entry exists for this + /// [`LogEntry::id`] with a matching `expanded` state; a miss recomputes + /// via the exact iterator path ([`Self::calculate_entry_display_rows`]) + /// and inserts the result. + fn entry_display_rows_cached( + &self, + state: &mut LogViewState, + entry: &LogEntry, + idx: usize, + visible_width: usize, + ) -> usize { + let is_linked = self + .link_highlight_state + .is_some_and(|s| s.links.iter().any(|l| l.entry_index == idx)); + if is_linked { + return self.measure_entry_exact_rows(entry, idx, visible_width); + } + + let expanded = self.is_entry_expanded(entry); + if let Some(cached) = state.row_cache.get(&entry.id) { + if cached.expanded == expanded { + return cached.rows as usize; + } + } + + let rows = self.calculate_entry_display_rows(entry, idx, visible_width); + state.row_cache.insert( + entry.id, + CachedRows { + expanded, + // Saturating: a single entry occupying more than 65535 wrap + // rows is not a realistic scenario (see CachedRows docs); the + // debug_assert surfaces it loudly in test builds if a future + // change ever inflates row counts. + rows: { + debug_assert!( + rows <= u16::MAX as usize, + "entry {} row count {rows} exceeds the u16 cache field", + entry.id + ); + u16::try_from(rows).unwrap_or(u16::MAX) + }, + }, + ); + rows + } + /// Render empty state with centered message fn render_empty(&self, area: Rect, buf: &mut Buffer) { let block = Block::default() @@ -1709,6 +1830,12 @@ impl<'a> LogView<'a> { ) { // Handle empty state specially if self.logs.is_empty() { + // A fully-empty buffer (clear-logs) means every cached row count + // is stale — and this early return skips the pruning pass below, + // so clear here. (An active filter matching nothing takes the + // later empty-filtered return instead and keeps the cache: its + // ids are still live.) + state.row_cache.clear(); self.render_empty(area, buf); return; } @@ -1780,13 +1907,26 @@ impl<'a> LogView<'a> { let visible_width = content_area.width as usize; let visible_lines = content_area.height as usize; + // Row-count cache (issue #75): wrap mode only — nowrap uses + // `calculate_entry_lines`, which has no measured-width dependency, so + // the cache is left untouched there. A global-key mismatch (width or + // wrap-mode change) invalidates every cached row count at once. + if self.wrap_mode { + let key = (visible_width as u16, self.wrap_mode); + if state.row_cache_key != Some(key) { + state.row_cache.clear(); + } + state.row_cache_key = Some(key); + } + // Calculate total lines including stack traces (accounting for collapse state). // In wrap mode, total_lines counts terminal rows (wrapped); in nowrap, logical lines. let total_lines: usize = if self.wrap_mode { - filtered_indices - .iter() - .map(|&idx| self.calculate_entry_display_rows(&self.logs[idx], idx, visible_width)) - .sum() + let mut total = 0usize; + for &idx in &filtered_indices { + total += self.entry_display_rows_cached(state, &self.logs[idx], idx, visible_width); + } + total } else { filtered_indices .iter() @@ -1794,6 +1934,26 @@ impl<'a> LogView<'a> { .sum() }; + // Pruning (memory hygiene only — evicted ids are never queried): once + // the cache outgrows the current filtered view by a comfortable + // margin, drop entries for ids that have scrolled off the front of + // the buffer. + if self.wrap_mode { + let prune_threshold = 2 * filtered_indices.len() + 64; + if state.row_cache.len() > prune_threshold { + // An empty buffer means every cached id is stale — clear + // outright (a `0` fallback would make the retain a no-op, + // since all u64 ids are >= 0). + match self.logs.front() { + Some(front) => { + let front_id = front.id; + state.row_cache.retain(|id, _| *id >= front_id); + } + None => state.row_cache.clear(), + } + } + } + // Update state with content dimensions state.update_content_size(total_lines, visible_lines); @@ -1830,7 +1990,7 @@ impl<'a> LogView<'a> { for &idx in &filtered_indices { let entry = &self.logs[idx]; let entry_units = if self.wrap_mode { - self.calculate_entry_display_rows(entry, idx, visible_width) + self.entry_display_rows_cached(state, entry, idx, visible_width) } else { self.calculate_entry_lines(entry) }; diff --git a/crates/fdemon-tui/src/widgets/log_view/tests.rs b/crates/fdemon-tui/src/widgets/log_view/tests.rs index df82bcea..7b27c2d9 100644 --- a/crates/fdemon-tui/src/widgets/log_view/tests.rs +++ b/crates/fdemon-tui/src/widgets/log_view/tests.rs @@ -4379,3 +4379,511 @@ fn test_click_regions_below_wrapped_collapsed_indicator_stay_aligned() { ), } } + +// ───────────────────────────────────────────────────────── +// Per-entry display-row-count cache (issue #75) +// ───────────────────────────────────────────────────────── + +/// Build a `LogView` for the row-cache tests with a consistent baseline +/// configuration: timestamps/source hidden (isolates row-count math from the +/// prefix-width constants) and stack traces expanded by default (no +/// `collapse_state` — `is_entry_expanded` always returns `true`). +fn row_cache_test_view(logs: &VecDeque) -> LogView<'_> { + LogView::new(logs, test_icons()) + .show_timestamps(false) + .show_source(false) + .default_collapsed(false) +} + +/// Ground-truth total: the sum of `rendered_row_count` (the #74 exact +/// formatter path) over every entry in `logs`, at content-area `width`. +/// Compared against `LogViewState::total_lines` to prove the row cache +/// (issue #75) never drifts from what the render loop actually produces. +fn expected_total_lines(view: &LogView, logs: &VecDeque, width: usize) -> usize { + (0..logs.len()) + .map(|idx| rendered_row_count(view, &logs[idx], idx, width)) + .sum() +} + +/// Mixed fixture for the coherence tests: a one-row entry, a message that +/// wraps across several rows, and an entry with the full [`wrap_invariant_trace`] +/// (async gap, long names/paths, CJK/emoji, frame numbers >= 1000). +fn row_cache_mixed_logs() -> VecDeque { + let mut logs = VecDeque::new(); + logs.push_back(make_entry(LogLevel::Info, LogSource::App, "short")); + logs.push_back(make_entry( + LogLevel::Info, + LogSource::App, + "a fairly long message that should wrap across multiple rows on a narrow viewport indeed", + )); + let mut with_trace = make_entry(LogLevel::Error, LogSource::App, "boom"); + with_trace.stack_trace = Some(wrap_invariant_trace()); + logs.push_back(with_trace); + logs +} + +/// Acceptance criterion #1: a steady-state second render must READ the +/// cache rather than recompute. Proven by overwriting a cached `rows` value +/// with a sentinel between two identical renders and observing `total_lines` +/// reflect the sentinel exactly. A width-change invalidation afterward must +/// both drop the sentinel and recompute the correct total. +#[test] +fn test_row_cache_sentinel_probe_proves_second_render_reads_cache() { + use ratatui::{buffer::Buffer, layout::Rect, widgets::StatefulWidget}; + + let logs = make_logs_no_traces(6); + let mut state = LogViewState::new(); + let area = Rect::new(0, 0, 40, 12); + + // First render populates the cache. + { + let mut buf = Buffer::empty(area); + StatefulWidget::render( + row_cache_test_view(&logs).wrap_mode(true), + area, + &mut buf, + &mut state, + ); + } + assert!( + !state.row_cache.is_empty(), + "first render must populate the cache" + ); + let correct_total = state.total_lines; + + // Pick one cached entry and overwrite its `rows` with an obviously-wrong + // sentinel that no real measurement of these short entries could produce. + let id = *state + .row_cache + .keys() + .next() + .expect("at least one cached entry"); + let original_rows = state.row_cache[&id].rows as usize; + const SENTINEL: u16 = 9_999; + state.row_cache.get_mut(&id).unwrap().rows = SENTINEL; + + // Second render with identical state/geometry: if the cache is actually + // READ (not recomputed), total_lines must reflect the sentinel exactly. + { + let mut buf = Buffer::empty(area); + StatefulWidget::render( + row_cache_test_view(&logs).wrap_mode(true), + area, + &mut buf, + &mut state, + ); + } + let expected_with_sentinel = correct_total - original_rows + SENTINEL as usize; + assert_eq!( + state.total_lines, expected_with_sentinel, + "second render must read the sentinel from the cache rather than recomputing \ + (proves the cache is consulted, not recomputed, in steady state)" + ); + + // Invalidate via a width change: the sentinel must be gone, and the + // recomputed total must match a fresh render at the new width exactly + // (not merely differ from the sentinel-corrupted total). + let area2 = Rect::new(0, 0, 30, 12); + { + let mut buf = Buffer::empty(area2); + StatefulWidget::render( + row_cache_test_view(&logs).wrap_mode(true), + area2, + &mut buf, + &mut state, + ); + } + assert_ne!( + state.row_cache.get(&id).map(|c| c.rows), + Some(SENTINEL), + "sentinel must not survive a width-change invalidation" + ); + + let mut fresh_state = LogViewState::new(); + { + let mut buf = Buffer::empty(area2); + StatefulWidget::render( + row_cache_test_view(&logs).wrap_mode(true), + area2, + &mut buf, + &mut fresh_state, + ); + } + assert_eq!( + state.total_lines, fresh_state.total_lines, + "after invalidation, the recomputed total must match a fresh render exactly" + ); +} + +/// Coherence: a content-width change must invalidate the cache (global key +/// mismatch) and every subsequent render must be exact. +#[test] +fn test_row_cache_coherent_after_width_change() { + use ratatui::{buffer::Buffer, layout::Rect, widgets::StatefulWidget}; + + let logs = row_cache_mixed_logs(); + let mut state = LogViewState::new(); + + for area in [ + Rect::new(0, 0, 42, 14), + Rect::new(0, 0, 27, 14), + Rect::new(0, 0, 62, 14), + ] { + { + let mut buf = Buffer::empty(area); + StatefulWidget::render( + row_cache_test_view(&logs).wrap_mode(true), + area, + &mut buf, + &mut state, + ); + } + let width = area.width as usize - 2; + let expected = expected_total_lines(&row_cache_test_view(&logs), &logs, width); + assert_eq!( + state.total_lines, expected, + "total_lines must be exact after a width change to {width}" + ); + } +} + +/// Coherence: toggling wrap mode off and back on (same width) must not +/// leave the cache stale — nowrap renders leave `row_cache` untouched, and +/// the next wrap-mode render must still be exact. +#[test] +fn test_row_cache_coherent_after_wrap_toggle() { + use ratatui::{buffer::Buffer, layout::Rect, widgets::StatefulWidget}; + + let logs = row_cache_mixed_logs(); + let mut state = LogViewState::new(); + let area = Rect::new(0, 0, 42, 14); + let width = area.width as usize - 2; + + for wrap in [true, false, true] { + let mut buf = Buffer::empty(area); + StatefulWidget::render( + row_cache_test_view(&logs).wrap_mode(wrap), + area, + &mut buf, + &mut state, + ); + if wrap { + let expected = expected_total_lines(&row_cache_test_view(&logs), &logs, width); + assert_eq!( + state.total_lines, expected, + "total_lines must be exact in wrap mode after a wrap-toggle round trip" + ); + } + } +} + +/// Coherence: an expand/collapse toggle (`CollapseState`) changes the +/// per-entry cache key (`expanded`), so the very next render — collapsed, +/// then expanded, then collapsed again — must stay exact at every step. +#[test] +fn test_row_cache_coherent_after_expand_collapse_toggle() { + use ratatui::{buffer::Buffer, layout::Rect, widgets::StatefulWidget}; + + let mut entry = make_entry(LogLevel::Error, LogSource::App, "boom"); + entry.stack_trace = Some(wrap_invariant_trace()); + let entry_id = entry.id; + let logs = logs_from(vec![entry]); + + let mut collapse_state = CollapseState::new(); + let mut state = LogViewState::new(); + let area = Rect::new(0, 0, 30, 14); + let width = area.width as usize - 2; + + // default_collapsed(true): starts collapsed; toggle -> expanded -> collapsed. + for step in 0..3 { + { + let mut buf = Buffer::empty(area); + let view = LogView::new(&logs, test_icons()) + .show_timestamps(false) + .show_source(false) + .default_collapsed(true) + .max_collapsed_frames(2) + .collapse_state(&collapse_state) + .wrap_mode(true); + StatefulWidget::render(view, area, &mut buf, &mut state); + } + + let check_view = LogView::new(&logs, test_icons()) + .show_timestamps(false) + .show_source(false) + .default_collapsed(true) + .max_collapsed_frames(2) + .collapse_state(&collapse_state); + let expected = expected_total_lines(&check_view, &logs, width); + assert_eq!( + state.total_lines, expected, + "total_lines must be exact immediately after collapse-toggle step {step}" + ); + + collapse_state.toggle(entry_id, true); + } +} + +/// Coherence: entering link-highlight mode adds a bypassed (never-cached) +/// linked entry alongside cached non-linked entries; exiting link mode must +/// fall back to the (still-valid) non-linked cache without drift. +#[test] +fn test_row_cache_coherent_after_link_mode_enter_and_exit() { + use ratatui::{buffer::Buffer, layout::Rect, widgets::StatefulWidget}; + + let logs = row_cache_mixed_logs(); // entry 0's message is "short" + let mut state = LogViewState::new(); + let area = Rect::new(0, 0, 40, 14); + let width = area.width as usize - 2; + + // Render without link mode to populate the cache. + { + let mut buf = Buffer::empty(area); + StatefulWidget::render( + row_cache_test_view(&logs).wrap_mode(true), + area, + &mut buf, + &mut state, + ); + } + let expected_no_link = expected_total_lines(&row_cache_test_view(&logs), &logs, width); + assert_eq!(state.total_lines, expected_no_link); + + // Enter link mode: entry 0 carries a message-level badge and must bypass + // the cache, while entries 1 and 2 keep using their existing cache hits. + let link_state = make_link_state(&[(0, None, '1', "short")]); + { + let mut buf = Buffer::empty(area); + let view = row_cache_test_view(&logs) + .wrap_mode(true) + .link_highlight_state(&link_state); + StatefulWidget::render(view, area, &mut buf, &mut state); + } + let expected_with_link = { + let check_view = row_cache_test_view(&logs).link_highlight_state(&link_state); + expected_total_lines(&check_view, &logs, width) + }; + assert_eq!( + state.total_lines, expected_with_link, + "total_lines must be exact while link mode is active (badge bypass)" + ); + + // Exit link mode: the next render must be exact again. + { + let mut buf = Buffer::empty(area); + StatefulWidget::render( + row_cache_test_view(&logs).wrap_mode(true), + area, + &mut buf, + &mut state, + ); + } + assert_eq!( + state.total_lines, expected_no_link, + "total_lines must return to the no-link value after exiting link mode" + ); +} + +/// Coherence: entries evicted from the log buffer (`pop_front`, as a live +/// session's ring buffer would do) leave stale ids behind in `row_cache` +/// until the pruning pass runs; regardless, the render for the *remaining* +/// entries must stay exact. +#[test] +fn test_row_cache_coherent_after_buffer_eviction() { + use ratatui::{buffer::Buffer, layout::Rect, widgets::StatefulWidget}; + + let mut logs = make_logs_no_traces(8); + let mut state = LogViewState::new(); + let area = Rect::new(0, 0, 40, 14); + let width = area.width as usize - 2; + + { + let mut buf = Buffer::empty(area); + StatefulWidget::render( + row_cache_test_view(&logs).wrap_mode(true), + area, + &mut buf, + &mut state, + ); + } + assert_eq!(state.row_cache.len(), 8); + + logs.pop_front(); + logs.pop_front(); + logs.pop_front(); + + { + let mut buf = Buffer::empty(area); + StatefulWidget::render( + row_cache_test_view(&logs).wrap_mode(true), + area, + &mut buf, + &mut state, + ); + } + + let expected = expected_total_lines(&row_cache_test_view(&logs), &logs, width); + assert_eq!( + state.total_lines, expected, + "total_lines must be exact for the remaining entries after eviction" + ); +} + +/// Acceptance criterion #2 (load-bearing): a message-level link whose badge +/// pushes the message line across a wrap boundary — the deferred #73-class +/// drift this cache's linked-entry bypass closes. This test MUST fail if +/// the bypass in `entry_display_rows_cached` is removed (verified manually +/// during implementation by disabling it and confirming the failure, then +/// restoring it). +#[test] +fn test_row_cache_message_badge_exactness_at_wrap_boundary() { + // 38-char message fits in one row at width 40 without a badge; the + // 3-char "[c]" badge inserted mid-line pushes it to 41 chars -> 2 rows. + let entry = make_entry(LogLevel::Error, LogSource::App, &"a".repeat(38)); + let logs = logs_from(vec![entry]); + let width = 40; + + let link_state = make_link_state(&[(0, None, '1', "aaa")]); + let view = LogView::new(&logs, test_icons()) + .show_timestamps(false) + .show_source(false) + .wrap_mode(true) + .link_highlight_state(&link_state); + + // Sanity: without the badge, the message fits in exactly one row. + let plain_view = LogView::new(&logs, test_icons()) + .show_timestamps(false) + .show_source(false); + let baseline = rendered_row_count(&plain_view, &logs[0], 0, width); + assert_eq!( + baseline, 1, + "sanity: message alone fits one row, got {baseline}" + ); + + // Ground truth: what the render loop actually produces with the badge. + let rendered = rendered_row_count(&view, &logs[0], 0, width); + assert_eq!( + rendered, 2, + "expected the badge to push the message to 2 rows, got {rendered}" + ); + + // The cached-lookup helper must equal the exact rendered count while + // link mode is active. + let mut state = LogViewState::new(); + let estimate = view.entry_display_rows_cached(&mut state, &logs[0], 0, width); + assert_eq!( + estimate, rendered, + "estimate must include the message badge width while link mode is active, \ + estimate={estimate} rendered={rendered}" + ); + assert!( + state.row_cache.is_empty(), + "linked entries must never be written to the cache" + ); +} + +/// Acceptance criterion #3: pruning is bounded and correct. Grows the cache +/// past the `2 * filtered_len + 64` threshold by shrinking the buffer via +/// `pop_front` (simulating max-logs eviction) and asserts no stale id +/// survives the next render's pruning pass, while every live id does. +#[test] +fn test_row_cache_pruning_drops_stale_ids_past_threshold() { + use ratatui::{buffer::Buffer, layout::Rect, widgets::StatefulWidget}; + + let mut logs = make_logs_no_traces(100); + let mut state = LogViewState::new(); + let area = Rect::new(0, 0, 40, 14); + + { + let mut buf = Buffer::empty(area); + StatefulWidget::render( + row_cache_test_view(&logs).wrap_mode(true), + area, + &mut buf, + &mut state, + ); + } + assert_eq!(state.row_cache.len(), 100); + + // Simulate a max_logs-style ring-buffer eviction: drop all but the last + // 5 entries. Eviction itself does not touch the cache — only the next + // render's pruning pass does. + while logs.len() > 5 { + logs.pop_front(); + } + let live_ids: std::collections::HashSet = logs.iter().map(|e| e.id).collect(); + let front_id = logs.front().unwrap().id; + + { + let mut buf = Buffer::empty(area); + StatefulWidget::render( + row_cache_test_view(&logs).wrap_mode(true), + area, + &mut buf, + &mut state, + ); + } + + // Threshold = 2 * filtered_len(5) + 64 = 74; the pre-prune size (100) + // exceeds it, so pruning must have fired during this render. + assert_eq!( + state.row_cache.len(), + 5, + "stale ids must be pruned once the cache outgrows the threshold" + ); + assert!( + state.row_cache.keys().all(|id| *id >= front_id), + "no cached id may be older than the current front id" + ); + for id in &live_ids { + assert!( + state.row_cache.contains_key(id), + "live id {id} must survive pruning" + ); + } +} + +/// #75 review round 0 (minor): after a full log clear the buffer is empty, so +/// there is no front id to prune against — the old `unwrap_or(0)` fallback +/// made the retain a no-op (all u64 ids are >= 0) and a large pre-clear cache +/// lingered indefinitely. An empty buffer must clear the cache outright. +#[test] +fn test_row_cache_cleared_when_buffer_empties() { + use ratatui::{buffer::Buffer, layout::Rect, widgets::StatefulWidget}; + + let mut logs = make_logs_no_traces(100); + let mut state = LogViewState::new(); + let area = Rect::new(0, 0, 40, 14); + + { + let mut buf = Buffer::empty(area); + StatefulWidget::render( + row_cache_test_view(&logs).wrap_mode(true), + area, + &mut buf, + &mut state, + ); + } + assert_eq!(state.row_cache.len(), 100); + + // Clear-logs equivalent: the buffer empties entirely. + logs.clear(); + + { + let mut buf = Buffer::empty(area); + StatefulWidget::render( + row_cache_test_view(&logs).wrap_mode(true), + area, + &mut buf, + &mut state, + ); + } + + // Threshold = 2 * 0 + 64 = 64; pre-clear size (100) exceeds it, so the + // pruning pass runs — and with no front entry it must drop everything. + assert!( + state.row_cache.is_empty(), + "an emptied buffer must clear the row cache, got {} stale entries", + state.row_cache.len() + ); +} diff --git a/docs/REVIEW_FOCUS.md b/docs/REVIEW_FOCUS.md index 3860228f..2c75ad40 100644 --- a/docs/REVIEW_FOCUS.md +++ b/docs/REVIEW_FOCUS.md @@ -40,6 +40,7 @@ handler layer. These fields: - `SettingsViewState::scroll_offset` — `resolve_scroll` (settings_panel/mod.rs) clamps this each render so the selected row stays within the viewport; the mouse-region recorder reads the same value to keep click rects aligned. A plain `usize` (not `Cell`) because `StatefulWidget::render` already receives `&mut SettingsViewState`. Default 0 (safe before any render). - `LogViewState` drag-selection geometry fields (`selection_rows`, `content_top_y`, `content_bottom_y`, `selection_top`, `selection_bottom`) — the log-view renderer publishes, each frame, the screen-rect→document mapping for every visible logical line plus the content area's vertical bounds and first/last visible line identities (`widgets/log_view/mod.rs`, `render_inner` publish block). The mouse handlers (`handler/mouse/selection.rs`) read them to convert pointer cells into `SelPoint`s and to arm edge auto-scroll; `tick_selection_autoscroll` reads the edge identities to extend the selection focus while scrolled past a viewport edge. Plain fields on `&mut LogViewState` (same `StatefulWidget` shape as `SettingsViewState::scroll_offset`). Safe defaults: empty `Vec`, 0, `None` — with no render yet, `locate_selection_point` finds nothing and no selection can start. - `LogViewState::selection_text` + `selection_text_key` — the renderer caches the full WYSIWYG text of the current non-empty selection (rebuilt only when the selection changes; `selection_text_key` is the cache key). Unlike the numeric hints above, this is the actual clipboard payload read by the `CopySelection` handler. The invariant that makes it sound: **the event loop renders before reading each input event**, so by the time a release/`c`-key reaches the handler, the most recent render has already measured the exact selection being copied — the copied text therefore always matches the on-screen highlight. `handle_release` deliberately does not re-snap the selection focus at release time so the selection it completes is identical to the one the last render measured. Cleared to `None` whenever the selection is empty, and by `clear_selection()` (session switch, `clear_logs`, filter changes, capture-off). +- `LogViewState::row_cache` + `row_cache_key` — in wrap mode, the log-view renderer's `render_inner` caches each entry's computed display-row count (`row_cache: HashMap`) to avoid re-walking the whole filtered buffer every frame. No handler-side invalidation exists by design: coherence is entirely lookup-time. A global key `(content width, wrap_mode)` clears the map wholesale on mismatch (resize or the first wrap-mode render); each hit additionally compares the cached `expanded` flag against the entry's current collapse state, so a collapse/expand toggle produces a guaranteed miss-and-recompute rather than a staleness hazard. Entries carrying active link badges (link mode only, ≤35, always viewport-visible) bypass the cache entirely and are measured exactly from the formatted lines, since badge width isn't captured by the cache key. This is sound because rendered char content depends only on immutable entry text plus `expanded` and link badges — level and search affect style only, and timestamps/source are hardcoded `true` (config field `UiSettings::show_timestamps` exists but is unwired); if either is ever wired to config, it must join the global key. The same assumption covers a collapsed entry's visible row count (`min(max_collapsed_frames, frame_count)` + indicator): `max_collapsed_frames` is a compile-time default of 3 that production never overrides (`render/mod.rs:256` builds `LogView` without calling `.max_collapsed_frames()`), but if it's ever wired to config it too must join the global key or force invalidation. Plain fields on `&mut LogViewState` (same `StatefulWidget` shape as `SettingsViewState::scroll_offset` / the selection fields above). Safe defaults: empty map / `None` (no render yet → every entry misses, still correct). Opportunistically pruned by `retain`ing ids ≥ the front entry's id once the map outgrows a threshold — memory hygiene, not a correctness mechanism; that threshold is filtered-view-relative (`2 × filtered_len + 64`, not buffer-relative), so after a filter narrows sharply the cache can transiently hold entries up to roughly buffer size until the next prune, bounded by `max_logs`. A fully-empty buffer clears the cache outright rather than relying on that prune, since the empty-log early return would otherwise skip it. New `Cell`-based or render-written hint fields require explicit review and documentation here. diff --git a/workflow/plans/features/row-count-cache/PLAN.md b/workflow/plans/features/row-count-cache/PLAN.md new file mode 100644 index 00000000..e47d41b5 --- /dev/null +++ b/workflow/plans/features/row-count-cache/PLAN.md @@ -0,0 +1,75 @@ +# Plan: per-entry display-row-count cache for the log view (issue #75) + +## TL;DR + +In wrap mode the scroll-bounds pass grapheme-walks the ENTIRE filtered buffer every frame (~20 fps) to compute `total_lines`. Research shows an entry's row count depends on exactly two frame-variable inputs — expanded state and link badges — so a lookup-time-keyed cache needs **zero handler-side invalidation wiring**: global key `(width, wrap_mode)`, per-entry key `expanded`, and exact formatted-line measurement for the ≤35 viewport-scoped linked entries while link mode is active. This also closes the last #73-class drift (message-line badges) and takes the two small deferred alignments. + +--- + +## Background + +#74 made `calculate_entry_display_rows` exact but kept it O(filtered buffer) per frame (message + frame-line grapheme walks, mod.rs:1785-1795 loop). Steady-state buffers don't change frame to frame, so ~all of that work is redundant. Deferred from #73's review: this cache, message-line badge measurement, constant derivation, collapse-helper extraction (issue #75). + +## Affected Modules + +- `crates/fdemon-app/src/log_view_state.rs` — cache storage on `LogViewState` (render-written, same `&mut` StatefulWidget shape as the approved `selection_text` fields). +- `crates/fdemon-tui/src/widgets/log_view/mod.rs` — cache consult/populate in the two `calculate_entry_display_rows` call paths; exact formatted-line path for linked entries; constants derivation; collapse-visibility helper. +- `docs/REVIEW_FOCUS.md` — registry entry for the new render-written fields (**doc_maintainer task**; implementor may not edit). + +## Development Phases + +### Phase 1: cache + exact linked-entry path (single implementation task) + +**Goal:** `total_lines` cost becomes O(changed entries) in steady state, exact everywhere including link mode. + +#### Steps + +1. **Cache storage** on `LogViewState`: + - `row_cache: HashMap` where `CachedRows { expanded: bool, rows: u16 }` + - `row_cache_key: Option<(u16 /*content width*/, bool /*wrap_mode*/)>` + - Cleared wholesale when the global key mismatches at render start. Safe defaults (empty/None) per the render-hint rules. +2. **Lookup path** (render_inner, both the total_lines loop and the render loop — they share it): + - Entry has any link (`links.iter().any(|l| l.entry_index == idx)`, only when `link_highlight_state` is active): **bypass cache**, measure exactly by formatting the entry's lines (message via `format_entry`, frames/indicator via the existing formatters) and summing `line_wrapped_row_count`. Bounded: ≤35 links, all visible. Do not write these to the cache. + - Otherwise: hit if `cached.expanded == is_entry_expanded(entry)` → reuse `rows`; miss → compute via the existing exact iterators (#74) and store. +3. **Pruning:** when `row_cache.len()` exceeds a threshold (e.g. `2 * filtered_len + 64`), `retain(|id, _| *id >= front_id)`. Memory hygiene only — evicted ids are never queried. +4. **Message-line badge exactness** falls out of step 2's bypass (no injection math; per-span badge-suppression corner handled by construction). +5. **Small alignments** (same task): derive the `4`/`25` literals in `frame_line_widths` from `styles::INDENT.len()` / `"".len()`; extract the collapse-visibility computation (`is_expanded`/`visible_frames`/`has_indicator`) into a helper shared by `calculate_entry_display_rows` and `calculate_entry_lines` (also trims the 60-line function). + +**Milestone:** steady-state wrap-mode frames do zero grapheme work on unchanged, unlinked entries; #74's invariant test still green; link-mode estimate now exact for message badges too. + +### Phase 2: REVIEW_FOCUS.md registry entry (doc_maintainer) + +Document `row_cache` + `row_cache_key` in the "Current usage" list: render-written plain fields on `&mut LogViewState`, numeric-per-entry hints, safe empty defaults, lookup-time keying rationale, and the note that `show_timestamps`/`show_source` (currently hardcoded true) must join the global key if ever wired to config. + +## Edge Cases & Risks + +### Stale rows on unforeseen content change +- **Risk:** some input outside (expanded, links, width, wrap) changes rendered chars. +- **Mitigation:** research enumerated the inputs — level is style-only, search is style-only, message/trace immutable, timestamps/source hardcoded. The #74 invariant test (estimate == rendered) plus new cache-specific tests are the backstop; a cache-coherence test renders twice with a toggled input and asserts equality. + +### Link-mode filter staleness (pre-existing, out of scope) +- `rescan_links_if_active` doesn't run on filter changes, so links (raw indices into the filtered view) can go stale in link mode **today**, badges included. The cache bypass uses the same (possibly stale) links the formatter uses, so estimate == render still holds. File the rescan gap as its own issue; do not fix here. + +### Eviction + raw link indices (pre-existing, noted) +- Raw `entry_index` shifts on `pop_front` eviction while link mode is active — badges can attach to wrong entries (formatter and estimate identically). Same bucket as above; out of scope. + +## Success Criteria + +### Phase 1 Complete When: +- [ ] Steady-state test: second render with unchanged state performs zero recomputation (observable via a test-only counter or by asserting cache hits/len) +- [ ] Cache-coherence tests: width change, wrap toggle, expand/collapse toggle, link-mode enter/exit, eviction — each followed by estimate == rendered rows (reuse #74's `rendered_row_count` ground truth) +- [ ] Link-mode message-badge test: message-level link at a wrap boundary → estimate == rendered (this was the deferred #73-class drift) +- [ ] Pruning test: after eviction past threshold, cache contains no ids < front id +- [ ] All #74 tests (invariant, regression, scroll-bounds, frame-badge, region alignment) still green; fmt/clippy (1.96 and 1.97) clean +- [ ] No allocations added to the non-linked steady-state path + +### Phase 2 Complete When: +- [ ] REVIEW_FOCUS.md registry entry present, following the existing entries' format + +## Further Considerations + +1. **File separately after this lands:** link rescan-on-filter-change gap (+ raw-index eviction shift) — pre-existing link-mode staleness, both formatter and estimate affected equally. + +## Task Dependency Graph + +T1 (implementation, `medium`) → T2 (doc_maintainer, `low`, depends on T1). Both sequential in the main loop; no file overlap (T1: code files; T2: docs/REVIEW_FOCUS.md only). Branch `feat/75-row-count-cache` is stacked on #74's branch — rebase onto main after #74 merges, before opening the PR. diff --git a/workflow/plans/features/row-count-cache/TASKS.md b/workflow/plans/features/row-count-cache/TASKS.md new file mode 100644 index 00000000..6bae9ef3 --- /dev/null +++ b/workflow/plans/features/row-count-cache/TASKS.md @@ -0,0 +1,44 @@ +# Tasks: row-count-cache (issue #75) + +Plan: [PLAN.md](PLAN.md) · Research: [research/RESEARCH.md](research/RESEARCH.md) +PHASE_BASE: eeb87633 (branch `feat/75-row-count-cache`, stacked on PR #74 head) + +## File Overlap Analysis + +| Task | Files Modified (Write) | Read-only deps | +|------|------------------------|----------------| +| 01-row-count-cache | `crates/fdemon-app/src/log_view_state.rs`, `crates/fdemon-tui/src/widgets/log_view/mod.rs`, `crates/fdemon-tui/src/widgets/log_view/tests.rs` | `crates/fdemon-app/src/hyperlinks.rs`, `crates/fdemon-app/src/session/{session,collapse}.rs` | +| 02-review-focus-registry | `docs/REVIEW_FOCUS.md` | task 01's diff | + +### Overlap Matrix + +01 vs 02: no shared write files, but 02 documents 01's output → **dependency chain, sequential**. Strategy: sequential (main loop), no worktrees. + +## Wave 1 + +- [x] **01-row-count-cache** — `tasks/01-row-count-cache.md` + - Complexity: medium + - Agent: implementor (sequential, main loop) + - Done: commit 59f76f3c, validator PASS (sentinel probe + load-bearing badge test verified; both-toolchain gates green) + +## Wave 2 (after 01) + +- [x] **02-review-focus-registry** — `tasks/02-review-focus-registry.md` + - Complexity: low + - Agent: doc_maintainer + - Done: commit 671ddb37, validated by inspection (single entry, all content requirements present, correct placement) + +## Notes (additional) + +- doc_maintainer flag: docs/REVIEW_FOCUS.md is at ~294 lines vs its 200-line cap (pre-existing bloat, mostly "Approved Optimizations") — schedule a docs-sync/compaction pass separately. + +## Notes + +- Branch stacked on #74 — rebase onto main after #74 merges, before opening the PR. +- Out of scope (file as new issue after landing): link rescan-on-filter-change gap + raw-index eviction shift (pre-existing link-mode staleness; formatter and estimate affected identically). + +## Phase Review + +| Round | Verdict | Review | Reviewed HEAD | +|-------|---------|--------|---------------| +| 0 | ✅ APPROVED_WITH_CONCERNS | ../../../reviews/row-count-cache/REVIEW.md | 671ddb37 (+ inline polish 261609fd, doc addendum d14be711) | diff --git a/workflow/plans/features/row-count-cache/research/RESEARCH.md b/workflow/plans/features/row-count-cache/research/RESEARCH.md new file mode 100644 index 00000000..33bf036d --- /dev/null +++ b/workflow/plans/features/row-count-cache/research/RESEARCH.md @@ -0,0 +1,28 @@ +# Research: per-entry row-count cache (issue #75) + +Two targeted codebase researchers (2026-07-14, branch `feat/75-row-count-cache` @ eeb87633, stacked on PR #74). Supplements the verified #73 sweep (`../../bugs/wrap-scroll-bounds-drift/research/RESEARCH.md`), which already covers the call graph, frame-line shapes, and eviction/level-mutation sites. + +## Q1 — What can change an entry's rendered char content between frames? + +- **No level icon exists in the line text** — `entry.level` affects styling only (`format_entry`, mod.rs:407-458; `_level_style` unused in span text). ⇒ **The `add_log` level retro-mutation is irrelevant to row counts** and drops out of the cache key. +- Message char content is immutable post-ingestion; search highlighting splits spans but changes **style only, not chars** (`format_message_with_highlights`). +- `estimate_prefix_width` (mod.rs:841-858) is **exact** vs `format_entry` for all levels (all-ASCII prefix). +- `show_timestamps`/`show_source`: builder methods exist but the render path never calls them — **hardcoded true** (render/mod.rs:256+; config field `UiSettings::show_timestamps` exists, unwired). Treat as constants; if ever wired, they become global-key inputs (document in REVIEW_FOCUS entry). +- `is_entry_expanded`: O(1) HashSet lookup (`CollapseState`, collapse.rs:23-31; Session field session.rs:107), available where `calculate_entry_display_rows` runs. +- Eviction pruning signal: `logs.front().map(|e| e.id)` — ids are process-global monotonic (`LOG_ENTRY_COUNTER`), so `id < front_id` ⇔ evicted. + +⇒ Per-entry variability reduces to exactly two inputs: **expanded state** and **link badges**. + +## Q2 — Link badge semantics (`LinkHighlightState`) + +- `DetectedLink.entry_index` is a **RAW VecDeque index** (scan_viewport, hyperlinks.rs:285-380 stores the raw `idx` from `filtered_indices`), matching the render loop's `idx` — consistent with #74's frame badge lookup. +- Links list: **max 35** (`MAX_LINK_SHORTCUTS`), **viewport-scoped** — only visible entries ever carry badges. +- Rebuilt ONLY on: link-mode entry (`EnterLinkMode`, update.rs:953-981) and every scroll (`rescan_links_if_active`, scroll.rs:128-155). NOT on filter change (**pre-existing staleness bug** — file separately) and NOT on new-log arrival. Cleared on exit (`deactivate()` clears `links`). +- Message badge: 3 chars `[c]` spliced **mid-line** before the matched `display_text` (insert_link_badge_into_spans, mod.rs:362-404 — per-span text search, so a search-highlight span split can suppress the match). +- Ownership: `Session.link_highlight_state` (session.rs:110), passed to LogView by reference only when active (render/mod.rs:266-269). + +## Design consequence (validated) + +- **Non-linked entries**: rows depend only on (message text, trace shape, expanded, width, wrap_mode) → cacheable with per-entry key `expanded: bool` and global key `(width, wrap_mode)`. No handler wiring needed anywhere — everything is checkable at lookup time. +- **Linked entries** (≤35, always visible, only while link mode active): measure **exactly from formatted lines** (`format_entry` + `format_stack_frame_line_with_links` + `line_wrapped_row_count`) instead of badge-width injection math — bounded allocations, byte-exact with the renderer by construction (including the search-split badge-suppression corner that injection math would get wrong). +- Pruning: opportunistic `retain(id >= front_id)` when the map outgrows a threshold; stale ids are never queried, so this is memory hygiene, not correctness. diff --git a/workflow/plans/features/row-count-cache/tasks/01-row-count-cache.md b/workflow/plans/features/row-count-cache/tasks/01-row-count-cache.md new file mode 100644 index 00000000..a4231590 --- /dev/null +++ b/workflow/plans/features/row-count-cache/tasks/01-row-count-cache.md @@ -0,0 +1,108 @@ +# Task 01: Per-entry display-row-count cache + exact linked-entry path + +Implements https://github.com/edTheGuy00/fdemon/issues/75. Read ../PLAN.md and ../research/RESEARCH.md FIRST — the design is fully decided there; this file is the operational spec. + +Repo: /home/ed/Dev/personal/fdemon-pro/fdemon, branch `feat/75-row-count-cache` (already checked out; stacked on PR #74). +Build/tests: `CARGO_TARGET_DIR=/data/cache/target/fdemon-73 cargo test -p fdemon-tui` / `-p fdemon-app`; gates also include `cargo fmt --all -- --check`, `cargo clippy --workspace --all-targets -- -D warnings` AND `cargo +1.97.0 clippy --workspace --all-targets -- -D warnings` (toolchain installed). + +## Changes + +### A. Cache storage — `crates/fdemon-app/src/log_view_state.rs` + +New plain fields on `LogViewState` (same render-written `&mut` StatefulWidget shape as the existing selection fields — do NOT use Cell): +- `pub row_cache: HashMap` with `pub struct CachedRows { pub expanded: bool, pub rows: u16 }` +- `pub row_cache_key: Option<(u16, bool)>` — (content width, wrap_mode) +Safe defaults: empty map / None. Doc comments must state: render-written; lookup-time keyed; REVIEW_FOCUS.md registry entry pending (task 02). + +### B. Cache consult/populate — `crates/fdemon-tui/src/widgets/log_view/mod.rs` + +In `render_inner`, wrap-mode only: +1. At render start (where `visible_width` is known): if `state.row_cache_key != Some((visible_width as u16, wrap_mode))` → `row_cache.clear()`, set key. (Nowrap mode: leave cache untouched — nowrap uses `calculate_entry_lines`, not measured widths.) +2. Replace the two direct `calculate_entry_display_rows` calls (total_lines loop + render loop) with a cached wrapper (helper fn or inline closure; both call sites MUST share it): + - **Linked entry** (only when `self.link_highlight_state` is `Some` and `links.iter().any(|l| l.entry_index == idx)`): bypass cache — measure exactly by formatting the entry's actual lines (message via `format_entry(entry, idx)` — which inserts the message badge — frames via `format_stack_frame_line_with_links`, indicator via `format_collapsed_indicator`, respecting expand/collapse) and summing `line_wrapped_row_count`. Do NOT insert into cache. (≤35 links, all visible — bounded allocations.) + - **Otherwise**: `expanded = is_entry_expanded(entry)`; hit iff cache entry exists with matching `expanded` → reuse; miss → compute via the EXISTING exact iterator path (#74's `calculate_entry_display_rows` internals) and insert `{expanded, rows}`. +3. Pruning: after the total_lines loop, if `row_cache.len() > 2 * filtered_indices.len() + 64`, `retain(|id, _| *id >= front_id)` where `front_id = self.logs.front-most id` (logs are ordered; take from the first entry, or pass in). Keep it allocation-free. +4. The cached `rows` MUST be exactly what `calculate_entry_display_rows` returns today — no behavior change for cache misses. + +### C. Small alignments (same commit or a second commit, your call) + +- `frame_line_widths`: replace the bare `4` INDENT literals with a const derived from `styles::INDENT` (e.g. `styles::INDENT.len()` — INDENT is 4 ASCII spaces so len==width) and `25` with `"".len()`. +- Extract the collapse-visibility computation (`is_expanded` / `visible_frames` / `has_indicator` / `hidden_count`) into one private helper used by BOTH `calculate_entry_display_rows` and `calculate_entry_lines` (dedupes the rules; trims the 60-line function). + +## Tests (crates/fdemon-tui/src/widgets/log_view/tests.rs, existing style; reuse `rendered_row_count` ground truth from #74) + +1. **Steady-state hit test**: render twice with identical state; after the second render assert the cache is populated and (via a probe: e.g. mutate a cached `rows` value to a sentinel between renders and assert `total_lines` reflects the sentinel) prove the second render READ the cache rather than recomputing. Then invalidate (width change) and assert the sentinel is gone (recomputed correctly). +2. **Coherence tests** — for each of: width change, wrap toggle, expand/collapse toggle (`CollapseState`), link-mode enter (badge appears) and exit, eviction of cached entries: perform the change, re-render, assert estimate == `rendered_row_count` per entry and correct `total_lines`. +3. **Message-badge exactness** (the deferred #73 drift): message-level link whose badge pushes the message line across a wrap boundary → estimate == rendered while link mode active (this test MUST fail if the linked-entry bypass is removed — verify by temporarily disabling it, then restore). +4. **Pruning test**: with a small `max_logs`-style setup (drive eviction via ids), grow the cache past threshold, render, assert no cached id < front id and live ids survive. +5. All #74 tests stay green (invariant, regression, scroll-bounds, frame-badge-boundary, region-alignment). + +## Constraints + +- No allocations on the non-linked steady-state path (cache hit = one HashMap get; miss = existing iterator math + one insert). +- Zero behavior change to rendered output — this is measurement/caching only. +- No edits to `docs/REVIEW_FOCUS.md` (task 02, doc_maintainer) or any other core doc. +- Gates: fmt, clippy on BOTH default and +1.97.0 toolchains, full `-p fdemon-tui` and `-p fdemon-app` suites. +- Commit source only: `git add --all -- . ':!workflow/plans/'`; message `perf(tui): cache per-entry display row counts in wrap mode (#75)` (second commit for the alignments OK: `refactor(tui): derive frame-line constants, share collapse-visibility helper`). + +## Acceptance criteria + +- [x] Steady-state render does zero row recomputation for unchanged unlinked entries (proven by the sentinel probe test) +- [x] All coherence tests pass; message-badge exactness test passes and is load-bearing +- [x] Pruning bounded; no stale ids after threshold pass +- [x] All #74 tests green; both-toolchain clippy + fmt clean; full suites green +- [x] No REVIEW_FOCUS.md/doc edits; commits exclude workflow/plans + +--- + +## Completion Summary + +**Status:** Done +**Branch:** `feat/75-row-count-cache` +**Commit:** `59f76f3c` — `perf(tui): cache per-entry display row counts in wrap mode (#75)` + +### Files Modified + +| File | Changes | +|------|---------| +| `crates/fdemon-app/src/log_view_state.rs` | Added `pub struct CachedRows { expanded: bool, rows: u16 }` and two new `LogViewState` fields: `pub row_cache: HashMap`, `pub row_cache_key: Option<(u16, bool)>`. Safe defaults (`HashMap::new()` / `None`) wired into `LogViewState::new()`. Doc comments state render-written / lookup-time-keyed / REVIEW_FOCUS.md entry pending (task 02), per spec. | +| `crates/fdemon-tui/src/widgets/log_view/mod.rs` | Added `collapse_visibility()` (shared `is_expanded`/`visible_frames`/`has_indicator`/`hidden_count` computation, now used by `calculate_entry_lines`, `calculate_entry_display_rows`, and the new `measure_entry_exact_rows`). Added `measure_entry_exact_rows()` (exact bypass measurement for linked entries: `format_entry` + `format_stack_frame_line_with_links` + `format_collapsed_indicator`, summed via `line_wrapped_row_count`). Added `entry_display_rows_cached()` — the single cached-lookup helper shared by both `render_inner` call sites (total_lines loop and the render loop's `entry_units`): bypasses the cache for linked entries, otherwise hits on matching `expanded` state or misses into `calculate_entry_display_rows` + insert. Added cache-key check/clear at render start (wrap mode only) and a pruning pass (`retain(id >= front_id)`) after the total_lines loop when `row_cache.len() > 2 * filtered_len + 64`. Section C alignments: `frame_line_widths`'s two bare `4` literals replaced by `FRAME_INDENT_WIDTH = styles::INDENT.len()`, and `25` by `ASYNC_GAP_TEXT_LEN = "".len()`. | +| `crates/fdemon-tui/src/widgets/log_view/tests.rs` | 8 new tests (see below) plus 3 shared test helpers: `row_cache_test_view`, `expected_total_lines`, `row_cache_mixed_logs`. | + +### New tests (all passing) + +- `test_row_cache_sentinel_probe_proves_second_render_reads_cache` — sentinel-probe proof: overwrites a cached `rows` value between two identical renders and asserts `total_lines` reflects the sentinel exactly on the second render (proving the cache is read, not recomputed), then invalidates via width change and asserts both the sentinel is gone and the recomputed total matches a fresh render. +- `test_row_cache_coherent_after_width_change` +- `test_row_cache_coherent_after_wrap_toggle` +- `test_row_cache_coherent_after_expand_collapse_toggle` +- `test_row_cache_coherent_after_link_mode_enter_and_exit` +- `test_row_cache_coherent_after_buffer_eviction` +- `test_row_cache_message_badge_exactness_at_wrap_boundary` — load-bearing (see proof below). +- `test_row_cache_pruning_drops_stale_ids_past_threshold` + +### Load-bearing verification (manual, not left in the codebase) + +1. **Sentinel probe**: temporarily short-circuited the cache-read branch in `entry_display_rows_cached` (`if false { ... }` around the `state.row_cache.get(&entry.id)` lookup) — `test_row_cache_sentinel_probe_proves_second_render_reads_cache` failed with `left: 6, right: 10004` (the un-sentineled recomputed total vs. the expected sentinel-corrupted total), confirming the test actually detects a cache-bypassed implementation. Reverted; full suite green again. +2. **Message-badge bypass**: temporarily forced `is_linked = false && ...` in `entry_display_rows_cached` — `test_row_cache_message_badge_exactness_at_wrap_boundary` failed with `estimate=1 rendered=2` (the un-bypassed cached estimator missed the badge-induced wrap), confirming the test is load-bearing. Reverted; full suite green again. + +### Notable Decisions/Tradeoffs + +1. **`total_lines` loop uses a plain `for` loop instead of `.map().sum()`**: `entry_display_rows_cached` needs `&mut state` per call; threading a unique borrow through a `.map` closure across iterations is legal via implicit reborrow at call sites but is less readable than an explicit loop for this mutable-accumulator case. Chose clarity over iterator-chaining purity here. +2. **`CachedRows.rows: u16` uses a saturating (`u16::try_from(...).unwrap_or(u16::MAX)`) cast on insert**, per the spec's literal field type. A single log entry occupying >65535 wrapped terminal rows is not a realistic scenario (documented inline); a fresh cache-miss render in that pathological case would still be numerically correct for that one frame, only a subsequent cache *hit* would read back the saturated value. Not exercised by any test — flagged here as a known, spec-mandated limitation rather than silently deviating from the given field type. +3. **Section C alignment scope**: only `frame_line_widths`'s two bare literals were touched (`4`→`FRAME_INDENT_WIDTH`, `25`→`ASYNC_GAP_TEXT_LEN`), per the task's explicit scoping to that function. `collapsed_indicator_widths` has a similar bare `4` INDENT literal but was left untouched — out of the task's stated scope, not accidental. +4. **Combined both task changes (cache + alignments) into a single commit**: the spec explicitly allowed this ("same commit or a second commit, your call"). + +### Testing Performed + +- `CARGO_TARGET_DIR=/data/cache/target/fdemon-73 cargo fmt --all -- --check` — Passed (after running `cargo fmt --all` once to apply 2 formatting fixes) +- `CARGO_TARGET_DIR=/data/cache/target/fdemon-73 cargo check --workspace --all-targets` — Passed +- `CARGO_TARGET_DIR=/data/cache/target/fdemon-73 cargo clippy --workspace --all-targets -- -D warnings` (default toolchain) — Passed, 0 warnings +- `CARGO_TARGET_DIR=/data/cache/target/fdemon-73-1970 cargo +1.97.0 clippy --workspace --all-targets -- -D warnings` — Passed, 0 warnings +- `CARGO_TARGET_DIR=/data/cache/target/fdemon-73 cargo test -p fdemon-tui` — Passed, 1614 passed / 0 failed / 1 ignored (was 1606; +8 new) +- `CARGO_TARGET_DIR=/data/cache/target/fdemon-73 cargo test -p fdemon-app` — Passed, 3370 passed / 0 failed / 5 ignored (unchanged — no fdemon-app test-suite changes beyond the new `CachedRows`/field additions, which are covered indirectly by the fdemon-tui render tests) +- All pre-existing #74 tests (invariant, regression, scroll-bounds, frame-badge-boundary, region-alignment, click-region alignment) — still green, unmodified + +### Risks/Limitations + +1. **`u16` row-count truncation edge case** (see Decision #2 above): purely theoretical at realistic terminal widths and message lengths; no test exercises it since triggering it would require an entry occupying tens of thousands of wrapped rows. +2. **Pre-existing link-mode staleness** (documented in RESEARCH.md/PLAN.md, out of scope for this task): `rescan_links_if_active` doesn't run on filter changes, and raw `entry_index` link indices can drift on `pop_front` eviction while link mode is active. The cache bypass uses the same (possibly stale) links the formatter uses, so estimate == render still holds — this task does not change that pre-existing behavior. diff --git a/workflow/plans/features/row-count-cache/tasks/02-review-focus-registry.md b/workflow/plans/features/row-count-cache/tasks/02-review-focus-registry.md new file mode 100644 index 00000000..3857b93d --- /dev/null +++ b/workflow/plans/features/row-count-cache/tasks/02-review-focus-registry.md @@ -0,0 +1,26 @@ +# Task 02: REVIEW_FOCUS.md registry entry for the row-count cache + +Depends on task 01 (read its committed diff first: `git log --oneline -3`, `git show` the cache commit). Context: ../PLAN.md Phase 2, ../research/RESEARCH.md. + +Repo: /home/ed/Dev/personal/fdemon-pro/fdemon, branch `feat/75-row-count-cache`. + +## Change + +Edit `docs/REVIEW_FOCUS.md` ONLY. In the approved render-hint exception's "Current usage" list (where the `LogViewState` drag-selection geometry and `selection_text` entries from PR #72 live), add one entry for: + +- `LogViewState::row_cache` + `LogViewState::row_cache_key` — per-entry wrapped display-row-count cache, written by the log-view renderer during `render_inner` (wrap mode). Cover, matching the format and depth of the neighboring entries: + - Shape: plain fields on `&mut LogViewState` via StatefulWidget (same shape as `SettingsViewState::scroll_offset` / the selection fields; not `Cell`). + - Keying rationale (the reviewer-relevant invariant): NO handler-side invalidation exists by design — coherence comes from lookup-time keys: global `(content width, wrap_mode)` clears the map; per-entry `expanded` compared on every hit; entries with active links bypass the cache entirely and are measured from formatted lines (links are viewport-scoped, ≤35). + - Why that is sound: rendered char content depends only on immutable entry text + expanded + badges (level and search affect style only; timestamps/source hardcoded true). + - Future-key note: if `show_timestamps`/`show_source` are ever wired to config (`UiSettings::show_timestamps` exists, unwired), they MUST join the global key. + - Safe defaults: empty map / `None` (no render yet → all misses, correct). + - Pruning: opportunistic retain of ids ≥ front entry id (memory hygiene, not correctness). + +Keep it to one list entry consistent in length/tone with the existing ones. Do not restructure the document, do not touch any other section or file. + +## Acceptance criteria + +- [ ] Single new entry in the "Current usage" list, format-consistent with neighbors +- [ ] States the lookup-time-keying invariant, the link bypass, the future config-key note, safe defaults +- [ ] No other files or sections changed +- [ ] Commit: `docs(review-focus): register LogViewState row-count cache render-hint fields (#75)` (exclude workflow/plans) diff --git a/workflow/reviews/row-count-cache/REVIEW.md b/workflow/reviews/row-count-cache/REVIEW.md new file mode 100644 index 00000000..551e96be --- /dev/null +++ b/workflow/reviews/row-count-cache/REVIEW.md @@ -0,0 +1,38 @@ +# Review: row-count-cache (issue #75) + +**Verdict:** ✅ APPROVED_WITH_CONCERNS (round 0, terminal) +**Diff range:** `eeb87633..671ddb37` · **Reviewed HEAD:** 671ddb37 (+ inline polish 261609fd, doc addendum d14be711) +**Run:** review-diff workflow (5 dimensions, adversarial refuter panel) — 0 confirmed Critical/Major, 16 Minors (several duplicates/positive notes) + +## Per-dimension + +| Dimension | Verdict | Confirmed | Minors | +|-----------|---------|-----------|--------| +| architecture | PASS | 0 | 2 | +| quality | PASS | 0 | 4 | +| logic | PASS | 0 | 4 | +| risks | CONCERNS (minors only) | 0 | 5 | +| security | PASS | 0 | 1 | + +## Minors — disposition + +**Fixed inline post-review (commit 261609fd):** +1. Empty-buffer cache staleness (dedup of 3 findings): the reviewer flagged the `unwrap_or(0)` prune fallback as a no-op; the actual hole was one level up — the `logs.is_empty()` early return skips the pruning pass entirely. Fix: clear `row_cache` in that early return (filter-matches-nothing path deliberately keeps the cache — ids still live). Regression test `test_row_cache_cleared_when_buffer_empties` (failed against both the original code AND the reviewer's suggested fix location; green now). The prune-pass fallback also replaced with a match (defensive). +2. Stale "registry entry pending (task 02)" doc comments ×3 → reworded to point at the landed REVIEW_FOCUS.md entry. +3. `row_cache_key` second element hardcoded `true` → writes `self.wrap_mode`. +4. Bare `4` in `collapsed_indicator_widths` → `FRAME_INDENT_WIDTH`. +5. `debug_assert` at the u16 rows insert (silent-saturation landmine now fails loudly in test builds). + +**Documented (doc addendum d14be711):** +6. `max_collapsed_frames` is a hidden row-count input — compile-time constant 3 in production; registry entry now lists it as assumed-constant (must join the global key if ever wired to config). +7. Pruning bound is filtered-view-relative, not buffer-relative → noted in the registry entry, with the empty-buffer clear behavior. + +**Accepted, no action:** +8. Render-only cache shape (no handler reads) — registry entry covers this variant. +9. u16 saturation unreachable at real widths (debug_assert added anyway). +10. Pre-existing link `entry_index` staleness correctly carried forward, not worsened — separate issue to file (with the rescan-on-filter gap). +11. `log_view/mod.rs` at 2.6k lines (pre-existing; `row_cache.rs` split candidate next touch). REVIEW_FOCUS.md over its size cap (pre-existing; schedule docs compaction). + +## Verification highlights from the review + +Reviewers confirmed: both render_inner call sites share `entry_display_rows_cached`; the linked-entry bypass condition matches the formatters' badge conditions; the cache-miss path is byte-identical to #74's math; the sentinel and message-badge tests are honest (mutation-verified by the implementor); hit path does no grapheme work or allocation.