Vault - #69
Closed
gajendraxdev wants to merge 8 commits into
Closed
Conversation
Contributor
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughImplements a local encrypted Vault (crypto, redb store, IPC), Google Drive sync (OAuth PKCE, upload/download), Vault-backed SSH auth resolution and session guards, frontend Vault workspace/sidebar/panels/modals/hooks, domain/type/state updates, build/CI env handling, tests, and test-data reset scripts. ChangesEnd-to-end Vault and Sync integration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/modals/AddConnectionModal.tsx (1)
420-429:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLet the port field transition through an empty state.
Ignoring
NaNhere leaves the previous port in state when the user clears the input, so the field is awkward to edit and validation never sees the empty/invalid value. Handle''explicitly instead of dropping the update.Suggested fix
onChange={e => { setTouched((prev) => ({ ...prev, port: true })); - const p = parseInt(e.target.value, 10); - if (!isNaN(p)) setFormData({ ...formData, port: p }); + const value = e.target.value; + setFormData({ + ...formData, + port: value === '' ? undefined : parseInt(value, 10), + }); }}🤖 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/components/modals/AddConnectionModal.tsx` around lines 420 - 429, The onChange handler for the Port Input currently ignores empty strings because parseInt returns NaN, leaving the previous numeric port in state; update the handler in the Input component to explicitly handle e.target.value === '' by calling setFormData({ ...formData, port: '' }) (or null) so the field can transition to an empty state, and keep the existing setTouched update; otherwise parse and set the numeric value with parseInt as before so validation can observe the empty/invalid value.
🟠 Major comments (22)
src-tauri/build.rs-25-33 (1)
25-33:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDo not treat
GOOGLE_CLIENT_SECRETas safe to embed.Whitelisting this key means a local secret gets compiled into the desktop binary via
cargo:rustc-env. Desktop clients are public clients, so that value is extractable and no longer meaningfully secret. Please keep it filtered and use an installed-app OAuth client or a server-side exchange for flows that truly require a secret.🤖 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-tauri/build.rs` around lines 25 - 33, Remove the special-case that treats GOOGLE_CLIENT_SECRET as non-sensitive in the is_sensitive_env_key function: ensure that the function does not return false for "GOOGLE_CLIENT_SECRET" (i.e., delete or reverse the if upper == "GOOGLE_CLIENT_SECRET" branch) so the key remains filtered out from cargo:rustc-env embedding; reference the is_sensitive_env_key function and the key/upper variables when making the change.src-tauri/.cargo/config.toml-1-4 (1)
1-4:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail the build when the OAuth client ID is still the placeholder.
This default lets the app compile with
PLACEHOLDER_CLIENT_ID, so Google Drive auth can make it all the way to runtime before failing. Prefer leaving the variable unset by default, or add a release/build-time guard that rejects the placeholder value.🤖 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-tauri/.cargo/config.toml` around lines 1 - 4, The config currently supplies a default placeholder for GOOGLE_CLIENT_ID in the [env] section which allows builds to succeed with "PLACEHOLDER_CLIENT_ID"; remove the hardcoded default (unset GOOGLE_CLIENT_ID) and instead require the environment to provide it, or add a build-time guard (e.g., in the tauri build script or CI check) that reads GOOGLE_CLIENT_ID and fails the build if its value equals "PLACEHOLDER_CLIENT_ID" or is empty; update references to the env var so code expects it to be provided at build/release time and ensure the guard runs during packaging to prevent shipping the placeholder.src/store/settingsSlice.ts-359-383 (1)
359-383:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProtect rollback from out-of-order toggle saves.
updateSidebarSectionsSettingsalways restoresprevious.sidebarSectionson failure. Two quick toggles can race so an older failed request rolls back a newer successful one, leaving the UI stale until the next reload. Only roll back if the current slice still matches this call's optimistic value, or serialize sidebar-section writes.🤖 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/store/settingsSlice.ts` around lines 359 - 383, updateSidebarSectionsSettings currently always reapplies previous.sidebarSections on persist failure which can undo newer user toggles when requests race; instead, before applying rollback in the catch block, read current = get().settings.sidebarSections and only include keys in rollbackPatch where current[key] still equals the optimistic value you set earlier (the value from `updated.sidebarSections` / `updates`) — build rollbackPatch from changedKeys filtered by this equality check and then merge that patch back into settings; this preserves newer changes made after this optimistic update while still reverting keys that remain at the failed optimistic value.src-tauri/build.rs-17-18 (1)
17-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep newline escapes literal when emitting Cargo directives.
decode_escapes()can turn one.envvalue into multiple stdout lines, but Cargo parses build-script output line-by-line. A value containing\nor\rcan therefore corrupt the directive stream or emit unintendedcargo:instructions instead of setting a single env var.Also applies to: 52-89
🤖 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-tauri/build.rs` around lines 17 - 18, The build script currently prints env values after calling clean_env_value (which uses decode_escapes) causing actual newline characters to be emitted and breaking Cargo's line-based directive parsing; change the emission step so cargo:rustc-env receives a single-line value by converting real newlines and carriage returns back into literal escape sequences (e.g., replace '\n' -> '\\n' and '\r' -> '\\r') before calling println!("cargo:rustc-env={}={}", key, value), and apply the same fix to the other emit sites in the same file (the block around lines 52–89) so no decoded newlines are written to stdout as separate lines.src/components/layout/sidebar/SidebarActionButton.tsx-22-34 (1)
22-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a visible keyboard focus style to the shared button.
This component removes the default outline but never adds a
focus-visiblereplacement, so sidebar actions can become hard to track when tabbing. Please add an explicit focus treatment on the button itself.Suggested change
- "group relative flex items-center transition-all cursor-pointer select-none outline-none w-full rounded-lg border border-transparent", + "group relative flex items-center transition-all cursor-pointer select-none w-full rounded-lg border border-transparent outline-none focus-visible:outline-hidden focus-visible:ring-2 focus-visible:ring-app-border focus-visible:ring-offset-2 focus-visible:ring-offset-app-bg",🤖 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/components/layout/sidebar/SidebarActionButton.tsx` around lines 22 - 34, The button currently strips the native outline ("outline-none") but doesn't add a visible focus replacement; update the className in SidebarActionButton (the <button> that builds classes via cn(...)) to include explicit focus-visible styles (for example a ring/outline and offset like "focus-visible:ring-2 focus-visible:ring-app-accent focus-visible:ring-offset-2 focus-visible:outline-none" or your design system equivalent) so keyboard users get a clear focus indicator while keeping the intended visual when not focused.src-tauri/src/vault/crypto.rs-333-374 (1)
333-374: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winPin actual test vectors before merge.
These “known-answer” tests currently just run the same implementation twice. A deterministic but wrong KDF/AEAD output would still pass, so this block does not catch the regression class it claims to guard against.
🤖 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-tauri/src/vault/crypto.rs` around lines 333 - 374, The tests known_answer_kdf_is_reproducible and known_answer_aead_is_reproducible currently only compare two calls of derive_kek and encrypt_with_nonce which won't catch an implementation-wide bug; replace the two-call assertions with fixed expected hex vectors: run derive_kek(TEST_PASSPHRASE, &test_salt(), &test_params()) and hex-encode kek.as_bytes(), and replace the assert_eq! comparing kek and kek2 with assert_eq!(hex::encode(kek.as_bytes()), "<pinned_kek_hex>") using a hex value captured by running the test once on each CI platform; likewise run encrypt_with_nonce(&SecretKey::from_bytes([0x77u8;32]), &test_nonce(), TEST_PLAINTEXT, TEST_AAD) on each CI platform, capture envelope.ciphertext hex, and replace the envelope vs envelope2 comparison with assert_eq!(hex::encode(envelope.ciphertext), "<pinned_envelope_hex>"). Ensure references remain to derive_kek, encrypt_with_nonce, SecretKey::from_bytes, test_salt, test_params, and test_nonce so the test asserts deterministic known-answer vectors instead of equality of two calls.src-tauri/src/ssh.rs-439-445 (1)
439-445:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeduplicate keys before appending to the shared virtual agent.
These lines push every successful key into the process-wide
agent_keyslist, and this file never removes entries. Reconnecting with the same key will expose duplicate identities to forwarded-agent requests and grow memory for the rest of the app session.Proposed fix
if auth_success { let mut keys = match agent_keys.lock() { Ok(keys) => keys, Err(poisoned) => poisoned.into_inner(), }; - keys.push((*privkey).clone()); + let new_public_key = (*privkey).public_key_bytes(); + if !keys + .iter() + .any(|existing| existing.public_key_bytes() == new_public_key) + { + keys.push((*privkey).clone()); + } }🤖 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-tauri/src/ssh.rs` around lines 439 - 445, When auth_success is true and you obtain the mutex guard from agent_keys (agent_keys.lock(), keys variable), avoid blindly calling keys.push((*privkey).clone()); instead check whether that private key (or its public identifier) already exists in keys (e.g., keys.iter().any(|k| k == &*privkey) or compare the derived public key/identity) and only push when not present; update the block around auth_success / agent_keys.lock() to perform this deduplication before pushing to prevent duplicate entries and unbounded growth.src/components/vault/RecoveryKeyModal.tsx-84-96 (1)
84-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMigrate CSS-variable references to Tailwind v4 parentheses syntax.
Lines 84-96 use Tailwind v3 style bracketed
var()arbitrary values:border-[var(--color-app-border)],text-[var(--color-app-muted)], etc. Tailwind v4 requires parentheses syntax:border-(--color-app-border),text-(--color-app-muted), etc. This pattern appears in 388 places across the codebase and should be systematically updated to ensure styles generate correctly.🤖 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/components/vault/RecoveryKeyModal.tsx` around lines 84 - 96, In RecoveryKeyModal (the JSX block rendering the recovery key and the surrounding div with classes like "rounded-xl border border-[var(--color-app-border)]/60 bg-[var(--color-app-bg)] p-4" and the inner spans with "text-[var(--color-app-muted)]" / "text-[var(--color-app-accent)]"), replace Tailwind v3 bracketed arbitrary var(...) usages with Tailwind v4 parentheses syntax (e.g., border-[var(--color-app-border)] → border-(--color-app-border), text-[var(--color-app-muted)] → text-(--color-app-muted), bg-[var(--color-app-bg)] → bg-(--color-app-bg)) consistently across these classNames; update the same pattern wherever it appears (about 388 occurrences) so all classes using var(...) use the parentheses form.src/features/connections/domain/merge.ts-17-28 (1)
17-28:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrip plaintext fields whenever the merged result stays vault-backed.
When
incoming.authRefis present, this helper returnsincominguntouched. The update path then spreads that overmatch, so any existingpassword/privateKeyPathcan survive next to the winning vault ref. That keeps plaintext secrets in persisted state after the connection has been vaulted.Suggested fix
export const preserveVaultCredentialOnUpdate = ( existing: Connection, incoming: Connection, ): Connection => { - if (!existing.authRef || incoming.authRef) return incoming; + const authRef = incoming.authRef ?? existing.authRef; + if (!authRef) return incoming; return { ...incoming, - authRef: existing.authRef, + authRef, password: undefined, privateKeyPath: undefined, }; };Also applies to: 61-62
🤖 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/features/connections/domain/merge.ts` around lines 17 - 28, The merge helper preserveVaultCredentialOnUpdate currently returns incoming unchanged when incoming.authRef exists, which lets plaintext password/privateKeyPath survive; change preserveVaultCredentialOnUpdate so that if incoming.authRef is present you return {...incoming, password: undefined, privateKeyPath: undefined} (i.e. strip plaintext fields whenever the merged result is vault-backed), and keep the existing behavior of copying existing.authRef when incoming lacks it; apply the identical fix to the other helper with the same logic referenced at lines 61-62.src/vault/useVaultStore.ts-45-53 (1)
45-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate item-load failures during unlock.
unlock()andunlockWithRecoveryKey()awaitrefreshItems(), butrefreshItems()swallows its own errors. That means the UI can treat unlock as successful and close the modal even when listing vault items failed.🔁 Let callers see the failure
refreshItems: async () => { try { const items = await vaultIpc.itemList(); set({ items }); } catch (e: unknown) { const msg = extractErrorMessage(e); console.warn('[Vault] refreshItems failed:', e); set({ items: [], error: msg }); + throw e; } },Also applies to: 68-90
🤖 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/vault/useVaultStore.ts` around lines 45 - 53, refreshItems currently catches errors and swallows them (setting items:[] and error) which prevents callers like unlock() and unlockWithRecoveryKey() from seeing failures; update refreshItems (and the similar block at lines 68-90) to still set the error state but then rethrow the error (or return a rejected promise) so callers can observe the failure and handle it (i.e., preserve the existing set({ items: [], error: msg }) but add "throw e" after logging), and ensure unlock and unlockWithRecoveryKey continue to await refreshItems so modal logic reacts to the thrown error.src/components/modals/useAutoVault.ts-60-68 (1)
60-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPasted-key rotation never retires the old vault item.
Unlike
autoVaultPassword()andautoVaultKeyFile(),savePastedKey()creates a new vault record and swapsauthRefwithout cleaning up the previousexisting.authRef.itemId. Re-editing the same connection in pasted-key mode will accumulate orphaned credentials in the vault.Also applies to: 92-95
🤖 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/components/modals/useAutoVault.ts` around lines 60 - 68, savePastedKey is creating a new vault item and swapping authRef but never deletes the previous vault item, causing orphaned credentials; update savePastedKey to call the existing deleteOldAuthItem() (or inline similar logic) after successfully creating the new vault item and after you update the connection's authRef so the previous existing.authRef.itemId is removed, and ensure vaultIpc.itemDelete is awaited or its promise handled with a catch that logs/shows the same toast message; also make the same change where pasted-key rotation is handled (the other block referenced around lines 92-95) so old items are always retired.src-tauri/src/vault/types.rs-31-33 (1)
31-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove
Debugfrom decrypted records.
PlaintextRecordcontains decryptedsecretandnotes. WithDebugderived, a stray{:?}log or panic context can dump plaintext credentials into logs/crash reports, which defeats the vault boundary. Prefer a redacted custom formatter if you still need diagnostics.🔐 Minimal hardening
-#[derive(Debug, Clone, Serialize, Deserialize, Zeroize, ZeroizeOnDrop)] +#[derive(Clone, Serialize, Deserialize, Zeroize, ZeroizeOnDrop)]🤖 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-tauri/src/vault/types.rs` around lines 31 - 33, The PlaintextRecord struct currently derives Debug which can accidentally expose decrypted secret and notes; remove Debug from the derive list for PlaintextRecord and, if diagnostics are still required, implement a custom Debug (or fmt::Display) for PlaintextRecord that redacts sensitive fields (e.g., show lengths or placeholders for secret and notes) so no plaintext is printed while retaining safe diagnostic info; update any uses that relied on auto-derived Debug to use the redacting formatter or explicit debug helpers instead.src/vault/useVaultStore.ts-38-40 (1)
38-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset vault state when
refresh()cannot determine status.If
vaultIpc.status()throws after a previous unlock, this catch only stores the error and leaves the oldstatus/itemsintact. The UI can keep showing stale decrypted data even though the real vault state is now unknown.🧹 Safer failure path
} catch (e) { - set({ error: extractErrorMessage(e) }); + set({ + status: null, + items: [], + error: extractErrorMessage(e), + }); } finally {🤖 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/vault/useVaultStore.ts` around lines 38 - 40, When refresh() calls vaultIpc.status() and it throws, the catch currently only calls set({ error: extractErrorMessage(e) }) leaving previous status/items in the store; change the error path in the catch block inside refresh() so it also clears sensitive/stale state by calling set to reset status, items, and any unlocked flags (e.g., set({ status: undefined, items: [], isUnlocked: false, error: extractErrorMessage(e) })) so the UI cannot show stale decrypted data; locate the catch around vaultIpc.status() in useVaultStore.ts where set and extractErrorMessage are used and update that single catch block accordingly.src/vault/syncIpc.ts-95-97 (1)
95-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't mark the provider disconnected on transfer errors.
These catch blocks broadcast
connected: falsefor any upload/download failure, but a transient Drive or network error does not mean the OAuth connection was revoked. That will flip listeners to “Not connected” and throw away the last known metadata after ordinary sync failures. Preserve the cached connection state here and attach the error instead.Also applies to: 112-114
🤖 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/vault/syncIpc.ts` around lines 95 - 97, The catch blocks currently call notifySyncStatusChanged(provider, { connected: false, error: ... }) which incorrectly marks the OAuth connection disconnected on transient transfer errors; instead, read the provider's current cached connection state (e.g., the existing sync-status cache or previous status object used by notifySyncStatusChanged) and call notifySyncStatusChanged(provider, { connected: <cachedConnected>, error: <error message> }) so you preserve the last known connected boolean while attaching the error; update both locations (the catch around the upload/download and the similar catch at the later block) to use the cached connected value rather than false.src/store/connectionSlice.ts-144-161 (1)
144-161:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove raw connection payloads from renderer logs.
loadedanduniqueConnscan include plaintext credentials, private key paths, hosts, and vault refs. Dumping them to the renderer console leaks sensitive connection data to devtools and collected logs. Please either remove these logs or redact auth-related fields before logging.🤖 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/store/connectionSlice.ts` around lines 144 - 161, The renderer is currently logging raw connection payloads (loaded and uniqueConns) which may contain sensitive fields; update the loadConnectionsIpc handling in connectionSlice.ts so you no longer print raw objects from loadConnectionsIpc or uniqueConns: either remove the console.info/console.log calls that output loaded and uniqueConns, or replace them with a sanitized log that maps PersistedConnection entries (from conns/uniqueConns) to a redacted form (omit or replace values for keys like password, privateKeyPath, vaultRef, token, username, host, port, and any auth-related fields) before logging; keep the deduplication logic (uniqueConns creation) and only log non-sensitive metadata (e.g., counts or folder names via folders variable) or a sanitized summary instead.src/vault/ipc.ts-8-17 (1)
8-17:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid returning decrypted secrets from the list API.
itemList()currently exposesVaultItem.secretto the renderer for every stored item, even though the UI here only needs metadata likeid,label, andkind. That unnecessarily loads all vault contents into renderer memory and makes accidental disclosure via logs, devtools, or any future XSS much worse. A metadata-only list DTO plus an explicit secret-resolve path when a connection actually needs the credential would keep the vault boundary much tighter.Also applies to: 52-53
🤖 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/vault/ipc.ts` around lines 8 - 17, itemList() is leaking VaultItem.secret (and optional notes) to the renderer; change the list API to return a metadata-only DTO (e.g., VaultItemMeta with id, kind, label, revision, createdAt, updatedAt) instead of VaultItem so secrets aren't serialized, and add an explicit resolver method (e.g., resolveVaultSecret(id) or getVaultItemSecret) that only returns VaultItem.secret when the renderer explicitly requests a credential; update the IPC handlers to use the new VaultItemMeta for listing and restrict the secret-resolve path to a separate, audited IPC channel.src/components/settings/tabs/VaultTab.tsx-359-385 (1)
359-385:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't deduplicate vault items by label alone.
This algorithm keeps at most one item per
label, so if two different connections intentionally reference two different items with the same label, one referenced item still gets deleted. That contradicts the dialog copy and can break live connections after cleanup. Group duplicates by a stable credential fingerprint instead (for examplekind+ secret hash), and only delete unreferenced entries within the same fingerprint group.🤖 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/components/settings/tabs/VaultTab.tsx` around lines 359 - 385, The current handleDeduplicateItems routine deduplicates solely by item.label which can delete distinct credentials that share a label; change the logic to group items by a stable credential fingerprint (e.g., `${item.kind}:${hashSecret(item)}`) instead of label, then within each fingerprint group sort by createdAt and apply the existing referenced-preserving keep/delete logic (use connections to build referencedIds as before) so only items with the same fingerprint are deduplicated; update variables used in the function (e.g., kept, toDelete, sorted) to operate per-group and ensure referenced items in a group are always preferred and not deleted.src-tauri/src/vault/store.rs-66-67 (1)
66-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate
try_open()failures instead of masking them.Discarding this result turns a corrupt or inaccessible
vault.redbintoUninitializedhere, and intoNotInitializedin the same pattern used byunlock/unlock_with_recovery_key. That hides the real recovery path and misreports vault state.Suggested fix
- let _ = self.try_open(); + self.try_open()?;Apply the same change to the other
let _ = self.try_open();call sites.🤖 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-tauri/src/vault/store.rs` around lines 66 - 67, The status() method currently discards the result of self.try_open(), masking real errors; change the call to propagate failures instead of ignoring them — replace the ignored call (let _ = self.try_open();) with a propagated call (use the try_open() result with the ? operator or explicitly return Err mapped from try_open()) so that status() returns the actual VaultError on open failures; apply the same change at the other call sites that use let _ = self.try_open(); (also check similar patterns in unlock and unlock_with_recovery_key to ensure they propagate real errors rather than turning them into Uninitialized/NotInitialized).src-tauri/src/vault/store.rs-497-505 (1)
497-505:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace the vault atomically during import.
The function backs up the existing vault but never restores it if the import fails. If
std::fs::copy()is interrupted or partially written (lines 504-506), or iftry_open()subsequently fails (line 508), the vault is left corrupted with the backup unused. Use atomic file operations: copy to a temporary file with fsync, then atomically rename it into place, or restore the backup on any error.🤖 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-tauri/src/vault/store.rs` around lines 497 - 505, The current import copies directly over self.vault_path() and leaves a backup unused if the copy or subsequent try_open() fails; change import to write atomically by copying src_path into a temporary file in the same directory (e.g., dest.with_extension("tmp.pre-import")), fsync the temp file, then perform an atomic rename/move to dest, and fsync the parent directory; if any step fails, restore the original backup (dest.with_extension("redb.pre-import")) or remove the temp file and return a VaultError::InvalidData; update the code paths around vault_path(), the copy step, and the place where try_open() is called to use this atomic replace/restore flow and ensure all errors map to VaultError::InvalidData.src-tauri/src/vault/migration.rs-173-205 (1)
173-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t dedupe migrated secrets by label alone.
(kind, label)is not a unique credential identity. Two connections can share the same generated label but have different passwords/keys, and this will silently bind both to the same vault item before clearing the original local secret. Key the reuse map by a stable secret fingerprint as well, or only reuse when the decrypted secret matches.Suggested fix
- let mut existing_by_label: HashMap<(String, String), (String, u64)> = HashMap::new(); + let mut existing_by_identity: HashMap<(String, String, String), (String, u64)> = HashMap::new(); for record in existing_records { - let key = (record.kind.clone(), record.label.clone()); - existing_by_label + let key = ( + record.kind.clone(), + record.label.clone(), + record.secret.clone(), + ); + existing_by_identity .entry(key) .and_modify(|current| { if record.created_at >= current.1 { *current = (record.id.clone(), record.created_at); } }) .or_insert((record.id.clone(), record.created_at)); } for migration in &prepared { let kind = migration.kind.as_str(); - let lookup_key = (kind.to_string(), migration.label.clone()); - if let Some((existing_id, _)) = existing_by_label.get(&lookup_key) { + let lookup_key = ( + kind.to_string(), + migration.label.clone(), + migration.secret.clone(), + ); + if let Some((existing_id, _)) = existing_by_identity.get(&lookup_key) { linked.push((migration.index, existing_id.clone(), migration.kind.clone())); continue; }🤖 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-tauri/src/vault/migration.rs` around lines 173 - 205, The current reuse map existing_by_label keyed only by (kind, label) can incorrectly dedupe different secrets; change the lookup to include a stable secret fingerprint (or only reuse after decrypt-and-compare) so we only link identical credentials. Concretely: compute a fingerprint/hash of migration.secret (or decrypt the vault record to compare plaintext) and change existing_by_label to key on (kind, label, fingerprint) (or perform a decrypted-secret equality check before reuse) in the lookup where lookup_key is built, in the insertion when a new record is created, and in the branch that chooses an existing_id to link; keep created_for_cleanup and linked logic the same but only push when the fingerprint (or decrypted match) confirms the secrets are identical.src-tauri/src/commands.rs-979-1005 (1)
979-1005:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftClean up active tunnels when disconnecting vault-backed sessions.
ssh_disconnect_vault_backedcloses PTYs and removes connections fromstate.connections, but does not clean up associated tunnels. Since tunnels hold cloned SSH session references andtunnel_iddoes not embedconnection_id, there is no way to identify and stop tunnels belonging to the disconnected connection. This leaves port forwards active after vault lock, circumventing the security boundary.Track which tunnels belong to each connection (either by embedding
connection_idintunnel_idor maintaining a reverse map inAppState), then callstate.tunnel_manager.stop_tunnel()for each associated tunnel before removing the connection handle.🤖 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-tauri/src/commands.rs` around lines 979 - 1005, ssh_disconnect_vault_backed currently closes PTYs and removes entries from state.connections but leaves associated tunnels running; update AppState and ssh_disconnect_vault_backed so you can find and stop tunnels for a given connection_id (either embed connection_id in tunnel_id when creating tunnels or maintain a reverse map like connection_to_tunnels: HashMap<ConnectionId, Vec<TunnelId>> in AppState). In ssh_disconnect_vault_backed, before removing the connection and after pty_manager.close_by_connection, iterate the associated TunnelIds and call state.tunnel_manager.stop_tunnel(tunnel_id).await (or the sync equivalent), then remove those tunnel entries from whatever tunnel registry you maintain, and only then remove the connection from state.connections to ensure all port-forwards are cleaned up.src-tauri/src/vault/store.rs-487-489 (1)
487-489:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate vault schema before overwriting vault.redb file.
Database::open(src_path)validates only that the file is a redb database, not that it contains the required vault tables (VAULT_META,KEY_SLOTS,RECORDS). A non-vault redb file will pass this check, overwrite the existing vault at line 504, and only fail gracefully whenstatus()returnsVaultStatus::Uninitialized. The vault is silently corrupted with no actionable error.Validate required tables exist before copying the file at line 504.
🤖 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-tauri/src/vault/store.rs` around lines 487 - 489, Database::open(src_path) only verifies the file is a redb DB but not that it contains the vault schema, so first open the source DB with Database::open(src_path) and then explicitly verify the presence of the required tables "VAULT_META", "KEY_SLOTS", and "RECORDS" (e.g. via the DB's table/metadata inspection API or by attempting a read transaction that queries those tables) before proceeding to copy/overwrite the current vault file; if any required table is missing, return VaultError::InvalidData with a clear message instead of continuing to the existing copy/overwrite and status() check.
🟡 Minor comments (3)
src/components/vault/RecoveryKeyModal.tsx-16-22 (1)
16-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset the copy state when the modal/key changes.
copiedis only cleared by the 2s timer or unmount. If the dialog is reopened quickly, orrecoveryKeychanges before that timer fires, the next key can render with a stale “Copied!” state.Proposed fix
- useEffect(() => { - return () => { - if (copyTimeoutRef.current !== null) { - window.clearTimeout(copyTimeoutRef.current); - } - }; - }, []); + useEffect(() => { + if (!isOpen) { + setCopied(false); + if (copyTimeoutRef.current !== null) { + window.clearTimeout(copyTimeoutRef.current); + copyTimeoutRef.current = null; + } + } + + return () => { + if (copyTimeoutRef.current !== null) { + window.clearTimeout(copyTimeoutRef.current); + } + }; + }, [isOpen, recoveryKey]);Also applies to: 24-36
🤖 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/components/vault/RecoveryKeyModal.tsx` around lines 16 - 22, The component RecoveryKeyModal leaves the "copied" state stale if the modal is reopened or recoveryKey changes before the existing 2s timer fires; update the effects so that when recoveryKey (or the modal open prop if present) changes you clear any existing copyTimeoutRef and reset copied to false. Specifically, in RecoveryKeyModal add/use an effect that depends on recoveryKey (and modal open flag if applicable) which calls window.clearTimeout(copyTimeoutRef.current), sets copyTimeoutRef.current = null, and calls setCopied(false), and keep the existing unmount cleanup that clears the timeout as well.src/features/connections/application/tabService.ts-125-133 (1)
125-133:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnsure
ensureVaultTabStateactually enforces a single vault tab.On Line 125, only the first vault tab is updated; extra
type === 'vault'tabs remain untouched. That allows duplicate vault tabs to persist in recovered/legacy state.💡 Proposed fix
export const ensureVaultTabState = ( tabs: Tab[], profileId: VaultProfileId, ): { tabs: Tab[]; activeTabId: string; activeConnectionId: null } => { const existing = tabs.find((tab) => tab.type === 'vault'); if (existing) { + let keptVault = false; + const nextTabs = tabs.flatMap((tab) => { + if (tab.type !== 'vault') return [tab]; + if (keptVault) return []; + keptVault = true; + return [{ ...tab, vaultProfileId: profileId }]; + }); return { - tabs: tabs.map((tab) => - tab.id === existing.id ? { ...tab, vaultProfileId: profileId } : tab, - ), + tabs: nextTabs, activeTabId: existing.id, activeConnectionId: null, }; }🤖 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/features/connections/application/tabService.ts` around lines 125 - 133, ensureVaultTabState currently only updates the first found vault tab and leaves other tabs with type === 'vault' untouched, allowing duplicates to persist; change the logic in ensureVaultTabState so when an existing vault tab is found (variable existing) you return a tabs array that removes any other tabs with type === 'vault' and only keeps the updated vault tab (the one with id === existing.id) with vaultProfileId set to profileId, and set activeTabId to existing.id and activeConnectionId to null; ensure you adjust the tabs transformation (the mapping/filtering around tabs) rather than only mapping to the matched id so legacy duplicate vault tabs are eliminated.src/components/vault/VaultUnlockModal.tsx-148-156 (1)
148-156:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the show/hide controls keyboard-focusable.
Both visibility toggle buttons set
tabIndex={-1}, so keyboard users can't reach them. That makes the passphrase/recovery-key reveal control mouse-only.⌨️ Minimal fix
<button type="button" onClick={() => setShowPass((v) => !v)} aria-label={showPass ? 'Hide recovery key' : 'Show recovery key'} className="text-app-muted hover:text-app-text transition-colors" - tabIndex={-1} ><button type="button" onClick={() => setShowPass((v) => !v)} aria-label={showPass ? 'Hide password' : 'Show password'} className="text-app-muted hover:text-app-text transition-colors" - tabIndex={-1} >Also applies to: 168-176
🤖 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/components/vault/VaultUnlockModal.tsx` around lines 148 - 156, The visibility toggle buttons in VaultUnlockModal (the button that calls setShowPass and the similar recovery-key toggle) are not keyboard-focusable because they set tabIndex={-1}; remove the tabIndex prop (or set tabIndex={0}) so the buttons are reachable via keyboard, keep the existing aria-label and onClick handler (setShowPass) intact to preserve accessibility and behavior.
🧹 Nitpick comments (4)
src/components/layout/TabBar.tsx (1)
76-84: ⚡ Quick winExtract the tab-type icon mapping into one helper.
The
tab.type→ icon switch now lives in two places. This PR had to update both forvault; the next tab type will have the same drift risk unless the mapping is centralized.Also applies to: 402-409
🤖 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/components/layout/TabBar.tsx` around lines 76 - 84, The icon selection logic for tabs is duplicated; extract it into a single helper (e.g., getTabIcon or getIconForTab) that accepts the tab (or tab.type and connection) and returns the appropriate JSX icon, centralizing cases for 'port-forwarding', 'settings', 'release-notes', 'vault' and the fallback using connection.icon; then replace the inline IIFE in TabBar (the current anonymous function returning Network/SettingsIcon/Gift/Shield/OSIcon) and the other duplicate site (the block around the other occurrence) to call this helper so future tab-type additions only need one change.src-tauri/src/vault/error.rs (1)
33-37: ⚡ Quick winReturn the wrapped storage error as the direct source.
Line 36 currently returns
e.source()instead of theanyhow::Erroritself, so callers can lose the directStorage(...)cause from the error chain. Return the wrapped error here, not its nested source.🤖 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-tauri/src/vault/error.rs` around lines 33 - 37, The match arm in the source(&self) method is returning e.source() for Self::Storage, which hides the direct Storage(...) cause; change the Self::Storage branch to return the wrapped storage error itself (i.e., return a reference to the stored anyhow::Error as the direct source) instead of calling e.source(); ensure the returned value coerces to Option<&(dyn std::error::Error + 'static)> so the Storage variant appears directly in the error chain.src-tauri/src/types.rs (1)
29-35: ⚡ Quick winAdd
skip_serializingfor defense-in-depth protection.While current code doesn't serialize
PrivateKeyData(it's resolved toPrivateKeyDataonly transiently during authentication, then discarded before storage), the struct'sSerializederive allows it. Addingskip_serializingprevents accidental key material leaks if the code is refactored to serialize configs in the future.Proposed fix
- #[serde(skip_deserializing)] + #[serde(skip_serializing, skip_deserializing)] PrivateKeyData {🤖 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-tauri/src/types.rs` around lines 29 - 35, The PrivateKeyData variant currently has #[serde(skip_deserializing)] but can still be serialized; add #[serde(skip_serializing)] to the PrivateKeyData declaration (the enum/struct variant named PrivateKeyData with fields key_data and passphrase) so serde will refuse to serialize this sensitive key material as a defense-in-depth measure.tests/connectionDomain.test.mjs (1)
235-295: ⚡ Quick winCover plaintext cleanup whenever
authRefwins.These cases only prove that the winning
authRefis selected. They still pass ifpasswordorprivateKeyPathsurvives from the matched row, which is the risky branch in the merge logic. Please assert both fields areundefinedin the incoming-vault scenarios too.Suggested assertions
const preserved = preserveVaultCredentialOnUpdate(existing, incoming); assert.deepEqual(preserved.authRef, authRef); assert.equal(preserved.password, undefined); + assert.equal(preserved.privateKeyPath, undefined); }); runTest('preserveVaultCredentialOnUpdate lets incoming vault auth win when both sides are vaulted', () => { @@ const preserved = preserveVaultCredentialOnUpdate(existing, incoming); assert.deepEqual(preserved.authRef, incomingAuthRef); + assert.equal(preserved.password, undefined); + assert.equal(preserved.privateKeyPath, undefined); });🤖 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 `@tests/connectionDomain.test.mjs` around lines 235 - 295, Update the two tests that assert which authRef wins (both using preserveVaultCredentialOnUpdate) to also verify that any plaintext credential fields are cleared when an authRef wins: after computing preserved (from preserveVaultCredentialOnUpdate) add assertions that preserved.password === undefined and preserved.privateKeyPath === undefined for the first test (incoming vault auth replacing plaintext) and likewise assert preserved.password === undefined and preserved.privateKeyPath === undefined for the second test (incoming vault auth wins when both sides are vaulted).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c627a96-9fd4-4419-bf61-ff0192344815
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (59)
docs/VAULT_AND_SYNC_ARCHITECTURE.mdpackage.jsonsrc-tauri/.cargo/config.tomlsrc-tauri/.gitignoresrc-tauri/Cargo.tomlsrc-tauri/build.rssrc-tauri/src/commands.rssrc-tauri/src/lib.rssrc-tauri/src/ssh.rssrc-tauri/src/sync/commands.rssrc-tauri/src/sync/mod.rssrc-tauri/src/types.rssrc-tauri/src/vault/commands.rssrc-tauri/src/vault/crypto.rssrc-tauri/src/vault/error.rssrc-tauri/src/vault/migration.rssrc-tauri/src/vault/mod.rssrc-tauri/src/vault/schema.rssrc-tauri/src/vault/store.rssrc-tauri/src/vault/types.rssrc/App.tsxsrc/components/layout/CommandPalette.tsxsrc/components/layout/MainLayout.tsxsrc/components/layout/Sidebar.tsxsrc/components/layout/TabBar.tsxsrc/components/layout/sidebar/SidebarActionButton.tsxsrc/components/layout/sidebar/VaultNavSection.tsxsrc/components/layout/sidebar/vaultNavConfig.tssrc/components/layout/sidebar/vaultNavState.tssrc/components/modals/AddConnectionModal.tsxsrc/components/modals/useAutoVault.tssrc/components/modals/useConnectionForm.tssrc/components/settings/tabs/VaultTab.tsxsrc/components/settings/tabs/vaultFocus.tssrc/components/ui/Input.tsxsrc/components/ui/Modal.tsxsrc/components/vault/RecoveryKeyModal.tsxsrc/components/vault/VaultUnlockModal.tsxsrc/components/vault/VaultWorkspacePanel.tsxsrc/features/connections/application/tabService.tssrc/features/connections/domain/connectionConfig.tssrc/features/connections/domain/formTransforms.tssrc/features/connections/domain/merge.tssrc/features/connections/domain/types.tssrc/features/connections/domain/validation.tssrc/features/connections/infrastructure/connectionIpc.tssrc/store/connectionSlice.tssrc/store/sessionPersistence.tssrc/store/settingsSlice.tssrc/vault/ipc.tssrc/vault/profileTypes.tssrc/vault/syncIpc.tssrc/vault/useVaultStore.tstests/connectionDomain.test.mjstests/connectionTabService.test.mjstests/sessionPersistence.test.mjstests/vaultFocus.test.mjstests/vaultNavState.test.mjstsconfig.agent-tests.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
31-32: ⚡ Quick winAdd cargo caching to improve CI performance.
Consider adding Rust/Cargo caching between the Rust toolchain setup and test execution to avoid re-downloading and re-compiling dependencies on each run, significantly reducing CI runtime.
⚡ Suggested addition for cargo caching
Add this step after Rust setup:
- name: Cache cargo registry and build artifacts uses: Swatinem/rust-cache@v2 with: workspaces: src-tauri🤖 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 @.github/workflows/ci.yml around lines 31 - 32, Add a Cargo caching step right after the "Rust setup" step to cache the cargo registry and build artifacts; insert a new step using the Swatinem/rust-cache action (Swatinem/rust-cache@v2) and set the workspace option (e.g., workspaces: src-tauri) so dependencies and build outputs are reused between runs, reducing CI time..github/workflows/release.yml (1)
56-56: ⚡ Quick winError handling for missing
GOOGLE_CLIENT_IDis already in place.The
GOOGLE_CLIENT_IDenvironment variable is properly consumed bysrc-tauri/build.rsand already has clear error handling. During release builds, the code validates the client ID and panics with a descriptive error message if it's missing or invalid: "GOOGLE_CLIENT_ID is missing or placeholder. Set a real client ID for release builds."Consider documenting this requirement in the repository's README or CONTRIBUTING guide if not already present.
🤖 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 @.github/workflows/release.yml at line 56, The comment notes release builds already validate GOOGLE_CLIENT_ID in src-tauri/build.rs and panic with "GOOGLE_CLIENT_ID is missing or placeholder. Set a real client ID for release builds."; add a short note to the repository README or CONTRIBUTING explaining the requirement to set GOOGLE_CLIENT_ID (and where it is validated) so contributors and CI maintainers know to provide a real client ID for release builds and avoid the build-time panic.
🤖 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 `@docs/VAULT_AND_SYNC_ARCHITECTURE.md`:
- Around line 131-134: The Write/Delete methods on the provider trait (async fn
write, async fn delete) lack idempotency and conditional-write guarantees;
update the contract and request models (WriteRequest, DeleteRequest) to include
an optional idempotency_key and an optional if_match/expected_revision field,
document that providers must honor if_match semantics and return a normalized
conflict error code (conflict_precondition_failed) when preconditions fail, and
update the trait docs for list/read/write/delete to state that retries are
allowed but clients should supply idempotency_key and may use if_match to avoid
clobbering concurrent updates.
In `@src-tauri/src/vault/commands.rs`:
- Around line 123-127: The code currently derives item fingerprints from raw
secret values (item_meta_from_plaintext called in vault_item_list and similar
functions), which exposes deterministic SHA-256(secret); change the design so
fingerprints are not raw-secret hashes: update VaultService (and its
item_list/item_get/item_export flows referenced by vault_item_list and the
functions around lines 147-153 and 159-162) to expose an opaque per-item token
stored with the item (or a keyed MAC/HMAC using a server-only key) instead of
computing SHA-256(secret) on the plaintext; then change item_meta_from_plaintext
to read that stored token or compute an HMAC with the service-held key, and
update any code calling item_meta_from_plaintext to use the new metadata field;
ensure storage/schema is updated to persist the opaque token (or the keyed flag)
so existing and new items use non-deterministic, non-secret-derived
fingerprints.
- Around line 289-293: The validate_import_path function currently only
canonicalizes the input and may accept directories; after calling
std::fs::canonicalize(path) (inside validate_import_path) capture the resulting
PathBuf and assert it's a regular file with PathBuf::is_file(); if is_file() is
false return Err(VaultCommandError { code: "invalid_path".into(), message:
format!("Import file does not exist or is not accessible: {}",
path_buf.display()), }) so vault_import and callers get a deterministic
"invalid_path" VaultResult error; preserve the existing VaultResult return type
and error mapping for canonicalize failures and only add the post-canonicalize
regular-file check.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 31-32: Add a Cargo caching step right after the "Rust setup" step
to cache the cargo registry and build artifacts; insert a new step using the
Swatinem/rust-cache action (Swatinem/rust-cache@v2) and set the workspace option
(e.g., workspaces: src-tauri) so dependencies and build outputs are reused
between runs, reducing CI time.
In @.github/workflows/release.yml:
- Line 56: The comment notes release builds already validate GOOGLE_CLIENT_ID in
src-tauri/build.rs and panic with "GOOGLE_CLIENT_ID is missing or placeholder.
Set a real client ID for release builds."; add a short note to the repository
README or CONTRIBUTING explaining the requirement to set GOOGLE_CLIENT_ID (and
where it is validated) so contributors and CI maintainers know to provide a real
client ID for release builds and avoid the build-time panic.
🪄 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: 6c9fc3ec-9ed8-49db-a767-a2680d7ba4d7
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (62)
.github/workflows/ci.yml.github/workflows/release.ymlCHANGELOG.mddocs/VAULT_AND_SYNC_ARCHITECTURE.mdpackage.jsonsrc-tauri/.cargo/config.tomlsrc-tauri/.gitignoresrc-tauri/Cargo.tomlsrc-tauri/build.rssrc-tauri/src/commands.rssrc-tauri/src/lib.rssrc-tauri/src/ssh.rssrc-tauri/src/sync/commands.rssrc-tauri/src/sync/mod.rssrc-tauri/src/types.rssrc-tauri/src/vault/commands.rssrc-tauri/src/vault/crypto.rssrc-tauri/src/vault/error.rssrc-tauri/src/vault/migration.rssrc-tauri/src/vault/mod.rssrc-tauri/src/vault/schema.rssrc-tauri/src/vault/store.rssrc-tauri/src/vault/types.rssrc/App.tsxsrc/components/layout/CommandPalette.tsxsrc/components/layout/MainLayout.tsxsrc/components/layout/Sidebar.tsxsrc/components/layout/TabBar.tsxsrc/components/layout/sidebar/SidebarActionButton.tsxsrc/components/layout/sidebar/VaultNavSection.tsxsrc/components/layout/sidebar/vaultNavConfig.tssrc/components/layout/sidebar/vaultNavState.tssrc/components/modals/AddConnectionModal.tsxsrc/components/modals/useAutoVault.tssrc/components/modals/useConnectionForm.tssrc/components/settings/tabs/VaultTab.tsxsrc/components/settings/tabs/vaultFocus.tssrc/components/ui/Input.tsxsrc/components/ui/Modal.tsxsrc/components/vault/RecoveryKeyModal.tsxsrc/components/vault/VaultUnlockModal.tsxsrc/components/vault/VaultWorkspacePanel.tsxsrc/features/connections/application/tabService.tssrc/features/connections/domain/connectionConfig.tssrc/features/connections/domain/formTransforms.tssrc/features/connections/domain/merge.tssrc/features/connections/domain/types.tssrc/features/connections/domain/validation.tssrc/features/connections/infrastructure/connectionIpc.tssrc/store/connectionSlice.tssrc/store/sessionPersistence.tssrc/store/settingsSlice.tssrc/vault/ipc.tssrc/vault/profileTypes.tssrc/vault/syncIpc.tssrc/vault/useVaultStore.tstests/connectionDomain.test.mjstests/connectionTabService.test.mjstests/sessionPersistence.test.mjstests/vaultFocus.test.mjstests/vaultNavState.test.mjstsconfig.agent-tests.json
✅ Files skipped from review due to trivial changes (11)
- src-tauri/src/sync/mod.rs
- CHANGELOG.md
- src-tauri/.gitignore
- src-tauri/src/vault/mod.rs
- src-tauri/.cargo/config.toml
- src-tauri/src/vault/schema.rs
- tests/vaultFocus.test.mjs
- tsconfig.agent-tests.json
- src/components/layout/sidebar/vaultNavState.ts
- tests/sessionPersistence.test.mjs
- src/features/connections/domain/validation.ts
🚧 Files skipped from review as they are similar to previous changes (47)
- src/components/settings/tabs/vaultFocus.ts
- src/App.tsx
- src/features/connections/application/tabService.ts
- package.json
- src/components/ui/Input.tsx
- src/components/layout/sidebar/SidebarActionButton.tsx
- src/components/layout/sidebar/vaultNavConfig.ts
- src/features/connections/domain/connectionConfig.ts
- src-tauri/src/vault/error.rs
- src/components/layout/MainLayout.tsx
- tests/connectionTabService.test.mjs
- src/vault/profileTypes.ts
- src/features/connections/infrastructure/connectionIpc.ts
- src-tauri/src/types.rs
- src-tauri/src/vault/types.rs
- src/components/vault/VaultWorkspacePanel.tsx
- src/store/sessionPersistence.ts
- src-tauri/build.rs
- src/components/modals/useConnectionForm.ts
- src-tauri/src/ssh.rs
- src-tauri/Cargo.toml
- src/components/layout/CommandPalette.tsx
- src/features/connections/domain/types.ts
- src/store/settingsSlice.ts
- src/vault/ipc.ts
- src/components/ui/Modal.tsx
- src/features/connections/domain/merge.ts
- src-tauri/src/lib.rs
- src/features/connections/domain/formTransforms.ts
- tests/vaultNavState.test.mjs
- src/vault/useVaultStore.ts
- tests/connectionDomain.test.mjs
- src-tauri/src/vault/migration.rs
- src/vault/syncIpc.ts
- src/components/layout/sidebar/VaultNavSection.tsx
- src/components/vault/VaultUnlockModal.tsx
- src/components/modals/useAutoVault.ts
- src-tauri/src/vault/store.rs
- src/components/layout/TabBar.tsx
- src/components/layout/Sidebar.tsx
- src-tauri/src/vault/crypto.rs
- src/store/connectionSlice.ts
- src-tauri/src/commands.rs
- src-tauri/src/sync/commands.rs
- src/components/vault/RecoveryKeyModal.tsx
- src/components/modals/AddConnectionModal.tsx
- src/components/settings/tabs/VaultTab.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (3)
tests/connectionFormTransforms.test.mjs (1)
107-132: ⚡ Quick winAdd test coverage for optional
credential_id.The test correctly verifies vault auth when
credential_idis present. However, according to the AI summary, the backend'sVaultRefauth model supportscredential_idas optional. Consider adding a complementary test case to verify the payload structure whencredential_idis absent or undefined inauthRef.🧪 Suggested additional test case
runTest('buildConnectionTestPayload handles vault auth without credential_id', () => { const formData = { id: 'main', name: 'main', host: '192.168.0.10', username: 'ec2-user', port: 22, authRef: { vaultId: 'vault-1', itemId: 'item-1', itemKind: 'ssh-private-key', purpose: 'ssh-auth', // credentialId intentionally omitted }, }; const payload = buildConnectionTestPayload({ formData, authMethod: 'vault', connections: [], }); assert.equal(payload.auth_method.type, 'VaultRef'); assert.equal(payload.auth_method.item_id, 'item-1'); assert.equal(payload.auth_method.credential_id, undefined); });🤖 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 `@tests/connectionFormTransforms.test.mjs` around lines 107 - 132, Add a complementary unit test for buildConnectionTestPayload that covers the case when formData.authRef.credentialId is omitted: create a test (e.g., "buildConnectionTestPayload handles vault auth without credential_id") that constructs formData with authRef missing credentialId, calls buildConnectionTestPayload with authMethod: 'vault', and asserts payload.auth_method.type === 'VaultRef', payload.auth_method.item_id === the expected item id, and that payload.auth_method.credential_id is either undefined or absent as the backend model expects; reference buildConnectionTestPayload and the payload.auth_method fields when adding the assertions.src-tauri/src/vault/store.rs (1)
359-372: ⚖️ Poor tradeoff
item_get_by_logical_iddecrypts the entire vault to resolve one record.Each call materializes a full
Vec<PlaintextRecord>containing every secret in the vault, just to scan for alogical_idmatch. This scales linearly in CPU (per-record HKDF + AEAD) and, more importantly, holds plaintext copies of all unrelated secrets in memory until the caller drops the unused records. For vaults with even a few hundred items this is wasteful, and it widens the in-memory exposure window for credentials this caller never asked for.A logical-id → item-id index in
RECORDS(or a separate redb table) would let this path read & decrypt only one record. Deferable, but worth budgeting before this is on hot paths like resolve-on-connect.🤖 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-tauri/src/vault/store.rs` around lines 359 - 372, item_get_by_logical_id currently calls item_list() which decrypts every PlaintextRecord; change it to look up the single matching record id and decrypt only that record. Implement or use a logical-id → record-id index (RECORDS index or a new redb table) to resolve logical_id to the underlying record identifier, then fetch that single encrypted record and run the existing per-record decrypt routine (avoid calling item_list or iterating all records). Update item_get_by_logical_id to trim/validate logical_id, query the index for the record id, load the encrypted record by id, decrypt it with the same decrypt helper used elsewhere, and return the PlaintextRecord; if the index lookup or fetch/decrypt fails return VaultError::RecordNotFound (or propagate the specific error) to preserve current semantics.src-tauri/src/vault/crypto.rs (1)
144-146: 💤 Low valueHMAC keying failure is mapped to
VaultCryptoError::HkdfExpand.
<SimpleHmac<Sha256> as HmacKeyInit>::new_from_slice(...)returns an HMACInvalidLengtherror, not an HKDF expand error. ReusingHkdfExpandhere makes diagnostics confusing if this ever fires. Either add a dedicatedHmacKeyInitvariant or reuse a more generic one (e.g.InvalidKeyLength).🛠️ Sketch
pub enum VaultCryptoError { Argon2(argon2::Error), Aead, HkdfExpand, + HmacKeyInit, InvalidSaltLength, } @@ - let mut mac = <SimpleHmac<Sha256> as HmacKeyInit>::new_from_slice(fingerprint_key.as_bytes()) - .map_err(|_| VaultCryptoError::HkdfExpand)?; + let mut mac = <SimpleHmac<Sha256> as HmacKeyInit>::new_from_slice(fingerprint_key.as_bytes()) + .map_err(|_| VaultCryptoError::HmacKeyInit)?;🤖 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-tauri/src/vault/crypto.rs` around lines 144 - 146, The error mapping for HMAC key initialization is incorrect: change the map_err on the SimpleHmac<Sha256>::new_from_slice call (the code that uses HmacKeyInit) so it does not map Hmac InvalidLength errors to VaultCryptoError::HkdfExpand; instead add or reuse a more appropriate VaultCryptoError variant (e.g. VaultCryptoError::InvalidHmacKeyLength or VaultCryptoError::InvalidKeyLength) and map the HmacKeyInit failure to that new/appropriate variant, updating any uses of derive_record_key / fingerprint_key error handling accordingly.
🤖 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 `@scripts/reset-vault-test-data.ps1`:
- Around line 194-205: The PowerShell transform inside the
Rewrite-ConnectionsJson call currently only nulls
authRef/privateKeyPath/password when those properties exist; change it to mirror
the bash behavior by unconditionally setting $connection.authRef = $null,
$connection.privateKeyPath = $null, and $connection.password = $null inside the
param($connection) scriptblock (i.e., remove the if checks) so the serialized
JSON includes explicit nulls for all connections consistently with the bash
reset script.
- Line 116: The current line uses Set-Content with -Encoding UTF8 which produces
a BOM on Windows PowerShell 5.1 and breaks JSON parsing; replace the Set-Content
call that writes $json (via ConvertTo-Json -Depth 100) to $ConnectionsFile with
a BOM-less UTF‑8 write using the .NET API (e.g. call
[System.IO.File]::WriteAllText with the ConvertTo-Json output and a
System.Text.UTF8Encoding instance constructed with BOM disabled) instead of
Set-Content so the file is written without an EF BB BF prefix.
In `@scripts/reset-vault-test-data.sh`:
- Around line 73-85: Replace the unversioned python invocation used in the two
heredoc blocks that populate data_path (the block assigning data_path="$(python
- <<'PY' "$native_settings_path" ... PY )") and the similar block at lines
185–197 with python3; in other words, change the command token "python" to
"python3" in both heredoc invocations so the script uses the modern interpreter
available on macOS/Debian/Ubuntu systems.
In `@src-tauri/src/commands.rs`:
- Around line 718-759: persist_relinked_vault_refs performs an unsafe
read-modify-write of connections.json and must acquire the same shared mutation
lock used by other writers (e.g., connections_save and repair_connection_refs)
before reading and hold it until after the write completes; update
persist_relinked_vault_refs to obtain the global connections.json mutation lock
(reuse the existing lock symbol used elsewhere, e.g., CONNECTIONS_MUTATION_LOCK
or the app-managed Mutex/RwLock), perform the read/modify logic while holding
the lock, and only then serialize/write the file and release the lock so
concurrent writers are serialized and last-writer-wins races are prevented.
In `@src-tauri/src/vault/commands.rs`:
- Around line 388-423: The repair loop over saved.connections is querying every
auth_ref against the currently-open vault; add a guard that compares
auth_ref.vault_id to the active vault's id before calling vault.item_get or
vault.item_get_by_logical_id (e.g., compare auth_ref.vault_id to the active
vault identifier available on the vault object such as
vault.id()/vault.vault_id), and if they differ either skip the ref (increment
skipped_missing_items or a new counter) or mark it as out-of-scope and continue;
apply this check at the start of the loop (around where auth_ref is bound) so
VaultService::record_logical_id, vault.item_get and vault.item_get_by_logical_id
are only called for refs belonging to the active vault.
In `@src-tauri/src/vault/secure_to_vault.rs`:
- Around line 168-176: The current code always overwrites the pre-migration
backup (written to backup_path) which loses the original plaintext snapshot;
change the logic in the block that reads original_json and writes backups so
that you only create the first pre-migration backup once: check
backup_path.exists() and if it does not exist write original_json to
backup_path, otherwise skip writing; keep the existing behavior for
legacy_backup_path (it already checks !legacy_backup_path.exists()) or
optionally add the same existence check there; update references in this
function (original_json, backup_path, legacy_backup_path, connections_path)
accordingly so the first snapshot is preserved.
In `@src-tauri/src/vault/store.rs`:
- Around line 200-219: The decrypt_record call in unlock and
unlock_with_recovery_key returns a Vec<u8> (vek_bytes) containing plaintext VEK
that is freed without zeroization and the stack array vek_arr is not zeroed in
the passphrase path; to fix, wrap or convert the decrypt_record output into a
zeroizing container (e.g., Zeroizing<Vec<u8>> or call vek_bytes.zeroize() before
drop) so the heap buffer is wiped, and after converting vek_bytes to the
fixed-size array used by SecretKey::from_bytes (vek_arr) call vek_arr.zeroize()
(or use a Zeroizing<[u8;32]>) immediately after constructing SecretKey; apply
the same zeroization steps in both unlock and unlock_with_recovery_key to ensure
no plaintext VEK remains on heap or stack.
In `@src/components/settings/tabs/vault/AddCredentialModal.tsx`:
- Around line 85-96: The "Private Key" label in AddCredentialModal isn't
associated with the textarea, so update the label/textarea pair to be
programmatically linked: add a unique id (e.g., "privateKey" or generated ID) to
the textarea element (the one with value={secret} and onChange calling
onSecretChange) and set the label's htmlFor to that same id; alternatively, add
an appropriate aria-label/aria-labelledby to the textarea if you prefer not to
use htmlFor — ensure the change is applied within AddCredentialModal around the
textarea and label to make screen readers announce the field correctly.
- Around line 49-59: The kind-switch buttons in AddCredentialModal.tsx (the
mapping over ['ssh-private-key','ssh-password']) still call onKindChange while
isCreating is true, allowing the UI to show a different kind than the in-flight
payload; update the mapped button elements to be disabled when isCreating (e.g.,
add disabled={isCreating} and prevent the onClick when isCreating) and adjust
the className/aria-disabled state so the buttons render visually disabled while
inputs remain locked, ensuring onKindChange cannot mutate form state during
submit.
In `@src/components/settings/tabs/vault/RotateCredentialModal.tsx`:
- Around line 65-76: The label in RotateCredentialModal isn't programmatically
associated with the textarea, so update the label/textarea pair: add a unique id
(e.g. "new-private-key") to the textarea that holds `value={secret}` and
`onChange={onSecretChange}`, and set the label's htmlFor to that id (or
alternatively add an aria-label to the textarea). This ties the visible label
"New Private Key" to the textarea for assistive tech while leaving `isLoading`,
`rows`, and existing classes intact.
In `@src/components/settings/tabs/vault/VaultItemsPanel.tsx`:
- Around line 95-96: The action buttons are hidden by default via the class
"flex items-center gap-1 opacity-0 group-hover:opacity-100
focus-within:opacity-100 transition-opacity", which makes them inaccessible on
touch/coarse-pointer devices; update that element in VaultItemsPanel (the div
wrapping the action controls) to be visible by default on small/coarse screens
and only use the hover behavior on larger/pointer devices — e.g. replace the
opacity classes so the controls use responsive classes like "flex items-center
gap-1 md:opacity-0 md:group-hover:opacity-100 focus-within:opacity-100
transition-opacity" (or add an equivalent `@media` (pointer:coarse) rule to force
opacity:1) so actions are discoverable on touch devices while preserving
hover-only reveal on pointer-based viewports.
- Around line 43-49: The buttons in VaultItemsPanel (e.g., the button using
onDeduplicate and the other native <button> around line 114) are missing an
explicit type and will default to type="submit"; update those native <button>
elements in VaultItemsPanel.tsx to include type="button" to prevent accidental
form submission (locate the button with onDeduplicate and the other button(s) in
the file and add type="button" to each).
- Around line 73-79: The search input in VaultItemsPanel (the <input> bound to
value={itemSearch} and onChange calling onItemSearchChange) lacks an explicit
accessible name; add one by providing either a visible <label> linked via
htmlFor/id or adding an aria-label (e.g., aria-label="Search vault items") on
the input, keep the placeholder as-is, and ensure the id/label text or
aria-label clearly describes the field for screen readers.
In `@src/components/settings/tabs/vault/VaultSyncCard.tsx`:
- Around line 114-118: The conditional rendering in VaultSyncCard currently uses
a truthy check on googleSync.lastSync which hides valid timestamps like 0;
change the condition to an explicit nullish check (e.g., googleSync.lastSync !=
null) so the span is rendered when lastSync is 0 but not for null/undefined,
updating the check around the element that formats new Date(googleSync.lastSync
* 1000).toLocaleString().
In `@src/features/connections/domain/credentialAssignments.ts`:
- Around line 28-56: The equality check in syncCredentialAssignments incorrectly
treats undefined === undefined as a match and can clear unrelated authRef;
update the usesCredential logic in syncCredentialAssignments to first check a
defined credentialId match (connection.authRef?.credentialId !== undefined &&
connection.authRef.credentialId === authRef.credentialId) and only fall back to
itemId comparison when credentialId is absent on one side (e.g., if credentialId
is undefined on either side, use a safe itemId match that also requires defined
strings); ensure the branch that assigns normalizeCredentialRef(authRef) remains
unchanged and add/adjust a test in tests/connectionDomain.test.mjs covering
legacy itemId-only and credentialId-only cases so the invariant is preserved.
---
Nitpick comments:
In `@src-tauri/src/vault/crypto.rs`:
- Around line 144-146: The error mapping for HMAC key initialization is
incorrect: change the map_err on the SimpleHmac<Sha256>::new_from_slice call
(the code that uses HmacKeyInit) so it does not map Hmac InvalidLength errors to
VaultCryptoError::HkdfExpand; instead add or reuse a more appropriate
VaultCryptoError variant (e.g. VaultCryptoError::InvalidHmacKeyLength or
VaultCryptoError::InvalidKeyLength) and map the HmacKeyInit failure to that
new/appropriate variant, updating any uses of derive_record_key /
fingerprint_key error handling accordingly.
In `@src-tauri/src/vault/store.rs`:
- Around line 359-372: item_get_by_logical_id currently calls item_list() which
decrypts every PlaintextRecord; change it to look up the single matching record
id and decrypt only that record. Implement or use a logical-id → record-id index
(RECORDS index or a new redb table) to resolve logical_id to the underlying
record identifier, then fetch that single encrypted record and run the existing
per-record decrypt routine (avoid calling item_list or iterating all records).
Update item_get_by_logical_id to trim/validate logical_id, query the index for
the record id, load the encrypted record by id, decrypt it with the same decrypt
helper used elsewhere, and return the PlaintextRecord; if the index lookup or
fetch/decrypt fails return VaultError::RecordNotFound (or propagate the specific
error) to preserve current semantics.
In `@tests/connectionFormTransforms.test.mjs`:
- Around line 107-132: Add a complementary unit test for
buildConnectionTestPayload that covers the case when
formData.authRef.credentialId is omitted: create a test (e.g.,
"buildConnectionTestPayload handles vault auth without credential_id") that
constructs formData with authRef missing credentialId, calls
buildConnectionTestPayload with authMethod: 'vault', and asserts
payload.auth_method.type === 'VaultRef', payload.auth_method.item_id === the
expected item id, and that payload.auth_method.credential_id is either undefined
or absent as the backend model expects; reference buildConnectionTestPayload and
the payload.auth_method fields when adding the assertions.
🪄 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: 0fc71899-8b6a-4504-86cc-667de199a70b
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
.github/workflows/ci.ymlCHANGELOG.mdREADME.mddocs/VAULT_AND_SYNC_ARCHITECTURE.mddocs/VAULT_CREDENTIAL_IDENTITY_MODEL.mdscripts/reset-vault-test-data.ps1scripts/reset-vault-test-data.shsrc-tauri/Cargo.tomlsrc-tauri/build.rssrc-tauri/src/commands.rssrc-tauri/src/lib.rssrc-tauri/src/ssh.rssrc-tauri/src/types.rssrc-tauri/src/vault/commands.rssrc-tauri/src/vault/crypto.rssrc-tauri/src/vault/mod.rssrc-tauri/src/vault/secure_to_vault.rssrc-tauri/src/vault/store.rssrc-tauri/src/vault/types.rssrc/components/modals/AddConnectionModal.tsxsrc/components/modals/useAutoVault.tssrc/components/settings/tabs/VaultTab.tsxsrc/components/settings/tabs/vault/AddCredentialModal.tsxsrc/components/settings/tabs/vault/ManageAssignmentsModal.tsxsrc/components/settings/tabs/vault/RotateCredentialModal.tsxsrc/components/settings/tabs/vault/VaultItemsPanel.tsxsrc/components/settings/tabs/vault/VaultStatusCard.tsxsrc/components/settings/tabs/vault/VaultSyncCard.tsxsrc/features/connections/domain/connectionConfig.tssrc/features/connections/domain/credentialAssignments.tssrc/features/connections/domain/formTransforms.tssrc/features/connections/domain/index.tssrc/features/connections/domain/types.tssrc/features/connections/infrastructure/connectionIpc.tssrc/vault/ipc.tstests/connectionDomain.test.mjstests/connectionFormTransforms.test.mjs
✅ Files skipped from review due to trivial changes (4)
- src/features/connections/domain/index.ts
- README.md
- CHANGELOG.md
- src/features/connections/domain/types.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- src-tauri/Cargo.toml
- src-tauri/src/vault/mod.rs
- src-tauri/src/vault/types.rs
- src/features/connections/domain/connectionConfig.ts
- .github/workflows/ci.yml
- src/features/connections/infrastructure/connectionIpc.ts
- src-tauri/src/types.rs
- src/components/modals/useAutoVault.ts
- src-tauri/build.rs
- src/features/connections/domain/formTransforms.ts
- src-tauri/src/lib.rs
- src/components/modals/AddConnectionModal.tsx
- src-tauri/src/ssh.rs
- src/components/settings/tabs/VaultTab.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src-tauri/src/commands.rs (1)
805-848: 💤 Low value
ssh_test_connectionhas a non-obvious disk-write side effect.A "test connection" command now silently rewrites
connections.jsonwheneverresolve_vault_refsrelinks aVaultRefbycredential_id(line 813 →persist_relinked_vault_refs). That is probably the intended behavior (keep stored references valid even if the user only tests), but it is surprising for a test path and means any concurrency/atomicity issue in the persist step (see prior comment) can also be triggered from a test action. Worth either: (a) documenting this behavior on the command, or (b) gating the persist behind a flag so test paths don't write to disk by default.🤖 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-tauri/src/commands.rs` around lines 805 - 848, The ssh_test_connection command currently calls resolve_vault_refs and unconditionally calls persist_relinked_vault_refs, causing a surprising disk write during a test; change the function signature of ssh_test_connection to accept an optional flag (e.g., persist_relinked: bool or dry_run: bool) and only call persist_relinked_vault_refs when that flag is true, leaving resolve_vault_refs unchanged so relinking can still be validated without writing; update any call sites to pass true when an intentional update is desired (or document the new flag behavior in the function comment) and ensure the new flag is propagated through any callers that expect the original atomic persist 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 `@src-tauri/src/commands.rs`:
- Around line 1093-1122: The ssh_disconnect_vault_backed function currently uses
the `?` on `state.pty_manager.close_by_connection(id)` which aborts the loop on
the first failure and prevents `stop_tunnels_for_connections` and
`state.connections` pruning; change the loop to handle each `id` individually by
collecting per-id errors (e.g., a Vec<(String, String)> or Vec<String> messages)
while continuing to attempt closing all PTYs, then always call
`stop_tunnels_for_connections(&app, &state, &ids).await` and remove all ids from
`state.connections` regardless of per-id failures, and finally return Ok(ids) if
no errors or an aggregated Err with joined error messages if any failures
occurred; update references to `pty_manager.close_by_connection`,
`stop_tunnels_for_connections`, and `state.connections` accordingly.
- Around line 720-767: persist_relinked_vault_refs currently uses std::fs::write
and must be changed to use the existing write_atomic_file helper; also in
ssh_migrate_all_keys acquire CONNECTIONS_MUTATION_LOCK before reading and hold
it until after file.sync_all() to prevent races with
connections_save/persist_relinked_vault_refs/repair_connection_refs; update
connections_save to use write_atomic_file as well to ensure atomic writes;
modify ssh_disconnect_vault_backed so it does not early-return on the first PTY
close error—collect all close errors while attempting to close every PTY/tunnel
and return a combined error (or the first) after the loop; and remove (or
clearly document) the side-effect in ssh_test_connection so it does not call
persist_relinked_vault_refs (or explicitly skip persistence) because test
commands must not write to disk.
---
Nitpick comments:
In `@src-tauri/src/commands.rs`:
- Around line 805-848: The ssh_test_connection command currently calls
resolve_vault_refs and unconditionally calls persist_relinked_vault_refs,
causing a surprising disk write during a test; change the function signature of
ssh_test_connection to accept an optional flag (e.g., persist_relinked: bool or
dry_run: bool) and only call persist_relinked_vault_refs when that flag is true,
leaving resolve_vault_refs unchanged so relinking can still be validated without
writing; update any call sites to pass true when an intentional update is
desired (or document the new flag behavior in the function comment) and ensure
the new flag is propagated through any callers that expect the original atomic
persist 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: 4d935708-7af1-4acf-bf35-3fa66b323dff
📒 Files selected for processing (15)
scripts/reset-vault-test-data.ps1scripts/reset-vault-test-data.shsrc-tauri/src/commands.rssrc-tauri/src/vault/commands.rssrc-tauri/src/vault/crypto.rssrc-tauri/src/vault/schema.rssrc-tauri/src/vault/secure_to_vault.rssrc-tauri/src/vault/store.rssrc/components/settings/tabs/vault/AddCredentialModal.tsxsrc/components/settings/tabs/vault/RotateCredentialModal.tsxsrc/components/settings/tabs/vault/VaultItemsPanel.tsxsrc/components/settings/tabs/vault/VaultSyncCard.tsxsrc/features/connections/domain/credentialAssignments.tstests/connectionDomain.test.mjstests/connectionFormTransforms.test.mjs
✅ Files skipped from review due to trivial changes (1)
- tests/connectionFormTransforms.test.mjs
🚧 Files skipped from review as they are similar to previous changes (12)
- src/components/settings/tabs/vault/RotateCredentialModal.tsx
- scripts/reset-vault-test-data.sh
- src/components/settings/tabs/vault/AddCredentialModal.tsx
- src-tauri/src/vault/schema.rs
- src/components/settings/tabs/vault/VaultItemsPanel.tsx
- src/features/connections/domain/credentialAssignments.ts
- tests/connectionDomain.test.mjs
- src-tauri/src/vault/crypto.rs
- src/components/settings/tabs/vault/VaultSyncCard.tsx
- src-tauri/src/vault/secure_to_vault.rs
- src-tauri/src/vault/store.rs
- src-tauri/src/vault/commands.rs
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
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 by CodeRabbit
New Features
Documentation