Forward invitee data on the invite-to-chat mention whisper so the invite email is sent#94125
Conversation
… whisper so backend can send invite email
…e-mention-invite-email
…rding Verify reportID and inviteeEmails are forwarded to the resolve command for the INVITE resolution, and not for NOTHING or empty invitee lists. Introduce a shared buildInviteWhisperAction helper used by the new and existing whisper tests.
…e-mention-invite-email
…e-mention-invite-email # Conflicts: # tests/actions/ReportTest.ts
…e-mention-invite-email
|
@ikevin127 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
melvin-bot
Bot
requested review from
JmillsExpensify
and removed request for
a team
![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: c63b1e1d28
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (isInviteResolution && originalMessage?.inviteeEmails?.length) { | ||
| parameters.reportID = reportID; | ||
| parameters.inviteeEmails = originalMessage.inviteeEmails; |
There was a problem hiding this comment.
When a stale actionable mention whisper is resolved after one of the mentioned users has already been added, buildParticipantsInviteData() skips that accountID so we don't change their participant settings, but this sends every originalMessage.inviteeEmails to the backend. Because these new fields are what trigger the invite email, the stale already-member address can receive another room invitation; filter the emails by the parallel inviteeAccountIDs that are not already in report.participants.
Useful? React with 👍 / 👎.
When a stale actionable mention whisper is resolved after one of the mentioned users has already joined the room, only forward the emails whose accountIDs are not yet participants, so the backend does not send an already-member a duplicate room invitation. Mirrors the existing buildParticipantsInviteData filtering.
…e-mention-invite-email
…e-mention-invite-email
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
Reviewer Checklist
Screenshots/Videosweb.mp4 |
| const inviteeEmailsToInvite = originalMessage.inviteeEmails.filter((email, index) => { | ||
| const inviteeAccountID = originalMessage.inviteeAccountIDs?.at(index); | ||
| return inviteeAccountID === undefined || !(inviteeAccountID in (report?.participants ?? {})); | ||
| }); |
There was a problem hiding this comment.
This couples inviteeEmails[i] to inviteeAccountIDs[i] by position. That correspondence is never asserted anywhere, in OriginalMessage.ts:152-157 they are two independent arrays populated by the backend:
inviteeEmails: string[]; // "Emails of users that aren't members of the room"
inviteeAccountIDs: number[]; // "Account IDs of users that aren't members of the room"Why this matters: if the two arrays are ever out of order or of different lengths (e.g. the backend appends brand-new-user emails that don't yet have an account ID, so the accountIDs array is shorter than the emails array), the index mapping silently shifts. Concretely, imagine:
inviteeEmails = ['alreadyMember@x.com', 'brandNew@x.com']
inviteeAccountIDs = [200] // brand-new user has no accountID yetThen:
- index 0 → email
alreadyMember@x.com,accountID200 → correctly evaluated - index 1 → email
brandNew@x.com,inviteeAccountIDs.at(1) === undefined→ kept (correct here by luck)
But the reverse ordering breaks it:
inviteeEmails = ['brandNew@x.com', 'alreadyMember@x.com']
inviteeAccountIDs = [200]- index 0 → email
brandNew@x.compaired withaccountID200 (which is actually the other person).
If 200 is already a participant, brandNew@x.com gets dropped and never invited - reintroducing the exact bug this PR fixes, but only for a specific data ordering. This is precisely the "edge case that appears only with a particular data combination" class of bug that review is supposed to catch.
Recommendation: This is likely safe in practice (@amyevans confirmed on the issue that the BE creates an account ID even for +alias new users, so the arrays are probably parallel and equal-length), but the invariant is load-bearing and invisible. At minimum, document it and guard against length mismatch:
if (isInviteResolution && originalMessage?.inviteeEmails?.length) {
const {inviteeEmails = [], inviteeAccountIDs = []} = originalMessage;
// inviteeEmails[i] and inviteeAccountIDs[i] are parallel (same order/length) as built by the
// backend whisper. Forward only invitees not already in the room so resolving a stale whisper
// doesn't re-invite a member added in the meantime. An email with no matching accountID
// (brand-new user) is always forwarded.
const inviteeEmailsToInvite = inviteeEmails.filter((email, index) => {
const inviteeAccountID = inviteeAccountIDs.at(index);
return inviteeAccountID === undefined || !(inviteeAccountID in (report.participants ?? {}));
});
...
}And ideally add a regression test with a shorter inviteeAccountIDs array than inviteeEmails (the brand-new-user shape, which is literally the bug's scenario) - none of the four new tests exercise a length mismatch; they all pass equal-length parallel arrays ❗
There was a problem hiding this comment.
Documented the parallel-array invariant + added a length-mismatch regression test (shorter inviteeAccountIDs) in 7a6f83ee7d1; verified live the BE mints an accountID even for +alias users, so the arrays stay parallel.
| @@ -5840,6 +5840,19 @@ function resolveActionableMentionWhisper( | |||
| resolution, | |||
| }; | |||
|
|
|||
There was a problem hiding this comment.
🟡 NAB: Consistency - the filter duplicates buildParticipantsInviteData's "skip existing participants" logic
buildParticipantsInviteData (lines 5681-5694) already computes exactly which invitees are not already participants, via accountID in targetReport.participants. The new filter re-implements the same "not already in room" test independently, keyed on emails instead of account IDs, which is duplicated intent that can drift.
Why it matters: the inline comment says "matching the optimistic participant update above" - but the two are only kept in sync by hand. If someone later changes the optimistic filter (e.g. to also skip pending-delete members), the email filter won't follow.
Suggestion: have buildParticipantsInviteData (or a small shared helper) return the set of newly added account IDs, then derive emails from that set - a single source of truth for "who is actually new":
// buildParticipantsInviteData could also return the accountIDs it actually added:
return {optimistic, failure, newlyAddedAccountIDs};Not a blocker, but it removes the hand-maintained coupling.
There was a problem hiding this comment.
Reduced it in 7a6f83ee7d1 — the filter now reuses the same inviteeAccountIDs that feeds buildParticipantsInviteData; skipped the newlyAddedAccountIDs refactor as a NAB, but happy to add it if you'd prefer.
| import type {MockFetch} from '../utils/TestHelper'; | ||
| import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; | ||
| import waitForNetworkPromises from '../utils/waitForNetworkPromises'; | ||
|
|
There was a problem hiding this comment.
🟡 Test gap: optimistic/offline path isn't asserted
All four tests fully mock API.write (jest.spyOn(API, 'write').mockResolvedValue(undefined)), so they verify what params are sent but never that the optimisticData (participant addition) is applied, nor the failureData rollback.
Since this action is offline-first and the PR touches the same function that builds those optimistic participant updates, one test that lets API.write run against the mock fetch and asserts report.participants gained the invitee (and rolls back on failure) would protect the offline behavior.
Right now a regression in the optimistic branch would pass all four new tests ❗
There was a problem hiding this comment.
Added an offline test in 7a6f83ee7d1 that lets API.write run and asserts report.participants gains the new invitee while the existing member's role stays untouched.
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM - Tests well
Only got 1 high priority comment and 2 NAB that should be addressed before merge to solidify the changes here 🙌
Document the inviteeEmails/inviteeAccountIDs parallel-array invariant and reuse the shared inviteeAccountIDs variable (single source with the participant update). Add a regression test for the brand-new-user shape (shorter inviteeAccountIDs) and an assertion of the optimistic participant update.
|
✅ Changes LGTM, @amyevans on to you for final review. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 amyevans has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/amyevans in version: 9.4.29-0 🚀
|
No help site changes requiredI reviewed this PR against the help site articles under Why: This is an invisible behavioral bug fix. It forwards the room ID and invited emails on the invite-to-chat mention-whisper resolution so the invitee actually receives the welcome/invitation email — matching what the Members → Invite member path already did. There is no new feature, no new/renamed UI, no changed button labels, and no change to the documented steps. The only article that covers this flow is @wildan-m — since no help site changes are required, there is no linked docs PR to review. If you believe an article should be updated (e.g. to document the invite-to-chat whisper flow more explicitly), let me know and I'll open a draft PR. |
|
🚀 Deployed to staging by https://github.com/amyevans in version: 9.4.31-0 🚀
|
Help site review — no changes requiredI reviewed the changes in this PR against the help site articles under Why: This PR is an internal behavior fix. It forwards the room ID and invitee emails on the Invite to chat whisper resolution so the backend sends the welcome/invitation email — matching what the Members → Invite path already does. There is no new or changed UI label, tab, setting, or user-facing flow. The closest article is
These steps remain accurate. The article never documented the invitation-email delivery mechanics that this PR corrects, so it stays consistent with current behavior. No draft PR was created. @wildan-m, since no help site changes were needed, there's no linked help site PR to review. If you believe an article should explicitly call out that |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.4.31-0 🚀
Bundle Size Analysis (Sentry): |
6 participants
Explanation of Change
Inviting a non-member into a room by @-mentioning their email and then clicking Invite to chat on the resulting whisper adds them to the room but never sends them the welcome/invite email. The same person invited from the room's Members → Invite page does receive it. The two paths hand the server different data: the Members page path includes the room and the invited emails — what the server uses to send the invite email — while the invite-to-chat path sends only the whisper's action ID and the chosen resolution, leaving the server with no invitee data to email.
This change forwards the room ID and the invited emails on the invite-to-chat resolution, reusing the values the whisper already carries, so the request now matches the Members → Invite one. The fields are attached only for that resolution and only when the whisper actually carries invitee emails, so the "Do nothing" and "Invite to submit expenses" resolutions are unchanged. The backend half — which had stopped sending this email — is already fixed and on production, so supplying the invitee data from the client is the remaining piece.
Fixed Issues
$ #90141
PROPOSAL: #90141 (comment)
Tests
+aliasof your own Gmail so it has no Expensify account but still lands in your inbox (e.g. if your Gmail isyouremail@gmail.com, useyouremail+test@gmail.com) — and send the message.invited @<email>action appears).Offline tests
No offline-specific behavior. While offline, clicking Invite to chat only adds the invitee to the room optimistically; the request is sent — and the invitation email delivered — once the connection is restored.
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Kapture.2026-07-02.at.14.17.17.mp4
Android: mWeb Chrome
Kapture.2026-07-02.at.14.20.37.mp4
iOS: Native
Kapture.2026-07-02.at.06.47.47.mp4
iOS: mWeb Safari
Kapture.2026-07-02.at.06.49.13.mp4
MacOS: Chrome / Safari
Kapture.2026-07-02.at.05.36.51.mp4