Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b19cc16f33
ℹ️ 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".
apanasenko-oai
force-pushed
the
anton_panasenko_fork
branch
from
b19cc16 to
3bded88
Compare
apanasenko-oai
force-pushed
the
anton_panasenko_fork
branch
from
40d306d to
9ada6f1
Compare
jif-oai
left a comment
jif-oai
left a comment
There was a problem hiding this comment.
I would definitely drop v1
Could you also pipe this in the CLI?
|
|
||
| #[derive(Serialize, Deserialize, Debug, Clone, JsonSchema, TS)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct ForkConversationResponse { |
There was a problem hiding this comment.
I don't think it is necessary to add it to v1 tbh
There was a problem hiding this comment.
agreed, we plan to delete v1 as soon as cloud is migrated to v2
|
|
||
| /// [UNSTABLE] Specify the rollout path to fork from. | ||
| /// If specified, the thread_id param will be ignored. | ||
| pub path: Option<PathBuf>, |
There was a problem hiding this comment.
It would be cleaner to have something like:
pub thread_reference: ThreadReference
...
enum ThreadReference {
ThreadId(String),
ThreadPath(PathBuf)
}
Then you can just make the thread_reference as untagged and you should have something clean
There was a problem hiding this comment.
I did it this way originally for thread/resume because I wanted to be able to deprecate the path option and have the client work solely using IDs
| } | ||
| } | ||
|
|
||
| async fn handle_fork_conversation( |
There was a problem hiding this comment.
+1 to drop v1. It does not make sense to duplicate all this code
owenlin0
left a comment
owenlin0
left a comment
There was a problem hiding this comment.
approving for v2 api changes. thanks for updating the readme too!
we still using v1 in web, until we drop it from web, I would like to keep them on par.
will follow up in separate review with tui changes |
4 participants
Summary
thread/forkusage