-
Notifications
You must be signed in to change notification settings - Fork 7.4k
Tui: use collaboration mode instead of model and effort #9507
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
Conversation
|
@codex review this |
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: 9f56b1be80
ℹ️ 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-rs/tui/src/chatwidget.rs
Outdated
| // Update stored collaboration mode with model and reasoning effort from session. | ||
| self.stored_collaboration_mode = CollaborationMode::Custom(Settings { | ||
| model: model_for_header.clone(), | ||
| reasoning_effort: event.reasoning_effort, | ||
| developer_instructions: None, |
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.
Preserve collaboration mode after session configured
When on_session_configured runs, it unconditionally replaces stored_collaboration_mode with CollaborationMode::Custom and clears developer_instructions. If the Collaboration Modes feature is enabled, this wipes the default/selected Plan/Pair/Execute variant as soon as the session is configured, so subsequent Op::UserTurn submissions carry Custom and no collaboration instructions until the user manually reselects a mode. This means the default collaboration mode never actually applies in normal flows (session configured → user sends first message). Consider preserving the existing variant and only updating model/effort (e.g., via with_updates) instead of forcing Custom.
Useful? React with 👍 / 👎.
|
@codex review this |
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: dd86ec0fb2
ℹ️ 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 |
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: c60c0ece11
ℹ️ 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 |
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: 6d089fd938
ℹ️ 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 |
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: 0f913bf94a
ℹ️ 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 |
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: 334824f9e2
ℹ️ 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".
| AppEvent::UpdateCollaborationMode(mode) => { | ||
| self.chat_widget.set_collaboration_mode(mode); |
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.
Update current_model when collaboration mode changes
When collaboration modes are enabled, selecting a preset can change the effective model stored in the widget (the mode carries its own model/effort), but the UpdateCollaborationMode handler only updates the widget and leaves current_model untouched. current_model is later used to seed new sessions/forks/resumes, so after switching to a mode that uses a different model, starting a new session will still use the previous model. Consider syncing current_model (and any related in-memory config) to mode.model() when handling UpdateCollaborationMode so session restarts reflect the user’s selected collaboration mode.
Useful? React with 👍 / 👎.
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 fix this by removing current model and use collaboration modes mainly for getting current model
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.
Summary
- Removed App-level
current_modeltracking and now seed new/resumed sessions from the ChatWidget’s collaboration-mode-backed model value. codex-rs/tui/src/app.rsL320-L725 - Exposed
current_modelonChatWidgetso the app can read the collaboration-mode model directly. codex-rs/tui/src/chatwidget.rsL4242-L4260
Testing
- ✅
just fmt - ❌
just fix -p codex-tui(fails:tui2/src/status/card.rsuses private modulesandbox_summary) - ❌
cargo test -p codex-tui(fails:tui2/src/status/card.rsuses private modulesandbox_summary)
|
@codex review this |
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: 2b363d8bab
ℹ️ 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".
| config, | ||
| model, | ||
| collaboration_mode: CollaborationModeSelection::default(), | ||
| stored_collaboration_mode, | ||
| auth_manager, | ||
| models_manager, | ||
| session_header: SessionHeader::new(model_for_header), |
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.
Keep header model in sync with collab default
When Feature::CollaborationModes is enabled, stored_collaboration_mode is initialized from the default preset (currently hard‑coded to gpt-5.2-codex), but the session header is still constructed from model_for_header (config/session model). If those differ (e.g., config still points at gpt‑5.1), the UI will show the old model while Op::UserTurn is sent with the preset model, so users are misled about which model they’re actually using. Consider deriving the header model from stored_collaboration_mode.model() or calling set_model after selecting the default mode.
Useful? React with 👍 / 👎.
|
@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: b8eed88685
ℹ️ 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".
| fn on_update_reasoning_effort(&mut self, effort: Option<ReasoningEffortConfig>) { | ||
| self.chat_widget.set_reasoning_effort(effort); |
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.
Keep config effort in sync for new sessions
The new on_update_reasoning_effort only updates the widget and no longer updates self.config.model_reasoning_effort. As a result, when a user changes reasoning effort (via /model or the popup) and then starts a new session in the same run, AppEvent::NewSession builds the new ChatWidgetInit from self.config.clone(), so the session configuration reverts to the old effort. This is a regression in the non-collaboration flow because the selected effort no longer carries over across /new without restarting. Consider updating the in-memory config here or explicitly passing the stored collaboration mode’s effort into new sessions.
Useful? React with 👍 / 👎.
|
@codex review this |
1 similar comment
|
@codex review this |
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: 8bd8302753
ℹ️ 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 |
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: ded72b61d6
ℹ️ 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".
| let model = model.filter(|m| !m.trim().is_empty()); | ||
| config.model = model.clone(); | ||
| let config = config; | ||
| let mut rng = rand::rng(); | ||
| let placeholder = PLACEHOLDERS[rng.random_range(0..PLACEHOLDERS.len())].to_string(); | ||
| let codex_op_tx = spawn_agent(config.clone(), app_event_tx.clone(), thread_manager); |
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.
New sessions start with stale model from config
In ChatWidget::new, the selected model from ChatWidgetInit is no longer copied into config before calling spawn_agent. spawn_agent passes this config into ThreadManager::start_thread, which derives the session model from config.model. Because AppEvent::UpdateModel now only updates the widget (not self.config), starting a new session after /model will spin up the thread with the old model while the UI shows the new one. Consider restoring config.model = model.clone() (or otherwise wiring the selected model into the Config passed to core).
Useful? React with 👍 / 👎.
| let model = model.filter(|m| !m.trim().is_empty()); | ||
| config.model = model.clone(); | ||
| let config = config; | ||
| let mut rng = rand::rng(); | ||
| let placeholder = PLACEHOLDERS[rng.random_range(0..PLACEHOLDERS.len())].to_string(); | ||
| let codex_op_tx = spawn_agent(config.clone(), app_event_tx.clone(), thread_manager); |
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.
New sessions start with stale model from config
Same issue in the TUI2 implementation: the selected model from ChatWidgetInit is not copied into config before spawn_agent, so start_thread still uses the old config.model. After a /model change, creating a new session will silently use the previous model while the UI reflects the updated selection. The config passed to core needs to be updated with the selected model.
Useful? React with 👍 / 👎.
|
@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: b773f91130
ℹ️ 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".
| // Only update stored collaboration settings when collaboration modes are disabled. | ||
| // When enabled, we preserve the selected variant (Plan/Pair/Execute/Custom) and its | ||
| // instructions as-is; the session configured event should not override it. | ||
| if !self.collaboration_modes_enabled() { | ||
| self.stored_collaboration_mode = self.stored_collaboration_mode.with_updates( |
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.
Keep collaboration mode in sync with SessionConfigured
When collaboration modes are enabled, this handler skips updating stored_collaboration_mode from the SessionConfiguredEvent. If the configured session model/effort differs from the stored preset (e.g., after resuming/forking a session or a server-side model migration), the TUI will keep the stale mode and the next Op::UserTurn will send the wrong model/effort, silently overriding the session configuration. Consider updating the stored collaboration mode with event.model/event.reasoning_effort while preserving the current variant/instructions so the TUI stays consistent with the configured session.
Useful? React with 👍 / 👎.
Uh oh!
There was an error while loading. Please reload this page.