Refactored Settings UI #65
Conversation
|
Claude finished @olesho's task —— View job PR Review: Refactored Settings UII've completed a comprehensive review of this Settings UI refactor. This is an impressive modularization effort that transforms a monolithic 3788-line dialog into a well-structured, maintainable architecture. Review Tasks:
✅ Architectural ImprovementsExcellent modularization: The refactor breaks down a massive monolithic file into a clean, organized structure: Strong inheritance pattern: Proper separation of concerns: Clean separation between UI rendering, data persistence, validation, and business logic. ✅ Code QualityTypeScript usage: Excellent type safety with comprehensive interfaces in Error handling: Good defensive programming in Consistent patterns: All providers follow the same architectural pattern, making the codebase predictable and maintainable. Clean abstractions: The storage utilities in ✅ Security AssessmentAPI key handling: Proper security practices:
Input validation: Good validation utilities for API keys and endpoints, though basic. No obvious vulnerabilities: No signs of XSS risks, SQL injection, or credential leakage.
|
WalkthroughThis pull request adds a comprehensive settings UI system for the AI chat panel, including provider-specific configuration components (OpenAI, LiteLLM, Groq, OpenRouter), advanced settings modules (MCP, tracing, vector database, evaluation), factory utilities for creating model selectors and UI components, and storage/validation helpers with i18n support. Changes
Sequence DiagramsequenceDiagram
participant User
participant SettingsDialog
participant ProviderSettings
participant Storage
participant LLMClient
User->>SettingsDialog: Open settings
SettingsDialog->>Storage: Load saved provider & API keys
SettingsDialog->>ProviderSettings: Initialize provider UI
ProviderSettings->>Storage: Load provider config
User->>ProviderSettings: Enter API key
ProviderSettings->>Storage: Auto-save API key
User->>ProviderSettings: Click "Fetch Models"
ProviderSettings->>LLMClient: Fetch provider models
LLMClient-->>ProviderSettings: Model list
ProviderSettings->>Storage: Cache models + timestamp
ProviderSettings->>ProviderSettings: Update model selectors
User->>ProviderSettings: Select mini/nano models
ProviderSettings->>Storage: Store selection
User->>SettingsDialog: Click Save
SettingsDialog->>ProviderSettings: Call save()
ProviderSettings->>Storage: Persist final config
SettingsDialog->>User: Settings saved ✓
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Nitpick comments (59)
front_end/panels/ai_chat/BUILD.gn (2)
229-248: Keep _ai_chat_sources in sync with module sources.The same note applies here; after renaming i18n strings file, ensure this list mirrors sources. Consider factoring a shared array to avoid maintenance overhead.
43-62: Remove conditional guidance about ModuleUIStrings rename; reframe consolidation as preventative refactoring.The i18n file remains named "ui/settings/i18n-strings.ts" (no rename to ModuleUIStrings.ts is in progress). Both settings file lists are currently identical with no drift. Consider consolidating the repeated list into a shared variable to prevent future maintenance issues.
front_end/panels/ai_chat/ui/settings/components/SettingsHeader.ts (2)
10-12: Fix JSDoc hyphen formatting.Remove hyphens before @param descriptions to satisfy jsdoc rule.
- * @param container - Parent element to append the header to - * @param onClose - Callback function when close button is clicked + * @param container Parent element to append the header to + * @param onClose Callback invoked when the close button is clicked
29-31: Localize the close button label.Add a UIStrings key (e.g.,
closeSettingsAriaLabel) and use it here.- closeButton.setAttribute('aria-label', 'Close settings'); + closeButton.setAttribute('aria-label', i18nString(UIStrings.closeSettingsAriaLabel));front_end/panels/ai_chat/ui/settings/utils/styles.ts (1)
509-513: Guard against duplicate style injection.If dialog opens multiple times, this can append multiple identical <style> tags. Add an id and check before injecting.
-export function applySettingsStyles(dialogElement: HTMLElement): void { - const styleElement = document.createElement('style'); - styleElement.textContent = getSettingsStyles(); - dialogElement.appendChild(styleElement); -} +export function applySettingsStyles(dialogElement: HTMLElement): void { + const STYLE_ID = 'ai-chat-settings-styles'; + if (!dialogElement.querySelector(`#${STYLE_ID}`)) { + const styleElement = document.createElement('style'); + styleElement.id = STYLE_ID; + styleElement.textContent = getSettingsStyles(); + dialogElement.appendChild(styleElement); + } +}front_end/panels/ai_chat/ui/settings/components/AdvancedToggle.ts (4)
5-7: Import UIStrings for localization.Use shared ModuleUIStrings to localize label/hint.
-import { ADVANCED_SETTINGS_ENABLED_KEY } from '../constants.js'; -import { getStorageBoolean, setStorageBoolean } from '../utils/storage.js'; +import { ADVANCED_SETTINGS_ENABLED_KEY } from '../constants.js'; +import { getStorageBoolean, setStorageBoolean } from '../utils/storage.js'; +import { UIStrings, i18nString } from '../ModuleUIStrings.js';
20-23: Fix JSDoc hyphens.Remove hyphens before @param descriptions.
- * @param container - Parent element to append the toggle to - * @param onChange - Callback function when toggle state changes + * @param container Parent element to append the toggle to + * @param onChange Callback invoked when toggle state changes
43-53: Localize label/hint and add ARIA wiring.
- Localize textContent via UIStrings (add keys: advancedSettingsLabel, advancedSettingsHint).
- Use a unique id and connect ARIA attributes; expose expanded state.
- advancedToggleCheckbox.id = 'advanced-settings-toggle'; + const toggleId = 'advanced-settings-toggle'; + advancedToggleCheckbox.id = toggleId; advancedToggleCheckbox.className = 'advanced-settings-checkbox'; advancedToggleCheckbox.checked = getStorageBoolean(ADVANCED_SETTINGS_ENABLED_KEY, false); + advancedToggleCheckbox.setAttribute('aria-expanded', String(advancedToggleCheckbox.checked)); advancedToggleContainer.appendChild(advancedToggleCheckbox); const advancedToggleLabel = document.createElement('label'); - advancedToggleLabel.htmlFor = 'advanced-settings-toggle'; + advancedToggleLabel.htmlFor = toggleId; advancedToggleLabel.className = 'advanced-settings-label'; - advancedToggleLabel.textContent = '⚙️ Advanced Settings'; + advancedToggleLabel.textContent = i18nString(UIStrings.advancedSettingsLabel); advancedToggleContainer.appendChild(advancedToggleLabel); const advancedToggleHint = document.createElement('div'); advancedToggleHint.className = 'settings-hint'; - advancedToggleHint.textContent = - 'Show advanced configuration options (Browsing History, Vector DB, Tracing, Evaluation)'; + advancedToggleHint.textContent = i18nString(UIStrings.advancedSettingsHint); advancedToggleSection.appendChild(advancedToggleHint);
55-60: Keep ARIA in sync with state.Update aria-expanded on change.
advancedToggleCheckbox.addEventListener('change', () => { const isEnabled = advancedToggleCheckbox.checked; + advancedToggleCheckbox.setAttribute('aria-expanded', String(isEnabled)); setStorageBoolean(ADVANCED_SETTINGS_ENABLED_KEY, isEnabled); onChange(isEnabled); });front_end/panels/ai_chat/ui/settings/advanced/BrowsingHistorySettings.ts (2)
41-46: Announce status to assistive tech and use UIStrings.- this.statusMessage = document.createElement('div'); + this.statusMessage = document.createElement('div'); this.statusMessage.className = 'settings-status history-status'; this.statusMessage.style.display = 'none'; - this.statusMessage.textContent = i18nString('historyCleared'); + this.statusMessage.setAttribute('aria-live', 'polite'); + this.statusMessage.textContent = i18nString(UIStrings.historyCleared);
48-75: Add confirm dialog and disable button during async.Avoid accidental data loss and prevent double clicks. Localize button label.
- clearHistoryButton.textContent = i18nString('clearHistoryButton'); + clearHistoryButton.textContent = i18nString(UIStrings.clearHistoryButton); @@ - clearHistoryButton.addEventListener('click', async () => { + clearHistoryButton.addEventListener('click', async () => { + // Confirm destructive action + // Add UIStrings.clearHistoryConfirm: 'Clear all stored browsing history? This cannot be undone.' + if (!window.confirm(i18nString(UIStrings.clearHistoryConfirm))) { + return; + } + clearHistoryButton.disabled = true; try { @@ - if (this.statusMessage) { + if (this.statusMessage) { this.statusMessage.style.display = 'block'; @@ - } + } } catch (error) { logger.error('Error clearing browsing history:', error); } + clearHistoryButton.disabled = false; });front_end/panels/ai_chat/ui/settings/advanced/EvaluationSettings.ts (2)
123-177: Localize client ID labels and hints; avoid hard-coded placeholders.Add UIStrings for clientIdLabel, clientIdHint, and clientIdAutoGenerated.
- clientIdLabel.textContent = 'Client ID'; + clientIdLabel.textContent = i18nString(UIStrings.clientIdLabel); @@ - clientIdHint.textContent = 'Unique identifier for this DevTools instance'; + clientIdHint.textContent = i18nString(UIStrings.clientIdHint); @@ - clientIdInput.value = currentEvaluationConfig.clientId || 'Auto-generated on first connection'; + clientIdInput.value = currentEvaluationConfig.clientId || i18nString(UIStrings.clientIdAutoGenerated); @@ - evaluationEndpointLabel.textContent = i18nString('evaluationEndpoint'); + evaluationEndpointLabel.textContent = i18nString(UIStrings.evaluationEndpoint); @@ - evaluationEndpointHint.textContent = i18nString('evaluationEndpointHint'); + evaluationEndpointHint.textContent = i18nString(UIStrings.evaluationEndpointHint); @@ - evaluationSecretKeyLabel.textContent = i18nString('evaluationSecretKey'); + evaluationSecretKeyLabel.textContent = i18nString(UIStrings.evaluationSecretKey); @@ - evaluationSecretKeyHint.textContent = i18nString('evaluationSecretKeyHint'); + evaluationSecretKeyHint.textContent = i18nString(UIStrings.evaluationSecretKeyHint);
36-39: Note on innerHTML lint warning.
this.container.innerHTML = ''is safe here (constant empty string). If the repo enforces the rule strictly, replace with a loop removing children.- this.container.innerHTML = ''; + while (this.container.firstChild) { + this.container.removeChild(this.container.firstChild); + }front_end/panels/ai_chat/ui/settings/advanced/TracingSettings.ts (3)
26-28: Avoid innerHTML; use safer DOM APIs.Replace with replaceChildren() to eliminate XSS lint and avoid layout thrash.
- this.container.innerHTML = ''; + this.container.replaceChildren();As per static analysis hints.
142-145: Localize user-visible strings.Replace hard-coded English with i18n keys to pass l10n lint and for translation.
- testTracingStatus.textContent = 'Testing connection...'; + testTracingStatus.textContent = i18nString(UIStrings.testingTracingConnection); @@ - throw new Error('All fields are required for testing'); + throw new Error(i18nString(UIStrings.tracingAllFieldsRequired)); @@ - testTracingStatus.textContent = '✓ Connection successful'; + testTracingStatus.textContent = i18nString(UIStrings.tracingConnectionSuccess); @@ - throw new Error(`HTTP ${response.status}: ${errorText}`); + throw new Error(i18nString(UIStrings.tracingHttpError, { PH1: String(response.status), PH2: errorText })); @@ - testTracingStatus.textContent = `✗ ${error instanceof Error ? error.message : 'Connection failed'}`; + testTracingStatus.textContent = error instanceof Error + ? error.message + : i18nString(UIStrings.tracingConnectionFailed);Note: add the missing UIStrings in i18n-strings.ts.
Also applies to: 151-153, 178-185, 187-189
199-220: Validate on save; avoid silent no-op and stale config.If enabled but incomplete, either disable tracing or surface error. Minimal option: disable.
save(): void { if (!this.tracingEnabledCheckbox || !this.endpointInput || !this.publicKeyInput || !this.secretKeyInput) { return; } if (this.tracingEnabledCheckbox.checked) { const endpoint = this.endpointInput.value.trim(); const publicKey = this.publicKeyInput.value.trim(); const secretKey = this.secretKeyInput.value.trim(); - if (endpoint && publicKey && secretKey) { + if (endpoint && publicKey && secretKey) { setTracingConfig({ provider: 'langfuse', endpoint, publicKey, secretKey }); - } + } else { + // Incomplete config; disable to avoid half-enabled state + setTracingConfig({ provider: 'disabled' }); + } } else { setTracingConfig({ provider: 'disabled' }); } }Alternatively, bubble a validation error to the footer and keep previous config.
front_end/panels/ai_chat/ui/settings/advanced/VectorDBSettings.ts (2)
35-37: Avoid innerHTML; use replaceChildren().Safer and passes XSS lint.
- this.container.innerHTML = ''; + this.container.replaceChildren();As per static analysis hints.
236-242: Localize literals and avoidanyin catch.Use i18n for messages and prefer
unknownwith narrowing.- vectorDBTestStatus.textContent = 'Please enter an endpoint URL'; + vectorDBTestStatus.textContent = i18nString(UIStrings.vectorDBEnterEndpoint); @@ - vectorDBTestStatus.textContent = i18nString('testingVectorDBConnection'); + vectorDBTestStatus.textContent = i18nString(UIStrings.testingVectorDBConnection); @@ - if (testResult.success) { - vectorDBTestStatus.textContent = i18nString('vectorDBConnectionSuccess'); + if (testResult.success) { + vectorDBTestStatus.textContent = i18nString(UIStrings.vectorDBConnectionSuccess); vectorDBTestStatus.style.color = 'var(--color-accent-green)'; } else { - vectorDBTestStatus.textContent = `${i18nString('vectorDBConnectionFailed')}: ${testResult.error}`; + vectorDBTestStatus.textContent = `${i18nString(UIStrings.vectorDBConnectionFailed)}: ${testResult.error}`; vectorDBTestStatus.style.color = 'var(--color-accent-red)'; } - } catch (error: any) { - vectorDBTestStatus.textContent = `${i18nString('vectorDBConnectionFailed')}: ${error.message}`; + } catch (error: unknown) { + const msg = error instanceof Error ? error.message : String(error); + vectorDBTestStatus.textContent = `${i18nString(UIStrings.vectorDBConnectionFailed)}: ${msg}`; vectorDBTestStatus.style.color = 'var(--color-accent-red)'; } finally {As per static analysis hints.
Also applies to: 246-249, 263-273
front_end/panels/ai_chat/ui/settings/components/SettingsFooter.ts (3)
18-21: Fix JSDoc formatting (no hyphen before descriptions).Conforms to jsdoc/require-hyphen-before-param-description rule.
- * @param container - Parent element to append the footer to - * @param onCancel - Callback function when cancel button is clicked - * @param onSave - Callback function when save button is clicked + * @param container Parent element to append the footer to + * @param onCancel Callback function when cancel button is clicked + * @param onSave Callback function when save button is clicked @@ - * @param statusElement - The status message element - * @param message - Message to display - * @param type - Type of message (info, success, error) - * @param duration - How long to show the message (ms), 0 = don't auto-hide + * @param statusElement The status message element + * @param message Message to display + * @param type Type of message (info, success, error) + * @param duration How long to show the message (ms), 0 = don't auto-hideAs per static analysis hints.
Also applies to: 66-70
41-53: Localize button labels.Replace literals with i18n strings.
- cancelButton.textContent = 'Cancel'; + cancelButton.textContent = i18nString(UIStrings.cancel); @@ - saveButton.textContent = 'Save'; + saveButton.textContent = i18nString(UIStrings.save);Note: import { i18nString, UIStrings } from '../i18n-strings.js'.
74-99: Remove redundant type annotation and cleanup overlapping timers.Simplify parameter type and clear any previous hide timer to avoid premature hide.
-export function showFooterStatus( +export function showFooterStatus( statusElement: HTMLElement, message: string, type: 'info' | 'success' | 'error' = 'info', - duration: number = 3000, + duration = 3000, ): void { statusElement.textContent = message; statusElement.style.display = 'block'; @@ - if (duration > 0) { - setTimeout(() => { - statusElement.style.display = 'none'; - }, duration); - } + if (duration > 0) { + // @ts-expect-error attach timer id on element + if ((statusElement as any).__footerTimerId) { + clearTimeout((statusElement as any).__footerTimerId); + } + // @ts-expect-error store timer id on element + (statusElement as any).__footerTimerId = setTimeout(() => { + statusElement.style.display = 'none'; + // @ts-expect-error cleanup + (statusElement as any).__footerTimerId = undefined; + }, duration); + }As per static analysis hints.
front_end/panels/ai_chat/ui/settings/types.ts (1)
36-43: Unify option shape with ModelOption for consistency.If consumers can handle it, prefer ModelOption[] to keep type parity across UI.
- options: Array<{value: string, label: string}>; + options: ModelOption[];If not feasible now, add a TODO to migrate later.
front_end/panels/ai_chat/ui/settings/providers/GroqSettings.ts (3)
5-13: Fix import order and groupings to satisfy lint.Place utility/type imports before provider class import, and separate groups with a blank line.
-import { BaseProviderSettings } from './BaseProviderSettings.js'; -import { createModelSelector, refreshModelSelectOptions } from '../components/ModelSelectorFactory.js'; -import { i18nString } from '../i18n-strings.js'; -import { getValidModelForProvider } from '../utils/validation.js'; -import { getStorageItem, setStorageItem } from '../utils/storage.js'; -import { GROQ_API_KEY_STORAGE_KEY, MINI_MODEL_STORAGE_KEY, NANO_MODEL_STORAGE_KEY } from '../constants.js'; -import type { UpdateModelOptionsFunction, GetModelOptionsFunction, AddCustomModelOptionFunction, RemoveCustomModelOptionFunction, ModelOption } from '../types.js'; -import { LLMClient } from '../../../LLM/LLMClient.js'; -import { createLogger } from '../../../core/Logger.js'; +import { createModelSelector, refreshModelSelectOptions } from '../components/ModelSelectorFactory.js'; +import { i18nString, UIStrings } from '../i18n-strings.js'; +import { getValidModelForProvider } from '../utils/validation.js'; +import { getStorageItem, setStorageItem } from '../utils/storage.js'; +import { GROQ_API_KEY_STORAGE_KEY, MINI_MODEL_STORAGE_KEY, NANO_MODEL_STORAGE_KEY } from '../constants.js'; +import type { UpdateModelOptionsFunction, GetModelOptionsFunction, AddCustomModelOptionFunction, RemoveCustomModelOptionFunction, ModelOption } from '../types.js'; +import { LLMClient } from '../../../LLM/LLMClient.js'; +import { createLogger } from '../../../core/Logger.js'; + +import { BaseProviderSettings } from './BaseProviderSettings.js';
51-58: Use UIStrings with i18nString; avoid raw string keys and hardcoded text.Switch to i18nString(UIStrings.key) and externalize “Model Size Selection” and placeholders.
- groqApiKeyLabel.textContent = i18nString('groqApiKeyLabel'); - groqApiKeyHint.textContent = i18nString('groqApiKeyHint'); + groqApiKeyLabel.textContent = i18nString(UIStrings.groqApiKeyLabel); + groqApiKeyHint.textContent = i18nString(UIStrings.groqApiKeyHint); - this.fetchModelsButton.textContent = i18nString('fetchGroqModelsButton'); + this.fetchModelsButton.textContent = i18nString(UIStrings.fetchGroqModelsButton); - this.fetchModelsStatus.textContent = i18nString('fetchingModels'); + this.fetchModelsStatus.textContent = i18nString(UIStrings.fetchingModels); - refreshModelSelectOptions(this.miniModelSelector as any, allGroqModels, miniModel, i18nString('defaultMiniOption')); + refreshModelSelectOptions(this.miniModelSelector as unknown as ModelSelectorElement, allGroqModels, miniModel, i18nString(UIStrings.defaultMiniOption)); - refreshModelSelectOptions(this.nanoModelSelector as any, allGroqModels, nanoModel, i18nString('defaultNanoOption')); + refreshModelSelectOptions(this.nanoModelSelector as unknown as ModelSelectorElement, allGroqModels, nanoModel, i18nString(UIStrings.defaultNanoOption)); - this.fetchModelsStatus.textContent = i18nString('fetchedModels', {PH1: actualModelCount}); + this.fetchModelsStatus.textContent = i18nString(UIStrings.fetchedModels, {PH1: actualModelCount}); - groqModelSectionTitle.textContent = 'Model Size Selection'; + // TODO: add UIStrings.modelSizeSelectionTitle in i18n-strings.ts + groqModelSectionTitle.textContent = i18nString(UIStrings.modelSizeSelectionTitle); - i18nString('miniModelLabel'), i18nString('miniModelDescription'), i18nString('defaultMiniOption'), + i18nString(UIStrings.miniModelLabel), i18nString(UIStrings.miniModelDescription), i18nString(UIStrings.defaultMiniOption), - i18nString('nanoModelLabel'), i18nString('nanoModelDescription'), i18nString('defaultNanoOption'), + i18nString(UIStrings.nanoModelLabel), i18nString(UIStrings.nanoModelDescription), i18nString(UIStrings.defaultNanoOption),Please confirm UIStrings include all referenced keys or share the list so I can generate a patch to add the missing ones.
Also applies to: 75-76, 96-101, 127-131, 133-136, 197-204, 211-218
92-99: Wrap single-line if statements with braces.Satisfies curly rule and improves readability.
- if (!this.fetchModelsButton || !this.fetchModelsStatus || !this.apiKeyInput) return; + if (!this.fetchModelsButton || !this.fetchModelsStatus || !this.apiKeyInput) { + return; + } - if (!this.container) return; + if (!this.container) { + return; + }Also applies to: 161-166
front_end/panels/ai_chat/ui/settings/components/ModelSelectorFactory.ts (2)
11-19: Fix JSDoc: remove hyphens before @param descriptions.Unblocks jsdoc lint.
- * @param container - Parent element to append the selector to - * @param labelText - Label text for the selector - * @param description - Description text below the label - * @param selectorType - Semantic identifier for the selector - * @param modelOptions - Available model options - * @param selectedModel - Currently selected model value - * @param defaultOptionText - Text for the default option - * @param onFocus - Optional callback for when the selector is opened/focused + * @param container Parent element to append the selector to + * @param labelText Label text for the selector + * @param description Description text below the label + * @param selectorType Semantic identifier for the selector + * @param modelOptions Available model options + * @param selectedModel Currently selected model value + * @param defaultOptionText Text for the default option + * @param onFocus Optional callback for when the selector is opened/focused @@ - * @param select - The model selector element - * @param models - New model options - * @param currentValue - Current selected value - * @param defaultLabel - Label for the default option + * @param select The model selector element + * @param models New model options + * @param currentValue Current selected value + * @param defaultLabel Label for the default optionAlso applies to: 74-82
94-101: Minor style: drop unnecessary parens around single arrow params.Keeps style consistent with lint.
- if (previousValue && opts.some((o) => o.value === previousValue)) { + if (previousValue && opts.some(o => o.value === previousValue)) { @@ - } else if (currentValue && opts.some((o) => o.value === currentValue)) { + } else if (currentValue && opts.some(o => o.value === currentValue)) { @@ - if (previousValue && Array.from(nativeSelect.options).some((opt) => opt.value === previousValue)) { + if (previousValue && Array.from(nativeSelect.options).some(opt => opt.value === previousValue)) { - } else if (currentValue && Array.from(nativeSelect.options).some((opt) => opt.value === currentValue)) { + } else if (currentValue && Array.from(nativeSelect.options).some(opt => opt.value === currentValue)) {Also applies to: 116-120
front_end/panels/ai_chat/ui/settings/providers/LiteLLMSettings.ts (8)
5-14: Fix import order; remove unused ModelOption; add UIStrings/type for selector.Aligns with lint and prepares to eliminate any-casts.
-import { BaseProviderSettings } from './BaseProviderSettings.js'; -import { createModelSelector, refreshModelSelectOptions } from '../components/ModelSelectorFactory.js'; -import { i18nString } from '../i18n-strings.js'; +import { createModelSelector, refreshModelSelectOptions } from '../components/ModelSelectorFactory.js'; +import { i18nString, UIStrings } from '../i18n-strings.js'; import { getValidModelForProvider } from '../utils/validation.js'; import { getStorageItem, setStorageItem } from '../utils/storage.js'; import { LITELLM_ENDPOINT_KEY, LITELLM_API_KEY_STORAGE_KEY, MINI_MODEL_STORAGE_KEY, NANO_MODEL_STORAGE_KEY } from '../constants.js'; -import type { UpdateModelOptionsFunction, GetModelOptionsFunction, AddCustomModelOptionFunction, RemoveCustomModelOptionFunction, FetchLiteLLMModelsFunction, ModelOption } from '../types.js'; +import type { UpdateModelOptionsFunction, GetModelOptionsFunction, AddCustomModelOptionFunction, RemoveCustomModelOptionFunction, FetchLiteLLMModelsFunction, ModelSelectorElement } from '../types.js'; import { LLMClient } from '../../../LLM/LLMClient.js'; import { createLogger } from '../../../core/Logger.js'; + +import { BaseProviderSettings } from './BaseProviderSettings.js';
30-33: Drop trivial type annotation.Let TS infer boolean.
-private testPassed: boolean = false; +private testPassed = false;
47-57: Use UIStrings with i18nString; externalize hardcoded strings.Replace string keys and literal messages with i18nString(UIStrings.*).
- litellmEndpointLabel.textContent = i18nString('litellmEndpointLabel'); - litellmEndpointHint.textContent = i18nString('litellmEndpointHint'); + litellmEndpointLabel.textContent = i18nString(UIStrings.litellmEndpointLabel); + litellmEndpointHint.textContent = i18nString(UIStrings.litellmEndpointHint); @@ - this.fetchModelsButton.textContent = i18nString('fetchModelsButton'); + this.fetchModelsButton.textContent = i18nString(UIStrings.fetchModelsButton); @@ - this.fetchModelsStatus.textContent = i18nString('fetchingModels'); + this.fetchModelsStatus.textContent = i18nString(UIStrings.fetchingModels); @@ - refreshModelSelectOptions(this.miniModelSelector as any, allLiteLLMModels, miniModel, i18nString('defaultMiniOption')); + refreshModelSelectOptions(this.miniModelSelector as unknown as ModelSelectorElement, allLiteLLMModels, miniModel, i18nString(UIStrings.defaultMiniOption)); @@ - refreshModelSelectOptions(this.nanoModelSelector as any, allLiteLLMModels, nanoModel, i18nString('defaultNanoOption')); + refreshModelSelectOptions(this.nanoModelSelector as unknown as ModelSelectorElement, allLiteLLMModels, nanoModel, i18nString(UIStrings.defaultNanoOption)); @@ - this.fetchModelsStatus.textContent = i18nString('wildcardModelsOnly'); + this.fetchModelsStatus.textContent = i18nString(UIStrings.wildcardModelsOnly); @@ - this.fetchModelsStatus.textContent = i18nString('wildcardAndCustomModels'); + this.fetchModelsStatus.textContent = i18nString(UIStrings.wildcardAndCustomModels); @@ - this.fetchModelsStatus.textContent = i18nString('wildcardAndOtherModels', {PH1: actualModelCount}); + this.fetchModelsStatus.textContent = i18nString(UIStrings.wildcardAndOtherModels, {PH1: actualModelCount}); @@ - this.fetchModelsStatus.textContent = i18nString('fetchedModels', {PH1: actualModelCount}); + this.fetchModelsStatus.textContent = i18nString(UIStrings.fetchedModels, {PH1: actualModelCount}); @@ - customModelsLabel.textContent = i18nString('customModelsLabel'); - customModelsHint.textContent = i18nString('customModelsHint'); + customModelsLabel.textContent = i18nString(UIStrings.customModelsLabel); + customModelsHint.textContent = i18nString(UIStrings.customModelsHint); @@ - addModelButton.textContent = i18nString('addButton'); + addModelButton.textContent = i18nString(UIStrings.addButton); @@ - testButton.setAttribute('aria-label', i18nString('testButton')); + testButton.setAttribute('aria-label', i18nString(UIStrings.testButton)); @@ - removeButton.setAttribute('aria-label', i18nString('removeButton')); + removeButton.setAttribute('aria-label', i18nString(UIStrings.removeButton)); @@ - throw new Error(i18nString('endpointRequired')); + throw new Error(i18nString(UIStrings.endpointRequired)); @@ - this.modelTestStatus.textContent = 'Please enter a model name'; + this.modelTestStatus.textContent = i18nString(UIStrings.enterModelNamePrompt); @@ - this.modelTestStatus.textContent = 'Testing model...'; + this.modelTestStatus.textContent = i18nString(UIStrings.testingModel); @@ - this.modelTestStatus.textContent = `Test passed: ${result.message}`; + this.modelTestStatus.textContent = i18nString(UIStrings.testPassedMessage, {PH1: result.message}); @@ - this.modelTestStatus.textContent = `Test failed: ${result.message}`; + this.modelTestStatus.textContent = i18nString(UIStrings.testFailedMessage, {PH1: result.message}); @@ - i18nString('miniModelLabel'), i18nString('miniModelDescription'), i18nString('defaultMiniOption'), + i18nString(UIStrings.miniModelLabel), i18nString(UIStrings.miniModelDescription), i18nString(UIStrings.defaultMiniOption), @@ - i18nString('nanoModelLabel'), i18nString('nanoModelDescription'), i18nString('defaultNanoOption'), + i18nString(UIStrings.nanoModelLabel), i18nString(UIStrings.nanoModelDescription), i18nString(UIStrings.defaultNanoOption),Please confirm UIStrings includes the referenced keys or share the file so I can add missing entries.
Also applies to: 76-85, 107-113, 123-131, 147-151, 154-172, 197-205, 228-235, 334-336, 349-355, 419-426, 429-433, 439-444, 466-473, 517-526, 531-540, 545-557
94-101: Add braces to single-line if/return and annotate function return types.Unblocks curly and explicit return type rules.
- const updateFetchButtonState = () => { + const updateFetchButtonState = (): void => { @@ - if (!this.fetchModelsButton || !this.fetchModelsStatus || !this.endpointInput || !this.apiKeyInput) return; + if (!this.fetchModelsButton || !this.fetchModelsStatus || !this.endpointInput || !this.apiKeyInput) { + return; + } @@ - if (!this.customModelInput) return; + if (!this.customModelInput) { + return; + } @@ - try { + try { @@ - } finally { + } finally { @@ - private async testModelConnection(modelName: string): Promise<boolean> { - if (!this.modelTestStatus) return false; + private async testModelConnection(modelName: string): Promise<boolean> { + if (!this.modelTestStatus) { + return false; + } @@ - if (!endpoint) { - throw new Error(i18nString('endpointRequired')); - } + if (!endpoint) { + throw new Error(i18nString(UIStrings.endpointRequired)); + } @@ - updateModelSelectors(): void { - if (!this.container) return; + updateModelSelectors(): void { + if (!this.container) { + return; + }Also applies to: 119-122, 236-243, 369-376, 398-402, 418-426, 434-441, 466-473
497-515: Annotate async focus handler return type.Satisfies explicit return type rule.
- const onLiteLLMSelectorFocus = async () => { + const onLiteLLMSelectorFocus = async (): Promise<void> => {
303-317: Avoid non-null assertion on DOM element.Use a local const after null-check.
- // Clear existing list - this.customModelsList.innerHTML = ''; + // Clear existing list + const list = this.customModelsList; + if (!list) return; + list.innerHTML = ''; @@ - this.customModelsList!.appendChild(modelRow); + list.appendChild(modelRow);
338-346: Replace innerHTML SVG injection with createElementNS to silence unsafe-html rules.Static strings are relatively safe, but using DOM APIs avoids the rule entirely.
- const checkIcon = document.createElement('span'); - checkIcon.className = 'check-icon'; - checkIcon.innerHTML = ` - <svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg"> - <path d="M12 5L6.5 10.5L4 8" stroke="currentColor" stroke-width="1.5" stroke-linecap="round" stroke-linejoin="round"/> - <circle cx="8" cy="8" r="6.25" stroke="currentColor" stroke-width="1.5"/> - </svg> - `; + const checkIcon = document.createElement('span'); + checkIcon.className = 'check-icon'; + const svg = document.createElementNS('http://www.w3.org/2000/svg', 'svg'); + svg.setAttribute('width', '16'); svg.setAttribute('height', '16'); svg.setAttribute('viewBox', '0 0 16 16'); + const p = document.createElementNS('http://www.w3.org/2000/svg', 'path'); + p.setAttribute('d','M12 5L6.5 10.5L4 8'); p.setAttribute('stroke','currentColor'); p.setAttribute('stroke-width','1.5'); p.setAttribute('stroke-linecap','round'); p.setAttribute('stroke-linejoin','round'); + const c = document.createElementNS('http://www.w3.org/2000/svg', 'circle'); + c.setAttribute('cx','8'); c.setAttribute('cy','8'); c.setAttribute('r','6.25'); c.setAttribute('stroke','currentColor'); c.setAttribute('stroke-width','1.5'); + svg.appendChild(p); svg.appendChild(c); checkIcon.appendChild(svg); @@ - const trashIcon = document.createElement('span'); - trashIcon.className = 'trash-icon'; - trashIcon.innerHTML = ` - <svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg"> - <path d="M6 2.5V2C6 1.44772 6.44772 1 7 1H9C9.55228 1 10 1.44772 10 2V2.5M2 2.5H14M12.5 2.5V13C12.5 13.5523 12.0523 14 11.5 14H4.5C3.94772 14 3.5 13.5523 3.5 13V2.5M5.5 5.5V11M8 5.5V11M10.5 5.5V11" - stroke="currentColor" stroke-width="1.2" stroke-linecap="round" stroke-linejoin="round"/> - </svg> - `; + const trashIcon = document.createElement('span'); + trashIcon.className = 'trash-icon'; + // Build minimal trash icon or reuse existing icon component here...Also applies to: 357-365
444-456: Simplify no-else-after-return in testModelConnection.Conforms to lint and reduces nesting.
- if (result.success) { - this.modelTestStatus.textContent = `Test passed: ${result.message}`; - this.modelTestStatus.style.backgroundColor = 'var(--color-accent-green-background)'; - this.modelTestStatus.style.color = 'var(--color-accent-green)'; - this.testPassed = true; - return true; - } else { - this.modelTestStatus.textContent = `Test failed: ${result.message}`; - this.modelTestStatus.style.backgroundColor = 'var(--color-accent-red-background)'; - this.modelTestStatus.style.color = 'var(--color-accent-red)'; - this.testPassed = false; - return false; - } + if (result.success) { + this.modelTestStatus.textContent = i18nString(UIStrings.testPassedMessage, {PH1: result.message}); + this.modelTestStatus.style.backgroundColor = 'var(--color-accent-green-background)'; + this.modelTestStatus.style.color = 'var(--color-accent-green)'; + this.testPassed = true; + return true; + } + this.modelTestStatus.textContent = i18nString(UIStrings.testFailedMessage, {PH1: result.message}); + this.modelTestStatus.style.backgroundColor = 'var(--color-accent-red-background)'; + this.modelTestStatus.style.color = 'var(--color-accent-red)'; + this.testPassed = false; + return false;front_end/panels/ai_chat/ui/settings/utils/validation.ts (1)
59-69: Optional: accept schemeless endpoints.Many users type host:port; consider auto-prefixing http:// for valid URL() parsing.
-export function validateEndpoint(endpoint: string): boolean { +export function validateEndpoint(endpoint: string): boolean { if (!endpoint.trim()) { return false; } try { - new URL(endpoint); + const candidate = /^https?:\/\//i.test(endpoint) ? endpoint : `http://${endpoint}`; + new URL(candidate); return true; } catch { return false; } }front_end/panels/ai_chat/ui/settings/providers/OpenAISettings.ts (3)
5-12: Fix import order and add UIStrings import.-import { BaseProviderSettings } from './BaseProviderSettings.js'; -import { createModelSelector } from '../components/ModelSelectorFactory.js'; -import { i18nString } from '../i18n-strings.js'; +import { createModelSelector } from '../components/ModelSelectorFactory.js'; +import { i18nString, UIStrings } from '../i18n-strings.js'; import { getValidModelForProvider } from '../utils/validation.js'; import { getStorageItem, setStorageItem } from '../utils/storage.js'; import { OPENAI_API_KEY_STORAGE_KEY, MINI_MODEL_STORAGE_KEY, NANO_MODEL_STORAGE_KEY } from '../constants.js'; import type { GetModelOptionsFunction, AddCustomModelOptionFunction, RemoveCustomModelOptionFunction } from '../types.js'; + +import { BaseProviderSettings } from './BaseProviderSettings.js';
41-50: Use UIStrings and externalize title.- apiKeyLabel.textContent = i18nString('apiKeyLabel'); - apiKeyHint.textContent = i18nString('apiKeyHint'); + apiKeyLabel.textContent = i18nString(UIStrings.apiKeyLabel); + apiKeyHint.textContent = i18nString(UIStrings.apiKeyHint); @@ - this.apiKeyInput.placeholder = 'Enter your OpenAI API key'; + // TODO: add UIStrings.openAiApiKeyPlaceholder + this.apiKeyInput.placeholder = i18nString(UIStrings.openAiApiKeyPlaceholder); @@ - openaiModelSectionTitle.textContent = 'Model Size Selection'; + // TODO: add UIStrings.modelSizeSelectionTitle + openaiModelSectionTitle.textContent = i18nString(UIStrings.modelSizeSelectionTitle); @@ - i18nString('miniModelLabel'), i18nString('miniModelDescription'), i18nString('defaultMiniOption'), + i18nString(UIStrings.miniModelLabel), i18nString(UIStrings.miniModelDescription), i18nString(UIStrings.defaultMiniOption), @@ - i18nString('nanoModelLabel'), i18nString('nanoModelDescription'), i18nString('defaultNanoOption'), + i18nString(UIStrings.nanoModelLabel), i18nString(UIStrings.nanoModelDescription), i18nString(UIStrings.defaultNanoOption),Also applies to: 55-57, 91-95, 99-108, 111-120
68-76: Wrap single-line if with braces.- if (!this.container) return; + if (!this.container) { + return; + }front_end/panels/ai_chat/ui/settings/utils/storage.ts (3)
8-10: Remove trivially inferrable type on defaulted parameter to satisfy ESLint.-export function getStorageItem(key: string, defaultValue: string = ''): string { +export function getStorageItem(key: string, defaultValue = ''): string { return localStorage.getItem(key) || defaultValue; }
26-32: Same here: drop redundant boolean annotation on default param.-export function getStorageBoolean(key: string, defaultValue: boolean = false): boolean { +export function getStorageBoolean(key: string, defaultValue = false): boolean { const value = localStorage.getItem(key); if (value === null) { return defaultValue; } return value === 'true'; }
15-21: Harden writes to handle quota/security exceptions; avoid breaking UI.export function setStorageItem(key: string, value: string): void { - if (value.trim()) { - localStorage.setItem(key, value); - } else { - localStorage.removeItem(key); - } + try { + if (value.trim()) { + localStorage.setItem(key, value); + } else { + localStorage.removeItem(key); + } + } catch { + // noop: storage unavailable or quota exceeded + } } ... export function setStorageJSON<T>(key: string, value: T): void { - localStorage.setItem(key, JSON.stringify(value)); + try { + localStorage.setItem(key, JSON.stringify(value)); + } catch { + // noop: storage unavailable or quota exceeded + } }Also applies to: 59-61
front_end/panels/ai_chat/ui/settings/providers/OpenRouterSettings.ts (8)
5-13: Fix import order per lint (place BaseProviderSettings after sibling imports).-import { BaseProviderSettings } from './BaseProviderSettings.js'; -import { createModelSelector } from '../components/ModelSelectorFactory.js'; +import { createModelSelector } from '../components/ModelSelectorFactory.js'; import { i18nString } from '../i18n-strings.js'; import { getValidModelForProvider } from '../utils/validation.js'; import { getStorageItem, setStorageItem } from '../utils/storage.js'; import { OPENROUTER_API_KEY_STORAGE_KEY, MINI_MODEL_STORAGE_KEY, NANO_MODEL_STORAGE_KEY, OPENROUTER_MODELS_CACHE_DURATION_MS } from '../constants.js'; import type { UpdateModelOptionsFunction, GetModelOptionsFunction, AddCustomModelOptionFunction, RemoveCustomModelOptionFunction, ModelOption } from '../types.js'; import { LLMClient } from '../../../LLM/LLMClient.js'; import { createLogger } from '../../../core/Logger.js'; +import { BaseProviderSettings } from './BaseProviderSettings.js';
68-71: Replace innerHTML clearing with safe child removal to satisfy XSS/static checks.- this.container.innerHTML = ''; + while (this.container.firstChild) { + this.container.removeChild(this.container.firstChild); + }
118-164: Prevent duplicating injected styles across renders; add id and guard.- const oauthStyles = document.createElement('style'); - oauthStyles.textContent = ` + if (!document.getElementById('openrouter-oauth-styles')) { + const oauthStyles = document.createElement('style'); + oauthStyles.id = 'openrouter-oauth-styles'; + oauthStyles.textContent = ` .settings-divider { text-align: center; margin: 15px 0; color: var(--color-text-secondary); font-size: 12px; font-weight: bold; } .oauth-button-container { margin-bottom: 10px; } .oauth-button { background: linear-gradient(135deg, #667eea 0%, #764ba2 100%); color: white; border: none; padding: 10px 20px; border-radius: 6px; cursor: pointer; font-weight: 500; transition: all 0.3s ease; width: 100%; margin-bottom: 8px; } .oauth-button:hover { transform: translateY(-1px); box-shadow: 0 4px 12px rgba(102, 126, 234, 0.3); } .oauth-button:disabled { opacity: 0.6; cursor: not-allowed; transform: none; box-shadow: none; } .oauth-button.disconnect { background: linear-gradient(135deg, #f093fb 0%, #f5576c 100%); } .oauth-status { font-size: 12px; margin-top: 5px; padding: 5px 8px; border-radius: 4px; background: var(--color-background-highlight); } - `; - document.head.appendChild(oauthStyles); + `; + document.head.appendChild(oauthStyles); + }
191-199: Comply with curly rule; avoid single-line early-returns and if-statements without braces.- this.oauthButton.addEventListener('click', async () => { - if (!this.oauthButton) return; + this.oauthButton.addEventListener('click', async () => { + if (!this.oauthButton) { + return; + } ... - } finally { - this.oauthButton.disabled = false; - if (!await OAuth.isOAuthAuthenticated()) { + } finally { + this.oauthButton.disabled = false; + if (!await OAuth.isOAuthAuthenticated()) { this.oauthButton.textContent = 'Connect with OpenRouter'; if (this.oauthStatus) { this.oauthStatus.style.display = 'none'; } } } }); ... - this.apiKeyInput.addEventListener('input', async () => { - if (!this.apiKeyInput) return; + this.apiKeyInput.addEventListener('input', async () => { + if (!this.apiKeyInput) { + return; + } ... - if (this.fetchModelsButton) { - this.fetchModelsButton.disabled = !this.apiKeyInput.value.trim(); - } + if (this.fetchModelsButton) { + this.fetchModelsButton.disabled = !this.apiKeyInput.value.trim(); + } ... - this.fetchModelsButton.addEventListener('click', async () => { - if (!this.fetchModelsButton || !this.fetchModelsStatus || !this.apiKeyInput) return; + this.fetchModelsButton.addEventListener('click', async () => { + if (!this.fetchModelsButton || !this.fetchModelsStatus || !this.apiKeyInput) { + return; + }Also applies to: 225-232, 298-304, 312-315, 335-339, 456-458
245-249: Remove any by narrowing the queried element type; keep null-safe.- const chatPanel = document.querySelector('ai-chat-panel') as any; - if (chatPanel && typeof chatPanel.refreshCredentials === 'function') { + const chatPanel = document.querySelector('ai-chat-panel') as (HTMLElement & { refreshCredentials?: () => void }) | null; + if (chatPanel?.refreshCredentials) { chatPanel.refreshCredentials(); }Apply similarly at Lines 283-287.
Also applies to: 283-287
347-359: Consider OAuth path for model fetch/refresh; current logic requires an API key even when OAuth is connected.If OpenRouterOAuth exposes a token or LLMClient supports OAuth tokens, add a branch:
- If OAuth.isOAuthAuthenticated(): use OAuth token to fetch models.
- Else: use API key.
Please confirm the supported method on LLMClient or OpenRouter provider for OAuth-based fetches and I can propose a concrete patch.
Also applies to: 434-441
80-86: i18n: pass UIStrings members instead of raw string keys to satisfy l10n rule.- openrouterApiKeyLabel.textContent = i18nString('openrouterApiKeyLabel'); + openrouterApiKeyLabel.textContent = i18nString(UIStrings.openrouterApiKeyLabel); ... - openrouterApiKeyHint.textContent = i18nString('openrouterApiKeyHint'); + openrouterApiKeyHint.textContent = i18nString(UIStrings.openrouterApiKeyHint); ... - this.fetchModelsButton.textContent = i18nString('fetchOpenRouterModelsButton'); + this.fetchModelsButton.textContent = i18nString(UIStrings.fetchOpenRouterModelsButton); - this.fetchModelsStatus.textContent = i18nString('fetchingModels'); + this.fetchModelsStatus.textContent = i18nString(UIStrings.fetchingModels); ... - this.fetchModelsStatus.textContent = i18nString('fetchedModels', {PH1: actualModelCount}); + this.fetchModelsStatus.textContent = i18nString(UIStrings.fetchedModels, {PH1: actualModelCount}); ... - i18nString('miniModelLabel'), - i18nString('miniModelDescription'), + i18nString(UIStrings.miniModelLabel), + i18nString(UIStrings.miniModelDescription), ... - i18nString('defaultMiniOption'), + i18nString(UIStrings.defaultMiniOption), ... - i18nString('nanoModelLabel'), - i18nString('nanoModelDescription'), + i18nString(UIStrings.nanoModelLabel), + i18nString(UIStrings.nanoModelDescription), ... - i18nString('defaultNanoOption'), + i18nString(UIStrings.defaultNanoOption),Add
import { UIStrings } from '../i18n-strings.js';at the top. Apply similarly for any other i18nString usage in this file. As per coding guidelines.Also applies to: 325-327, 339-343, 369-372, 489-506
361-364: Centralize cache/auth storage keys; avoid magic strings.Define in constants.ts:
+export const OPENROUTER_MODELS_CACHE_KEY = 'openrouter_models_cache'; +export const OPENROUTER_MODELS_CACHE_TS_KEY = 'openrouter_models_cache_timestamp'; +export const OPENROUTER_AUTH_METHOD_KEY = 'openrouter_auth_method';Then use:
- localStorage.setItem('openrouter_models_cache_timestamp', Date.now().toString()); + localStorage.setItem(OPENROUTER_MODELS_CACHE_TS_KEY, Date.now().toString()); ... - localStorage.setItem('openrouter_models_cache', JSON.stringify(modelOptions)); - localStorage.setItem('openrouter_models_cache_timestamp', Date.now().toString()); + localStorage.setItem(OPENROUTER_MODELS_CACHE_KEY, JSON.stringify(modelOptions)); + localStorage.setItem(OPENROUTER_MODELS_CACHE_TS_KEY, Date.now().toString());Also applies to: 443-449
front_end/panels/ai_chat/ui/settings/advanced/MCPSettings.ts (7)
56-66: Avoid innerHTML clearing; use safe child removal.- this.container.innerHTML = ''; + while (this.container.firstChild) { + this.container.removeChild(this.container.firstChild); + } this.container.className = 'settings-section mcp-section';
333-345: Add explicit return type to the arrow handler; keeps lint happy.- const updateBudgetControls = () => { + const updateBudgetControls: () => void = () => { const maxTools = Math.max(1, Math.min(100, parseInt(mcpMaxToolsInput.value, 10) || 20)); const maxMcp = Math.max(1, Math.min(50, parseInt(mcpMaxMcpInput.value, 10) || 8)); setMCPConfig({ maxToolsPerTurn: maxTools, maxMcpPerTurn: maxMcp, }); this.onSettingsSaved(); };
347-368: Conform to curly rule in formatMCPError; minor cleanup.- private formatMCPError(error: string, errorType?: string): {message: string, hint?: string} { - if (!errorType) return {message: error}; + private formatMCPError(error: string, errorType?: string): {message: string, hint?: string} { + if (!errorType) { + return {message: error}; + } switch (errorType) { case 'connection': return {message: `Connection failed: ${error}`, hint: 'Check if the MCP server is running and the endpoint URL is correct.'}; ... default: - return {message: error}; + return {message: error}; } }
438-449: no-lonely-if: collapse the else { if (...) } into else-if for readability.- } else { - if (serverAuthError) { + } else if (serverAuthError) { statusDot.style.backgroundColor = '#ef4444'; statusBadge.style.backgroundColor = '#fee2e2'; statusBadge.style.color = '#991b1b'; statusBadge.textContent = 'Auth Required'; - } else { + } else { statusDot.style.backgroundColor = '#9ca3af'; statusBadge.style.backgroundColor = '#f3f4f6'; statusBadge.style.color = '#6b7280'; statusBadge.textContent = 'Disconnected'; - } - } + }
491-504: Remove non-null assertions; guard once and reuse a local.- this.mcpStatusDetails!.appendChild(row); + const detailsEl = this.mcpStatusDetails; + if (detailsEl) { + detailsEl.appendChild(row); + } ... - this.mcpStatusDetails!.appendChild(errorDetails); + if (detailsEl) { + detailsEl.appendChild(errorDetails); + }
375-383: Minor perf: compute auth errors map once; avoids per-server scans.- const appendServerRow = (server: typeof status.servers[number], isConnected: boolean) => { + const authErrorsById = new Map(getStoredAuthErrors().map(e => [e.serverId, e])); + const appendServerRow = (server: typeof status.servers[number], isConnected: boolean) => { ... - const authErrors = getStoredAuthErrors(); - const serverAuthError = authErrors.find(error => error.serverId === server.id); + const serverAuthError = authErrorsById.get(server.id);Also applies to: 515-543, 575-592
190-231: i18n: ensure i18nString receives UIStrings members, not raw strings; update calls accordingly.Replace:
- i18nString('mcpConnectionsHeader') → i18nString(UIStrings.mcpConnectionsHeader)
- mcp...Hint/Labels/Button texts similarly.
Addimport { UIStrings } from '../i18n-strings.js';. As per coding guidelines.Also applies to: 255-274, 296-330
front_end/panels/ai_chat/ui/settings/constants.ts (2)
28-29: Complement OpenRouter cache/auth keys so other modules avoid magic strings.export const OPENROUTER_MODELS_CACHE_DURATION_MS = 60 * 60 * 1000; // 60 minutes +export const OPENROUTER_MODELS_CACHE_KEY = 'openrouter_models_cache'; +export const OPENROUTER_MODELS_CACHE_TS_KEY = 'openrouter_models_cache_timestamp'; +export const OPENROUTER_AUTH_METHOD_KEY = 'openrouter_auth_method';
19-24: Heads-up: API keys in localStorage persist and are exposed to any script running in the same origin; ensure threat model accepts this.If untrusted content can execute, prefer a more protected store (e.g., chrome.storage with extension-scoped isolation) or limit surface by scoping the origin. Document this in SECURITY.md.
Also applies to: 33-39
| import { i18nString } from '../i18n-strings.js'; | ||
| import * as Common from '../../../../../core/common/common.js'; | ||
|
|
There was a problem hiding this comment.
Import UIStrings and fix import order.
- Place Common import first per import/order.
- Import UIStrings and use it with i18nString.
-import { i18nString } from '../i18n-strings.js';
-import * as Common from '../../../../../core/common/common.js';
+import * as Common from '../../../../../core/common/common.js';
+import { UIStrings, i18nString } from '../ModuleUIStrings.js';Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 ESLint
[error] 6-6: ../../../../../core/common/common.js import should occur before import of ../i18n-strings.js
(import/order)
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/ui/settings/advanced/BrowsingHistorySettings.ts
around lines 5 to 7, the import order should place the Common module before
local modules and you need to import UIStrings and use it with i18nString;
reorder imports so Common is first, add an import for UIStrings from the file
that defines the UI strings (e.g., import { UIStrings } from
'./BrowsingHistorySettings.i18n.js' or the correct path), then replace direct
string literals passed to i18nString with references into UIStrings (i.e.,
i18nString(UIStrings.someKey)) to comply with localization usage and
import/order rules.
| const historyTitle = document.createElement('h3'); | ||
| historyTitle.className = 'settings-subtitle'; | ||
| historyTitle.textContent = i18nString('browsingHistoryTitle'); | ||
| this.container.appendChild(historyTitle); | ||
|
|
||
| // Description | ||
| const historyDescription = document.createElement('p'); | ||
| historyDescription.className = 'settings-description'; | ||
| historyDescription.textContent = i18nString('browsingHistoryDescription'); | ||
| this.container.appendChild(historyDescription); |
There was a problem hiding this comment.
Use i18nString with UIStrings members.
Replace literal keys with UIStrings refs.
- historyTitle.textContent = i18nString('browsingHistoryTitle');
+ historyTitle.textContent = i18nString(UIStrings.browsingHistoryTitle);
@@
- historyDescription.textContent = i18nString('browsingHistoryDescription');
+ historyDescription.textContent = i18nString(UIStrings.browsingHistoryDescription);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const historyTitle = document.createElement('h3'); | |
| historyTitle.className = 'settings-subtitle'; | |
| historyTitle.textContent = i18nString('browsingHistoryTitle'); | |
| this.container.appendChild(historyTitle); | |
| // Description | |
| const historyDescription = document.createElement('p'); | |
| historyDescription.className = 'settings-description'; | |
| historyDescription.textContent = i18nString('browsingHistoryDescription'); | |
| this.container.appendChild(historyDescription); | |
| const historyTitle = document.createElement('h3'); | |
| historyTitle.className = 'settings-subtitle'; | |
| historyTitle.textContent = i18nString(UIStrings.browsingHistoryTitle); | |
| this.container.appendChild(historyTitle); | |
| // Description | |
| const historyDescription = document.createElement('p'); | |
| historyDescription.className = 'settings-description'; | |
| historyDescription.textContent = i18nString(UIStrings.browsingHistoryDescription); | |
| this.container.appendChild(historyDescription); |
🧰 Tools
🪛 ESLint
[error] 29-29: Prefer template literals over imperative DOM API calls
(rulesdir/no-imperative-dom-api)
[error] 31-31: Calling i18nString/i18nLazyString requires a UIStrings member as the first argument (e.g., UIStrings.someString).
(rulesdir/l10n-i18nString-call-only-with-uistrings)
[error] 35-35: Prefer template literals over imperative DOM API calls
(rulesdir/no-imperative-dom-api)
[error] 37-37: Calling i18nString/i18nLazyString requires a UIStrings member as the first argument (e.g., UIStrings.someString).
(rulesdir/l10n-i18nString-call-only-with-uistrings)
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/ui/settings/advanced/BrowsingHistorySettings.ts
around lines 29 to 38, the i18nString calls use literal keys; replace those
literals with the UIStrings members (e.g. use
i18nString(UIStrings.browsingHistoryTitle) and
i18nString(UIStrings.browsingHistoryDescription)), and ensure UIStrings is
imported/available in this module if not already so the calls reference the
typed UIStrings constants instead of raw string keys.
| import { i18nString } from '../i18n-strings.js'; | ||
| import { | ||
| getEvaluationConfig, | ||
| setEvaluationConfig, | ||
| isEvaluationEnabled, | ||
| connectToEvaluationService, | ||
| disconnectFromEvaluationService, | ||
| getEvaluationClientId, | ||
| isEvaluationConnected | ||
| } from '../../../common/EvaluationConfig.js'; | ||
| import { createLogger } from '../../../core/Logger.js'; | ||
|
|
There was a problem hiding this comment.
Reorder imports and bring in UIStrings for proper i18n usage.
-import { i18nString } from '../i18n-strings.js';
+import { UIStrings, i18nString } from '../ModuleUIStrings.js';
@@
-import { createLogger } from '../../../core/Logger.js';
+import { createLogger } from '../../../core/Logger.js'; // keep Logger before i18n per import/orderCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 ESLint
[error] 5-5: ../i18n-strings.js import should occur after import of ../../../core/Logger.js
(import/order)
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/ui/settings/advanced/EvaluationSettings.ts around
lines 5–16, reorder the imports so related i18n imports come first, followed by
the EvaluationConfig utilities, then core modules (Logger); add an import for
the UIStrings export from ../i18n-strings.js (alongside the existing i18nString
import) and update usages to reference UIStrings for any hard-coded or
previously direct i18nString keys so the component uses the centralized
UIStrings definitions; ensure import ordering follows project lint rules and run
type checks.
| const evaluationSectionTitle = document.createElement('h3'); | ||
| evaluationSectionTitle.className = 'settings-subtitle'; | ||
| evaluationSectionTitle.textContent = i18nString('evaluationSection'); | ||
| this.container.appendChild(evaluationSectionTitle); | ||
|
|
||
| // Get current evaluation configuration | ||
| const currentEvaluationConfig = getEvaluationConfig(); | ||
|
|
||
| // Evaluation enabled checkbox | ||
| const evaluationEnabledContainer = document.createElement('div'); | ||
| evaluationEnabledContainer.className = 'evaluation-enabled-container'; | ||
| this.container.appendChild(evaluationEnabledContainer); | ||
|
|
||
| this.evaluationEnabledCheckbox = document.createElement('input'); | ||
| this.evaluationEnabledCheckbox.type = 'checkbox'; | ||
| this.evaluationEnabledCheckbox.id = 'evaluation-enabled'; | ||
| this.evaluationEnabledCheckbox.className = 'evaluation-checkbox'; | ||
| this.evaluationEnabledCheckbox.checked = isEvaluationEnabled(); | ||
| evaluationEnabledContainer.appendChild(this.evaluationEnabledCheckbox); | ||
|
|
||
| const evaluationEnabledLabel = document.createElement('label'); | ||
| evaluationEnabledLabel.htmlFor = 'evaluation-enabled'; | ||
| evaluationEnabledLabel.className = 'evaluation-label'; | ||
| evaluationEnabledLabel.textContent = i18nString('evaluationEnabled'); | ||
| evaluationEnabledContainer.appendChild(evaluationEnabledLabel); | ||
|
|
||
| const evaluationEnabledHint = document.createElement('div'); | ||
| evaluationEnabledHint.className = 'settings-hint'; | ||
| evaluationEnabledHint.textContent = i18nString('evaluationEnabledHint'); | ||
| this.container.appendChild(evaluationEnabledHint); |
There was a problem hiding this comment.
Replace literal i18n keys with UIStrings members.
These calls will fail the l10n rule.
- evaluationSectionTitle.textContent = i18nString('evaluationSection');
+ evaluationSectionTitle.textContent = i18nString(UIStrings.evaluationSection);
@@
- evaluationEnabledLabel.textContent = i18nString('evaluationEnabled');
+ evaluationEnabledLabel.textContent = i18nString(UIStrings.evaluationEnabled);
@@
- evaluationEnabledHint.textContent = i18nString('evaluationEnabledHint');
+ evaluationEnabledHint.textContent = i18nString(UIStrings.evaluationEnabledHint);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const evaluationSectionTitle = document.createElement('h3'); | |
| evaluationSectionTitle.className = 'settings-subtitle'; | |
| evaluationSectionTitle.textContent = i18nString('evaluationSection'); | |
| this.container.appendChild(evaluationSectionTitle); | |
| // Get current evaluation configuration | |
| const currentEvaluationConfig = getEvaluationConfig(); | |
| // Evaluation enabled checkbox | |
| const evaluationEnabledContainer = document.createElement('div'); | |
| evaluationEnabledContainer.className = 'evaluation-enabled-container'; | |
| this.container.appendChild(evaluationEnabledContainer); | |
| this.evaluationEnabledCheckbox = document.createElement('input'); | |
| this.evaluationEnabledCheckbox.type = 'checkbox'; | |
| this.evaluationEnabledCheckbox.id = 'evaluation-enabled'; | |
| this.evaluationEnabledCheckbox.className = 'evaluation-checkbox'; | |
| this.evaluationEnabledCheckbox.checked = isEvaluationEnabled(); | |
| evaluationEnabledContainer.appendChild(this.evaluationEnabledCheckbox); | |
| const evaluationEnabledLabel = document.createElement('label'); | |
| evaluationEnabledLabel.htmlFor = 'evaluation-enabled'; | |
| evaluationEnabledLabel.className = 'evaluation-label'; | |
| evaluationEnabledLabel.textContent = i18nString('evaluationEnabled'); | |
| evaluationEnabledContainer.appendChild(evaluationEnabledLabel); | |
| const evaluationEnabledHint = document.createElement('div'); | |
| evaluationEnabledHint.className = 'settings-hint'; | |
| evaluationEnabledHint.textContent = i18nString('evaluationEnabledHint'); | |
| this.container.appendChild(evaluationEnabledHint); | |
| const evaluationSectionTitle = document.createElement('h3'); | |
| evaluationSectionTitle.className = 'settings-subtitle'; | |
| evaluationSectionTitle.textContent = i18nString(UIStrings.evaluationSection); | |
| this.container.appendChild(evaluationSectionTitle); | |
| // Get current evaluation configuration | |
| const currentEvaluationConfig = getEvaluationConfig(); | |
| // Evaluation enabled checkbox | |
| const evaluationEnabledContainer = document.createElement('div'); | |
| evaluationEnabledContainer.className = 'evaluation-enabled-container'; | |
| this.container.appendChild(evaluationEnabledContainer); | |
| this.evaluationEnabledCheckbox = document.createElement('input'); | |
| this.evaluationEnabledCheckbox.type = 'checkbox'; | |
| this.evaluationEnabledCheckbox.id = 'evaluation-enabled'; | |
| this.evaluationEnabledCheckbox.className = 'evaluation-checkbox'; | |
| this.evaluationEnabledCheckbox.checked = isEvaluationEnabled(); | |
| evaluationEnabledContainer.appendChild(this.evaluationEnabledCheckbox); | |
| const evaluationEnabledLabel = document.createElement('label'); | |
| evaluationEnabledLabel.htmlFor = 'evaluation-enabled'; | |
| evaluationEnabledLabel.className = 'evaluation-label'; | |
| evaluationEnabledLabel.textContent = i18nString(UIStrings.evaluationEnabled); | |
| evaluationEnabledContainer.appendChild(evaluationEnabledLabel); | |
| const evaluationEnabledHint = document.createElement('div'); | |
| evaluationEnabledHint.className = 'settings-hint'; | |
| evaluationEnabledHint.textContent = i18nString(UIStrings.evaluationEnabledHint); | |
| this.container.appendChild(evaluationEnabledHint); |
🧰 Tools
🪛 ESLint
[error] 41-41: Prefer template literals over imperative DOM API calls
(rulesdir/no-imperative-dom-api)
[error] 43-43: Calling i18nString/i18nLazyString requires a UIStrings member as the first argument (e.g., UIStrings.someString).
(rulesdir/l10n-i18nString-call-only-with-uistrings)
[error] 50-50: Prefer template literals over imperative DOM API calls
(rulesdir/no-imperative-dom-api)
[error] 64-64: Calling i18nString/i18nLazyString requires a UIStrings member as the first argument (e.g., UIStrings.someString).
(rulesdir/l10n-i18nString-call-only-with-uistrings)
[error] 67-67: Prefer template literals over imperative DOM API calls
(rulesdir/no-imperative-dom-api)
[error] 69-69: Calling i18nString/i18nLazyString requires a UIStrings member as the first argument (e.g., UIStrings.someString).
(rulesdir/l10n-i18nString-call-only-with-uistrings)
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/ui/settings/advanced/EvaluationSettings.ts around
lines 41 to 70, the file currently calls i18nString with literal string keys
(e.g. 'evaluationSection', 'evaluationEnabled', 'evaluationEnabledHint') which
violates the l10n rule; replace these literal keys with members from the
UIStrings object (e.g. UIStrings.evaluationSection, UIStrings.evaluationEnabled,
UIStrings.evaluationEnabledHint), ensure UIStrings is imported/defined in the
module and that i18nString is invoked with the UIStrings member references
instead of raw string literals so the localization tooling can pick them up.
| // Function to update connection status | ||
| const updateConnectionStatus = () => { | ||
| const isConnected = isEvaluationConnected(); | ||
|
|
||
| logger.debug('Updating connection status', { isConnected }); | ||
|
|
||
| if (isConnected) { | ||
| connectionStatusDot.style.backgroundColor = 'var(--color-accent-green)'; | ||
| connectionStatusText.textContent = 'Connected to evaluation server'; | ||
| connectionStatusText.style.color = 'var(--color-accent-green)'; | ||
| } else { | ||
| connectionStatusDot.style.backgroundColor = 'var(--color-text-disabled)'; | ||
| connectionStatusText.textContent = 'Not connected'; | ||
| connectionStatusText.style.color = 'var(--color-text-disabled)'; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return type and localize connection status texts.
Also externalize the two status strings in ModuleUIStrings (e.g., evaluationConnectedStatus, evaluationDisconnectedStatus).
- const updateConnectionStatus = () => {
+ const updateConnectionStatus = (): void => {
const isConnected = isEvaluationConnected();
@@
- connectionStatusText.textContent = 'Connected to evaluation server';
+ connectionStatusText.textContent = i18nString(UIStrings.evaluationConnectedStatus);
@@
- connectionStatusText.textContent = 'Not connected';
+ connectionStatusText.textContent = i18nString(UIStrings.evaluationDisconnectedStatus);Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 ESLint
[error] 95-95: Missing return type on function.
(@typescript-eslint/explicit-function-return-type)
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/ui/settings/advanced/EvaluationSettings.ts around
lines 94 to 109, add an explicit return type to updateConnectionStatus (e.g.,
updateConnectionStatus(): void) and ensure local typing for isConnected
(boolean); replace the hardcoded status strings 'Connected to evaluation server'
and 'Not connected' with localized values from ModuleUIStrings (e.g.,
ModuleUIStrings.evaluationConnectedStatus and
ModuleUIStrings.evaluationDisconnectedStatus), import ModuleUIStrings at the top
if not already imported, and use those localized strings when setting
connectionStatusText.textContent while keeping the same color and dot logic.
| // Update model selectors | ||
| this.updateModelSelectors(); | ||
|
|
||
| // Hide status after a delay | ||
| setTimeout(() => { | ||
| if (this.modelTestStatus) { | ||
| this.modelTestStatus.style.display = 'none'; | ||
| } | ||
| }, 3000); | ||
| } |
There was a problem hiding this comment.
Eliminate duplicate refresh + rebuild after fetch.
Avoid flicker by choosing one strategy (refresh or rebuild).
- // Update LiteLLM model selections
- this.updateModelSelectors();
+ // If you refreshed existing selectors above, do not rebuild here to prevent flicker.Also applies to: 174-176
| // Dynamically imported OAuth module | ||
| private OpenRouterOAuth: any = null; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid any; type the OAuth surface explicitly and narrow the return type.
- private OpenRouterOAuth: any = null;
+ type OpenRouterOAuthAPI = {
+ isOAuthAuthenticated(): Promise<boolean>;
+ startAuthFlow(): Promise<void>;
+ revokeToken(): Promise<void>;
+ switchToManualApiKey(): void;
+ };
+ private OpenRouterOAuth: OpenRouterOAuthAPI | null = null;
...
- private async getOpenRouterOAuth(): Promise<any> {
+ private async getOpenRouterOAuth(): Promise<OpenRouterOAuthAPI> {
if (!this.OpenRouterOAuth) {
const module = await import('../../../auth/OpenRouterOAuth.js');
- this.OpenRouterOAuth = module.OpenRouterOAuth;
+ this.OpenRouterOAuth = module.OpenRouterOAuth as OpenRouterOAuthAPI;
}
return this.OpenRouterOAuth;
}Also applies to: 60-66
🧰 Tools
🪛 ESLint
[error] 43-43: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/ui/settings/providers/OpenRouterSettings.ts around
lines 42-44 and 60-66, the dynamically imported OAuth module and related
variables are typed as any; replace these with a concrete interface that
describes the OAuth surface (methods, properties, and return types you use,
e.g., init, authorize, getToken, revokeToken, config shape). Change the private
OpenRouterOAuth property to that interface or null, and change any dynamic
import return types to Promise<ThatOAuthInterface | null> (or the module
namespace type if you import a named export), updating all usages to match the
narrowed types so callers use the defined methods/props instead of any.
| const updateOAuthButton = async () => { | ||
| const OAuth = await this.getOpenRouterOAuth(); | ||
| if (await OAuth.isOAuthAuthenticated()) { | ||
| if (this.oauthButton) { | ||
| this.oauthButton.textContent = 'Disconnect OpenRouter'; | ||
| this.oauthButton.classList.add('disconnect'); | ||
| } | ||
| if (this.oauthStatus) { | ||
| this.oauthStatus.textContent = '✓ Connected via OpenRouter account'; | ||
| this.oauthStatus.style.color = 'var(--color-accent-green)'; | ||
| this.oauthStatus.style.display = 'block'; | ||
| } | ||
| } else { | ||
| if (this.oauthButton) { | ||
| this.oauthButton.textContent = 'Connect with OpenRouter'; | ||
| this.oauthButton.classList.remove('disconnect'); | ||
| } | ||
| if (this.oauthStatus) { | ||
| this.oauthStatus.style.display = 'none'; | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return type and enable fetch button when OAuth is connected (no API key).
- const updateOAuthButton = async () => {
+ const updateOAuthButton: () => Promise<void> = async () => {
const OAuth = await this.getOpenRouterOAuth();
if (await OAuth.isOAuthAuthenticated()) {
if (this.oauthButton) {
this.oauthButton.textContent = 'Disconnect OpenRouter';
this.oauthButton.classList.add('disconnect');
}
+ if (this.fetchModelsButton) {
+ this.fetchModelsButton.disabled = false;
+ }
if (this.oauthStatus) {
this.oauthStatus.textContent = '✓ Connected via OpenRouter account';
this.oauthStatus.style.color = 'var(--color-accent-green)';
this.oauthStatus.style.display = 'block';
}
} else {
if (this.oauthButton) {
this.oauthButton.textContent = 'Connect with OpenRouter';
this.oauthButton.classList.remove('disconnect');
}
+ if (this.fetchModelsButton && this.apiKeyInput) {
+ this.fetchModelsButton.disabled = !this.apiKeyInput.value.trim();
+ }
if (this.oauthStatus) {
this.oauthStatus.style.display = 'none';
}
}
};🧰 Tools
🪛 ESLint
[error] 166-166: Missing return type on function.
(@typescript-eslint/explicit-function-return-type)
| dataset: { | ||
| modelType?: string; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Satisfy member-delimiter-style: use commas in inline type.
- dataset: {
- modelType?: string;
- };
+ dataset: {
+ modelType?: string,
+ };As per static analysis hints.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dataset: { | |
| modelType?: string; | |
| }; | |
| dataset: { | |
| modelType?: string, | |
| }; |
🧰 Tools
🪛 ESLint
[error] 40-40: Expected a comma.
(@stylistic/member-delimiter-style)
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/ui/settings/types.ts around lines 39 to 41, the
inline type for dataset uses a semicolon as a member delimiter which violates
member-delimiter-style; replace the semicolon with a comma so the property is
comma-delimited (i.e., change "modelType?: string;" to "modelType?: string,") to
satisfy the lint rule.
| export function isVectorDBEnabled(): boolean { | ||
| return getStorageBoolean('ai_chat_vector_db_enabled', false); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid hardcoded storage key; use the exported constant to prevent drift.
+import { VECTOR_DB_ENABLED_KEY } from '../constants.js';
...
export function isVectorDBEnabled(): boolean {
- return getStorageBoolean('ai_chat_vector_db_enabled', false);
+ return getStorageBoolean(VECTOR_DB_ENABLED_KEY, false);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/ui/settings/utils/storage.ts around lines 73 to 75,
the function isVectorDBEnabled() uses a hardcoded storage key string; replace
that string with the module-exported constant for the ai_chat vector DB key
(import it from its defining module if not already imported) and pass the same
default false value so the call becomes
getStorageBoolean(EXPORTED_CONSTANT_NAME, false). Ensure the import is
added/organized and tests/usage updated to reference the same constant to
prevent future drift.
Summary of what we fixed:
- BaseProviderSettings: Logger path (already correct)
- BrowsingHistorySettings: Changed ../../../../ to ../../../../../ for Common import
Summary by CodeRabbit