feat: phase 15 link sharing#190
Merged
Merged
Conversation
WalkthroughPhase 15: Link Sharing implementation introduces a complete invite-link system with ephemeral key encryption. Adds a ShareInvite entity with database migration, REST API endpoints for public/authenticated invite operations, frontend UI components for creating and claiming invites, ephemeral key-based cryptography, and comprehensive end-to-end testing infrastructure. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Sharer)
participant Web as Web App
participant API as API Server
participant DB as Database
User->>Web: Click "Create Invite Link" in ShareDialog
Web->>Web: Generate ephemeral secp256k1 keypair
Note over Web: ephemeralPrivateKey (to be placed in URL fragment)
Web->>Web: Unwrap item key with vault privateKey
Web->>Web: Wrap item key with ephemeral publicKey
Web->>Web: Collect & wrap child keys (for folders)
Web->>API: POST /shares/invites {itemType, ipnsName, itemName, encryptedKey, encryptedChildKeys}
API->>API: Generate random token (base64url)
API->>API: Set expiry = now + 7 days
API->>DB: INSERT into share_invites (token, sharer_id, encryptedKey, ..., status='active')
DB-->>API: ShareInvite created
API-->>Web: {id, token, ...}
Web->>Web: Build URL: /invite/{token}#{ephemeralPrivKeyHex}
Web->>Web: Zero ephemeral material
User->>User: Copy & share invite URL
sequenceDiagram
participant Recipient as Recipient (Unauthenticated)
participant Web as Web App
participant API as API Server
participant Auth as Auth System
participant DB as Database
Recipient->>Web: Navigate to /invite/{token}#{ephemeralPrivKeyHex}
Web->>Web: Parse token & ephemeralPrivKey from URL
Web->>Web: Strip ephemeralPrivKey from address bar
Web->>API: GET /invites/{token} (public, throttled)
API->>DB: SELECT * from share_invites WHERE token = ? AND status='active'
DB-->>API: ShareInvite record (or null if expired)
API-->>Web: {status: 'active'} or error
Web->>Web: Render login options if valid
Recipient->>Web: Click "Login with Google" (or other auth)
Web->>Auth: Initiate OAuth flow
Auth-->>Web: JWT token + user context
Web->>Web: Auto-trigger claim flow (isOpen && authenticated && valid)
Web->>API: GET /invites/{token}/data (authenticated)
API->>DB: SELECT * from share_invites WHERE token = ? AND status='active'
DB-->>API: Full invite data (encryptedKey, encryptedChildKeys)
API-->>Web: {status, encryptedKey, encryptedChildKeys, itemType, ipnsName, itemName}
Web->>Web: Unwrap encryptedKey with ephemeralPrivKey
Web->>Web: Re-wrap with recipient's vault publicKey
Web->>Web: Re-wrap all childKeys with recipient key
Web->>API: POST /invites/{token}/claim {encryptedKey, childKeys}
API->>DB: BEGIN TRANSACTION
API->>DB: UPDATE share_invites SET status='claimed', claimed_by=?, claim_count++ WHERE token=? AND status='active'
API->>DB: INSERT into shares (item_ipns_name, ...) returning share_id
API->>DB: INSERT into share_keys (share_id, key_type, encrypted_key) for each childKey
DB-->>API: success or 409 Conflict (already claimed)
API->>DB: COMMIT TRANSACTION
API-->>Web: {shareId}
Web->>Web: Zero ephemeral key material
Web->>Web: Redirect to /shared
Recipient->>Web: View auto-claimed shared content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Phase 15: Link Sharing - Split Phase 15 into Link Sharing (15) and Client-Side Search (15.1) - Implementation decisions documented (invite model, ephemeral key bridge) - Phase boundary established (account-required invite links, not unauthenticated) - Design mockups added to .pen file (invite landing page, share dialog with tabs) - Roadmap and state updated to reflect phase split Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 0edb1bc2f4bf
Phase 15: Link Sharing - Standard stack identified (existing crypto primitives, no new deps) - Critical discovery: HashRouter fragment collision requires URL format change - Architecture patterns documented (entity, migration, claim flow, tab UI) - Pitfalls catalogued (6 items including migration discipline, auth guard) - Phase 14 codebase audit complete with file references Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: b68a1ee43cda
Phase 15: Link Sharing - 3 plans in 3 waves - 0 parallel, 3 sequential - Ready for execution Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 4740cf2db694
Address 5 checker issues: add authenticated GET /invites/:token/data endpoint for claim flow, split into two NestJS controllers for two path prefixes, make collectChildKeys extraction mandatory, split InvitePage task into landing page + auth integration, fix duplicate </output> tag. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: ea8cf70f5362
Pre-implementation review of ephemeral key bridge design. 0 critical, 3 high (all hardening), 3 medium, 1 low findings. No execution blockers - design is cryptographically sound. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: e711c610a51d
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 547f588c7fdb
Add missing 15-04-PLAN.md to roadmap plan list, update research flag from NEEDS to COMPLETE, and advance current position to Phase 15 planned. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 9b308ecfa201
- ShareInvite entity with token, encryptedKey, encryptedChildKeys (JSONB), status, expiresAt - Migration 1740400000000 creates share_invites table with IF NOT EXISTS - CreateInviteDto, ClaimInviteDto with class-validator decorators - InviteResponseDto, InviteStatusResponseDto, InviteDataResponseDto for all endpoints - entities/index.ts exports ShareInvite Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: a3dc9cde62c0
…tion - InvitesController at /invites: public status check, authenticated data fetch, claim - ShareInvitesController at /shares/invites: authenticated create, list, revoke - SharesService: 6 invite methods with auto-expire on read and atomic single-claim - ShareInvite registered in shares.module, app.module, and generate-openapi.ts - Regenerated API client with invites and share-invites endpoints Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: d87e9f5afafc
Tasks completed: 2/2 - ShareInvite entity, migration, and DTOs - Two controller classes, service methods, and module registration SUMMARY: .planning/phases/15-link-sharing/15-01-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: c4d1a5be010e
- Extract collectChildKeys and reWrapEncryptedKey to apps/web/src/lib/crypto/key-wrapping.ts - ShareDialog imports from shared utility instead of inline definitions - Enables code sharing between direct share (Phase 14) and invite link (Phase 15) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 17ecc5521db7
- generateEphemeralKeypair using secp256k1.keygen() for invite creation - createInviteLink wraps item+child keys with ephemeral pubkey, builds URL - claimInvite fetches from authenticated /invites/:token/data, unwraps+re-wraps - buildInviteUrl produces HashRouter-safe #/invite/:token?key=<hex> format - checkInviteStatus, fetchInvitesForItem, revokeInvite for invite management - All ephemeral and plaintext key material zeroed in finally blocks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: b1144c95ee6e
Tasks completed: 2/2 - Extract shared key-wrapping utilities from ShareDialog - Frontend invite service with ephemeral key bridge crypto SUMMARY: .planning/phases/15-link-sharing/15-02-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 69064e94ab85
- Add tab bar (DIRECT SHARE | INVITE LINK) with ARIA tablist/tab roles - Create InviteLinkTab with link creation, clipboard copy, and revoke - Widen modal to 600px via share-dialog-backdrop CSS - Add focus-visible styles for tab buttons and create button - Active invites shown with status badge and inline revoke confirm Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 8dc6a2eb42b0
- Create InvitePage with state machine (loading/valid/claiming/claimed/error) - Parse ephemeral key from hash fragment query param via useSearchParams - Check invite status on mount, show branded card or error state - Add /invite/:token route outside AppShell in routes/index.tsx - Create invite-page.css with terminal aesthetic, green/red card variants - Include MatrixBackground and StagingBanner matching Login page Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 9581541e6f3a
…ication - Add inline auth (Google, Email, Wallet) matching Login.tsx patterns - Auto-claim via useEffect watching isAuthenticated state transition - Handle MFA/REQUIRED_SHARE with DeviceWaitingScreen and RecoveryInput - Navigate to /shared after successful claim with 500ms delay - Zero ephemeral key ref in both success and finally paths - Fix pre-existing shares.service.spec.ts missing ShareInvite mock - Full monorepo builds, all 529 API tests pass, web lint clean Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 466b17a6972d
Tasks completed: 3/3 - ShareDialog tabbed interface + InviteLinkTab - InvitePage landing page with status check and route config - InvitePage auth integration, claim flow, and build verification SUMMARY: .planning/phases/15-link-sharing/15-03-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 2be087c9e6b4
- InviteLinkTabPage: tab switching, create link, clipboard intercept, invite list, revoke - InvitePageObject: navigation, state detection, auth area, error states, wait helpers - Updated barrel exports in dialogs/index.ts and page-objects/index.ts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 78bb9ac5187e
- 21 serial tests covering complete invite link lifecycle - ShareDialog tab UI: switching, default state, empty state - File invite: create link, capture URL, claim via authenticated context - Folder invite: create, claim, navigate folder tree - Link management: revoke, revoked landing page error - Error states: invalid URL, non-existent token, already-claimed, self-claim - URL parsing helper for HashRouter invite URLs - Multi-account setup with Alice (sharer), Dave, Eve (recipients) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tasks completed: 2/2 - Page objects for InviteLinkTab and InvitePage - Invite link E2E test suite (21 serial tests) Phase 15 (Link Sharing) complete: 4/4 plans executed. SUMMARY: .planning/phases/15-link-sharing/15-04-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 068d605becf5
InviteResponseDto was missing the `id` field, causing revoke to send a base64url token to ParseUUIDPipe which rejects non-UUID strings with 400. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 9ebcbebb2860
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: befc2640a74b
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 1a7590ec42cd
FSM1
force-pushed
the
feat/phase-15-link-sharing
branch
from
5342783 to
00a69cd
Compare
InvitesController (8 tests), ShareInvitesController (7 tests), and 24 invite service method tests. Fixes branch/function coverage thresholds (80%/85%). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: bd4c031eafd5
secp256k1.keygen() returns a 33-byte compressed public key, but wrapKey() requires a 65-byte uncompressed key (0x04 prefix). This caused "Key wrapping failed" when creating invite links for files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: f4b9aa21b2f0
InviteLinkTab initialized invitesLoading=false, causing a brief render of the empty state before useEffect fired to fetch invites. This race condition made E2E waitForLoaded() resolve on the empty state before the API response arrived, causing getInviteCount() to return 0. Initialize invitesLoading=true so the loading state shows until the first fetch completes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 0af6ee07a3f7
InvitePageObject.goto() derived the base URL from page.url(), but for
fresh browser contexts the URL is "about:blank". The split('#')[0]
fallback didn't trigger because "about:blank" is truthy. This caused
navigation to about:blank#/invite/... instead of localhost:5173.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Entire-Checkpoint: 7b4953f1eb41
The list endpoint only returns active invites (status='active'). The Phase 3 invite was claimed by Dave, so it correctly doesn't appear. Remove the toBeGreaterThanOrEqual(1) assertion; the rest of the test already handles creating a fresh invite to revoke. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: e3f4049b6635
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (15)
apps/web/src/styles/share-dialog.css (1)
365-390: Duplicated button styles — consider a shared utility class.
.invite-link-create-btnand.share-submit-btnare nearly identical (same padding, font, color, background, border, cursor, transition, hover,:focus-visible, and disabled rules). The only functional difference is theflex-shrink: 0on.share-submit-btn. Extracting a shared.btn-ghost-greenutility class would eliminate the duplication and make future theming changes a single-point edit.♻️ Proposed shared utility class
+/* Shared ghost green button base */ +.btn-ghost-green { + padding: var(--spacing-xs) var(--spacing-sm); + font-family: var(--font-family-mono); + font-size: var(--font-size-sm); + font-weight: var(--font-weight-semibold); + color: var(--color-green-primary); + background: transparent; + border: var(--border-thickness) solid var(--color-green-dim); + cursor: pointer; + transition: background-color 0.15s ease, color 0.15s ease, border-color 0.15s ease; +} + +.btn-ghost-green:hover:not(:disabled) { + background-color: var(--color-green-darker); + border-color: var(--color-green-primary); +} + +.btn-ghost-green:focus-visible { + outline: 1px solid var(--color-green-primary); + outline-offset: 1px; +} + +.btn-ghost-green:disabled { + opacity: 0.4; + cursor: not-allowed; +} -.share-submit-btn { - flex-shrink: 0; - padding: var(--spacing-xs) var(--spacing-sm); - font-family: var(--font-family-mono); - font-size: var(--font-size-sm); - font-weight: var(--font-weight-semibold); - color: var(--color-green-primary); - background: transparent; - border: var(--border-thickness) solid var(--color-green-dim); - cursor: pointer; - transition: background-color 0.15s ease, color 0.15s ease, border-color 0.15s ease; -} - -.share-submit-btn:hover:not(:disabled) { ... } -.share-submit-btn:focus-visible { ... } -.share-submit-btn:disabled { ... } +.share-submit-btn { + flex-shrink: 0; +} -.invite-link-create-btn { - padding: var(--spacing-xs) var(--spacing-sm); - ...all duplicated rules... -} - -.invite-link-create-btn:hover:not(:disabled) { ... } -.invite-link-create-btn:focus-visible { ... } -.invite-link-create-btn:disabled { ... }Then add
btn-ghost-greenalongsideshare-submit-btn/invite-link-create-btnin the JSX.🤖 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 365 - 390, Extract the duplicated styles from .invite-link-create-btn and .share-submit-btn into a new utility class .btn-ghost-green containing the shared padding, font-family, font-size, font-weight, color, background, border, cursor, transition, hover, :focus-visible, and :disabled rules; keep only the unique rule (flex-shrink: 0) on .share-submit-btn and remove duplicated rules from .invite-link-create-btn, then update the JSX to add the .btn-ghost-green class alongside .share-submit-btn and .invite-link-create-btn so both buttons inherit the shared styling.apps/web/src/components/file-browser/InviteLinkTab.tsx (2)
139-141:role="tabpanel"is missingaria-labelledbyto associate it with its controlling tab.For ARIA compliance, a
tabpanelshould reference theidof the tab element that controls it viaaria-labelledby. The parentShareDialogpresumably renders a tab with anid— pass that as a prop or use a convention.Proposed fix
- <div className="invite-link-tab" role="tabpanel"> + <div className="invite-link-tab" role="tabpanel" aria-labelledby="share-tab-invite-link">Ensure the corresponding tab element in
ShareDialog.tsxhasid="share-tab-invite-link".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/InviteLinkTab.tsx` around lines 139 - 141, The InviteLinkTab component renders a div with role="tabpanel" but lacks aria-labelledby; update InviteLinkTab to accept a prop (e.g., tabId or labelledBy) and set aria-labelledby={tabId} on the div, and ensure the corresponding tab in ShareDialog uses id="share-tab-invite-link" (or pass that id into InviteLinkTab) so the tabpanel is properly associated with its controlling tab.
42-68: Stale success/error messages persist across tab reopens.When
isOpentransitions totrue, the effect refetches invites but doesn't clearerrorandsuccessstate from a previous session. A user could see a stale "link copied" message or old error from a prior interaction.Proposed fix
useEffect(() => { if (!isOpen) return; let cancelled = false; setInvitesLoading(true); + setError(null); + setSuccess(null); + setConfirmRevokeId(null); fetchInvitesForItem(ipnsName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/InviteLinkTab.tsx` around lines 42 - 68, When the tab opens, clear any previous success/error state so stale messages don't persist: inside the useEffect that runs when isOpen becomes true (the block that calls fetchInvitesForItem and uses setInvitesLoading), add calls to reset the component's error and success state (e.g. call setError(null/undefined) and setSuccess(null/undefined) or the actual setters used in this component) before starting the fetch; keep the existing cancelled checks and loading handling intact.apps/web/src/styles/invite-page.css (2)
116-135:.invite-card__loading,.invite-card__claiming, and.invite-card__successare nearly identical.All three share the same
font-family,font-size, andtext-align— they differ only incolor. A shared base class (or grouping the common properties) would reduce duplication.Proposed consolidation
+.invite-card__loading, +.invite-card__claiming, +.invite-card__success { + font-family: var(--font-family-mono); + font-size: var(--font-size-sm); + text-align: center; +} + .invite-card__loading { - font-family: var(--font-family-mono); - font-size: var(--font-size-sm); color: var(--color-text-secondary); - text-align: center; } .invite-card__claiming { - font-family: var(--font-family-mono); - font-size: var(--font-size-sm); color: var(--color-green-primary); - text-align: center; } .invite-card__success { - font-family: var(--font-family-mono); - font-size: var(--font-size-sm); color: var(--color-green-primary); - text-align: center; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/styles/invite-page.css` around lines 116 - 135, These three classes (.invite-card__loading, .invite-card__claiming, .invite-card__success) duplicate font-family, font-size, and text-align; consolidate common properties by creating a shared base selector (e.g., .invite-card__status or grouping the three selectors together) that contains font-family: var(--font-family-mono), font-size: var(--font-size-sm), and text-align: center, then keep only the differing color declarations on .invite-card__loading, .invite-card__claiming, and .invite-card__success to remove duplication.
39-41: Hardcoded#ef4444error color used in 3 places — consider using a CSS variable.Lines 40, 144, and 189 all use the hardcoded
#ef4444color while the rest of the file consistently uses CSS variables. Extracting this to a design token (e.g.,--color-error) would keep the invite page consistent with the design system and simplify future theme changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/styles/invite-page.css` around lines 39 - 41, Replace the three hardcoded uses of `#ef4444` with a CSS variable (e.g., --color-error) and update the .invite-card--error rule to use var(--color-error); add or reference the design token in the stylesheet (or :root) so other rules on lines 144 and 189 can use var(--color-error) as well, ensuring consistency with the file's existing CSS variable usage and theming conventions.apps/api/src/shares/share-invites.controller.spec.ts (1)
22-22: Consider typingmockReqto avoid repeatedas anycasts.The lint warnings on lines 28, 77, 100, 118, 135, 143, 154 are all about
any. You can reduce noise by typingmockReqmore precisely and castingsharerto the User entity type.🔧 Suggested minimal type for mockReq
- const mockReq: { user: { id: string } } = { user: { id: userId } }; + const mockReq = { user: { id: userId } } as RequestWithUser;This requires importing
RequestWithUser(or defining a compatible local type). Similarly for line 28:- sharer: {} as any, + sharer: { id: userId } as User,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/share-invites.controller.spec.ts` at line 22, The tests are using an untyped mockReq which forces multiple "as any" casts; replace the inline type for mockReq with the proper RequestWithUser (or define a local compatible interface) and import it where needed, then replace ad-hoc casts by casting the test user/sharer to the actual User entity type (e.g., cast sharer to User) so occurrences around mockReq and variables named sharer no longer require "as any" casts; update any test declarations that reference mockReq, RequestWithUser, or sharer to use these types consistently.tests/e2e/page-objects/dialogs/invite-link-tab.page.ts (1)
208-216: Clipboard interceptor doesn't restore the originalwriteText— may leak across tests.If multiple tests run sequentially in the same page context, the patched
writeTextaccumulates wrappers. Consider restoring the original in a cleanup method, or at least making the interceptor idempotent.🔧 Idempotent interceptor
async interceptClipboard(): Promise<void> { await this.page.evaluate(() => { - (window as unknown as Record<string, string>).__clipboardContent = ''; - const original = navigator.clipboard.writeText.bind(navigator.clipboard); - navigator.clipboard.writeText = async (text: string) => { - (window as unknown as Record<string, string>).__clipboardContent = text; - return original(text); - }; + const w = window as unknown as Record<string, unknown>; + if (!w.__clipboardIntercepted) { + const original = navigator.clipboard.writeText.bind(navigator.clipboard); + navigator.clipboard.writeText = async (text: string) => { + (w as Record<string, string>).__clipboardContent = text; + return original(text); + }; + w.__clipboardIntercepted = true; + } + (w as Record<string, string>).__clipboardContent = ''; }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/page-objects/dialogs/invite-link-tab.page.ts` around lines 208 - 216, The interceptClipboard method currently replaces navigator.clipboard.writeText without restoring it or guarding against repeated patches; make it idempotent and/or provide a restore hook: when interceptClipboard runs, detect and preserve the original writeText (store it on window under a unique symbol like __originalClipboardWriteText) and only wrap if not already patched, and add a complementary restoreClipboard method that restores navigator.clipboard.writeText from __originalClipboardWriteText and clears __clipboardContent; reference interceptClipboard, __clipboardContent, __originalClipboardWriteText, and navigator.clipboard.writeText to locate and implement the fix.apps/web/src/components/file-browser/ShareDialog.tsx (1)
350-370: ARIA tabs pattern: consider addingid,aria-controls, andaria-labelledbyfor screen reader linkage.The
role="tablist",role="tab", andaria-selectedattributes are correct. For full WAI-ARIA Tabs Pattern compliance, each tab should have anidandaria-controlspointing to the panelid, and each panel should havearia-labelledbypointing back to its tab.♿ Proposed enhancement
<button type="button" role="tab" + id="share-tab-direct" aria-selected={activeTab === 'direct'} + aria-controls="share-panel-direct" className={`share-tab${activeTab === 'direct' ? ' share-tab--active' : ''}`} onClick={() => setActiveTab('direct')} > {'DIRECT SHARE'} </button> <button type="button" role="tab" + id="share-tab-invite" aria-selected={activeTab === 'invite'} + aria-controls="share-panel-invite" className={`share-tab${activeTab === 'invite' ? ' share-tab--active' : ''}`} onClick={() => setActiveTab('invite')} > {'INVITE LINK'} </button> // And on the panels: - <div role="tabpanel"> + <div role="tabpanel" id="share-panel-direct" aria-labelledby="share-tab-direct">As per coding guidelines:
apps/web/**: "Focus on: Accessibility (a11y) concerns."🤖 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 350 - 370, The tab buttons in ShareDialog (elements with className "share-tab" and state activeTab / setter setActiveTab inside the "share-tab-bar") need accessible linkage: give each tab button a stable id and an aria-controls that references the corresponding panel id (e.g., "share-tab-direct" -> "share-panel-direct" and "share-tab-invite" -> "share-panel-invite"), and update each panel element to include the matching id and aria-labelledby pointing back to its tab id; ensure aria-selected is kept and the onClick handlers (setActiveTab) still toggle tabs.apps/api/src/shares/dto/invite-response.dto.ts (1)
60-68: ExtractInviteChildKeyDtoto resolve loose OpenAPI schema forencryptedChildKeys.The inline type for
encryptedChildKeyscauses NestJS Swagger to generate an incorrect OpenAPI schema:"type": "object"instead of"type": "array"with proper item definitions. This results in loose typing in the generated API client.
InviteChildKeyDtoalready exists increate-invite.dto.tswith proper@ApiPropertydecorators and generates the correct array schema there. Extract it to a shared location (or move fromcreate-invite.dto.ts) and reuse it inInviteDataResponseDtoto ensure consistent, properly-typed OpenAPI documentation.Suggested refactor
+class InviteChildKeyDto { + `@ApiProperty`({ + description: 'Type of key: file or folder', + enum: ['file', 'folder'], + }) + keyType!: 'file' | 'folder'; + + `@ApiProperty`({ + description: 'UUID of the file or subfolder', + }) + itemId!: string; + + `@ApiProperty`({ + description: 'Hex-encoded ECIES ciphertext of the key wrapped with ephemeral public key', + }) + encryptedKey!: string; +} + export class InviteDataResponseDto { `@ApiProperty`({ description: 'Array of child keys wrapped with ephemeral public key, or null', nullable: true, + type: [InviteChildKeyDto], }) - encryptedChildKeys!: Array<{ - keyType: 'file' | 'folder'; - itemId: string; - encryptedKey: string; - }> | null; + encryptedChildKeys!: InviteChildKeyDto[] | null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/dto/invite-response.dto.ts` around lines 60 - 68, Replace the inline object type for encryptedChildKeys in InviteDataResponseDto with the shared InviteChildKeyDto: move or centralize InviteChildKeyDto (currently in create-invite.dto.ts) into a shared DTO file or export it from its module, then import it into invite-response.dto.ts and change encryptedChildKeys to use InviteChildKeyDto[] | null and update the `@ApiProperty` decorator to reference the DTO (e.g. type: () => InviteChildKeyDto, isArray: true, nullable: true) so Swagger generates an array schema with proper item definitions.apps/api/src/shares/shares.service.ts (1)
367-379: Auto-expire hard-deletes invites on GET — verify this is intended for the status endpoint.
getInviteStatusis called from the public (unauthenticated) endpoint. A malicious actor could force-expire an invite by polling with the token after the TTL passes. Since expiry is TTL-based and the invite is already past its useful life, this is likely benign, but worth noting that any unauthenticated caller with the token can trigger the delete.Additionally, if
getInviteStatusandgetInviteForClaimrace on the same expired invite, one could delete the entity while the other is mid-operation. The likelihood is low given the narrow window, but consider soft-expiring (settingstatus = 'expired') instead of hard-deleting to avoid potentialEntityNotFounderrors from concurrent access.🤖 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 367 - 379, getInviteStatus currently hard-deletes expired invites (inviteRepo.remove) when called from the public endpoint, which allows anyone with the token to trigger deletion and can race with getInviteForClaim; change this to perform a safe "soft-expire" update: in getInviteStatus, when invite.status === 'active' && invite.expiresAt < new Date(), set invite.status = 'expired' and persist via inviteRepo.save() or inviteRepo.update({ id: invite.id }, { status: 'expired' }) instead of remove, and ensure callers rely on invite.status to determine validity; optionally wrap the update in a transaction or use optimistic locking if your entity supports versioning to avoid races with getInviteForClaim.tests/e2e/tests/invite-link-workflow.spec.ts (1)
38-47:parseInviteUrlwill throw on malformed URLs.If the clipboard returns an unexpected value (no
#, no?key=),hashPartisundefinedand the subsequent.split('?')throws an unhandled error, producing a confusing test failure. Adding a guard or using theURLconstructor would make failures more diagnosable.🔧 Suggested defensive version
function parseInviteUrl(url: string): { token: string; ephemeralKey: string } { - // URL format: http://localhost:5173/#/invite/TOKEN?key=KEY - const hashPart = url.split('#')[1]; // /invite/TOKEN?key=KEY - const [path, query] = hashPart.split('?'); - const token = path.split('/invite/')[1]; - const params = new URLSearchParams(query); - const ephemeralKey = params.get('key')!; + const hashIndex = url.indexOf('#'); + if (hashIndex === -1) throw new Error(`Invalid invite URL (no hash): ${url}`); + const hashPart = url.slice(hashIndex + 1); + const [path, query] = hashPart.split('?'); + const token = path.split('/invite/')[1]; + if (!token) throw new Error(`Invalid invite URL (no token): ${url}`); + const params = new URLSearchParams(query); + const ephemeralKey = params.get('key'); + if (!ephemeralKey) throw new Error(`Invalid invite URL (no key): ${url}`); return { token, ephemeralKey }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/invite-link-workflow.spec.ts` around lines 38 - 47, The parseInviteUrl function currently assumes the input contains a hash and query and will throw on malformed URLs; update parseInviteUrl to defensively validate the URL parts (or use the built-in URL constructor) before splitting: check that url.includes('#') and that the hash part contains '/invite/' and a '?key=' (or parse via new URL(urlWithHash) after ensuring a base origin) and throw a clear, descriptive error if missing; reference the function name parseInviteUrl and the local variables hashPart, path, token, ephemeralKey when adding the guards so the test failure becomes explicit instead of an unhandled exception.apps/api/src/shares/invites.controller.spec.ts (2)
21-27: Consider typingmockReqandmockInvite.sharerto satisfy lint.The linter flags
anyat lines 27, 136, and 149. Forsharer, a minimal typed stub would silence the warning. FormockReq, it's already typed but the controller parameter expects the fullRequestinterface, so theas anycast at call sites is pragmatic for tests.🔧 Suggested minimal fix for the sharer field
- sharer: {} as any, + sharer: { id: '660e8400-e29b-41d4-a716-446655440001' } as unknown as ShareInvite['sharer'],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/invites.controller.spec.ts` around lines 21 - 27, The test linter errors come from using `any` for mock objects; replace the `mockInvite.sharer` stub with a minimal typed object (e.g., a Partial<User>-shape with only the properties the controller accesses such as id and email) instead of `{} as any`, and change `mockReq` to a narrower Request-compatible type (e.g., `Partial<Request> & { user: { id: string } }` or import Express' Request and use `Partial<Request>`) while keeping the existing `as any` casts at controller call sites if needed; update references in invites.controller.spec.ts to use these types for `mockReq` and `mockInvite.sharer` (symbols: mockReq, mockInvite, sharer) to satisfy the linter.
107-114: Double invocation ofgetInviteDatain the NotFoundException test.Lines 110–113 call
controller.getInviteData(testToken)twice — once to assert the exception type and once for the message. Each call invokes the mock again. Consider chaining the assertion or usingrejects.toThrowwith a single call to avoid redundant execution:🔧 Suggested fix
- await expect(controller.getInviteData(testToken)).rejects.toThrow(NotFoundException); - await expect(controller.getInviteData(testToken)).rejects.toThrow( - 'Invite not found or expired' - ); + await expect(controller.getInviteData(testToken)).rejects.toThrow( + new NotFoundException('Invite not found or expired') + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/invites.controller.spec.ts` around lines 107 - 114, The test calls controller.getInviteData(testToken) twice causing duplicate mock invocations; replace the two expects with a single assertion that checks both type and message in one call — e.g. call await expect(controller.getInviteData(testToken)).rejects.toThrowError(new NotFoundException('Invite not found or expired')) so the test uses one invocation of getInviteData and still verifies the NotFoundException and its message (referencing controller.getInviteData and mockSharesService.getInviteForClaim).apps/api/src/shares/invites.controller.ts (1)
18-22: ExtractRequestWithUserinterface to a shared location to reduce duplication.This interface is defined in 8 separate controller files across the codebase (vault, shares, share-invites, invites, ipfs, ipns, device-approval, and auth controllers). Extract it to a common location such as
src/common/interfaces/request-with-user.interface.tsto maintain a single source of truth and prevent drift if the interface needs future updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/invites.controller.ts` around lines 18 - 22, Extract the local RequestWithUser interface into a single shared module (exporting an interface with user: { id: string }) and update invites.controller.ts to import that shared interface instead of declaring it locally; then remove the duplicated interface declaration from this file and replace equivalent declarations in the other controllers (vault, shares, share-invites, invites, ipfs, ipns, device-approval, auth) to import the shared RequestWithUser to keep one source of truth.apps/web/src/services/invite.service.ts (1)
299-308:checkInviteStatusswallows all errors as'expired'.Network failures, server 500s, and auth errors will all appear as "expired" to the user. This is acceptable for a status pre-check (user sees a recoverable error card), but consider logging the error at
warnlevel (without sensitive data) for debugging.Proposed change
} catch { + // Network/server errors treated as expired for UX, but log for debugging + // (no sensitive data in status check) return 'expired'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/services/invite.service.ts` around lines 299 - 308, The function checkInviteStatus currently swallows all exceptions and returns 'expired'; update its catch to capture the error (e.g. catch (err)) and emit a warning-level log that includes the error message/stack but not the token or other sensitive data, then continue returning 'expired'; use the existing application logger (or import a shared logger) for the warning so you log via the same facility as other services; keep the return behavior unchanged, only add the logging around invitesControllerGetInviteStatus failure.
🤖 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/api/src/migrations/1740400000000-AddShareInvites.ts`:
- Around line 16-37: Add a foreign key on share_invites.claimed_by to users(id)
to prevent orphaned UUIDs: alter the CREATE TABLE block for "share_invites" to
include a CONSTRAINT (e.g., "FK_share_invites_claimed_by") FOREIGN KEY
("claimed_by") REFERENCES "users" ("id") with an appropriate ON DELETE action
(recommend ON DELETE SET NULL so a deleted user doesn't drop the invite) and ON
UPDATE NO ACTION; additionally decide whether multi-claim behavior is
intended—either remove "max_claims" if never used or add a code comment
documenting that "max_claims" supports multiple claimers while "claimed_by" is
only the last/single claimer and adjust schema (e.g., separate claim table) if
true.
In `@apps/api/src/shares/entities/share-invite.entity.ts`:
- Around line 17-18: The token column in the ShareInvite entity is missing the
TypeORM unique flag; update the `@Column` decorator for the token property (token
in the ShareInvite entity/class) to include unique: true so the entity matches
the existing DB constraint (UQ_share_invites_token) and implicit index created
by the migration.
In `@apps/api/src/shares/invites.controller.ts`:
- Around line 111-131: The claimInvite controller method returns an object {
shareId } but the `@ApiResponse`({ status: 201 }) has no response schema; add a
small DTO (e.g., ClaimInviteResponseDto with an `@ApiProperty`() shareId: string)
and update the decorator to `@ApiResponse`({ status: 201, type:
ClaimInviteResponseDto }) so the OpenAPI spec includes the response shape;
ensure the DTO is exported/imported and ApiProperty is imported from
`@nestjs/swagger` and referenced in the claimInvite method's decorator.
In `@apps/api/src/shares/share-invites.controller.ts`:
- Around line 22-26: Duplicate RequestWithUser interfaces across multiple
controllers should be consolidated into one shared type module (e.g.,
request.types) that exports interface RequestWithUser extends Request { user: {
id: string } }; create that module, update all controllers that currently
declare RequestWithUser (vault, auth, ipfs, device-approval, shares/*, ipns) to
import the shared RequestWithUser instead of redeclaring it, and standardize on
the same Request type by replacing any use of ExpressRequest with Request from
'express' so all imports reference the same Express Request type; finally remove
the local interface declarations from each controller.
- Around line 104-106: Create a ListInvitesQueryDto (e.g., class
ListInvitesQueryDto with `@IsString`, `@IsNotEmpty`, `@MaxLength`(255) and
`@ApiProperty` on ipnsName) and update the listInvites controller method signature
to accept `@Query`() query: ListInvitesQueryDto and pass query.ipnsName into the
service call instead of the raw `@Query`('ipnsName') ipnsName: string; also move
the RequestWithUser type into a shared types module (e.g., request-with-user.ts)
and import that shared type where controllers currently declare RequestWithUser
to remove duplication across controllers.
In `@apps/web/src/api/models/inviteDataResponseDto.ts`:
- Around line 17-21: The `@ApiProperty`() on the encryptedChildKeys field is
missing array metadata; update the decorator on encryptedChildKeys in
InviteResponseDto (the DTO class where encryptedChildKeys is declared) to
include type: 'array' and an items schema (either items: { $ref: ... } pointing
to the DTO/class that represents a single encrypted child key or items: { type:
'object', properties: { keyType: { type: 'string' }, itemId: { type: 'string' },
encryptedKey: { type: 'string' } } } ) and keep nullable: true and the existing
description so Orval generates Array<...> | null instead of { [key:string]:
unknown } | null; reference the encryptedChildKeys property and the DTO class
name used for individual child-key objects when adding the items metadata.
In `@apps/web/src/api/models/inviteDataResponseDtoEncryptedChildKeys.ts`:
- Around line 9-13: The response DTO's encryptedChildKeys is missing an explicit
ApiProperty type causing the generated client type to be { [key: string]:
unknown } | null; update the InviteDataResponseDto class to annotate
encryptedChildKeys with `@ApiProperty`({ type: [InviteChildKeyDto], nullable: true
}) (or equivalent) so it becomes Array<InviteChildKeyDto> | null, then re-run
the client generator (pnpm api:generate) to regenerate the typed frontend model.
In `@apps/web/src/components/file-browser/InviteLinkTab.tsx`:
- Around line 70-105: The handleCreate function currently wraps both
createInviteLink and fetchInvitesForItem in a single try/catch so a failure
during fetchInvitesForItem overwrites the successful creation message; split the
logic so createInviteLink (and clipboard write + setSuccess) are awaited in
their own try block and only then call fetchInvitesForItem inside a separate
try/catch; on fetch failure log the error and setError (or ignore) but do not
clear or overwrite the previously setSuccess; refer to handleCreate,
createInviteLink, fetchInvitesForItem, setSuccess, setError, and setInvites when
making this change.
- Around line 83-93: The current code swallows navigator.clipboard.writeText
errors but always calls setSuccess("link copied to clipboard..."), which is
misleading and can lose the ephemeral URL; update InviteLinkTab so the clipboard
write result controls the message: attempt navigator.clipboard.writeText(url)
and on success call setSuccess with the truncated displayUrl, but on failure
setSuccess to a different message that includes the truncated displayUrl (or a
clear "copy failed — here is the link" text) and ensure the raw url is stored in
component state (e.g., inviteUrl/url state) or rendered so the user can manually
copy it; reference navigator.clipboard.writeText, displayUrl, setSuccess and the
url variable when making the change.
In `@apps/web/src/lib/crypto/key-wrapping.ts`:
- Around line 71-119: The folder branch calls
reWrapEncryptedKey(folder.folderKeyEncrypted, ownerPrivateKey,
targetPubKeyBytes) without a try/catch, so if it throws it aborts traversal;
wrap that call (and the subsequent childKeys.push/wrapped/onProgress) in a
try/catch mirroring the file branch: on error log the failure (include folder.id
or folder.name and the error), skip adding that folder's key (do not increment
wrapped or call onProgress), and continue processing remaining children; keep
the existing recursion
(resolveIpnsRecord/unwrapKey/decryptFolderMetadata/collectChildKeys) unchanged
and ensure folderKeyBytes is still zeroed in its finally block.
In `@apps/web/src/routes/index.tsx`:
- Around line 6-13: InvitePage reads the ephemeral key into ephemeralKeyRef from
searchParams.get('key') but never removes it from the URL; update the mount
logic in InvitePage (where ephemeralKeyRef is set) to immediately call the
router navigate to the same path without the key query param (use the existing
navigate from react-router and pass replace: true) so the key is stripped from
the address bar and not written as a new history entry; ensure you construct the
new location from the current pathname and any other safe params but omit 'key'.
In `@apps/web/src/routes/InvitePage.tsx`:
- Around line 76-91: The effect calling checkInviteStatus(token) must handle
rejected promises: append a .catch(...) to the checkInviteStatus(token) call to
handle network/errors, check the existing cancelled flag inside the catch to
avoid state updates after unmount, and then call setErrorReason(...) (or set a
generic error reason like 'network') and setPageState('error'); also log the
error (console.error or process logger) so rejections are not unhandled. Ensure
you reference the existing cancelled variable, checkInviteStatus, setPageState,
and setErrorReason so the handler updates the component state correctly.
- Line 84: The code is casting an arbitrary status to ErrorReason
(setErrorReason(status as ErrorReason)), which can produce undefined error text;
change the assignment to validate the returned status against your allowed
ErrorReason values (or ERROR_MESSAGES keys) and if it is not recognized, fall
back to 'invalid' before calling setErrorReason (e.g., compute const reason =
isValidErrorReason(status) ? status : 'invalid' and then
setErrorReason(reason)). Ensure you reference the symbols setErrorReason,
checkInviteStatus, ErrorReason, and ERROR_MESSAGES when implementing the
guard/fallback.
- Around line 44-58: useSearchParams is reading query params but the ephemeral
key is in the URL hash fragment, so change how ephemeralKeyRef is initialized:
read window.location.hash (or parse key=value from the hash like "#key=...") and
set ephemeralKeyRef.current to that value instead of using
useSearchParams.get('key'); update any code that depends on ephemeralKeyRef
accordingly and remove the unused useSearchParams import; ensure you do not log
or persist the key anywhere (no console.log or storage).
- Around line 104-128: The setTimeout started inside the claimInvite promise
chain can fire after the component unmounts; to fix it, capture the timer id
(e.g., const timerRef = useRef<number | null>(null)) when calling setTimeout in
the claimInvite.then block and replace direct setTimeout with timerRef.current =
window.setTimeout(...), then clear it in a cleanup (useEffect return) or on
unmount via if (timerRef.current) { clearTimeout(timerRef.current);
timerRef.current = null; } to prevent navigate('/shared', { replace: true })
from running on an unmounted component; keep existing ephemeralKeyRef.current =
null behavior in finally.
In `@apps/web/src/services/invite.service.ts`:
- Around line 279-284: The client uses an unsafe cast around
invitesControllerClaimInvite because the backend claim endpoint lacks an
explicit ApiResponse type, causing Orval to generate void; fix the backend
controller method that handles the claim (the claim/claimInvite handler) by
adding an `@ApiResponse`({ status: 201, description: 'Invite claimed
successfully', type: <DTOClass> }) where <DTOClass> is a DTO that defines {
shareId: string } (or create a new DTO like ClaimInviteResponseDto with a
shareId field) so Orval can regenerate correct types and you can remove the as
unknown as { shareId: string } cast in invite.service.ts.
In `@designs/cipher-box-design.pen`:
- Around line 40956-40964: The text node with id "gzqpC" (name: policyNote)
hardcodes “expires in 7 days, single-claim” which may diverge from backend
TTL/claim policy; verify the backend contract for invite TTL and single-claim
semantics and update this node's content (and the corresponding expired-state
copy elsewhere that currently reads “valid for 7 days”) to either reflect the
actual configurable values or use a generic phrase (e.g., “expires after the
configured period” / “single-use” only when guaranteed). Ensure both the
policyNote ("gzqpC") and the matching expired-state copy are kept in sync or
replaced with a variable-driven/generic string.
- Around line 39304-39312: The subtext element (id "TBokI", name "subtext") uses
fill "#006644" at fontSize 11 and JetBrains Mono which likely fails WCAG AA;
update this element to use the project’s accessible muted-text token (or a
lighter hex color that meets 4.5:1 contrast against black) or increase the text
severity (e.g., bump fontSize and/or fontWeight) so the combination of fill,
fontSize 11/JetBrains Mono meets AA; apply the same change to any other elements
using "#006644" for small text.
In `@packages/api-client/openapi.json`:
- Around line 1612-1614: The `@ApiResponse` decorator on the claim endpoint lacks
a response schema so the OpenAPI spec omits the { shareId: string } body; update
the `@ApiResponse` on the controller method that returns the claim response
(invites.controller.ts, the claim handler) to include a schema describing an
object with a string property "shareId" (and optional example), so the generated
spec shows the response body structure.
- Around line 2871-2875: The OpenAPI schema incorrectly emits encryptedChildKeys
as "object" because the InviteDataResponseDto.encryptedChildKeys `@ApiProperty`
lacks an explicit type; update the decorator to specify the array element type
(e.g., type: [InviteChildKeyDto] or use a class reference matching
CreateInviteDto's pattern) so the generated schema becomes type: "array" with
the correct item shape, then regenerate the spec with pnpm api:generate.
---
Nitpick comments:
In `@apps/api/src/shares/dto/invite-response.dto.ts`:
- Around line 60-68: Replace the inline object type for encryptedChildKeys in
InviteDataResponseDto with the shared InviteChildKeyDto: move or centralize
InviteChildKeyDto (currently in create-invite.dto.ts) into a shared DTO file or
export it from its module, then import it into invite-response.dto.ts and change
encryptedChildKeys to use InviteChildKeyDto[] | null and update the `@ApiProperty`
decorator to reference the DTO (e.g. type: () => InviteChildKeyDto, isArray:
true, nullable: true) so Swagger generates an array schema with proper item
definitions.
In `@apps/api/src/shares/invites.controller.spec.ts`:
- Around line 21-27: The test linter errors come from using `any` for mock
objects; replace the `mockInvite.sharer` stub with a minimal typed object (e.g.,
a Partial<User>-shape with only the properties the controller accesses such as
id and email) instead of `{} as any`, and change `mockReq` to a narrower
Request-compatible type (e.g., `Partial<Request> & { user: { id: string } }` or
import Express' Request and use `Partial<Request>`) while keeping the existing
`as any` casts at controller call sites if needed; update references in
invites.controller.spec.ts to use these types for `mockReq` and
`mockInvite.sharer` (symbols: mockReq, mockInvite, sharer) to satisfy the
linter.
- Around line 107-114: The test calls controller.getInviteData(testToken) twice
causing duplicate mock invocations; replace the two expects with a single
assertion that checks both type and message in one call — e.g. call await
expect(controller.getInviteData(testToken)).rejects.toThrowError(new
NotFoundException('Invite not found or expired')) so the test uses one
invocation of getInviteData and still verifies the NotFoundException and its
message (referencing controller.getInviteData and
mockSharesService.getInviteForClaim).
In `@apps/api/src/shares/invites.controller.ts`:
- Around line 18-22: Extract the local RequestWithUser interface into a single
shared module (exporting an interface with user: { id: string }) and update
invites.controller.ts to import that shared interface instead of declaring it
locally; then remove the duplicated interface declaration from this file and
replace equivalent declarations in the other controllers (vault, shares,
share-invites, invites, ipfs, ipns, device-approval, auth) to import the shared
RequestWithUser to keep one source of truth.
In `@apps/api/src/shares/share-invites.controller.spec.ts`:
- Line 22: The tests are using an untyped mockReq which forces multiple "as any"
casts; replace the inline type for mockReq with the proper RequestWithUser (or
define a local compatible interface) and import it where needed, then replace
ad-hoc casts by casting the test user/sharer to the actual User entity type
(e.g., cast sharer to User) so occurrences around mockReq and variables named
sharer no longer require "as any" casts; update any test declarations that
reference mockReq, RequestWithUser, or sharer to use these types consistently.
In `@apps/api/src/shares/shares.service.ts`:
- Around line 367-379: getInviteStatus currently hard-deletes expired invites
(inviteRepo.remove) when called from the public endpoint, which allows anyone
with the token to trigger deletion and can race with getInviteForClaim; change
this to perform a safe "soft-expire" update: in getInviteStatus, when
invite.status === 'active' && invite.expiresAt < new Date(), set invite.status =
'expired' and persist via inviteRepo.save() or inviteRepo.update({ id: invite.id
}, { status: 'expired' }) instead of remove, and ensure callers rely on
invite.status to determine validity; optionally wrap the update in a transaction
or use optimistic locking if your entity supports versioning to avoid races with
getInviteForClaim.
In `@apps/web/src/components/file-browser/InviteLinkTab.tsx`:
- Around line 139-141: The InviteLinkTab component renders a div with
role="tabpanel" but lacks aria-labelledby; update InviteLinkTab to accept a prop
(e.g., tabId or labelledBy) and set aria-labelledby={tabId} on the div, and
ensure the corresponding tab in ShareDialog uses id="share-tab-invite-link" (or
pass that id into InviteLinkTab) so the tabpanel is properly associated with its
controlling tab.
- Around line 42-68: When the tab opens, clear any previous success/error state
so stale messages don't persist: inside the useEffect that runs when isOpen
becomes true (the block that calls fetchInvitesForItem and uses
setInvitesLoading), add calls to reset the component's error and success state
(e.g. call setError(null/undefined) and setSuccess(null/undefined) or the actual
setters used in this component) before starting the fetch; keep the existing
cancelled checks and loading handling intact.
In `@apps/web/src/components/file-browser/ShareDialog.tsx`:
- Around line 350-370: The tab buttons in ShareDialog (elements with className
"share-tab" and state activeTab / setter setActiveTab inside the
"share-tab-bar") need accessible linkage: give each tab button a stable id and
an aria-controls that references the corresponding panel id (e.g.,
"share-tab-direct" -> "share-panel-direct" and "share-tab-invite" ->
"share-panel-invite"), and update each panel element to include the matching id
and aria-labelledby pointing back to its tab id; ensure aria-selected is kept
and the onClick handlers (setActiveTab) still toggle tabs.
In `@apps/web/src/services/invite.service.ts`:
- Around line 299-308: The function checkInviteStatus currently swallows all
exceptions and returns 'expired'; update its catch to capture the error (e.g.
catch (err)) and emit a warning-level log that includes the error message/stack
but not the token or other sensitive data, then continue returning 'expired';
use the existing application logger (or import a shared logger) for the warning
so you log via the same facility as other services; keep the return behavior
unchanged, only add the logging around invitesControllerGetInviteStatus failure.
In `@apps/web/src/styles/invite-page.css`:
- Around line 116-135: These three classes (.invite-card__loading,
.invite-card__claiming, .invite-card__success) duplicate font-family, font-size,
and text-align; consolidate common properties by creating a shared base selector
(e.g., .invite-card__status or grouping the three selectors together) that
contains font-family: var(--font-family-mono), font-size: var(--font-size-sm),
and text-align: center, then keep only the differing color declarations on
.invite-card__loading, .invite-card__claiming, and .invite-card__success to
remove duplication.
- Around line 39-41: Replace the three hardcoded uses of `#ef4444` with a CSS
variable (e.g., --color-error) and update the .invite-card--error rule to use
var(--color-error); add or reference the design token in the stylesheet (or
:root) so other rules on lines 144 and 189 can use var(--color-error) as well,
ensuring consistency with the file's existing CSS variable usage and theming
conventions.
In `@apps/web/src/styles/share-dialog.css`:
- Around line 365-390: Extract the duplicated styles from
.invite-link-create-btn and .share-submit-btn into a new utility class
.btn-ghost-green containing the shared padding, font-family, font-size,
font-weight, color, background, border, cursor, transition, hover,
:focus-visible, and :disabled rules; keep only the unique rule (flex-shrink: 0)
on .share-submit-btn and remove duplicated rules from .invite-link-create-btn,
then update the JSX to add the .btn-ghost-green class alongside
.share-submit-btn and .invite-link-create-btn so both buttons inherit the shared
styling.
In `@tests/e2e/page-objects/dialogs/invite-link-tab.page.ts`:
- Around line 208-216: The interceptClipboard method currently replaces
navigator.clipboard.writeText without restoring it or guarding against repeated
patches; make it idempotent and/or provide a restore hook: when
interceptClipboard runs, detect and preserve the original writeText (store it on
window under a unique symbol like __originalClipboardWriteText) and only wrap if
not already patched, and add a complementary restoreClipboard method that
restores navigator.clipboard.writeText from __originalClipboardWriteText and
clears __clipboardContent; reference interceptClipboard, __clipboardContent,
__originalClipboardWriteText, and navigator.clipboard.writeText to locate and
implement the fix.
In `@tests/e2e/tests/invite-link-workflow.spec.ts`:
- Around line 38-47: The parseInviteUrl function currently assumes the input
contains a hash and query and will throw on malformed URLs; update
parseInviteUrl to defensively validate the URL parts (or use the built-in URL
constructor) before splitting: check that url.includes('#') and that the hash
part contains '/invite/' and a '?key=' (or parse via new URL(urlWithHash) after
ensuring a base origin) and throw a clear, descriptive error if missing;
reference the function name parseInviteUrl and the local variables hashPart,
path, token, ephemeralKey when adding the guards so the test failure becomes
explicit instead of an unhandled exception.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (66)
.planning/REQUIREMENTS.md.planning/ROADMAP.md.planning/STATE.md.planning/phases/15-link-sharing/15-01-PLAN.md.planning/phases/15-link-sharing/15-01-SUMMARY.md.planning/phases/15-link-sharing/15-02-PLAN.md.planning/phases/15-link-sharing/15-02-SUMMARY.md.planning/phases/15-link-sharing/15-03-PLAN.md.planning/phases/15-link-sharing/15-03-SUMMARY.md.planning/phases/15-link-sharing/15-04-PLAN.md.planning/phases/15-link-sharing/15-04-SUMMARY.md.planning/phases/15-link-sharing/15-CONTEXT.md.planning/phases/15-link-sharing/15-RESEARCH.md.planning/phases/15-link-sharing/15-VERIFICATION.md.planning/security/REVIEW-phase15-link-sharing.mdapps/api/scripts/generate-openapi.tsapps/api/src/app.module.tsapps/api/src/migrations/1740400000000-AddShareInvites.tsapps/api/src/shares/dto/claim-invite.dto.tsapps/api/src/shares/dto/create-invite.dto.tsapps/api/src/shares/dto/invite-response.dto.tsapps/api/src/shares/entities/index.tsapps/api/src/shares/entities/share-invite.entity.tsapps/api/src/shares/invites.controller.spec.tsapps/api/src/shares/invites.controller.tsapps/api/src/shares/share-invites.controller.spec.tsapps/api/src/shares/share-invites.controller.tsapps/api/src/shares/shares.module.tsapps/api/src/shares/shares.service.spec.tsapps/api/src/shares/shares.service.tsapps/web/src/api/invites/invites.tsapps/web/src/api/models/claimChildKeyDto.tsapps/web/src/api/models/claimChildKeyDtoKeyType.tsapps/web/src/api/models/claimInviteDto.tsapps/web/src/api/models/createInviteDto.tsapps/web/src/api/models/createInviteDtoItemType.tsapps/web/src/api/models/index.tsapps/web/src/api/models/inviteChildKeyDto.tsapps/web/src/api/models/inviteChildKeyDtoKeyType.tsapps/web/src/api/models/inviteDataResponseDto.tsapps/web/src/api/models/inviteDataResponseDtoEncryptedChildKeys.tsapps/web/src/api/models/inviteDataResponseDtoItemType.tsapps/web/src/api/models/inviteDataResponseDtoStatus.tsapps/web/src/api/models/inviteResponseDto.tsapps/web/src/api/models/inviteResponseDtoItemType.tsapps/web/src/api/models/inviteResponseDtoStatus.tsapps/web/src/api/models/inviteStatusResponseDto.tsapps/web/src/api/models/inviteStatusResponseDtoStatus.tsapps/web/src/api/models/shareInvitesControllerListInvitesParams.tsapps/web/src/api/share-invites/share-invites.tsapps/web/src/components/file-browser/InviteLinkTab.tsxapps/web/src/components/file-browser/ShareDialog.tsxapps/web/src/lib/crypto/key-wrapping.tsapps/web/src/routes/InvitePage.tsxapps/web/src/routes/index.tsxapps/web/src/services/invite.service.tsapps/web/src/styles/invite-page.cssapps/web/src/styles/share-dialog.cssdesigns/DESIGN.mddesigns/cipher-box-design.penpackages/api-client/openapi.jsontests/e2e/page-objects/dialogs/index.tstests/e2e/page-objects/dialogs/invite-link-tab.page.tstests/e2e/page-objects/index.tstests/e2e/page-objects/pages/invite.page.tstests/e2e/tests/invite-link-workflow.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (18)
packages/api-client/openapi.json (2)
1612-1615:⚠️ Potential issue | 🟠 MajorConfirm the claim response schema is emitted.
The 201 response for
/invites/{token}/claimhas no schema (Line 1612-1614). If the handler returns a body (e.g.,{ shareId: string }), the generated client will drop it. Add an explicit response schema (or align the handler to return no body) and regenerate the spec.🔧 Suggested `@ApiResponse` schema (controller)
- `@ApiResponse`({ status: 201, description: 'Invite claimed successfully' }) + `@ApiResponse`({ + status: 201, + description: 'Invite claimed successfully', + schema: { + type: 'object', + properties: { + shareId: { type: 'string', description: 'UUID of the created share' } + }, + required: ['shareId'] + } + })#!/bin/bash # Inspect the claim invite handler return value and swagger decorator. rg -n "claimInvite" apps/api/src/shares -g '*.controller.ts' -C 4 rg -n "@ApiResponse" apps/api/src/shares/invites.controller.ts -C 4🤖 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 1612 - 1615, The 201 response for the claim endpoint is missing a response schema, so update the controller's claim handler (e.g., the method named claimInvite in invites.controller.ts) to include an explicit `@ApiResponse` decorator that defines the returned schema (for example { type: 'object', properties: { shareId: { type: 'string' } } } or the proper DTO class), or change the handler to return no body; after adding the proper `@ApiResponse/`@ApiOkResponse (or matching DTO) regenerate the OpenAPI spec so the 201 response includes the schema.
2871-2875:⚠️ Potential issue | 🟠 MajorFix
encryptedChildKeysschema to be an array.
InviteDataResponseDto.encryptedChildKeysis emitted as"type": "object"(Line 2871-2874). If the DTO is an array (with nullable), the spec should be"type": "array"withInviteChildKeyDtoitems. Update the@ApiPropertyto specify the array element type and regenerate the spec.🔧 Suggested DTO annotation
- `@ApiProperty`({ - description: 'Array of child keys wrapped with ephemeral public key, or null', - nullable: true, - }) + `@ApiProperty`({ + type: [InviteChildKeyDto], + description: 'Array of child keys wrapped with ephemeral public key, or null', + nullable: true, + }) encryptedChildKeys: InviteChildKeyDto[] | null;#!/bin/bash # Locate the DTO and the encryptedChildKeys property decoration. rg -n "InviteDataResponseDto" apps/api/src/shares/dto -g '*.dto.ts' -C 4 rg -n "encryptedChildKeys" apps/api/src/shares/dto -g '*.dto.ts' -C 4🤖 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 2871 - 2875, The OpenAPI schema shows InviteDataResponseDto.encryptedChildKeys as an object but it should be an array of InviteChildKeyDto; update the DTO decorator for the encryptedChildKeys property (InviteDataResponseDto.encryptedChildKeys) to specify the element type and array shape (use the InviteChildKeyDto type and mark as an array and nullable in the `@ApiProperty` options), then regenerate the OpenAPI spec so the generated JSON uses "type": "array" with the correct items schema.designs/cipher-box-design.pen (2)
39515-39523:⚠️ Potential issue | 🟡 MinorAlign invite-expiry copy with backend TTL/claim semantics.
The policy note (Line 40956–Line 40964) and expired-state copy (Line 39515–Line 39523) hardcode “7 days” and “single-claim.” If TTL or claim policy is configurable, this will mislead users. Please verify the backend contract and update both strings (or make them generic) and keep them in sync.
✏️ Suggested generic copy (if TTL is configurable)
- "content": "Invite links are valid for 7 days.\nAsk the sender for a new link.", + "content": "Invite links expire after the configured period.\nAsk the sender for a new link.", - "content": "// expires in 7 days, single-claim", + "content": "// expires after the configured period",Also applies to: 40956-40964
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@designs/cipher-box-design.pen` around lines 39515 - 39523, The invite expiry copy hardcodes “7 days” and “single-claim” which may diverge from backend TTL/claim semantics; confirm the backend contract (TTL and claim policy) and either make the UI strings generic or read the actual values from the same source used by the backend, then update the text nodes (e.g., the text element with id "errSubtext" and the policy-note string used in the expired-state/policy block) so they no longer hardcode “7 days”/“single-claim” but instead reflect the configured TTL/claim policy or a generic phrase.
39304-39312:⚠️ Potential issue | 🟠 MajorIncrease contrast for muted text at small sizes (
#006644).
#006644at 10–11px on black likely misses WCAG AA for normal text. This shows up in the subtext (Line 39304–Line 39312) and other small labels in this hunk (e.g., Line 39670, Line 39823, Line 39867, Line 40004, Line 40658, Line 40733). Please use the accessible muted-text token or increase size/weight across all occurrences.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@designs/cipher-box-design.pen` around lines 39304 - 39312, The subtext element with id "TBokI" (name "subtext", fontFamily "JetBrains Mono", fontSize 11, fill "#006644") uses too-low contrast for small text; update this and all other small-label occurrences currently using "#006644" (e.g., the other muted labels called out in the review) to use the accessible muted-text token (instead of the hardcoded hex) or switch to a higher-contrast color/weight/size (e.g., darker hex token, increase fontSize or set fontWeight to "medium") so the text meets WCAG AA for 10–11px text; apply the same change consistently across each matching element that uses "#006644".apps/web/src/lib/crypto/key-wrapping.ts (1)
74-85:⚠️ Potential issue | 🟠 MajorFolder key re-wrapping still not wrapped in try-catch.
The
reWrapEncryptedKeycall (and the subsequentchildKeys.push/wrapped++/onProgress) for the folder branch has no error handling. IfreWrapEncryptedKeythrows for any subfolder, traversal aborts entirely — inconsistent with the file branch (Lines 53–70) which continues on error.🛡️ Proposed fix (mirrors file branch pattern)
} else { const folder = child as FolderEntry; - // Re-wrap the subfolder's folderKey for the target - const folderKeyRewrapped = await reWrapEncryptedKey( - folder.folderKeyEncrypted, - ownerPrivateKey, - targetPubKeyBytes - ); - 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 { + try { + // Re-wrap the subfolder's folderKey for the target + const folderKeyRewrapped = await reWrapEncryptedKey( + folder.folderKeyEncrypted, + ownerPrivateKey, + targetPubKeyBytes + ); + 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 const resolved = await resolveIpnsRecord(folder.ipnsName); // ... rest of recursion unchanged ... } catch (err) { - console.error(`Failed to traverse subfolder ${folder.name}:`, 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/lib/crypto/key-wrapping.ts` around lines 74 - 85, Wrap the folder branch call to reWrapEncryptedKey and the subsequent childKeys.push/wrapped++/onProgress in a try-catch similar to the file branch: try to call reWrapEncryptedKey(folder.folderKeyEncrypted, ownerPrivateKey, targetPubKeyBytes), push the child key on success, increment wrapped and call onProgress; in the catch block log or warn about the error (including folder.id and the error) and continue without throwing so traversal doesn't abort. Ensure you reference reWrapEncryptedKey, folder.folderKeyEncrypted, childKeys.push, wrapped and onProgress when making the change.apps/api/src/shares/entities/share-invite.entity.ts (1)
17-18:⚠️ Potential issue | 🟡 MinorEntity should declare
unique: trueontokencolumn to match the database constraint.The migration includes a
UNIQUEconstraint ontoken, but the entity decorator doesn't reflect it. This mismatch means TypeORM's schema metadata won't be aware of the uniqueness, which matters forsynchronizemode and for developer clarity.Proposed fix
- `@Column`({ type: 'varchar', length: 44, name: 'token' }) + `@Column`({ type: 'varchar', length: 44, name: 'token', unique: true }) token!: 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-invite.entity.ts` around lines 17 - 18, The token column in the ShareInvite entity is missing the unique flag that the migration enforces; update the `@Column` decorator for token (in the ShareInvite entity/class) to include unique: true so the entity metadata matches the DB UNIQUE constraint and TypeORM knows the column is unique.apps/web/src/api/models/inviteDataResponseDtoEncryptedChildKeys.ts (1)
9-13:⚠️ Potential issue | 🟠 MajorUnresolved:
encryptedChildKeysstill generates as{ [key: string]: unknown } | null.This issue was flagged in a previous review and remains unaddressed. The backend
InviteDataResponseDtois still missingtype: [InviteChildKeyDto]in its@ApiPropertydecorator, so orval cannot infer the array element type and falls back to a generic object map. Until fixed, any frontend code iterating over child keys during invite claim/re-wrap will have no type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/api/models/inviteDataResponseDtoEncryptedChildKeys.ts` around lines 9 - 13, The generated frontend type InviteDataResponseDtoEncryptedChildKeys is too generic because the backend DTO is missing an explicit array element type; fix the backend InviteDataResponseDto by adding an `@ApiProperty` decorator on the encryptedChildKeys property with type: [InviteChildKeyDto] (reference InviteDataResponseDto and InviteChildKeyDto), then re-run the OpenAPI/orval generation so the client type is emitted as InviteChildKeyDto[] | null instead of { [key: string]: unknown } | null.apps/api/src/migrations/1740400000000-AddShareInvites.ts (1)
29-35:⚠️ Potential issue | 🟡 MinorAdd a foreign key for
claimed_byto prevent orphaned references.
claimed_bycurrently lacks a FK tousers(id), so deleted users can leave orphaned UUIDs. ConsiderON DELETE SET NULLto preserve the invite record.🔧 Proposed migration tweak
CONSTRAINT "FK_share_invites_sharer" FOREIGN KEY ("sharer_id") - REFERENCES "users" ("id") ON DELETE CASCADE ON UPDATE NO ACTION + REFERENCES "users" ("id") ON DELETE CASCADE ON UPDATE NO ACTION, + CONSTRAINT "FK_share_invites_claimed_by" FOREIGN KEY ("claimed_by") + REFERENCES "users" ("id") ON DELETE SET NULL ON UPDATE NO ACTION )#!/bin/bash # Locate claimed_by and constraints in the migration rg -n 'claimed_by|FK_share_invites' apps/api/src/migrations/1740400000000-AddShareInvites.ts -C3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/migrations/1740400000000-AddShareInvites.ts` around lines 29 - 35, The migration leaves "claimed_by" as a plain uuid without a foreign key, allowing orphaned UUIDs; update the AddShareInvites migration to make "claimed_by" nullable (if not already) and add a FK constraint (e.g., CONSTRAINT "FK_share_invites_claimed_by") that references users(id) with ON DELETE SET NULL and ON UPDATE NO ACTION so deleted users set claimed_by to null instead of leaving orphans; edit the migration where "claimed_by" and other CONSTRAINTs are declared to add this new FK next to "FK_share_invites_sharer".apps/web/src/api/models/inviteDataResponseDto.ts (1)
17-21: TightenencryptedChildKeystyping by fixing the OpenAPI schema and regenerating.The generated type is
{ [key: string]: unknown } | null, which weakens type safety in claim flows. Update the backendInviteResponseDto@ApiProperty()forencryptedChildKeysto describe an array of{ keyType, itemId, encryptedKey }, then regenerate the client so this becomesArray<...> | null.#!/bin/bash # Verify backend schema and generated model for encryptedChildKeys rg -n 'encryptedChildKeys' apps/api/src/shares/dto -g '*.ts' -C2 echo "=== Generated client model ===" rg -n 'encryptedChildKeys' apps/web/src/api/models/inviteDataResponseDto*.ts -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/api/models/inviteDataResponseDto.ts` around lines 17 - 21, The generated client types show encryptedChildKeys as `{ [key: string]: unknown } | null`; update the backend DTO by changing the InviteResponseDto `@ApiProperty`() for encryptedChildKeys to explicitly describe an array of objects with properties keyType, itemId, and encryptedKey (e.g., type: 'array' with items schema having those three fields and their types, and nullable: true), then re-run the OpenAPI client generation to regenerate InviteDataResponseDto so encryptedChildKeys becomes Array<{ keyType, itemId, encryptedKey }> | null; target the InviteResponseDto class and the encryptedChildKeys property when making the change and regenerating.apps/api/src/shares/invites.controller.ts (2)
121-121:claimInvite@ApiResponse(201)has no response schema — generated client returnsunknown.Without a
typeon the decorator, the OpenAPI spec omits theshareIdfield and the frontend client loses type-safety on the claim result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/invites.controller.ts` at line 121, The `@ApiResponse`({ status: 201, description: 'Invite claimed successfully' }) on the claimInvite endpoint doesn't specify a response type, so the OpenAPI spec omits the shareId and the generated client returns unknown; update the claimInvite controller to provide an explicit response DTO (e.g., ClaimInviteResponseDto or InviteClaimResponseDto) in the `@ApiResponse/type` (or provide a schema with the shareId field) so the OpenAPI output includes shareId; ensure the DTO contains shareId and use that DTO type in the `@ApiResponse` decorator on the claimInvite method.
18-22:RequestWithUserinterface duplicated again.Same local re-declaration as in
share-invites.controller.ts. The prior review recommended extracting it to a shared types file. This new file should import from that shared location once created.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/invites.controller.ts` around lines 18 - 22, The RequestWithUser interface is duplicated here; remove the local declaration and import the shared type instead: create or use the previously suggested shared types file (e.g., a common types module) that exports RequestWithUser, then replace the local interface in invites.controller.ts with an import of RequestWithUser and update any usages in functions/methods (e.g., request handlers) to use the imported type; ensure the import name matches the exported symbol and run type-checks to confirm no remaining duplicates.apps/web/src/components/file-browser/InviteLinkTab.tsx (2)
83-93: Clipboard failure still shows misleading "link copied to clipboard" message.The inner
catchon line 86 silently swallows the error, then line 93 unconditionally shows the "copied" message. Since the ephemeral URL is not persisted, a user in an insecure context loses the link with no indication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/InviteLinkTab.tsx` around lines 83 - 93, The current logic in InviteLinkTab calls navigator.clipboard.writeText(url) inside a try/catch but always calls setSuccess("link copied to clipboard: ...") regardless of whether the write succeeded; change it to set success only when navigator.clipboard.writeText(url) resolves, and in the catch branch call setSuccess (or setError) with a clear message that the link was created but clipboard copy failed and include the displayUrl so the user can copy it manually; update references to navigator.clipboard.writeText, url, displayUrl and setSuccess (or setError) in InviteLinkTab accordingly so the UI reflects clipboard failure instead of misleadingly reporting a copy.
95-104:fetchInvitesForItemfailure inside the outertrystill overwrites the creation success message.If
fetchInvitesForItemon line 96 throws, thecatchon line 98 shows"failed to create invite link"even though the invite was created and the URL was copied. The list refresh should be isolated in its owntry/catch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/InviteLinkTab.tsx` around lines 95 - 104, The outer try's call to fetchInvitesForItem can throw and trigger the same catch that reports invite-creation failure; wrap the refresh in its own try/catch so creation success isn't overwritten. After the invite creation and URL copy, call fetchInvitesForItem(ipnsName) inside a nested try block, setInvites(updatedInvites) on success, and in that inner catch only log the refresh error (e.g., console.error) and avoid calling setError for creation; keep the outer catch reserved for actual creation failures and leave setIsCreating(false) in the finally block.apps/api/src/shares/share-invites.controller.ts (2)
22-26:RequestWithUserinterface is duplicated — extract to shared module.Same local interface in both
invites.controller.tsand this file. The prior review identified this across 7+ controllers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/share-invites.controller.ts` around lines 22 - 26, The RequestWithUser interface is duplicated across controllers; extract it into a shared typings module (e.g., create and export a UserRequest or RequestWithUser type from a central types file) and replace the local interface in share-invites.controller.ts (and other controllers like invites.controller.ts) with an import of that shared type; update imports to reference the new exported symbol RequestWithUser (or chosen name) and remove the local interface declarations to avoid duplication.
104-106:ipnsNamequery parameter still lacks DTO-level validation.A bare
@Query('ipnsName') ipnsName: stringbypasses the globalValidationPipe. Empty or oversized strings reach the service without rejection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/share-invites.controller.ts` around lines 104 - 106, The listInvites handler accepts a raw `@Query`('ipnsName') string which bypasses the ValidationPipe; create a query DTO (e.g., ListInvitesQueryDto) with decorators like `@IsString`(), `@IsNotEmpty`() and `@MaxLength`(...) for ipnsName, update the controller method signature to accept `@Query`() query: ListInvitesQueryDto and read ipnsName from query.ipnsName, and ensure the DTO type is exported/used so global validation will reject empty/oversized values before reaching listInvites.apps/web/src/routes/InvitePage.tsx (2)
104-112:⚠️ Potential issue | 🟡 MinorClear the redirect timer on unmount.
ThesetTimeoutcan fire after unmount and navigate unexpectedly. Track and clear it in a cleanup effect.🧹 Suggested fix
+ const redirectTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); + + useEffect(() => { + return () => { + if (redirectTimerRef.current) clearTimeout(redirectTimerRef.current); + }; + }, []); ... - setTimeout(() => { + redirectTimerRef.current = setTimeout(() => { navigate('/shared', { replace: true }); }, 500);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/InvitePage.tsx` around lines 104 - 112, The setTimeout started after claimInvite can fire after the component unmounts and cause an unexpected navigate; store the timeout id (e.g., in a ref like redirectTimerRef) when calling setTimeout in the claimInvite.then block and clear it on unmount (and when starting a new timer) in a useEffect cleanup so navigate('/shared', { replace: true }) won't run after unmount; update references to ephemeralKeyRef.current = null and the new redirectTimerRef to ensure the timer is cleared appropriately.
68-87:⚠️ Potential issue | 🟠 MajorHandle invite status lookup failures to avoid infinite loading.
A rejectedcheckInviteStatusleavespageStatestuck at"loading"and produces an unhandled rejection. Add a catch that respects thecancelledflag and sets an error state.🔧 Suggested fix
- checkInviteStatus(token).then((status) => { - if (cancelled) return; - - if (status === 'active') { - setPageState('valid'); - } else { - setErrorReason(status as ErrorReason); - setPageState('error'); - } - }); + checkInviteStatus(token) + .then((status) => { + if (cancelled) return; + if (status === 'active') { + setPageState('valid'); + } else { + setErrorReason(status as ErrorReason); + setPageState('error'); + } + }) + .catch(() => { + if (cancelled) return; + setErrorReason('invalid'); + setPageState('error'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/InvitePage.tsx` around lines 68 - 87, The useEffect calling checkInviteStatus(token) lacks a .catch, causing unhandled rejections and the pageState to remain "loading"; add a .catch handler on the promise returned by checkInviteStatus that first returns early if the local cancelled flag is true, then sets setErrorReason to an appropriate ErrorReason (e.g., 'invalid' or 'unknown') and calls setPageState('error'), and optionally logs the error; ensure this catch lives alongside the existing .then so rejected lookups are handled and the cancelled gate is respected.apps/web/src/services/invite.service.ts (1)
279-283:⚠️ Potential issue | 🟠 MajorRemove the unsafe cast on the claim response by fixing the OpenAPI typing.
invitesControllerClaimInviteis typed asvoid, so this cast can hide backend/client mismatches. Add a response DTO to the claim endpoint and regenerate the client to get a real{ shareId }type.#!/bin/bash # Verify claim invite response typing in backend controllers/OpenAPI. rg -n "claimInvite" apps/api/src -C 3 rg -n "@ApiResponse" apps/api/src -C 3 | rg -i "claim"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/services/invite.service.ts` around lines 279 - 283, invitesControllerClaimInvite is currently typed as void, so remove the unsafe cast by adding a proper response DTO for the claim endpoint in the backend OpenAPI (e.g., a ClaimInviteResponse with shareId), annotate the controller method with the new `@ApiResponse` referencing that DTO, regenerate the TypeScript client so invitesControllerClaimInvite returns the correct { shareId: string } type, and then update the call site (where result is assigned) to use the typed response instead of casting.
🧹 Nitpick comments (9)
apps/api/src/shares/dto/claim-invite.dto.ts (1)
23-28: Add@IsUUID()toitemIdfor format validation.The field is documented as "UUID of the file or subfolder" but only
@MinLength(1)is applied — any non-empty string passes validation. Adding@IsUUID()provides defense-in-depth input validation that matches the documented semantics and rejects structurally invalid item IDs before they reach the service layer.♻️ Proposed fix
-import { - IsString, - IsIn, - IsArray, - ValidateNested, - IsOptional, - Matches, - MaxLength, - MinLength, -} from 'class-validator'; +import { + IsString, + IsIn, + IsArray, + IsUUID, + ValidateNested, + IsOptional, + Matches, + MaxLength, + MinLength, +} from 'class-validator';`@IsString`() - `@MinLength`(1) + `@IsUUID`() 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/claim-invite.dto.ts` around lines 23 - 28, The itemId property in ClaimInviteDto is documented as a UUID but currently only has `@IsString`() and `@MinLength`(1); add the `@IsUUID`() class-validator decorator to the itemId field to enforce UUID format, and import IsUUID from 'class-validator' at the top of the file (keep existing `@IsString/`@MinLength if desired or remove `@MinLength` if redundant). Target the itemId field in the ClaimInviteDto (itemId!: string) when applying this change.apps/api/src/shares/dto/invite-response.dto.ts (1)
60-68: ExtractencryptedChildKeyselement shape to a named DTO class.The NestJS Swagger CLI plugin does not reliably introspect inline anonymous object types inside generics. Without an explicit
type: () => [ChildKeyResponseDto]hint, the generated OpenAPI schema will render each array element as a plainobject, producing an imprecise client contract. Extracting to a named class (or reusing a shared one) is the safe path.♻️ Proposed refactor
+export class ChildKeyResponseDto { + `@ApiProperty`({ enum: ['file', 'folder'] }) + keyType!: 'file' | 'folder'; + + `@ApiProperty`() + itemId!: string; + + `@ApiProperty`({ description: 'Hex-encoded ECIES ciphertext' }) + encryptedKey!: string; +} export class InviteDataResponseDto { // ... `@ApiProperty`({ description: 'Array of child keys wrapped with ephemeral public key, or null', nullable: true, + type: () => [ChildKeyResponseDto], }) - encryptedChildKeys!: Array<{ - keyType: 'file' | 'folder'; - itemId: string; - encryptedKey: string; - }> | null; + encryptedChildKeys!: ChildKeyResponseDto[] | null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/dto/invite-response.dto.ts` around lines 60 - 68, Extract the inline anonymous element shape into a named DTO (e.g., ChildKeyResponseDto) and update the encryptedChildKeys property to use that DTO type; create a class ChildKeyResponseDto with properties keyType: 'file' | 'folder', itemId: string, encryptedKey: string, then change encryptedChildKeys in InviteResponseDto to type ChildKeyResponseDto[] | null and annotate it with `@ApiProperty`({ type: () => [ChildKeyResponseDto], description: 'Array of child keys wrapped with ephemeral public key, or null', nullable: true }) so Swagger correctly generates the array element schema.apps/api/src/shares/dto/create-invite.dto.ts (1)
14-14:InviteChildKeyDtois not exported, forcing duplication.The identical
{ keyType, itemId, encryptedKey }shape is duplicated as an inline anonymous type inInviteDataResponseDto.encryptedChildKeys. Exporting this class lets the response DTO reference it directly and aligns with the namedInviteChildKeyDtothat Orval already generates on the frontend.-class InviteChildKeyDto { +export class InviteChildKeyDto {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/dto/create-invite.dto.ts` at line 14, The InviteChildKeyDto class is declared but not exported, causing duplication of the { keyType, itemId, encryptedKey } shape; export InviteChildKeyDto and update InviteDataResponseDto to reference InviteChildKeyDto for its encryptedChildKeys property instead of the inline anonymous type so the backend matches the named DTO Orval generates on the frontend (ensure the class is exported and any existing inline type is removed or replaced).apps/api/src/shares/entities/share-invite.entity.ts (1)
58-65:claimedByonly stores one user ID, butmaxClaimsallows more than 1.If
maxClaimsis ever set to > 1, only the last claimer's ID would be recorded inclaimedBy, losing traceability for earlier claims. If multi-claim invites are intended for future use, consider a join table or an array column. If single-claim is the only supported scenario, consider droppingmaxClaims/claimCountand using a simple boolean, or adding a comment documenting thatmaxClaims > 1is unsupported.This isn't blocking since the default is 1 and the current service code enforces single-claim, but it's worth clarifying intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/entities/share-invite.entity.ts` around lines 58 - 65, The ShareInvite entity currently has maxClaims and claimCount but claimedBy stores only one UUID, which will lose earlier claimer IDs if maxClaims > 1; update the model to make intent explicit: either (A) support multi-claim by replacing claimedBy with a relation/join table (e.g., ShareInvite.claims many-to-one/many-to-many or a claims array column) and adjust claim handling to push new claim entries and increment claimCount, or (B) enforce single-claim by removing/maxClaims/claimCount or replacing them with a boolean (e.g., claimed) and add a clear comment on claimedBy being single-use; update the ShareInvite entity and corresponding service code that references claimedBy, maxClaims, and claimCount to match the chosen approach (use symbols claimedBy, maxClaims, claimCount, and the ShareInvite entity to locate changes).tests/e2e/page-objects/dialogs/invite-link-tab.page.ts (1)
208-217:interceptClipboardaccumulates closure chains when called repeatedly.Each call captures the already-patched
writeTextasoriginal, growing the call chain. Consider adding arestoreClipboard()counterpart or resetting the override per call:♻️ Suggested fix
async interceptClipboard(): Promise<void> { await this.page.evaluate(() => { + // Store the true original once; skip re-patching if already intercepted. + const win = window as unknown as Record<string, unknown>; + if (!win.__clipboardOriginal) { + win.__clipboardOriginal = navigator.clipboard.writeText.bind(navigator.clipboard); + } + const original = win.__clipboardOriginal as (text: string) => Promise<void>; - (window as unknown as Record<string, string>).__clipboardContent = ''; - const original = navigator.clipboard.writeText.bind(navigator.clipboard); + win.__clipboardContent = ''; navigator.clipboard.writeText = async (text: string) => { - (window as unknown as Record<string, string>).__clipboardContent = text; + (win as Record<string, string>).__clipboardContent = text; return original(text); }; }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/page-objects/dialogs/invite-link-tab.page.ts` around lines 208 - 217, The interceptClipboard function wraps navigator.clipboard.writeText each time it's called, causing closure chains to accumulate; modify it to store the original writeText once (e.g., save to window.__originalClipboardWriteText) before overriding or add a restoreClipboard() that restores navigator.clipboard.writeText from that saved original; ensure interceptClipboard checks for an existing saved original and does not rewrap it so repeated calls don’t grow the chain (referencing interceptClipboard and the new restoreClipboard or window.__originalClipboardWriteText symbol).apps/api/src/shares/share-invites.controller.spec.ts (1)
22-22: Replaceas anycasts withas unknown as Requestto fix lint warnings.
mockReqis already narrowly typed on Line 22 but then widened back toanyat every call site. The same pattern applies tosharerat Line 28. The static analysis rightly flags all seven occurrences.♻️ Suggested fix
+import type { Request } from 'express'; +import type { User } from '../users/entities/user.entity'; // adjust import path ... - const mockReq: { user: { id: string } } = { user: { id: userId } }; + const mockReq = { user: { id: userId } } as unknown as Request; ... - sharer: {} as any, + sharer: {} as unknown as User, ... - const result = await controller.createInvite(mockReq as any, dto); + const result = await controller.createInvite(mockReq, dto); // apply same de-casting to all other call sites (listInvites, revokeInvite)Also applies to: 77-77, 100-100, 118-118, 135-135, 143-143, 154-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/share-invites.controller.spec.ts` at line 22, Replace the loose "as any" casts for the test request and user objects with a safe cast to Express's Request by using "as unknown as Request": change the declaration of mockReq ({ user: { id: userId } }) and the sharer variable to be cast via "as unknown as Request" (and update each call site that currently does "as any") so the seven occurrences referenced (roughly around mockReq and sharer uses) use "as unknown as Request" instead of "as any"; ensure you import Request from 'express' if not already present and apply the same replacement at the other listed occurrences (lines ~77, 100, 118, 135, 143, 154).apps/api/src/shares/share-invites.controller.ts (1)
56-80: Inline return types diverge from the declared@ApiResponseDTO — useInviteResponseDtoas the method return type.Both
createInviteandlistInvitesannotate their@ApiResponsewithtype: InviteResponseDto/type: [InviteResponseDto], but the TypeScript method signatures use anonymous inline types. Any future field added toInviteResponseDtowon't surface a compile-time error if the method still returns the old shape.♻️ Proposed refactor
- async createInvite( - `@Request`() req: RequestWithUser, - `@Body`() dto: CreateInviteDto - ): Promise<{ - id: string; token: string; itemType: string; ipnsName: string; - itemName: string; status: string; expiresAt: Date; createdAt: Date; - }> { + async createInvite( + `@Request`() req: RequestWithUser, + `@Body`() dto: CreateInviteDto + ): Promise<InviteResponseDto> {Apply the same change to
listInvites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/share-invites.controller.ts` around lines 56 - 80, The controller methods currently return anonymous inline types that diverge from the `@ApiResponse` DTOs; change the createInvite signature to return InviteResponseDto and listInvites to return InviteResponseDto[] (import InviteResponseDto if not already imported), and ensure the returned objects from createInvite and listInvites exactly match the fields on InviteResponseDto (add any missing properties or map fields accordingly) so TypeScript will enforce future DTO changes.tests/e2e/tests/invite-link-workflow.spec.ts (2)
126-129: Prefer state-based waits over fixed timeouts.
waitForTimeoutcan be flaky across environments; consider waiting on breadcrumbs, folder rows, or the shared list to stabilize navigation.Also applies to: 438-457
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/invite-link-workflow.spec.ts` around lines 126 - 129, Replace the fixed 500ms timeout in aliceNavigateBack with a state-based wait: after calling alice.page.locator('[data-testid="parent-dir-row"]').dblclick() wait for a deterministic UI signal such as the breadcrumb locator, the target folder row, or the shared list locator to appear or become stable (e.g., await alice.page.waitForSelector(...) or await locator.waitFor({ state: 'visible' })) so the test does not rely on timing; apply the same replacement to the other waitForTimeout usages referenced around the block covering lines 438-457 to wait on the appropriate breadcrumb/folder/shared-list locators instead of using fixed timeouts.
38-46: GuardparseInviteUrlfor malformed URLs.
Add explicit validation so failures are clearer if the URL format changes.🔎 Suggested fix
function parseInviteUrl(url: string): { token: string; ephemeralKey: string } { // URL format: http://localhost:5173/#/invite/TOKEN?key=KEY - const hashPart = url.split('#')[1]; // /invite/TOKEN?key=KEY + const hashPart = url.split('#')[1]; // /invite/TOKEN?key=KEY + if (!hashPart) throw new Error('Invite URL missing hash fragment'); const [path, query] = hashPart.split('?'); + if (!query) throw new Error('Invite URL missing query string'); const token = path.split('/invite/')[1]; const params = new URLSearchParams(query); - const ephemeralKey = params.get('key')!; + const ephemeralKey = params.get('key'); + if (!ephemeralKey) throw new Error('Invite URL missing key'); return { token, ephemeralKey }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/invite-link-workflow.spec.ts` around lines 38 - 46, The parseInviteUrl function currently assumes a well-formed hash and query; add explicit validation inside parseInviteUrl to guard against malformed URLs by checking that the URL contains a '#' and a non-empty hashPart, that hashPart splits into a path and query, that the path contains '/invite/' and yields a non-empty token, and that URLSearchParams(query).get('key') returns a non-empty ephemeralKey; if any check fails, throw a clear Error with a descriptive message (e.g., "Invalid invite URL: missing hash", "Invalid invite URL: missing token", or "Invalid invite URL: missing key") so callers can see exactly what part of the URL was malformed.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (66)
.planning/REQUIREMENTS.md.planning/ROADMAP.md.planning/STATE.md.planning/phases/15-link-sharing/15-01-PLAN.md.planning/phases/15-link-sharing/15-01-SUMMARY.md.planning/phases/15-link-sharing/15-02-PLAN.md.planning/phases/15-link-sharing/15-02-SUMMARY.md.planning/phases/15-link-sharing/15-03-PLAN.md.planning/phases/15-link-sharing/15-03-SUMMARY.md.planning/phases/15-link-sharing/15-04-PLAN.md.planning/phases/15-link-sharing/15-04-SUMMARY.md.planning/phases/15-link-sharing/15-CONTEXT.md.planning/phases/15-link-sharing/15-RESEARCH.md.planning/phases/15-link-sharing/15-VERIFICATION.md.planning/security/REVIEW-phase15-link-sharing.mdapps/api/scripts/generate-openapi.tsapps/api/src/app.module.tsapps/api/src/migrations/1740400000000-AddShareInvites.tsapps/api/src/shares/dto/claim-invite.dto.tsapps/api/src/shares/dto/create-invite.dto.tsapps/api/src/shares/dto/invite-response.dto.tsapps/api/src/shares/entities/index.tsapps/api/src/shares/entities/share-invite.entity.tsapps/api/src/shares/invites.controller.spec.tsapps/api/src/shares/invites.controller.tsapps/api/src/shares/share-invites.controller.spec.tsapps/api/src/shares/share-invites.controller.tsapps/api/src/shares/shares.module.tsapps/api/src/shares/shares.service.spec.tsapps/api/src/shares/shares.service.tsapps/web/src/api/invites/invites.tsapps/web/src/api/models/claimChildKeyDto.tsapps/web/src/api/models/claimChildKeyDtoKeyType.tsapps/web/src/api/models/claimInviteDto.tsapps/web/src/api/models/createInviteDto.tsapps/web/src/api/models/createInviteDtoItemType.tsapps/web/src/api/models/index.tsapps/web/src/api/models/inviteChildKeyDto.tsapps/web/src/api/models/inviteChildKeyDtoKeyType.tsapps/web/src/api/models/inviteDataResponseDto.tsapps/web/src/api/models/inviteDataResponseDtoEncryptedChildKeys.tsapps/web/src/api/models/inviteDataResponseDtoItemType.tsapps/web/src/api/models/inviteDataResponseDtoStatus.tsapps/web/src/api/models/inviteResponseDto.tsapps/web/src/api/models/inviteResponseDtoItemType.tsapps/web/src/api/models/inviteResponseDtoStatus.tsapps/web/src/api/models/inviteStatusResponseDto.tsapps/web/src/api/models/inviteStatusResponseDtoStatus.tsapps/web/src/api/models/shareInvitesControllerListInvitesParams.tsapps/web/src/api/share-invites/share-invites.tsapps/web/src/components/file-browser/InviteLinkTab.tsxapps/web/src/components/file-browser/ShareDialog.tsxapps/web/src/lib/crypto/key-wrapping.tsapps/web/src/routes/InvitePage.tsxapps/web/src/routes/index.tsxapps/web/src/services/invite.service.tsapps/web/src/styles/invite-page.cssapps/web/src/styles/share-dialog.cssdesigns/DESIGN.mddesigns/cipher-box-design.penpackages/api-client/openapi.jsontests/e2e/page-objects/dialogs/index.tstests/e2e/page-objects/dialogs/invite-link-tab.page.tstests/e2e/page-objects/index.tstests/e2e/page-objects/pages/invite.page.tstests/e2e/tests/invite-link-workflow.spec.ts
- Add FK constraint for claimed_by -> users(id) in share_invites migration - Add unique: true to token column on ShareInvite entity - Create ClaimInviteResponseDto and type the claim @apiresponse(201) - Fix encryptedChildKeys @ApiProperty to use type: [InviteChildKeyDto] - Export InviteChildKeyDto from create-invite.dto for reuse in response DTO - Extract RequestWithUser to apps/api/src/common/types.ts (was duplicated 7x) - Separate clipboard success tracking in InviteLinkTab (no false "copied" msg) - Isolate invite list refresh error from creation success in InviteLinkTab - Wrap folder key re-wrapping in try-catch in collectChildKeys (key-wrapping.ts) - Strip ephemeral key from URL on mount in InvitePage - Add .catch() to checkInviteStatus promise in InvitePage - Validate ErrorReason before cast (fall back to 'invalid') - Clean up redirect setTimeout on unmount via ref in InvitePage - Remove unsafe cast in invite.service.ts (now properly typed by Orval) - Regenerate API client with corrected OpenAPI spec Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 72568cd8adeb
Security: - Collapse public invite status endpoint to prevent token-existence oracle (returns 404 for all non-active states instead of leaking claimed/revoked) - Wrap claimInvite in DB transaction to prevent "claimed-but-no-share" state - Add @IsUUID() validation on itemId in both create and claim DTOs - Increase @minlength to 258 on encryptedKey (minimum ECIES secp256k1 ciphertext) Accessibility: - Wire ARIA attributes on ShareDialog tab widget (id, aria-controls, aria-labelledby, roving tabIndex, ArrowLeft/ArrowRight keyboard nav) UX: - Show error to user when invite list fetch fails (was console-only) - Use var(--font-size-xs) instead of hardcoded 10px on invite-status-badge - Add .share-error selector to e2e waitForLoaded for fast failure on API errors Tests: - Update invites.controller.spec to match collapsed status endpoint behavior - Add test for revoked status returning NotFoundException - Add mock DataSource for transactional claimInvite in shares.service.spec - Add encryptedChildKeys exposure check in share-invites.controller.spec Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 43c02df5aab2
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.planning/REQUIREMENTS.md (1)
427-427:⚠️ Potential issue | 🟡 MinorStale
Last updatedtimestamp.The footer still reads
2026-02-11 after Milestone 3 roadmap creation; it should be bumped to reflect this Phase 15 status update.📝 Proposed fix
-Last updated: 2026-02-11 after Milestone 3 roadmap creation (M3 phase mappings added) +Last updated: 2026-02-23 after Phase 15 completion (SHARE-06/07 marked Complete)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/REQUIREMENTS.md at line 427, Update the stale footer string "Last updated: 2026-02-11 after Milestone 3 roadmap creation (M3 phase mappings added)" in .planning/REQUIREMENTS.md to the current Phase 15 update date and wording (e.g., "Last updated: 2026-02-23 after Phase 15 status update") so the document reflects the new status; locate the exact footer line containing that original phrase and replace the date and parenthetical note accordingly.
♻️ Duplicate comments (17)
apps/api/src/shares/share-invites.controller.spec.ts (1)
140-147:⚠️ Potential issue | 🟡 MinorAdd
encryptedChildKeysto the “no internal fields” list assertion.This mirrors the
createInvitetest and ensures list responses never expose child key data.🔧 Suggested fix
expect('encryptedKey' in result[0]).toBe(false); expect('sharerId' in result[0]).toBe(false); +expect('encryptedChildKeys' in result[0]).toBe(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/share-invites.controller.spec.ts` around lines 140 - 147, The test in share-invites.controller.spec.ts should also assert that list responses do not include child key data: after calling mockSharesService.getInvitesForItem and awaiting controller.listInvites (the existing test using mockReq and item id), add an assertion that 'encryptedChildKeys' is not present on result[0] (similar to the existing checks for 'encryptedKey' and 'sharerId'); update the test that uses mockSharesService.getInvitesForItem and the result variable to include expect('encryptedChildKeys' in result[0]).toBe(false).apps/web/src/styles/share-dialog.css (1)
430-435: Use a design-token font size for the status badge.This still hard-codes
10pxand diverges from the tokenized sizing used elsewhere in the file.♻️ Suggested tweak
.invite-status-badge { flex-shrink: 0; padding: 1px 6px; font-family: var(--font-family-mono); - font-size: 10px; + font-size: var(--font-size-xs); font-weight: var(--font-weight-semibold); letter-spacing: 0.05em; color: var(--color-green-primary); border: var(--border-thickness) solid var(--color-green-dim); }🤖 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 430 - 435, The .invite-status-badge rule currently hard-codes font-size: 10px; replace that literal with the project's design token (e.g., font-size: var(--font-size-xs) or the appropriate token used elsewhere in this file) so the badge follows tokenized sizing; update the font-size declaration in the .invite-status-badge CSS rule to use the matching --font-size-* token used by other selectors in this stylesheet.apps/api/src/shares/dto/create-invite.dto.ts (2)
23-28:⚠️ Potential issue | 🟡 MinorValidate
itemIdas a UUID.
@IsString()+@MinLength(1)accepts non-UUIDs, pushing malformed IDs into DB queries.✅ Suggested fix
import { IsString, IsIn, IsArray, ValidateNested, IsOptional, Matches, MaxLength, MinLength, + IsUUID, } from 'class-validator'; `@IsString`() + `@IsUUID`() `@MinLength`(1) 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/create-invite.dto.ts` around lines 23 - 28, The itemId property on the CreateInviteDto is currently validated only as a non-empty string and must be validated as a UUID to avoid malformed IDs; update the CreateInviteDto's itemId validators to use `@IsUUID`() (and remove or replace `@IsString`()/@MinLength(1) as appropriate) and add the necessary import for IsUUID from class-validator so incoming itemId values are enforced as valid UUIDs.
30-71:⚠️ Potential issue | 🟡 MinorIncrease
encryptedKeyMinLength to a realistic ECIES payload size.
@MinLength(2)allows garbage ciphertexts; downstream unwrap will fail. Please constrain to the minimum ECIES secp256k1 AES‑256‑GCM payload length.🔐 Suggested fix (both encryptedKey fields)
- `@MinLength`(2) + `@MinLength`(258) // minimum for ECIES secp256k1 + AES-256-GCM wrapping a 32-byte key `@MaxLength`(2048) encryptedKey!: string;What is the minimum ciphertext length (in hex characters) produced by eciesjs secp256k1 AES-256-GCM when encrypting a 32-byte payload with an uncompressed public key?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/dto/create-invite.dto.ts` around lines 30 - 71, The encryptedKey MinLength is too low (currently 2) allowing invalid ciphertexts; update the `@MinLength` on both encryptedKey properties (the one above and the encryptedKey in class CreateInviteDto) to the realistic minimum for ECIES secp256k1 AES‑256‑GCM with an uncompressed public key: 250 hex characters (125 bytes * 2), i.e., set `@MinLength`(250) for those encryptedKey fields so validators reject undersized/garbage payloads.apps/web/src/components/file-browser/ShareDialog.tsx (1)
351-490:⚠️ Potential issue | 🟡 MinorComplete the ARIA tab pattern + keyboard handling for the tab bar (Line 351+).
role="tab"without ids,aria-controls, rovingtabIndex, and arrow-key handling leaves the tabset inaccessible; both tabpanels also needaria-labelledby.♿ Suggested wiring
-<div className="share-tab-bar" role="tablist"> +<div + className="share-tab-bar" + role="tablist" + aria-label="Share method" + onKeyDown={(e) => { + if (e.key === 'ArrowRight') setActiveTab('invite'); + if (e.key === 'ArrowLeft') setActiveTab('direct'); + }} +> <button type="button" + id="share-tab-direct" role="tab" aria-selected={activeTab === 'direct'} + aria-controls="share-panel-direct" + tabIndex={activeTab === 'direct' ? 0 : -1} className={`share-tab${activeTab === 'direct' ? ' share-tab--active' : ''}`} onClick={() => setActiveTab('direct')} > {'DIRECT SHARE'} </button> <button type="button" + id="share-tab-invite" role="tab" aria-selected={activeTab === 'invite'} + aria-controls="share-panel-invite" + tabIndex={activeTab === 'invite' ? 0 : -1} className={`share-tab${activeTab === 'invite' ? ' share-tab--active' : ''}`} onClick={() => setActiveTab('invite')} > {'INVITE LINK'} </button> </div> {activeTab === 'direct' && ( - <div role="tabpanel"> + <div id="share-panel-direct" role="tabpanel" aria-labelledby="share-tab-direct">Also ensure
InviteLinkTab’s root panel usesid="share-panel-invite"andaria-labelledby="share-tab-invite"(or pass props through) so both panels are correctly associated.As per coding guidelines, “When adding
roleandtabIndex={0}to an element, must also add keyboard interaction contract for that role.”🤖 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 351 - 490, The tab bar in ShareDialog uses role="tab" but lacks ids, aria-controls, roving tabIndex and arrow-key handling, and the panels lack aria-labelledby—make the tab buttons fully accessible by assigning unique ids (e.g., "share-tab-direct" and "share-tab-invite"), add matching aria-controls pointing to panel ids ("share-panel-direct"/"share-panel-invite"), implement roving focus/tabIndex logic and left/right/up/down arrow key handling in a new handler (e.g., handleTabKeyDown) used on the tab buttons to call setActiveTab and move focus, and ensure each tabpanel (including InviteLinkTab) has id and aria-labelledby referencing its tab id (or pass those props into InviteLinkTab so its root uses id="share-panel-invite" and aria-labelledby="share-tab-invite"); update any aria-selected usage to reflect the activeTab variable.apps/web/src/components/file-browser/InviteLinkTab.tsx (1)
48-58:⚠️ Potential issue | 🟡 MinorSurface invite-fetch errors instead of showing a misleading empty state.
If the fetch fails, users see “no active invite links.” Set an error so the existing alert renders.
🛠️ Suggested fix
.catch((err) => { if (!cancelled) { console.error('Failed to fetch invites:', err); + setError('failed to load invite links'); } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/InviteLinkTab.tsx` around lines 48 - 58, When fetchInvitesForItem(ipnsName) fails, the catch currently only logs the error which leads the UI to show "no active invite links"; update the catch to set an error state so the existing error alert renders: inside the .catch((err) => { if (!cancelled) { ... } }) call set the component's invite-fetch error state (use the existing error setter if present, otherwise add one like setInvitesError) with the error or a message, and ensure you still keep console.error; keep the cancelled check and do not modify the successful .then branch that calls setInvites.designs/cipher-box-design.pen (4)
39304-39312: Unresolved past-review issue:TBokIsubtext contrast.
#006644on#000000yields ≈ 3.36:1, which fails the 4.5:1 AA minimum for normal text at 11px. This element was flagged in the previous review and remains unaddressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@designs/cipher-box-design.pen` around lines 39304 - 39312, The subtext element with id "TBokI" currently uses fill "#006644" on a "#000000" background and fails WCAG AA contrast for 11px text (≈3.36:1); update the element (id "TBokI", name "subtext") to use a darker color value that achieves at least 4.5:1 contrast against black (or alternatively increase the font size/weight to meet 4.5:1), e.g., replace the fill with a darker hex that meets the ratio and keep fontFamily "JetBrains Mono" and fontSize 11 unless you choose the size/weight change. Ensure the changed color/typography is applied to the "subtext" element so it passes automated contrast checks.
39514-39524: Unresolved past-review issues in expired-state subtext: contrast + hardcoded TTL copy.
hSiQp(errSubtext) carries two problems already raised in the previous review:
#006644at 11 px on black — same contrast failure (~3.36:1) asTBokI."Invite links are valid for 7 days."— the previous review explicitly called out this expired-state copy as a hardcoded TTL that may diverge from the backend contract.Both remain unresolved.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@designs/cipher-box-design.pen` around lines 39514 - 39524, The errSubtext element (id: hSiQp / name: errSubtext) has two outstanding issues: the color `#006644` at fontSize 11 fails contrast on a dark background, and the copy contains a hardcoded TTL string ("Invite links are valid for 7 days.") that can drift from backend values; fix by choosing a higher-contrast color or increasing size/weight so WCAG AA contrast is met for 11px text (or switch to a vetted token from the design palette) and replace the literal TTL sentence with a dynamic placeholder/variable (e.g., use a backend-driven {ttlText} or localization key) so the UI displays the canonical TTL from the backend rather than a hardcoded value.
40956-40964: Unresolved past-review issue: hardcoded"// expires in 7 days, single-claim"ingzqpC.This node was specifically flagged in the previous review cycle. The policy copy remains hardcoded and has not been updated to either reflect the confirmed backend TTL/claim contract or replaced with a generic/variable-driven string.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@designs/cipher-box-design.pen` around lines 40956 - 40964, The node gzqpC (name: policyNote) contains a hardcoded policy string "// expires in 7 days, single-claim"; replace this hardcoded copy with a variable-driven or templated value sourced from the canonical backend TTL/claim contract (e.g., use a policyNoteText variable or placeholder that is populated at build/runtime from the backend config or localization file) so the UI reflects the confirmed TTL/claim terms; update any code that renders gzqpC to read that variable instead of the literal string and ensure the variable name matches the backend field used for TTL/claim text.
39673-39678: Unresolved past-review issue:#006644on black recurs pervasively across the new Share Dialog frame.The previous review asked to fix all occurrences of
#006644at small sizes. The new "P15 - Share Dialog (Invite Tab)" frame introduces at least a dozen additional instances at fontSize 9–11 (e.g.userEmaili074c, breadcrumbEAQ4X/EbdH7, column headersVeXI1/c3Y0s/pV4PG, file row metadata cells, sidebarsettingsText/quotaText, footer links,statusText,tabDirectText,desc,linksLabel). None of these would pass the 4.5:1 AA threshold at their current sizes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@designs/cipher-box-design.pen` around lines 39673 - 39678, Replace all occurrences of the color "#006644" in the "P15 - Share Dialog (Invite Tab)" frame with an accessible color that meets WCAG AA 4.5:1 contrast for the small text sizes used (fontSize 9–11), or alternatively increase the text weight/size where appropriate; specifically update instances referenced as userEmail / i074c, breadcrumb EAQ4X and EbdH7, column header layers VeXI1, c3Y0s, pV4PG, file row metadata cells, sidebar labels settingsText and quotaText, footer links, statusText, tabDirectText, desc, and linksLabel so that each element's computed contrast ratio against its background meets 4.5:1. Ensure the replacement color is applied to the "fill" property for those text layers (e.g., where "fill": "#006644" appears) and verify visually at the target font sizes..planning/phases/15-link-sharing/15-VERIFICATION.md (1)
6-20: Verify whether this gap is still valid —InviteResponseDtonow includesidfield.The DTO in the current code (
invite-response.dto.ts, line 8–9) includes theidfield. However, the frontendfetchInvitesForItemmapping ininvite.service.ts(line 321) may still be mappinginv.tokenasid. If the frontend mapping was also updated, this gap section should be marked as resolved.#!/bin/bash # Check if invite.service.ts still maps token as id rg -n 'id:\s*inv\.token' --type=ts # Also check for the correct mapping using inv.id rg -n 'id:\s*inv\.id' --type=ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/phases/15-link-sharing/15-VERIFICATION.md around lines 6 - 20, The frontend still maps invite.token to the visible id causing revoke to send a base64 token to the DELETE endpoint which expects a UUID; update fetchInvitesForItem in apps/web/src/services/invite.service.ts to use the real UUID field (inv.id) returned by InviteResponseDto (or adjust the backend list response to include id and regenerate the API client), and ensure revokeInvite (share-invites.controller's DELETE handler) receives that UUID value rather than the token so ParseUUIDPipe succeeds.apps/api/src/shares/dto/invite-response.dto.ts (1)
26-30: Add explicitformat: 'date-time'to@ApiPropertyonDatefields.Without
{ type: 'string', format: 'date-time' }, the OpenAPI spec relies on the Swagger CLI plugin to infer the correct type. Making it explicit avoids ambiguity.Proposed fix
- `@ApiProperty`({ description: 'When the invite expires' }) + `@ApiProperty`({ description: 'When the invite expires', type: 'string', format: 'date-time' }) expiresAt!: Date; - `@ApiProperty`({ description: 'When the invite was created' }) + `@ApiProperty`({ description: 'When the invite was created', type: 'string', format: 'date-time' }) createdAt!: Date;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/dto/invite-response.dto.ts` around lines 26 - 30, Update the `@ApiProperty` decorators for the Date fields in InviteResponseDto: change the decorators on the expiresAt and createdAt properties (expiresAt, createdAt) to explicitly specify the OpenAPI type and format by adding { type: 'string', format: 'date-time' } so the generated spec marks these Date fields as date-time strings rather than relying on inference.tests/e2e/page-objects/dialogs/invite-link-tab.page.ts (1)
151-157:waitForLoadedstill doesn't handle the API error state — will time out instead of failing fast.If the API call fails and the component renders
.share-error, neither.invite-link-itemnor.share-recipients-emptymatches, causing a 10s timeout before the test fails with an unhelpful error.Proposed fix
async waitForLoaded(): Promise<void> { - await this.page - .locator('.invite-link-item, .invite-link-tab .share-recipients-empty') - .first() - .waitFor({ state: 'visible', timeout: 10000 }); + await this.page + .locator( + '.invite-link-item, .invite-link-tab .share-recipients-empty, .invite-link-tab .share-error' + ) + .first() + .waitFor({ state: 'visible', timeout: 10000 }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/page-objects/dialogs/invite-link-tab.page.ts` around lines 151 - 157, The waitForLoaded method currently waits only for '.invite-link-item' or '.share-recipients-empty' and will timeout if the API error state renders '.share-error'; update waitForLoaded to wait for any of the three selectors ('.invite-link-item', '.invite-link-tab .share-recipients-empty', '.share-error') and immediately fail-fast when '.share-error' becomes visible by throwing a clear error (reference the waitForLoaded method and the '.share-error' selector so the test fails with a helpful message instead of timing out).apps/api/src/shares/shares.service.ts (1)
403-504:⚠️ Potential issue | 🟠 MajorMake invite claim + share creation transactional.
The invite is marked claimed before Share/ShareKey writes. If those writes fail, the invite stays claimed and the recipient can’t retry. Wrap the UPDATE and share creation in a single transaction (or reorder with a conditional update after successful writes).
🤖 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 403 - 504, The current claimInvite flow marks the ShareInvite as claimed via inviteRepo.createQueryBuilder().update(...) before creating Share/ShareKey, so failures leave the invite consumed; wrap the UPDATE and the subsequent Share/ShareKey creation (use of inviteRepo, shareRepo, shareKeyRepo and operations create/save/remove) in a single database transaction (or perform the UPDATE as the final step inside the same transaction) so the invite status and the created Share/ShareKey are committed atomically and rolled back together on error; use your ORM transaction manager/QueryRunner to beginTransaction(), run the SELECT/UPDATE, create/save Share and ShareKey records, then commit or rollback on failure.apps/web/src/services/invite.service.ts (1)
12-67:⚠️ Potential issue | 🟠 MajorReplace
@noble/secp256k1with an approved ECIES/ECDSA library.Frontend crypto guidelines restrict ECIES/ECDSA operations to ethers.js or libsodium.js. Please switch keypair generation to one of those libraries.
🔧 Example using ethers.js
-import * as secp256k1 from '@noble/secp256k1'; +import { Wallet, getBytes } from 'ethers'; ... function generateEphemeralKeypair(): { privateKey: Uint8Array; publicKey: Uint8Array; privateKeyHex: string; } { - const keypair = secp256k1.keygen(); - return { - privateKey: keypair.secretKey, - publicKey: secp256k1.getPublicKey(keypair.secretKey, false), // uncompressed 65-byte key for ECIES - privateKeyHex: bytesToHex(keypair.secretKey), - }; + const wallet = Wallet.createRandom(); + const privateKey = getBytes(wallet.privateKey); // 32 bytes + const publicKey = getBytes(wallet.publicKey); // uncompressed 65 bytes + return { + privateKey, + publicKey, + privateKeyHex: wallet.privateKey.replace(/^0x/, ''), + }; }#!/bin/bash # Verify which crypto libs are available and where `@noble/secp256k1` is used. rg -n "@noble/secp256k1|ethers|libsodium" apps/web/src/services/invite.service.ts apps/web/package.jsonAs per coding guidelines: Use Web Crypto API or libsodium.js for implementing AES-256-GCM, HKDF-SHA256 key derivation - for ECIES secp256k1 and ECDSA use ethers.js or libsodium.js; never implement custom cryptographic functions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/services/invite.service.ts` around lines 12 - 67, The generateEphemeralKeypair function currently imports and uses `@noble/secp256k1`; replace that with an approved library (ethers.js or libsodium.js) per frontend crypto guidelines. Update the import (remove `@noble/secp256k1`) and re-implement generateEphemeralKeypair to produce the same outputs (privateKey: Uint8Array, publicKey: Uint8Array as uncompressed 65-byte EC public key, privateKeyHex string) using ethers.js (e.g., Wallet.createRandom() for a private key and utils.computePublicKey to derive the uncompressed public key) or libsodium.js equivalent; keep bytesToHex for privateKeyHex and ensure any downstream callers of generateEphemeralKeypair and its returned shapes remain compatible. Also remove/replace any other uses of secp256k1 in this file (search for secp256k1) so the module imports only approved crypto libs.packages/api-client/openapi.json (2)
1587-1641: Claim endpoint response schema now properly defined.The
ClaimInviteResponseDtoschema (lines 2963–2972) correctly describes the{ shareId: string }response body, resolving the previously flagged issue about the missing response schema.🤖 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 1587 - 1641, The OpenAPI spec for the POST /invites/{token}/claim (operationId InvitesController_claimInvite) must return the defined ClaimInviteResponseDto for the 201 response; update the operation's responses so the 201 content schema references ClaimInviteResponseDto (which describes { shareId: string }) and ensure the summary/tags remain accurate; verify the requestBody still references ClaimInviteDto and that security includes bearer as shown.
2885-2926:InviteDataResponseDto.encryptedChildKeyscorrectly typed as array.The field is now
"type": "array"with"items": { "$ref": "...InviteChildKeyDto" }(lines 2900–2903), resolving the previously flagged issue where it was incorrectly generated as"type": "object".🤖 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 2885 - 2926, InviteDataResponseDto previously had encryptedChildKeys typed as "object" but the diff fixes it to "type": "array" with "items": { "$ref": "#/components/schemas/InviteChildKeyDto" }; confirm and keep this change by ensuring InviteDataResponseDto.encryptedChildKeys is an array with the correct items schema (InviteChildKeyDto) and nullable: true remains if null is allowed, and verify the property remains listed in the required array.
🧹 Nitpick comments (9)
apps/web/src/styles/invite-page.css (2)
123-135:.invite-card__claimingand.invite-card__successare identical rule sets.These two classes share the exact same declarations. If the intent is for them to diverge later, this is fine as-is; otherwise they could be combined into a single rule with a comma-separated selector to reduce duplication.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/styles/invite-page.css` around lines 123 - 135, The CSS contains duplicated rule sets for .invite-card__claiming and .invite-card__success; combine them into a single rule using a comma-separated selector (.invite-card__claiming, .invite-card__success) to remove duplication (or keep separate only if you plan differing styles later), ensuring the shared declarations (font-family, font-size, color, text-align) are defined once.
39-41: Hardcoded error color#ef4444repeated in three places — consider a CSS variable.The color
#ef4444is used for.invite-card--error,.invite-card__error, and.invite-card__login-error. Extracting it into a CSS custom property (e.g.,--color-error) would improve maintainability and keep it consistent with the rest of the design token pattern used throughout this file.Also applies to: 141-147, 186-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/styles/invite-page.css` around lines 39 - 41, Extract the hardcoded error color into a CSS custom property and use it in the three selectors; define something like --color-error: `#ef4444` (e.g., in :root or the .invite-card root) and replace the literal color in .invite-card--error, .invite-card__error, and .invite-card__login-error with var(--color-error) so all three use the same token and are easy to update.apps/api/src/shares/invites.controller.spec.ts (2)
131-140: Static analysis:as anycasts onmockReq.Lines 136 and 149 cast
mockReq as anyto satisfy the controller parameter type. SincemockReqis already typed as{ user: { id: string } }, you could type it asRequestWithUser(importing from../common/types) to eliminate theas anycasts and satisfy the linter.Proposed improvement
+import { RequestWithUser } from '../common/types'; ... -const mockReq: { user: { id: string } } = { user: { id: userId } }; +const mockReq = { user: { id: userId } } as RequestWithUser;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/invites.controller.spec.ts` around lines 131 - 140, The test uses an unsafe cast "mockReq as any" to call controller.claimInvite; replace this by typing mockReq as RequestWithUser (import RequestWithUser from ../common/types) so the request matches the controller signature without any casts, update the mockReq declaration to use RequestWithUser and keep using the same mock shape ({ user: { id: userId } }) so controller.claimInvite and the expectations (mockSharesService.claimInvite) compile cleanly without as any.
27-27: Static analysis:as anyfor mock entity relation.The
sharer: {} as anyis flagged by the linter. In test files this is a common pattern for mocking entity relations, but you could define a minimal type or use a type assertion to a partial entity instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/invites.controller.spec.ts` at line 27, The test uses a broad cast "sharer: {} as any" which the linter flags; replace it with a minimally typed mock for the relation instead: declare a typed partial mock for the "sharer" (e.g. const sharer: Partial<YourEntityType> = { id: 'mock-id', email: 'mock@example.com' } or similar minimal properties used by the test) and use that in place of "{} as any" (reference the "sharer" variable in invites.controller.spec.ts and cast only if necessary to the exact entity type rather than using any)..planning/phases/15-link-sharing/15-RESEARCH.md (1)
60-61: Add a language to the fenced code block (MD040).markdownlint flags the project-structure block missing a language identifier.
✍️ Suggested fix
-``` +```text apps/api/src/shares/ entities/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/phases/15-link-sharing/15-RESEARCH.md around lines 60 - 61, The fenced code block that lists the project structure (the block containing "apps/api/src/shares/" and "entities/") is missing a language identifier (MD040); open the code block's opening triple backticks and add a language token (e.g., "text") so the block starts with ```text, keeping the inner content unchanged to satisfy markdownlint.tests/e2e/tests/invite-link-workflow.spec.ts (3)
38-47:parseInviteUrl— non-null assertion onephemeralKeywill throw opaque error on malformed URLs.If the URL doesn't contain a
?key=param,params.get('key')returnsnulland the!assertion silently producesnullat runtime (TypeScript assertions are erased). The test would then pass anullephemeral key, causing a confusing downstream failure.Since this is test code, this is low-risk — a malformed URL means the test is already broken. But a guard would improve debuggability.
Optional improvement
const params = new URLSearchParams(query); - const ephemeralKey = params.get('key')!; + const ephemeralKey = params.get('key'); + if (!ephemeralKey) throw new Error(`No 'key' param in invite URL: ${url}`); return { token, ephemeralKey };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/invite-link-workflow.spec.ts` around lines 38 - 47, The parseInviteUrl function uses a non-null assertion on params.get('key') which can yield a null ephemeralKey for malformed URLs; update parseInviteUrl to validate inputs (ensure hashPart exists, token parsed from path is present, and params.get('key') !== null) and throw a clear, descriptive Error (including the original url) when any piece is missing so tests fail fast and with a helpful message; reference the parseInviteUrl function and its local vars hashPart, token, params, and ephemeralKey when making the checks.
126-129: Hard-codedwaitForTimeout(500)inaliceNavigateBackis a minor flakiness risk.Consider waiting for a specific navigation state (e.g., breadcrumb change or URL change) instead of a fixed delay. However, this pattern appears to be used in existing sharing workflow tests, so it's consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/invite-link-workflow.spec.ts` around lines 126 - 129, The helper aliceNavigateBack currently uses a hard-coded await alice.page.waitForTimeout(500) which can cause flakiness; update aliceNavigateBack to wait for a deterministic UI/navigation signal instead (for example: wait for breadcrumb text to change, wait for the parent directory row to appear/disappear, or use alice.page.waitForURL to detect the URL change) by replacing the fixed timeout with an explicit wait (e.g., waitForSelector or waitForFunction targeting the breadcrumb or row state) after the dblclick on '[data-testid="parent-dir-row"]' so the test proceeds only when navigation is complete.
431-463: Folder traversal test uses multiple hard-coded waits (3s + 2s) plus a retry pattern.Lines 438 and 456 use
waitForTimeoutwith 3s and 2s respectively. This is pragmatic given IPNS resolution latency but could contribute to test flakiness or unnecessarily slow runs. The retry pattern on lines 440–447 is a good resilience measure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/invite-link-workflow.spec.ts` around lines 431 - 463, Replace the hard-coded waits and one-shot retry with deterministic waits and a bounded retry loop: instead of eve.page.waitForTimeout(3000) and waitForTimeout(2000), wait for specific UI conditions such as parentDirRow().isVisible(), waitForSharedItem(sharedFolderName), or getFolderItemNames() to include the expected items; update the post-open logic in navigateToShared/openSharedItem flow to poll for parentDirRow visibility (or folder item presence) with a short interval and overall timeout, and turn the single retry block (which calls navigateToShared, waitForLoaded, waitForSharedItem, navigateIntoFolder) into a retry-until-success loop that throws after a max timeout to avoid flakiness; use the existing helpers (parentDirRow, waitForLoaded, waitForSharedItem, getFolderItemNames, doubleClickFolderItem, navigateIntoFolder, navigateToRoot) to implement these waits.apps/web/src/lib/crypto/key-wrapping.ts (1)
101-110: Progress callback in recursive case reports approximate values.The sub-progress callback
onProgress(wrapped + subWrapped)(line 107) uses thewrappedvalue captured at the start of this folder's processing. For deeply nested trees, the total reported during recursion may briefly under-count keys wrapped in sibling branches that were processed before this subfolder. This is cosmetic and fine for a progress indicator, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/crypto/key-wrapping.ts` around lines 101 - 110, The progress callback is using a stale pre-captured total (wrapped) when reporting subfolder progress via onProgress(wrapped + subWrapped); update the callback so it computes the total at callback execution instead of using the initial captured value — e.g., read the current outer total inside the subWrapped callback or have collectChildKeys pass absolute counts back (use symbols: collectChildKeys, onProgress, wrapped, subWrapped, metadata.children) so the reported progress is always wrapped (current) + subWrapped rather than a potentially stale wrapped value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.planning/phases/15-link-sharing/15-CONTEXT.md:
- Line 20: The Heading "### Sharing model" is flagged for a level jump (MD001);
fix it by adjusting the heading level or surrounding structure so heading levels
only change by one—either change "### Sharing model" to the correct level (e.g.,
"## Sharing model" or "#### Sharing model" to match adjacent headings) or insert
the missing intermediate heading above it; locate the literal heading "###
Sharing model" in the file and update it accordingly.
In `@apps/api/src/shares/dto/claim-invite.dto.ts`:
- Around line 23-28: The itemId property in the DTO is currently only validated
as a non-empty string; add UUID validation by decorating itemId with `@IsUUID`()
(on the same property as `@IsString`()/@MinLength(1)) so malformed DB IDs fail
fast, and import IsUUID from class-validator alongside the other validators
(target the itemId field in the claim-invite DTO).
- Around line 30-48: The encryptedKey fields currently use `@MinLength`(2) which
is too small for an ECIES secp256k1 ciphertext; update the validation on the
encryptedKey property in both ClaimChildKeyDto (the first class with
encryptedKey) and ClaimInviteDto to use `@MinLength`(258) (258 hex chars = 129
bytes) and keep the existing `@MaxLength`(1024) and `@Matches` regex unchanged so
downstream ECIES unwrap will receive a realistically sized payload.
In `@apps/api/src/shares/share-invites.controller.spec.ts`:
- Around line 22-24: The test uses an ad-hoc type for mockReq and several "as
any" casts; import RequestWithUser from ../common/types, change the mockReq
declaration to use RequestWithUser (e.g., const mockReq: RequestWithUser = {
user: { id: userId } }), update any places where mockReq is passed to controller
methods to remove "as any" casts, and run tests to ensure type compatibility
with controller methods (adjust mockReq shape to include any additional
RequestWithUser fields used by methods).
In `@apps/web/src/api/models/inviteDataResponseDtoEncryptedChildKeys.ts`:
- Around line 9-13: Remove the orphaned generated type
InviteDataResponseDtoEncryptedChildKeys by deleting the file
apps/web/src/api/models/inviteDataResponseDtoEncryptedChildKeys.ts; verify there
are no remaining imports of InviteDataResponseDtoEncryptedChildKeys (search
across the repo), remove any references from barrels or index exports, and
confirm InviteDataResponseDto now uses the inlined encryptedChildKeys:
InviteChildKeyDto[] | null type; run the TypeScript build/tests to ensure no
missing type references remain.
In `@designs/cipher-box-design.pen`:
- Around line 39323-39332: The text color `#4a5a4e` used for small informational
elements (e.g., secNote id 9NV7i, signupNote id QR1Lu, expiry1 id K7mUm and
similar 9–10px labels) fails WCAG AA contrast against black (~3.12:1); update
these text fills to a lighter muted token that meets ≥4.5:1 on `#000000` (or
alternatively increase those specific fontSize values to ≥24px to qualify as
large text) — locate and replace the fill value for secNote, signupNote,
footerText, errFooterText, expiry1 (and other instances of `#4a5a4e`) with an
accessible hex (or update fontSize) and verify contrast ratio reaches at least
4.5:1.
---
Outside diff comments:
In @.planning/REQUIREMENTS.md:
- Line 427: Update the stale footer string "Last updated: 2026-02-11 after
Milestone 3 roadmap creation (M3 phase mappings added)" in
.planning/REQUIREMENTS.md to the current Phase 15 update date and wording (e.g.,
"Last updated: 2026-02-23 after Phase 15 status update") so the document
reflects the new status; locate the exact footer line containing that original
phrase and replace the date and parenthetical note accordingly.
---
Duplicate comments:
In @.planning/phases/15-link-sharing/15-VERIFICATION.md:
- Around line 6-20: The frontend still maps invite.token to the visible id
causing revoke to send a base64 token to the DELETE endpoint which expects a
UUID; update fetchInvitesForItem in apps/web/src/services/invite.service.ts to
use the real UUID field (inv.id) returned by InviteResponseDto (or adjust the
backend list response to include id and regenerate the API client), and ensure
revokeInvite (share-invites.controller's DELETE handler) receives that UUID
value rather than the token so ParseUUIDPipe succeeds.
In `@apps/api/src/shares/dto/create-invite.dto.ts`:
- Around line 23-28: The itemId property on the CreateInviteDto is currently
validated only as a non-empty string and must be validated as a UUID to avoid
malformed IDs; update the CreateInviteDto's itemId validators to use `@IsUUID`()
(and remove or replace `@IsString`()/@MinLength(1) as appropriate) and add the
necessary import for IsUUID from class-validator so incoming itemId values are
enforced as valid UUIDs.
- Around line 30-71: The encryptedKey MinLength is too low (currently 2)
allowing invalid ciphertexts; update the `@MinLength` on both encryptedKey
properties (the one above and the encryptedKey in class CreateInviteDto) to the
realistic minimum for ECIES secp256k1 AES‑256‑GCM with an uncompressed public
key: 250 hex characters (125 bytes * 2), i.e., set `@MinLength`(250) for those
encryptedKey fields so validators reject undersized/garbage payloads.
In `@apps/api/src/shares/dto/invite-response.dto.ts`:
- Around line 26-30: Update the `@ApiProperty` decorators for the Date fields in
InviteResponseDto: change the decorators on the expiresAt and createdAt
properties (expiresAt, createdAt) to explicitly specify the OpenAPI type and
format by adding { type: 'string', format: 'date-time' } so the generated spec
marks these Date fields as date-time strings rather than relying on inference.
In `@apps/api/src/shares/share-invites.controller.spec.ts`:
- Around line 140-147: The test in share-invites.controller.spec.ts should also
assert that list responses do not include child key data: after calling
mockSharesService.getInvitesForItem and awaiting controller.listInvites (the
existing test using mockReq and item id), add an assertion that
'encryptedChildKeys' is not present on result[0] (similar to the existing checks
for 'encryptedKey' and 'sharerId'); update the test that uses
mockSharesService.getInvitesForItem and the result variable to include
expect('encryptedChildKeys' in result[0]).toBe(false).
In `@apps/api/src/shares/shares.service.ts`:
- Around line 403-504: The current claimInvite flow marks the ShareInvite as
claimed via inviteRepo.createQueryBuilder().update(...) before creating
Share/ShareKey, so failures leave the invite consumed; wrap the UPDATE and the
subsequent Share/ShareKey creation (use of inviteRepo, shareRepo, shareKeyRepo
and operations create/save/remove) in a single database transaction (or perform
the UPDATE as the final step inside the same transaction) so the invite status
and the created Share/ShareKey are committed atomically and rolled back together
on error; use your ORM transaction manager/QueryRunner to beginTransaction(),
run the SELECT/UPDATE, create/save Share and ShareKey records, then commit or
rollback on failure.
In `@apps/web/src/components/file-browser/InviteLinkTab.tsx`:
- Around line 48-58: When fetchInvitesForItem(ipnsName) fails, the catch
currently only logs the error which leads the UI to show "no active invite
links"; update the catch to set an error state so the existing error alert
renders: inside the .catch((err) => { if (!cancelled) { ... } }) call set the
component's invite-fetch error state (use the existing error setter if present,
otherwise add one like setInvitesError) with the error or a message, and ensure
you still keep console.error; keep the cancelled check and do not modify the
successful .then branch that calls setInvites.
In `@apps/web/src/components/file-browser/ShareDialog.tsx`:
- Around line 351-490: The tab bar in ShareDialog uses role="tab" but lacks ids,
aria-controls, roving tabIndex and arrow-key handling, and the panels lack
aria-labelledby—make the tab buttons fully accessible by assigning unique ids
(e.g., "share-tab-direct" and "share-tab-invite"), add matching aria-controls
pointing to panel ids ("share-panel-direct"/"share-panel-invite"), implement
roving focus/tabIndex logic and left/right/up/down arrow key handling in a new
handler (e.g., handleTabKeyDown) used on the tab buttons to call setActiveTab
and move focus, and ensure each tabpanel (including InviteLinkTab) has id and
aria-labelledby referencing its tab id (or pass those props into InviteLinkTab
so its root uses id="share-panel-invite" and
aria-labelledby="share-tab-invite"); update any aria-selected usage to reflect
the activeTab variable.
In `@apps/web/src/services/invite.service.ts`:
- Around line 12-67: The generateEphemeralKeypair function currently imports and
uses `@noble/secp256k1`; replace that with an approved library (ethers.js or
libsodium.js) per frontend crypto guidelines. Update the import (remove
`@noble/secp256k1`) and re-implement generateEphemeralKeypair to produce the same
outputs (privateKey: Uint8Array, publicKey: Uint8Array as uncompressed 65-byte
EC public key, privateKeyHex string) using ethers.js (e.g.,
Wallet.createRandom() for a private key and utils.computePublicKey to derive the
uncompressed public key) or libsodium.js equivalent; keep bytesToHex for
privateKeyHex and ensure any downstream callers of generateEphemeralKeypair and
its returned shapes remain compatible. Also remove/replace any other uses of
secp256k1 in this file (search for secp256k1) so the module imports only
approved crypto libs.
In `@apps/web/src/styles/share-dialog.css`:
- Around line 430-435: The .invite-status-badge rule currently hard-codes
font-size: 10px; replace that literal with the project's design token (e.g.,
font-size: var(--font-size-xs) or the appropriate token used elsewhere in this
file) so the badge follows tokenized sizing; update the font-size declaration in
the .invite-status-badge CSS rule to use the matching --font-size-* token used
by other selectors in this stylesheet.
In `@designs/cipher-box-design.pen`:
- Around line 39304-39312: The subtext element with id "TBokI" currently uses
fill "#006644" on a "#000000" background and fails WCAG AA contrast for 11px
text (≈3.36:1); update the element (id "TBokI", name "subtext") to use a darker
color value that achieves at least 4.5:1 contrast against black (or
alternatively increase the font size/weight to meet 4.5:1), e.g., replace the
fill with a darker hex that meets the ratio and keep fontFamily "JetBrains Mono"
and fontSize 11 unless you choose the size/weight change. Ensure the changed
color/typography is applied to the "subtext" element so it passes automated
contrast checks.
- Around line 39514-39524: The errSubtext element (id: hSiQp / name: errSubtext)
has two outstanding issues: the color `#006644` at fontSize 11 fails contrast on a
dark background, and the copy contains a hardcoded TTL string ("Invite links are
valid for 7 days.") that can drift from backend values; fix by choosing a
higher-contrast color or increasing size/weight so WCAG AA contrast is met for
11px text (or switch to a vetted token from the design palette) and replace the
literal TTL sentence with a dynamic placeholder/variable (e.g., use a
backend-driven {ttlText} or localization key) so the UI displays the canonical
TTL from the backend rather than a hardcoded value.
- Around line 40956-40964: The node gzqpC (name: policyNote) contains a
hardcoded policy string "// expires in 7 days, single-claim"; replace this
hardcoded copy with a variable-driven or templated value sourced from the
canonical backend TTL/claim contract (e.g., use a policyNoteText variable or
placeholder that is populated at build/runtime from the backend config or
localization file) so the UI reflects the confirmed TTL/claim terms; update any
code that renders gzqpC to read that variable instead of the literal string and
ensure the variable name matches the backend field used for TTL/claim text.
- Around line 39673-39678: Replace all occurrences of the color "#006644" in the
"P15 - Share Dialog (Invite Tab)" frame with an accessible color that meets WCAG
AA 4.5:1 contrast for the small text sizes used (fontSize 9–11), or
alternatively increase the text weight/size where appropriate; specifically
update instances referenced as userEmail / i074c, breadcrumb EAQ4X and EbdH7,
column header layers VeXI1, c3Y0s, pV4PG, file row metadata cells, sidebar
labels settingsText and quotaText, footer links, statusText, tabDirectText,
desc, and linksLabel so that each element's computed contrast ratio against its
background meets 4.5:1. Ensure the replacement color is applied to the "fill"
property for those text layers (e.g., where "fill": "#006644" appears) and
verify visually at the target font sizes.
In `@packages/api-client/openapi.json`:
- Around line 1587-1641: The OpenAPI spec for the POST /invites/{token}/claim
(operationId InvitesController_claimInvite) must return the defined
ClaimInviteResponseDto for the 201 response; update the operation's responses so
the 201 content schema references ClaimInviteResponseDto (which describes {
shareId: string }) and ensure the summary/tags remain accurate; verify the
requestBody still references ClaimInviteDto and that security includes bearer as
shown.
- Around line 2885-2926: InviteDataResponseDto previously had encryptedChildKeys
typed as "object" but the diff fixes it to "type": "array" with "items": {
"$ref": "#/components/schemas/InviteChildKeyDto" }; confirm and keep this change
by ensuring InviteDataResponseDto.encryptedChildKeys is an array with the
correct items schema (InviteChildKeyDto) and nullable: true remains if null is
allowed, and verify the property remains listed in the required array.
In `@tests/e2e/page-objects/dialogs/invite-link-tab.page.ts`:
- Around line 151-157: The waitForLoaded method currently waits only for
'.invite-link-item' or '.share-recipients-empty' and will timeout if the API
error state renders '.share-error'; update waitForLoaded to wait for any of the
three selectors ('.invite-link-item', '.invite-link-tab
.share-recipients-empty', '.share-error') and immediately fail-fast when
'.share-error' becomes visible by throwing a clear error (reference the
waitForLoaded method and the '.share-error' selector so the test fails with a
helpful message instead of timing out).
---
Nitpick comments:
In @.planning/phases/15-link-sharing/15-RESEARCH.md:
- Around line 60-61: The fenced code block that lists the project structure (the
block containing "apps/api/src/shares/" and "entities/") is missing a language
identifier (MD040); open the code block's opening triple backticks and add a
language token (e.g., "text") so the block starts with ```text, keeping the
inner content unchanged to satisfy markdownlint.
In `@apps/api/src/shares/invites.controller.spec.ts`:
- Around line 131-140: The test uses an unsafe cast "mockReq as any" to call
controller.claimInvite; replace this by typing mockReq as RequestWithUser
(import RequestWithUser from ../common/types) so the request matches the
controller signature without any casts, update the mockReq declaration to use
RequestWithUser and keep using the same mock shape ({ user: { id: userId } }) so
controller.claimInvite and the expectations (mockSharesService.claimInvite)
compile cleanly without as any.
- Line 27: The test uses a broad cast "sharer: {} as any" which the linter
flags; replace it with a minimally typed mock for the relation instead: declare
a typed partial mock for the "sharer" (e.g. const sharer:
Partial<YourEntityType> = { id: 'mock-id', email: 'mock@example.com' } or
similar minimal properties used by the test) and use that in place of "{} as
any" (reference the "sharer" variable in invites.controller.spec.ts and cast
only if necessary to the exact entity type rather than using any).
In `@apps/web/src/lib/crypto/key-wrapping.ts`:
- Around line 101-110: The progress callback is using a stale pre-captured total
(wrapped) when reporting subfolder progress via onProgress(wrapped +
subWrapped); update the callback so it computes the total at callback execution
instead of using the initial captured value — e.g., read the current outer total
inside the subWrapped callback or have collectChildKeys pass absolute counts
back (use symbols: collectChildKeys, onProgress, wrapped, subWrapped,
metadata.children) so the reported progress is always wrapped (current) +
subWrapped rather than a potentially stale wrapped value.
In `@apps/web/src/styles/invite-page.css`:
- Around line 123-135: The CSS contains duplicated rule sets for
.invite-card__claiming and .invite-card__success; combine them into a single
rule using a comma-separated selector (.invite-card__claiming,
.invite-card__success) to remove duplication (or keep separate only if you plan
differing styles later), ensuring the shared declarations (font-family,
font-size, color, text-align) are defined once.
- Around line 39-41: Extract the hardcoded error color into a CSS custom
property and use it in the three selectors; define something like --color-error:
`#ef4444` (e.g., in :root or the .invite-card root) and replace the literal color
in .invite-card--error, .invite-card__error, and .invite-card__login-error with
var(--color-error) so all three use the same token and are easy to update.
In `@tests/e2e/tests/invite-link-workflow.spec.ts`:
- Around line 38-47: The parseInviteUrl function uses a non-null assertion on
params.get('key') which can yield a null ephemeralKey for malformed URLs; update
parseInviteUrl to validate inputs (ensure hashPart exists, token parsed from
path is present, and params.get('key') !== null) and throw a clear, descriptive
Error (including the original url) when any piece is missing so tests fail fast
and with a helpful message; reference the parseInviteUrl function and its local
vars hashPart, token, params, and ephemeralKey when making the checks.
- Around line 126-129: The helper aliceNavigateBack currently uses a hard-coded
await alice.page.waitForTimeout(500) which can cause flakiness; update
aliceNavigateBack to wait for a deterministic UI/navigation signal instead (for
example: wait for breadcrumb text to change, wait for the parent directory row
to appear/disappear, or use alice.page.waitForURL to detect the URL change) by
replacing the fixed timeout with an explicit wait (e.g., waitForSelector or
waitForFunction targeting the breadcrumb or row state) after the dblclick on
'[data-testid="parent-dir-row"]' so the test proceeds only when navigation is
complete.
- Around line 431-463: Replace the hard-coded waits and one-shot retry with
deterministic waits and a bounded retry loop: instead of
eve.page.waitForTimeout(3000) and waitForTimeout(2000), wait for specific UI
conditions such as parentDirRow().isVisible(),
waitForSharedItem(sharedFolderName), or getFolderItemNames() to include the
expected items; update the post-open logic in navigateToShared/openSharedItem
flow to poll for parentDirRow visibility (or folder item presence) with a short
interval and overall timeout, and turn the single retry block (which calls
navigateToShared, waitForLoaded, waitForSharedItem, navigateIntoFolder) into a
retry-until-success loop that throws after a max timeout to avoid flakiness; use
the existing helpers (parentDirRow, waitForLoaded, waitForSharedItem,
getFolderItemNames, doubleClickFolderItem, navigateIntoFolder, navigateToRoot)
to implement these waits.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (73)
.planning/REQUIREMENTS.md.planning/ROADMAP.md.planning/STATE.md.planning/phases/15-link-sharing/15-01-PLAN.md.planning/phases/15-link-sharing/15-01-SUMMARY.md.planning/phases/15-link-sharing/15-02-PLAN.md.planning/phases/15-link-sharing/15-02-SUMMARY.md.planning/phases/15-link-sharing/15-03-PLAN.md.planning/phases/15-link-sharing/15-03-SUMMARY.md.planning/phases/15-link-sharing/15-04-PLAN.md.planning/phases/15-link-sharing/15-04-SUMMARY.md.planning/phases/15-link-sharing/15-CONTEXT.md.planning/phases/15-link-sharing/15-RESEARCH.md.planning/phases/15-link-sharing/15-VERIFICATION.md.planning/security/REVIEW-phase15-link-sharing.mdapps/api/scripts/generate-openapi.tsapps/api/src/app.module.tsapps/api/src/common/types.tsapps/api/src/device-approval/device-approval.controller.tsapps/api/src/ipfs/ipfs.controller.tsapps/api/src/ipns/ipns.controller.tsapps/api/src/migrations/1740400000000-AddShareInvites.tsapps/api/src/shares/dto/claim-invite.dto.tsapps/api/src/shares/dto/create-invite.dto.tsapps/api/src/shares/dto/invite-response.dto.tsapps/api/src/shares/entities/index.tsapps/api/src/shares/entities/share-invite.entity.tsapps/api/src/shares/invites.controller.spec.tsapps/api/src/shares/invites.controller.tsapps/api/src/shares/share-invites.controller.spec.tsapps/api/src/shares/share-invites.controller.tsapps/api/src/shares/shares.controller.tsapps/api/src/shares/shares.module.tsapps/api/src/shares/shares.service.spec.tsapps/api/src/shares/shares.service.tsapps/api/src/vault/vault.controller.tsapps/web/src/api/invites/invites.tsapps/web/src/api/models/claimChildKeyDto.tsapps/web/src/api/models/claimChildKeyDtoKeyType.tsapps/web/src/api/models/claimInviteDto.tsapps/web/src/api/models/claimInviteResponseDto.tsapps/web/src/api/models/createInviteDto.tsapps/web/src/api/models/createInviteDtoItemType.tsapps/web/src/api/models/index.tsapps/web/src/api/models/inviteChildKeyDto.tsapps/web/src/api/models/inviteChildKeyDtoKeyType.tsapps/web/src/api/models/inviteDataResponseDto.tsapps/web/src/api/models/inviteDataResponseDtoEncryptedChildKeys.tsapps/web/src/api/models/inviteDataResponseDtoItemType.tsapps/web/src/api/models/inviteDataResponseDtoStatus.tsapps/web/src/api/models/inviteResponseDto.tsapps/web/src/api/models/inviteResponseDtoItemType.tsapps/web/src/api/models/inviteResponseDtoStatus.tsapps/web/src/api/models/inviteStatusResponseDto.tsapps/web/src/api/models/inviteStatusResponseDtoStatus.tsapps/web/src/api/models/shareInvitesControllerListInvitesParams.tsapps/web/src/api/share-invites/share-invites.tsapps/web/src/components/file-browser/InviteLinkTab.tsxapps/web/src/components/file-browser/ShareDialog.tsxapps/web/src/lib/crypto/key-wrapping.tsapps/web/src/routes/InvitePage.tsxapps/web/src/routes/index.tsxapps/web/src/services/invite.service.tsapps/web/src/styles/invite-page.cssapps/web/src/styles/share-dialog.cssdesigns/DESIGN.mddesigns/cipher-box-design.penpackages/api-client/openapi.jsontests/e2e/page-objects/dialogs/index.tstests/e2e/page-objects/dialogs/invite-link-tab.page.tstests/e2e/page-objects/index.tstests/e2e/page-objects/pages/invite.page.tstests/e2e/tests/invite-link-workflow.spec.ts
Delete InviteDataResponseDtoEncryptedChildKeys which was left over from before the DTO was fixed to produce InviteChildKeyDto[]. No code imports it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 74bfe3774b89
Add per-file coverage overrides for 3 new shares controllers whose NestJS/Swagger decorators create phantom branches (following established pattern for auth, vault, ipfs controllers). Add unit tests for 0x-prefix normalization, duplicate-key race condition, and non-duplicate error rethrow in SharesService. Fix E2E invite-link tests to tolerate headless CI clipboard unavailability by catching writeText errors and accepting either success message variant. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 052e8e48c9d3
The status endpoint collapses all non-active statuses to 404 to prevent token-existence oracle attacks, so the client cannot distinguish revoked from expired. Update assertion to accept either message. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: d4b54879e7a4
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/web/src/components/file-browser/InviteLinkTab.tsx (1)
84-99:⚠️ Potential issue | 🟡 MinorClipboard failure still hides the full invite URL.
When copy fails, the success message only shows a truncated URL, which isn’t usable for manual copy. Consider showing the full URL in the failure path.
💡 Suggested fix
- setSuccess( - copied - ? `link copied to clipboard: ${displayUrl}` - : `link created (copy failed -- save this): ${displayUrl}` - ); + setSuccess( + copied + ? `link copied to clipboard: ${displayUrl}` + : `link created (copy failed -- save this): ${url}` + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/InviteLinkTab.tsx` around lines 84 - 99, The success message currently always uses the truncated displayUrl (const displayUrl) so when clipboard copy fails users only see a shortened link; update the failure branch to include the full url variable instead of displayUrl by changing the setSuccess call so the copied ? branch stays as-is but the false branch uses the full url (and still includes the existing explanatory text), locating this logic around navigator.clipboard.writeText, the copied flag, displayUrl, setSuccess and url in InviteLinkTab.tsx.
🧹 Nitpick comments (5)
apps/api/jest.config.js (1)
84-95: LGTM – new thresholds are consistent with the established controller pattern.All three entries mirror the
auth.controller.ts/vault.controller.tsprecedent (lines 80, branches 65) with the same decorator-inflation rationale. The slightly higher branch floor forinvites.controller.ts(74 vs 65) is a reasonable distinction if that controller carries fewer Swagger-decorated paths.One optional observation: having three separate threshold entries (and presumably three separate controller files) for the
shares/module—shares.controller.ts,share-invites.controller.ts, andinvites.controller.ts—is worth a quick double-check. Ifshare-invites.controller.tsandinvites.controller.tsrepresent overlapping resource concerns, consolidating them would reduce coverage-config fragmentation over time, but this is a design call rather than a defect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/jest.config.js` around lines 84 - 95, The coverage thresholds for '**/shares/shares.controller.ts', '**/shares/share-invites.controller.ts', and '**/shares/invites.controller.ts' are consistent with existing controller patterns (80 lines, branches 65/74) and require no change; keep the three distinct entries as-is, but optionally consider consolidating the two invites controllers later if they prove to be overlapping resources to reduce coverage-config fragmentation.tests/e2e/tests/invite-link-workflow.spec.ts (4)
180-211: Phase numbering is inconsistent: two "Phase 2" headers and test1.2sits under the wrong phase.Test
1.2 Alice creates test content(line 184) sits under a comment block labelled "Phase 2" (line 181), and there is a second "Phase 2: ShareDialog Tab UI" block at line 210. The test name prefix1.2also doesn't match its phase header. Renaming the test to2.0(or adjusting the section comment to "Phase 1 cont'd") would keep the numbering scheme self-consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/invite-link-workflow.spec.ts` around lines 180 - 211, The phase numbering is inconsistent: the comment block "Phase 2: Alice Creates Content" contains the test named "1.2 Alice creates test content" while another "Phase 2: ShareDialog Tab UI" follows; update either the section comment or the test name to keep numbering consistent—e.g., rename the test "1.2 Alice creates test content" to "2.0 Alice creates test content" (reference the test identifier string and the surrounding comment headers "Phase 2: Alice Creates Content" and "Phase 2: ShareDialog Tab UI"), or change the first comment to "Phase 1 (cont'd)". Ensure the test name prefix and the phase comment match.
339-361: Bare browser contexts leak when an assertion throws beforeclose().Tests 4.1, 6.2, 7.1, and 7.2 each create a
bareContextand callbareContext.close()at the end, but if any assertion orwaitForin between throws, the close is skipped. Wrapping intry/finallyprevents context leaks in CI:Example pattern
const bareContext = await browser.newContext(); + try { const barePage = await bareContext.newPage(); const bareInvitePage = new InvitePageObject(barePage); // ... assertions ... - await bareContext.close(); + } finally { + await bareContext.close(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/invite-link-workflow.spec.ts` around lines 339 - 361, The test creates a bareContext/barePage and may skip bareContext.close() if an assertion throws; wrap the lifecycle so the context is always closed: after creating bareContext (and barePage/InvitePageObject) put the assertions and awaits (including waitForValidState, expect checks, etc.) inside a try block and call await bareContext.close() in a finally block to guarantee cleanup; apply the same pattern to other tests that create bareContext (tests 6.2, 7.1, 7.2) so InvitePageObject, waitForValidState, and any subsequent expects cannot prevent closing the context.
39-47:parseInviteUrlsilently swallows a missingkeyparam via non-null assertion.If the invite URL is ever malformed (e.g., missing
?key=),params.get('key')!returnsnullcast tostring, and the real failure surfaces far downstream with a confusing message. Similarly, if#is absent,hashPartisundefinedand line 42 throws a generic TypeError.Adding a guard makes test failures self-explanatory:
Suggested improvement
function parseInviteUrl(url: string): { token: string; ephemeralKey: string } { - // URL format: http://localhost:5173/#/invite/TOKEN?key=KEY const hashPart = url.split('#')[1]; // /invite/TOKEN?key=KEY + if (!hashPart) throw new Error(`parseInviteUrl: no hash fragment in "${url}"`); const [path, query] = hashPart.split('?'); const token = path.split('/invite/')[1]; const params = new URLSearchParams(query); - const ephemeralKey = params.get('key')!; + const ephemeralKey = params.get('key'); + if (!token || !ephemeralKey) { + throw new Error(`parseInviteUrl: could not extract token/key from "${url}"`); + } return { token, ephemeralKey }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/invite-link-workflow.spec.ts` around lines 39 - 47, The parseInviteUrl function currently assumes a hash and key exist and uses non-null assertion on params.get('key'); update parseInviteUrl to validate the hashPart (after url.split('#')), ensure the path contains '/invite/' and token is present, ensure the query exists and URLSearchParams returns a non-null 'key', and throw clear, specific errors (e.g., "Missing URL hash", "Missing invite token", "Missing ephemeral key") referencing the variables hashPart, token, query/params and ephemeralKey so malformed invite URLs fail fast with descriptive messages.
126-129:waitForTimeoutis a Playwright anti-pattern that introduces flakiness.
aliceNavigateBack(and similar calls on lines 438, 456) rely on fixed-duration sleeps. Playwright's own docs recommend waiting on a concrete signal (URL, element, network-idle) instead. For this helper, waiting on a breadcrumb change orwaitForURLwould be more robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/invite-link-workflow.spec.ts` around lines 126 - 129, The helper aliceNavigateBack uses a fixed sleep (alice.page.waitForTimeout) which is flaky; replace that sleep with an explicit Playwright wait for a concrete signal instead—e.g., after alice.page.locator('[data-testid="parent-dir-row"]').dblclick(), wait for the breadcrumb/parent-dir breadcrumb selector to change (waitForSelector with state: 'visible' or 'detached'), or use alice.page.waitForURL to assert the expected URL, or wait for the target directory row to appear via locator.waitFor; update aliceNavigateBack and the similar helpers (the other functions calling waitForTimeout) to use one of these concrete waits rather than waitForTimeout.
🤖 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/api/src/shares/invites.controller.ts`:
- Around line 37-56: The Swagger metadata for getInviteStatus does not document
the 404 thrown via NotFoundException; update the controller annotations by
adding an `@ApiResponse`({ status: 404, description: 'Invite not found' })
alongside the existing `@ApiResponse` for InviteStatusResponseDto (refer to the
getInviteStatus method and InviteStatusResponseDto) so the OpenAPI spec reflects
the NotFoundException response.
In `@apps/api/src/shares/shares.service.ts`:
- Around line 416-448: After fetching the invite in claimInvite
(inviteRepo.findOne), explicitly check the invite's expiry and throw
NotFoundException for expired tokens (e.g. if (invite.expiresAt <= new Date())
throw new NotFoundException('Invite not found or expired')) before the
self-claim check and before entering the transaction; keep the existing atomic
UPDATE in the transaction (createQueryBuilder().update(ShareInvite)...) to
detect concurrent claimed/revoked races and still throw ConflictException when
result.affected < 1 so expired tokens are reported as 404 while
claimed/revoked/contended cases remain 409.
In `@apps/web/src/components/file-browser/InviteLinkTab.tsx`:
- Around line 41-64: The current useEffect that fetches invites (triggered when
isOpen and ipnsName) leaves previous invites in state on refetch or on fetch
failure, which can cause stale entries to be displayed or revoked; update the
effect to clear invites at fetch start by calling setInvites([]) (and keep
setInvitesLoading(true)), and also clear invites on error before setting
setError, ensuring you still honor the cancellation flag (cancelled) — modify
the effect around fetchInvitesForItem, setInvites, setInvitesLoading, setError
and the cancelled handling accordingly.
---
Duplicate comments:
In `@apps/web/src/components/file-browser/InviteLinkTab.tsx`:
- Around line 84-99: The success message currently always uses the truncated
displayUrl (const displayUrl) so when clipboard copy fails users only see a
shortened link; update the failure branch to include the full url variable
instead of displayUrl by changing the setSuccess call so the copied ? branch
stays as-is but the false branch uses the full url (and still includes the
existing explanatory text), locating this logic around
navigator.clipboard.writeText, the copied flag, displayUrl, setSuccess and url
in InviteLinkTab.tsx.
---
Nitpick comments:
In `@apps/api/jest.config.js`:
- Around line 84-95: The coverage thresholds for
'**/shares/shares.controller.ts', '**/shares/share-invites.controller.ts', and
'**/shares/invites.controller.ts' are consistent with existing controller
patterns (80 lines, branches 65/74) and require no change; keep the three
distinct entries as-is, but optionally consider consolidating the two invites
controllers later if they prove to be overlapping resources to reduce
coverage-config fragmentation.
In `@tests/e2e/tests/invite-link-workflow.spec.ts`:
- Around line 180-211: The phase numbering is inconsistent: the comment block
"Phase 2: Alice Creates Content" contains the test named "1.2 Alice creates test
content" while another "Phase 2: ShareDialog Tab UI" follows; update either the
section comment or the test name to keep numbering consistent—e.g., rename the
test "1.2 Alice creates test content" to "2.0 Alice creates test content"
(reference the test identifier string and the surrounding comment headers "Phase
2: Alice Creates Content" and "Phase 2: ShareDialog Tab UI"), or change the
first comment to "Phase 1 (cont'd)". Ensure the test name prefix and the phase
comment match.
- Around line 339-361: The test creates a bareContext/barePage and may skip
bareContext.close() if an assertion throws; wrap the lifecycle so the context is
always closed: after creating bareContext (and barePage/InvitePageObject) put
the assertions and awaits (including waitForValidState, expect checks, etc.)
inside a try block and call await bareContext.close() in a finally block to
guarantee cleanup; apply the same pattern to other tests that create bareContext
(tests 6.2, 7.1, 7.2) so InvitePageObject, waitForValidState, and any subsequent
expects cannot prevent closing the context.
- Around line 39-47: The parseInviteUrl function currently assumes a hash and
key exist and uses non-null assertion on params.get('key'); update
parseInviteUrl to validate the hashPart (after url.split('#')), ensure the path
contains '/invite/' and token is present, ensure the query exists and
URLSearchParams returns a non-null 'key', and throw clear, specific errors
(e.g., "Missing URL hash", "Missing invite token", "Missing ephemeral key")
referencing the variables hashPart, token, query/params and ephemeralKey so
malformed invite URLs fail fast with descriptive messages.
- Around line 126-129: The helper aliceNavigateBack uses a fixed sleep
(alice.page.waitForTimeout) which is flaky; replace that sleep with an explicit
Playwright wait for a concrete signal instead—e.g., after
alice.page.locator('[data-testid="parent-dir-row"]').dblclick(), wait for the
breadcrumb/parent-dir breadcrumb selector to change (waitForSelector with state:
'visible' or 'detached'), or use alice.page.waitForURL to assert the expected
URL, or wait for the target directory row to appear via locator.waitFor; update
aliceNavigateBack and the similar helpers (the other functions calling
waitForTimeout) to use one of these concrete waits rather than waitForTimeout.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/api/jest.config.jsapps/api/src/shares/dto/claim-invite.dto.tsapps/api/src/shares/dto/create-invite.dto.tsapps/api/src/shares/dto/invite-response.dto.tsapps/api/src/shares/invites.controller.spec.tsapps/api/src/shares/invites.controller.tsapps/api/src/shares/share-invites.controller.spec.tsapps/api/src/shares/shares.service.spec.tsapps/api/src/shares/shares.service.tsapps/web/src/api/models/index.tsapps/web/src/components/file-browser/InviteLinkTab.tsxapps/web/src/components/file-browser/ShareDialog.tsxapps/web/src/styles/share-dialog.csspackages/api-client/openapi.jsontests/e2e/page-objects/dialogs/invite-link-tab.page.tstests/e2e/tests/invite-link-workflow.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/api/src/shares/dto/invite-response.dto.ts
- apps/web/src/styles/share-dialog.css
- tests/e2e/page-objects/dialogs/invite-link-tab.page.ts
…agger - Add @apiresponse(404) to getInviteStatus swagger metadata - Add pre-transaction expiry/status check in claimInvite so expired and non-active invites return 404 instead of leaking to atomic UPDATE (409) which would signal token existence - Clear stale invite list on refetch start and on error in InviteLinkTab - Add unit tests for expired/claimed/revoked pre-transaction checks - Regenerate API client Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 4b637e3f971d
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/api/src/shares/invites.controller.ts`:
- Around line 52-58: getInviteStatus currently throws NotFound for any
non‑active result and always returns "active", which contradicts
InviteStatusResponseDto; change getInviteStatus to only throw NotFound when
sharesService.getInviteStatus(token) returns null/undefined (invalid token) and
otherwise return the actual result.status (e.g.,
'active'|'claimed'|'revoked'|'expired') so the response matches
InviteStatusResponseDto; update the return to use the value from
sharesService.getInviteStatus(token) and ensure the controller signature and any
docs/DTOs reflect all allowed statuses.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/api/src/shares/invites.controller.tsapps/api/src/shares/shares.service.spec.tsapps/api/src/shares/shares.service.tsapps/web/src/api/invites/invites.tsapps/web/src/components/file-browser/InviteLinkTab.tsxpackages/api-client/openapi.json
Reviews all 22 implementation files against the pre-implementation architectural review. All HIGH findings addressed. Two MEDIUM findings (referrer policy, swagger enum) and four LOW findings identified. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 9933309a5cd6
… narrowing Address findings from post-implementation security review: - M-01: Add `<meta name="referrer" content="no-referrer">` defense-in-depth for invite URL fragments - M-02: Narrow swagger enum to `['active']` since controller collapses all non-active statuses to 404 (oracle prevention) - L-01: Narrow client-side invite status types to match actual API contract (`'active' | 'expired'`), remove unreachable `'revoked'` from ErrorReason - L-02: Add ParseTokenPipe (base64url regex, 16-44 char) to all invite endpoints for input validation at API boundary Tests added: - 15 unit tests for ParseTokenPipe (valid/invalid/edge cases) - Concurrent double-claim race condition test - Oracle prevention test (expired → 404, not 409) - ParseTokenPipe integration in invites.controller.spec Regenerated API client to reflect swagger changes. Updated security review report with fix status. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: b4dfd49ac3d6
There was a problem hiding this comment.
🧹 Nitpick comments (6)
apps/web/index.html (1)
7-7: Good privacy hardening for invite-link token leakage — consider also setting the HTTP response header.The
no-referrermeta tag is the right call for Phase 15: invite tokens embedded in the URL (e.g./invite/TOKEN) would otherwise be forwarded to any external site the user navigates to via theRefererheader. Suppressing it entirely is a reasonable, deliberate choice for a security-focused app.One gap to be aware of: the meta referrer policy only governs requests initiated by the HTML document (fetch, XHR, navigations, subresource loads). It does not affect HTTP response headers that the server itself emits. Pairing this with a
Referrer-Policy: no-referrerHTTP response header — set in the Vite dev-server config — ensures the policy is enforced universally and takes precedence per the spec.🛡️ Example: add to vite.config.ts server.headers
server: { port: 5173, headers: { 'Cross-Origin-Opener-Policy': 'same-origin-allow-popups', + 'Referrer-Policy': 'no-referrer', }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/index.html` at line 7, Add the HTTP response header "Referrer-Policy: no-referrer" in the dev server config so the referrer policy is enforced for server responses as well as the HTML meta tag; update the Vite dev-server configuration (the server.headers config) to emit Referrer-Policy: no-referrer, ensuring the server-side header matches the existing <meta name="referrer" content="no-referrer" /> policy.apps/api/src/shares/shares.service.spec.ts (1)
1090-1098: Prefer.rejectsmatcher overtry/catch+fail().
fail()was deprecated in Jest 27 and removed in some configurations — if it's not in scope, the test silently passes without reaching the assertion.♻️ Idiomatic rewrite
- try { - await service.claimInvite('claim-token', recipientId, claimDto); - fail('Should have thrown'); - } catch (err) { - expect(err).toBeInstanceOf(NotFoundException); - expect(err).not.toBeInstanceOf(ConflictException); - } + const err = await service + .claimInvite('claim-token', recipientId, claimDto) + .catch((e) => e); + expect(err).toBeInstanceOf(NotFoundException); + expect(err).not.toBeInstanceOf(ConflictException);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/shares.service.spec.ts` around lines 1090 - 1098, The test uses a try/catch with fail() to assert service.claimInvite('claim-token', recipientId, claimDto) throws, but fail() is deprecated; replace the try/catch with Jest's promise assertion style: use await expect(service.claimInvite('claim-token', recipientId, claimDto)).rejects.toBeInstanceOf(NotFoundException) and also assert it is not an instance of ConflictException via rejects.not.toBeInstanceOf(ConflictException) (or split into two awaits) so the test fails correctly without using fail(); update the test around the claimInvite invocation and remove the try/catch/fail block.apps/api/src/shares/invites.controller.ts (1)
85-96: Return type could use the existingInviteDataResponseDtoinstead of the inline shape.
InviteDataResponseDtoalready defines this exact shape (seeapps/api/src/shares/dto/invite-response.dto.ts:50-81). The inline type duplicates it and can drift if the DTO changes.♻️ Proposed refactor
- async getInviteData(`@Param`('token', ParseTokenPipe) token: string): Promise<{ - status: string; - encryptedKey: string; - encryptedChildKeys: Array<{ - keyType: 'file' | 'folder'; - itemId: string; - encryptedKey: string; - }> | null; - itemType: string; - ipnsName: string; - itemName: string; - }> { + async getInviteData(`@Param`('token', ParseTokenPipe) token: string): Promise<InviteDataResponseDto> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/invites.controller.ts` around lines 85 - 96, The inline return type in getInviteData duplicates an existing DTO; replace the inline shape with the InviteDataResponseDto type: import InviteDataResponseDto and change the method signature from Promise<{...}> to Promise<InviteDataResponseDto>, ensuring any referenced nested types match the DTO and removing the duplicated inline type definition in the getInviteData declaration.apps/api/src/shares/invites.controller.spec.ts (1)
22-22: Replaceas anycasts with typed alternatives — flagged by both Lint and Verify API Spec checks.Three occurrences:
sharer: {} as any(line 28), andmockReq as any(lines 159 & 172). The request mock already has the exact shape ofRequestWithUser; casting it properly removes the lint warnings.♻️ Proposed fix
+import { RequestWithUser } from '../common/types'; ... - const mockReq: { user: { id: string } } = { user: { id: userId } }; + const mockReq = { user: { id: userId } } as unknown as RequestWithUser; ... const mockInvite: ShareInvite = { ... - sharer: {} as any, + sharer: {} as unknown as User, ... }; ... - const result = await controller.claimInvite(mockReq as any, testToken, dto); + const result = await controller.claimInvite(mockReq, testToken, dto); ... - await expect(controller.claimInvite(mockReq as any, testToken, dto)).rejects.toThrow( + await expect(controller.claimInvite(mockReq, testToken, dto)).rejects.toThrow(Also applies to: 28-28, 159-159, 172-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/invites.controller.spec.ts` at line 22, The test uses unsafe "as any" casts for the request and sharer objects; remove those by typing mockReq as the RequestWithUser type (use the existing RequestWithUser interface) instead of "mockReq as any", and type the sharer stub (used where you currently have "sharer: {} as any") with a minimal/partial user type such as Partial<User> or the actual Sharer/ShareUser type your code expects; update the two other casts (the uses of mockReq at assertions/setup) to use the strongly typed mockReq variable rather than casting, ensuring all three occurrences (mockReq declaration and the two mockReq usages, plus sharer) are replaced with proper typed variables.apps/web/src/services/invite.service.ts (2)
139-157: Silent fallback when IPNS resolution fails — recipients get folder key without child key access.If
resolveIpnsRecordreturnsnull,encryptedChildKeysstaysundefinedand the invite is created with no child keys. The recipient can claim the invite and get the folder key, but cannot decrypt any files inside the folder.Consider surfacing this as an error to the sharer rather than silently creating a degraded invite:
const resolved = await resolveIpnsRecord(folderEntry.ipnsName); - if (resolved) { - const encryptedBytes = ... - ... - encryptedChildKeys = await collectChildKeys(...); - } + if (!resolved) { + throw new Error('Failed to resolve folder metadata — cannot create invite without child keys'); + } + const encryptedBytes = await fetchFromIpfs(resolved.cid); + ... + encryptedChildKeys = await collectChildKeys(...);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/services/invite.service.ts` around lines 139 - 157, The code currently silently proceeds when resolveIpnsRecord(folderEntry.ipnsName) returns null leaving encryptedChildKeys undefined so recipients get the folder key but no child keys; update the invite creation flow to detect a failed IPNS resolution (resolveIpnsRecord returning null/undefined) and surface an error instead of continuing — e.g., throw or return an explicit error from the function so the sharer is notified; reference resolveIpnsRecord, encryptedChildKeys, collectChildKeys, decryptFolderMetadata and itemFolderKey to locate where to add the check before decryptFolderMetadata/collectChildKeys are invoked and ensure the finally block still zeroes itemFolderKey.
104-110:parentFolderIdis defined in the params type but never used.The parameter is required by callers but has no effect inside the function. Either remove it if it's truly unnecessary, or document why it's reserved for future use.
♻️ Proposed fix
export async function createInviteLink(params: { item: FolderChild; folderKey: Uint8Array; ipnsName: string; - parentFolderId: string; }): Promise<{ url: string; token: string }> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/services/invite.service.ts` around lines 104 - 110, The createInviteLink function's params type includes parentFolderId but the value is unused; remove parentFolderId from the params object type and from the function signature (params: { item: FolderChild; folderKey: Uint8Array; ipnsName: string }) and update all callers to stop passing parentFolderId, or if you intend to reserve it, explicitly mark it as reserved by documenting it in JSDoc and renaming to _parentFolderId to avoid linter/unused warnings; reference: createInviteLink and the parentFolderId param name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/api/src/shares/invites.controller.spec.ts`:
- Line 22: The test uses unsafe "as any" casts for the request and sharer
objects; remove those by typing mockReq as the RequestWithUser type (use the
existing RequestWithUser interface) instead of "mockReq as any", and type the
sharer stub (used where you currently have "sharer: {} as any") with a
minimal/partial user type such as Partial<User> or the actual Sharer/ShareUser
type your code expects; update the two other casts (the uses of mockReq at
assertions/setup) to use the strongly typed mockReq variable rather than
casting, ensuring all three occurrences (mockReq declaration and the two mockReq
usages, plus sharer) are replaced with proper typed variables.
In `@apps/api/src/shares/invites.controller.ts`:
- Around line 85-96: The inline return type in getInviteData duplicates an
existing DTO; replace the inline shape with the InviteDataResponseDto type:
import InviteDataResponseDto and change the method signature from Promise<{...}>
to Promise<InviteDataResponseDto>, ensuring any referenced nested types match
the DTO and removing the duplicated inline type definition in the getInviteData
declaration.
In `@apps/api/src/shares/shares.service.spec.ts`:
- Around line 1090-1098: The test uses a try/catch with fail() to assert
service.claimInvite('claim-token', recipientId, claimDto) throws, but fail() is
deprecated; replace the try/catch with Jest's promise assertion style: use await
expect(service.claimInvite('claim-token', recipientId,
claimDto)).rejects.toBeInstanceOf(NotFoundException) and also assert it is not
an instance of ConflictException via
rejects.not.toBeInstanceOf(ConflictException) (or split into two awaits) so the
test fails correctly without using fail(); update the test around the
claimInvite invocation and remove the try/catch/fail block.
In `@apps/web/index.html`:
- Line 7: Add the HTTP response header "Referrer-Policy: no-referrer" in the dev
server config so the referrer policy is enforced for server responses as well as
the HTML meta tag; update the Vite dev-server configuration (the server.headers
config) to emit Referrer-Policy: no-referrer, ensuring the server-side header
matches the existing <meta name="referrer" content="no-referrer" /> policy.
In `@apps/web/src/services/invite.service.ts`:
- Around line 139-157: The code currently silently proceeds when
resolveIpnsRecord(folderEntry.ipnsName) returns null leaving encryptedChildKeys
undefined so recipients get the folder key but no child keys; update the invite
creation flow to detect a failed IPNS resolution (resolveIpnsRecord returning
null/undefined) and surface an error instead of continuing — e.g., throw or
return an explicit error from the function so the sharer is notified; reference
resolveIpnsRecord, encryptedChildKeys, collectChildKeys, decryptFolderMetadata
and itemFolderKey to locate where to add the check before
decryptFolderMetadata/collectChildKeys are invoked and ensure the finally block
still zeroes itemFolderKey.
- Around line 104-110: The createInviteLink function's params type includes
parentFolderId but the value is unused; remove parentFolderId from the params
object type and from the function signature (params: { item: FolderChild;
folderKey: Uint8Array; ipnsName: string }) and update all callers to stop
passing parentFolderId, or if you intend to reserve it, explicitly mark it as
reserved by documenting it in JSDoc and renaming to _parentFolderId to avoid
linter/unused warnings; reference: createInviteLink and the parentFolderId param
name.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.planning/security/REVIEW-2026-02-24-phase15-implementation.mdapps/api/src/common/pipes/parse-token.pipe.spec.tsapps/api/src/common/pipes/parse-token.pipe.tsapps/api/src/shares/dto/invite-response.dto.tsapps/api/src/shares/invites.controller.spec.tsapps/api/src/shares/invites.controller.tsapps/api/src/shares/shares.service.spec.tsapps/web/index.htmlapps/web/src/api/models/inviteStatusResponseDto.tsapps/web/src/api/models/inviteStatusResponseDtoStatus.tsapps/web/src/routes/InvitePage.tsxapps/web/src/services/invite.service.tspackages/api-client/openapi.json
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/api/models/inviteStatusResponseDto.ts
- apps/api/src/shares/dto/invite-response.dto.ts
This was referenced Feb 24, 2026
4 tasks
Closed
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
User Interface