App: [CRITICAL] Incorrect previousReportActionID for whispers from workspace invite

Problem

Inviting someone to a workspace will lead the inviter to repeatedly attempt to load whisper actions they can’t access (see the first image from Taras’ post here)

Screenshot 2024-01-25 at 10 58 07

Reproduction:

Must be tested in connection with https://github.com/Expensify/App/pull/30269

Create a workspace as User A.
Invite User B.
Navigate to the created Workspace Chat as User B.
Observe User B has a whisper message.
Send a few messagse as User B inside the workspace chat.
Navigate to Chat as User A.
Observe User A does not see any whisper messages nor have them in their Onyx data.
Look at Onyx data as User A and observe the first comment from User B has a previousReportActionID equal to the whisper message meant only for User B.
User A will see a continuous loading state because we see a “gap” and we are trying to backfill a “missing report action” (i.e. the whisper) that User A should not be able to access.

Expectation:

Continuous loading does not happen and we are able to see the CREATED action for this workspace report instead of an infinite load.

Important note: This is but one example, but we are assuming the same problem would exist anywhere a whisper exists.

Observations:

Deleted comments do not suffer the same problem because all users can still access the report action itself. Whispers can be “hidden” from the front end if the current user’s accountID is not present in the reportAction.message.whisperedTo accountID array.

Solution:

Stop filtering out whisper messages on a per user level and treat them similar to deleted report actions (i.e. everyone can access them so they pose no problems for Comment Linking).

More context here about this decision is in this Slack thread.

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01d5dec33974b22a01
Upwork Job ID: 1749864653925597184
Last Price Increase: 2024-01-23

About this issue

Original URL
State: closed
Created 5 months ago
Comments: 36 (29 by maintainers)

Most upvoted comments

Alright so, quick update here. This issue has turned out to be significantly more challenging than initially anticipated. I’m going to step out of the way for now since I already have a large project I’m working on.

This was a fun investigation though so hopefully what I have to share makes sense and can easily be picked up by someone (feel free to ask me questions if not).

My recommendation based on the initial research is that we do these things in this order:

Unwind any code that might be returning incorrect previousReportActionID e.g. there is some low level code in Report::getActionList() that has some modifiers like filterByActions and skipWhispers. Those will need to get rolled back first because whenever they are used the previousReportActionID will be incorrect.
Following that - we should update anywhere we request a list of report actions so it will not filter out whispers (or any other kinds of actions for that matter). Anytime this happens we could end up with a potentially inaccurate previousReportActionID. Maybe filtering is fine if we don’t use the previousReportActionID - but it seems like it gets returned with the actions. So, I assume it can be wrong at least sometimes.
We might also need to track down all the places where whispers are created. Sometimes these happen before the report actions list is fetched. In those cases, the requestor would get the action - but we may possibly need to also send Onyx updates to any participants of the chat (since everyone should be getting the whispers). I say “maybe” because we might be able to avoid this due to the nature of how Comment Linking works. If we fail to send an update for a whisper there will be a “gap” and then get filled in by the client with the empty whisper message and the client should mostly be unaware that this has happened.
Follow up: Once clients are storing whisper report actions that they do not own we may need to ensure that the content is not accessible to them. I don’t think this is a blocker for Comment Linking, but do think that it might require some more consideration.

I think if we do all that we should be in a pretty good spot.

marcaaron on Jan 29, 2024

I can grab this one.

marcaaron on Jan 23, 2024

Update: Web-E PR is in staging. I’m actively addressing all the feedback on the Auth PR and doing my best to have it deployed tomorrow.

cristipaval on Feb 27, 2024

This is HOLD this week, we’ll merge Web-E PR after the merge freeze

cristipaval on Feb 21, 2024

@Beamanator approved the App PR, but because perf-tests were failing, his approval was dismissed when I pushed a fix.

cristipaval on Feb 12, 2024

Ok, according to this last message on Slack, the work to be done here is:

App PR to only show whispers in the UI when they are actually targeted to the current user
Auth PR to return whispers to all users, even when they aren’t targeted to them

cristipaval on Feb 5, 2024

I assigned myself this one. I’ll start gathering all the context and read all the discussions that have already happened. If anyone else thinks they can fix this faster, let me know ASAP.

cristipaval on Feb 5, 2024

Do you need anything from me?

Thanks, I think this issue is prettymuch just awaiting an internal engineer to take it on. It’s on my radar, pending clearing my plate of some other issues I’m working on.

roryabraham on Feb 2, 2024

Hi! This was just assigned to me in a re-shuffle. As far as I can tell, I’m just along for the ride at the moment. Do you need anything from me?

johncschuster on Feb 2, 2024

Heads up I updated the description with more details about everything so it should be clearer to follow what this issue is trying to fix.

marcaaron on Feb 2, 2024

Great conversation happening in #engineering-chat for this one. Put it to a vote here - I think the right path is to start sending whispers to everyone, but it’s a big change so I’d like a bit more consensus before we dive down the 🐰 🕳️ .

marcaaron on Jan 29, 2024