Persist text element ranges and attached images across history/resume#9116
Persist text element ranges and attached images across history/resume#9116charley-oai merged 2 commits intomainfrom
Conversation
5f1939c to
1affb03
Compare
b04477d to
93d0571
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds support for persisting text element ranges and attached images across history, resume, and backtracking operations. Text elements are byte-span metadata that enable the UI to highlight special content (e.g., image placeholders) in user messages without modifying the underlying text. The changes ensure that when users edit history or backtrack, the UI can restore both the original text and its associated visual markers, including rehydrating local image attachments.
Changes:
- Added
text_elementsandlocal_imagesfields to user input/message events with backward-compatible serialization defaults - Threaded text elements through protocol, app-server, core, and TUI layers to preserve UI metadata across history operations
- Refactored backtracking flow to be asynchronous, requesting conversation history before forking threads and properly rehydrating composer state
Reviewed changes
Copilot reviewed 89 out of 89 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
codex-rs/protocol/src/user_input.rs |
Added TextElement and ByteRange types to represent UI spans within text |
codex-rs/protocol/src/protocol.rs |
Added local_images and text_elements fields to UserMessageEvent |
codex-rs/protocol/src/items.rs |
Added methods to extract and rebase text elements from user message content |
codex-rs/tui2/src/bottom_pane/textarea.rs |
Added support for setting/getting text with element ranges |
codex-rs/tui2/src/bottom_pane/chat_composer.rs |
Enhanced submission logic to capture, trim, and preserve text elements through the input pipeline |
codex-rs/tui2/src/history_cell.rs |
Added rendering logic to display text elements with cyan styling in history |
codex-rs/tui2/src/app_backtrack.rs |
Refactored backtrack to async two-step flow with conversation history request and proper error handling |
codex-rs/tui/src/* |
Mirror of TUI2 changes for the original TUI implementation |
codex-rs/core/src/codex.rs |
Modified user turn handling to emit events from UserInput to preserve UI metadata |
codex-rs/app-server/* |
Propagated text elements through v1/v2 API types and history rebuilding |
| Test files | Updated tests to include empty text_elements and local_images vectors |
Comments suppressed due to low confidence (2)
codex-rs/tui2/src/bottom_pane/chat_composer.rs:1
- The Enter key handling now queues messages when
steer_enabledis false, which changes the previous behavior where Enter always submitted immediately. This logic should match line 1046 in the test file where Tab was changed to Enter, suggesting the original intent was for Enter to always submit. Theshould_queuelogic appears inverted or the comment at the old line 1615 ("Enter always sends messages immediately") is now incorrect.
codex-rs/tui2/src/app_backtrack.rs:1 - The struct fields
text,text_elements, andlocal_image_pathsinBacktrackPrefillshould be private (notpub) since the struct is markedpub(crate). Public fields on a crate-internal struct break encapsulation and may allow incorrect mutations.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 89 out of 89 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2cb95a9 to
2a47905
Compare
Initial type tweaking PR to make the diff of #9116 smaller This should not change any behavior, just adds some fields to types
d01cf2b to
f098faf
Compare
|
@codex review this |
aibrahim-oai
left a comment
There was a problem hiding this comment.
I feel the PR is still pretty big and has a lot of moving parts.
I can see it broken down to support in app-server, core, and maybe two-stacked for tui one for general support of the text element and one for the backtracking changes
That way it be easier to track those moving parts and remove unnecessary changes. Also, will enable us to think carefully about how to test it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 600f1a15bf
ℹ️ 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".
The second part of breaking up PR #9116 Summary: - Add `TextElement` / `ByteRange` to protocol user inputs and user message events with defaults. - Thread `text_elements` through app-server v1/v2 request handling and history rebuild. - Preserve UI metadata only in user input/events (not `ContentItem`) while keeping local image attachments in user events for rehydration. Details: - Protocol: `UserInput::Text` carries `text_elements`; `UserMessageEvent` carries `text_elements` + `local_images`. Serialization includes empty vectors for backward compatibility. - app-server-protocol: v1 defines `V1TextElement` / `V1ByteRange` in camelCase with conversions; v2 uses its own camelCase wrapper. - app-server: v1/v2 input mapping includes `text_elements`; thread history rebuilds include them. - Core: user event emission preserves UI metadata while model history stays clean; history replay round-trips the metadata.
28350b4 to
7625377
Compare
a641172 to
a23e6d6
Compare
1329889 to
f96ab1f
Compare
Continuation of breaking up this PR #9116 ## Summary - Thread user text element ranges through TUI/TUI2 input, submission, queueing, and history so placeholders survive resume/edit flows. - Preserve local image attachments alongside text elements and rehydrate placeholders when restoring drafts. - Keep model-facing content shapes clean by attaching UI metadata only to user input/events (no API content changes). ## Key Changes - TUI/TUI2 composer now captures text element ranges, trims them with text edits, and restores them when submission is suppressed. - User history cells render styled spans for text elements and keep local image paths for future rehydration. - Initial chat widget bootstraps accept empty `initial_text_elements` to keep initialization uniform. - Protocol/core helpers updated to tolerate the new InputText field shape without changing payloads sent to the API.
d1044cf to
5e35681
Compare
58deaa1 to
4808391
Compare
|
@codex review this |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ 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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4808391ff1
ℹ️ 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 this |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ 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". |
| } | ||
|
|
||
| // Keep image placeholders normalized to [Image #1].. in attachment order. | ||
| self.relabel_attached_images_and_update_placeholders(); |
There was a problem hiding this comment.
Codex pointed out that the external edits path didn't relabel attached images, which could break the image labeling assumptions made by this PR
b3aa0f2 to
aebb2f3
Compare
|
@codex review this |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ 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". |
Summary
text_elementsandlocal_image_pathsfrom the chosen user history cell so Esc‑Esc history edits preserve image placeholders and attachments.tuiandtui2.Changes
tui/src/app_backtrack.rs: Backtrack selection now carries text elements + local image paths; composer prefill uses them (removes TODO).tui2/src/app_backtrack.rs: Same as above.tui/src/app.rs: Updated backtrack test to assert restored elements/paths.tui2/src/app.rs: Same test updates.The original scope of this PR (threading text elements and image attachments through the codex harness thoroughly/persistently) was broken into the following PRs other than this one:
The diff of this PR was reduced by changing types in a starter PR: #9235
Then text element metadata was added to protocol, app server, and core in this PR: #9331
Then the end-to-end flow was completed by wiring TUI/TUI2 input, history, and restore behavior in #9393
Prompt expansion was supported in this PR: #9518
TextElement optional placeholder field was protected in #9545