[FIX] Omnichannel - Fix issue with modals on room preview mode.

[KevLehman]

If an agent access a livechat room before being subscripted to it, there's a chance the server will die due to an unhandled invalid-subscription error. This does not remove the error, but prevents the server from dying.

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

[sampaiodiego]

the process will not die in production, it should happen only in dev mode..

I have to ask, why is it calling to mark a room as read if the user/agent is not subscribed to that room in the first place?

[KevLehman]

Won't it? Unhandled promise rejections are always killing the server 😬 since they happen... somewhere in the process.

The issue happens when the routing is set up as manual selection, so agents can see the rooms before "joining" them (so they don't have a subscription yet).

[sampaiodiego]

Won't it? Unhandled promise rejections are always killing the server since they happen... somewhere in the process.

nope.. see

Rocket.Chat/app/lib/server/lib/meteorFixes.js

Line 62 in 18bee7a

process.on('unhandledRejection', (error) => {

The issue happens when the routing is set up as manual selection, so agents can see the rooms before "joining" them (so they don't have a subscription yet).

I think in this scenario it should not be calling the endpoint then.. there is no reason to do it.. I think we should keep showing the error, because it is a handled error, and stop calling the endpoint..

from another point of view, if you call this API, referencing a room you're not a member of, I think the API should return 404 (or some other 40x), that's why I think we should keep the error.

[KevLehman]

I think not calling the endpoint should be the appropiate way of handling that error, and I'll take a look at avoiding that.

But 🤔 even when the process.on is "handling" the error, the user won't be notified of this one, since is node itself who is managing it. It will only appear in the logs and look like an "unhandled" error. So, I was thinking in removing the part of code that conditionally sends the error, and keep the await, so the error is handled by meteor and not by node. What do you think?

[sampaiodiego]

I think not calling the endpoint should be the appropiate way of handling that error, and I'll take a look at avoiding that.

great 👍

But even when the process.on is "handling" the error, the user won't be notified of this one, since is node itself who is managing it. It will only appear in the logs and look like an "unhandled" error. So, I was thinking in removing the part of code that conditionally sends the error, and keep the await, so the error is handled by meteor and not by node. What do you think?

yeah, you're right. we're throwing the error, so we should handle it and return it to the end user..