Skip to content

Conversation

hannesrudolph
Copy link
Collaborator

@hannesrudolph hannesrudolph commented Sep 18, 2025

BONUS: fix(scripts): handle dots in JSON keys for translation validation

Fixed getValueAtPath function to properly handle JSON keys containing
dots (e.g., "glm-4.5") instead of incorrectly splitting them as separate
path segments. This eliminates false positives in translation checking.


Important

Consolidates Z AI entrypoints to two main options, updates configurations, tests, and UI, and removes legacy enum values.

  • Behavior:
    • Consolidate Z AI entrypoints to two options: International (https://api.z.ai/api/coding/paas/v4) and China (https://open.bigmodel.cn/api/coding/paas/v4).
    • Remove legacy international_coding and china_coding enum values, default to international.
    • Update provider config, handler defaults, and settings UI dropdown/link.
    • Update tests in zai.spec.ts to assert new coding endpoints.
  • Files:
    • provider-settings.ts: Update zaiApiLineSchema to remove legacy values.
    • zai.ts: Adjust zaiApiLineConfigs to reflect new endpoints.
    • zai.spec.ts: Modify tests to use updated endpoints.
  • Misc:
    • Built artifacts still reference old strings, to be updated during build/release.

This description was created by Ellipsis for 023ea9a. You can customize this summary. It will automatically update as commits are pushed.

…) and update tests/UI; default to 'international'; update endpoints to /api/coding/paas/v4
@Copilot Copilot AI review requested due to automatic review settings September 18, 2025 19:40
@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Sep 18, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR consolidates Z AI API endpoints to only two regional options - International and China - removing the legacy standard vs coding plan distinction. The changes update the provider configuration to use the coding-specific endpoints for both regions as the default.

  • Simplified Z AI entrypoint options from four to two (international and china)
  • Updated all endpoints to use the /api/coding/paas/v4 path consistently
  • Updated UI components, provider handlers, and tests to reflect the new configuration

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/types/src/provider-settings.ts Updated enum schema to only include 'international' and 'china' options
packages/types/src/providers/zai.ts Consolidated API line configs to two entries with coding endpoints
src/api/providers/zai.ts Updated default fallback from 'international_coding' to 'international'
src/api/providers/tests/zai.spec.ts Updated test assertions to expect new coding endpoint URLs
webview-ui/src/components/settings/providers/ZAi.tsx Updated UI dropdown default and API key link logic

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link

@roomote roomote bot left a 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 this is a well-executed consolidation that simplifies the Z AI provider configuration. The code changes are clean and the tests have been properly updated. I have some suggestions for improvement below.

export class ZAiHandler extends BaseOpenAiCompatibleProvider<InternationalZAiModelId | MainlandZAiModelId> {
constructor(options: ApiHandlerOptions) {
const isChina = zaiApiLineConfigs[options.zaiApiLine ?? "international_coding"].isChina
const isChina = zaiApiLineConfigs[options.zaiApiLine ?? "international"].isChina
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this removes existing enum values ('international_coding' and 'china_coding'), users with existing configurations using these values might encounter runtime issues. Could we consider adding a migration handler here?

Suggested change
const isChina = zaiApiLineConfigs[options.zaiApiLine ?? "international"].isChina
const isChina = zaiApiLineConfigs[options.zaiApiLine ?? "international"].isChina ||
// Handle legacy values for backward compatibility
(options.zaiApiLine === "china_coding" ||
(options.zaiApiLine as any) === "international_coding" && false);

This would gracefully handle legacy configurations.

international_coding: {
name: "International Coding Plan",
international: {
name: "International",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the naming convention intentional where the enum uses lowercase 'international' and 'china' but the display names are 'International' and 'China'? Just confirming this is the desired pattern - it's consistent throughout, which is good.

<label className="block font-medium mb-1">{t("settings:providers.zaiEntrypoint")}</label>
<VSCodeDropdown
value={apiConfiguration.zaiApiLine || zaiApiLineSchema.enum.international_coding}
value={apiConfiguration.zaiApiLine || zaiApiLineSchema.enum.international}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job updating the default value here. This ensures the UI properly reflects the new simplified options.

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Sep 18, 2025
…n keys in UI; keep brand string 'Z AI' unlocalized
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Sep 18, 2025
"description": "GLM-4.5 是智譜最新的旗艦模型,在推理、程式設計與智能體方面具備頂尖能力,達到開源模型的先進水準,支援最長 128k 上下文。"
},
"glm-4.5-air": {
"description": "GLM-4.5‑Air 是 GLM-4.5 的輕量版,在效能與成本之間取得平衡,並可彈性切換至混合思維模式。"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: The hyphen in "GLM-4.5‑Air" appears non-standard compared to other dashes used (e.g., in "GLM-4.5"). Consider replacing it with a standard ASCII hyphen for consistency.

Suggested change
"description": "GLM-4.5Air 是 GLM-4.5 的輕量版,在效能與成本之間取得平衡,並可彈性切換至混合思維模式。"
"description": "GLM-4.5-Air 是 GLM-4.5 的輕量版,在效能與成本之間取得平衡,並可彈性切換至混合思維模式。"

let found = false

// Try to find the longest matching key first
for (const key of Object.keys(current)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment indicates 'Try to find the longest matching key first,' but the code iterates over Object.keys(current) without sorting by key length. This may cause ambiguous matches when multiple keys (e.g. 'aaa' and 'aaa.b') exist. Consider sorting the keys in descending order by length to ensure the longest matching key is selected.

Suggested change
for (const key of Object.keys(current)) {
for (const key of Object.keys(current).sort((a, b) => b.length - a.length)) {

@hannesrudolph hannesrudolph moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Sep 19, 2025
@hannesrudolph hannesrudolph added PR - Needs Preliminary Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Sep 19, 2025
@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Changes Requested] in Roo Code Roadmap Sep 19, 2025
"china": "China"
},
"zaiModels": {
"glm-4.5": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we save these descriptions in the types package and they are all in English, unless we want to change all of them I think it makes sense to keep them like that

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it's necessary for this to handle keys with dots, I would think that would be a bad practice for translation keys.

@hannesrudolph
Copy link
Collaborator Author

@roomote-agent run pr-fixer on this PR

@hannesrudolph hannesrudolph self-assigned this Sep 23, 2025
Copy link

roomote bot commented Sep 23, 2025

Hi @hannesrudolph! I see your request to run pr-fixer on this PR. I am now reviewing the changes and checking for any issues that need to be addressed. All CI checks are currently passing. I will analyze the code changes and provide an update shortly.

Copy link

roomote bot commented Sep 23, 2025

Hi @hannesrudolph! I have completed the review of PR #8162.

Summary

All checks are passing - The PR successfully consolidates Z AI entrypoints to two main options (International and China) and removes legacy enum values.

What I verified:

  • ✅ CI/CD checks are all passing
  • ✅ Translation files are complete (no missing translations)
  • ✅ Unit tests for Z AI provider are passing (15/15 tests)
  • ✅ TypeScript compilation is successful with no type errors
  • ✅ The code changes correctly:
    • Update zaiApiLineSchema to only include "international" and "china" options
    • Configure correct endpoints for each region
    • Update UI components to reflect the new options
    • Maintain backward compatibility by defaulting to "international"

Key changes reviewed:

  • packages/types/src/provider-settings.ts: Updated schema to remove legacy values
  • packages/types/src/providers/zai.ts: Consolidated endpoint configurations
  • src/api/providers/zai.ts: Updated handler to use new configurations
  • webview-ui/src/components/settings/providers/ZAi.tsx: Updated UI dropdown
  • Translation files: Added new translation keys for all supported languages
  • scripts/find-missing-translations.js: Fixed handling of dots in JSON keys

The PR is ready to merge. All functionality has been verified and no issues were found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR - Changes Requested size:M This PR changes 30-99 lines, ignoring generated files.
Projects
Status: PR [Changes Requested]
Development

Successfully merging this pull request may close these issues.

2 participants