Update notification to snackbar

[lucasleite01]

Snackbar displaying device name and event.

[tananaev]

Can we do this?

Suggested change

	message: `${devices[event.deviceId]?.name}: ${t(prefixString('event', `${event.type}`))}`,
	message: `${devices[event.deviceId]?.name}: ${t(prefixString('event', event.type))}`,

[tananaev]

Why do we have both autoHideDuration and setTimeout? Can you please explain how it works.

[tananaev]

This doesn't seem to support multiple events.

[tananaev]

Why do we need two variables? Can't we just use message?

[tananaev]

Can you also please rename issue to something better.

[tananaev]

I don't like adding new library just for supporting multiple notifications. Can't we just stack default ones?

[lucasleite01]

I think we can, but we basically would have to do that notistack does. There's a discussion about stacking snackbars.

[tananaev]

I would say if it's too complicated or requires too much code, let's only support a single one.

[lucasleite01]

Agreed, this was my first approach. But since we cannot control when we will receive events from websocket we need to have some kind of list or queue for the notifications that arrive.

Otherwise, if we receive an event from websocket and just update the component state it could lead to too much re-rendering if many events arrive in short time, causing error.

[tananaev]

Why would there be an error?

[lucasleite01]

Because an event can arrive and change the component state, generating a re-rendering just in the same time the component is being rendered.

[tananaev]

That's fine. Why would cause an error?

[lucasleite01]

Sorry, I might be mistaken:

“Too many re-renderers” is a React error that happens after you have reached an infinite render loop, typically caused by code that in a useEffect hook or the main body of the component itself unconditionally calls state setters.

I'll have to try something like the first approach but keeping in mind the multiple events concern.

[lucasleite01]

Anton, your question about multiple events pointed a big problem in the implementation, so I searched and found notistack package.

It uses the same Snackbar from MaterialUI and handle multiple Snackbars by enqueuing the messages and showing them later with a better control. I think this made the implementation more simple.

Now, the notification appears without device name, though, because when the notification arrives we cannot guarantee that we already have loaded the devices:

[tananaev]

Not showing device is not acceptable. It basically makes it pointless.

[lucasleite01]

Not showing device is not acceptable. It basically makes it pointless.

Right. We need to think in a better way to ensure that we cover multiple events notification and to have the devices when showing the notification.

[tananaev]

I also don't like adding dependency. Is there no way to display multiple toast as is?

[tananaev]

Why do we need to save events in the state? Can't we just do this directly and call setNotifications?

Also, can we please not reuse event object. It gets confusing. Create a new object for notifications.

[lucasleite01]

Why do we need to save events in the state? Can't we just do this directly and call setNotifications?

I tested that way, but here we don't have the devices yet:

socket.onmessage = (event) => {
      const data = JSON.parse(event.data);
      if (data.devices) {
        dispatch(devicesActions.update(data.devices));
      }
      if (data.positions) {
        dispatch(positionsActions.update(data.positions));
      }
      if (data.events) {
        // setEvents(data.events);
        // if we use setNotifications here, we don't have devices yet to construct the message.
        // something like that will return undefined for devices name:
        setNotifications(events.map((event) => {
          event.message = `${devices[event.deviceId]?.name}: ${t(prefixString('event', event.type))}`;
          event.show = true;
          setTimeout(() => event.show = false, 5000);
          return event;
        }));
      }
    };

Also, can we please not reuse event object. It gets confusing. Create a new object for notifications.

Do you mean to rename events?

const [events, setEvents] = useState([]);

[tananaev]

I see what you mean. In that case:

1. Don't reuse the same event object. Create a new object for notification.
2. Set devices as a dependency.

[lucasleite01]

1. Oh, I see now. Do you mean that:

    setNotifications(events.map((event) => {
      const notification = {
        message: `${devices[event.deviceId]?.name}: ${t(prefixString('event', event.type))}`,
        show: true,
      };
      setTimeout(() => notification.show = false, 5000);
      return notification;
    }));

2. Devices as dependency will trigger multiple snackbars with the same notification every time devices were updated.

[tananaev]

This also depends on the devices.

[lucasleite01]

This uses devices, but we don't want this to be triggered every time devices update because devices are updated frequently. If we add devices to the array the snackbars will be displayed every time new devices arrive even if a new event wasn't arrived.

[tananaev]

No, wait.. you need to clear notifications once they are dismissed. Looks like that's another thing that's missing.

[lucasleite01]

That's right. I'll do that.

[lucasleite01]

Now I used autoHiddeDurantion with 5 seconds and ater that time the onClose function is called to remove the notification from events.

[tananaev]

Everything else looks good.

[tananaev]

I don't like that there's a callback in a data object. Why do we need it here? We can just call it directly in onClose.

[lucasleite01]

Done.

[tananaev]

Merged, thank you.