feat(spaces): support email invites#3067
Conversation
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
67cb6d2 to
382a4b5
Compare
| .expect(409) | ||
| .expect({ | ||
| message: `${memberAddress} is already in this space or has a pending invite.`, | ||
| message: 'User is already in this space or has a pending invite.', |
There was a problem hiding this comment.
Removed this to exclude addresses and emails from logs.
382a4b5 to
0189e58
Compare
| email: string, | ||
| userRepository: Repository<DbUser>, | ||
| ): Promise<User['id'] | null> { | ||
| const normalizedEmail = email.trim().toLowerCase(); |
There was a problem hiding this comment.
We trim the email twice here, but I like this better than renaming the email parameter to normalizedEmail.
0189e58 to
aef54ad
Compare
aef54ad to
1b0aa03
Compare
|
@claude review |
|
Claude finished @LucieFaire's task —— View job Code Review: feat(spaces): support email invitesOverall a solid PR with good test coverage and clear separation of concerns. Below are the items worth addressing: Bugs / Correctness
Security
Performance
Code Quality
|
|
somehow i cannot comment on files, so here is my review:
the only caveat you have to drop
lets use faker and then just increment whatever values it provides
dont know why do we need the type here if we could just simply put:
The rest will come when GH fixes the issue of commenting on files |
|
Addressed Claude's comments in 81aa055 And @LucieFaire's in 27bcf9d |
|
@claude review again |
|
Claude finished @vseehausen's task —— View job Code Review: feat(spaces): support email invites (re-review)Previous feedback from Claude and @LucieFaire has been well addressed — the schema now uses Bugs / Correctness
Security
Code Quality
Overall the changes look solid. The union schema, rate limiting, and race condition handling in |
|
Claude finished @vseehausen's task —— View job Code Review: feat(spaces): support email invites (third review)Previous feedback has been well addressed. The code is in good shape. A few minor items remain: Code Quality
LGTM overall. The union schema, rate limiting, race condition handling, and email visibility control are all solid. |
|
@claude review again.
|
|
Claude finished @vseehausen's task —— View job Code Review: feat(spaces): support email invites (fourth review)Previous feedback has been addressed well. The discriminated union on Code Quality
LGTM. Clean implementation with good test coverage and well-structured email visibility controls. |
|
@claude review again |
|
Claude finished @vseehausen's task —— View job Code Review: feat(spaces): support email invites (fifth review)Previous feedback has been well addressed across all rounds. The Code Quality
LGTM overall. No bugs or security issues found. The implementation is solid with good test coverage and proper handling of race conditions, email visibility, and rate limiting. |
|
@claude review again |
|
Claude finished @vseehausen's task —— View job Code Review: feat(spaces): support email invites (sixth review)All prior feedback has been addressed well. The code is clean and nearly ready to merge. One remaining item: Security
LGTM otherwise. The union schemas, discriminated union types, rate limiting, race condition handling, and email visibility in the members endpoints are all solid. |
|
@claude review again |
|
Claude finished @vseehausen's task —— View job Code Review: feat(spaces): support email invites (seventh review)All prior feedback across six rounds has been well addressed. The code is clean and in good shape. One remaining item: Code Quality
LGTM overall. The implementation is solid across all layers — union schemas, discriminated union types, rate limiting, race condition handling, email visibility controls, and comprehensive test coverage are all well done. Ready to merge. |
That is by designs. Admins are allowed to view invitees' emails. |
Push the resend-invite endpoint's negative paths down to the lowest layer that can prove them: ForbiddenException (non-admin caller) and NotFoundException (no matching invitation row) on the repository unit spec, and the union/strict zod rules (both fields, neither, malformed, oversize) on a new ResendInviteDto schema spec. Keeps e2e to a single happy-path wiring smoke per the test-pyramid principle. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LucieFaire
left a comment
There was a problem hiding this comment.
I could not review it all, because there is a lot going on. In the future i would suggest to split big tickets into subtasks and tackle each topic separately, that way we can review it much quicker and less places to miss something
| type InvitationIdentifier = { | ||
| address?: Address; | ||
| email?: string; | ||
| }; | ||
|
|
||
| export type AddressInvitationIdentifier = InvitationIdentifier & | ||
| Required<Pick<InvitationIdentifier, 'address'>> & { | ||
| email?: never; | ||
| }; | ||
|
|
||
| export type EmailInvitationIdentifier = InvitationIdentifier & | ||
| Required<Pick<InvitationIdentifier, 'email'>> & { | ||
| address?: never; | ||
| }; | ||
|
|
There was a problem hiding this comment.
The InvitationIdentifier base + Required<Pick<…>> indirection is doing nothing — the & { email?: never } clause already overrides the optional field. We can simplify it:
export type InvitationIdentifier =
| { address: Address; email?: never }
| { email: string; address?: never };
| authPayload: AuthPayload; | ||
| spaceId: Space['id']; | ||
| inviteExpiresAt: Member['inviteExpiresAt']; | ||
| } & (AddressInvitationIdentifier | EmailInvitationIdentifier), |
There was a problem hiding this comment.
& InvitationIdentifier,
| if (isUniqueConstraintError(err)) { | ||
| throw new UniqueConstraintError( | ||
| `${userToInvite.address} is already in this space or has a pending invite.`, | ||
| 'User is already in this space or has a pending invite.', |
There was a problem hiding this comment.
wonder if we should add some identifier to know which user it is? Does wallet parse this error for the notification?
| const invitation: Invitation = { | ||
| userId: userIdToInvite, | ||
| spaceId: space.id, | ||
| name: userToInvite.name, | ||
| role: userToInvite.role, | ||
| status: 'INVITED', | ||
| invitedBy, | ||
| }); | ||
| inviteExpiresAt: args.inviteExpiresAt, | ||
| }; | ||
| if (userToInvite.email) { | ||
| // Avoid exposing internal user IDs for email-based invite probes. | ||
| invitation.userId = undefined; | ||
| } | ||
| invitations.push(invitation); |
There was a problem hiding this comment.
can be inlined:
invitations.push({
// Avoid exposing internal user IDs for email-based invite probes.
userId: userToInvite.email ? undefined : userIdToInvite,
spaceId: space.id,
name: userToInvite.name,
role: userToInvite.role,
status: 'INVITED',
invitedBy,
inviteExpiresAt: args.inviteExpiresAt,
});
| await this.postgresDatabaseService.transaction(async (entityManager) => { | ||
| await entityManager.update(DbMember, member.id, { | ||
| status: 'DECLINED', | ||
| inviteExpiresAt: null, |
There was a problem hiding this comment.
Its inconsistent with the migration. The method explicitly sets inviteExpiresAt: null but the migration backfills DECLINED rows (status 2) with created_at + 7 days, violating that invariant for historical data.
The migration's WHERE "status" IN (0, 2) should be WHERE "status" = 0 — only INVITED rows need backfilling. (And as a bonus, that drops the dependency on the DECLINED enum value being 2.)
| } | ||
|
|
||
| public async resendInvite( | ||
| args: Parameters<IMembersRepository['resendInvite']>[0], |
There was a problem hiding this comment.
could we extend it to actual parameters as we dont really use this notation
| const actingUserId = getAuthenticatedUserIdOrFail(args.authPayload); | ||
| const activeAdmins = await this.findActiveAdminsOrFail(args.spaceId); | ||
|
|
||
| this.assertIsActiveAdmin({ members: activeAdmins, userId: actingUserId }); |
There was a problem hiding this comment.
Fetching every admin and scanning in memory is wasteful — a single targeted findOne for the caller would be equivalent and lighter, matching the inviteUsers pattern.
| }) | ||
| .where('space_id = :spaceId', { spaceId: args.spaceId }) | ||
| .andWhere('status IN (:...statuses)', { | ||
| statuses: [MemberStatus.INVITED, MemberStatus.DECLINED], |
There was a problem hiding this comment.
why would you want to resend an invitation if it was declined already?
| if (args.email !== undefined) { | ||
| queryBuilder.andWhere( | ||
| 'user_id IN (SELECT id FROM users WHERE email = :email)', | ||
| { email: args.email.trim().toLowerCase() }, | ||
| ); | ||
| } else { | ||
| queryBuilder.andWhere( | ||
| 'user_id IN (SELECT user_id FROM wallets WHERE address = :address)', | ||
| { address: getAddress(args.address) }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
why dont we just fetch the existing member by its id and retrigger? we know that invitation is already present, so we dont need to look by diff criteria. I would propose to update the API to POST /:spaceId/members/:memberId/resend and use id to fetch and update, that will simplify this query
| const isAdmin = members.some((member) => { | ||
| return ( | ||
| member.user.id === userId && | ||
| member.status === 'ACTIVE' && | ||
| member.role === 'ADMIN' | ||
| ); | ||
| }); |
There was a problem hiding this comment.
findAuthorizedMembersOrFail already fetches the caller's row inside findActiveOrInvitedMemberOrFail for the authz check, then discards it. We then re-scan the bulk list here to recompute isAdmin, which is the exact info that row already had.
Could we maybe surface the caller's row from the repository and read isAdmin directly off it instead of .some()-ing the list? Or maybe you have other ideas?
Resolves: https://linear.app/safe-global/issue/WA-2022/email-invite-creation-endpoint
This PR adds email-based space invite creation alongside the existing wallet-address invite flow, so admins can invite people before they have a wallet connected.
Summary
Notes