feat: SecretStorage for AI provider keys #1082
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughSwitches AI provider API keys to Obsidian SecretStorage: adds Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as "Provider Modal / Picker"
participant AppSecret as "Obsidian SecretStorage"
participant Secrets as "providerSecrets.ts"
participant Discovery as "modelDiscoveryService"
participant Provider as "AI Provider"
User->>UI: select/create provider & choose secret
UI->>AppSecret: setSecret(name, apiKey)
AppSecret-->>UI: confirm stored
UI->>Secrets: resolveProviderApiKey(app, provider)
Secrets->>AppSecret: getSecret(provider.apiKeyRef)
AppSecret-->>Secrets: return apiKey
Secrets-->>UI: resolved apiKey
UI->>Discovery: discoverProviderModels(provider, apiKeyOverride=resolvedKey)
Discovery->>Provider: request models using apiKeyOverride
Provider-->>Discovery: models list
Discovery-->>UI: models
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa61c0a532
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
🤖 Fix all issues with AI agents
In `@src/ai/providerSecrets.ts`:
- Around line 73-74: The call to app.secretStorage.setSecret inside
storeProviderApiKeyInSecretStorage must be wrapped in a try/catch so the
function honors its Promise<string | null> contract; change the logic around the
existing await Promise.resolve(app.secretStorage.setSecret(candidate, trimmed))
to try to set the secret, catch any thrown error, log the failure using the same
pattern used for getSecret (include error details), and return null on
failure—otherwise return the candidate string on success.
🧹 Nitpick comments (2)
src/migrations/migrateProviderApiKeysToSecretStorage.ts (2)
23-68: Consider cloning providers before mutation.The migration mutates
providerobjects directly (lines 36, 39-40, 59-60), but these are references fromsettingsStore.getState(). If the store returns frozen objects or uses immutable patterns, this could fail silently. Consider cloning the providers array upfront to ensure safe mutation.♻️ Suggested approach
const currentSettings = settingsStore.getState(); - const providers = currentSettings.ai.providers ?? []; + const providers = deepClone(currentSettings.ai.providers ?? []); let updated = false;This ensures mutations are applied to a local copy, and the final
deepClone(providers)at line 76 becomes redundant (though harmless).
29-35: UnnecessaryPromise.resolvewrappers.
secretStorage.getSecretandsecretStorage.setSecretalready return promises per Obsidian's API. ThePromise.resolve()wrappers are redundant.♻️ Suggested simplification
- const existing = await Promise.resolve( - secretStorage.getSecret(secretName), - ); + const existing = await secretStorage.getSecret(secretName); if (!existing && legacyKey) { - await Promise.resolve( - secretStorage.setSecret(secretName, legacyKey), - ); + await secretStorage.setSecret(secretName, legacyKey);
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@manifest.json`:
- Line 5: The bundled artifact versions.json is out of sync with manifest.json:
update the entry for plugin version "2.9.4" in versions.json to match the new
manifest minAppVersion "1.11.4" (replace the current mapping "2.9.4": "1.6.0"
with "2.9.4": "1.11.4"); then rebuild to ensure bundled artifacts are consistent
by running the project build (e.g., bun run build-with-lint) or commit the
manual change so versions.json and manifest.json stay synchronized.
🧹 Nitpick comments (1)
src/ai/providerSecrets.ts (1)
41-82: LGTM with an optional note on loop bounds.The implementation is solid: proper null checks, deduplication logic at line 67, and comprehensive error handling for both read and write operations (addressing the previous review feedback).
The
while (true)loop lacks an explicit upper bound—in an adversarial scenario with many colliding secret names, it could iterate indefinitely. In practice this is unlikely given typical usage patterns, so it's not a blocking concern.♻️ Optional: Add a safety bound to the loop
const base = buildSecretBase(provider); let candidate = base; let suffix = 1; +const MAX_SUFFIX = 1000; -while (true) { +while (suffix <= MAX_SUFFIX) { let existing: string | null = null; try { existing = await Promise.resolve( app.secretStorage.getSecret(candidate), ); } catch (err) { log.logWarning( `Failed to read SecretStorage entry "${candidate}": ${(err as Error).message ?? err}`, ); } if (!existing) break; if (existing === trimmed) return candidate; suffix += 1; candidate = `${base}-${suffix}`; } +if (suffix > MAX_SUFFIX) { + log.logWarning(`Exceeded maximum suffix attempts for secret "${base}"`); + return null; +}
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Summary
Testing
Summary by CodeRabbit
New Features
Documentation
Migrations
Chores
✏️ Tip: You can customize this high-level summary in your review settings.