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.

Sign up for GitHub

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

Jump to bottom

jumpToMessage jumps to bottom instead of to message if message is in different chat #4508

Closed
WofWca opened this issue Jan 18, 2025 · 2 comments · Fixed by #4510
Closed

jumpToMessage jumps to bottom instead of to message if message is in different chat #4508

WofWca opened this issue Jan 18, 2025 · 2 comments · Fixed by #4510
Assignees
@WofWca
Labels
bug Something isn't working

Comments

@WofWca
Copy link
Collaborator

WofWca commented Jan 18, 2025 •
edited
Loading

  • Operating System (Linux/Mac/Windows/iOS/Android):
  • Delta Chat Version: 1.51.0
  • Expected behavior:
  • Actual behavior:
  • Steps to reproduce the problem:
    1. Search for messages. Make sure the search returns messages from different chats
    2. Click on a search result that is from a chat that is not the currently open chat
    3. The chat will open scrolled to bottom, regardless of where the target message is.
  • Screenshots:
  • Logs:

This also applies to "reply privately" quotes, and all the other jumpToMessage calls where the chat is different.

This is because we re-creae the MessageListStore every time chatId changes:

deltachat-desktop/packages/frontend/src/stores/messagelist.ts

Lines 52 to 56 in c28d796

const store = useMemo(() => {
const store = new MessageListStore(accountId, chatId)
store.effect.loadChat()
return store
}, [accountId, chatId])

But jumpToMessage claims to support jumping to message from a different chat:

deltachat-desktop/packages/frontend/src/stores/messagelist.ts

Lines 406 to 414 in c28d796

