Skip to content
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

fix: close 5315 #5316

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions app/components/model-config.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,16 @@ import { InputRange } from "./input-range";
import { ListItem, Select } from "./ui-lib";
import { useAllModels } from "../utils/hooks";

function getModelName(model: { model: string; providerName: string }) {
return `${model.model}----${model.providerName}`;
}

export function ModelConfigList(props: {
modelConfig: ModelConfig;
updateConfig: (updater: (config: ModelConfig) => void) => void;
}) {
const allModels = useAllModels();
const value = `${props.modelConfig.model}@${props.modelConfig?.providerName}`;
const value = getModelName(props.modelConfig);

return (
<>
Expand All @@ -20,7 +24,7 @@ export function ModelConfigList(props: {
aria-label={Locale.Settings.Model}
value={value}
onChange={(e) => {
const [model, providerName] = e.currentTarget.value.split("@");
const [model, providerName] = e.currentTarget.value.split("----");
props.updateConfig((config) => {
config.model = ModalConfigValidator.model(model);
config.providerName = providerName as ServiceProvider;
Expand All @@ -30,7 +34,13 @@ export function ModelConfigList(props: {
{allModels
.filter((v) => v.available)
.map((v, i) => (
<option value={`${v.name}@${v.provider?.providerName}`} key={i}>
<option
value={getModelName({
model: v.name,
providerName: v.provider?.providerName!,
})}
key={i}
>
{v.displayName}({v.provider?.providerName})
</option>
))}
Expand Down
52 changes: 52 additions & 0 deletions app/utils/__tests__/model.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { describe, it, expect } from "vitest";
import { parseModelName } from "../model";

describe("parseModelName", () => {
it('should parse a simple model name without "@"', () => {
const result = parseModelName("simpleModel");
expect(result).toEqual({
customModelName: "simpleModel",
customProviderName: "",
});
});

it('should parse a model name with "@"', () => {
const result = parseModelName("modelName@providerName");
expect(result).toEqual({
customModelName: "modelName",
customProviderName: "providerName",
});
});

it('should parse a quoted model name without "@"', () => {
const result = parseModelName('"quotedModel"');
expect(result).toEqual({
customModelName: "quotedModel",
customProviderName: "quotedModel",
});
});

it('should parse a quoted model name with "@"', () => {
const result = parseModelName('"quotedModel@providerName"');
expect(result).toEqual({
customModelName: "quotedModel@providerName",
customProviderName: "quotedModel@providerName",
});
});

it('should parse a model name with multiple "@" symbols', () => {
const result = parseModelName("modelName@providerName@extra");
expect(result).toEqual({
customModelName: "modelName",
customProviderName: "providerName@extra",
});
});

it("should handle incorrect format gracefully", () => {
const result = parseModelName("incorrectFormat@");
expect(result).toEqual({
customModelName: "incorrectFormat",
customProviderName: "",
});
});
});
Comment on lines +4 to +52
Copy link
Contributor

Choose a reason for hiding this comment

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

Review of Unit Tests for parseModelName Function

  1. Simple Model Name Test (Lines 5-11): This test is well-implemented and correctly checks for a model name without the "@" symbol.

  2. Model Name with "@" Test (Lines 13-19): The test correctly handles model names with an "@" symbol, splitting it into customModelName and customProviderName.

  3. Quoted Model Name without "@" Test (Lines 21-27): The expected result seems incorrect. The customProviderName should not be the same as customModelName if the "@" symbol is absent.

  4. Quoted Model Name with "@" Test (Lines 29-35): This test incorrectly expects the entire input as both customModelName and customProviderName. The function should ideally split the name at the "@" symbol.

  5. Multiple "@" Symbols Test (Lines 37-43): This test is correct and handles multiple "@" symbols appropriately, assigning the first part to customModelName and the rest to customProviderName.

  6. Incorrect Format Test (Lines 45-51): This test handles an edge case where the format is incorrect but could be more robust by testing more malformed inputs.

  • The tests for quoted model names (Lines 21-35) have incorrect expectations. Consider revising the expected results to accurately reflect the function's behavior.
  • Enhance the robustness of the incorrect format test by including more edge cases.

Consider using parameterized tests to reduce redundancy and cover more scenarios with less code.

28 changes: 26 additions & 2 deletions app/utils/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,25 @@ const sortModelTable = (models: ReturnType<typeof collectModels>) =>
}
});

export function parseModelName(name: string): {
customModelName: string;
customProviderName: string;
} {
let customModelName = name.split("@")[0];
let customProviderName = name.slice(customModelName.length + 1);
if (name.startsWith("'") || name.startsWith('"')) {
const match = name.match(/^(['"])(.*?)\1(@.*)?$/);
if (match) {
customModelName = match[2];
customProviderName = match[3]?.slice(1) || customModelName;
}
}
return { customModelName, customProviderName } as {
customModelName: string;
customProviderName: string;
};
}

export function collectModelTable(
models: readonly LLMModel[],
customModels: string,
Expand Down Expand Up @@ -79,7 +98,12 @@ export function collectModelTable(
);
} else {
// 1. find model by name, and set available value
const [customModelName, customProviderName] = name.split("@");
// modelName@azure => 'modelName', 'azure'
// 'modelName@azure' => 'modelName@azure', ''
// modelName@azure=deploymentName => 'modelName', 'azure=deploymentName'
// 'modelName@azure'=deploymentName => 'modelName@azure', '=deploymentName
// 'modelName@azure'@azure=deploymentName => 'modelName@azure', 'azure=deploymentName'
let { customModelName, customProviderName } = parseModelName(name);
let count = 0;
for (const fullName in modelTable) {
const [modelName, providerName] = fullName.split("@");
Expand All @@ -102,7 +126,7 @@ export function collectModelTable(
}
// 2. if model not exists, create new model with available value
if (count === 0) {
let [customModelName, customProviderName] = name.split("@");
let { customModelName, customProviderName } = parseModelName(name);
const provider = customProvider(
customProviderName || customModelName,
);
Expand Down
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
"app:build": "yarn mask && yarn tauri build",
"prompts": "node ./scripts/fetch-prompts.mjs",
"prepare": "husky install",
"proxy-dev": "sh ./scripts/init-proxy.sh && proxychains -f ./scripts/proxychains.conf yarn dev"
"proxy-dev": "sh ./scripts/init-proxy.sh && proxychains -f ./scripts/proxychains.conf yarn dev",
"test": "vitest run"
},
"dependencies": {
"@fortaine/fetch-event-source": "^3.0.6",
Expand Down Expand Up @@ -66,6 +67,7 @@
"prettier": "^3.0.2",
"tsx": "^4.16.0",
"typescript": "5.2.2",
"vitest": "^2.0.5",
"watch": "^1.0.2",
"webpack": "^5.88.1"
},
Expand Down
Loading
Loading