Displayed error modals when connection to server fails

[Mounayer]

Description

Closes #172

Using the already existing modal component, I have covered two edge cases when an error happens, and nothing is displayed to the user.

First case

If the user goes to their room and clicks on the QR code button quickly, before the Connection closed modal pops up, they are presented with the modal that displays the QR code, with a broken image of the QR code, since there's no connection to the server. I simply added an onError listener to the img element, that now clearly displays an error modal with information to the user, i.e.:

instead of just:

Second case

If the connection to the server is failing, we are unable to load the local peers, this failure is not being displayed to the user in any way. A simple modal is now being displayed when the peers are failing to load to the user, asking them to check their internet connection! i.e.:

Whereas before, while peers fail to load, no errors were being displayed at all.

Testing

• Test by running the client without the server, go to localhost:8080/app, you should have an error modal pop up telling you what the issue is.

• Test by running the client without the server, go to localhost:8080/app, and try to go to the local network room really quick before the error modal pops up, and continue to quickly click on the QR button, that displays the modal which displays the QR code. You should not get an error message when it fails to retrieve the proper QR code from the server.

Please let me know if there are any suggestion or requested changes, I'd be glad to apply them!

[blenderskool]

@Mounayer Thanks for taking this up and preparing such a nice PR description! I don't recall what the context of the original issue was (which is a miss from my side) but I think it was related to socket connections getting closed due to errors resulting in no UI feedback.

WebSocket has 4 events:

• open
• message
• close
• error

The first 3 are already handled and errors sent by server leading to socket closure are handled through close event. The error event however is not handled which is fired due to some external factor(could not connect because of flaky internet, incorrect socket URL configuration, etc.). Along with this, sometimes error can occur when new WebSocket() is called too (errors related to insecure connections on secure sites, etc.). This can be shown as a modal inside file sharing room as it affects the primary purpose of the page and there's no way to recover from it without a page refresh.

I like the other error cases you've handled, we can improve it even more:

• First case: Instead of showing a full screen error modal, what if we just show something like "Could not load QR code" when the image fails to load inside the QR code modal? Idea behind this is that there could be some error with QR code endpoint itself and it doesn't make sense to block file sharing completely as there are other ways to share the room URL too. And if a server connection error does happen, it would be captured by other modals from error and close events of web socket itself. What do you think?
• Second case: Again, instead of showing a full screen error modal and preventing any other action from the user, maybe we can just show a small "Could not show local peers, try again later" where the local peers get rendered. Again this could be an isolated issue from SSE endpoint so I don't think we should block other actions in the app like opening rooms, etc.

[Mounayer]

@Mounayer Thanks for taking this up and preparing such a nice PR description! I don't recall what the context of the original issue was (which is a miss from my side) but I think it was related to socket connections getting closed due to errors resulting in no UI feedback.

WebSocket has 4 events:

• open
• message
• close
• error

The first 3 are already handled and errors sent by server leading to socket closure are handled through close event. The error event however is not handled which is fired due to some external factor(could not connect because of flaky internet, incorrect socket URL configuration, etc.). Along with this, sometimes error can occur when new WebSocket() is called too (errors related to insecure connections on secure sites, etc.). This can be shown as a modal inside file sharing room as it affects the primary purpose of the page and there's no way to recover from it without a page refresh.

I like the other error cases you've handled, we can improve it even more:

• First case: Instead of showing a full screen error modal, what if we just show something like "Could not load QR code" when the image fails to load inside the QR code modal? Idea behind this is that there could be some error with QR code endpoint itself and it doesn't make sense to block file sharing completely as there are other ways to share the room URL too. And if a server connection error does happen, it would be captured by other modals from error and close events of web socket itself. What do you think?
• Second case: Again, instead of showing a full screen error modal and preventing any other action from the user, maybe we can just show a small "Could not show local peers, try again later" where the local peers get rendered. Again this could be an isolated issue from SSE endpoint so I don't think we should block other actions in the app like opening rooms, etc.

