-
Notifications
You must be signed in to change notification settings - Fork 6.1k
fix: move account struct to app-server-protocol and use camelCase #5829
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.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema, TS)] | ||
| #[serde(tag = "type", rename_all = "camelCase")] | ||
| #[ts(tag = "type")] | ||
| pub enum Account { | ||
| #[serde(rename = "apiKey")] | ||
| #[ts(rename = "apiKey")] | ||
| ApiKey { api_key: String }, | ||
| #[serde(rename = "chatgpt")] | ||
| #[ts(rename = "chatgpt")] | ||
| ChatGpt { | ||
| email: Option<String>, | ||
| plan_type: PlanType, |
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.
Use camelCase for Account payload fields
The new Account enum is intended to serialize as camelCase, but the fields api_key and plan_type remain in snake_case. #[serde(rename_all = "camelCase")] on the enum only affects variant names, so JSON and ts-rs output will still be { "type": "apiKey", "api_key": … } and { "type": "chatgpt", "plan_type": … }, which contradicts the stated goal of producing apiKey/planType. Any client updated to expect camelCase properties will fail to deserialize these messages. Rename the fields or add per-field/variant serde and ts renames to produce camelCase.
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.
@owenlin0 can you verify 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 is right, updated and added a quick test to verify
Makes sense to move this struct to
app-server-protocol/since we want to serialize as camelCase, but we don't for structs defined inprotocol/It was:
But we want: