fix: harden AI/WebDAV config sync to prevent data loss#168
Open
AmintaCCCP wants to merge 8 commits into
Open
Conversation
…lures - Wrap encrypt() calls in try-catch so a single config encryption failure doesn't abort the entire bulk sync transaction - Add safety guard: if ALL configs are skipped (missing keys), roll back the transaction instead of committing an empty database - Track and return skipped config details in the response so the frontend can surface partial failures - Switch syncAIConfigs/syncWebDAVConfigs from fetchWithTimeout to fetchWithRetry (3 retries) matching the pattern used by repo/release sync - Add pre-sync warnings for configs with empty apiKey/password - Add new error codes: SYNC_AI_CONFIGS_ALL_SKIPPED, SYNC_WEBDAV_CONFIGS_ALL_SKIPPED Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughServer bulk sync handlers accumulate per-item inserted/skipped outcomes, try per-item secret encryption with fallback to stored encrypted values, and return ChangesBulk Config Sync Per-Item Tracking and Resilience
Sequence Diagram(s)sequenceDiagram
participant Client as Frontend Sync Orchestrator
participant Adapter as BackendAdapter (sync methods)
participant Retry as fetchWithRetry
participant Server as Bulk Sync Handler (/api/configs/*/bulk)
participant Encrypt as ServerEncryption
participant Parser as Response Parser
Client->>Adapter: Trigger bulk syncs (concurrent)
Adapter->>Retry: PUT /configs/*/bulk (retry)
Retry->>Server: Send configs payload
Server->>Encrypt: Try encrypt per-item secret
Encrypt-->>Server: Return encrypted secret or throw
Server->>Server: Fallback to stored encrypted secret or mark skipped
Server-->>Retry: Respond { synced, skipped, errors } or throw ALL_CONFIGS_SKIPPED
Retry-->>Adapter: Return response
Adapter->>Parser: Parse JSON response
Parser-->>Adapter: Log skipped items / throw on partial failures (per method)
Adapter-->>Client: Resolve or reject result
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
🤖 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 `@server/src/routes/configs.ts`:
- Around line 195-199: When errMsg === 'ALL_CONFIGS_SKIPPED' the handler only
returns a numeric count so the frontend loses per-config rejection reasons;
update the 422 response to include the detailed skipped items (use the
already-populated syncResult.skipped variable) along with the existing
count/message so callers get per-config reasons—apply the same change to the
other identical branch (the second 422 block around the 407-411 area). Ensure
you reference errMsg, req.body?.configs, syncResult.skipped and the
res.status(...).json(...) call when making the change.
In `@src/services/backendAdapter.ts`:
- Around line 350-363: The current bulk AI config PUT call (the method in
backendAdapter that uses fetchWithRetry for `/configs/ai/bulk`) treats any
HTTP-ok response as success even when the parsed body reports data.skipped > 0,
which causes callers (autoSync and the settings panel) to clear pending changes
incorrectly; change the behavior so that after parsing res.json() you do not
silently log and return on partial skips — either throw a translated error via
throwTranslatedError (including the skipped count and data.errors) or return the
parsed result object (synced/skipped/errors) to callers so they can surface a
partial-sync state; apply the same fix to the other identical block around the
second occurrence (the similar fetchWithRetry/parse block).
🪄 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: b0cf4aaa-1f5d-478c-a2d9-d0236768c305
📒 Files selected for processing (3)
server/src/routes/configs.tssrc/services/backendAdapter.tssrc/utils/backendErrors.ts
… and throw on partial sync - Move syncResult declaration before try block so catch handler can access skip details in the ALL_CONFIGS_SKIPPED 422 response - Frontend sync methods now throw on partial failure (skipped > 0) instead of silently logging, so autoSync keeps _hasPendingLocalChanges and won't overwrite local data with incomplete backend state Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Closed
Problem 1: Backend returned 422 on ALL_CONFIGS_SKIPPED, causing frontend to throw and set _hasPendingLocalChanges=true, which blocked all subsequent pulls from backend and prevented other syncs (settings, repos) from completing. Problem 2: Backend decrypt_failed status caused empty apiKey values to overwrite valid local data, and subsequent push would skip all configs. Problem 3: handleSyncToBackend used sequential await, so one failure (like AI config sync) would abort the entire sync operation. Solutions: 1. Backend returns 200 (not 422) for ALL_CONFIGS_SKIPPED, since the transaction rollback preserves existing data correctly. 2. Frontend logs warnings instead of throwing on partial skip. 3. autoSync merges decrypt_failed configs with local values to preserve valid apiKey data instead of overwriting with empty strings. 4. handleSyncToBackend uses Promise.allSettled to isolate failures. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/services/autoSync.ts`:
- Around line 167-169: The code stores _lastHash.ai = quickHash(mergedConfigs)
but change detection compares _lastHash.ai against the raw backend payload hash,
causing changed.ai to stay true; make the hash basis consistent by assigning
_lastHash.ai to the same value used in the comparison (i.e., use the backend
payload hash variable instead of quickHash(mergedConfigs), or else change the
comparison to use quickHash(mergedConfigs)). Update the assignment near
state.setAIConfigs(mergedConfigs) so _lastHash.ai is set to the exact hash
computed from the backend payload (the same symbol used where changed.ai is
evaluated), ensuring consistent use of quickHash(...) on the same input across
both assignment and comparison.
🪄 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: 3be8756e-4cd4-434a-8528-679abdb07138
📒 Files selected for processing (4)
server/src/routes/configs.tssrc/components/settings/BackendPanel.tsxsrc/services/autoSync.tssrc/services/backendAdapter.ts
CodeRabbit identified that storing quickHash(mergedConfigs) in _lastHash.ai would cause a mismatch with the raw backend payload hash used in change detection, triggering infinite update loops on every poll cycle. Now storing hashes.ai (raw backend hash) to maintain consistency with the change detection comparison logic.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/services/autoSync.ts (1)
167-168:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep AI hash basis consistent with change detection.
Line 168 stores a hash of
mergedConfigs, but Line 95-Line 99 compares against hash of raw backend payload. This can cause perpetualchanged.ai = trueand repeated store updates/log spam.💡 Minimal fix
- _lastHash.ai = quickHash(mergedConfigs); + _lastHash.ai = hashes.ai;🤖 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 `@src/services/autoSync.ts` around lines 167 - 168, The AI hash is being computed from mergedConfigs while change detection compares the hash of the raw backend payload, causing mismatches and perpetual changed.ai=true; make the basis consistent by computing and storing the hash from the same object used for comparison—either change the comparison to use mergedConfigs or change the assignment _lastHash.ai = quickHash(mergedConfigs) to use the raw backend payload variable (the same ai payload used in the earlier hash check), ensuring you use the same identifier (e.g., aiPayload or fetchedAIConfig) and the same quickHash(...) call in both places.
🤖 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.
Duplicate comments:
In `@src/services/autoSync.ts`:
- Around line 167-168: The AI hash is being computed from mergedConfigs while
change detection compares the hash of the raw backend payload, causing
mismatches and perpetual changed.ai=true; make the basis consistent by computing
and storing the hash from the same object used for comparison—either change the
comparison to use mergedConfigs or change the assignment _lastHash.ai =
quickHash(mergedConfigs) to use the raw backend payload variable (the same ai
payload used in the earlier hash check), ensuring you use the same identifier
(e.g., aiPayload or fetchedAIConfig) and the same quickHash(...) call in both
places.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99754e34-1d83-41dd-a72e-059f695a626a
📒 Files selected for processing (5)
server/src/routes/configs.tssrc/components/settings/BackendPanel.tsxsrc/services/autoSync.tssrc/services/backendAdapter.tssrc/utils/backendErrors.ts
Contributor
|
Actionable comments posted: 0 |
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/services/backendAdapter.ts (1)
350-358:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAI partial skips are still treated as success by callers.
When
data.skipped > 0, this path only logs and returns fulfilled. Downstream orchestrators (src/services/autoSync.ts:233-297,src/components/settings/BackendPanel.tsx:93-119) treat fulfilled as success, so AI pending state can be cleared (and UI can report success) while configs were skipped. Consider matchingsyncWebDAVConfigsbehavior (reject on partial skip) or returning structured sync results to callers and handling partial state explicitly.Proposed minimal fix
- // Parse response to detect partial failures (some configs skipped due to missing keys). - // Log warnings but do not throw — the backend has preserved existing data via rollback, - // and throwing would block subsequent syncs of other data types (repos, settings). + // Parse response and reject on partial failure so callers can surface partial-sync state. try { const data = await res.json() as { synced?: number; skipped?: number; errors?: Array<{ id: string; name: string; reason: string }> }; if (data.skipped && data.skipped > 0) { const reasons = data.errors?.map(e => `${e.name}: ${e.reason}`).join('; ') ?? ''; - console.warn(`[sync] ${data.skipped} AI config(s) skipped${reasons ? ` (${reasons})` : ''}`); + throw new Error(`Sync AI configs partial failure: ${data.skipped} skipped${reasons ? ` (${reasons})` : ''}`); } - } catch { - // Ignore parse errors (empty body, etc.) + } catch (err) { + if (err instanceof Error && err.message.startsWith('Sync AI configs partial failure')) throw err; + // Ignore parse errors (empty body, etc.) }🤖 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 `@src/services/backendAdapter.ts` around lines 350 - 358, The current handler that parses res.json() treats partial skips (data.skipped > 0) as success by only logging a warning; update the behavior to mirror syncWebDAVConfigs by failing the promise when partial skips occur (i.e., throw or return a rejected Promise with a clear Error containing count and reasons built from data.errors) so callers in autoSync/BackendPanel see a rejection instead of a fulfilled result; specifically modify the branch that checks data.skipped (the code reading const data = await res.json() and the subsequent if (data.skipped && data.skipped > 0) block) to construct a descriptive error message (include `${data.skipped}` and the joined reasons) and throw it rather than only console.warn, or alternatively return a structured result object indicating partialSkip=true so callers can handle it explicitly.
🤖 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 `@server/src/routes/configs.ts`:
- Around line 195-199: The branch that checks errMsg === 'ALL_CONFIGS_SKIPPED'
should return a non-2xx response so downstream mapping can emit the proper
event: replace the res.json(...) call with a 422 response and include the error
code in the body (e.g. res.status(422).json({ code: 'ALL_CONFIGS_SKIPPED',
synced: 0, skipped: syncResult.skipped.length, errors: syncResult.skipped })).
Make the same change in the other identical branch that handles WebDAV sync (the
branch currently returning 200 for ALL_CONFIGS_SKIPPED) so both
SYNC_AI_CONFIGS_ALL_SKIPPED / SYNC_WEBDAV_CONFIGS_ALL_SKIPPED can be emitted.
---
Duplicate comments:
In `@src/services/backendAdapter.ts`:
- Around line 350-358: The current handler that parses res.json() treats partial
skips (data.skipped > 0) as success by only logging a warning; update the
behavior to mirror syncWebDAVConfigs by failing the promise when partial skips
occur (i.e., throw or return a rejected Promise with a clear Error containing
count and reasons built from data.errors) so callers in autoSync/BackendPanel
see a rejection instead of a fulfilled result; specifically modify the branch
that checks data.skipped (the code reading const data = await res.json() and the
subsequent if (data.skipped && data.skipped > 0) block) to construct a
descriptive error message (include `${data.skipped}` and the joined reasons) and
throw it rather than only console.warn, or alternatively return a structured
result object indicating partialSkip=true so callers can handle it explicitly.
🪄 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: 9f824ae0-6f06-4660-91e7-3069ec50f3e4
📒 Files selected for processing (5)
server/src/routes/configs.tssrc/components/settings/BackendPanel.tsxsrc/services/autoSync.tssrc/services/backendAdapter.tssrc/utils/backendErrors.ts
Backend now returns HTTP 422 (not 200) when all configs are skipped, so the error codes SYNC_AI_CONFIGS_ALL_SKIPPED and SYNC_WEBDAV_CONFIGS_ALL_SKIPPED reach the frontend error handling. Promise.allSettled keeps the rest of the sync run from being blocked.
Contributor
|
Actionable comments posted: 0 |
Align syncAIConfigs behavior with syncWebDAVConfigs - both now throw errors when configs are skipped, ensuring callers (autoSync, BackendPanel) properly track pending changes instead of clearing them on partial success. This addresses CodeRabbit's consistency concern about partial skip handling.
1. WebDAV decrypt_failed protection: autoSync now merges decrypt_failed WebDAV configs with local password values, same as AI configs. Prevents backend decryption failures from overwriting valid local data. 2. Align syncAIConfigs/syncWebDAVConfigs: added pre-sync validation warnings to syncAIConfigs for empty apiKey configs, matching the pattern already in syncWebDAVConfigs. 3. BackendPanel handleSyncToBackend: now passes all 7 settings fields (activeAIConfig, activeWebDAVConfig, categoryOrder, customCategories, assetFilters, collapsedSidebarCategoryCount) instead of only hiddenDefaultCategoryIds. This prevents category data loss when manually syncing to backend — the root cause of the user's "分类数据丢失" complaint in Issue #166 comments. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review the latest commit (a913d1c) which fixes remaining issues: WebDAV decrypt_failed protection, syncAIConfigs pre-sync warnings alignment, and BackendPanel settings completeness. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the diff for commit a913d1c
gh api repos/AmintaCCCP/GithubStarsManager/commits/a913d1c --jq '.files[] | "=== \(.filename) ===\n\(.patch // "(no patch)")'Length of output: 609 🏁 Script executed: #!/bin/bash
gh api repos/AmintaCCCP/GithubStarsManager/commits/a913d1c > /tmp/commit.json
jq -r '.files[] | "=== " + .filename + " ===\n" + (.patch // "(no patch)")' /tmp/commit.jsonLength of output: 3156 🏁 Script executed: #!/bin/bash
echo "=== syncSettings definition ==="
rg -n "syncSettings" --type ts -A 5 -B 2
echo ""
echo "=== settings route/endpoint ==="
rg -n "syncSettings\|settings.*bulk\|/configs/settings" --type ts -A 8 -B 2
echo ""
echo "=== store fields for new keys ==="
rg -n "activeAIConfig\|activeWebDAVConfig\|categoryOrder\|customCategories\|assetFilters\|collapsedSidebarCategoryCount" --type ts -B 1 -A 1 | head -80Length of output: 2396 🏁 Script executed: #!/bin/bash
echo "=== Full syncSettings call in autoSync.ts ==="
rg -n "syncSettings" src/services/autoSync.ts -A 15
echo ""
echo "=== Full syncSettings call in BackendPanel.tsx ==="
rg -n "syncSettings" src/components/settings/BackendPanel.tsx -A 15
echo ""
echo "=== WebDAV passwordStatus type definition ==="
rg -n "passwordStatus\|decrypt_failed" --type ts -B 1 -A 2 | head -60
echo ""
echo "=== AI apiKeyStatus type definition ==="
rg -n "apiKeyStatus\|decrypt_failed" --type ts -B 1 -A 2 | head -40Length of output: 2197 🏁 Script executed: #!/bin/bash
echo "=== WebDAV config type ==="
rg -n "WebDAVConfig\|WebdavConfig" --type ts -A 10 | head -60
echo ""
echo "=== AIConfig type ==="
rg -n "interface AIConfig\|type AIConfig" --type ts -A 10 | head -40
echo ""
echo "=== passwordStatus field ==="
rg -n "passwordStatus" --type ts -B 2 -A 2 | head -40
echo ""
echo "=== apiKeyStatus field ==="
rg -n "apiKeyStatus" --type ts -B 2 -A 2 | head -40Length of output: 5481 🏁 Script executed: #!/bin/bash
echo "=== SecretStatus type ==="
rg -n "SecretStatus" --type ts -A 5 -B 1
echo ""
echo "=== fetchWithRetry vs fetchWithTimeout in backendAdapter ==="
rg -n "fetchWithRetry\|fetchWithTimeout" src/services/backendAdapter.ts -B 1 -A 2 | head -60
echo ""
echo "=== syncSettings uses fetchWithTimeout still? ==="
rg -n "fetchWithTimeout\|fetchWithRetry" src/services/backendAdapter.ts | head -30Length of output: 2496 🏁 Script executed: #!/bin/bash
echo "=== All fetch method usages in backendAdapter.ts ==="
grep -n "fetchWith\|this\.fetch" src/services/backendAdapter.ts | head -50
echo ""
echo "=== syncSettings full method ==="
sed -n '420,445p' src/services/backendAdapter.ts
echo ""
echo "=== syncAIConfigs full method ==="
grep -n "syncAIConfigs\|syncWebDAVConfigs" src/services/backendAdapter.ts -A 30 | head -80Length of output: 7304
✅ WebDAV
|
| Field | autoSync.ts (before) | BackendPanel.tsx (before) | BackendPanel.tsx (after) |
|---|---|---|---|
activeAIConfig |
✅ | ❌ | ✅ |
activeWebDAVConfig |
✅ | ❌ | ✅ |
hiddenDefaultCategoryIds |
✅ | ✅ | ✅ |
categoryOrder |
✅ | ❌ | ✅ |
customCategories |
✅ | ❌ | ✅ |
assetFilters |
✅ | ❌ | ✅ |
collapsedSidebarCategoryCount |
✅ | ❌ | ✅ |
⚠️ Minor nit — inconsistent indentation in BackendPanel.tsx
The syncSettings(...) arguments use 12-space indent while the sibling calls use 8-space:
// Sibling calls (8-space indent):
backend.syncAIConfigs(aiConfigs),
backend.syncWebDAVConfigs(webdavConfigs),
backend.syncSettings({
activeAIConfig, // ← 12-space, should be 10-space to match
...
}),Not blocking, but worth a cleanup pass.
💡 Follow-up observation (out of scope for this commit)
syncSettings (line 426) still uses fetchWithTimeout while the bulk config sync methods now use fetchWithRetry. If settings sync is equally critical to reliability, it could be worth upgrading in a follow-up. Would you like me to open an issue for that?
Overall: The three stated goals are all correctly implemented. The commit is ready to merge from a functional standpoint.
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/services/backendAdapter.ts (1)
357-367: ⚡ Quick winExtract shared partial-sync response handling into one helper.
These two blocks are functionally identical; centralizing parse/throw logic will reduce drift risk and keep AI/WebDAV behavior consistent.
Refactor sketch
+ private async throwIfPartialSync( + res: Response, + label: 'AI configs' | 'WebDAV configs' + ): Promise<void> { + try { + const data = await res.json() as { + skipped?: number; + errors?: Array<{ id: string; name: string; reason: string }>; + }; + if (data.skipped && data.skipped > 0) { + const reasons = data.errors?.map(e => `${e.name}: ${e.reason}`).join('; ') ?? ''; + throw new Error(`Sync ${label} partial failure: ${data.skipped} skipped${reasons ? ` (${reasons})` : ''}`); + } + } catch (err) { + if (err instanceof Error && err.message.startsWith(`Sync ${label} partial failure`)) throw err; + } + } ... - try { - const data = await res.json() as { synced?: number; skipped?: number; errors?: Array<{ id: string; name: string; reason: string }> }; - if (data.skipped && data.skipped > 0) { - const reasons = data.errors?.map(e => `${e.name}: ${e.reason}`).join('; ') ?? ''; - throw new Error(`Sync AI configs partial failure: ${data.skipped} skipped${reasons ? ` (${reasons})` : ''}`); - } - } catch (err) { - if (err instanceof Error && err.message.startsWith('Sync AI configs partial failure')) throw err; - } + await this.throwIfPartialSync(res, 'AI configs'); ... - try { - const data = await res.json() as { synced?: number; skipped?: number; errors?: Array<{ id: string; name: string; reason: string }> }; - if (data.skipped && data.skipped > 0) { - const reasons = data.errors?.map(e => `${e.name}: ${e.reason}`).join('; ') ?? ''; - throw new Error(`Sync WebDAV configs partial failure: ${data.skipped} skipped${reasons ? ` (${reasons})` : ''}`); - } - } catch (err) { - if (err instanceof Error && err.message.startsWith('Sync WebDAV configs partial failure')) throw err; - } + await this.throwIfPartialSync(res, 'WebDAV configs');Also applies to: 397-407
🤖 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 `@src/services/backendAdapter.ts` around lines 357 - 367, Extract the duplicated try/catch into a single async helper (e.g., parseAndThrowPartialSyncResponse or handlePartialSyncResponse) that accepts the Response (res), calls await res.json(), checks data.skipped>0, constructs the reasons string from data.errors, and throws an Error whose message starts with "Sync AI configs partial failure" so existing instanceof/message checks keep working; ensure the helper catches JSON parse errors and only re-throws errors that start with that message. Replace both duplicated blocks (the current one using res and the similar block later) with a call to this helper to centralize behavior.
🤖 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 `@server/src/routes/configs.ts`:
- Around line 195-202: The 422 response branch that checks errMsg ===
'ALL_CONFIGS_SKIPPED' currently returns a top-level error string that suggests
missing API keys; change that top-level "error" message to a neutral phrase
(e.g., "No AI configs were synced") so it doesn't misattribute causes—leave
synced/skipped/errors (syncResult.skipped and syncResult.skipped array) intact
so per-config failure details remain in errors; apply the same neutralization to
the similar 412-419 branch and ensure backendErrors translation entries in
backendErrors.ts stay aligned (do not remove per-config details that are already
appended from data.error).
---
Nitpick comments:
In `@src/services/backendAdapter.ts`:
- Around line 357-367: Extract the duplicated try/catch into a single async
helper (e.g., parseAndThrowPartialSyncResponse or handlePartialSyncResponse)
that accepts the Response (res), calls await res.json(), checks data.skipped>0,
constructs the reasons string from data.errors, and throws an Error whose
message starts with "Sync AI configs partial failure" so existing
instanceof/message checks keep working; ensure the helper catches JSON parse
errors and only re-throws errors that start with that message. Replace both
duplicated blocks (the current one using res and the similar block later) with a
call to this helper to centralize behavior.
🪄 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: c8e661bc-c14d-4619-b17b-f58f29ab686c
📒 Files selected for processing (5)
server/src/routes/configs.tssrc/components/settings/BackendPanel.tsxsrc/services/autoSync.tssrc/services/backendAdapter.tssrc/utils/backendErrors.ts
CodeRabbit nitpick: the 422 error messages were too specific ('due to
missing API keys'/'due to missing passwords'), which could misdiagnose
the real failure. The per-config reasons are in the errors field, so
the top-level message should be neutral and point users to that field.
Aligned backend error messages, frontend error translations, and the
error code naming convention.
Owner
Author
|
@coderabbitai review the latest commit (e20b22f) |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the diff for commit e20b22f
gh api repos/AmintaCCCP/GithubStarsManager/commits/e20b22f > /tmp/commit_e20b22f.json
jq -r '.files[] | "=== " + .filename + " ===\n" + (.patch // "(no patch)")' /tmp/commit_e20b22f.json
echo ""
echo "=== Commit message ==="
jq -r '.commit.message' /tmp/commit_e20b22f.jsonLength of output: 402 🏁 Script executed: #!/bin/bash
# List commits on the PR branch
gh api "repos/AmintaCCCP/GithubStarsManager/pulls/168/commits" | jq -r '.[] | .sha + " " + .commit.message' | head -20
echo ""
echo "=== Try with full SHA lookup for prefix e20b22f ==="
gh api "repos/AmintaCCCP/GithubStarsManager/git/refs/heads/fix/168-ai-config-sync-failure" | jq -r '.object.sha'Length of output: 1613 🏁 Script executed: #!/bin/bash
# Try to fetch the commit directly with the short SHA
gh api "repos/AmintaCCCP/GithubStarsManager/commits/e20b22f" 2>&1| head -5 || true
# Check the PR's head SHA
gh api "repos/AmintaCCCP/GithubStarsManager/pulls/168" | jq -r '.head.sha, .head.ref, .head.repo.full_name'
echo ""
echo "=== Commits on the PR branch (via compare) ==="
BASE_SHA=$(gh api "repos/AmintaCCCP/GithubStarsManager/pulls/168" | jq -r '.base.sha')
HEAD_SHA=$(gh api "repos/AmintaCCCP/GithubStarsManager/pulls/168" | jq -r '.head.sha')
echo "Base: $BASE_SHA"
echo "Head: $HEAD_SHA"
# Try fetching all commits via the compare endpoint
gh api "repos/AmintaCCCP/GithubStarsManager/compare/${BASE_SHA}...${HEAD_SHA}" | jq -r '.commits[] | .sha + " " + .commit.message[:80]'Length of output: 2033 🏁 Script executed: #!/bin/bash
# Fetch the diff using the full SHA
gh api "repos/AmintaCCCP/GithubStarsManager/commits/e20b22f45dc09a1b87aa6123786af63a798ad157" > /tmp/commit_full.json
jq -r '.commit.message' /tmp/commit_full.json
echo ""
echo "=== Files changed ==="
jq -r '.files[] | "=== " + .filename + " ===\n" + (.patch // "(no patch)")' /tmp/commit_full.json
echo ""
echo "=== Stats ==="
jq -r '.files[] | .filename + ": +" + (.additions|tostring) + " -" + (.deletions|tostring)' /tmp/commit_full.jsonLength of output: 586 🏁 Script executed: #!/bin/bash
# Read current state of backendErrors.ts
cat src/utils/backendErrors.ts
echo ""
echo "=== 422 / ALL_CONFIGS_SKIPPED in configs.ts ==="
rg -n "ALL_CONFIGS_SKIPPED\|422\|neutral\|error.*message\|SKIPPED" server/src/routes/configs.ts -A 3 -B 2 | head -80
echo ""
echo "=== Error messages around skipped ==="
rg -n "SKIPPED\|skipped\|all.*config" server/src/routes/configs.ts -A 2 -B 2 | head -60
echo ""
echo "=== backendErrors.ts content ==="
cat src/utils/backendErrors.ts 2>/dev/null || fd "backendErrors" --exec cat {}Length of output: 10719
✅
|
| Code | EN | ZH |
|---|---|---|
SYNC_AI_CONFIGS_ALL_SKIPPED |
Failed to sync AI configs: all configs skipped, see errors for details |
同步 AI 配置失败：所有配置均被跳过，请查看错误详情 |
SYNC_WEBDAV_CONFIGS_ALL_SKIPPED |
Failed to sync WebDAV configs: all configs skipped, see errors for details |
同步 WebDAV 配置失败：所有配置均被跳过，请查看错误详情 |
Observations:
- ✅ Tone is consistent with sibling entries (
SYNC_AI_CONFIGS_FAILED,SYNC_WEBDAV_CONFIGS_FAILED). - ✅ The
see errors for detailsdirective usefully points the user to theerrorsfield in the 422 response body. - ✅ ZH translation is accurate and natural.
- ✅ No other files are touched — this is a clean, single-concern change.
Overall: This commit cleanly closes the last open nit from the previous review. The PR is now in good shape to merge. 🐇
Owner
Author
|
@coderabbitai ignore the nitpick about extracting shared partial-sync response handling — that's a pre-existing pattern not introduced by this PR. All previous actionable comments (422 response details, hash basis consistency, partial skip throw, neutral error messages) have been resolved in commits b083f9a, df23785, a913d1c, and e20b22f. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
With all actionable items resolved across commits ✏️ Learnings added
|
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
skipped/errors字段，前端可诊断哪些配置未同步syncAIConfigs/syncWebDAVConfigs改用 3 次重试，与 repo/release 同步一致SYNC_AI_CONFIGS_ALL_SKIPPED、SYNC_WEBDAV_CONFIGS_ALL_SKIPPEDRoot Cause
PR #167 修复了 AI 代理请求时 configId 找不到的问题，但配置同步本身有多个脆弱点：
encrypt()无 try-catch — 单个配置加密失败导致 DELETE + INSERT 事务回滚，返回 500syncAIConfigs用fetchWithTimeout而非fetchWithRetry— 瞬时网络错误导致永久失败Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Updates