fix(17.1): close bin integration gaps - CID unpinning + Windows bin#268
Merged
Conversation
Milestone audit found 2 cross-phase integration gaps: - GAP-1 (CRITICAL): bin permanent delete can't unpin file CIDs (encrypted metadata parsed as plain JSON) - GAP-2 (MODERATE): Windows desktop deletes bypass recycle bin (no spawn_bin_entry_publish in WinFsp) Phase 17.1 created to close both gaps before milestone archival. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 5a0928b81dda
Phase 17.1: Recycle Bin Integration Fixes - 3 plans in 2 waves - 2 parallel (Wave 1), 1 sequential (Wave 2) - Ready for execution Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 640bd9e51e1b
Address blocker: cleanupFolderCids now unwraps folderKey and decrypts nested file metadata for folder permanent delete (option c). Split Plan 01 Task 2 into web-side (Task 2) and desktop-side (Task 3). Fix TC06 conflict (use TC07), extend existing PS1 instead of recreating, make permanent-delete-completes primary assertion over quota check. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 8feb921839ce
- Add optional contentCid (string) and contentSize (number) to TS BinEntry type - Add schema validation for new fields (non-empty string, non-negative number) - Add content_cid (Option<String>) and content_size (Option<u64>) to Rust BinEntry - Fields are optional for backward compat with existing bin entries Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 4b13d7551f79
- Port bin entry logic from macOS/Linux handle_unlink/handle_rmdir to Windows handle_cleanup - File deletes now create BinEntry with FilePointer (soft-delete, CID stays pinned) - Folder deletes now create BinEntry with FolderEntry and ECIES-wrapped IPNS key - Remove direct CID unpin on delete -- CIDs stay pinned for recovery - Add build_folder_path helper for breadcrumb path in bin entries - Fix missing content_cid/content_size fields in macOS BinEntry constructors Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: be1e3a570f81
…AP-1) - Capture contentCid/contentSize at soft-delete time in addToBin/addManyToBin by resolving and decrypting file metadata with folderKey when available - Rewrite unpinFileCids to prefer stored contentCid, avoiding re-decryption of encrypted file metadata (the root cause of GAP-1) - Rewrite cleanupFolderCids to unwrap folderKey via ECIES, decrypt folder metadata, then decrypt each nested file's metadata to extract CID for unpin - Pass userPrivateKey through cleanupEntryCids to all callers (permanentlyDelete, permanentlyDeleteBatch, emptyBin, purgeExpired) - Pass folderKey from useFolderMutations handleDelete/handleDeleteItems to addManyToBin for CID capture at soft-delete time - Legacy bin entries without contentCid fall back to IPNS resolution Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: cca5da659991
Tasks completed: 1/1 - Add bin entry creation to Windows handle_cleanup SUMMARY: .planning/phases/17.1-bin-integration-fixes/17.1-02-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: c8296833a2cd
- Destructure cid from InodeKind::File in handle_unlink - Include cid in bin_entry_data tuple for BinEntry construction - Set content_cid to Some(cid) when non-empty, None when empty - content_size already set to Some(file_size) from inode - Folder rmdir already correctly sets content_cid/content_size to None Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 77f1bc61c930
Tasks completed: 3/3 - Add contentCid/contentSize to BinEntry types and schema - Web-side fix: capture CID at soft-delete, fix unpinFileCids, fix cleanupFolderCids - Desktop-side: capture CID+size at soft-delete time SUMMARY: .planning/phases/17.1-bin-integration-fixes/17.1-01-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: fec395b80674
- TC07 verifies permanent delete from bin completes without crash - Primary assertion: item disappears (proves JSON.parse crash is fixed) - Secondary assertion: quota store is functional (best-effort) - Existing TC01-TC06 unchanged Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: e87f52f1d5b9
- Add Test 5: verify bin IPNS name exists and resolves after soft-delete - Proves GAP-2 fix (Windows bin entry creation) works end-to-end - Falls back to presence check if IPNS resolve is slow - Script already included in run-all.ps1 (Step 5) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 4d8f7b3d6615
Tasks completed: 3/3 - Task 1: Add TC07 permanent delete E2E test - Task 2: Extend Windows PowerShell bin test with GAP-2 assertion - Task 3: Build verification across all platforms SUMMARY: .planning/phases/17.1-bin-integration-fixes/17.1-03-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 76e06ee557a9
Windows handle_cleanup was destructuring InodeKind::File with `..` which skipped the `cid` field, causing all Windows file bin entries to have content_cid: None. This meant permanent delete on Windows- originated bin entries would fall through to the legacy IPNS-resolution path which hits the original GAP-1 encrypted-metadata parsing bug. Now destructures `cid` from the inode and sets content_cid on the BinEntry, matching macOS/Linux behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 7b1aecf6272f
Phase 17.1 verified: 7/7 must-haves pass. GAP-1 (CID unpinning) and GAP-2 (Windows bin bypass) both closed with E2E test coverage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 97ccfca0524e
|
Caution Review failedPull request was closed or merged during review 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:
WalkthroughAdds Phase 17.1 Recycle Bin integration fixes: extends BinEntry (contentCid/contentSize/versionCids) across TS/Rust, captures CIDs at soft-delete (desktop), publishes BinEntry on Windows deletes, updates web cleanup to unwrap/decrypt nested metadata for unpinning, and adds E2E tests and roadmap/state/audit docs. Changes
Sequence Diagram(s)sequenceDiagram
participant UserUI as Web UI
participant WebSvc as Web Bin Service
participant Crypto as Crypto Lib
participant Desktop as Desktop FUSE
participant IPFS as IPFS/IPNS
participant Storage as Backend (Pins/Quota)
rect rgba(200,200,255,0.5)
UserUI->>WebSvc: soft-delete(item, parentIpnsName, folderKey?)
WebSvc->>Crypto: unwrapKey & decrypt metadata (if folderKey) -> extract contentCid/contentSize/versionCids
WebSvc->>Storage: publish BinEntry (non-blocking) including content fields
end
rect rgba(200,255,200,0.5)
Desktop->>Desktop: on unlink/remove -> inspect inode -> build BinEntry (content_cid?, content_size?, version_cids?)
Desktop->>IPFS: optionally resolve parent IPNS
Desktop->>WebSvc: spawn_bin_entry_publish(BinEntry)
end
rect rgba(255,200,200,0.5)
WebSvc->>Storage: permanentlyDelete -> cleanupEntryCids(entry, userPrivateKey)
WebSvc->>Crypto: unwrap folderKey (if folder) -> decrypt nested metadata -> gather descendant contentCids/versionCids
WebSvc->>IPFS: unpin CIDs -> update quota
Storage->>WebSvc: confirm unpin/quota reclaim
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
FSM1
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Closes two recycle-bin integration gaps by ensuring permanent delete can reclaim IPFS storage (without attempting to parse encrypted metadata as plaintext) and by bringing Windows desktop delete behavior in line with macOS/Linux via bin entry publishing.
Changes:
- Extend
BinEntrywith optionalcontentCid/contentSizeand validate them in crypto schema. - Web: capture file content CID/size at soft-delete time; rewrite permanent-delete cleanup to use stored CIDs (files) and unwrap/decrypt metadata to discover nested CIDs (folders).
- Desktop: capture content CID for file bin entries and add WinFsp
handle_cleanupbin entry publishing; add E2E regressions for web + Windows script assertion.
Reviewed changes
Copilot reviewed 17 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/tests/recycle-bin.spec.ts | Adds TC07 regression ensuring permanent delete completes (no crash) after GAP-1 fix. |
| tests/e2e-desktop/scripts/test-recycle-bin.ps1 | Adds Windows Test 5 to assert bin IPNS presence/resolve after delete. |
| packages/crypto/src/bin/types.ts | Extends BinEntry with optional contentCid/contentSize. |
| packages/crypto/src/bin/schema.ts | Adds runtime validation for the new optional bin entry fields. |
| apps/web/src/services/bin.service.ts | Captures CID/size at soft-delete; updates permanent-delete CID cleanup for files/folders (unwrap+decrypt for folders). |
| apps/web/src/hooks/useFolderMutations.ts | Threads folderKey into bin add flows to enable soft-delete CID capture. |
| apps/desktop/src-tauri/src/fuse/write_ops.rs | Captures file content CID into bin entries on unlink; includes new optional fields in bin entry construction. |
| apps/desktop/src-tauri/src/fuse/windows/write_ops.rs | Implements bin entry creation/publish in WinFsp cleanup-delete path; adds build_folder_path helper. |
| apps/desktop/src-tauri/src/crypto/bin.rs | Extends Rust BinEntry struct with content_cid/content_size. |
| .planning/v1.0-production-MILESTONE-AUDIT.md | Adds milestone audit documenting the two integration gaps and their closure context. |
| .planning/phases/17.1-bin-integration-fixes/17.1-VERIFICATION.md | Adds verification report for phase 17.1. |
| .planning/phases/17.1-bin-integration-fixes/17.1-03-SUMMARY.md | Adds execution summary for E2E coverage plan. |
| .planning/phases/17.1-bin-integration-fixes/17.1-03-PLAN.md | Adds plan document for E2E coverage work. |
| .planning/phases/17.1-bin-integration-fixes/17.1-02-SUMMARY.md | Adds execution summary for Windows desktop bin integration plan. |
| .planning/phases/17.1-bin-integration-fixes/17.1-02-PLAN.md | Adds plan document for Windows bin integration work. |
| .planning/phases/17.1-bin-integration-fixes/17.1-01-SUMMARY.md | Adds execution summary for CID capture/unpin fix plan. |
| .planning/phases/17.1-bin-integration-fixes/17.1-01-PLAN.md | Adds plan document for CID capture/unpin fix work. |
| .planning/phases/17.1-bin-integration-fixes/.gitkeep | Keeps phase directory tracked. |
| .planning/STATE.md | Updates planning state to reflect Phase 17.1 completion. |
| .planning/ROADMAP.md | Updates roadmap to include Phase 17.1 and ordering/progress. |
You can also share your feedback on Copilot code review. Take the survey.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/e2e/tests/recycle-bin.spec.ts (1)
353-367: Best-effort quota check is appropriately guarded.The secondary assertion correctly handles the case where
window.__ZUSTAND_STORES__may not be exposed. Theanycast is necessary here since TypeScript doesn't know about runtime-injected globals in the browser context.Consider adding a type annotation to suppress the lint warning while maintaining clarity:
🔧 Suggested fix for lint warning
const quotaDecreased = await page.evaluate(() => { try { - const store = (window as any).__ZUSTAND_STORES__?.quota; + // eslint-disable-next-line `@typescript-eslint/no-explicit-any` + const store = (window as unknown as { __ZUSTAND_STORES__?: { quota?: { getState: () => { usedBytes: number } } } }).__ZUSTAND_STORES__?.quota; if (!store) return null; // Store not accessible, skip🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/recycle-bin.spec.ts` around lines 353 - 367, The lint warning comes from using (window as any) inside the page.evaluate; instead of any, add a narrow type for the injected global or augment the Window type so the runtime-injected __ZUSTAND_STORES__ is typed. Concretely, either declare a small local interface for __ZUSTAND_STORES__ (with quota store exposing getState() returning usedBytes:number) and cast via unknown→that interface inside the page.evaluate, or add a test-file global Window augmentation for __ZUSTAND_STORES__; update the code that reads (window as any).__ZUSTAND_STORES__ to use that typed reference (refer to quotaDecreased, page.evaluate, __ZUSTAND_STORES__, and store.getState).apps/web/src/services/bin.service.ts (1)
219-235: Extract duplicated file-meta resolution into one helper.The same CID+size resolution/decrypt block exists in both
addToBinandaddManyToBin. Consolidating it reduces drift risk and keeps GAP-1 behavior consistent across both paths.♻️ Suggested refactor
+async function tryAttachContentInfo( + item: FolderChild, + folderKey: Uint8Array | undefined, + entry: BinEntry +): Promise<void> { + if (item.type !== 'file' || !folderKey) return; + try { + const fp = item as FilePointer; + const resolved = await resolveIpnsRecord(fp.fileMetaIpnsName); + if (!resolved?.cid) return; + const metaBytes = await fetchFromIpfs(resolved.cid); + const encrypted = JSON.parse(new TextDecoder().decode(metaBytes)) as EncryptedFileMetadata; + const fileMeta = await decryptFileMetadata(encrypted, folderKey); + entry.contentCid = fileMeta.cid; + entry.contentSize = fileMeta.size; + } catch (err) { + console.warn('[Bin] Failed to resolve file CID for bin entry (non-blocking):', err); + } +} @@ - if (isFile && folderKey) { - ... - } + await tryAttachContentInfo(item, folderKey, entry); @@ - if (isFile && folderKey) { - ... - } + await tryAttachContentInfo(item, folderKey, entry);Also applies to: 295-311
🤖 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 219 - 235, Duplicate logic that resolves a file's CID+size (using resolveIpnsRecord, fetchFromIpfs, TextDecoder/JSON parse, decryptFileMetadata and FilePointer handling) appears in both addToBin and addManyToBin; extract that block into a single helper function (e.g., resolveAndDecryptFileMeta(fp: FilePointer, folderKey): Promise<{cid:string,size:number}|null>) and call it from both addToBin and addManyToBin, returning null on failure so the callers keep the same non-blocking behavior; ensure the helper performs the same try/catch and logging semantics and that callers assign entry.contentCid and entry.contentSize from the helper result.
🤖 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/web/src/services/bin.service.ts`:
- Around line 750-804: The function cleanupFolderCids currently only unpins
resolved.cid at the end of the main try block, so any exception
(JSON/decrypt/child processing) prevents the folder metadata CID from being
unpinned; update cleanupFolderCids to always attempt unpinning of the
resolved.cid (call unpinFromIpfs(resolved.cid)) in a finally block or ensure a
try/finally around the code that runs after resolveIpnsRecord so the folder
metadata unpin is executed regardless of downstream errors; keep existing
per-child try/catch behavior and still swallow/unlogged errors from
unpinFromIpfs as before.
---
Nitpick comments:
In `@apps/web/src/services/bin.service.ts`:
- Around line 219-235: Duplicate logic that resolves a file's CID+size (using
resolveIpnsRecord, fetchFromIpfs, TextDecoder/JSON parse, decryptFileMetadata
and FilePointer handling) appears in both addToBin and addManyToBin; extract
that block into a single helper function (e.g., resolveAndDecryptFileMeta(fp:
FilePointer, folderKey): Promise<{cid:string,size:number}|null>) and call it
from both addToBin and addManyToBin, returning null on failure so the callers
keep the same non-blocking behavior; ensure the helper performs the same
try/catch and logging semantics and that callers assign entry.contentCid and
entry.contentSize from the helper result.
In `@tests/e2e/tests/recycle-bin.spec.ts`:
- Around line 353-367: The lint warning comes from using (window as any) inside
the page.evaluate; instead of any, add a narrow type for the injected global or
augment the Window type so the runtime-injected __ZUSTAND_STORES__ is typed.
Concretely, either declare a small local interface for __ZUSTAND_STORES__ (with
quota store exposing getState() returning usedBytes:number) and cast via
unknown→that interface inside the page.evaluate, or add a test-file global
Window augmentation for __ZUSTAND_STORES__; update the code that reads (window
as any).__ZUSTAND_STORES__ to use that typed reference (refer to quotaDecreased,
page.evaluate, __ZUSTAND_STORES__, and store.getState).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b17b40a-8481-4f67-941f-7c0d830802ba
📒 Files selected for processing (20)
.planning/ROADMAP.md.planning/STATE.md.planning/phases/17.1-bin-integration-fixes/.gitkeep.planning/phases/17.1-bin-integration-fixes/17.1-01-PLAN.md.planning/phases/17.1-bin-integration-fixes/17.1-01-SUMMARY.md.planning/phases/17.1-bin-integration-fixes/17.1-02-PLAN.md.planning/phases/17.1-bin-integration-fixes/17.1-02-SUMMARY.md.planning/phases/17.1-bin-integration-fixes/17.1-03-PLAN.md.planning/phases/17.1-bin-integration-fixes/17.1-03-SUMMARY.md.planning/phases/17.1-bin-integration-fixes/17.1-VERIFICATION.md.planning/v1.0-production-MILESTONE-AUDIT.mdapps/desktop/src-tauri/src/crypto/bin.rsapps/desktop/src-tauri/src/fuse/windows/write_ops.rsapps/desktop/src-tauri/src/fuse/write_ops.rsapps/web/src/hooks/useFolderMutations.tsapps/web/src/services/bin.service.tspackages/crypto/src/bin/schema.tspackages/crypto/src/bin/types.tstests/e2e-desktop/scripts/test-recycle-bin.ps1tests/e2e/tests/recycle-bin.spec.ts
…ntry test initializers The BinEntry struct gained content_cid and content_size fields but two test initializers were not updated, causing compilation failures in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 8577aa1180f7
- Use Number.isFinite() instead of truthy check for quota size reclaim, so 0-byte files are handled correctly (lines 701, 783) - Move folder metadata CID unpin to finally block in cleanupFolderCids so it always runs even if child cleanup throws (line 804) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 95bed50a110f
Add unpinVersionCids helper that iterates fileMeta.versions and unpins each version's content CID while reclaiming quota. Applied in all three permanent-delete code paths: unpinFileCids preferred path (best-effort plaintext parse), unpinFileCids fallback path, and cleanupFolderCids (fully decrypted). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 341e40576416
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #268 +/- ##
==========================================
+ Coverage 46.31% 48.08% +1.77%
==========================================
Files 106 110 +4
Lines 8266 9046 +780
Branches 591 652 +61
==========================================
+ Hits 3828 4350 +522
- Misses 4271 4524 +253
- Partials 167 172 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
… history When uploading a file that already exists in the folder, instead of blocking with an error, a Replace dialog now offers to update the existing file (creating a version entry) or skip. This enables file versioning through the web UI. Also adds: - TC08 E2E test: versioned permanent delete reclaims all quota - 6 Rust unit tests for FileMetadata+VersionEntry serialization roundtrips Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: d0c862b7144e
…eplacements store - 11 new tests for validateBinMetadata contentCid/contentSize branches (empty string, non-string, negative, non-finite, valid, zero, combined) - Update createTestBinMetadata helper to include contentCid/contentSize - Verify contentCid/contentSize survive encrypt/decrypt round-trip - 6 new tests for upload store pendingReplacements state management Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: c6d8d20dc4bb
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
apps/desktop/src-tauri/src/crypto/tests.rs (1)
1376-1399: Extend optional-field omission assertions for new fields.This test now initializes
content_cidandcontent_sizeasNone(Line [1389]-Line [1390]) but does not assert their omission in JSON. Adding those assertions would lock in expected wire format behavior.Small coverage enhancement
let json = serde_json::to_string(&metadata).unwrap(); assert!(!json.contains("filePointer"), "None filePointer should be omitted"); assert!(!json.contains("folderEntry"), "None folderEntry should be omitted"); + assert!(!json.contains("contentCid"), "None contentCid should be omitted"); + assert!(!json.contains("contentSize"), "None contentSize should be omitted"); }🤖 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 1376 - 1399, The test function bin_metadata_optional_fields_omitted_when_none should also assert that optional fields content_cid and content_size are omitted from the serialized JSON; update the test to check that the produced json string does not contain "contentCid" and does not contain "contentSize" after serializing the bin::RecycleBinMetadata instance (the same way it already checks filePointer and folderEntry), so the wire-format omission behavior for content_cid and content_size is enforced.tests/e2e/tests/recycle-bin.spec.ts (1)
353-356: Type the test-window bridge instead of usingany.Lines 355, 406, and 436 use
(window as any).__ZUSTAND_STORES__, which removes type checks and triggers lint warnings.♻️ Proposed fix
+type QuotaStoreShape = { + getState: () => { usedBytes?: number }; +}; + // inside page.evaluate blocks -const store = (window as any).__ZUSTAND_STORES__?.quota; +const w = window as Window & { __ZUSTAND_STORES__?: { quota?: QuotaStoreShape } }; +const store = w.__ZUSTAND_STORES__?.quota;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/recycle-bin.spec.ts` around lines 353 - 356, Replace the unsafe casts to any when accessing the test bridge by declaring and using a typed window interface for __ZUSTAND_STORES__ and its quota store; replace occurrences of (window as any).__ZUSTAND_STORES__ in recycle-bin.spec.ts (around the quotaDecreased evaluation and the other two lines) with a typed access using that interface (e.g., declare global interface TestWindow { __ZUSTAND_STORES__?: { quota?: QuotaStoreType } } and then use (window as unknown as TestWindow).__ZUSTAND_STORES__) so the store variable and its properties are properly typed and lint warnings are resolved.
🤖 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/crypto/tests.rs`:
- Around line 1576-1597: The test
file_metadata_versions_camel_case_serialization is ambiguous because it only
checks top-level FileMetadata keys; update it to assert the nested VersionEntry
serialization uses camelCase by extracting or isolating the serialized versions
array (from serde_json::to_string(&metadata) or by serializing metadata.versions
directly) and then assert that the serialized versions entry contains
"fileKeyEncrypted", "fileIv", and "encryptionMode" and does NOT contain their
snake_case counterparts; reference the FileMetadata struct, the VersionEntry
type (from make_test_version_entry), and the
file_metadata_versions_camel_case_serialization test to locate where to change
the assertions.
In `@apps/web/src/components/file-browser/ReplaceFileDialog.tsx`:
- Around line 47-52: The code currently calls advance() unconditionally after
the try/catch, causing failed replacements to be treated as resolved; update the
flow so advance() only runs on successful replace: either move the advance()
call into the try block immediately after the successful updateFile(...) call
(or set a success flag and call advance() only when true). Keep
setIsLoading(false) in a finally-equivalent place so loading is cleared on both
success and failure. Ensure you reference the same identifiers (current,
updateFile, setIsLoading, advance) when making the change.
- Around line 54-66: handleSkip and handleClose currently call
unpinFromIpfs(...) but never call removeUsage, so skipped/closed items still
count against quota; update both handlers (handleSkip and the loop inside
handleClose) to also call removeUsage(...) for the same CID (e.g.,
current.encryptedData.cid and replacements[i].encryptedData.cid) after/until
unpinFromIpfs, handling errors the same way (void ... .catch(() => {})) so quota
is decremented in local state when a pin is removed. Ensure you reference
unpinFromIpfs and removeUsage for each CID and keep the existing
isLoading/advance logic intact.
In `@apps/web/src/services/bin.service.ts`:
- Around line 705-719: The current file fast-path swallows parse errors for
encrypted v2 metadata (in the try/catch around resolveIpnsRecord → fetchFromIpfs
→ JSON.parse → unpinVersionCids), leaving historical version CIDs pinned; update
the catch block so that when plaintext parse fails you fall back to the
folder-based cleanup path by invoking the existing cleanupFolderCids flow (pass
the file pointer or its IPNS name/identifier as appropriate) so version CID
unpinning still occurs for encrypted v2 entries; reference resolveIpnsRecord,
fetchFromIpfs, unpinVersionCids, and cleanupFolderCids to locate and modify the
code.
In `@tests/e2e/tests/recycle-bin.spec.ts`:
- Around line 431-446: The test currently only asserts quotaAfter < quotaBefore
which allows partial reclaim; update the assertion to verify the full expected
reclaim by computing the expected freed bytes and asserting quotaAfter equals
(or is <= within a small tolerance) quotaBefore - expectedFreed. Locate the
quota reads around the quotaBefore and quotaAfter variables and the
page.evaluate that reads store.getState().usedBytes; compute expectedFreedBytes
from the known version CIDs/sizes produced earlier in this spec (or capture the
size when creating versions) and replace
expect(quotaAfter).toBeLessThan(quotaBefore) with an assertion like
expect(quotaAfter).toBe(quotaBefore - expectedFreedBytes) or
expect(quotaAfter).toBeLessThanOrEqual(quotaBefore - expectedFreedBytes +
tolerance) to ensure “reclaims all quota.”
---
Nitpick comments:
In `@apps/desktop/src-tauri/src/crypto/tests.rs`:
- Around line 1376-1399: The test function
bin_metadata_optional_fields_omitted_when_none should also assert that optional
fields content_cid and content_size are omitted from the serialized JSON; update
the test to check that the produced json string does not contain "contentCid"
and does not contain "contentSize" after serializing the bin::RecycleBinMetadata
instance (the same way it already checks filePointer and folderEntry), so the
wire-format omission behavior for content_cid and content_size is enforced.
In `@tests/e2e/tests/recycle-bin.spec.ts`:
- Around line 353-356: Replace the unsafe casts to any when accessing the test
bridge by declaring and using a typed window interface for __ZUSTAND_STORES__
and its quota store; replace occurrences of (window as any).__ZUSTAND_STORES__
in recycle-bin.spec.ts (around the quotaDecreased evaluation and the other two
lines) with a typed access using that interface (e.g., declare global interface
TestWindow { __ZUSTAND_STORES__?: { quota?: QuotaStoreType } } and then use
(window as unknown as TestWindow).__ZUSTAND_STORES__) so the store variable and
its properties are properly typed and lint warnings are resolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8800288b-e188-43a5-a7f0-bbbb62c06b2d
📒 Files selected for processing (7)
apps/desktop/src-tauri/src/crypto/tests.rsapps/web/src/components/file-browser/ReplaceFileDialog.tsxapps/web/src/components/file-browser/UploadZone.tsxapps/web/src/hooks/useDropUpload.tsapps/web/src/services/bin.service.tsapps/web/src/stores/upload.store.tstests/e2e/tests/recycle-bin.spec.ts
- Move advance() inside try block so failed replacements don't skip ahead - Add quota removeUsage() on skip/close to reclaim orphaned upload quota - Tighten VersionEntry camelCase test to check versions[0] directly Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 621949dc59a9
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/crypto/src/__tests__/bin.test.ts (1)
528-748: Consider table-driven tests to reduce repetition.Coverage is solid, but this matrix is verbose. Converting repeated cases to
it.eachwould keep intent clearer and reduce maintenance overhead.♻️ Optional refactor sketch
+ it.each([ + ['valid contentCid', { contentCid: 'bafybeicontent123' }, true], + ['empty contentCid', { contentCid: '' }, false], + ['non-string contentCid', { contentCid: 12345 }, false], + ['valid contentSize', { contentSize: 2048 }, true], + ['zero contentSize', { contentSize: 0 }, true], + ['negative contentSize', { contentSize: -1 }, false], + ['non-number contentSize', { contentSize: '2048' }, false], + ['non-finite contentSize', { contentSize: Infinity }, false], + ['both contentCid/contentSize', { contentCid: 'bafybeicontent123', contentSize: 2048 }, true], + ])('content field validation: %s', (_, patch, shouldPass) => { + const candidate = { + version: 'v1', + sequenceNumber: 1, + entries: [{ + id: 'abc', + itemType: 'file', + name: 'test.txt', + originalParentIpnsName: 'k51xxx', + originalPath: '/test.txt', + deletedAt: Date.now(), + size: 100, + mimeType: 'text/plain', + ...patch, + }], + }; + if (shouldPass) expect(() => validateBinMetadata(candidate)).not.toThrow(); + else expect(() => validateBinMetadata(candidate)).toThrow('Invalid bin metadata format'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/crypto/src/__tests__/bin.test.ts` around lines 528 - 748, Tests for validateBinMetadata are repetitive across contentCid and contentSize cases; refactor by consolidating similar scenarios into parameterized table-driven tests using Jest's it.each (or describe.each) to iterate input variants and expected outcomes. Identify the blocks that call validateBinMetadata with differing entry payloads (the individual it(...) cases in the bin.test.ts file) and replace groups of repetitive it() assertions for contentCid and contentSize with a single it.each table that supplies the entry variant and whether it should throw, keeping the same expectation logic (calling validateBinMetadata and asserting .not.toThrow() or .toThrow('Invalid bin metadata format')). Ensure unique identifiers like validateBinMetadata and the test case descriptions are preserved or adapted so failures remain clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/crypto/src/__tests__/bin.test.ts`:
- Around line 528-748: Tests for validateBinMetadata are repetitive across
contentCid and contentSize cases; refactor by consolidating similar scenarios
into parameterized table-driven tests using Jest's it.each (or describe.each) to
iterate input variants and expected outcomes. Identify the blocks that call
validateBinMetadata with differing entry payloads (the individual it(...) cases
in the bin.test.ts file) and replace groups of repetitive it() assertions for
contentCid and contentSize with a single it.each table that supplies the entry
variant and whether it should throw, keeping the same expectation logic (calling
validateBinMetadata and asserting .not.toThrow() or .toThrow('Invalid bin
metadata format')). Ensure unique identifiers like validateBinMetadata and the
test case descriptions are preserved or adapted so failures remain clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c9262a71-f2a6-4794-98df-94e57e5621b6
📒 Files selected for processing (2)
apps/web/src/stores/__tests__/upload-error-recovery.test.tspackages/crypto/src/__tests__/bin.test.ts
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 27 changed files in this pull request and generated 5 comments.
You can also share your feedback on Copilot code review. Take the survey.
…e, fix 0-byte quota check - Forward encryptionMode from PendingReplacement through updateFile to metadata - On failed replace, unpin orphaned CID and reclaim quota before advancing - Use Number.isFinite instead of truthy check for legacy 0-byte file quota reclaim Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 0e3cad131bd4
…im on permanent delete Previously, permanent delete of versioned files could not unpin historical version CIDs because file metadata is AES-GCM encrypted and the folderKey is no longer available at permanent-delete time. Now captures version CIDs and sizes into BinEntry.versionCids at soft-delete time (when folderKey is still available), enabling full storage reclaim on permanent delete without re-decryption. - Add versionCids field to BinEntry (TS + Rust) with schema validation - Capture version data from decrypted file metadata during soft-delete - Use stored versionCids for unpin + quota reclaim on permanent delete - Backward compatible: old entries without versionCids skip version cleanup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 9158dd39fe5a
…ecrease Assert that quota reclaimed exceeds the v2 content size alone, proving versionCids cleanup also ran (v1 content was unpinned too). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: c5c13b1522cf
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src-tauri/src/fuse/write_ops.rs (1)
275-280:⚠️ Potential issue | 🟠 MajorCapture file version metadata when building bin entries.
version_cidsis hard-coded toNoneon unlink, so files with retained versions lose cleanup metadata at soft-delete. Permanent delete then cannot unpin those historical version CIDs.💡 Proposed fix
- let bin_entry_data = match fs.inodes.get(child_ino) { + let bin_entry_data = match fs.inodes.get(child_ino) { Some(inode) => match &inode.kind { InodeKind::File { file_meta_ipns_name, file_ipns_key_encrypted_hex, size, cid, + versions, .. } => { @@ - Some((inode.name.clone(), *size, file_pointer, cid.clone())) + let version_cids = versions.as_ref().and_then(|items| { + let mapped: Vec<crate::crypto::bin::VersionCidEntry> = items + .iter() + .filter(|v| !v.cid.is_empty()) + .map(|v| crate::crypto::bin::VersionCidEntry { + cid: v.cid.clone(), + size: v.size, + }) + .collect(); + if mapped.is_empty() { None } else { Some(mapped) } + }); + Some((inode.name.clone(), *size, file_pointer, cid.clone(), version_cids)) } else { None } @@ - if let Some((item_name, file_size, file_pointer, content_cid)) = bin_entry_data { + if let Some((item_name, file_size, file_pointer, content_cid, version_cids)) = bin_entry_data { @@ - version_cids: None, + version_cids, file_pointer: Some(file_pointer), folder_entry: None, };Also applies to: 316-316, 348-380
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src-tauri/src/fuse/write_ops.rs` around lines 275 - 280, When constructing the "bin" (soft-delete) entries for InodeKind::File during unlink, you're currently setting version_cids to None which discards retained version metadata; update the unlink/bin-entry creation code to extract and include the file's version CIDs instead of hard-coding None. Locate the branch that matches InodeKind::File (fields file_meta_ipns_name, file_ipns_key_encrypted_hex, size, cid) used when building bin entries and populate version_cids from the inode's version metadata (the same source used elsewhere for version cleanup) so permanent delete can later unpin historical version CIDs. Ensure all other instances noted (around the other similar blocks) are updated the same way.
🧹 Nitpick comments (1)
apps/desktop/src-tauri/src/crypto/tests.rs (1)
1671-1717: Add roundtrip assertions forversion_cidsin the new bin-entry test.The new test validates
content_cid/content_size, butversion_cidsis still unverified and can regress silently.💡 Suggested test tightening
fn bin_entry_content_cid_roundtrip() { @@ - version_cids: None, + version_cids: Some(vec![ + bin::VersionCidEntry { + cid: "bafyversioncid1".to_string(), + size: 111, + }, + bin::VersionCidEntry { + cid: "bafyversioncid2".to_string(), + size: 222, + }, + ]), @@ assert_eq!( entry.content_size, Some(12345), "content_size should survive encrypt/decrypt roundtrip" ); + let version_cids = entry + .version_cids + .as_ref() + .expect("version_cids should survive encrypt/decrypt roundtrip"); + assert_eq!(version_cids.len(), 2); + assert_eq!(version_cids[0].cid, "bafyversioncid1"); + assert_eq!(version_cids[0].size, 111); + assert_eq!(version_cids[1].cid, "bafyversioncid2"); + assert_eq!(version_cids[1].size, 222); }🤖 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 1671 - 1717, The test bin_entry_content_cid_roundtrip currently omits checks for version_cids; add a non-empty version_cids value to the sample bin::BinEntry (e.g. Some(vec!["some-version-cid".to_string()])) and then assert that decrypted.entries[0].version_cids equals that same Some(vec![...]) to ensure version_cids survives encrypt/decrypt roundtrip; update both the metadata construction (bin::BinEntry.version_cids) and add an assert_eq! for entry.version_cids in the test.
🤖 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/windows/write_ops.rs`:
- Around line 502-543: The file bin entry code never captures the file's
version_cids, so soft-deleted Windows bin entries lose version-CID metadata;
modify the pattern for InodeKind::File in the bin collection logic to bind and
clone the version_cids field (e.g., add version_cids to the match:
InodeKind::File { file_meta_ipns_name, file_ipns_key_encrypted_hex, size, cid,
version_cids, .. }) and include that cloned version_cids in the tuple returned
for bin entries (update the bin_file_data tuple type and all call sites
accordingly, and apply the same change in the other block referenced around the
other occurrence).
In `@apps/web/src/services/bin.service.ts`:
- Around line 219-231: The single-item soft-delete path in addToBin resolves
file metadata but only sets entry.contentCid/contentSize, so historical version
CIDs get missed by unpinFileCids on permanent delete; update the logic in the
addToBin branch (the block that resolves fp via resolveIpnsRecord,
fetchFromIpfs, decryptFileMetadata) to also copy the file's version Cids into
entry.versionCids (e.g., entry.versionCids = fileMeta.versionCids || []) so that
unpinFileCids can iterate and reclaim historical versions the same way
addManyToBin does.
---
Outside diff comments:
In `@apps/desktop/src-tauri/src/fuse/write_ops.rs`:
- Around line 275-280: When constructing the "bin" (soft-delete) entries for
InodeKind::File during unlink, you're currently setting version_cids to None
which discards retained version metadata; update the unlink/bin-entry creation
code to extract and include the file's version CIDs instead of hard-coding None.
Locate the branch that matches InodeKind::File (fields file_meta_ipns_name,
file_ipns_key_encrypted_hex, size, cid) used when building bin entries and
populate version_cids from the inode's version metadata (the same source used
elsewhere for version cleanup) so permanent delete can later unpin historical
version CIDs. Ensure all other instances noted (around the other similar blocks)
are updated the same way.
---
Nitpick comments:
In `@apps/desktop/src-tauri/src/crypto/tests.rs`:
- Around line 1671-1717: The test bin_entry_content_cid_roundtrip currently
omits checks for version_cids; add a non-empty version_cids value to the sample
bin::BinEntry (e.g. Some(vec!["some-version-cid".to_string()])) and then assert
that decrypted.entries[0].version_cids equals that same Some(vec![...]) to
ensure version_cids survives encrypt/decrypt roundtrip; update both the metadata
construction (bin::BinEntry.version_cids) and add an assert_eq! for
entry.version_cids in the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ef31fc36-30d9-4bff-8e4d-001cc91baeb9
📒 Files selected for processing (10)
apps/desktop/src-tauri/src/crypto/bin.rsapps/desktop/src-tauri/src/crypto/tests.rsapps/desktop/src-tauri/src/fuse/windows/write_ops.rsapps/desktop/src-tauri/src/fuse/write_ops.rsapps/web/src/components/file-browser/ReplaceFileDialog.tsxapps/web/src/hooks/useFileOperations.tsapps/web/src/services/bin.service.tsapps/web/src/services/file-metadata.service.tspackages/crypto/src/bin/schema.tspackages/crypto/src/bin/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/crypto/src/bin/types.ts
…E paths - addToBin (single-item web path) now captures versionCids like addManyToBin - macOS FUSE unlink extracts versions from inode for bin entry - Windows FUSE cleanup_delete extracts versions from inode for bin entry Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 7cf06b4f94dd
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 29 changed files in this pull request and generated 4 comments.
You can also share your feedback on Copilot code review. Take the survey.
- Detect file-folder name collisions in useDropUpload before uploading - Populate entry.size and entry.mimeType from decrypted fileMeta in addToBin/addManyToBin - Fix inaccurate comment about JSON.parse behavior on encrypted metadata Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: f851ca343187
…ite_ops and_then already returns Option<Vec<VersionCidEntry>>, so the extra .flatten() call was a compile error on Windows (not an iterator). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 91fffa527be9
Extract shared build_folder_path and versions_to_bin_entries into fuse/helpers.rs (were identical in macOS and Windows write_ops). Extract enrichBinEntryWithFileMetadata in bin.service.ts to replace duplicated CID enrichment blocks in addToBin/addManyToBin, reusing existing resolveFileMetadata utility. Consolidate inline version CID unpin with existing unpinVersionCids helper. Compute now_ms once in Windows cleanup-delete path instead of three separate SystemTime calls. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: efc89275ac53
This was referenced Mar 28, 2026
Merged
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
contentCidandcontentSizecaptured at soft-delete time,unpinFileCidsrewritten to use stored CID instead of parsing encrypted metadata as plain JSONhandle_cleanup, achieving parity with macOS/LinuxChanges
Crypto types (
packages/crypto)BinEntryextended with optionalcontentCid(string) andcontentSize(number)Web app (
apps/web)bin.service.ts:unpinFileCidsuses storedcontentCid;cleanupFolderCidsunwraps folder key via ECIES to decrypt nested file metadata for CID extraction;addToBin/addManyToBincapture CID at soft-delete timeuseFolderMutations.ts: passesfolderKeytoaddManyToBinin delete flowsDesktop (
apps/desktop)BinEntrystruct: addedcontent_cid: Option<String>,content_size: Option<u64>write_ops.rs: capturescidfrom inode inhandle_unlinkbin entrywrite_ops.rs: full bin entry creation inhandle_cleanup(file + folder), capturescidfrom inodeE2E tests
recycle-bin.spec.ts: TC07 permanent delete regression testtest-recycle-bin.ps1: Test 5 Windows bin IPNS verificationVerification
Phase verified 7/7 must-haves — see
.planning/phases/17.1-bin-integration-fixes/17.1-VERIFICATION.mdTest plan
pnpm test— crypto package (274 tests)pnpm --filter web build— web app buildscargo check --features fuse— desktop compiles (macOS/Linux)cargo check --features winfsp— desktop compiles (Windows CI)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation