mirror of
https://github.com/openai/codex.git
synced 2026-11-14 21:40:39 +00:00
Refill the transcript viewport when collapsing tool groups (#61424)
## Why Collapsing a tool group while reading its expanded details can leave the scroll anchor beyond the new bottom limit, leaving available transcript space unfilled. ## What changed Clamp the reading position to the bottom limit during collapse layout and persist it after the final viewport height is known. Keep activity focus and reading mode intact, including when copy confirmation changes the composer layout. ## Testing Add regression coverage for collapsing a scrolled tool group, preserving the resulting view across renders, and rendering a collapsed group with clipboard confirmation. GitOrigin-RevId: f48271720f7b400736bc02b08b6eafefdb60ac63
This commit is contained in:
@@ -673,6 +673,7 @@ impl App {
|
||||
);
|
||||
needs_redraw = false;
|
||||
}
|
||||
view.finish_collapse_layout(&self.transcript_cells);
|
||||
let blossom_tick = chat_widget
|
||||
.empty_state_animation
|
||||
.borrow_mut()
|
||||
|
||||
@@ -192,6 +192,42 @@ async fn follow_control_click_preserves_draft_caret_and_composer_geometry() -> R
|
||||
super::tests::buffer_text(buffer)
|
||||
));
|
||||
}
|
||||
|
||||
app.local_settings.tui.collapse_turn_details = true;
|
||||
for id in 1..=6 {
|
||||
app.transcript_cells
|
||||
.push(Arc::new(crate::history_cell::new_web_search_call(
|
||||
format!("lookup-{id}"),
|
||||
format!("field note {id}"),
|
||||
codex_app_server_protocol::WebSearchAction::Other,
|
||||
)));
|
||||
}
|
||||
let size = Size::new(/*width*/ 80, /*height*/ 16);
|
||||
tui.screen_size_for_event(&TuiEvent::Resize(size))?;
|
||||
app.render_owned_transcript(&mut tui, size)?;
|
||||
for key in [KeyCode::F(4), KeyCode::Enter] {
|
||||
app.transcript_view
|
||||
.handle_key(key.into(), &app.transcript_cells);
|
||||
}
|
||||
app.render_owned_transcript(&mut tui, size)?;
|
||||
app.transcript_view
|
||||
.scroll(&app.transcript_cells, /*rows*/ 2);
|
||||
app.render_owned_transcript(&mut tui, size)?;
|
||||
app.transcript_view
|
||||
.handle_key(KeyCode::Enter.into(), &app.transcript_cells);
|
||||
app.transcript_view.show_copy_feedback(
|
||||
&Ok(crate::clipboard_copy::CopyStatus::Confirmed),
|
||||
/*characters*/ 24,
|
||||
);
|
||||
app.render_owned_transcript(&mut tui, size)?;
|
||||
let buffer = crate::custom_terminal::test_support::last_rendered_buffer(&tui.terminal);
|
||||
let collapsed = super::tests::buffer_text(buffer);
|
||||
assert!(collapsed.contains("6 tool calls"), "{collapsed}");
|
||||
assert!(app.transcript_view.is_activity_focused());
|
||||
snapshots.push(format!(
|
||||
"Collapsed tool group with copy feedback\n{collapsed}"
|
||||
));
|
||||
|
||||
insta::assert_snapshot!(
|
||||
"running_copy_feedback_spacing",
|
||||
crate::chatwidget::tests::helpers::normalize_working_snapshot(
|
||||
|
||||
+19
-1
@@ -1,6 +1,6 @@
|
||||
---
|
||||
source: tui/src/app/owned_transcript_follow_tests.rs
|
||||
expression: "crate::chatwidget::tests::helpers::normalize_snapshot_paths(snapshots.join(\"\\n\\n\"))"
|
||||
expression: "crate::chatwidget::tests::helpers::normalize_working_snapshot(snapshots.join(\"\\n\\n\"))"
|
||||
---
|
||||
80 columns, 12 rows
|
||||
transcript row 37
|
||||
@@ -59,3 +59,21 @@ row 39
|
||||
|
||||
GPT-5.6-Sol default · /tmp/project
|
||||
tab to queue message
|
||||
|
||||
Collapsed tool group with copy feedback
|
||||
transcript row 34
|
||||
transcript row 35
|
||||
transcript row 36
|
||||
transcript row 37
|
||||
transcript row 38
|
||||
transcript row 39
|
||||
⌕ 6 tool calls · Searched the web for field note 6
|
||||
|
||||
• Working (0s • esc to interrupt)
|
||||
|
||||
Copied 24 chars to host clipboard
|
||||
|
||||
› draft stays here
|
||||
|
||||
GPT-5.6-Sol default · /tmp/project
|
||||
↑ previous · ↓ next · enter expand · esc back
|
||||
|
||||
+12
@@ -0,0 +1,12 @@
|
||||
---
|
||||
source: tui/src/transcript_view_tests.rs
|
||||
expression: collapsed
|
||||
---
|
||||
|
||||
|
||||
Opened the field notes
|
||||
Checking dates and locations
|
||||
Checking names and totals
|
||||
⌕ 6 tool calls · Searched the web for field note
|
||||
|
||||
Checking the last source
|
||||
@@ -186,6 +186,11 @@ impl TranscriptView {
|
||||
}
|
||||
|
||||
pub(crate) fn render(&mut self, area: Rect, buf: &mut Buffer, cells: &[Arc<dyn HistoryCell>]) {
|
||||
self.paint_rows(area, buf, cells);
|
||||
self.finish_collapse_layout(cells);
|
||||
}
|
||||
|
||||
fn paint_rows(&mut self, area: Rect, buf: &mut Buffer, cells: &[Arc<dyn HistoryCell>]) {
|
||||
self.composer_tip = None;
|
||||
self.prompt_header_target = None;
|
||||
self.cache.begin_frame();
|
||||
@@ -218,7 +223,19 @@ impl TranscriptView {
|
||||
anchor.row_bias = layout.row_for_offset(anchor.offset) as isize - row as isize;
|
||||
self.position = Position::Reading(anchor);
|
||||
}
|
||||
let initial_start = self.start(cells);
|
||||
// Closing a disclosure can leave its reading anchor below the new scroll limit.
|
||||
let clamp_after_collapse = self.disclosure.clamp_after_collapse
|
||||
&& self.selection.is_none()
|
||||
&& matches!(self.position, Position::Reading(_));
|
||||
let render_start = |view: &mut Self| {
|
||||
let start = view.start(cells);
|
||||
if clamp_after_collapse {
|
||||
start.min(view.bottom_start(cells))
|
||||
} else {
|
||||
start
|
||||
}
|
||||
};
|
||||
let initial_start = render_start(self);
|
||||
let initial_position = self.position;
|
||||
let mut start = initial_start;
|
||||
let mut body = area;
|
||||
@@ -242,8 +259,8 @@ impl TranscriptView {
|
||||
if resized {
|
||||
self.reveal_copy_context(cells);
|
||||
}
|
||||
// Following may advance into the next turn after reserving the header row.
|
||||
start = self.start(cells);
|
||||
// The bottom limit changes after reserving the header row.
|
||||
start = render_start(self);
|
||||
if let Some((header, key)) =
|
||||
prompt_header::line(cells, start.0, area.width, self.detailed)
|
||||
{
|
||||
@@ -324,6 +341,26 @@ impl TranscriptView {
|
||||
self.render_search(buf);
|
||||
}
|
||||
|
||||
/// Persist the clamped reading position after the last paint at the final viewport height.
|
||||
pub(crate) fn finish_collapse_layout(&mut self, cells: &[Arc<dyn HistoryCell>]) {
|
||||
if self.area.is_empty()
|
||||
|| !std::mem::take(&mut self.disclosure.clamp_after_collapse)
|
||||
|| self.selection.is_some()
|
||||
|| !matches!(self.position, Position::Reading(_))
|
||||
{
|
||||
return;
|
||||
}
|
||||
let snapshot = self.snapshot_cells();
|
||||
let cells = snapshot.as_deref().unwrap_or(cells);
|
||||
let start = self.start(cells);
|
||||
if let Some(first) = self.visible.first()
|
||||
&& (first.index, first.row) < start
|
||||
{
|
||||
let (index, row, layout) = (first.index, first.row, Arc::clone(&first.layout));
|
||||
self.read_from_row(cells, index, row, &layout);
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn image_placements(&self) -> Vec<crate::transcript_image::ImagePlacement> {
|
||||
let mut placements = Vec::new();
|
||||
let mut y = self.area.y;
|
||||
@@ -583,17 +620,27 @@ impl TranscriptView {
|
||||
return;
|
||||
}
|
||||
if let Some(layout) = self.layout(cells, index) {
|
||||
let offset = layout.position_at(row, /*column*/ 0);
|
||||
self.position = Position::Reading(Anchor {
|
||||
key: self.entry_key(cells, index),
|
||||
index,
|
||||
offset,
|
||||
row_bias: layout.row_for_offset(offset) as isize - row as isize,
|
||||
});
|
||||
self.read_from_row(cells, index, row, &layout);
|
||||
self.hold_live_reading(current_cells, layout);
|
||||
}
|
||||
}
|
||||
|
||||
fn read_from_row(
|
||||
&mut self,
|
||||
cells: &[Arc<dyn HistoryCell>],
|
||||
index: usize,
|
||||
row: usize,
|
||||
layout: &TextLayout,
|
||||
) {
|
||||
let offset = layout.position_at(row, /*column*/ 0);
|
||||
self.position = Position::Reading(Anchor {
|
||||
key: self.entry_key(cells, index),
|
||||
index,
|
||||
offset,
|
||||
row_bias: layout.row_for_offset(offset) as isize - row as isize,
|
||||
});
|
||||
}
|
||||
|
||||
pub(crate) fn jump_to_entry(&mut self, cells: &[Arc<dyn HistoryCell>], index: usize) {
|
||||
self.cancel_beginning();
|
||||
self.release_live_reading();
|
||||
|
||||
@@ -21,6 +21,7 @@ pub(super) struct Disclosure {
|
||||
pub(super) resume_after_approval: Option<(ThreadId, String)>,
|
||||
// Resolve against the next layout, once a toggled live row has also been rebuilt.
|
||||
pub(super) anchor_control: Option<EntryKey>,
|
||||
pub(super) clamp_after_collapse: bool,
|
||||
pub(super) live_ids: Vec<String>,
|
||||
pub(super) keymap: RuntimeKeymap,
|
||||
}
|
||||
@@ -32,6 +33,7 @@ impl Default for Disclosure {
|
||||
focused: None,
|
||||
resume_after_approval: None,
|
||||
anchor_control: None,
|
||||
clamp_after_collapse: false,
|
||||
live_ids: Vec::new(),
|
||||
keymap: RuntimeKeymap::defaults(),
|
||||
}
|
||||
@@ -429,11 +431,13 @@ impl TranscriptView {
|
||||
offset: 0,
|
||||
row_bias: 0,
|
||||
});
|
||||
if matched || self.disclosure.is_expanded(&ids) {
|
||||
let collapsing = matched || self.disclosure.is_expanded(&ids);
|
||||
if collapsing {
|
||||
self.disclosure.expanded.retain(|id| !ids.contains(id));
|
||||
} else {
|
||||
self.disclosure.expanded.extend(ids.iter().cloned());
|
||||
}
|
||||
self.disclosure.clamp_after_collapse = collapsing;
|
||||
self.disclosure.focused = Some((key, ids));
|
||||
self.position = Position::Reading(anchor);
|
||||
self.cache.clear();
|
||||
|
||||
@@ -25,7 +25,7 @@ impl TranscriptView {
|
||||
})
|
||||
.map(EntryKey::cell)
|
||||
});
|
||||
self.render(
|
||||
self.paint_rows(
|
||||
Rect {
|
||||
height: area.height - u16::from(tip.is_some() && self.turn_tip_key.is_none()),
|
||||
..area
|
||||
@@ -39,7 +39,7 @@ impl TranscriptView {
|
||||
&& self.visible.first().is_none_or(|row| row.key == key)
|
||||
{
|
||||
self.turn_tip_key = None;
|
||||
self.render(area, buf, cells);
|
||||
self.paint_rows(area, buf, cells);
|
||||
return None;
|
||||
}
|
||||
tip?;
|
||||
|
||||
@@ -1421,6 +1421,42 @@ fn assistant_message(
|
||||
)
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn collapsing_tool_group_refills_viewport() {
|
||||
let mut cells = vec![
|
||||
prompt("Check the field notes"),
|
||||
cell("Opened the field notes\nChecking dates and locations\nChecking names and totals"),
|
||||
];
|
||||
for id in 1..=6 {
|
||||
cells.push(Arc::new(crate::history_cell::new_web_search_call(
|
||||
format!("lookup-{id}"),
|
||||
format!("field note {id}"),
|
||||
codex_app_server_protocol::WebSearchAction::Other,
|
||||
)));
|
||||
}
|
||||
cells.push(cell("Checking the last source"));
|
||||
let mut view = TranscriptView::default();
|
||||
view.sync_turn_details(&cells, /*enabled*/ true, /*active*/ None);
|
||||
let screen = |view: &mut TranscriptView| {
|
||||
text(&render(view, &cells, /*width*/ 52, /*height*/ 8))
|
||||
};
|
||||
let initial = screen(&mut view);
|
||||
view.handle_key(KeyCode::F(4).into(), &cells);
|
||||
view.handle_key(KeyCode::Enter.into(), &cells);
|
||||
screen(&mut view);
|
||||
// Read down to the expanded group's details, then close it in place.
|
||||
view.scroll(&cells, /*rows*/ 2);
|
||||
let expanded = screen(&mut view);
|
||||
assert!(expanded.contains("▾ 6 tool calls"), "{expanded}");
|
||||
view.handle_key(KeyCode::Enter.into(), &cells);
|
||||
let collapsed = screen(&mut view);
|
||||
assert_eq!(collapsed, initial);
|
||||
assert!(view.is_activity_focused());
|
||||
assert!(!view.is_following());
|
||||
assert_eq!(screen(&mut view), collapsed);
|
||||
insta::assert_snapshot!(collapsed);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn tool_groups_expand_independently_inside_a_completed_turn() {
|
||||
use crate::exec_cell::CommandOutput;
|
||||
|
||||
Reference in New Issue
Block a user