De-dupe network requests #40059

roryabraham · 2024-04-11T01:30:51Z

Details

This fixes a few problems:

If there are no serialized requests, DeleteComment will "conflict" with itself, causing it to not be executed.
If there is a pending AddComment request, and you try to delete it, the delete will not run.
If you are offline and try to delete a comment that you sent when you were online, it will not appear strikethrough.

Fixed Issues

$ #39432
$ #39307
PROPOSAL:

Tests / Offline Tests / QA Steps

Open a DM with someone
Send a comment
Delete it
Verify the comment deletes. Open a different report and come back - verify it does not come back.
Send another comment
Go offline
Delete the comment.
Verify that the comment appears with strikethrough
Come back online
Verify that the comment deletes. Open a different report and come back - verify it does not come back.
Go offline
(web) open the network tab in the dev tools and clear the requests
Send a comment
Delete the comment
Verify that the comment disappears (no strikethrough)
Come back online
(web) Verify that no AddComment or DeleteComment requests were sent. Open a different report and come back - verify the comment does not appear.
(web) In the network tab, click on the throttling dropdown, then add
(web) add a profile with 5-10 seconds of latency (enough time that an AddComment request will still be pending when you go to delete it)
Enable the new profile and clear the network requests
Send a comment and quickly delete it. Verify it appears and then delete soon thereafter
Verify that the comment is added and then disappears as expected.
Verify that both the AddComment and DeleteComment request are processed.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Only did screenshots on web for #urgency and because I'm close to the end of my day, and because this is a platform agnostic issue.

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

web.mov

MacOS: Desktop

melvin-bot · 2024-04-11T02:05:17Z

@hungvu193 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]

hungvu193 · 2024-04-11T02:21:32Z

~~Update: I can't reproduce anymore~~

Last testcase is failing for me:

(web) add a profile with 5-10 seconds of latency (enough time that an AddComment request will still be pending when you go to delete it)
Enable the new profile and clear the network requests
Send a comment and quickly delete it. Verify it appears and then delete soon thereafter
Verify that the comment is added and then disappears as expected.
Verify that both the AddComment and DeleteComment request are processed.

Now GetOlderActions get called repeatedly.

Screen.Recording.2024-04-11.at.09.20.34.mov

hungvu193 · 2024-04-11T02:41:20Z

Also test is failing @roryabraham

hungvu193 · 2024-04-11T02:58:04Z

OK, I can reproduce this, consistently now. We need to repeat step 1 to step 17 few times.

Test 1

Test1.mov

Test 2

Test2.mov

bernhardoj · 2024-04-11T03:02:57Z

src/libs/Network/SequentialQueue.ts

    // Add request to Persisted Requests so that it can be retried if it fails
