-
Notifications
You must be signed in to change notification settings - Fork 236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[fix] Re-rendering fixed while opening pinned and starred messages. #644
base: develop
Are you sure you want to change the base?
Conversation
Hey @Barrylimarti , Thanks for raising the PR. Could you please let me know why this issue is happening? I believe that even in the useEffect, the dependencies are already defined, so it should only run when the dependencies change. What specifically is causing this to run infinitely? The last time I tested, it was working fine, but since some new PRs were merged, this issue may have been introduced. I don't have my laptop with me right now, so please help me understand the issue. |
Hey @Spiral-Memory, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM !
Approving to test
If possible, i would request you to deep dive into it to figure out the actual reason why exactly this is happening. Thanks 🙏 |
Sure will definitely check this out! |
Hey @Spiral-Memory, I investigated this further and found that PR #628 was the breaking change. |
Thanks for the detailed information, @Barrylimarti . I really appreciate it! Based on the current situation, please make the necessary changes to resolve the root cause of the issue. Well, i have one more doubt, even if there is no action or render, why is new array is being created infinitely ? Is it something that it is setting some state and that state is causing a re-render ? Let me know once everything looks good to you, and I’ll review it. Thanks again! |
Yes you are right @Spiral-Memory, hook sets a state which triggers the re-render of the whole component, which in turn creates the I am pushing couple of more changes as a safety measure for this situation in some time. Please review and let me know! Thanks. |
I have pushed all the changes required @Spiral-Memory. You can review it now. |
Missed some imports and linting errors. Fixed it in the latest commit. |
Hey @Barrylimarti, The pin messages sidebar is crashing might be due to your changes in the your deployed pr: https://rocketchat.github.io/EmbeddedChat/pulls/pr-644 Screencast.from.28-10-24.10.55.54.AM.IST.webmCould you please check this out? why this is occuring also check in your localhost |
Same goes with starred messages as well Screencast.from.28-10-24.11.09.19.AM.IST.webm |
Hi @devanshkansagra, I checked the code on my localhost it is working perfectly fine. These were some import error that I missed in a commit and it was fixed in this commit. I am not sure why this is not showing in the deployed preview. Maybe re-triggering build can fix this? Can you please check? |
@Spiral-Memory, can you re-approve the changes? |
might be the latest changes were not deployed |
yep now working well |
Brief Title
While opening starred messages, useSetMessageList hook was getting re-rendered many infinitely.
Acceptance Criteria fulfillment
Fixes #642
Video/Screenshots
Previously:
error.mov
Now:
Screen.Recording.2024-10-26.at.1.57.02.PM.mov
PR Test Details
Added memoization to stop the infinite re-render cycle.
Note: The PR will be ready for live testing at https://rocketchat.github.io/EmbeddedChat/pulls/pr-<pr_number> after approval.