hpi-sam / digital-fuesim-manv

A German simulation system for training emergency medical services leadership personnel on how to manage Mass Casualty Incidents.
https://fuesim-manv.de/
GNU Affero General Public License v3.0
16 stars 8 forks source link

A failing move proposal doesn't revert the position of the respective element #298

Open Dassderdie opened 2 years ago

Dassderdie commented 2 years ago

Bug report:

On #291

If the proposal to move an image fails, the image remains on the invalid position instead of being reverted to its original position.

Direct response from @Dassderdie

The debugging was more difficult than I thought, but I think I got it now.

  1. On this branch, you weren't using optimistic updates.
            apiService.proposeAction(
                {
                    type: '[MapImage] Move MapImage',
                    mapImageId: mapImage.id + 'a',
                    targetPosition,
                },
                // this must be true to be optimistic
                true
            );

I'd propose we remove the default for this parameter. However, against my original prediction, we use optimistic updates quite a lot. Therefore a conscious choice should be made.

Actual problem

  1. There is a possible desynchronization between the state and OpenLayers. We drag a feature before we propose the action. Suppose we don't use optimistic updates and the proposal gets rejected. In that case, we have a desync between OpenLayers and the client state because we only pass changes in the state to OpenLayers.

  2. If we use optimistic updates and the action got applied locally, sent to the server, got rejected, and the local changes get reverted, the state changes in between. Therefore OpenLayers should update the position of the element afterward.

This is (most of the time) not the case. The local action is probably not applied because we share the same reducers on the server and the client. If the server rejects the proposal, the client will do it too (Error while applying server action. This is expected if an optimistic update has been applied). Therefore the state never changes.

TLDR: The effects you saw are not our custom optimistic updates not working, but the optimistic updates we do in OpenLayers by changing the position of elements during the dragging without respecting the state in the store. The client and server are in sync, but OpenLayers and the client state are not.

Solution

Handling OpenLayers optimistic updates nicely encapsulated as we do in the store is not possible (I think). Instead, we could implement a handler specifically for the case the proposal doesn't go through in the translateHandler.

On the other hand, I believe that if we propose a valid action there that doesn't get rejected by the client, too, the state should change two times and, therefore, automatically redraw the element to the correct position.

Dassderdie commented 2 years ago
  1. If we use optimistic updates and the action got applied locally, sent to the server, got rejected, and the local changes get reverted, the state changes in between. Therefore OpenLayers should update the position of the element afterward.

This isn't working currently, because we have

// TODO: this is workaround for not emitting synchronously
// currently, the setState of the optimistic update and the actions that are reapplied each bring the state to synchronously emit
debounceTime(0),

at this place. I will fix this as part of my bachelor thesis.

ClFeSc commented 2 years ago

@ClFeSc @Dassderdie Document that all proposals involving movement on the map must be made optimistically, which resolves this problem.

Dassderdie commented 1 year ago

Only the documentation has been updated, but no changes were made in the code (setting optimistic to true). Therefore I reopen this.

Dassderdie commented 1 year ago

Fix desynchronization because (only) the server rejected the proposal:

Fix desynchronization because the client (and server) errored during the proposal: