search

appirio-tech / connect-app

Build your next project on Connect with the power of crowdsourcing
https://connect.topcoder.com
44 stars 139 forks source link

[$100] Lag in showing the Notifications popup #3014

Closed vikasrohit closed 5 years ago

vikasrohit commented 5 years ago

Expected behavior

Notifications popup should be almost instantly visible after user clicks on the bell icon.

Actual behavior

It takes few seconds to render the popup after one clicks on the bell icon. It might be happening for only admins because of the very long list of notifications.

Steps to reproduce the problem

Screenshot/screencast

There is visible lag.

--

Environment

vikasrohit commented 5 years ago

fyi @maxceem

maxceem commented 5 years ago

Yeah, I've also noticed that it was working quite slow. And as was mentioned by sumitdaga it looks like when we open/close this popover, the whole page is being re-rendered.

maxceem commented 5 years ago

See demo video how to see parts of the app which are re-rendered during Notification dropdown is opened https://monosnap.com/file/8kX2eQLVwxjKOCVVHHyWKcThyEAg3N. Most likely, but not for sure, these re-drawings are making this dropdown slow.

codeMinter commented 5 years ago

@maxceem plz assign me this

maxceem commented 5 years ago

sure

codeMinter commented 5 years ago

@maxceem Sorry got delayed on this, kindly give me few more hrs.

maxceem commented 5 years ago

@gurmeetb do you have any progress with this issue?

codeMinter commented 5 years ago

hey @maxceem Sorry can you open it?

maxceem commented 5 years ago

@gurmeetb yep, thanks for the update. This issue is now open for pickup.

gets0ul commented 5 years ago

Can I work on this?

maxceem commented 5 years ago

Sure @gets0ul, it's on you.

gets0ul commented 5 years ago

Got distracted, seems that I can't finish it on time so I'm releasing this. Please unassign me. Thanks.

maxceem commented 5 years ago

Thank you for update @gets0ul.

This issue is now open for pickup.

r0hit-gupta commented 5 years ago

Please assign it to me.

maxceem commented 5 years ago

done

maxceem commented 5 years ago

As PR hasn't been created during 12 hours and no comment provided, this issue is now open for pick up.

abdullatku commented 5 years ago

please assign it to me

maxceem commented 5 years ago

@abdullatku You may start working on the issues since you've commented as per our Bug Bash rules. To keep it clear I would like to mark you assigned to the issue. To do so, please, accept the invitation you should get now to this repository. Thanks.

abdullatku commented 5 years ago

Sorry I may not be able to complete this in time. pls unassign me. Thanks

maxceem commented 5 years ago

Thank you for letting me know @abdullatku.

This is issue is now open for pick up. The one who first comments on it may start working on it without waiting to be assigned.

preyankjain8 commented 5 years ago

Please assign it to me!

maxceem commented 5 years ago

@preyankjain8 It's on you since you've commented as per our Bug Bash rules. To keep it clear I would like to mark you assigned to the issue. To do so, please, accept the invitation you should get now to this repository. Thanks.

preyankjain8 commented 5 years ago

I tried but won't be able to do it in time. Please unassign me.

maxceem commented 5 years ago

Thank you for update @preyankjain8.

The prize for this issue is raised to $100 and this issue is now open for pickup.

sumitdaga commented 5 years ago

@maxceem i would like to give it a try

maxceem commented 5 years ago

Sure, it looks like you've completed another issue.

sumitdaga commented 5 years ago

@maxceem i have figured out the reason and a potential fix too ... so basically the two actions VISIT_NOTIFICATIONS and TOGGLE_NOTIFICATIONS_DROPDOWN_WEB (and i suppose TOGGLE_NOTIFICATIONS_DROPDOWN_MOBILE) change the state of lastVisited and isDropdownWebOpen ...because of these redux store changes and the components are being re-rendered ....

but we need these two properties in only one component NotificationsDropdownContainer ... so instead of maintaining these properties in redux store we can maintain these in local state of the above component ...i am working on implementing this ...let me know if you see any issue with the approach?

sumitdaga commented 5 years ago

@maxceem
i will proceed with the implementation once you confirm (at least more or less see it working) ...since it will involve many changes (i should clean up the code right? ...remove unused actions, reducer states, etc)

maxceem commented 5 years ago

Thanks for update. Generally saying using redux should be a problem. As we use redux for many other actions but they don't cause rerendering of the whole screen. Also, it would be great if we could avoid wast refactoring for fixing this issue. This is just the first thoughts from the top of my head. I'll check it more detailed tomorrow and will let you know.

sumitdaga commented 5 years ago

@maxceem
i implemented it anyway ...and it works pretty nicely
the only problem is we do sort of need lastVisited in the store to remember between route changes that we have read notifications ... what i have done for this issue is call VISIT_NOTIFICATIONS when the component is unmounted and then store it in the local state when the component is mounted here is the PR for you to check #3041

sumitdaga commented 5 years ago

Generally saying using redux should be a problem.

did you mean to say shoudln't ?

As we use redux for many other actions but they don't cause rerendering of the whole screen.

yes but in this case visitNotifications, toggleDropdown actions are changing the store object each time we open/close notificationsPopup which is causing re-render if the store.notifications is passed to the page which it is in both dashboard and project-detail page ... let me know if there is a flaw in my understanding ...

vikasrohit commented 5 years ago

@maxceem a demo video after the fix would be good here for backtracking in future.

maxceem commented 5 years ago

@vikasrohit here is a demo video after fix: https://monosnap.com/file/IQkvor5U4cJNdmIy3X0Gax09BWB1RO

sumitdaga commented 5 years ago

@maxceem i have a suggestion about this
we could store LAST_VISITED in local storage too (instead of redux store) ..so that the unread red dot can be implemented across sessions too ..now after visiting notifications if user refreshes the red dot is visible again(depending on other factors also of course)!

maxceem commented 5 years ago

Thanks for suggestion @sumitdaga.

@vikasrohit let me know if you think it would be good to keep the status of LAST_VISITED during the session, or showing a red dot on every load (depending on conditions) is the intentional behavior.

vikasrohit commented 5 years ago

Thanks @maxceem for the video.

I am not sure about the using the local storage for improving the red dot behaviour. I don't think the red dot appears again, if all notifications are marked as seen once and then there is no notification till user refreshes the page. Is behaviour different than what I described? Anyway, I think this topic requires more details discussion, so could you please create new issue for that so that we can discuss it with product managers as well.

maxceem commented 5 years ago

Is behaviour different than what I described?

@vikasrohit the logic and behavior look different from what you described. I've created a new issue with a description of the current logic https://github.com/appirio-tech/connect-app/issues/3051

vikasrohit commented 5 years ago

Much better than earlier stage, in case we need more improvements, we will create new ticket.