Skip to content

Bug fix: reliably detect UserMessage boundaries via typed spans #2879

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

Bug fix: reliably detect UserMessage boundaries via typed spans #2879

wants to merge 6 commits into from

Conversation

warpdev
Copy link
Contributor

@warpdev warpdev commented Aug 29, 2025 •
edited
Loading

Fixes #2755
Replaces #2757 to fix root cause.

Summary

  • Replace fragile string-based detection of user messages with typed metadata and span indices for backtracking.
  • Scope remains “User”-only typing; all other cells stay “Other”.
  • Backtracking logic is extracted and simplified with a consistent Esc/Enter flow across main and overlay views.

Root cause

Backtrack relied on rendered text parsing (“user” header join/trim), which loses type info and misidentifies boundaries.

Fix

  • Add MessageKind (User|Other) and MessageSpan to HistoryCell (default keeps existing cells unchanged).
  • Introduce UserHistoryCell and return it from new_user_prompt (transcript rendering unchanged).
  • Replace transcript_lines with transcript_cells in App.
  • Compute flattened transcript and absolute user spans on demand via build_transcript_flattened().

Before

2025-08-29.2.07.37.mov

After

2025-08-29.2.06.51.mov

@warpdev warpdev force-pushed the fix/user-message-span branch 5 times, most recently from 9026a37 to 25c14d1 Compare September 3, 2025 03:44
@warpdev warpdev changed the title fix(tui): reliably detect UserMessage boundaries via typed spans Bug fix: reliably detect UserMessage boundaries via typed spans Sep 3, 2025
@warpdev
Copy link
Contributor Author

warpdev commented Sep 3, 2025

Update: Here’s the result video after the TUI rework(#2877).

Before

2025-09-03.12.40.25.mov

After

2025-09-03.12.39.21.mov

@warpdev warpdev force-pushed the fix/user-message-span branch from 25c14d1 to 57370c7 Compare September 3, 2025 12:17
@warpdev
Copy link
Contributor Author

warpdev commented Sep 4, 2025

Hi — This PR fixes a bug where Resume may select the wrong user message. When you have a moment, could you please take a look so others don’t run into it? Thank you! @nornagon-openai @easong-openai @bolinfest

nornagon-openai
nornagon-openai reviewed Sep 4, 2025
View reviewed changes
codex-rs/tui/src/app.rs Outdated
Comment on lines 41 to 43
pub(crate) transcript_lines: Vec<Line<'static>>,
// Absolute [header..end) ranges for user messages within `transcript_lines`.
pub(crate) user_spans: Vec<(usize, usize)>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we try a version of this where instead of transcript_lines we store transcript_cells, and compute transcript_lines when we trigger transcript mode? then the transcript / backtrack view knows which cell is which and we can encapsulate this logic inside the backtrack view.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I'll start refactoring based on your suggestion.

@warpdev warpdev force-pushed the fix/user-message-span branch from 57370c7 to d981d44 Compare September 5, 2025 05:24
@warpdev
Copy link
Contributor Author

warpdev commented Sep 5, 2025 •
edited
Loading

@nornagon-openai I’ve pushed an update; could you please take another look?

As a next step, we could switch Overlay and Transcript to be cell‑based (instead of line‑based). That would help decouple a few utils and the logic between tui/src/app.rs and Overlay, but that feels out of scope for this PR—I'd suggest a follow‑up refactor. What do you think?

Thank you!

Update: Having spent additional time understanding the Overlay and Transcript code, I believe a follow-up refactor to make them cell-based is unnecessary.

@warpdev warpdev force-pushed the fix/user-message-span branch 2 times, most recently from 8447534 to ab1f588 Compare September 8, 2025 17:58
@warpdev warpdev force-pushed the fix/user-message-span branch from ab1f588 to 9e2fcb7 Compare September 10, 2025 17:54
@warpdev
Copy link
Contributor Author

warpdev commented Sep 10, 2025

Hi @nornagon-openai — could you please take another look when you have a moment?
I implemented the suggested transcript_cells approach and updated the PR description. Thanks!

@nornagon-openai nornagon-openai mentioned this pull request Sep 12, 2025
Merged
nornagon-openai added a commit that referenced this pull request Sep 14, 2025
@nornagon-openai
refactor transcript view to handle HistoryCells (#3538)
4891ee2 
No (intended) functional change.

This refactors the transcript view to hold a list of HistoryCells
instead of a list of Lines. This simplifies and makes much of the logic
more robust, as well as laying the groundwork for future changes, e.g.
live-updating history cells in the transcript.

Similar to #2879 in goal. Fixes #2755.
@nornagon-openai
Copy link
Collaborator

Closing this as #3538 supersedes it.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 15, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@easong-openai easong-openai Awaiting requested review from easong-openai

@nornagon-openai nornagon-openai Awaiting requested review from nornagon-openai

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Message edit mixes last user message with "stream error … retrying…" text
2 participants
@warpdev @nornagon-openai