Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors and modularizes the AI provider definitions by splitting them into dedicated files under src/shared/constants/providers/ (e.g., apiKey.ts, audio.ts, local.ts, oauth.ts, etc.) and exposing them via a central index. The review feedback highlights critical inconsistencies in duplicate key and alias resolution strategies across the combined provider sections, suggesting a consistent first-one-wins approach. Additionally, the reviewer recommends renaming the duplicate huggingchat and phind keys in WEB_COOKIE_PROVIDERS to avoid collisions, updating the optional API key utility accordingly, and adding corresponding unit tests to comply with the repository style guide when modifying production code.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function getOrCreateAiProviders(): Record<string, any> { | ||
| if (!_aiProviders) { | ||
| _aiProviders = {}; | ||
| for (const section of _PROVIDER_SECTIONS) { | ||
| Object.assign(_aiProviders, section); | ||
| } | ||
| } | ||
| return _aiProviders; | ||
| } |
There was a problem hiding this comment.
There is a critical inconsistency in how duplicate keys across sections are resolved. getProviderById uses nullish coalescing (??) in forward order, resulting in a first-one-wins strategy (returning the APIKEY_PROVIDERS version of huggingchat). However, getOrCreateAiProviders loops forward and uses Object.assign, resulting in a last-one-wins strategy (overwriting earlier definitions with the WEB_COOKIE_PROVIDERS version). This causes getProviderById("huggingchat") and AI_PROVIDERS["huggingchat"] to return different objects. To align them, we should loop backwards when building the combined maps so that earlier sections consistently take priority.
| function getOrCreateAiProviders(): Record<string, any> { | |
| if (!_aiProviders) { | |
| _aiProviders = {}; | |
| for (const section of _PROVIDER_SECTIONS) { | |
| Object.assign(_aiProviders, section); | |
| } | |
| } | |
| return _aiProviders; | |
| } | |
| function getOrCreateAiProviders(): Record<string, any> { | |
| if (!_aiProviders) { | |
| _aiProviders = {}; | |
| for (let i = _PROVIDER_SECTIONS.length - 1; i >= 0; i--) { | |
| Object.assign(_aiProviders, _PROVIDER_SECTIONS[i]); | |
| } | |
| } | |
| return _aiProviders; | |
| } |
| function getOrCreateAliasToId(): Record<string, string> { | ||
| if (!_ALIAS_TO_ID) { | ||
| _ALIAS_TO_ID = {}; | ||
| for (const section of _PROVIDER_SECTIONS) { | ||
| for (const p of Object.values(section)) { | ||
| if ((p as any).alias) _ALIAS_TO_ID[(p as any).alias] = (p as any).id; | ||
| } | ||
| } | ||
| } | ||
| return _ALIAS_TO_ID; | ||
| } |
There was a problem hiding this comment.
To align with the first-one-wins resolution strategy of getProviderByAlias, getOrCreateAliasToId should also prioritize earlier sections in _PROVIDER_SECTIONS when resolving duplicate aliases (like poe or v0). Looping backwards ensures that the first definition's alias mapping is preserved.
| function getOrCreateAliasToId(): Record<string, string> { | |
| if (!_ALIAS_TO_ID) { | |
| _ALIAS_TO_ID = {}; | |
| for (const section of _PROVIDER_SECTIONS) { | |
| for (const p of Object.values(section)) { | |
| if ((p as any).alias) _ALIAS_TO_ID[(p as any).alias] = (p as any).id; | |
| } | |
| } | |
| } | |
| return _ALIAS_TO_ID; | |
| } | |
| function getOrCreateAliasToId(): Record<string, string> { | |
| if (!_ALIAS_TO_ID) { | |
| _ALIAS_TO_ID = {}; | |
| for (let i = _PROVIDER_SECTIONS.length - 1; i >= 0; i--) { | |
| for (const p of Object.values(_PROVIDER_SECTIONS[i])) { | |
| if ((p as any).alias) _ALIAS_TO_ID[(p as any).alias] = (p as any).id; | |
| } | |
| } | |
| } | |
| return _ALIAS_TO_ID; | |
| } |
| function getOrCreateIdToAlias(): Record<string, string> { | ||
| if (!_ID_TO_ALIAS) { | ||
| _ID_TO_ALIAS = {}; | ||
| for (const section of _PROVIDER_SECTIONS) { | ||
| for (const p of Object.values(section)) { | ||
| _ID_TO_ALIAS[(p as any).id] = (p as any).alias || (p as any).id; | ||
| } | ||
| } | ||
| } | ||
| return _ID_TO_ALIAS; | ||
| } |
There was a problem hiding this comment.
Similarly, getOrCreateIdToAlias should be updated to loop backwards through _PROVIDER_SECTIONS to maintain consistency with the first-one-wins strategy used by getProviderById and getProviderAlias.
| function getOrCreateIdToAlias(): Record<string, string> { | |
| if (!_ID_TO_ALIAS) { | |
| _ID_TO_ALIAS = {}; | |
| for (const section of _PROVIDER_SECTIONS) { | |
| for (const p of Object.values(section)) { | |
| _ID_TO_ALIAS[(p as any).id] = (p as any).alias || (p as any).id; | |
| } | |
| } | |
| } | |
| return _ID_TO_ALIAS; | |
| } | |
| function getOrCreateIdToAlias(): Record<string, string> { | |
| if (!_ID_TO_ALIAS) { | |
| _ID_TO_ALIAS = {}; | |
| for (let i = _PROVIDER_SECTIONS.length - 1; i >= 0; i--) { | |
| for (const p of Object.values(_PROVIDER_SECTIONS[i])) { | |
| _ID_TO_ALIAS[(p as any).id] = (p as any).alias || (p as any).id; | |
| } | |
| } | |
| } | |
| return _ID_TO_ALIAS; | |
| } |
| huggingchat: { | ||
| id: "huggingchat", | ||
| // "hc" belongs to the hackclub provider; huggingchat uses its own id as alias. | ||
| alias: "huggingchat", | ||
| name: "HuggingChat (Free)", | ||
| icon: "auto_awesome", | ||
| color: "#FFD21E", | ||
| textIcon: "HC", | ||
| website: "https://huggingface.co/chat", | ||
| hasFree: true, | ||
| freeNote: "Free LLM chat — no subscription required. Rate limits apply.", | ||
| authHint: | ||
| "Paste your hf-chat cookie value from huggingface.co/chat (DevTools → Application → Cookies → hf-chat). Optional — works without auth for basic use.", | ||
| riskNoticeVariant: "webCookie", | ||
| }, |
There was a problem hiding this comment.
The keys huggingchat and phind are duplicated between APIKEY_PROVIDERS and WEB_COOKIE_PROVIDERS. To follow the established naming convention of other web/cookie providers (e.g., kimi-web, doubao-web, qwen-web), these should be renamed to huggingchat-web and phind-web. This prevents key collisions and resolution ambiguity.
| huggingchat: { | |
| id: "huggingchat", | |
| // "hc" belongs to the hackclub provider; huggingchat uses its own id as alias. | |
| alias: "huggingchat", | |
| name: "HuggingChat (Free)", | |
| icon: "auto_awesome", | |
| color: "#FFD21E", | |
| textIcon: "HC", | |
| website: "https://huggingface.co/chat", | |
| hasFree: true, | |
| freeNote: "Free LLM chat — no subscription required. Rate limits apply.", | |
| authHint: | |
| "Paste your hf-chat cookie value from huggingface.co/chat (DevTools → Application → Cookies → hf-chat). Optional — works without auth for basic use.", | |
| riskNoticeVariant: "webCookie", | |
| }, | |
| "huggingchat-web": { | |
| id: "huggingchat-web", | |
| // "hc" belongs to the hackclub provider; huggingchat uses its own id as alias. | |
| alias: "huggingchat", | |
| name: "HuggingChat (Free)", | |
| icon: "auto_awesome", | |
| color: "#FFD21E", | |
| textIcon: "HC", | |
| website: "https://huggingface.co/chat", | |
| hasFree: true, | |
| freeNote: "Free LLM chat — no subscription required. Rate limits apply.", | |
| authHint: | |
| "Paste your hf-chat cookie value from huggingface.co/chat (DevTools → Application → Cookies → hf-chat). Optional — works without auth for basic use.", | |
| riskNoticeVariant: "webCookie", | |
| }, |
| phind: { | ||
| id: "phind", | ||
| alias: "ph", | ||
| name: "Phind (Free)", | ||
| icon: "auto_awesome", | ||
| color: "#000000", | ||
| textIcon: "PH", | ||
| website: "https://www.phind.com", | ||
| hasFree: true, | ||
| freeNote: "Free dev-focused AI chat with code search. Rate limits apply.", | ||
| authHint: | ||
| "Paste your session cookie from phind.com (DevTools → Application → Cookies). Optional — works with free tier.", | ||
| riskNoticeVariant: "webCookie", | ||
| }, |
There was a problem hiding this comment.
Rename phind to phind-web to avoid duplicate key conflicts with APIKEY_PROVIDERS and maintain naming consistency.
| phind: { | |
| id: "phind", | |
| alias: "ph", | |
| name: "Phind (Free)", | |
| icon: "auto_awesome", | |
| color: "#000000", | |
| textIcon: "PH", | |
| website: "https://www.phind.com", | |
| hasFree: true, | |
| freeNote: "Free dev-focused AI chat with code search. Rate limits apply.", | |
| authHint: | |
| "Paste your session cookie from phind.com (DevTools → Application → Cookies). Optional — works with free tier.", | |
| riskNoticeVariant: "webCookie", | |
| }, | |
| "phind-web": { | |
| id: "phind-web", | |
| alias: "ph", | |
| name: "Phind (Free)", | |
| icon: "auto_awesome", | |
| color: "#000000", | |
| textIcon: "PH", | |
| website: "https://www.phind.com", | |
| hasFree: true, | |
| freeNote: "Free dev-focused AI chat with code search. Rate limits apply.", | |
| authHint: | |
| "Paste your session cookie from phind.com (DevTools → Application → Cookies). Optional — works with free tier.", | |
| riskNoticeVariant: "webCookie", | |
| }, |
| export function providerAllowsOptionalApiKey(providerId: unknown): boolean { | ||
| return ( | ||
| providerId === "searxng-search" || | ||
| providerId === "pollinations" || | ||
| providerId === "copilot-web" || | ||
| providerId === "duckduckgo-web" || | ||
| providerId === "veoaifree-web" || | ||
| providerId === "hackclub" || | ||
| providerId === "huggingchat" || | ||
| providerId === "gitlawb" || | ||
| providerId === "gitlawb-gmi" || | ||
| isLocalProvider(providerId) || | ||
| isSelfHostedChatProvider(providerId) || | ||
| isOpenAICompatibleProvider(providerId) || | ||
| isAnthropicCompatibleProvider(providerId) | ||
| ); | ||
| } |
There was a problem hiding this comment.
If huggingchat and phind are renamed to huggingchat-web and phind-web in WEB_COOKIE_PROVIDERS, they should be added to providerAllowsOptionalApiKey to ensure they are correctly recognized as allowing optional/no auth.
export function providerAllowsOptionalApiKey(providerId: unknown): boolean {
return (
providerId === "searxng-search" ||
providerId === "pollinations" ||
providerId === "copilot-web" ||
providerId === "duckduckgo-web" ||
providerId === "veoaifree-web" ||
providerId === "hackclub" ||
providerId === "huggingchat" ||
providerId === "huggingchat-web" ||
providerId === "phind-web" ||
providerId === "gitlawb" ||
providerId === "gitlawb-gmi" ||
isLocalProvider(providerId) ||
isSelfHostedChatProvider(providerId) ||
isOpenAICompatibleProvider(providerId) ||
isAnthropicCompatibleProvider(providerId)
);
}| @@ -0,0 +1,13 @@ | |||
| // Re-export everything to maintain backward compatibility | |||
There was a problem hiding this comment.
According to the Repository Style Guide (Hard Rules, Rule 9):
Always include tests when changing production code (
src/,open-sse/,electron/,bin/).
Since this PR refactors and modularizes the provider constants under src/shared/constants/providers/, please ensure that corresponding unit tests (e.g., verifying that getProviderById, getProviderByAlias, and the proxies resolve correctly) are added or updated in the tests/ directory.
References
- Always include tests when changing production code (src/, open-sse/, electron/, bin/). (link)
|
Thanks @oyi77 — blockers for the providers-constants modularization:
|
ba5e277 to
ed357ca
Compare
… tests Address review feedback on diegosouzapw#3994: - Align getOrCreateAliasToId and getOrCreateIdToAlias to first-one-wins strategy - Rename huggingchat→huggingchat-web, phind→phind-web in WEB_COOKIE_PROVIDERS - Add renamed providers to providerAllowsOptionalApiKey - Add resolution strategy tests
… tests Address review feedback on diegosouzapw#3994: - Align getOrCreateAliasToId and getOrCreateIdToAlias to first-one-wins strategy - Rename huggingchat→huggingchat-web, phind→phind-web in WEB_COOKIE_PROVIDERS - Add renamed providers to providerAllowsOptionalApiKey - Add resolution strategy tests
…zapw#3594) - Splits the monolithic provider constants file into 12 domain-specific files - Extracts provider dictionaries into definitions/ (apiKey, oauth, webCookie, local, search, etc.) - Uses folder-as-module pattern with index.ts facade for zero backward-compatibility breakages - Auto-prunes and manages all imports for strict typechecking
… tests Address review feedback on diegosouzapw#3994: - Align getOrCreateAliasToId and getOrCreateIdToAlias to first-one-wins strategy - Rename huggingchat→huggingchat-web, phind→phind-web in WEB_COOKIE_PROVIDERS - Add renamed providers to providerAllowsOptionalApiKey - Add resolution strategy tests
…larization The provider-consistency CI gate was failing because mimocode was missing from the modularized providers/definitions/noauth.ts. Added it back with all fields matching upstream release/v3.8.28. Closes: check:provider-consistency failure
|
Thanks for all the modularization work here, @oyi77 🙏. We've decided to hold the per-module "non-stacked" refactors and run the decomposition as one coordinated pass after the in-flight quality-gate work lands, instead of merging them piecemeal. Reason: on the two we did merge (#3993, #3988) we caught logic being dropped during the move — and the gates (provider-consistency / typecheck) don't detect internal-logic loss — so each of these needs a full lossless audit, which isn't tractable across many overlapping PRs against a moving release branch right now. The coordinated modularization is tracked in #3501 / #3594; we'd genuinely value your input on that plan once it's up. Closing for now — purely sequencing, not a reflection on the effort. |
…egosouzapw#3994) Split src/shared/constants/providers.ts (3241 lines) into 21 focused modules. Sub-split oversized apiKey.ts (1731→6 sub-files). Original file replaced with thin re-export. Closes diegosouzapw#3594
Replaces #3794 as an independent, non-stacked PR branched from release/v3.8.27.
Extracts 3.1K-line providers.ts constants into modular structure.
Supersedes: #3794