Page MenuHomePhabricator
Differential D5082

[native] base on existing thread info when there is any and user is not searching
ClosedPublic
Actions

Authored by kamil on Sep 8 2022, 6:44 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 9, 6:19 PM
Unknown Object (File)
Sat, Nov 9, 3:33 PM
Unknown Object (File)
Sat, Nov 9, 7:22 AM
Unknown Object (File)
Sun, Nov 3, 2:53 AM
Unknown Object (File)
Oct 12 2024, 12:25 PM
Unknown Object (File)
Oct 10 2024, 4:16 PM
Unknown Object (File)
Oct 10 2024, 4:16 PM
Unknown Object (File)
Oct 10 2024, 4:16 PM
View All Files
Subscribers
• abosh
ashoat
atul

Details

Reviewers
tomek
atul
• abosh
ashoat
Commits
rCOMM901d0bf8d6ba: [native] base on existing thread info when there is any and user is not…
Summary

Solution for bug with missing thread content after adding user to thread.

Problem
Logic works fine when the user is searching, but after sending a message and closing the search bar we still rely on pending thread.
After adding new user from modal we still have a pending thread in ConnectedMessageListContainers state, and useExistingThreadInfoFinders logic will return the same since it's pending and what is more, the thread doesn't exist anymore, because now our thread has more users and changed (that is why content is missing).

Solution
Always when there is an existing thread and user is not searching we should update a state to not rely on pending thread anymore.
I think there aren't any contraindications to do it and it helps with the issue.

Test Plan
  1. Create new message -> add 2 users -> send message -> click on conversation title and add one more user from modal -> click Back and content should be present. (check if issue was solved)
  2. A lot of other testing scenarios connected with searching for users (friend/non-friend), adding new user, creating threads, searching and at the same time creating threads with the same users on different account etc. to make sure this change don't modify any current logic.

Diff Detail

Repository
rCOMM Comm
Branch
fix-threads
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil created this revision.Sep 8 2022, 6:44 AM
kamil held this revision as a draft.
Herald added subscribers: • abosh, atul, ashoat. · View Herald TranscriptSep 8 2022, 6:44 AM
Harbormaster completed remote builds in B12017: Diff 16499.Sep 8 2022, 6:51 AM
kamil edited the summary of this revision. (Show Details)Sep 8 2022, 7:04 AM
kamil edited the test plan for this revision. (Show Details)
kamil added reviewers: atul, • abosh, ashoat.
kamil published this revision for review.Sep 8 2022, 7:07 AM
kamil edited the summary of this revision. (Show Details)
ashoat requested changes to this revision.Sep 15 2022, 7:01 AM

Generally I should not be put on a first-pass review except for the reasons listed here. @kamil, please keep this in mind going forward.

That said, in this case it appears that the other reviewers are ignoring the diff (presumably they are overloaded) and I have a lot more context on how the systems work, so I'll take a look.

Problem
Logic works fine when the user is searching, but after sending a message and closing the search bar we still rely on pending thread.
After adding new user from modal we still have a pending thread in ConnectedMessageListContainers state, and useExistingThreadInfoFinders logic will return the same since it's pending and what is more, the thread doesn't exist anymore, because now our thread has more users and changed (that is why content is missing).

Thanks for the problem analysis – I think you're right.

Solution
Always when there is an existing thread and user is not searching we should update a state to not rely on pending thread anymore.
I think there aren't any contraindications to do it and it helps with the issue.

I'm not sure about the solution. Consider this scenario: what if the user didn't send a message, and directly navigated to the ThreadSettings and then added a new user. In your case, since the message was not sent, isSearching would still be true, correct?

Instead, I think we should consider two changes:

  1. Make sure that things that end searching/pending state (eg. hideSearch or resolveToUser) correctly set things (eg. baseThreadInfo and setParams({ threadInfo }))
  2. End searching/pending state when the user navigates to settings

Separately, I observed that there are some discrepancies between the conditions that control whether thread settings are accessible on iOS vs. Android. @kamil, can you create a task to unify the logic there?

native/chat/message-list-container.react.js
267 ↗(On Diff #16499)

I'm confused... doesn't hideSearch get called after the message is sent? Wouldn't that result in setBaseThreadInfo(threadInfo)? Maybe the threadInfo is wrong?

This revision now requires changes to proceed.Sep 15 2022, 7:01 AM
kamil added a comment.Sep 16 2022, 5:11 AM

Generally I should not be put on a first-pass review except for the reasons listed here. @kamil, please keep this in mind going forward.

Oops, sorry for that, I missed this notion note - I will keep this in mind.

I'm not sure about the solution. Consider this scenario: what if the user didn't send a message, and directly navigated to the ThreadSettings and then added a new user. In your case, since the message was not sent, isSearching would still be true, correct?

This is partially true (the fact that isSearching will be true). However, let's keep in mind that the problem occurs only when we have a pending thread in state - but in this scenario (pending thread) we disable possibility to navigate there (links you added: iOS and Android).
Maybe I am missing something but I've tried to reproduce this case and didn't succeed.

Instead, I think we should consider two changes:

  1. Make sure that things that end searching/pending state (eg. hideSearch or resolveToUser) correctly set things (eg. baseThreadInfo and setParams({ threadInfo }))
  2. End searching/pending state when the user navigates to settings

This is alternative solution to mine but will require calling existingThreadInfoFinder inside hideSearch and we won't reuse memoized threadInfo.

Separately, I observed that there are some discrepancies between the conditions that control whether thread settings are accessible on iOS vs. Android. @kamil, can you create a task to unify the logic there?

Done: https://linear.app/comm/issue/ENG-1827/unify-logic-for-controlling-thread-setting-accessibility-across

Thank you for taking a look @ashoat and sorry for bothering you with this.

native/chat/message-list-container.react.js
267 ↗(On Diff #16499)

I'm confused... doesn't hideSearch get called after the message is sent?

It is called.

Wouldn't that result in setBaseThreadInfo(threadInfo)? Maybe the threadInfo is wrong?

Yes and yes - on hideSearch call there is a pending thread inside threadInfo.
Only after hideSearch call the searching prop is set to false and after that threadInfo is recalculated by existingThreadInfoFinder (which mean on hideSearch() call we don't have actual thread information).

This is exactly what this diff should fix - fix that state by observing if searching ended and threadInfo is updated and sets the state with proper thread. (Now I think that probably I badly expressed my intentions in diff's Summary).

ashoat added a comment.Sep 16 2022, 7:55 AM

This is partially true (the fact that isSearching will be true). However, let's keep in mind that the problem occurs only when we have a pending thread in state - but in this scenario (pending thread) we disable possibility to navigate there (links you added: iOS and Android).
Maybe I am missing something but I've tried to reproduce this case and didn't succeed.

Hmm... here's what I see:

This is alternative solution to mine but will require calling existingThreadInfoFinder inside hideSearch and we won't reuse memoized threadInfo.

I'm not sure I follow why. I trust you are correct (I have not done as much thinking about this), but I think the best next step would be to see the proposed solution – then I will be able to understand those concerns better, and we can discuss potential solutions.

Thank you for taking a look @ashoat and sorry for bothering you with this.

All good – I'm probably the best reviewer for this anyways.

native/chat/message-list-container.react.js
267 ↗(On Diff #16499)

Makes sense, thanks for explaining. This code is very complicated unfortunately...

Another question for you: is the setBaseThreadInfo(threadInfo) call within hideSearch necessary now, after you've added this?

kamil updated this revision to Diff 16952.Sep 21 2022, 9:40 AM

hide search on navigation to settings, refactor logic

Harbormaster completed remote builds in B12350: Diff 16952.Sep 21 2022, 9:50 AM
kamil added a comment.Sep 21 2022, 10:05 AM
In D5082#151054, @ashoat wrote:

Hmm... here's what I see:

Ah right, I was confused because you showed a private chat where we can not add another user but overall good point because there is a scenario (let's create a group chat with 2 users, then search their names, go to settings, add another) when it won't work, so thank you!

Now I implemented the suggested by you solution:

Instead, I think we should consider two changes:

  1. Make sure that things that end searching/pending state (eg. hideSearch or resolveToUser) correctly set things (eg. baseThreadInfo and setParams({ threadInfo }))
  2. End searching/pending state when the user navigates to settings
native/chat/message-list-container.react.js
267 ↗(On Diff #16499)

Another question for you: is the setBaseThreadInfo(threadInfo) call within hideSearch necessary now, after you've added this?

Yes, it is necessary, this is why:

  1. Initially as a param we use a thread with yourself but in a pending state in this line
  2. This threadInfo lives in baseThreadInfo state.
  3. Users perform some operations - adds the user to searching and threadInfo is changing but baseThreadInfo stays the same.
  4. The message is sent so searching is closed and hideSearch is called.

Now let's suppose that setBaseThreadInfo(threadInfo) is not called and that is what happens:

  1. existingThreadInfoFinder calculate threadInfo but with search: false which leads to returning what we have in baseThreadInfo - and this is a conversation with yourself.
  2. A useEffect is triggered and params changing which navigates to privates chat with yourself

But, when setBaseThreadInfo(threadInfo) is called in state there is a pending thread but with user's IDs joined by + and the app stays in the right view.

kamil added inline comments.Sep 21 2022, 10:06 AM
native/chat/message-list-container.react.js
294 ↗(On Diff #16952)

we can use this or previous proposition:

React.useEffect(() => {
      if (!threadIsPending(threadInfo.id) && !isSearching) {
        setBaseThreadInfo(threadInfo);
      }
    }, [isSearching, threadInfo]);
ashoat requested changes to this revision.Sep 21 2022, 3:08 PM
ashoat added inline comments.
native/chat/message-list-container.react.js
270 ↗(On Diff #16952)

I would call this topRoute instead of lastRoute... lastRoute makes me think it is the route before this one

275 ↗(On Diff #16952)

Shouldn't we set threadInfo in params here too?

284–287 ↗(On Diff #16952)

What is the difference between this and threadInfo? I assume hideSearch is only called when isSearching is true, so my guess is that it would be the same...

If that's true, it's more simple to just use threadInfo, which is what was being done previously.

286 ↗(On Diff #16952)

Nit: shorthand

267 ↗(On Diff #16499)

Thanks for explaining in such a detailed way!!

This revision now requires changes to proceed.Sep 21 2022, 3:08 PM
kamil updated this revision to Diff 16977.Sep 22 2022, 1:45 AM

resolve requested changes

Harbormaster completed remote builds in B12366: Diff 16977.Sep 22 2022, 1:54 AM
kamil added inline comments.Sep 22 2022, 2:12 AM
native/chat/message-list-container.react.js
284–287 ↗(On Diff #16952)

Well threadInfo can be used here and it works but I felt uncomfortable about that, here is why and how it works now:

  1. User searching for other users, adding them, and in memoized threadInfo there is a thread in pending state.
  2. User sends a message, hideSearch is called, and because searching param didn't change yet we set baseThreadInfo as a thread in a pending state (previous major problem).
  3. hideSearch changes searching: false which will trigger threadInfo update.
  4. Now we have proper threadInfo but still a pending thread in baseThreadInfo (this is why I previously implemented this differently).
  5. User tries to add someone to the thread, goes to settings, and the code from line 269 is executed, calls setBaseThreadInfo(threadInfo); and it fixes state and makes it workable.

But overall after applying your comments code seems to work well.

ashoat requested changes to this revision.Sep 22 2022, 5:35 AM

Really close here!! Two nits and one question

native/chat/message-list-container.react.js
270–279 ↗(On Diff #16977)

Nit: we should exit early here

286 ↗(On Diff #16977)

Nit: shorthand

286 ↗(On Diff #16977)

One thing I'm realizing... the condition on line 297 should actually handle calling setParams({ threadInfo }) whenever it changes already. So by the time hideSearch is called, this should not result in any changes.

Question for you: if you remove threadInfo from the setParams calls on line 276 and line 286, does everything still work as expected?

This revision now requires changes to proceed.Sep 22 2022, 5:35 AM
kamil updated this revision to Diff 16984.Sep 22 2022, 6:07 AM

Improve code

kamil added inline comments.Sep 22 2022, 6:13 AM
native/chat/message-list-container.react.js
286 ↗(On Diff #16977)

Question for you: if you remove threadInfo from the setParams calls on line 276 and line 286, does everything still work as expected?

Yes, it will work.

Probably I made a bad decision previously when you requested this https://phab.comm.dev/D5082?id=16952#inline-34029 I thought you want to add this explicitly so I've implemented this without elaborating.

Do you think it's cleaner and better to remove threadInfo and left only searching: false?

ashoat added a comment.Sep 22 2022, 6:14 AM

Yes, it was bad advice on my part...

I think we can remove those. We only need to set threadInfo in resolveToUser since it is not the same as the memoized threadInfo.

kamil updated this revision to Diff 16989.Sep 22 2022, 6:19 AM

set threadInfo in params only when needed

Harbormaster completed remote builds in B12371: Diff 16984.Sep 22 2022, 6:19 AM
kamil added a comment.Sep 22 2022, 6:20 AM

I think we can remove those. We only need to set threadInfo in resolveToUser since it is not the same as the memoized threadInfo.

Done!

ashoat accepted this revision.Sep 22 2022, 6:21 AM

Awesome!! Great work on this

This revision is now accepted and ready to land.Sep 22 2022, 6:21 AM
Harbormaster completed remote builds in B12376: Diff 16989.Sep 22 2022, 6:31 AM
Closed by commit rCOMM901d0bf8d6ba: [native] base on existing thread info when there is any and user is not… (authored by kamil). · Explain WhySep 23 2022, 3:29 AM
This revision was automatically updated to reflect the committed changes.
kamil added a commit: rCOMM901d0bf8d6ba: [native] base on existing thread info when there is any and user is not….
atul mentioned this in D5219: [native] Unify thread settings accessibility.Sep 27 2022, 6:42 AM

Revision Contents
Changeset List

PathSize
native/
chat/
22 lines

Diff 16989

View Options

native/chat/message-list-container.react.js

Loading...