[#7819] fix - avoid unnecessary effects when joining/syncing already active public chat #7820
Conversation
Pull Request Checklist
|
hesterbruikman
requested review from
cammellos,
flexsurfer and
yenda
and removed request for
cammellos
Jenkins BuildsClick to see older builds (108)
|
There was a problem hiding this comment.
Nice finding, here is how I would recommend to fix it according to guidelines https://github.com/status-im/status-react/blob/develop/doc/codebase-structure-and-guidelines.md
The event :chat.ui/start-public-chat doesn't have a good name for an event, it should say what happened and not what will happen, so something like "chat.ui/public-chat-topic-pressed" for instance.
That way the event will still allow for the inclusion of the semantics that you propose, which is to navigate to the chat if it is already open.
The number one argument for doing this is that we want to keep the logic in the views as small as possible to make things more testable. You can easily write a test that shows that chat.ui/public-chat-topic-pressed returns the effects to navigate to the right chat when it already exists or create the chat and navigate to it when it doesn't, but it is harder to do so if the logic is in the view in the first place.
Also keep in mind that reagent component rerender when their argument are changing, active-chat subscription aggregates a lot of things (contacts, chats, messages..) so it will trigger a lot of unnecessary re-renders (probably not a big deal though in that case)
Keep up the good work and thank you very much for your contributions and interest in learning Clojure!
Thanks @yenda ! Totally make sense! I see what you mean. Thanks for a detailed feedback. It did feel a bit off, implementing this PR the way it is. I shall go about refactoring as you suggested. |
|
@bitsikka yes please, also note that there is other occurrences of |
Understood. This refactoring will touch a bit wider area. |
|
@yenda in the changes I just pushed I kept the event name same Maybe this thing is complecting too many concerns? 🤔 Apart from that, now it should handle all cases of checking-chat-exists(only if not, then creating it) and navigating to it. Should I still change the name? |
|
Not sure why the builds are failing. Can't see the Details. I get Access Denied when trying to see the details. |
nvm.. circular dependency |
|
@yenda fixed the |
|
example of "improvement" an inconsistency bug |
|
@bitsikka why did you use a negative predicate |
It is weird indeed - I was just following |
|
@bitsikka then you can give it a default value of true
|
nice 😄 I like that very much 👍 fix coming up |
|
done! so far so good gained a bit! thanks! |
bitsikka
changed the title
|
@bitsikka is it ready for review again? |
@annadanchenko yes |
|
@yenda is your "Request Changes" still actual? If no, can you dismiss or re-review this? |
|
one change required and good to go |
The Change you requested causes circular dependency, and I don't know yet any other way to avoid that other than the way I approached it. Do you know how else to avoid it? |
|
@yenda done! |
|
@bitsikka Looks good thanks! |
|
@churik rebased All the issues are handled in this PR I'm sorry about terse descriptions and lack of tests - will do better next time Joining already joined public chat causes never ending "Loading..." #7952
The "Never ending Loading.." part is not happening in my brief test anymore for some reason, However, still the underlaying not necessary and I should've added tests to make this more apparent - sorry about that upon re-syncing unseen message badges disappear(marked seen) without seeing the new messages #7974
new public chat started as a result of clicking a universal link is not synced across paired devices #7992
Initially, the implementation was such that this had to be in this PR. Implementation changed towards the end with @yenda's guidance, and this should then have been a separate PR, but it is not - it is in this PR as well. Sorry about that. |
100% of end-end tests have passedPassed tests (48)Click to expand
|
|
Tested on IOS / Android. Also briefly checked that can install \ join the public chat from Mac OSx build. |
…active public chat Signed-off-by: yenda <eric@status.im>
Fixes #7819 - which was created first when all repercussions were not fully known; Later #7952, #7974, #7992 were created as they were discovered.
status: ready