Add keyring unlock option to per-profile unlock modal#52
Merged
Conversation
When a profile is managed by the keyring, the unlock modal now shows a 'Use keyring master password' toggle (defaulting to on). When enabled, the user enters the keyring master password to unlock just that profile instead of its individual password. The toggle hides automatically when switching to a non-managed profile. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
5 tasks
|
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 (1)
📝 WalkthroughWalkthroughAdds an optional keyring-based unlock mode to the unlock modal: a toggle to enter a keyring master password, fetch the selected profile's stored password from KeyringService, and use it to unlock the profile; updates labels, validation, and error messages, and snapshots form state before async calls. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Modal as UnlockModal
participant Keyring as KeyringService
participant Session as SessionService
User->>Modal: Select profile, enter password, toggle useKeyring?
alt useKeyring == true
Modal->>Keyring: getProfilePasswords(masterPassword)
alt keyring returns profile password
Keyring-->>Modal: profilePassword
Modal->>Session: unlockProfile(profileId, profilePassword)
Session-->>Modal: unlock success
Modal-->>User: Profile unlocked
else keyring error / missing profile
Keyring-->>Modal: error / no password
Modal-->>User: Incorrect keyring master password / Profile not found in keyring
end
else useKeyring == false
Modal->>Session: unlockProfile(profileId, enteredPassword)
alt unlock success
Session-->>Modal: success
Modal-->>User: Profile unlocked
else unlock failure
Session-->>Modal: error
Modal-->>User: Incorrect password or corrupted database.
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🤖 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/modals/unlock-modal.ts`:
- Around line 175-180: The catch-all error message in the unlock flow is
misleading when useKeyring is true because failures can be either opening the
keyring (master/keyring password) or decrypting the stored profile password;
update the logic in the UnlockModal/unlock flow to separate the two failure
cases: wrap the keyring-opening call (the branch that uses the keyring unlock)
in its own try/catch and call this.showError with "Incorrect keyring master
password or corrupted keyring." on failure, and then wrap the subsequent
retrieval/decryption of the profile password (the code that reads and decrypts
the stored profile password) in a separate try/catch that calls this.showError
with "Stored profile password is incorrect or corrupted." on failure; keep the
existing console.error calls for debugging and preserve the this.useKeyring flag
to choose messaging.
🪄 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: ab5e3d1f-4a32-4266-b2e1-2c0dab7c0a35
📒 Files selected for processing (1)
src/modals/unlock-modal.ts
…odal Split the keyring unlock branch into two distinct try/catch blocks: - Opening the keyring now reports "Incorrect keyring master password or corrupted keyring." - Decrypting the stored profile password now reports "Stored profile password is incorrect or corrupted." This addresses CodeRabbit feedback that the previous catch-all was misleading when the keyring opened fine but the stored password was stale. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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)
src/modals/unlock-modal.ts (1)
149-177:⚠️ Potential issue | 🟠 MajorFreeze the profile ID for the whole submit flow.
src/keyring-service.ts:33-52returns a map keyed by theprofileIdspassed intogetProfilePasswords(). Here we pass[this.selectedProfileId],await, and then readthis.selectedProfileIdagain. If the user changes the dropdown mid-submit, the lookup can miss the originally requested entry andonDonecan report the wrong profile. Snapshot the mutable form state before the firstawait.🛠️ Suggested fix
- const config = this.plugin.settings.profiles[this.selectedProfileId]; + const profileId = this.selectedProfileId; + const submittedPassword = this.password; + const useKeyring = this.useKeyring; + const config = this.plugin.settings.profiles[profileId]; if (!config) { this.showError("Profile not found."); return; } @@ - if (this.useKeyring) { + if (useKeyring) { let passwords: Map<string, string>; try { passwords = await this.plugin.keyringService.getProfilePasswords( this.plugin.settings.masterKeyringPath, - this.password, - [this.selectedProfileId], + submittedPassword, + [profileId], ); } catch (e) { @@ - const profilePassword = passwords.get(this.selectedProfileId); + const profilePassword = passwords.get(profileId); if (!profilePassword) { this.showError("Profile not found in keyring."); return; } try { - await this.plugin.sessionService.unlockProfile(this.selectedProfileId, config, profilePassword); + await this.plugin.sessionService.unlockProfile(profileId, config, profilePassword); } catch (e) { @@ } else { - await this.plugin.sessionService.unlockProfile(this.selectedProfileId, config, this.password); + await this.plugin.sessionService.unlockProfile(profileId, config, submittedPassword); } this.close(); - this.onDone?.(this.selectedProfileId); + this.onDone?.(profileId);Also applies to: 187-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modals/unlock-modal.ts` around lines 149 - 177, Snapshot the mutable selectedProfileId into a local const before the first await in the submit flow so the value used for getProfilePasswords, lookup in the returned Map, and the subsequent unlockProfile call is stable even if UI state changes mid-submit; specifically capture this.selectedProfileId into a local variable (e.g., selectedProfileId) before calling keyringService.getProfilePasswords and then use that local variable for the Map.get(...) and for plugin.sessionService.unlockProfile(...) (also apply the same snapshot for the later occurrence around the other await at the end of the submit flow).
🤖 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/modals/unlock-modal.ts`:
- Around line 61-64: When the unlock mode toggles (the useKeyring switch), stale
credentials must be cleared: update the toggle handler(s) that call
dd.setValue(...).onChange(...) and the methods that flip useKeyring (e.g.,
updateKeyringSection and any handlers referenced around the dd.setValue call and
the other related handlers) to reset this.password to an empty string and clear
the associated input element value (the password input DOM reference) whenever
useKeyring changes; ensure the UI input is also cleared so a previously typed
keyring master password cannot be reused as a profile password after flipping
modes.
---
Outside diff comments:
In `@src/modals/unlock-modal.ts`:
- Around line 149-177: Snapshot the mutable selectedProfileId into a local const
before the first await in the submit flow so the value used for
getProfilePasswords, lookup in the returned Map, and the subsequent
unlockProfile call is stable even if UI state changes mid-submit; specifically
capture this.selectedProfileId into a local variable (e.g., selectedProfileId)
before calling keyringService.getProfilePasswords and then use that local
variable for the Map.get(...) and for plugin.sessionService.unlockProfile(...)
(also apply the same snapshot for the later occurrence around the other await at
the end of the submit flow).
🪄 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: 61e55bf2-d332-452b-981f-11fd5aa84661
📒 Files selected for processing (1)
src/modals/unlock-modal.ts
- Capture selectedProfileId, password, and useKeyring into local consts before the first await in submit() to prevent stale reads if the user changes the dropdown or toggle while the async flow is in flight. - Store a reference to the password input element and add clearPassword() to reset both this.password and the DOM value whenever the unlock mode toggles (via the toggle handler or updateKeyringSection), so a previously typed keyring master password cannot be reused as a profile password after switching modes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This was referenced Apr 2, 2026
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.
Changes
Summary by CodeRabbit
New Features
Bug Fixes