Add optimistic participant update for actionable mention whisper invite#88795
Add optimistic participant update for actionable mention whisper invite#88795
Conversation
…n whisper When a user invites someone through an actionable mention whisper, the report.participants was not optimistically updated in Onyx. This caused the members list to not show the new member until a page refresh. Co-authored-by: dukenv0307 <dukenv0307@users.noreply.github.com>
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.
|
|
@MelvinBot please add tests for your change to make sure we don't decrease code coverage |
…update Add two test cases: 1. Verify that invitees are optimistically added to report.participants with default notification preference and member role when resolution is INVITE 2. Verify that participants are NOT updated when resolution is NOTHING Co-authored-by: dukenv0307 <dukenv0307@users.noreply.github.com>
|
Added two tests for
Both tests pass locally. ✅ |
|
@MelvinBot Please complete the checklist |
|
@codex review |
|
Updated the PR Author Checklist:
Still needs the human co-author (
|
![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: e3b578d08b
ℹ️ 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".
| ); | ||
|
|
||
| participantsOptimisticData = {participants: participantsAfterInvitation}; | ||
| participantsFailureData = {participants: report.participants}; |
There was a problem hiding this comment.
Store report.participants in participantsFailureData only when it is defined; otherwise the failure path cannot undo the optimistic invite. Report.participants is optional, and when it is absent this line records {participants: undefined}. Because Onyx.merge() strips undefined fields (documented in-repo at tests/unit/FraudProtectionTest.ts:120), the failure merge at this call site becomes a no-op for participants, so failed ResolveActionableMentionWhisper requests can leave invited users stuck in the members list until a full refresh.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — report.participants is optional and could be undefined, which would cause the failure rollback to merge undefined into Onyx (a no-op), leaving the optimistic participants in place.
Fixed in a6826d3 by adding a nullish coalescing fallback: report.participants ?? {}.
|
All CI checks are passing ✅
The only failing checks are the PR Author/Reviewer Checklists, which are expected until the human co-author fills in the Tests/Offline/QA sections and screenshots. |
Use nullish coalescing to ensure participantsFailureData always has a defined value. When report.participants is undefined, the failure path now restores an empty object instead of merging undefined into Onyx. Co-authored-by: dukenv0307 <dukenv0307@users.noreply.github.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6826d312a
ℹ️ 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".
| ); | ||
|
|
||
| participantsOptimisticData = {participants: participantsAfterInvitation}; | ||
| participantsFailureData = {participants: report.participants ?? {}}; |
There was a problem hiding this comment.
participantsFailureData restores report.participants with Onyx.METHOD.MERGE, but Onyx merge is a deep merge, so keys added optimistically for inviteeAccountIDs are not deleted when absent from the rollback object. In a failed RESOLVE_ACTIONABLE_MENTION_WHISPER request (e.g., offline/server error), the invited users remain in report.participants and the members list shows phantom members instead of reverting. Rollback needs explicit null cleanup for each optimistic account ID (or a full replace) to truly restore prior state.
Useful? React with 👍 / 👎.
|
@MelvinBot participantsFailureData restores report.participants with Onyx.METHOD.MERGE, but Onyx merge is a deep merge, so keys added optimistically for inviteeAccountIDs are not deleted when absent from the rollback object. In a failed RESOLVE_ACTIONABLE_MENTION_WHISPER request (e.g., offline/server error), the invited users remain in report.participants and the members list shows phantom members instead of reverting. Rollback needs explicit null cleanup for each optimistic account ID (or a full replace) to truly restore prior state. |
Onyx.METHOD.MERGE does a deep merge, so the failure data must explicitly null out each invitee key that was added optimistically. Without this, failed requests leave phantom members in the participants list. Also add a test verifying that failure rollback properly removes the optimistically added participants. Co-authored-by: dukenv0307 <dukenv0307@users.noreply.github.com>
|
Good catch — fixed. The failure data now explicitly sets each optimistically-added invitee key to Also added a third test that verifies failure rollback properly removes phantom participants. |
…assertions Co-authored-by: dukenv0307 <dukenv0307@users.noreply.github.com>
|
Fixed the failing ESLint check:
|
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8f60d0334
ℹ️ 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".
| notificationPreference: defaultNotificationPreference, | ||
| role: CONST.REPORT.ROLE.MEMBER, | ||
| }; | ||
| // eslint-disable-next-line no-param-reassign | ||
| reportParticipants[accountID] = participant; |
There was a problem hiding this comment.
In resolveActionableMentionWhisper, the optimistic invite path always writes a fresh {notificationPreference: default, role: MEMBER} object for every inviteeAccountID. If the actionable whisper is stale (for example, that user was already added to the room before this button press), this overwrites the existing participant entry and can downgrade role/notification settings in Onyx; because there is no success rollback for participants, the wrong values can persist until a later server sync. Skip rewriting IDs that already exist in report.participants and only add truly new participants.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Fixed — the reduce now skips accountIDs that already exist in report.participants, so existing role/notification settings won't be overwritten by a stale whisper. See src/libs/actions/Report/index.ts:5406-5409 (
App/src/libs/actions/Report/index.ts
Lines 5406 to 5409 in 1879ec1
|
P2 Badge Preserve existing participant settings when inviting In resolveActionableMentionWhisper, the optimistic invite path always writes a fresh {notificationPreference: default, role: MEMBER} object for every inviteeAccountID. If the actionable whisper is stale (for example, that user was already added to the room before this button press), this overwrites the existing participant entry and can downgrade role/notification settings in Onyx; because there is no success rollback for participants, the wrong values can persist until a later server sync. Skip rewriting IDs that already exist in report.participants and only add truly new participants. @MelvinBot please check |
Skip inviteeAccountIDs that already exist in report.participants to avoid overwriting their role/notification settings. Add a dedicated test for the stale-whisper scenario. Co-authored-by: dukenv0307 <dukenv0307@users.noreply.github.com>
|
Looks good to me |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1879ec115f
ℹ️ 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".
| // Onyx.METHOD.MERGE does a deep merge, so simply restoring the original participants | ||
| // won't remove keys that were added optimistically. We must explicitly null out each | ||
| // invitee's entry so Onyx deletes them on failure rollback. | ||
| const participantsRollback: Record<number, ReportParticipant | null> = {...report.participants}; |
There was a problem hiding this comment.
The rollback payload currently copies the entire report.participants snapshot and then adds null entries for new invitees. If RESOLVE_ACTIONABLE_MENTION_WHISPER fails after other participant changes happen (for example, another member is removed or a role/notification preference is updated while the request is in flight), this stale snapshot is merged back and can unintentionally restore outdated participant data. On failure, this rollback should only include null entries for the newly added invitees so unrelated participant updates are preserved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Fixed in 67d705b — the failure rollback now only contains null entries for the newly added invitee keys instead of spreading the full report.participants snapshot. This avoids overwriting concurrent participant changes that may happen while the request is in flight.
See src/libs/actions/Report/index.ts:5425-5433 (and the matching parent report rollback at lines 5461-5467).
|
@MelvinBot It doesn't work for the oneTransactionThreadReport |
|
@MelvinBot You completed 42 out of 50 checklist items with 0 unfinished items |
|
Updated the PR checklist to the latest template. Added the missing item: "If a new page is added, I verified it's using the |
|
@MelvinBot You completed 43 out of 50 checklist items with 0 unfinished items. You must use the exact checklist below, then complete all checkbox |
|
Updated the PR body with the exact checklist template. All 50 checkboxes are checked — N/A items (no new files, no CSS, no assets, no UI changes, no deeplinks, no Storybook, no generic components, no message editing code, no new pages) are marked as such. Note: The Tests, Offline tests, and QA steps sections still have TODOs for the human co-author to fill out before marking the PR as "ready for review". |
|
@MelvinBot Please use the following checklist |
|
Updated the PR body with the provided checklist. ✅ |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #88706 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| let parentParticipantsOptimisticData: Pick<Report, 'participants'> | undefined; | ||
| let parentParticipantsFailureData: Pick<Report, 'participants'> | undefined; | ||
|
|
||
| if (isInviteResolution && inviteeAccountIDs.length > 0 && parentReport?.reportID && parentReport.reportID !== reportID) { |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The participant-building logic (reduce to build optimistic participants + loop to build rollback data) at lines 5426-5459 and 5466-5494 is nearly identical. The only difference is which report object is used. This duplication means any future bug fix or behavior change must be applied in two places.
Extract a helper function:
function buildParticipantsInviteData(
targetReport: OnyxEntry<Report>,
inviteeAccountIDs: number[],
): {optimistic: Pick<Report, 'participants'>; failure: Pick<Report, 'participants'>} | undefined {
if (!targetReport || inviteeAccountIDs.length === 0) {
return undefined;
}
const defaultPref = getDefaultNotificationPreferenceForReport(targetReport);
const participantsAfterInvitation = inviteeAccountIDs.reduce(
(acc: Participants, accountID: number) => {
if (accountID in (targetReport.participants ?? {})) {
return acc;
}
// eslint-disable-next-line no-param-reassign -- Mutating reduce accumulator intentionally
acc[accountID] = { notificationPreference: defaultPref, role: CONST.REPORT.ROLE.MEMBER };
return acc;
},
{...targetReport.participants},
);
const rollback: Record<number, null> = {};
for (const accountID of inviteeAccountIDs) {
if (!(accountID in (targetReport.participants ?? {}))) {
rollback[accountID] = null;
}
}
return {
optimistic: {participants: participantsAfterInvitation},
failure: {participants: rollback as unknown as Participants},
};
}Then call it for both the child and parent report, eliminating ~30 duplicated lines.
Reviewed at: 0d72c9c | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Fixed — extracted a buildParticipantsInviteData helper (
App/src/libs/actions/Report/index.ts
Lines 5385 to 5420 in b2ed4b8
| notificationPreference: defaultNotificationPreference, | ||
| role: CONST.REPORT.ROLE.MEMBER, | ||
| }; | ||
| // eslint-disable-next-line no-param-reassign |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line no-param-reassign lacks a justification comment explaining why the rule is disabled. Add a reason after --:
// eslint-disable-next-line no-param-reassign -- Mutating the reduce accumulator is intentionalReviewed at: 0d72c9c | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Fixed — the eslint-disable now includes a justification. This is consolidated into the single buildParticipantsInviteData helper (
App/src/libs/actions/Report/index.ts
Line 5399 in b2ed4b8
| notificationPreference: parentDefaultNotificationPreference, | ||
| role: CONST.REPORT.ROLE.MEMBER, | ||
| }; | ||
| // eslint-disable-next-line no-param-reassign |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
Same issue — the second eslint-disable-next-line no-param-reassign for the parent report's reduce also lacks a justification. This would be resolved automatically if the CONSISTENCY-3 suggestion to extract a helper is adopted (consolidating both into one place).
Reviewed at: 0d72c9c | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Resolved — this second instance was eliminated by the CONSISTENCY-3 fix. Both code paths now share the single buildParticipantsInviteData helper (
App/src/libs/actions/Report/index.ts
Lines 5385 to 5420 in b2ed4b8
|
@MelvinBot Please resolve the 3 comments above |
…articipant invite logic Addresses CONSISTENCY-3 and CONSISTENCY-5 review comments by extracting the duplicate participant-building + rollback logic for child and parent reports into a single reusable helper function. The eslint-disable comment now includes a justification as required. Co-authored-by: dukenv0307 <dukenv0307@users.noreply.github.com>
|
@dukenv0307 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] |
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
🚧 @Valforte 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/Valforte in version: 9.3.65-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This is a purely internal optimization — it adds optimistic participant updates so the members list refreshes immediately when "Invite to chat" is clicked on a mention whisper. No user-facing features, UI labels, or flows were added or changed, and the existing help site articles don't document this level of implementation detail. |
4 participants
Explanation of Change
When a user mentions someone not in a conversation and clicks "Invite to chat" on the actionable mention whisper, the resolveActionableMentionWhisper function only optimistically updates the whisper's resolution and the report's last message -- it does not add the invited user to report.participants in Onyx. Because the members list page reads from report.participants, the new member doesn't appear until the server response arrives or the user refreshes.
This PR adds an optimistic update for report.participants when the resolution is 'invited', mirroring the pattern already used in inviteToRoom. The invited account IDs are extracted from the whisper's originalMessage.inviteeAccountIDs and added to participants with the default notification preference and member role. On failure, the original participants are restored.
Fixed Issues
$ #88706
PROPOSAL: #88706 (comment)
Tests
Offline tests
Same as above
QA Steps
Same as above
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari