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

Arrow Redirection in Pinned Message and starred Message Modal Fails for Non-Visible Messages #871

Open
smritidoneria opened this issue Jan 11, 2025 · 36 comments
Labels
bug Something isn't working

Comments

@smritidoneria
Copy link
Contributor

Description:

The arrow redirection in the pinned message modal does not properly redirect to messages that are not currently visible in the viewport. When clicking the jump arrow for a pinned message that is outside the visible area, the expected behavior is to scroll to and highlight the message. However, this redirection fails for messages that are not loaded in the DOM due to viewport limitations.

Steps to reproduce:

1.Open the chat interface.
2.Pin a message that is not currently visible in the viewport (e.g., an older message).
3.Open the pinned message modal.
4.Click the jump arrow for the pinned message.
5.Observe that the redirection does not properly scroll to and highlight the message.

Expected behavior:

It should redirect properly.

Actual behavior:

Screen.Recording.2025-01-11.at.11.12.06.PM.mov
@smritidoneria smritidoneria added the bug Something isn't working label Jan 11, 2025
@smritidoneria
Copy link
Contributor Author

working on this!

@smritidoneria
Copy link
Contributor Author

If I click on the arrow of a pinned message that is not in the viewport and not in the DOM, how is its element being found? The element should not be in the DOM if it is not visible in the viewport.
Reference-

Screen.Recording.2025-01-11.at.11.18.08.PM.mov
Screenshot 2025-01-11 at 11 19 15 PM

this is setJumpToMessage in messageAggregator.js file

Any suggestions @Spiral-Memory , @abirc8010 , @devanshkansagra , @dhairyashiil ?

@Spiral-Memory
Copy link
Collaborator

I noticed that too, I don't remember, who exactly implemented that..

But whoever did, pls look into it

It can't jump a lot of times when the message is not in the view..

I think they were attaching some ref id to each msg at the start.. and looking for that

@abirc8010
Copy link
Contributor

Hey @Spiral-Memory @smritidoneria if the message is not in the view , then it will require to fetch the older messages , there is an open PR by @SinghaAnirban005 to load more messages maybe we can reutilize that method

@Spiral-Memory
Copy link
Collaborator

Not just that.. it's fetched.. but it's just not in the view.. still many times I've noticed it doesn't go to that msg

@smritidoneria
Copy link
Contributor Author

yaa, the message is fetched as that is why it is showing in the element. but it is not in the DOM

@smritidoneria
Copy link
Contributor Author

In RocketChat, they maintains the route (signifying the message Id )that gets pushed when the jump arrow button hits.

@Spiral-Memory
Copy link
Collaborator

Yaa but we can't do like this.. we can't change urls..

It's like an integrated app when in use

@smritidoneria
Copy link
Contributor Author

yes agreed.

maybe we can fetch all the messages using the getMessage api in the embedded Chat by setting the count perimeter.
and then by the lazy loading , we will only set 50 messages at the viewport

@Spiral-Memory
Copy link
Collaborator

@abirc8010

Can you look into it.. ig you implemented jump to message right if I'm not wrong..

@abirc8010
Copy link
Contributor

Yes @Spiral-Memory I am looking into it

@dhairyashiil
Copy link
Contributor

@Spiral-Memory, @abirc8010, @smritidoneria, I noticed this as well. If in between, there are user action messages or a quote message, then this problem occurs

@abirc8010
Copy link
Contributor

abirc8010 commented Jan 12, 2025

yaa, the message is fetched as that is why it is showing in the element. but it is not in the DOM

Hey @smritidoneria @Spiral-Memory @dhairyashiil ,

What I found is that the message is not fetched. The element you see in the log is actually from the sidebar. The getElementById function will scroll into view the first element with a matching ID.

So, if the message exists in the chat window and is fetched, it will scroll that element into view.

You can try removing setShowSidebar(false); and check the behaviour.

image

@Spiral-Memory
Copy link
Collaborator

Hey @abirc8010

That's what I'm saying.. i was testing it with web pr links..

I had a few messages in the chatwindow.. just not in the view.. but when i clicked on jump to message, it was not scrolling to that message.

@abirc8010
Copy link
Contributor

I had a few messages in the chatwindow.. just not in the view.. but when i clicked on jump to message, it was not scrolling to that message.

Hey @Spiral-Memory,
If it’s not too much trouble, could you please share a video of the issue? I’m currently unable to reproduce it on my end, and a video would help me analyze the behavior more effectively.

@Spiral-Memory
Copy link
Collaborator

@smritidoneria

Could you share him one

@smritidoneria
Copy link
Contributor Author

yaa, the message is fetched as that is why it is showing in the element. but it is not in the DOM

Hey @smritidoneria @Spiral-Memory @dhairyashiil ,

What I found is that the message is not fetched. The element you see in the log is actually from the sidebar. The getElementById function will scroll into view the first element with a matching ID.

So, if the message exists in the chat window and is fetched, it will scroll that element into view.

You can try removing setShowSidebar(false); and check the behaviour.

image

yaa, I see the element it shows is from the sidebar only. what should we do then?
do we make a function to fetch the older messages?

@smritidoneria
Copy link
Contributor Author

I had a few messages in the chatwindow.. just not in the view.. but when i clicked on jump to message, it was not scrolling to that message.

Hey @Spiral-Memory, If it’s not too much trouble, could you please share a video of the issue? I’m currently unable to reproduce it on my end, and a video would help me analyze the behavior more effectively.

hey @abirc8010 , I have attached the video in the issue itself, check the actual behaviour, kindly refer the same.

@Spiral-Memory
Copy link
Collaborator

I'll also attach one @abirc8010

@SinghaAnirban005
Copy link
Contributor

yaa, the message is fetched as that is why it is showing in the element. but it is not in the DOM

Hey @smritidoneria @Spiral-Memory @dhairyashiil ,
What I found is that the message is not fetched. The element you see in the log is actually from the sidebar. The getElementById function will scroll into view the first element with a matching ID.
So, if the message exists in the chat window and is fetched, it will scroll that element into view.
You can try removing setShowSidebar(false); and check the behaviour.
image

yaa, I see the element it shows is from the sidebar only. what should we do then? do we make a function to fetch the older messages?

Well @smritidoneria you may refer to one of my open PR's for loading of older messages , there I have made a function that handles loading of older messages

@abirc8010
Copy link
Contributor

hey @abirc8010 , I have attached the video in the issue itself, check the actual behaviour, kindly refer the same.

@smritidoneria I think you’re trying to scroll to an older message. The starred message uses an API (which is not the case for pinned messages) that returns only starred messages, instead of filtering them out from the main chat window. As a result, it’s possible to see a message in the starred section that’s not visible (not fetched) in the main chat window.

@smritidoneria
Copy link
Contributor Author

hey @abirc8010 , I have attached the video in the issue itself, check the actual behaviour, kindly refer the same.

@smritidoneria I think you’re trying to scroll to an older message. The starred message uses an API (which is not the case for pinned messages) that returns only starred messages, instead of filtering them out from the main chat window. As a result, it’s possible to see a message in the starred section that’s not visible (not fetched) in the main chat window.

@abirc8010 , that is fine. but when the user hits the jump arrow button, the window should scroll down to that message, but for this to happen, the selected message should be available in the dom which is not right now. for this, we might to load the older messages and store it in dom so that scrollintoview work properly.

@Spiral-Memory
Copy link
Collaborator

hey @abirc8010 , I have attached the video in the issue itself, check the actual behaviour, kindly refer the same.

@smritidoneria I think you’re trying to scroll to an older message. The starred message uses an API (which is not the case for pinned messages) that returns only starred messages, instead of filtering them out from the main chat window. As a result, it’s possible to see a message in the starred section that’s not visible (not fetched) in the main chat window.

@abirc8010 , that is fine. but when the user hits the jump arrow button, the window should scroll down to that message, but for this to happen, the selected message should be available in the dom which is not right now. for this, we might to load the older messages and store it in dom so that scrollintoview work properly.

For that we can use @SinghaAnirban005 code and modify it accordingly..

Anyone wants to work on that ?

@abirc8010
Copy link
Contributor

Anyone wants to work on that ?

I can work on it

@Spiral-Memory
Copy link
Collaborator

Sure @abirc8010
Looking for your PR 😌

@Spiral-Memory
Copy link
Collaborator

Also, let's shift pinned messages to api call for keeping the same behaviour there as well

Though we'll not be able to update in real time then 🥲.. What do you think, should we do that @abirc8010

@Spiral-Memory
Copy link
Collaborator

Can we do something like this ?

Fetch all pinned messages through api instead of reading it from messages..

Then whenever a pinned event is triggered, maybe add that as well.. I'm not sure about the feasibility.. but yeah

@smritidoneria
Copy link
Contributor Author

Can we do something like this ?

Fetch all pinned messages through api instead of reading it from messages..

Then whenever a pinned event is triggered, maybe add that as well.. I'm not sure about the feasibility.. but yeah

maybe, I think that can work

@abirc8010
Copy link
Contributor

maybe, I think that can work

Hey @smritidoneria could you please check if your PR #854 fixes it ?

@abirc8010
Copy link
Contributor

yeah I can see it fixes the issue

@smritidoneria
Copy link
Contributor Author

hey @abirc8010 ,I think that pr revolves around fixing the appearance of the pin messages of thread to appear on the sidebar even if the user is on the main chat interface.

@abirc8010
Copy link
Contributor

abirc8010 commented Jan 12, 2025

hey @abirc8010 ,I think that pr revolves around fixing the appearance of the pin messages of thread to appear on the sidebar even if the user is on the main chat interface.

@smritidoneria , I can see you are using RCInstance.getPinnedMessages() to fetch the pinned messages right ? (similar to starred messages) this causes the messages to be fetched from the api hence even the older messages are shown in the sidebar.

I tested it locally, and you can verify it as well.

@smritidoneria
Copy link
Contributor Author

yes @abirc8010 , that is fine, older messages do not get shown in the sidebar by my pr, but we do have the issue that if the user press the jump arrow button for the older message that is not currently in the viewport, then the window did not get scroll to that older message. this is the issue that we are having

@abirc8010
Copy link
Contributor

Also, let's shift pinned messages to api call for keeping the same behaviour there as well

@smritidoneria, I think there is some confusion here. I was referring to this issue of fetching pinned messages via an API call instead of filtering them from the main chat window messages. This can be fixed by #854, not the jump-to-message one.

@smritidoneria
Copy link
Contributor Author

@abirc8010 , yes fetching of pinned message via an API call can be fixed by pr #854. we have to look the solution for jump-to-message issue.

@abirc8010
Copy link
Contributor

Yeah , waiting for #810 to get merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants