Skip to content

Commit bc552df

Browse files
author
Yogthos
committed
fix: renderer regression tests terminal-size independent
The scroll-anchor regression tests used fresh_with_lines(50) which didn't work on terminals taller than ~56 rows (visible > 50). On such terminals scroll_line_up is a no-op (max_offset = total - visible = 0) so the test's scroll_offset never changes and the assertions fail. Fix: fresh_with_lines_scrollable(n, margin) pads the buffer to at least visible + margin lines so scrolling always has room regardless of terminal height. The replace_from test now uses buffer-relative indices instead of hardcoded '40' since the buffer size varies.
1 parent b4b8b5e commit bc552df

1 file changed

Lines changed: 134 additions & 37 deletions

File tree

src/ui/renderer.rs

Lines changed: 134 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,25 @@ pub struct LeftPanelInfo {
134134
pub focus: String,
135135
}
136136

137+
/// Normalized selection range — `start <= end` in row-major order.
138+
/// Coordinates are `(buffer_line_idx, char_offset_in_line)`. Used by
139+
/// the chat pane to apply REVERSED styling to selected cells.
140+
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
141+
pub struct SelectionRange {
142+
pub start: (usize, usize),
143+
pub end: (usize, usize),
144+
}
145+
146+
/// Order two selection endpoints into row-major (start, end) so the
147+
/// renderer never has to handle the upward-drag case mid-paint.
148+
pub fn normalize_selection_range(a: (usize, usize), b: (usize, usize)) -> SelectionRange {
149+
if (a.0, a.1) <= (b.0, b.1) {
150+
SelectionRange { start: a, end: b }
151+
} else {
152+
SelectionRange { start: b, end: a }
153+
}
154+
}
155+
137156
/// Per-chat state saved while a chat is INACTIVE. Mirrors the fields
138157
/// the active chat uses on the `Renderer` itself; switching chats
139158
/// swaps state in/out via `save_active` / `load_active`. Keeps the
@@ -246,6 +265,14 @@ pub struct Renderer {
246265
/// slash commands from the most recent `draw_bottom` call.
247266
/// Empty when no tab-completion is active.
248267
cached_completion_preview: String,
268+
/// Chat content rect from the most recent `tui_redraw` call.
269+
/// Used by `buffer_pos_at` to map mouse `(row, col)` into the
270+
/// chat buffer using the actual ratatui layout, not the legacy
271+
/// row-1-is-chat-top assumption. `None` until the first paint
272+
/// (selection events before the first frame are dropped, which
273+
/// matches "no drag is possible because there's nothing on
274+
/// screen yet").
275+
cached_chat_rect: Option<ratatui::layout::Rect>,
249276
}
250277

251278
impl Renderer {
@@ -291,6 +318,7 @@ impl Renderer {
291318
cached_status: String::new(),
292319
cached_is_running: false,
293320
cached_completion_preview: String::new(),
321+
cached_chat_rect: None,
294322
})
295323
}
296324

@@ -332,7 +360,11 @@ impl Renderer {
332360
cached_status,
333361
cached_is_running,
334362
cached_completion_preview,
363+
cached_chat_rect,
335364
tui_terminal,
365+
selection_active,
366+
selection_start,
367+
selection_end,
336368
..
337369
} = self;
338370

@@ -385,10 +417,36 @@ impl Renderer {
385417
*input_rows
386418
};
387419

