Update provider DB refresh flow and add manual model config refresh#1421
Merged
Update provider DB refresh flow and add manual model config refresh#1421
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a structured provider DB refresh API and result type, deduplicates concurrent refreshes, changes TTL/initialization refresh behavior, exposes a presenter method, wires a renderer UI to force-refresh with localized toasts, centralizes shell/env merging utilities, and adds comprehensive tests for these flows. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as DataSettings.vue
participant Presenter as ConfigPresenter
participant Loader as ProviderDbLoader
participant Network as HTTP/Network
User->>UI: Click "Force refresh"
UI->>Presenter: refreshProviderDb(force=true)
Presenter->>Loader: refreshIfNeeded(force=true)
alt existing in-flight refresh
Loader-->>Presenter: return existing ProviderDbRefreshResult
else no in-flight refresh
Loader->>Loader: evaluate freshness (lastAttemptedAt/lastUpdated, TTL)
alt skipped (fresh & not forced)
Loader-->>Presenter: {status: "skipped", lastUpdated, providersCount}
else perform fetch
Loader->>Network: fetch(sourceUrl)
alt 304 Not Modified
Network-->>Loader: 304
Loader->>Loader: write lastAttemptedAt/meta
Loader-->>Presenter: {status: "not-modified", lastUpdated, providersCount}
else 200 OK + valid JSON
Network-->>Loader: 200 + data
Loader->>Loader: atomically write cache/meta, update in-memory DB
Loader-->>Presenter: {status: "updated", lastUpdated, providersCount}
else error/invalid
Network-->>Loader: error/non-OK/parse
Loader->>Loader: write attempt meta (if prior)
Loader-->>Presenter: {status: "error", message, lastUpdated, providersCount}
end
end
end
Presenter-->>UI: ProviderDbRefreshResult
UI->>User: show toast per status (updated / not-modified|skipped / error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/lib/agentRuntime/shellEnvHelper.ts (1)
239-243: Consider preserving whitespace in environment variable values.Trimming both the key and value may unintentionally alter environment variables that legitimately contain leading/trailing whitespace. While rare, some applications depend on exact values.
💡 Suggested minimal change
- const key = line.slice(0, separatorIndex).trim() - const value = line.slice(separatorIndex + 1).trim() + const key = line.slice(0, separatorIndex).trim() + const value = line.slice(separatorIndex + 1) if (key.length > 0) { env[key] = value }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/lib/agentRuntime/shellEnvHelper.ts` around lines 239 - 243, The code currently trims both key and value which can remove intentional leading/trailing whitespace; change the parsing in the block that computes key/value (using separatorIndex, key, value, env) to only trim the key (keep key = line.slice(0, separatorIndex).trim()) but do not call .trim() on the value — use value = line.slice(separatorIndex + 1) so the original leading/trailing whitespace is preserved before storing env[key] = value; keep the existing guard key.length > 0 unchanged.
🤖 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/lib/agentRuntime/backgroundExecSessionManager.ts`:
- Around line 134-137: The spawn call currently replaces the entire environment
when options.env is provided which discards the baseline process.env; update the
env passed to spawn at the spawn(...) call (the object with cwd and env) to
start from process.env and merge/overlay options.env instead of replacing it
(i.e. use a shallow merge like { ...process.env, ...(options?.env || {}) }) so
callers can provide only deltas such as PATH without losing other environment
variables.
In `@src/main/presenter/configPresenter/acpInitHelper.ts`:
- Around line 515-551: The PATH rewrite currently calls setPathEntriesOnEnv with
includeDefaultPaths: false, which can drop standard OS search dirs if
mergeCommandEnvironment()/getShellEnvironment() provide sparse PATHs; before any
call that sets PATH (the setPathEntriesOnEnv invocation that uses
prependPathSources and later similar calls around the code paths noted), seed
the path sources with the platform default paths by adding
this.runtimeHelper.getDefaultPaths(app.getPath('home')) to the list of path
sources (or pass it as an explicit source into setPathEntriesOnEnv) so that
mergeCommandEnvironment(), getShellEnvironment(), prependPathSources,
getPathEntriesFromEnv and setPathEntriesOnEnv keep the system defaults available
when includeDefaultPaths is false.
---
Nitpick comments:
In `@src/main/lib/agentRuntime/shellEnvHelper.ts`:
- Around line 239-243: The code currently trims both key and value which can
remove intentional leading/trailing whitespace; change the parsing in the block
that computes key/value (using separatorIndex, key, value, env) to only trim the
key (keep key = line.slice(0, separatorIndex).trim()) but do not call .trim() on
the value — use value = line.slice(separatorIndex + 1) so the original
leading/trailing whitespace is preserved before storing env[key] = value; keep
the existing guard key.length > 0 unchanged.
🪄 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: 53393e7f-554f-45ef-aec1-05f32c1e04e4
📒 Files selected for processing (13)
src/main/lib/agentRuntime/backgroundExecSessionManager.tssrc/main/lib/agentRuntime/rtkRuntimeService.tssrc/main/lib/agentRuntime/shellEnvHelper.tssrc/main/presenter/configPresenter/acpInitHelper.tssrc/main/presenter/llmProviderPresenter/acp/acpProcessManager.tssrc/main/presenter/skillPresenter/skillExecutionService.tssrc/main/presenter/toolPresenter/agentTools/agentBashHandler.tstest/main/lib/agentRuntime/backgroundExecSessionManager.test.tstest/main/lib/agentRuntime/rtkRuntimeService.test.tstest/main/lib/agentRuntime/shellEnvHelper.test.tstest/main/presenter/llmProviderPresenter/acp/acpProcessManager.test.tstest/main/presenter/skillPresenter/skillExecutionService.test.tstest/main/presenter/toolPresenter/agentTools/agentBashHandler.test.ts
Comment on lines
+515
to
+551
| let env = mergeCommandEnvironment() | ||
| const systemEnvCount = Object.keys(env).length | ||
| console.log('[ACP Init] Added system environment variables:', systemEnvCount) | ||
|
|
||
| // Merge shell environment (includes user PATH from shell startup) | ||
| const pathKeys = ['PATH', 'Path', 'path'] | ||
| const existingPaths: string[] = [] | ||
| pathKeys.forEach((key) => { | ||
| const value = env[key] | ||
| if (value) existingPaths.push(value) | ||
| }) | ||
|
|
||
| try { | ||
| const shellEnv = await getShellEnvironment() | ||
| Object.entries(shellEnv).forEach(([key, value]) => { | ||
| if (value !== undefined && value !== '' && !pathKeys.includes(key)) { | ||
| env[key] = value | ||
| } | ||
| }) | ||
| const shellPath = shellEnv.PATH || shellEnv.Path || shellEnv.path | ||
| if (shellPath) { | ||
| existingPaths.unshift(shellPath) | ||
| } | ||
| env = mergeCommandEnvironment({ shellEnv }) | ||
| } catch (error) { | ||
| console.warn('[ACP Init] Failed to merge shell environment variables:', error) | ||
| } | ||
|
|
||
| // Prepare PATH merging | ||
| const HOME_DIR = app.getPath('home') | ||
| const defaultPaths = this.runtimeHelper.getDefaultPaths(HOME_DIR) | ||
| const allPaths = [...existingPaths, ...defaultPaths] | ||
| const prependPathSources: string[] = [] | ||
|
|
||
| // Add runtime paths to PATH if using builtin runtime | ||
| if (useBuiltinRuntime) { | ||
| const runtimePaths: string[] = [] | ||
|
|
||
| const uvRuntimePath = this.runtimeHelper.getUvRuntimePath() | ||
| const nodeRuntimePath = this.runtimeHelper.getNodeRuntimePath() | ||
|
|
||
| if (uvRuntimePath) { | ||
| runtimePaths.push(uvRuntimePath) | ||
| prependPathSources.push(uvRuntimePath) | ||
| console.log('[ACP Init] Added UV runtime path:', uvRuntimePath) | ||
| } | ||
|
|
||
| if (process.platform === 'win32') { | ||
| if (nodeRuntimePath) { | ||
| runtimePaths.push(nodeRuntimePath) | ||
| prependPathSources.push(nodeRuntimePath) | ||
| console.log('[ACP Init] Added Node runtime path (Windows):', nodeRuntimePath) | ||
| } | ||
| } else { | ||
| if (nodeRuntimePath) { | ||
| const nodeBinPath = path.join(nodeRuntimePath, 'bin') | ||
| runtimePaths.push(nodeBinPath) | ||
| console.log('[ACP Init] Added Node runtime path (Unix):', nodeBinPath) | ||
| } | ||
| } else if (nodeRuntimePath) { | ||
| const nodeBinPath = path.join(nodeRuntimePath, 'bin') | ||
| prependPathSources.push(nodeBinPath) | ||
| console.log('[ACP Init] Added Node runtime path (Unix):', nodeBinPath) | ||
| } | ||
|
|
||
| if (runtimePaths.length > 0) { | ||
| allPaths.unshift(...runtimePaths) | ||
| if (prependPathSources.length > 0) { | ||
| setPathEntriesOnEnv(env, [prependPathSources, getPathEntriesFromEnv(env)], { | ||
| includeDefaultPaths: false | ||
| }) |
Contributor
There was a problem hiding this comment.
Seed the default OS PATH before rewriting PATH.
includeDefaultPaths: false makes the PATH here depend entirely on the bundled/profile sources plus whatever mergeCommandEnvironment() already exposed. If getShellEnvironment() falls back or returns a sparse GUI PATH, the ACP init shell loses the standard system search dirs, so npm, git, or other helper binaries can stop resolving. The later rewrites at Line 582 and Line 604 keep carrying that trimmed PATH forward. Please seed this.runtimeHelper.getDefaultPaths(app.getPath('home')) once before these rewrites, or include it as an explicit path source.
Also applies to: 582-584, 604-606
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/configPresenter/acpInitHelper.ts` around lines 515 - 551,
The PATH rewrite currently calls setPathEntriesOnEnv with includeDefaultPaths:
false, which can drop standard OS search dirs if
mergeCommandEnvironment()/getShellEnvironment() provide sparse PATHs; before any
call that sets PATH (the setPathEntriesOnEnv invocation that uses
prependPathSources and later similar calls around the code paths noted), seed
the path sources with the platform default paths by adding
this.runtimeHelper.getDefaultPaths(app.getPath('home')) to the list of path
sources (or pass it as an explicit source into setPathEntriesOnEnv) so that
mergeCommandEnvironment(), getShellEnvironment(), prependPathSources,
getPathEntriesFromEnv and setPathEntriesOnEnv keep the system defaults available
when includeDefaultPaths is false.
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
1 participant
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
configPresenter.refreshProviderDb()path so the renderer can force-refresh provider metadata and reuse the existing provider/model update eventssettings.jsonlocales with region-appropriate translationsApproach
updated,not-modified,skipped,error) so UI feedback is deterministic instead of relying on implicit failuresTesting
pnpm run formatpnpm run i18npnpm run lintpnpm exec vitest --config vitest.config.ts --run test/main/presenter/configPresenter/providerDbLoader.test.tspnpm exec vitest --config vitest.config.renderer.ts --run test/renderer/components/DataSettings.test.tsSummary by CodeRabbit
New Features
Bug Fixes
Chores
Tests