-    PersistedRequests.save(request);
+    if (!hasConflict || !shouldIncludeCurrentRequestOnConflict) {


Because we flip the condition from shouldExcludeCurrentRequestOnConflict to shouldIncludeCurrentRequestOnConflict, we should remove the negation too.

Suggested change

if (!hasConflict || !shouldIncludeCurrentRequestOnConflict) {

if (!hasConflict || shouldIncludeCurrentRequestOnConflict) {

I think we also need to set the default value for shouldIncludeCurrentRequestOnConflict to true.

@bernhardoj applying those changes doesn't seem to fix the issue I mentioned above:

Screen.Recording.2024-04-11.at.10.29.22.mov

Oh, the suggestion is to have the name match what it does. shouldIncludeCurrentRequestOnConflict tells whether we should include the current request even if there is a conflict. If we negate it (!), then it's read as, don't include the current request when there is a conflict, but inside the if, we include the request.

I see, the issue above also happened on Staging, it doesn't relate to this PR.

I think the problem is that the name is confusing. I see why too - right now shouldIncludeCurrentRequestOnConflict=true means something like:

"if this request conflicts with other serialized requests, cancel it along with those other requests"

and shouldIncludeCurrentRequestOnConflict=false means something like:

"if this request conflicts with other serialized requests, cancel those other requests but still send this one"

I went ahead and renamed this param to shouldSkipThisRequestOnConflict, which feels more clear to me:

If hasConflict && shouldSkipThisRequestOnConflict then the request is skipped

if hasConflict && !shouldSkipThisRequestOnConflict, then the request is not skipped

bernhardoj · 2024-04-11T03:04:29Z

src/libs/actions/Report.ts

                ) as string[];
                return persistedRequests.filter((request) => conflictingCommands.includes(request.command) && request.data?.reportActionID === reportActionID);
            },
            handleConflictingRequest: () => Onyx.update(successData),
-            shouldIncludeCurrentRequest: !isDeletedParentAction,
+            shouldIncludeCurrentRequestOnConflict: !isDeletedParentAction,


Same with above, we need to remove the negation here.

Suggested change

shouldIncludeCurrentRequestOnConflict: !isDeletedParentAction,

shouldIncludeCurrentRequestOnConflict: isDeletedParentAction,

bernhardoj · 2024-04-11T08:59:30Z

src/libs/actions/Report.ts

-                    isDeletedParentAction
-                        ? [WRITE_COMMANDS.UPDATE_COMMENT]
-                        : [WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.ADD_ATTACHMENT, WRITE_COMMANDS.UPDATE_COMMENT, WRITE_COMMANDS.DELETE_COMMENT]
+                    isDeletedParentAction ? [WRITE_COMMANDS.UPDATE_COMMENT] : [WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.ADD_ATTACHMENT, WRITE_COMMANDS.UPDATE_COMMENT]


Btw, do we not want to conditionally add UPDATE_COMMENT only when isOptimisticAction is true? (just like I did in my PR)

Suggested change

isDeletedParentAction ? [WRITE_COMMANDS.UPDATE_COMMENT] : [WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.ADD_ATTACHMENT, WRITE_COMMANDS.UPDATE_COMMENT]

isDeletedParentAction ? [WRITE_COMMANDS.UPDATE_COMMENT] : [WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.ADD_ATTACHMENT, reportAction.isOptimisticAction ? WRITE_COMMANDS.UPDATE_COMMENT : '']

What will happen if we always include it?

Add a comment while online (added to server)

Go offline

Edit the comment. At this point, the persisted request contains UPDATE_COMMENT

Delete the comment.
Both edit and delete request is cancelled out, the message is optimistically deleted, but the message will reappears when reopening the chat.

shubham1206agra · 2024-04-11T09:10:26Z

tests/unit/SequentialQueueTest.ts

+
+    it('should always ignore any requests that have already been sent', async () => {
+        expect(PersistedRequests.getAll().length).toBe(1);
+        SequentialQueue.flush();


@roryabraham You may need to mock flush as it will never resolve in tests.

shubham1206agra · 2024-04-11T09:17:30Z

@roryabraham Can you add an automated test?

Create an AddComment with a 5-second response time, which has been started processing.
After 1 second, an EditComment API call was made.
After another 1 second, a DeleteComment API call was made.

Result: Only AddComment and DeleteComment calls will be made.

shubham1206agra · 2024-04-11T09:22:45Z

@roryabraham Can you define the ideal behavior in this case?

Go offline.
Add a comment in the report.
Create a thread on the comment added in the previous step.
Add a comment on the thread.
Delete the comment added in step 2.
Delete the comment added in step 4.
Go online and observe the Network logs.

src/libs/Network/SequentialQueue.ts

roryabraham · 2024-04-11T18:44:22Z

thanks for the comments here, jumping back in

roryabraham · 2024-04-11T19:02:31Z

Also test is failing @roryabraham

I broke this when fixing lint, should be fixed now.

roryabraham · 2024-04-11T19:38:41Z

hmmm just thought of another case that's not covered correctly though:

Create a comment (online)
Go offline
Edit the comment
Delete the comment
Come back online

In this case we'd want the UpdateComment request to be skipped and replaced by the DeleteComment request. So we'd want shouldSkipThisRequestOnConflict to be false.

But in this case:

Go offline
Create a comment
Delete the comment
Come back online

we'd want both the AddComment and DeleteComment requests to be skipped. So we'd want shouldSkipThisRequestOnConflict to be true.

roryabraham · 2024-04-11T23:31:46Z

I fixed that issue by making shouldSkipThisRequestOnConflict a function, and ensuring that it runs after getConflictingRequests and handleConflictingRequest. That way, you can know which request(s) conflict and use it to determine if the new request should be skipped or not.

shubham1206agra · 2024-04-12T01:38:26Z

@roryabraham We might want to add a new API in this.
PR #39007

hungvu193 · 2024-04-12T03:31:34Z

hmmm just thought of another case that's not covered correctly though:

Create a comment (online)

Go offline

Edit the comment

Delete the comment

Come back online

In this case we'd want the UpdateComment request to be skipped and replaced by the DeleteComment request. So we'd want shouldSkipThisRequestOnConflict to be false.

But in this case:

Go offline

Create a comment

Delete the comment

Come back online

we'd want both the AddComment and DeleteComment requests to be skipped. So we'd want shouldSkipThisRequestOnConflict to be true.

Screen.Recording.2024-04-12.at.10.29.41.mov

I think the first case you mentioned here is still failing. It skipped both update and delete comment API.

roryabraham · 2024-04-12T18:40:21Z

Thanks for all the reviews everyone. We've agreed that this is too risky / too low-level a PR to CP. Instead, we're going to proceed with a revert of all request de-duping features, CP that, then proceed with this PR on a less-rushed timeline, ensuring that everything works as expected and it receives full QA on staging

shubham1206agra · 2024-04-14T12:50:02Z

@roryabraham You can close this PR

roryabraham · 2024-04-14T16:01:50Z

@shubham1206agra why do you say that? We reverted the request de-duping logic completely with the intention of completing this PR and having it go through full regression tests

shubham1206agra · 2024-04-14T16:18:33Z

Oh
I thought you were going to open a new PR to do that.

shubham1206agra · 2024-04-29T06:35:30Z

@roryabraham Can we start again to push this PR through?

# Conflicts: # src/libs/actions/PersistedRequests.ts # src/libs/actions/Report.ts # src/types/onyx/Request.ts

roryabraham · 2024-09-17T18:56:52Z

Closing in favor of #47913

roryabraham added 7 commits April 10, 2024 16:49

Add SequentialQueue tests

f6214f7

Move implementation into SequentialQueue

1f054cf

Add test for bulkRemove

6b85393

Add test to ensure request never conflicts with itself

0bef8cd

Add correctly failing tests

631a743

Fix case where new request is cancelled out

c09c1bd

Ignore currently processing request

00adcf3

roryabraham self-assigned this Apr 11, 2024

Fix usage in deleteReportComment

1104c19

This was referenced Apr 11, 2024

Fix delete comment doesn't work #39931

Closed

[HOLD for payment 2024-10-11][$500] DeleteComment not working as expected #39432

Closed

Cleanup

61ef30c

roryabraham marked this pull request as ready for review April 11, 2024 02:05

roryabraham requested a review from a team as a code owner April 11, 2024 02:05

melvin-bot bot requested review from hungvu193 and removed request for a team April 11, 2024 02:05

fix lint

257b1aa

bernhardoj reviewed Apr 11, 2024

View reviewed changes

shubham1206agra reviewed Apr 11, 2024

View reviewed changes

hungvu193 reviewed Apr 11, 2024

View reviewed changes

src/libs/Network/SequentialQueue.ts Show resolved Hide resolved

Merge branch 'main' into Rory-FixDeleteComment

d2d054e

Fix test

22b34d2

Rename confusing param

7c25ef7

ishpaul777 mentioned this pull request Apr 11, 2024

[$250] Public room - Deleted messages aren't actually removed from the room #39350

Closed

6 tasks

Make shouldSkipThisRequestOnConflict a function

7a16bea

roryabraham mentioned this pull request Apr 12, 2024

Remove all request de-duping logic #40180

Merged

49 tasks

ishpaul777 mentioned this pull request Apr 14, 2024

[WAITING ON #40059][$250] Conversation - Deleted message reappears when reopening the conversation #39521

Closed

6 tasks

roryabraham mentioned this pull request Apr 15, 2024

Fix: deleted messages are shown until revisit page #38609

Merged

50 tasks

roryabraham added 5 commits July 12, 2024 14:34

Merge branch 'main' into Rory-FixDeleteComment

2f2afa6

# Conflicts: # src/libs/actions/PersistedRequests.ts # src/libs/actions/Report.ts # src/types/onyx/Request.ts

Add back logic in API layer

b8ddf39

Add JSDoc comment for RequestConflictResolver

79b02f2

Merge branch 'main' into Rory-FixDeleteComment

2b1133c

Merge branch 'main' into Rory-FixDeleteComment

33b9e17

roryabraham changed the title ~~Fix DeleteComment~~ De-dupe network requests Aug 21, 2024

gedu mentioned this pull request Aug 23, 2024

De-dupe ReconnectApp in the persisted requests queue #47913

Merged

50 tasks

roryabraham closed this Sep 17, 2024

roryabraham deleted the Rory-FixDeleteComment branch September 17, 2024 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

De-dupe network requests #40059

De-dupe network requests #40059

roryabraham commented Apr 11, 2024 •

edited

Loading

melvin-bot bot commented Apr 11, 2024

hungvu193 commented Apr 11, 2024 •

edited

Loading

hungvu193 commented Apr 11, 2024

hungvu193 commented Apr 11, 2024

bernhardoj Apr 11, 2024

hungvu193 Apr 11, 2024 •

edited

Loading

bernhardoj Apr 11, 2024

hungvu193 Apr 11, 2024

roryabraham Apr 11, 2024

roryabraham Apr 11, 2024

bernhardoj Apr 11, 2024

bernhardoj Apr 11, 2024

shubham1206agra Apr 11, 2024

shubham1206agra commented Apr 11, 2024

shubham1206agra commented Apr 11, 2024

roryabraham commented Apr 11, 2024

roryabraham commented Apr 11, 2024

roryabraham commented Apr 11, 2024

roryabraham commented Apr 11, 2024

shubham1206agra commented Apr 12, 2024

hungvu193 commented Apr 12, 2024

roryabraham commented Apr 12, 2024

shubham1206agra commented Apr 14, 2024

roryabraham commented Apr 14, 2024

shubham1206agra commented Apr 14, 2024

shubham1206agra commented Apr 29, 2024

roryabraham commented Sep 17, 2024

	if (!hasConflict \|\| !shouldIncludeCurrentRequestOnConflict) {
	if (!hasConflict \|\| shouldIncludeCurrentRequestOnConflict) {

	shouldIncludeCurrentRequestOnConflict: !isDeletedParentAction,
	shouldIncludeCurrentRequestOnConflict: isDeletedParentAction,

	isDeletedParentAction ? [WRITE_COMMANDS.UPDATE_COMMENT] : [WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.ADD_ATTACHMENT, WRITE_COMMANDS.UPDATE_COMMENT]
	isDeletedParentAction ? [WRITE_COMMANDS.UPDATE_COMMENT] : [WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.ADD_ATTACHMENT, reportAction.isOptimisticAction ? WRITE_COMMANDS.UPDATE_COMMENT : '']

De-dupe network requests #40059

De-dupe network requests #40059

Conversation

roryabraham commented Apr 11, 2024 • edited Loading

Details

Fixed Issues

Tests / Offline Tests / QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Apr 11, 2024

hungvu193 commented Apr 11, 2024 • edited Loading

hungvu193 commented Apr 11, 2024

hungvu193 commented Apr 11, 2024

Choose a reason for hiding this comment

hungvu193 Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubham1206agra commented Apr 11, 2024

shubham1206agra commented Apr 11, 2024

roryabraham commented Apr 11, 2024

roryabraham commented Apr 11, 2024

roryabraham commented Apr 11, 2024

roryabraham commented Apr 11, 2024

shubham1206agra commented Apr 12, 2024

hungvu193 commented Apr 12, 2024

roryabraham commented Apr 12, 2024

shubham1206agra commented Apr 14, 2024

roryabraham commented Apr 14, 2024

shubham1206agra commented Apr 14, 2024

shubham1206agra commented Apr 29, 2024

roryabraham commented Sep 17, 2024

roryabraham commented Apr 11, 2024 •

edited

Loading

hungvu193 commented Apr 11, 2024 •

edited

Loading

hungvu193 Apr 11, 2024 •

edited

Loading