feat: Phase 17 — Recycle Bin#262
Merged
Merged
Conversation
Phase 17: Recycle Bin - Implementation decisions documented - Phase boundary established Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 2c6a71ea5f74
Phase 17: Recycle Bin - Standard stack identified (no new dependencies) - Architecture patterns documented (ECIES bin metadata, HKDF IPNS derivation) - Delete/restore/purge flows mapped to existing codebase - Pitfalls catalogued (orphaned IPNS, blob growth, stale paths, shared items) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: f5e50b357969
Phase 17: Recycle Bin - 4 plans in 3 waves - Wave 1: crypto primitives + API config (parallel-ready) - Wave 2: bin service + store + soft-delete rewiring - Wave 3: bin UI + desktop FUSE soft-delete + E2E tests (parallel) - Ready for execution Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 888005a46529
- Plan 02: split Task 1 into store+init+add vs restore+delete+empty+purge; move useAuth.ts bin initialization wiring from Plan 03 into Plan 02 - Plan 03: split Task 2 into new components vs existing component mods; remove useAuth.ts (moved to Plan 02) - Plan 04: research actual CipherBoxFS architecture (no AppHandle, no emit); commit to single approach: remove unpin_content calls only; split E2E tests into new Plan 05 - Plan 05: new plan for E2E tests at Wave 4 (depends on Plan 03 UI); fix page object path to tests/e2e/page-objects/pages/bin.page.ts - Add explicit quota verification in permanentlyDelete/emptyBin - Update ROADMAP.md to reflect 5 plans Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: bd0c70a54e93
Plan 04: Desktop FUSE deletes now create proper bin entries instead of just removing unpin calls. Adds Rust bin crypto module (HKDF derivation, ECIES encrypt/decrypt, serde structs matching TypeScript types), wires bin entry creation into handle_unlink and handle_rmdir. depends_on changed from 17-02 to 17-01 (needs crypto types, not web service layer). Plan 05: Added desktop E2E test scripts (test-recycle-bin.sh/.ps1) that verify FUSE delete creates recoverable soft-delete state. Updated run-all scripts to include recycle bin as Step 5. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 95095ab2d6c8
- Add RecycleBinMetadata and BinEntry types with FilePointer/FolderEntry refs - Add HKDF derivation for bin IPNS keypair (info: cipherbox-recycle-bin-ipns-v1) - Add ECIES encrypt/decrypt for bin metadata blob (same pattern as DeviceRegistry) - Add runtime schema validator for decrypted bin metadata - Export all bin types and functions from @cipherbox/crypto root Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 3a6fd55b5d8b
- Add GET /vault/config returning { recycleBinRetentionDays } from env
- Create VaultConfigResponseDto with Swagger decorators
- Inject ConfigService into VaultService with default 30 days
- Add unit tests for controller and service getConfig methods
- Regenerate API client with typed getConfig function
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Entire-Checkpoint: f98de1a70d61
Tasks completed: 2/2 - Create bin metadata crypto module - Add retention config API endpoint SUMMARY: .planning/phases/17-recycle-bin/17-01-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 9982c0853bd1
- Zustand bin store with CRUD actions, loading state, retention config - Bin service: initializeBin (load from IPNS), addToBin (encrypt + publish) - Full implementations for restore, permanentlyDelete, emptyBin, purgeExpired - Internal helpers: loadBinMetadata, saveBinMetadata with TEE enrollment - Recursive parent restore with max depth 5 fallback to root - CID cleanup with quota updates on permanent delete Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 36cc384c0ec9
… auth - folder.service: deleteFolder/deleteFileFromFolder return removedChild - useFolderMutations: replace unpinFromIpfs with fire-and-forget addToBin - useFolderMutations: capture removed children in batch delete for bin - useBin hook: restore, permanentDelete, emptyAll, batch ops, daysRemaining - useAuth: initializeBin after vault load, fetch retention config from API - useAuth: clearBin on both logout paths - buildFolderPath helper for breadcrumb display in bin entries Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: f5c9ea7211d0
Tasks completed: 3/3 - Create bin store and bin service with full IPNS lifecycle - (Included in Task 1) Implement restore, permanent delete, empty, purge - Rewire delete flow, create useBin hook, wire useAuth SUMMARY: .planning/phases/17-recycle-bin/17-02-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: a10573e69520
- Create crypto/bin.rs with RecycleBinMetadata, BinEntry, BinItemType serde structs matching TypeScript types (camelCase serialization) - Add ECIES encrypt/decrypt for bin metadata (same pattern as DeviceRegistry) - Add derive_bin_ipns_keypair to hkdf.rs with info "cipherbox-recycle-bin-ipns-v1" - Add generate_uuid_v4 and guess_mime_type helpers to avoid external deps - Register bin module in crypto/mod.rs with re-exports Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: b360071c1f4c
- Add 'bin' icon type to NavItem with wastebasket emoji - Add Bin nav item to AppSidebar between Shared and Settings - Create BinPage route component with auth guard - Register /bin route in routes/index.tsx after /shared Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 55db7327fdcd
- BinBrowser: flat list view with sortable columns (name, deleted, size, time left) - BinBrowser: multi-select with Shift/Ctrl+click, batch restore and permanent delete - BinBrowser: Empty Bin button with confirmation dialog, auto-purge on mount - BinBrowser: inline context menu with Restore and Delete Permanently actions - BinListItem: row component with checkbox, type icon, name, path, date, size, countdown - BinListItem: warning styling for items with <= 3 days remaining - BinEmptyState: terminal ASCII art empty state with retention period info - bin-browser.css: 5-column grid layout, warning states, destructive button styles Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 42f0121d0d27
- Replace unpin_content in handle_unlink with BinEntry creation for files - Replace unpin_content in handle_rmdir with BinEntry creation for folders - Add spawn_bin_entry_publish to mod.rs (reads existing bin IPNS, appends entry, encrypts with ECIES, publishes updated bin metadata) - Add build_folder_path helper for breadcrumb path generation - CIDs remain pinned on delete for recovery via web app bin UI - Preserve unpin calls in handle_mkdir (parent metadata) and rename (overwrite) - Use inline generate_uuid_v4 and guess_mime_type to avoid new dependencies Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: ef3ecaaeade0
Tasks completed: 2/2 - Task 1: Add Rust bin crypto module and HKDF derivation - Task 2: Wire bin entry creation into FUSE unlink and rmdir SUMMARY: .planning/phases/17-recycle-bin/17-04-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 1c6637d54a19
The bin context menu's mousedown capture handler was closing the menu on ANY click, including clicks on the menu items themselves. This caused Restore and Delete Permanently to silently fail. Fixed by checking .contains() before dismissing, matching the pattern in ContextMenu.tsx. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 8acac744c38b
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 56fa4b17b200
- Add BinPage page object at tests/e2e/page-objects/pages/bin.page.ts - Add recycle-bin.spec.ts with 6 test cases: delete-to-bin, restore, permanent delete, empty bin, metadata display, sidebar navigation - Export BinPage from page-objects barrel file - Follow existing E2E patterns (serial describe, test-login auth) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 9be4e075838d
- Add test-recycle-bin.sh verifying FUSE delete soft-delete behavior: create file, delete, verify gone from mount, verify API reachable - Add test-recycle-bin.ps1 Windows PowerShell equivalent - Update run-all.sh and run-all.ps1 with Step 5 (recycle bin) - Follow existing desktop E2E patterns (pass/fail counting, auth setup) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 86de130ea7d5
Tasks completed: 2/2 - Task 1: Create web app bin E2E tests (Playwright) - Task 2: Create desktop E2E recycle bin test scripts SUMMARY: .planning/phases/17-recycle-bin/17-05-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 0b1209e7681f
Phase 17 verified: 8/8 must-haves passed. All 5 plans executed across 4 waves. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 24cfb395df5a
Phase 17 requirements were referenced in ROADMAP.md but missing from REQUIREMENTS.md. Added and marked complete. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 9dd9d3a04de5
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a cross-platform Recycle Bin: encrypted IPNS-backed bin metadata (TS + Rust), HKDF-based IPNS key derivation, ECIES encrypt/decrypt, client bin store/service/hooks and UI, desktop FUSE soft-delete publishing, GET /vault/config retention endpoint, unit and E2E tests, and orchestration updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Web as Web App
participant BinService as Bin Service
participant Crypto as Crypto (ECIES/HKDF)
participant IPFS as IPFS/IPNS
participant API as API (Vault Config)
Note over User,API: Delete → Bin (Web)
User->>Web: Delete file/folder
Web->>BinService: addToBin(item, parentPath)
BinService->>Crypto: deriveBinIpnsKeypair(userPrivateKey)
Crypto-->>BinService: {privateKey, publicKey, ipnsName}
BinService->>IPFS: Resolve/decrypt existing bin metadata
BinService->>BinService: Append BinEntry
BinService->>Crypto: encryptBinMetadata(updated, userPublicKey)
Crypto-->>BinService: encryptedBytes
BinService->>IPFS: Pin encrypted metadata -> CID
BinService->>IPFS: Publish CID to IPNS (seq++)
BinService-->>Web: update store
Web->>User: show item in Bin
Note over User,API: Retention config
API->>Web: GET /vault/config => {recycleBinRetentionDays}
Web->>BinService: purgeExpired(retentionDays)
sequenceDiagram
participant User as FUSE User
participant FUSE as Desktop FUSE
participant CryptoR as Crypto (Rust)
participant IPFS as IPFS/IPNS
User->>FUSE: unlink/rmdir
FUSE->>FUSE: capture inode -> FilePointer/FolderEntry
FUSE->>CryptoR: derive_bin_ipns_keypair(user_private_key)
CryptoR-->>FUSE: (privKey, pubKey, ipnsName)
FUSE->>FUSE: build BinEntry
FUSE->>FUSE: spawn_bin_entry_publish(entry) [background]
FUSE-->>User: return success
rect rgba(100, 200, 100, 0.5)
FUSE->>IPFS: resolve/decrypt existing bin metadata
FUSE->>FUSE: append entry
FUSE->>CryptoR: encrypt_bin_metadata(updated, user_public_key)
FUSE->>IPFS: pin + publish IPNS (seq++)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
|
FSM1
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #262 +/- ##
==========================================
+ Coverage 46.31% 48.15% +1.84%
==========================================
Files 106 110 +4
Lines 8266 8989 +723
Branches 591 639 +48
==========================================
+ Hits 3828 4329 +501
- Misses 4271 4490 +219
- Partials 167 170 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Implements Phase 17 “Recycle Bin” across the crypto package, API, web app, desktop FUSE layer, and E2E tests. The goal is to convert deletes into soft-deletes stored in an encrypted, IPNS-backed bin record with configurable retention, plus UI + restore/permanent-delete workflows.
Changes:
- Add
@cipherbox/cryptobin module (types, ECIES encrypt/decrypt, HKDF-derived bin IPNS keypair, schema validation) and re-export it. - Add API endpoint
GET /vault/configexposingrecycleBinRetentionDays(default 30) and regenerate the web API client. - Add web + desktop implementations for bin lifecycle (soft-delete wiring, bin UI/route/nav, desktop FUSE unlink/rmdir -> bin publish) plus Playwright + desktop scripts.
Reviewed changes
Copilot reviewed 53 out of 58 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/tests/recycle-bin.spec.ts | Playwright E2E suite covering bin lifecycle scenarios |
| tests/e2e/page-objects/pages/bin.page.ts | Page object for bin navigation + actions |
| tests/e2e/page-objects/index.ts | Barrel export for BinPage |
| tests/e2e-desktop/scripts/test-recycle-bin.sh | Desktop bash script validating FUSE soft-delete workflow |
| tests/e2e-desktop/scripts/test-recycle-bin.ps1 | Desktop PowerShell script validating FUSE soft-delete workflow |
| tests/e2e-desktop/scripts/run-all.sh | Adds recycle bin step to desktop test runner |
| tests/e2e-desktop/scripts/run-all.ps1 | Adds recycle bin step to desktop test runner |
| packages/crypto/src/index.ts | Re-export bin module from crypto package root |
| packages/crypto/src/bin/types.ts | Defines BinEntry / RecycleBinMetadata |
| packages/crypto/src/bin/schema.ts | Runtime schema validation for decrypted bin metadata |
| packages/crypto/src/bin/index.ts | Bin module barrel exports |
| packages/crypto/src/bin/encrypt.ts | ECIES encrypt/decrypt for bin metadata blob |
| packages/crypto/src/bin/derive-ipns.ts | HKDF-based deterministic bin IPNS keypair derivation |
| packages/api-client/openapi.json | Adds /vault/config endpoint + VaultConfigResponseDto schema |
| apps/web/src/styles/bin-browser.css | Bin browser-specific styling |
| apps/web/src/stores/bin.store.ts | Zustand store for bin state |
| apps/web/src/services/folder.service.ts | Delete helpers now return removed child for bin insertion |
| apps/web/src/routes/index.tsx | Registers /bin route |
| apps/web/src/routes/BinPage.tsx | Bin page wrapper with auth guard |
| apps/web/src/hooks/useFolderMutations.ts | Rewires delete -> addToBin (soft-delete) |
| apps/web/src/hooks/useBin.ts | Hook wrapper for bin operations + days remaining calc |
| apps/web/src/hooks/useAuth.ts | Bin init on login; clear bin on logout; fetch retention config |
| apps/web/src/components/layout/NavItem.tsx | Adds bin icon option |
| apps/web/src/components/layout/AppSidebar.tsx | Adds Bin nav item |
| apps/web/src/components/file-browser/BinListItem.tsx | Bin list row UI + selection + context menu trigger |
| apps/web/src/components/file-browser/BinEmptyState.tsx | Empty-state UI for bin |
| apps/web/src/components/file-browser/BinBrowser.tsx | Main bin browser UI (sorting, multi-select, actions) |
| apps/web/src/api/vault/vault.ts | Generated client adds vaultControllerGetConfig |
| apps/web/src/api/models/vaultConfigResponseDto.ts | Generated DTO for config response |
| apps/web/src/api/models/index.ts | Exports the new DTO |
| apps/desktop/src-tauri/src/fuse/write_ops.rs | FUSE unlink/rmdir now creates bin entries (soft-delete) |
| apps/desktop/src-tauri/src/fuse/mod.rs | Background helper to publish bin entry to bin IPNS |
| apps/desktop/src-tauri/src/crypto/mod.rs | Exposes bin crypto module in Rust crate |
| apps/desktop/src-tauri/src/crypto/hkdf.rs | Adds derive_bin_ipns_keypair |
| apps/desktop/src-tauri/src/crypto/bin.rs | Rust bin metadata structs + ECIES encrypt/decrypt |
| apps/api/src/vault/vault.service.ts | Adds getConfig() using RECYCLE_BIN_RETENTION_DAYS |
| apps/api/src/vault/vault.service.spec.ts | Unit tests for getConfig() |
| apps/api/src/vault/vault.controller.ts | Adds GET /vault/config endpoint |
| apps/api/src/vault/vault.controller.spec.ts | Controller tests for config endpoint |
| apps/api/src/vault/dto/vault-config.dto.ts | DTO definition for config response |
| apps/api/src/vault/dto/index.ts | Exports config DTO |
| .planning/phases/17-recycle-bin/17-VERIFICATION.md | Phase verification report |
| .planning/phases/17-recycle-bin/17-CONTEXT.md | Phase context doc |
| .planning/phases/17-recycle-bin/17-05-SUMMARY.md | Testing plan summary |
| .planning/phases/17-recycle-bin/17-05-PLAN.md | Testing plan details |
| .planning/phases/17-recycle-bin/17-04-SUMMARY.md | Desktop integration summary |
| .planning/phases/17-recycle-bin/17-03-SUMMARY.md | Web UI summary |
| .planning/phases/17-recycle-bin/17-03-PLAN.md | Web UI plan details |
| .planning/phases/17-recycle-bin/17-02-SUMMARY.md | Store/service/delete wiring summary |
| .planning/phases/17-recycle-bin/17-02-PLAN.md | Store/service/delete wiring plan |
| .planning/phases/17-recycle-bin/17-01-SUMMARY.md | Crypto+API config summary |
| .planning/phases/17-recycle-bin/17-01-PLAN.md | Crypto+API config plan |
| .planning/STATE.md | Updates project state to Phase 17 complete |
| .planning/ROADMAP.md | Marks Phase 17 complete + lists plans |
| .planning/REQUIREMENTS.md | Adds BIN-01..BIN-05 requirements as satisfied |
You can also share your feedback on Copilot code review. Take the survey.
Adds comprehensive tests for the bin IPNS derivation, ECIES encrypt/decrypt round-trip, and schema validation. Brings crypto package coverage from 76.42% to 85.2%, above the 80% CI threshold. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 00fb51fb7c17
- Add Ed25519-vs-ECIES key mismatch test for encryptBinMetadata - Fix misleading test name: "accepts null filePointer" -> "rejects" - Surface pre-delete read failures in desktop E2E script - Make cleanup conditional on actual deletion success Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 60fe4e8bab41
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
apps/desktop/src-tauri/src/crypto/tests.rs (1)
1227-1260: Build test fixture shape fromitem_type, notidprefix.
make_test_bin_entrycurrently decidesfile_pointer/folder_entryusingid.starts_with(...), which can create inconsistent fixtures ifitem_typeandiddiverge. Driving those fields fromitem_typewill make tests stricter and less error-prone.Proposed refactor
fn make_test_bin_entry(id: &str, item_type: bin::BinItemType) -> bin::BinEntry { - bin::BinEntry { - id: id.to_string(), - item_type, - name: format!("test-{}", id), - original_parent_ipns_name: "k51qzi5uqu5dlvj2bv6qmx8snx6m8xq7nprfqqopzz6p0".to_string(), - original_path: "My Vault / Documents".to_string(), - deleted_at: 1700000000000, - size: 1024, - mime_type: "text/plain".to_string(), - file_pointer: match id { - _ if id.starts_with("file") => Some(FilePointer { - id: "fp-001".to_string(), - name: format!("test-{}", id), - file_meta_ipns_name: "k51file".to_string(), - ipns_private_key_encrypted: Some("deadbeef".to_string()), - created_at: 1700000000000, - modified_at: 1700000000000, - }), - _ => None, - }, - folder_entry: match id { - _ if id.starts_with("folder") => Some(FolderEntry { - id: "fe-001".to_string(), - name: format!("test-{}", id), - ipns_name: "k51folder".to_string(), - folder_key_encrypted: "aabbccdd".to_string(), - ipns_private_key_encrypted: "eeff0011".to_string(), - created_at: 1700000000000, - modified_at: 1700000000000, - }), - _ => None, - }, - } + let (file_pointer, folder_entry) = match item_type { + bin::BinItemType::File => ( + Some(FilePointer { + id: "fp-001".to_string(), + name: format!("test-{}", id), + file_meta_ipns_name: "k51file".to_string(), + ipns_private_key_encrypted: Some("deadbeef".to_string()), + created_at: 1700000000000, + modified_at: 1700000000000, + }), + None, + ), + bin::BinItemType::Folder => ( + None, + Some(FolderEntry { + id: "fe-001".to_string(), + name: format!("test-{}", id), + ipns_name: "k51folder".to_string(), + folder_key_encrypted: "aabbccdd".to_string(), + ipns_private_key_encrypted: "eeff0011".to_string(), + created_at: 1700000000000, + modified_at: 1700000000000, + }), + ), + }; + + bin::BinEntry { + id: id.to_string(), + item_type, + name: format!("test-{}", id), + original_parent_ipns_name: "k51qzi5uqu5dlvj2bv6qmx8snx6m8xq7nprfqqopzz6p0".to_string(), + original_path: "My Vault / Documents".to_string(), + deleted_at: 1700000000000, + size: 1024, + mime_type: "text/plain".to_string(), + file_pointer, + folder_entry, + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src-tauri/src/crypto/tests.rs` around lines 1227 - 1260, The test fixture builder make_test_bin_entry currently infers whether to populate file_pointer or folder_entry from id.starts_with(...), which can produce inconsistent fixtures when the id and item_type disagree; update make_test_bin_entry to use the item_type parameter (bin::BinItemType) to decide which branch to populate (set file_pointer Some(...) when item_type is File and folder_entry None, set folder_entry Some(...) when item_type is Folder and file_pointer None, and leave both None for other types), keeping the same field values and structure but switching the match conditions to inspect item_type instead of id.tests/e2e-desktop/scripts/test-recycle-bin.ps1 (2)
88-89: Make publish waits configurable for CI stability.The comments explain the wait rationale, but fixed sleeps are still brittle across environments. Promote them to env-configurable values so CI can tune without script edits.
💡 Suggested fix
$TestSecret = if ($env:TEST_SECRET) { $env:TEST_SECRET } else { "e2e-test-secret-ci-only" } +$PublishWaitSeconds = if ($env:E2E_PUBLISH_WAIT_SECONDS) { [int]$env:E2E_PUBLISH_WAIT_SECONDS } else { 10 } @@ - Start-Sleep -Seconds 10 # Wait for upload + IPNS publish + Start-Sleep -Seconds $PublishWaitSeconds # Wait for upload + IPNS publish @@ - Start-Sleep -Seconds 10 # Wait for folder metadata publish + bin IPNS publish + Start-Sleep -Seconds $PublishWaitSeconds # Wait for folder metadata publish + bin IPNS publishBased on learnings, sleep-based waits are a known FUSE test limitation and should be documented and made configurable to handle CI variability.
Also applies to: 111-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e-desktop/scripts/test-recycle-bin.ps1` around lines 88 - 89, Replace the hard-coded Start-Sleep calls with env-configurable wait variables: read environment variables (e.g., $UPLOAD_WAIT_SECONDS and $IPNS_PUBLISH_WAIT_SECONDS) with sensible defaults (10s) and use those values in the Start-Sleep calls in the test-recycle-bin.ps1 script (the Start-Sleep at the shown diff and the similar sleeps around lines 111-112); ensure the script documents the env var names and defaults so CI can override them without editing the script.
23-23: Avoid embedded fallback secret; requireTEST_SECRETexplicitly.Keeping a default secret literal in-repo weakens secret hygiene for shared CI/test infra. Fail fast when
TEST_SECRETis missing.💡 Suggested fix
-$TestSecret = if ($env:TEST_SECRET) { $env:TEST_SECRET } else { "e2e-test-secret-ci-only" } +if (-not $env:TEST_SECRET) { + throw "TEST_SECRET environment variable is required" +} +$TestSecret = $env:TEST_SECRETBased on learnings, desktop E2E scripts should pass secrets via environment variables rather than embedding fallback values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e-desktop/scripts/test-recycle-bin.ps1` at line 23, Remove the embedded fallback secret and require TEST_SECRET to be set: change the logic around $TestSecret so it reads only from $env:TEST_SECRET and throws/exit with a clear error message if that environment variable is absent (update the assignment/validation for $TestSecret in the script).apps/desktop/src-tauri/src/fuse/mod.rs (1)
486-510: Keep private-key buffers zeroizing in this background path.
user_private_keyis handled as plainVec<u8>here, and this path creates additional non-zeroizing secret buffers. PreferZeroizing<Vec<u8>>in the function boundary and zeroize temporary key arrays after use.Hardening direction
pub(crate) fn spawn_bin_entry_publish( api: Arc<ApiClient>, rt: tokio::runtime::Handle, entry: crate::crypto::bin::BinEntry, - user_private_key: Vec<u8>, + user_private_key: Zeroizing<Vec<u8>>, user_public_key: Vec<u8>, coordinator: Arc<PublishCoordinator>, ) { @@ - let pk_arr: [u8; 32] = user_private_key.as_slice().try_into() + let mut pk_arr: [u8; 32] = user_private_key.as_slice().try_into() .map_err(|_| "Invalid private key length for bin derivation".to_string())?; let (bin_ipns_private_key, _bin_ipns_public_key, bin_ipns_name) = crate::crypto::hkdf::derive_bin_ipns_keypair(&pk_arr) .map_err(|e| format!("Bin IPNS derivation failed: {}", e))?; + pk_arr.zeroize();And at the callsite (
apps/desktop/src-tauri/src/fuse/write_ops.rs:348-365), pass a zeroizing wrapper:Zeroizing::new(fs.private_key.to_vec())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src-tauri/src/fuse/mod.rs` around lines 486 - 510, Change the function signature to accept a zeroizing wrapper for the user key (replace user_private_key: Vec<u8> with user_private_key: Zeroizing<Vec<u8>>) and update the callsite to pass Zeroizing::new(fs.private_key.to_vec()); convert temporary key buffers to zeroizing types (e.g., use let pk_arr: Zeroizing<[u8;32]> = user_private_key.as_slice().try_into().map_err(...)? and/or wrap the [u8;32] into Zeroizing) and ensure any derived secrets such as bin_ipns_private_key and decrypted buffers from crate::api::ipfs::fetch_content are dropped/zeroized (use Zeroizing or call zeroize) immediately after use; keep references to the existing helper functions (crate::crypto::hkdf::derive_bin_ipns_keypair, crate::crypto::bin::decrypt_bin_metadata) but ensure returned secret values are stored in zeroizing containers or explicitly zeroized before exiting the background thread.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src-tauri/src/fuse/mod.rs`:
- Around line 526-529: The code currently treats every error from resolve_ipns
as “bin does not exist” and returns (crate::crypto::bin::empty_bin_metadata(),
None); instead, change the Err(_) arm to inspect the error from resolve_ipns and
only return empty_bin_metadata when the error indicates a missing/not-found IPNS
record; for other errors (transient network, timeout, permission, etc.)
propagate or return the error upward (or log and retry) so we don't silently
publish a fresh index; look for the resolve_ipns call and replace the blanket
Err(_) match with a match that checks error kinds (or maps specific variants)
and only falls back to crate::crypto::bin::empty_bin_metadata() for a
NotFound/missing-record case while returning Err(e) or retrying for all other
cases.
In `@apps/web/src/components/file-browser/BinBrowser.tsx`:
- Around line 212-214: The catch blocks in BinBrowser.tsx that currently call
console.error('Restore failed:', err) (and similar catches at the other noted
locations) must stop logging raw error objects; replace those console.error
calls with a sanitized static message or telemetry code only (e.g., "Bin action
failed: restore" or a stable error code) and, if you need error details for
diagnostics, forward the full err to a secure telemetry/monitoring API rather
than emitting it to the console or logs. Locate the catch blocks in the
restore/delete/other bin action handlers in BinBrowser.tsx and update them to
emit only the static message/telemetry code and route detailed payloads to a
secure channel.
In `@apps/web/src/hooks/useBin.ts`:
- Around line 128-171: restoreMultiple and permanentDeleteMultiple currently
loop calling restoreFromBin and permanentlyDelete for each entry causing
repeated IPNS publishing; replace the per-item loop with a single batch service
call (e.g., add or use restoreFromBinBatch/restoreMultipleEntries and
permanentlyDeleteBatch/permanentlyDeleteMultipleEntries) that accepts the
entryIds array plus userPublicKey/userPrivateKey and performs one publish at the
end, then setState({ isLoading: false, error: null }) on success and preserve
existing try/catch error handling (convert err to message and rethrow) so UI
state and errors behave the same.
In `@apps/web/src/hooks/useFolderMutations.ts`:
- Around line 743-758: The current implementation in useFolderMutations loops
over removedChildren and calls addToBin per item, causing N IPNS publishes;
change this to perform a single batched publish by calling a single
addToBin-like operation with the whole removedChildren array (e.g.,
addToBinBatch or extend addToBin to accept an items: File[] parameter) and pass
parentIpnsName, parentPath (from buildFolderPath(parentId)), and the
auth.vaultKeypair keys once; keep the call fire-and-forget and attach a single
.catch to log a consolidated error (referencing addToBin, removedChildren,
buildFolderPath, parentFolder, and useAuthStore.getState) so one publish handles
the entire batch instead of one per child.
In `@tests/e2e-desktop/scripts/test-recycle-bin.ps1`:
- Around line 125-130: The current check only verifies that $VaultResponse from
Invoke-RestMethod against "$ApiUrl/vault/config" is non-empty; update the
validation to assert that the response contains a numeric
recycleBinRetentionDays field and that its value is >= 1, and use Test-Pass when
that condition is met or Test-Fail with a descriptive message when
missing/invalid; locate the logic around $VaultResponse,
recycleBinRetentionDays, Test-Pass and Test-Fail to implement this additional
validation and error messaging.
- Around line 150-158: The cleanup currently unconditionally calls Remove-Item
on "$MountPoint\$TestFile" which will throw a "path not found" error if Test 2
already deleted the file; change the cleanup to first check existence with
Test-Path("$MountPoint\$TestFile") and only call Remove-Item when the path
exists, and/or treat a missing-path error as non-fatal by catching the specific
error and calling Test-Pass "Cleanup" when the file is already absent; update
the block around Remove-Item, Test-Path, Test-Pass, and Test-Fail accordingly so
cleanup only fails for real deletion errors.
---
Nitpick comments:
In `@apps/desktop/src-tauri/src/crypto/tests.rs`:
- Around line 1227-1260: The test fixture builder make_test_bin_entry currently
infers whether to populate file_pointer or folder_entry from
id.starts_with(...), which can produce inconsistent fixtures when the id and
item_type disagree; update make_test_bin_entry to use the item_type parameter
(bin::BinItemType) to decide which branch to populate (set file_pointer
Some(...) when item_type is File and folder_entry None, set folder_entry
Some(...) when item_type is Folder and file_pointer None, and leave both None
for other types), keeping the same field values and structure but switching the
match conditions to inspect item_type instead of id.
In `@apps/desktop/src-tauri/src/fuse/mod.rs`:
- Around line 486-510: Change the function signature to accept a zeroizing
wrapper for the user key (replace user_private_key: Vec<u8> with
user_private_key: Zeroizing<Vec<u8>>) and update the callsite to pass
Zeroizing::new(fs.private_key.to_vec()); convert temporary key buffers to
zeroizing types (e.g., use let pk_arr: Zeroizing<[u8;32]> =
user_private_key.as_slice().try_into().map_err(...)? and/or wrap the [u8;32]
into Zeroizing) and ensure any derived secrets such as bin_ipns_private_key and
decrypted buffers from crate::api::ipfs::fetch_content are dropped/zeroized (use
Zeroizing or call zeroize) immediately after use; keep references to the
existing helper functions (crate::crypto::hkdf::derive_bin_ipns_keypair,
crate::crypto::bin::decrypt_bin_metadata) but ensure returned secret values are
stored in zeroizing containers or explicitly zeroized before exiting the
background thread.
In `@tests/e2e-desktop/scripts/test-recycle-bin.ps1`:
- Around line 88-89: Replace the hard-coded Start-Sleep calls with
env-configurable wait variables: read environment variables (e.g.,
$UPLOAD_WAIT_SECONDS and $IPNS_PUBLISH_WAIT_SECONDS) with sensible defaults
(10s) and use those values in the Start-Sleep calls in the test-recycle-bin.ps1
script (the Start-Sleep at the shown diff and the similar sleeps around lines
111-112); ensure the script documents the env var names and defaults so CI can
override them without editing the script.
- Line 23: Remove the embedded fallback secret and require TEST_SECRET to be
set: change the logic around $TestSecret so it reads only from $env:TEST_SECRET
and throws/exit with a clear error message if that environment variable is
absent (update the assignment/validation for $TestSecret in the script).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bd123637-77b3-4346-8282-697b3c5ee30f
📒 Files selected for processing (11)
apps/api/src/vault/vault.service.spec.tsapps/desktop/src-tauri/src/crypto/tests.rsapps/desktop/src-tauri/src/fuse/mod.rsapps/web/src/components/file-browser/BinBrowser.tsxapps/web/src/components/file-browser/BinListItem.tsxapps/web/src/hooks/useBin.tsapps/web/src/hooks/useFolderMutations.tsapps/web/src/stores/bin.store.tspackages/crypto/src/__tests__/bin.test.tspackages/crypto/src/bin/schema.tstests/e2e-desktop/scripts/test-recycle-bin.ps1
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/api/src/vault/vault.service.spec.ts
- apps/web/src/components/file-browser/BinListItem.tsx
- packages/crypto/src/bin/schema.ts
- packages/crypto/src/tests/bin.test.ts
- Fix sequence number double-increment bug in addToBin/removeEntriesFromBin - Add batch operations (addManyToBin, restoreFromBinBatch, permanentlyDeleteBatch) to avoid N IPNS publishes for multi-select operations - Distinguish "not found" from transient errors in Rust bin IPNS resolve - Remove raw error object logging from BinBrowser catch blocks - Extract shared utilities (formatRelativeTime, getItemIcon, generate_uuid_v4, mime_from_extension) to eliminate code duplication - Remove dead store actions (addEntry, removeEntry, removeEntries, setLoaded) - Add BIN_METADATA_VERSION constant, extract unpinFileCids helper - Fix redundant SystemTime::now() calls and deriveBinIpnsKeypair double-call - Validate recycleBinRetentionDays in e2e test and fix cleanup after soft-delete Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: b6b084016bf3
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
apps/desktop/src-tauri/src/fuse/mod.rs (1)
482-488:⚠️ Potential issue | 🟠 MajorUse zeroizing ownership for
user_private_keyin this background path.
user_private_keyis accepted as plainVec<u8>, which causes call sites inwrite_ops.rs(lines 359, 735) to create non-zeroized copies via.to_vec()on theZeroizing<Vec<u8>>wrapper. In this crypto-sensitive path, wrap the parameter to ensure automatic zeroization on drop.🔐 Proposed hardening
pub(crate) fn spawn_bin_entry_publish( api: Arc<ApiClient>, rt: tokio::runtime::Handle, entry: crate::crypto::bin::BinEntry, - user_private_key: Vec<u8>, + user_private_key: Zeroizing<Vec<u8>>, user_public_key: Vec<u8>, coordinator: Arc<PublishCoordinator>, ) {Update call sites in
write_ops.rs(lines 359, 735) to pass the zeroizing wrapper directly:crate::fuse::spawn_bin_entry_publish( fs.api.clone(), fs.rt.clone(), bin_entry, - fs.private_key.to_vec(), + fs.private_key.clone(), fs.public_key.to_vec(), fs.publish_coordinator.clone(), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src-tauri/src/fuse/mod.rs` around lines 482 - 488, Change the spawn_bin_entry_publish signature to take a zeroizing-owned private key (e.g., Zeroizing<Vec<u8>>) instead of plain Vec<u8> and update any internal uses to match that type so the private key is zeroized on drop; update call sites in write_ops.rs (where fs.private_key.to_vec() is used) to pass the Zeroizing wrapper directly (e.g., fs.private_key.clone()) and leave user_public_key as Vec<u8> (or also Zeroizing if desired), and add the necessary import of zeroize::Zeroizing where spawn_bin_entry_publish and the callers are defined.
🧹 Nitpick comments (1)
tests/e2e-desktop/scripts/test-recycle-bin.ps1 (1)
32-42: Prefer stream-friendly output commands overWrite-Host.Consider switching test reporting to
Write-Output/Write-Informationso CI logs can be redirected, filtered, and captured more reliably.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e-desktop/scripts/test-recycle-bin.ps1` around lines 32 - 42, Replace the use of Write-Host in the Test-Pass and Test-Fail functions with stream-friendly output (e.g., Write-Output or Write-Information) so CI can capture and redirect logs; update the two functions Test-Pass and Test-Fail to emit their messages via a pipeline-friendly cmdlet and keep incrementing $script:Pass / $script:Fail as-is, ensuring the output strings remain "PASS: $Name" and "FAIL: $Name" so callers and log parsers behave the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src-tauri/src/fuse/write_ops.rs`:
- Around line 290-297: The code currently constructs a FilePointer using
unwrap_or_default() for file_meta_ipns_name which can produce an empty string
and an invalid, non-restorable record; instead, check file_meta_ipns_name before
constructing/emitting the FilePointer (i.e. only create the
crate::crypto::folder::FilePointer when file_meta_ipns_name.is_some()), and if
it is None skip emitting/saving the pointer (or return an error/path indicating
missing meta); update the logic around the FilePointer creation (the block that
sets id, name, file_meta_ipns_name, ipns_private_key_encrypted, created_at,
modified_at) to use the unwrapped value rather than unwrap_or_default() and
avoid creating a pointer with an empty file_meta_ipns_name.
- Around line 651-662: The code currently assigns an empty String to
ipns_key_encrypted when ipns_private_key is None or wrap_key fails, producing
malformed FolderEntry; update the logic around ipns_key_encrypted (the variable
used when constructing FolderEntry) to use an Option (None when no key or when
wrap_key errors) instead of String::new(), and in the wrap_key Err arm avoid
returning an empty string—either propagate or set None and log the error via the
existing log::warn!; adapt the FolderEntry construction to accept Option<String>
(or omit the field) so restore flows receive a missing key as None rather than
an empty string, and ensure crate::crypto::ecies::wrap_key call remains the
place handling the wrap and error logging.
In `@apps/web/src/hooks/useFolderMutations.ts`:
- Around line 34-35: buildFolderPath currently omits the "My Vault" prefix for
non-root folders (returning "Documents / Reports"); update the function so that
when parts.length > 0 it prepends "My Vault" before joining (e.g., ['My Vault',
...parts].join(' / ')), while keeping the existing behavior that returns "My
Vault" for an empty parts array; modify the logic in buildFolderPath to always
include the "My Vault" segment for non-root paths.
In `@apps/web/src/services/bin.service.ts`:
- Around line 301-305: The recursive depth guard isn't accumulating because
calls reset depth to 0; update the functions so a single depth parameter is
threaded through recursion: change calls that pass a hard-coded 0 (e.g.,
resolveTargetFolder(entry.originalParentIpnsName, 0, {...}) and any
restoreFromBin(...) invocations that reset depth) to pass the current depth (or
currentDepth+1 for recursive parent restores), ensure restoreFromBin has a
default depth parameter (e.g., depth = 0) and increments/passes depth+1 into
recursive restoreFromBin and into resolveTargetFolder, and keep the max-depth
check (the existing guard around depth in restoreFromBin) using that accumulated
depth so it actually stops deep recursion.
- Around line 659-676: cleanupFolderCids currently only iterates children when a
folderNode is present in the in-memory useFolderStore, so if the folder isn't
loaded it skips recursive cleanup; update cleanupFolderCids to ensure the
folder's entries are loaded before iterating: locate the cleanupFolderCids
function and the folderNode lookup (uses useFolderStore.getState().folders and
folderNode variable), and if folderNode is missing call the appropriate
loader/fetch function (e.g., a store action or API method that loads the folder
by ipnsName) or fetch the folder entries directly, then iterate its children and
continue calling unpinFileCids for files and cleanupFolderCids recursively for
subfolders; ensure this loader call is awaited and handles errors so descendant
file CIDs are always processed even when the folder wasn't previously loaded.
- Around line 219-223: Wrap the publish call to saveBinMetadata in a
non-throwing construct so addToBin remains fire-and-forget: either remove the
await and call saveBinMetadata(...).catch(...) or surround the await with
try/catch and swallow/log the error, ensuring you still call
store.setEntries(currentEntries, newSeq). Reference saveBinMetadata and
store.setEntries (and the addToBin flow) so failures are logged but do not
propagate to callers.
- Around line 424-425: The current console.error/console.warn calls log
sensitive file/folder names like entry.name (e.g., the line in bin.service.ts
that does console.error(`[Bin] Failed to restore ${entry.name}:`, err)); change
these log statements to never include plaintext names or parent names—use
non-sensitive identifiers (e.g., entry.id or a generated obfuscated id), counts,
or sanitized placeholders instead and keep the actual error object (err) for
diagnostics; update every similar call in this file (the other occurrences you
noted around the same function/flow) to replace entry.name/parentName with
entry.id or "[redacted]" and ensure stack/error details are preserved in the
log.
In `@apps/web/src/utils/format.ts`:
- Around line 66-70: The function formatRelativeTime currently hardcodes the
'en-US' locale causing inconsistent rendering; update formatRelativeTime to use
the user's locale instead (e.g., pass undefined or use
Intl.DateTimeFormat/navigator.language like formatDate does) so it becomes
locale-aware and consistent with formatDate; locate and modify the
formatRelativeTime implementation to remove the hardcoded 'en-US' and use the
same locale resolution approach as formatDate.
---
Duplicate comments:
In `@apps/desktop/src-tauri/src/fuse/mod.rs`:
- Around line 482-488: Change the spawn_bin_entry_publish signature to take a
zeroizing-owned private key (e.g., Zeroizing<Vec<u8>>) instead of plain Vec<u8>
and update any internal uses to match that type so the private key is zeroized
on drop; update call sites in write_ops.rs (where fs.private_key.to_vec() is
used) to pass the Zeroizing wrapper directly (e.g., fs.private_key.clone()) and
leave user_public_key as Vec<u8> (or also Zeroizing if desired), and add the
necessary import of zeroize::Zeroizing where spawn_bin_entry_publish and the
callers are defined.
---
Nitpick comments:
In `@tests/e2e-desktop/scripts/test-recycle-bin.ps1`:
- Around line 32-42: Replace the use of Write-Host in the Test-Pass and
Test-Fail functions with stream-friendly output (e.g., Write-Output or
Write-Information) so CI can capture and redirect logs; update the two functions
Test-Pass and Test-Fail to emit their messages via a pipeline-friendly cmdlet
and keep incrementing $script:Pass / $script:Fail as-is, ensuring the output
strings remain "PASS: $Name" and "FAIL: $Name" so callers and log parsers behave
the same.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fef0fa9b-bdd4-45d8-89e2-edf8d43ffd5a
📒 Files selected for processing (17)
apps/desktop/src-tauri/src/crypto/bin.rsapps/desktop/src-tauri/src/crypto/tests.rsapps/desktop/src-tauri/src/crypto/utils.rsapps/desktop/src-tauri/src/fuse/helpers.rsapps/desktop/src-tauri/src/fuse/mod.rsapps/desktop/src-tauri/src/fuse/write_ops.rsapps/desktop/src-tauri/src/registry/mod.rsapps/web/src/components/file-browser/BinBrowser.tsxapps/web/src/components/file-browser/BinListItem.tsxapps/web/src/components/file-browser/FileListItem.tsxapps/web/src/components/mfa/AuthorizedDevices.tsxapps/web/src/hooks/useBin.tsapps/web/src/hooks/useFolderMutations.tsapps/web/src/services/bin.service.tsapps/web/src/stores/bin.store.tsapps/web/src/utils/format.tstests/e2e-desktop/scripts/test-recycle-bin.ps1
- Fail-fast on missing file_meta_ipns_name in FUSE unlink (no empty FilePointer) - Fail-fast on missing/unwrappable folder IPNS key in FUSE rmdir (no empty string) - Fix buildFolderPath to include "My Vault" prefix for non-root paths - Make addToBin/addManyToBin fire-and-forget (catch publish failures) - Fix recursive depth guard: thread depth through restoreFromBinInternal - Sanitize log messages: replace entry.name with entry.id - Use locale-aware Intl.DateTimeFormat in formatRelativeTime Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 7d1630bd9018
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 61 out of 66 changed files in this pull request and generated 4 comments.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
apps/web/src/services/bin.service.ts (1)
105-106:⚠️ Potential issue | 🟠 MajorSanitize bin-service error logs instead of logging raw error objects.
These paths still print raw
err/error, which can leak sensitive payloads from crypto/IPNS flows.Suggested pattern
+function logBinError(context: string, err: unknown): void { + const reason = err instanceof Error ? err.name : 'UnknownError'; + console.error(`[Bin] ${context}: ${reason}`); +} ... - } catch (err) { - console.error('[Bin] TEE enrollment failed (non-blocking):', err); + } catch (err) { + logBinError('TEE enrollment failed (non-blocking)', err); } ... - } catch (error) { - console.error('[Bin] Failed to initialize bin:', error); + } catch (error) { + logBinError('Failed to initialize bin', error);As per coding guidelines, "Never log, console.log, or persist to disk sensitive data including ECDSA private keys, decrypted file content, plaintext passwords, or key material."
Also applies to: 162-163, 438-439, 502-503, 533-534, 582-583, 661-662, 694-695
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/services/bin.service.ts` around lines 105 - 106, Replace raw error object prints in the bin service with sanitized error logging: do not pass the full err/error object to console.error; for the specific log that currently reads console.error('[Bin] TEE enrollment failed (non-blocking):', err) (and the other occurrences around lines 162-163, 438-439, 502-503, 533-534, 582-583, 661-662, 694-695), log a limited, non-sensitive message and safe metadata only (e.g., console.error('[Bin] TEE enrollment failed (non-blocking): %s', sanitizeError(err)) or use a helper like sanitizeError(error) / safeSerializeError(error) that returns only error.name, error.message (truncated) and a non-sensitive code/ID), or use the project logger with built-in redaction; ensure no stack trace, buffer content, keys or full error objects are emitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src-tauri/src/fuse/write_ops.rs`:
- Around line 949-967: The 20-iteration safety cap in build_folder_path can
silently truncate deep paths and drop the root; replace the fixed loop with a
proper traversal that walks parents until you hit InodeKind::Root (or current ==
0) while protecting against cycles (e.g., track visited inode ids) instead of
stopping at 20, and if traversal must abort (cycle or missing inode) return or
annotate the original_path (from the caller) rather than a truncated breadcrumb;
update the loop that reads fs.inodes.get(current) and uses inode.parent_ino,
parts, and InodeKind::Root accordingly so deep paths and the "My Vault" root are
preserved or the original_path is returned on failure.
In `@apps/web/src/hooks/useFolderMutations.ts`:
- Around line 27-35: In buildFolderPath, the loop currently includes the root
folder node which causes "My Vault" to appear twice; when iterating folders
(using folders and FolderNode) skip adding the folder.name for the root node by
checking folder.parentId === null (or otherwise identifying the root) and only
unshift names for non-root nodes, keeping the final return logic the same (`My
Vault` when parts empty, otherwise `My Vault / ${parts.join(' / ')}`).
In `@apps/web/src/services/bin.service.ts`:
- Around line 96-104: The code sets binTeeEnrolled true before the IPNS publish
completes, so if createAndPublishIpnsRecord fails subsequent writes will
incorrectly skip enrollment; change the flow in the block that wraps the key
(the try/catch around wrapKey and createAndPublishIpnsRecord) to only assign
binTeeEnrolled = true (and set keyEpoch/encryptedIpnsKey) after
createAndPublishIpnsRecord returns successfully (i.e., move the flag and any
state updates into the success path following createAndPublishIpnsRecord), and
apply the same change to the other occurrence handling (the second try/catch
around wrapKey/createAndPublishIpnsRecord) so the enrollment flag is only set on
successful publish.
- Around line 386-393: The loop currently uses a single captured snapshot
(binStore) so recursive parent restores can mutate state mid-loop and cause
stale reads; update the loop to fetch fresh state each iteration (e.g., replace
the captured binStore lookup with useBinStore.getState().entries.find(...) when
resolving entry by entryId) and also skip if restoredIds already contains
entryId to avoid double-restoring; reference the variables entryIds, restoredIds
and the existing updateFolderMetadata import when making this change.
- Around line 315-336: The restore flow uses resolveTargetFolder and then
publishes via updateFolderMetadata even when targetFolder is a placeholder
(isLoaded: false) or has zeroed/missing crypto fields; add a guard after
resolveTargetFolder to verify targetFolder.isLoaded === true and that folderKey,
ipnsPrivateKey, and ipnsName are present and valid (not zeroed/empty) and if
not, abort/fail the restore with a clear error (or attempt to load/refresh the
folder state before proceeding); apply the same validation check to the other
publish sites referenced (around the blocks using updateFolderMetadata at the
other locations) so no publish is attempted with synthesized or unloaded crypto
state.
In `@apps/web/src/utils/format.ts`:
- Around line 56-60: The current logic computes diff = Date.now() - timestampMs
and treats any diff < 60_000 as "just now", which misclassifies future
timestamps; update the block that checks diff (the diff and timestampMs
variables) to first guard for future times (diff < 0) and in that case return a
formatted absolute date using the existing formatDate(...) helper instead of
"just now", then keep the existing minute/hour/day branches for non-future
diffs.
---
Duplicate comments:
In `@apps/web/src/services/bin.service.ts`:
- Around line 105-106: Replace raw error object prints in the bin service with
sanitized error logging: do not pass the full err/error object to console.error;
for the specific log that currently reads console.error('[Bin] TEE enrollment
failed (non-blocking):', err) (and the other occurrences around lines 162-163,
438-439, 502-503, 533-534, 582-583, 661-662, 694-695), log a limited,
non-sensitive message and safe metadata only (e.g., console.error('[Bin] TEE
enrollment failed (non-blocking): %s', sanitizeError(err)) or use a helper like
sanitizeError(error) / safeSerializeError(error) that returns only error.name,
error.message (truncated) and a non-sensitive code/ID), or use the project
logger with built-in redaction; ensure no stack trace, buffer content, keys or
full error objects are emitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9954254d-b74c-4c5a-802d-004f8af544a1
📒 Files selected for processing (4)
apps/desktop/src-tauri/src/fuse/write_ops.rsapps/web/src/hooks/useFolderMutations.tsapps/web/src/services/bin.service.tsapps/web/src/utils/format.ts
…blish Change spawn_bin_entry_publish signature to accept Zeroizing<Vec<u8>> instead of plain Vec<u8> so the private key is automatically zeroized on drop. Call sites now pass fs.private_key.clone() instead of fs.private_key.to_vec() which previously created a non-zeroized copy. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 3560ad9e880c
- Fix double "My Vault" prefix in buildFolderPath (skip root node in walk) - Allow retention days 0 for immediate purge (Math.max(0, ...) not 1) - Guard future timestamps in formatRelativeTime (fallback to date format) - Move binTeeEnrolled flag after successful IPNS publish - Use fresh bin state in batch restore loop (avoid stale snapshot) - Fix test fixtures to match actual FilePointer/FolderEntry types Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: dbc63b593b9b
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 61 out of 66 changed files in this pull request and generated 1 comment.
You can also share your feedback on Copilot code review. Take the survey.
Bin entry creation is best-effort. Don't fail rm with EIO when file_meta_ipns_name is not yet resolved — skip the bin entry instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: b2d2cec82fad
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/web/src/services/bin.service.ts (3)
763-786: Minor: Redundantfolders['root']check in fallback path.Line 779 references
folders['root']?.childrenin the fallback path, but we only reach this code whenfolders['root']is alreadyundefined(Line 765-766). The expression will always evaluate to[]. While harmless, it could be simplified.Suggested fix
return { id: 'root', name: 'My Vault', ipnsName: vault.rootIpnsName, parentId: null, - children: folders['root']?.children ?? [], + children: [], isLoaded: true, isLoading: false,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/services/bin.service.ts` around lines 763 - 786, The fallback in getRootFolder() uses folders['root']?.children even though the function only reaches that branch when folders['root'] is falsy; update the returned FolderNode so the children field uses an explicit empty array instead of folders['root']?.children (e.g., set children: []), removing the redundant reference to folders['root'] while keeping all other fields (id, name, ipnsName, parentId, isLoaded, isLoading, sequenceNumber, folderKey, ipnsPrivateKey) unchanged.
166-169: Sanitize error logging ininitializeBin.Line 167 logs the raw error object, which could contain sensitive information from IPNS resolution or decryption failures.
Suggested fix
} catch (error) { - console.error('[Bin] Failed to initialize bin:', error); + const reason = error instanceof Error ? error.message : 'Unknown error'; + console.error(`[Bin] Failed to initialize bin: ${reason}`); store.setError(error instanceof Error ? error.message : 'Failed to initialize bin');As per coding guidelines, "Never log, console.log, or persist to disk sensitive data."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/services/bin.service.ts` around lines 166 - 169, The catch block in initializeBin is logging the raw error object and passing it straight to store.setError; replace this by sanitizing the error before logging and storing: in the initializeBin catch, create a safeMessage (e.g., a generic "Failed to initialize bin" plus non-sensitive error.name or a short error.code if available), call console.error or the app logger with the generic context message only (not the full error object), and pass the sanitized safeMessage to store.setError / store.setLoading(false); ensure you reference the initializeBin function and the catch that currently calls console.error('[Bin] Failed to initialize bin:', error) and store.setError(...) when making the change.
104-106: Consider sanitizing the error log for TEE enrollment failure.Line 105 logs the raw error object from the
wrapKeycall. While TEE enrollment errors are non-blocking, the error could potentially contain stack traces or internal state from the crypto operation. For consistency with the project's security posture, consider logging only a sanitized message.Suggested fix
} catch (err) { - console.error('[Bin] TEE enrollment failed (non-blocking):', err); + const reason = err instanceof Error ? err.name : 'UnknownError'; + console.error(`[Bin] TEE enrollment failed (non-blocking): ${reason}`); }As per coding guidelines, "Never log, console.log, or persist to disk sensitive data including ... key material."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/services/bin.service.ts` around lines 104 - 106, The catch block that logs TEE enrollment failures currently prints the raw error object (see the catch around the wrapKey/TEE enrollment call) which may leak sensitive crypto internals; change it to log a sanitized, non-sensitive message only (e.g., "[Bin] TEE enrollment failed (non-blocking)") and, if you need minimal diagnostic info, log a non-sensitive error indicator such as a normalized error code or a boolean flag (never stack traces, key material, or raw error objects). Update the catch in the function that calls wrapKey/handles TEE enrollment to replace the raw console.error(err) usage with this sanitized logging approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src-tauri/src/fuse/write_ops.rs`:
- Around line 348-355: The code obtains parent_ipns_name via
fs.inodes.get(parent) and may produce an empty string (""), which must be
guarded before publishing bin entries; update the unlink and rmdir handling that
use original_parent_ipns_name (and the block around parent_ipns_name resolution)
to check for empty or missing ipns_name and skip or abort publishing/restoration
entries when parent_ipns_name.is_empty(), logging a concise warning or returning
an error instead of emitting a bin record with an ambiguous target; locate uses
in the unlink/rmdir blocks and the resolution logic that matches InodeKind::Root
and InodeKind::Folder to add this guard.
---
Nitpick comments:
In `@apps/web/src/services/bin.service.ts`:
- Around line 763-786: The fallback in getRootFolder() uses
folders['root']?.children even though the function only reaches that branch when
folders['root'] is falsy; update the returned FolderNode so the children field
uses an explicit empty array instead of folders['root']?.children (e.g., set
children: []), removing the redundant reference to folders['root'] while keeping
all other fields (id, name, ipnsName, parentId, isLoaded, isLoading,
sequenceNumber, folderKey, ipnsPrivateKey) unchanged.
- Around line 166-169: The catch block in initializeBin is logging the raw error
object and passing it straight to store.setError; replace this by sanitizing the
error before logging and storing: in the initializeBin catch, create a
safeMessage (e.g., a generic "Failed to initialize bin" plus non-sensitive
error.name or a short error.code if available), call console.error or the app
logger with the generic context message only (not the full error object), and
pass the sanitized safeMessage to store.setError / store.setLoading(false);
ensure you reference the initializeBin function and the catch that currently
calls console.error('[Bin] Failed to initialize bin:', error) and
store.setError(...) when making the change.
- Around line 104-106: The catch block that logs TEE enrollment failures
currently prints the raw error object (see the catch around the wrapKey/TEE
enrollment call) which may leak sensitive crypto internals; change it to log a
sanitized, non-sensitive message only (e.g., "[Bin] TEE enrollment failed
(non-blocking)") and, if you need minimal diagnostic info, log a non-sensitive
error indicator such as a normalized error code or a boolean flag (never stack
traces, key material, or raw error objects). Update the catch in the function
that calls wrapKey/handles TEE enrollment to replace the raw console.error(err)
usage with this sanitized logging approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae04f585-09a9-411f-9d85-8271cecc6d93
📒 Files selected for processing (7)
apps/desktop/src-tauri/src/fuse/mod.rsapps/desktop/src-tauri/src/fuse/write_ops.rsapps/web/src/hooks/useFolderMutations.tsapps/web/src/services/bin.service.tsapps/web/src/stores/bin.store.tsapps/web/src/utils/format.tspackages/crypto/src/__tests__/bin.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/crypto/src/tests/bin.test.ts
- apps/web/src/utils/format.ts
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 61 out of 66 changed files in this pull request and generated 1 comment.
You can also share your feedback on Copilot code review. Take the survey.
Skip bin entry creation in unlink/rmdir when parent IPNS name is empty, instead of publishing an entry with an unrestorable target. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: c6b549522b4e
5 tasks
This was referenced Mar 21, 2026
Merged
Closed
Merged
4 tasks
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
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
packages/crypto/src/bin/): BinEntry/RecycleBinMetadata types, HKDF-derived IPNS keypair, ECIES encrypt/decrypt, schema validationGET /vault/configreturnsrecycleBinRetentionDays(default 30)apps/web/src/stores/bin.store.ts,services/bin.service.ts): Full IPNS-backed lifecycle — init, add, restore (recursive parent restore), permanent delete (CID unpin + quota), empty, auto-purge expireduseFolderMutationsdelete now callsaddToBininstead ofunpinFromIpfs; CIDs preserved for recoveryBinBrowser.tsx,BinListItem.tsx,BinEmptyState.tsx): Sidebar nav,/binroute, flat list with 5-column sortable grid (Name, Location, Deleted, Size, Time Left), context menu (Restore / Delete Permanently), multi-select, Empty Bin with confirmation, terminal-aesthetic empty statecrypto/bin.rs,write_ops.rs): Rust bin crypto module,handle_unlink/handle_rmdircreate BinEntry and publish to bin IPNS recordBug fix
Context menu outside-click handler used capture-phase
mousedownwithout.contains()check, dismissing the menu before click handlers on menu items could fire. Restore and Delete Permanently silently did nothing. Fixed by adding.contains()guard matching the existingContextMenu.tsxpattern.Verification
@cipherbox/cryptoandwebbuild successfullyTest plan
rmon FUSE mount creates bin entry (not permanent delete)pnpm exec playwright test tests/recycle-bin.spec.tspasses🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests