feat(14): user-to-user encrypted sharing#183
Merged
Merged
Conversation
Phase 14: User-to-User Sharing - Implementation decisions documented - Phase boundary established - Design direction: terminal command style (Option A) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: fbd4105f9c83
Four screens added to Pencil design file: - P14.1: Context menu with Share item (@ icon) - P14.2: Share dialog modal (terminal command style) - P14.3: Shared With Me view (~/shared, [RO] badges) - P14.4: Settings public key section (--copy button) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: c644f3afc869
Phase 14: User-to-User Sharing - ECIES re-wrapping protocol documented with sequence diagrams - Share record storage architecture (shares + share_keys tables) - Recipient discovery via API polling - Lazy key rotation protocol on revocation - File-level vs folder-level sharing analysis - "Shared with me" browsing architecture - Common pitfalls and risk assessment catalogued - Recommended plan structure outlined Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 4eba54210b34
Research covers ECIES re-wrapping protocol, share record storage architecture, lazy key rotation, and recipient browsing flow. Plans structured in 4 waves: crypto+entities → backend API → frontend store+dialog → browsing+revocation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 73f910f63661
- Create reWrapKey() that unwraps with owner key and re-wraps with recipient key - Zero plaintext key from memory after re-wrapping - Add KEY_REWRAP_FAILED error code to CryptoErrorCode type - Export reWrapKey from @cipherbox/crypto package - 4 test vectors: round-trip, multi-recipient, wrong key, invalid pubkey Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 9c367ec4d286
- Share entity with unique constraint on (sharer, recipient, ipnsName) - ShareKey entity with CASCADE delete on share removal - Soft-delete revokedAt for lazy key rotation pattern - hiddenByRecipient for recipient to dismiss unwanted shares - itemName plaintext field for display in "Shared with me" - Both entities follow existing FolderIpns entity pattern Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 21f9857aa519
Tasks completed: 2/2 - Create reWrapKey crypto utility with tests - Create Share and ShareKey TypeORM entities SUMMARY: .planning/phases/14-user-to-user-sharing/14-01-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 42ecfa22f6a4
- CreateShareDto with class-validator decorations for share creation - AddShareKeysDto for adding re-wrapped keys to existing shares - SharesService with 10 methods: createShare, getReceivedShares, getSentShares, getShareKeys, addShareKeys, revokeShare, hideShare, lookupUserByPublicKey, getPendingRotations, completeRotation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 9948cc3c8fe5
…egenerate API client - SharesController with 8 JWT-authenticated endpoints: create, received, sent, lookup, getKeys, addKeys, revoke, hide - SharesModule registered in app.module with Share/ShareKey entities - OpenAPI generator script updated with shares controller/service - API client regenerated with typed share endpoints and model DTOs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 6ef1cf2733da
Tasks completed: 2/2 - Create DTOs and shares service - Create shares controller, module, register in app.module, regenerate API client SUMMARY: .planning/phases/14-user-to-user-sharing/14-02-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: b8af9ad1ecb7
- Zustand share store with received/sent shares, loading state, and CRUD actions - Share service wrapping generated Orval API client for all 8 share endpoints - Type-safe mapping between API responses and frontend domain types - getSentSharesForItem with 30s cache for share dialog lookups Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: ab01c45548e2
- ShareDialog with pubkey input, validation, and share creation - Folder sharing traverses descendants and re-wraps keys with progress - File sharing resolves metadata and re-wraps file key for recipient - Recipient list with truncated pubkeys and inline revoke confirm - Root folder sharing prevention - Terminal aesthetic with share-dialog.css styles Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 046f4c1b3b1e
- Display user's secp256k1 public key in bordered monospace box - Copy-to-clipboard button with 2s feedback (--copy / --copied) - Terminal aesthetic: // comment headers, -- command style button - Conditional render: only shows when vaultKeypair is available - CSS with hover/focus-visible styles for accessibility Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 94603816a1a6
- Add onShare prop to ContextMenu with @ icon between Move and Details - Add handleShare handler that closes menu and triggers callback - Add shareItem state and handleShareClick in FileBrowser - Render ShareDialog with item context (folderKey, ipnsName, parentFolderId) - Share not shown in multi-select context menu (individual sharing only) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 696e5ec9a42a
Tasks completed: 2/2 - Create share store and share service - Add public key display section to Settings page SUMMARY: .planning/phases/14-user-to-user-sharing/14-03-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 899fee325ca9
Tasks completed: 2/2 - Create ShareDialog modal component - Add Share action to ContextMenu and wire in FileBrowser SUMMARY: .planning/phases/14-user-to-user-sharing/14-04-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 6b85002884cf
…onent - useSharedNavigation hook with folder browsing, file download, and hide actions - SharedFileBrowser renders top-level shared list with SHARED BY column and [RO] badges - ContextMenu gains readOnly and onHide props for read-only shared context - share.service.ts: add reWrapForRecipients for post-upload/create key propagation - useFolder.ts: wire fire-and-forget re-wrapping on file upload and subfolder create Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 69f086a8c326
…ntext menu - SharedPage route at /shared with auth guard matching FilesPage pattern - AppSidebar: add Shared nav item between Files and Settings - NavItem: extend icon types with 'shared' (link emoji) - Barrel export SharedFileBrowser from file-browser/index.ts - API: add pending-rotations endpoint and completeRotation auth guard - Regenerate API client for new endpoint Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 535642eb002d
- Restore rotation-related imports in share.service.ts (used by lazy rotation code) - Remove premature checkPendingRotation/executeLazyRotation import from folder.service.ts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 59da6cd5ca36
…KeyDto - folder.service.ts: add checkAndRotateIfNeeded with dynamic import to avoid circular deps - API controller: use UpdateEncryptedKeyDto for type-safe body validation - Add UpdateEncryptedKeyDto and regenerate API client - Preparation for Plan 14-06 lazy rotation execution Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 6ecfe2627c48
Tasks completed: 2/2 - Task 1: Post-upload and post-create share key propagation - Task 2: Lazy key rotation on folder modification after revoke SUMMARY: .planning/phases/14-user-to-user-sharing/14-06-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: eb03d42b3142
Tasks completed: 2/2 - Create useSharedNavigation hook and SharedFileBrowser component - Register shared route, add sidebar nav, and read-only ContextMenu SUMMARY: .planning/phases/14-user-to-user-sharing/14-05-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 59c79b34eb12
Phase 14 verified (5/5 must-haves passed). SHARE-01 through SHARE-05 requirements marked Complete. SHARE-02/03 rescoped per research: public key paste (no email invite) and instant share (no accept/decline). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 9389be54f2c7
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 8841524be31f
H1: Re-wrap file keys during folder sharing (collectChildKeys was skipping files) H2: Add DTO input validation (hex format, UUID, secp256k1 pubkey, length limits) H3: Remove user ID exposure from lookup endpoint (return boolean only) H4: Replace absolute unique constraint with partial index (WHERE revoked_at IS NULL) M2: Document zeroing contract on reWrapForRecipients M3: Zero folder keys on shared navigation (navigateToRoot, navigateUp) M4: Add 60s TTL to share keys cache L2: Remove dead fallback code in useSharedNavigation L3: Fix child count in ShareDialog progress (count files + folders) L4: Clean up revoked share records before re-sharing Also adds 7 new security test cases for ECIES re-wrapping and creates migration for partial unique index on shares table. Deferred: M1 (key zeroing audit), M5 (public key format validation), L1 (error handler masking), L4 (server-side child key limit) — tracked in GSD todo. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 6abe57e9ad35
|
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:
WalkthroughPhase 14 adds user-to-user sharing: ECIES re-wrap primitive and tests; Share and ShareKey entities plus partial-unique-index migration; full Shares API and OpenAPI/client; frontend store/service, ShareDialog, Shared (read-only) browser, post-upload re-wrapping and lazy rotation; E2E tests and security/planning docs. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Sharer)
participant UI as ShareDialog UI
participant API as Shares API
participant Svc as SharesService
participant DB as Database
User->>UI: paste recipient public key
UI->>API: GET /shares/lookup?publicKey=X
API->>Svc: lookupUserByPublicKey(X)
Svc->>DB: SELECT user WHERE public_key = X
DB-->>Svc: user row
Svc-->>API: { exists: true }
API-->>UI: recipient exists
UI->>UI: unwrap owner key, re-wrap per-recipient (collect child keys)
UI->>API: POST /shares (CreateShareDto + childKeys)
API->>Svc: createShare(sharerId, dto)
Svc->>DB: INSERT share, INSERT share_keys
DB-->>Svc: persisted
Svc-->>API: 201 { shareId }
API-->>UI: Success
sequenceDiagram
participant Recipient as User (Recipient)
participant Browser as SharedFileBrowser
participant Nav as useSharedNavigation
participant API as API Client
participant Crypto as Crypto Utils
participant IPFS as IPFS/Metadata
Recipient->>Browser: open /shared
Browser->>Nav: init()
Nav->>API: GET /shares/received
API-->>Nav: [ReceivedShare(encryptedKey...)]
Nav->>Crypto: unwrapKey(encryptedKey, vaultPrivateKey)
Crypto-->>Nav: folderKey
Nav->>IPFS: resolve IPNS + fetch metadata
IPFS-->>Nav: folder contents
Nav-->>Browser: render shared folder (read-only)
Browser->>Nav: download file
Nav->>API: GET /shares/{shareId}/keys
API-->>Nav: [ShareKey(encryptedKey)]
Nav->>Crypto: unwrapKey(reWrappedFileKey, vaultPrivateKey)
Crypto-->>Nav: fileKey
Nav->>IPFS: fetch + decrypt file content
IPFS-->>Nav: decrypted file
Nav-->>Browser: start download
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)
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 |
Key learnings: file keys silently skipped during sharing, soft-delete vs unique constraints, lookup endpoint info leakage, cache TTL patterns, key zeroing in navigation stacks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 2ae68ec6f7e8
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/components/file-browser/ContextMenu.tsx (1)
233-279:⚠️ Potential issue | 🟡 MinorAdd
readOnlyguard to multi-select batch Move and Delete actions for consistency with single-select mode.In multi-select mode,
onBatchMoveandonBatchDeletebuttons render without checkingreadOnly, whereas single-select Rename, Move, Share, and Delete all gate on!readOnly. This creates an inconsistency: if a caller setsreadOnly={true}with batch action handlers, destructive operations would still appear. While SharedFileBrowser currently hardcodesselectedCount={1}to avoid this, the component should be defensively consistent.Apply
!readOnlychecks to batch Move and Delete (and the divider between them):{/* Batch Move */} -{onBatchMove && ( +{!readOnly && onBatchMove && ( <button ...> Move to... </button> )} {/* Divider before destructive action */} -<div className="context-menu-divider" role="separator" /> +{!readOnly && <div className="context-menu-divider" role="separator" />} {/* Batch Delete */} -{onBatchDelete && ( +{!readOnly && onBatchDelete && ( <button ... className="context-menu-item context-menu-item--destructive" ...> Delete {selectedCount} items </button> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/ContextMenu.tsx` around lines 233 - 279, The multi-select UI renders batch Move and Delete (and the divider) without respecting the readOnly prop; update the conditional rendering so Move and Delete and the preceding divider only render when !readOnly (in addition to existing onBatchMove/onBatchDelete checks). Specifically, in the isMultiSelect block gate the Move button (currently rendered when onBatchMove) with onBatchMove && !readOnly (or equivalent) and gate the destructive divider and Delete button (currently rendered when onBatchDelete) with onBatchDelete && !readOnly so handleBatchMove/handleBatchDelete and the "Delete {selectedCount} items" control are hidden in readOnly mode.
🟡 Minor comments (13)
apps/api/src/shares/dto/update-encrypted-key.dto.ts-8-12 (1)
8-12:⚠️ Potential issue | 🟡 Minor
MinLength(2)is too permissive for ECIES ciphertext — tighten the floorA secp256k1 ECIES-wrapped 32-byte key has a hard minimum size of roughly:
- Uncompressed ephemeral pubkey: 65 B → 130 hex chars
- AES-256-GCM ciphertext (32 B key) + auth tag (16 B): → 96 B → 192 hex chars
- Total ≥ ~226 hex chars
MinLength(2)allows storing 1-byte hex strings that can never be valid wrapped key material, enabling accidental or deliberate DB pollution that will silently break key rotation for the recipient. As per coding guidelines, the ECIES secp256k1 primitive must be used for key wrapping; aligning the validation floor with the actual format makes the constraint self-documenting.🛡️ Proposed fix
- `@MinLength`(2) + `@MinLength`(220) `@MaxLength`(1024)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/dto/update-encrypted-key.dto.ts` around lines 8 - 12, The MinLength(2) on the encryptedKey DTO is too permissive for secp256k1 ECIES-wrapped 32-byte keys; update the decorator on encryptedKey (the property with `@IsString`(), `@Matches`(...), `@MinLength`(...), `@MaxLength`(...)) to enforce a hex-length floor matching the ECIES format (use MinLength(226) to reflect the ~226 hex-char minimum for uncompressed ephemeral pubkey + AES-256-GCM ciphertext + tag) so invalid/too-short hex values are rejected at validation time.apps/web/src/App.css-1059-1062 (1)
1059-1062:⚠️ Potential issue | 🟡 Minor
var(--color-bg)is undefined — usevar(--color-black)insteadThe custom property
--color-bgis not defined anywhere in the project. Line 1061 is the only use of this property in the codebase, which will result in an unset color value. All other inverted-button hover states inApp.css(.login-button, etc.) usevar(--color-black)for text on a green background. This should match that pattern.🐛 Proposed fix
.settings-pubkey-copy:hover { background: var(--color-green-primary); - color: var(--color-bg); + color: var(--color-black); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/App.css` around lines 1059 - 1062, The hover rule for .settings-pubkey-copy uses an undefined custom property var(--color-bg); update the CSS selector .settings-pubkey-copy:hover to use the existing var(--color-black) for the text color (matching other inverted-button hover states like .login-button) so the hover text color is defined and consistent.apps/web/src/routes/SharedPage.tsx-22-28 (1)
22-28:⚠️ Potential issue | 🟡 MinorLoading state missing
role="status"for screen readers.The loading div is visually presented but invisible to assistive technology. Add
role="status"(and optionallyaria-live="polite") so screen readers can announce the loading state.♿ Proposed fix
- <div className="loading">Loading...</div> + <div className="loading" role="status" aria-live="polite">Loading...</div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/SharedPage.tsx` around lines 22 - 28, The loading indicator returned in SharedPage (the isLoading branch that renders <AppShell> with <div className="loading">Loading...</div>) lacks accessibility semantics; update that div to include role="status" and aria-live="polite" (or aria-live="assertive" if immediate announcement is required) so screen readers will announce the loading state when SharedPage is checking authentication.apps/web/src/routes/SettingsPage.tsx-50-57 (1)
50-57:⚠️ Potential issue | 🟡 MinorMissing error handler for
navigator.clipboard.writeText.If the clipboard API is unavailable or the write is denied (e.g., non-secure context, iframe sandbox), this produces an unhandled promise rejection. Add a
.catch()or useasync/awaitwith try/catch.🐛 Proposed fix
const handleCopyPublicKey = useCallback(() => { if (!publicKeyHex) return; - navigator.clipboard.writeText(publicKeyHex).then(() => { - setCopied(true); - if (copyTimeoutRef.current) clearTimeout(copyTimeoutRef.current); - copyTimeoutRef.current = setTimeout(() => setCopied(false), 2000); - }); + navigator.clipboard.writeText(publicKeyHex).then(() => { + setCopied(true); + if (copyTimeoutRef.current) clearTimeout(copyTimeoutRef.current); + copyTimeoutRef.current = setTimeout(() => setCopied(false), 2000); + }).catch(() => { + // Silently fail or show user feedback + }); }, [publicKeyHex]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/SettingsPage.tsx` around lines 50 - 57, The handler handleCopyPublicKey currently calls navigator.clipboard.writeText without handling failures; update it to catch promise rejections (either add .catch(...) to the writeText promise or convert the function to async and wrap await navigator.clipboard.writeText(...) in try/catch) and in the catch block reset any UI state as needed and surface or log the error; keep use of copyTimeoutRef and setCopied the same on success, but ensure failures do not produce unhandled promise rejections (e.g., call console.error or show a toast/notification on error).apps/web/src/components/file-browser/SharedFileBrowser.tsx-364-371 (1)
364-371:⚠️ Potential issue | 🟡 MinorCalling
navigateUp()in a loop may produce incorrect intermediate states.If the user clicks a breadcrumb two levels up, this calls
navigateUp()twice. Each call reads from the nav stack and sets React state. Even in React's batched update mode, each invocation reads the state as it was before the handler started — the secondnavigateUp()won't see the result of the first. The result depends on whethernavigateUpuses a ref-based stack (safe) or React state (unsafe).Consider adding a
navigateToBreadcrumb(index)function touseSharedNavigationthat pops the stack to the desired level in a single operation.Proposed approach
In
useSharedNavigation, add a dedicated function:const navigateToBreadcrumb = useCallback((targetIndex: number) => { // Pop the nav stack to targetIndex in one operation const popsNeeded = navStackRef.current.length - targetIndex; for (let i = 0; i < popsNeeded; i++) { navStackRef.current.pop(); } // Restore state from the target level in a single setState call const target = navStackRef.current[navStackRef.current.length - 1]; // ... set folderChildren, folderKey, breadcrumbs in one update }, []);Then in the breadcrumb click:
- const crumbIndex = breadcrumbs.indexOf(crumb); - const popsNeeded = breadcrumbs.length - 1 - crumbIndex; - for (let i = 0; i < popsNeeded; i++) { - navigateUp(); - } + navigateToBreadcrumb(breadcrumbs.indexOf(crumb));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/SharedFileBrowser.tsx` around lines 364 - 371, The breadcrumb onClick loops calling navigateUp() which may read stale React state; instead add a new method navigateToBreadcrumb(targetIndex) inside useSharedNavigation that mutates the navigation stack in one operation (use navStackRef.current to pop to targetIndex), then derive the target entry and update folderChildren, folderKey and breadcrumbs in a single state update/callback; replace the loop in the breadcrumb onClick with a single call to navigateToBreadcrumb(crumbIndex) so the stack and UI move to the desired level atomically.apps/web/src/components/file-browser/ContextMenu.tsx-47-50 (1)
47-50:⚠️ Potential issue | 🟡 Minor
readOnlyJSDoc is inaccurate — Edit is not hidden in read-only mode.The prop description says
readOnlyhides "Edit" actions, but lines 296–307 show the Edit button is gated only byisFile && onEdit, with no!readOnlycheck. If read-only mode is intentionally permissive for Edit (since editing is gated by the callback), update the comment to match actual behaviour.📝 Suggested correction
- /** Read-only mode: hides Rename, Move, Delete, Edit, Share actions */ + /** Read-only mode: hides Rename, Move, Delete, and Share actions (Download, Edit, Preview remain available) */ readOnly?: boolean;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/ContextMenu.tsx` around lines 47 - 50, The JSDoc for the readOnly prop is inaccurate: the Edit action is currently rendered based on the render condition isFile && onEdit inside the ContextMenu component and is not gated by readOnly; update the readOnly comment to state that Rename/Move/Delete/Share are hidden but Edit remains available when an onEdit callback is provided, or alternatively, if you want Edit truly hidden in read-only mode, add a !readOnly check to the Edit render condition (isFile && onEdit && !readOnly) to enforce hiding—modify the readOnly JSDoc or the Edit rendering accordingly.apps/api/src/shares/dto/share-key.dto.ts-41-50 (1)
41-50:⚠️ Potential issue | 🟡 MinorMissing
@ArrayMinSize(1)— emptykeysarray passes validation.An empty array satisfies
@IsArray()and thePOST /shares/:shareId/keyscall becomes a silent no-op (or triggers unexpected service behavior). Requiring at least one entry makes the intent explicit.🛡️ Proposed fix
-import { IsArray, ValidateNested } from 'class-validator'; +import { IsArray, ArrayMinSize, ValidateNested } from 'class-validator'; ... `@IsArray`() + `@ArrayMinSize`(1) `@ValidateNested`({ each: true }) `@Type`(() => ShareKeyEntryDto) keys!: ShareKeyEntryDto[];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/dto/share-key.dto.ts` around lines 41 - 50, Add a minimum-size validation to the AddShareKeysDto.keys property so an empty array fails validation: import and apply `@ArrayMinSize`(1) from class-validator to the keys field (alongside the existing `@IsArray`(), `@ValidateNested`(), and `@Type`(() => ShareKeyEntryDto)) to require at least one ShareKeyEntryDto in the array and prevent silent no-op POST /shares/:shareId/keys behavior.apps/web/src/hooks/useSharedNavigation.ts-301-314 (1)
301-314:⚠️ Potential issue | 🟡 MinorMissing
navigateToRootinnavigateUpdependency array.
navigateUpcallsnavigateToRoot()at line 312, butnavigateToRootis not listed in the dependency array at line 314. This is an exhaustive-deps violation. Both currently depend onfolderKey, so it's unlikely to cause issues in practice, but it could break subtly ifnavigateToRoot's dependencies diverge in the future.Suggested fix
- }, [currentView, folderKey]); + }, [currentView, folderKey, navigateToRoot]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useSharedNavigation.ts` around lines 301 - 314, The navigateUp useCallback is missing navigateToRoot in its dependency array; update the useCallback declaration for navigateUp to include navigateToRoot (i.e., change the dependencies from [currentView, folderKey] to [currentView, folderKey, navigateToRoot]) so React's exhaustive-deps rules are satisfied and navigateUp will see the latest navigateToRoot implementation; if navigateToRoot is unstable, wrap it with useCallback or stabilize it similarly to avoid unnecessary re-creations of navigateUp.apps/api/src/shares/shares.service.ts-264-277 (1)
264-277:⚠️ Potential issue | 🟡 Minor
completeRotationdoesn't verify the share is actually revoked.A caller could hard-delete an active, non-revoked share by calling this endpoint. Consider adding a guard to ensure
share.revokedAtis set before allowing the hard delete, to prevent accidental data loss.Suggested guard
if (share.sharerId !== sharerId) { throw new ForbiddenException('Only the sharer can complete rotation'); } + if (!share.revokedAt) { + throw new ConflictException('Share must be revoked before completing rotation'); + } + // CASCADE will remove all associated ShareKey records await this.shareRepo.remove(share);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/shares.service.ts` around lines 264 - 277, In completeRotation, add a guard to ensure the share is revoked before hard-deleting: after loading the share in completeRotation(shareId, sharerId) check that share.revokedAt is set (non-null) and if not throw an appropriate HTTP error (e.g., BadRequestException or Conflict) to block removal; only call this.shareRepo.remove(share) when share.revokedAt exists. This prevents callers from hard-deleting active/non-revoked shares.apps/web/src/components/file-browser/ShareDialog.tsx-179-181 (1)
179-181:⚠️ Potential issue | 🟡 MinorProgress bar total will be inaccurate for nested folders.
countFolderChildrenonly counts immediate children, butcollectChildKeysrecursively traverses all descendants. For a folder with subfolders,progress.currentwill exceedprogress.total, causing the progress bar to overflow past 100%.Suggested fix: use an unbounded progress display or count recursively
The simplest fix is to change the progress display to not depend on a pre-computed total:
- const totalSubfolders = countFolderChildren(metadata.children); - setProgress({ current: 0, total: totalSubfolders }); + // Total is unknown for deeply nested folders; use current count only + setProgress({ current: 0, total: 0 }); childKeys = await collectChildKeys( metadata.children, itemFolderKey, ownerPrivateKey, recipientPubKeyBytes, - (wrapped) => setProgress({ current: wrapped, total: totalSubfolders }) + (wrapped) => setProgress((prev) => ({ current: wrapped, total: Math.max(prev?.total ?? 0, wrapped) })) );Or alternatively, compute the total recursively before starting (though this doubles the traversal work).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/ShareDialog.tsx` around lines 179 - 181, countFolderChildren currently returns only immediate children count, which mismatches the recursive traversal done by collectChildKeys and causes progress.current to exceed progress.total; update countFolderChildren to compute the recursive total of all descendant FolderChild items (i.e., walk subfolders and sum their descendants) so progress.total reflects the full number of keys collected, or alternatively switch the UI progress display to an unbounded/indeterminate mode so it doesn't rely on progress.total; locate and modify the countFolderChildren function (and any callers that set progress.total) to perform a recursive count or toggle the progress component to indeterminate.apps/api/src/shares/shares.controller.ts-144-150 (1)
144-150:⚠️ Potential issue | 🟡 Minor
publicKeyquery parameter lacks input validation.The
publicKeyparameter is a raw string with no length or format constraints. While TypeORM parameterizes queries (no SQL injection risk), an unbounded string will hit the database on every request. Consider adding aValidationPipewith a query DTO, or at minimum a@MaxLengthor regex validation, consistent with theCreateShareDto.recipientPublicKeywhich validates^(0x)?04[0-9a-fA-F]{128}$.Example: inline Pipe-based validation
+ import { MaxLength } from 'class-validator'; ... - async lookupUser(`@Query`('publicKey') publicKey: string): Promise<{ exists: boolean }> { + async lookupUser( + `@Query`('publicKey', new ParseStringPipe({ maxLength: 132 })) publicKey: string + ): Promise<{ exists: boolean }> {Or create a small
LookupUserQueryDtowithclass-validatordecorators and use@Query() dto: LookupUserQueryDtowith aValidationPipe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/shares.controller.ts` around lines 144 - 150, The lookupUser controller method accepts an unvalidated publicKey string — update the method (lookupUser) to validate input before calling sharesService.lookupUserByPublicKey: either replace the raw `@Query`('publicKey') publicKey: string with a validated DTO (e.g., LookupUserQueryDto used as `@Query`() dto: LookupUserQueryDto) and enable ValidationPipe, or at minimum apply class-validator decorators like `@Matches`(/^(0x)?04[0-9a-fA-F]{128}$/) and `@MaxLength`(...) consistent with CreateShareDto.recipientPublicKey; ensure you then pass the validated value to sharesService.lookupUserByPublicKey and keep the existing NotFoundException behavior.apps/web/src/services/share.service.ts-34-56 (1)
34-56:⚠️ Potential issue | 🟡 MinorJSDoc claims "Updates the share store" but functions don't touch the store.
Both
fetchReceivedSharesandfetchSentSharesdocument "Updates the share store with the results" but only return the mapped arrays without calling any store setter. Callers (getSentSharesForItem,ensureFreshSentShares) handle the store update themselves. Remove the misleading JSDoc sentence to avoid confusion.Also applies to: 62-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/services/share.service.ts` around lines 34 - 56, Update the JSDoc for fetchReceivedShares and fetchSentShares to remove the incorrect sentence claiming they "update the share store" — these functions only call sharesControllerGetReceivedShares / sharesControllerGetSentShares and return mapped ReceivedShare/SentShare arrays; revise the comment to state they fetch and return mapped shares (or simply "Fetches received/sent shares and returns a mapped array") so it accurately reflects the behavior of fetchReceivedShares and fetchSentShares.packages/api-client/openapi.json-1427-1460 (1)
1427-1460:⚠️ Potential issue | 🟡 Minor
/shares/{shareId}/complete-rotationis missing a403response.Other sharer-only mutation endpoints (
/shares/{shareId},/shares/{shareId}/encrypted-key,/shares/{shareId}/keysPOST) define a 403 "Only the sharer can …" response. This endpoint omits it, which could cause inconsistent error handling on the client if an unauthorized user attempts to call it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api-client/openapi.json` around lines 1427 - 1460, The DELETE operation at "/shares/{shareId}/complete-rotation" (operationId SharesController_completeRotation) is missing a 403 response; add a 403 response object matching the other sharer-only endpoints (e.g., description "Only the sharer can complete key rotation" or similar) to the responses block so clients receive a consistent "Only the sharer can …" error for unauthorized callers.
🧹 Nitpick comments (20)
.planning/todos/pending/2026-02-21-phase14-security-review-deferred.md (3)
15-15: M1 implementation note:itemNamemust be encrypted per-recipient, not onceThe current wording — "Encrypt
itemNamewith recipient's public key" — implies a single ciphertext. In multi-recipient shares the same folder/file is shared with several users, so the implementation will need one encrypted copy ofitemNameper share recipient (mirroring theShareKeyper-recipient pattern). A single ciphertext encrypted to one public key is unreadable by other recipients.Worth clarifying this in the solution wording before pick-up to avoid an incorrect schema design at implementation time.
Also applies to: 22-22
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/todos/pending/2026-02-21-phase14-security-review-deferred.md at line 15, The note about encrypting itemName is ambiguous: do not produce a single ciphertext for itemName; instead, follow the existing per-recipient pattern (same as ShareKey) and store one encrypted itemName per recipient in the shares model/table so each share recipient has an itemName ciphertext encrypted to their public key and can decrypt for display; update schema/logic that writes/reads itemName to mirror ShareKey creation, storage and retrieval for each recipient rather than a single encrypted value.
16-16: M5: Define behavior when all retries are exhaustedThe solution mentions a "retry queue" but does not describe the failure escalation path. If retries are exhausted the recipient will hold a share record with no usable key indefinitely. Consider specifying an explicit terminal state (e.g., auto-revoke the share and notify the sharer) so implementors have a clear contract.
Also applies to: 23-23
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/todos/pending/2026-02-21-phase14-security-review-deferred.md at line 16, The current reWrapForRecipients flow and its "retry queue" need a clear terminal-state contract: update the reWrapForRecipients implementation and accompanying retry-queue logic to, after a configurable maxRetry count is exhausted for a recipient, (1) mark that recipient's share record with an explicit terminal status field (e.g., "rewrap_failed"), (2) optionally auto-revoke the share if a config flag is set, and (3) emit a non-blocking notification/event to the sharer (and an auditable metric/log entry) so the UI or operator can surface the failure; ensure the code paths that enqueue retries and the retry worker check maxRetry and transition to the terminal state instead of silently logging only, and document the new terminal status and notification behavior for implementors.
18-18: L4: Consider documenting a maximumlimitcapThe default
limit=50is sensible, but without a documented upper-bound (e.g.,limit ≤ 100) the implementation has no guidance on clamping. An uncappedlimitaccepted from the client can become a DoS vector on the database query. Worth adding a maximum to the solution note.Also applies to: 25-25
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/todos/pending/2026-02-21-phase14-security-review-deferred.md at line 18, Add and enforce a documented maximum for the pagination `limit` to prevent DoS: introduce a constant (e.g., MAX_SHARE_LIMIT = 100) and in the request handlers getReceivedShares and getSentShares validate and clamp the incoming `limit` query param (default to 50, if client sends > MAX_SHARE_LIMIT set it to MAX_SHARE_LIMIT; if <=0 treat as default or return 400 per your API convention). Update the handler logic that builds the DB query to use the clamped `limit` and the `offset`, and add a short comment/docs note describing the default and maximum cap so clients know the upper bound..planning/phases/14-user-to-user-sharing/14-CONTEXT.md (1)
16-16: Optional: fix heading-level skip flagged by markdownlint
### Invitation flow(h3) appears directly under the# Phase 14(h1), skipping h2. This triggersMD001 heading-increment. Fixing it is purely cosmetic and non-blocking, but it would silence the lint warning.💅 Proposed fix (optional)
-### Invitation flow +## Invitation flowApply the same promotion to each
###heading under<decisions>and<specifics>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/phases/14-user-to-user-sharing/14-CONTEXT.md at line 16, Change the heading level for "Invitation flow" from ### to ## to avoid skipping h2 under "Phase 14" and similarly promote any other occurrences of ### headings that live directly under the top-level "Phase 14" (specifically those inside the <decisions> and <specifics> sections) to ## so the heading hierarchy is linear (h1 -> h2 -> h3) and MD001 is silenced.apps/web/src/services/folder.service.ts (1)
980-986: Silently skipping rotation when vault keypair is absent could mask problems.When
auth.vaultKeypairis null, the function returns the original key withrotated: false, and the pending rotation remains in the database. This means the rotation check will fire again on the next modification, which is the correct retry behavior.However, if the vault keypair is persistently unavailable (e.g., a bug in auth state), this will silently degrade every folder modification with an extra network call to
checkPendingRotationthat always results in a skip. Consider tracking this (e.g., a counter or one-time warning) so it doesn't become a hidden performance drain in degraded states.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/services/folder.service.ts` around lines 980 - 986, The code currently returns early when auth.vaultKeypair is missing (in the lazy-rotation path that returns { folderKey: folderNode.folderKey, rotated: false }) which can cause repeated silent skips; add lightweight tracking to avoid repeated no-op calls: create a module-level map or counter (e.g., pendingRotationSkipCounts keyed by folderNode.id or folderNode.folderKey) and increment it each time auth.vaultKeypair is null inside the same check where it currently logs the console.warn, emit the console.warn only the first time per folder (or when the count exceeds a threshold) and optionally record/emit a metrics event; keep the existing return value and leave checkPendingRotation behavior unchanged, but ensure the log/metric uses folderNode.folderKey and the auth.vaultKeypair check so you can locate and update the early-return block.packages/crypto/src/index.ts (1)
14-43: Optionally extend the module JSDoc example to includereWrapKey.
reWrapKeyis now a first-class public primitive, but the@exampleblock only demonstrateswrapKey/unwrapKey. A brief snippet would improve discoverability for consumers of@cipherbox/crypto.📝 Suggested JSDoc addition
* // Wrap file key with user's public key * const wrappedKey = await wrapKey(fileKey, vaultKey.publicKey); * * // Unwrap and decrypt * const unwrappedKey = await unwrapKey(wrappedKey, vaultKey.privateKey); * const decrypted = await decryptAesGcm(ciphertext, unwrappedKey, iv); + * + * // Re-wrap a key from owner to recipient (user-to-user sharing) + * const recipientWrapped = await reWrapKey(wrappedKey, ownerPrivateKey, recipientPublicKey); * ```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/crypto/src/index.ts` around lines 14 - 43, Update the module JSDoc example to demonstrate the new reWrapKey primitive: after generating or unwrapping the file key (functions generateFileKey, wrapKey, unwrapKey), show how to call reWrapKey to re-encrypt/wrap an existing wrappedKey to a different recipient public key (referencing reWrapKey and its inputs like wrappedKey and newRecipientPublicKey), then show unwrapping the re-wrapped key with unwrapKey and using it with decryptAesGcm; keep the snippet concise and placed near the existing wrapKey/unwrapKey example so consumers can discover reWrapKey alongside wrapKey/unwrapKey and encryptAesGcm/decryptAesGcm.apps/web/src/api/models/childKeyDto.ts (1)
10-17:ChildKeyDtois structurally identical toShareKeyEntryDto— consider consolidating at the API schema level.Both DTOs have the same three fields with the same types and JSDoc comments. Their key-type enums (
ChildKeyDtoKeyTypeandShareKeyEntryDtoKeyType) are also identical ({ file, folder }). Since these are Orval-generated, the fix belongs in the backend OpenAPI schema: replace both backend DTOs with a single shared type (e.g.,KeyEntryDto) referenced from bothCreateShareDto.childKeysandAddShareKeysDto.keys. This would collapse the generated duplication on the frontend too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/api/models/childKeyDto.ts` around lines 10 - 17, ChildKeyDto and ShareKeyEntryDto are identical — consolidate them in the backend OpenAPI schema by introducing a single shared type (e.g., KeyEntryDto) with the three fields and a single enum for key type, then update CreateShareDto.childKeys and AddShareKeysDto.keys to reference KeyEntryDto so Orval generates only one DTO (replace ChildKeyDto, ShareKeyEntryDto, ChildKeyDtoKeyType, and ShareKeyEntryDtoKeyType with the unified KeyEntryDto and its enum).apps/web/src/routes/SharedPage.tsx (1)
4-4: Prefer barrel import forSharedFileBrowser.
SharedFileBrowseris already re-exported from the../components/file-browserindex barrel (confirmed atapps/web/src/components/file-browser/index.ts:18). Importing from the barrel keeps imports consistent with the rest of the codebase.♻️ Proposed change
-import { SharedFileBrowser } from '../components/file-browser/SharedFileBrowser'; +import { SharedFileBrowser } from '../components/file-browser';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/SharedPage.tsx` at line 4, Replace the direct import of SharedFileBrowser with the barrel export: in SharedPage.tsx update the import statement that currently imports SharedFileBrowser from '../components/file-browser/SharedFileBrowser' to import { SharedFileBrowser } from the components/file-browser barrel (use the re-exported symbol SharedFileBrowser) so imports match the project's barrel pattern and stay consistent with other modules.packages/crypto/src/__tests__/rewrap.test.ts (1)
66-83: Nit: duplicatereWrapKeycalls in error assertion tests.Tests 3, 4, and 5 each invoke
reWrapKeytwice — once to assert the error type, once for the message. Each invocation runs the full unwrap/wrap cycle. You can consolidate with a single call usingtoThrowErroror chaining matchers, though this is purely a style preference.♻️ Optional: single-call pattern
- await expect( - reWrapKey(wrappedForAlice, wrongKeypair.privateKey, bob.publicKey) - ).rejects.toThrow(CryptoError); - - await expect( - reWrapKey(wrappedForAlice, wrongKeypair.privateKey, bob.publicKey) - ).rejects.toThrow('Key re-wrapping failed'); + const rejection = reWrapKey(wrappedForAlice, wrongKeypair.privateKey, bob.publicKey); + await expect(rejection).rejects.toThrow(CryptoError); + await expect(rejection).rejects.toThrow('Key re-wrapping failed');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/crypto/src/__tests__/rewrap.test.ts` around lines 66 - 83, The test calls reWrapKey twice to assert type and message; consolidate into a single assertion by invoking reWrapKey(wrappedForAlice, wrongKeypair.privateKey, bob.publicKey) once and assert both the error type and message (e.g., use a single rejects.toThrowError or capture the rejected promise/error and assert its instanceof CryptoError and message contains "Key re-wrapping failed"); update the test around reWrapKey/wrongKeypair/privateKey/bob.publicKey to remove the duplicate call and perform both checks from the single invocation.apps/web/src/styles/share-dialog.css (1)
211-236: Hardcoded#ef4444should use the existingvar(--color-error)design token.Lines 216, 225, 229, 262, 268, 272 use hardcoded
#ef4444and itsrgb(239 68 68 / …)variants, while.share-error(line 111) already referencesvar(--color-error). For consistency and easier theming, derive these from the design token.♻️ Example fix for .share-revoke-btn
.share-revoke-btn { flex-shrink: 0; padding: 2px 8px; font-family: var(--font-family-mono); font-size: var(--font-size-xs); - color: `#ef4444`; + color: var(--color-error); background: transparent; - border: var(--border-thickness) solid rgb(239 68 68 / 30%); + border: var(--border-thickness) solid color-mix(in srgb, var(--color-error) 30%, transparent); cursor: pointer; transition: background-color 0.15s ease, color 0.15s ease, border-color 0.15s ease; } .share-revoke-btn:hover:not(:disabled) { - background-color: rgb(239 68 68 / 10%); - border-color: `#ef4444`; + background-color: color-mix(in srgb, var(--color-error) 10%, transparent); + border-color: var(--color-error); } .share-revoke-btn:focus-visible { - outline: 1px solid `#ef4444`; + outline: 1px solid var(--color-error); outline-offset: 1px; }Apply the same pattern to
.share-revoke-confirm-btn--yes(lines 261-274).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/styles/share-dialog.css` around lines 211 - 236, Replace hardcoded `#ef4444` and its rgb(...) translucent variants in .share-revoke-btn and .share-revoke-confirm-btn--yes with the design token var(--color-error): use color: var(--color-error) and border-color: var(--color-error) (or outline: var(--color-error)) and convert translucent uses to derive from the token (e.g., color-mix(to srgb, var(--color-error) 10%) or rgba from a --color-error-rgb token if your CSS variables include it) so all instances (text color, border, hover background, focus outline, disabled styles) consistently reference var(--color-error) instead of the hardcoded hex/rgb values.apps/web/src/components/file-browser/SharedFileBrowser.tsx (1)
24-75: File type detection helpers are duplicated.These extension sets and helper functions (
isImageFile,isPdfFile,isAudioFile,isVideoFile,isPreviewableFile) are likely duplicated fromFileBrowser.tsxor a similar location. Consider extracting them into a shared utility (e.g.,utils/file-types.ts) to reduce duplication across components.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/SharedFileBrowser.tsx` around lines 24 - 75, The file-type detection (IMAGE_EXTENSIONS, PDF_EXTENSIONS, AUDIO_EXTENSIONS, VIDEO_EXTENSIONS and the helpers isImageFile, isPdfFile, isAudioFile, isVideoFile, isPreviewableFile) is duplicated; extract these constants and functions into a single shared utility module and replace the local copies with imports. Create a module that exports the extension sets and the five helper functions with the exact names above, update SharedFileBrowser.tsx to import them instead of defining them locally, and update any other components (e.g., FileBrowser) to import the same shared utilities so all callers use the centralized implementation.apps/api/src/shares/dto/create-share.dto.ts (1)
70-75:itemNameaccepts empty strings — consider adding@MinLength(1).The
itemNamefield has@MaxLength(255)but no minimum length check, so an empty string""would pass validation. If a shared item with an empty name would cause display issues for the recipient (e.g., invisible row in the Shared With Me list), consider adding@MinLength(1).Proposed fix
`@IsString`() + `@MinLength`(1) `@MaxLength`(255) itemName!: string;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/dto/create-share.dto.ts` around lines 70 - 75, The itemName property currently allows empty strings; add a `@MinLength`(1) decorator to the itemName field in create-share.dto (the property named itemName) to enforce a non-empty value and update the imports to include MinLength from class-validator; ensure the `@MinLength`(1) appears alongside `@IsString`() and `@MaxLength`(255) so validation rejects "" inputs.apps/api/src/shares/entities/share-key.entity.ts (1)
36-38:itemIdshould usetype: 'uuid'rather thanvarchar(255).The DTO validates
itemIdas a UUID (via@MatchesUUID regex); the entity should match. PostgreSQL stores auuidcolumn as 16 bytes internally versus a 36-character string forvarchar, making the composite unique constraint and the@Index()more efficient.♻️ Proposed fix
`@Index`() - `@Column`({ type: 'varchar', length: 255, name: 'item_id' }) + `@Column`({ type: 'uuid', name: 'item_id' }) itemId!: string;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/entities/share-key.entity.ts` around lines 36 - 38, Change the itemId column definition in the ShareKey entity to use PostgreSQL uuid type instead of varchar(255): update the `@Column` decorator on itemId (the itemId property in the share-key.entity TypeORM entity) to use type: 'uuid' and remove the length option so the database column is a native uuid, keeping the column name 'item_id' and existing `@Index`() intact to match the DTO UUID validation.apps/api/src/shares/dto/share-key.dto.ts (2)
13-13:ShareKeyEntryDtoshould be exported.Without
export, the class is inaccessible to unit tests and other modules. NestJS Swagger resolves it correctly via the same-file@ApiProperty({ type: [ShareKeyEntryDto] })reference (the generatedshareKeyEntryDto.tsweb model confirms this), but testability and reusability suffer.♻️ Proposed fix
-class ShareKeyEntryDto { +export class ShareKeyEntryDto {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/dto/share-key.dto.ts` at line 13, The class ShareKeyEntryDto is not exported, making it inaccessible to tests and other modules; update the class declaration for ShareKeyEntryDto in share-key.dto.ts to include the export keyword so it becomes export class ShareKeyEntryDto, ensuring unit tests and other modules can import and reuse it (keep other DTOs and ApiProperty references unchanged).
25-28: Prefer@IsUUID()over a hand-rolled UUID regex.
class-validatorships@IsUUID()which validates the same format and is more readable.♻️ Proposed fix
-import { - IsString, - IsIn, - IsArray, - ValidateNested, - Matches, - MinLength, - MaxLength, -} from 'class-validator'; +import { + IsString, + IsIn, + IsArray, + IsUUID, + ValidateNested, + Matches, + MinLength, + MaxLength, +} from 'class-validator';- `@IsString`() - `@Matches`(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i, { - message: 'itemId must be a valid UUID', - }) + `@IsUUID`('4', { message: 'itemId must be a valid UUID' }) itemId!: string;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/dto/share-key.dto.ts` around lines 25 - 28, The itemId property currently uses a hand-rolled regex via `@Matches`(...) (and `@IsString`()) to validate a UUID; replace that with class-validator's built-in `@IsUUID`() decorator on the itemId property, remove the custom `@Matches` regex (and you can drop `@IsString`() since `@IsUUID` implies string validation), and add an import for IsUUID from 'class-validator' to the DTO file so validation is cleaner and more readable.apps/web/src/hooks/useSharedNavigation.ts (1)
99-108: Share keys cache is never cleared on navigation reset.
shareKeysCache(useRef<Map>) accumulates entries across share navigations. When the user callsnavigateToRoot()(line 320), the folder state is reset and keys are zeroed, but the cache retains stale entries indefinitely. The 60-second TTL provides some protection, but if the user navigates between many shares in a session, the cache will grow unbounded. Consider clearing the cache innavigateToRoot.Also applies to: 150-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useSharedNavigation.ts` around lines 99 - 108, shareKeysCache (the useRef Map defined as shareKeysCache) is never cleared on navigation reset and can grow unbounded; update the navigation reset logic by calling shareKeysCache.current.clear() inside navigateToRoot and any other functions that reset/share key state (the same reset paths referenced around the 150-165 region), so the cache is emptied whenever folder/share state is zeroed; ensure you reference the shareKeysCache variable and invoke .clear() rather than reassigning the ref.apps/api/src/shares/shares.service.ts (1)
169-192:addShareKeysissues N individual queries per key entry.Each iteration queries for an existing record and then saves. For a large folder share with many files/subfolders, this becomes an O(n) roundtrip pattern. Consider using TypeORM's
upsertto batch this into a single query, or at minimum usequeryRunnerfor a single transaction.Example using TypeORM upsert
- for (const entry of dto.keys) { - const existing = await this.shareKeyRepo.findOne({ - where: { shareId, keyType: entry.keyType, itemId: entry.itemId }, - }); - if (existing) { - existing.encryptedKey = Buffer.from(entry.encryptedKey, 'hex'); - await this.shareKeyRepo.save(existing); - } else { - const shareKey = this.shareKeyRepo.create({ - shareId, - keyType: entry.keyType, - itemId: entry.itemId, - encryptedKey: Buffer.from(entry.encryptedKey, 'hex'), - }); - await this.shareKeyRepo.save(shareKey); - } - } + const entities = dto.keys.map((entry) => + this.shareKeyRepo.create({ + shareId, + keyType: entry.keyType, + itemId: entry.itemId, + encryptedKey: Buffer.from(entry.encryptedKey, 'hex'), + }) + ); + await this.shareKeyRepo.upsert(entities, ['shareId', 'keyType', 'itemId']);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/shares.service.ts` around lines 169 - 192, The addShareKeys loop issues N separate find/save calls; replace it with a single batched upsert using this.shareKeyRepo.upsert by preparing an array of records with shareId, keyType, itemId and encryptedKey (Buffer.from(entry.encryptedKey, 'hex')) and calling upsert(records, { conflictPaths: ['shareId','keyType','itemId'] }) so DB does insert-or-update in one query; alternatively wrap the loop in a single transaction using a QueryRunner (startTransaction(), save all, commit/rollback) to avoid per-item roundtrips—update the addShareKeys implementation to build the records array and call shareKeyRepo.upsert (or use QueryRunner) instead of per-entry findOne/save.apps/web/src/components/file-browser/ShareDialog.tsx (2)
231-246: Unsafe cast of API response to localSentShare[].Line 235:
(shares as unknown as SentShare[])bypasses type safety entirely. If the API response shape changes, this will silently produce incorrect data. This is a consequence of the Orval-generated client returningvoidtypes. Consider adding runtime shape validation or at minimum a type assertion function.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/ShareDialog.tsx` around lines 231 - 246, The code unsafely casts the API result from sharesControllerGetSentShares() to SentShare[]; replace the cast with runtime validation and safe handling: check that the response is an array, implement a small type guard like isSentShare(obj) that verifies required fields (e.g., ipnsName, recipient, etc.), then filter using that guard and ipnsName before calling setRecipients; if validation fails, setRecipients([]) and log a warning so malformed API responses don't corrupt state. Ensure you reference sharesControllerGetSentShares, SentShare, ipnsName, setRecipients, and recipientsLoading in the updated flow.
35-43: LocalSentSharetype duplicatesSentSharefromshare.store.ts.This local type (with
itemType: string) duplicates theSentSharetype inapps/web/src/stores/share.store.ts(which usesitemType: 'folder' | 'file'). Consider importing and reusing the store type to keep them in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/ShareDialog.tsx` around lines 35 - 43, Replace the local SentShare type in ShareDialog.tsx with the shared SentShare type exported from the store (the SentShare type in the share store that defines itemType as 'folder' | 'file'); remove the duplicate type declaration, import the store's SentShare type where ShareDialog uses it, and update any usages in the component to rely on the imported type so itemType and other fields remain consistent across the codebase.apps/web/src/services/share.service.ts (1)
379-382:checkPendingRotationfetches all pending rotations on every call.This makes a full network round-trip to
GET /shares/pending-rotationseach time a folder modification begins. If the folder store already knows the set of pending rotation IPNS names (or if the caller already fetched them), this could be cached or batched similarly to sent shares. For now the overhead may be acceptable, but it will become a bottleneck once users accumulate many revoked shares.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/services/share.service.ts` around lines 379 - 382, checkPendingRotation currently issues a network fetch by calling fetchPendingRotations() on every invocation, causing unnecessary round-trips; modify it to accept an optional pre-fetched set or cache of pending IPNS names to avoid repeated GET /shares/pending-rotations calls: update checkPendingRotation(folderIpnsName: string, pending?: Set<string> | string[]) to check the provided pending collection first (or populate/refresh a short-lived in-memory cache inside the module) and only fall back to calling fetchPendingRotations() when the pending list is not supplied or the cache is stale; refer to the function names checkPendingRotation and fetchPendingRotations to locate the change.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
tests/e2e/utils/web3auth-helpers.ts (1)
290-316: Consider documenting (or eliminating) the in-place mutation of the caller'sstateparameter.Lines 305–306 mutate
authStatedirectly:authState.accessToken = accessToken; if (email) authState.email = email;When
stateis provided (new multi-account path), this silently modifies the caller's object — which is easy to miss when managing severalTestAuthStatereferences in a sharing test. The singleton case is unaffected since mutation is expected there.♻️ Option A — document the mutation contract in the JSDoc
/** * ... + * `@param` state - Optional explicit state (multi-account). When provided, its + * `accessToken` (and optionally `email`) fields will be updated in-place + * with a freshly refreshed token before re-injection. */ export async function reinjectTestAuthAfterReload(♻️ Option B — avoid mutation by returning the (possibly updated) state
export async function reinjectTestAuthAfterReload( page: Page, state?: TestAuthState - ): Promise<void> { - const authState = state ?? cachedTestAuthState; + ): Promise<TestAuthState | null> { + let authState = state ? { ...state } : cachedTestAuthState ? { ...cachedTestAuthState } : null; if (!process.env.TEST_LOGIN_SECRET || !authState) return; // ... if (refreshResponse.ok()) { const { accessToken, email } = await refreshResponse.json(); - authState.accessToken = accessToken; - if (email) authState.email = email; + authState = { ...authState, accessToken, ...(email ? { email } : {}) }; + if (!state) cachedTestAuthState = authState; // keep singleton in sync } await injectAuthState(page, authState); // ... + return authState; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/utils/web3auth-helpers.ts` around lines 290 - 316, The function reinjectTestAuthAfterReload mutates the caller's state object (authState) when refreshing tokens; to fix, avoid in-place mutation: create a new object (e.g., newAuthState = { ...authState, accessToken, ...(email && { email }) }), call injectAuthState(page, newAuthState), and return the updated TestAuthState from reinjectTestAuthAfterReload; update any callers to use the returned state (and preserve cachedTestAuthState behavior when state was undefined), or alternatively add a clear JSDoc on reinjectTestAuthAfterReload and TestAuthState noting that the provided state will be mutated if passed.apps/api/src/shares/shares.controller.spec.ts (5)
84-107:createShareis missing aConflictExceptionpropagation test.The PR lists "Fixed TOCTOU race in
createShare… converting duplicate-constraint violations intoConflictException" as a Critical Fix. The controller spec only covers the happy path; there's no test verifying that aConflictExceptionthrown by the service is correctly propagated to the caller.✅ Suggested test case
it('should propagate ConflictException when share already exists', async () => { mockSharesService.createShare.mockRejectedValue( new ConflictException('Share already exists'), ); const dto = { recipientPublicKey, itemType: 'folder' as const, ipnsName: 'k51qzi5uqu5dg12345', itemName: 'My Folder', encryptedKey: testEncryptedKey, }; await expect(controller.createShare(mockReq, dto)).rejects.toThrow(ConflictException); });Also worth importing
ConflictExceptionfrom@nestjs/commonat the top of the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/shares.controller.spec.ts` around lines 84 - 107, Add a test that verifies controller.createShare propagates a ConflictException from the service: mock mockSharesService.createShare to reject with new ConflictException('Share already exists'), call controller.createShare(mockReq, dto) with the same dto used in the happy-path test, and assert the promise rejects with ConflictException (use await expect(...).rejects.toThrow(ConflictException)); also import ConflictException from '@nestjs/common' at the top of the spec file.
145-168: Missing test for service error propagation inlookupUser.The PR specifically highlights: "Fixed
lookupUserswallowing errors: now returns false only for 404 and re-throws other network/server errors." The existing tests covertrue,false, and invalid format — but there is no test verifying that whenlookupUserByPublicKeyrejects with an unexpected error (e.g.,InternalServerErrorException), the controller propagates it rather than converting it toNotFoundException.✅ Suggested test case
it('should propagate unexpected service errors', async () => { const serviceError = new Error('DB connection failed'); mockSharesService.lookupUserByPublicKey.mockRejectedValue(serviceError); const validKey = '0x04' + 'ab'.repeat(64); await expect(controller.lookupUser(validKey)).rejects.toThrow('DB connection failed'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/shares.controller.spec.ts` around lines 145 - 168, Add a test that verifies unexpected service errors are re-thrown by controller.lookupUser: mock mockSharesService.lookupUserByPublicKey to reject with an error (e.g., new Error('DB connection failed')), call controller.lookupUser with a valid public key ('0x04' + 'ab'.repeat(64)), and assert the call rejects with the original error message so lookupUser does not convert non-404 errors into NotFoundException.
163-167: Assert service is not called for invalid key formats.The test correctly expects
BadRequestExceptionfor malformed keys, but doesn't assert thatlookupUserByPublicKeywas never reached. Without this, a regression where the controller calls the service first (returningundefined) and then throwsBadRequestExceptionfor a different reason would still pass.♻️ Proposed addition
await expect(controller.lookupUser('')).rejects.toThrow(BadRequestException); + expect(mockSharesService.lookupUserByPublicKey).not.toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/shares.controller.spec.ts` around lines 163 - 167, Update the test that checks invalid public key formats to also assert the service method is never invoked: for each call to controller.lookupUser with invalid inputs, add an expectation that the mocked SharesService.lookupUserByPublicKey (or whatever mock is injected as "service.lookupUserByPublicKey") was not called (e.g., expect(service.lookupUserByPublicKey).not.toHaveBeenCalled()). Ensure the mock is reset/cleared before each assertion if needed so calls from previous assertions don't interfere.
109-274: Happy-path and delegation coverage is solid.
getReceivedSharestests both a populated and empty result. The delegation tests forrevokeShare,hideShare,updateShareEncryptedKey, andcompleteRotationall correctly assert thatshareIdanduserIdare threaded through. ThegetShareKeystest verifies the Buffer→hex conversion path.One gap worth noting:
getSentShareshas no empty-array test (unlikegetReceivedShares). Low impact since the controller path is identical, but consistent coverage would be cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/shares.controller.spec.ts` around lines 109 - 274, Add a mirror empty-array test for getSentShares: mock mockSharesService.getSentShares to resolve to [] and call controller.getSentShares(mockReq), then assert the result equals [] to match the existing getReceivedShares empty-case; locate the test block around getSentShares and add the new spec using the same mockReq, mockSharesService.getSentShares, and controller.getSentShares symbols.
159-160: Capture the rejected promise once instead of invoking the controller twice.Each
controller.lookupUser(validKey)on lines 159 and 160 is an independent call that triggers the service mock a second time. Capture the promise once and assert both properties against it.♻️ Proposed fix
- await expect(controller.lookupUser(validKey)).rejects.toThrow(NotFoundException); - await expect(controller.lookupUser(validKey)).rejects.toThrow('User not found'); + const rejection = controller.lookupUser(validKey); + await expect(rejection).rejects.toThrow(NotFoundException); + await expect(rejection).rejects.toThrow('User not found');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/shares.controller.spec.ts` around lines 159 - 160, The test currently calls controller.lookupUser(validKey) twice causing the mock to be invoked twice; instead capture the rejected promise once (e.g., const promise = controller.lookupUser(validKey)) and then assert both expectations against that single promise: await expect(promise).rejects.toThrow(NotFoundException) and await expect(promise).rejects.toThrow('User not found'), referencing controller.lookupUser and validKey so the service mock is only triggered once.apps/web/src/components/file-browser/ShareDialog.tsx (1)
36-43: LocalSentSharetype duplicatesSentSharefromapps/web/src/stores/share.store.ts.The local type uses the looser
itemType: stringwhile the store's type uses the narrower'folder' | 'file'. Import and reuse the store type (castings.itemType as 'folder' | 'file'when mapping from the API response) to keep a single source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/ShareDialog.tsx` around lines 36 - 43, Replace the local duplicate SentShare type with the canonical SentShare from the share.store by importing SentShare from apps/web/src/stores/share.store.ts and reusing it in ShareDialog; when mapping API responses into SentShare (e.g., in the function or mapping that produces objects with shareId, recipientPublicKey, itemType, ipnsName, itemName, createdAt) cast the incoming s.itemType as 'folder' | 'file' (e.g., s.itemType as 'folder' | 'file') so the mapped objects conform to the store's narrower itemType union.apps/web/src/components/file-browser/SharedFileBrowser.tsx (1)
577-611:onClickandonDoubleClickboth fire on a double-click — apply the 250 ms debounce.For
SharedListRow, bothonClickandonDoubleClickare wired toonOpen. A user double-clicking a folder row triggers twonavigateToSharecalls (two IPNS fetches, twosetFolderKeycalls). The same issue exists inSharedFolderRowfor folder items.As per the coding guidelines, use a ~250 ms timer to delay the single-click action and cancel it on double-click to prevent these duplicate state changes.
As per coding guidelines, "When an element handles both
onClickandonDoubleClick, use a ~250ms timer to delay single-click and cancel it on double-click to prevent conflicting state changes."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/SharedFileBrowser.tsx` around lines 577 - 611, The SharedListRow (and similarly SharedFolderRow) currently calls onOpen from both onClick and onDoubleClick causing duplicate actions; change the handlers to use a ~250ms single-click delay: in the component create a ref-held timeout id (e.g., clickTimeoutRef), have onClick start a setTimeout(~250) that calls navigateToShare / setFolderKey (the existing onOpen logic), and have onDoubleClick clear the timeout and immediately perform the double-click action (call navigateToShare / setFolderKey once); also clear the timeout in a cleanup effect to avoid leaks. Use the unique symbols SharedListRow, SharedFolderRow, onClick/onDoubleClick, onOpen, navigateToShare and setFolderKey to locate where to implement the timeout logic.
🤖 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/components/file-browser/SharedFileBrowser.tsx`:
- Around line 174-178: handleDownload currently calls downloadSharedFile which
early-returns when currentShareId or folderKey are null (true for top-level list
view) and also fails key lookup because the synthetic FolderChild uses id:
sharedItem.share.shareId; add a path that downloads directly from the share
record: in the useSharedNavigation hook either expose a new
downloadSharedFileFromShareRecord(shareId, fileId) function (or reuse existing
navigateToShare → downloadSharedFileFromShare flow) and then update
handleDownload to branch: if contextShareId is set and currentView === 'list'
call the new download-from-share-record method with contextShareId and the
file's real id, otherwise fall back to downloadSharedFile; reference symbols:
handleDownload, downloadSharedFile, useSharedNavigation, navigateToShare,
downloadSharedFileFromShareRecord, contextShareId, currentView,
FolderChild/sharedItem.share.shareId.
- Around line 467-477: The empty-state block in SharedFileBrowser is rendering
even when an error exists; update the render condition for the empty-state (the
JSX guarded by {!isLoading && !hasChildren && (...)}) to also require no error
(e.g., add a check like && !error) so the empty-state only shows when not
loading, there are no children, and there is no error; locate the conditional
around the empty-state JSX in SharedFileBrowser (referencing isLoading,
hasChildren, and error) and modify it accordingly.
- Around line 363-370: Replace the synchronous loop that calls navigateUp()
repeatedly; instead compute popsNeeded, then pop that many entries directly from
navStackRef (or the component's nav stack) in a single pass, zeroing each popped
entry's folderKey Uint8Array with .fill(0) immediately after extracting it, and
finally perform a single state update to set the current folder/folderKey (via
the same setter used by navigateUp or by calling navigateToRoot/navigateUp once)
so no intermediate folderKey gets written into React state and left orphaned;
update the click handler that references breadcrumbs and navigateUp to use this
single-pass pop-and-zero approach.
In `@apps/web/src/components/file-browser/ShareDialog.tsx`:
- Around line 184-186: countFolderChildren currently returns only direct
children, causing progress to finish early vs. collectChildKeys which recurses;
update countFolderChildren to perform a recursive traversal of FolderChild[]
(mirroring collectChildKeys' recursion) and sum 1 per file/folder plus all
nested descendant counts so the progress total reflects the full set of keys to
be re-wrapped; locate and modify the countFolderChildren function and ensure it
handles the same FolderChild shape used by collectChildKeys.
- Around line 304-311: The catch is masking all failures from
sharesControllerLookupUser into "user not found"; update the error handling so
lookupUser errors are classified (e.g., inspect error.status or throw a typed
NotFoundError from the service) and only map 404/not-found to setError('user not
found'), while for other errors call setError('lookup failed, please try again')
and keep setIsSharing(false); change either the API client that implements
sharesControllerLookupUser to surface HTTP status or custom error types, or
update the catch in this component to branch on error.status or error.name
before setting the message.
In `@apps/web/src/hooks/useSharedNavigation.ts`:
- Around line 195-233: The folder branch leaves the decrypted itemKey in memory
if any of resolveIpnsRecord, fetchFromIpfs, or decryptFolderMetadata throws; to
fix, ensure itemKey is zeroed in all failure paths by wrapping the
folder-specific logic (the block after unwrapKey where you call
resolveIpnsRecord, fetchFromIpfs, JSON.parse, decryptFolderMetadata,
setFolderKey, setFolderChildren, setBreadcrumbs, etc.) in its own try/finally
and call itemKey.fill(0) in the finally; keep the existing file path behavior
that zeroes itemKey before calling downloadSharedFileFromShare and only
setFolderKey after successful decryption so the finally always clears secrets on
errors.
- Around line 318-331: The navigateUp callback omits navigateToRoot from its
useCallback dependency array, causing a react-hooks/exhaustive-deps violation;
update the useCallback dependencies for the navigateUp function to include
navigateToRoot (e.g., change from [currentView, folderKey] to [currentView,
folderKey, navigateToRoot]) so navigateUp always captures the latest
navigateToRoot reference.
---
Duplicate comments:
In `@tests/e2e/utils/web3auth-helpers.ts`:
- Around line 27-47: This is a duplicate review comment: no code changes
required—the TestAuthState interface already includes the requested `@security`
JSDoc on the interface and the privateKeyArr field; leave the TestAuthState
declaration (interface TestAuthState and its fields like privateKeyArr,
publicKeyArr, rootIpnsPrivateKeyArr) as-is and do not add or modify additional
annotations.
---
Nitpick comments:
In `@apps/api/src/shares/shares.controller.spec.ts`:
- Around line 84-107: Add a test that verifies controller.createShare propagates
a ConflictException from the service: mock mockSharesService.createShare to
reject with new ConflictException('Share already exists'), call
controller.createShare(mockReq, dto) with the same dto used in the happy-path
test, and assert the promise rejects with ConflictException (use await
expect(...).rejects.toThrow(ConflictException)); also import ConflictException
from '@nestjs/common' at the top of the spec file.
- Around line 145-168: Add a test that verifies unexpected service errors are
re-thrown by controller.lookupUser: mock mockSharesService.lookupUserByPublicKey
to reject with an error (e.g., new Error('DB connection failed')), call
controller.lookupUser with a valid public key ('0x04' + 'ab'.repeat(64)), and
assert the call rejects with the original error message so lookupUser does not
convert non-404 errors into NotFoundException.
- Around line 163-167: Update the test that checks invalid public key formats to
also assert the service method is never invoked: for each call to
controller.lookupUser with invalid inputs, add an expectation that the mocked
SharesService.lookupUserByPublicKey (or whatever mock is injected as
"service.lookupUserByPublicKey") was not called (e.g.,
expect(service.lookupUserByPublicKey).not.toHaveBeenCalled()). Ensure the mock
is reset/cleared before each assertion if needed so calls from previous
assertions don't interfere.
- Around line 109-274: Add a mirror empty-array test for getSentShares: mock
mockSharesService.getSentShares to resolve to [] and call
controller.getSentShares(mockReq), then assert the result equals [] to match the
existing getReceivedShares empty-case; locate the test block around
getSentShares and add the new spec using the same mockReq,
mockSharesService.getSentShares, and controller.getSentShares symbols.
- Around line 159-160: The test currently calls controller.lookupUser(validKey)
twice causing the mock to be invoked twice; instead capture the rejected promise
once (e.g., const promise = controller.lookupUser(validKey)) and then assert
both expectations against that single promise: await
expect(promise).rejects.toThrow(NotFoundException) and await
expect(promise).rejects.toThrow('User not found'), referencing
controller.lookupUser and validKey so the service mock is only triggered once.
In `@apps/web/src/components/file-browser/SharedFileBrowser.tsx`:
- Around line 577-611: The SharedListRow (and similarly SharedFolderRow)
currently calls onOpen from both onClick and onDoubleClick causing duplicate
actions; change the handlers to use a ~250ms single-click delay: in the
component create a ref-held timeout id (e.g., clickTimeoutRef), have onClick
start a setTimeout(~250) that calls navigateToShare / setFolderKey (the existing
onOpen logic), and have onDoubleClick clear the timeout and immediately perform
the double-click action (call navigateToShare / setFolderKey once); also clear
the timeout in a cleanup effect to avoid leaks. Use the unique symbols
SharedListRow, SharedFolderRow, onClick/onDoubleClick, onOpen, navigateToShare
and setFolderKey to locate where to implement the timeout logic.
In `@apps/web/src/components/file-browser/ShareDialog.tsx`:
- Around line 36-43: Replace the local duplicate SentShare type with the
canonical SentShare from the share.store by importing SentShare from
apps/web/src/stores/share.store.ts and reusing it in ShareDialog; when mapping
API responses into SentShare (e.g., in the function or mapping that produces
objects with shareId, recipientPublicKey, itemType, ipnsName, itemName,
createdAt) cast the incoming s.itemType as 'folder' | 'file' (e.g., s.itemType
as 'folder' | 'file') so the mapped objects conform to the store's narrower
itemType union.
In `@tests/e2e/utils/web3auth-helpers.ts`:
- Around line 290-316: The function reinjectTestAuthAfterReload mutates the
caller's state object (authState) when refreshing tokens; to fix, avoid in-place
mutation: create a new object (e.g., newAuthState = { ...authState, accessToken,
...(email && { email }) }), call injectAuthState(page, newAuthState), and return
the updated TestAuthState from reinjectTestAuthAfterReload; update any callers
to use the returned state (and preserve cachedTestAuthState behavior when state
was undefined), or alternatively add a clear JSDoc on
reinjectTestAuthAfterReload and TestAuthState noting that the provided state
will be mutated if passed.
The SharedFileBrowser and file browser hooks fetch data on mount only (useEffect with empty deps). When navigateToShared is called twice without leaving the route (e.g., tests 3.2 then 4.2), the hash doesn't change, the component doesn't remount, and stale data is shown. Fix by navigating away first when already on the target route. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 771434b7d415
1. Fix context menu download broken for top-level file shares (downloadSharedFile guards on null currentShareId/folderKey in list view) 2. Fix multi-level breadcrumb navigation leaking intermediate folder keys (add navigateToBreadcrumb hook with single-pass pop-and-zero) 3. Fix empty folder state rendering alongside error state 4. Fix misleading progress bar (switch to unbounded counter) 5. Distinguish 404 from network errors in user lookup 6. Fix itemKey not zeroed when folder navigation fails in navigateToShare 7. Fix navigateToRoot missing from navigateUp dependency array Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 4985d640fadb
…est robustness - Handle empty response bodies (201/204) in customInstance via text-first parsing - Fix ShareDialog race condition where "no recipients yet" flashes before fetch starts by tracking recipientsFetched state - Fix waitForRecipientsLoaded to wait for actual content instead of loading absence - Fix breadcrumb exact matching (hasText: /^shared$/) to avoid partial matches - Add IPNS resolution retry in test 4.2 and settle wait in test 7.2 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: af1dd25ebb3e
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
apps/web/src/components/file-browser/ShareDialog.tsx (2)
325-336: Consider removing the long explanatory comment block.Lines 325-336 contain stream-of-consciousness reasoning about which key is which. This reads more like implementation notes than documentation. A single concise comment would be clearer:
- // For folders shared from a parent, folderKey IS the folder's own key - // (it's the folderKey passed as prop which is the parent's key -- - // but the item is a FolderEntry child that has folderKeyEncrypted) - // We need to use folderKeyEncrypted from the FolderEntry to get the actual folder key. - // Wait -- the folderKey prop is the PARENT folder's key. The item's own key - // is obtained by unwrapping folderKeyEncrypted from the FolderEntry. - // Actually, looking at the plan more carefully: - // "For folders: folderKey is passed as prop" - // But folderKey is the PARENT's key. For sharing a subfolder, we need the subfolder's key. - // Let's check: if the item is a direct child folder, its folderKeyEncrypted - // is wrapped with the owner's public key (ECIES). We unwrap it to get the folder's own key. - - // Unwrap the folder's own key from its encrypted form + // Unwrap the subfolder's own AES key from its ECIES-wrapped form🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/ShareDialog.tsx` around lines 325 - 336, Remove the long stream‑of‑consciousness comment block in ShareDialog.tsx (lines discussing folderKey, folderKeyEncrypted and parent vs child keys) and replace it with a single concise comment that states: for folders, the passed-in folderKey is the parent folder's key and the item's own key must be obtained by unwrapping FolderEntry.folderKeyEncrypted (ECIES) to get the actual folder key; reference the FolderEntry type and the folderKey prop to make locating the logic that unwraps folderKeyEncrypted clear.
108-157: Folder re-wrap failure at line 111 aborts processing of all remaining siblings.If
reWrapEncryptedKeythrows for one subfolder (line 111), the error propagates unhandled out of theforloop, aborting the entirecollectChildKeys— discarding already-collected keys. By contrast, the file branch (lines 90-107) catches errors per-child and continues.For consistency and resilience, consider wrapping the folder re-wrap in a try/catch as well:
Proposed fix
} else { const folder = child as FolderEntry; // Re-wrap the subfolder's folderKey for the recipient + try { const folderKeyRewrapped = await reWrapEncryptedKey( folder.folderKeyEncrypted, ownerPrivateKey, recipientPubKeyBytes ); childKeys.push({ keyType: 'folder' as ChildKeyDto['keyType'], itemId: folder.id, encryptedKey: folderKeyRewrapped, }); wrapped++; onProgress(wrapped); // Recurse into subfolder: resolve its metadata and collect its children - try { + // ... existing try/catch for recursion ... const resolved = await resolveIpnsRecord(folder.ipnsName); // ... - } catch (err) { - console.error(`Failed to traverse subfolder ${folder.name}:`, err); - // Continue with other children - } + } catch (err) { + console.error(`Failed to re-wrap/traverse subfolder ${folder.name}:`, err); + // Continue with other children + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/ShareDialog.tsx` around lines 108 - 157, The folder branch in collectChildKeys currently calls reWrapEncryptedKey (reWrapEncryptedKey) without local error handling which lets a thrown error abort the whole loop; wrap the reWrapEncryptedKey call (and the subsequent childKeys push/wrapped/onProgress increment) in a try/catch so failures for a single subfolder are logged and skipped rather than propagating, mirroring the file branch behavior; ensure any sensitive folderKeyBytes created by unwrapKey are still zeroed in a finally block and that traversal of subfolders (resolveIpnsRecord, fetchFromIpfs, decryptFolderMetadata, and recursive collectChildKeys) only runs for successfully rewrapped folders.apps/web/src/components/file-browser/SharedFileBrowser.tsx (3)
369-373: Use theindexparameter from.map()instead ofbreadcrumbs.indexOf(crumb).The
indexis already available from themapcallback. UsingindexOfdoes a redundant O(n) scan and could return-1ifbreadcrumbsis updated between render and click (stale closure edge case).Proposed fix
- <button - type="button" - className="breadcrumb-item" - onClick={() => navigateToBreadcrumb(breadcrumbs.indexOf(crumb))} - > + <button + type="button" + className="breadcrumb-item" + onClick={() => navigateToBreadcrumb(index)} + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/SharedFileBrowser.tsx` around lines 369 - 373, The click handler currently computes the breadcrumb position with breadcrumbs.indexOf(crumb), causing an extra O(n) scan and potential -1 from stale closures; update the map callback to use its provided index parameter and pass that index to navigateToBreadcrumb (i.e., within the map over breadcrumbs use (crumb, index) => onClick={() => navigateToBreadcrumb(index)}) so the button uses the mapped index directly instead of calling breadcrumbs.indexOf(crumb).
24-75: File type detection helpers andtruncatePubkey/sortItemsare duplicated.These utilities (
isImageFile,isPdfFile,isAudioFile,isVideoFile,truncatePubkey,sortItems) appear to duplicate logic from the mainFileBrowserandShareDialog. Consider extracting them into a shared utility module to reduce duplication across components.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/SharedFileBrowser.tsx` around lines 24 - 75, The helper functions isImageFile, isPdfFile, isAudioFile, isVideoFile, isPreviewableFile and the utility functions truncatePubkey and sortItems are duplicated across components; extract them into a single shared utility module (e.g., fileBrowserUtils) and export each function, then replace the local definitions in SharedFileBrowser, FileBrowser, and ShareDialog with imports from that module; ensure the exported functions keep their exact behavior/signature (same parameter types and return values), update all imports where those symbols are used, and run the build/tests to confirm no behavioral changes.
324-326: No-op handlersonRename={() => {}}andonDelete={() => {}}in read-only mode.Passing empty functions for actions that should never fire is a code smell. If the
ContextMenucomponent supportsreadOnlymode (which it does — line 328/497), consider makingonRenameandonDeleteoptional in theContextMenuprops so you don't need to pass dummy handlers.Also applies to: 494-495
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/SharedFileBrowser.tsx` around lines 324 - 326, Remove the dummy no-op handlers by making onRename and onDelete optional in the ContextMenu prop types and then only pass them from SharedFileBrowser when not in read-only mode; update the ContextMenu component signature (props/interface) to mark onRename?: (...) and onDelete?: (...), adjust any internal calls to guard for undefined handlers, and in SharedFileBrowser.tsx stop passing onRename={() => {}} and onDelete={() => {}} — only pass the real handlers (e.g., the rename/delete functions) when readOnly is false (same change for the other occurrence around lines 494-495), leaving onDownload as-is.tests/e2e/tests/sharing-workflow.spec.ts (2)
117-120: Consider replacing hard-codedwaitForTimeoutwith a UI assertion.
aliceNavigateBackuses a blanket 500ms wait. This can be flaky in CI. Consider waiting for a specific breadcrumb/element change instead.async function aliceNavigateBack(): Promise<void> { await alice.page.locator('[data-testid="parent-dir-row"]').dblclick(); - await alice.page.waitForTimeout(500); + // Wait for the breadcrumb to update (adjust selector to match the expected state) + await alice.page.locator('.breadcrumb-item').first().waitFor({ state: 'visible', timeout: 5000 }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/sharing-workflow.spec.ts` around lines 117 - 120, The helper aliceNavigateBack currently uses a fixed 500ms wait which is flaky; replace the hard-coded waitForTimeout in aliceNavigateBack with a deterministic UI assertion: after dblclicking the '[data-testid="parent-dir-row"]' locator, wait for a specific element change (e.g., breadcrumb text update, a parent folder breadcrumb becoming visible, or the previous directory row to appear/disappear) using page.waitForSelector or locator.waitFor with the appropriate state so the test proceeds only when the UI has actually updated.
278-298: Hard-codedwaitForTimeout(3000)/waitForTimeout(2000)for IPNS resolution.These waits are fragile. The retry logic at lines 282-288 is a good mitigation for the first one, but the second wait (line 298) has no fallback. Consider polling for a visible element instead.
That said, IPNS resolution latency is inherently unpredictable in E2E, so this may be an acceptable pragmatic choice. Just flag for awareness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/sharing-workflow.spec.ts` around lines 278 - 298, Replace the fragile hard-coded sleeps (bob.page.waitForTimeout(3000) and 2000) with polling/wait-for-visible checks: after calling bobSharedBrowser.openSharedItem(...) use bobSharedBrowser.parentDirRow().waitFor({ timeout: 30000 }) or an explicit loop that calls bobSharedBrowser.parentDirRow().isVisible() with short delays and a timeout (mirroring the existing retry used around navigateToShared / waitForLoaded / waitForSharedItem / navigateIntoFolder), and after doubleClickFolderItem(subFolderName) wait for a known subfolder content indicator (e.g. bobSharedBrowser.getFolderItemNames() to return non-empty or a specific element’s isVisible()) with a timeout instead of waitForTimeout; update or add helper methods on bobSharedBrowser if needed to encapsulate these waits.apps/web/src/hooks/useSharedNavigation.ts (2)
391-448:downloadSharedFileFromSharedoesn't need to be inside the hook body.This function only uses its parameters and imported modules — it captures no hook state or refs. Extracting it as a module-level function would make the independence explicit and avoid unnecessary recreation on each render.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useSharedNavigation.ts` around lines 391 - 448, The function downloadSharedFileFromShare should be moved out of the hook body to a module-level export because it does not close over any hook state; extract it as a top-level async function (keeping its signature downloadSharedFileFromShare(share: ReceivedShare, privateKey: Uint8Array): Promise<void>) and update the hook to call it. Ensure all referenced utilities and stores (useDownloadStore.getState(), unwrapKey, hexToBytes, resolveIpnsRecord, fetchFromIpfs, decryptFileMetadata, fetchShareKeys, downloadFile, triggerBrowserDownload) remain imported at the top of the file and that parentFolderKey.fill(0) and error handling logic are preserved; if the hook previously relied on any hook-local variables, remove those dependencies or pass them in as params and export the function if other modules need to call it.
454-508: Note: In-flight downloads will fail if the user navigates away mid-download.
downloadSharedFilecapturesfolderKeyby reference. If the user triggers a download and then navigates back (navigateToRootzeroes the sameUint8Array), the in-progressdecryptFileMetadatacall at line 476 will receive zeroed bytes and fail. The error is caught at line 501, so this won't crash — but the user gets a generic "Download failed" message with no indication that navigation caused it.This is acceptable from a security standpoint (zeroing keys promptly is the right call), but consider showing a more specific message if decryption fails with all-zero input, or disabling navigation while a download is active.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useSharedNavigation.ts` around lines 454 - 508, downloadSharedFile can receive a zeroed folderKey if the user navigates away (navigateToRoot) mid-download which causes decryptFileMetadata to fail and produce a generic "Download failed" message; to fix, make the handler detect and surface that case explicitly by (a) marking the download state when downloadStore.startDownload is called (use/downloadStore flags like isDownloading), (b) before calling decryptFileMetadata and again on catching its error, check whether folderKey is all-zero (inspect the Uint8Array bytes) and if so set a clear message like "Download cancelled: keys cleared by navigation" via downloadStore.setError, and (c) optionally add a guard in navigateToRoot (or the shared navigation logic) to prevent navigation while downloadStore.isDownloading is true or prompt the user — update references to downloadSharedFile, decryptFileMetadata, folderKey, downloadStore, and navigateToRoot accordingly.
🤖 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/api/custom-instance.ts`:
- Around line 51-54: The check that guards JSON.parse should treat
whitespace-only bodies as empty; update the conditional around the variable text
(in custom-instance.ts) to also return undefined when text.trim() is empty
(e.g., if (!text || text.trim() === "")) so newline-only or space-only bodies
don't reach JSON.parse and throw a SyntaxError.
In `@apps/web/src/components/file-browser/SharedFileBrowser.tsx`:
- Around line 510-556: Preview dialogs (ImagePreviewDialog, PdfPreviewDialog,
AudioPlayerDialog, VideoPlayerDialog) attempt to download shared files using
downloadFileFromIpns which expects fileMeta.fileKeyEncrypted to be unwrap-able
by the recipient but currently only has owner-wrapped keys; add a shareId prop
to each preview dialog and, inside their download flow (the handler that calls
downloadFileFromIpns), fetch the recipient-wrapped key from the share_keys table
(same approach used in useSharedNavigation.ts), substitute the re-wrapped key
into the FilePointer/fileMeta before calling downloadFileFromIpns, and ensure
the onClose/set state flows remain unchanged so recipient previewing uses the
correct re-wrapped file key.
---
Nitpick comments:
In `@apps/web/src/components/file-browser/SharedFileBrowser.tsx`:
- Around line 369-373: The click handler currently computes the breadcrumb
position with breadcrumbs.indexOf(crumb), causing an extra O(n) scan and
potential -1 from stale closures; update the map callback to use its provided
index parameter and pass that index to navigateToBreadcrumb (i.e., within the
map over breadcrumbs use (crumb, index) => onClick={() =>
navigateToBreadcrumb(index)}) so the button uses the mapped index directly
instead of calling breadcrumbs.indexOf(crumb).
- Around line 24-75: The helper functions isImageFile, isPdfFile, isAudioFile,
isVideoFile, isPreviewableFile and the utility functions truncatePubkey and
sortItems are duplicated across components; extract them into a single shared
utility module (e.g., fileBrowserUtils) and export each function, then replace
the local definitions in SharedFileBrowser, FileBrowser, and ShareDialog with
imports from that module; ensure the exported functions keep their exact
behavior/signature (same parameter types and return values), update all imports
where those symbols are used, and run the build/tests to confirm no behavioral
changes.
- Around line 324-326: Remove the dummy no-op handlers by making onRename and
onDelete optional in the ContextMenu prop types and then only pass them from
SharedFileBrowser when not in read-only mode; update the ContextMenu component
signature (props/interface) to mark onRename?: (...) and onDelete?: (...),
adjust any internal calls to guard for undefined handlers, and in
SharedFileBrowser.tsx stop passing onRename={() => {}} and onDelete={() => {}} —
only pass the real handlers (e.g., the rename/delete functions) when readOnly is
false (same change for the other occurrence around lines 494-495), leaving
onDownload as-is.
In `@apps/web/src/components/file-browser/ShareDialog.tsx`:
- Around line 325-336: Remove the long stream‑of‑consciousness comment block in
ShareDialog.tsx (lines discussing folderKey, folderKeyEncrypted and parent vs
child keys) and replace it with a single concise comment that states: for
folders, the passed-in folderKey is the parent folder's key and the item's own
key must be obtained by unwrapping FolderEntry.folderKeyEncrypted (ECIES) to get
the actual folder key; reference the FolderEntry type and the folderKey prop to
make locating the logic that unwraps folderKeyEncrypted clear.
- Around line 108-157: The folder branch in collectChildKeys currently calls
reWrapEncryptedKey (reWrapEncryptedKey) without local error handling which lets
a thrown error abort the whole loop; wrap the reWrapEncryptedKey call (and the
subsequent childKeys push/wrapped/onProgress increment) in a try/catch so
failures for a single subfolder are logged and skipped rather than propagating,
mirroring the file branch behavior; ensure any sensitive folderKeyBytes created
by unwrapKey are still zeroed in a finally block and that traversal of
subfolders (resolveIpnsRecord, fetchFromIpfs, decryptFolderMetadata, and
recursive collectChildKeys) only runs for successfully rewrapped folders.
In `@apps/web/src/hooks/useSharedNavigation.ts`:
- Around line 391-448: The function downloadSharedFileFromShare should be moved
out of the hook body to a module-level export because it does not close over any
hook state; extract it as a top-level async function (keeping its signature
downloadSharedFileFromShare(share: ReceivedShare, privateKey: Uint8Array):
Promise<void>) and update the hook to call it. Ensure all referenced utilities
and stores (useDownloadStore.getState(), unwrapKey, hexToBytes,
resolveIpnsRecord, fetchFromIpfs, decryptFileMetadata, fetchShareKeys,
downloadFile, triggerBrowserDownload) remain imported at the top of the file and
that parentFolderKey.fill(0) and error handling logic are preserved; if the hook
previously relied on any hook-local variables, remove those dependencies or pass
them in as params and export the function if other modules need to call it.
- Around line 454-508: downloadSharedFile can receive a zeroed folderKey if the
user navigates away (navigateToRoot) mid-download which causes
decryptFileMetadata to fail and produce a generic "Download failed" message; to
fix, make the handler detect and surface that case explicitly by (a) marking the
download state when downloadStore.startDownload is called (use/downloadStore
flags like isDownloading), (b) before calling decryptFileMetadata and again on
catching its error, check whether folderKey is all-zero (inspect the Uint8Array
bytes) and if so set a clear message like "Download cancelled: keys cleared by
navigation" via downloadStore.setError, and (c) optionally add a guard in
navigateToRoot (or the shared navigation logic) to prevent navigation while
downloadStore.isDownloading is true or prompt the user — update references to
downloadSharedFile, decryptFileMetadata, folderKey, downloadStore, and
navigateToRoot accordingly.
In `@tests/e2e/tests/sharing-workflow.spec.ts`:
- Around line 117-120: The helper aliceNavigateBack currently uses a fixed 500ms
wait which is flaky; replace the hard-coded waitForTimeout in aliceNavigateBack
with a deterministic UI assertion: after dblclicking the
'[data-testid="parent-dir-row"]' locator, wait for a specific element change
(e.g., breadcrumb text update, a parent folder breadcrumb becoming visible, or
the previous directory row to appear/disappear) using page.waitForSelector or
locator.waitFor with the appropriate state so the test proceeds only when the UI
has actually updated.
- Around line 278-298: Replace the fragile hard-coded sleeps
(bob.page.waitForTimeout(3000) and 2000) with polling/wait-for-visible checks:
after calling bobSharedBrowser.openSharedItem(...) use
bobSharedBrowser.parentDirRow().waitFor({ timeout: 30000 }) or an explicit loop
that calls bobSharedBrowser.parentDirRow().isVisible() with short delays and a
timeout (mirroring the existing retry used around navigateToShared /
waitForLoaded / waitForSharedItem / navigateIntoFolder), and after
doubleClickFolderItem(subFolderName) wait for a known subfolder content
indicator (e.g. bobSharedBrowser.getFolderItemNames() to return non-empty or a
specific element’s isVisible()) with a timeout instead of waitForTimeout; update
or add helper methods on bobSharedBrowser if needed to encapsulate these waits.
Preview dialogs (image, PDF, audio, video) now receive shareId from SharedFileBrowser and use re-wrapped keys from share_keys table instead of owner-encrypted fileKeyEncrypted. Also guards against whitespace-only response bodies in custom fetch instance. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 6f96d356a6f4
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 (2)
apps/web/src/components/file-browser/VideoPlayerDialog.tsx (1)
63-80:⚠️ Potential issue | 🟠 MajorSame CTR streaming / re-wrapped-key blocking issue as
AudioPlayerDialog.
useStreamingPreviewruns withoutshareId, andblobPreview.openis gated on!streaming.isCtr. For sharedvideo/mp4orvideo/webmfiles (the most common formats), if the streaming probe returnsisCtr = true, the blob path — which carries the re-wrapped key — is permanently suppressed. Recipients cannot preview these files.🐛 Proposed fix — same pattern as AudioPlayerDialog
const streaming = useStreamingPreview({ - open: open && isStreamingCandidate, + open: open && isStreamingCandidate && !shareId, item, mimeType, folderKey, }); const blobPreview = useFilePreview({ open: open && - (!isStreamingCandidate || !streaming.isSwReady || (!streaming.loading && !streaming.isCtr)), + (!!shareId || !isStreamingCandidate || !streaming.isSwReady || (!streaming.loading && !streaming.isCtr)), item, mimeType, folderKey, shareId, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/VideoPlayerDialog.tsx` around lines 63 - 80, The streaming probe is run without shareId which can set streaming.isCtr=true and permanently suppress the blob preview; fix by passing shareId into useStreamingPreview (call site: useStreamingPreview({ open: open && isStreamingCandidate, item, mimeType, folderKey, shareId })) and update the blobPreview open gate in useFilePreview to not block when a shareId is present (change the open expression so it does not short-circuit blob preview solely because streaming.isCtr when shareId exists), ensuring blobPreview and useStreamingPreview both receive shareId and the blob path is allowed for shared files.apps/web/src/components/file-browser/AudioPlayerDialog.tsx (1)
68-85:⚠️ Potential issue | 🟠 MajorCTR streaming path blocks the re-wrapped-key blob path for shared audio.
useStreamingPreviewruns withoutshareId, so for a shared audio file that is identified as a CTR streaming candidate (isCtr = true),blobPreview.openevaluates tofalse— permanently locking out the re-wrapped key path thatuseFilePreviewprovides. Even when streaming subsequently errors,blobPreviewnever gets a chance to run because the open condition only checks!streaming.isCtr, not!streaming.error.Recipients will see a streaming error with no recovery path for any shared audio file whose MIME type falls in
STREAMING_AUDIO_MIMES.🐛 Proposed fix — gate streaming off when previewing shared content
const streaming = useStreamingPreview({ - open: open && isStreamingCandidate, + open: open && isStreamingCandidate && !shareId, item, mimeType, folderKey, }); const blobPreview = useFilePreview({ open: open && - (!isStreamingCandidate || !streaming.isSwReady || (!streaming.loading && !streaming.isCtr)), + (!!shareId || !isStreamingCandidate || !streaming.isSwReady || (!streaming.loading && !streaming.isCtr)), item, mimeType, folderKey, shareId, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/AudioPlayerDialog.tsx` around lines 68 - 85, The CTR streaming path is blocking the fallback blob preview for shared audio; change the useStreamingPreview invocation so streaming is disabled for shared items (e.g., set open: open && isStreamingCandidate && !shareId) so that useFilePreview can run for shared content and recover when streaming errors; update the useStreamingPreview call and its props (and any related gating logic that inspects streaming.isCtr or streaming.error) to ensure shared items prefer the blob re-wrapped-key path while non-shared items still use streaming.
🤖 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/components/file-browser/SharedFileBrowser.tsx`:
- Around line 419-430: The parent-dir row and other rows call the same handler
from both onClick and onDoubleClick (e.g., navigateUp, navigateToShare in
SharedListRow, navigateToSubfolder / onOpen in SharedFolderRow) which causes
triple-invocation on double-click; change these to use the project's ~250ms
click/double-click debounce pattern: on single click start a ~250ms timer that
invokes the single-click handler (navigateUp / navigateToShare /
navigateToSubfolder or onOpen), but if a double-click occurs before the timer
fires cancel the timer and invoke only the double-click behavior; apply this
pattern to the parent-dir row (replace direct onClick/onDoubleClick calls to
navigateUp) and likewise refactor SharedListRow and SharedFolderRow to use the
same debounced onOpen / double-click handling to prevent triple-firing.
- Around line 250-262: The list-view empty state in SharedFileBrowser is
rendering even when an error is present; update the JSX conditional that
currently reads {!isLoading && sharedItems.length === 0 && ( ... )} to also
check that there is no error (e.g., {!isLoading && !error && sharedItems.length
=== 0 && (...)}), ensuring the empty-state block only renders when not loading
and no error exists; adjust the condition around the empty-state markup that
includes sharedEmptyArt and data-testid="shared-empty-state".
---
Outside diff comments:
In `@apps/web/src/components/file-browser/AudioPlayerDialog.tsx`:
- Around line 68-85: The CTR streaming path is blocking the fallback blob
preview for shared audio; change the useStreamingPreview invocation so streaming
is disabled for shared items (e.g., set open: open && isStreamingCandidate &&
!shareId) so that useFilePreview can run for shared content and recover when
streaming errors; update the useStreamingPreview call and its props (and any
related gating logic that inspects streaming.isCtr or streaming.error) to ensure
shared items prefer the blob re-wrapped-key path while non-shared items still
use streaming.
In `@apps/web/src/components/file-browser/VideoPlayerDialog.tsx`:
- Around line 63-80: The streaming probe is run without shareId which can set
streaming.isCtr=true and permanently suppress the blob preview; fix by passing
shareId into useStreamingPreview (call site: useStreamingPreview({ open: open &&
isStreamingCandidate, item, mimeType, folderKey, shareId })) and update the
blobPreview open gate in useFilePreview to not block when a shareId is present
(change the open expression so it does not short-circuit blob preview solely
because streaming.isCtr when shareId exists), ensuring blobPreview and
useStreamingPreview both receive shareId and the blob path is allowed for shared
files.
- TextEditorDialog: readOnly + shareId props for shared text viewing - SharedFileBrowser: text file detection, preview routing, editor dialog - E2E Phase 11: 4 new tests for shared folder preview workflows - Test utilities: createTestImageFile, context menu preview/share/hide Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: d860f5573b95
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/web/src/components/file-browser/TextEditorDialog.tsx (1)
91-131:⚠️ Potential issue | 🟠 Major
plaintextbytes are never zeroed — decrypted file content leaks in memory
downloadFile/downloadFileFromIpnseach return aUint8Arraywhose fileKey is already zeroed internally (seedownload.service.tsfinallyblock), but the returnedplaintextbuffer is left entirely to the caller to clear. Two paths here fail to do so:
cancelledearly-return (line 125) — the bytes are simply abandoned without zeroing.- Happy path (line 127) —
TextDecoder.decode(plaintext)copies bytes into a JS string, but the sourceUint8Arraypersists in memory unreferenced until GC.Given the codebase's explicit key-hygiene pattern (fileKey zeroed in
finally, private key never persisted), this is inconsistent and leaves decrypted file content in memory longer than necessary.🔒 Proposed fix — zero `plaintext` on both paths
- let plaintext: Uint8Array; + let plaintext: Uint8Array | undefined; if (shareId) { // Shared file path: use re-wrapped file key from share_keys const [{ metadata: fileMeta }, keys] = await Promise.all([ resolveFileMetadata(item.fileMetaIpnsName, folderKey!), fetchShareKeys(shareId), ]); const fileKeyRecord = keys.find((k) => k.keyType === 'file' && k.itemId === item.id); if (!fileKeyRecord) { throw new Error('No re-wrapped file key available for this file'); } plaintext = await downloadFile( { cid: fileMeta.cid, iv: fileMeta.fileIv, wrappedKey: fileKeyRecord.encryptedKey, originalName: item.name, encryptionMode: fileMeta.encryptionMode, }, auth.vaultKeypair.privateKey ); } else { // Owner path: use file key from metadata directly plaintext = await downloadFileFromIpns({ fileMetaIpnsName: item.fileMetaIpnsName, folderKey: folderKey!, privateKey: auth.vaultKeypair.privateKey, fileName: item.name, }); } - if (cancelled) return; - - const text = new TextDecoder().decode(plaintext); - setContent(text); - setOriginalContent(text); - setLoading(false); - - // Focus textarea after content loads (only in edit mode) - if (!readOnly) { - requestAnimationFrame(() => { - textareaRef.current?.focus(); - }); - } + try { + if (cancelled) return; + + const text = new TextDecoder().decode(plaintext); + setContent(text); + setOriginalContent(text); + setLoading(false); + + // Focus textarea after content loads (only in edit mode) + if (!readOnly) { + requestAnimationFrame(() => { + textareaRef.current?.focus(); + }); + } + } finally { + plaintext?.fill(0); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/TextEditorDialog.tsx` around lines 91 - 131, The decrypted plaintext Uint8Array returned from downloadFile / downloadFileFromIpns must be explicitly zeroed to avoid leaving sensitive bytes in memory: after obtaining plaintext (variable plaintext) ensure you clear it on the cancelled early-return path (before returning) and also after producing the JS string (after TextDecoder.decode and after calling setContent/setOriginalContent) — best done in a finally block or with plaintext.fill(0) / a secure zero utility so zeroing always runs even on exceptions; reference the variables/plaintext handling around downloadFile, downloadFileFromIpns, the cancelled check, and the TextDecoder.decode usage to locate where to add the zeroing.
🧹 Nitpick comments (6)
tests/e2e/tests/sharing-workflow.spec.ts (5)
373-383: Revocation negative test could be flaky if the shared view is cached.
getSharedItemNames()on line 380 is called immediately afterwaitForLoaded. If the backend cache or frontend store hasn't fully propagated the revocation, the folder could still appear briefly. Consider adding awaitForSharedItemGonecall (which is available onSharedFileBrowserPage) instead of a point-in-time assertion.♻️ Suggested improvement
- const itemNames = await bobSharedBrowser.getSharedItemNames(); - const hasFolderShare = itemNames.some((n) => n.includes(sharedFolderName)); - expect(hasFolderShare).toBe(false); + // Wait for the revoked folder to disappear (handles cache propagation delay) + await bobSharedBrowser.waitForSharedItemGone(sharedFolderName, { timeout: 15000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/sharing-workflow.spec.ts` around lines 373 - 383, The test currently does a point-in-time check using bobSharedBrowser.getSharedItemNames() after navigateToShared and waitForLoaded, which can be flaky due to caching; replace that assertion with the built-in polling helper on the page object (use bobSharedBrowser.waitForSharedItemGone(sharedFolderName) or equivalent on SharedFileBrowserPage) so the test waits until the shared folder is absent before asserting; keep navigateToShared and waitForLoaded but remove the immediate getSharedItemNames()/expect block and call the waitForSharedItemGone helper instead to reliably detect revocation.
540-550: Duplicate IPNS retry pattern — consider extracting a helper.This retry pattern (open shared folder → wait → check parentDirRow → retry if not visible) is repeated from test 4.2 (lines 285–296). Extracting it into a shared helper would reduce duplication and make the retry strategy consistent.
♻️ Suggested helper
async function bobOpenSharedFolder(name: string): Promise<void> { await bobSharedBrowser.waitForSharedItem(name, { timeout: 15000 }); await bobSharedBrowser.openSharedItem(name); try { await bobSharedBrowser.parentDirRow().waitFor({ state: 'visible', timeout: 15000 }); } catch { await navigateToShared(bob); await bobSharedBrowser.waitForLoaded({ timeout: 30000 }); await bobSharedBrowser.waitForSharedItem(name, { timeout: 15000 }); await bobSharedBrowser.openSharedItem(name); await bobSharedBrowser.parentDirRow().waitFor({ state: 'visible', timeout: 30000 }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/sharing-workflow.spec.ts` around lines 540 - 550, The test duplicates an IPNS retry pattern (open shared folder → wait → verify parentDirRow visibility → retry) across tests; extract this logic into a helper (e.g., bobOpenSharedFolder) and replace the repeated block with a single call to that helper; the helper should use bobSharedBrowser.waitForSharedItem, bobSharedBrowser.openSharedItem, then await bobSharedBrowser.parentDirRow().waitFor(...) and on timeout perform the retry flow (navigateToShared, bobSharedBrowser.waitForLoaded, waitForSharedItem, openSharedItem, then waitFor parentDirRow again) so both test 4.2 and this file call the same robust routine.
120-123: HardcodedwaitForTimeout(500)is fragile.Instead of a fixed 500ms sleep, wait for a meaningful condition (e.g., the breadcrumb to update or the expected folder list to re-render). This reduces flakiness on slower CI environments.
♻️ Suggested improvement
async function aliceNavigateBack(): Promise<void> { await alice.page.locator('[data-testid="parent-dir-row"]').dblclick(); - await alice.page.waitForTimeout(500); + // Wait for navigation to complete by checking for file list to stabilize + await aliceFileList.waitForLoaded({ timeout: 15000 }); }If
FileListPagedoesn't exposewaitForLoaded, wait for the breadcrumb to change or a known element to appear.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/sharing-workflow.spec.ts` around lines 120 - 123, Replace the fragile fixed sleep in aliceNavigateBack by waiting for a meaningful UI change: after calling alice.page.locator('[data-testid="parent-dir-row"]').dblclick() remove waitForTimeout(500) and instead wait for the breadcrumb or file list to update (e.g., use FileListPage.waitForLoaded() if available, or waitForSelector on the updated breadcrumb text/selector or for a known file/folder row to appear/disappear). Ensure you reference the aliceNavigateBack function and the parent-dir-row locator when implementing the replacement so the test only proceeds once the navigation finish condition is observed.
285-296: IPNS retry logic uses arbitrary sleeps and inconsistent entry methods.Line 287 uses
waitForTimeout(3000)and the retry path (line 295) usesnavigateIntoFolder(double-click) while the first attempt (line 286) usesopenSharedItem(single-click). If the entry methods have different semantics, the retry may not reproduce the same state. Consider unifying the approach and replacing the fixed sleep with awaitForon a meaningful condition (e.g.,parentDirRowbecoming visible).♻️ Suggested improvement
- await bobSharedBrowser.openSharedItem(sharedFolderName); - await bob.page.waitForTimeout(3000); - - // If IPNS resolution failed on first try, retry once - const parentDirVisible = await bobSharedBrowser.parentDirRow().isVisible(); - if (!parentDirVisible) { - await navigateToShared(bob); - await bobSharedBrowser.waitForLoaded({ timeout: 30000 }); - await bobSharedBrowser.waitForSharedItem(sharedFolderName, { timeout: 15000 }); - await bobSharedBrowser.navigateIntoFolder(sharedFolderName); - } + await bobSharedBrowser.openSharedItem(sharedFolderName); + + // IPNS resolution can be slow on first access — wait with retry + try { + await bobSharedBrowser.parentDirRow().waitFor({ state: 'visible', timeout: 15000 }); + } catch { + // Retry once: navigate back and re-enter + await navigateToShared(bob); + await bobSharedBrowser.waitForLoaded({ timeout: 30000 }); + await bobSharedBrowser.waitForSharedItem(sharedFolderName, { timeout: 15000 }); + await bobSharedBrowser.openSharedItem(sharedFolderName); + await bobSharedBrowser.parentDirRow().waitFor({ state: 'visible', timeout: 30000 }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/sharing-workflow.spec.ts` around lines 285 - 296, Replace the arbitrary sleep and inconsistent entry method: remove bob.page.waitForTimeout(3000) and instead wait for the meaningful condition by using bobSharedBrowser.parentDirRow().waitFor({ state: 'visible', timeout: 30000 }) (or an equivalent waitFor that checks visibility), and make the retry path use the same entry method as the first attempt (use bobSharedBrowser.openSharedItem(sharedFolderName) for the retry instead of bobSharedBrowser.navigateIntoFolder). Keep the existing fallback calls (navigateToShared, bobSharedBrowser.waitForLoaded, bobSharedBrowser.waitForSharedItem) but follow them with the unified visibility wait and the same openSharedItem invocation so both attempts reproduce the same behavior.
304-306: Another hardcodedwaitForTimeout(2000)for subfolder navigation.Same concern as with other fixed sleeps — prefer waiting for a DOM condition such as the subfolder file appearing.
♻️ Suggested improvement
// Navigate into subfolder await bobSharedBrowser.doubleClickFolderItem(subFolderName); - // Wait for subfolder content to load - await bob.page.waitForTimeout(2000); + // Wait for subfolder content to load — expect the file to appear + await bobSharedBrowser.getFolderItem(folderFile2Name).waitFor({ + state: 'visible', + timeout: 15000, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/sharing-workflow.spec.ts` around lines 304 - 306, Replace the hardcoded sleep after doubleClickFolderItem: instead of calling bob.page.waitForTimeout(2000), wait for a DOM condition that indicates the subfolder content finished loading (e.g., use bob.page.waitForSelector or waitForFunction to detect the expected subfolder file or item element). Update the test around bobSharedBrowser.doubleClickFolderItem(subFolderName) to await a selector that uniquely identifies the subfolder's file list entry (or a visible container) before proceeding, so the test reliably continues when the UI has actually rendered.apps/web/src/components/file-browser/TextEditorDialog.tsx (1)
93-123: Document (or enforce) theshareId→readOnlyinvariant
shareIdandreadOnlyare independent props, but the PR's intent is that shared files are always read-only. If a caller passesshareIdwithoutreadOnly, the file is loaded via the re-wrapped key path, yet the textarea is editable and the save button is shown. The save path (handleSave) re-encrypts with the user's own keypair and publishes to the owner's folder metadata — semantically correct for the owner but not for a recipient viewing a shared file.♻️ Options
Option A — runtime invariant (defensive):
+ // shareId implies a shared (read-only) view; enforce the invariant early. + const effectiveReadOnly = readOnly ?? (shareId != null ? true : false);Then replace all
readOnlyreferences witheffectiveReadOnly.Option B — type-level enforcement via discriminated union:
-type TextEditorDialogProps = { - readOnly?: boolean; - shareId?: string | null; - ... -}; +type TextEditorDialogProps = { + ... +} & ( + | { readOnly?: false; shareId?: never } + | { readOnly: true; shareId?: string | null } +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/TextEditorDialog.tsx` around lines 93 - 123, Shared files can be editable if callers pass shareId without readOnly; compute an effectiveReadOnly flag (e.g. const effectiveReadOnly = !!shareId || readOnly) inside TextEditorDialog and replace all direct uses of readOnly with effectiveReadOnly (controls textarea disabled state, save button visibility, and any UI that allows editing). Also update handleSave to early-return or throw when effectiveReadOnly is true (and ensure the save button is disabled/hidden when effectiveReadOnly), while leaving the existing download paths (downloadFile, downloadFileFromIpns, fetchShareKeys) unchanged.
🤖 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/components/file-browser/TextEditorDialog.tsx`:
- Around line 209-213: The triggerBrowserDownload implementation in
download.service.ts currently constructs a Blob using content.buffer which can
include extra bytes when content is a subarray; update triggerBrowserDownload to
pass the typed array (Uint8Array) itself to new Blob (or convert ArrayBuffer to
a Uint8Array first) instead of using content.buffer, ensuring you handle both
ArrayBuffer and Uint8Array inputs and preserve mimeType and filename handling in
the existing function.
In `@tests/e2e/utils/test-files.ts`:
- Around line 95-167: The png Buffer in tests/e2e/utils/test-files.ts is
malformed: it contains an extra byte (0x33) which breaks chunk boundaries
(variable png), so fix by producing a valid 69-byte PNG — either remove the
spurious 0x33 from the array (so IDAT CRC bytes remain correct) or if the
intended IDAT was 13 bytes, change the IDAT length byte (currently 0x0c) to 0x0d
and recompute/update the following IDAT CRC bytes accordingly; ensure the final
png buffer is exactly the canonical 69 bytes and that the IDAT CRC matches the
IDAT data.
---
Outside diff comments:
In `@apps/web/src/components/file-browser/TextEditorDialog.tsx`:
- Around line 91-131: The decrypted plaintext Uint8Array returned from
downloadFile / downloadFileFromIpns must be explicitly zeroed to avoid leaving
sensitive bytes in memory: after obtaining plaintext (variable plaintext) ensure
you clear it on the cancelled early-return path (before returning) and also
after producing the JS string (after TextDecoder.decode and after calling
setContent/setOriginalContent) — best done in a finally block or with
plaintext.fill(0) / a secure zero utility so zeroing always runs even on
exceptions; reference the variables/plaintext handling around downloadFile,
downloadFileFromIpns, the cancelled check, and the TextDecoder.decode usage to
locate where to add the zeroing.
---
Nitpick comments:
In `@apps/web/src/components/file-browser/TextEditorDialog.tsx`:
- Around line 93-123: Shared files can be editable if callers pass shareId
without readOnly; compute an effectiveReadOnly flag (e.g. const
effectiveReadOnly = !!shareId || readOnly) inside TextEditorDialog and replace
all direct uses of readOnly with effectiveReadOnly (controls textarea disabled
state, save button visibility, and any UI that allows editing). Also update
handleSave to early-return or throw when effectiveReadOnly is true (and ensure
the save button is disabled/hidden when effectiveReadOnly), while leaving the
existing download paths (downloadFile, downloadFileFromIpns, fetchShareKeys)
unchanged.
In `@tests/e2e/tests/sharing-workflow.spec.ts`:
- Around line 373-383: The test currently does a point-in-time check using
bobSharedBrowser.getSharedItemNames() after navigateToShared and waitForLoaded,
which can be flaky due to caching; replace that assertion with the built-in
polling helper on the page object (use
bobSharedBrowser.waitForSharedItemGone(sharedFolderName) or equivalent on
SharedFileBrowserPage) so the test waits until the shared folder is absent
before asserting; keep navigateToShared and waitForLoaded but remove the
immediate getSharedItemNames()/expect block and call the waitForSharedItemGone
helper instead to reliably detect revocation.
- Around line 540-550: The test duplicates an IPNS retry pattern (open shared
folder → wait → verify parentDirRow visibility → retry) across tests; extract
this logic into a helper (e.g., bobOpenSharedFolder) and replace the repeated
block with a single call to that helper; the helper should use
bobSharedBrowser.waitForSharedItem, bobSharedBrowser.openSharedItem, then await
bobSharedBrowser.parentDirRow().waitFor(...) and on timeout perform the retry
flow (navigateToShared, bobSharedBrowser.waitForLoaded, waitForSharedItem,
openSharedItem, then waitFor parentDirRow again) so both test 4.2 and this file
call the same robust routine.
- Around line 120-123: Replace the fragile fixed sleep in aliceNavigateBack by
waiting for a meaningful UI change: after calling
alice.page.locator('[data-testid="parent-dir-row"]').dblclick() remove
waitForTimeout(500) and instead wait for the breadcrumb or file list to update
(e.g., use FileListPage.waitForLoaded() if available, or waitForSelector on the
updated breadcrumb text/selector or for a known file/folder row to
appear/disappear). Ensure you reference the aliceNavigateBack function and the
parent-dir-row locator when implementing the replacement so the test only
proceeds once the navigation finish condition is observed.
- Around line 285-296: Replace the arbitrary sleep and inconsistent entry
method: remove bob.page.waitForTimeout(3000) and instead wait for the meaningful
condition by using bobSharedBrowser.parentDirRow().waitFor({ state: 'visible',
timeout: 30000 }) (or an equivalent waitFor that checks visibility), and make
the retry path use the same entry method as the first attempt (use
bobSharedBrowser.openSharedItem(sharedFolderName) for the retry instead of
bobSharedBrowser.navigateIntoFolder). Keep the existing fallback calls
(navigateToShared, bobSharedBrowser.waitForLoaded,
bobSharedBrowser.waitForSharedItem) but follow them with the unified visibility
wait and the same openSharedItem invocation so both attempts reproduce the same
behavior.
- Around line 304-306: Replace the hardcoded sleep after doubleClickFolderItem:
instead of calling bob.page.waitForTimeout(2000), wait for a DOM condition that
indicates the subfolder content finished loading (e.g., use
bob.page.waitForSelector or waitForFunction to detect the expected subfolder
file or item element). Update the test around
bobSharedBrowser.doubleClickFolderItem(subFolderName) to await a selector that
uniquely identifies the subfolder's file list entry (or a visible container)
before proceeding, so the test reliably continues when the UI has actually
rendered.
The fire-and-forget re-wrapping of file keys for share recipients was silently failing due to two bugs: 1. share.store had a single `lastFetchedAt` timestamp shared between received and sent share caches. Fetching received shares (e.g. when navigating to /shared) would mark the sent shares cache as "fresh" even though sent shares were never fetched, causing ensureFreshSentShares() to return an empty array. Fix: split into `lastReceivedFetchedAt` and `lastSentFetchedAt`. 2. ShareDialog created shares via the API but never updated the global share store, so the sent shares cache was always stale after creating a new share. Fix: call addSentShare()/removeSentShare() on the store after create/revoke operations. Added E2E test 7.3 that verifies Charlie can decrypt and preview a file uploaded to an already-shared folder, proving re-wrapping works end-to-end. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 7ba0c8ebd2ac
- Add `!error` guard to list-view empty state (prevents rendering empty state alongside error banner) - Remove onClick from shared rows, use onDoubleClick only (consistent with normal FileBrowser; prevents triple-fire on double-click) - Fix triggerBrowserDownload: cast Uint8Array as BlobPart instead of using .buffer (avoids potential data corruption from extra bytes) - Replace hand-crafted PNG bytes with valid fixture file (69 bytes, correct CRC checksums); createTestImageFile now copies the fixture Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 812995ab46e2
This was referenced Feb 23, 2026
6 tasks
5 tasks
This was referenced Mar 21, 2026
Merged
Closed
This was referenced Mar 26, 2026
Merged
This was referenced Jun 16, 2026
This was referenced Jun 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
What's included
Crypto foundation:
reWrapKeyprimitive — unwrap with owner's private key, re-wrap with recipient's public key, zero plaintextAPI (NestJS):
WHERE revoked_at IS NULL) for soft-delete compatibilityFrontend (React):
Security review fixes (applied in final commit):
Test plan
pnpm --filter @cipherbox/crypto test)pnpm build).planning/security/REVIEW-2026-02-21-phase14.md)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores