fix(provider-cache): hydrate synced providers in fresh browsers#204
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 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)
📝 WalkthroughWalkthroughProvider-cache request sequencing prevents race conditions where stale in-flight responses overwrite fresher forced-refresh results using a monotonic counter guard. The backend exposes cached Claude provider metadata via a new API endpoint with safe fields only (no secrets). Frontend Claude config validation is extended to accept ChangesProvider cache integration for Claude configuration
Sequence Diagram(s)sequenceDiagram
participant Startup
participant loadProviderCacheRecords as loadProviderCacheRecords
participant seq as providerCacheRequestSeq
participant API as Backend API
participant State as App State
Startup->>loadProviderCacheRecords: background: true
loadProviderCacheRecords->>seq: increment → seq=1
loadProviderCacheRecords->>API: fetch records (req=1)
Startup->>loadProviderCacheRecords: forceRefresh: true
loadProviderCacheRecords->>seq: increment → seq=2
loadProviderCacheRecords->>API: fetch records (req=2)
API-->>loadProviderCacheRecords: respond (req=2)
loadProviderCacheRecords->>loadProviderCacheRecords: isLatestRequest(2) → true
loadProviderCacheRecords->>State: apply records, providerCacheLoadedAt
API-->>loadProviderCacheRecords: respond (req=1, stale)
loadProviderCacheRecords->>loadProviderCacheRecords: isLatestRequest(1) → false
Note over loadProviderCacheRecords: skip update, return
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Implemented the incognito/provider-cache follow-up. What changed:
Verified:
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== PR Info ==="
gh pr view 204 --repo SakuraByteCore/codexmate --json title,body,files,additions,deletions,changedFiles
echo ""
echo "=== PR Diff ==="
gh pr diff 204 --repo SakuraByteCore/codexmateLength of output: 9019
In void this.loadProviderCacheRecords({ background: true });But async loadProviderCacheRecords(options = {}) {
const forceRefresh = options && options.forceRefresh === true;
// `background` is never readAs a result, the startup hydration sets Two options to fix this: // Option A – implement the flag (suppress spinner for background loads)
async loadProviderCacheRecords(options = {}) {
const forceRefresh = options?.forceRefresh === true;
const background = options?.background === true;
if (this.providerCacheLoading && !forceRefresh) return;
const requestSeq = (Number(this.providerCacheRequestSeq) || 0) + 1;
this.providerCacheRequestSeq = requestSeq;
const isLatestRequest = () => requestSeq === Number(this.providerCacheRequestSeq || 0);
if (!background) this.providerCacheLoading = true; // skip spinner for background
...
} finally {
if (isLatestRequest() && !background) {
this.providerCacheLoading = false;
}
}
}// Option B – remove the unused option (keep the call site honest)
void this.loadProviderCacheRecords();Test only verifies the call shape, not the
Everything else looks good:
|
|
Addressed CodeRabbit's background-load concern. Follow-up changes:
Re-verified after the follow-up:
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Current PR Diff ==="
gh pr diff 204 --repo SakuraByteCore/codexmateLength of output: 8176
Early-return in
New regression test ✅
Everything is well-addressed. The implementation is clean and the test coverage is solid. (ﾉ◕ヮ◕)ﾉ*:･ﾟ✧ ✅ Action performedReview finished.
|
awsl233777
changed the title
|
Implemented the Claude/provider-cache follow-up. What changed:
Verified:
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Current PR Diff ==="
gh pr diff 204 --repo SakuraByteCore/codexmate
echo ""
echo "=== PR Files Changed ==="
gh pr view 204 --repo SakuraByteCore/codexmate --json files --jq '.files[].path'Length of output: 37409
Security boundary — API key never leaves the server ✅
Anti-reimport in The new branch in Startup ordering ✅ In
Stale model load identity check ✅
&& (typeof latestConfig.providerCacheRef === 'string' ? latestConfig.providerCacheRef.trim() : '') === providerCacheRefThis correctly discards stale model-fetch results when the user switches provider-cache entries mid-flight. Test coverage ✅
One minor observation (non-blocking): The Everything else is clean: Solid work on the security model and the isolation between browser state and server secrets. ✨ [approve] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-ui/modules/app.methods.claude-config.mjs (1)
98-101:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
providerCacheRefcheck inonClaudeModelChangecauses false "API key required" errors for provider-cache-backed configs.The credential validation at line 98 was not updated to include
providerCacheRef, unlike the parallel checks inapplyClaudeConfig(line 354),saveAndApplyConfig(line 271), and validation (line 44). A provider-cache-backed config hasapiKey: ''and noexternalCredentialType, so this check incorrectly blocks model changes with an error.Proposed fix
- if (!this.claudeConfigs[name].apiKey && !this.claudeConfigs[name].externalCredentialType && this.claudeConfigs[name].targetApi !== 'ollama') { + if (!this.claudeConfigs[name].apiKey && !this.claudeConfigs[name].providerCacheRef && !this.claudeConfigs[name].externalCredentialType && this.claudeConfigs[name].targetApi !== 'ollama') {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/modules/app.methods.claude-config.mjs` around lines 98 - 101, The credential validation check in the `onClaudeModelChange` method is missing the `providerCacheRef` check, causing it to incorrectly reject provider-cache-backed configs with an "API key required" error. Update the condition that checks for missing credentials (which currently checks apiKey, externalCredentialType, and targetApi) to also include a check for `providerCacheRef`, similar to how it's already implemented in `applyClaudeConfig`, `saveAndApplyConfig`, and the validation check. This will allow configs backed by provider cache to pass the validation since they don't require an explicit apiKey.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web-ui/modules/app.methods.startup-claude.mjs`:
- Around line 265-274: The mergeClaudeConfig function's source and hasKey field
assignments can retain stale credential state even after credentials are
cleared. At line 265, the source field logic should ensure that source is only
set to 'provider-cache' if at least one of the actual credential properties
(next.apiKey, externalCredentialType, or providerCacheRef) is present.
Similarly, at line 273, the hasKey field should only be true when there are
valid credentials currently present, rather than falling back to raw.hasKey ===
true which can preserve stale state. Restructure both conditional checks to
reset these fields to falsy values when all credential properties are empty.
---
Outside diff comments:
In `@web-ui/modules/app.methods.claude-config.mjs`:
- Around line 98-101: The credential validation check in the
`onClaudeModelChange` method is missing the `providerCacheRef` check, causing it
to incorrectly reject provider-cache-backed configs with an "API key required"
error. Update the condition that checks for missing credentials (which currently
checks apiKey, externalCredentialType, and targetApi) to also include a check
for `providerCacheRef`, similar to how it's already implemented in
`applyClaudeConfig`, `saveAndApplyConfig`, and the validation check. This will
allow configs backed by provider cache to pass the validation since they don't
require an explicit apiKey.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c3a08fec-60cd-4fd6-974e-6bf0990faf2f
📒 Files selected for processing (8)
cli.jstests/unit/claude-settings-sync.test.mjstests/unit/web-ui-behavior-parity.test.mjstests/unit/web-ui-logic.test.mjsweb-ui/app.jsweb-ui/logic.claude.mjsweb-ui/modules/app.methods.claude-config.mjsweb-ui/modules/app.methods.startup-claude.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/app.js
📜 Review details
🔇 Additional comments (16)
tests/unit/web-ui-behavior-parity.test.mjs (1)
771-771: LGTM!web-ui/logic.claude.mjs (1)
194-201: LGTM!web-ui/modules/app.methods.startup-claude.mjs (1)
468-470: LGTM!Also applies to: 480-480, 532-532
cli.js (3)
2880-2908: LGTM!
10014-10042: LGTM!
12384-12386: LGTM!tests/unit/claude-settings-sync.test.mjs (6)
15-17: LGTM!
1410-1474: LGTM!
1476-1523: LGTM!
1524-1534: LGTM!
1536-1547: LGTM!
1548-1555: LGTM!web-ui/modules/app.methods.claude-config.mjs (3)
19-70: LGTM!
117-174: LGTM!
207-220: LGTM!Also applies to: 222-242, 254-297, 299-324, 348-375
tests/unit/web-ui-logic.test.mjs (1)
191-205: LGTM!
| const source = raw.source === 'provider-cache' ? 'provider-cache' : (existing.source === 'provider-cache' && providerCacheRef ? 'provider-cache' : ''); | ||
| const externalCredentialType = next.apiKey | ||
| ? '' | ||
| : (next.externalCredentialType || previous.externalCredentialType || ''); | ||
| return { | ||
| const merged = { | ||
| apiKey: next.apiKey, | ||
| baseUrl: next.baseUrl, | ||
| model: next.model || previous.model || 'glm-4.7', | ||
| hasKey: !!(next.apiKey || externalCredentialType), | ||
| hasKey: !!(next.apiKey || externalCredentialType || providerCacheRef || raw.hasKey === true), | ||
| externalCredentialType, |
There was a problem hiding this comment.
mergeClaudeConfig can keep stale credential/source state after credentials are cleared
Line 265 and Line 273 can leave source: 'provider-cache' / hasKey: true even when apiKey, externalCredentialType, and providerCacheRef are empty, which creates inconsistent config state for downstream checks.
Suggested fix
- const source = raw.source === 'provider-cache' ? 'provider-cache' : (existing.source === 'provider-cache' && providerCacheRef ? 'provider-cache' : '');
+ const source = providerCacheRef && (raw.source === 'provider-cache' || existing.source === 'provider-cache')
+ ? 'provider-cache'
+ : '';
const merged = {
apiKey: next.apiKey,
baseUrl: next.baseUrl,
model: next.model || previous.model || 'glm-4.7',
- hasKey: !!(next.apiKey || externalCredentialType || providerCacheRef || raw.hasKey === true),
+ hasKey: !!(next.apiKey || externalCredentialType || providerCacheRef),
externalCredentialType,
targetApi: next.targetApi || previous.targetApi || 'responses'
};📝 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 source = raw.source === 'provider-cache' ? 'provider-cache' : (existing.source === 'provider-cache' && providerCacheRef ? 'provider-cache' : ''); | |
| const externalCredentialType = next.apiKey | |
| ? '' | |
| : (next.externalCredentialType || previous.externalCredentialType || ''); | |
| return { | |
| const merged = { | |
| apiKey: next.apiKey, | |
| baseUrl: next.baseUrl, | |
| model: next.model || previous.model || 'glm-4.7', | |
| hasKey: !!(next.apiKey || externalCredentialType), | |
| hasKey: !!(next.apiKey || externalCredentialType || providerCacheRef || raw.hasKey === true), | |
| externalCredentialType, | |
| const source = providerCacheRef && (raw.source === 'provider-cache' || existing.source === 'provider-cache') | |
| ? 'provider-cache' | |
| : ''; | |
| const externalCredentialType = next.apiKey | |
| ? '' | |
| : (next.externalCredentialType || previous.externalCredentialType || ''); | |
| const merged = { | |
| apiKey: next.apiKey, | |
| baseUrl: next.baseUrl, | |
| model: next.model || previous.model || 'glm-4.7', | |
| hasKey: !!(next.apiKey || externalCredentialType || providerCacheRef), | |
| externalCredentialType, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/modules/app.methods.startup-claude.mjs` around lines 265 - 274, The
mergeClaudeConfig function's source and hasKey field assignments can retain
stale credential state even after credentials are cleared. At line 265, the
source field logic should ensure that source is only set to 'provider-cache' if
at least one of the actual credential properties (next.apiKey,
externalCredentialType, or providerCacheRef) is present. Similarly, at line 273,
the hasKey field should only be true when there are valid credentials currently
present, rather than falling back to raw.hasKey === true which can preserve
stale state. Restructure both conditional checks to reset these fields to falsy
values when all credential properties are empty.
|
Addressed the non-blocking CodeRabbit test coverage suggestion. Follow-up change:
Re-verified after the follow-up:
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Current PR Diff ==="
gh pr diff 204 --repo SakuraByteCore/codexmateLength of output: 39001
The (ﾉ◕ヮ◕)ﾉ*:･ﾟ✧ |
|
Updated the PR branch for the What changed:
Verified before commit/tag:
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Current PR Diff ==="
gh pr diff 204 --repo SakuraByteCore/codexmateLength of output: 39781
[approve] |
2 participants
Summary
providerCacheRef/safe metadata only, whileapply-claude-configresolves the real key from~/.codexmate/claude-providers.jsonon the backend.refreshClaudeSelectionFromSettingsfrom importing a real cached-provider secret back into localStorage after applying a cached Claude provider.Verification
npm run lint→ Lint passed for 224 file(s)npm run test:unit→ All 685 tests passed/tmp/openclaw/codexmate-pr203-claude-cache-e2e.mjs):alpha-sync/beta-syncalpha-syncwrote the backend secret to isolated~/.claude/settings.json0600Summary by CodeRabbit
New Features
loadProviderCacheRecords(options)withbackgroundandforceRefresh.providerCacheRef, including a new endpoint to list safe provider-cache metadata.Bug Fixes
Tests