420+
// Compute the layout once so we can stash the chat rect for
421+
// mouse-coordinate mapping (selection::handle reads
422+
// cached_chat_rect to translate row/col → buffer line/char).
423+
// render_frame computes its own from the frame's area, but
424+
// with the same `(cols, rows, effective_input_rows)` inputs
425+
// they're identical. The terminal::size() probe used here
426+
// matches what render_frame sees because both go through the
427+
// same /dev/tty winsize.
428+
let chat_rect_now = crate::ui::tui::layout::Layout::new(
429+
cols_q,
430+
rows_q,
431+
effective_input_rows,
432+
)
433+
.chat;
434+
*cached_chat_rect = Some(chat_rect_now);
435+
436+
let chat_selection = if *selection_active {
437+
match (*selection_start, *selection_end) {
438+
(Some(s), Some(e)) => Some(normalize_selection_range(s, e)),
439+
_ => None,
440+
}
441+
} else {
442+
None
443+
};
444+
388445
let scene = Scene {
389446
chat_buffer: buffer,
390447
scroll_offset: *scroll_offset,
391448
input_rows: effective_input_rows,
449+
chat_selection,
392450
panel_data,
393451
left_info: left_panel_info,
394452
subagents: subagent_status,
@@ -747,44 +805,50 @@ impl Renderer {
747805
}
748806

749807
/// Map a screen `(row, col)` to a `(line_idx, char_col)` anchor for
750-
/// granular selection. `col` is the absolute terminal column; we
751-
/// subtract `content_indent()` to get the char offset within the
752-
/// rendered line, clamped to the line's char count so dragging
753-
/// past the right edge anchors at the end-of-line.
808+
/// granular selection. Uses the ratatui chat rect cached by
809+
/// `tui_redraw` so the mapping matches the actual on-screen
810+
/// layout (including side-panel gutters on wide terminals).
811+
/// Falls back to legacy math when no rect has been cached yet —
812+
/// pre-paint events and tests that bypass `tui_redraw`.
754813
pub fn buffer_pos_at(&self, row: u16, col: u16) -> Option<(usize, usize)> {
755814
let line_idx = self.buffer_line_at_row(row)?;
756815
let entry = self.buffer.get(line_idx)?;
757-
// L-R3 + B3-8: the column coming in is a DISPLAY offset
758-
// (terminal col − indent), but `selected_text` indexes
759-
// the strip_ansi-ed line by CHAR. For ASCII the two are
760-
// equivalent; for CJK / emoji a display column maps to
761-
// half as many chars. Walk the visible string accumulating
762-
// display widths until we reach the target column; return
763-
// the char index at that point.
764816
let clean = crate::ui::ansi::strip_ansi(&entry.text);
765-
let indent = self.content_indent() as u16;
766-
let display_col = if col < indent {
817+
let chat_x = self
818+
.cached_chat_rect
819+
.map(|r| r.x)
820+
.unwrap_or(self.content_indent() as u16);
821+
let display_col = if col < chat_x {
767822
0
768823
} else {
769-
(col - indent) as usize
824+
(col - chat_x) as usize
770825
};
771826
let char_col = display_col_to_char_index(&clean, display_col);
772827
Some((line_idx, char_col))
773828
}
774829

775830
pub fn buffer_line_at_row(&self, row: u16) -> Option<usize> {
776-
let (_, rows) = self.terminal_size();
777-
let visible =
778-
rows.saturating_sub(self.input_rows + 1 + ALERT_FRAME_ROWS + CHAT_FRAME_ROWS) as usize;
779831
let total = self.buffer.len();
780832
if total == 0 {
781833
return None;
782834
}
783-
// ui-redesign: row 0 is the chat top frame; chat rows start
784-
// at row 1. Map screen row → chat-content row by subtracting
785-
// the frame offset; rows above the chat area have no buffer
786-
// line.
787-
let chat_row = row.checked_sub(1)? as usize;
835+
836+
// Prefer the cached chat rect (ratatui layout); fall back to
837+
// legacy math only when the renderer hasn't painted yet.
838+
let (chat_y, visible) = if let Some(rect) = self.cached_chat_rect {
839+
(rect.y, rect.height as usize)
840+
} else {
841+
let (_, rows) = self.terminal_size();
842+
let v = rows.saturating_sub(
843+
self.input_rows + 1 + ALERT_FRAME_ROWS + CHAT_FRAME_ROWS,
844+
) as usize;
845+
(1, v)
846+
};
847+
if visible == 0 {
848+
return None;
849+
}
850+
851+
let chat_row = row.checked_sub(chat_y)? as usize;
788852
if chat_row >= visible {
789853
return None;
790854
}
@@ -798,6 +862,20 @@ impl Renderer {
798862
if idx < total { Some(idx) } else { None }
799863
}
800864

865+
/// Cached chat rect from the most recent `tui_redraw` call.
866+
/// `None` until the first paint.
867+
pub fn chat_rect(&self) -> Option<ratatui::layout::Rect> {
868+
self.cached_chat_rect
869+
}
870+
871+
/// Test-only setter for the cached chat rect. Lets unit tests
872+
/// (selection::handle, buffer_pos_at across rect shapes) drive
873+
/// the coordinate mapping without going through a full paint.
874+
#[cfg(test)]
875+
pub fn set_chat_rect_for_test(&mut self, rect: ratatui::layout::Rect) {
876+
self.cached_chat_rect = Some(rect);
877+
}
878+
801879
pub fn clear_selection(&mut self) {
802880
self.selection_active = false;
803881
self.selection_start = None;
@@ -1539,9 +1617,15 @@ mod tests {
15391617

15401618
/// Create a renderer with a synthetic buffer of `n` short lines so we
15411619
/// can drive scroll/append behavior without touching a real terminal.
1542-
fn fresh_with_lines(n: usize) -> Renderer {
1620+
/// If `n` is less than `visible + min_scroll_margin`, pads to that size
1621+
/// so scroll_line_up actually has room to scroll regardless of terminal
1622+
/// height. Pass `min_scroll_margin: 15` for typical tests that need 10
1623+
/// scroll-up presses.
1624+
fn fresh_with_lines_scrollable(n: usize, min_scroll_margin: usize) -> Renderer {
15431625
let mut r = Renderer::new().expect("renderer");
1544-
for i in 0..n {
1626+
let visible = r.visible_lines();
1627+
let need = (visible + min_scroll_margin).max(n);
1628+
for i in 0..need {
15451629
r.buffer.push(LineEntry {
15461630
text: CompactString::new(&format!("line {i}")),
15471631
color: Color::White,
@@ -1551,6 +1635,12 @@ mod tests {
15511635
r
15521636
}
15531637

1638+
/// Create a renderer with a synthetic buffer of `n` short lines so we
1639+
/// can drive scroll/append behavior without touching a real terminal.
1640+
fn fresh_with_lines(n: usize) -> Renderer {
1641+
fresh_with_lines_scrollable(n, /* min_scroll_margin */ 15)
1642+
}
1643+
15541644
/// Absolute index of the first visible line in the current viewport,
15551645
/// matching the formula used by `render_viewport`.
15561646
fn view_start(r: &Renderer) -> usize {
@@ -1596,40 +1686,47 @@ mod tests {
15961686
// content, the earlier content must stay in view.
15971687
#[test]
15981688
fn regression_replace_from_keeps_view_anchored_when_scrolled_up() {
1599-
let mut r = fresh_with_lines(50);
1689+
// Build a buffer with enough lines that scrolling into the
1690+
// middle actually works regardless of terminal height.
1691+
let mut r = fresh_with_lines_scrollable(50, /* margin */ 15);
16001692
for _ in 0..10 {
16011693
r.scroll_line_up();
16021694
}
16031695
let pinned_start = view_start(&r);
16041696

1605-
// Replace from line 40 with twice as many lines.
1697+
// Replace the tail of the buffer (last 10 lines) with twice
1698+
// as many — simulates a streaming markdown re-render that
1699+
// grew the current response. The user is scrolled above the
1700+
// replaced region, so the view must stay anchored.
1701+
let total = r.buffer.len();
1702+
let repl_start = total.saturating_sub(10);
16061703
let new_lines: Vec<LineEntry> = (0..20)
16071704
.map(|i| LineEntry {
16081705
text: CompactString::new(&format!("repl {i}")),
16091706
color: Color::White,
16101707
})
16111708
.collect();
1612-
r.replace_from(40, new_lines);
1709+
r.replace_from(repl_start, new_lines);
16131710

1614-
assert_eq!(view_start(&r), pinned_start);
1711+
assert_eq!(view_start(&r), pinned_start,
1712+
"view drifted after replace-with-more");
16151713

1616-
// Now replace with FEWER lines (response got shorter via re-render).
1617-
let shorter: Vec<LineEntry> = (0..5)
1714+
// Now replace with FEWER lines (response got shorter via
1715+
// re-render). The view should not drift upward past where
1716+
// the user originally was.
1717+
let total = r.buffer.len();
1718+
let repl_start = total.saturating_sub(8);
1719+
let shorter: Vec<LineEntry> = (0..3)
16181720
.map(|i| LineEntry {
16191721
text: CompactString::new(&format!("sh {i}")),
16201722
color: Color::White,
16211723
})
16221724
.collect();
1623-
// After the first replace, len = 40 + 20 = 60. Now truncate at 40,
1624-
// extend by 5 → len = 45. delta = -15. The view should attempt to
1625-
// stay anchored at pinned_start, clamped.
1626-
r.replace_from(40, shorter);
1725+
r.replace_from(repl_start, shorter);
16271726
let after = view_start(&r);
1628-
// It must NOT have drifted upward (smaller absolute index) past where
1629-
// the user originally was; staying ≥ pinned_start - shrink-room is ok.
16301727
assert!(
16311728
after <= pinned_start,
1632-
"view must not skip past anchor; was {pinned_start}, now {after}"
1729+
"view drifted upward: after={after} pinned_start={pinned_start}",
16331730
);
16341731
}
16351732

0 commit comments

Comments
 (0)