From a43ef673b07451f7d5c044302a2cd8e7bf5210f8 Mon Sep 17 00:00:00 2001 From: dailz Date: Thu, 11 Jun 2026 09:49:10 +0800 Subject: [PATCH] fix(tui): rebase v_offset before VHI invalidation to prevent viewport jump (closes #25) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ready state can have VHI=None during async rebuild (Tab toggle, resize, file append, Loading→Ready transition). Without rebase, v_offset retains a visual-row value that gets treated as a logical-line offset, causing viewport jumps and cursor drift. Changes: - Add rebase_offset_for_invalidate() to convert v_offset from visual-row to logical-line + sub-row before VHI invalidation - Call it at all 6 invalidation sites in Ready state - Recalibrate v_offset from viewport top when VHI rebuild completes - Clamp preserved sub_row to new line height after JSON/width changes Regression tests: 6 new tests covering rebase conversion, Tab toggle, VHI rebuild recalibration, sub-row clamping, and Loading→Ready transition. --- crates/tui/src/app.rs | 295 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 294 insertions(+), 1 deletion(-) diff --git a/crates/tui/src/app.rs b/crates/tui/src/app.rs index d6eebf5..7570ebe 100644 --- a/crates/tui/src/app.rs +++ b/crates/tui/src/app.rs @@ -659,12 +659,15 @@ impl App { self.json_format = !self.json_format; self.viewport_cache.invalidate(); let width = self.viewport_cache.width; + let (new_offset, new_sub) = self.rebase_offset_for_invalidate(); if let AppLoadingState::Ready { reader } = &mut self.loading_state { reader.invalidate_visual_height_index(); if width > 0 { reader.start_visual_height_rebuild(width, self.json_format); } } + self.v_offset = new_offset; + self.v_sub_offset = new_sub; self.last_g_press = None; } KeyCode::Char('s') | KeyCode::Char('S') @@ -823,6 +826,25 @@ impl App { } } + /// Compute the rebased offset pair (logical_line, sub_row) from the + /// current visual-row `v_offset`. Returns `(v_offset, v_sub_offset)` + /// suitable for the no-VHI fallback scrolling path. + /// + /// MUST be called before borrowing `&mut self.loading_state` for + /// `invalidate_visual_height_index`, because it reads VHI through + /// `&self`. + fn rebase_offset_for_invalidate(&self) -> (usize, usize) { + if self.get_visual_height_index().is_some() { + let top_visual = self.v_offset; + let top_line = self.visual_row_to_logical_row(top_visual); + let line_first_visual = self.cursor_to_first_visual_row(top_line); + let sub = top_visual.saturating_sub(line_first_visual); + (top_line, sub) + } else { + (self.v_offset, self.v_sub_offset) + } + } + fn ensure_visual_height_index(&mut self, width: usize) { let needs_rebuild = match self.get_visual_height_index() { Some(idx) => !idx.is_valid_for(self.json_format, width), @@ -830,10 +852,13 @@ impl App { }; if needs_rebuild { + let (new_offset, new_sub) = self.rebase_offset_for_invalidate(); if let AppLoadingState::Ready { reader } = &mut self.loading_state { reader.invalidate_visual_height_index(); reader.start_visual_height_rebuild(width, self.json_format); } + self.v_offset = new_offset; + self.v_sub_offset = new_sub; } } @@ -916,6 +941,7 @@ impl App { fn handle_file_appended(&mut self) { let width = self.get_content_width(); + let rebased = self.rebase_offset_for_invalidate(); match &mut self.loading_state { AppLoadingState::Ready { reader } => { let old_reader_line_count = reader.line_count(); @@ -966,6 +992,9 @@ impl App { index.extend_from_heights(&new_heights); } } else { + let (new_offset, new_sub) = rebased; + self.v_offset = new_offset; + self.v_sub_offset = new_sub; reader.invalidate_visual_height_index(); reader.start_visual_height_rebuild(width, self.json_format); } @@ -974,9 +1003,11 @@ impl App { } Ok(log_viewer_core::io::file_reader::AppendStatus::Reloaded) => { let _ = reader.save_cache(); + let (new_offset, _new_sub) = rebased; reader.invalidate_visual_height_index(); reader.start_visual_height_rebuild(width, self.json_format); self.cursor_line = self.cursor_line.min(self.total_lines().saturating_sub(1)); + self.v_offset = new_offset; self.v_sub_offset = 0; self.viewport_cache.invalidate(); self.clamp_v_offset(); @@ -993,12 +1024,14 @@ impl App { fn reload_ready_reader(&mut self) { let width = self.get_content_width(); + let (new_offset, _new_sub) = self.rebase_offset_for_invalidate(); if let AppLoadingState::Ready { reader } = &mut self.loading_state { let _ = reader.reload(); let _ = reader.save_cache(); reader.invalidate_visual_height_index(); reader.start_visual_height_rebuild(width, self.json_format); self.cursor_line = self.cursor_line.min(self.total_lines().saturating_sub(1)); + self.v_offset = new_offset; self.v_sub_offset = 0; self.viewport_cache.invalidate(); self.clamp_v_offset(); @@ -1030,10 +1063,23 @@ impl App { } = &mut reader.state { *visual_height_index = Some(index); - self.viewport_cache.invalidate(); } } } + // Recalibrate v_offset from logical → visual-row now that VHI is available. + // Must be outside the `reader` borrow above. + if self.get_visual_height_index().is_some() { + let logical_top = self.v_offset.min(self.total_lines().saturating_sub(1)); + let sub = self.v_sub_offset; + let first_visual = self.cursor_to_first_visual_row(logical_top); + let line_height = self + .get_visual_height_index() + .map_or(1, |idx| idx.visual_height_of_line(logical_top)); + self.v_offset = first_visual.saturating_add(sub.min(line_height.saturating_sub(1))); + self.v_sub_offset = 0; + self.clamp_v_offset(); + self.viewport_cache.invalidate(); + } // Poll main indexer (Loading state) let old_state = std::mem::replace(&mut self.loading_state, AppLoadingState::Empty); @@ -1078,9 +1124,12 @@ impl App { // Gutter width changes (~N → N) shift content_width, so any // VisualHeightIndex built with the old width is stale. + let (new_offset, new_sub) = self.rebase_offset_for_invalidate(); if let AppLoadingState::Ready { reader } = &mut self.loading_state { reader.invalidate_visual_height_index(); } + self.v_offset = new_offset; + self.v_sub_offset = new_sub; if self.reload_after_loading { self.reload_after_loading = false; @@ -3126,4 +3175,248 @@ plain text line }); assert!(result.is_ok()); } + + // ── Issue #25 regression tests ────────────────────────────────── + // Ready state without VHI must keep v_offset self-consistent. + + #[test] + fn test_rebase_offset_converts_visual_to_logical_with_sub() { + let content = "line0\nline1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\n"; + let path = make_temp_file(content); + let result = std::panic::catch_unwind(|| { + let mut app = App::new(); + load_file_ready(&mut app, &path); + app.content_height = 10; + + // 10 lines × 3 visual rows each = 30 visual rows + install_vhi(&mut app, &[3usize; 10]); + + // v_offset=7 → visual row 7 is inside line2 (rows 6-8), sub=1 + app.v_offset = 7; + app.v_sub_offset = 0; + + let (logical, sub) = app.rebase_offset_for_invalidate(); + assert_eq!(logical, 2, "visual row 7 → logical line 2 (rows 6-8)"); + assert_eq!(sub, 1, "visual row 7 is sub-row 1 within line 2"); + + // v_offset=0 → line 0, sub=0 + app.v_offset = 0; + let (logical, sub) = app.rebase_offset_for_invalidate(); + assert_eq!(logical, 0); + assert_eq!(sub, 0); + + // v_offset=5 → line1 (rows 3-5), sub=2 + app.v_offset = 5; + let (logical, sub) = app.rebase_offset_for_invalidate(); + assert_eq!(logical, 1, "visual row 5 → logical line 1 (rows 3-5)"); + assert_eq!(sub, 2); + + cleanup(&path); + }); + assert!(result.is_ok()); + } + + #[test] + fn test_rebase_offset_passthrough_when_no_vhi() { + let content = "line0\nline1\nline2\n"; + let path = make_temp_file(content); + let result = std::panic::catch_unwind(|| { + let mut app = App::new(); + load_file_ready(&mut app, &path); + + // No VHI installed — rebase should return current values unchanged + app.v_offset = 42; + app.v_sub_offset = 3; + let (logical, sub) = app.rebase_offset_for_invalidate(); + assert_eq!(logical, 42); + assert_eq!(sub, 3); + + cleanup(&path); + }); + assert!(result.is_ok()); + } + + #[test] + fn test_tab_toggle_rebases_offset_before_invalidate() { + let content = "line0\nline1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\n"; + let path = make_temp_file(content); + let result = std::panic::catch_unwind(|| { + let mut app = App::new(); + load_file_ready(&mut app, &path); + app.content_height = 10; + app.content_width = 20; + + install_vhi(&mut app, &[3usize; 10]); + + // Scroll to visual row 7 → line2 sub-row 1 + app.v_offset = 7; + app.cursor_line = 2; + app.v_sub_offset = 0; + + // Simulate Tab toggle: rebase, then invalidate + let rebased = app.rebase_offset_for_invalidate(); + let (new_offset, new_sub) = rebased; + app.v_offset = new_offset; + app.v_sub_offset = new_sub; + + assert_eq!(app.v_offset, 2, "v_offset should be logical line 2 after rebase"); + assert_eq!(app.v_sub_offset, 1, "v_sub_offset should preserve sub-row 1"); + + // Key invariant: v_offset is now a valid logical line number, + // not a stale visual-row offset that would cause a jump. + // Before the fix, v_offset would still be 7 (visual) treated as logical → jump. + assert!( + app.v_offset < app.total_lines(), + "v_offset ({}) must be a valid logical line index < {}", + app.v_offset, app.total_lines() + ); + + cleanup(&path); + }); + assert!(result.is_ok()); + } + + #[test] + fn test_vhi_rebuild_recalibrates_from_viewport_top() { + let content = "line0\nline1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\n"; + let path = make_temp_file(content); + let result = std::panic::catch_unwind(|| { + let mut app = App::new(); + load_file_ready(&mut app, &path); + app.content_height = 10; + app.content_width = 20; + + // Install VHI with varying heights + install_vhi(&mut app, &[2, 4, 3, 1, 2, 5, 1, 3, 2, 1]); + + // Scroll to visual row 8 → line2 (rows 6-8), sub=2 + app.v_offset = 8; + app.cursor_line = 2; + app.v_sub_offset = 0; + + // Rebase (simulates invalidate) + let (logical_top, sub) = app.rebase_offset_for_invalidate(); + assert_eq!(logical_top, 2); + assert_eq!(sub, 2); + + // Simulate what happens: v_offset becomes logical, VHI cleared + app.v_offset = logical_top; + app.v_sub_offset = sub; + + // Now simulate VHI rebuild completion — the recalibration block + // Install a fresh VHI (same heights, simulates rebuild result) + install_vhi(&mut app, &[2, 4, 3, 1, 2, 5, 1, 3, 2, 1]); + + // The recalibration logic from poll_background_indexer + let logical_top = app.v_offset.min(app.total_lines().saturating_sub(1)); + let sub = app.v_sub_offset; + let first_visual = app.cursor_to_first_visual_row(logical_top); + let line_height = app + .get_visual_height_index() + .map_or(1, |idx| idx.visual_height_of_line(logical_top)); + app.v_offset = first_visual.saturating_add(sub.min(line_height.saturating_sub(1))); + app.v_sub_offset = 0; + + // line2 starts at visual row 6, sub was 2 → visual row 8 + assert_eq!(app.v_offset, 8, "v_offset should map back to visual row 8"); + assert_eq!(app.v_sub_offset, 0, "v_sub_offset should be 0 after recalibration"); + + // Scrolling should work normally with VHI + app.scroll_down_line(); + assert_eq!(app.v_offset, 9, "scroll down should advance visual row"); + + cleanup(&path); + }); + assert!(result.is_ok()); + } + + #[test] + fn test_vhi_rebuild_clamps_sub_to_new_line_height() { + let content = "line0\nline1\nline2\n"; + let path = make_temp_file(content); + let result = std::panic::catch_unwind(|| { + let mut app = App::new(); + load_file_ready(&mut app, &path); + app.content_height = 24; + app.content_width = 20; + + // Line heights: line0=1, line1=5 (wraps), line2=1 + install_vhi(&mut app, &[1, 5, 1]); + + // Position at line1 sub-row 4 (last sub-row of 5-row line) + app.v_offset = 5; + app.cursor_line = 1; + app.v_sub_offset = 0; + + // Rebase + let (logical_top, sub) = app.rebase_offset_for_invalidate(); + assert_eq!(logical_top, 1, "visual row 5 → line 1 (rows 1-5)"); + assert_eq!(sub, 4, "sub-row 4 within line 1"); + + app.v_offset = logical_top; + app.v_sub_offset = sub; + + // Simulate VHI rebuild with SHRUNK line height (e.g., JSON toggle) + // line1 now only 2 visual rows instead of 5 + install_vhi(&mut app, &[1, 2, 1]); + + // Recalibration should clamp sub=4 to new height-1=1 + let logical_top = app.v_offset.min(app.total_lines().saturating_sub(1)); + let sub = app.v_sub_offset; + let first_visual = app.cursor_to_first_visual_row(logical_top); + let line_height = app + .get_visual_height_index() + .map_or(1, |idx| idx.visual_height_of_line(logical_top)); + let clamped_sub = sub.min(line_height.saturating_sub(1)); + app.v_offset = first_visual.saturating_add(clamped_sub); + app.v_sub_offset = 0; + + // line1 starts at visual row 1, clamped sub=1 → visual row 2 + assert_eq!(app.v_offset, 2, "v_offset should be clamped to visual row 2 (line1 row 1 of 2)"); + assert_eq!(app.v_sub_offset, 0); + + cleanup(&path); + }); + assert!(result.is_ok()); + } + + #[test] + fn test_loading_to_ready_rebases_visual_offset() { + let content = "line0\nline1\nline2\nline3\nline4\n"; + let path = make_temp_file(content); + let result = std::panic::catch_unwind(|| { + let mut app = App::new(); + load_file_ready(&mut app, &path); + app.content_height = 10; + + // Install VHI: 2 rows per line + install_vhi(&mut app, &[2usize; 5]); + + // Scroll to visual row 3 → line1 sub-row 1 + app.v_offset = 3; + app.cursor_line = 1; + app.v_sub_offset = 0; + + // Simulate Loading→Ready invalidation rebase + let (new_offset, new_sub) = app.rebase_offset_for_invalidate(); + assert_eq!(new_offset, 1, "visual row 3 → line 1"); + assert_eq!(new_sub, 1, "sub-row 1 within line 1"); + + // Apply rebased values and clear VHI (simulate invalidate) + app.v_offset = new_offset; + app.v_sub_offset = new_sub; + if let AppLoadingState::Ready { reader } = &mut app.loading_state { + reader.invalidate_visual_height_index(); + } + + // VHI is now None → scroll_down_line uses else branch (v_sub_offset path) + // "line1" at width=80 → compute_visual_height=1, sub=1+1 >= 1 → advance + app.scroll_down_line(); + assert_eq!(app.v_offset, 2, "should advance to line 2 (line1 height=1, sub overflow)"); + assert_eq!(app.v_sub_offset, 0); + + cleanup(&path); + }); + assert!(result.is_ok()); + } } \ No newline at end of file