[fix] Re-rendering fixed while opening pinned and starred messages.

[Barrylimarti]

Brief Title

While opening starred messages, useSetMessageList hook was getting re-rendered many infinitely.

Acceptance Criteria fulfillment

• Stop the re-rendering while keeping the functionality same.

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.

[CLAassistant]

All committers have signed the CLA.

[Spiral-Memory]

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.

[Barrylimarti]

Hey @Spiral-Memory,
As far as I investigated, inside the hook while setting up the message list, apparently the messages array re-rendered everytime. I haven't really dug that deep in this but it maybe due to the component re-rendering itself due to a change or something else.
I think either way it is a good practice to memoize the dependencies and functions to prevent any case like this in future.

[Spiral-Memory]

Code LGTM !
Approving to test

[Spiral-Memory]

Hey @Spiral-Memory,
As far as I investigated, inside the hook while setting up the message list, apparently the messages array re-rendered everytime. I haven't really dug that deep in this but it maybe due to the component re-rendering itself due to a change or something else.
I think either way it is a good practice to memoize the dependencies and functions to prevent any case like this in future.

If possible, i would request you to deep dive into it to figure out the actual reason why exactly this is happening. Thanks 🙏

[Barrylimarti]

Sure will definitely check this out!

[Barrylimarti]

Hey @Spiral-Memory, I investigated this further and found that PR #628 was the breaking change.
In MessageAggregator,
When allMessages was created with [...messages, ...threadMessages], a new array reference is being created on every render. This means that even if the contents of messages and threadMessages haven't changed, allMessages will be a new object each time.
Thus, causing the useSetMessageList hook to re-render.
I tried memoizing allMessages also, and it worked but I guess doing it inside the hook is much better as it will prevent further errors like this.

[Spiral-Memory]

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!

[Barrylimarti]

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 allMessages array again even though there's no change in messages or threadMessages because of the spread operator. Thus, triggering the hook again.

I am pushing couple of more changes as a safety measure for this situation in some time. Please review and let me know! Thanks.

[Barrylimarti]

I have pushed all the changes required @Spiral-Memory. You can review it now.

[Barrylimarti]

Missed some imports and linting errors. Fixed it in the latest commit.

[devanshkansagra]

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.webm

Could you please check this out? why this is occuring

also check in your localhost

[devanshkansagra]

Same goes with starred messages as well

Screencast.from.28-10-24.11.09.19.AM.IST.webm

[Barrylimarti]

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?

[devanshkansagra]

@Spiral-Memory, can you re-approve the changes?

[devanshkansagra]

might be the latest changes were not deployed

[devanshkansagra]

yep now working well