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

Refactor to remove possible races when switching account in message list / chatstore #3675

Closed
Tracked by #3671
Simon-Laux opened this issue Feb 9, 2024 · 2 comments · Fixed by #3725
Closed
Tracked by #3671
Assignees
Labels
bug Something isn't working

Comments

@Simon-Laux
Copy link
Member

          > I'm receiving an "Failed loading chat Chat#22 from database\n\nCaused by:\n Query returned no rows" error when switching accounts sometimes (roughly every 2nd time), it seems to be a race condition but I can't pin down yet where it comes from.

I think the chatstore / message list store depends too much on the global selected account id (window.__selectedAccountId).
If there is an action running and you switch accounts it might try to complete the action with the other account. so this needs a refactoring I think making sure those stores live inside of the react tree and are not reused globally if possible.
But this is too much to do inside of this pr.

Originally posted by @Simon-Laux in #3621 (comment)

@Simon-Laux Simon-Laux changed the title Refactor to remove possible races when switching account in message list Refactor to remove possible races when switching account in message list / chatstore Feb 20, 2024
@Simon-Laux
Copy link
Member Author

The current idea is to remove the global chat store and move to a local chat store that lives inside of the react tree

Where is it currently used

  • ChatMethods.ts -> maybe move these functions to a chatlist functions context or take chatstore instance as parameter
    • selectChat()
      • ChatList
      • CreateChat dialog
      • Forward Message dialog
      • QrCode dialog
      • ViewGroup dialog
      • ViewProfile dialog
      • WebxdcSendToChat dialog
      • mailtoUrl
      • MessageMarkdown #mostly_inside_msg_list
      • messageFunctions #mostly_inside_msg_list
      • MainScreen
    • setChatView()
      • MessageMarkdown
      • MainScreen
    • jumpToMessage()
      • mediaAttachment (in gallery)
      • ChatListItemRowMessage - message search results
      • EmojiAndStickerPicker - sending sticker
      • ChatAuditLog Dialog - show message or webxdc in chat
      • Fullscreen media Dialog - show message in chat
      • Message #mostly_inside_msg_list
      • notifications.ts #outside_of_react
    • unselectChat()
      • Composer #mostly_inside_react_chat - blocking chat
      • MainScreen - unselect chat when going to global gallery
    • setChatVisibility(), openDeleteChatDialog() and openBlockFirstContactOfChatDialog() - (conditionally call unselectChat() internally)
      • ChatListContextMenu
      • ThreeDotMenu #mostly_inside_react_chat
    • sendCallInvitation() - jumps to new message
      • Composer -> Attachment Menu
    • createChatByContactIdAndSelectIt() - calls selectChat
      • ChatList - create new contact in ChatList contact search
      • Create Chat Dialog
    • sendMessage() - jumps to newest message after sending
      • Composer #mostly_inside_react_chat
      • MessageListAndComposer - drag multiple files in to send them at once
      • Map - send a message with a location like a point of interest
        • comment: not sure if we should reload the map after sending to show the message instead of switching to the message list view.
    • clearChat() - select and unselect chat
      • ThreeDotMenu #mostly_inside_react_chat
    • createDraftMessage() - select chat
      • Mailto Dialog - when there is no to field you can select the recipient
      • Mailto Url handler
  • Link.tsx
    • to find out if the selected chat is the device chat, we can refactor this to use the message display context, that seems like the right place to put this info simon/start with chatstore cleanup #3700
      • why? because the device chat is whitelisted so every url there does not get a confirmation/warning dialog in case it is a labeled link
  • MessageList.tsx #mostly_inside_msg_list
    • local reference is passed to MessageList component as react property
    • the global reference is only used for the EmptyChatMessage component, we should maybe move that component into its own file and also pass the chatlist store down as property? simon/start with chatstore cleanup #3700
  • MessageListAndComposer.tsx - only uses the type and the store passed through by react props
  • MainScreen.tsx
    • uses the global instance of the chatstore to pass it to the MessageListAndComposer component as property.
    • I currently think the chat context should live in the MainScreen component and be local to it.

#mostly_inside_msg_list, #mostly_inside_react_chat: could be probably solved with a context
#outside_of_react: must be accessible as global outside of react

comments

  • select chat is used from many different places, maybe it could be some action/event that is handled by the currently mounted instance
  • in ChatMethods.ts are both methods that use the chatstore and methods that do not, should we separate them somehow?
  • can we really do this in one step/pr? because there are so many functions/cases that we need to test
  • ChatStore.effect.jumpToMessage() uses window.__internal_jump_to_message to find/call the currently active messagelist store instance, we can continue working with such hacks for now IMO.

@Simon-Laux
Copy link
Member Author

Simon-Laux commented Feb 21, 2024

Chat Store:

  • always use account id from state instead of global selected account id
  • rewrite the chat store with zustand or jotai (sth that makes multiple instances possible for the next step)
  • recreate when switching account and keep in react context probably in MainScreen

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
No open projects
Development

Successfully merging a pull request may close this issue.

2 participants