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

Error when rendering system messages about started calls in the Files sidebar #4572

Closed
danxuliu opened this issue Nov 9, 2020 · 6 comments · Fixed by #4949
Closed

Error when rendering system messages about started calls in the Files sidebar #4572

danxuliu opened this issue Nov 9, 2020 · 6 comments · Fixed by #4949
Assignees
Labels
1. to develop bug feature: chat 💬 Chat and system messages feature: frontend 🖌️ "Web UI" client feature: talk-sidebar ⬅️ Sidebar integration of Talk into other apps like sharing and documents

Comments

@danxuliu
Copy link
Member

danxuliu commented Nov 9, 2020

How to test

  • Build Talk JavaScript files in development mode
  • Open the Files app
  • Share two files
  • Refresh (due to the bug in the Files app that causes the FileInfo to not be updated)
  • Open the sidebar for the first shared file
  • Open the Chat tab
  • Join the conversation
  • Start a call
  • Leave the call
  • Open the sidebar for the second shared file
  • Open again the sidebar for the first shared file
  • Join the conversation

Expected result

The You started a call message is shown.

Actual result

The You started a call message is shown. In the browser console there are errors like Error in getter for watcher "showJoinCallButton": "TypeError: Cannot read property 'hasCall' of undefined".

@danxuliu danxuliu added 1. to develop bug feature: chat 💬 Chat and system messages feature: frontend 🖌️ "Web UI" client feature: talk-sidebar ⬅️ Sidebar integration of Talk into other apps like sharing and documents labels Nov 9, 2020
@PVince81
Copy link
Member

is this still happening after all the work we did in files sidebar ?

@PVince81
Copy link
Member

PVince81 commented Jan 14, 2021

Still happening on master.

There are even more undefined property / watcher error messages now, for:

  • lastCommonReadMessage
  • showJoinCallButton
  • hasCall
  • clientHeight

I'll have a look

@PVince81 PVince81 self-assigned this Jan 14, 2021
@PVince81
Copy link
Member

The thing that triggers the message is whenever we replace the token on this line https://github.com/nextcloud/spreed/blob/bugfix/4683/group-display-names-in-admin-settings/src/store/tokenStore.js#L56.
The stack trace points at this line so it means the token update is reactive, even before the mutation ends (I thought these were supposed to be atomic)

@PVince81
Copy link
Member

Out of curiosity I tried to swap the assignments but not it's the state.fileIdForToken that triggers the error.

My guess is that there's some old vue that is still trying to render the messages before the moment we got a chance to run fetch. Maybe we need to clear the old message list or even destroy the view upon leaving. Let's see.

@PVince81
Copy link
Member

It appears that CallView, MessagesList and Message vues are still mounted with the token for which the conversation does not exist yet in the store. Adding a loading flag to not render CallView until the conversation is fetched seems to prevent the issue.

I'll try to also add a spinner there as our usual "chat loading placeholder" would not fit well in that space.

@PVince81
Copy link
Member

wait... now I get it: the "messages" in the store is still populated while the "conversations" bit is empty.
this explains why the MessagesList still contains a list of Message views.

so upon leaving the conversation we should also purge the messages list

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1. to develop bug feature: chat 💬 Chat and system messages feature: frontend 🖌️ "Web UI" client feature: talk-sidebar ⬅️ Sidebar integration of Talk into other apps like sharing and documents
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants