LLS: error handling on stopping beacon (#8406)

* shared stopping error state for timeline, maxi and room warnign

Signed-off-by: Kerry Archibald <kerrya@element.io>

* check for stopping errors in roomlist share warning

Signed-off-by: Kerry Archibald <kerrya@element.io>

* lint

Signed-off-by: Kerry Archibald <kerrya@element.io>

* test stopping errors in OwnBeaconStore

Signed-off-by: Kerry Archibald <kerrya@element.io>

* update LeftPanelLiveShareWarning tests for stopping errors

Signed-off-by: Kerry Archibald <kerrya@element.io>

* reinstate try/catch for stopping beacons in create

Signed-off-by: Kerry Archibald <kerrya@element.io>

* remove unnecessary and buggy beacon stopping on creation

Signed-off-by: Kerry Archibald <kerrya@element.io>

src/components/views/beacon/LeftPanelLiveShareWarning.tsx

@@ -16,6 +16,7 @@ limitations under the License.
 
 import classNames from 'classnames';
 import React from 'react';
+import { BeaconIdentifier, Room } from 'matrix-js-sdk/src/matrix';
 
 import { useEventEmitterState } from '../../../hooks/useEventEmitter';
 import { _t } from '../../../languageHandler';
@@ -34,10 +35,14 @@ interface Props {
  * Choose the most relevant beacon
  * and get its roomId
  */
-const chooseBestBeaconRoomId = (liveBeaconIds, errorBeaconIds): string | undefined => {
+const chooseBestBeaconRoomId = (
+    liveBeaconIds: BeaconIdentifier[],
+    updateErrorBeaconIds: BeaconIdentifier[],
+    locationErrorBeaconIds: BeaconIdentifier[],
+): Room['roomId'] | undefined => {
     // both lists are ordered by creation timestamp in store
     // so select latest beacon
-    const beaconId = errorBeaconIds?.[0] ?? liveBeaconIds?.[0];
+    const beaconId = updateErrorBeaconIds?.[0] ?? locationErrorBeaconIds?.[0] ?? liveBeaconIds?.[0];
     if (!beaconId) {
         return undefined;
     }
@@ -46,6 +51,16 @@ const chooseBestBeaconRoomId = (liveBeaconIds, errorBeaconIds): string | undefin
     return beacon?.roomId;
 };
 
+const getLabel = (hasStoppingErrors: boolean, hasLocationErrors: boolean): string => {
+    if (hasStoppingErrors) {
+        return _t('An error occurred while stopping your live location');
+    }
+    if (hasLocationErrors) {
+        return _t('An error occured whilst sharing your live location');
+    }
+    return _t('You are sharing your live location');
+};
+
 const LeftPanelLiveShareWarning: React.FC<Props> = ({ isMinimized }) => {
     const isMonitoringLiveLocation = useEventEmitterState(
         OwnBeaconStore.instance,
@@ -59,6 +74,14 @@ const LeftPanelLiveShareWarning: React.FC<Props> = ({ isMinimized }) => {
         () => OwnBeaconStore.instance.getLiveBeaconIdsWithLocationPublishError(),
     );
 
+    const beaconIdsWithStoppingError = useEventEmitterState(
+        OwnBeaconStore.instance,
+        OwnBeaconStoreEvent.BeaconUpdateError,
+        () => OwnBeaconStore.instance.getLiveBeaconIds().filter(
+            beaconId => OwnBeaconStore.instance.beaconUpdateErrors.has(beaconId),
+        ),
+    );
+
     const liveBeaconIds = useEventEmitterState(
         OwnBeaconStore.instance,
         OwnBeaconStoreEvent.LivenessChange,
@@ -66,12 +89,15 @@ const LeftPanelLiveShareWarning: React.FC<Props> = ({ isMinimized }) => {
     );
 
     const hasLocationPublishErrors = !!beaconIdsWithLocationPublishError.length;
+    const hasStoppingErrors = !!beaconIdsWithStoppingError.length;
 
     if (!isMonitoringLiveLocation) {
         return null;
     }
 
-    const relevantBeaconRoomId = chooseBestBeaconRoomId(liveBeaconIds, beaconIdsWithLocationPublishError);
+    const relevantBeaconRoomId = chooseBestBeaconRoomId(
+        liveBeaconIds, beaconIdsWithStoppingError, beaconIdsWithLocationPublishError,
+    );
 
     const onWarningClick = relevantBeaconRoomId ? () => {
         dispatcher.dispatch<ViewRoomPayload>({
@@ -81,14 +107,12 @@ const LeftPanelLiveShareWarning: React.FC<Props> = ({ isMinimized }) => {
         });
     } : undefined;
 
-    const label = hasLocationPublishErrors ?
+    const label = getLabel(hasStoppingErrors, hasLocationPublishErrors);
-        _t('An error occured whilst sharing your live location') :
-        _t('You are sharing your live location');
 
     return <AccessibleButton
         className={classNames('mx_LeftPanelLiveShareWarning', {
             'mx_LeftPanelLiveShareWarning__minimized': isMinimized,
-            'mx_LeftPanelLiveShareWarning__error': hasLocationPublishErrors,
+            'mx_LeftPanelLiveShareWarning__error': hasLocationPublishErrors || hasStoppingErrors,
         })}
         title={isMinimized ? label : undefined}
         onClick={onWarningClick}

src/components/views/beacon/RoomLiveShareWarning.tsx

@@ -15,7 +15,6 @@ limitations under the License.
 */
 
 import React from 'react';
-import classNames from 'classnames';
 import { Room } from 'matrix-js-sdk/src/matrix';
 
 import { _t } from '../../../languageHandler';
@@ -67,7 +66,7 @@ const RoomLiveShareWarningInner: React.FC<RoomLiveShareWarningInnerProps> = ({ l
     };
 
     return <div
-        className={classNames('mx_RoomLiveShareWarning')}
+        className='mx_RoomLiveShareWarning'
     >
         <StyledLiveBeaconIcon className="mx_RoomLiveShareWarning_icon" withError={hasError} />
 

src/components/views/messages/MBeaconBody.tsx

@@ -146,6 +146,7 @@ const MBeaconBody: React.FC<IBodyProps> = React.forwardRef(({ mxEvent }, ref) =>
                     className='mx_MBeaconBody_chin'
                     beacon={beacon}
                     displayStatus={displayStatus}
+                    withIcon
                 /> :
                 <BeaconStatus
                     className='mx_MBeaconBody_chin'

src/i18n/strings/en_EN.json

@@ -2948,6 +2948,7 @@
     "View list": "View list",
     "View List": "View List",
     "Close sidebar": "Close sidebar",
+    "An error occurred while stopping your live location": "An error occurred while stopping your live location",
     "An error occured whilst sharing your live location": "An error occured whilst sharing your live location",
     "You are sharing your live location": "You are sharing your live location",
     "%(timeRemaining)s left": "%(timeRemaining)s left",

src/stores/OwnBeaconStore.ts

@@ -51,6 +51,7 @@ export enum OwnBeaconStoreEvent {
     LivenessChange = 'OwnBeaconStore.LivenessChange',
     MonitoringLivePosition = 'OwnBeaconStore.MonitoringLivePosition',
     LocationPublishError = 'LocationPublishError',
+    BeaconUpdateError = 'BeaconUpdateError',
 }
 
 const MOVING_UPDATE_INTERVAL = 2000;
@@ -61,6 +62,7 @@ const BAIL_AFTER_CONSECUTIVE_ERROR_COUNT = 2;
 type OwnBeaconStoreState = {
     beacons: Map<BeaconIdentifier, Beacon>;
     beaconLocationPublishErrorCounts: Map<BeaconIdentifier, number>;
+    beaconUpdateErrors: Map<BeaconIdentifier, Error>;
     beaconsByRoomId: Map<Room['roomId'], Set<BeaconIdentifier>>;
     liveBeaconIds: BeaconIdentifier[];
 };
@@ -99,6 +101,7 @@ export class OwnBeaconStore extends AsyncStoreWithClient<OwnBeaconStoreState> {
      * Reset on successful publish of location
      */
     public readonly beaconLocationPublishErrorCounts = new Map<BeaconIdentifier, number>();
+    public readonly beaconUpdateErrors = new Map<BeaconIdentifier, Error>();
     /**
      * ids of live beacons
      * ordered by creation time descending
@@ -144,6 +147,7 @@ export class OwnBeaconStore extends AsyncStoreWithClient<OwnBeaconStoreState> {
         this.beaconsByRoomId.clear();
         this.liveBeaconIds = [];
         this.beaconLocationPublishErrorCounts.clear();
+        this.beaconUpdateErrors.clear();
     }
 
     protected async onReady(): Promise<void> {
@@ -215,7 +219,6 @@ export class OwnBeaconStore extends AsyncStoreWithClient<OwnBeaconStoreState> {
         }
 
         await this.updateBeaconEvent(beacon, { live: false });
-
         // prune from local store
         removeLocallyCreateBeaconEventId(beacon.beaconInfoId);
     };
@@ -300,7 +303,10 @@ export class OwnBeaconStore extends AsyncStoreWithClient<OwnBeaconStoreState> {
      * Live beacon ids that do not have wire errors
      */
     private get healthyLiveBeaconIds() {
-        return this.liveBeaconIds.filter(beaconId => !this.beaconHasLocationPublishError(beaconId));
+        return this.liveBeaconIds.filter(beaconId =>
+            !this.beaconHasLocationPublishError(beaconId) &&
+            !this.beaconUpdateErrors.has(beaconId),
+        );
     }
 
     private initialiseBeaconState = () => {
@@ -393,19 +399,6 @@ export class OwnBeaconStore extends AsyncStoreWithClient<OwnBeaconStoreState> {
         );
 
         storeLocallyCreateBeaconEventId(event_id);
-
-        // try to stop any other live beacons
-        // in this room
-        this.beaconsByRoomId.get(roomId)?.forEach(beaconId => {
-            if (this.getBeaconById(beaconId)?.isLive) {
-                try {
-                    // don't await, this is best effort
-                    this.stopBeacon(beaconId);
-                } catch (error) {
-                    logger.error('Failed to stop live beacons', error);
-                }
-            }
-        });
     };
 
     /**
@@ -510,6 +503,11 @@ export class OwnBeaconStore extends AsyncStoreWithClient<OwnBeaconStoreState> {
      * MatrixClient api
      */
 
+    /**
+     * Updates beacon with provided content update
+     * Records error in beaconUpdateErrors
+     * rethrows
+     */
     private updateBeaconEvent = async (beacon: Beacon, update: Partial<BeaconInfoState>): Promise<void> => {
         const { description, timeout, timestamp, live, assetType } = {
             ...beacon.beaconInfo,
@@ -523,7 +521,21 @@ export class OwnBeaconStore extends AsyncStoreWithClient<OwnBeaconStoreState> {
             timestamp,
         );
 
+        try {
             await this.matrixClient.unstable_setLiveBeacon(beacon.roomId, updateContent);
+            // cleanup any errors
+            const hadError = this.beaconUpdateErrors.has(beacon.identifier);
+            if (hadError) {
+                this.beaconUpdateErrors.delete(beacon.identifier);
+                this.emit(OwnBeaconStoreEvent.BeaconUpdateError, beacon.identifier, false);
+            }
+        } catch (error) {
+            logger.error('Failed to update beacon', error);
+            this.beaconUpdateErrors.set(beacon.identifier, error);
+            this.emit(OwnBeaconStoreEvent.BeaconUpdateError, beacon.identifier, true);
+
+            throw error;
+        }
     };
 
     /**

src/utils/beacon/useOwnLiveBeacons.ts

@@ -39,7 +39,6 @@ type LiveBeaconsState = {
  */
 export const useOwnLiveBeacons = (liveBeaconIds: BeaconIdentifier[]): LiveBeaconsState => {
     const [stoppingInProgress, setStoppingInProgress] = useState(false);
-    const [error, setError] = useState<Error>();
 
     const hasLocationPublishError = useEventEmitterState(
         OwnBeaconStore.instance,
@@ -48,10 +47,22 @@ export const useOwnLiveBeacons = (liveBeaconIds: BeaconIdentifier[]): LiveBeacon
             liveBeaconIds.some(OwnBeaconStore.instance.beaconHasLocationPublishError),
     );
 
+    const hasStopSharingError = useEventEmitterState(
+        OwnBeaconStore.instance,
+        OwnBeaconStoreEvent.BeaconUpdateError,
+        () =>
+            liveBeaconIds.some(id => OwnBeaconStore.instance.beaconUpdateErrors.has(id)),
+    );
+
+    useEffect(() => {
+        if (hasStopSharingError) {
+            setStoppingInProgress(false);
+        }
+    }, [hasStopSharingError]);
+
     // reset stopping in progress on change in live ids
     useEffect(() => {
         setStoppingInProgress(false);
-        setError(undefined);
     }, [liveBeaconIds]);
 
     // select the beacon with latest expiry to display expiry time
@@ -64,10 +75,6 @@ export const useOwnLiveBeacons = (liveBeaconIds: BeaconIdentifier[]): LiveBeacon
         try {
             await Promise.all(liveBeaconIds.map(beaconId => OwnBeaconStore.instance.stopBeacon(beaconId)));
         } catch (error) {
-            // only clear loading in case of error
-            // to avoid flash of not-loading state
-            // after beacons have been stopped but we wait for sync
-            setError(error);
             setStoppingInProgress(false);
         }
     };
@@ -82,6 +89,6 @@ export const useOwnLiveBeacons = (liveBeaconIds: BeaconIdentifier[]): LiveBeacon
         beacon,
         stoppingInProgress,
         hasLocationPublishError,
-        hasStopSharingError: !!error,
+        hasStopSharingError,
     };
 };

test/components/views/beacon/LeftPanelLiveShareWarning-test.tsx

@@ -18,7 +18,7 @@ import React from 'react';
 import { mocked } from 'jest-mock';
 import { mount } from 'enzyme';
 import { act } from 'react-dom/test-utils';
-import { Beacon } from 'matrix-js-sdk/src/matrix';
+import { Beacon, BeaconIdentifier } from 'matrix-js-sdk/src/matrix';
 
 import LeftPanelLiveShareWarning from '../../../../src/components/views/beacon/LeftPanelLiveShareWarning';
 import { OwnBeaconStore, OwnBeaconStoreEvent } from '../../../../src/stores/OwnBeaconStore';
@@ -33,6 +33,7 @@ jest.mock('../../../../src/stores/OwnBeaconStore', () => {
         public getLiveBeaconIdsWithLocationPublishError = jest.fn().mockReturnValue([]);
         public getBeaconById = jest.fn();
         public getLiveBeaconIds = jest.fn().mockReturnValue([]);
+        public readonly beaconUpdateErrors = new Map<BeaconIdentifier, Error>();
     }
     return {
         // @ts-ignore
@@ -59,6 +60,8 @@ describe('<LeftPanelLiveShareWarning />', () => {
     beforeEach(() => {
         jest.spyOn(global.Date, 'now').mockReturnValue(now);
         jest.spyOn(dispatcher, 'dispatch').mockClear().mockImplementation(() => { });
+
+        OwnBeaconStore.instance.beaconUpdateErrors.clear();
     });
 
     afterAll(() => {
@@ -191,5 +194,55 @@ describe('<LeftPanelLiveShareWarning />', () => {
 
             expect(component.html()).toBe(null);
         });
+
+        describe('stopping errors', () => {
+            it('renders stopping error', () => {
+                OwnBeaconStore.instance.beaconUpdateErrors.set(beacon2.identifier, new Error('error'));
+                const component = getComponent();
+                expect(component.text()).toEqual('An error occurred while stopping your live location');
+            });
+
+            it('starts rendering stopping error on beaconUpdateError emit', () => {
+                const component = getComponent();
+                // no error
+                expect(component.text()).toEqual('You are sharing your live location');
+
+                act(() => {
+                    OwnBeaconStore.instance.beaconUpdateErrors.set(beacon2.identifier, new Error('error'));
+                    OwnBeaconStore.instance.emit(OwnBeaconStoreEvent.BeaconUpdateError, beacon2.identifier, true);
+                });
+
+                expect(component.text()).toEqual('An error occurred while stopping your live location');
+            });
+
+            it('renders stopping error when beacons have stopping and location errors', () => {
+                mocked(OwnBeaconStore.instance).getLiveBeaconIdsWithLocationPublishError.mockReturnValue(
+                    [beacon1.identifier],
+                );
+                OwnBeaconStore.instance.beaconUpdateErrors.set(beacon2.identifier, new Error('error'));
+                const component = getComponent();
+                expect(component.text()).toEqual('An error occurred while stopping your live location');
+            });
+
+            it('goes to room of latest beacon with stopping error when clicked', () => {
+                mocked(OwnBeaconStore.instance).getLiveBeaconIdsWithLocationPublishError.mockReturnValue(
+                    [beacon1.identifier],
+                );
+                OwnBeaconStore.instance.beaconUpdateErrors.set(beacon2.identifier, new Error('error'));
+                const component = getComponent();
+                const dispatchSpy = jest.spyOn(dispatcher, 'dispatch');
+
+                act(() => {
+                    component.simulate('click');
+                });
+
+                expect(dispatchSpy).toHaveBeenCalledWith({
+                    action: Action.ViewRoom,
+                    metricsTrigger: undefined,
+                    // stopping error beacon's room
+                    room_id: beacon2.roomId,
+                });
+            });
+        });
     });
 });

test/stores/OwnBeaconStore-test.ts

@@ -765,6 +765,64 @@ describe('OwnBeaconStore', () => {
             );
         });
 
+        it('records error when stopping beacon event fails to send', async () => {
+            jest.spyOn(logger, 'error').mockImplementation(() => {});
+            makeRoomsWithStateEvents([
+                alicesRoom1BeaconInfo,
+            ]);
+            const store = await makeOwnBeaconStore();
+            const emitSpy = jest.spyOn(store, 'emit');
+            const error = new Error('oups');
+            mockClient.unstable_setLiveBeacon.mockRejectedValue(error);
+
+            await expect(store.stopBeacon(getBeaconInfoIdentifier(alicesRoom1BeaconInfo))).rejects.toEqual(error);
+
+            expect(store.beaconUpdateErrors.get(getBeaconInfoIdentifier(alicesRoom1BeaconInfo))).toEqual(error);
+            expect(emitSpy).toHaveBeenCalledWith(
+                OwnBeaconStoreEvent.BeaconUpdateError, getBeaconInfoIdentifier(alicesRoom1BeaconInfo), true,
+            );
+        });
+
+        it('clears previous error and emits when stopping beacon works on retry', async () => {
+            jest.spyOn(logger, 'error').mockImplementation(() => {});
+            makeRoomsWithStateEvents([
+                alicesRoom1BeaconInfo,
+            ]);
+            const store = await makeOwnBeaconStore();
+            const emitSpy = jest.spyOn(store, 'emit');
+            const error = new Error('oups');
+            mockClient.unstable_setLiveBeacon.mockRejectedValueOnce(error);
+
+            await expect(store.stopBeacon(getBeaconInfoIdentifier(alicesRoom1BeaconInfo))).rejects.toEqual(error);
+            expect(store.beaconUpdateErrors.get(getBeaconInfoIdentifier(alicesRoom1BeaconInfo))).toEqual(error);
+
+            await store.stopBeacon(getBeaconInfoIdentifier(alicesRoom1BeaconInfo));
+
+            // error cleared
+            expect(store.beaconUpdateErrors.get(getBeaconInfoIdentifier(alicesRoom1BeaconInfo))).toBeFalsy();
+
+            // emit called for error clearing
+            expect(emitSpy).toHaveBeenCalledWith(
+                OwnBeaconStoreEvent.BeaconUpdateError, getBeaconInfoIdentifier(alicesRoom1BeaconInfo), false,
+            );
+        });
+
+        it('does not emit BeaconUpdateError when stopping succeeds and beacon did not have errors', async () => {
+            jest.spyOn(logger, 'error').mockImplementation(() => {});
+            makeRoomsWithStateEvents([
+                alicesRoom1BeaconInfo,
+            ]);
+            const store = await makeOwnBeaconStore();
+            const emitSpy = jest.spyOn(store, 'emit');
+            // error cleared
+            expect(store.beaconUpdateErrors.get(getBeaconInfoIdentifier(alicesRoom1BeaconInfo))).toBeFalsy();
+
+            // emit called for error clearing
+            expect(emitSpy).not.toHaveBeenCalledWith(
+                OwnBeaconStoreEvent.BeaconUpdateError, getBeaconInfoIdentifier(alicesRoom1BeaconInfo), false,
+            );
+        });
+
         it('updates beacon to live:false when it is expired but live property is true', async () => {
             makeRoomsWithStateEvents([
                 alicesRoom1BeaconInfo,
@@ -1051,6 +1109,31 @@ describe('OwnBeaconStore', () => {
                 );
             });
 
+            it('stops publishing positions when a beacon has a stopping error', async () => {
+                // reject stopping beacon
+                const error = new Error('oups');
+                mockClient.unstable_setLiveBeacon.mockRejectedValue(error);
+                makeRoomsWithStateEvents([
+                    alicesRoom1BeaconInfo,
+                ]);
+                const store = await makeOwnBeaconStore();
+                // wait for store to settle
+                await flushPromisesWithFakeTimers();
+
+                // 2 positions from watchPosition in this period
+                await advanceAndFlushPromises(5000);
+
+                // attempt to stop the beacon
+                await expect(store.stopBeacon(getBeaconInfoIdentifier(alicesRoom1BeaconInfo))).rejects.toEqual(error);
+                expect(store.beaconUpdateErrors.get(getBeaconInfoIdentifier(alicesRoom1BeaconInfo))).toEqual(error);
+
+                // 2 more positions in this period
+                await advanceAndFlushPromises(50000);
+
+                // only two positions pre-stopping were sent
+                expect(mockClient.sendEvent).toHaveBeenCalledTimes(3);
+            });
+
             it('restarts publishing a beacon after resetting location publish error', async () => {
                 // always fails to send events
                 mockClient.sendEvent.mockRejectedValue(new Error('oups'));
@@ -1266,21 +1349,5 @@ describe('OwnBeaconStore', () => {
             // didn't throw, no error log
             expect(loggerErrorSpy).not.toHaveBeenCalled();
         });
-
-        it('stops live beacons for room after creating new beacon', async () => {
-            // room1 already has a beacon
-            makeRoomsWithStateEvents([
-                alicesRoom1BeaconInfo,
-            ]);
-            // but it was not created on this device
-            localStorageGetSpy.mockReturnValue(undefined);
-
-            const store = await makeOwnBeaconStore();
-            const content = makeBeaconInfoContent(100);
-            await store.createLiveBeacon(room1Id, content);
-
-            // update beacon called
-            expect(mockClient.unstable_setLiveBeacon).toHaveBeenCalled();
-        });
     });
 });