feat: add request timeout configuration to model settings - #1497
Merged
Conversation
- Introduced a new timeout input field in the ModelConfigDialog for setting request timeouts. - Added validation for the timeout value to ensure it is within the range of 1000ms to 600000ms. - Updated the model configuration defaults to include a default timeout value. - Enhanced the model configuration store and related types to support the new timeout property. - Implemented timeout handling in the ACP and GitHub Copilot providers to abort requests when the timeout elapses. - Updated tests to cover the new timeout functionality and ensure proper behavior under timeout conditions. - Added localization strings for timeout settings in multiple languages.
Contributor
📝 WalkthroughWalkthroughAdded end-to-end per-request timeout support (config, validation, persistence, provider aborts, UI and i18n), bumped three agents in the ACP registry, extended the sessions DB schema with Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/UI
participant Config as Config Layer
participant Provider as LLM Provider
participant Request as HTTP/API Request
participant Abort as Abort Controller
User->>Config: Set timeout (ms)
Config->>Config: Validate & persist timeout
User->>Provider: Send inference request
Provider->>Config: getModelConfig(...)
Config-->>Provider: modelConfig (includes timeout)
Provider->>Abort: createModelRequestSignal(timeout)
Abort->>Abort: schedule setTimeout -> abort
Provider->>Request: Execute request with AbortSignal
alt Timeout triggers
Abort->>Request: signal abort
Request-->>Provider: AbortError
Provider->>Provider: handle timeout, cleanup
Provider-->>User: emit timeout error/event
else Request completes
Request-->>Provider: response
Provider->>Abort: dispose()
Provider-->>User: deliver result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/shared/types/presenters/legacy.presenters.d.ts (1)
155-180:⚠️ Potential issue | 🟠 MajorAdd
timeouttoCONVERSATION_SETTINGSas well.
ModelConfignow exposestimeout, and the session persistence path already reads/writessettings.timeoutviatimeout_msinsrc/main/presenter/sqlitePresenter/tables/deepchatSessions.ts. The exported conversation settings type still omits it, so typed callers usingPartial<CONVERSATION_SETTINGS>won’t have a validtimeoutfield.Proposed type update
export type CONVERSATION_SETTINGS = { systemPrompt: string temperature: number contextLength: number maxTokens: number + timeout?: number providerId: string modelId: string artifacts: 0 | 1Also applies to: 1242-1259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/types/presenters/legacy.presenters.d.ts` around lines 155 - 180, The exported CONVERSATION_SETTINGS type is missing the timeout field even though ModelConfig includes timeout and session persistence reads/writes settings.timeout via timeout_ms in deepchatSessions (sqlitePresenter tables); update the CONVERSATION_SETTINGS definition to include timeout?: number (matching ModelConfig timeout semantics) so typed callers using Partial<CONVERSATION_SETTINGS> can pass timeout, and ensure any related type aliases or mappings that translate to/from timeout_ms (e.g., session settings serialization/parsing code) reflect this field as well.src/main/presenter/configPresenter/modelConfig.ts (1)
458-467:⚠️ Potential issue | 🟠 MajorNormalize
timeoutbefore merging or persisting it.
normalizedTimeoutbecomesundefinedfor invalid input, but...configkeeps the original invalidtimeoutinstoredConfig. The non-user merge also accepts any stored value. This bypasses the 1000–600000ms contract and can disable or distort request timeouts.🛡️ Proposed normalization fix
Add a shared helper near the module constants:
const MIN_MODEL_TIMEOUT_MS = 1000 const MAX_MODEL_TIMEOUT_MS = 600000 const normalizeModelTimeout = (timeout: unknown): number | undefined => { if (typeof timeout !== 'number' || !Number.isFinite(timeout)) { return undefined } const roundedTimeout = Math.round(timeout) return roundedTimeout >= MIN_MODEL_TIMEOUT_MS && roundedTimeout <= MAX_MODEL_TIMEOUT_MS ? roundedTimeout : undefined }Then apply it at both merge and persistence points:
- timeout: storedConfig.timeout ?? finalConfig.timeout, + timeout: normalizeModelTimeout(storedConfig.timeout) ?? finalConfig.timeout, ... - const normalizedTimeout = - typeof config.timeout === 'number' && Number.isFinite(config.timeout) && config.timeout > 0 - ? Math.round(config.timeout) - : undefined + const normalizedTimeout = normalizeModelTimeout(config.timeout) const storedConfig: ModelConfig = { ...config, ...(normalizedMaxTokens !== undefined ? { maxTokens: normalizedMaxTokens } : {}), - ...(normalizedTimeout !== undefined ? { timeout: normalizedTimeout } : {}), + timeout: normalizedTimeout, isUserDefined: source === 'user' }Also applies to: 510-518
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/configPresenter/modelConfig.ts` around lines 458 - 467, Add a normalization helper and use it wherever timeouts are merged or persisted: define MIN_MODEL_TIMEOUT_MS and MAX_MODEL_TIMEOUT_MS and implement normalizeModelTimeout(timeout: unknown): number | undefined (validate numeric finite, round, and return value only if within bounds); then replace direct uses of storedConfig.timeout when building finalConfig in the merge block and at the persistence point (the code paths around the finalConfig assignment and the save/serialize logic near the 510-518 area) to call normalizeModelTimeout(...) and only apply the normalized value (fall back to finalConfig.timeout when normalizeModelTimeout returns undefined).src/renderer/src/components/settings/ModelConfigDialog.vue (1)
1101-1133:⚠️ Potential issue | 🟡 MinorReject non-integer timeout values before saving.
The shared validator rejects non-integers, but this dialog accepts decimals and then silently rounds them on save. That can persist a different timeout than the user entered.
🔧 Proposed validation alignment
if (config.value.timeout !== undefined && config.value.timeout !== null) { const timeout = Number(config.value.timeout) - if (!Number.isFinite(timeout) || timeout < 1000) { + if (!Number.isFinite(timeout) || !Number.isInteger(timeout) || timeout < 1000) { errors.value.timeout = t('settings.model.modelConfig.validation.timeoutMin') } else if (timeout > 600000) { errors.value.timeout = t('settings.model.modelConfig.validation.timeoutMax') } } ... const timeout = Number(config.value.timeout) const normalizedTimeout = - Number.isFinite(timeout) && timeout > 0 ? Math.round(timeout) : undefined + Number.isFinite(timeout) && timeout > 0 ? timeout : undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/settings/ModelConfigDialog.vue` around lines 1101 - 1133, The dialog currently accepts decimal timeout values and silently rounds them in handleSave (config.value.timeout → normalizedTimeout), so add a validation check in validateForm (or reuse the existing timeout block) to reject non-integer values: when config.value.timeout is defined, parse Number(config.value.timeout) and if Number.isFinite(timeout) but !Number.isInteger(timeout) set errors.value.timeout to the appropriate i18n message and ensure isValid (the computed that calls validateForm) will block saving; also update handleSave to early-return if errors.value.timeout exists (i.e., keep the current guard if (!isValid.value) return) so no rounding/persisting occurs for decimals.src/main/presenter/llmProviderPresenter/providers/githubCopilotProvider.ts (1)
74-79:⚠️ Potential issue | 🟠 MajorPass the timeout signal through to the device-flow token retrieval.
getCopilotToken(signal)ignores the signal when callingthis.deviceFlow.getCopilotToken()at line 78, leaving the device-flow token exchange unprotected from timeouts. TheGitHubCopilotDeviceFlow.getCopilotToken()method currently does not accept anAbortSignalparameter and must be updated to support cancellation.Update the device-flow method signature to accept an optional
AbortSignal, then pass it through from the call site:- return await this.deviceFlow.getCopilotToken() + return await this.deviceFlow.getCopilotToken(signal)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/llmProviderPresenter/providers/githubCopilotProvider.ts` around lines 74 - 79, getCopilotToken currently ignores the provided AbortSignal when calling this.deviceFlow.getCopilotToken(), so update the GitHubCopilotDeviceFlow.getCopilotToken signature to accept an optional AbortSignal and propagate cancellation; then modify the call in getCopilotToken to pass the received signal through (i.e., call deviceFlow.getCopilotToken(signal)). Ensure both the interface/implementation of GitHubCopilotDeviceFlow and the call site in getCopilotToken are updated to accept and forward the AbortSignal.src/main/presenter/llmProviderPresenter/providers/voiceAIProvider.ts (1)
419-449:⚠️ Potential issue | 🟠 MajorMove setup work inside the
try/finallyso timeout cleanup always runs.
createModelRequestSignal()is called before validation/auth/trace work, butdispose()only runs after Line 451. If any setup throws, the timeout timer can be left alive until it fires.Suggested structure
- const { signal, dispose } = this.createModelRequestSignal(modelConfig) - const config = this.getTtsConfig() - if (!SUPPORTED_LANGUAGES.has(config.language)) { - throw new Error( - `Unsupported language code: ${config.language}. Supported languages: ${Array.from( - SUPPORTED_LANGUAGES - ).join(', ')}` - ) - } - const voiceId = this.resolveVoiceId(modelId) - const requestBody: Record<string, unknown> = { - text, - audio_format: config.audioFormat, - model: config.model, - language: config.language, - temperature: typeof temperature === 'number' ? temperature : config.temperature, - top_p: config.topP - } - - if (voiceId) { - requestBody['voice_id'] = voiceId - } - - const headers = this.getAuthHeaders() - if (modelConfig) { - await this.emitRequestTrace(modelConfig, { - endpoint: this.buildUrl('/api/v1/tts/speech'), - headers, - body: requestBody - }) - } - - try { + const { signal, dispose } = this.createModelRequestSignal(modelConfig) + try { + const config = this.getTtsConfig() + if (!SUPPORTED_LANGUAGES.has(config.language)) { + throw new Error( + `Unsupported language code: ${config.language}. Supported languages: ${Array.from( + SUPPORTED_LANGUAGES + ).join(', ')}` + ) + } + const voiceId = this.resolveVoiceId(modelId) + const requestBody: Record<string, unknown> = { + text, + audio_format: config.audioFormat, + model: config.model, + language: config.language, + temperature: typeof temperature === 'number' ? temperature : config.temperature, + top_p: config.topP + } + + if (voiceId) { + requestBody['voice_id'] = voiceId + } + + const headers = this.getAuthHeaders() + if (modelConfig) { + await this.emitRequestTrace(modelConfig, { + endpoint: this.buildUrl('/api/v1/tts/speech'), + headers, + body: requestBody + }) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/llmProviderPresenter/providers/voiceAIProvider.ts` around lines 419 - 449, Move the lifetime management of the model request signal into a try/finally so dispose() always runs: call createModelRequestSignal(modelConfig) and then immediately enter a try { ... } block that contains the validation (SUPPORTED_LANGUAGES check), auth setup (getAuthHeaders()), trace emission (emitRequestTrace / buildUrl('/api/v1/tts/speech')), requestBody construction (resolveVoiceId(), getTtsConfig()), and any awaits; in the finally block call dispose() to ensure the timeout/cleanup is always executed even if validation or emitRequestTrace throws.
🧹 Nitpick comments (3)
src/shared/types/agent-interface.d.ts (1)
25-25: Document the timeout unit on the shared contract.
timeoutis validated and displayed as milliseconds elsewhere, but the shared type does not say that. A brief JSDoc prevents future callers from accidentally treating it as seconds.Suggested clarification
+ /** Request timeout in milliseconds. Valid range: 1000-600000. */ timeout: number🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/types/agent-interface.d.ts` at line 25, Add a brief JSDoc to the timeout property in the shared interface so callers know the unit is milliseconds: update the declaration for timeout in the agent interface (the timeout: number property in src/shared/types/agent-interface.d.ts) to include a one-line JSDoc like "@param timeout - timeout in milliseconds" (or similar phrasing) so the shared contract clearly documents the unit.test/main/presenter/acpProvider.test.ts (1)
505-526: Assert the prompt starts before advancing the timeout.This timeout test would still pass if
runPromptscheduled the timeout but accidentally never calledconnection.prompt. Add a pre-timeout assertion to prove the request was actually in flight.🧪 Suggested test strengthening
const runPrompt = provider['runPrompt']( { sessionId: 'session-timeout', connection: { prompt, cancel } }, [], queue, { timeout: 25 } ) + expect(prompt).toHaveBeenCalledTimes(1) + expect(cancel).not.toHaveBeenCalled() + await vi.advanceTimersByTimeAsync(25) await runPrompt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/acpProvider.test.ts` around lines 505 - 526, The test must assert the prompt has actually started before advancing the timers: after creating runPrompt (the promise returned by provider['runPrompt'] with connection: { prompt, cancel }), add an assertion that connection.prompt (or the mock "prompt") has been called (or at least invoked once) to prove the request is in flight, then advance timers and await runPrompt; keep existing assertions for cancel, queue.push and queue.done as-is.src/shared/utils/generationSettingsValidation.ts (1)
18-19: Export shared timeout bounds to prevent drift.The same
1000/600000values are now needed by the dialog and provider-side guard. Exporting these as shared constants avoids future mismatches.♻️ Proposed shared constants
-const TIMEOUT_MIN = 1000 -const TIMEOUT_MAX = 600000 +export const TIMEOUT_MIN_MS = 1000 +export const TIMEOUT_MAX_MS = 600000 ... - if (numeric < TIMEOUT_MIN) { + if (numeric < TIMEOUT_MIN_MS) { return 'timeout_too_small' } - if (numeric > TIMEOUT_MAX) { + if (numeric > TIMEOUT_MAX_MS) { return 'timeout_too_large' }As per coding guidelines, "Constants must use SCREAMING_SNAKE_CASE naming".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/utils/generationSettingsValidation.ts` around lines 18 - 19, Export the shared timeout bounds so other modules can import them and avoid drift: convert the existing local constants TIMEOUT_MIN and TIMEOUT_MAX in generationSettingsValidation.ts into exported constants (e.g., export const TIMEOUT_MIN and export const TIMEOUT_MAX) using SCREAMING_SNAKE_CASE, and ensure any modules that need these values import them instead of duplicating literals; update usages to import the new exported symbols so the single source of truth is used.
🤖 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/main/presenter/llmProviderPresenter/aiSdk/runtime.ts`:
- Around line 145-151: resolveRequestTimeout currently only checks for
numeric/finite/positive values but does not enforce documented bounds, which can
lead to AbortSignal.timeout receiving invalid values; update
resolveRequestTimeout to clamp/validate the timeout against TIMEOUT_MIN and
TIMEOUT_MAX (use those constants) before Math.round and return undefined for
out-of-range values, or simply delegate to
BaseProvider.resolveModelRequestTimeout to centralize logic; apply the same
change to the other similar helpers noted (the occurrences around lines 227,
265, 337) so all timeout resolution paths use the shared bounds enforcement.
In `@src/main/presenter/llmProviderPresenter/baseProvider.ts`:
- Around line 81-90: The resolveModelRequestTimeout function currently accepts
any positive finite timeout; enforce the UI contract by validating that timeout
(from ModelConfig) is within the allowed bounds (min 1000 ms, max 600000 ms)
before returning a rounded value; if timeout is not a number, not finite, <=0,
or outside [1000,600000], return undefined. Update the logic inside
resolveModelRequestTimeout in baseProvider.ts to apply these bounds and still
use Math.round for the returned value.
In `@src/renderer/src/i18n/da-DK/settings.json`:
- Around line 429-430: Update the Danish localization for the JSON keys
"timeoutMin" and "timeoutMax" in the da-DK settings by replacing the English
strings with Danish translations (e.g., "Tidsgrænsen for forespørgsler skal være
mindst 1000 ms" and "Tidsgrænsen for forespørgsler må ikke overstige 600000
ms"); ensure you update both occurrences referenced (the block containing
"timeoutMin"/"timeoutMax" and the other occurrence around lines 466-468) so the
model configuration dialog shows fully localized Danish text.
In `@src/renderer/src/i18n/fa-IR/settings.json`:
- Around line 496-497: The fa-IR settings.json contains untranslated English
strings for the keys "timeoutMin" and "timeoutMax" (and additional similar keys
around lines noted) so update those values to Persian translations; locate the
keys "timeoutMin" and "timeoutMax" in the fa-IR settings resource (and the other
untranslated entries around the later block) and replace the English messages
with the provided Persian equivalents (or accurate Persian phrasing) while
preserving the JSON key names and punctuation so the UI uses localized text for
the model configuration dialog.
In `@src/renderer/src/i18n/fr-FR/settings.json`:
- Around line 496-497: The fr-FR locale file contains English messages for the
timeout validation keys "timeoutMin" and "timeoutMax" (and the similar keys
around lines 533-535), causing mixed-language UI; update the fr-FR entries for
"timeoutMin" and "timeoutMax" (and the other new timeout-related keys mentioned)
to proper French translations so validation messages display entirely in French,
ensuring you edit the keys "timeoutMin" and "timeoutMax" in
src/renderer/src/i18n/fr-FR/settings.json to the translated strings.
In `@src/renderer/src/i18n/he-IL/settings.json`:
- Around line 496-497: The Hebrew locale still contains English text for the two
new timeout keys; update the "timeoutMin" and "timeoutMax" entries in the he-IL
settings JSON to use the proper Hebrew translations (also check and update the
duplicate entries around lines 533–535 mentioned in the comment). Locate the
keys "timeoutMin" and "timeoutMax" in src/renderer/src/i18n/he-IL/settings.json
and replace their English values with the provided Hebrew strings (or
appropriate localized copy), ensuring punctuation and unit notation (ms) remain
correct and that both occurrences are updated consistently.
In `@src/renderer/src/i18n/ja-JP/settings.json`:
- Around line 496-497: Replace the English values for the "timeoutMin" and
"timeoutMax" keys in src/renderer/src/i18n/ja-JP/settings.json with Japanese
translations so the settings UI is localized; update both occurrences (the block
around keys "timeoutMin" and "timeoutMax" and the other occurrence at lines
~533-535). Use translations such as "timeoutMin":
"リクエストのタイムアウトは少なくとも1000msである必要があります" and "timeoutMax":
"リクエストのタイムアウトは600000msを超えることはできません" to ensure the UI shows Japanese text.
In `@src/renderer/src/i18n/ko-KR/settings.json`:
- Around line 496-497: The new Korean localization entries "timeoutMin" and
"timeoutMax" in settings.json are still in English; replace their values with
proper Korean translations so the UI/validation is fully localized (also update
the duplicate occurrences around the other block noted for lines 533-535).
Locate the keys "timeoutMin" and "timeoutMax" and set their values to the
appropriate Korean strings (e.g., for timeoutMin use a phrase like "요청 타임아웃은 최소
1000ms여야 합니다" and for timeoutMax use "요청 타임아웃은 600000ms를 초과할 수 없습니다"), ensuring
character encoding remains UTF-8.
In `@src/renderer/src/i18n/pt-BR/settings.json`:
- Around line 496-497: Keys "timeoutMin" and "timeoutMax" in the Portuguese
locale are still in English; update their values to Portuguese (e.g.,
"timeoutMin": "O tempo limite da requisição deve ser de no mínimo 1000ms" and
"timeoutMax": "O tempo limite da requisição não pode exceder 600000ms") and make
the same translation for the other occurrences referenced (around the second
block at lines 533-535) so both places use the localized strings.
In `@src/renderer/src/i18n/ru-RU/settings.json`:
- Around line 496-497: The ru-RU localization currently leaves the "timeoutMin"
and "timeoutMax" entries in English; update the values for the keys "timeoutMin"
and "timeoutMax" in src/renderer/src/i18n/ru-RU/settings.json to Russian (e.g.
"Таймаут запроса должен быть не меньше 1000 мс" and "Таймаут запроса не может
превышать 600000 мс") and make the same replacements for the duplicate
occurrences referenced in the file (the other timeout entries around the same
block).
In `@test/main/presenter/llmProviderPresenter/githubCopilotProvider.test.ts`:
- Around line 35-47: The device-flow token path isn't abortable because
GithubCopilotProvider calls this.deviceFlow.getCopilotToken() without
passing/using an AbortSignal; update the code so timeouts apply to both branches
by either (A) updating GitHubCopilotDeviceFlow.getCopilotToken to accept and
respect an AbortSignal and then pass that signal from GithubCopilotProvider when
calling deviceFlow.getCopilotToken, or (B) add a timeout wrapper in
GithubCopilotProvider around deviceFlow.getCopilotToken (the same wrapper used
for the API-key path) before the try block so the call can be aborted on
timeout; modify the relevant methods (GithubCopilotProvider and
GitHubCopilotDeviceFlow.getCopilotToken) and tests to cover the device-flow
timeout behavior accordingly.
---
Outside diff comments:
In `@src/main/presenter/configPresenter/modelConfig.ts`:
- Around line 458-467: Add a normalization helper and use it wherever timeouts
are merged or persisted: define MIN_MODEL_TIMEOUT_MS and MAX_MODEL_TIMEOUT_MS
and implement normalizeModelTimeout(timeout: unknown): number | undefined
(validate numeric finite, round, and return value only if within bounds); then
replace direct uses of storedConfig.timeout when building finalConfig in the
merge block and at the persistence point (the code paths around the finalConfig
assignment and the save/serialize logic near the 510-518 area) to call
normalizeModelTimeout(...) and only apply the normalized value (fall back to
finalConfig.timeout when normalizeModelTimeout returns undefined).
In `@src/main/presenter/llmProviderPresenter/providers/githubCopilotProvider.ts`:
- Around line 74-79: getCopilotToken currently ignores the provided AbortSignal
when calling this.deviceFlow.getCopilotToken(), so update the
GitHubCopilotDeviceFlow.getCopilotToken signature to accept an optional
AbortSignal and propagate cancellation; then modify the call in getCopilotToken
to pass the received signal through (i.e., call
deviceFlow.getCopilotToken(signal)). Ensure both the interface/implementation of
GitHubCopilotDeviceFlow and the call site in getCopilotToken are updated to
accept and forward the AbortSignal.
In `@src/main/presenter/llmProviderPresenter/providers/voiceAIProvider.ts`:
- Around line 419-449: Move the lifetime management of the model request signal
into a try/finally so dispose() always runs: call
createModelRequestSignal(modelConfig) and then immediately enter a try { ... }
block that contains the validation (SUPPORTED_LANGUAGES check), auth setup
(getAuthHeaders()), trace emission (emitRequestTrace /
buildUrl('/api/v1/tts/speech')), requestBody construction (resolveVoiceId(),
getTtsConfig()), and any awaits; in the finally block call dispose() to ensure
the timeout/cleanup is always executed even if validation or emitRequestTrace
throws.
In `@src/renderer/src/components/settings/ModelConfigDialog.vue`:
- Around line 1101-1133: The dialog currently accepts decimal timeout values and
silently rounds them in handleSave (config.value.timeout → normalizedTimeout),
so add a validation check in validateForm (or reuse the existing timeout block)
to reject non-integer values: when config.value.timeout is defined, parse
Number(config.value.timeout) and if Number.isFinite(timeout) but
!Number.isInteger(timeout) set errors.value.timeout to the appropriate i18n
message and ensure isValid (the computed that calls validateForm) will block
saving; also update handleSave to early-return if errors.value.timeout exists
(i.e., keep the current guard if (!isValid.value) return) so no
rounding/persisting occurs for decimals.
In `@src/shared/types/presenters/legacy.presenters.d.ts`:
- Around line 155-180: The exported CONVERSATION_SETTINGS type is missing the
timeout field even though ModelConfig includes timeout and session persistence
reads/writes settings.timeout via timeout_ms in deepchatSessions
(sqlitePresenter tables); update the CONVERSATION_SETTINGS definition to include
timeout?: number (matching ModelConfig timeout semantics) so typed callers using
Partial<CONVERSATION_SETTINGS> can pass timeout, and ensure any related type
aliases or mappings that translate to/from timeout_ms (e.g., session settings
serialization/parsing code) reflect this field as well.
---
Nitpick comments:
In `@src/shared/types/agent-interface.d.ts`:
- Line 25: Add a brief JSDoc to the timeout property in the shared interface so
callers know the unit is milliseconds: update the declaration for timeout in the
agent interface (the timeout: number property in
src/shared/types/agent-interface.d.ts) to include a one-line JSDoc like "@param
timeout - timeout in milliseconds" (or similar phrasing) so the shared contract
clearly documents the unit.
In `@src/shared/utils/generationSettingsValidation.ts`:
- Around line 18-19: Export the shared timeout bounds so other modules can
import them and avoid drift: convert the existing local constants TIMEOUT_MIN
and TIMEOUT_MAX in generationSettingsValidation.ts into exported constants
(e.g., export const TIMEOUT_MIN and export const TIMEOUT_MAX) using
SCREAMING_SNAKE_CASE, and ensure any modules that need these values import them
instead of duplicating literals; update usages to import the new exported
symbols so the single source of truth is used.
In `@test/main/presenter/acpProvider.test.ts`:
- Around line 505-526: The test must assert the prompt has actually started
before advancing the timers: after creating runPrompt (the promise returned by
provider['runPrompt'] with connection: { prompt, cancel }), add an assertion
that connection.prompt (or the mock "prompt") has been called (or at least
invoked once) to prove the request is in flight, then advance timers and await
runPrompt; keep existing assertions for cancel, queue.push and queue.done as-is.
🪄 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: 7bf691a5-4c71-4741-a945-b02320ef6958
📒 Files selected for processing (34)
resources/acp-registry/registry.jsonresources/model-db/providers.jsonsrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/configPresenter/modelConfig.tssrc/main/presenter/llmProviderPresenter/aiSdk/runtime.tssrc/main/presenter/llmProviderPresenter/baseProvider.tssrc/main/presenter/llmProviderPresenter/providers/acpProvider.tssrc/main/presenter/llmProviderPresenter/providers/githubCopilotProvider.tssrc/main/presenter/llmProviderPresenter/providers/voiceAIProvider.tssrc/main/presenter/sqlitePresenter/tables/deepchatSessions.tssrc/renderer/src/components/chat/ChatStatusBar.vuesrc/renderer/src/components/settings/ModelConfigDialog.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/stores/ui/draft.tssrc/shared/modelConfigDefaults.tssrc/shared/types/agent-interface.d.tssrc/shared/types/presenters/legacy.presenters.d.tssrc/shared/utils/generationSettingsValidation.tstest/main/presenter/acpProvider.test.tstest/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.tstest/main/presenter/llmProviderPresenter/githubCopilotProvider.test.tstest/main/presenter/sqlitePresenter.test.tstest/main/presenter/sqlitePresenter/deepchatSessionsTable.test.ts
Comment on lines
+145
to
+151
| function resolveRequestTimeout(modelConfig: ModelConfig): number | undefined { | ||
| const timeout = modelConfig.timeout | ||
| if (typeof timeout !== 'number' || !Number.isFinite(timeout) || timeout <= 0) { | ||
| return undefined | ||
| } | ||
| return Math.round(timeout) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how the current runtime handles valid and out-of-range AbortSignal.timeout delays.
# Expected: 600000 succeeds; very large values should be rejected or otherwise demonstrate why bounds are needed.
node - <<'NODE'
for (const value of [600000, 4294967296, 1e20]) {
try {
AbortSignal.timeout(value)
console.log(`${value}: ok`)
} catch (error) {
console.log(`${value}: ${error.name}: ${error.message}`)
}
}
NODERepository: ThinkInAIXYZ/deepchat
Length of output: 335
🏁 Script executed:
# Search for timeout bounds constants and validation
rg '(1000|600000|timeout)' src/main --type ts -i -A 3 -B 1 | head -100Repository: ThinkInAIXYZ/deepchat
Length of output: 7467
🏁 Script executed:
# Check BaseProvider for timeout handling
fd 'baseProvider' src/main --type tsRepository: ThinkInAIXYZ/deepchat
Length of output: 234
🏁 Script executed:
# Look for dialog/config validation related to timeout
rg '(dialog|validation|timeout)' src/main --type ts | grep -i timeout | head -20Repository: ThinkInAIXYZ/deepchat
Length of output: 2051
🏁 Script executed:
# Inspect the runtime.ts file around the timeout function
cat -n src/main/presenter/llmProviderPresenter/aiSdk/runtime.ts | sed -n '140,160p'Repository: ThinkInAIXYZ/deepchat
Length of output: 796
🏁 Script executed:
# Check how timeout is being passed to AbortSignal.timeout
rg 'AbortSignal\.timeout' src/main --type ts -B 2 -A 1Repository: ThinkInAIXYZ/deepchat
Length of output: 2751
🏁 Script executed:
# Search for timeout configuration schema with bounds
rg '(timeout|1000|600000)' src --type-list | head -5Repository: ThinkInAIXYZ/deepchat
Length of output: 454
🏁 Script executed:
# Find BaseProvider file
find src -name '*aseProvider*' -o -name '*baseProvider*'Repository: ThinkInAIXYZ/deepchat
Length of output: 120
🏁 Script executed:
# Search for timeout config/schema definitions
rg 'timeout.*min|timeout.*max|min.*timeout|max.*timeout' src -i -B 2 -A 2Repository: ThinkInAIXYZ/deepchat
Length of output: 18509
🏁 Script executed:
# Look for ModelConfig type definition
ast-grep --pattern 'type ModelConfig = $_'Repository: ThinkInAIXYZ/deepchat
Length of output: 47
🏁 Script executed:
# Search in config types directory
find src -type d -name configRepository: ThinkInAIXYZ/deepchat
Length of output: 47
🏁 Script executed:
cat -n src/main/presenter/llmProviderPresenter/baseProvider.ts | head -150Repository: ThinkInAIXYZ/deepchat
Length of output: 5765
🏁 Script executed:
# Search for resolveModelRequestTimeout in BaseProvider
rg 'resolveModelRequestTimeout' src/main -B 5 -A 10Repository: ThinkInAIXYZ/deepchat
Length of output: 4279
🏁 Script executed:
# Check if there are other timeout resolution functions
rg 'resolveTimeout|resolveRequestTimeout' src -B 2 -A 8Repository: ThinkInAIXYZ/deepchat
Length of output: 3196
Enforce documented timeout bounds before passing to AbortSignal.timeout().
resolveRequestTimeout duplicates logic from BaseProvider.resolveModelRequestTimeout but neither enforces the documented 1000–600000 ms bounds. While dialog validation guards the UI path, programmatic or persisted config values can bypass it. Out-of-range values passed to AbortSignal.timeout() will either cause immediate aborts (values <1000) or raise RangeError (values >4294967295). Consolidate timeout resolution to a single shared path using the bounds constants from src/shared/utils/generationSettingsValidation.ts (TIMEOUT_MIN, TIMEOUT_MAX), or reuse the BaseProvider method and add bounds enforcement there.
Also applies to: 227, 265, 337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/llmProviderPresenter/aiSdk/runtime.ts` around lines 145 -
151, resolveRequestTimeout currently only checks for numeric/finite/positive
values but does not enforce documented bounds, which can lead to
AbortSignal.timeout receiving invalid values; update resolveRequestTimeout to
clamp/validate the timeout against TIMEOUT_MIN and TIMEOUT_MAX (use those
constants) before Math.round and return undefined for out-of-range values, or
simply delegate to BaseProvider.resolveModelRequestTimeout to centralize logic;
apply the same change to the other similar helpers noted (the occurrences around
lines 227, 265, 337) so all timeout resolution paths use the shared bounds
enforcement.
Comment on lines
+81
to
+90
| protected resolveModelRequestTimeout( | ||
| modelConfig?: Pick<ModelConfig, 'timeout'> | null | ||
| ): number | undefined { | ||
| const timeout = modelConfig?.timeout | ||
| if (typeof timeout !== 'number' || !Number.isFinite(timeout) || timeout <= 0) { | ||
| return undefined | ||
| } | ||
|
|
||
| return Math.round(timeout) | ||
| } |
Contributor
There was a problem hiding this comment.
Enforce the same timeout bounds in the provider helper.
resolveModelRequestTimeout() accepts any positive finite value, so malformed persisted/imported config like 1ms or 999999999ms bypasses the UI’s 1000–600000ms contract.
🛡️ Proposed backend guard
export abstract class BaseLLMProvider {
// Maximum tool calls limit in a single conversation turn
protected static readonly MAX_TOOL_CALLS = 12800
protected static readonly DEFAULT_MODEL_FETCH_TIMEOUT = 12000 // Increased to 12 seconds as universal default
+ protected static readonly MODEL_REQUEST_TIMEOUT_MIN = 1000
+ protected static readonly MODEL_REQUEST_TIMEOUT_MAX = 600000
...
- return Math.round(timeout)
+ const timeoutMs = Math.round(timeout)
+ if (
+ timeoutMs < BaseLLMProvider.MODEL_REQUEST_TIMEOUT_MIN ||
+ timeoutMs > BaseLLMProvider.MODEL_REQUEST_TIMEOUT_MAX
+ ) {
+ return undefined
+ }
+
+ return timeoutMs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/llmProviderPresenter/baseProvider.ts` around lines 81 -
90, The resolveModelRequestTimeout function currently accepts any positive
finite timeout; enforce the UI contract by validating that timeout (from
ModelConfig) is within the allowed bounds (min 1000 ms, max 600000 ms) before
returning a rounded value; if timeout is not a number, not finite, <=0, or
outside [1000,600000], return undefined. Update the logic inside
resolveModelRequestTimeout in baseProvider.ts to apply these bounds and still
use Math.round for the returned value.
Comment on lines
+496
to
+497
| "timeoutMin": "Request timeout must be at least 1000ms", | ||
| "timeoutMax": "Request timeout cannot exceed 600000ms" |
Contributor
There was a problem hiding this comment.
Localize the new timeout strings for pt-BR.
These new user-facing strings are still in English in the Portuguese locale.
Suggested Portuguese copy
- "timeoutMin": "Request timeout must be at least 1000ms",
- "timeoutMax": "Request timeout cannot exceed 600000ms"
+ "timeoutMin": "O tempo limite da solicitação deve ser de pelo menos 1000 ms",
+ "timeoutMax": "O tempo limite da solicitação não pode exceder 600000 ms"
...
- "label": "Request Timeout (ms)",
- "description": "Set the timeout for a single model request. If exceeded, the request will be aborted."
+ "label": "Tempo limite da solicitação (ms)",
+ "description": "Defina o tempo limite para uma única solicitação do modelo. Se excedido, a solicitação será interrompida."Also applies to: 533-535
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/i18n/pt-BR/settings.json` around lines 496 - 497, Keys
"timeoutMin" and "timeoutMax" in the Portuguese locale are still in English;
update their values to Portuguese (e.g., "timeoutMin": "O tempo limite da
requisição deve ser de no mínimo 1000ms" and "timeoutMax": "O tempo limite da
requisição não pode exceder 600000ms") and make the same translation for the
other occurrences referenced (around the second block at lines 533-535) so both
places use the localized strings.
Comment on lines
+35
to
+47
| const provider = Object.create(GithubCopilotProvider.prototype) as GithubCopilotProvider & { | ||
| provider: { id: string; name: string } | ||
| configPresenter: { getModelConfig: ReturnType<typeof vi.fn> } | ||
| baseApiUrl: string | ||
| getCopilotToken: ReturnType<typeof vi.fn> | ||
| } | ||
| provider.provider = { id: 'github-copilot', name: 'GitHub Copilot' } | ||
| provider.configPresenter = { | ||
| getModelConfig: vi.fn().mockReturnValue({ timeout: 25 }) | ||
| } | ||
| provider.baseApiUrl = 'https://api.githubcopilot.com' | ||
| provider.getCopilotToken = vi.fn().mockResolvedValue('token') | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether GithubCopilotProvider forwards AbortSignal into device-flow token retrieval.
rg -n -C4 "getCopilotToken\\s*\\(" --iglob "githubCopilotProvider.ts" --iglob "!**/node_modules/**"
rg -n -C4 "deviceFlow\\.getCopilotToken\\s*\\(" --iglob "githubCopilotProvider.ts" --iglob "!**/node_modules/**"Repository: ThinkInAIXYZ/deepchat
Length of output: 5756
🏁 Script executed:
#!/bin/bash
# 1. Check the test file at the specified lines
cat -n test/main/presenter/llmProviderPresenter/githubCopilotProvider.test.ts | sed -n '35,82p'
# 2. Find the deviceFlow getCopilotToken signature to understand if it accepts signal
rg -n -C3 "getCopilotToken" --iglob "deviceFlow*.ts" --iglob "!**/node_modules/**"
# 3. Check the complete getCopilotToken method implementation
sed -n '74,100p' src/main/presenter/llmProviderPresenter/providers/githubCopilotProvider.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 3351
🏁 Script executed:
#!/bin/bash
# Search for deviceFlow interface/type definition
rg -n "deviceFlow" --iglob "githubCopilotProvider.ts" -A 2 -B 2 | head -50
# Look for the DeviceFlow type or interface
rg -n "interface.*DeviceFlow\|class.*DeviceFlow\|type.*DeviceFlow" --iglob "*.ts" --iglob "!**/node_modules/**"
# Check test imports and setup to understand deviceFlow context
head -35 test/main/presenter/llmProviderPresenter/githubCopilotProvider.test.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 3796
🏁 Script executed:
#!/bin/bash
# Find GitHubCopilotDeviceFlow class/interface definition
rg -n "class GitHubCopilotDeviceFlow\|interface GitHubCopilotDeviceFlow" --iglob "*.ts" --iglob "!**/node_modules/**" -A 20
# Check the getCopilotToken signature in the actual implementation
rg -n "getCopilotToken\s*\(" src/main/presenter/githubCopilotDeviceFlow.ts -A 5Repository: ThinkInAIXYZ/deepchat
Length of output: 617
🏁 Script executed:
#!/bin/bash
# Check the complete getCopilotToken implementation to see if signal is used anywhere
sed -n '74,120p' src/main/presenter/llmProviderPresenter/providers/githubCopilotProvider.ts
# Verify test expectations - what does it actually assert?
sed -n '48,62p' test/main/presenter/llmProviderPresenter/githubCopilotProvider.test.ts
# Check if there are other tests that might exercise the real path
grep -n "deviceFlow\|getCopilotToken" test/main/presenter/llmProviderPresenter/githubCopilotProvider.test.ts | head -20Repository: ThinkInAIXYZ/deepchat
Length of output: 2356
Add timeout coverage for device-flow token retrieval.
The tests stub getCopilotToken with a mock that ignores the signal parameter, so they only verify timeout behavior for the fallback API key path. The production code at line 78 calls this.deviceFlow.getCopilotToken() without forwarding the signal, meaning a timeout will not interrupt device-flow token acquisition. Since GitHubCopilotDeviceFlow.getCopilotToken() does not accept a signal parameter, consider either:
- Updating the DeviceFlow implementation to accept and respect AbortSignal, or
- Adding a timeout wrapper at the call site (line 78) before the try block
Current tests verify the timeout path only for the fallback branch (API key fetch), leaving the device-flow scenario untested for timeout behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/main/presenter/llmProviderPresenter/githubCopilotProvider.test.ts`
around lines 35 - 47, The device-flow token path isn't abortable because
GithubCopilotProvider calls this.deviceFlow.getCopilotToken() without
passing/using an AbortSignal; update the code so timeouts apply to both branches
by either (A) updating GitHubCopilotDeviceFlow.getCopilotToken to accept and
respect an AbortSignal and then pass that signal from GithubCopilotProvider when
calling deviceFlow.getCopilotToken, or (B) add a timeout wrapper in
GithubCopilotProvider around deviceFlow.getCopilotToken (the same wrapper used
for the API-key path) before the try block so the call can be aborted on
timeout; modify the relevant methods (GithubCopilotProvider and
GitHubCopilotDeviceFlow.getCopilotToken) and tests to cover the device-flow
timeout behavior accordingly.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/renderer/src/i18n/ru-RU/settings.json (1)
496-497:⚠️ Potential issue | 🟡 MinorFinish localizing the timeout strings.
The validation messages still use English
ms, and the timeout label/description remain English in the Russian locale, causing mixed-language UI.🌐 Proposed localization fix
- "timeoutMin": "Тайм-аут запроса должен быть не менее 1000ms", - "timeoutMax": "Тайм-аут запроса не может превышать 600000ms" + "timeoutMin": "Тайм-аут запроса должен быть не менее 1000 мс", + "timeoutMax": "Тайм-аут запроса не может превышать 600000 мс" ... - "label": "Request Timeout (ms)", - "description": "Set the timeout for a single model request. If exceeded, the request will be aborted." + "label": "Тайм-аут запроса (мс)", + "description": "Задаёт тайм-аут для одного запроса к модели. При превышении этого времени запрос будет прерван."Also applies to: 533-535
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/ru-RU/settings.json` around lines 496 - 497, Translate the remaining English timeout strings into Russian: update the values for "timeoutMin" and "timeoutMax" to use Russian text and the Russian abbreviation for milliseconds ("мс") instead of "ms", and also translate the timeout label/description entries (e.g., keys like "timeoutLabel" / "timeoutDesc" or the equivalent strings referenced near the same block) to Russian so the UI uses consistent Russian wording across all timeout messages.src/renderer/src/i18n/fr-FR/settings.json (1)
533-536:⚠️ Potential issue | 🟡 MinorTimeout label/description still in English.
While
timeoutMin/timeoutMaxwere translated, thetimeout.labelandtimeout.descriptionentries in this same PR remain English, which will produce mixed-language UI in the French locale.🌐 Proposed localization
"timeout": { - "label": "Request Timeout (ms)", - "description": "Set the timeout for a single model request. If exceeded, the request will be aborted." + "label": "Délai d'attente de la requête (ms)", + "description": "Définit le délai d'attente d'une requête de modèle unique. En cas de dépassement, la requête sera interrompue." }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/fr-FR/settings.json` around lines 533 - 536, The timeout UI strings are still in English: update the i18n entries named "timeout.label" and "timeout.description" in the fr-FR settings JSON so they are translated to French (consistent with the already-translated timeoutMin/timeoutMax); locate the "timeout" object and replace the English values for "label" and "description" with appropriate French translations that match the tone of the rest of the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/renderer/src/i18n/fr-FR/settings.json`:
- Around line 533-536: The timeout UI strings are still in English: update the
i18n entries named "timeout.label" and "timeout.description" in the fr-FR
settings JSON so they are translated to French (consistent with the
already-translated timeoutMin/timeoutMax); locate the "timeout" object and
replace the English values for "label" and "description" with appropriate French
translations that match the tone of the rest of the file.
In `@src/renderer/src/i18n/ru-RU/settings.json`:
- Around line 496-497: Translate the remaining English timeout strings into
Russian: update the values for "timeoutMin" and "timeoutMax" to use Russian text
and the Russian abbreviation for milliseconds ("мс") instead of "ms", and also
translate the timeout label/description entries (e.g., keys like "timeoutLabel"
/ "timeoutDesc" or the equivalent strings referenced near the same block) to
Russian so the UI uses consistent Russian wording across all timeout messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee9fe02a-f96c-43ea-be30-372857640732
📒 Files selected for processing (8)
src/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.json
✅ Files skipped from review due to trivial changes (1)
- src/renderer/src/i18n/fa-IR/settings.json
🚧 Files skipped from review as they are similar to previous changes (5)
- src/renderer/src/i18n/he-IL/settings.json
- src/renderer/src/i18n/ja-JP/settings.json
- src/renderer/src/i18n/pt-BR/settings.json
- src/renderer/src/i18n/ko-KR/settings.json
- src/renderer/src/i18n/da-DK/settings.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores
Tests