mirror of
https://github.com/openai/codex.git
synced 2026-04-24 14:45:27 +00:00
fix(tui): rehydrate drafts and restore image placeholders (#9040)
Fixes #9050 When a draft is stashed with Ctrl+C, we now persist the full draft state (text elements, local image paths, and pending paste payloads) in local history. Up/Down recall rehydrates placeholder elements and attachments so styling remains correct and large pastes still expand on submit. Persistent (cross‑session) history remains text‑only. Backtrack prefills now reuse the selected user message’s text elements and local image paths, so image placeholders/attachments rehydrate when rolling back. External editor replacements keep only attachments whose placeholders remain and then normalize image placeholders to `[Image #1]..[Image #N]` to keep the attachment mapping consistent. Docs: - docs/tui-chat-composer.md Testing: - just fix -p codex-tui - cargo test -p codex-tui Co-authored-by: Eric Traut <etraut@openai.com>
This commit is contained in:
@@ -540,12 +540,7 @@ impl ChatComposer {
|
||||
};
|
||||
// Persistent ↑/↓ history is text-only (backwards-compatible and avoids persisting
|
||||
// attachments), but local in-session ↑/↓ history can rehydrate elements and image paths.
|
||||
self.set_text_content_with_mention_bindings(
|
||||
entry.text,
|
||||
entry.text_elements,
|
||||
entry.local_image_paths,
|
||||
entry.mention_bindings,
|
||||
);
|
||||
self.apply_history_entry(entry);
|
||||
true
|
||||
}
|
||||
|
||||
@@ -837,6 +832,7 @@ impl ChatComposer {
|
||||
.iter()
|
||||
.map(|img| img.path.clone())
|
||||
.collect();
|
||||
let pending_pastes = std::mem::take(&mut self.pending_pastes);
|
||||
let mention_bindings = self.snapshot_mention_bindings();
|
||||
self.set_text_content(String::new(), Vec::new(), Vec::new());
|
||||
self.history.reset_navigation();
|
||||
@@ -845,6 +841,7 @@ impl ChatComposer {
|
||||
text_elements,
|
||||
local_image_paths,
|
||||
mention_bindings,
|
||||
pending_pastes,
|
||||
});
|
||||
Some(previous)
|
||||
}
|
||||
@@ -854,6 +851,23 @@ impl ChatComposer {
|
||||
self.textarea.text().to_string()
|
||||
}
|
||||
|
||||
fn apply_history_entry(&mut self, entry: HistoryEntry) {
|
||||
let HistoryEntry {
|
||||
text,
|
||||
text_elements,
|
||||
local_image_paths,
|
||||
mention_bindings,
|
||||
pending_pastes,
|
||||
} = entry;
|
||||
self.set_text_content_with_mention_bindings(
|
||||
text,
|
||||
text_elements,
|
||||
local_image_paths,
|
||||
mention_bindings,
|
||||
);
|
||||
self.set_pending_pastes(pending_pastes);
|
||||
}
|
||||
|
||||
pub(crate) fn text_elements(&self) -> Vec<TextElement> {
|
||||
self.textarea.text_elements()
|
||||
}
|
||||
@@ -2065,6 +2079,7 @@ impl ChatComposer {
|
||||
text_elements: text_elements.clone(),
|
||||
local_image_paths,
|
||||
mention_bindings: original_mention_bindings,
|
||||
pending_pastes: Vec::new(),
|
||||
});
|
||||
}
|
||||
self.pending_pastes.clear();
|
||||
@@ -2352,12 +2367,7 @@ impl ChatComposer {
|
||||
_ => unreachable!(),
|
||||
};
|
||||
if let Some(entry) = replace_entry {
|
||||
self.set_text_content_with_mention_bindings(
|
||||
entry.text,
|
||||
entry.text_elements,
|
||||
entry.local_image_paths,
|
||||
entry.mention_bindings,
|
||||
);
|
||||
self.apply_history_entry(entry);
|
||||
return (InputResult::None, true);
|
||||
}
|
||||
}
|
||||
@@ -4057,10 +4067,120 @@ mod tests {
|
||||
|
||||
assert_eq!(
|
||||
composer.history.navigate_up(&composer.app_event_tx),
|
||||
Some(HistoryEntry::from_text("draft text".to_string()))
|
||||
Some(HistoryEntry::new("draft text".to_string()))
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn clear_for_ctrl_c_preserves_pending_paste_history_entry() {
|
||||
use crossterm::event::KeyCode;
|
||||
use crossterm::event::KeyEvent;
|
||||
use crossterm::event::KeyModifiers;
|
||||
|
||||
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,
|
||||
);
|
||||
|
||||
let large = "x".repeat(LARGE_PASTE_CHAR_THRESHOLD + 5);
|
||||
composer.handle_paste(large.clone());
|
||||
let char_count = large.chars().count();
|
||||
let placeholder = format!("[Pasted Content {char_count} chars]");
|
||||
assert_eq!(composer.textarea.text(), placeholder);
|
||||
assert_eq!(
|
||||
composer.pending_pastes,
|
||||
vec![(placeholder.clone(), large.clone())]
|
||||
);
|
||||
|
||||
composer.clear_for_ctrl_c();
|
||||
assert!(composer.is_empty());
|
||||
|
||||
let history_entry = composer
|
||||
.history
|
||||
.navigate_up(&composer.app_event_tx)
|
||||
.expect("expected history entry");
|
||||
let text_elements = vec![TextElement::new(
|
||||
(0..placeholder.len()).into(),
|
||||
Some(placeholder.clone()),
|
||||
)];
|
||||
assert_eq!(
|
||||
history_entry,
|
||||
HistoryEntry::with_pending(
|
||||
placeholder.clone(),
|
||||
text_elements,
|
||||
Vec::new(),
|
||||
vec![(placeholder.clone(), large.clone())]
|
||||
)
|
||||
);
|
||||
|
||||
composer.apply_history_entry(history_entry);
|
||||
assert_eq!(composer.textarea.text(), placeholder);
|
||||
assert_eq!(composer.pending_pastes, vec![(placeholder.clone(), large)]);
|
||||
assert_eq!(composer.textarea.element_payloads(), vec![placeholder]);
|
||||
|
||||
composer.set_steer_enabled(true);
|
||||
let (result, _needs_redraw) =
|
||||
composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
|
||||
match result {
|
||||
InputResult::Submitted {
|
||||
text,
|
||||
text_elements,
|
||||
} => {
|
||||
assert_eq!(text, "x".repeat(LARGE_PASTE_CHAR_THRESHOLD + 5));
|
||||
assert!(text_elements.is_empty());
|
||||
}
|
||||
_ => panic!("expected Submitted"),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn clear_for_ctrl_c_preserves_image_draft_state() {
|
||||
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,
|
||||
);
|
||||
|
||||
let path = PathBuf::from("example.png");
|
||||
composer.attach_image(path.clone());
|
||||
let placeholder = local_image_label_text(1);
|
||||
|
||||
composer.clear_for_ctrl_c();
|
||||
assert!(composer.is_empty());
|
||||
|
||||
let history_entry = composer
|
||||
.history
|
||||
.navigate_up(&composer.app_event_tx)
|
||||
.expect("expected history entry");
|
||||
let text_elements = vec![TextElement::new(
|
||||
(0..placeholder.len()).into(),
|
||||
Some(placeholder.clone()),
|
||||
)];
|
||||
assert_eq!(
|
||||
history_entry,
|
||||
HistoryEntry::with_pending(
|
||||
placeholder.clone(),
|
||||
text_elements,
|
||||
vec![path.clone()],
|
||||
Vec::new()
|
||||
)
|
||||
);
|
||||
|
||||
composer.apply_history_entry(history_entry);
|
||||
assert_eq!(composer.textarea.text(), placeholder);
|
||||
assert_eq!(composer.local_image_paths(), vec![path]);
|
||||
assert_eq!(composer.textarea.element_payloads(), vec![placeholder]);
|
||||
}
|
||||
|
||||
/// Behavior: `?` toggles the shortcut overlay only when the composer is otherwise empty. After
|
||||
/// any typing has occurred, `?` should be inserted as a literal character.
|
||||
#[test]
|
||||
@@ -7481,6 +7601,34 @@ mod tests {
|
||||
assert!(composer.attached_images.is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn apply_external_edit_renumbers_image_placeholders() {
|
||||
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,
|
||||
);
|
||||
|
||||
let first_path = PathBuf::from("img1.png");
|
||||
let second_path = PathBuf::from("img2.png");
|
||||
composer.attach_image(first_path);
|
||||
composer.attach_image(second_path.clone());
|
||||
|
||||
let placeholder2 = local_image_label_text(2);
|
||||
composer.apply_external_edit(format!("Keep {placeholder2}"));
|
||||
|
||||
let placeholder1 = local_image_label_text(1);
|
||||
assert_eq!(composer.current_text(), format!("Keep {placeholder1}"));
|
||||
assert_eq!(composer.attached_images.len(), 1);
|
||||
assert_eq!(composer.attached_images[0].placeholder, placeholder1);
|
||||
assert_eq!(composer.local_image_paths(), vec![second_path]);
|
||||
assert_eq!(composer.textarea.element_payloads(), vec![placeholder1]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn current_text_with_pending_expands_placeholders() {
|
||||
let (tx, _rx) = unbounded_channel::<AppEvent>();
|
||||
|
||||
@@ -8,25 +8,23 @@ use crate::mention_codec::decode_history_mentions;
|
||||
use codex_core::protocol::Op;
|
||||
use codex_protocol::user_input::TextElement;
|
||||
|
||||
/// A composer history entry that can rehydrate draft state.
|
||||
#[derive(Debug, Clone, PartialEq)]
|
||||
pub(crate) struct HistoryEntry {
|
||||
/// Raw text stored in history (may include placeholder strings).
|
||||
pub(crate) text: String,
|
||||
/// Text element ranges for placeholders inside `text`.
|
||||
pub(crate) text_elements: Vec<TextElement>,
|
||||
/// Local image paths captured alongside `text_elements`.
|
||||
pub(crate) local_image_paths: Vec<PathBuf>,
|
||||
/// Mention bindings for tool/app/skill references inside `text`.
|
||||
pub(crate) mention_bindings: Vec<MentionBinding>,
|
||||
/// Placeholder-to-payload pairs used to restore large paste content.
|
||||
pub(crate) pending_pastes: Vec<(String, String)>,
|
||||
}
|
||||
|
||||
impl HistoryEntry {
|
||||
fn empty() -> Self {
|
||||
Self {
|
||||
text: String::new(),
|
||||
text_elements: Vec::new(),
|
||||
local_image_paths: Vec::new(),
|
||||
mention_bindings: Vec::new(),
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn from_text(text: String) -> Self {
|
||||
pub(crate) fn new(text: String) -> Self {
|
||||
let decoded = decode_history_mentions(&text);
|
||||
Self {
|
||||
text: decoded.text,
|
||||
@@ -40,6 +38,23 @@ impl HistoryEntry {
|
||||
path: mention.path,
|
||||
})
|
||||
.collect(),
|
||||
pending_pastes: Vec::new(),
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
pub(crate) fn with_pending(
|
||||
text: String,
|
||||
text_elements: Vec<TextElement>,
|
||||
local_image_paths: Vec<PathBuf>,
|
||||
pending_pastes: Vec<(String, String)>,
|
||||
) -> Self {
|
||||
Self {
|
||||
text,
|
||||
text_elements,
|
||||
local_image_paths,
|
||||
mention_bindings: Vec::new(),
|
||||
pending_pastes,
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -55,9 +70,10 @@ pub(crate) struct ChatComposerHistory {
|
||||
history_entry_count: usize,
|
||||
|
||||
/// Messages submitted by the user *during this UI session* (newest at END).
|
||||
/// Local entries retain full draft state (text elements, image paths, pending pastes).
|
||||
local_history: Vec<HistoryEntry>,
|
||||
|
||||
/// Cache of persistent history entries fetched on-demand.
|
||||
/// Cache of persistent history entries fetched on-demand (text-only).
|
||||
fetched_history: HashMap<usize, HistoryEntry>,
|
||||
|
||||
/// Current cursor within the combined (persistent + local) history. `None`
|
||||
@@ -95,10 +111,14 @@ impl ChatComposerHistory {
|
||||
/// Record a message submitted by the user in the current session so it can
|
||||
/// be recalled later.
|
||||
pub fn record_local_submission(&mut self, entry: HistoryEntry) {
|
||||
if entry.text.is_empty() && entry.local_image_paths.is_empty() {
|
||||
if entry.text.is_empty()
|
||||
&& entry.text_elements.is_empty()
|
||||
&& entry.local_image_paths.is_empty()
|
||||
&& entry.mention_bindings.is_empty()
|
||||
&& entry.pending_pastes.is_empty()
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
self.history_cursor = None;
|
||||
self.last_history_text = None;
|
||||
|
||||
@@ -177,7 +197,7 @@ impl ChatComposerHistory {
|
||||
// Past newest – clear and exit browsing mode.
|
||||
self.history_cursor = None;
|
||||
self.last_history_text = None;
|
||||
Some(HistoryEntry::empty())
|
||||
Some(HistoryEntry::new(String::new()))
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -192,8 +212,7 @@ impl ChatComposerHistory {
|
||||
if self.history_log_id != Some(log_id) {
|
||||
return None;
|
||||
}
|
||||
let text = entry?;
|
||||
let entry = HistoryEntry::from_text(text);
|
||||
let entry = HistoryEntry::new(entry?);
|
||||
self.fetched_history.insert(offset, entry.clone());
|
||||
|
||||
if self.history_cursor == Some(offset as isize) {
|
||||
@@ -241,6 +260,7 @@ mod tests {
|
||||
use super::*;
|
||||
use crate::app_event::AppEvent;
|
||||
use codex_core::protocol::Op;
|
||||
use pretty_assertions::assert_eq;
|
||||
use tokio::sync::mpsc::unbounded_channel;
|
||||
|
||||
#[test]
|
||||
@@ -248,27 +268,27 @@ mod tests {
|
||||
let mut history = ChatComposerHistory::new();
|
||||
|
||||
// Empty submissions are ignored.
|
||||
history.record_local_submission(HistoryEntry::from_text(String::new()));
|
||||
history.record_local_submission(HistoryEntry::new(String::new()));
|
||||
assert_eq!(history.local_history.len(), 0);
|
||||
|
||||
// First entry is recorded.
|
||||
history.record_local_submission(HistoryEntry::from_text("hello".to_string()));
|
||||
history.record_local_submission(HistoryEntry::new("hello".to_string()));
|
||||
assert_eq!(history.local_history.len(), 1);
|
||||
assert_eq!(
|
||||
history.local_history.last().unwrap(),
|
||||
&HistoryEntry::from_text("hello".to_string())
|
||||
&HistoryEntry::new("hello".to_string())
|
||||
);
|
||||
|
||||
// Identical consecutive entry is skipped.
|
||||
history.record_local_submission(HistoryEntry::from_text("hello".to_string()));
|
||||
history.record_local_submission(HistoryEntry::new("hello".to_string()));
|
||||
assert_eq!(history.local_history.len(), 1);
|
||||
|
||||
// Different entry is recorded.
|
||||
history.record_local_submission(HistoryEntry::from_text("world".to_string()));
|
||||
history.record_local_submission(HistoryEntry::new("world".to_string()));
|
||||
assert_eq!(history.local_history.len(), 2);
|
||||
assert_eq!(
|
||||
history.local_history.last().unwrap(),
|
||||
&HistoryEntry::from_text("world".to_string())
|
||||
&HistoryEntry::new("world".to_string())
|
||||
);
|
||||
}
|
||||
|
||||
@@ -300,7 +320,7 @@ mod tests {
|
||||
|
||||
// Inject the async response.
|
||||
assert_eq!(
|
||||
Some(HistoryEntry::from_text("latest".to_string())),
|
||||
Some(HistoryEntry::new("latest".to_string())),
|
||||
history.on_entry_response(1, 2, Some("latest".into()))
|
||||
);
|
||||
|
||||
@@ -321,7 +341,7 @@ mod tests {
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
Some(HistoryEntry::from_text("older".to_string())),
|
||||
Some(HistoryEntry::new("older".to_string())),
|
||||
history.on_entry_response(1, 1, Some("older".into()))
|
||||
);
|
||||
}
|
||||
@@ -335,17 +355,17 @@ mod tests {
|
||||
history.set_metadata(1, 3);
|
||||
history
|
||||
.fetched_history
|
||||
.insert(1, HistoryEntry::from_text("command2".to_string()));
|
||||
.insert(1, HistoryEntry::new("command2".to_string()));
|
||||
history
|
||||
.fetched_history
|
||||
.insert(2, HistoryEntry::from_text("command3".to_string()));
|
||||
.insert(2, HistoryEntry::new("command3".to_string()));
|
||||
|
||||
assert_eq!(
|
||||
Some(HistoryEntry::from_text("command3".to_string())),
|
||||
Some(HistoryEntry::new("command3".to_string())),
|
||||
history.navigate_up(&tx)
|
||||
);
|
||||
assert_eq!(
|
||||
Some(HistoryEntry::from_text("command2".to_string())),
|
||||
Some(HistoryEntry::new("command2".to_string())),
|
||||
history.navigate_up(&tx)
|
||||
);
|
||||
|
||||
@@ -354,7 +374,7 @@ mod tests {
|
||||
assert!(history.last_history_text.is_none());
|
||||
|
||||
assert_eq!(
|
||||
Some(HistoryEntry::from_text("command3".to_string())),
|
||||
Some(HistoryEntry::new("command3".to_string())),
|
||||
history.navigate_up(&tx)
|
||||
);
|
||||
}
|
||||
|
||||
@@ -123,6 +123,51 @@ positional args, Enter auto-submits without calling `prepare_submission_text`. T
|
||||
- Prunes attachments based on expanded placeholders.
|
||||
- Clears pending pastes after a successful auto-submit.
|
||||
|
||||
## History navigation (Up/Down) and backtrack prefill
|
||||
|
||||
`ChatComposerHistory` merges two kinds of history:
|
||||
|
||||
- **Persistent history** (cross-session, fetched from core on demand): text-only.
|
||||
- **Local history** (this UI session): full draft state.
|
||||
|
||||
Local history entries capture:
|
||||
|
||||
- raw text (including placeholders),
|
||||
- `TextElement` ranges for placeholders,
|
||||
- local image paths,
|
||||
- pending large-paste payloads (for drafts).
|
||||
|
||||
Persistent history entries only restore text. They intentionally do **not** rehydrate attachments
|
||||
or pending paste payloads.
|
||||
|
||||
### Draft recovery (Ctrl+C)
|
||||
|
||||
Ctrl+C clears the composer but stashes the full draft state (text elements, image paths, and
|
||||
pending paste payloads) into local history. Pressing Up immediately restores that draft, including
|
||||
image placeholders and large-paste placeholders with their payloads.
|
||||
|
||||
### Submitted message recall
|
||||
|
||||
After a successful submission, the local history entry stores the submitted text and any element
|
||||
ranges and local image paths. Pending paste payloads are cleared during submission, so large-paste
|
||||
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.
|
||||
- Large-paste placeholders are not expected in recalled submitted history; the text is the
|
||||
expanded paste content.
|
||||
|
||||
### Backtrack prefill
|
||||
|
||||
Backtrack selections read `UserHistoryCell` data from the transcript. The composer prefill now
|
||||
reuses the selected message’s text elements and local image paths, so image placeholders and
|
||||
attachments rehydrate when rolling back to a prior user message.
|
||||
|
||||
### External editor edits
|
||||
|
||||
When the composer content is replaced from an external editor, the composer rebuilds text elements
|
||||
and keeps only attachments whose placeholders still appear in the new text. Image placeholders are
|
||||
then normalized to `[Image #1]..[Image #N]` to keep attachment mapping consistent after edits.
|
||||
|
||||
## Paste burst: concepts and assumptions
|
||||
|
||||
The burst detector is intentionally conservative: it only processes “plain” character input
|
||||
|
||||
Reference in New Issue
Block a user