* and reloading `messageListItems` if the message is missing from there.
* @param msgId - when `undefined`, pop the jump stack, or,
* if the stack is empty, jump to last message.
*/
jumpToMessage: this.scheduler.lockedQueuedEffect(
'scroll',
async ({
msgId: jumpToMessageId,
highlight = true,

This appears to be a regression, caused by my performance changes, which surfaced the race condition. In 1.46 the bug is not present I think, at least sometimes.
Edit: on 1.50.1 it also works fine. Maybe a React or core upgrade surfaced this?
@WofWca WofWca added the bug Something isn't working label Jan 18, 2025
@WofWca WofWca self-assigned this Jan 18, 2025
@WofWca WofWca transferred this issue from deltachat/deltachat-core-rust Jan 18, 2025
WofWca added a commit that referenced this issue Jan 18, 2025
@WofWca
fix: jumpToMessage not working sometimes
100bd20 
...when the chatId of the message is different
from the currently open chat.

Closes #4508.
This is probably related to the React upgrade,
which changed the behaviour of `useEffect`.

The issue was that we called `window.__internal_jump_to_message`
_before_ `MessageListStore` has been reinstantiated,
so we'd call it on an old instance.

I think there might still be cases where this bug appears,
but at least this makes it appear less often.
@WofWca WofWca mentioned this issue Jan 18, 2025
Merged
WofWca added a commit that referenced this issue Jan 18, 2025
@WofWca
fix: jumpToMessage not working sometimes
64b4aad 
...when the chatId of the message is different
from the currently open chat.

Closes #4508.
This is probably related to the React upgrade,
which changed the behaviour of `useEffect`.

The issue was that we called `window.__internal_jump_to_message`
_before_ `MessageListStore` has been reinstantiated,
so we'd call it on an old instance.

I think there might still be cases where this bug appears,
but at least this makes it appear less often.
WofWca added a commit that referenced this issue Jan 18, 2025
@WofWca
fix: jumpToMessage not working sometimes
7209c8f 
...when the chatId of the message is different
from the currently open chat.

Closes #4508.
This is probably related to the React upgrade,
which changed the behaviour of `useEffect`.

The issue was that we called `window.__internal_jump_to_message`
_before_ `MessageListStore` has been reinstantiated
after the change of chatId (in `useMessageList`),
so we'd call it on an old instance.

I think there might still be cases where this bug appears,
but at least this makes it appear less often.
WofWca added a commit that referenced this issue Jan 18, 2025
@WofWca
fix: jumpToMessage not working sometimes
79e93ac 
...when the chatId of the message is different
from the currently open chat.

Closes #4508.
This is probably related to the React upgrade
(#3958),
which changed the behaviour of `useEffect`.

The issue was that we called `window.__internal_jump_to_message`
_before_ `MessageListStore` has been reinstantiated
after the change of chatId (in `useMessageList`),
so we'd call it on an old instance.

I think there might still be cases where this bug appears,
but at least this makes it appear less often.
@WofWca WofWca closed this as completed in 6a656f9 Jan 20, 2025
@WofWca
Copy link
Collaborator Author

WofWca commented Jan 23, 2025

About 1/10 times this is still reproducible on 1.52.0

@WofWca WofWca reopened this Jan 23, 2025
WofWca added a commit that referenced this issue Jan 24, 2025
@WofWca
fix: improve __jump_to_message reliability
4832061 
Follow-up to 6a656f9
(#4510).
Partially addresses #4508.
Why partially? Because the bug appears to still happen sometimes.
But this commit should make it more reliable, or at least be a refactor.

This should also improve performance in some cases,
thanks to the removal of `setTimeout()`.
@WofWca WofWca mentioned this issue Jan 24, 2025
Merged
WofWca added a commit that referenced this issue Jan 24, 2025
@WofWca
fix: improve __jump_to_message reliability
4701d5d 
Follow-up to 6a656f9
(#4510).
Partially addresses #4508.
Why partially? Because the bug appears to still happen sometimes.
But this commit should make it more reliable, or at least be a refactor.

This should also improve performance in some cases,
thanks to the removal of `setTimeout()`.
WofWca added a commit that referenced this issue Jan 25, 2025
@WofWca
fix: improve __jump_to_message reliability
e8f3b2e 
Follow-up to 6a656f9
(#4510).
Partially addresses #4508.
Why partially? Because the bug appears to still happen sometimes.
But this commit should make it more reliable, or at least be a refactor.

This should also improve performance in some cases,
thanks to the removal of `setTimeout()`.
WofWca added a commit that referenced this issue Jan 25, 2025
@WofWca
fix: improve __jump_to_message reliability
6dddd28 
Follow-up to 6a656f9
(#4510).
Partially addresses #4508.
Why partially? Because the bug appears to still happen sometimes.
But this commit should make it more reliable, or at least be a refactor.

This should also improve performance in some cases,
thanks to the removal of `setTimeout()`.
WofWca added a commit that referenced this issue Jan 25, 2025
@WofWca
fix: improve __jump_to_message reliability
4d7b919 
Follow-up to 6a656f9
(#4510).
Partially addresses #4508.
Why partially? Because the bug appears to still happen sometimes.
But this commit should make it more reliable, or at least be a refactor.

This should also improve performance in some cases,
thanks to the removal of `setTimeout()`.
WofWca added a commit that referenced this issue Jan 25, 2025
@WofWca
fix: improve __jump_to_message reliability
321ef27 
Follow-up to 6a656f9
(#4510).
Partially addresses #4508.
Why partially? Because the bug appears to still happen sometimes.
But this commit should make it more reliable, or at least be a refactor.

This should also improve performance in some cases,
thanks to the removal of `setTimeout()`.
@WofWca
Copy link
Collaborator Author

WofWca commented Jan 25, 2025

OK, after #4554 and #4562 I was not able to reproduce this. Let's test for a bit more and close. Then reopen if needed.

WofWca added a commit that referenced this issue Jan 25, 2025
@WofWca
fix: jumpToMessage flashing the bottom of new chat
fb94da3 
That is, when you jump to a message in a different chat
(e.g. by clicking on a notification, or from message search,
or from a "private reply" quote),
we would momentarily show the new chat scrolled to bottom
prior to jumping to the desired message.

Why this is bad:

- We mark the visible messages as read.
  In this case that would be the last messages in the chat.
  But they weren't actually read, the user did not intend
  to scroll to them.
- Extra layout shifting: not great visually.
- Bad for performance. Let's just load the part of the chat
  that we need right away.

Related:
- #4508
- #4510
- #4554
@WofWca WofWca mentioned this issue Jan 25, 2025
Merged
WofWca added a commit that referenced this issue Jan 28, 2025
@WofWca
fix: improve __jump_to_message reliability
b052640 
Follow-up to 6a656f9
(#4510).
Partially addresses #4508.
Why partially? Because the bug appears to still happen sometimes.
But this commit should make it more reliable, or at least be a refactor.

This should also improve performance in some cases,
thanks to the removal of `setTimeout()`.
WofWca added a commit that referenced this issue Jan 29, 2025
@WofWca
fix: improve __jump_to_message reliability
c424a95 
Follow-up to 6a656f9
(#4510).
Partially addresses #4508.
Why partially? Because the bug appears to still happen sometimes.
But this commit should make it more reliable, or at least be a refactor.

This should also improve performance in some cases,
thanks to the removal of `setTimeout()`.
WofWca added a commit that referenced this issue Jan 29, 2025
@WofWca
fix: improve __jump_to_message reliability
da4b916 
Follow-up to 6a656f9
(#4510).
Partially addresses #4508.
Why partially? Because the bug appears to still happen sometimes.
But this commit should make it more reliable, or at least be a refactor.

This should also improve performance in some cases,
thanks to the removal of `setTimeout()`.
WofWca added a commit that referenced this issue Jan 31, 2025
@WofWca
fix: jumpToMessage flashing the bottom of new chat
7aafc25 
That is, when you jump to a message in a different chat
(e.g. by clicking on a notification, or from message search,
or from a "private reply" quote),
we would momentarily show the new chat scrolled to bottom
prior to jumping to the desired message.

Why this is bad:

- We mark the visible messages as read.
  In this case that would be the last messages in the chat.
  But they weren't actually read, the user did not intend
  to scroll to them.
- Extra layout shifting: not great visually.
- Bad for performance. Let's just load the part of the chat
  that we need right away.

Related:
- #4508
- #4510
- #4554
WofWca added a commit that referenced this issue Jan 31, 2025
@WofWca
fix: jumpToMessage flashing the bottom of new chat
637497c 
That is, when you jump to a message in a different chat
(e.g. by clicking on a notification, or from message search,
or from a "private reply" quote),
we would momentarily show the new chat scrolled to bottom
prior to jumping to the desired message.

Why this is bad:

- We mark the visible messages as read.
  In this case that would be the last messages in the chat.
  But they weren't actually read, the user did not intend
  to scroll to them.
- Extra layout shifting: not great visually.
- Bad for performance. Let's just load the part of the chat
  that we need right away.

Related:
- #4508
- #4510
- #4554
@WofWca WofWca closed this as completed Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@WofWca WofWca

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: jumpToMessage not working sometimes deltachat/deltachat-desktop
1 participant
@WofWca