-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add Z.ai coding plan support #8003
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
Adds support for Z.ai's coding plan subscription tiers: - International Coding Plan - China Coding Plan Pulls changes from downstream PR Kilo-Org/kilocode#2402
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.
Thank you for your contribution! I've reviewed the changes and found the implementation to be solid overall. The addition of Z.ai's coding plan subscription tiers is well-structured. I've left some suggestions inline to help improve the implementation.
packages/types/src/providers/zai.ts
Outdated
| @@ -1,4 +1,5 @@ | |||
| import type { ModelInfo } from "../model.js" | |||
| import { ZaiApiLine } from "../provider-settings.js" // kilocode_change | |||
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.
Could we remove these comments (lines 2, 108, 119)? They appear to be merge/tracking comments from the downstream PR that don't belong in the final code.
| export class ZAiHandler extends BaseOpenAiCompatibleProvider<InternationalZAiModelId | MainlandZAiModelId> { | ||
| constructor(options: ApiHandlerOptions) { | ||
| const isChina = options.zaiApiLine === "china" | ||
| const isChina = zaiApiLineConfigs[options.zaiApiLine ?? "international_coding"].isChina |
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.
Is the change in default from "international" to "international_coding" intentional? This is a breaking change for existing users who haven't specified a . Consider documenting this change clearly or maintaining backward compatibility by defaulting to "international" instead.
| }) | ||
|
|
||
| describe("Default behavior", () => { | ||
| it("should default to international when no zaiApiLine is specified", () => { |
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.
Would it be beneficial to add explicit test cases for the new "international_coding" and "china_coding" options? Currently, the tests only cover "international" and "china" options, but not the new coding-specific plans.
| const config = zaiApiLineConfigs[zaiApiLine] | ||
| return ( | ||
| <VSCodeOption key={zaiApiLine} value={zaiApiLine} className="p-2"> | ||
| {config.name} ({config.baseUrl}) |
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.
Consider showing just the name in the dropdown and moving the URL to the description/help text below for better UI clarity. The current format with both name and URL might be a bit verbose:
Adds support for Z.ai's coding plan subscription tiers (International and China).
Pulled from downstream PR Kilo-Org/kilocode#2402. Thanks @chrarnoldus.
Important
Add support for Z.ai's new coding plan subscription tiers, updating schemas, configurations, and tests to handle international and China options.
international_coding,china_coding) inprovider-settings.tsandzai.ts.ZAiHandlerinzai.tsto usezaiApiLineConfigsfor determining base URL and region.international_codingifzaiApiLineis not specified.zaiApiLineSchemaandzaiApiLineConfigsinprovider-settings.tsto manage Z.ai API lines.zaiApiLinehandling inZAi.tsxto reflect new subscription tiers.zai.spec.tsto cover new subscription tiers and default behavior.This description was created by
for 40d6155. You can customize this summary. It will automatically update as commits are pushed.