feat(vault): add credential history and harden vault workflows#71
Open
gajendraxdev wants to merge 8 commits into
Open
feat(vault): add credential history and harden vault workflows#71gajendraxdev wants to merge 8 commits into
gajendraxdev wants to merge 8 commits into
Conversation
Add credential revision history and restore support across the vault backend, IPC layer, and vault workspace UI. Modularize vault workspace actions into focused hooks, tighten credential modal validation, improve restore/error handling, and preserve vault credential references across sync/import flows. Update docs, changelog, CI checks, regression tests, and small UI/build-warning polish.
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ 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:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a provider-agnostic sync system with Google Drive adapter and per-credential encrypted records, vault revision history/restore, extended IPC and React UI for sync and history, improved unlock UX, tighter build checks, updated docs/tests, and reset scripts. ChangesProvider sync, vault revisions, and UI integration
Sequence Diagram(s)sequenceDiagram
participant ComponentA
participant ComponentB
ComponentA->>ComponentB: observable interaction
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/vault/syncIpc.ts (1)
53-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't discard the
sync_connectpayload.
sync_connectstill returnsSyncProviderStatus, including the connected account email, but this path ignores it and immediately refetches viasync_status().sync_status()currently returnsemail: Nonefor Google, so the UI loses the identity it just obtained after a successful 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/vault/syncIpc.ts` around lines 53 - 63, The connect method is throwing away the payload returned by invoke('sync_connect') which contains the SyncProviderStatus (including Google email); change connect in syncIpc.ts to capture the invoke result (await invoke<SyncProviderStatus>('sync_connect', { provider })) and use that as latestStatus (and pass it to notifySyncStatusChanged and return it) instead of immediately calling syncIpc.status(provider); if you still want a fallback, only call syncIpc.status(provider) when the invoke response is missing required fields and use fallbackStatus(provider, { connected: true }, error) on errors as currently done.
🧹 Nitpick comments (3)
src/components/settings/tabs/vault/hooks/useRotateCredentialModal.ts (2)
44-48: ⚡ Quick winSimplify error message extraction.
The error message extraction uses an overly complex type assertion pattern that can be simplified.
♻️ Suggested simplification
} catch (error: unknown) { console.warn('[Vault] Failed to load item for rotation:', error); - const msg = String((error as { message?: unknown } | null | undefined)?.message ?? error); + const msg = error instanceof Error ? error.message : String(error); showToast('error', `Failed to load vault item notes: ${msg}`); }This pattern is clearer and handles the common case of
Errorinstances more idiomatically.🤖 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/vault/hooks/useRotateCredentialModal.ts` around lines 44 - 48, In the catch block inside useRotateCredentialModal, replace the complex type assertion used to build the error text with a simpler, idiomatic check (e.g., use instanceof Error to get error.message, otherwise String(error)); update the const msg assignment and leave the existing console.warn and showToast calls untouched so showToast('error', `Failed to load vault item notes: ${msg}`) receives the simplified message.
103-108: ⚡ Quick winSimplify error message extraction (duplicate pattern).
Same complex type assertion pattern as in the
open()method.♻️ Suggested simplification
} catch (e: unknown) { - const msg = - e && typeof e === 'object' && 'message' in e - ? String((e as { message: unknown }).message) - : String(e); + const msg = e instanceof Error ? e.message : String(e); showToast('error', `Failed to rotate credential: ${msg}`); } 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/components/settings/tabs/vault/hooks/useRotateCredentialModal.ts` around lines 103 - 108, The catch block in useRotateCredentialModal repeats the complex error-extraction logic (see the local variable msg and the showToast call), duplicating the same pattern used in open(); refactor by extracting that logic into a single helper (e.g., extractErrorMessage) either in this file or imported from a shared util, then replace the inline type-assertion block in the rotate handler and the open() method with a call to extractErrorMessage(error) and pass its result into showToast('error', `Failed to rotate credential: ${msg}`) (or the corresponding open() toast) to remove duplication and centralize error parsing.src/components/settings/tabs/vault/hooks/useAddCredentialModal.ts (1)
64-82: ⚡ Quick winConsider simplifying error message extraction.
The error message extraction pattern can be simplified for better readability.
♻️ Suggested simplification
} catch (e: unknown) { - const msg = String((e as { message?: unknown } | null | undefined)?.message ?? e); + const msg = e instanceof Error ? e.message : String(e); showToast('error', `Failed to add credential: ${msg}`); } finally {This pattern is more idiomatic and handles the common
Errorinstance case clearly.🤖 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/vault/hooks/useAddCredentialModal.ts` around lines 64 - 82, The current catch block in useAddCredentialModal's create flow extracts the error message via a verbose cast; simplify it by replacing the msg computation in the catch of the async create handler with an idiomatic check like using "e instanceof Error ? e.message : String(e)" so the showToast('error', ...) receives a clear string; keep the surrounding setIsCreating, vaultIpc.itemCreate, onCreated, reset, and setIsOpen logic unchanged.
🤖 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/vault/secure_to_vault.rs`:
- Around line 133-145: The secret format varies based on conn.password which
breaks deduplication because derive_secret_fingerprint currently fingerprints
the whole secret string; fix by normalizing secrets and fingerprinting: always
serialize secrets as JSON with explicit "key" and "passphrase" fields when
creating secret in the PreparedSecureItem (change the secret construction around
key_content and conn.password in secure_to_vault.rs), update
derive_secret_fingerprint (crypto.rs) to compute the fingerprint from the
canonical "key" value (or parse legacy raw secrets into the canonical JSON
shape), and update the deduplication lookup logic that uses the fingerprint to
accept both legacy raw and new JSON forms by normalizing stored items before
comparison; touch the secret creation site, derive_secret_fingerprint function,
and deduplication lookup to implement this consistent normalization/migration
path.
In `@src/components/dashboard/WelcomeScreen.tsx`:
- Line 324: The Tailwind class bg-size-[18px_18px] is non-standard; in the
WelcomeScreen component update the className on the absolute background element
to use Tailwind arbitrary property syntax for CSS properties (replace
bg-size-[18px_18px] with [background-size:18px_18px]) so the background-size is
applied correctly; keep the rest of the classes (absolute inset-0 -z-10
bg-app-bg
bg-[radial-gradient(circle_at_center,var(--color-app-border)_1px,transparent_1px)]
opacity-40) unchanged.
In `@src/components/settings/tabs/vault/hooks/useAssignCredentialModal.ts`:
- Around line 71-76: The modal never closes because close() returns early while
isAssigning is true; either clear isAssigning before calling close or let close
bypass the guard when intended. Edit submit() (and the other call sites at lines
~122-124) so they call setIsAssigning(false) before invoking close(), or modify
close(itemId?, {force}) / remove the early return so close() will still clear
state (setItemId(null), setSearch(''), setSelectedConnectionIds([])) when a
successful submit finishes; reference the close, submit, isAssigning,
setIsAssigning, setItemId and setSelectedConnectionIds symbols to locate and
update the logic.
In `@src/components/settings/tabs/vault/hooks/useRotateCredentialModal.ts`:
- Line 66: The code sets baseSecret using the untrimmed secret for non-SSH keys
causing saved credentials to include whitespace; update the assignment in
useRotateCredentialModal (baseSecret) to use trimmedSecret for all kinds
(instead of secret) so secretToSave uses the already-trimmed value; ensure this
change affects the logic where secretToSave is derived (including the branch
that checks for passphrase) so non-SSH credentials are trimmed before saving.
In `@src/components/settings/tabs/vault/hooks/useVaultPanelActions.ts`:
- Around line 96-98: The mutating actions call only onRefresh(), which updates
vault status but not the item list; update the code paths (including the block
around useVaultPanelActions.ts lines where onLoadConnections(), onRefresh(),
loadSecurePreview() are called and the similar block at the other location
referenced) to call the item-reload routine after item mutations—either call
onLoadConnections() (or the specific item reload function used elsewhere) after
onRefresh(), or replace onRefresh() with a call sequence that ensures both
status and items are reloaded so the UI reflects secure/delete changes
immediately.
- Around line 461-473: The code marks a vault as backfilled before the IPC call
completes, preventing retries if backfill fails; move the
backfilledVaultIdsRef.current.add(vaultId) call so it only runs after
vaultIpc.backfillConnectionRefs() succeeds (e.g., after checking result.updated
> 0 or after a successful try block), and keep the early-return check (if
(backfilledVaultIdsRef.current.has(vaultId)) return;) intact; update the logic
in useVaultPanelActions around backfilledVaultIdsRef,
vaultIpc.backfillConnectionRefs, onLoadConnections, and the showToast call so
the vault ID is only added when the backfill completes without throwing.
- Around line 133-152: The toast incorrectly claims all vault-backed sessions
were disconnected even when some onDisconnectConnection calls failed; use the
Promise.allSettled results from disconnectResults to compute how many succeeded
(count result.status === 'fulfilled' for entries corresponding to
vaultBackedIds) and use that count in the showToast message instead of
vaultBackedConnectionIds.size, and if any were rejected, add a separate
warning/error toast (or include in the message) indicating partial failure and
referencing the failed vaultBackedIds by index from disconnectResults so the
user knows some sessions remain connected; keep the existing await onLocked()
call sequence (call onLocked before showing toasts) and update messages
accordingly.
In `@src/components/vault/VaultUnlockModal.tsx`:
- Around line 8-9: The frontend-only 12-character passphrase check
(PASSPHRASE_MIN_LENGTH in VaultUnlockModal.tsx) must be mirrored in the backend:
update VaultService::initialize() to import/use the same PASSPHRASE_MIN_LENGTH
constant and validate the provided passphrase length, returning/throwing a clear
error (e.g., InvalidPassphraseLength) when passphrase.length <
PASSPHRASE_MIN_LENGTH so IPC callers cannot bypass the rule; update any
callers/tests to handle the new error path and ensure the error propagates back
over IPC for user-facing display.
In `@src/lib/tauri-ipc.ts`:
- Line 174: The new channel mapping 'ssh:disconnectVaultBacked' ->
'ssh_disconnect_vault_backed' must be treated as a no-arg command so it
serializes to an empty payload instead of { args: [] }; update the payload
builder (where channel keys are translated to payloads) to add an explicit
branch for 'ssh_disconnect_vault_backed' that returns {} (empty object) rather
than the default args array handling so command deserialization succeeds.
In `@src/store/connectionSlice.ts`:
- Around line 694-696: The current assignment lets non-vault tabs keep a valid
vaultProfileId; change the logic so you first check s.tabType === 'vault' and
only then validate/or default the id: for example set vaultProfileId to
undefined for any non-'vault' tab, and for 'vault' tabs use
isVaultProfileId(s.vaultProfileId) ? s.vaultProfileId :
DEFAULT_VAULT_PROFILE_ID; update the expression containing vaultProfileId,
isVaultProfileId, s.tabType and DEFAULT_VAULT_PROFILE_ID accordingly.
---
Outside diff comments:
In `@src/vault/syncIpc.ts`:
- Around line 53-63: The connect method is throwing away the payload returned by
invoke('sync_connect') which contains the SyncProviderStatus (including Google
email); change connect in syncIpc.ts to capture the invoke result (await
invoke<SyncProviderStatus>('sync_connect', { provider })) and use that as
latestStatus (and pass it to notifySyncStatusChanged and return it) instead of
immediately calling syncIpc.status(provider); if you still want a fallback, only
call syncIpc.status(provider) when the invoke response is missing required
fields and use fallbackStatus(provider, { connected: true }, error) on errors as
currently done.
---
Nitpick comments:
In `@src/components/settings/tabs/vault/hooks/useAddCredentialModal.ts`:
- Around line 64-82: The current catch block in useAddCredentialModal's create
flow extracts the error message via a verbose cast; simplify it by replacing the
msg computation in the catch of the async create handler with an idiomatic check
like using "e instanceof Error ? e.message : String(e)" so the
showToast('error', ...) receives a clear string; keep the surrounding
setIsCreating, vaultIpc.itemCreate, onCreated, reset, and setIsOpen logic
unchanged.
In `@src/components/settings/tabs/vault/hooks/useRotateCredentialModal.ts`:
- Around line 44-48: In the catch block inside useRotateCredentialModal, replace
the complex type assertion used to build the error text with a simpler,
idiomatic check (e.g., use instanceof Error to get error.message, otherwise
String(error)); update the const msg assignment and leave the existing
console.warn and showToast calls untouched so showToast('error', `Failed to load
vault item notes: ${msg}`) receives the simplified message.
- Around line 103-108: The catch block in useRotateCredentialModal repeats the
complex error-extraction logic (see the local variable msg and the showToast
call), duplicating the same pattern used in open(); refactor by extracting that
logic into a single helper (e.g., extractErrorMessage) either in this file or
imported from a shared util, then replace the inline type-assertion block in the
rotate handler and the open() method with a call to extractErrorMessage(error)
and pass its result into showToast('error', `Failed to rotate credential:
${msg}`) (or the corresponding open() toast) to remove duplication and
centralize error parsing.
🪄 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: e53d9265-a4f6-4eac-a4ba-78937fcb91ed
📒 Files selected for processing (38)
.github/workflows/ci.ymlCHANGELOG.mddocs/VAULT_AND_SYNC_ARCHITECTURE.mdsrc-tauri/build.rssrc-tauri/src/lib.rssrc-tauri/src/sync/commands.rssrc-tauri/src/vault/commands.rssrc-tauri/src/vault/schema.rssrc-tauri/src/vault/secure_to_vault.rssrc-tauri/src/vault/store.rssrc-tauri/src/vault/types.rssrc/components/dashboard/WelcomeScreen.tsxsrc/components/layout/Sidebar.tsxsrc/components/layout/sidebar/VaultNavSection.tsxsrc/components/modals/AddTunnelModal.tsxsrc/components/modals/useAutoVault.tssrc/components/settings/tabs/VaultTab.tsxsrc/components/settings/tabs/vault/CredentialHistoryModal.tsxsrc/components/settings/tabs/vault/VaultItemsPanel.tsxsrc/components/settings/tabs/vault/VaultStatusCard.tsxsrc/components/settings/tabs/vault/hooks/useAddCredentialModal.tssrc/components/settings/tabs/vault/hooks/useAssignCredentialModal.tssrc/components/settings/tabs/vault/hooks/useHistoryModal.tssrc/components/settings/tabs/vault/hooks/useRotateCredentialModal.tssrc/components/settings/tabs/vault/hooks/useVaultPanelActions.tssrc/components/ui/Modal.tsxsrc/components/vault/RecoveryKeyModal.tsxsrc/components/vault/VaultUnlockModal.tsxsrc/components/vault/VaultWorkspacePanel.tsxsrc/features/connections/domain/merge.tssrc/features/connections/infrastructure/connectionIpc.tssrc/lib/tauri-ipc.tssrc/store/connectionSlice.tssrc/vault/ipc.tssrc/vault/syncIpc.tssrc/vault/useVaultStore.tstests/sessionPersistence.test.mjstests/vaultNavState.test.mjs
Comment on lines
+96
to
+98
| await onLoadConnections(); | ||
| await onRefresh(); | ||
| await loadSecurePreview(); |
Contributor
There was a problem hiding this comment.
Refresh vault items after item-mutating actions, not only vault status.
Both paths mutate the vault's item set, but they only call onRefresh(). In VaultTab, onRefresh is the status refresh path; it does not re-run the item reload effect while the vault stays unlocked, so the list can remain stale after securing credentials or deleting an item.
Proposed fix
- await onLoadConnections();
- await onRefresh();
+ await onLoadConnections();
+ await Promise.all([onRefresh(), onRefreshItems()]);
await loadSecurePreview();
@@
- await vaultIpc.itemDelete(itemId);
- await onRefresh();
+ await vaultIpc.itemDelete(itemId);
+ await onRefreshItems();
showToast('success', `Deleted "${label}".`);Also applies to: 374-376
🤖 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/vault/hooks/useVaultPanelActions.ts` around
lines 96 - 98, The mutating actions call only onRefresh(), which updates vault
status but not the item list; update the code paths (including the block around
useVaultPanelActions.ts lines where onLoadConnections(), onRefresh(),
loadSecurePreview() are called and the similar block at the other location
referenced) to call the item-reload routine after item mutations—either call
onLoadConnections() (or the specific item reload function used elsewhere) after
onRefresh(), or replace onRefresh() with a call sequence that ensures both
status and items are reloaded so the UI reflects secure/delete changes
immediately.
Comment on lines
+8
to
+9
| /** Minimum passphrase length enforced at vault creation time. */ | ||
| export const PASSPHRASE_MIN_LENGTH = 12; |
Contributor
There was a problem hiding this comment.
Enforce the passphrase floor in the backend too.
This 12-character rule only exists in the modal right now; VaultService::initialize() still accepts shorter passphrases. Any non-UI IPC caller can bypass the guarantee this comment implies.
Also applies to: 54-57
🤖 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 8 - 9, The
frontend-only 12-character passphrase check (PASSPHRASE_MIN_LENGTH in
VaultUnlockModal.tsx) must be mirrored in the backend: update
VaultService::initialize() to import/use the same PASSPHRASE_MIN_LENGTH constant
and validate the provided passphrase length, returning/throwing a clear error
(e.g., InvalidPassphraseLength) when passphrase.length < PASSPHRASE_MIN_LENGTH
so IPC callers cannot bypass the rule; update any callers/tests to handle the
new error path and ensure the error propagates back over IPC for user-facing
display.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/src/vault/secure_to_vault.rs`:
- Around line 133-139: Update parse_key_secret to treat empty-string and null
passphrases the same: when extracting the passphrase from the JSON object (in
parse_key_secret), call as_str() on val["passphrase"] and chain .filter(|s|
!s.is_empty()) before .map(|s| s.to_string()) so that "" becomes None rather
than Some(""), ensuring parse_key_secret (and the local passphrase variable)
yields None for both null and empty-string passphrases and matches how
secure_to_vault.rs serializes them.
In `@src/components/settings/tabs/vault/hooks/useAssignCredentialModal.ts`:
- Around line 122-137: The submit function currently lumps the committed write
(saveConnectionsIpc) and follow-up callbacks into one try/catch so failures in
onAssigned/onPromptDisconnect are reported as assignment failures; split this
into two phases: first call saveConnectionsIpc (and await it) inside its own
try/catch and on error call showToast('error', ...) describing a save/commit
failure, setIsAssigning(false) and return; then, in a separate try block invoke
onAssigned(), close(), showToast('success', ...) and await
onPromptDisconnect(...), with its own catch to report callback/disconnect errors
(so failures in onAssigned or onPromptDisconnect are not mislabeled as save
failures). Ensure you reference and keep the existing functions
saveConnectionsIpc, onAssigned, onPromptDisconnect, setIsAssigning, close, and
showToast and preserve current success 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: ec14bae4-33a9-44e1-9c42-afac3ac0af5b
📒 Files selected for processing (14)
CHANGELOG.mdsrc-tauri/src/vault/commands.rssrc-tauri/src/vault/crypto.rssrc-tauri/src/vault/error.rssrc-tauri/src/vault/secure_to_vault.rssrc-tauri/src/vault/store.rssrc/components/dashboard/WelcomeScreen.tsxsrc/components/settings/tabs/vault/hooks/useAddCredentialModal.tssrc/components/settings/tabs/vault/hooks/useAssignCredentialModal.tssrc/components/settings/tabs/vault/hooks/useRotateCredentialModal.tssrc/components/settings/tabs/vault/hooks/useVaultPanelActions.tssrc/lib/tauri-ipc.tssrc/store/connectionSlice.tssrc/vault/syncIpc.ts
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (8)
- src/lib/tauri-ipc.ts
- src/components/dashboard/WelcomeScreen.tsx
- src/components/settings/tabs/vault/hooks/useAddCredentialModal.ts
- src-tauri/src/vault/store.rs
- src/store/connectionSlice.ts
- src-tauri/src/vault/commands.rs
- src/components/settings/tabs/vault/hooks/useRotateCredentialModal.ts
- src/components/settings/tabs/vault/hooks/useVaultPanelActions.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/vault/VaultUnlockModal.tsx (1)
39-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset secret visibility state on close.
If a user toggles “show” and reopens the modal, secrets can render in plaintext by default.
Proposed change
const handleClose = () => { setPassphrase(''); setConfirm(''); setRecoveryKey(''); + setShowPass(false); setUnlockMode('passphrase'); setLocalError(''); clearError(); onClose(); };🤖 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 39 - 47, The handleClose function currently resets passphrase, confirm, recoveryKey, unlockMode, and error state but does not reset any secret-visibility flags; update handleClose to also clear the modal's visibility state by calling the visibility setters (e.g., setShowPassphrase, setShowConfirm, setShowRecoveryKey or whichever show* state variables exist in VaultUnlockModal) so that any "show" toggles are reset to false when closing the modal, then call clearError() and onClose() as before.
🧹 Nitpick comments (1)
src/components/vault/SecretField.tsx (1)
32-39: ⚡ Quick winExpose toggle state with
aria-pressedon the reveal button.This improves assistive-tech feedback for the show/hide state.
Proposed change
<button type="button" onClick={onToggleShow} aria-label={showSecret ? `Hide ${label}` : `Show ${label}`} + aria-pressed={showSecret} className="p-1.5 -m-1.5 rounded text-app-muted hover:text-app-text transition-colors" >🤖 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/SecretField.tsx` around lines 32 - 39, The reveal button does not expose its toggle state to assistive tech; update the button in SecretField (the element using onToggleShow, showSecret, and label, rendering Eye / EyeOff) to include an aria-pressed attribute set to the boolean showSecret and ensure aria-label remains descriptive (e.g., keep the existing `Show ${label}` / `Hide ${label}` logic); this will allow screen readers to know the current pressed/toggled state without changing the existing onClick handler or visual 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 `@scripts/reset-vault-test-data.ps1`:
- Line 55: The script is adding the installer path
("%LOCALAPPDATA%\Programs\zync") to the $candidates that Resolve-DataDir chooses
from, which can cause reset to target the install directory; remove that
Join-Path $env:LOCALAPPDATA 'Programs\zync' entry (or move it after real data
locations) and instead ensure Resolve-DataDir prefers true app data locations
such as Join-Path $env:LOCALAPPDATA 'zync' or Join-Path $env:APPDATA 'zync' so
the data dir resolution (Resolve-DataDir and $candidates) never auto-selects the
installation directory.
In `@scripts/reset-vault-test-data.sh`:
- Around line 145-146: The script uses unsafe unquoted array expansions (e.g.,
files=($pattern) and matches=($pattern)) which will word-split paths with
spaces; change the assignments to use quoted expansions like files=("$pattern")
and matches=("$pattern") and ensure any later usage expands the array as
"${files[@]}" or "${matches[@]}" instead of unquoted ${files[@]} to preserve
entries with spaces (look for the symbols files, matches, and pattern in the
file to locate all occurrences).
In `@src-tauri/src/sync/collection.rs`:
- Around line 107-141: The code updates existing.key_policy_mode before
attempting to load/unwrap the collection key, but key_wrap_aad() depends on the
original mode so this can break unwrap_collection_key() when the keyring cache
is empty; to fix, preserve the original mode for the unwrap step (either by not
assigning existing.key_policy_mode until after successful
unwrap_collection_key(&manifest, passphrase) or by saving let original_mode =
existing.key_policy_mode and using that when constructing the manifest or AAD
for unwrap), and only apply key_policy_mode to existing (or update manifest)
after the key has been successfully loaded/unwrapped; check symbols: existing,
manifest, key_policy_mode, unwrap_collection_key, load_collection_key_secret,
key_wrap_aad, existing_manifest.
- Around line 86-92: The rename of temp_path to final_path in save_manifest
fails on Windows when final_path already exists; before calling
std::fs::rename(&temp_path, &final_path) check for and remove the existing
final_path (std::fs::remove_file(&final_path)) handling NotFound as non-fatal,
then perform the rename and still ensure temp_path is cleaned up on any error;
update the error mapping around std::fs::rename to attempt
remove_file(&final_path) first (and include temp_path cleanup on failures) so
updates succeed on Windows for the existing manifest.
In `@src-tauri/src/sync/commands.rs`:
- Around line 1057-1099: The restore path (branches handling
RestoreDecision::UpdateExisting and RestoreDecision::RestoreNew that call
svc.item_update and svc.item_create_with_logical_id) does not persist the remote
record's revision/updated_at metadata, causing repeated re-application;
implement a restore-specific write API or augment the service calls used here to
accept and set the remote revision/timestamp (or implement a sync watermark
update) after decide_restore_action produces RestoreNew/UpdateExisting for the
given logical_id and plaintext so that the local record's revision/updated_at
are advanced to the remote values and reconciliation will converge.
In `@src-tauri/src/sync/profiles.rs`:
- Around line 185-191: The rename in save_profiles_store can fail on Windows
when final_path already exists; update the finalize logic to handle that by
attempting to remove final_path if rename returns an already-exists error and
then retrying the rename (or atomically replace by removing final_path first),
ensuring you still clean up temp_path on any failure; reference the
save_profiles_store function and the temp_path/final_path variables and make
sure SyncError is returned on unrecoverable errors while preserving the existing
cleanup behavior.
In `@src-tauri/src/sync/provider.rs`:
- Around line 62-70: The tests use the EncryptionMode type but it is not
imported; update the imports in this module to include EncryptionMode from
super::types so make_test_capabilities(encryption_mode: EncryptionMode) and the
tests at lines referencing EncryptionMode compile; locate the top-of-file
use/import block where ProviderCapabilities is used and add EncryptionMode to
the same import list.
In `@src-tauri/src/sync/providers/google.rs`:
- Line 334: The arithmetic setting tokens.expires_at uses now_secs() +
resp["expires_in"].as_u64().unwrap_or(3600) - 60 which can underflow when
expires_in < 60; update the logic in the code path that assigns
tokens.expires_at (and the other occurrence around line 745) to compute the
expiry safely by treating the 60s clock-skew subtraction with saturation or
checked subtraction—e.g., compute expires_in =
resp["expires_in"].as_u64().unwrap_or(3600) and set tokens.expires_at =
now_secs() + expires_in.saturating_sub(60) or use checked_sub to ensure you
never subtract past zero, then add now_secs().
In `@src/components/settings/tabs/vault/VaultStatusCard.tsx`:
- Line 27: The Tailwind class strings in the VaultStatusCard component use
invalid syntax like border-(--color-app-border), bg-(--color-app-surface), and
text-(--color-app-muted); update each arbitrary value to use Tailwind's bracket
+ var() form (e.g., border-[var(--color-app-border)],
bg-[var(--color-app-surface)], text-[var(--color-app-muted)]) wherever those
invalid classes appear in the VaultStatusCard.tsx JSX (the className on the
outer div and the other instances mentioned) so styling compiles correctly.
In `@src/components/vault/VaultUnlockModal.tsx`:
- Around line 124-128: The submitDisabled logic currently enables the button for
whitespace-only recovery keys because it checks recoveryKey truthiness without
trimming; update the condition in the submitDisabled expression (the ternary
that checks unlockMode === 'recovery') to use a trimmed check (e.g.,
!recoveryKey?.trim()) so whitespace-only input is treated as empty, and keep the
existing checks for passphrase, isUninitialized, confirm, and isLoading
unchanged; adjust references to recoveryKey, passphrase, unlockMode, confirm,
isUninitialized, and isLoading to locate and modify the expression inside the
submitDisabled prop.
In `@src/vault/syncIpc.ts`:
- Around line 212-215: The branch that assigns connectStatus directly to
latestStatus and caches it (variables connectStatus, latestStatus,
lastKnownStatusByProvider, provider in syncIpc.ts) skips the normalization
applied in status(), so normalize connectStatus the same way before
assigning/caching and before notifying listeners: run the connectStatus through
the same normalization helper or call the status()-style normalizer used
elsewhere (or extract that logic into normalizeConnectStatus) and then set
latestStatus = normalizedStatus and lastKnownStatusByProvider[provider] =
normalizedStatus so provider/error/errorCode fields are consistent.
In `@tests/unlockModalConsistency.test.mjs`:
- Around line 33-39: The hasImportNamed helper only checks the local binding
(element.name.text) and ignores aliased imports and module source; update
hasImportNamed to also check element.propertyName?.text (the original imported
name) so aliases like "import { UnlockModalShell as Shell }" are detected, and
add an expectedModule parameter (or compare against the intended module string)
to verify statement.moduleSpecifier?.text matches the correct module source
before accepting the import; keep references: hasImportNamed,
sourceFile.statements, ts.isImportDeclaration,
statement.importClause.namedBindings, ts.isNamedImports, and
element.propertyName/.name when implementing the change.
---
Outside diff comments:
In `@src/components/vault/VaultUnlockModal.tsx`:
- Around line 39-47: The handleClose function currently resets passphrase,
confirm, recoveryKey, unlockMode, and error state but does not reset any
secret-visibility flags; update handleClose to also clear the modal's visibility
state by calling the visibility setters (e.g., setShowPassphrase,
setShowConfirm, setShowRecoveryKey or whichever show* state variables exist in
VaultUnlockModal) so that any "show" toggles are reset to false when closing the
modal, then call clearError() and onClose() as before.
---
Nitpick comments:
In `@src/components/vault/SecretField.tsx`:
- Around line 32-39: The reveal button does not expose its toggle state to
assistive tech; update the button in SecretField (the element using
onToggleShow, showSecret, and label, rendering Eye / EyeOff) to include an
aria-pressed attribute set to the boolean showSecret and ensure aria-label
remains descriptive (e.g., keep the existing `Show ${label}` / `Hide ${label}`
logic); this will allow screen readers to know the current pressed/toggled state
without changing the existing onClick handler or visual 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: 909150a2-ab65-4cc7-b4b9-b4864b20bad0
📒 Files selected for processing (49)
CHANGELOG.mddocs/VAULT_AND_SYNC_ARCHITECTURE.mddocs/VAULT_CREDENTIAL_IDENTITY_MODEL.mddocs/VAULT_PROVIDER_SYNC_KEY_MODEL.mddocs/VAULT_SYNC_PHASE2_SMOKE_TEST.mdpackage.jsonscripts/reset-vault-test-data.ps1scripts/reset-vault-test-data.shsrc-tauri/src/commands.rssrc-tauri/src/lib.rssrc-tauri/src/sync/collection.rssrc-tauri/src/sync/commands.rssrc-tauri/src/sync/mod.rssrc-tauri/src/sync/profiles.rssrc-tauri/src/sync/provider.rssrc-tauri/src/sync/providers/google.rssrc-tauri/src/sync/providers/mod.rssrc-tauri/src/sync/types.rssrc-tauri/src/vault/commands.rssrc-tauri/src/vault/crypto.rssrc-tauri/src/vault/error.rssrc-tauri/src/vault/secure_to_vault.rssrc-tauri/src/vault/store.rssrc-tauri/src/vault/types.rssrc/components/dashboard/WelcomeScreen.tsxsrc/components/settings/tabs/VaultTab.tsxsrc/components/settings/tabs/vault/RestoreConflictModal.tsxsrc/components/settings/tabs/vault/SyncCollectionSetupModal.tsxsrc/components/settings/tabs/vault/SyncCollectionUnlockModal.tsxsrc/components/settings/tabs/vault/VaultItemsPanel.tsxsrc/components/settings/tabs/vault/VaultStatusCard.tsxsrc/components/settings/tabs/vault/VaultSyncCard.tsxsrc/components/settings/tabs/vault/hooks/useAddCredentialModal.tssrc/components/settings/tabs/vault/hooks/useAssignCredentialModal.tssrc/components/settings/tabs/vault/hooks/useRotateCredentialModal.tssrc/components/settings/tabs/vault/hooks/useVaultPanelActions.tssrc/components/vault/RecoveryKeyModal.tsxsrc/components/vault/SecretField.tsxsrc/components/vault/UnlockModalShell.tsxsrc/components/vault/VaultModeSwitch.tsxsrc/components/vault/VaultUnlockModal.tsxsrc/lib/tauri-ipc.tssrc/store/connectionSlice.tssrc/vault/ipc.tssrc/vault/syncError.tssrc/vault/syncIpc.tstests/syncErrorParser.test.mjstests/unlockModalConsistency.test.mjstsconfig.agent-tests.json
✅ Files skipped from review due to trivial changes (6)
- tsconfig.agent-tests.json
- docs/VAULT_CREDENTIAL_IDENTITY_MODEL.md
- src/components/dashboard/WelcomeScreen.tsx
- docs/VAULT_SYNC_PHASE2_SMOKE_TEST.md
- CHANGELOG.md
- docs/VAULT_AND_SYNC_ARCHITECTURE.md
🚧 Files skipped from review as they are similar to previous changes (12)
- src-tauri/src/vault/error.rs
- src/store/connectionSlice.ts
- src/vault/ipc.ts
- src-tauri/src/vault/secure_to_vault.rs
- src/lib/tauri-ipc.ts
- src-tauri/src/vault/crypto.rs
- src-tauri/src/vault/commands.rs
- src/components/settings/tabs/vault/hooks/useRotateCredentialModal.ts
- src/components/settings/tabs/vault/hooks/useAddCredentialModal.ts
- src/components/settings/tabs/vault/hooks/useAssignCredentialModal.ts
- src/components/settings/tabs/vault/hooks/useVaultPanelActions.ts
- src-tauri/src/vault/store.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/src/sync/profiles.rs`:
- Around line 185-216: The rename-failure fallback currently deletes final_path
regardless of the error type, which can destroy valid data; change the branch
handling Err(rename_err) in the rename of temp_path -> final_path to only
attempt the destructive remove-and-retry path when rename_err.kind() ==
std::io::ErrorKind::AlreadyExists (i.e., the destination truly conflicts).
Specifically, in the match on std::fs::rename(&temp_path, &final_path) check
rename_err.kind() first: if AlreadyExists then try
std::fs::remove_file(&final_path) and retry the rename (keeping the existing
retry/removal-on-temp_path logic and SyncError construction), otherwise do not
remove final_path—clean up temp_path and return Err(SyncError::new(...)) with
the original rename_err.
🪄 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: a0b606c5-aff8-4bec-86aa-feb39f51bc5a
📒 Files selected for processing (14)
CHANGELOG.mdscripts/reset-vault-test-data.ps1scripts/reset-vault-test-data.shsrc-tauri/src/sync/collection.rssrc-tauri/src/sync/commands.rssrc-tauri/src/sync/profiles.rssrc-tauri/src/sync/provider.rssrc-tauri/src/sync/providers/google.rssrc-tauri/src/vault/store.rssrc/components/settings/tabs/vault/VaultStatusCard.tsxsrc/components/vault/SecretField.tsxsrc/components/vault/VaultUnlockModal.tsxsrc/vault/syncIpc.tstests/unlockModalConsistency.test.mjs
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/unlockModalConsistency.test.mjs
- src-tauri/src/sync/provider.rs
- src/components/vault/VaultUnlockModal.tsx
- scripts/reset-vault-test-data.sh
- src-tauri/src/sync/providers/google.rs
- src/vault/syncIpc.ts
- src-tauri/src/sync/commands.rs
- src-tauri/src/sync/collection.rs
…ze flows Preserve remote revision/timestamp metadata during restore create/update so reconciliation converges, and apply review-driven robustness fixes across sync finalize paths, token expiry arithmetic, and vault UX/accessibility edges. Constraint: Restore must remain compatible with existing logical-id conflict resolution and vault revision history behavior Rejected: Sync watermark-only workaround in commands layer | does not guarantee per-record local metadata convergence Confidence: high Scope-risk: moderate Reversibility: clean Directive: Keep sync-specific write APIs as the only entry points that set remote revision/updated_at values Tested: cargo check --manifest-path src-tauri/Cargo.toml; pnpm run type-check; node --test tests/unlockModalConsistency.test.mjs Not-tested: End-to-end multi-provider cloud restore cycle on real external accounts
Apply the latest review follow-ups by replacing brittle source-regex assertions with behavioral status-normalization tests and tightening rename fallback branches to avoid unsafe replacement attempts. Constraint: Keep existing sync/provider behavior stable while improving verification strength Rejected: Keep regex-based static test checks | too brittle and missed behavior-level regressions Confidence: high Scope-risk: narrow Reversibility: clean Directive: Prefer behavioral tests over source-string/regex tests for sync normalization and IPC paths Tested: node --test tests/unlockModalConsistency.test.mjs tests/syncIpcStatusNormalization.test.mjs; cargo check --manifest-path src-tauri/Cargo.toml; pnpm run type-check Not-tested: Full live cloud round-trip with network fault injection
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.
Add credential revision history and restore support across the vault backend, IPC layer, and vault workspace UI.
Modularize vault workspace actions into focused hooks, tighten credential modal validation, improve restore/error handling, and preserve vault credential references across sync/import flows.
Update docs, changelog, CI checks, regression tests, and small UI/build-warning polish.
Summary by CodeRabbit
Release Notes
New Features
UI Improvements
Documentation