feat: add ASR and LLM connections - #25
Conversation
Co-Authored-By: Claude <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for managing multiple independent ASR and LLM API connections, allowing users to save, switch, and configure multiple providers in the UI. The backend configuration has been updated to support connection lists with automatic migration from legacy single-value settings, accompanied by robust unit tests. Feedback on these changes highlights opportunities to improve robustness and compatibility, including adding a safety guard for uninitialized configurations in the frontend, trimming input values (endpoints, models, API keys) on both sides to prevent whitespace issues, replacing unstable Rust let_chains with standard nested if let statements for stable compiler compatibility, and optimizing struct cloning in the configuration normalization logic.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function ensureConnectionShape() { | ||
| if (!currentConfig.asr) currentConfig.asr = {}; |
There was a problem hiding this comment.
Add a guard to prevent a TypeError if currentConfig is null or undefined when ensureConnectionShape is called (for example, if the configuration fails to load or if an event handler triggers before initialization completes).
| function ensureConnectionShape() { | |
| if (!currentConfig.asr) currentConfig.asr = {}; | |
| function ensureConnectionShape() { | |
| if (!currentConfig) return; | |
| if (!currentConfig.asr) currentConfig.asr = {}; |
| function saveAsrConnectionFields(connectionId = currentConfig.asr.active_connection) { | ||
| ensureConnectionShape(); | ||
| const connection = currentConfig.asr.connections.find(c => c.id === connectionId); | ||
| if (!connection) return; | ||
| connection.name = document.getElementById('asr-connection-name').value || connection.id; | ||
| connection.provider = document.getElementById('asr-provider').value || 'mock'; | ||
| connection.endpoint = document.getElementById('asr-endpoint').value || null; | ||
| connection.api_key = document.getElementById('asr-api-key').value || null; | ||
| connection.model = document.getElementById('asr-model').value || null; | ||
| connection.language = document.getElementById('asr-language').value || null; | ||
| } | ||
|
|
||
| function saveLlmConnectionFields(connectionId = currentConfig.llm.active_connection) { | ||
| ensureConnectionShape(); | ||
| const connection = currentConfig.llm.connections.find(c => c.id === connectionId); | ||
| if (!connection) return; | ||
| connection.name = document.getElementById('llm-connection-name').value || connection.id; | ||
| connection.provider = document.getElementById('llm-provider').value || 'mock'; | ||
| connection.endpoint = document.getElementById('llm-endpoint').value || null; | ||
| connection.api_key = document.getElementById('llm-api-key').value || null; | ||
| connection.model = document.getElementById('llm-model').value || null; | ||
| } |
There was a problem hiding this comment.
Trimming the input values before saving them to the connection object is highly recommended. Since isValidEndpoint trims the endpoint value during validation, not trimming it here can lead to saving values with leading/trailing whitespaces (e.g., " http://api.openai.com/v1 "), which might cause URI parsing or connection failures in the Rust backend.
function saveAsrConnectionFields(connectionId = currentConfig.asr.active_connection) {
ensureConnectionShape();
const connection = currentConfig.asr.connections.find(c => c.id === connectionId);
if (!connection) return;
connection.name = document.getElementById('asr-connection-name').value.trim() || connection.id;
connection.provider = document.getElementById('asr-provider').value || 'mock';
connection.endpoint = document.getElementById('asr-endpoint').value.trim() || null;
connection.api_key = document.getElementById('asr-api-key').value.trim() || null;
connection.model = document.getElementById('asr-model').value.trim() || null;
connection.language = document.getElementById('asr-language').value.trim() || null;
}
function saveLlmConnectionFields(connectionId = currentConfig.llm.active_connection) {
ensureConnectionShape();
const connection = currentConfig.llm.connections.find(c => c.id === connectionId);
if (!connection) return;
connection.name = document.getElementById('llm-connection-name').value.trim() || connection.id;
connection.provider = document.getElementById('llm-provider').value || 'mock';
connection.endpoint = document.getElementById('llm-endpoint').value.trim() || null;
connection.api_key = document.getElementById('llm-api-key').value.trim() || null;
connection.model = document.getElementById('llm-model').value.trim() || null;
}| if !self | ||
| .connections | ||
| .iter() | ||
| .any(|connection| connection.id == self.active_connection) | ||
| && let Some(first) = self.connections.first() | ||
| { | ||
| self.active_connection = first.id.clone(); | ||
| } |
There was a problem hiding this comment.
The use of let_chains (e.g., && let Some(first) = ...) is an unstable Rust feature that requires a nightly compiler and the #![feature(let_chains)] attribute. To ensure compatibility with the stable Rust compiler and improve portability, it is better to use standard nested if let statements.
| if !self | |
| .connections | |
| .iter() | |
| .any(|connection| connection.id == self.active_connection) | |
| && let Some(first) = self.connections.first() | |
| { | |
| self.active_connection = first.id.clone(); | |
| } | |
| if !self | |
| .connections | |
| .iter() | |
| .any(|connection| connection.id == self.active_connection) | |
| { | |
| if let Some(first) = self.connections.first() { | |
| self.active_connection = first.id.clone(); | |
| } | |
| } |
| if let Some(active) = self.active_connection_config().cloned() { | ||
| self.provider = active.provider; | ||
| self.endpoint = active.endpoint; | ||
| self.model = active.model; | ||
| self.api_key = active.api_key; | ||
| self.language = active.language; | ||
| } |
There was a problem hiding this comment.
Using .cloned() on active_connection_config() clones the entire AsrConnection struct, only to immediately discard most of its fields. It is more idiomatic and efficient to borrow the active connection and clone only the individual fields that are needed.
| if let Some(active) = self.active_connection_config().cloned() { | |
| self.provider = active.provider; | |
| self.endpoint = active.endpoint; | |
| self.model = active.model; | |
| self.api_key = active.api_key; | |
| self.language = active.language; | |
| } | |
| if let Some(active) = self.active_connection_config() { | |
| self.provider = active.provider.clone(); | |
| self.endpoint = active.endpoint.clone(); | |
| self.model = active.model.clone(); | |
| self.api_key = active.api_key.clone(); | |
| self.language = active.language.clone(); | |
| } |
| if !self | ||
| .connections | ||
| .iter() | ||
| .any(|connection| connection.id == self.active_connection) | ||
| && let Some(first) = self.connections.first() | ||
| { | ||
| self.active_connection = first.id.clone(); | ||
| } |
There was a problem hiding this comment.
The use of let_chains (e.g., && let Some(first) = ...) is an unstable Rust feature that requires a nightly compiler and the #![feature(let_chains)] attribute. To ensure compatibility with the stable Rust compiler and improve portability, it is better to use standard nested if let statements.
| if !self | |
| .connections | |
| .iter() | |
| .any(|connection| connection.id == self.active_connection) | |
| && let Some(first) = self.connections.first() | |
| { | |
| self.active_connection = first.id.clone(); | |
| } | |
| if !self | |
| .connections | |
| .iter() | |
| .any(|connection| connection.id == self.active_connection) | |
| { | |
| if let Some(first) = self.connections.first() { | |
| self.active_connection = first.id.clone(); | |
| } | |
| } |
| if let Some(active) = self.active_connection_config().cloned() { | ||
| self.provider = active.provider; | ||
| self.endpoint = active.endpoint; | ||
| self.model = active.model; | ||
| self.api_key = active.api_key; | ||
| } |
There was a problem hiding this comment.
Using .cloned() on active_connection_config() clones the entire LlmConnection struct, only to immediately discard most of its fields. It is more idiomatic and efficient to borrow the active connection and clone only the individual fields that are needed.
| if let Some(active) = self.active_connection_config().cloned() { | |
| self.provider = active.provider; | |
| self.endpoint = active.endpoint; | |
| self.model = active.model; | |
| self.api_key = active.api_key; | |
| } | |
| if let Some(active) = self.active_connection_config() { | |
| self.provider = active.provider.clone(); | |
| self.endpoint = active.endpoint.clone(); | |
| self.model = active.model.clone(); | |
| self.api_key = active.api_key.clone(); | |
| } |
| let endpoint = connection | ||
| .endpoint | ||
| .as_deref() | ||
| .unwrap_or("https://api.openai.com/v1"); |
There was a problem hiding this comment.
The endpoint field is not trimmed or validated for empty strings on the Rust side. If a user accidentally saves a configuration with leading or trailing whitespaces (which is a common copy-paste error), it can cause URI parsing or connection failures. Trimming and filtering out empty strings makes the provider creation much more robust.
let endpoint = connection
.endpoint
.as_deref()
.map(|s| s.trim())
.filter(|s| !s.is_empty())
.unwrap_or("https://api.openai.com/v1");| .filter(|s| !s.is_empty()) | ||
| }); | ||
| let model = config.asr.model.as_deref().unwrap_or("whisper-1"); | ||
| let model = connection.model.as_deref().unwrap_or("whisper-1"); |
There was a problem hiding this comment.
The model field is not trimmed or validated for empty strings on the Rust side. Trimming and filtering out empty strings makes the provider creation much more robust against accidental whitespace in the configuration.
let model = connection
.model
.as_deref()
.map(|s| s.trim())
.filter(|s| !s.is_empty())
.unwrap_or("whisper-1");| let endpoint = connection | ||
| .endpoint | ||
| .as_deref() | ||
| .unwrap_or("https://api.openai.com/v1"); |
There was a problem hiding this comment.
The endpoint field is not trimmed or validated for empty strings on the Rust side. If a user accidentally saves a configuration with leading or trailing whitespaces (which is a common copy-paste error), it can cause URI parsing or connection failures. Trimming and filtering out empty strings makes the provider creation much more robust.
let endpoint = connection
.endpoint
.as_deref()
.map(|s| s.trim())
.filter(|s| !s.is_empty())
.unwrap_or("https://api.openai.com/v1");| }); | ||
|
|
||
| let model = config.llm.model.as_deref().unwrap_or("gpt-4o-mini"); | ||
| let model = connection.model.as_deref().unwrap_or("gpt-4o-mini"); |
There was a problem hiding this comment.
The model field is not trimmed or validated for empty strings on the Rust side. Trimming and filtering out empty strings makes the provider creation much more robust against accidental whitespace in the configuration.
let model = connection
.model
.as_deref()
.map(|s| s.trim())
.filter(|s| !s.is_empty())
.unwrap_or("gpt-4o-mini");Co-Authored-By: Claude <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to save, manage, and switch between multiple independent ASR and LLM API connections, migrating existing single-value configurations automatically. The changes span the frontend UI, localization files, and backend Rust configuration management. Feedback on these changes highlights potential TypeErrors in the frontend JavaScript when resolving active connections or loading fields, the use of the unstable Rust let_chains feature which breaks stable compiler compatibility, and incomplete Chinese translations in the localization files.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function saveAsrConnectionFields(connectionId = currentConfig.asr.active_connection) { | ||
| if (!ensureConnectionShape()) return; | ||
| const connection = currentConfig.asr.connections.find(c => c.id === connectionId); | ||
| if (!connection) return; |
There was a problem hiding this comment.
Evaluating currentConfig.asr.active_connection as a default parameter will throw a TypeError if currentConfig is null at call time. This bypasses the ensureConnectionShape() safety check inside the function body. It is safer to resolve the active connection ID inside the function after the safety check.
| function saveAsrConnectionFields(connectionId = currentConfig.asr.active_connection) { | |
| if (!ensureConnectionShape()) return; | |
| const connection = currentConfig.asr.connections.find(c => c.id === connectionId); | |
| if (!connection) return; | |
| function saveAsrConnectionFields(connectionId) { | |
| if (!ensureConnectionShape()) return; | |
| const id = connectionId || currentConfig.asr.active_connection; | |
| const connection = currentConfig.asr.connections.find(c => c.id === id); | |
| if (!connection) return; |
| function saveLlmConnectionFields(connectionId = currentConfig.llm.active_connection) { | ||
| if (!ensureConnectionShape()) return; | ||
| const connection = currentConfig.llm.connections.find(c => c.id === connectionId); | ||
| if (!connection) return; |
There was a problem hiding this comment.
Evaluating currentConfig.llm.active_connection as a default parameter will throw a TypeError if currentConfig is null at call time. This bypasses the ensureConnectionShape() safety check inside the function body. It is safer to resolve the active connection ID inside the function after the safety check.
| function saveLlmConnectionFields(connectionId = currentConfig.llm.active_connection) { | |
| if (!ensureConnectionShape()) return; | |
| const connection = currentConfig.llm.connections.find(c => c.id === connectionId); | |
| if (!connection) return; | |
| function saveLlmConnectionFields(connectionId) { | |
| if (!ensureConnectionShape()) return; | |
| const id = connectionId || currentConfig.llm.active_connection; | |
| const connection = currentConfig.llm.connections.find(c => c.id === id); | |
| if (!connection) return; |
| function loadAsrConnectionFields(connection) { | ||
| document.getElementById('asr-connection-name').value = connection.name || ''; |
There was a problem hiding this comment.
Add a defensive guard to prevent a TypeError if connection is null or undefined when this function is called.
| function loadAsrConnectionFields(connection) { | |
| document.getElementById('asr-connection-name').value = connection.name || ''; | |
| function loadAsrConnectionFields(connection) { | |
| if (!connection) return; | |
| document.getElementById('asr-connection-name').value = connection.name || ''; |
| function loadLlmConnectionFields(connection) { | ||
| document.getElementById('llm-connection-name').value = connection.name || ''; |
There was a problem hiding this comment.
Add a defensive guard to prevent a TypeError if connection is null or undefined when this function is called.
| function loadLlmConnectionFields(connection) { | |
| document.getElementById('llm-connection-name').value = connection.name || ''; | |
| function loadLlmConnectionFields(connection) { | |
| if (!connection) return; | |
| document.getElementById('llm-connection-name').value = connection.name || ''; |
| if !self | ||
| .connections | ||
| .iter() | ||
| .any(|connection| connection.id == self.active_connection) | ||
| && let Some(first) = self.connections.first() | ||
| { | ||
| self.active_connection = first.id.clone(); | ||
| } |
There was a problem hiding this comment.
The let_chains feature (&& let Some(...)) is currently unstable in Rust and requires a nightly compiler. To maintain compatibility with stable Rust, rewrite this using nested if let statements.
| if !self | |
| .connections | |
| .iter() | |
| .any(|connection| connection.id == self.active_connection) | |
| && let Some(first) = self.connections.first() | |
| { | |
| self.active_connection = first.id.clone(); | |
| } | |
| if !self | |
| .connections | |
| .iter() | |
| .any(|connection| connection.id == self.active_connection) | |
| { | |
| if let Some(first) = self.connections.first() { | |
| self.active_connection = first.id.clone(); | |
| } | |
| } |
| if !self | ||
| .connections | ||
| .iter() | ||
| .any(|connection| connection.id == self.active_connection) | ||
| && let Some(first) = self.connections.first() | ||
| { | ||
| self.active_connection = first.id.clone(); | ||
| } |
There was a problem hiding this comment.
The let_chains feature (&& let Some(...)) is currently unstable in Rust and requires a nightly compiler. To maintain compatibility with stable Rust, rewrite this using nested if let statements.
| if !self | |
| .connections | |
| .iter() | |
| .any(|connection| connection.id == self.active_connection) | |
| && let Some(first) = self.connections.first() | |
| { | |
| self.active_connection = first.id.clone(); | |
| } | |
| if !self | |
| .connections | |
| .iter() | |
| .any(|connection| connection.id == self.active_connection) | |
| { | |
| if let Some(first) = self.connections.first() { | |
| self.active_connection = first.id.clone(); | |
| } | |
| } |
| "settings.asr_connections": "ASR Connections", | ||
| "settings.llm_connections": "LLM Connections", |
There was a problem hiding this comment.
Translate the connection labels to Chinese to ensure complete localization of the user interface.
| "settings.asr_connections": "ASR Connections", | |
| "settings.llm_connections": "LLM Connections", | |
| "settings.asr_connections": "ASR 连接", | |
| "settings.llm_connections": "LLM 连接", |
| "settings.new_asr_connection": "新的 ASR Connection", | ||
| "settings.new_llm_connection": "新的 LLM Connection", |
There was a problem hiding this comment.
Translate the connection labels to Chinese to ensure complete localization of the user interface.
| "settings.new_asr_connection": "新的 ASR Connection", | |
| "settings.new_llm_connection": "新的 LLM Connection", | |
| "settings.new_asr_connection": "新的 ASR 连接", | |
| "settings.new_llm_connection": "新的 LLM 连接", |
Co-Authored-By: Claude <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for managing multiple independent ASR and LLM API connections, allowing users to save, delete, and switch between different configurations. The changes span the frontend UI, localization files, configuration structs, and core provider initialization logic, including backward-compatible migration for legacy single-value configurations. The review feedback suggests several robustness improvements: using window.crypto instead of the global crypto object in the frontend to prevent potential ReferenceError in older webviews, and trimming/filtering whitespace from provider and language configuration strings in the Rust core to prevent matching or initialization failures.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function createConnectionId(prefix) { | ||
| if (crypto?.randomUUID) return `${prefix}-${crypto.randomUUID()}`; | ||
| return `${prefix}-${Date.now()}`; | ||
| } |
There was a problem hiding this comment.
Referencing the global crypto object directly can throw a ReferenceError if it is not defined in the environment (e.g., in older webviews or specific non-browser environments). Referencing it via window.crypto is safer as it will safely evaluate to undefined instead of throwing an error.
| function createConnectionId(prefix) { | |
| if (crypto?.randomUUID) return `${prefix}-${crypto.randomUUID()}`; | |
| return `${prefix}-${Date.now()}`; | |
| } | |
| function createConnectionId(prefix) { | |
| if (window.crypto?.randomUUID) return `${prefix}-${window.crypto.randomUUID()}`; | |
| return `${prefix}-${Date.now()}`; | |
| } |
| .active_connection_config() | ||
| .ok_or_else(|| anyhow::anyhow!("active ASR connection not found"))?; | ||
|
|
||
| match connection.provider.as_str() { |
There was a problem hiding this comment.
Trimming the provider string before matching ensures that any accidental leading or trailing whitespace in the configuration file or user input does not cause the provider matching to fail.
| match connection.provider.as_str() { | |
| match connection.provider.trim() { |
| if let Some(lang) = &connection.language { | ||
| provider = provider.with_language(lang.clone()); | ||
| } |
There was a problem hiding this comment.
Trimming and filtering the language string ensures that empty or whitespace-only language codes are not passed to the ASR provider, which could cause errors or unexpected behavior.
if let Some(lang) = connection
.language
.as_deref()
.map(|s| s.trim())
.filter(|s| !s.is_empty())
{
provider = provider.with_language(lang.to_string());
}| .active_connection_config() | ||
| .ok_or_else(|| anyhow::anyhow!("active LLM connection not found"))?; | ||
|
|
||
| match connection.provider.as_str() { |
There was a problem hiding this comment.
Trimming the provider string before matching ensures that any accidental leading or trailing whitespace in the configuration file or user input does not cause the provider matching to fail.
| match connection.provider.as_str() { | |
| match connection.provider.trim() { |
Co-Authored-By: Claude <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for multiple independent ASR and LLM API connections, allowing users to save, manage, and switch between them in the settings. It updates the configuration structures, adds normalization logic to migrate legacy single-value configurations, and implements the corresponding frontend UI and state management. Feedback on the changes highlights a compatibility issue where the unstable let_chains feature (&& let) is used, which will cause compilation errors on stable Rust toolchains. Additionally, the reviewer pointed out inefficient cloning of entire connection structs during normalization and suggested cloning only the required fields instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if !self | ||
| .connections | ||
| .iter() | ||
| .any(|connection| connection.id == self.active_connection) | ||
| && let Some(first) = self.connections.first() | ||
| { | ||
| self.active_connection = first.id.clone(); | ||
| } |
There was a problem hiding this comment.
Using the unstable let_chains feature (&& let) will cause compilation errors on stable Rust toolchains. It is highly recommended to rewrite this using standard nested if let statements to ensure compatibility with stable Rust.
| if !self | |
| .connections | |
| .iter() | |
| .any(|connection| connection.id == self.active_connection) | |
| && let Some(first) = self.connections.first() | |
| { | |
| self.active_connection = first.id.clone(); | |
| } | |
| if !self | |
| .connections | |
| .iter() | |
| .any(|connection| connection.id == self.active_connection) | |
| { | |
| if let Some(first) = self.connections.first() { | |
| self.active_connection = first.id.clone(); | |
| } | |
| } |
| if !self | ||
| .connections | ||
| .iter() | ||
| .any(|connection| connection.id == self.active_connection) | ||
| && let Some(first) = self.connections.first() | ||
| { | ||
| self.active_connection = first.id.clone(); | ||
| } |
There was a problem hiding this comment.
Using the unstable let_chains feature (&& let) will cause compilation errors on stable Rust toolchains. It is highly recommended to rewrite this using standard nested if let statements to ensure compatibility with stable Rust.
| if !self | |
| .connections | |
| .iter() | |
| .any(|connection| connection.id == self.active_connection) | |
| && let Some(first) = self.connections.first() | |
| { | |
| self.active_connection = first.id.clone(); | |
| } | |
| if !self | |
| .connections | |
| .iter() | |
| .any(|connection| connection.id == self.active_connection) | |
| { | |
| if let Some(first) = self.connections.first() { | |
| self.active_connection = first.id.clone(); | |
| } | |
| } |
| if let Some(active) = self.active_connection_config().cloned() { | ||
| self.provider = active.provider; | ||
| self.endpoint = active.endpoint; | ||
| self.model = active.model; | ||
| self.api_key = active.api_key; | ||
| self.language = active.language; | ||
| } |
There was a problem hiding this comment.
Cloning the entire AsrConnection struct is unnecessary and inefficient because we only need to copy a subset of its fields to self. We can avoid this overhead by referencing the active connection directly and cloning only the required fields.
| if let Some(active) = self.active_connection_config().cloned() { | |
| self.provider = active.provider; | |
| self.endpoint = active.endpoint; | |
| self.model = active.model; | |
| self.api_key = active.api_key; | |
| self.language = active.language; | |
| } | |
| if let Some(active) = self.active_connection_config() { | |
| self.provider = active.provider.clone(); | |
| self.endpoint = active.endpoint.clone(); | |
| self.model = active.model.clone(); | |
| self.api_key = active.api_key.clone(); | |
| self.language = active.language.clone(); | |
| } |
| if let Some(active) = self.active_connection_config().cloned() { | ||
| self.provider = active.provider; | ||
| self.endpoint = active.endpoint; | ||
| self.model = active.model; | ||
| self.api_key = active.api_key; | ||
| } |
There was a problem hiding this comment.
Cloning the entire LlmConnection struct is unnecessary and inefficient because we only need to copy a subset of its fields to self. We can avoid this overhead by referencing the active connection directly and cloning only the required fields.
| if let Some(active) = self.active_connection_config().cloned() { | |
| self.provider = active.provider; | |
| self.endpoint = active.endpoint; | |
| self.model = active.model; | |
| self.api_key = active.api_key; | |
| } | |
| if let Some(active) = self.active_connection_config() { | |
| self.provider = active.provider.clone(); | |
| self.endpoint = active.endpoint.clone(); | |
| self.model = active.model.clone(); | |
| self.api_key = active.api_key.clone(); | |
| } |
1 participant
Summary
Closes #24
🤖 Generated with Claude Code