app-server: Add egress websocket transport#15951
Conversation
| tokio::spawn(async move { | ||
| let server_envelope = QueuedServerEnvelope { | ||
| event: ServerEvent::Pong { | ||
| status: PongStatus::Unknown, |
There was a problem hiding this comment.
PongStatus::Active?
There was a problem hiding this comment.
this is unhappy path if app server doesn't know about the client
…everse-4 # Conflicts: # codex-rs/features/src/lib.rs
…everse-4 # Conflicts: # codex-rs/app-server/src/in_process.rs # codex-rs/app-server/src/lib.rs # codex-rs/app-server/src/message_processor.rs # codex-rs/app-server/src/message_processor/tracing_tests.rs
owenlin0
left a comment
owenlin0
left a comment
There was a problem hiding this comment.
i noticed migration was renamed: codex-rs/state/migrations/0024_drop_logs.sql, why is that? shouldn't remote_control_enrollments.sql be 0024?
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub(crate) struct ClientEnvelope { | ||
| #[serde(flatten)] |
There was a problem hiding this comment.
I'm generally not a fan of #[serde(flatten)], do we need it?
There was a problem hiding this comment.
options are either have flatten here or duplicate client_id/stream_id/seq_id/cursor across all branches of the enum
do you strongly prefer one over another?
There was a problem hiding this comment.
don't feel strongly, up to you
oh, yes, I have fix for this locally but somehow didn't push, let me fix that |
| &self, | ||
| websocket_url: &str, | ||
| account_id: Option<&str>, | ||
| ) -> anyhow::Result<Option<(String, String, String)>> { |
There was a problem hiding this comment.
Should we define a type like RemoteControlEnrollment, similar to the other sqlite tables we have? Option<(String, String, String>) looks awkward
| @@ -0,0 +1,219 @@ | |||
| use super::*; | |||
|
|
|||
| const REMOTE_CONTROL_ACCOUNT_ID_NONE: &str = ""; | |||
There was a problem hiding this comment.
should we have account_id actually be nullable in the DB? seems weird to deal with ""
There was a problem hiding this comment.
it looks like in sqlite NULL values are not equal to other NULLs, so can not be part of unique constraint/primary key..
4 participants
No description provided.