Easily Selectable History#1672
Conversation
bolinfest
left a comment
bolinfest
left a comment
There was a problem hiding this comment.
Overall, this is really great!
Unsurprisingly, the implementation of insert_history_lines() is what gives me the most pause. I haven't played with these APIs/edge cases myself, so I assume all the complexity is necessary to make things work.
It would certainly be helpful to add unit tests in the near future to be sure that we are catching all these edge cases.
|
|
||
| /// Append immutable history lines above the inline viewport. Part of the | ||
| /// incremental migration to the hybrid scrollback model described in | ||
| /// `fix-history-plan.md`. |
There was a problem hiding this comment.
I think the reference to fix-history-plan.md is outdated now?
| token_usage: TokenUsage, | ||
| reasoning_buffer: String, | ||
| answer_buffer: String, | ||
| // Buffer for streaming assistant answer text; we do not surface partial |
There was a problem hiding this comment.
I feel like this comment should be "attached" to fn or struct?
There was a problem hiding this comment.
Note doc comments in Rust should have ///.
| if let Some(lines) = self.conversation_history.last_entry_plain_lines() { | ||
| self.app_event_tx.send(AppEvent::InsertHistory(lines)); | ||
| } |
There was a problem hiding this comment.
This seems to happen enough that it feels like it should be a helper function.
| | HistoryCell::PendingPatch { view } | ||
| | HistoryCell::ActiveExecCommand { view, .. } | ||
| | HistoryCell::ActiveMcpToolCall { view, .. } => view.lines.clone(), | ||
| HistoryCell::CompletedMcpToolCallWithImageOutput { .. } => vec![ |
There was a problem hiding this comment.
It's not the most high priority thing, but can we not support images anymore?
There was a problem hiding this comment.
We will support images, but not immediately (which I think is the same as the current UX?)
| @@ -0,0 +1,183 @@ | |||
| use crate::tui; // for the Tui type alias | |||
| pub fn run_main( | ||
| cli: Cli, | ||
| codex_linux_sandbox_exe: Option<PathBuf>, | ||
| ) -> std::io::Result<codex_core::protocol::TokenUsage> { |
There was a problem hiding this comment.
I think it might be helpful to introduce a struct like FinalOutput that has TokenUsage (and maybe final_message: Option<String>) as a field.
Then I would have it impl Display for FinalOutput with the implementation that corresponds to this code:
println!(
"Token usage: total={} input={}{} output={}{}",
usage.total_tokens,
usage.input_tokens,
usage
.cached_input_tokens
.map(|c| format!(" (cached {c})"))
.unwrap_or_default(),
usage.output_tokens,
usage
.reasoning_output_tokens
.map(|r| format!(" (reasoning {r})"))
.unwrap_or_default()
);
so that you can just do:
println!("{final_output}");
rather than copy/paste the two implementations.
| pub fn description(self) -> &'static str { | ||
| match self { | ||
| SlashCommand::New => "Start a new chat.", | ||
| SlashCommand::ToggleMouseMode => { |
There was a problem hiding this comment.
Nice! We can also remove disable_mouse_capture in the docs (config.md) and Config code.
| /// `ConversationHistory`. They may contain embedded newlines and arbitrary | ||
| /// runs of whitespace inside individual [`Span`]s. All of that must be | ||
| /// normalised before writing to the backing terminal buffer because the | ||
| /// ratatui [`Paragraph`] widget does not perform soft‑wrapping when used in |
There was a problem hiding this comment.
So even if we explicitly use the wrap() method of Paragraph, there is nothing we can do?
There was a problem hiding this comment.
I intend to refactor this code significantly to try to support live updates before committing a final version (which would allow us to bring back streaming/command outputs).
| let style = span.style; | ||
| let mut buf_word = String::new(); | ||
| let mut buf_space = String::new(); | ||
| let flush_word = |word: &mut String, |
There was a problem hiding this comment.
Can we pull out the appropriate arguments so this can be a top-level function instead of a closure? I think that would make things a bit easier to follow.
There was a problem hiding this comment.
Alternatively, maybe it would be easier to create a struct with some methods on it to manage this entire computation? Then things can be read off self instead of passed in?
There was a problem hiding this comment.
Struct makes sense to me!
bolinfest
left a comment
bolinfest
left a comment
There was a problem hiding this comment.
Just a few small suggestions: great work!
| use std::path::Path; | ||
| use std::path::PathBuf; | ||
| use std::str::FromStr; | ||
| use std::str::FromStr; // Added for FinalOutput Display implementation |
| pub total_tokens: u64, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Deserialize, Serialize)] |
There was a problem hiding this comment.
protocol.rs should only have things that are part of the protocol between the business logic and the UI layer. This seems more appropriate for codex-rs/common?
| token_usage: TokenUsage, | ||
| reasoning_buffer: String, | ||
| answer_buffer: String, | ||
| // Buffer for streaming assistant answer text; we do not surface partial |
There was a problem hiding this comment.
Note doc comments in Rust should have ///.
2 participants
This update replaces the previous ratatui history widget with an append-only log so that the terminal can handle text selection and scrolling. It also disables streaming responses, which we'll do our best to bring back in a later PR. It also adds a small summary of token use after the TUI exits.