fix: prevent API key erasure on save and resolve Graph View unmount crash#75
Conversation
|
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 with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR updates API key save and clear behavior in the Settings flow and Tauri keychain command, and refreshes the changelog and audit log entries. ChangesAPI key persistence and release notes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
AUDIT_LOG.md (2)
17-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove or merge redundant Graph View bugfix entry.
This entry is subsumed by the more comprehensive entry above (lines 5-16). Both share the same date and both cover Graph View fixes, but the newer entry is more accurate (correct
fg.graphDatareference vse.graphDatahere) and covers all changes in this release cycle. Keeping both creates audit trail fragmentation.🤖 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 `@AUDIT_LOG.md` around lines 17 - 25, Remove the redundant Graph View bugfix audit entry in AUDIT_LOG so the release notes stay consolidated under the more complete 2026-06-27 entry. Update or merge the duplicate text in the Graph View section so only one entry remains for this fix set, keeping the accurate description tied to GraphView.tsx and the onNodeClick/graphData cleanup changes.
5-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate redundant audit log entries.
There are two consecutive
2026-06-27entries that both document Graph View fixes. The new entry (lines 5-16) is more comprehensive and accurate (usesfg.graphDatamatching the actual code, and includes API key fixes). The older entry (lines 17-25) is now redundant and partially outdated (e.graphDatavsfg.graphData). For a clean audit trail, merge or remove the older entry so each fix is documented once.🤖 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 `@AUDIT_LOG.md` around lines 5 - 16, The audit log contains duplicate 2026-06-27 entries for Graph View fixes, and the older one is now redundant and partially outdated. Remove or merge the older Graph View-only entry so the consolidated `AUDIT_LOG.md` record appears once, keeping the more complete summary that matches the actual `GraphView.tsx`, `src/Settings.tsx`, and `src-tauri/src/commands/keychain.rs` changes.
🤖 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-tauri/src/commands/keychain.rs`:
- Around line 17-22: The Clear Key flow in keychain handling is swallowing
errors from delete_credential(), which can make the command succeed even when
removal fails. Update the keychain command logic around the delete_credential()
calls to treat the missing-entry case as a successful no-op, but propagate any
other delete errors back to the caller; use the existing keychain entry handling
in the relevant function that returns Ok(true) for the missing case.
In `@src/Settings.tsx`:
- Around line 104-106: Saving settings currently clears the API key implicitly
in the save flow when isApiKeySet is still false, which can erase an existing
key before getApiKeyStatus() finishes. Update Settings.tsx so the save handler
only persists a non-empty key through the existing key-setting path, and remove
the setApiKey('') branch from the save logic; keep key removal only in the
explicit Clear Key action. Use the save handler in Settings.tsx and the
electronAPI.setApiKey call sites to locate the change.
---
Nitpick comments:
In `@AUDIT_LOG.md`:
- Around line 17-25: Remove the redundant Graph View bugfix audit entry in
AUDIT_LOG so the release notes stay consolidated under the more complete
2026-06-27 entry. Update or merge the duplicate text in the Graph View section
so only one entry remains for this fix set, keeping the accurate description
tied to GraphView.tsx and the onNodeClick/graphData cleanup changes.
- Around line 5-16: The audit log contains duplicate 2026-06-27 entries for
Graph View fixes, and the older one is now redundant and partially outdated.
Remove or merge the older Graph View-only entry so the consolidated
`AUDIT_LOG.md` record appears once, keeping the more complete summary that
matches the actual `GraphView.tsx`, `src/Settings.tsx`, and
`src-tauri/src/commands/keychain.rs` changes.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b3de5ba-fcc8-4830-aabd-780756e7dc85
📒 Files selected for processing (4)
AUDIT_LOG.mdCHANGELOG.mdsrc-tauri/src/commands/keychain.rssrc/Settings.tsx
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
1 participant
Summary
Fixes two issues reported by users:
Cmd+Gcausedfg.graphData is not a functionTypeError.Root Cause & Changes
apiKeystate initialized to''. Saving settings ranelse { await window.electronAPI.setApiKey('') }, unintentionally deleting valid credentials from the OS keyring. UpdatedsaveSettingsto only overwrite credentials when a non-empty string is provided and added an explicit Clear Key UI button.keyring::Entry), setting a password on an existing item could return duplicate item errors. Updatedset_api_keyinkeychain.rsto delete existing items before storing updated secrets.typeof fg.method === 'function') and ref caching intoGraphView.tsxto prevent unmount race conditions when toggling Graph View.Verification
npm run lint&npx vitest runCHANGELOG.md&AUDIT_LOG.mdSummary by CodeRabbit