"MessagesList.vue:646 Message to focus not found in DOM 988" when I clicked a search result in the Nextcloud unified search prompt

jimlinntu commented 3 years ago

Steps to reproduce

Type the text I want to search in the Nextcloud unified search box
Click one of the search result

Expected behaviour

The expected behaviour should be my focus will be on the search result I click. But it turned out I was still in the same place. I think the potential reason might be the text messages are too long.

Actual behaviour

The focus was still at the same place

Talk app

Talk app version: (see apps admin page: /index.php/settings/apps) 11.3.1

Browser

Operating system: macOS

Browser name: Chrome

Browser version: 91.0.4472.164

Browser log

Server configuration

Operating system: Ubuntu/RedHat/...

Web server: Apache/Nginx

Database: MySQL/Maria/SQLite/PostgreSQL

PHP version: 7.2/7.3/7.4

Nextcloud Version: (see admin page)

List of activated apps:

``` If you have access to your command line run e.g.: sudo -u www-data php occ app:list from within your server installation folder ```

Nextcloud configuration:

``` If you have access to your command line run e.g.: sudo -u www-data php occ config:list system from within your Nextcloud installation folder ```

Server log (data/nextcloud.log)

``` Insert your server log here ```

jimlinntu commented 3 years ago

It seems related to this statement?

https://github.com/nextcloud/spreed/blob/90e0c72e69fbcf17c20c58b68eaea967b8b77bd6/src/components/MessagesList/MessagesList.vue#L426-L428

jimlinntu commented 3 years ago

No offense here. I understand this feature may be still in progress. But wouldn't you think if we cannot focus on the exact position if that text is not present on the current page, isn't the unified search engine basically not really effective in this use case?

PVince81 commented 3 years ago

@jimlinntu there's some logic missing that would need to find out how far to scroll up to load previous pages before focussing.

So if you search for a term that is present in the latest pages, the focus should work. Otherwise you'll run into this issue.

This indeed makes search not that useful currently

PVince81 commented 3 years ago

Now thinking, the current logic uses the "last unread message" as the separator between "fetch one page of old messages" and "fetch all new messages". Maybe when the hash in the URL contains a message, we could use that message id instead.

It also means that if the message in question is a million pages in the past, the "fetch all new messages" logic will take a while. We'll also need to fix the "fetch all new messages" logic to be paginated, basically just fetch 1-2 pages of new messages and stop there until the user scrolls down.

jimlinntu commented 3 years ago

Thanks for your explanation!

Can you elaborate more about what are the main challenges of "fetch 1-2 pages of messages in question and fetch more only when the use scroll down or up"? Are these challenges caused by backend database or purely frontend challenges?

PVince81 commented 3 years ago

With the current logic, if we don't fetch all messages to the bottom, it will conflict with the logic that "long polls for new messages". Should new messages appear in the chat if not scrolled to bottom ? Probably not I guess.

And possibly more potential issues, like sticky mode when scrolled to the bottom of that chat window. There might be more I'm not aware of.

It's not trivial as there's a risk to break existing logic which might need to be redone/rethought.

jimlinntu commented 3 years ago

Can you point out:

Where is the "long polls for new messages" code? (and as far as you know, how Facebook Messenger handles
What is the sticky mode and where is the code of that?

I am just interested in that.

Perhaps if given more information, I might be able to help with this feature.

If you have more time, do you mind explaining:

what's the current implementation in Nextcloud Talk different from Facebook Messenger in terms of the search feature?
Is the implementation mainly inside: https://github.com/nextcloud/spreed/tree/master/src/components/MessagesList ?

PVince81 commented 3 years ago

@jimlinntu you can start reading here: https://github.com/nextcloud/spreed/blob/master/src/components/MessagesList/MessagesList.vue#L450

this is what happens when the message list needs an update due to switching conversations

it will first fetch the old messages getOldMessages based on the indicators "first know message id" and "last known message id".

Then later below it calls lookForNewMessages which:

if the last know message id is not the last message from the chat: it will pull all messages (or up to 200) after "last known message id"
if the last known message id is the same as last message, it means all messages were fetched already, so the backend will keep the connection open in a "long poll" mode. as soon as new messages come in, it will send them back and close the connection. The long poll has a timeout.

Also, if you look more closely, you can see that lookForNewMessages will be called again and again to make sure we always have the long poll in place.

Regarding similarities of search with Facebook Messenger, I don't know, sorry.

blizzz commented 2 years ago

Suggestion, instead of loading 3 years worth of message, with click on the search result you might

show an overlay with the target message with possibility to load more earlier or later messages.
have ↑ integrated into the current message view and display a "load more" button between the messages from the result and presently visible recent messages. This bites a but with the load-on-scroll-up behaviour.

This would prevent loading a lot of stuff, memory consumption in the browser etc, and would also be more clear and manageable for the user.

nickvergessen commented 2 years ago

show an overlay with the target message with possibility to load more earlier or later messages.

Sounds like a good workaround for the time being?!

XueSheng-GIT commented 2 years ago

I'm using links to talk messages especially for references in deck. Even if the message is part of the recent discussion, focussing of the message doesn't seem to work reliable (message out of view, message not highlighted). Is this already covered by this issue or should I open a separate one?

jimlinntu commented 2 years ago

I am glad to see there is some update after a year.

Frankly, I just wanna it to behave like Facebook messenger to increase the usability of this app

nickvergessen commented 1 year ago

API endpoint for this was added in #8717

Also the frontend is now using the new endpoint when the chat has no messages loaded for that conversation yet (due to technical limitations).

SystemKeeper commented 1 year ago

Also the frontend is now using the new endpoint when the chat has no messages loaded for that conversation yet (due to technical limitations).

One way we could tackle this would be to clear the loaded messages when we try to open a conversation from search. So if messages are already loaded for that conversation and the message to display is not in the loaded messages, clear the message store for that conversation so that the context api can load it correctly. I think we would benefit quite a lot from fixing and the drawback of clearing the locally loaded messages is neglect able in contrast.

Antreesy commented 1 year ago

Since the introduced solution is merged with PR mentioned above:

If message is not presented in store yet, we're fetching it and its context https://github.com/nextcloud/spreed/pull/9897/commits/92a63328470719a160cc66d910c9a230b58caf0c :heavy_check_mark:
As long as the old logic of polling new messages exists, we keep fetching new messages, request by request, until we reach the current date (so for 3-years old context, it will be full list of messages introduced for these 3 years, which will definitely kill a browser) :warning:
One of the mentioned solutions above is close to the proposal I'd like to put there: based on reference values of conversation / context message (lastActivity, id, simple count, e.t.c.) we could estimate the limit of new messages, which will be fetched after the context loaded, and after that stop the process, disable long poll for new messages, and preserve a chat in "history mode", until it will be reloaded / rejoined / scrolled to the end (with fetching context of last known chat message)
If we're doing this within already opened chat, could be wise to purge previously loaded messages, to not have a 3-year gap between current conversation and a history part, or preserve it, but disable filling the gap automatically (example from GitHub):

Will keep an issue open until questions are solved

nextcloud / spreed