Skip to content

Commit 1c6a3f1

Browse files
authored
fix: artifacts from previous frames were bleeding through in TUI (openai#989)
Prior to this PR, I would frequently see glyphs from previous frames "bleed" through like this: ![image](https://github.com/user-attachments/assets/8784b3d7-f691-4df6-8666-34e2f134ee85) I think this was due to two issues (now addressed in this PR): * We were not making use of `ratatui::widgets::Clear` to clear out the buffer before drawing into it. * To calculate the `width` used with `wrapped_line_count_for_cell()`, we were not accounting for the scrollbar. * Now we calculate `effective_width` using `inner.width.saturating_sub(1)` where the `1` is for the scrollbar. * We compute `text_area` using `effective_with` and pass the `text_area` to `paragraph.render()`. * We eliminate the conditional `needs_scrollbar` check and always call `render(Scrollbar)` I suspect this bug was introduced in openai#937, though I did not try to verify: I'm just happy that it appears to be fixed!
1 parent f8b6b1d commit 1c6a3f1

File tree

1 file changed

+57
-36
lines changed

1 file changed

+57
-36
lines changed

codex-rs/tui/src/conversation_history_widget.rs

Lines changed: 57 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -362,21 +362,23 @@ impl WidgetRef for ConversationHistoryWidget {
362362
let inner = block.inner(area);
363363
let viewport_height = inner.height as usize;
364364

365-
// Cache (and if necessary recalculate) the wrapped line counts for
366-
// every [`HistoryCell`] so that our scrolling math accounts for text
367-
// wrapping.
368-
let width = inner.width; // Width of the viewport in terminal cells.
369-
if width == 0 {
365+
// Cache (and if necessary recalculate) the wrapped line counts for every
366+
// [`HistoryCell`] so that our scrolling math accounts for text
367+
// wrapping. We always reserve one column on the right-hand side for the
368+
// scrollbar so that the content never renders "under" the scrollbar.
369+
let effective_width = inner.width.saturating_sub(1);
370+
371+
if effective_width == 0 {
370372
return; // Nothing to draw – avoid division by zero.
371373
}
372374

373-
// Recompute cache if the width changed.
374-
let num_lines: usize = if self.cached_width.get() != width {
375-
self.cached_width.set(width);
375+
// Recompute cache if the effective width changed.
376+
let num_lines: usize = if self.cached_width.get() != effective_width {
377+
self.cached_width.set(effective_width);
376378

377379
let mut num_lines: usize = 0;
378380
for entry in &self.entries {
379-
let count = wrapped_line_count_for_cell(&entry.cell, width);
381+
let count = wrapped_line_count_for_cell(&entry.cell, effective_width);
380382
num_lines += count;
381383
entry.line_count.set(count);
382384
}
@@ -433,56 +435,75 @@ impl WidgetRef for ConversationHistoryWidget {
433435
// Build the Paragraph with wrapping enabled so long lines are not
434436
// clipped. Apply vertical scroll so that `offset_into_first` wrapped
435437
// lines are hidden at the top.
438+
// ------------------------------------------------------------------
439+
// Render order:
440+
// 1. Clear the whole widget area so we do not leave behind any glyphs
441+
// from the previous frame.
442+
// 2. Draw the surrounding Block (border and title).
443+
// 3. Draw the Paragraph inside the Block, **leaving the right-most
444+
// column free** for the scrollbar.
445+
// 4. Finally draw the scrollbar (if needed).
446+
// ------------------------------------------------------------------
447+
448+
// Clear the widget area to avoid visual artifacts from previous frames.
449+
Clear.render(area, buf);
450+
451+
// Draw the outer border and title first so the Paragraph does not
452+
// overwrite it.
453+
block.render(area, buf);
454+
455+
// Area available for text after accounting for the scrollbar.
456+
let text_area = Rect {
457+
x: inner.x,
458+
y: inner.y,
459+
width: effective_width,
460+
height: inner.height,
461+
};
462+
436463
let paragraph = Paragraph::new(visible_lines)
437-
.block(block)
438464
.wrap(wrap_cfg())
439465
.scroll((offset_into_first as u16, 0));
440466

441-
paragraph.render(area, buf);
467+
paragraph.render(text_area, buf);
468+
469+
// Always render a scrollbar *track* so that the reserved column is
470+
// visually filled, even when the content fits within the viewport.
471+
// We only draw the *thumb* when the content actually overflows.
472+
473+
let overflow = num_lines.saturating_sub(viewport_height);
442474

443-
// Draw scrollbar if necessary.
444-
let needs_scrollbar = num_lines > viewport_height;
445-
if needs_scrollbar {
446-
let mut scroll_state = ScrollbarState::default()
447-
// The Scrollbar widget expects the *content* height minus the
448-
// viewport height, mirroring the calculation used previously.
449-
.content_length(num_lines.saturating_sub(viewport_height))
450-
.position(scroll_pos);
475+
let mut scroll_state = ScrollbarState::default()
476+
// The Scrollbar widget expects the *content* height minus the
477+
// viewport height. When there is no overflow we still provide 0
478+
// so that the widget renders only the track without a thumb.
479+
.content_length(overflow)
480+
.position(scroll_pos);
451481

482+
{
452483
// Choose a thumb color that stands out only when this pane has focus so that the
453484
// user’s attention is naturally drawn to the active viewport. When unfocused we show
454-
// a lowcontrast thumb so the scrollbar fades into the background without becoming
485+
// a low-contrast thumb so the scrollbar fades into the background without becoming
455486
// invisible.
456487
let thumb_style = if self.has_input_focus {
457488
Style::reset().fg(Color::LightYellow)
458489
} else {
459490
Style::reset().fg(Color::Gray)
460491
};
461492

493+
// By default the Scrollbar widget inherits any style that was
494+
// present in the underlying buffer cells. That means if a colored
495+
// line happens to be underneath the scrollbar, the track (and
496+
// potentially the thumb) adopt that color. Explicitly setting the
497+
// track/thumb styles ensures we always draw the scrollbar with a
498+
// consistent palette regardless of what content is behind it.
462499
StatefulWidget::render(
463-
// By default the Scrollbar widget inherits the style that was already present
464-
// in the underlying buffer cells. That means if a colored line (for example a
465-
// background task notification that we render in blue) happens to be underneath
466-
// the scrollbar, the track and thumb adopt that color and the scrollbar appears
467-
// to "change color." Explicitly setting the *track* and *thumb* styles ensures
468-
// we always draw the scrollbar with the same palette regardless of what content
469-
// is behind it.
470-
//
471-
// N.B. Only the *foreground* color matters here because the scrollbar symbols
472-
// themselves are filled‐in block glyphs that completely overwrite the prior
473-
// character cells. We therefore leave the background at its default value so it
474-
// blends nicely with the surrounding `Block`.
475500
Scrollbar::new(ScrollbarOrientation::VerticalRight)
476501
.begin_symbol(Some("↑"))
477502
.end_symbol(Some("↓"))
478503
.begin_style(Style::reset().fg(Color::DarkGray))
479504
.end_style(Style::reset().fg(Color::DarkGray))
480-
// A solid thumb so that we can color it distinctly from the track.
481505
.thumb_symbol("█")
482-
// Apply the dynamic thumb color computed above. We still start from
483-
// Style::reset() to clear any inherited modifiers.
484506
.thumb_style(thumb_style)
485-
// Thin vertical line for the track.
486507
.track_symbol(Some("│"))
487508
.track_style(Style::reset().fg(Color::DarkGray)),
488509
inner,

0 commit comments

Comments
 (0)