-
Notifications
You must be signed in to change notification settings - Fork 2.4k
feat: show model ID in API configuration dropdown #7423
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,7 @@ interface ApiConfigSelectorProps { | |
| title: string | ||
| onChange: (value: string) => void | ||
| triggerClassName?: string | ||
| listApiConfigMeta: Array<{ id: string; name: string }> | ||
| listApiConfigMeta: Array<{ id: string; name: string; modelId?: string }> | ||
| pinnedApiConfigs?: Record<string, boolean> | ||
| togglePinnedApiConfig: (id: string) => void | ||
| } | ||
|
|
@@ -87,7 +87,7 @@ export const ApiConfigSelector = ({ | |
| }, []) | ||
|
|
||
| const renderConfigItem = useCallback( | ||
| (config: { id: string; name: string }, isPinned: boolean) => { | ||
| (config: { id: string; name: string; modelId?: string }, isPinned: boolean) => { | ||
| const isCurrentConfig = config.id === value | ||
|
|
||
| return ( | ||
|
|
@@ -100,7 +100,19 @@ export const ApiConfigSelector = ({ | |
| isCurrentConfig && | ||
| "bg-vscode-list-activeSelectionBackground text-vscode-list-activeSelectionForeground", | ||
| )}> | ||
| <span className="flex-1 truncate">{config.name}</span> | ||
| <div className="flex-1 min-w-0 flex items-center gap-1 overflow-hidden"> | ||
| <span className="flex-shrink-0">{config.name}</span> | ||
| {config.modelId && ( | ||
| <> | ||
| <span className="text-vscode-descriptionForeground opacity-70 flex-shrink-0">·</span> | ||
| <span | ||
| className="text-vscode-descriptionForeground opacity-70 min-w-0 overflow-hidden" | ||
| style={{ direction: "rtl", textOverflow: "ellipsis", whiteSpace: "nowrap" }}> | ||
| {config.modelId} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice implementation of the smart truncation! The visual hierarchy is clear with the opacity and separator. For future consideration: if this pattern of displaying model IDs with profile names is needed elsewhere, this could be extracted into a small reusable component like . |
||
| </span> | ||
| </> | ||
| )} | ||
| </div> | ||
| <div className="flex items-center gap-1"> | ||
| {isCurrentConfig && ( | ||
| <div className="size-5 p-1 flex items-center justify-center"> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,21 @@ vi.mock("@/components/ui/hooks/useRooPortal", () => ({ | |
| useRooPortal: () => document.body, | ||
| })) | ||
|
|
||
| // Mock the ExtensionStateContext | ||
| vi.mock("@/context/ExtensionStateContext", () => ({ | ||
| useExtensionState: () => ({ | ||
| apiConfiguration: { | ||
| apiProvider: "anthropic", | ||
| apiModelId: "claude-3-opus-20240229", | ||
| }, | ||
| }), | ||
| })) | ||
|
|
||
| // Mock the getModelId function from @roo-code/types | ||
| vi.mock("@roo-code/types", () => ({ | ||
| getModelId: (config: any) => config?.apiModelId || undefined, | ||
| })) | ||
|
|
||
| // Mock Popover components to be testable | ||
| vi.mock("@/components/ui", () => ({ | ||
| Popover: ({ children, open }: any) => ( | ||
|
|
@@ -52,9 +67,9 @@ describe("ApiConfigSelector", () => { | |
| title: "API Config", | ||
| onChange: mockOnChange, | ||
| listApiConfigMeta: [ | ||
| { id: "config1", name: "Config 1" }, | ||
| { id: "config2", name: "Config 2" }, | ||
| { id: "config3", name: "Config 3" }, | ||
| { id: "config1", name: "Config 1", modelId: "claude-3-opus-20240229" }, | ||
| { id: "config2", name: "Config 2", modelId: "gpt-4" }, | ||
| { id: "config3", name: "Config 3", modelId: "claude-3-sonnet-20240229" }, | ||
| ], | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great test coverage! Consider adding one more test case specifically for profiles without model IDs to ensure the component handles undefined modelId gracefully (though the code already handles this well with optional chaining). |
||
| pinnedApiConfigs: { config1: true }, | ||
| togglePinnedApiConfig: mockTogglePinnedApiConfig, | ||
|
|
@@ -120,13 +135,13 @@ describe("ApiConfigSelector", () => { | |
| const props = { | ||
| ...defaultProps, | ||
| listApiConfigMeta: [ | ||
| { id: "config1", name: "Config 1" }, | ||
| { id: "config2", name: "Config 2" }, | ||
| { id: "config3", name: "Config 3" }, | ||
| { id: "config4", name: "Config 4" }, | ||
| { id: "config5", name: "Config 5" }, | ||
| { id: "config6", name: "Config 6" }, | ||
| { id: "config7", name: "Config 7" }, | ||
| { id: "config1", name: "Config 1", modelId: "claude-3-opus-20240229" }, | ||
| { id: "config2", name: "Config 2", modelId: "gpt-4" }, | ||
| { id: "config3", name: "Config 3", modelId: "claude-3-sonnet-20240229" }, | ||
| { id: "config4", name: "Config 4", modelId: "gpt-3.5-turbo" }, | ||
| { id: "config5", name: "Config 5", modelId: "claude-3-haiku-20240307" }, | ||
| { id: "config6", name: "Config 6", modelId: "gpt-4-turbo" }, | ||
| { id: "config7", name: "Config 7", modelId: "claude-2.1" }, | ||
| ], | ||
| } | ||
| render(<ApiConfigSelector {...props} />) | ||
|
|
@@ -154,13 +169,13 @@ describe("ApiConfigSelector", () => { | |
| const props = { | ||
| ...defaultProps, | ||
| listApiConfigMeta: [ | ||
| { id: "config1", name: "Config 1" }, | ||
| { id: "config2", name: "Config 2" }, | ||
| { id: "config3", name: "Config 3" }, | ||
| { id: "config4", name: "Config 4" }, | ||
| { id: "config5", name: "Config 5" }, | ||
| { id: "config6", name: "Config 6" }, | ||
| { id: "config7", name: "Config 7" }, | ||
| { id: "config1", name: "Config 1", modelId: "claude-3-opus-20240229" }, | ||
| { id: "config2", name: "Config 2", modelId: "gpt-4" }, | ||
| { id: "config3", name: "Config 3", modelId: "claude-3-sonnet-20240229" }, | ||
| { id: "config4", name: "Config 4", modelId: "gpt-3.5-turbo" }, | ||
| { id: "config5", name: "Config 5", modelId: "claude-3-haiku-20240307" }, | ||
| { id: "config6", name: "Config 6", modelId: "gpt-4-turbo" }, | ||
| { id: "config7", name: "Config 7", modelId: "claude-2.1" }, | ||
| ], | ||
| } | ||
| render(<ApiConfigSelector {...props} />) | ||
|
|
@@ -184,13 +199,13 @@ describe("ApiConfigSelector", () => { | |
| const props = { | ||
| ...defaultProps, | ||
| listApiConfigMeta: [ | ||
| { id: "config1", name: "Config 1" }, | ||
| { id: "config2", name: "Config 2" }, | ||
| { id: "config3", name: "Config 3" }, | ||
| { id: "config4", name: "Config 4" }, | ||
| { id: "config5", name: "Config 5" }, | ||
| { id: "config6", name: "Config 6" }, | ||
| { id: "config7", name: "Config 7" }, | ||
| { id: "config1", name: "Config 1", modelId: "claude-3-opus-20240229" }, | ||
| { id: "config2", name: "Config 2", modelId: "gpt-4" }, | ||
| { id: "config3", name: "Config 3", modelId: "claude-3-sonnet-20240229" }, | ||
| { id: "config4", name: "Config 4", modelId: "gpt-3.5-turbo" }, | ||
| { id: "config5", name: "Config 5", modelId: "claude-3-haiku-20240307" }, | ||
| { id: "config6", name: "Config 6", modelId: "gpt-4-turbo" }, | ||
| { id: "config7", name: "Config 7", modelId: "claude-2.1" }, | ||
| ], | ||
| } | ||
| render(<ApiConfigSelector {...props} />) | ||
|
|
@@ -210,13 +225,13 @@ describe("ApiConfigSelector", () => { | |
| const props = { | ||
| ...defaultProps, | ||
| listApiConfigMeta: [ | ||
| { id: "config1", name: "Config 1" }, | ||
| { id: "config2", name: "Config 2" }, | ||
| { id: "config3", name: "Config 3" }, | ||
| { id: "config4", name: "Config 4" }, | ||
| { id: "config5", name: "Config 5" }, | ||
| { id: "config6", name: "Config 6" }, | ||
| { id: "config7", name: "Config 7" }, | ||
| { id: "config1", name: "Config 1", modelId: "claude-3-opus-20240229" }, | ||
| { id: "config2", name: "Config 2", modelId: "gpt-4" }, | ||
| { id: "config3", name: "Config 3", modelId: "claude-3-sonnet-20240229" }, | ||
| { id: "config4", name: "Config 4", modelId: "gpt-3.5-turbo" }, | ||
| { id: "config5", name: "Config 5", modelId: "claude-3-haiku-20240307" }, | ||
| { id: "config6", name: "Config 6", modelId: "gpt-4-turbo" }, | ||
| { id: "config7", name: "Config 7", modelId: "claude-2.1" }, | ||
| ], | ||
| } | ||
| render(<ApiConfigSelector {...props} />) | ||
|
|
@@ -263,7 +278,8 @@ describe("ApiConfigSelector", () => { | |
| const config1Elements = screen.getAllByText("Config 1") | ||
| // Find the one that's in the dropdown content (not the trigger) | ||
| const configInDropdown = config1Elements.find((el) => el.closest('[data-testid="popover-content"]')) | ||
| const selectedConfigRow = configInDropdown?.closest("div") | ||
| // Navigate up to find the parent row that contains both the text and the check icon | ||
| const selectedConfigRow = configInDropdown?.closest(".group") | ||
| const checkIcon = selectedConfigRow?.querySelector(".codicon-check") | ||
| expect(checkIcon).toBeInTheDocument() | ||
| }) | ||
|
|
@@ -280,13 +296,24 @@ describe("ApiConfigSelector", () => { | |
| fireEvent.click(trigger) | ||
|
|
||
| const content = screen.getByTestId("popover-content") | ||
| const configTexts = content.querySelectorAll(".truncate") | ||
| // Get all config items by looking for the group class | ||
| const configRows = content.querySelectorAll(".group") | ||
|
|
||
| // Extract the config names from each row | ||
| const configNames: string[] = [] | ||
| configRows.forEach((row) => { | ||
| // Find the first span that's flex-shrink-0 (the profile name) | ||
| const nameElement = row.querySelector(".flex-1 span.flex-shrink-0") | ||
| if (nameElement?.textContent) { | ||
| configNames.push(nameElement.textContent) | ||
| } | ||
| }) | ||
|
|
||
| // Pinned configs should appear first | ||
| expect(configTexts[0]).toHaveTextContent("Config 1") | ||
| expect(configTexts[1]).toHaveTextContent("Config 3") | ||
| expect(configNames[0]).toBe("Config 1") | ||
| expect(configNames[1]).toBe("Config 3") | ||
| // Unpinned config should appear after separator | ||
| expect(configTexts[2]).toHaveTextContent("Config 2") | ||
| expect(configNames[2]).toBe("Config 2") | ||
| }) | ||
|
|
||
| test("toggles pin status when pin button is clicked", () => { | ||
|
|
@@ -296,8 +323,10 @@ describe("ApiConfigSelector", () => { | |
| fireEvent.click(trigger) | ||
|
|
||
| // Find the pin button for Config 2 (unpinned) | ||
| const config2Row = screen.getByText("Config 2").closest("div") | ||
| const pinButton = config2Row?.querySelector("button") | ||
| const config2Row = screen.getByText("Config 2").closest(".group") | ||
| // Find the button with the pin icon (it's the second button, first is the row itself) | ||
| const buttons = config2Row?.querySelectorAll("button") | ||
| const pinButton = Array.from(buttons || []).find((btn) => btn.querySelector(".codicon-pin")) | ||
|
|
||
| if (pinButton) { | ||
| fireEvent.click(pinButton) | ||
|
|
@@ -332,13 +361,13 @@ describe("ApiConfigSelector", () => { | |
| const props = { | ||
| ...defaultProps, | ||
| listApiConfigMeta: [ | ||
| { id: "config1", name: "Config 1" }, | ||
| { id: "config2", name: "Config 2" }, | ||
| { id: "config3", name: "Config 3" }, | ||
| { id: "config4", name: "Config 4" }, | ||
| { id: "config5", name: "Config 5" }, | ||
| { id: "config6", name: "Config 6" }, | ||
| { id: "config7", name: "Config 7" }, | ||
| { id: "config1", name: "Config 1", modelId: "claude-3-opus-20240229" }, | ||
| { id: "config2", name: "Config 2", modelId: "gpt-4" }, | ||
| { id: "config3", name: "Config 3", modelId: "claude-3-sonnet-20240229" }, | ||
| { id: "config4", name: "Config 4", modelId: "gpt-3.5-turbo" }, | ||
| { id: "config5", name: "Config 5", modelId: "claude-3-haiku-20240307" }, | ||
| { id: "config6", name: "Config 6", modelId: "gpt-4-turbo" }, | ||
| { id: "config7", name: "Config 7", modelId: "claude-2.1" }, | ||
| ], | ||
| } | ||
| render(<ApiConfigSelector {...props} />) | ||
|
|
@@ -389,13 +418,13 @@ describe("ApiConfigSelector", () => { | |
| const props = { | ||
| ...defaultProps, | ||
| listApiConfigMeta: [ | ||
| { id: "config1", name: "Config 1" }, | ||
| { id: "config2", name: "Config 2" }, | ||
| { id: "config3", name: "Config 3" }, | ||
| { id: "config4", name: "Config 4" }, | ||
| { id: "config5", name: "Config 5" }, | ||
| { id: "config6", name: "Config 6" }, | ||
| { id: "config7", name: "Config 7" }, | ||
| { id: "config1", name: "Config 1", modelId: "claude-3-opus-20240229" }, | ||
| { id: "config2", name: "Config 2", modelId: "gpt-4" }, | ||
| { id: "config3", name: "Config 3", modelId: "claude-3-sonnet-20240229" }, | ||
| { id: "config4", name: "Config 4", modelId: "gpt-3.5-turbo" }, | ||
| { id: "config5", name: "Config 5", modelId: "claude-3-haiku-20240307" }, | ||
| { id: "config6", name: "Config 6", modelId: "gpt-4-turbo" }, | ||
| { id: "config7", name: "Config 7", modelId: "claude-2.1" }, | ||
| ], | ||
| } | ||
| render(<ApiConfigSelector {...props} />) | ||
|
|
||
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 this intentional to duplicate the model ID cleaning logic here? Consider moving the cleanModelId() helper to a shared utility location (perhaps in packages/types/src/provider-settings.ts near the existing getModelId() function) since this logic might be useful elsewhere in the codebase.
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.
It's not duplicated