Repository navigation
feat(invites): invite non-existent users by email - #1229
Closed
2witstudios wants to merge 31 commits into
Closed
2witstudios wants to merge 31 commits into
2witstudios wants to merge 31 commits into
Conversation
Drive invite route now accepts an email payload variant alongside today's userId variant. New emails route through createMagicLinkToken to bind a temp user, insert a pending drive_members row (acceptedAt null), and send a DriveInvitationEmail with a magic-link verify URL carrying inviteDriveId. Existing emails fall through to today's auto-accept path; re-invites of pending emails return 409 with existingMemberId; email is normalized to lowercase trimmed at the boundary.
Magic-link verify route accepts pending invitations after session creation: looks up acceptedAt-null rows for the authenticated user, atomically flips each via a conditional UPDATE, broadcasts member_added per accepted row, and overrides the post-verify redirect to /dashboard/{inviteDriveId} when a pending row matches. Independent discovery is automatic — pending rows are accepted on any sign-in, not just via the invitation link.
Adds tasks/drive-invites-by-email.md tracking the 5-task epic.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When the search query is a valid email, returns zero results, and an onInviteEmail callback is provided, UserSearch now renders an "Invite [email] to PageSpace" button instead of today's generic empty-state hint. Email is lowercased and trimmed before bubbling up.
Invite page tracks pendingInviteEmail alongside selectedUser, gates the role and permissions cards on either, and branches the invite POST payload between { userId } and { email }. The response's kind field drives a distinct "Invitation sent" toast for new-user invites.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
DriveMembers splits the list into accepted and pending groups, rendering pending invitations under a "Pending invitations (N)" subheader. Header count reflects accepted only; pending rows show an amber Pending badge, hide the Member Settings link, and expose a Revoke action gated to owners and admins. Revoke uses the correct userId param and applies optimistically via setMembers filter rather than a full refetch. Subscribes to drive:member_added and drive:member_removed via useSocket so previously pending rows promote to the accepted section in real time when the invitee signs in. Events for other drives are ignored. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
New POST /api/drives/[driveId]/members/[userId]/resend route gated to owners and admins. Reissues a magic-link token, resends the DriveInvitationEmail, and bumps invitedAt on the pending member row. Distributed rate limit caps the resend at 3 attempts per (drive, userId) per 24 hours and returns 429 with Retry-After. Already-accepted rows return 400; missing target emails return 404. MemberRow renders a Send icon button for pending rows when an onResend handler is provided, gated to owners and admins. DriveMembers wires the handler to the new route and toasts on success or failure. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements invite-by-email with pending drive_members (acceptedAt = null), extended magic-link TTLs, atomic acceptance on verify with per-invite websocket broadcasts, a resend endpoint with rate-limiting, frontend invite-by-email UI and pending-invitations UI, recipient-scoped websocket fan-out, and codewide tightening to treat only accepted memberships as active members/recipients. Drive Invites by Email
Sequence Diagrams sequenceDiagram
actor Owner as Owner/Admin
participant Web as Web App
participant API as Backend API
participant DB as Database
participant Email as Email Service
participant Socket as WebSocket
Owner->>Web: Submit invite email
Web->>API: POST /api/drives/[id]/members/invite { email }
API->>DB: findUserIdByEmail / findActivePendingMemberByEmail
DB-->>API: no user / no active pending
API->>API: rate-limit checks
API->>API: create magic-link token (expiryMinutes or invitation TTL)
API->>DB: create drive_member (acceptedAt = null)
API->>Email: sendPendingDriveInvitationEmail(magicLinkUrl)
Email-->>User: email with magic link
sequenceDiagram
actor User as Invitee
participant Web as Web App (browser)
participant API as Backend API
participant DB as Database
participant Socket as WebSocket
User->>Web: Click magic link (token X, inviteDriveId=Y)
Web->>API: GET /api/auth/magic-link/verify?token=X&inviteDriveId=Y
API->>DB: verify token, findPendingMembersForUser
DB-->>API: pending rows
API->>DB: acceptPendingMember (atomic update per row)
DB-->>API: update OK
API->>Socket: broadcastDriveMemberEventToRecipients(payload, recipients)
Socket-->>Web: drive:member_added events
Web->>Web: refetch members -> pending -> accepted
API-->>User: redirect to /dashboard/Y (if matched)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b687dff9cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Filter `acceptedAt IS NOT NULL` from `checkDriveAccess`, `isMemberOfDrive`, `getDriveMemberUserIds`, and `getDriveRecipientUserIds` so pending invitees are NOT treated as authorized drive members. Without this, a pending temp user that ever obtains a session (now or via future code paths) would inherit drive access without ever clicking the invitation link. Apply per-email rate limit to the email-payload branch of the invite route using the same MAGIC_LINK config as the magic-link send endpoint. Closes the email-bombing primitive where an authenticated owner could repeatedly invite an external email and trigger unlimited emails. Add an optional `expiryMinutes` parameter to `createMagicLinkToken` and a new `INVITATION_LINK_EXPIRY_MINUTES` (7 days) constant. Invitation and resend flows pass the longer TTL so links don't go stale before invitees see them. Read inviter display name (users.name) for the email subject and template instead of leaking the inviter's email address. Use the existing `data.share` audit event type for resend rather than `authz.permission.granted`, which mis-tagged the operation in audit-trail queries. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Hoist findActivePendingMemberByEmail above the existing-user check so the 409-on-pending path also fires when the email already maps to a temp user from a prior invitation. Without this, a second invite for an unaccepted email silently routed through the existing-user branch, updating role/permissions on the pending row and skipping the invitation email — flagged by Codex as P1. Sanitize the error message logged when sendPendingDriveInvitationEmail fails to remove control characters and cap length, addressing CodeQL js/log-injection alert #177 in notification-email-service.ts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/app/api/drives/[driveId]/members/invite/route.ts (1)
158-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPending existing memberships are not auto-accepted, but
member_addedis still emitted.At Line 158-163, an existing pending row only gets role updates. Then Line 168-175 broadcasts
member_addedregardless of acceptance state. This can leave users pending while clients receive a “member added” realtime event.Suggested fix (accept before broadcast, and broadcast only when accepted)
- if (!existingMember) { + let membershipAccepted = false; + if (!existingMember) { // Add as drive member with specified role; pending invites leave acceptedAt null const newMember = await driveInviteRepository.createDriveMember({ driveId, userId: invitedUserId, role, customRoleId: customRoleId || null, invitedBy: userId, acceptedAt: inviteKind === 'invited' ? null : new Date(), }); memberId = newMember.id; + membershipAccepted = inviteKind !== 'invited'; } else { // Update role if member exists await driveInviteRepository.updateDriveMemberRole( existingMember.id, role, customRoleId || null ); memberId = existingMember.id; + if (existingMember.acceptedAt === null && inviteKind === 'added') { + membershipAccepted = await driveInviteRepository.acceptPendingMember(existingMember.id); + } else { + membershipAccepted = existingMember.acceptedAt !== null; + } } - // Broadcast member added/updated event to the affected user - await broadcastDriveMemberEvent( - createDriveMemberEventPayload(driveId, invitedUserId, 'member_added', { - role, - driveName: drive.name - }) - ); + if (membershipAccepted) { + await broadcastDriveMemberEvent( + createDriveMemberEventPayload(driveId, invitedUserId, 'member_added', { + role, + driveName: drive.name + }) + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/drives/`[driveId]/members/invite/route.ts around lines 158 - 175, Existing pending memberships are having their role updated via driveInviteRepository.updateDriveMemberRole(existingMember.id, role, customRoleId || null) but the code still emits a "member_added" event via broadcastDriveMemberEvent(createDriveMemberEventPayload(...)) even when the membership remains pending; change the flow so that when existingMember is in a pending state you first accept the membership (call the repository method that marks it accepted or set accepted=true for existingMember and persist that change) before assigning memberId and broadcasting, and only call broadcastDriveMemberEvent/createDriveMemberEventPayload when the membership is accepted; adjust logic around existingMember, updateDriveMemberRole, and the branch that sets memberId to ensure broadcast happens exclusively after acceptance.
🧹 Nitpick comments (3)
apps/web/src/components/members/__tests__/DriveMembers.test.tsx (1)
208-209: ⚡ Quick winAvoid real sleep in the unrelated-drive realtime test.
The hardcoded
setTimeout(50)can make this test flaky/slower. For this branch, a direct “still 1 call” assertion after dispatch is enough and deterministic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/members/__tests__/DriveMembers.test.tsx` around lines 208 - 209, The test in DriveMembers.test.tsx uses a real sleep via await new Promise((r) => setTimeout(r, 50)) which is unnecessary and flaky; remove that sleep and assert immediately after the action dispatch that fetchWithAuth has been called once (keep expect(fetchWithAuth).toHaveBeenCalledTimes(1) directly after the dispatch). If the test previously used a helper like flushPromises or an async helper, prefer that over hard timeouts, but for this unrelated-drive realtime test simply drop the setTimeout and perform the direct assertion.apps/web/src/app/api/drives/[driveId]/members/[userId]/resend/__tests__/route.test.ts (1)
178-217: ⚡ Quick winAdd an explicit test for unverified inviter behavior (403 +
requiresEmailVerification).The route has a dedicated early-return contract for unverified callers; covering it here will protect that security gate from regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/drives/`[driveId]/members/[userId]/resend/__tests__/route.test.ts around lines 178 - 217, Add a test that simulates an authenticated caller who is the drive owner/admin but is unverified and assert the route returns 403 with a JSON body containing requiresEmailVerification: true; to implement it, mock authenticateRequestWithOptions/isAuthError to produce a successful auth, mock driveInviteRepository.findDriveById to return the drive with the caller as owner (or driveInviteRepository.findAdminMembership to return a membership object that indicates unverified—for example an object with requiresEmailVerification: true), then call POST(createResendRequest(...), createContext(...)) and expect response.status toBe(403), expect(await response.json()).toMatchObject({ requiresEmailVerification: true }), and also assert createMagicLinkToken and driveInviteRepository.bumpInvitedAt were not called.apps/web/src/app/api/drives/[driveId]/members/invite/__tests__/route.test.ts (1)
776-808: ⚡ Quick winAdd an assertion that pending invites do not emit
member_added.This case is the right place to lock the pending semantics: after
kind: 'invited', assert realtime broadcast is not fired.Suggested assertion addition
expect(sendPendingDriveInvitationEmail).toHaveBeenCalledTimes(1); + expect(broadcastDriveMemberEvent).not.toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/drives/`[driveId]/members/invite/__tests__/route.test.ts around lines 776 - 808, Add an assertion after the existing checks that ensures no realtime "member_added" broadcast was emitted for pending invites: verify the mocked realtime publisher (e.g., realtimeClient.publish / broadcastService.trigger) was not called with an event name 'member_added' or with payload containing the new pending member; place this assertion in the same test that calls POST(createInviteRequest(...)) so it validates that when driveInviteRepository.findUserIdByEmail resolves to null and driveInviteRepository.createDriveMember creates a pending member, the broadcast/emit function is not invoked for 'member_added'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/api/auth/magic-link/verify/__tests__/route.test.ts`:
- Around line 95-105: The mock for websocket helpers returns a payload with an
event property which diverges from production where payloads use operation;
update the mocked createDriveMemberEventPayload (and any related mock return
shapes) to include operation instead of event and preserve driveId, userId and
spread data so tests mirror the real contract; ensure broadcastDriveMemberEvent
mock remains a resolved fn and adjust expectations in route.test.ts to assert
against operation rather than event.
In `@apps/web/src/app/api/drives/`[driveId]/members/[userId]/resend/route.ts:
- Around line 96-101: The magic link URL in route.ts builds magicLinkUrl by
interpolating tokenResult.data.token and driveId directly into the query string,
which can break if those values contain reserved characters; update the
composition to URL-encode both values (e.g., apply encodeURIComponent to
tokenResult.data.token and driveId) when constructing magicLinkUrl so the token
and inviteDriveId are safely encoded before concatenation (keep appUrl and
inviter logic unchanged).
In `@apps/web/src/app/api/drives/`[driveId]/members/invite/route.ts:
- Around line 87-126: Normalize and validate the incoming email
(normalizedEmail) immediately after assignment and before any DB lookups or
rate-limit/token creation; if the email fails a proper format check (e.g.,
simple regex or shared email validator), return NextResponse.json({ error:
'Invalid email' }, { status: 400 }). Insert this guard in the route handling
flow (in route.ts) right after normalizedEmail is created and before calling
driveInviteRepository.findUserIdByEmail / findActivePendingMemberByEmail and
before checkDistributedRateLimit / createMagicLinkToken so invalid input never
reaches createMagicLinkToken.
In `@apps/web/src/components/members/UserSearch.tsx`:
- Around line 128-153: The empty-state invite CTA uses the live query string
while search results are produced from debouncedQuery, causing a transient
"Invite ..." button; update the conditional and normalization to use
debouncedQuery (and its length check) instead of query inside the block in
UserSearch (where results are rendered), i.e., replace references to query with
debouncedQuery when computing normalized, isEmail, and the length threshold so
the CTA only appears for the actual debounced search term before calling
onInviteEmail.
In `@apps/web/src/lib/repositories/drive-invite-repository.ts`:
- Around line 67-90: Both repository helpers (findUserIdByEmail and
findActivePendingMemberByEmail) compare directly to users.email and must
normalize inputs to avoid mismatches; update both functions to trim and
lowercase the incoming email before building the query (e.g., const normalized =
email.trim().toLowerCase()) and use that normalized value in the where clause so
callers no longer need to pre-normalize; ensure you only change the parameter
usage (not schema) and keep the return behavior the same.
In `@packages/lib/src/services/__tests__/drive-member-service.test.ts`:
- Around line 52-56: The mock implementations in the vi.mock for
'@pagespace/db/operators' use implicitly-typed parameters (e.g., the first param
named "a") which defaults to any; update each mock signature (eq, and,
isNotNull, sql) to use explicit TypeScript types (for example unknown or the
real operator types used in the codebase) so no parameter is implicitly any —
e.g., change eq: vi.fn((a, b) => ...) to eq: vi.fn((a: unknown, b: unknown) =>
...), and: vi.fn((...args: unknown[]) => ...) already typed but ensure
consistency, and isNotNull: vi.fn((a: unknown) => ...) and give sql an explicit
function type as needed; keep return shapes the same.
In `@packages/lib/src/services/notification-email-service.ts`:
- Around line 277-294: The catch block in sendPendingDriveInvitationEmail
swallows sendEmail failures; update sendPendingDriveInvitationEmail (the
function) so failures propagate instead of returning success — either remove the
try/catch or rethrow the caught error after logging (preserve the
processLogger/console.error call but throw error), ensuring callers of
sendPendingDriveInvitationEmail and any invite/resend route handlers can observe
and handle the error; reference sendEmail and DriveInvitationEmail when locating
the call site to change.
---
Outside diff comments:
In `@apps/web/src/app/api/drives/`[driveId]/members/invite/route.ts:
- Around line 158-175: Existing pending memberships are having their role
updated via driveInviteRepository.updateDriveMemberRole(existingMember.id, role,
customRoleId || null) but the code still emits a "member_added" event via
broadcastDriveMemberEvent(createDriveMemberEventPayload(...)) even when the
membership remains pending; change the flow so that when existingMember is in a
pending state you first accept the membership (call the repository method that
marks it accepted or set accepted=true for existingMember and persist that
change) before assigning memberId and broadcasting, and only call
broadcastDriveMemberEvent/createDriveMemberEventPayload when the membership is
accepted; adjust logic around existingMember, updateDriveMemberRole, and the
branch that sets memberId to ensure broadcast happens exclusively after
acceptance.
---
Nitpick comments:
In
`@apps/web/src/app/api/drives/`[driveId]/members/[userId]/resend/__tests__/route.test.ts:
- Around line 178-217: Add a test that simulates an authenticated caller who is
the drive owner/admin but is unverified and assert the route returns 403 with a
JSON body containing requiresEmailVerification: true; to implement it, mock
authenticateRequestWithOptions/isAuthError to produce a successful auth, mock
driveInviteRepository.findDriveById to return the drive with the caller as owner
(or driveInviteRepository.findAdminMembership to return a membership object that
indicates unverified—for example an object with requiresEmailVerification:
true), then call POST(createResendRequest(...), createContext(...)) and expect
response.status toBe(403), expect(await response.json()).toMatchObject({
requiresEmailVerification: true }), and also assert createMagicLinkToken and
driveInviteRepository.bumpInvitedAt were not called.
In
`@apps/web/src/app/api/drives/`[driveId]/members/invite/__tests__/route.test.ts:
- Around line 776-808: Add an assertion after the existing checks that ensures
no realtime "member_added" broadcast was emitted for pending invites: verify the
mocked realtime publisher (e.g., realtimeClient.publish /
broadcastService.trigger) was not called with an event name 'member_added' or
with payload containing the new pending member; place this assertion in the same
test that calls POST(createInviteRequest(...)) so it validates that when
driveInviteRepository.findUserIdByEmail resolves to null and
driveInviteRepository.createDriveMember creates a pending member, the
broadcast/emit function is not invoked for 'member_added'.
In `@apps/web/src/components/members/__tests__/DriveMembers.test.tsx`:
- Around line 208-209: The test in DriveMembers.test.tsx uses a real sleep via
await new Promise((r) => setTimeout(r, 50)) which is unnecessary and flaky;
remove that sleep and assert immediately after the action dispatch that
fetchWithAuth has been called once (keep
expect(fetchWithAuth).toHaveBeenCalledTimes(1) directly after the dispatch). If
the test previously used a helper like flushPromises or an async helper, prefer
that over hard timeouts, but for this unrelated-drive realtime test simply drop
the setTimeout and perform the direct assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dfbb1b87-d74c-4fac-84a7-3eec06b896ec
📒 Files selected for processing (21)
apps/web/src/app/api/auth/magic-link/verify/__tests__/desktop-verify.test.tsapps/web/src/app/api/auth/magic-link/verify/__tests__/route.test.tsapps/web/src/app/api/auth/magic-link/verify/route.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/resend/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/resend/route.tsapps/web/src/app/api/drives/[driveId]/members/invite/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/members/invite/route.tsapps/web/src/app/dashboard/[driveId]/members/invite/page.tsxapps/web/src/components/members/DriveMembers.tsxapps/web/src/components/members/MemberRow.tsxapps/web/src/components/members/UserSearch.tsxapps/web/src/components/members/__tests__/DriveMembers.test.tsxapps/web/src/components/members/__tests__/UserSearch.test.tsxapps/web/src/lib/repositories/drive-invite-repository.tspackages/lib/package.jsonpackages/lib/src/auth/magic-link-service.tspackages/lib/src/services/__tests__/drive-member-service.test.tspackages/lib/src/services/drive-member-service.tspackages/lib/src/services/notification-email-service.tsplan.mdtasks/drive-invites-by-email.md
First pass missed several authorization-deciding membership queries that don't filter on acceptedAt. Second self-review found pending rows still leaked through getDriveIdsForUser, getDriveAccess (drive-service), getDriveAccessWithDrive, getUserDriveSummaries, checkDriveAccessForRoles, and the GET /api/drives/[driveId]/assignees route. All four now require acceptedAt IS NOT NULL so a pending temp user that ever obtains a session truly has no drive access. Updated affected test mocks (drive-service, drive-role-service, assignees route) to expose isNotNull on the @pagespace/db/operators mock surface and acceptedAt on the driveMembers schema mock. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cover the remaining authorization-deciding driveMembers queries that the previous pass missed: permission-mutations.ts admin-share check, permissions-tree route admin gate, [driveId]/pages route admin gate, [driveId]/trash route admin gate, plus the user-drive-list reads in pulse and activity-summary routes. All now require acceptedAt IS NOT NULL. Updated affected test mocks (trash route, activity summary route) to expose isNotNull on the @pagespace/db/operators mock surface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Propagate sendPendingDriveInvitationEmail failures instead of swallowing them — invitation rows that fail email send now surface a 500 to the inviter who can see the pending row in the UI and retry via Resend. Validate the email payload through Zod's email() before passing to createMagicLinkToken so an obviously malformed input gets a 400 from the route instead of bubbling through as a 500. URL-encode token + driveId in invitation magic-link URLs (defense in depth — both values are CUID2/base64url today, so this is preventive). Use debouncedQuery for the UserSearch empty-state CTA gate so the affordance only renders after the search has actually returned no-results. Normalize email inside drive-invite-repository's findUserIdByEmail and findActivePendingMemberByEmail helpers so callers cannot bypass case-folding by mistake. Align createDriveMemberEventPayload mocks across verify, desktop-verify, and invite tests with the production payload contract (operation, not event). Type the new isNotNull mock parameter as unknown across drive-member-service, drive-service, drive-role-service, and permission-mutations-unit tests to satisfy the no-implicit-any rule. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit out-of-diff finding: the invite route emitted member_added even when the resulting member row was pending (acceptedAt null). UI clients listening for member_added would surface a join event for somebody who hasn't actually joined yet. Track membershipAccepted explicitly and broadcast only when truly accepted. If the legacy userId path lands on a pending row from an earlier email invite (re-invite of the same temp user via the old payload), accept the pending row in-place so the operation truly completes. The email path can't reach this branch — 409 fires earlier. Add a test asserting pending-invite path does NOT broadcast member_added. Add a test for unverified-inviter behavior on the resend route. Drop a flaky 50ms sleep in the unrelated-drive realtime test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
Thanks @coderabbitai. Addressed the outside-diff finding and the three nitpicks in f8a6850:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Everything looks solid. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Add auditRequest calls on the resend route's 403 (not owner/admin) and 429 (rate-limited) denial paths so security-relevant denials no longer bypass the audit trail. Strip CR/LF and other control characters from inviterName and driveName before interpolating into the email subject so a user-controlled field cannot inject additional email headers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Without this, a body that omits the permissions field hit permissions.map(...) on undefined, which was caught by the outer catch and returned a generic 500. Defensive default + Array.isArray guard makes this a friendlier no-op (zero permissions granted) rather than a crash. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Hoist 'added' | 'invited' from inline literals into an exported InviteKind type alias and an InviteMemberResponse shape, then thread the alias into the invite page so the client and server share a single source of truth for the response contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a4efdd05e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex P2: broadcastDriveMemberEvent only routed to the affected user's user:{userId}:drives channel, so admins watching the members page never received the event when somebody else accepted an invitation. The realtime listener in DriveMembers couldn't fire for them, so pending rows wouldn't promote to the accepted section without a manual refresh.
Add broadcastDriveMemberEventToRecipients in socket-utils that fans out the same payload to every supplied recipient channel (deduped against the affected user). Wire the verify route's pending-acceptance loop and the invite route's auto-accept path through getDriveRecipientUserIds + the new helper so every drive admin's session receives the event.
Test mocks updated to expect the new helper and stub getDriveRecipientUserIds.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb338d9336
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/app/api/activity/summary/__tests__/route.test.ts (1)
45-53:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the new
acceptedAtpredicate observable in this test.Right now this only mocks
isNotNull; it doesn't verify the route is filtering ondriveMembers.acceptedAt. Since the schema mock omitsacceptedAt, the suite still passes even if that predicate is removed or misspelled.Suggested tightening
vi.mock('@pagespace/db/schema/members', () => ({ - driveMembers: { driveId: 'driveId', userId: 'userId' }, + driveMembers: { driveId: 'driveId', userId: 'userId', acceptedAt: 'acceptedAt' }, }));import { authenticateRequestWithOptions, isAuthError } from '@/lib/auth'; +import { isNotNull } from '@pagespace/db/operators'; ... it('logs audit event on successful summary fetch', async () => { const request = new Request('https://example.com/api/activity/summary'); await GET(request); + expect(vi.mocked(isNotNull)).toHaveBeenCalledWith('acceptedAt'); expect(mockAuditRequest).toHaveBeenCalledWith(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/activity/summary/__tests__/route.test.ts` around lines 45 - 53, The test's schema mock for driveMembers is missing the acceptedAt field so the route's predicate on driveMembers.acceptedAt isn't asserted; update the mock in route.test.ts to include acceptedAt: 'acceptedAt' in the vi.mock('@pagespace/db/schema/members', ...) stub and make the isNotNull spy (vi.fn()) observable for that field by asserting that isNotNull was called with the expected identifier (driveMembers.acceptedAt) when the route is exercised, ensuring the predicate is actually used.packages/lib/src/permissions/__tests__/permission-mutations-unit.test.ts (1)
19-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winModel
driveMembers.acceptedAtin this mock as well.
permission-mutations.tsnow builds the admin check asisNotNull(driveMembers.acceptedAt), but this test double still exposes noacceptedAtfield. WithisNotNullstubbed to a constant, the suite won't catch a regression where the query references the wrong column or never wires the field in at all.🧪 Suggested mock adjustment
vi.mock('@pagespace/db/schema/members', () => ({ - driveMembers: { driveId: 'driveId', userId: 'userId', role: 'role', id: 'id' }, + driveMembers: { driveId: 'driveId', userId: 'userId', role: 'role', acceptedAt: 'acceptedAt', id: 'id' }, pagePermissions: { pageId: 'pageId', userId: 'userId', canView: 'canView', canEdit: 'canEdit', canShare: 'canShare', canDelete: 'canDelete', id: 'id', grantedBy: 'grantedBy', }, }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/src/permissions/__tests__/permission-mutations-unit.test.ts` around lines 19 - 33, The test mock for the members schema is missing the acceptedAt field used by permission-mutations.ts; add acceptedAt to the driveMembers mock (e.g., driveMembers: { driveId, userId, role, id, acceptedAt }) so calls like isNotNull(driveMembers.acceptedAt) resolve to a real property in the test double and the suite will catch regressions where the query references or omits that column; keep the existing vi.mock for '@pagespace/db/operators' but ensure tests assert that isNotNull is invoked with driveMembers.acceptedAt where applicable.
🧹 Nitpick comments (3)
apps/web/src/app/api/drives/[driveId]/assignees/__tests__/route.test.ts (1)
27-31: ⚡ Quick winMirror
acceptedAtin the schema mock so this hardening is actually tested.The route now calls
isNotNull(driveMembers.acceptedAt), but this mock still omitsacceptedAt, so the suite can keep passing withisNotNull(undefined). Add the column here and cover at least one pending-row case; otherwise this change doesn't really protect the accepted-members-only filter.🧪 Minimal mock fix
vi.mock('@pagespace/db/schema/members', () => ({ driveMembers: { userId: 'col_dm_userId', role: 'col_dm_role', driveId: 'col_dm_driveId', + acceptedAt: 'col_dm_acceptedAt', }, userProfiles: { displayName: 'col_up_displayName', avatarUrl: 'col_up_avatarUrl', userId: 'col_up_userId',Also applies to: 51-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/drives/`[driveId]/assignees/__tests__/route.test.ts around lines 27 - 31, The test's operators mock omits the driveMembers.acceptedAt column so isNotNull(driveMembers.acceptedAt) isn't meaningfully exercised; update the mock of '@pagespace/db/operators' used in route.test.ts to include an acceptedAt field in the mocked schema/object and add at least one test row representing a pending member (acceptedAt: null/undefined) plus one accepted member (acceptedAt: non-null) so the route's isNotNull filter (and the isNotNull helper) is actually asserted; target symbols: the mock declaration for '@pagespace/db/operators', isNotNull, and any test fixtures/rows used in the suite.apps/web/src/app/api/drives/[driveId]/permissions-tree/route.ts (1)
37-66: 🏗️ Heavy liftUse the shared drive-access helper instead of open-coding owner/admin checks here.
This route is still duplicating membership semantics in-line, which is exactly why
acceptedAtnow has to be patched route-by-route. Moving this block onto the centralized permission helper would keep pending-invite behavior consistent with the rest of the app and reduce future drift.As per coding guidelines "Use centralized permission logic via
getUserAccessLevelandcanUserEditPagefrom@pagespace/lib/permissions/permissions".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/drives/`[driveId]/permissions-tree/route.ts around lines 37 - 66, Replace the inline owner/admin membership checks in the route handler with the centralized permission helpers: call getUserAccessLevel from `@pagespace/lib/permissions/permissions` (passing the current user and driveId or drive record) and/or canUserEditPage to determine if the user has OWNER or ADMIN access instead of manually querying driveMembers/acceptedAt; remove the ad-hoc driveMembers query and the isOwner/isAdmin logic, and return the same 403/404 responses when the helper indicates insufficient access (use the existing drive lookup for the 404 check, then consult getUserAccessLevel/canUserEditPage to decide permission).apps/web/src/app/api/drives/[driveId]/trash/__tests__/route.test.ts (1)
24-29: ⚡ Quick winThis contract test still won't catch pending-admin access regressions.
The route now depends on
driveMembers.acceptedAt, but the schema mock still omits that column and the admin fixture has no acceptance state. Please addacceptedAtto the mock and cover theacceptedAt: nulldenial case so this suite actually protects the new authorization rule.🧪 Minimal mock fix
vi.mock('@pagespace/db/schema/members', () => ({ - driveMembers: { driveId: 'driveMembers.driveId', userId: 'driveMembers.userId', role: 'driveMembers.role' }, + driveMembers: { + driveId: 'driveMembers.driveId', + userId: 'driveMembers.userId', + role: 'driveMembers.role', + acceptedAt: 'driveMembers.acceptedAt', + }, }));Also applies to: 34-36, 186-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/drives/`[driveId]/trash/__tests__/route.test.ts around lines 24 - 29, The test suite's mock and fixtures are missing driveMembers.acceptedAt so the new authorization (pending-admin denial) isn't exercised; update the vi.mock in route.test.ts for '@pagespace/db/operators' and any schema/fixture objects used by the tests to include an acceptedAt field (e.g., add acceptedAt to the mocked drive member rows and fixture labeled "admin"), and add a new assertion/test case that simulates acceptedAt: null for the admin member to assert access is denied (target the test helper/fixture references and the authorization check in the route test to ensure the pending-admin path is covered).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/api/auth/magic-link/verify/__tests__/route.test.ts`:
- Around line 125-128: createVerifyRequest currently uses truthy checks so
calling createVerifyRequest('') omits the token param; change the conditionals
to test for undefined explicitly (e.g., token !== undefined and inviteDriveId
!== undefined) so empty strings are preserved and the helper can produce ?token=
for the "empty token" test case; update the conditional logic inside
createVerifyRequest accordingly.
In `@apps/web/src/app/api/drives/`[driveId]/members/invite/route.ts:
- Around line 193-215: The code currently conflates prior-accepted members with
newly-accepted ones by setting membershipAccepted = true for already-accepted
existingMember, causing createDriveMemberEventPayload(..., 'member_added', ...)
and broadcastDriveMemberEventToRecipients to run incorrectly; change the logic
in the existing-member branch (where acceptPendingMember is called) to compute
two booleans (e.g., becameAccepted for the acceptance transition result and
isAcceptedNow for current accepted state) instead of a single
membershipAccepted, then only call getDriveRecipientUserIds,
createDriveMemberEventPayload with 'member_added', and
broadcastDriveMemberEventToRecipients when becameAccepted is true; ensure
updates that only change role for already-accepted members follow the
role-changed path (emit role change events instead) and do not trigger the
invite/member_added side effects.
In `@apps/web/src/app/api/pulse/route.ts`:
- Around line 95-102: The pulse generation paths
(apps/web/src/app/api/pulse/generate/route.ts and .../cron/route.ts) build
driveId sets from drive_members without filtering out pending invites; mirror
the accepted-membership filter used in the GET /api/pulse path by applying the
same db.select(...) .from(driveMembers).where(and(eq(driveMembers.userId,
userId), isNotNull(driveMembers.acceptedAt))) (or equivalent predicate) when
constructing driveIds in those files so only drives with acceptedAt not null for
the current user are included (update the driveIds query that references
driveMembers to include eq(driveMembers.userId, userId) and
isNotNull(driveMembers.acceptedAt)).
In `@apps/web/src/lib/websocket/socket-utils.ts`:
- Around line 276-299: The fan-out loop only treats rejected fetch promises as
failures, but fetch resolves for HTTP errors; update the uniqueRecipients.map in
the broadcast block so each fetch's response is checked (e.g., await response
and if !response.ok throw an Error that includes status and body/text) so
non-2xx responses become rejections and show up in results; keep using
createSignedBroadcastHeaders and the same AbortSignal timeout, and leave the
realtimeLogger.warn/ maskIdentifier payload reporting as-is so failedCount and
totalCount accurately reflect HTTP failures too.
In `@packages/lib/src/services/drive-service.ts`:
- Around line 80-85: The added filter on driveMembers.acceptedAt in
listAccessibleDrives hides OWNER rows created by updateDriveLastAccessed (which
inserts/updates owner rows without setting acceptedAt), causing owners to remain
invisible and lastAccessedAt to stay null; update the query in
listAccessibleDrives (and similar logic around the 399-432 region) to include
OWNER role rows regardless of acceptedAt (e.g., include rows where
driveMembers.role == 'OWNER' OR acceptedAt IS NOT NULL) so owner memberships
inserted by updateDriveLastAccessed are returned while still enforcing
acceptedAt for non-owner members.
---
Outside diff comments:
In `@apps/web/src/app/api/activity/summary/__tests__/route.test.ts`:
- Around line 45-53: The test's schema mock for driveMembers is missing the
acceptedAt field so the route's predicate on driveMembers.acceptedAt isn't
asserted; update the mock in route.test.ts to include acceptedAt: 'acceptedAt'
in the vi.mock('@pagespace/db/schema/members', ...) stub and make the isNotNull
spy (vi.fn()) observable for that field by asserting that isNotNull was called
with the expected identifier (driveMembers.acceptedAt) when the route is
exercised, ensuring the predicate is actually used.
In `@packages/lib/src/permissions/__tests__/permission-mutations-unit.test.ts`:
- Around line 19-33: The test mock for the members schema is missing the
acceptedAt field used by permission-mutations.ts; add acceptedAt to the
driveMembers mock (e.g., driveMembers: { driveId, userId, role, id, acceptedAt
}) so calls like isNotNull(driveMembers.acceptedAt) resolve to a real property
in the test double and the suite will catch regressions where the query
references or omits that column; keep the existing vi.mock for
'@pagespace/db/operators' but ensure tests assert that isNotNull is invoked with
driveMembers.acceptedAt where applicable.
---
Nitpick comments:
In `@apps/web/src/app/api/drives/`[driveId]/assignees/__tests__/route.test.ts:
- Around line 27-31: The test's operators mock omits the driveMembers.acceptedAt
column so isNotNull(driveMembers.acceptedAt) isn't meaningfully exercised;
update the mock of '@pagespace/db/operators' used in route.test.ts to include an
acceptedAt field in the mocked schema/object and add at least one test row
representing a pending member (acceptedAt: null/undefined) plus one accepted
member (acceptedAt: non-null) so the route's isNotNull filter (and the isNotNull
helper) is actually asserted; target symbols: the mock declaration for
'@pagespace/db/operators', isNotNull, and any test fixtures/rows used in the
suite.
In `@apps/web/src/app/api/drives/`[driveId]/permissions-tree/route.ts:
- Around line 37-66: Replace the inline owner/admin membership checks in the
route handler with the centralized permission helpers: call getUserAccessLevel
from `@pagespace/lib/permissions/permissions` (passing the current user and
driveId or drive record) and/or canUserEditPage to determine if the user has
OWNER or ADMIN access instead of manually querying driveMembers/acceptedAt;
remove the ad-hoc driveMembers query and the isOwner/isAdmin logic, and return
the same 403/404 responses when the helper indicates insufficient access (use
the existing drive lookup for the 404 check, then consult
getUserAccessLevel/canUserEditPage to decide permission).
In `@apps/web/src/app/api/drives/`[driveId]/trash/__tests__/route.test.ts:
- Around line 24-29: The test suite's mock and fixtures are missing
driveMembers.acceptedAt so the new authorization (pending-admin denial) isn't
exercised; update the vi.mock in route.test.ts for '@pagespace/db/operators' and
any schema/fixture objects used by the tests to include an acceptedAt field
(e.g., add acceptedAt to the mocked drive member rows and fixture labeled
"admin"), and add a new assertion/test case that simulates acceptedAt: null for
the admin member to assert access is denied (target the test helper/fixture
references and the authorization check in the route test to ensure the
pending-admin path is covered).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7d9103f-ba01-45df-94a5-2d38154a61e3
📒 Files selected for processing (30)
apps/web/src/app/api/activity/summary/__tests__/route.test.tsapps/web/src/app/api/activity/summary/route.tsapps/web/src/app/api/auth/magic-link/verify/__tests__/desktop-verify.test.tsapps/web/src/app/api/auth/magic-link/verify/__tests__/route.test.tsapps/web/src/app/api/auth/magic-link/verify/route.tsapps/web/src/app/api/drives/[driveId]/assignees/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/assignees/route.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/resend/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/resend/route.tsapps/web/src/app/api/drives/[driveId]/members/invite/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/members/invite/route.tsapps/web/src/app/api/drives/[driveId]/pages/route.tsapps/web/src/app/api/drives/[driveId]/permissions-tree/route.tsapps/web/src/app/api/drives/[driveId]/trash/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/trash/route.tsapps/web/src/app/api/pulse/route.tsapps/web/src/app/dashboard/[driveId]/members/invite/page.tsxapps/web/src/components/members/UserSearch.tsxapps/web/src/components/members/__tests__/DriveMembers.test.tsxapps/web/src/lib/repositories/drive-invite-repository.tsapps/web/src/lib/websocket/socket-utils.tspackages/lib/src/permissions/__tests__/permission-mutations-unit.test.tspackages/lib/src/permissions/permission-mutations.tspackages/lib/src/permissions/permissions.tspackages/lib/src/services/__tests__/drive-member-service.test.tspackages/lib/src/services/__tests__/drive-role-service.test.tspackages/lib/src/services/__tests__/drive-service.test.tspackages/lib/src/services/drive-role-service.tspackages/lib/src/services/drive-service.tspackages/lib/src/services/notification-email-service.ts
✅ Files skipped from review due to trivial changes (4)
- packages/lib/src/services/tests/drive-service.test.ts
- apps/web/src/app/api/auth/magic-link/verify/route.ts
- apps/web/src/app/dashboard/[driveId]/members/invite/page.tsx
- apps/web/src/app/api/drives/[driveId]/members/[userId]/resend/route.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/src/app/api/auth/magic-link/verify/tests/desktop-verify.test.ts
- apps/web/src/app/api/drives/[driveId]/members/[userId]/resend/tests/route.test.ts
- apps/web/src/app/api/drives/[driveId]/members/invite/tests/route.test.ts
- apps/web/src/lib/repositories/drive-invite-repository.ts
After switching the verify route to broadcastDriveMemberEventToRecipients, the test file's import of the older single-channel helper became unused and tripped no-unused-vars in CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex P1: if acceptPendingMember threw mid-loop, the verify route logged and continued issuing a session. Combined with page_permissions that the invite route pre-creates, this could let the invitee reach those pages without ever clearing the pending gate. Acceptance failure now revokes the just-created session and redirects to /auth/signin?error=server_error so the user has to retry, keeping the invitation gate intact. CodeRabbit Major: split membershipBecameAccepted away from "is currently accepted" so a pure role/permissions update on an already-accepted member no longer re-emits member_added or createDriveNotification. Existing-accepted members are role-changed only; new join transitions still notify. CodeRabbit Minor: createVerifyRequest helper now uses `!== undefined` so an explicit empty token preserves `?token=` instead of being rewritten to a missing-token request, which was confusing the empty-token vs missing-token coverage. Add tests covering the no-broadcast path on already-accepted role updates and the broadcast path on a userId-on-pending acceptance transition. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When createMagicLinkToken rejects with USER_SUSPENDED in either the invite or resend path, the route now returns 403 with a clear suspended-account message instead of a generic 500. The error is the user's actual suspension state, not a server failure.
Add explicit verify-route test that asserts acceptance failure revokes the just-created session via revokeSession('ps_sess_mock_token', 'invite_acceptance_failed') AND redirects to /auth/signin?error=server_error so the partial-acceptance state cannot complete login.
Add invite + resend route tests for the USER_SUSPENDED → 403 path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a76f0ba183
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex P1: pending-invite acceptance was running only on the magic-link verify route, but the new acceptedAt-IS-NOT-NULL filter on authorization queries gates drive access. So an invitee who signed in via passkey, Google, or Apple OAuth would authenticate but be unable to reach the drive they were invited to — the invite path pre-creates page_permissions yet the drive membership stayed pending. Extract acceptUserPendingInvitations(userId) into a shared lib/auth/post-login-pending-acceptance helper that performs the conditional UPDATE per row, broadcasts member_added to drive recipients, and returns the rows that transitioned from pending to accepted. Wire it into magic-link verify, google/callback, passkey/authenticate, and apple/callback. On any per-row failure the helper throws, the route revokes the just-created session, and the user is redirected to /auth/signin?error=server_error so the invitation gate stays intact. Updated affected test mocks to stub the new helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Self-review found five session-creating routes still bypassed the post-login pending-invite acceptance hook: signup-passkey (the canonical onboarding flow for invitees!), google/native, google/one-tap, apple/native, and mobile/oauth/google/exchange. Wire acceptUserPendingInvitations into each so an invitee who completes any login flow promotes their pending rows. Each route revokes the just-created session and returns 500 on acceptance failure to keep the invitation gate intact. Add a dedicated unit test for acceptUserPendingInvitations covering: empty list, single accepted row, conditional UPDATE returning false (concurrent accept), multi-row fan-out, accept throws, and broadcast throws. Updated affected route tests to mock the helper as a resolved no-op. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai full review |
Owner
Author
|
@codex review |
Contributor
✅ Actions performedFull review triggered. |
/api/drives/[driveId]/members POST was a sibling endpoint that auto-accepted new members and bypassed every safeguard the invite route added: no email-verified-inviter check, no per-drive or per-email rate limit, no pending-state for unaccepted invitees, no member_added fan-out broadcast, owner-only (the invite route allows ADMIN). It auto-accepted via addDriveMember (acceptedAt: new Date()). The frontend never used it (DriveDeleteSection and DriveMembers both call this URL with GET via fetchWithAuth without method override). Removing the handler eliminates the parallel path so /api/drives/[driveId]/members/invite is the canonical add-member entry point with the full gating model. Drop the now-orphaned POST tests + cascade unused imports (audit-log, monitoring/activity-logger) from the route. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8255384a94
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/web/src/app/api/drives/[driveId]/trash/__tests__/route.test.ts (1)
24-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMock
driveMembers.acceptedAtin the schema stub to keep the contract test meaningful.The new
isNotNullpredicate is good, butdriveMembers.acceptedAtis currently undefined in the mocked schema, so this test won’t catch regressions around that column usage.Suggested fix
vi.mock('@pagespace/db/schema/members', () => ({ - driveMembers: { driveId: 'driveMembers.driveId', userId: 'driveMembers.userId', role: 'driveMembers.role' }, + driveMembers: { + driveId: 'driveMembers.driveId', + userId: 'driveMembers.userId', + role: 'driveMembers.role', + acceptedAt: 'driveMembers.acceptedAt', + }, }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/drives/`[driveId]/trash/__tests__/route.test.ts around lines 24 - 36, The mocked schema for driveMembers in route.test.ts is missing the acceptedAt field which the production code now uses with isNotNull; update the mock returned by vi.mock('@pagespace/db/schema/members') to include driveMembers.acceptedAt (e.g., 'driveMembers.acceptedAt') so tests exercise the same schema contract as runtime code and will catch regressions in any logic that checks driveMembers.acceptedAt with isNotNull.apps/web/src/app/api/auth/apple/callback/route.ts (1)
236-238:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not send raw email to
trackAuthEvent.This auth route currently forwards unmasked email in telemetry.
Based on learnings: In auth route handlers under `apps/web/src/app/api/auth/**`, ensure `trackAuthEvent` does not receive raw email and use the `maskEmail` utility.🔧 Suggested fix
trackAuthEvent(user.id, 'login', { - email, + email: maskEmail(email), ip: clientIP, provider: 'apple', userAgent: req.headers.get('user-agent') });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/auth/apple/callback/route.ts` around lines 236 - 238, The telemetry call passes raw email to trackAuthEvent; update the handler to import and use the maskEmail utility and pass maskEmail(email) (instead of email) to trackAuthEvent (retain user.id and clientIP), adding the maskEmail import if missing and ensuring any other auth route handlers under apps/web/src/app/api/auth/** follow the same pattern.apps/web/src/app/api/auth/google/native/route.ts (1)
239-241:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMask email before
trackAuthEventin this auth route.Raw email is being sent to activity tracking here.
Based on learnings: In auth route handlers under `apps/web/src/app/api/auth/**`, ensure `trackAuthEvent` does not receive raw email and use the `maskEmail` utility.🔧 Suggested fix
trackAuthEvent(user.id, 'login', { - email, + email: maskEmail(email), ip: clientIP, provider: 'google-native', platform, userAgent: req.headers.get('user-agent'), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/auth/google/native/route.ts` around lines 239 - 241, The trackAuthEvent call is sending raw email; update the auth route in route.ts so you pass a masked email instead of the raw email: call maskEmail(email) and use its result in the payload to trackAuthEvent (e.g., replace email with maskedEmail in the object passed to trackAuthEvent(user.id, 'login', {...})); if maskEmail is not already imported into apps/web/src/app/api/auth/google/native/route.ts, add the import from the shared utils before using it.apps/web/src/app/api/drives/[driveId]/members/invite/route.ts (1)
83-93: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftReplace duplicated owner/admin checks with centralized permission helpers.
This endpoint still performs local authorization branching, which can drift from shared permission semantics over time.
As per coding guidelines, "Use centralized permission logic via
getUserAccessLevelandcanUserEditPagefrom@pagespace/lib/permissions/permissions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/drives/`[driveId]/members/invite/route.ts around lines 83 - 93, The code duplicates owner/admin checks in the invite route (uses drive.ownerId, userId, and driveInviteRepository.findAdminMembership) instead of centralized permission helpers; replace the local branching that sets isOwner/isAdmin with a single call to getUserAccessLevel(driveId, userId) and/or canUserEditPage(userId, driveId) from `@pagespace/lib/permissions/permissions` and use their result to gate access (returning the same 403 response when permission denies). Remove the manual isOwner/isAdmin logic and rely on the permission helper(s) to decide whether to allow adding members and keep the error message and status intact.
🧹 Nitpick comments (6)
packages/lib/src/permissions/__tests__/permission-mutations-unit.test.ts (1)
20-33: ⚡ Quick winAlign the schema mock with the new
acceptedAtpredicate to avoid false-positive tests.
permission-mutations.tsnow readsdriveMembers.acceptedAt, but this mock omits that field. Adding it makes the test fixture reflect the real query shape.♻️ Proposed mock fix
vi.mock('@pagespace/db/schema/members', () => ({ - driveMembers: { driveId: 'driveId', userId: 'userId', role: 'role', id: 'id' }, + driveMembers: { + driveId: 'driveId', + userId: 'userId', + role: 'role', + id: 'id', + acceptedAt: 'acceptedAt', + }, pagePermissions: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/src/permissions/__tests__/permission-mutations-unit.test.ts` around lines 20 - 33, The test fixture mock is missing the driveMembers.acceptedAt field used by permission-mutations.ts, causing false positives; update the mock in permission-mutations-unit.test.ts to include an acceptedAt entry for driveMembers (e.g., add acceptedAt: 'acceptedAt' to the driveMembers mock or the schema mock returned by vi.mock('@pagespace/db/schema/auth')) so the mocked query shape matches the real code.packages/lib/src/services/__tests__/drive-service.test.ts (1)
35-41: ⚡ Quick winAdd explicit assertions that accepted-membership predicates are invoked.
The mock additions are good, but there’s still no test assertion that
isNotNull(driveMembers.acceptedAt)is part of access/list queries. Adding one assertion in each affected suite would guard this behavior from silent regressions.Based on learnings: "In vitest, avoid using two type arguments with vi.fn<>() in tests (it triggers TS2558)."Example assertion pattern
+import { isNotNull } from '@pagespace/db/operators'; +import { driveMembers } from '@pagespace/db/schema/members'; // after calling listAccessibleDrives / getDriveAccess / getDriveAccessWithDrive +expect(vi.mocked(isNotNull)).toHaveBeenCalledWith(driveMembers.acceptedAt);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/src/services/__tests__/drive-service.test.ts` around lines 35 - 41, Add explicit assertions in the affected test suites to verify that the predicate isNotNull was called with driveMembers.acceptedAt when building access and list queries: after the code that triggers query construction in drive-service.test.ts, assert vi.mocked(isNotNull).toHaveBeenCalledWith(expect.objectContaining({ /* identifier for driveMembers.acceptedAt */ })) or simply toHaveBeenCalledWith(driveMembers.acceptedAt) depending on how the test imports driveMembers; do this once in each suite that exercises access/list so regressions are caught, and avoid using vi.fn type argument overloads in the tests to prevent TS2558.apps/web/src/app/api/auth/google/callback/__tests__/route.test.ts (1)
113-115: ⚡ Quick winAdd one assertion that invitation acceptance is executed after login.
This mock currently avoids side effects but doesn’t verify the callback still invokes
acceptUserPendingInvitations. A single happy-path expectation would protect this contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/auth/google/callback/__tests__/route.test.ts` around lines 113 - 115, Add an assertion in the test that verifies acceptUserPendingInvitations was called after the Google callback/login flow; locate the mocked function acceptUserPendingInvitations (from '@/lib/auth/post-login-pending-acceptance') and, after exercising the route handler or invoking the login callback in the test, add a single expectation like expect(acceptUserPendingInvitations).toHaveBeenCalled() (or toHaveBeenCalledWith(...) if specific args are known) to ensure invitation acceptance runs in the happy path.apps/web/src/app/api/activity/summary/__tests__/route.test.ts (1)
45-45: ⚡ Quick winStrengthen this test to validate accepted-membership filtering.
isNotNullis now mocked, but this doesn’t verify the route actually uses it correctly. Consider addingacceptedAtto thedriveMembersmock and an assertion thatisNotNull(driveMembers.acceptedAt)is used at least once.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/activity/summary/__tests__/route.test.ts` at line 45, The test currently mocks isNotNull but doesn't confirm the route actually filters by accepted membership; update the driveMembers test fixture to include an acceptedAt value (e.g., a non-null timestamp) and then add an assertion that the mocked isNotNull function was called with that value (e.g., expect(isNotNull).toHaveBeenCalledWith(driveMembers.acceptedAt)) so the test verifies the route uses isNotNull(driveMembers.acceptedAt) at least once; locate and modify the driveMembers mock and the test assertions in route.test.ts where isNotNull is defined and used.apps/web/src/app/api/auth/apple/callback/__tests__/route.test.ts (1)
92-94: ⚡ Quick winAssert the pending-invite hook is called in a success path.
Right now this only prevents runtime failures; it doesn’t lock in the new behavior. Add one happy-path assertion that
acceptUserPendingInvitationsis called with the authenticated user id so this integration can’t silently regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/auth/apple/callback/__tests__/route.test.ts` around lines 92 - 94, Add a happy-path assertion in the existing test in route.test.ts that verifies the mocked acceptUserPendingInvitations hook is invoked with the authenticated user's id: locate the mock created via vi.mock('@/lib/auth/post-login-pending-acceptance') and after the request that simulates a successful Apple callback, assert that the mocked acceptUserPendingInvitations was called once and with the expected user id (use the same user id value the test uses for the authenticated user). Ensure you access the mock via the vi.mocked helper or the exported mock function reference and call toHaveBeenCalledWith(expectedUserId) so the integration can't regress.apps/web/src/app/api/auth/magic-link/verify/__tests__/route.test.ts (1)
606-617: ⚡ Quick winStrengthen the non-matching
inviteDriveIdcase with real pending rows.This currently passes with an empty pending set, so it won’t catch regressions where non-matching
inviteDriveIdaccidentally suppresses acceptance of other pending memberships.Suggested test adjustment
- it('given inviteDriveId does not match any pending row for this user, falls through to default redirect rather than error', async () => { - vi.mocked(driveInviteRepository.findPendingMembersForUser).mockResolvedValue([]); + it('given inviteDriveId does not match any pending row for this user, still accepts pending rows and falls through to default redirect', async () => { + vi.mocked(driveInviteRepository.findPendingMembersForUser).mockResolvedValue([ + pendingRow('mem_other', 'drive_other'), + ]); const response = await GET(createVerifyRequest('valid-token', 'drive_unrelated')); const location = response.headers.get('Location')!; expect(response.status).toBe(302); expect(location).not.toContain('/dashboard/drive_unrelated'); expect(location).toContain('/dashboard'); - expect(broadcastDriveMemberEventToRecipients).not.toHaveBeenCalled(); + expect(driveInviteRepository.acceptPendingMember).toHaveBeenCalledWith('mem_other'); + expect(broadcastDriveMemberEventToRecipients).toHaveBeenCalledTimes(1); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/auth/magic-link/verify/__tests__/route.test.ts` around lines 606 - 617, The test currently stubs driveInviteRepository.findPendingMembersForUser to return an empty array which doesn't validate the non-matching inviteDriveId path; change the mock in this spec (the one calling GET(createVerifyRequest('valid-token', 'drive_unrelated'))) to return a non-empty array of pending invite objects whose driveId values do NOT equal 'drive_unrelated' (use the same shape the code expects), then assert the response still 302s to a generic '/dashboard' (not '/dashboard/drive_unrelated') and that broadcastDriveMemberEventToRecipients was not called; this ensures the code handles a set of other pending rows rather than an empty set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/api/auth/mobile/oauth/google/exchange/route.ts`:
- Around line 311-317: The invitation acceptance (acceptUserPendingInvitations)
must run before any successful-login side effects—move the try/catch that calls
acceptUserPendingInvitations so it executes before trackAuthEvent(..., 'login',
...) and before any rate-limit reset logic; on failure keep the existing error
logging and sessionService.revokeSession(sessionToken,
'invite_acceptance_failed') behavior and return the 500 response so we don't
emit false-positive login telemetry or reset rate limits for failed invite
acceptance.
In `@apps/web/src/app/api/drives/`[driveId]/members/[userId]/resend/route.ts:
- Around line 44-63: Replace the ad-hoc owner/admin branching with the
centralized permission helpers: call getUserAccessLevel(callerId, driveId) (or
canUserEditPage(callerId, driveId) if available for this action) from
`@pagespace/lib/permissions/permissions` and use its result to decide whether the
caller can resend invitations instead of querying
driveInviteRepository.findAdminMembership and comparing drive.ownerId; keep the
existing auditRequest and error response path but use the permission helper
outcome (and its canonical enum/boolean) to decide when to log
authz.access.denied and return the 403 response, ensuring you import the
permission helpers and map their return values to the resend_invitation
authorization check.
In `@apps/web/src/app/api/drives/`[driveId]/members/invite/route.ts:
- Around line 331-337: The response message always says "User added…" even when
inviteKind indicates an email invitation; update the response generation in the
route handler that returns NextResponse.json (referencing memberId, inviteKind,
normalizedEmail, and validResults) to branch the message by inviteKind: if
inviteKind === 'invited' return a message like "Invitation sent to
{normalizedEmail}" or "Invitation pending for {normalizedEmail}" (or similar)
that reflects a pending invite, otherwise keep the existing "User added with
{validResults.length} page permissions" message; ensure the chosen message
includes normalizedEmail when present and still reports permissionsGranted using
validResults.length.
- Around line 60-75: Before destructuring the request body, validate that the
parsed body is a non-null plain object (not an array or primitive) and return a
400 validation response for malformed JSON; specifically wrap the current call
to request.json() and the destructuring of bodyUserId, bodyEmail, role,
customRoleId, rawPermissions with a guard like: if typeof body !== 'object' ||
body === null || Array.isArray(body) then respond 400 with a clear message,
otherwise perform the destructuring and coerce permissions into an array as now;
include basic type checks for userId/email/role/customRoleId/rawPermissions to
ensure downstream code using permissions and role (the variables role,
rawPermissions, permissions) won’t throw on bad shapes.
In `@apps/web/src/app/api/pulse/cron/route.ts`:
- Around line 144-154: The teamMembers query currently omits the acceptedAt
check and can return pending invites; add the same membership filter used for
driveMembers (i.e., include isNotNull(teamMembers.acceptedAt) in the where
clause alongside eq(teamMembers.userId, userId)) so only accepted memberships
are returned; apply the same fix to the other similar teamMembers/teamIds query
later in the file (the second occurrence that mirrors the driveMembers logic).
In `@apps/web/src/app/api/pulse/generate/route.ts`:
- Around line 80-90: The team-members query in the pulse generation route still
returns pending invites; update the query that selects from teamMembers (used to
build contextData) to mirror the drive-members filter by adding an
accepted-membership predicate—use and(eq(teamMembers.userId, userId),
isNotNull(teamMembers.acceptedAt)) (or equivalent for teamId lookups) so only
rows with non-null teamMembers.acceptedAt are returned; apply the same change to
the other teamMembers query referenced around lines 109-120 to prevent leaking
pending invitee info.
In `@apps/web/src/app/dashboard/`[driveId]/members/invite/page.tsx:
- Around line 145-150: The success toast currently reads pendingInviteEmail from
component state which can change before the request resolves; capture a
request-scoped email (e.g., const recipient = pendingInviteEmail) at the time
you start the invite/resend/revoke request and pass that recipient into
successToast (and any other toast callers) instead of reading pendingInviteEmail
inside successToast; update successToast to accept and use that recipient
parameter and modify the three call sites (the current successToast usage around
the invite submit, resend and revoke flows referenced at lines ~149, ~173-174,
~223-224) to pass the captured recipient.
In `@apps/web/src/components/members/DriveMembers.tsx`:
- Around line 137-138: The current filters in DriveMembers.tsx use truthy/falsy
checks on m.acceptedAt which misclassifies undefined; update the grouping to use
strict null checks: set acceptedMembers = members.filter(m => m.acceptedAt !==
null) and pendingMembers = members.filter(m => m.acceptedAt === null) so only
explicit null is treated as pending (refer to the acceptedMembers and
pendingMembers variables in this file).
In `@tasks/drive-invites-by-email.md`:
- Line 60: Update the epic doc text to reference the actual helper function name
used by the implementation: replace the stale sendDriveInvitationEmail with
sendPendingDriveInvitationEmail in the POST
/api/drives/[driveId]/members/[userId]/resend description so it accurately
states that the endpoint re-fires createMagicLinkToken and
sendPendingDriveInvitationEmail for a pending row (rate-limited by
checkDistributedRateLimit) and that the members UI exposes a Resend button on
each pending row.
---
Outside diff comments:
In `@apps/web/src/app/api/auth/apple/callback/route.ts`:
- Around line 236-238: The telemetry call passes raw email to trackAuthEvent;
update the handler to import and use the maskEmail utility and pass
maskEmail(email) (instead of email) to trackAuthEvent (retain user.id and
clientIP), adding the maskEmail import if missing and ensuring any other auth
route handlers under apps/web/src/app/api/auth/** follow the same pattern.
In `@apps/web/src/app/api/auth/google/native/route.ts`:
- Around line 239-241: The trackAuthEvent call is sending raw email; update the
auth route in route.ts so you pass a masked email instead of the raw email: call
maskEmail(email) and use its result in the payload to trackAuthEvent (e.g.,
replace email with maskedEmail in the object passed to trackAuthEvent(user.id,
'login', {...})); if maskEmail is not already imported into
apps/web/src/app/api/auth/google/native/route.ts, add the import from the shared
utils before using it.
In `@apps/web/src/app/api/drives/`[driveId]/members/invite/route.ts:
- Around line 83-93: The code duplicates owner/admin checks in the invite route
(uses drive.ownerId, userId, and driveInviteRepository.findAdminMembership)
instead of centralized permission helpers; replace the local branching that sets
isOwner/isAdmin with a single call to getUserAccessLevel(driveId, userId) and/or
canUserEditPage(userId, driveId) from `@pagespace/lib/permissions/permissions` and
use their result to gate access (returning the same 403 response when permission
denies). Remove the manual isOwner/isAdmin logic and rely on the permission
helper(s) to decide whether to allow adding members and keep the error message
and status intact.
In `@apps/web/src/app/api/drives/`[driveId]/trash/__tests__/route.test.ts:
- Around line 24-36: The mocked schema for driveMembers in route.test.ts is
missing the acceptedAt field which the production code now uses with isNotNull;
update the mock returned by vi.mock('@pagespace/db/schema/members') to include
driveMembers.acceptedAt (e.g., 'driveMembers.acceptedAt') so tests exercise the
same schema contract as runtime code and will catch regressions in any logic
that checks driveMembers.acceptedAt with isNotNull.
---
Nitpick comments:
In `@apps/web/src/app/api/activity/summary/__tests__/route.test.ts`:
- Line 45: The test currently mocks isNotNull but doesn't confirm the route
actually filters by accepted membership; update the driveMembers test fixture to
include an acceptedAt value (e.g., a non-null timestamp) and then add an
assertion that the mocked isNotNull function was called with that value (e.g.,
expect(isNotNull).toHaveBeenCalledWith(driveMembers.acceptedAt)) so the test
verifies the route uses isNotNull(driveMembers.acceptedAt) at least once; locate
and modify the driveMembers mock and the test assertions in route.test.ts where
isNotNull is defined and used.
In `@apps/web/src/app/api/auth/apple/callback/__tests__/route.test.ts`:
- Around line 92-94: Add a happy-path assertion in the existing test in
route.test.ts that verifies the mocked acceptUserPendingInvitations hook is
invoked with the authenticated user's id: locate the mock created via
vi.mock('@/lib/auth/post-login-pending-acceptance') and after the request that
simulates a successful Apple callback, assert that the mocked
acceptUserPendingInvitations was called once and with the expected user id (use
the same user id value the test uses for the authenticated user). Ensure you
access the mock via the vi.mocked helper or the exported mock function reference
and call toHaveBeenCalledWith(expectedUserId) so the integration can't regress.
In `@apps/web/src/app/api/auth/google/callback/__tests__/route.test.ts`:
- Around line 113-115: Add an assertion in the test that verifies
acceptUserPendingInvitations was called after the Google callback/login flow;
locate the mocked function acceptUserPendingInvitations (from
'@/lib/auth/post-login-pending-acceptance') and, after exercising the route
handler or invoking the login callback in the test, add a single expectation
like expect(acceptUserPendingInvitations).toHaveBeenCalled() (or
toHaveBeenCalledWith(...) if specific args are known) to ensure invitation
acceptance runs in the happy path.
In `@apps/web/src/app/api/auth/magic-link/verify/__tests__/route.test.ts`:
- Around line 606-617: The test currently stubs
driveInviteRepository.findPendingMembersForUser to return an empty array which
doesn't validate the non-matching inviteDriveId path; change the mock in this
spec (the one calling GET(createVerifyRequest('valid-token',
'drive_unrelated'))) to return a non-empty array of pending invite objects whose
driveId values do NOT equal 'drive_unrelated' (use the same shape the code
expects), then assert the response still 302s to a generic '/dashboard' (not
'/dashboard/drive_unrelated') and that broadcastDriveMemberEventToRecipients was
not called; this ensures the code handles a set of other pending rows rather
than an empty set.
In `@packages/lib/src/permissions/__tests__/permission-mutations-unit.test.ts`:
- Around line 20-33: The test fixture mock is missing the
driveMembers.acceptedAt field used by permission-mutations.ts, causing false
positives; update the mock in permission-mutations-unit.test.ts to include an
acceptedAt entry for driveMembers (e.g., add acceptedAt: 'acceptedAt' to the
driveMembers mock or the schema mock returned by
vi.mock('@pagespace/db/schema/auth')) so the mocked query shape matches the real
code.
In `@packages/lib/src/services/__tests__/drive-service.test.ts`:
- Around line 35-41: Add explicit assertions in the affected test suites to
verify that the predicate isNotNull was called with driveMembers.acceptedAt when
building access and list queries: after the code that triggers query
construction in drive-service.test.ts, assert
vi.mocked(isNotNull).toHaveBeenCalledWith(expect.objectContaining({ /*
identifier for driveMembers.acceptedAt */ })) or simply
toHaveBeenCalledWith(driveMembers.acceptedAt) depending on how the test imports
driveMembers; do this once in each suite that exercises access/list so
regressions are caught, and avoid using vi.fn type argument overloads in the
tests to prevent TS2558.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 41300777-f0b1-4ce4-9fb6-4e947c59189b
📒 Files selected for processing (62)
apps/web/src/app/api/activity/summary/__tests__/route.test.tsapps/web/src/app/api/activity/summary/route.tsapps/web/src/app/api/auth/__tests__/google-callback-redirect.test.tsapps/web/src/app/api/auth/__tests__/mobile-oauth-google-exchange.test.tsapps/web/src/app/api/auth/apple/callback/__tests__/route.test.tsapps/web/src/app/api/auth/apple/callback/route.tsapps/web/src/app/api/auth/apple/native/__tests__/route.test.tsapps/web/src/app/api/auth/apple/native/route.tsapps/web/src/app/api/auth/google/__tests__/google-callback-redirect.test.tsapps/web/src/app/api/auth/google/__tests__/one-tap.test.tsapps/web/src/app/api/auth/google/__tests__/open-redirect-protection.test.tsapps/web/src/app/api/auth/google/callback/__tests__/route.test.tsapps/web/src/app/api/auth/google/callback/route.tsapps/web/src/app/api/auth/google/native/__tests__/route.test.tsapps/web/src/app/api/auth/google/native/route.tsapps/web/src/app/api/auth/google/one-tap/__tests__/route.test.tsapps/web/src/app/api/auth/google/one-tap/route.tsapps/web/src/app/api/auth/magic-link/verify/__tests__/desktop-verify.test.tsapps/web/src/app/api/auth/magic-link/verify/__tests__/route.test.tsapps/web/src/app/api/auth/magic-link/verify/route.tsapps/web/src/app/api/auth/mobile/oauth/google/exchange/route.tsapps/web/src/app/api/auth/passkey/authenticate/__tests__/route.test.tsapps/web/src/app/api/auth/passkey/authenticate/route.tsapps/web/src/app/api/auth/signup-passkey/__tests__/route.test.tsapps/web/src/app/api/auth/signup-passkey/route.tsapps/web/src/app/api/drives/[driveId]/assignees/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/assignees/route.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/resend/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/resend/route.tsapps/web/src/app/api/drives/[driveId]/members/invite/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/members/invite/route.tsapps/web/src/app/api/drives/[driveId]/pages/route.tsapps/web/src/app/api/drives/[driveId]/permissions-tree/route.tsapps/web/src/app/api/drives/[driveId]/trash/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/trash/route.tsapps/web/src/app/api/pulse/cron/route.tsapps/web/src/app/api/pulse/generate/route.tsapps/web/src/app/api/pulse/route.tsapps/web/src/app/dashboard/[driveId]/members/invite/page.tsxapps/web/src/components/members/DriveMembers.tsxapps/web/src/components/members/MemberRow.tsxapps/web/src/components/members/UserSearch.tsxapps/web/src/components/members/__tests__/DriveMembers.test.tsxapps/web/src/components/members/__tests__/UserSearch.test.tsxapps/web/src/lib/auth/__tests__/post-login-pending-acceptance.test.tsapps/web/src/lib/auth/post-login-pending-acceptance.tsapps/web/src/lib/repositories/drive-invite-repository.tsapps/web/src/lib/websocket/socket-utils.tspackages/lib/package.jsonpackages/lib/src/auth/magic-link-service.tspackages/lib/src/permissions/__tests__/permission-mutations-unit.test.tspackages/lib/src/permissions/permission-mutations.tspackages/lib/src/permissions/permissions.tspackages/lib/src/services/__tests__/drive-member-service.test.tspackages/lib/src/services/__tests__/drive-role-service.test.tspackages/lib/src/services/__tests__/drive-service.test.tspackages/lib/src/services/drive-member-service.tspackages/lib/src/services/drive-role-service.tspackages/lib/src/services/drive-service.tspackages/lib/src/services/notification-email-service.tsplan.mdtasks/drive-invites-by-email.md
Codex P2: the PATCH and DELETE handlers in /api/drives/[driveId]/members/[userId] still emitted via broadcastDriveMemberEvent (singular), which routes to user:{targetUserId}:drives — the affected user's personal channel — so other admins watching the members page never received the realtime nudge. The new realtime sync the DriveMembers component depends on was incomplete for role-change and removal until users manually refreshed.
Switch both broadcasts to broadcastDriveMemberEventToRecipients with getDriveRecipientUserIds(driveId) so the same drive-wide fan-out as the new join path now applies symmetrically across role-change and removal.
Update test mocks to expose the new helper, drop the stale Auto-accept-for-now comment in addDriveMember, and update assertions to expect the fan-out helper.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Removing the POST handler in /api/drives/[driveId]/members left the file with a GET-only handler. The security-audit-coverage gate test scans every route file for an audit call and fails if one is missing AND the route isn't on the exemption list. The GET handler is a read-only data fetch and the write paths now live in /members/invite (audited) and /members/[userId] (audited), so it joins the existing read-only drive sub-routes on the AUDIT_EXEMPT_ROUTES list with a justification. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
**Major fixes** - mobile/oauth/google/exchange: move acceptUserPendingInvitations BEFORE rate-limit reset and trackAuthEvent so a mid-login failure can't leave success-side telemetry behind. - invite/route.ts: guard the parsed JSON body against null/array/primitive before destructuring so malformed payloads return 400 instead of crashing into 500. - pulse/cron + pulse/generate: add isNotNull(driveMembers.acceptedAt) to the teamMembers query to match the existing driveIds filter, so pending invites can't leak into pulse summaries. **Minor fixes** - invite/route.ts: branch the response message on inviteKind so 'invited' responses no longer claim the user was added. - invite/page.tsx: snapshot pendingInviteEmail at submit time so the success toast renders the correct email even if state is reset before the response lands. - DriveMembers.tsx: classify pending vs accepted with a strict acceptedAt === null check (was relying on truthy/falsy which would silently misclassify undefined). - tasks/drive-invites-by-email.md: rename sendDriveInvitationEmail to sendPendingDriveInvitationEmail to match the implementation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai full review |
Owner
Author
|
@codex review |
Contributor
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c84f9df729
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- findAdminMembership: require acceptedAt IS NOT NULL so a pending admin invitee can't exercise admin powers (sending further invites, etc.) before they've completed their own invitation. - Detect existing temp users (emailVerified IS NULL) and re-route them through the invitation flow instead of silently auto-accepting a never-confirmed account when re-invited (e.g. after a prior revoke). - PATCH /members/[userId] accepts role-only updates without a permissions array — still rejects non-array permissions and totally empty payloads. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/app/api/auth/mobile/oauth/google/exchange/route.ts (1)
309-317:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMask email before
trackAuthEventin auth success telemetry.
trackAuthEventis still receiving rawuser.email, which leaks PII in auth analytics/audit streams.🔧 Proposed fix
trackAuthEvent(user.id, 'login', { - email: user.email, + email: maskEmail(user.email), ip: clientIP, provider: 'google', userAgent: req.headers.get('user-agent'), platform, appVersion, });Based on learnings: In auth route handlers under
apps/web/src/app/api/auth/**, ensuretrackAuthEventdoes not receive raw email (PII) and usemaskEmailinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/auth/mobile/oauth/google/exchange/route.ts` around lines 309 - 317, Replace the raw email passed to telemetry with a masked version: call maskEmail(user.email) and pass that result to trackAuthEvent instead of user.email so PII is not sent; update the call in the OAuth success handler where trackAuthEvent(user.id, 'login', {...}) is invoked (referencing trackAuthEvent and maskEmail) to use the masked email value and keep other fields (ip, provider, userAgent, platform, appVersion) unchanged.
🧹 Nitpick comments (1)
apps/web/src/app/api/drives/[driveId]/members/[userId]/__tests__/route.test.ts (1)
804-811: ⚡ Quick winStrengthen fan-out tests to assert recipients, not just call count.
These assertions only check invocation count. Since this PR’s key behavior is recipient-scoped fan-out, also assert
getDriveRecipientUserIds(mockDriveId)and the exact recipient array passed tobroadcastDriveMemberEventToRecipients.Suggested test assertion upgrade
@@ -import { checkDriveAccess, getDriveMemberDetails, getMemberPermissions, updateMemberRole, updateMemberPermissions } from '@pagespace/lib/services/drive-member-service' +import { checkDriveAccess, getDriveMemberDetails, getMemberPermissions, updateMemberRole, updateMemberPermissions, getDriveRecipientUserIds } from '@pagespace/lib/services/drive-member-service' @@ it('should broadcast event when role changes', async () => { + vi.mocked(getDriveRecipientUserIds).mockResolvedValue(['owner_1', 'admin_2']); @@ - expect(broadcastDriveMemberEventToRecipients).toHaveBeenCalledTimes(1); + expect(getDriveRecipientUserIds).toHaveBeenCalledWith(mockDriveId); + expect(broadcastDriveMemberEventToRecipients).toHaveBeenCalledWith( + expect.anything(), + ['owner_1', 'admin_2'] + ); @@ it('should broadcast member removal event', async () => { + vi.mocked(getDriveRecipientUserIds).mockResolvedValue(['owner_1', 'admin_2']); @@ - expect(broadcastDriveMemberEventToRecipients).toHaveBeenCalledTimes(1); + expect(getDriveRecipientUserIds).toHaveBeenCalledWith(mockDriveId); + expect(broadcastDriveMemberEventToRecipients).toHaveBeenCalledWith( + expect.anything(), + ['owner_1', 'admin_2'] + );Also applies to: 1155-1162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/drives/`[driveId]/members/[userId]/__tests__/route.test.ts around lines 804 - 811, The test currently only checks broadcastDriveMemberEventToRecipients was called once; update it to also assert that getDriveRecipientUserIds(mockDriveId) was called and that broadcastDriveMemberEventToRecipients was invoked with the exact recipient array returned from getDriveRecipientUserIds(mockDriveId) (not just the call count) alongside the payload from createDriveMemberEventPayload; locate the assertions around createDriveMemberEventPayload and broadcastDriveMemberEventToRecipients in the test and replace/augment the expect(broadcastDriveMemberEventToRecipients).toHaveBeenCalledTimes(1) with expects that validate getDriveRecipientUserIds was called with mockDriveId and that broadcastDriveMemberEventToRecipients was calledWith(theRecipientArray, mockDriveId, createDriveMemberEventPayload(...)) so the fan-out recipients are explicitly asserted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/api/drives/`[driveId]/members/[userId]/route.ts:
- Around line 143-152: The fan-out recipient lookup and broadcast (calls to
getDriveRecipientUserIds and broadcastDriveMemberEventToRecipients using
createDriveMemberEventPayload) must not cause the whole route to return 500
after the membership write commits; wrap the recipient lookup + broadcast block
in a local try/catch, log the error (including context like
driveId/userId/operation), and allow the core flow (membership mutation and any
immediate kick flow) to continue normally; apply the same change to the other
similar block around the DELETE flow so failures in fan-out are non-fatal to the
primary write.
In `@tasks/drive-invites-by-email.md`:
- Line 3: Update the epic status string "**Status**: 📋 PLANNED" to reflect the
implemented state (e.g., "**Status**: ✅ COMPLETED" or "**Status**: 🚀
IMPLEMENTED") so tracking and handoff docs are accurate; locate the literal
"**Status**: 📋 PLANNED" line in the tasks/drive-invites-by-email.md content and
replace it with the chosen updated status text, then save and include a short
note in the PR describing the status change.
---
Outside diff comments:
In `@apps/web/src/app/api/auth/mobile/oauth/google/exchange/route.ts`:
- Around line 309-317: Replace the raw email passed to telemetry with a masked
version: call maskEmail(user.email) and pass that result to trackAuthEvent
instead of user.email so PII is not sent; update the call in the OAuth success
handler where trackAuthEvent(user.id, 'login', {...}) is invoked (referencing
trackAuthEvent and maskEmail) to use the masked email value and keep other
fields (ip, provider, userAgent, platform, appVersion) unchanged.
---
Nitpick comments:
In
`@apps/web/src/app/api/drives/`[driveId]/members/[userId]/__tests__/route.test.ts:
- Around line 804-811: The test currently only checks
broadcastDriveMemberEventToRecipients was called once; update it to also assert
that getDriveRecipientUserIds(mockDriveId) was called and that
broadcastDriveMemberEventToRecipients was invoked with the exact recipient array
returned from getDriveRecipientUserIds(mockDriveId) (not just the call count)
alongside the payload from createDriveMemberEventPayload; locate the assertions
around createDriveMemberEventPayload and broadcastDriveMemberEventToRecipients
in the test and replace/augment the
expect(broadcastDriveMemberEventToRecipients).toHaveBeenCalledTimes(1) with
expects that validate getDriveRecipientUserIds was called with mockDriveId and
that broadcastDriveMemberEventToRecipients was calledWith(theRecipientArray,
mockDriveId, createDriveMemberEventPayload(...)) so the fan-out recipients are
explicitly asserted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 983e42db-07c8-4dee-848a-ed25ddbc6787
📒 Files selected for processing (15)
apps/web/src/app/api/__tests__/security-audit-coverage.test.tsapps/web/src/app/api/auth/mobile/oauth/google/exchange/route.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/route.tsapps/web/src/app/api/drives/[driveId]/members/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/members/invite/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/members/invite/route.tsapps/web/src/app/api/drives/[driveId]/members/route.tsapps/web/src/app/api/pulse/cron/route.tsapps/web/src/app/api/pulse/generate/route.tsapps/web/src/app/dashboard/[driveId]/members/invite/page.tsxapps/web/src/components/members/DriveMembers.tsxapps/web/src/lib/repositories/drive-invite-repository.tspackages/lib/src/services/drive-member-service.tstasks/drive-invites-by-email.md
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/app/dashboard/[driveId]/members/invite/page.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/web/src/app/api/pulse/generate/route.ts
- apps/web/src/lib/repositories/drive-invite-repository.ts
- apps/web/src/app/api/drives/[driveId]/members/invite/tests/route.test.ts
- packages/lib/src/services/drive-member-service.ts
- apps/web/src/app/api/drives/[driveId]/members/invite/route.ts
- apps/web/src/components/members/DriveMembers.tsx
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wrap getDriveRecipientUserIds + broadcastDriveMemberEventToRecipients in try/catch on PATCH (role change) and DELETE (member removal). The membership write has already committed by the time fan-out runs; if the realtime side fails (recipient lookup error or 5xx from realtime service), the route would otherwise return 500 and the DELETE flow would skip the kick-from-rooms revocation step. The kick step is the primary security boundary for removal — it must run unconditionally. Also flip tasks/drive-invites-by-email.md status from PLANNED to IMPLEMENTED so handoff docs match reality. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 4, 2026
2witstudios
added a commit
that referenced
this pull request
May 5, 2026
Adds an email-payload branch to POST /api/drives/[driveId]/members/invite so owners and admins can invite users who don't yet have a PageSpace account. Closes Epic 4 of the pu/invites redo. - Slice 4.1: createMagicLinkToken accepts expiryMinutes (default 5, ceiling 30 days). Exports INVITATION_LINK_EXPIRY_MINUTES = 7 days. - Slice 4.2: driveInviteRepository gains findUserIdByEmail, findActivePendingMemberByEmail, findInviterDisplay, and a transactional createAcceptedMemberWithPermissions helper. - Slice 4.3: sendPendingDriveInvitationEmail strips CR/LF/control chars from interpolated fields and propagates send failures. - Slice 4.4: New invite route. Zod schema validates role enum at the boundary (PR #1229 missed OWNER); transactional member+permission insert closes the partial-state gap; explicit env check refuses to send when WEB_APP_URL and NEXT_PUBLIC_APP_URL are both unset; rate limit on (driveId, email) and global email at 3 / 15 min; USER_SUSPENDED maps to 403, not 500. Email path normalizes to trim/lowercase before lookup AND storage. Pending insert does not broadcast member_added (the invitee hasn't joined yet). - Slice 4.5: Retired the legacy POST /members route and its snapshot test; added members to AUDIT_EXEMPT_ROUTES (now read-only). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2witstudios
added a commit
that referenced
this pull request
May 5, 2026
* feat(invites): invite by email Adds an email-payload branch to POST /api/drives/[driveId]/members/invite so owners and admins can invite users who don't yet have a PageSpace account. Closes Epic 4 of the pu/invites redo. - Slice 4.1: createMagicLinkToken accepts expiryMinutes (default 5, ceiling 30 days). Exports INVITATION_LINK_EXPIRY_MINUTES = 7 days. - Slice 4.2: driveInviteRepository gains findUserIdByEmail, findActivePendingMemberByEmail, findInviterDisplay, and a transactional createAcceptedMemberWithPermissions helper. - Slice 4.3: sendPendingDriveInvitationEmail strips CR/LF/control chars from interpolated fields and propagates send failures. - Slice 4.4: New invite route. Zod schema validates role enum at the boundary (PR #1229 missed OWNER); transactional member+permission insert closes the partial-state gap; explicit env check refuses to send when WEB_APP_URL and NEXT_PUBLIC_APP_URL are both unset; rate limit on (driveId, email) and global email at 3 / 15 min; USER_SUSPENDED maps to 403, not 500. Email path normalizes to trim/lowercase before lookup AND storage. Pending insert does not broadcast member_added (the invitee hasn't joined yet). - Slice 4.5: Retired the legacy POST /members route and its snapshot test; added members to AUDIT_EXEMPT_ROUTES (now read-only). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(invites): close adversarial review gaps Five fixes from a self-critique pass on PR #1239: 1. BLOCKER — suspended verified user could bypass suspension via the email→user fall-through. findUserIdByEmail now selects suspendedAt and the route 403s before fall-through. Added two regression tests (suspended verified, suspended unverified). 2. HIGH — page-level permissions on the email-pending path were silently dropped (no target user to attach to yet). Now rejects 422 with a message directing the inviter to grant permissions after accept. 3. HIGH — if sendPendingDriveInvitationEmail throws, the just-inserted pending member row is now rolled back so a re-invite isn't blocked by findActivePendingMemberByEmail. Returns 502 (upstream email delivery failure). Added test. 4. LOW — preserve the original email selector in audit details when email payload falls through to userId path (sourceEmail field). Added test. 5. Cleanup — removed two `as never` casts (logMemberActivity now passes targetUserId for the email path; loggers.api.error uses the real (message, error?, metadata) signature). Removed redundant trim/lowercase since Zod's pipe already normalizes. Fixed misleading "base64url" comment — magic-link tokens are CUID2. 43/43 invite route tests pass; typecheck + lint clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(invites): tighten test-file quality Addressing test-quality issues from a second adversarial pass: - afterEach restore env so a test that deletes WEB_APP_URL doesn't poison subsequent tests (afterAll fires too late). - Replace mockResolvedValueOnce ordering assumptions in the rate-limit tests with key-prefix dispatch via mockImplementation, so the tests don't break if the route reorders its rate-limit calls. - Tighten loose assertions: orphan test now asserts the email + expiry arguments to createMagicLinkToken; pure-role-update test now asserts 200 status; existing-member-update test asserts response.status and json.kind. - Add the missing "userId path doesn't touch findUserIdByEmail" lock to keep the two paths disjoint. - Add the missing "email → verified user already accepted in drive → 409" coverage gap. 45/45 invite tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7 tasks done
2witstudios
added a commit
that referenced
this pull request
May 15, 2026
* feat(invites): invite by email Adds an email-payload branch to POST /api/drives/[driveId]/members/invite so owners and admins can invite users who don't yet have a PageSpace account. Closes Epic 4 of the pu/invites redo. - Slice 4.1: createMagicLinkToken accepts expiryMinutes (default 5, ceiling 30 days). Exports INVITATION_LINK_EXPIRY_MINUTES = 7 days. - Slice 4.2: driveInviteRepository gains findUserIdByEmail, findActivePendingMemberByEmail, findInviterDisplay, and a transactional createAcceptedMemberWithPermissions helper. - Slice 4.3: sendPendingDriveInvitationEmail strips CR/LF/control chars from interpolated fields and propagates send failures. - Slice 4.4: New invite route. Zod schema validates role enum at the boundary (PR #1229 missed OWNER); transactional member+permission insert closes the partial-state gap; explicit env check refuses to send when WEB_APP_URL and NEXT_PUBLIC_APP_URL are both unset; rate limit on (driveId, email) and global email at 3 / 15 min; USER_SUSPENDED maps to 403, not 500. Email path normalizes to trim/lowercase before lookup AND storage. Pending insert does not broadcast member_added (the invitee hasn't joined yet). - Slice 4.5: Retired the legacy POST /members route and its snapshot test; added members to AUDIT_EXEMPT_ROUTES (now read-only). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(invites): close adversarial review gaps Five fixes from a self-critique pass on PR #1239: 1. BLOCKER — suspended verified user could bypass suspension via the email→user fall-through. findUserIdByEmail now selects suspendedAt and the route 403s before fall-through. Added two regression tests (suspended verified, suspended unverified). 2. HIGH — page-level permissions on the email-pending path were silently dropped (no target user to attach to yet). Now rejects 422 with a message directing the inviter to grant permissions after accept. 3. HIGH — if sendPendingDriveInvitationEmail throws, the just-inserted pending member row is now rolled back so a re-invite isn't blocked by findActivePendingMemberByEmail. Returns 502 (upstream email delivery failure). Added test. 4. LOW — preserve the original email selector in audit details when email payload falls through to userId path (sourceEmail field). Added test. 5. Cleanup — removed two `as never` casts (logMemberActivity now passes targetUserId for the email path; loggers.api.error uses the real (message, error?, metadata) signature). Removed redundant trim/lowercase since Zod's pipe already normalizes. Fixed misleading "base64url" comment — magic-link tokens are CUID2. 43/43 invite route tests pass; typecheck + lint clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(invites): tighten test-file quality Addressing test-quality issues from a second adversarial pass: - afterEach restore env so a test that deletes WEB_APP_URL doesn't poison subsequent tests (afterAll fires too late). - Replace mockResolvedValueOnce ordering assumptions in the rate-limit tests with key-prefix dispatch via mockImplementation, so the tests don't break if the route reorders its rate-limit calls. - Tighten loose assertions: orphan test now asserts the email + expiry arguments to createMagicLinkToken; pure-role-update test now asserts 200 status; existing-member-update test asserts response.status and json.kind. - Add the missing "userId path doesn't touch findUserIdByEmail" lock to keep the two paths disjoint. - Add the missing "email → verified user already accepted in drive → 409" coverage gap. 45/45 invite tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
2 participants
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
Closes the gap where a drive owner could not invite a colleague who hadn't yet signed up to PageSpace. The empty-state of
UserSearchpreviously dead-ended with "Try searching by email address" and offered no way to actually send an invite. This PR makes that case work end-to-end without introducing a new table or a parallel invitation namespace.The schema already supports the pending state (
drive_members.acceptedAtis nullable), the magic-link service already auto-creates a temp user when sent to a new email, andDriveInvitationEmailis already wired intonotification-email-service. This PR connects those pieces.How it works
POST /api/drives/[driveId]/members/invite) now accepts{ email, … }alongside today's{ userId, … }shape. New emails route throughcreateMagicLinkTokento bind a temp user, insert a pending member row (acceptedAt: null), and sendDriveInvitationEmailwithacceptUrl = …/api/auth/magic-link/verify?token=…&inviteDriveId=…. Existing emails fall through to today's auto-accept path. Pending re-invite check fires before the existing-user lookup, so the second invite for an already-pending email returns 409 even when the temp user is already in theuserstable.acceptedAtvia a conditional UPDATE, broadcastsmember_added, and overrides the post-verify redirect to/dashboard/{inviteDriveId}when a pending row matches. Independent discovery is automatic — pending rows accept on any sign-in (passkey, separate magic link), not just via the invitation link.pendingInviteEmailalongsideselectedUser, branches the POST payload, and shows a distinct "Invitation sent" toast for new-user invites.Sendicon) and Revoke (Trashicon) actions, both gated to owners and admins. Subscribes todrive:member_added/drive:member_removedfor this drive so a previously pending row promotes to accepted in real time.POST /api/drives/[driveId]/members/[userId]/resend) reissues a fresh magic-link token and resends the email, rate-limited to 3 attempts per (drive, userId) per 24h viacheckDistributedRateLimit. BumpsinvitedAtso the UI can show "last sent N minutes ago" without a new column.Security & UX hardening
checkDriveAccess,isMemberOfDrive,getDriveMemberUserIds, andgetDriveRecipientUserIdsnow filter onacceptedAt IS NOT NULL. Without this, a temp user that ever obtains a session would inherit drive access without ever clicking the invitation link.MAGIC_LINKdistributed rate limit as/api/auth/magic-link/send, closing the email-bombing primitive where an authenticated owner could repeatedly invite an external email and trigger unlimited emails.expiryMinutesparameter oncreateMagicLinkToken. The default 5-minute expiry remains for sign-in magic links; invitations and resends opt into the longer TTL so links don't go stale before invitees see them.users.name) is rendered in the email subject and body, with a fallback to email only when the inviter has no name on file. Avoids leaking the inviter's email and reads naturally.data.sharefor resend (a closedSecurityEventTypeenum), replacing the misleadingauthz.permission.grantedtag.sendPendingDriveInvitationEmailnow sanitizes the error message before logging.Strict email-to-userId binding is implicit: pending rows are keyed by
userId, and only the user who controls that email can authenticate as that userId. No forwarded-link risk, no token-vs-email match needed.What this is not
drive_invitationstable —acceptedAt IS NULLis the pending state, matching theconnections.statusandevent_attendees.statuspatterns already used elsewhere in the schema./api/invitations/*route namespace — everything lives under/membersbecause it's the same row in different states.Files of note
apps/web/src/app/api/drives/[driveId]/members/invite/route.ts— payload branch, rate limit, pending re-invite orderingapps/web/src/app/api/auth/magic-link/verify/route.ts— pending acceptance + redirect overrideapps/web/src/app/api/drives/[driveId]/members/[userId]/resend/route.ts— new resend routeapps/web/src/lib/repositories/drive-invite-repository.ts—findUserIdByEmail,findActivePendingMemberByEmail,findPendingMembersForUser,acceptPendingMember,bumpInvitedAt,findInviterDisplaypackages/lib/src/services/drive-member-service.ts—acceptedAt IS NOT NULLfilter on authorization helperspackages/lib/src/auth/magic-link-service.ts—expiryMinutesparameter +INVITATION_LINK_EXPIRY_MINUTESpackages/lib/src/services/notification-email-service.ts—sendPendingDriveInvitationEmail+ sanitized logapps/web/src/components/members/UserSearch.tsx,DriveMembers.tsx,MemberRow.tsxapps/web/src/app/dashboard/[driveId]/members/invite/page.tsx— pending-email state + branched submitTest plan
kind: 'added'), new-email pending creation (kind: 'invited'+ email send), 409 on duplicate pending including the temp-user-with-pending regression, lowercase + trimmed email normalization, per-email rate-limit 429?inviteDriveIdmatching, no-param independent discovery, conditional UPDATE TOCTOU race, unrelatedinviteDriveIdfall-throughbumpInvitedAtmember_addedfor this drive refetches, unrelated drive event ignoredacceptedAtfilter regression-tested via existingdrive-member-service.test.tspnpm typecheckclean,pnpm lintno new warnings/dashboard/{driveId}and member promotes to accepted section🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation