-
Notifications
You must be signed in to change notification settings - Fork 7.2k
fix(tui): only show 'Worked for' separator when actual work was performed #8958
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
base: main
Are you sure you want to change the base?
fix(tui): only show 'Worked for' separator when actual work was performed #8958
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93baa9bf16
ℹ️ 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".
| if self.stream_controller.is_none() { | ||
| if self.needs_final_message_separator { | ||
| if self.needs_final_message_separator && self.had_work_activity { | ||
| let elapsed_seconds = self | ||
| .bottom_pane | ||
| .status_widget() | ||
| .map(super::status_indicator_widget::StatusIndicatorWidget::elapsed_seconds); | ||
| self.add_to_history(history_cell::FinalMessageSeparator::new(elapsed_seconds)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Count view-image tool calls as work for separator
The new guard only shows the separator when had_work_activity is true, but that flag is set only in exec/patch/MCP end handlers. A turn that only uses the view‑image tool (on_view_image_tool_call adds a history cell but never flips had_work_activity) will now skip the “Worked for” separator even though a tool call occurred. If view‑image calls are meant to represent actual work, this is a regression; consider setting had_work_activity in that handler or broadening the check.
Useful? React with 👍 / 👎.
|
Thanks for the review feedback! Here's my analysis: Regarding view-image tool callsThe
The current behavior is correct: separator only shows when |
Fixes #7919.
This PR addresses a TUI display bug where the "Worked for" separator would appear prematurely during the planning stage.
Changes:
had_work_activityflag toChatWidgetto track if actual work (exec commands, MCP tool calls, patches) was performed in the current turn.handle_streaming_deltato only display theFinalMessageSeparatorif bothneeds_final_message_separatorANDhad_work_activityare true.handle_exec_end_now,handle_patch_apply_end_now, andhandle_mcp_end_nowto sethad_work_activity = true.Verification:
cargo test -p codex-tuito ensure no regressions.