Delete user MENTIONWHISPER when ADDCOMMENT is deleted#82720
Delete user MENTIONWHISPER when ADDCOMMENT is deleted#82720youssef-lr merged 21 commits intomainfrom
Conversation
0e9b46d to
11482d9
Compare
|
This does not need to hold on https://github.com/Expensify/Auth/pull/19971, but it will be a no-op till that PR makes it to production. It can be reviewed in the meantime. |
|
@jayeshmangwani 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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid.movAndroid: mWeb Chromemweb-chrome.moviOS: HybridAppios.moviOS: mWeb Safarimweb-safari.movMacOS: Chrome / Safariweb-2.movweb.mov |
|
@deetergp Changes look good 👍 Just two quick confirmations on expected behavior:
q-1.mov
q-2.mov |
|
Good catch @jayeshmangwani 👍 I've updated it so that we optimistically delete the mention whisper which should also get us number 2 for free, if I'm not mistaken. |
|
@deetergp It appears that Whisper remains visible after deleting the parent message while offline, and the options are still selectable. offline-case.mov |
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.
|
Search report actions for an unresolved ACTIONABLE_MENTION_WHISPER or ACTIONABLE_REPORT_MENTION_WHISPER and mark it deleted in the same optimistic Onyx update as the comment deletion, so the whisper disappears immediately (online and offline).
|
@deetergp , we are seeing two bugs.
Issue: whisper-bug-no-1.mov
Issue: whisper-bug-no-2.mov |
|
@jayeshmangwani Updated and retested and those bugs have been resolved. |
|
@deetergp , sorry, we still have one issue to resolve in this PR. When two invite messages are sent back-to-back, the whisper for the first message gets hidden. bug-invite-user.mov |
|
@jayeshmangwani I'm pretty sure that's by design. It's caused by code on the back end that specifically states To "fix" it would mean undoing those changes that @jasperhuangg put in place nearly two years ago. |
|
@jayeshmangwani yep that's by design, no cause for concern |
|
@deetergp @jasperhuangg Problem is that we see separate whispers on staging and main, but not on ours PR, which makes it look like a regression. That said, if we’re okay with the current PR behavior, we’re good to go. tested with the same user and report on both main and staging. Screenshots showing the differences are below. |
|
@jayeshmangwani Updated! |
jayeshmangwani
left a comment
There was a problem hiding this comment.
LGTM 🚀 let’s gooo!
|
@youssef-lr 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] |
|
🚧 @youssef-lr 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/youssef-lr in version: 9.3.42-0 🚀
Bundle Size Analysis (Sentry): |
|
Deploy Blocker #86084 was identified to be related to this PR. |
|
We've gone from none of them being deleted in production to not all of them being deleted, I wouldn't consider that to be a blocker, right? |


Explanation of Change
When a user sends a message with an
@usernamemention targeting someone who is not a member of the chat room, the backend creates anACTIONABLEMENTIONWHISPERatparentCommentID + 1. This whisper prompts the sender to either invite the mentioned user or dismiss the notification.A companion backend fix (Auth PR #19971) was made so that when the parent comment is deleted, the backend cascades the deletion to the associated whisper by setting
originalMessage.deletedon it.However, the frontend was not hiding the whisper after this cascade deletion. The root cause is that
isDeletedAction()inReportActionsUtils.tsexplicitly returnsfalseforACTIONABLEMENTIONWHISPER(since its deletion semantics differ from regular comments), bypassing the normalmessage.deletedcheck.The fix is in
isResolvedActionableWhisper(), which is already used byshouldReportActionBeVisible()to gate whisper visibility. We now treatoriginalMessage.deleted(set by the backend cascade deletion) the same asoriginalMessage.resolution(set when the user actively chooses invite/dismiss) — both cause the whisper to be hidden.Fixed Issues
$ #63874
PROPOSAL: N/A
Tests
@mentionsa user who is not a member of the room@mention(long-press → Delete)Offline tests
No offline-specific behavior changes. The fix operates on already-synced Onyx data.
QA Steps
Same as tests. Note: requires the companion Auth backend fix (PR #19971) to be deployed first.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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