Skip to content

Add /compact #1724

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.

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add /compact #1724

wants to merge 7 commits into from

Conversation

easong-openai
Copy link
Contributor

Adds an initial /compact command that summarizes the existing conversation and also introduces scrolling to the slash command modal. There's probably more work to do on the prompt.

Copy link

Summary

Makes several codex-rs core modules public and overhauls the TUI: adds scroll-aware slash-command popup, new widgets/event types, related tests, and minor model/client tweaks.

Review

Nice functional upgrade and tidy Rust implementation! A few quick suggestions:

  • Exporting client, client_common, models, and ResponseStream widens the public API — consider a re-export module or #[doc(hidden)] guards to avoid an unnecessary semver bump.
  • visible_rows: Cell<usize> is mutated only in render; a plain usize returned from the calc would avoid interior mutability.
  • Please run just fmt / cargo fmt to fix a handful of rustfmt nits (long lines in command_popup.rs).
  • Add a short note to CHANGELOG and TUI help about the new scrolling behaviour.

Other than those minor points, looks solid and the added tests are great.


View workflow run

Comment on lines +329 to +333
let appended = match cfg.base_instructions.take() {
Some(existing) => format!("{existing}\n\n{prefix}\n{summary}\n"),
None => format!("{prefix}\n{summary}\n"),
};
cfg.base_instructions = Some(appended);
Copy link
Collaborator

Choose a reason for hiding this comment

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

won't this result in repeatedly appending to the base instructions if using /compact more than once?

@bolinfest
Copy link
Collaborator

Adding @pakrym-oai and @ibahmed-oai as reviewers because there was already a lot of discussion on a related PR: #1529

@aibrahim-oai
Copy link
Collaborator

aibrahim-oai commented Jul 30, 2025

I talked about this with @easong-openai . This approach is building on top of #1495 and #1497 . We thought that this approach isn't the best because we are calling the model in tui. Also, this will make it harder to use /compact in the mcp because we will have to build the same thing again.

As @bolinfest mentioned, we had good iterations on #1527 and #1529 . I think this is a better appraoch because the logic goes to the /core. Admittedly, the approach makes the summary appear in the tui which may not be the best UX but I think you can go around that in the tui implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants