From 4db6da32a3a2a70227a93a7119f81e7261a19f48 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Mon, 26 Jan 2026 21:30:09 -0800 Subject: [PATCH] tui: wrapping user input questions (#9971) --- .../core/templates/collaboration_mode/plan.md | 2 +- .../bottom_pane/request_user_input/layout.rs | 361 ++++++++++++++---- .../src/bottom_pane/request_user_input/mod.rs | 141 +++++++ .../bottom_pane/request_user_input/render.rs | 32 +- ..._request_user_input_scrolling_options.snap | 11 +- ...ests__request_user_input_tight_height.snap | 3 +- ...s__request_user_input_wrapped_options.snap | 21 + 7 files changed, 456 insertions(+), 115 deletions(-) create mode 100644 codex-rs/tui/src/bottom_pane/request_user_input/snapshots/codex_tui__bottom_pane__request_user_input__tests__request_user_input_wrapped_options.snap diff --git a/codex-rs/core/templates/collaboration_mode/plan.md b/codex-rs/core/templates/collaboration_mode/plan.md index 0999958ec..4028ac8af 100644 --- a/codex-rs/core/templates/collaboration_mode/plan.md +++ b/codex-rs/core/templates/collaboration_mode/plan.md @@ -62,7 +62,7 @@ Rules: * Status updates must not include questions or plan content. * Internal tool/repo exploration is allowed privately before A, B, or C. -Status updates are required and should be frequent during exploration. Provide 1-2 sentence updates that summarize discoveries, assumption changes, or why you are changing direction. Provide an update before beginning a new area of exploration. +Status updates should be frequent during exploration. Provide 1-2 sentence updates that summarize discoveries, assumption changes, or why you are changing direction. Use Parallel tools for exploration. ## Ask a lot, but never ask trivia diff --git a/codex-rs/tui/src/bottom_pane/request_user_input/layout.rs b/codex-rs/tui/src/bottom_pane/request_user_input/layout.rs index 7aa54e953..bb2b06dea 100644 --- a/codex-rs/tui/src/bottom_pane/request_user_input/layout.rs +++ b/codex-rs/tui/src/bottom_pane/request_user_input/layout.rs @@ -19,100 +19,301 @@ pub(super) struct LayoutSections { impl RequestUserInputOverlay { /// Compute layout sections, collapsing notes and hints as space shrinks. pub(super) fn layout_sections(&self, area: Rect) -> LayoutSections { - let question_lines = self - .current_question() - .map(|q| { - textwrap::wrap(&q.question, area.width.max(1) as usize) - .into_iter() - .map(|line| line.to_string()) - .collect::>() - }) - .unwrap_or_default(); - let question_text_height = question_lines.len() as u16; let has_options = self.has_options(); - let mut notes_input_height = self.notes_input_height(area.width); - // Keep the question + options visible first; notes and hints collapse as space shrinks. - let footer_lines = if self.unanswered_count() > 0 { 2 } else { 1 }; - let mut notes_title_height = if has_options { 1 } else { 0 }; + let footer_pref = if self.unanswered_count() > 0 { 2 } else { 1 }; + let notes_pref_height = self.notes_input_height(area.width); + let mut question_lines = self.wrapped_question_lines(area.width); + let question_height = question_lines.len() as u16; + let ( + question_height, + progress_height, + answer_title_height, + notes_title_height, + notes_height, + options_height, + footer_lines, + ) = if has_options { + self.layout_with_options( + area.height, + area.width, + question_height, + notes_pref_height, + footer_pref, + &mut question_lines, + ) + } else { + self.layout_without_options( + area.height, + question_height, + notes_pref_height, + footer_pref, + &mut question_lines, + ) + }; + + let ( + progress_area, + header_area, + question_area, + answer_title_area, + options_area, + notes_title_area, + notes_area, + ) = self.build_layout_areas( + area, + progress_height, + question_height, + answer_title_height, + options_height, + notes_title_height, + notes_height, + ); + + LayoutSections { + progress_area, + header_area, + question_area, + answer_title_area, + question_lines, + options_area, + notes_title_area, + notes_area, + footer_lines, + } + } + + /// Layout calculation when options are present. + /// + /// Handles both tight layout (when space is constrained) and normal layout + /// (when there's sufficient space for all elements). + /// + /// Returns: (question_height, progress_height, answer_title_height, notes_title_height, notes_height, options_height, footer_lines) + fn layout_with_options( + &self, + available_height: u16, + width: u16, + question_height: u16, + notes_pref_height: u16, + footer_pref: u16, + question_lines: &mut Vec, + ) -> (u16, u16, u16, u16, u16, u16, u16) { + let options_required_height = self.options_required_height(width); + let min_options_height = 1u16; + let required = 1u16 + .saturating_add(question_height) + .saturating_add(options_required_height); + + if required > available_height { + self.layout_with_options_tight( + available_height, + question_height, + min_options_height, + question_lines, + ) + } else { + self.layout_with_options_normal( + available_height, + question_height, + options_required_height, + notes_pref_height, + footer_pref, + ) + } + } + + /// Tight layout for options case: allocate header + question + options first + /// and drop everything else when space is constrained. + fn layout_with_options_tight( + &self, + available_height: u16, + question_height: u16, + min_options_height: u16, + question_lines: &mut Vec, + ) -> (u16, u16, u16, u16, u16, u16, u16) { + let max_question_height = + available_height.saturating_sub(1u16.saturating_add(min_options_height)); + let adjusted_question_height = question_height.min(max_question_height); + question_lines.truncate(adjusted_question_height as usize); + let options_height = + available_height.saturating_sub(1u16.saturating_add(adjusted_question_height)); + + (adjusted_question_height, 0, 0, 0, 0, options_height, 0) + } + + /// Normal layout for options case: allocate space for all elements with + /// preference order: notes, footer, labels, then progress. + fn layout_with_options_normal( + &self, + available_height: u16, + question_height: u16, + options_required_height: u16, + notes_pref_height: u16, + footer_pref: u16, + ) -> (u16, u16, u16, u16, u16, u16, u16) { + let options_height = options_required_height; + let used = 1u16 + .saturating_add(question_height) + .saturating_add(options_height); + let mut remaining = available_height.saturating_sub(used); + + // Prefer notes next, then footer, then labels, with progress last. + let mut notes_height = notes_pref_height.min(remaining); + remaining = remaining.saturating_sub(notes_height); + + let footer_lines = footer_pref.min(remaining); + remaining = remaining.saturating_sub(footer_lines); + + let mut answer_title_height = 0; + if remaining > 0 { + answer_title_height = 1; + remaining = remaining.saturating_sub(1); + } + + let mut notes_title_height = 0; + if remaining > 0 { + notes_title_height = 1; + remaining = remaining.saturating_sub(1); + } + + let mut progress_height = 0; + if remaining > 0 { + progress_height = 1; + remaining = remaining.saturating_sub(1); + } + + // Expand the notes composer with any leftover rows. + notes_height = notes_height.saturating_add(remaining); + + ( + question_height, + progress_height, + answer_title_height, + notes_title_height, + notes_height, + options_height, + footer_lines, + ) + } + + /// Layout calculation when no options are present. + /// + /// Handles both tight layout (when space is constrained) and normal layout + /// (when there's sufficient space for all elements). + /// + /// Returns: (question_height, progress_height, answer_title_height, notes_title_height, notes_height, options_height, footer_lines) + fn layout_without_options( + &self, + available_height: u16, + question_height: u16, + notes_pref_height: u16, + footer_pref: u16, + question_lines: &mut Vec, + ) -> (u16, u16, u16, u16, u16, u16, u16) { + let required = 1u16.saturating_add(question_height); + if required > available_height { + self.layout_without_options_tight(available_height, question_height, question_lines) + } else { + self.layout_without_options_normal( + available_height, + question_height, + notes_pref_height, + footer_pref, + ) + } + } + + /// Tight layout for no-options case: truncate question to fit available space. + fn layout_without_options_tight( + &self, + available_height: u16, + question_height: u16, + question_lines: &mut Vec, + ) -> (u16, u16, u16, u16, u16, u16, u16) { + let max_question_height = available_height.saturating_sub(1); + let adjusted_question_height = question_height.min(max_question_height); + question_lines.truncate(adjusted_question_height as usize); + + (adjusted_question_height, 0, 0, 0, 0, 0, 0) + } + + /// Normal layout for no-options case: allocate space for notes, footer, and progress. + fn layout_without_options_normal( + &self, + available_height: u16, + question_height: u16, + notes_pref_height: u16, + footer_pref: u16, + ) -> (u16, u16, u16, u16, u16, u16, u16) { + let required = 1u16.saturating_add(question_height); + let mut remaining = available_height.saturating_sub(required); + let mut notes_height = notes_pref_height.min(remaining); + remaining = remaining.saturating_sub(notes_height); + + let footer_lines = footer_pref.min(remaining); + remaining = remaining.saturating_sub(footer_lines); + + let mut progress_height = 0; + if remaining > 0 { + progress_height = 1; + remaining = remaining.saturating_sub(1); + } + + notes_height = notes_height.saturating_add(remaining); + + ( + question_height, + progress_height, + 0, + 0, + notes_height, + 0, + footer_lines, + ) + } + + /// Build the final layout areas from computed heights. + fn build_layout_areas( + &self, + area: Rect, + progress_height: u16, + question_height: u16, + answer_title_height: u16, + options_height: u16, + notes_title_height: u16, + notes_height: u16, + ) -> ( + Rect, // progress_area + Rect, // header_area + Rect, // question_area + Rect, // answer_title_area + Rect, // options_area + Rect, // notes_title_area + Rect, // notes_area + ) { let mut cursor_y = area.y; let progress_area = Rect { x: area.x, y: cursor_y, width: area.width, - height: 1, + height: progress_height, }; - cursor_y = cursor_y.saturating_add(1); + cursor_y = cursor_y.saturating_add(progress_height); + let header_height = area.height.saturating_sub(progress_height).min(1); let header_area = Rect { x: area.x, y: cursor_y, width: area.width, - height: 1, + height: header_height, }; - cursor_y = cursor_y.saturating_add(1); + cursor_y = cursor_y.saturating_add(header_height); let question_area = Rect { x: area.x, y: cursor_y, width: area.width, - height: question_text_height, + height: question_height, }; - cursor_y = cursor_y.saturating_add(question_text_height); - // Remaining height after progress/header/question areas. - let remaining = area.height.saturating_sub(cursor_y.saturating_sub(area.y)); - let mut answer_title_height = if has_options { 1 } else { 0 }; - let mut options_height = 0; - if has_options { - let remaining_content = remaining.saturating_sub(footer_lines); - let options_len = self.options_len() as u16; - if remaining_content == 0 { - answer_title_height = 0; - notes_title_height = 0; - notes_input_height = 0; - options_height = 0; - } else { - let min_notes = 1u16; - let full_notes = 3u16; - // Prefer to keep all options visible, then allocate notes height. - if remaining_content - >= options_len + answer_title_height + notes_title_height + full_notes - { - let max_notes = remaining_content - .saturating_sub(options_len) - .saturating_sub(answer_title_height) - .saturating_sub(notes_title_height); - notes_input_height = notes_input_height.min(max_notes).max(full_notes); - } else if remaining_content > options_len + answer_title_height + min_notes { - notes_title_height = 0; - notes_input_height = min_notes; - } else { - // Tight layout: hide section titles and shrink notes to one line. - answer_title_height = 0; - notes_title_height = 0; - notes_input_height = min_notes; - } - - // Reserve notes/answer title area so options are scrollable if needed. - let reserved = answer_title_height - .saturating_add(notes_title_height) - .saturating_add(notes_input_height); - options_height = remaining_content.saturating_sub(reserved); - if options_height > options_len { - // Expand the notes composer with any leftover rows so we - // do not leave a large blank gap between options and notes. - let extra_rows = options_height.saturating_sub(options_len); - options_height = options_len; - notes_input_height = notes_input_height.saturating_add(extra_rows); - } - } - } else { - let max_notes = remaining.saturating_sub(footer_lines); - if max_notes == 0 { - notes_input_height = 0; - } else { - // When no options exist, notes are the primary input. - notes_input_height = notes_input_height.min(max_notes).max(3.min(max_notes)); - } - } + cursor_y = cursor_y.saturating_add(question_height); let answer_title_area = Rect { x: area.x, @@ -140,19 +341,17 @@ impl RequestUserInputOverlay { x: area.x, y: cursor_y, width: area.width, - height: notes_input_height, + height: notes_height, }; - LayoutSections { + ( progress_area, header_area, question_area, answer_title_area, - question_lines, options_area, notes_title_area, notes_area, - footer_lines, - } + ) } } diff --git a/codex-rs/tui/src/bottom_pane/request_user_input/mod.rs b/codex-rs/tui/src/bottom_pane/request_user_input/mod.rs index c710b2b2c..1da955ec3 100644 --- a/codex-rs/tui/src/bottom_pane/request_user_input/mod.rs +++ b/codex-rs/tui/src/bottom_pane/request_user_input/mod.rs @@ -24,6 +24,8 @@ use crate::bottom_pane::ChatComposerConfig; use crate::bottom_pane::InputResult; use crate::bottom_pane::bottom_pane_view::BottomPaneView; use crate::bottom_pane::scroll_state::ScrollState; +use crate::bottom_pane::selection_popup_common::GenericDisplayRow; +use crate::bottom_pane::selection_popup_common::measure_rows_height; use crate::render::renderable::Renderable; use codex_core::protocol::Op; @@ -164,6 +166,62 @@ impl RequestUserInputOverlay { .map(|option| option.label.as_str()) } + pub(super) fn wrapped_question_lines(&self, width: u16) -> Vec { + self.current_question() + .map(|q| { + textwrap::wrap(&q.question, width.max(1) as usize) + .into_iter() + .map(|line| line.to_string()) + .collect::>() + }) + .unwrap_or_default() + } + + pub(super) fn option_rows(&self) -> Vec { + self.current_question() + .and_then(|question| question.options.as_ref()) + .map(|options| { + options + .iter() + .enumerate() + .map(|(idx, opt)| { + let selected = self + .current_answer() + .and_then(|answer| answer.selected) + .is_some_and(|sel| sel == idx); + let prefix = if selected { "(x)" } else { "( )" }; + GenericDisplayRow { + name: format!("{prefix} {}", opt.label), + description: Some(opt.description.clone()), + ..Default::default() + } + }) + .collect::>() + }) + .unwrap_or_default() + } + + pub(super) fn options_required_height(&self, width: u16) -> u16 { + if !self.has_options() { + return 0; + } + + let rows = self.option_rows(); + if rows.is_empty() { + return 1; + } + + let mut state = self + .current_answer() + .map(|answer| answer.option_state) + .unwrap_or_default(); + if state.selected_idx.is_none() { + state.selected_idx = Some(0); + } + + measure_rows_height(&rows, &state, rows.len(), width.max(1)) + } + fn capture_composer_draft(&self) -> ComposerDraft { ComposerDraft { text: self.composer.current_text_with_pending(), @@ -594,6 +652,35 @@ mod tests { } } + fn question_with_wrapped_options(id: &str, header: &str) -> RequestUserInputQuestion { + RequestUserInputQuestion { + id: id.to_string(), + header: header.to_string(), + question: "Choose the next step for this task.".to_string(), + is_other: false, + options: Some(vec![ + RequestUserInputQuestionOption { + label: "Discuss a code change".to_string(), + description: + "Walk through a plan, then implement it together with careful checks." + .to_string(), + }, + RequestUserInputQuestionOption { + label: "Run targeted tests".to_string(), + description: + "Pick the most relevant crate and validate the current behavior first." + .to_string(), + }, + RequestUserInputQuestionOption { + label: "Review the diff".to_string(), + description: + "Summarize the changes and highlight the most important risks and gaps." + .to_string(), + }, + ]), + } + } + fn question_without_options(id: &str, header: &str) -> RequestUserInputQuestion { RequestUserInputQuestion { id: id.to_string(), @@ -797,6 +884,60 @@ mod tests { ); } + #[test] + fn layout_allocates_all_wrapped_options_when_space_allows() { + let (tx, _rx) = test_sender(); + let overlay = RequestUserInputOverlay::new( + request_event( + "turn-1", + vec![question_with_wrapped_options("q1", "Next Step")], + ), + tx, + true, + false, + false, + ); + + let width = 48u16; + let question_height = overlay.wrapped_question_lines(width).len() as u16; + let options_height = overlay.options_required_height(width); + let height = 1u16 + .saturating_add(question_height) + .saturating_add(options_height) + .saturating_add(4); + let sections = overlay.layout_sections(Rect::new(0, 0, width, height)); + + assert_eq!(sections.options_area.height, options_height); + } + + #[test] + fn request_user_input_wrapped_options_snapshot() { + let (tx, _rx) = test_sender(); + let overlay = RequestUserInputOverlay::new( + request_event( + "turn-1", + vec![question_with_wrapped_options("q1", "Next Step")], + ), + tx, + true, + false, + false, + ); + + let width = 52u16; + let question_height = overlay.wrapped_question_lines(width).len() as u16; + let options_height = overlay.options_required_height(width); + let height = 1u16 + .saturating_add(question_height) + .saturating_add(options_height) + .saturating_add(4); + let area = Rect::new(0, 0, width, height); + insta::assert_snapshot!( + "request_user_input_wrapped_options", + render_snapshot(&overlay, area) + ); + } + #[test] fn request_user_input_scroll_options_snapshot() { let (tx, _rx) = test_sender(); diff --git a/codex-rs/tui/src/bottom_pane/request_user_input/render.rs b/codex-rs/tui/src/bottom_pane/request_user_input/render.rs index 368f174b5..b64f70434 100644 --- a/codex-rs/tui/src/bottom_pane/request_user_input/render.rs +++ b/codex-rs/tui/src/bottom_pane/request_user_input/render.rs @@ -6,7 +6,6 @@ use ratatui::text::Line; use ratatui::widgets::Paragraph; use ratatui::widgets::Widget; -use crate::bottom_pane::selection_popup_common::GenericDisplayRow; use crate::bottom_pane::selection_popup_common::menu_surface_inset; use crate::bottom_pane::selection_popup_common::menu_surface_padding_height; use crate::bottom_pane::selection_popup_common::render_menu_surface; @@ -21,21 +20,21 @@ impl Renderable for RequestUserInputOverlay { let outer = Rect::new(0, 0, width, u16::MAX); let inner = menu_surface_inset(outer); let inner_width = inner.width.max(1); - let sections = self.layout_sections(Rect::new(0, 0, inner_width, u16::MAX)); - let question_height = sections.question_lines.len(); + let question_height = self.wrapped_question_lines(inner_width).len(); + let options_height = self.options_required_height(inner_width) as usize; let notes_height = self.notes_input_height(inner_width) as usize; - let footer_height = sections.footer_lines as usize; + let footer_height = if self.unanswered_count() > 0 { 2 } else { 1 }; // Tight minimum height: progress + header + question + (optional) titles/options // + notes composer + footer + menu padding. let mut height = question_height + .saturating_add(options_height) .saturating_add(notes_height) .saturating_add(footer_height) .saturating_add(2); // progress + header if self.has_options() { height = height .saturating_add(1) // answer title - .saturating_add(self.options_len()) .saturating_add(1); // notes title } height = height.saturating_add(menu_surface_padding_height() as usize); @@ -113,28 +112,7 @@ impl RequestUserInputOverlay { } // Build rows with selection markers for the shared selection renderer. - let option_rows = self - .current_question() - .and_then(|question| question.options.as_ref()) - .map(|options| { - options - .iter() - .enumerate() - .map(|(idx, opt)| { - let selected = self - .current_answer() - .and_then(|answer| answer.selected) - .is_some_and(|sel| sel == idx); - let prefix = if selected { "(x)" } else { "( )" }; - GenericDisplayRow { - name: format!("{prefix} {}", opt.label), - description: Some(opt.description.clone()), - ..Default::default() - } - }) - .collect::>() - }) - .unwrap_or_default(); + let option_rows = self.option_rows(); if self.has_options() { let mut option_state = self diff --git a/codex-rs/tui/src/bottom_pane/request_user_input/snapshots/codex_tui__bottom_pane__request_user_input__tests__request_user_input_scrolling_options.snap b/codex-rs/tui/src/bottom_pane/request_user_input/snapshots/codex_tui__bottom_pane__request_user_input__tests__request_user_input_scrolling_options.snap index f15d53e11..ace8eb453 100644 --- a/codex-rs/tui/src/bottom_pane/request_user_input/snapshots/codex_tui__bottom_pane__request_user_input__tests__request_user_input_scrolling_options.snap +++ b/codex-rs/tui/src/bottom_pane/request_user_input/snapshots/codex_tui__bottom_pane__request_user_input__tests__request_user_input_scrolling_options.snap @@ -3,11 +3,12 @@ source: tui/src/bottom_pane/request_user_input/mod.rs expression: "render_snapshot(&overlay, area)" --- - Question 1/1 Next Step What would you like to do next? - ( ) Run tests Pick a crate and run its tests. - ( ) Review a diff Summarize or review current changes. - (x) Refactor Tighten structure and remove dead code. - + ( ) Discuss a code change (Recommended) Walk through a plan and + edit code together. + ( ) Run tests Pick a crate and run + its tests. + ( ) Review a diff Summarize or review + current changes. Option 4 of 5 | ↑/↓ scroll | enter next question | esc interrupt diff --git a/codex-rs/tui/src/bottom_pane/request_user_input/snapshots/codex_tui__bottom_pane__request_user_input__tests__request_user_input_tight_height.snap b/codex-rs/tui/src/bottom_pane/request_user_input/snapshots/codex_tui__bottom_pane__request_user_input__tests__request_user_input_tight_height.snap index 7820e0d0f..e8cd2bd22 100644 --- a/codex-rs/tui/src/bottom_pane/request_user_input/snapshots/codex_tui__bottom_pane__request_user_input__tests__request_user_input_tight_height.snap +++ b/codex-rs/tui/src/bottom_pane/request_user_input/snapshots/codex_tui__bottom_pane__request_user_input__tests__request_user_input_tight_height.snap @@ -3,9 +3,10 @@ source: tui/src/bottom_pane/request_user_input/mod.rs expression: "render_snapshot(&overlay, area)" --- - Question 1/1 Area Choose an option. (x) Option 1 First choice. + ( ) Option 2 Second choice. + ( ) Option 3 Third choice. Option 1 of 3 | ↑/↓ scroll | enter next question | esc i diff --git a/codex-rs/tui/src/bottom_pane/request_user_input/snapshots/codex_tui__bottom_pane__request_user_input__tests__request_user_input_wrapped_options.snap b/codex-rs/tui/src/bottom_pane/request_user_input/snapshots/codex_tui__bottom_pane__request_user_input__tests__request_user_input_wrapped_options.snap new file mode 100644 index 000000000..4ae9dd048 --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/request_user_input/snapshots/codex_tui__bottom_pane__request_user_input__tests__request_user_input_wrapped_options.snap @@ -0,0 +1,21 @@ +--- +source: tui/src/bottom_pane/request_user_input/mod.rs +expression: "render_snapshot(&overlay, area)" +--- + + Next Step + Choose the next step for this task. + (x) Discuss a code change Walk through a plan, + then implement it + together with careful + checks. + ( ) Run targeted tests Pick the most + relevant crate and + validate the current + behavior first. + ( ) Review the diff Summarize the changes + and highlight the + most important risks + and gaps. + + Option 1 of 3 | ↑/↓ scroll | enter next question