Hi @blenderskool , I'm glad my PR description was good! I generally try to be as clear as possible in my pull requests, and provide as much context as I can!

First, let me address your suggested changes:

First Case: I completely agree! That's what I personally would have done myself, I just assumed the standard way to handle errors were through the modals, after seeing the error handler in the FileTransfer.js file! This makes more sense for sure, I'll work on it at once.

Second case: I also agree about this, I think showing an error message related to the specific cause of the error without disabling access to other things that might not be faulty is the right way to go. However, I'm not sure what you mean by where the local peers get rendered, could you please point out the location where the local peers get rendered?

Finally, I'll immediately get into displaying an error modal in the file sharing page when an error occurs, since there's no way to recover from this besides refreshing the page!

Thank you for your detailed feedback!

Edit:

I have updated the QR code error to display in the same QR modal, instead of the QR code image, i.e.:

I have also added an event listener that listens in on any error event in FileTransfer.js, that uses the already existing Connection Closed modal.

I would just like more clarification regarding the Second case mentioned above, so I know for sure where to display the error!

Thanks again!

Edit 2:

Upon further inspection, while I did manage to find way to display "Local peers could not be loaded, please try again" in the peer room, It is being shadowed by the exiting error that appears if the connection is bad, i.e.:

This is in the http://localhost:8080/app/t/ route, therefore, I kind of think this is unnecessary, and we should just let the existing error modal handle this, what do you think?

[blenderskool]

I think we can remove the error modal for local peers. I thought about showing it at the place where local peers are getting rendered, but I don't think it'll look good. We should be fine with not showing anything when an error occurs for local peers.

[blenderskool]

Can we catch the errors from this line too?

blaze/client/src/routes/App/FileTransfer/FileTransfer.js

Line 240 in e927920

this.fileShare = socketConnect(this.client.isLocal ? '' : this.client.room, this.client.name);

new WebSocket() throws errors related to insecure connections on secure sites, etc. and it would be good if we can catch it and show the error. I don't know if it'll be good as an error modal or just a console error message (which already happens). Maybe we can play around with the messaging.

[Mounayer]

Can we catch the errors from this line too?

blaze/client/src/routes/App/FileTransfer/FileTransfer.js

Line 240 in e927920

this.fileShare = socketConnect(this.client.isLocal ? '' : this.client.room, this.client.name);

new WebSocket() throws errors related to insecure connections on secure sites, etc. and it would be good if we can catch it and show the error. I don't know if it'll be good as an error modal or just a console error message (which already happens). Maybe we can play around with the messaging.

I've removed the error modal for local peers, also great catch on the new WebSocket() error that can be thrown. I suppose we have to make a decision, the way I see it, if this code executes:

    this.fileShare = socketConnect(this.client.isLocal ? '' : this.client.room, this.client.name);
    const { socket } = this.fileShare;

Then the error modal will be triggered below if there is an error in the connection, here:

    socket.on('error', err => {
      this.setState({
        errorModal: {
          isOpen: true,
          type: err.reason || constants.ERR_CONN_CLOSED,
        },
      });
    });

This can be a beneficial message to the client, having a modal pop up which it already does, however... If the new WebSocket() throws an error inside the socketConnect() function, i.e, here:

function socketConnect(room, username) {
  const socket = new Socket(new WebSocket(urls.WS_HOST));
  const fileShare = new FileShare(socket);
  socket.name = username;
  
  socket.on('open', () => {
    socket.send(constants.JOIN, {
      roomName: room,
      name: username,
      peerId: fileShare.isWebRTC ? fileShare.torrentClient.peerId : null,
    });
  });

  return fileShare;
}

the previously mentioned lines would not execute, because it'll hang here, no? I think for this very specific case, printing the error message to stderr is a better approach... Not sure what the user can do if they are faced with such an error in a modal popup!

I can maybe wrap this line with a try catch?

try{
        // code before
        this.fileShare = socketConnect(this.client.isLocal ? '' : this.client.room, this.client.name);
        // rest of code
}
catch(err)
{
        console.err(err);
}

Let me know what you think is best, I'm open to suggestions!