Unify OpenAI-compatible providers and custom provider flow#930
Conversation
📝 WalkthroughWalkthroughAdds provider-secret override state and queued commit/persistence flows to General settings, derives provider-aware display labels, replaces legacy per-provider API-key inputs with a provider-gated key UI, and implements selective session migration/rollback during override commits. ChangesGeneral settings — provider secret override
Estimated code review effort 🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Summary of ChangesHello @PeterDaveHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the extension's architecture for managing AI API providers. By unifying OpenAI-compatible API interactions and introducing a comprehensive provider registry with robust configuration migration, the changes aim to improve maintainability, simplify the addition of new providers, and enhance the user experience for custom API configurations. This also ensures better consistency in how API keys and endpoints are handled across the application. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||||
![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 significantly refactors the API mode handling by unifying OpenAI-compatible providers and introducing a custom provider flow. The changes centralize API logic, improve configuration management through schema versioning and migration, and enhance the UI for custom provider creation and selection. The refactoring reduces code duplication and sets a solid foundation for future extensibility. Overall, the changes are well-structured and address key areas for maintainability and user experience.
I am having trouble creating individual review comments. Click here to see my feedback.
src/background/index.mjs (437-458)
The removal of the isUsingCustomModel(session) block and its associated generateAnswersWithCustomApi calls is a direct result of the unification efforts. This simplifies the executeApi function by delegating custom API handling to the new generateAnswersWithOpenAICompatibleApi function, reducing conditional logic and improving code clarity.
src/background/index.mjs (566-568)
The introduction of isUsingOpenAICompatibleApiSession(session) and the call to generateAnswersWithOpenAICompatibleApi consolidates the logic for various OpenAI-compatible providers. This is a core part of the unification, making the executeApi function more streamlined and extensible. All previously separate API calls for OpenAI, Moonshot, ChatGLM, DeepSeek, Ollama, OpenRouter, and AIML are now routed through this single entry point.
src/config/index.mjs (550-556)
The addition of providerId, customOpenAIProviders, providerSecrets, and configSchemaVersion to the defaultConfig is crucial for the new configuration schema and migration system. providerId allows custom API modes to link to specific providers, while customOpenAIProviders and providerSecrets manage custom provider details and their API keys. configSchemaVersion enables future-proof migration logic.
src/config/index.mjs (729-733)
The getUserConfig function has been updated to incorporate the migrateUserConfig logic. This ensures that any legacy configurations are automatically migrated to the new schema upon loading, providing backward compatibility and consistency across user settings. The dirty flag correctly triggers a save if migration occurs.
src/popup/sections/ApiModes.jsx (31)
The addition of providerId to defaultApiMode is necessary to support the new provider registry system. This allows API modes to be explicitly linked to a specific provider, whether built-in or custom, enabling more granular control and clearer configuration.
src/popup/sections/ApiModes.jsx (98)
The useLayoutEffect now includes config.customOpenAIProviders as a dependency. This ensures that the UI re-renders and updates the list of custom providers whenever the underlying configuration for custom OpenAI providers changes, maintaining UI consistency with the backend data.
src/popup/sections/ApiModes.jsx (121-134)
The persistApiMode function is a new utility that centralizes the logic for saving API mode configurations. It handles updating customApiModes and optionally customOpenAIProviders, and ensures that the currently selected API mode is also updated if it's being edited. This improves code organization and reduces duplication.
src/popup/sections/ApiModes.jsx (136-195)
The onSaveEditing function has been completely refactored to support the new custom provider creation and selection flow. It now handles the creation of new providers, assigns provider IDs, and manages the clearing of API keys when switching providers. This is a critical piece of the custom provider workflow, ensuring data integrity and proper linking between API modes and providers.
src/popup/sections/ApiModes.jsx (267-288)
This new UI section allows users to select an existing custom provider or create a new one. The providerSelector state manages the selection, and the options are dynamically populated from customProviders. This is a key enhancement for the custom provider workflow, making it user-friendly.
src/popup/sections/ApiModes.jsx (290-305)
This new block provides input fields for creating a new custom provider, including its name and base URL. This directly supports the custom provider creation flow, allowing users to define new OpenAI-compatible endpoints directly within the UI.
src/popup/sections/GeneralPart.jsx (12)
The removal of isUsingOpenAiApiModel and isUsingChatGLMApiModel from imports reflects the consolidation of these checks into the new isUsingOpenAICompatibleProvider utility. This simplifies the import list and centralizes the logic for identifying OpenAI-compatible API usage.
src/popup/sections/GeneralPart.jsx (96-105)
The LEGACY_API_KEY_FIELD_BY_PROVIDER_ID constant maps provider IDs to their legacy API key field names in the configuration. This is essential for the migration and unified secret management, allowing the system to correctly identify and update API keys for various providers.
src/popup/sections/GeneralPart.jsx (107-181)
The buildProviderSecretUpdate function is a critical new utility for managing API keys. It handles updating providerSecrets and also ensures that legacy API key fields are updated for backward compatibility. Furthermore, it propagates API key changes to customApiModes and the apiMode if they are currently selected, ensuring consistency across the application. This function is vital for the new unified secret management system.
src/popup/sections/GeneralPart.jsx (197-203)
These new state variables and derived values (selectedProviderRequest, selectedProviderId, selectedProvider, selectedProviderApiKey, isUsingOpenAICompatibleProvider) are crucial for dynamically displaying and managing API keys for the currently selected OpenAI-compatible provider. They leverage the new provider-registry.mjs to resolve provider information.
src/popup/sections/GeneralPart.jsx (206-217)
The getBalance function has been updated to use the selectedProviderApiKey and openAiApiUrl derived from the new provider registry. This ensures that balance checks are performed against the correct API key and endpoint for the currently active OpenAI-compatible provider, centralizing the logic and making it more robust.
src/popup/sections/GeneralPart.jsx (275)
The condition isUsingOpenAICompatibleProvider replaces multiple individual checks for OpenAI-compatible models. This simplifies the conditional rendering logic for the API key input field, making it more concise and easier to understand.
src/popup/sections/GeneralPart.jsx (326-356)
This block now dynamically renders the API key input and balance check button based on whether an isUsingOpenAICompatibleProvider is selected. The selectedProviderApiKey and selectedProviderId are used to manage the input value and conditional rendering of the 'Get'/'Balance' buttons, centralizing API key management.
src/services/apis/custom-api.mjs (1)
The import statement has been simplified to only include generateAnswersWithOpenAICompatible. This is a direct consequence of unifying the OpenAI-compatible API execution into a shared core module, reducing the number of individual API service imports.
src/services/apis/custom-api.mjs (19-29)
The generateAnswersWithCustomApi function has been refactored to delegate its logic to the new generateAnswersWithOpenAICompatible function. This significantly reduces code duplication and centralizes the core logic for handling OpenAI-compatible API requests, including custom ones. The allowLegacyResponseField: true is important for maintaining backward compatibility with custom APIs that might use a response field.
src/services/apis/openai-api.mjs (3-4)
The import of generateAnswersWithOpenAICompatible and resolveOpenAICompatibleRequest from the new core and provider registry modules is key to unifying API handling. This allows openai-api.mjs to leverage the shared logic instead of duplicating it.
src/services/apis/openai-api.mjs (6-8)
The normalizeBaseUrl function is a useful utility to ensure consistency in API endpoint URLs by trimming trailing slashes. This helps prevent issues with URL construction and improves the robustness of API requests.
src/services/apis/openai-api.mjs (10-21)
The resolveModelName function centralizes the logic for determining the actual model name to be used in API requests, especially for custom models and API modes. This ensures that the correct model identifier is passed to the unified API handler.
src/services/apis/openai-api.mjs (24-40)
The touchOllamaKeepAlive function is a new addition specifically for Ollama providers. It sends a small request to the Ollama endpoint to keep the model loaded, which is important for performance and responsiveness. This demonstrates how provider-specific logic can be integrated while still using a unified API flow.
src/services/apis/openai-api.mjs (48-58)
The generateAnswersWithGptCompletionApi function has been refactored to use the new generateAnswersWithOpenAICompatible core function. This significantly reduces code duplication and centralizes the logic for handling completion-type API requests, improving maintainability.
src/services/apis/openai-api.mjs (69-70)
The baseUrl for generateAnswersWithChatgptApiCompat now uses normalizeBaseUrl, ensuring consistent URL formatting. This is a minor but good improvement for robustness.
src/services/apis/openai-api.mjs (89-99)
The generateAnswersWithChatgptApiCompat function has been refactored to use the new generateAnswersWithOpenAICompatible core function. This change eliminates duplicated SSE fetching logic and centralizes the handling of chat-completion type API requests, making the code cleaner and more maintainable.
src/services/apis/openai-api.mjs (109-133)
This new function, generateAnswersWithOpenAICompatibleApi, serves as the unified entry point for all OpenAI-compatible providers. It resolves the correct provider, endpoint type, URL, API key, and model using the new provider registry, then delegates to generateAnswersWithOpenAICompatible. This is the cornerstone of the PR's unification efforts, drastically simplifying the executeApi logic in background/index.mjs.
src/services/apis/openai-compatible-core.mjs (8-14)
The buildHeaders function centralizes the logic for constructing HTTP headers, including the Authorization header with the API key. This promotes consistency and reusability across all OpenAI-compatible API requests.
src/services/apis/openai-compatible-core.mjs (17-28)
The buildMessageAnswer function encapsulates the logic for extracting the answer content from different API response formats (delta, content, text, and legacy response field). This abstraction makes the SSE onMessage handler cleaner and more adaptable to various provider responses.
src/services/apis/openai-compatible-core.mjs (31-33)
The hasFinished function provides a clear and concise way to determine if an API response indicates the end of a stream, based on the finish_reason field. This improves readability in the SSE onMessage handler.
src/services/apis/openai-compatible-core.mjs (49-159)
This new generateAnswersWithOpenAICompatible function is the core of the unified API handling. It abstracts away the details of constructing request bodies for both 'completion' and 'chat' endpoints, handling token parameters, and managing the SSE connection. This significantly reduces code duplication across different OpenAI-compatible API services and makes it easier to add new providers in the future.
src/services/apis/provider-registry.mjs (4-13)
The LEGACY_KEY_BY_PROVIDER_ID constant is crucial for the migration process, mapping older configuration keys to the new provider IDs. This ensures that existing user configurations are correctly interpreted and migrated to the unified secret management system.
src/services/apis/provider-registry.mjs (15-85)
The BUILTIN_PROVIDER_TEMPLATE defines a standardized structure for all built-in OpenAI-compatible providers. This template includes essential information like ID, name, base URLs, and paths, which is fundamental for the new provider registry and unified API handling. It also includes allowLegacyResponseField for backward compatibility.
src/services/apis/provider-registry.mjs (88-98)
The OPENAI_COMPATIBLE_GROUP_TO_PROVIDER_ID mapping is vital for translating legacy API mode group names into the new standardized provider IDs. This enables the system to correctly identify which provider corresponds to a given API mode, facilitating the migration and unification.
src/services/apis/provider-registry.mjs (101-134)
The getModelNamePresetPart and resolveProviderIdFromLegacyModelName functions are essential for backward compatibility. They allow the system to infer the correct provider ID from older, less structured model names, ensuring that existing user configurations continue to work seamlessly with the new provider registry.
src/services/apis/provider-registry.mjs (136-139)
The isLegacyCompletionModelName function helps identify models that historically used the 'completion' endpoint. This is important for correctly routing requests to the appropriate endpoint type within the unified API handler.
src/services/apis/provider-registry.mjs (141-157)
The toStringOrEmpty, trimSlashes, ensureLeadingSlash, and joinUrl utility functions provide robust and consistent string manipulation for URLs and paths. These are crucial for correctly constructing API endpoints, especially when dealing with user-provided custom URLs and paths.
src/services/apis/provider-registry.mjs (160-184)
The buildBuiltinProviders function dynamically constructs the list of built-in providers, incorporating user-specific configuration values like customOpenAiApiUrl and ollamaEndpoint. This ensures that built-in providers are correctly configured based on user settings.
src/services/apis/provider-registry.mjs (186-201)
The normalizeCustomProvider function ensures that custom provider configurations are consistently structured and validated. It assigns default values and normalizes fields like id, name, baseUrl, and paths, which is essential for reliable custom provider management.
src/services/apis/provider-registry.mjs (203-208)
The getCustomOpenAIProviders function retrieves and normalizes custom provider configurations from the user's settings. This provides a clean, consistent list of custom providers for use throughout the application.
src/services/apis/provider-registry.mjs (210-212)
The getAllOpenAIProviders function combines both built-in and custom providers into a single, comprehensive list. This unified list is then used by other functions to resolve provider details.
src/services/apis/provider-registry.mjs (215-228)
The resolveProviderIdForSession function is crucial for determining the correct provider ID based on the current session's API mode or model name. It handles both new API mode structures and legacy model names, ensuring that the appropriate provider is identified for each request.
src/services/apis/provider-registry.mjs (230-238)
The resolveEndpointTypeForSession function determines whether a session should use a 'chat' or 'completion' endpoint. This is important for correctly formatting the request body in the unified API handler, especially for legacy completion models.
src/services/apis/provider-registry.mjs (240-246)
The getProviderById function retrieves a provider's full configuration by its ID. It filters out disabled providers, ensuring that only active and valid providers are used.
src/services/apis/provider-registry.mjs (248-268)
The getProviderSecret function centralizes the logic for retrieving API keys. It prioritizes API keys specified directly in the apiMode, then checks the new providerSecrets map, and finally falls back to legacy API key fields. This ensures that the correct API key is used for each provider, supporting both new and old configurations.
src/services/apis/provider-registry.mjs (270-301)
The resolveUrlFromProvider function constructs the full API request URL based on the provider's configuration and the endpoint type (chat or completion). It handles custom URLs specified in the API mode, as well as base URLs and paths defined in the provider template, ensuring accurate endpoint resolution.
src/services/apis/provider-registry.mjs (303-317)
The resolveOpenAICompatibleRequest function is the main entry point for obtaining all necessary information to make an OpenAI-compatible API request. It orchestrates the resolution of provider ID, provider details, endpoint type, request URL, and API key, providing a complete request object to the unified API handler.
src/services/init-session.mjs (5)
The import of normalizeApiMode is essential for ensuring that API mode objects are consistently structured and validated when a new session is initialized. This helps prevent unexpected behavior due to malformed API mode data.
src/services/init-session.mjs (75)
The apiMode property is now explicitly normalized using normalizeApiMode during session initialization. This ensures that all API mode objects conform to a consistent structure, which is critical for the new provider registry and unified API handling.
src/services/wrappers.mjs (13)
The import of normalizeApiMode is crucial here to ensure that any API mode object received or processed by the port listener is normalized. This guarantees consistency and prevents issues with potentially malformed API mode data.
src/services/wrappers.mjs (110)
The session.apiMode is now explicitly normalized using normalizeApiMode within the port listener. This ensures that any API mode passed into the executeApi function is in a consistent and expected format, which is vital for the new unified API handling logic.
src/utils/model-name-convert.mjs (81-93)
The new normalizeApiMode function ensures that API mode objects always have a consistent structure and default values. This is critical for reliable processing of API modes throughout the application, especially with the introduction of new fields like providerId and the migration logic.
src/utils/model-name-convert.mjs (97-98)
The apiModeToModelName function now explicitly normalizes the apiMode object at the beginning. This ensures that any subsequent logic in the function operates on a well-formed API mode object, improving robustness.
src/utils/model-name-convert.mjs (111-115)
The getApiModesFromConfig function now normalizes config.customApiModes before processing them. This ensures that custom API modes are always in a consistent format, which is important for accurate filtering and mapping.
src/utils/model-name-convert.mjs (132-133)
The logic for Ollama model names has been updated to explicitly set modelName to ollamaModel- followed by the configured Ollama model name. This ensures consistency in how Ollama models are identified within the system, aligning with the new provider registry.
src/utils/model-name-convert.mjs (139)
The normalizedCustomApiModes are now correctly filtered and included in the returned array. This ensures that all custom API modes, after normalization, are properly accounted for when retrieving the list of available API modes.
src/utils/model-name-convert.mjs (148-157)
The normalizeForCompare helper function is introduced to create a consistent string representation of API mode objects for comparison. This is crucial for accurately determining if an API mode is selected, especially with the addition of new fields like providerId and the need to ignore transient fields like apiKey and customUrl during comparison.
src/utils/model-name-convert.mjs (160)
The isApiModeSelected function now uses the normalizeForCompare helper to compare API mode objects. This ensures that comparisons are robust and accurate, preventing false negatives or positives due to differences in non-essential fields or object ordering.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
PeterDaveHello
force-pushed
the
refactor/openai-provider-platform
branch
from
36ed81e to
fdc0497
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/background/index.mjs (1)
8-8:⚠️ Potential issue | 🟠 MajorMissing
.mjsextension on theopenai-apiimport.This was flagged in a prior review and still needs the extension for runtime resolution consistency.
-import { generateAnswersWithOpenAICompatibleApi } from '../services/apis/openai-api' +import { generateAnswersWithOpenAICompatibleApi } from '../services/apis/openai-api.mjs'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/background/index.mjs` at line 8, The import statement for generateAnswersWithOpenAICompatibleApi is missing the .mjs extension; update the import in src/background/index.mjs to reference '../services/apis/openai-api.mjs' (i.e., change the module path used where generateAnswersWithOpenAICompatibleApi is imported) so the runtime can resolve the module consistently.src/popup/sections/GeneralPart.jsx (1)
96-105: Duplication of provider-to-key mapping remains — already flagged in prior review.
LEGACY_API_KEY_FIELD_BY_PROVIDER_IDhere andLEGACY_SECRET_KEY_TO_PROVIDER_IDinsrc/config/index.mjsexpress the same mapping in opposite directions. A shared constants module would keep them in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/popup/sections/GeneralPart.jsx` around lines 96 - 105, LEGACY_API_KEY_FIELD_BY_PROVIDER_ID duplicates the inverse mapping LEGACY_SECRET_KEY_TO_PROVIDER_ID; extract the canonical mapping into a new shared constants module (e.g., export LEGACY_API_KEY_FIELD_BY_PROVIDER_ID from a central module), import that constant into GeneralPart.jsx and update src/config's code to derive LEGACY_SECRET_KEY_TO_PROVIDER_ID from it (use Object.entries/Object.fromEntries to invert the mapping), and remove the hard-coded duplicate so both modules consume the single source of truth.src/services/apis/openai-api.mjs (1)
24-40: Missing timeout ontouchOllamaKeepAlivefetch — can block indefinitely.The
awaitat line 129 will hang if the Ollama endpoint is unresponsive. While the.catch()prevents unhandled rejections, the promise itself never resolves until the TCP stack gives up. Add anAbortControllerwith a reasonable timeout.🛡️ Proposed fix
async function touchOllamaKeepAlive(config, model, apiKey) { + const controller = new AbortController() + const timeout = setTimeout(() => controller.abort(), 5000) return fetch(`${normalizeBaseUrl(config.ollamaEndpoint)}/api/generate`, { method: 'POST', + signal: controller.signal, headers: { 'Content-Type': 'application/json', ...(apiKey ? { Authorization: `Bearer ${apiKey}` } : {}), }, body: JSON.stringify({ model, prompt: 't', options: { num_predict: 1, }, keep_alive: config.ollamaKeepAliveTime === '-1' ? -1 : config.ollamaKeepAliveTime, }), - }) + }).finally(() => clearTimeout(timeout)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/apis/openai-api.mjs` around lines 24 - 40, The fetch in touchOllamaKeepAlive can hang indefinitely; modify touchOllamaKeepAlive to create an AbortController, pass controller.signal to fetch, and set a timer (e.g., via setTimeout) to call controller.abort() after a reasonable timeout (e.g., 5-10s), clearing the timer on completion; ensure the fetch call uses the controller.signal and that any catch handles AbortError gracefully so the calling await won't hang.src/services/apis/provider-registry.mjs (1)
15-86: OpenAI template paths include/v1prefix while other providers don't — potential/v1duplication.The OpenAI provider template defines
chatCompletionsPath: '/v1/chat/completions'(line 19), while the OpenAIbaseUrlis set fromconfig.customOpenAiApiUrlinbuildBuiltinProviders(line 165). If a user has stored a base URL already ending in/v1(e.g.,https://api.openai.com/v1),joinUrlwill producehttps://api.openai.com/v1/v1/chat/completions.Strip a trailing
/v1from the OpenAI base URL before storing, or unify the path convention across all providers.🛠️ Proposed fix in buildBuiltinProviders
if (provider.id === 'openai') { + const rawUrl = trimSlashes(config.customOpenAiApiUrl || 'https://api.openai.com') return { ...provider, - baseUrl: trimSlashes(config.customOpenAiApiUrl || 'https://api.openai.com'), + baseUrl: rawUrl.replace(/\/v1$/i, ''), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/apis/provider-registry.mjs` around lines 15 - 86, BUILTIN_PROVIDER_TEMPLATE currently sets OpenAI's chatCompletionsPath to '/v1/chat/completions' which can duplicate '/v1' if buildBuiltinProviders uses a baseUrl that already ends with '/v1'; update buildBuiltinProviders to normalize the OpenAI base URL by stripping a trailing '/v1' or '/v1/' before saving (e.g., use baseUrl = config.customOpenAiApiUrl.replace(/\/v1\/?$/, '') when building the provider entry for id 'openai'), or alternatively change the OpenAI template path to '/chat/completions' so all providers use the same path convention; adjust the code that creates the OpenAI provider (referencing BUILTIN_PROVIDER_TEMPLATE and buildBuiltinProviders) to perform the normalization.
🧹 Nitpick comments (8)
src/config/index.mjs (2)
740-751: Minor:aimlModelKeysandaimlApiModelKeysboth map to'aiml'.
ModelGroupsat line 197 uses the keyaimlModelKeys(notaimlApiModelKeys). The extra entry is harmless (belt-and-suspenders), but if it's intentional for forward-compatibility, a comment would help. Otherwise it's dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/index.mjs` around lines 740 - 751, The mapping API_MODE_GROUP_TO_PROVIDER_ID currently contains both aimlModelKeys and aimlApiModelKeys pointing to 'aiml'; remove the redundant key aimlApiModelKeys (or add a brief clarifying comment if the duplicate is intentional for future compatibility) so only the canonical key used by ModelGroups (aimlModelKeys) remains, updating the object API_MODE_GROUP_TO_PROVIDER_ID to eliminate dead code while preserving behavior.
796-1101: Migration logic is thorough but consider extracting shared apiMode migration into a helper.The migration for
customApiModes(lines 879-970) andselectedApiMode(lines 973-1086) share significant structural similarity — both normalize provider IDs, create providers from legacy custom URLs, and migrate API keys toproviderSecrets. This isn't urgent since the migration runs once per config version bump, but extracting a sharedmigrateApiModeEntry(apiMode, context)helper would reduce the ~100 lines of duplication and lower the risk of one branch drifting from the other in future migrations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/index.mjs` around lines 796 - 1101, The customApiModes loop and selectedApiMode handling in migrateUserConfig share repeated logic; extract that shared logic into a helper named migrateApiModeEntry(apiMode, {providerSecrets, customOpenAIProviders, providerIdSet, providerIdRenameLookup, customProviderCounter, LEGACY_CUSTOM_PROVIDER_SECRET, API_MODE_GROUP_TO_PROVIDER_ID}) that normalizes providerId, applies renames, creates new provider entries from customUrl (updating providerIdSet, customOpenAIProviders and customProviderCounter), moves apiKey into providerSecrets, clears apiMode.customUrl/apiKey/providerId as appropriate, and returns {updatedApiMode, providerIdAssignedFromLegacyCustomUrl, customProviderCounter, dirtyFlags} so migrateUserConfig can call it for each entry in customApiModes and once for selectedApiMode (merging returned updates and flags) to remove the duplicated ~100 lines while preserving existing side-effects on providerSecrets, customOpenAIProviders, providerIdSet, and dirty/custom*Dirty flags.src/services/apis/openai-compatible-core.mjs (2)
8-15:extraHeaderscan be silently overridden whenapiKeyis provided.If a caller passes
AuthorizationinsideextraHeadersand providesapiKey, the spread on line 11 setsAuthorizationfirst, then line 13 unconditionally overwrites it. This is likely the desired precedence, but it's implicit. A small comment or an explicitdeletewould prevent surprises for custom provider integrations that may need a non-Bearer auth header.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/apis/openai-compatible-core.mjs` around lines 8 - 15, The buildHeaders function currently allows an Authorization in extraHeaders to be silently overwritten when apiKey is provided; make this precedence explicit by removing any Authorization from extraHeaders before merging (e.g., delete extraHeaders.Authorization) when you intend apiKey to win, or alternatively check for an existing Authorization and only set headers.Authorization = `Bearer ${apiKey}` if none exists — update the buildHeaders function and add a brief comment clarifying the chosen precedence so custom providers aren’t surprised.
109-117: Partial answers are silently discarded when the stream ends without afinish_reason.When
onEndfires andfinishedis false (line 148), only{ done: true }is posted —finish()is never called, sopushRecordis skipped and the partial answer is lost. This appears intentional based on the corresponding test (emits fallback done message when stream ends without finish reason), but it means any accumulatedanswertext from intermediate deltas is dropped from the conversation history.If this is by design (don't persist incomplete answers), a brief comment would help future readers understand the intent.
async onEnd() { - if (!finished) port.postMessage({ done: true }) + // Stream ended without a finish_reason — notify the UI but do not + // persist the partial answer into the conversation history. + if (!finished) port.postMessage({ done: true })Also applies to: 146-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/apis/openai-compatible-core.mjs` around lines 109 - 117, The onEnd handler currently posts a fallback done message without calling finish(), so any accumulated partial answer in the answer variable is never persisted via pushRecord; update the onEnd logic to call finish() whenever finished is false (ensuring finish() uses pushRecord(session, question, answer) and sends the final port.postMessage with done:true and session) so partial deltas are saved, or if dropping partial answers is intentional add a concise comment near finish()/onEnd explaining the design decision; reference finish, pushRecord, answer, onEnd and port.postMessage when making the change.src/services/apis/provider-registry.mjs (3)
249-269:getProviderSecretcascade is well-layered but the final fallback (line 268) may surface stale keys.The priority is: (1) apiMode.apiKey for
customApiModelKeys, (2)providerSecretsmap, (3) legacy config key, (4) apiMode.apiKey as last resort for all groups. The last fallback means an oldapiMode.apiKeyvalue embedded in a session could override a rotated key inproviderSecretsor legacy config — but only if both are empty. This is a reasonable last-resort behavior; just document that the apiMode-embedded key is the weakest source.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/apis/provider-registry.mjs` around lines 249 - 269, getProviderSecret currently falls back to session.apiMode.apiKey as the weakest source after checking providerSecrets and legacy keys, which can surface stale embedded keys; update the function (getProviderSecret) to include a concise inline comment or docstring clarifying the priority order (apiMode.customApiModelKeys -> providerSecrets map -> LEGACY_KEY_BY_PROVIDER_ID lookup -> session.apiMode.apiKey as last resort) and explicitly note that the final fallback (session.apiMode.apiKey) is the weakest/least-authoritative source and may be stale so should not override rotated keys in providerSecrets or legacy config.
241-247:getProviderByIdsilently returnsnullfor disabled providers — callers should be aware.Line 245 returns
nullifprovider.enabled === false. This meansresolveOpenAICompatibleRequestwill returnnull, and the caller inopenai-api.mjswill throw'Unknown OpenAI-compatible provider configuration'. The error message doesn't distinguish "provider not found" from "provider disabled," which could confuse users troubleshooting.💡 Consider returning a richer error or distinct null reasons
One option is to have
getProviderByIdreturn a status object, or haveresolveOpenAICompatibleRequestproduce a more descriptive error reason:export function resolveOpenAICompatibleRequest(config, session) { const providerId = resolveProviderIdForSession(session) if (!providerId) return null const provider = getProviderById(config, providerId) - if (!provider) return null + if (!provider) { + const allProviders = getAllOpenAIProviders(config) + const disabled = allProviders.find((p) => p.id === providerId && p.enabled === false) + if (disabled) return { error: `Provider '${providerId}' is disabled` } + return null + }The caller would then need to check for
.error— but this is optional depending on how much UX polish is desired.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/apis/provider-registry.mjs` around lines 241 - 247, getProviderById currently returns null for both "not found" and "disabled" cases which makes callers like resolveOpenAICompatibleRequest unable to distinguish why resolution failed; change getProviderById to return a richer result (e.g. { provider } on success, { error: 'not_found' } or { error: 'disabled' } when appropriate) or throw a descriptive error, then update resolveOpenAICompatibleRequest (and callers such as openai-api.mjs) to handle the new shape/error and produce a clearer message like "provider not found" vs "provider disabled"; reference the getProviderById and resolveOpenAICompatibleRequest symbols when making these changes.
101-134:resolveProviderIdFromLegacyModelName— thorough legacy mapping, one subtle gap.Line 130 uses
preset.startsWith('chatglm')which would also match a hypothetical model starting withchatglmthat isn't actually ChatGLM-related. Given the bounded set of known model names in this extension, the practical risk is negligible, but it's worth noting the loose match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/apis/provider-registry.mjs` around lines 101 - 134, The loose match in resolveProviderIdFromLegacyModelName uses preset.startsWith('chatglm') which can incorrectly match unrelated names; tighten the condition by matching the known ChatGLM tokens only (for example check preset === 'chatglm' OR preset.startsWith('chatglm_') OR preset === 'chatglmApiModelKeys') so it only identifies legitimate ChatGLM presets; update the condition near the other provider checks (see resolveProviderIdFromLegacyModelName and getModelNamePresetPart) to use these stricter comparisons.src/services/apis/openai-api.mjs (1)
6-8:normalizeBaseUrlduplicatestrimSlashesin provider-registry.mjs.Both
normalizeBaseUrlhere andtrimSlashesinprovider-registry.mjsstrip trailing slashes from a string. Consider importingtrimSlashes(or exporting a shared helper) instead of maintaining a parallel implementation that could drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/apis/openai-api.mjs` around lines 6 - 8, Replace the duplicate trailing-slash logic in normalizeBaseUrl by reusing the existing trimSlashes helper: remove the local normalizeBaseUrl function and import trimSlashes (from the module that currently exports it) into this module, then call trimSlashes wherever normalizeBaseUrl was used; alternatively, if trimSlashes is not exported, export it from provider-registry.mjs and update imports so openai-api.mjs uses that shared helper instead of duplicating the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/apis/provider-registry.mjs`:
- Around line 186-201: The fallback ID in normalizeCustomProvider currently uses
the unstable index-based `custom-provider-${index + 1}` which will change when
array order changes and break lookups (e.g., providerSecrets and
session.apiMode.providerId); change normalizeCustomProvider to produce a stable
fallback ID when provider.id is missing by deriving it deterministically (for
example, compute a short hash from stable fields like name + baseUrl) or by
using a persistent UUID assigned at provider creation time, and update any
creation flow so newly created providers get that stable id; reference
normalizeCustomProvider and ensure code paths that persist/lookup provider IDs
(providerSecrets, session.apiMode.providerId) use the new stable id scheme.
---
Duplicate comments:
In `@src/background/index.mjs`:
- Line 8: The import statement for generateAnswersWithOpenAICompatibleApi is
missing the .mjs extension; update the import in src/background/index.mjs to
reference '../services/apis/openai-api.mjs' (i.e., change the module path used
where generateAnswersWithOpenAICompatibleApi is imported) so the runtime can
resolve the module consistently.
In `@src/popup/sections/GeneralPart.jsx`:
- Around line 96-105: LEGACY_API_KEY_FIELD_BY_PROVIDER_ID duplicates the inverse
mapping LEGACY_SECRET_KEY_TO_PROVIDER_ID; extract the canonical mapping into a
new shared constants module (e.g., export LEGACY_API_KEY_FIELD_BY_PROVIDER_ID
from a central module), import that constant into GeneralPart.jsx and update
src/config's code to derive LEGACY_SECRET_KEY_TO_PROVIDER_ID from it (use
Object.entries/Object.fromEntries to invert the mapping), and remove the
hard-coded duplicate so both modules consume the single source of truth.
In `@src/services/apis/openai-api.mjs`:
- Around line 24-40: The fetch in touchOllamaKeepAlive can hang indefinitely;
modify touchOllamaKeepAlive to create an AbortController, pass controller.signal
to fetch, and set a timer (e.g., via setTimeout) to call controller.abort()
after a reasonable timeout (e.g., 5-10s), clearing the timer on completion;
ensure the fetch call uses the controller.signal and that any catch handles
AbortError gracefully so the calling await won't hang.
In `@src/services/apis/provider-registry.mjs`:
- Around line 15-86: BUILTIN_PROVIDER_TEMPLATE currently sets OpenAI's
chatCompletionsPath to '/v1/chat/completions' which can duplicate '/v1' if
buildBuiltinProviders uses a baseUrl that already ends with '/v1'; update
buildBuiltinProviders to normalize the OpenAI base URL by stripping a trailing
'/v1' or '/v1/' before saving (e.g., use baseUrl =
config.customOpenAiApiUrl.replace(/\/v1\/?$/, '') when building the provider
entry for id 'openai'), or alternatively change the OpenAI template path to
'/chat/completions' so all providers use the same path convention; adjust the
code that creates the OpenAI provider (referencing BUILTIN_PROVIDER_TEMPLATE and
buildBuiltinProviders) to perform the normalization.
---
Nitpick comments:
In `@src/config/index.mjs`:
- Around line 740-751: The mapping API_MODE_GROUP_TO_PROVIDER_ID currently
contains both aimlModelKeys and aimlApiModelKeys pointing to 'aiml'; remove the
redundant key aimlApiModelKeys (or add a brief clarifying comment if the
duplicate is intentional for future compatibility) so only the canonical key
used by ModelGroups (aimlModelKeys) remains, updating the object
API_MODE_GROUP_TO_PROVIDER_ID to eliminate dead code while preserving behavior.
- Around line 796-1101: The customApiModes loop and selectedApiMode handling in
migrateUserConfig share repeated logic; extract that shared logic into a helper
named migrateApiModeEntry(apiMode, {providerSecrets, customOpenAIProviders,
providerIdSet, providerIdRenameLookup, customProviderCounter,
LEGACY_CUSTOM_PROVIDER_SECRET, API_MODE_GROUP_TO_PROVIDER_ID}) that normalizes
providerId, applies renames, creates new provider entries from customUrl
(updating providerIdSet, customOpenAIProviders and customProviderCounter), moves
apiKey into providerSecrets, clears apiMode.customUrl/apiKey/providerId as
appropriate, and returns {updatedApiMode, providerIdAssignedFromLegacyCustomUrl,
customProviderCounter, dirtyFlags} so migrateUserConfig can call it for each
entry in customApiModes and once for selectedApiMode (merging returned updates
and flags) to remove the duplicated ~100 lines while preserving existing
side-effects on providerSecrets, customOpenAIProviders, providerIdSet, and
dirty/custom*Dirty flags.
In `@src/services/apis/openai-api.mjs`:
- Around line 6-8: Replace the duplicate trailing-slash logic in
normalizeBaseUrl by reusing the existing trimSlashes helper: remove the local
normalizeBaseUrl function and import trimSlashes (from the module that currently
exports it) into this module, then call trimSlashes wherever normalizeBaseUrl
was used; alternatively, if trimSlashes is not exported, export it from
provider-registry.mjs and update imports so openai-api.mjs uses that shared
helper instead of duplicating the implementation.
In `@src/services/apis/openai-compatible-core.mjs`:
- Around line 8-15: The buildHeaders function currently allows an Authorization
in extraHeaders to be silently overwritten when apiKey is provided; make this
precedence explicit by removing any Authorization from extraHeaders before
merging (e.g., delete extraHeaders.Authorization) when you intend apiKey to win,
or alternatively check for an existing Authorization and only set
headers.Authorization = `Bearer ${apiKey}` if none exists — update the
buildHeaders function and add a brief comment clarifying the chosen precedence
so custom providers aren’t surprised.
- Around line 109-117: The onEnd handler currently posts a fallback done message
without calling finish(), so any accumulated partial answer in the answer
variable is never persisted via pushRecord; update the onEnd logic to call
finish() whenever finished is false (ensuring finish() uses pushRecord(session,
question, answer) and sends the final port.postMessage with done:true and
session) so partial deltas are saved, or if dropping partial answers is
intentional add a concise comment near finish()/onEnd explaining the design
decision; reference finish, pushRecord, answer, onEnd and port.postMessage when
making the change.
In `@src/services/apis/provider-registry.mjs`:
- Around line 249-269: getProviderSecret currently falls back to
session.apiMode.apiKey as the weakest source after checking providerSecrets and
legacy keys, which can surface stale embedded keys; update the function
(getProviderSecret) to include a concise inline comment or docstring clarifying
the priority order (apiMode.customApiModelKeys -> providerSecrets map ->
LEGACY_KEY_BY_PROVIDER_ID lookup -> session.apiMode.apiKey as last resort) and
explicitly note that the final fallback (session.apiMode.apiKey) is the
weakest/least-authoritative source and may be stale so should not override
rotated keys in providerSecrets or legacy config.
- Around line 241-247: getProviderById currently returns null for both "not
found" and "disabled" cases which makes callers like
resolveOpenAICompatibleRequest unable to distinguish why resolution failed;
change getProviderById to return a richer result (e.g. { provider } on success,
{ error: 'not_found' } or { error: 'disabled' } when appropriate) or throw a
descriptive error, then update resolveOpenAICompatibleRequest (and callers such
as openai-api.mjs) to handle the new shape/error and produce a clearer message
like "provider not found" vs "provider disabled"; reference the getProviderById
and resolveOpenAICompatibleRequest symbols when making these changes.
- Around line 101-134: The loose match in resolveProviderIdFromLegacyModelName
uses preset.startsWith('chatglm') which can incorrectly match unrelated names;
tighten the condition by matching the known ChatGLM tokens only (for example
check preset === 'chatglm' OR preset.startsWith('chatglm_') OR preset ===
'chatglmApiModelKeys') so it only identifies legitimate ChatGLM presets; update
the condition near the other provider checks (see
resolveProviderIdFromLegacyModelName and getModelNamePresetPart) to use these
stricter comparisons.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/background/index.mjssrc/config/index.mjssrc/popup/sections/ApiModes.jsxsrc/popup/sections/GeneralPart.jsxsrc/services/apis/custom-api.mjssrc/services/apis/openai-api.mjssrc/services/apis/openai-compatible-core.mjssrc/services/apis/provider-registry.mjssrc/services/init-session.mjssrc/services/wrappers.mjssrc/utils/model-name-convert.mjstests/unit/services/apis/openai-api-compat.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/init-session.mjs
| function normalizeCustomProvider(provider, index) { | ||
| if (!provider || typeof provider !== 'object') return null | ||
| const id = toStringOrEmpty(provider.id).trim() || `custom-provider-${index + 1}` | ||
| return { | ||
| id, | ||
| name: toStringOrEmpty(provider.name).trim() || `Custom Provider ${index + 1}`, | ||
| baseUrl: trimSlashes(provider.baseUrl), | ||
| chatCompletionsPath: ensureLeadingSlash(provider.chatCompletionsPath, DEFAULT_CHAT_PATH), | ||
| completionsPath: ensureLeadingSlash(provider.completionsPath, DEFAULT_COMPLETION_PATH), | ||
| chatCompletionsUrl: toStringOrEmpty(provider.chatCompletionsUrl).trim(), | ||
| completionsUrl: toStringOrEmpty(provider.completionsUrl).trim(), | ||
| builtin: false, | ||
| enabled: provider.enabled !== false, | ||
| allowLegacyResponseField: Boolean(provider.allowLegacyResponseField), | ||
| } | ||
| } |
There was a problem hiding this comment.
Index-based fallback ID for custom providers is fragile.
Line 188 generates custom-provider-${index + 1} when provider.id is empty. Since the index depends on array position, reordering or removing providers will shift IDs, breaking providerSecrets lookups and persisted session.apiMode.providerId references. Consider generating a stable ID (e.g., hash of name+baseUrl, or a UUID at creation time) when the ID is missing.
#!/bin/bash
# Check how custom providers are created to see if IDs are assigned at creation
rg -n -C5 'customOpenAIProviders' --type=js -g '!node_modules/**'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/apis/provider-registry.mjs` around lines 186 - 201, The fallback
ID in normalizeCustomProvider currently uses the unstable index-based
`custom-provider-${index + 1}` which will change when array order changes and
break lookups (e.g., providerSecrets and session.apiMode.providerId); change
normalizeCustomProvider to produce a stable fallback ID when provider.id is
missing by deriving it deterministically (for example, compute a short hash from
stable fields like name + baseUrl) or by using a persistent UUID assigned at
provider creation time, and update any creation flow so newly created providers
get that stable id; reference normalizeCustomProvider and ensure code paths that
persist/lookup provider IDs (providerSecrets, session.apiMode.providerId) use
the new stable id scheme.
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
PeterDaveHello
force-pushed
the
refactor/openai-provider-platform
branch
from
fdc0497 to
407d734
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { controller, messageListener, disconnectListener } = setAbortController(port) | ||
| const config = await getUserConfig() | ||
|
|
||
| let requestBody |
There was a problem hiding this comment.
generateAnswersWithOpenAICompatibleApi already receives a fully-loaded config, but generateAnswersWithOpenAICompatible calls getUserConfig() again to read token/temperature/context settings. This adds an extra storage read (and potential migration write) per request and can lead to inconsistencies if callers pass a config snapshot. Consider passing the needed config fields into generateAnswersWithOpenAICompatible (or accepting config as a parameter) instead of reloading from storage.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 407d734e9e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
PeterDaveHello
force-pushed
the
refactor/openai-provider-platform
branch
from
407d734 to
3b4527c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
PeterDaveHello
force-pushed
the
refactor/openai-provider-platform
branch
from
3b4527c to
2cdb121
Compare
|
@codex review |
PeterDaveHello
force-pushed
the
refactor/openai-provider-platform
branch
from
6dd73c3 to
a2dde36
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/services/apis/provider-registry.mjs (2)
286-316:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftIndex-based fallback ID for custom providers is still unstable.
custom-provider-${index + 1}depends on array position, so reordering or removing entries will shift IDs and breakproviderSecretslookups as well as persistedsession.apiMode.providerIdreferences on next normalization. Generate a stable identifier (e.g., a UUID at provider-creation time, or a deterministic hash of stable fields) whenprovider.idis missing, and ensure the creation flow assigns it before persisting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/apis/provider-registry.mjs` around lines 286 - 316, normalizeCustomProvider currently falls back to an unstable index-based id (`custom-provider-${index + 1}`) which breaks providerSecrets and persisted session.apiMode.providerId when array order changes; replace this with a stable id generation strategy: if provider.id is missing, generate and assign a stable identifier (e.g., a UUID created at provider-creation time or a deterministic hash of stable fields) before calling normalizeCustomProvider so that the normalized object uses that stable id instead of an index-dependent string; update the provider creation/persist flow to save that generated id to storage (so providerSecrets and session.apiMode.providerId can reliably reference it) and ensure normalizeCustomProvider continues to use the provider.id when present.
621-633:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent config sources for
legacy-custom-defaultfallback.The completion branch derives the base URL from
config.customOpenAiApiUrl(defaulthttps://api.openai.com), while the chat branch readsconfig.customModelApiUrl(defaulthttp://localhost:8000/...). For a user whose custom endpoint is configured only incustomModelApiUrl, completion requests will silently fall back to OpenAI instead of the user's endpoint. Either honorcustomModelApiUrlfor both endpoint types or document the asymmetry explicitly.🛠️ Suggested alignment
if (provider.id === 'legacy-custom-default') { if (endpointType === 'completion') { - const baseUrl = normalizeBaseUrlWithoutVersionSuffix( - config.customOpenAiApiUrl, - 'https://api.openai.com', - ) - return `${baseUrl}/v1/completions` + const baseUrl = normalizeBaseUrlWithoutVersionSuffix( + config.customModelApiUrl, + 'http://localhost:8000', + ) + return `${baseUrl}/v1/completions` } return ( toStringOrEmpty(config.customModelApiUrl).trim() || 'http://localhost:8000/v1/chat/completions' ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/apis/provider-registry.mjs` around lines 621 - 633, The completion branch for provider.id 'legacy-custom-default' inconsistently uses config.customOpenAiApiUrl (via normalizeBaseUrlWithoutVersionSuffix) while the chat branch uses config.customModelApiUrl, causing completions to fall back to OpenAI when a user only set customModelApiUrl; update the logic in the provider.id === 'legacy-custom-default' block (and the normalizeBaseUrlWithoutVersionSuffix usage) so both endpoint types prefer config.customModelApiUrl when present and only then fall back to config.customOpenAiApiUrl (or the existing defaults), ensuring completion and chat resolve the same custom endpoint source.
🧹 Nitpick comments (1)
src/services/apis/provider-registry.mjs (1)
463-553: ⚡ Quick winConsolidate the three legacy-URL matchers to remove duplicated matching logic.
getCustomProvidersMatchedByLegacySessionUrl,hasEnabledCustomProviderMatchByLegacySessionUrl, andhasDisabledCustomProviderMatchByLegacySessionUrlreimplement the samecustomUrlcomparison three times.hasEnabled...is functionallygetCustomProvidersMatchedByLegacySessionUrl(...).length > 0, andhasDisabled...only differs by the enabled filter. Extracting a shared predicate keeps the URL-comparison rules in one place and avoids drift if the matching rules evolve.♻️ Proposed refactor
+function matchesLegacyCustomUrl(item, customUrl) { + const directChatCompletionsUrl = normalizeEndpointUrlForCompare(item.chatCompletionsUrl) + if (directChatCompletionsUrl) return directChatCompletionsUrl === customUrl + const derivedChatCompletionsUrl = + item.baseUrl && item.chatCompletionsPath + ? normalizeEndpointUrlForCompare(joinUrl(item.baseUrl, item.chatCompletionsPath)) + : '' + return Boolean(derivedChatCompletionsUrl) && derivedChatCompletionsUrl === customUrl +} + function getCustomProvidersMatchedByLegacySessionUrl(customProviders, session) { const customUrl = normalizeEndpointUrlForCompare(session?.apiMode?.customUrl) if (!customUrl) return [] - - return customProviders.filter((item) => { - if (item.enabled === false) return false - const directChatCompletionsUrl = normalizeEndpointUrlForCompare(item.chatCompletionsUrl) - if (directChatCompletionsUrl && directChatCompletionsUrl === customUrl) return true - if (directChatCompletionsUrl) return false - const derivedChatCompletionsUrl = - item.baseUrl && item.chatCompletionsPath - ? normalizeEndpointUrlForCompare(joinUrl(item.baseUrl, item.chatCompletionsPath)) - : '' - return derivedChatCompletionsUrl && derivedChatCompletionsUrl === customUrl - }) + return customProviders.filter( + (item) => item.enabled !== false && matchesLegacyCustomUrl(item, customUrl), + ) } @@ -function hasDisabledCustomProviderMatchByLegacySessionUrl(customProviders, session) { - const customUrl = normalizeEndpointUrlForCompare(session?.apiMode?.customUrl) - if (!customUrl) return false - return customProviders.some((item) => { - if (item.enabled !== false) return false - /* duplicated logic */ - }) -} +function hasDisabledCustomProviderMatchByLegacySessionUrl(customProviders, session) { + const customUrl = normalizeEndpointUrlForCompare(session?.apiMode?.customUrl) + if (!customUrl) return false + return customProviders.some( + (item) => item.enabled === false && matchesLegacyCustomUrl(item, customUrl), + ) +} @@ -function hasEnabledCustomProviderMatchByLegacySessionUrl(customProviders, session) { - /* full duplicated logic */ -} +function hasEnabledCustomProviderMatchByLegacySessionUrl(customProviders, session) { + return getCustomProvidersMatchedByLegacySessionUrl(customProviders, session).length > 0 +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/apis/provider-registry.mjs` around lines 463 - 553, The three functions getCustomProvidersMatchedByLegacySessionUrl, hasEnabledCustomProviderMatchByLegacySessionUrl, and hasDisabledCustomProviderMatchByLegacySessionUrl duplicate the same URL-comparison logic; extract a single helper predicate (e.g., matchesProviderLegacySessionUrl(item, session)) that encapsulates normalizeEndpointUrlForCompare(item.chatCompletionsUrl), the derivedChatCompletionsUrl via joinUrl(item.baseUrl, item.chatCompletionsPath), and the session customUrl comparison, plus an enabled check parameter when needed; then implement getCustomProvidersMatchedByLegacySessionUrl to filter using that predicate, implement hasEnabledCustomProviderMatchByLegacySessionUrl as checking matchedProviders.length > 0 (or using predicate with enabled-only), and implement hasDisabledCustomProviderMatchByLegacySessionUrl using the predicate with enabled=false (or by filtering matchedProviders for enabled===false), removing the duplicated comparison code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/_locales/in/main.json`:
- Line 213: The translation string "This API key is set on the selected custom
mode. Editing it here will create a dedicated provider for that mode." uses the
English word "provider"; update the Indonesian value to replace "provider" with
"penyedia" so it matches other occurrences in the file (e.g., the value for the
same phrasing and nearby keys) by editing the value for that exact source string
in src/_locales/in/main.json to read "...akan membuat penyedia khusus untuk mode
tersebut.".
---
Duplicate comments:
In `@src/services/apis/provider-registry.mjs`:
- Around line 286-316: normalizeCustomProvider currently falls back to an
unstable index-based id (`custom-provider-${index + 1}`) which breaks
providerSecrets and persisted session.apiMode.providerId when array order
changes; replace this with a stable id generation strategy: if provider.id is
missing, generate and assign a stable identifier (e.g., a UUID created at
provider-creation time or a deterministic hash of stable fields) before calling
normalizeCustomProvider so that the normalized object uses that stable id
instead of an index-dependent string; update the provider creation/persist flow
to save that generated id to storage (so providerSecrets and
session.apiMode.providerId can reliably reference it) and ensure
normalizeCustomProvider continues to use the provider.id when present.
- Around line 621-633: The completion branch for provider.id
'legacy-custom-default' inconsistently uses config.customOpenAiApiUrl (via
normalizeBaseUrlWithoutVersionSuffix) while the chat branch uses
config.customModelApiUrl, causing completions to fall back to OpenAI when a user
only set customModelApiUrl; update the logic in the provider.id ===
'legacy-custom-default' block (and the normalizeBaseUrlWithoutVersionSuffix
usage) so both endpoint types prefer config.customModelApiUrl when present and
only then fall back to config.customOpenAiApiUrl (or the existing defaults),
ensuring completion and chat resolve the same custom endpoint source.
---
Nitpick comments:
In `@src/services/apis/provider-registry.mjs`:
- Around line 463-553: The three functions
getCustomProvidersMatchedByLegacySessionUrl,
hasEnabledCustomProviderMatchByLegacySessionUrl, and
hasDisabledCustomProviderMatchByLegacySessionUrl duplicate the same
URL-comparison logic; extract a single helper predicate (e.g.,
matchesProviderLegacySessionUrl(item, session)) that encapsulates
normalizeEndpointUrlForCompare(item.chatCompletionsUrl), the
derivedChatCompletionsUrl via joinUrl(item.baseUrl, item.chatCompletionsPath),
and the session customUrl comparison, plus an enabled check parameter when
needed; then implement getCustomProvidersMatchedByLegacySessionUrl to filter
using that predicate, implement hasEnabledCustomProviderMatchByLegacySessionUrl
as checking matchedProviders.length > 0 (or using predicate with enabled-only),
and implement hasDisabledCustomProviderMatchByLegacySessionUrl using the
predicate with enabled=false (or by filtering matchedProviders for
enabled===false), removing the duplicated comparison code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d678ab59-f01e-4728-b5ee-6feae7b568d9
📒 Files selected for processing (54)
src/_locales/de/main.jsonsrc/_locales/en/main.jsonsrc/_locales/es/main.jsonsrc/_locales/fr/main.jsonsrc/_locales/in/main.jsonsrc/_locales/it/main.jsonsrc/_locales/ja/main.jsonsrc/_locales/ko/main.jsonsrc/_locales/pt/main.jsonsrc/_locales/ru/main.jsonsrc/_locales/tr/main.jsonsrc/_locales/zh-hans/main.jsonsrc/_locales/zh-hant/main.jsonsrc/background/index.mjssrc/components/ConversationCard/index.jsxsrc/config/index.mjssrc/config/openai-provider-mappings.mjssrc/popup/Popup.jsxsrc/popup/popup-config-utils.mjssrc/popup/sections/AdvancedPart.jsxsrc/popup/sections/ApiModes.jsxsrc/popup/sections/GeneralPart.jsxsrc/popup/sections/api-modes-provider-utils.mjssrc/popup/sections/general-balance-utils.mjssrc/popup/sections/general-provider-override-utils.mjssrc/popup/sections/provider-secret-utils.mjssrc/services/apis/aiml-api.mjssrc/services/apis/chatglm-api.mjssrc/services/apis/custom-api.mjssrc/services/apis/deepseek-api.mjssrc/services/apis/moonshot-api.mjssrc/services/apis/ollama-api.mjssrc/services/apis/openai-api.mjssrc/services/apis/openai-compatible-core.mjssrc/services/apis/openrouter-api.mjssrc/services/apis/provider-registry.mjssrc/services/init-session.mjssrc/services/wrappers.mjssrc/utils/model-name-convert.mjstests/unit/config/migrate-user-config.test.mjstests/unit/config/user-config.test.mjstests/unit/popup/api-modes-provider-utils.test.mjstests/unit/popup/api-modes.test.mjstests/unit/popup/general-balance-utils.test.mjstests/unit/popup/general-part-override.test.mjstests/unit/popup/popup-config-update.test.mjstests/unit/popup/provider-secret-utils.test.mjstests/unit/services/apis/custom-api.test.mjstests/unit/services/apis/openai-api-compat.test.mjstests/unit/services/apis/provider-registry.test.mjstests/unit/services/apis/thin-adapters.test.mjstests/unit/services/handle-port-error.test.mjstests/unit/services/wrappers-register.test.mjstests/unit/utils/model-name-convert.test.mjs
💤 Files with no reviewable changes (6)
- src/services/apis/aiml-api.mjs
- src/services/apis/chatglm-api.mjs
- src/services/apis/moonshot-api.mjs
- src/services/apis/openrouter-api.mjs
- src/services/apis/deepseek-api.mjs
- src/services/apis/ollama-api.mjs
✅ Files skipped from review due to trivial changes (14)
- tests/unit/popup/api-modes.test.mjs
- src/_locales/de/main.json
- src/config/openai-provider-mappings.mjs
- src/_locales/it/main.json
- src/_locales/zh-hant/main.json
- src/_locales/ko/main.json
- src/_locales/ru/main.json
- src/_locales/zh-hans/main.json
- src/_locales/es/main.json
- src/_locales/fr/main.json
- src/_locales/ja/main.json
- src/_locales/pt/main.json
- tests/unit/popup/general-part-override.test.mjs
- tests/unit/popup/general-balance-utils.test.mjs
🚧 Files skipped from review as they are similar to previous changes (31)
- src/popup/sections/AdvancedPart.jsx
- tests/unit/services/apis/custom-api.test.mjs
- src/popup/popup-config-utils.mjs
- src/_locales/en/main.json
- src/services/init-session.mjs
- src/services/wrappers.mjs
- tests/unit/config/user-config.test.mjs
- tests/unit/services/wrappers-register.test.mjs
- src/popup/sections/GeneralPart.jsx
- src/popup/Popup.jsx
- src/config/index.mjs
- src/_locales/tr/main.json
- src/popup/sections/general-provider-override-utils.mjs
- src/components/ConversationCard/index.jsx
- src/services/apis/custom-api.mjs
- tests/unit/services/apis/thin-adapters.test.mjs
- src/services/apis/openai-api.mjs
- src/services/apis/openai-compatible-core.mjs
- src/background/index.mjs
- tests/unit/popup/popup-config-update.test.mjs
- src/popup/sections/ApiModes.jsx
- src/popup/sections/general-balance-utils.mjs
- src/utils/model-name-convert.mjs
- tests/unit/config/migrate-user-config.test.mjs
- tests/unit/services/apis/provider-registry.test.mjs
- src/popup/sections/provider-secret-utils.mjs
- tests/unit/services/apis/openai-api-compat.test.mjs
- src/popup/sections/api-modes-provider-utils.mjs
- tests/unit/utils/model-name-convert.test.mjs
- tests/unit/popup/provider-secret-utils.test.mjs
- tests/unit/popup/api-modes-provider-utils.test.mjs
PeterDaveHello
force-pushed
the
refactor/openai-provider-platform
branch
from
a2dde36 to
c7ef713
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 54 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/popup/sections/GeneralPart.jsx:564
- The API mode
<select>is effectively uncontrolled but usesselected={...}on<option>elements. In React this pattern is discouraged and can lead to warnings/inconsistent selection; prefer controlling the<select>via itsvalue(ordefaultValue) and removeselectedfrom options.
if (desc) {
return (
<option value={index} key={index} selected={isApiModeSelected(apiMode, config)}>
{desc}
</option>
| payload.providerSecrets = { | ||
| ...(config.providerSecrets || {}), | ||
| [nextProviderId]: normalizedNextApiKey, | ||
| } |
There was a problem hiding this comment.
Fixed in the current branch. The override path now uses the same getProviderSecretsRecord(config) source before spreading providerSecrets, and the materialization path has a malformed-providerSecrets regression test.
| else port.postMessage({ error: message }) | ||
| } else { | ||
| const errMsg = JSON.stringify(err) | ||
| const errMsg = JSON.stringify(err) ?? 'unknown error' | ||
| if (isUsingBingWebModel(session) && errMsg.includes('isTrusted')) | ||
| port.postMessage({ error: t('Please login at https://bing.com first') }) | ||
| else port.postMessage({ error: errMsg ?? 'unknown error' }) | ||
| else port.postMessage({ error: errMsg }) |
There was a problem hiding this comment.
Verified but not taking this in this scoped branch follow-up: the JSON.stringify(err) fallback already exists on master, so it is not introduced by the provider-secret/custom-provider changes being finalized here. Keeping this out avoids expanding the patch beyond the branch-caused issues.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/popup/sections/GeneralPart.jsx`:
- Around line 434-481: getBalance can apply results from stale in-flight
requests; add a numeric ref (e.g., balanceRequestIdRef) and increment it in a
useLayoutEffect keyed by balanceCacheKey so each new balance context invalidates
prior requests, then capture the current id at the start of getBalance and check
it before calling setBalance or openUrl to ensure only the latest request
updates state (update all places in getBalance that setBalance or call openUrl
to guard with the captured id).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7ee6a34-cae6-44de-acfa-f58fc8828e0d
📒 Files selected for processing (29)
src/_locales/de/main.jsonsrc/_locales/en/main.jsonsrc/_locales/es/main.jsonsrc/_locales/fr/main.jsonsrc/_locales/in/main.jsonsrc/_locales/it/main.jsonsrc/_locales/ja/main.jsonsrc/_locales/ko/main.jsonsrc/_locales/pt/main.jsonsrc/_locales/ru/main.jsonsrc/_locales/tr/main.jsonsrc/_locales/zh-hans/main.jsonsrc/_locales/zh-hant/main.jsonsrc/components/ConversationCard/index.jsxsrc/popup/Popup.jsxsrc/popup/popup-config-utils.mjssrc/popup/sections/AdvancedPart.jsxsrc/popup/sections/ApiModes.jsxsrc/popup/sections/GeneralPart.jsxsrc/popup/sections/api-modes-provider-utils.mjssrc/popup/sections/general-balance-utils.mjssrc/popup/sections/general-provider-override-utils.mjssrc/popup/sections/provider-secret-utils.mjstests/unit/popup/api-modes-provider-utils.test.mjstests/unit/popup/api-modes.test.mjstests/unit/popup/general-balance-utils.test.mjstests/unit/popup/general-part-override.test.mjstests/unit/popup/popup-config-update.test.mjstests/unit/popup/provider-secret-utils.test.mjs
✅ Files skipped from review due to trivial changes (11)
- tests/unit/popup/api-modes.test.mjs
- tests/unit/popup/popup-config-update.test.mjs
- src/_locales/tr/main.json
- src/_locales/it/main.json
- src/_locales/pt/main.json
- src/_locales/in/main.json
- src/_locales/es/main.json
- src/_locales/ko/main.json
- src/_locales/de/main.json
- src/_locales/zh-hans/main.json
- tests/unit/popup/general-part-override.test.mjs
🚧 Files skipped from review as they are similar to previous changes (17)
- src/popup/sections/AdvancedPart.jsx
- src/_locales/ja/main.json
- src/popup/Popup.jsx
- src/components/ConversationCard/index.jsx
- src/_locales/zh-hant/main.json
- src/_locales/en/main.json
- src/popup/popup-config-utils.mjs
- src/popup/sections/general-provider-override-utils.mjs
- tests/unit/popup/general-balance-utils.test.mjs
- src/_locales/ru/main.json
- src/popup/sections/ApiModes.jsx
- src/_locales/fr/main.json
- src/popup/sections/provider-secret-utils.mjs
- src/popup/sections/general-balance-utils.mjs
- tests/unit/popup/provider-secret-utils.test.mjs
- src/popup/sections/api-modes-provider-utils.mjs
- tests/unit/popup/api-modes-provider-utils.test.mjs
PeterDaveHello
force-pushed
the
refactor/openai-provider-platform
branch
from
c7ef713 to
224f30b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/services/apis/openai-compatible-core.mjs (2)
115-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove or gate verbose payload logging in runtime path.
These debug logs include full conversation/SSE payload content and can leak sensitive user/provider data.
🔧 Proposed fix
const finish = () => { if (finished) return finished = true pushRecord(session, question, answer) - console.debug('conversation history', { content: session.conversationRecords }) port.postMessage({ answer: null, done: true, session: session }) } @@ onMessage(message) { - console.debug('sse message', message) if (finished) returnAlso applies to: 125-126
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/apis/openai-compatible-core.mjs` around lines 115 - 116, The console.debug that logs full conversation payload (console.debug('conversation history', { content: session.conversationRecords })) must be removed or gated; replace it with a safe conditional or sanitized log controlled by a debug flag (e.g., process.env.DEBUG_CONVERSATION) or the existing logger.level check, and never log raw session.conversationRecords or SSE payloads. Locate the two occurrences of console.debug around the port.postMessage({ answer: null, done: true, session: session }) and change them to either no-op or to log only non-sensitive metadata (IDs, timestamps, redacted lengths) when the debug flag is enabled, ensuring sensitive fields are stripped before any logging.
16-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAppend legacy
responsechunks; don’t overwrite accumulated output.With legacy streaming enabled, returning
data.responsedirectly resets prior buffered text and can lose earlier chunks.🔧 Proposed fix
function buildMessageAnswer(answer, data, allowLegacyResponseField) { - if (allowLegacyResponseField && typeof data?.response === 'string' && data.response) { - return data.response + if (allowLegacyResponseField && typeof data?.response === 'string') { + return (answer || '') + data.response }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/apis/openai-compatible-core.mjs` around lines 16 - 19, In buildMessageAnswer, don't return data.response directly when allowLegacyResponseField is true because that overwrites previously buffered text; instead, append the legacy chunk to the existing answer (e.g., set answer = (answer || '') + data.response) and return the combined string so earlier chunks are preserved; update the function's return path for the allowLegacyResponseField branch to concatenate rather than replace.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/popup/sections/GeneralPart.jsx`:
- Around line 385-407: The save/commit paths accept a raw providerApiKeyDraft
but the gating uses normalizedProviderApiKeyDraft (trimmed), so trim the draft
at the commit boundary: in handleSaveProviderKeyOverride call
commitSelectedModeProviderKeyOverride(providerApiKeyDraft.trim()) (and update
providerApiKeyDraft to the trimmed value if you want the UI to reflect it), and
in the shared-secret commit/blur path (commitProviderApiKeyDraft) ensure you
persist providerApiKeyDraft.trim() instead of the raw value; keep
handleUseSharedProviderKey behavior the same when clearing but ensure any
non-empty saves are trimmed before passing to commit functions.
---
Duplicate comments:
In `@src/services/apis/openai-compatible-core.mjs`:
- Around line 115-116: The console.debug that logs full conversation payload
(console.debug('conversation history', { content: session.conversationRecords
})) must be removed or gated; replace it with a safe conditional or sanitized
log controlled by a debug flag (e.g., process.env.DEBUG_CONVERSATION) or the
existing logger.level check, and never log raw session.conversationRecords or
SSE payloads. Locate the two occurrences of console.debug around the
port.postMessage({ answer: null, done: true, session: session }) and change them
to either no-op or to log only non-sensitive metadata (IDs, timestamps, redacted
lengths) when the debug flag is enabled, ensuring sensitive fields are stripped
before any logging.
- Around line 16-19: In buildMessageAnswer, don't return data.response directly
when allowLegacyResponseField is true because that overwrites previously
buffered text; instead, append the legacy chunk to the existing answer (e.g.,
set answer = (answer || '') + data.response) and return the combined string so
earlier chunks are preserved; update the function's return path for the
allowLegacyResponseField branch to concatenate rather than replace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3184335-5ecd-40f9-8d0e-a7c26cbdc47c
📒 Files selected for processing (52)
src/_locales/de/main.jsonsrc/_locales/en/main.jsonsrc/_locales/es/main.jsonsrc/_locales/fr/main.jsonsrc/_locales/in/main.jsonsrc/_locales/it/main.jsonsrc/_locales/ja/main.jsonsrc/_locales/ko/main.jsonsrc/_locales/pt/main.jsonsrc/_locales/ru/main.jsonsrc/_locales/tr/main.jsonsrc/_locales/zh-hans/main.jsonsrc/_locales/zh-hant/main.jsonsrc/background/index.mjssrc/components/ConversationCard/index.jsxsrc/config/index.mjssrc/config/openai-provider-mappings.mjssrc/popup/Popup.jsxsrc/popup/popup-config-utils.mjssrc/popup/sections/AdvancedPart.jsxsrc/popup/sections/ApiModes.jsxsrc/popup/sections/GeneralPart.jsxsrc/popup/sections/api-modes-provider-utils.mjssrc/popup/sections/general-provider-override-utils.mjssrc/popup/sections/provider-secret-utils.mjssrc/services/apis/aiml-api.mjssrc/services/apis/chatglm-api.mjssrc/services/apis/custom-api.mjssrc/services/apis/deepseek-api.mjssrc/services/apis/moonshot-api.mjssrc/services/apis/ollama-api.mjssrc/services/apis/openai-api.mjssrc/services/apis/openai-compatible-core.mjssrc/services/apis/openrouter-api.mjssrc/services/apis/provider-registry.mjssrc/services/init-session.mjssrc/services/wrappers.mjssrc/utils/model-name-convert.mjstests/unit/config/migrate-user-config.test.mjstests/unit/config/user-config.test.mjstests/unit/popup/api-modes-provider-utils.test.mjstests/unit/popup/api-modes.test.mjstests/unit/popup/general-part-override.test.mjstests/unit/popup/popup-config-update.test.mjstests/unit/popup/provider-secret-utils.test.mjstests/unit/services/apis/custom-api.test.mjstests/unit/services/apis/openai-api-compat.test.mjstests/unit/services/apis/provider-registry.test.mjstests/unit/services/apis/thin-adapters.test.mjstests/unit/services/handle-port-error.test.mjstests/unit/services/wrappers-register.test.mjstests/unit/utils/model-name-convert.test.mjs
💤 Files with no reviewable changes (6)
- src/services/apis/moonshot-api.mjs
- src/services/apis/aiml-api.mjs
- src/services/apis/deepseek-api.mjs
- src/services/apis/ollama-api.mjs
- src/services/apis/openrouter-api.mjs
- src/services/apis/chatglm-api.mjs
✅ Files skipped from review due to trivial changes (10)
- src/_locales/tr/main.json
- tests/unit/services/handle-port-error.test.mjs
- src/_locales/de/main.json
- src/_locales/pt/main.json
- src/_locales/ru/main.json
- src/_locales/ja/main.json
- src/_locales/zh-hans/main.json
- src/_locales/it/main.json
- src/_locales/in/main.json
- src/_locales/fr/main.json
🚧 Files skipped from review as they are similar to previous changes (34)
- tests/unit/services/apis/custom-api.test.mjs
- src/popup/popup-config-utils.mjs
- src/services/apis/custom-api.mjs
- src/services/init-session.mjs
- src/services/wrappers.mjs
- src/config/openai-provider-mappings.mjs
- tests/unit/popup/api-modes.test.mjs
- src/_locales/en/main.json
- tests/unit/services/wrappers-register.test.mjs
- src/popup/sections/AdvancedPart.jsx
- src/components/ConversationCard/index.jsx
- src/popup/Popup.jsx
- src/_locales/ko/main.json
- src/background/index.mjs
- src/_locales/es/main.json
- tests/unit/services/apis/provider-registry.test.mjs
- src/popup/sections/general-provider-override-utils.mjs
- tests/unit/services/apis/thin-adapters.test.mjs
- tests/unit/config/user-config.test.mjs
- tests/unit/popup/popup-config-update.test.mjs
- src/popup/sections/provider-secret-utils.mjs
- src/services/apis/provider-registry.mjs
- src/config/index.mjs
- src/_locales/zh-hant/main.json
- tests/unit/utils/model-name-convert.test.mjs
- tests/unit/config/migrate-user-config.test.mjs
- tests/unit/popup/general-part-override.test.mjs
- src/services/apis/openai-api.mjs
- tests/unit/popup/provider-secret-utils.test.mjs
- src/utils/model-name-convert.mjs
- src/popup/sections/api-modes-provider-utils.mjs
- tests/unit/popup/api-modes-provider-utils.test.mjs
- src/popup/sections/ApiModes.jsx
- tests/unit/services/apis/openai-api-compat.test.mjs
PeterDaveHello
force-pushed
the
refactor/openai-provider-platform
branch
from
224f30b to
4b7c963
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 52 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/popup/sections/GeneralPart.jsx:502
- The API mode
<select>still uses per-<option>selected={isApiModeSelected(...)}. With the new looserisApiModeSelected()semantics (it can match multiple custom modes that differ only by endpoint/key), this can mark multiple options as selected and lead to an arbitrary displayed selection. Prefer controlling the<select>via a singlevaluederived from a unique match (e.g.,getUniquelySelectedApiModeIndex(...)), and show a clear fallback when the selection is ambiguous.
}}
>
{apiModes.map((apiMode, index) => {
const desc = getApiModeDisplayLabel(
apiMode,
t,
Array.isArray(config.customOpenAIProviders) ? config.customOpenAIProviders : [],
)
if (desc) {
return (
<option value={index} key={index} selected={isApiModeSelected(apiMode, config)}>
{desc}
</option>
| export function isApiModeSelected(apiMode, configOrSession, { sessionCompat = false } = {}) { | ||
| const normalizeForCompare = (value, { includeProviderState = true } = {}) => { | ||
| const normalized = normalizeApiMode(value) | ||
| if (!normalized) return null | ||
| const normalizedForCompare = { | ||
| groupName: normalized.groupName, | ||
| itemName: normalized.itemName, | ||
| isCustom: normalized.isCustom, | ||
| customName: normalized.customName, | ||
| } | ||
| if (includeProviderState) { | ||
| normalizedForCompare.providerId = normalized.providerId | ||
| normalizedForCompare.active = normalized.active | ||
| } | ||
| return JSON.stringify(normalizedForCompare) |
There was a problem hiding this comment.
Verified against the current data model and tests. This does not need a follow-up in this branch: migrated legacy custom URL/key state is canonicalized into providerId/providerSecrets, and providerId is part of isApiModeSelected comparison. The legacy-session ambiguity case is handled by getUniquelySelectedApiModeIndex and covered by model-name-convert tests; changing popup callers here would broaden semantics beyond the scoped fix.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/popup/sections/GeneralPart.jsx (1)
471-480:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConvert string
styleprops to style objects.At Line 471 and the other ranges above,
style="..."and string-returning style expressions (Line 479) are invalid in React;stylemust be an object. This will throw at runtime.Suggested fix pattern
- <span style="display: flex; gap: 15px;"> + <span style={{ display: 'flex', gap: '15px' }}> - style={ + style={ isUsingOpenAICompatibleProvider || isUsingMultiModeModel(config) || isUsingSpecialCustomModel(config) || isUsingAzureOpenAiApiModel(config) || isUsingClaudeApiModel(config) - ? 'width: 50%;' + ? { width: '50%' } : undefined } - <select style="width: 50%;" + <select style={{ width: '50%' }} - <span style="display: inline-flex; align-items: center; gap: 8px; margin-top: 8px;"> + <span style={{ display: 'inline-flex', alignItems: 'center', gap: '8px', marginTop: '8px' }}>Also applies to: 512-513, 529-530, 549-549, 558-558, 596-597, 627-627, 646-647
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/popup/sections/GeneralPart.jsx` around lines 471 - 480, The JSX uses string-valued style props (e.g., style="display: flex; gap: 15px;" and conditional expressions that return strings) which is invalid in React; update those to style objects. Replace literal strings with objects like style={{ display: 'flex', gap: '15px' }} and change conditional returns to objects or merged objects (e.g., for the select: style={ isUsingOpenAICompatibleProvider || isUsingMultiModeModel(config) || isUsingSpecialCustomModel(config) || isUsingAzureOpenAiApiModel(config) || isUsingClaudeApiModel(config) ? { width: '50%' } : undefined }). Apply the same pattern to all other occurrences mentioned (lines around the same select and similar spans/inputs in the GeneralPart component) so every style prop is an object or undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/_locales/fr/main.json`:
- Around line 211-215: Remove the duplicated locale entry for the key "Anthropic
(Claude Sonnet 4.6)" by deleting the second occurrence (the one in the shown
diff block), leaving only the original definition; then run a JSON linter /
parser to validate src/_locales/fr/main.json to ensure no other duplicate keys
remain and the file is still valid.
---
Duplicate comments:
In `@src/popup/sections/GeneralPart.jsx`:
- Around line 471-480: The JSX uses string-valued style props (e.g.,
style="display: flex; gap: 15px;" and conditional expressions that return
strings) which is invalid in React; update those to style objects. Replace
literal strings with objects like style={{ display: 'flex', gap: '15px' }} and
change conditional returns to objects or merged objects (e.g., for the select:
style={ isUsingOpenAICompatibleProvider || isUsingMultiModeModel(config) ||
isUsingSpecialCustomModel(config) || isUsingAzureOpenAiApiModel(config) ||
isUsingClaudeApiModel(config) ? { width: '50%' } : undefined }). Apply the same
pattern to all other occurrences mentioned (lines around the same select and
similar spans/inputs in the GeneralPart component) so every style prop is an
object or undefined.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1f6f73f-1605-4aed-bd09-5e76233bffc5
📒 Files selected for processing (52)
src/_locales/de/main.jsonsrc/_locales/en/main.jsonsrc/_locales/es/main.jsonsrc/_locales/fr/main.jsonsrc/_locales/in/main.jsonsrc/_locales/it/main.jsonsrc/_locales/ja/main.jsonsrc/_locales/ko/main.jsonsrc/_locales/pt/main.jsonsrc/_locales/ru/main.jsonsrc/_locales/tr/main.jsonsrc/_locales/zh-hans/main.jsonsrc/_locales/zh-hant/main.jsonsrc/background/index.mjssrc/components/ConversationCard/index.jsxsrc/config/index.mjssrc/config/openai-provider-mappings.mjssrc/popup/Popup.jsxsrc/popup/popup-config-utils.mjssrc/popup/sections/AdvancedPart.jsxsrc/popup/sections/ApiModes.jsxsrc/popup/sections/GeneralPart.jsxsrc/popup/sections/api-modes-provider-utils.mjssrc/popup/sections/general-provider-override-utils.mjssrc/popup/sections/provider-secret-utils.mjssrc/services/apis/aiml-api.mjssrc/services/apis/chatglm-api.mjssrc/services/apis/custom-api.mjssrc/services/apis/deepseek-api.mjssrc/services/apis/moonshot-api.mjssrc/services/apis/ollama-api.mjssrc/services/apis/openai-api.mjssrc/services/apis/openai-compatible-core.mjssrc/services/apis/openrouter-api.mjssrc/services/apis/provider-registry.mjssrc/services/init-session.mjssrc/services/wrappers.mjssrc/utils/model-name-convert.mjstests/unit/config/migrate-user-config.test.mjstests/unit/config/user-config.test.mjstests/unit/popup/api-modes-provider-utils.test.mjstests/unit/popup/api-modes.test.mjstests/unit/popup/general-part-override.test.mjstests/unit/popup/popup-config-update.test.mjstests/unit/popup/provider-secret-utils.test.mjstests/unit/services/apis/custom-api.test.mjstests/unit/services/apis/openai-api-compat.test.mjstests/unit/services/apis/provider-registry.test.mjstests/unit/services/apis/thin-adapters.test.mjstests/unit/services/handle-port-error.test.mjstests/unit/services/wrappers-register.test.mjstests/unit/utils/model-name-convert.test.mjs
💤 Files with no reviewable changes (6)
- src/services/apis/moonshot-api.mjs
- src/services/apis/chatglm-api.mjs
- src/services/apis/deepseek-api.mjs
- src/services/apis/aiml-api.mjs
- src/services/apis/ollama-api.mjs
- src/services/apis/openrouter-api.mjs
✅ Files skipped from review due to trivial changes (10)
- tests/unit/services/wrappers-register.test.mjs
- src/_locales/zh-hans/main.json
- src/_locales/pt/main.json
- src/_locales/zh-hant/main.json
- src/_locales/it/main.json
- src/_locales/ja/main.json
- src/_locales/en/main.json
- src/_locales/in/main.json
- src/_locales/ko/main.json
- src/_locales/tr/main.json
🚧 Files skipped from review as they are similar to previous changes (32)
- tests/unit/services/apis/custom-api.test.mjs
- src/popup/popup-config-utils.mjs
- tests/unit/popup/api-modes.test.mjs
- src/services/init-session.mjs
- tests/unit/services/handle-port-error.test.mjs
- src/background/index.mjs
- tests/unit/popup/popup-config-update.test.mjs
- src/_locales/es/main.json
- src/config/openai-provider-mappings.mjs
- src/popup/sections/provider-secret-utils.mjs
- src/components/ConversationCard/index.jsx
- tests/unit/config/user-config.test.mjs
- src/services/wrappers.mjs
- src/services/apis/openai-compatible-core.mjs
- src/popup/sections/AdvancedPart.jsx
- tests/unit/popup/general-part-override.test.mjs
- src/popup/Popup.jsx
- src/_locales/ru/main.json
- src/_locales/de/main.json
- src/config/index.mjs
- src/popup/sections/general-provider-override-utils.mjs
- tests/unit/services/apis/thin-adapters.test.mjs
- tests/unit/services/apis/provider-registry.test.mjs
- src/services/apis/provider-registry.mjs
- src/utils/model-name-convert.mjs
- src/popup/sections/api-modes-provider-utils.mjs
- tests/unit/services/apis/openai-api-compat.test.mjs
- tests/unit/popup/api-modes-provider-utils.test.mjs
- src/services/apis/openai-api.mjs
- tests/unit/config/migrate-user-config.test.mjs
- src/popup/sections/ApiModes.jsx
- tests/unit/utils/model-name-convert.test.mjs
| if (hasModeReference || sessionReferencedProviderIds.includes(normalizedCandidateId)) { | ||
| return currentConfigUpdate | ||
| } | ||
|
|
||
| const nextProviderSecrets = { ...(nextConfig.providerSecrets || {}) } | ||
| delete nextProviderSecrets[normalizedCandidateId] | ||
|
|
||
| return { |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/services/apis/openai-compatible-core.mjs (1)
17-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLegacy
responsechunks still overwrite the accumulator.The previous review on this hunk was marked addressed, but Line 18 still returns
data.responseinstead ofanswer + data.response. For any streamed legacy provider (e.g., Ollama-styleresponsedeltas), only the final chunk's fragment will survive — every prior partial token is dropped on eachonMessagecall.Also note the
&& data.responsetruthiness guard will skip empty-string chunks, which is fine, but the bigger problem is the overwrite semantics.🔧 Proposed fix
function buildMessageAnswer(answer, data, allowLegacyResponseField) { - if (allowLegacyResponseField && typeof data?.response === 'string' && data.response) { - return data.response + if (allowLegacyResponseField && typeof data?.response === 'string') { + return answer + data.response }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/apis/openai-compatible-core.mjs` around lines 17 - 19, The legacy-`response` branch in openai-compatible-core.mjs currently returns only data.response, which overwrites the accumulator; change the behavior in the allowLegacyResponseField branch (the if that checks allowLegacyResponseField && typeof data?.response === 'string') to append the incoming fragment to the existing accumulator (variable answer) and return answer + data.response instead of data.response, preserving previous partial tokens; keep the existing truthiness guard for skipping empty-string chunks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/services/apis/openai-compatible-core.mjs`:
- Around line 17-19: The legacy-`response` branch in openai-compatible-core.mjs
currently returns only data.response, which overwrites the accumulator; change
the behavior in the allowLegacyResponseField branch (the if that checks
allowLegacyResponseField && typeof data?.response === 'string') to append the
incoming fragment to the existing accumulator (variable answer) and return
answer + data.response instead of data.response, preserving previous partial
tokens; keep the existing truthiness guard for skipping empty-string chunks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c89df71-138e-48e6-ac8c-561237c91a83
📒 Files selected for processing (52)
src/_locales/de/main.jsonsrc/_locales/en/main.jsonsrc/_locales/es/main.jsonsrc/_locales/fr/main.jsonsrc/_locales/in/main.jsonsrc/_locales/it/main.jsonsrc/_locales/ja/main.jsonsrc/_locales/ko/main.jsonsrc/_locales/pt/main.jsonsrc/_locales/ru/main.jsonsrc/_locales/tr/main.jsonsrc/_locales/zh-hans/main.jsonsrc/_locales/zh-hant/main.jsonsrc/background/index.mjssrc/components/ConversationCard/index.jsxsrc/config/index.mjssrc/config/openai-provider-mappings.mjssrc/popup/Popup.jsxsrc/popup/popup-config-utils.mjssrc/popup/sections/AdvancedPart.jsxsrc/popup/sections/ApiModes.jsxsrc/popup/sections/GeneralPart.jsxsrc/popup/sections/api-modes-provider-utils.mjssrc/popup/sections/general-provider-override-utils.mjssrc/popup/sections/provider-secret-utils.mjssrc/services/apis/aiml-api.mjssrc/services/apis/chatglm-api.mjssrc/services/apis/custom-api.mjssrc/services/apis/deepseek-api.mjssrc/services/apis/moonshot-api.mjssrc/services/apis/ollama-api.mjssrc/services/apis/openai-api.mjssrc/services/apis/openai-compatible-core.mjssrc/services/apis/openrouter-api.mjssrc/services/apis/provider-registry.mjssrc/services/init-session.mjssrc/services/wrappers.mjssrc/utils/model-name-convert.mjstests/unit/config/migrate-user-config.test.mjstests/unit/config/user-config.test.mjstests/unit/popup/api-modes-provider-utils.test.mjstests/unit/popup/api-modes.test.mjstests/unit/popup/general-part-override.test.mjstests/unit/popup/popup-config-update.test.mjstests/unit/popup/provider-secret-utils.test.mjstests/unit/services/apis/custom-api.test.mjstests/unit/services/apis/openai-api-compat.test.mjstests/unit/services/apis/provider-registry.test.mjstests/unit/services/apis/thin-adapters.test.mjstests/unit/services/handle-port-error.test.mjstests/unit/services/wrappers-register.test.mjstests/unit/utils/model-name-convert.test.mjs
💤 Files with no reviewable changes (6)
- src/services/apis/moonshot-api.mjs
- src/services/apis/ollama-api.mjs
- src/services/apis/chatglm-api.mjs
- src/services/apis/deepseek-api.mjs
- src/services/apis/aiml-api.mjs
- src/services/apis/openrouter-api.mjs
✅ Files skipped from review due to trivial changes (13)
- src/_locales/de/main.json
- src/_locales/ja/main.json
- tests/unit/services/apis/custom-api.test.mjs
- src/_locales/es/main.json
- src/_locales/zh-hant/main.json
- src/_locales/ru/main.json
- src/_locales/ko/main.json
- src/_locales/pt/main.json
- src/_locales/fr/main.json
- src/_locales/zh-hans/main.json
- src/_locales/en/main.json
- src/_locales/it/main.json
- src/_locales/in/main.json
🚧 Files skipped from review as they are similar to previous changes (29)
- src/config/openai-provider-mappings.mjs
- src/services/init-session.mjs
- tests/unit/popup/api-modes.test.mjs
- src/popup/sections/AdvancedPart.jsx
- tests/unit/services/handle-port-error.test.mjs
- src/components/ConversationCard/index.jsx
- src/services/apis/custom-api.mjs
- tests/unit/config/user-config.test.mjs
- tests/unit/services/apis/thin-adapters.test.mjs
- src/background/index.mjs
- src/services/wrappers.mjs
- tests/unit/popup/popup-config-update.test.mjs
- tests/unit/utils/model-name-convert.test.mjs
- src/config/index.mjs
- tests/unit/config/migrate-user-config.test.mjs
- src/popup/sections/general-provider-override-utils.mjs
- src/utils/model-name-convert.mjs
- src/popup/sections/ApiModes.jsx
- tests/unit/services/apis/provider-registry.test.mjs
- src/_locales/tr/main.json
- src/popup/Popup.jsx
- src/services/apis/openai-api.mjs
- tests/unit/popup/general-part-override.test.mjs
- src/services/apis/provider-registry.mjs
- tests/unit/popup/provider-secret-utils.test.mjs
- src/popup/sections/provider-secret-utils.mjs
- src/popup/sections/api-modes-provider-utils.mjs
- src/popup/sections/GeneralPart.jsx
- tests/unit/services/apis/openai-api-compat.test.mjs
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/popup/sections/api-modes-provider-utils.mjs (1)
490-565: ⚡ Quick winDeduplicate label-matching logic between
getProviderIdsMatchingSessionLabelandcanSessionRecoverViaLegacyLabelFallback.Both functions independently re-derive the same
fallbackApiModesfromsession.apiMode+apiModes(group/customName filter → exactitemName/isCustomcandidates → legacy fallback when there's noitemNameand exactly one candidate). Only the final filter differs: 490 excludeslegacy-custom-default(and requires an enabled provider), while 531 asks whetherlegacy-custom-defaultis present. Keeping two copies invites the two paths to drift (e.g., a tweak to the legacy-shape rule getting applied to only one), which would silently weaken the deletion/rewrite guard.♻️ Proposed refactor: extract a shared candidate resolver
+function getSessionLabelProviderIds(session = null, apiModes = []) { + if (normalizeText(session?.apiMode?.groupName) !== 'customApiModelKeys') return [] + + const normalizedSessionLabel = { + groupName: normalizeText(session?.apiMode?.groupName), + itemName: normalizeText(session?.apiMode?.itemName), + isCustom: Boolean(session?.apiMode?.isCustom), + customName: normalizeText(session?.apiMode?.customName), + } + if (!normalizedSessionLabel.customName) return [] + + const allCandidates = (Array.isArray(apiModes) ? apiModes : []).filter((apiMode) => { + if (!apiMode || typeof apiMode !== 'object') return false + return ( + normalizeText(apiMode.groupName) === normalizedSessionLabel.groupName && + normalizeText(apiMode.customName) === normalizedSessionLabel.customName + ) + }) + const exactCandidates = allCandidates.filter( + (apiMode) => + normalizeText(apiMode?.itemName) === normalizedSessionLabel.itemName && + Boolean(apiMode?.isCustom) === normalizedSessionLabel.isCustom, + ) + const matchedApiModes = exactCandidates.length === 1 ? exactCandidates : [] + const isLegacyCustomShape = !normalizedSessionLabel.itemName + const fallbackApiModes = + matchedApiModes.length === 0 && isLegacyCustomShape && allCandidates.length === 1 + ? allCandidates + : matchedApiModes + + return fallbackApiModes + .map((apiMode) => normalizeProviderId(apiMode?.providerId)) + .filter((providerId, index, providerIds) => providerIds.indexOf(providerId) === index) +} + function getProviderIdsMatchingSessionLabel(session = null, providers = [], apiModes = []) { - if (normalizeText(session?.apiMode?.groupName) !== 'customApiModelKeys') return [] - - const normalizedSessionLabel = { - groupName: normalizeText(session?.apiMode?.groupName), - itemName: normalizeText(session?.apiMode?.itemName), - isCustom: Boolean(session?.apiMode?.isCustom), - customName: normalizeText(session?.apiMode?.customName), - } - if (!normalizedSessionLabel.customName) return [] - - const allCandidates = (Array.isArray(apiModes) ? apiModes : []).filter((apiMode) => { - if (!apiMode || typeof apiMode !== 'object') return false - return ( - normalizeText(apiMode.groupName) === normalizedSessionLabel.groupName && - normalizeText(apiMode.customName) === normalizedSessionLabel.customName - ) - }) - const exactCandidates = allCandidates.filter( - (apiMode) => - normalizeText(apiMode?.itemName) === normalizedSessionLabel.itemName && - Boolean(apiMode?.isCustom) === normalizedSessionLabel.isCustom, - ) - const matchedApiModes = exactCandidates.length === 1 ? exactCandidates : [] - const isLegacyCustomShape = !normalizedSessionLabel.itemName - const fallbackApiModes = - matchedApiModes.length === 0 && isLegacyCustomShape && allCandidates.length === 1 - ? allCandidates - : matchedApiModes - - return fallbackApiModes - .map((apiMode) => normalizeProviderId(apiMode?.providerId)) + return getSessionLabelProviderIds(session, apiModes) .filter((providerId) => providerId && providerId !== 'legacy-custom-default') - .filter((providerId, index, providerIds) => providerIds.indexOf(providerId) === index) .filter((providerId) => (Array.isArray(providers) ? providers : []).some( (provider) => provider?.enabled !== false && normalizeText(provider?.id) === providerId, ), ) } function canSessionRecoverViaLegacyLabelFallback(session = null, apiModes = []) { - if (normalizeText(session?.apiMode?.groupName) !== 'customApiModelKeys') return false - - const normalizedSessionLabel = { - groupName: normalizeText(session?.apiMode?.groupName), - itemName: normalizeText(session?.apiMode?.itemName), - isCustom: Boolean(session?.apiMode?.isCustom), - customName: normalizeText(session?.apiMode?.customName), - } - if (!normalizedSessionLabel.customName) return false - - const allCandidates = (Array.isArray(apiModes) ? apiModes : []).filter((apiMode) => { - if (!apiMode || typeof apiMode !== 'object') return false - return ( - normalizeText(apiMode.groupName) === normalizedSessionLabel.groupName && - normalizeText(apiMode.customName) === normalizedSessionLabel.customName - ) - }) - const exactCandidates = allCandidates.filter( - (apiMode) => - normalizeText(apiMode?.itemName) === normalizedSessionLabel.itemName && - Boolean(apiMode?.isCustom) === normalizedSessionLabel.isCustom, - ) - const matchedApiModes = exactCandidates.length === 1 ? exactCandidates : [] - const isLegacyCustomShape = !normalizedSessionLabel.itemName - const fallbackApiModes = - matchedApiModes.length === 0 && isLegacyCustomShape && allCandidates.length === 1 - ? allCandidates - : matchedApiModes - - return fallbackApiModes - .map((apiMode) => normalizeProviderId(apiMode?.providerId)) - .filter((providerId, index, providerIds) => providerIds.indexOf(providerId) === index) - .includes('legacy-custom-default') + return getSessionLabelProviderIds(session, apiModes).includes('legacy-custom-default') }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/popup/sections/api-modes-provider-utils.mjs` around lines 490 - 565, Both getProviderIdsMatchingSessionLabel and canSessionRecoverViaLegacyLabelFallback duplicate the same label-matching and fallback derivation; extract that logic into a single helper (e.g., resolveFallbackApiModes(session, apiModes) or getFallbackApiModesForSession) that returns the array of fallbackApiModes, then call that helper from both functions; keep the final, function-specific steps in each (getProviderIdsMatchingSessionLabel should map -> normalizeProviderId -> exclude 'legacy-custom-default' -> dedupe -> filter by enabled providers, while canSessionRecoverViaLegacyLabelFallback should map -> normalizeProviderId -> dedupe -> check includes('legacy-custom-default')). Ensure the helper uses the existing normalization checks (normalizeText on groupName/customName/itemName and Boolean on isCustom) and the legacy-shape fallback rule (no itemName and allCandidates.length === 1).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/popup/sections/api-modes-provider-utils.mjs`:
- Around line 490-565: Both getProviderIdsMatchingSessionLabel and
canSessionRecoverViaLegacyLabelFallback duplicate the same label-matching and
fallback derivation; extract that logic into a single helper (e.g.,
resolveFallbackApiModes(session, apiModes) or getFallbackApiModesForSession)
that returns the array of fallbackApiModes, then call that helper from both
functions; keep the final, function-specific steps in each
(getProviderIdsMatchingSessionLabel should map -> normalizeProviderId -> exclude
'legacy-custom-default' -> dedupe -> filter by enabled providers, while
canSessionRecoverViaLegacyLabelFallback should map -> normalizeProviderId ->
dedupe -> check includes('legacy-custom-default')). Ensure the helper uses the
existing normalization checks (normalizeText on groupName/customName/itemName
and Boolean on isCustom) and the legacy-shape fallback rule (no itemName and
allCandidates.length === 1).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de9cebcd-1a71-43e4-95d4-377b2fcb081b
📒 Files selected for processing (27)
src/_locales/de/main.jsonsrc/_locales/en/main.jsonsrc/_locales/es/main.jsonsrc/_locales/fr/main.jsonsrc/_locales/in/main.jsonsrc/_locales/it/main.jsonsrc/_locales/ja/main.jsonsrc/_locales/ko/main.jsonsrc/_locales/pt/main.jsonsrc/_locales/ru/main.jsonsrc/_locales/tr/main.jsonsrc/_locales/zh-hans/main.jsonsrc/_locales/zh-hant/main.jsonsrc/components/ConversationCard/index.jsxsrc/popup/Popup.jsxsrc/popup/popup-config-utils.mjssrc/popup/sections/AdvancedPart.jsxsrc/popup/sections/ApiModes.jsxsrc/popup/sections/GeneralPart.jsxsrc/popup/sections/api-modes-provider-utils.mjssrc/popup/sections/general-provider-override-utils.mjssrc/popup/sections/provider-secret-utils.mjstests/unit/popup/api-modes-provider-utils.test.mjstests/unit/popup/api-modes.test.mjstests/unit/popup/general-part-override.test.mjstests/unit/popup/popup-config-update.test.mjstests/unit/popup/provider-secret-utils.test.mjs
✅ Files skipped from review due to trivial changes (10)
- src/_locales/ru/main.json
- src/_locales/it/main.json
- src/_locales/es/main.json
- src/_locales/ja/main.json
- src/_locales/zh-hans/main.json
- src/_locales/pt/main.json
- src/_locales/de/main.json
- src/_locales/ko/main.json
- tests/unit/popup/popup-config-update.test.mjs
- src/_locales/tr/main.json
🚧 Files skipped from review as they are similar to previous changes (16)
- src/popup/sections/AdvancedPart.jsx
- src/popup/popup-config-utils.mjs
- src/_locales/in/main.json
- src/popup/sections/provider-secret-utils.mjs
- tests/unit/popup/api-modes.test.mjs
- src/_locales/en/main.json
- src/_locales/zh-hant/main.json
- src/components/ConversationCard/index.jsx
- src/popup/sections/GeneralPart.jsx
- src/_locales/fr/main.json
- src/popup/sections/ApiModes.jsx
- tests/unit/popup/general-part-override.test.mjs
- src/popup/Popup.jsx
- src/popup/sections/general-provider-override-utils.mjs
- tests/unit/popup/provider-secret-utils.test.mjs
- tests/unit/popup/api-modes-provider-utils.test.mjs
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Consolidate OpenAI-compatible API request handling into a single shared module and route background dispatch through provider registry lookup. This removes duplicated streaming/parsing logic from openai-api and custom-api while keeping existing behavior. Add config migration to preserve existing API keys and custom mode entries by mapping them into providerSecrets and custom provider records. Keep legacy fallbacks for apiMode customUrl/custom apiKey to avoid user-visible regressions during rollout. Normalize apiMode objects at runtime and compare selection using stable identity fields so migrated and legacy session data continue to match correctly.
Split provider management from API mode saving so users can add and edit custom OpenAI-compatible providers in a dedicated editor. Require a full chat-completions endpoint URL for provider setup and derive the paired completions endpoint with shared popup utilities backed by unit tests. Keep API mode provider binding explicit at save time and preserve provider-secret synchronization in General settings for backward compatibility. Add provider-related locale keys across supported locales.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/popup/sections/GeneralPart.jsx (4)
593-675: ⚖️ Poor tradeoffSignificant duplication between the two override-controls blocks.
The override-controls UI for the non-special-custom branch (Lines 593–625) and the special-custom-model branch (Lines 643–675) is essentially identical: same explanatory
<small>copy, same Save / Use shared key button pair, same disabled gating, same handlers. The only meaningful difference is the surrounding gating predicate. Extracting a small<ProviderKeyOverrideControls />helper (or a single render branch keyed byisUsingSpecialCustomModel(config)) would eliminate the duplicated translation string and reduce the risk of the two copies drifting apart when one is edited.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/popup/sections/GeneralPart.jsx` around lines 593 - 675, The two nearly identical override-control blocks should be consolidated to remove duplication: create a small helper component (e.g. ProviderKeyOverrideControls) or a single render branch and move the shared JSX and translation string into it, then replace both duplicated blocks with <ProviderKeyOverrideControls /> (or a call) gated by the existing predicates; ensure the new component accepts any needed props/state (e.g. isOverrideProviderKeyActionPending, normalizedProviderApiKeyDraft, normalizedSelectedProviderApiKey) and reuses the existing handlers (handleProviderOverrideActionMouseDown, handleSaveProviderKeyOverride, handleUseSharedProviderKey) and the same disabled logic so behavior is unchanged.
493-497: 💤 Low valueHoist the
customOpenAIProvidersfallback out of the map.
Array.isArray(config.customOpenAIProviders) ? config.customOpenAIProviders : []is re-evaluated for every API mode on every render. Compute it once before the.map(...)and pass the stable reference intogetApiModeDisplayLabel.♻️ Proposed tweak
+ {(() => { + const customOpenAIProviders = Array.isArray(config.customOpenAIProviders) + ? config.customOpenAIProviders + : [] + return apiModes.map((apiMode, index) => { + const desc = getApiModeDisplayLabel(apiMode, t, customOpenAIProviders) + ... + }) + })()}Or hoist the array into a
constnear the top of the function body and reference it inside the existing.map(...).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/popup/sections/GeneralPart.jsx` around lines 493 - 497, Hoist the fallback expression for config.customOpenAIProviders out of the map by creating a single const (e.g. const customOpenAIProviders = Array.isArray(config.customOpenAIProviders) ? config.customOpenAIProviders : []) near the top of the component/function, then use that stable reference when calling getApiModeDisplayLabel(apiMode, t, customOpenAIProviders) inside the .map instead of re-evaluating Array.isArray(...) each iteration; update any references to the inline fallback (currently used where desc is computed) to use this new constant.
159-161: ⚡ Quick winEffect on
selectedProviderApiKeycan clobber the user's in-flight draft.This
useLayoutEffectre-runs wheneverselectedProviderApiKeychanges, not just on selection changes. If the persisted secret is updated out-of-band (e.g., a sibling tab/popup writes viaBrowser.storage.local, or a queued override commit completes while the input has been re-edited), the user's currently-typed draft will be silently overwritten without warning. Consider keying the reset on the selection signature only and handling post-commit re-sync separately, e.g.:♻️ Possible refactor
- useLayoutEffect(() => { - setProviderApiKeyDraft(selectedProviderApiKey) - }, [selectedProviderApiKey, selectedProviderId, selectedProviderSecretTargetId]) + useLayoutEffect(() => { + setProviderApiKeyDraft(selectedProviderApiKey) + // Reset only when the active provider/secret target changes; do not + // overwrite an in-flight draft when the persisted key updates out-of-band. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [providerApiKeySelectionSignature])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/popup/sections/GeneralPart.jsx` around lines 159 - 161, The current useLayoutEffect in GeneralPart.jsx resets the input draft whenever selectedProviderApiKey changes, which can clobber an in-flight user edit; change the effect that calls setProviderApiKeyDraft to only run when the selection identity changes (use [selectedProviderId, selectedProviderSecretTargetId] as the dependency signature) so switching providers still resets the draft, and implement a separate sync that handles out-of-band persisted updates without overwriting active edits (e.g., add another effect that watches selectedProviderApiKey but only calls setProviderApiKeyDraft when the current draft equals the previous persisted value or when an explicit "not editing" / isDirty flag is false). Ensure you reference useLayoutEffect, setProviderApiKeyDraft, selectedProviderApiKey, selectedProviderId, and selectedProviderSecretTargetId when making the change.
337-343: 💤 Low valueDead/dormant branch in
commitProviderApiKeyDraft.When
isSelectedProviderKeyManagedByModeOverrideis true,handleProviderApiKeyBlur(Lines 376–382) short-circuits before invokingcommitProviderApiKeyDraft, and the override-managedEnterpath callshandleSaveProviderKeyOverridedirectly. The only call site forcommitProviderApiKeyDraftis the non-override blur path, so this branch (including theoverrideCommitGenerationRef.current += 1generation bump) is currently unreachable. Either remove it, or wire a call site that actually exercises this path so the generation-invalidation is meaningful.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/popup/sections/GeneralPart.jsx` around lines 337 - 343, The branch in commitProviderApiKeyDraft that checks isSelectedProviderKeyManagedByModeOverride and increments overrideCommitGenerationRef.current is dead because commitProviderApiKeyDraft is only called from the non-override blur path; either remove that branch entirely (delete the isSelectedProviderKeyManagedByModeOverride check and the overrideCommitGenerationRef.current += 1 line) or wire a call site so it is exercised (e.g., call commitProviderApiKeyDraft from handleProviderApiKeyBlur or the Enter-path used by handleSaveProviderKeyOverride when override mode is active), or move the generation bump into handleSaveProviderKeyOverride so overrideCommitGenerationRef is updated when overrides are saved — reference commitProviderApiKeyDraft, handleProviderApiKeyBlur, handleSaveProviderKeyOverride, and overrideCommitGenerationRef.current in your changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/popup/sections/GeneralPart.jsx`:
- Around line 593-675: The two nearly identical override-control blocks should
be consolidated to remove duplication: create a small helper component (e.g.
ProviderKeyOverrideControls) or a single render branch and move the shared JSX
and translation string into it, then replace both duplicated blocks with
<ProviderKeyOverrideControls /> (or a call) gated by the existing predicates;
ensure the new component accepts any needed props/state (e.g.
isOverrideProviderKeyActionPending, normalizedProviderApiKeyDraft,
normalizedSelectedProviderApiKey) and reuses the existing handlers
(handleProviderOverrideActionMouseDown, handleSaveProviderKeyOverride,
handleUseSharedProviderKey) and the same disabled logic so behavior is
unchanged.
- Around line 493-497: Hoist the fallback expression for
config.customOpenAIProviders out of the map by creating a single const (e.g.
const customOpenAIProviders = Array.isArray(config.customOpenAIProviders) ?
config.customOpenAIProviders : []) near the top of the component/function, then
use that stable reference when calling getApiModeDisplayLabel(apiMode, t,
customOpenAIProviders) inside the .map instead of re-evaluating
Array.isArray(...) each iteration; update any references to the inline fallback
(currently used where desc is computed) to use this new constant.
- Around line 159-161: The current useLayoutEffect in GeneralPart.jsx resets the
input draft whenever selectedProviderApiKey changes, which can clobber an
in-flight user edit; change the effect that calls setProviderApiKeyDraft to only
run when the selection identity changes (use [selectedProviderId,
selectedProviderSecretTargetId] as the dependency signature) so switching
providers still resets the draft, and implement a separate sync that handles
out-of-band persisted updates without overwriting active edits (e.g., add
another effect that watches selectedProviderApiKey but only calls
setProviderApiKeyDraft when the current draft equals the previous persisted
value or when an explicit "not editing" / isDirty flag is false). Ensure you
reference useLayoutEffect, setProviderApiKeyDraft, selectedProviderApiKey,
selectedProviderId, and selectedProviderSecretTargetId when making the change.
- Around line 337-343: The branch in commitProviderApiKeyDraft that checks
isSelectedProviderKeyManagedByModeOverride and increments
overrideCommitGenerationRef.current is dead because commitProviderApiKeyDraft is
only called from the non-override blur path; either remove that branch entirely
(delete the isSelectedProviderKeyManagedByModeOverride check and the
overrideCommitGenerationRef.current += 1 line) or wire a call site so it is
exercised (e.g., call commitProviderApiKeyDraft from handleProviderApiKeyBlur or
the Enter-path used by handleSaveProviderKeyOverride when override mode is
active), or move the generation bump into handleSaveProviderKeyOverride so
overrideCommitGenerationRef is updated when overrides are saved — reference
commitProviderApiKeyDraft, handleProviderApiKeyBlur,
handleSaveProviderKeyOverride, and overrideCommitGenerationRef.current in your
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33135edd-b99c-4cd4-8b64-1330f1e60b7e
📒 Files selected for processing (52)
src/_locales/de/main.jsonsrc/_locales/en/main.jsonsrc/_locales/es/main.jsonsrc/_locales/fr/main.jsonsrc/_locales/in/main.jsonsrc/_locales/it/main.jsonsrc/_locales/ja/main.jsonsrc/_locales/ko/main.jsonsrc/_locales/pt/main.jsonsrc/_locales/ru/main.jsonsrc/_locales/tr/main.jsonsrc/_locales/zh-hans/main.jsonsrc/_locales/zh-hant/main.jsonsrc/background/index.mjssrc/components/ConversationCard/index.jsxsrc/config/index.mjssrc/config/openai-provider-mappings.mjssrc/popup/Popup.jsxsrc/popup/popup-config-utils.mjssrc/popup/sections/AdvancedPart.jsxsrc/popup/sections/ApiModes.jsxsrc/popup/sections/GeneralPart.jsxsrc/popup/sections/api-modes-provider-utils.mjssrc/popup/sections/general-provider-override-utils.mjssrc/popup/sections/provider-secret-utils.mjssrc/services/apis/aiml-api.mjssrc/services/apis/chatglm-api.mjssrc/services/apis/custom-api.mjssrc/services/apis/deepseek-api.mjssrc/services/apis/moonshot-api.mjssrc/services/apis/ollama-api.mjssrc/services/apis/openai-api.mjssrc/services/apis/openai-compatible-core.mjssrc/services/apis/openrouter-api.mjssrc/services/apis/provider-registry.mjssrc/services/init-session.mjssrc/services/wrappers.mjssrc/utils/model-name-convert.mjstests/unit/config/migrate-user-config.test.mjstests/unit/config/user-config.test.mjstests/unit/popup/api-modes-provider-utils.test.mjstests/unit/popup/api-modes.test.mjstests/unit/popup/general-part-override.test.mjstests/unit/popup/popup-config-update.test.mjstests/unit/popup/provider-secret-utils.test.mjstests/unit/services/apis/custom-api.test.mjstests/unit/services/apis/openai-api-compat.test.mjstests/unit/services/apis/provider-registry.test.mjstests/unit/services/apis/thin-adapters.test.mjstests/unit/services/handle-port-error.test.mjstests/unit/services/wrappers-register.test.mjstests/unit/utils/model-name-convert.test.mjs
💤 Files with no reviewable changes (6)
- src/services/apis/deepseek-api.mjs
- src/services/apis/chatglm-api.mjs
- src/services/apis/moonshot-api.mjs
- src/services/apis/openrouter-api.mjs
- src/services/apis/aiml-api.mjs
- src/services/apis/ollama-api.mjs
✅ Files skipped from review due to trivial changes (15)
- src/popup/popup-config-utils.mjs
- tests/unit/services/apis/custom-api.test.mjs
- src/_locales/zh-hant/main.json
- src/_locales/pt/main.json
- src/_locales/es/main.json
- src/_locales/ko/main.json
- src/_locales/ru/main.json
- src/_locales/de/main.json
- src/_locales/en/main.json
- src/_locales/zh-hans/main.json
- src/_locales/ja/main.json
- src/_locales/fr/main.json
- src/_locales/tr/main.json
- src/_locales/it/main.json
- src/_locales/in/main.json
🚧 Files skipped from review as they are similar to previous changes (30)
- tests/unit/services/wrappers-register.test.mjs
- src/config/openai-provider-mappings.mjs
- src/popup/sections/AdvancedPart.jsx
- src/services/init-session.mjs
- src/services/wrappers.mjs
- tests/unit/popup/api-modes.test.mjs
- src/services/apis/custom-api.mjs
- tests/unit/config/user-config.test.mjs
- src/popup/Popup.jsx
- src/background/index.mjs
- src/components/ConversationCard/index.jsx
- tests/unit/services/apis/thin-adapters.test.mjs
- tests/unit/popup/popup-config-update.test.mjs
- tests/unit/config/migrate-user-config.test.mjs
- tests/unit/services/handle-port-error.test.mjs
- src/services/apis/provider-registry.mjs
- tests/unit/utils/model-name-convert.test.mjs
- src/utils/model-name-convert.mjs
- tests/unit/popup/general-part-override.test.mjs
- src/config/index.mjs
- src/services/apis/openai-compatible-core.mjs
- src/popup/sections/provider-secret-utils.mjs
- src/popup/sections/ApiModes.jsx
- src/popup/sections/api-modes-provider-utils.mjs
- tests/unit/popup/api-modes-provider-utils.test.mjs
- tests/unit/services/apis/provider-registry.test.mjs
- src/services/apis/openai-api.mjs
- src/popup/sections/general-provider-override-utils.mjs
- tests/unit/popup/provider-secret-utils.test.mjs
- tests/unit/services/apis/openai-api-compat.test.mjs
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
User description
This PR unifies OpenAI-compatible API execution into a shared core + provider registry,
and completes the custom provider workflow in API Modes.
Included changes
max_completion_tokenswhere required)Compatibility
Validation
npm run lintpassednpm run buildpassedGitHub Copilot PR summary
PR Type
Enhancement, Tests, Documentation
Description
Unified OpenAI-compatible provider system: Consolidated all OpenAI-compatible API providers (custom, Ollama, DeepSeek, Moonshot, ChatGLM, OpenRouter, AIML) into a single shared execution path via
generateAnswersWithOpenAICompatibleApi, eliminating duplicate provider-specific implementationsConfiguration migration and schema versioning: Introduced comprehensive
migrateUserConfigfunction that normalizes provider IDs, consolidates legacy secrets, handles custom provider deduplication, and maintains backward compatibility with existing configsUnified provider registry: Created
provider-registry.mjswith centralized provider resolution logic supporting both built-in and custom providers, with secret lookup and endpoint URL constructionCustom provider UI workflow: Refactored API Modes popup to support custom provider creation/editing with provider selector dropdown, validation, and dynamic provider list management
Provider secret management: Added
buildProviderSecretUpdateutility for managing provider secrets across built-in and custom providers with legacy field supportAPI mode normalization: Implemented
normalizeApiModeutility to ensure consistent API mode object structure throughout the codebaseStream completion signaling: Fixed stream completion consistency to emit final message with
{ answer: null, done: true, session }formatComprehensive test coverage: Added 16 config migration tests, 8 provider registry tests, and additional tests for provider utilities and stream handling
Multilingual support: Updated localization strings across 12 languages for custom provider UI labels and validation messages
Removed 6 provider-specific API modules: Deleted
ollama-api.mjs,chatglm-api.mjs,aiml-api.mjs,moonshot-api.mjs,openrouter-api.mjs, anddeepseek-api.mjsas functionality consolidated into unified handlerDiagram Walkthrough
File Walkthrough
13 files
index.mjs
Configuration migration and provider registry unificationsrc/config/index.mjs
migrateUserConfigfunction to normalize provider IDs, consolidatelegacy secrets, and handle custom provider deduplication
customOpenAIProviders,providerSecrets,and
configSchemaVersionfor unified provider managementcustomUrlandapiKeyfields into provider-based storage while maintaining backward
compatibility
getUserConfigto automatically persist migrated configurationchanges to storage
ApiModes.jsx
Custom provider UI editor and provider selector integrationsrc/popup/sections/ApiModes.jsx
create/edit provider dialogs
chat completions URL
sanitizeApiModeForSaveto ensure proper provider ID andAPI key handling during save
getCustomOpenAIProvidersand provider utilities for dynamicprovider list management
provider-registry.mjs
Unified provider registry and resolution systemsrc/services/apis/provider-registry.mjs
getAllOpenAIProviders,getProviderById, andresolveOpenAICompatibleRequestfunctionsproviders (OpenAI, DeepSeek, Ollama, etc.) and custom providers
getProviderSecretfunction to resolve API keys fromproviderSecretsmap with fallback to legacy fieldsprovider configurations
GeneralPart.jsx
Unified provider detection and API key managementsrc/popup/sections/GeneralPart.jsx
resolveOpenAICompatibleRequestcallcredentials via
buildProviderSecretUpdateisUsingXxxApiModelchecks into singleisUsingOpenAICompatibleProviderflag
openai-api.mjs
OpenAI-compatible API core extraction and unificationsrc/services/apis/openai-api.mjs
generateAnswersWithOpenAICompatiblecore functiongenerateAnswersWithOpenAICompatibleApias unified entry pointthat resolves provider configuration and delegates to core
touchOllamaKeepAlivefor Ollama-specific keep-alivehandling
implementation
index.mjs
Unified background API execution routingsrc/background/index.mjs
generateAnswersWithOpenAICompatibleApicallisUsingOpenAICompatibleApiSessionutility to route allOpenAI-compatible providers through unified handler
DeepSeek, Moonshot, ChatGLM, OpenRouter, and AIML APIs
openai-compatible-core.mjs
OpenAI-compatible API core implementationsrc/services/apis/openai-compatible-core.mjs
generateAnswersWithOpenAICompatiblefunction handling both chat and completion endpoints
parsing, and message answer building
allowLegacyResponseFieldparameter for backward compatibility
OpenAI-compatible providers
model-name-convert.mjs
API mode normalization and comparison utilitiessrc/utils/model-name-convert.mjs
normalizeApiModefunction to ensure consistent API mode objectstructure with default values
apiModeToModelNameandgetApiModesFromConfigto use normalizedAPI modes for safer property access
isApiModeSelectedcomparison logic to normalize both sidesbefore comparing relevant fields
init-session.mjs
Session initialization API mode normalizationsrc/services/init-session.mjs
normalizeApiModeutility functionnormalizeApiModeto session initialization to ensure API modeobjects have consistent structure
custom-api.mjs
Consolidate custom API into unified OpenAI-compatible handlersrc/services/apis/custom-api.mjs
generateAnswersWithOpenAICompatiblefunctionparameters
provider-secret-utils.mjs
Provider secret update utility for API modessrc/popup/sections/provider-secret-utils.mjs
buildProviderSecretUpdatefunction formanaging provider secrets
field support
api-modes-provider-utils.mjs
Provider configuration utilities for API modes UIsrc/popup/sections/api-modes-provider-utils.mjs
API modes UI
createProviderIdgenerates unique normalized provider IDs avoidingconflicts
parseChatCompletionsEndpointUrlvalidates and parses chat completionsendpoint URLs
resolveProviderChatEndpointUrlresolves endpoint URL from providerconfiguration
wrappers.mjs
API mode normalization in port listenersrc/services/wrappers.mjs
normalizeApiModefunction from model-name-convertutility
session.apiModeduring port listenerregistration
5 files
migrate-user-config.test.mjs
Configuration migration test suitetests/unit/config/migrate-user-config.test.mjs
including legacy URL/key consolidation, provider ID normalization, and
deduplication
customUrlfields, secret key consolidation, and backward compatibility
same URL but different keys, and schema version updates
provider-registry.test.mjs
Provider registry resolution test suitetests/unit/services/apis/provider-registry.test.mjs
endpoint type detection and URL normalization
matching, and duplicate
/v1path preventioncustom providers
provider-secret-utils.test.mjs
Unit tests for provider secret update logictests/unit/popup/provider-secret-utils.test.mjs
buildProviderSecretUpdatefunction
providers
selected mode keys
updates
api-modes-provider-utils.test.mjs
Unit tests for API modes provider utilitiestests/unit/popup/api-modes-provider-utils.test.mjs
openai-api-compat.test.mjs
Stream completion signaling consistency teststests/unit/services/apis/openai-api-compat.test.mjs
{ answer: null, done: true, session }final message instead of
{ done: true }finish reason
incomplete streams
13 files
main.json
Japanese localization updatessrc/_locales/ja/main.json
Custom ProviderandProviderUI labelsmain.json
Turkish localization updatessrc/_locales/tr/main.json
Custom ProviderandProviderUI labelsmain.json
Traditional Chinese localization updatessrc/_locales/zh-hant/main.json
Custom ProviderandProviderUI labelsmain.json
Russian localization updatessrc/_locales/ru/main.json
Custom ProviderandProviderUI labelsmain.json
Italian localization updatessrc/_locales/it/main.json
Custom ProviderandProviderUI labelsmain.json
French localization updatessrc/_locales/fr/main.json
Custom ProviderandProviderUI labelsmain.json
German localization updatessrc/_locales/de/main.json
Custom ProviderandProviderUI labelsmain.json
Korean localization updatessrc/_locales/ko/main.json
Custom ProviderandProviderUI labelsmain.json
Spanish localization updatessrc/_locales/es/main.json
Custom ProviderandProviderUI labelsmain.json
Indonesian localization updatessrc/_locales/in/main.json
Custom ProviderandProviderUIlabels
main.json
Portuguese localization updatessrc/_locales/pt/main.json
Custom ProviderandProviderUIlabels
main.json
Simplified Chinese localization updatessrc/_locales/zh-hans/main.json
Custom ProviderandProviderUI labels
main.json
Localization strings for custom provider UIsrc/_locales/en/main.json
"Custom Provider"for UI display"Provider"for provider selection UI"Please enter a full Chat CompletionsURL"for URL validation error message1 files
openai-provider-mappings.mjs
Provider ID and legacy field mapping configurationsrc/config/openai-provider-mappings.mjs
field mappings
LEGACY_API_KEY_FIELD_BY_PROVIDER_IDfor 8 built-in providersLEGACY_SECRET_KEY_TO_PROVIDER_IDformigration
OPENAI_COMPATIBLE_GROUP_TO_PROVIDER_ID6 files
Summary by CodeRabbit
New Features
Configuration Migration
Bug Fixes & Reliability
Localization
Tests