feat: resend workspace invite with backoff#14654
Open
ibex088 wants to merge 8 commits intotoeverything:canaryfrom
Open
feat: resend workspace invite with backoff#14654ibex088 wants to merge 8 commits intotoeverything:canaryfrom
ibex088 wants to merge 8 commits intotoeverything:canaryfrom
Conversation
|
|
Contributor
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a GraphQL mutation to resend pending workspace member invites, implements deterministic job deduplication in the workspace service to avoid duplicate notification jobs, exposes frontend APIs/UI for resending, and extends tests and i18n for the resend flow. Returns boolean indicating enqueue success. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as Member Options UI
participant Store as Members Store
participant GQL as GraphQL Resolver
participant Service as Workspace Service
participant Queue as Job Queue
User->>UI: Click "Resend Invite"
UI->>UI: Validate email present
UI->>Store: resendMemberInvite(workspaceId, inviteId)
Store->>GQL: resendWorkspaceTeamMemberInviteMutation(workspaceId, inviteId)
GQL->>GQL: Check Workspace.Users.Manage permission
GQL->>GQL: Load invite record, verify workspaceId & status
GQL->>GQL: Load invitee user
GQL->>Service: sendInvitationNotification(inviterId, inviteId)
Service->>Service: Compute deterministic jobId (SHA-256 of normalized email)
Service->>Queue: get(jobId, 'notification.sendInvitation')
alt job exists
Queue-->>Service: job found
Service-->>GQL: return false
else job missing
Queue-->>Service: not found
Service->>Queue: add('notification.sendInvitation', payload, { jobId })
Service-->>GQL: return true
end
GQL-->>Store: Boolean result
Store-->>UI: Boolean result
UI->>UI: Show success (true) or cooldown (false) notification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
github-actions
bot
added
mod:i18n
ibex088
changed the title
![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: 3
🧹 Nitpick comments (2)
packages/frontend/i18n/src/resources/en.json (1)
1047-1047: Consider pluralized copy for second(s).
{{second}}scan read awkwardly for singular values; optional_one/_otherplural keys would improve UX text quality.Optional i18n pluralization direction
- "com.affine.payment.member.team.resendInvite.hint": "Resend in {{second}}s", + "com.affine.payment.member.team.resendInvite.hint_one": "Resend in {{count}} second", + "com.affine.payment.member.team.resendInvite.hint_other": "Resend in {{count}} seconds", - "com.affine.payment.member.team.resendInvite.cooldown.notify.message": "You can resend the invitation for {{name}} in {{second}}s", + "com.affine.payment.member.team.resendInvite.cooldown.notify.message_one": "You can resend the invitation for {{name}} in {{count}} second", + "com.affine.payment.member.team.resendInvite.cooldown.notify.message_other": "You can resend the invitation for {{name}} in {{count}} seconds"Also applies to: 1067-1067
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/i18n/src/resources/en.json` at line 1047, The key com.affine.payment.member.team.resendInvite.hint currently uses a naive "{{second}}s" suffix; change the translation to use proper pluralization (e.g., provide separate variants like com.affine.payment.member.team.resendInvite.hint_one and ..._other or use ICU plural syntax) and update the code that renders this key to pass the numeric value (e.g., second or count) so the i18n library can select singular vs. plural form; also apply the same change to the other occurrence noted (around the second reported key at the same file).packages/frontend/core/src/modules/permissions/services/members.ts (1)
36-41: Consider suppressing the ESLint Finnish notation rule for this line.The static analysis flags the
$suffix, but this appears to be a project convention for observables (consistent withpermission.isTeam$in member-option.tsx). If this is an intentional pattern, consider adding an inline disable comment.💡 Suggested inline disable
+ // eslint-disable-next-line rxjs/finnish readonly resendMemberInviteBackoff$ = LiveData.from(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/core/src/modules/permissions/services/members.ts` around lines 36 - 41, The ESLint Finnish notation rule is flagging the observable property resendMemberInviteBackoff$ (a project convention like permission.isTeam$); suppress it by adding an inline eslint-disable-next-line comment immediately above the declaration of resendMemberInviteBackoff$ (the LiveData.from(...) line that calls this.globalStateService.globalState.watch with RESEND_MEMBER_INVITE_BACKOFF_KEY) to disable the specific rule for that line so the $ suffix convention is preserved consistently with member-option.tsx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/backend/server/src/core/workspaces/resolvers/member.ts`:
- Around line 305-311: The resend logic currently allows any
non-Accepted/UnderReview status which lets states like AllocatingSeat be resent;
change it to only allow the explicit statuses that the invite flow actually
emails (match the same statuses used in inviteMembers that emit
"workspace.members.invite" — e.g. only WorkspaceMemberStatus.Pending).
Concretely, update the checks around role.status in the resend mutation so that:
if role.status === WorkspaceMemberStatus.Accepted throw AlreadyInSpace; else if
role.status !== WorkspaceMemberStatus.Pending throw InvalidInvitation; this
prevents AllocatingSeat and other non-emailed states from being resendable and
keeps behavior consistent with inviteMembers and acceptInviteById.
- Around line 318-367: The backoff update must be made atomic and fail closed:
instead of the current read/compute/write that ignores cache errors, acquire a
short-lived lock or use an atomic conditional set on the backoff key (use the
same key from getMemberResendInviteBackoffKey(workspaceId, invitee.email)) so
parallel requests cannot both succeed, and check the cache.set result (or lock
acquisition) — if the atomic set/lock or the cache write fails or returns false,
throw/return an error and do not call
this.event.emit('workspace.members.invite', ...) ; keep the rest of the logic
(computing nextAttempt, nextAllowedAt, ttl using MEMBER_RESEND_INVITE_BACKOFF_*
constants and getMemberResendInviteBackoff) but ensure cache.set is performed
atomically and its failure is treated as a fatal error before emitting the
invite.
In `@packages/frontend/core/src/modules/permissions/services/members.ts`:
- Line 71: Remove the debug console.log in the members permission service:
delete the console.log('latestStoreeeeeeeee: ', latestStore) statement inside
the members.ts implementation (where latestStore is referenced) so no debugging
output remains; if logging is required for production, replace it with the
project's logger (e.g., use the existing logging utility) at an appropriate
level instead of console.log.
---
Nitpick comments:
In `@packages/frontend/core/src/modules/permissions/services/members.ts`:
- Around line 36-41: The ESLint Finnish notation rule is flagging the observable
property resendMemberInviteBackoff$ (a project convention like
permission.isTeam$); suppress it by adding an inline eslint-disable-next-line
comment immediately above the declaration of resendMemberInviteBackoff$ (the
LiveData.from(...) line that calls this.globalStateService.globalState.watch
with RESEND_MEMBER_INVITE_BACKOFF_KEY) to disable the specific rule for that
line so the $ suffix convention is preserved consistently with
member-option.tsx.
In `@packages/frontend/i18n/src/resources/en.json`:
- Line 1047: The key com.affine.payment.member.team.resendInvite.hint currently
uses a naive "{{second}}s" suffix; change the translation to use proper
pluralization (e.g., provide separate variants like
com.affine.payment.member.team.resendInvite.hint_one and ..._other or use ICU
plural syntax) and update the code that renders this key to pass the numeric
value (e.g., second or count) so the i18n library can select singular vs. plural
form; also apply the same change to the other occurrence noted (around the
second reported key at the same file).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b08e979d-48aa-4d9e-85d0-7a4d01f2043c
📒 Files selected for processing (14)
packages/backend/server/src/__tests__/e2e/workspace/member.spec.tspackages/backend/server/src/core/workspaces/resolvers/member.tspackages/backend/server/src/core/workspaces/types.tspackages/backend/server/src/schema.gqlpackages/common/graphql/src/graphql/index.tspackages/common/graphql/src/graphql/workspace-team-resend-invite.gqlpackages/common/graphql/src/schema.tspackages/frontend/core/src/desktop/dialogs/setting/workspace-setting/members/member-option.tsxpackages/frontend/core/src/modules/permissions/index.tspackages/frontend/core/src/modules/permissions/services/members.tspackages/frontend/core/src/modules/permissions/stores/members.tspackages/frontend/i18n/src/i18n-completenesses.jsonpackages/frontend/i18n/src/i18n.gen.tspackages/frontend/i18n/src/resources/en.json
Comment on lines
+318
to
+367
| const backoff = await this.getMemberResendInviteBackoff( | ||
| workspaceId, | ||
| invitee.email | ||
| ); | ||
| const now = new Date(); | ||
| let attempt = backoff?.attempt ?? 0; | ||
|
|
||
| if ( | ||
| backoff && | ||
| now.getTime() - backoff.lastSentAt.getTime() >= | ||
| MEMBER_RESEND_INVITE_BACKOFF_RESET_AFTER_MS | ||
| ) { | ||
| attempt = 0; | ||
| } else if (backoff && now.getTime() < backoff.nextAllowedAt.getTime()) { | ||
| return { | ||
| allowed: false, | ||
| attempt, | ||
| retryAfterMs: Math.max( | ||
| backoff.nextAllowedAt.getTime() - now.getTime(), | ||
| 0 | ||
| ), | ||
| nextAllowedAt: backoff.nextAllowedAt, | ||
| }; | ||
| } | ||
|
|
||
| const nextAttempt = attempt + 1; | ||
| const retryAfterMs = Math.min( | ||
| MEMBER_RESEND_INVITE_BACKOFF_BASE_MS * 2 ** (nextAttempt - 1), | ||
| MEMBER_RESEND_INVITE_BACKOFF_MAX_MS | ||
| ); | ||
| const nextAllowedAt = new Date(now.getTime() + retryAfterMs); | ||
| const key = this.getMemberResendInviteBackoffKey( | ||
| workspaceId, | ||
| invitee.email | ||
| ); | ||
|
|
||
| await this.cache.set( | ||
| key, | ||
| { | ||
| attempt: nextAttempt, | ||
| nextAllowedAt: nextAllowedAt.toISOString(), | ||
| lastSentAt: now.toISOString(), | ||
| }, | ||
| { ttl: MEMBER_RESEND_INVITE_BACKOFF_TTL_MS } | ||
| ); | ||
|
|
||
| this.event.emit('workspace.members.invite', { | ||
| inviteId, | ||
| inviterId: me.id, | ||
| }); |
Contributor
There was a problem hiding this comment.
Make the backoff update atomic and fail closed.
This is a read/compute/write sequence with no lock, so parallel resend requests can both observe the same state and both emit an invite. It also fails open if Redis is unhealthy: Cache.get() collapses read errors to a miss and the cache.set() result is ignored, so the abuse guard can turn off instead of stopping the resend.
Suggested direction
+ const backoffKey = this.getMemberResendInviteBackoffKey(
+ workspaceId,
+ invitee.email
+ );
+ await using lock = await this.mutex.acquire(`resend-invite:${backoffKey}`);
+ if (!lock) {
+ throw new TooManyRequest();
+ }
+
const backoff = await this.getMemberResendInviteBackoff(
workspaceId,
invitee.email
);
@@
- const key = this.getMemberResendInviteBackoffKey(
- workspaceId,
- invitee.email
- );
+ const key = backoffKey;Handle a failed cache write as an error before emitting the invite as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/backend/server/src/core/workspaces/resolvers/member.ts` around lines
318 - 367, The backoff update must be made atomic and fail closed: instead of
the current read/compute/write that ignores cache errors, acquire a short-lived
lock or use an atomic conditional set on the backoff key (use the same key from
getMemberResendInviteBackoffKey(workspaceId, invitee.email)) so parallel
requests cannot both succeed, and check the cache.set result (or lock
acquisition) — if the atomic set/lock or the cache write fails or returns false,
throw/return an error and do not call
this.event.emit('workspace.members.invite', ...) ; keep the rest of the logic
(computing nextAttempt, nextAllowedAt, ttl using MEMBER_RESEND_INVITE_BACKOFF_*
constants and getMemberResendInviteBackoff) but ensure cache.set is performed
atomically and its failure is treated as a fatal error before emitting the
invite.
packages/frontend/core/src/modules/permissions/services/members.ts
Outdated
Show resolved
Hide resolved
darkskygit
reviewed
Mar 21, 2026
Comment on lines
+354
to
+362
| await this.cache.set( | ||
| key, | ||
| { | ||
| attempt: nextAttempt, | ||
| nextAllowedAt: nextAllowedAt.toISOString(), | ||
| lastSentAt: now.toISOString(), | ||
| }, | ||
| { ttl: MEMBER_RESEND_INVITE_BACKOFF_TTL_MS } | ||
| ); |
Member
There was a problem hiding this comment.
I think manually managing backoff via the cache is not a good idea.
Please consider managing the sending queue through a queue component, and only initiate a new sending event when the corresponding email address does not exist in the queue.
Contributor
Author
There was a problem hiding this comment.
hope this is what you meant
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/backend/server/src/core/workspaces/resolvers/member.ts (1)
254-260:⚠️ Potential issue | 🔴 CriticalFence resend to actual emailed invite states.
inviteMembers()only sends email invites from thePendingpath, but this mutation still allowsAllocatingSeat/NeedMoreSeat*records through. If one of those team invites is resent,acceptInviteById()will later accept it without another seat/quota check, which can bypass the allocation flow. Restrict this toWorkspaceMemberStatus.Pending(and ideallyWorkspaceMemberSource.Email).🔒 Suggested guard
- if (role.status === WorkspaceMemberStatus.UnderReview) { + if ( + role.status !== WorkspaceMemberStatus.Pending || + role.source !== WorkspaceMemberSource.Email + ) { throw new InvalidInvitation(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/server/src/core/workspaces/resolvers/member.ts` around lines 254 - 260, The resend guard is too permissive: change the checks in the invite-resend path (where role.status is inspected, in the same logic that references WorkspaceMemberStatus.Accepted and WorkspaceMemberStatus.UnderReview) so that only records with role.status === WorkspaceMemberStatus.Pending (and additionally role.source === WorkspaceMemberSource.Email) are allowed to be resent; for any other status (including AllocatingSeat / NeedMoreSeat*) or non-email sources, throw InvalidInvitation (or the existing error used for invalid resend attempts). Also keep the Accepted -> AlreadyInSpace behavior unchanged and ensure this logic aligns with inviteMembers() / acceptInviteById() flows to prevent bypassing seat allocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/backend/server/src/__tests__/mocks/queue.mock.ts`:
- Around line 9-25: The test queue mock's count() is incorrectly using the add()
stub invocation history so it overcounts when add() short-circuits on duplicate
jobId; update the count() implementation to reflect the simulated queued jobs by
returning the size of the internal store (this.jobs.size) instead of relying on
add.callCount, and apply the same change to the other count() occurrence
referenced around lines 88-91 so tests assert actual de-duplicated queue depth.
In `@packages/backend/server/src/core/workspaces/resolvers/member.ts`:
- Around line 237-271: Change the resendMemberInvite mutation to return a typed
payload instead of a bare Boolean: define a GraphQL payload type (e.g.,
ResendMemberInvitePayload) with fields like allowed: boolean, retryAfterMs?:
number, nextAllowedAt?: string and maybe success?: boolean or errorCode?:
string; update the `@Mutation` return type and resolver signature for
resendMemberInvite to return that payload; call
workspaceService.sendInvitationNotification(invokerId, inviteId), map its result
and any cooldown/queue metadata returned by workspaceService into the payload
(set allowed=true when sending is permitted, include retryAfterMs/nextAllowedAt
when cooling down or disallowed, and include success for downstream falsey
cases) and adjust any callers/tests to expect the new payload shape. Ensure all
references to resendMemberInvite and workspaceService.sendInvitationNotification
are updated accordingly.
In `@packages/backend/server/src/core/workspaces/service.ts`:
- Around line 104-137: The current sendInvitationNotification method only uses
queue.get(jobId, 'notification.sendInvitation') for de-duplication, which ties
cooldown to queue retention; change it to persist per-workspace/email resend
state (e.g., in a cache or DB table via this.cache or this.models) keyed by
workspaceId+invitee.email so you can compute allowed/retryAfterMs according to
the intended backoff policy before enqueuing; keep the existing
jobId/de-duplication (getInvitationNotificationJobId + queue.get/queue.add) as a
secondary safeguard after validating allowed/retryAfterMs from the persisted
state and updating that state when scheduling retries.
In
`@packages/frontend/core/src/desktop/dialogs/setting/workspace-setting/members/member-option.tsx`:
- Around line 81-85: The missing-email toast uses hardcoded English text in
member-option.tsx (inside the branch that calls notify.error) and bypasses
useI18n(); replace the literal 'No email found for this member' with a localized
string via the i18n helper (e.g., obtain the t function from useI18n() in the
component or accept it from props) and call notify.error with message:
t('workspace.members.noEmail') (or a similar new key), then add that key and
translations to the locale files so non-English users see a localized message.
---
Duplicate comments:
In `@packages/backend/server/src/core/workspaces/resolvers/member.ts`:
- Around line 254-260: The resend guard is too permissive: change the checks in
the invite-resend path (where role.status is inspected, in the same logic that
references WorkspaceMemberStatus.Accepted and WorkspaceMemberStatus.UnderReview)
so that only records with role.status === WorkspaceMemberStatus.Pending (and
additionally role.source === WorkspaceMemberSource.Email) are allowed to be
resent; for any other status (including AllocatingSeat / NeedMoreSeat*) or
non-email sources, throw InvalidInvitation (or the existing error used for
invalid resend attempts). Also keep the Accepted -> AlreadyInSpace behavior
unchanged and ensure this logic aligns with inviteMembers() / acceptInviteById()
flows to prevent bypassing seat allocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24e2fab5-f8ad-4316-b60b-c4d83d17b35b
📒 Files selected for processing (12)
packages/backend/server/src/__tests__/e2e/workspace/member.spec.tspackages/backend/server/src/__tests__/mocks/queue.mock.tspackages/backend/server/src/core/workspaces/resolvers/member.tspackages/backend/server/src/core/workspaces/service.tspackages/backend/server/src/schema.gqlpackages/common/graphql/src/graphql/index.tspackages/common/graphql/src/graphql/workspace-team-resend-invite.gqlpackages/common/graphql/src/schema.tspackages/frontend/core/src/desktop/dialogs/setting/workspace-setting/members/member-option.spec.tsxpackages/frontend/core/src/desktop/dialogs/setting/workspace-setting/members/member-option.tsxpackages/frontend/core/src/modules/permissions/services/members.tspackages/frontend/i18n/src/resources/en.json
✅ Files skipped from review due to trivial changes (1)
- packages/frontend/i18n/src/resources/en.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/common/graphql/src/graphql/workspace-team-resend-invite.gql
- packages/backend/server/src/tests/e2e/workspace/member.spec.ts
- packages/common/graphql/src/graphql/index.ts
- packages/common/graphql/src/schema.ts
Comment on lines
+9
to
+25
| add = Sinon.stub().callsFake( | ||
| async <Job extends JobName>( | ||
| name: Job, | ||
| payload: Jobs[Job], | ||
| opts?: { jobId?: string } | ||
| ) => { | ||
| if (opts?.jobId && this.jobs.has(opts.jobId)) { | ||
| return { id: opts.jobId, name }; | ||
| } | ||
|
|
||
| if (opts?.jobId) { | ||
| this.jobs.set(opts.jobId, { name, payload }); | ||
| } | ||
|
|
||
| return { id: opts?.jobId, name }; | ||
| } | ||
| ); |
Contributor
There was a problem hiding this comment.
Make count() reflect queued jobs after jobId de-duplication.
add() now short-circuits when a job with the same jobId already exists, but count() still counts stub invocations. Any test asserting queue depth will overcount duplicates and stop verifying the new de-duplication behavior. Please count the simulated queued jobs instead of add() call history.
Also applies to: 88-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/backend/server/src/__tests__/mocks/queue.mock.ts` around lines 9 -
25, The test queue mock's count() is incorrectly using the add() stub invocation
history so it overcounts when add() short-circuits on duplicate jobId; update
the count() implementation to reflect the simulated queued jobs by returning the
size of the internal store (this.jobs.size) instead of relying on add.callCount,
and apply the same change to the other count() occurrence referenced around
lines 88-91 so tests assert actual de-duplicated queue depth.
Comment on lines
+237
to
+271
| @Mutation(() => Boolean) | ||
| async resendMemberInvite( | ||
| @CurrentUser() me: CurrentUser, | ||
| @Args('workspaceId') workspaceId: string, | ||
| @Args('inviteId') inviteId: string | ||
| ) { | ||
| await this.ac | ||
| .user(me.id) | ||
| .workspace(workspaceId) | ||
| .assert('Workspace.Users.Manage'); | ||
|
|
||
| const role = await this.models.workspaceUser.getById(inviteId); | ||
|
|
||
| if (!role || role.workspaceId !== workspaceId) { | ||
| throw new MemberNotFoundInSpace({ spaceId: workspaceId }); | ||
| } | ||
|
|
||
| if (role.status === WorkspaceMemberStatus.Accepted) { | ||
| throw new AlreadyInSpace({ spaceId: workspaceId }); | ||
| } | ||
|
|
||
| if (role.status === WorkspaceMemberStatus.UnderReview) { | ||
| throw new InvalidInvitation(); | ||
| } | ||
|
|
||
| const invitee = await this.models.user.get(role.userId); | ||
| if (!invitee) { | ||
| throw new UserNotFound(); | ||
| } | ||
|
|
||
| return await this.workspaceService.sendInvitationNotification( | ||
| me.id, | ||
| inviteId | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Expose resend state instead of a bare boolean.
This mutation collapses “allowed”, “already queued/cooling down”, and some downstream falsey cases into the same Boolean result, and it gives the client no retryAfterMs / nextAllowedAt to drive a disabled countdown UX. Please return a typed payload here instead of forwarding the raw sendInvitationNotification() boolean.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/backend/server/src/core/workspaces/resolvers/member.ts` around lines
237 - 271, Change the resendMemberInvite mutation to return a typed payload
instead of a bare Boolean: define a GraphQL payload type (e.g.,
ResendMemberInvitePayload) with fields like allowed: boolean, retryAfterMs?:
number, nextAllowedAt?: string and maybe success?: boolean or errorCode?:
string; update the `@Mutation` return type and resolver signature for
resendMemberInvite to return that payload; call
workspaceService.sendInvitationNotification(invokerId, inviteId), map its result
and any cooldown/queue metadata returned by workspaceService into the payload
(set allowed=true when sending is permitted, include retryAfterMs/nextAllowedAt
when cooling down or disallowed, and include success for downstream falsey
cases) and adjust any callers/tests to expect the new payload shape. Ensure all
references to resendMemberInvite and workspaceService.sendInvitationNotification
are updated accordingly.
Comment on lines
104
to
+137
| async sendInvitationNotification(inviterId: string, inviteId: string) { | ||
| await this.queue.add('notification.sendInvitation', { | ||
| inviterId, | ||
| inviteId, | ||
| }); | ||
| const invite = await this.models.workspaceUser.getById(inviteId); | ||
| if (!invite) { | ||
| return false; | ||
| } | ||
|
|
||
| const invitee = await this.models.user.get(invite.userId); | ||
| if (!invitee) { | ||
| return false; | ||
| } | ||
|
|
||
| const jobId = this.getInvitationNotificationJobId( | ||
| invite.workspaceId, | ||
| invitee.email | ||
| ); | ||
| const queuedJob = await this.queue.get( | ||
| jobId, | ||
| 'notification.sendInvitation' | ||
| ); | ||
|
|
||
| if (queuedJob) { | ||
| return false; | ||
| } | ||
|
|
||
| await this.queue.add( | ||
| 'notification.sendInvitation', | ||
| { | ||
| inviterId, | ||
| inviteId, | ||
| }, | ||
| { jobId } | ||
| ); | ||
|
|
||
| return true; |
Contributor
There was a problem hiding this comment.
This is queue de-duplication, not persistent resend backoff.
The only guard here is queue.get(jobId, 'notification.sendInvitation'), so the effective block window becomes whatever the queue retains for that job after enqueue/processing, not a defined resend policy. That means cooldown length will drift with queue behavior instead of the intended backoff schedule. Persist per-workspace/email resend state in cache/DB and compute allowed / retryAfterMs from that state; keep the jobId de-duplication as a secondary safeguard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/backend/server/src/core/workspaces/service.ts` around lines 104 -
137, The current sendInvitationNotification method only uses queue.get(jobId,
'notification.sendInvitation') for de-duplication, which ties cooldown to queue
retention; change it to persist per-workspace/email resend state (e.g., in a
cache or DB table via this.cache or this.models) keyed by
workspaceId+invitee.email so you can compute allowed/retryAfterMs according to
the intended backoff policy before enqueuing; keep the existing
jobId/de-duplication (getInvitationNotificationJobId + queue.get/queue.add) as a
secondary safeguard after validating allowed/retryAfterMs from the persisted
state and updating that state when scheduling retries.
packages/frontend/core/src/desktop/dialogs/setting/workspace-setting/members/member-option.tsx
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/backend/server/src/__tests__/mocks/queue.mock.ts (1)
94-97:⚠️ Potential issue | 🟠 Major
count()still tracks stub invocations, not queued jobs.This still overcounts when
add()de-duplicates byjobId, so queue-depth assertions can pass/fail for the wrong reason.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/server/src/__tests__/mocks/queue.mock.ts` around lines 94 - 97, count() currently returns the number of times the stubbed add() was called, which overcounts when add() de-duplicates by jobId; modify the mock so add(...) records actual enqueued jobs (e.g., maintain an internal Map<JobName, Set<string>> that add pushes jobId into only when not already present) and then have count(name: JobName) return the size of that Set (or deduplicate by extracting unique call.args[0]/call.args[1].jobId if you prefer not to add state); update references to the add stub and count function accordingly so queue-depth assertions reflect unique queued jobIds rather than stub invocations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/backend/server/src/__tests__/mocks/queue.mock.ts`:
- Around line 44-47: The stubbed remove() currently always resolves undefined
which breaks WorkspaceService.sendInvitationNotification's truthiness check;
update the Sinon.stub for remove to return the boolean result of
this.jobs.delete(jobId) (or Boolean(...) around it) instead of undefined so
tests reflect success/failure correctly—locate the remove =
Sinon.stub().callsFake(async (jobId: string) => { ... }) in the mock and change
its return to the deletion result.
---
Duplicate comments:
In `@packages/backend/server/src/__tests__/mocks/queue.mock.ts`:
- Around line 94-97: count() currently returns the number of times the stubbed
add() was called, which overcounts when add() de-duplicates by jobId; modify the
mock so add(...) records actual enqueued jobs (e.g., maintain an internal
Map<JobName, Set<string>> that add pushes jobId into only when not already
present) and then have count(name: JobName) return the size of that Set (or
deduplicate by extracting unique call.args[0]/call.args[1].jobId if you prefer
not to add state); update references to the add stub and count function
accordingly so queue-depth assertions reflect unique queued jobIds rather than
stub invocations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12a725b6-6ae8-45a6-8b12-d7d7e33b96b1
📒 Files selected for processing (4)
packages/backend/server/src/__tests__/e2e/workspace/member.spec.tspackages/backend/server/src/__tests__/mocks/queue.mock.tspackages/backend/server/src/core/workspaces/resolvers/member.tspackages/backend/server/src/core/workspaces/service.ts
✅ Files skipped from review due to trivial changes (1)
- packages/backend/server/src/core/workspaces/resolvers/member.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/backend/server/src/tests/e2e/workspace/member.spec.ts
Comment on lines
+44
to
+47
| remove = Sinon.stub().callsFake(async (jobId: string) => { | ||
| this.jobs.delete(jobId); | ||
| return undefined; | ||
| }); |
Contributor
There was a problem hiding this comment.
Return a boolean from remove() to match service gating.
remove() always resolves undefined, but WorkspaceService.sendInvitationNotification uses its truthiness to decide whether re-enqueue is allowed. In tests, that makes the replace-existing-job path fail even when deletion succeeded.
Suggested fix
remove = Sinon.stub().callsFake(async (jobId: string) => {
- this.jobs.delete(jobId);
- return undefined;
+ return this.jobs.delete(jobId);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/backend/server/src/__tests__/mocks/queue.mock.ts` around lines 44 -
47, The stubbed remove() currently always resolves undefined which breaks
WorkspaceService.sendInvitationNotification's truthiness check; update the
Sinon.stub for remove to return the boolean result of this.jobs.delete(jobId)
(or Boolean(...) around it) instead of undefined so tests reflect
success/failure correctly—locate the remove = Sinon.stub().callsFake(async
(jobId: string) => { ... }) in the mock and change its return to the deletion
result.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/frontend/core/src/desktop/dialogs/setting/workspace-setting/members/member-option.tsx (1)
110-115: Remove unused interpolation arg in cooldown message payload.
secondis passed here, butcom.affine.payment.member.team.resendInvite.cooldown.notify.messagecurrently only uses{{name}}. Keeping only used fields avoids confusion.✂️ Optional cleanup
message: t.t( 'com.affine.payment.member.team.resendInvite.cooldown.notify.message', { name: inviteeName, - second: '', } ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/core/src/desktop/dialogs/setting/workspace-setting/members/member-option.tsx` around lines 110 - 115, Remove the unused interpolation arg "second" from the i18n payload in the t.t call inside member-option.tsx: locate the message construction where t.t('com.affine.payment.member.team.resendInvite.cooldown.notify.message', { name: inviteeName, second: '' }) and delete the "second" property so the payload only passes { name: inviteeName }, keeping the rest of the call and variable names (t.t and inviteeName) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/frontend/core/src/desktop/dialogs/setting/workspace-setting/members/member-option.tsx`:
- Around line 110-115: Remove the unused interpolation arg "second" from the
i18n payload in the t.t call inside member-option.tsx: locate the message
construction where
t.t('com.affine.payment.member.team.resendInvite.cooldown.notify.message', {
name: inviteeName, second: '' }) and delete the "second" property so the payload
only passes { name: inviteeName }, keeping the rest of the call and variable
names (t.t and inviteeName) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2640604-c67c-4ef4-9d50-c4ce78e440b3
📒 Files selected for processing (3)
packages/frontend/core/src/desktop/dialogs/setting/workspace-setting/members/member-option.tsxpackages/frontend/i18n/src/i18n.gen.tspackages/frontend/i18n/src/resources/en.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/frontend/i18n/src/i18n.gen.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/frontend/core/src/desktop/dialogs/setting/workspace-setting/members/member-option.tsx`:
- Around line 110-116: The notification currently passes an empty string for the
`second` interpolation (t.t(..., { name: inviteeName, second: '' })) which
yields awkward text; update the `message` construction in member-option.tsx to
either remove the `second` interpolation from the i18n call or supply a real
cooldown value from the invite/cooldown data (e.g., use the backend cooldown
field or compute a fallback like "0" when available) so `t.t` receives a
meaningful `second` value instead of ''. Ensure you update the call site where
`t.t` is invoked and adjust the i18n key or data source accordingly (refer to
`inviteeName`, the `second` param, and the `t.t` invocation).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9c4876e-915b-4eb5-9365-2553d4dff099
📒 Files selected for processing (3)
packages/frontend/core/src/desktop/dialogs/setting/workspace-setting/members/member-option.tsxpackages/frontend/i18n/src/i18n.gen.tspackages/frontend/i18n/src/resources/en.json
✅ Files skipped from review due to trivial changes (1)
- packages/frontend/i18n/src/resources/en.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/frontend/i18n/src/i18n.gen.ts
Comment on lines
+110
to
+116
| message: t.t( | ||
| 'com.affine.payment.member.team.resendInvite.cooldown.notify.message', | ||
| { | ||
| name: inviteeName, | ||
| second: '', | ||
| } | ||
| ), |
Contributor
There was a problem hiding this comment.
Empty second parameter may produce confusing notification text.
The second: '' parameter is being passed as an empty string to the cooldown notification message. If the i18n string includes a placeholder like "{name} will be resent in {second} seconds", users will see awkward text with a missing value.
Consider either:
- Removing the
{second}placeholder from the i18n string if the backend doesn't return cooldown duration - Passing an actual cooldown value if the backend provides it
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/frontend/core/src/desktop/dialogs/setting/workspace-setting/members/member-option.tsx`
around lines 110 - 116, The notification currently passes an empty string for
the `second` interpolation (t.t(..., { name: inviteeName, second: '' })) which
yields awkward text; update the `message` construction in member-option.tsx to
either remove the `second` interpolation from the i18n call or supply a real
cooldown value from the invite/cooldown data (e.g., use the backend cooldown
field or compute a fallback like "0" when available) so `t.t` receives a
meaningful `second` value instead of ''. Ensure you update the call site where
`t.t` is invoked and adjust the i18n key or data source accordingly (refer to
`inviteeName`, the `second` param, and the `t.t` invocation).
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
3 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
This PR adds a Resend invite action for pending workspace members and prevents duplicate resend emails by deduplicating invitation sends in the notification queue per workspace and invitee email.
https://www.loom.com/share/ab0e2ca3c7dc401cae2a2039bed2729b
What Changed
Backend
resendMemberInvite(workspaceId, inviteId): Boolean!.notification.sendInvitation.Frontend
Behavior
Why
Invite emails can fail or be missed, so admins need a retry path. At the same time, resend should not enqueue duplicate invitation emails for the same recipient. This PR adds a simple recovery flow while using queue-level deduplication to guard against duplicate sends.
Tests
Summary by CodeRabbit
New Features
Behavior
Localization
Tests