tui: keep history recall cursor at line end (#11295)
## Summary - keep cursor at end-of-line after Up/Down history recall - allow continued history navigation when recalled text cursor is at start or end boundary - add regression tests and document the history cursor contract in composer docs ## Testing - just fmt - cargo test -p codex-tui --lib history_navigation_leaves_cursor_at_end_of_line - cargo test -p codex-tui --lib should_handle_navigation_when_cursor_is_at_line_boundaries - cargo test -p codex-tui *(fails in existing integration test `suite::no_panic_on_startup::malformed_rules_should_not_panic` because `target/debug/codex` is not present in this environment)*
This commit is contained in:
parent
3322b99900
commit
e704f488bd
3 changed files with 99 additions and 10 deletions
|
|
@ -25,6 +25,8 @@
|
|||
//!
|
||||
//! When recalling a local entry, the composer rehydrates text elements and image attachments.
|
||||
//! When recalling a persistent entry, only the text is restored.
|
||||
//! Recalled entries move the cursor to end-of-line so repeated Up/Down presses keep shell-like
|
||||
//! history traversal semantics instead of dropping to column 0.
|
||||
//!
|
||||
//! # Submission and Prompt Expansion
|
||||
//!
|
||||
|
|
@ -532,9 +534,12 @@ impl ChatComposer {
|
|||
self.history.set_metadata(log_id, entry_count);
|
||||
}
|
||||
|
||||
/// Integrate an asynchronous response to an on-demand history lookup. If
|
||||
/// the entry is present and the offset matches the current cursor we
|
||||
/// immediately populate the textarea.
|
||||
/// Integrate an asynchronous response to an on-demand history lookup.
|
||||
///
|
||||
/// If the entry is present and the offset still matches the active history cursor, the
|
||||
/// composer rehydrates the entry immediately. This path intentionally routes through
|
||||
/// [`Self::apply_history_entry`] so cursor placement remains aligned with keyboard history
|
||||
/// recall semantics.
|
||||
pub(crate) fn on_history_entry_response(
|
||||
&mut self,
|
||||
log_id: u64,
|
||||
|
|
@ -783,6 +788,10 @@ impl ChatComposer {
|
|||
/// draft; if callers restore only text and images, mentions can appear
|
||||
/// intact to users while resolving to the wrong target or dropping on
|
||||
/// retry.
|
||||
///
|
||||
/// This helper intentionally places the cursor at the start of the restored text. Callers
|
||||
/// that need end-of-line restore behavior (for example shell-style history recall) should call
|
||||
/// [`Self::move_cursor_to_end`] after this method.
|
||||
pub(crate) fn set_text_content_with_mention_bindings(
|
||||
&mut self,
|
||||
text: String,
|
||||
|
|
@ -857,6 +866,13 @@ impl ChatComposer {
|
|||
self.textarea.text().to_string()
|
||||
}
|
||||
|
||||
/// Rehydrate a history entry into the composer with shell-like cursor placement.
|
||||
///
|
||||
/// This path restores text, elements, images, mention bindings, and pending paste payloads,
|
||||
/// then moves the cursor to end-of-line. If a caller reused
|
||||
/// [`Self::set_text_content_with_mention_bindings`] directly for history recall and forgot the
|
||||
/// final cursor move, repeated Up/Down would stop navigating history because cursor-gating
|
||||
/// treats interior positions as normal editing mode.
|
||||
fn apply_history_entry(&mut self, entry: HistoryEntry) {
|
||||
let HistoryEntry {
|
||||
text,
|
||||
|
|
@ -872,6 +888,7 @@ impl ChatComposer {
|
|||
mention_bindings,
|
||||
);
|
||||
self.set_pending_pastes(pending_pastes);
|
||||
self.move_cursor_to_end();
|
||||
}
|
||||
|
||||
pub(crate) fn text_elements(&self) -> Vec<TextElement> {
|
||||
|
|
@ -5973,6 +5990,51 @@ mod tests {
|
|||
assert_eq!(text_elements.len(), 1);
|
||||
assert_eq!(text_elements[0].placeholder(&text), Some("[Image #1]"));
|
||||
assert_eq!(composer.local_image_paths(), vec![path]);
|
||||
assert_eq!(composer.textarea.cursor(), composer.textarea.text().len());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn history_navigation_leaves_cursor_at_end_of_line() {
|
||||
let (tx, _rx) = unbounded_channel::<AppEvent>();
|
||||
let sender = AppEventSender::new(tx);
|
||||
let mut composer = ChatComposer::new(
|
||||
true,
|
||||
sender,
|
||||
false,
|
||||
"Ask Codex to do anything".to_string(),
|
||||
false,
|
||||
);
|
||||
composer.set_steer_enabled(true);
|
||||
|
||||
type_chars_humanlike(&mut composer, &['f', 'i', 'r', 's', 't']);
|
||||
let (result, _needs_redraw) =
|
||||
composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
|
||||
assert!(matches!(result, InputResult::Submitted { .. }));
|
||||
|
||||
type_chars_humanlike(&mut composer, &['s', 'e', 'c', 'o', 'n', 'd']);
|
||||
let (result, _needs_redraw) =
|
||||
composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
|
||||
assert!(matches!(result, InputResult::Submitted { .. }));
|
||||
|
||||
let (_result, _needs_redraw) =
|
||||
composer.handle_key_event(KeyEvent::new(KeyCode::Up, KeyModifiers::NONE));
|
||||
assert_eq!(composer.textarea.text(), "second");
|
||||
assert_eq!(composer.textarea.cursor(), composer.textarea.text().len());
|
||||
|
||||
let (_result, _needs_redraw) =
|
||||
composer.handle_key_event(KeyEvent::new(KeyCode::Up, KeyModifiers::NONE));
|
||||
assert_eq!(composer.textarea.text(), "first");
|
||||
assert_eq!(composer.textarea.cursor(), composer.textarea.text().len());
|
||||
|
||||
let (_result, _needs_redraw) =
|
||||
composer.handle_key_event(KeyEvent::new(KeyCode::Down, KeyModifiers::NONE));
|
||||
assert_eq!(composer.textarea.text(), "second");
|
||||
assert_eq!(composer.textarea.cursor(), composer.textarea.text().len());
|
||||
|
||||
let (_result, _needs_redraw) =
|
||||
composer.handle_key_event(KeyEvent::new(KeyCode::Down, KeyModifiers::NONE));
|
||||
assert!(composer.textarea.is_empty());
|
||||
assert_eq!(composer.textarea.cursor(), composer.textarea.text().len());
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
|
|
|||
|
|
@ -82,7 +82,8 @@ pub(crate) struct ChatComposerHistory {
|
|||
|
||||
/// The text that was last inserted into the composer as a result of
|
||||
/// history navigation. Used to decide if further Up/Down presses should be
|
||||
/// treated as navigation versus normal cursor movement.
|
||||
/// treated as navigation versus normal cursor movement, together with the
|
||||
/// "cursor at line boundary" check in [`Self::should_handle_navigation`].
|
||||
last_history_text: Option<String>,
|
||||
}
|
||||
|
||||
|
|
@ -136,8 +137,16 @@ impl ChatComposerHistory {
|
|||
self.last_history_text = None;
|
||||
}
|
||||
|
||||
/// Should Up/Down key presses be interpreted as history navigation given
|
||||
/// the current content and cursor position of `textarea`?
|
||||
/// Returns whether Up/Down should navigate history for the current textarea state.
|
||||
///
|
||||
/// Empty text always enables history traversal. For non-empty text, this requires both:
|
||||
///
|
||||
/// - the current text exactly matching the last recalled history entry, and
|
||||
/// - the cursor being at a line boundary (start or end).
|
||||
///
|
||||
/// This boundary gate keeps multiline cursor movement usable while preserving shell-like
|
||||
/// history recall. If callers moved the cursor into the middle of a recalled entry and still
|
||||
/// forced navigation, users would lose normal vertical movement within the draft.
|
||||
pub fn should_handle_navigation(&self, text: &str, cursor: usize) -> bool {
|
||||
if self.history_entry_count == 0 && self.local_history.is_empty() {
|
||||
return false;
|
||||
|
|
@ -147,10 +156,11 @@ impl ChatComposerHistory {
|
|||
return true;
|
||||
}
|
||||
|
||||
// Textarea is not empty – only navigate when cursor is at start and
|
||||
// text matches last recalled history entry so regular editing is not
|
||||
// hijacked.
|
||||
if cursor != 0 {
|
||||
// Textarea is not empty – only navigate when text matches the last
|
||||
// recalled history entry and the cursor is at a line boundary. This
|
||||
// keeps shell-like Up/Down recall working while still allowing normal
|
||||
// multiline cursor movement from interior positions.
|
||||
if cursor != 0 && cursor != text.len() {
|
||||
return false;
|
||||
}
|
||||
|
||||
|
|
@ -378,4 +388,16 @@ mod tests {
|
|||
history.navigate_up(&tx)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn should_handle_navigation_when_cursor_is_at_line_boundaries() {
|
||||
let mut history = ChatComposerHistory::new();
|
||||
history.record_local_submission(HistoryEntry::new("hello".to_string()));
|
||||
history.last_history_text = Some("hello".to_string());
|
||||
|
||||
assert!(history.should_handle_navigation("hello", 0));
|
||||
assert!(history.should_handle_navigation("hello", "hello".len()));
|
||||
assert!(!history.should_handle_navigation("hello", 1));
|
||||
assert!(!history.should_handle_navigation("other", 0));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -144,6 +144,10 @@ Local history entries capture:
|
|||
Persistent history entries only restore text. They intentionally do **not** rehydrate attachments
|
||||
or pending paste payloads.
|
||||
|
||||
For non-empty drafts, Up/Down navigation is only treated as history recall when the current text
|
||||
matches the last recalled history entry and the cursor is at a boundary (start or end of the
|
||||
line). This keeps multiline cursor movement intact while preserving shell-like history traversal.
|
||||
|
||||
### Draft recovery (Ctrl+C)
|
||||
|
||||
Ctrl+C clears the composer but stashes the full draft state (text elements, image paths, and
|
||||
|
|
@ -157,6 +161,7 @@ ranges and local image paths. Pending paste payloads are cleared during submissi
|
|||
placeholders are expanded into their full text before being recorded. This means:
|
||||
|
||||
- Up/Down recall of a submitted message restores image placeholders and their local paths.
|
||||
- Recalled entries place the cursor at end-of-line to match typical shell history editing.
|
||||
- Large-paste placeholders are not expected in recalled submitted history; the text is the
|
||||
expanded paste content.
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue