-
Notifications
You must be signed in to change notification settings - Fork 7.4k
feat: /fork the current session instead of opening session picker #9385
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
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: 0f9c682970
ℹ️ 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 let Some(path) = self.chat_widget.rollout_path() { | ||
| match self | ||
| .server | ||
| .fork_thread(usize::MAX, self.config.clone(), path.clone()) | ||
| .await |
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.
Flush rollout before forking active session
This now forks the current session by reading the rollout file via ThreadManager::fork_thread, but there’s no flush of the recorder beforehand. RolloutRecorder::record_items only enqueues writes and the writer is async, so if a user runs /fork right after a response (or during streaming) the rollout file can lag behind and the forked thread will miss the latest items. Consider flushing the session’s rollout before invoking fork_thread, or fork from the in‑memory history to avoid a stale snapshot.
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.
maybe we need to do that but in practice this is super fast
| if let Some(path) = self.chat_widget.rollout_path() { | ||
| match self | ||
| .server | ||
| .fork_thread(usize::MAX, self.config.clone(), path.clone()) | ||
| .await |
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.
Flush rollout before forking active session
Same issue in the tui2 flow: forking the current session reads the rollout file without ensuring pending writes are flushed, so /fork immediately after a response can produce a fork that is missing the latest items. A flush (or in‑memory history clone) should happen before fork_thread to make the forked snapshot consistent.
Useful? React with 👍 / 👎.
Implemented /fork to fork the current session directly (no picker), handling it via a new ForkCurrentSession app event in both tui and tui2. Updated slash command descriptions/tooltips and adjusted the fork tests accordingly. Removed the unused in-session fork picker event.