Skip to content

Add spacing before composer footer hints #3469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 12, 2025
+92 −19
Merged

Add spacing before composer footer hints #3469

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9421005
Add spacing before composer footer hints
aibrahim-oai Sep 11, 2025
8481975
clippy
aibrahim-oai Sep 11, 2025
642cfd9
max instead of length
aibrahim-oai Sep 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 84 additions & 13 deletions codex-rs/tui/src/bottom_pane/chat_composer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ enum ActivePopup {
File(FileSearchPopup),
}

const FOOTER_HINT_HEIGHT: u16 = 1;
const FOOTER_SPACING_HEIGHT: u16 = 1;
const FOOTER_HEIGHT_WITH_HINT: u16 = FOOTER_HINT_HEIGHT + FOOTER_SPACING_HEIGHT;

impl ChatComposer {
pub fn new(
has_input_focus: bool,
Expand Down Expand Up @@ -134,20 +138,20 @@ impl ChatComposer {
pub fn desired_height(&self, width: u16) -> u16 {
self.textarea.desired_height(width - 1)
+ match &self.active_popup {
ActivePopup::None => 1u16,
ActivePopup::None => FOOTER_HEIGHT_WITH_HINT,
ActivePopup::Command(c) => c.calculate_required_height(),
ActivePopup::File(c) => c.calculate_required_height(),
}
}

pub fn cursor_pos(&self, area: Rect) -> Option<(u16, u16)> {
let popup_height = match &self.active_popup {
ActivePopup::Command(popup) => popup.calculate_required_height(),
ActivePopup::File(popup) => popup.calculate_required_height(),
ActivePopup::None => 1,
let popup_constraint = match &self.active_popup {
ActivePopup::Command(popup) => Constraint::Max(popup.calculate_required_height()),
ActivePopup::File(popup) => Constraint::Max(popup.calculate_required_height()),
ActivePopup::None => Constraint::Length(FOOTER_HEIGHT_WITH_HINT),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i feel like this should still be Constraint::Max?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed it

};
let [textarea_rect, _] =
Layout::vertical([Constraint::Min(1), Constraint::Max(popup_height)]).areas(area);
Layout::vertical([Constraint::Min(1), popup_constraint]).areas(area);
let mut textarea_rect = textarea_rect;
textarea_rect.width = textarea_rect.width.saturating_sub(1);
textarea_rect.x += 1;
Expand Down Expand Up @@ -1219,13 +1223,16 @@ impl ChatComposer {

impl WidgetRef for ChatComposer {
fn render_ref(&self, area: Rect, buf: &mut Buffer) {
let popup_height = match &self.active_popup {
ActivePopup::Command(popup) => popup.calculate_required_height(),
ActivePopup::File(popup) => popup.calculate_required_height(),
ActivePopup::None => 1,
let (popup_constraint, hint_spacing) = match &self.active_popup {
ActivePopup::Command(popup) => (Constraint::Max(popup.calculate_required_height()), 0),
ActivePopup::File(popup) => (Constraint::Max(popup.calculate_required_height()), 0),
ActivePopup::None => (
Constraint::Length(FOOTER_HEIGHT_WITH_HINT),
FOOTER_SPACING_HEIGHT,
),
};
let [textarea_rect, popup_rect] =
Layout::vertical([Constraint::Min(1), Constraint::Max(popup_height)]).areas(area);
Layout::vertical([Constraint::Min(1), popup_constraint]).areas(area);
match &self.active_popup {
ActivePopup::Command(popup) => {
popup.render_ref(popup_rect, buf);
Expand All @@ -1234,7 +1241,16 @@ impl WidgetRef for ChatComposer {
popup.render_ref(popup_rect, buf);
}
ActivePopup::None => {
let bottom_line_rect = popup_rect;
let hint_rect = if hint_spacing > 0 {
let [_, hint_rect] = Layout::vertical([
Constraint::Length(hint_spacing),
Constraint::Length(FOOTER_HINT_HEIGHT),
])
.areas(popup_rect);
hint_rect
} else {
popup_rect
};
let mut hint: Vec<Span<'static>> = if self.ctrl_c_quit_hint {
let ctrl_c_followup = if self.is_task_running {
" to interrupt"
Expand Down Expand Up @@ -1305,7 +1321,7 @@ impl WidgetRef for ChatComposer {

Line::from(hint)
.style(Style::default().dim())
.render_ref(bottom_line_rect, buf);
.render_ref(hint_rect, buf);
}
}
let border_style = if self.has_focus {
Expand Down Expand Up @@ -1340,6 +1356,7 @@ mod tests {
use super::*;
use image::ImageBuffer;
use image::Rgba;
use pretty_assertions::assert_eq;
use std::path::PathBuf;
use tempfile::tempdir;

Expand All @@ -1352,6 +1369,60 @@ mod tests {
use crate::bottom_pane::textarea::TextArea;
use tokio::sync::mpsc::unbounded_channel;

#[test]
fn footer_hint_row_is_separated_from_composer() {
let (tx, _rx) = unbounded_channel::<AppEvent>();
let sender = AppEventSender::new(tx);
let composer = ChatComposer::new(
true,
sender,
false,
"Ask Codex to do anything".to_string(),
false,
);

let area = Rect::new(0, 0, 40, 6);
let mut buf = Buffer::empty(area);
composer.render_ref(area, &mut buf);

let row_to_string = |y: u16| {
let mut row = String::new();
for x in 0..area.width {
row.push(buf[(x, y)].symbol().chars().next().unwrap_or(' '));
}
row
};

let mut hint_row: Option<(u16, String)> = None;
for y in 0..area.height {
let row = row_to_string(y);
if row.contains(" send") {
hint_row = Some((y, row));
break;
}
}

let (hint_row_idx, hint_row_contents) =
hint_row.expect("expected footer hint row to be rendered");
assert_eq!(
hint_row_idx,
area.height - 1,
"hint row should occupy the bottom line: {hint_row_contents:?}",
);

assert!(
hint_row_idx > 0,
"expected a spacing row above the footer hints",
);

let spacing_row = row_to_string(hint_row_idx - 1);
assert_eq!(
spacing_row.trim(),
"",
"expected blank spacing row above hints but saw: {spacing_row:?}",
);
}

#[test]
fn test_current_at_token_basic_cases() {
let test_cases = vec![
Expand Down
2 changes: 1 addition & 1 deletion ..._pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__backspace_after_pastes.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ expression: terminal.backend()
"▌ "
"▌ "
"▌ "
"▌ "
" "
" ⏎ send ⌃J newline ⌃T transcript ⌃C quit "
2 changes: 1 addition & 1 deletion ...rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__empty.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ expression: terminal.backend()
"▌ "
"▌ "
"▌ "
"▌ "
" "
" ⏎ send ⌃J newline ⌃T transcript ⌃C quit "
2 changes: 1 addition & 1 deletion ...rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__large.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ expression: terminal.backend()
"▌ "
"▌ "
"▌ "
"▌ "
" "
" ⏎ send ⌃J newline ⌃T transcript ⌃C quit "
2 changes: 1 addition & 1 deletion .../bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__multiple_pastes.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ expression: terminal.backend()
"▌ "
"▌ "
"▌ "
"▌ "
" "
" ⏎ send ⌃J newline ⌃T transcript ⌃C quit "
2 changes: 1 addition & 1 deletion ...rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__small.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ expression: terminal.backend()
"▌ "
"▌ "
"▌ "
"▌ "
" "
" ⏎ send ⌃J newline ⌃T transcript ⌃C quit "
2 changes: 1 addition & 1 deletion codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chat_small_idle_h2.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@ source: tui/src/chatwidget/tests.rs
expression: terminal.backend()
---
"▌ Ask Codex to do anything "
" ⏎ send ⌃J newline ⌃T transcript ⌃"
" "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like we lost the hints here? i feel this is a bit worse for very short terminals, though it's a bit of an edge case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should optimize for the size we recommend. @edward-bayes thinks it's a better UI like that.

1 change: 1 addition & 0 deletions ...shots/codex_tui__chatwidget__tests__chatwidget_exec_and_status_layout_vt100_snapshot.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,5 @@ expression: visual
Investigating rendering code (0s • Esc to interrupt)

▌Summarize recent commits

⏎ send ⌃J newline ⌃T transcript ⌃C quit
1 change: 1 addition & 0 deletions ...x-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_active.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@ expression: terminal.backend()
" Analyzing (0s • Esc to interrupt) "
" "
"▌ Ask Codex to do anything "
" "
" ⏎ send ⌃J newline ⌃T transcript ⌃C quit "
" "
Loading