feat(tui): restore composer history in app-server tui#14945
feat(tui): restore composer history in app-server tui#14945etraut-openai merged 1 commit intoopenai:mainfrom
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a36ffd6dd2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Restores cross-session composer history support in the app-server TUI by handling history reads/writes locally against $CODEX_HOME/history.jsonl (matching the legacy TUI), including mention encoding/decoding for durable recall.
Changes:
- Persist submitted composer text to the shared history file (with mention placeholder encoding) via
Op::AddToHistory. - Replace the previous “stub” Up/Down behavior with async history lookup requests (
Op::GetHistoryEntryRequest) and local handling inApp. - Expose
codex_core::message_historypublicly and usehistory_metadata()during session bootstrap to populatehistory_log_id/history_entry_count.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
codex-rs/tui_app_server/src/chatwidget.rs |
Encodes mentions and submits AddToHistory on user submit. |
codex-rs/tui_app_server/src/bottom_pane/chat_composer_history.rs |
Emits real history lookup ops instead of inserting a stub error cell; updates tests accordingly. |
codex-rs/tui_app_server/src/app_server_session.rs |
Makes session bootstrap mapping async to fetch local history_metadata() for history navigation. |
codex-rs/tui_app_server/src/app.rs |
Intercepts AddToHistory / GetHistoryEntryRequest and handles them locally (append + spawn_blocking lookup). |
codex-rs/core/src/message_history.rs |
Widens visibility of history helpers to pub and updates docs. |
codex-rs/core/src/lib.rs |
Exports message_history module publicly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Move app-server TUI `Op::AddToHistory` persistence onto a background task so local history writes no longer block the app event loop while waiting on file locks. This matches the existing `codex-core` behavior more closely and avoids UI stalls under multi-session use, which was called out in PR feedback on `try_handle_local_history_op`.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0abe5ae12
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5677ee8242
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Move app-server TUI `Op::AddToHistory` persistence onto a background task so local history writes no longer block the app event loop while waiting on file locks. This matches the existing `codex-core` behavior more closely and avoids UI stalls under multi-session use, which was called out in PR feedback on `try_handle_local_history_op`.
fcoury
force-pushed
the
fcoury/fix/composer-history
branch
3 times, most recently
from
ab20e38 to
9e99840
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e99840090
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
fcoury
force-pushed
the
fcoury/fix/composer-history
branch
from
9e99840 to
7607f68
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Restore cross-session composer history in `tui_app_server` without changing the app-server protocol. The app-server TUI now fetches and persists composer history locally through `codex_core::message_history`, populates real history metadata during session bootstrap, and routes lookup responses back to the requesting thread. This rebuilds the feature directly on top of `upstream/main` while keeping the upstream app-server restore path intact. The branch no longer rewrites startup replay around `initial_messages`, so the PR is focused on the composer-history behavior only.
fcoury
force-pushed
the
fcoury/fix/composer-history
branch
from
7607f68 to
4d02185
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Problem
The app-server TUI (
tui_app_server) lacked composer history support. Pressing Up/Down to recall previous prompts hit a stub that logged a warning and displayed "Not available in app-server TUI yet." New submissions were silently dropped from the shared history file, so nothing persisted for future sessions.Mental model
Codex maintains a single, append-only history file (
$CODEX_HOME/history.jsonl) shared across all TUI processes on the same machine. The legacy (in-process) TUI already reads/writes this file throughcodex_core::message_history. The app-server TUI delegates most operations to a separate process over RPC, but history is intentionally not an RPC concern — it's a client-local file.This PR makes the app-server TUI access the same history file directly, bypassing the app-server process entirely. The composer's Up/Down navigation and submit-time persistence now follow the same code paths as the legacy TUI, with the only difference being where the call is dispatched (locally in
App, rather than insideCodexThread).The branch is rebuilt directly on top of
upstream/main, so it keeps theexisting app-server restore architecture intact.
AppServerStartedThreadstill restores transcript history from the server
Threadsnapshot viathread_snapshot_events; this PR only adds composer-history support.Non-goals
history.jsonl.Tradeoffs
message_historyfrompub(crate)topub590cfa617exposedmention_syntaxfor TUI consumption,752402c4fexposed plugin APIs (PluginsManager), and14fcb6645/edacbf7b6widened internal core APIs for other crates. These were all narrow, intentional exposures of specific APIs — not broad "make internals public" moves.1af2a37adeven went the other direction, reducing broad re-exports to tighten boundaries. This change follows the same pattern: a small, deliberate API surface (3 functions) rather than a wholesale visibility change.AddToHistory/GetHistoryEntryRequestinAppbefore RPC fallbacksubmit_thread_opentry point, which is safer than the original duplicated dispatch. The remaining risk is organizational: future thread-op submission paths need to keep using that shared entry point.session_configured_from_thread_responseis nowasyncawaitonhistory_metadata()to populate realhistory_log_id/history_entry_count.history.max_bytesand matches the legacy TUI's cost profile, but startup latency still scales with file size.Architecture
Observability
tracing::warnonappend_entryfailure (includes thread ID).tracing::warnonspawn_blockinglookup join error.tracing::warnfrommessage_historyinternals on file-open, lock, or parse failures.Tests
chat_composer_history::tests::navigation_with_async_fetch— verifies that Up emitsOp::GetHistoryEntryRequest(was: checked for stub error cell).app::tests::history_lookup_response_is_routed_to_requesting_thread— verifies multi-thread composer recall routes the lookup result back to the originating thread.app_server_session::tests::resume_response_relies_on_snapshot_replay_not_initial_messages— verifies app-server session restore still uses the upstream thread-snapshot path.app_server_session::tests::session_configured_populates_history_metadata— verifies bootstrap sets nonzerohistory_log_id/history_entry_countfrom the shared local history file.