Skip to content
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 Loading of Older Thread Messages in Thread Message Modal #896

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

smritidoneria
Copy link
Contributor

Brief Title

This pull request addresses the issue where older thread messages are not being loaded in the thread message modal.

Acceptance Criteria fulfillment

  • Updated the getAllThreadMessages function in EmbeddedChatApi.ts to correctly fetch older thread messages.
    -Updated the ChatBody component to correctly use the getAllThreadMessages function from the useFetchChatData hook and update the state.

Fixes #895

Video/Screenshots

Screen.Recording.2025-01-14.at.5.32.42.PM.mov

PR Test Details

Note: The PR will be ready for live testing at https://rocketchat.github.io/EmbeddedChat/pulls/pr-<pr_number> after approval. Contributors are requested to replace <pr_number> with the actual PR number.

@abirc8010
Copy link
Contributor

Hey @smritidoneria, I think fetching threads only through the API once on opening the sidebar might not retrieve older messages due to the limit on the number of items it returns. Perhaps we could consider implementing a "load more threads" (by setting required offset) feature on scroll down to handle this

@smritidoneria
Copy link
Contributor Author

hey @abirc8010 , Thanks for pointing out, I will look after this.

@abirc8010
Copy link
Contributor

Hi @smritidoneria, since this issue affects not only threads but also other items in the sidebar, it might be better to address the loading of more items in the sidebar in a single PR. You can refer to a similar issue #915 for reference.

@smritidoneria
Copy link
Contributor Author

smritidoneria commented Jan 19, 2025

I was reviewing the pr and find out some issue
the query parameters are not passing in the url correctly except for the roomID
refrence-
Screenshot 2025-01-20 at 1 21 58 AM

Calling the function like this
Screenshot 2025-01-20 at 1 22 34 AM

but the url shows
Screenshot 2025-01-20 at 1 22 57 AM

and, where can I see the logs for the file EmbeddedChatApi.ts. it is neither showing in browser tools nor in terminal?

Any suggestions @abirc8010 , @Spiral-Memory , @SinghaAnirban005

@abirc8010
Copy link
Contributor

and, where can I see the logs for the file EmbeddedChatApi.ts. it is neither showing in browser tools nor in terminal?

Hey @smritidoneria you can find the logs in the browser console, did you run yarn build in the api directory after your changes?

image

Also I found this in ChatLayout.js , try to import the getAllThreadMessages function from useFetchchatData() and call it inside the useEffect

image

Btw is there any reason you’re passing rid? You can append it to the request URL as this.rid.

@SinghaAnirban005
Copy link
Contributor

@smritidoneria you probably missed out on building the api package ,
Also take care of the pagination params like offset and count

@abirc8010 i believe doing that useEffect thing in ChatLayout.js is causing the threads endpoint to be called many times such as closing of sidebar and opening of sidebar for pinned or starred messages. Thus we must avoid calling it there

@smritidoneria smritidoneria marked this pull request as draft January 20, 2025 14:31
@smritidoneria
Copy link
Contributor Author

@smritidoneria you probably missed out on building the api package , Also take care of the pagination params like offset and count

@abirc8010 i believe doing that useEffect thing in ChatLayout.js is causing the threads endpoint to be called many times such as closing of sidebar and opening of sidebar for pinned or starred messages. Thus we must avoid calling it there

yaa, actually I forgot to build the package, thanks!

@smritidoneria
Copy link
Contributor Author

hey @abirc8010 , @SinghaAnirban005 , I'm encountering an issue with the scroll event not being detected in the message aggregator component,. I have tried using many ways , I have tried giving ref to the threaded message main container that didn't work, and then the message aggregator component main box, which is also not working
kindly ignore the rest of the code, it needs to be optimized

suggestions?

@abirc8010
Copy link
Contributor

Hey @smritidoneria instead of maintaining a ref could we go with this approach ?

Additionally, I think the logic in the MessageAggregator component needs to be refactored to make it more extensible, currently it fetches all the messages from the messageStore then filters messages based on shoudRender , maybe we can pass only the required filtered messages as a prop to MessageAggregator

The functionality needs to be extendable so that a similar logic can be applied to rest of the MessageAggregators like starred or pinned

@smritidoneria
Copy link
Contributor Author

Yes, @abirc8010, we can proceed with the approach you mentioned. However, in the main ChatInterface, we are currently addressing this issue using ref. To maintain the application's consistency and serializability, we should first attempt to resolve this using ref. If we are unable to do so after further effort, we can then switch to your proposed approach.

@Spiral-Memory
Copy link
Collaborator

Spiral-Memory commented Jan 22, 2025

Hey all,

Really busy these days.. and not getting the time to review.. will review as soon as i get some time

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Older thread messages are not being loaded in the thread message modal
4 participants