fix: Ensure a bare ptz_digital action resets the zoom (#2635)

- Closes: #2632

src/card-controller/actions/actions/effect.ts

@@ -9,13 +9,21 @@ export class EffectAction extends AdvancedCameraCardAction<EffectActionConfig> {
     const action = this._getAction();
     switch (action.effect_action) {
       case 'start':
-        void api.getEffectsManager().startEffect(action.effect);
+        // An effect runs for its own duration, so it is deliberately not
+        // awaited: doing so would stall the rest of the action set.
+        api
+          .getEffectsManager()
+          .startEffect(action.effect)
+          .catch(() => {});
         break;
       case 'stop':
         api.getEffectsManager().stopEffect(action.effect);
         break;
       case 'toggle':
-        void api.getEffectsManager().toggleEffect(action.effect);
+        api
+          .getEffectsManager()
+          .toggleEffect(action.effect)
+          .catch(() => {});
         break;
     }
   }

src/card-controller/actions/actions/ptz-digital.ts

@@ -6,12 +6,12 @@ import {
   ZOOM_DEFAULT_SCALE,
   type PartialZoomSettings,
 } from '../../../components-lib/zoom/types';
-import { generateViewContextForZoom } from '../../../components-lib/zoom/zoom-view-context';
 import type { PTZDigitialActionConfig } from '../../../config/schema/actions/custom/ptz-digital';
 import { ZOOM_MAX, ZOOM_MIN } from '../../../config/schema/common/zoom';
 import { getPTZTarget } from '../../../utils/ptz';
 import { Timer } from '../../../utils/timer';
 import type { CardActionsAPI } from '../../types';
+import { ZoomRequestViewModifier } from '../../view/modifiers/zoom-request';
 import type { TargetedActionContext } from '../types';
 import {
   setInProgressForThisTarget,
@@ -33,13 +33,16 @@ export class PTZDigitalAction extends AdvancedCameraCardAction<PTZDigitialAction
   private _timer = new Timer();
 
   private async _stepChange(api: CardActionsAPI, targetID: string): Promise<void> {
-    api.getViewManager().setViewWithMergedContext(
+    api
-      generateViewContextForZoom(targetID, {
+      .getViewManager()
-        requested: this._convertActionToZoomSettings(
+      .setViewWithModifiers([
-          api.getViewManager().getView()?.context?.zoom?.[targetID]?.observed,
+        new ZoomRequestViewModifier(
+          targetID,
+          this._convertActionToZoomSettings(
+            api.getViewManager().getView()?.context?.zoom?.[targetID]?.observed,
+          ),
         ),
-      }),
+      ]);
-    );
   }
 
   public async stop(): Promise<void> {

src/card-controller/actions/actions/ptz-multi.ts

@@ -28,7 +28,7 @@ export class PTZMultiAction extends AdvancedCameraCardAction<PTZMultiActionConfi
       return;
     }
 
-    void (
+    await (
       type === 'ptz' ? this._toPTZAction(targetID) : this._toPTZDigitalAction(targetID)
     ).execute(api);
   }

src/card-controller/view/modifiers/index.ts

@@ -4,6 +4,7 @@ import type { ViewModifier } from '../types';
 export const applyViewModifiers = (
   view: View,
   modifiers?: ViewModifier[] | null,
-): void => {
+): View => {
   modifiers?.forEach((modifier) => modifier.modify(view));
+  return view;
 };

src/card-controller/view/modifiers/zoom-request.ts

@@ -0,0 +1,30 @@
+import type { PartialZoomSettings } from '../../../components-lib/zoom/types';
+import { generateViewContextForZoom } from '../../../components-lib/zoom/zoom-view-context';
+import type { View } from '../../../view/view';
+import type { ViewModifier } from '../types';
+
+// The single write path for a zoom request. The settings are *assigned* rather than
+// *merged*, so a new request always replaces the one before it. This is
+// necessary as a request to reset to default is expressed as an empty object,
+// and a deep merge cannot use that to *erase* the values of an earlier request.
+export class ZoomRequestViewModifier implements ViewModifier {
+  private _targetID: string;
+  private _requested: PartialZoomSettings;
+
+  constructor(targetID: string, requested: PartialZoomSettings) {
+    this._targetID = targetID;
+    this._requested = requested;
+  }
+
+  public modify(view: View): void {
+    const target = view.context?.zoom?.[this._targetID];
+    if (target) {
+      target.requested = this._requested;
+      return;
+    }
+
+    view.mergeInContext(
+      generateViewContextForZoom(this._targetID, { requested: this._requested }),
+    );
+  }
+}

src/card-controller/view/types.ts

@@ -82,6 +82,7 @@ export interface ViewManagerInterface {
   setViewByParametersWithExistingQuery(options?: ViewFactoryOptions): Promise<void>;
 
   setViewWithMergedContext(context: ViewContext | null): void;
+  setViewWithModifiers(modifiers: ViewModifier[]): void;
 
   hasMajorMediaChange(oldView?: View | null, newView?: View | null): boolean;
 }

src/card-controller/view/view-manager.ts

@@ -334,6 +334,12 @@ export class ViewManager implements ViewManagerInterface {
     }
   }
 
+  public setViewWithModifiers(modifiers: ViewModifier[]): void {
+    if (this._view) {
+      return this._setView(applyViewModifiers(this._view.clone(), modifiers));
+    }
+  }
+
   /**
    * Detect if the current view has a major "media change" for the given previous view.
    * @param oldView The previous view.

src/components-lib/zoom/zoom-view-context.ts

@@ -8,7 +8,9 @@ interface ZoomViewContext {
   observed?: ZoomSettingsObserved;
 
   // Populate this to request zoom to a particular scale/x/y. An empty object
-  // will reset to default, null will make no change.
+  // will reset to default, null will make no change. Requests must be written
+  // by `ZoomRequestViewModifier` since an empty object needs to mean "reset to
+  // default" (and a merged empty object != an assigned empty object).
   requested?: PartialZoomSettings | null;
 }
 

tests/card-controller/actions/actions/effect.test.ts

@@ -1,4 +1,4 @@
-import { describe, expect, it } from 'vitest';
+import { describe, expect, it, vi } from 'vitest';
 
 import { EffectAction } from '../../../../src/card-controller/actions/actions/effect';
 import { createEffectAction } from '../../../../src/utils/action';
@@ -7,6 +7,7 @@ import { createCardAPI } from '../../../test-utils';
 describe('EffectAction', () => {
   it('should call startEffect when action is start', async () => {
     const api = createCardAPI();
+    vi.mocked(api.getEffectsManager().startEffect).mockResolvedValue(undefined);
     const actionConfig = createEffectAction('snow', 'start');
     const action = new EffectAction({}, actionConfig);
 
@@ -27,6 +28,7 @@ describe('EffectAction', () => {
 
   it('should call toggleEffect when action is toggle', async () => {
     const api = createCardAPI();
+    vi.mocked(api.getEffectsManager().toggleEffect).mockResolvedValue(undefined);
     const actionConfig = createEffectAction('snow', 'toggle');
     const action = new EffectAction({}, actionConfig);
 
@@ -35,6 +37,24 @@ describe('EffectAction', () => {
     expect(api.getEffectsManager().toggleEffect).toHaveBeenCalledWith('snow');
   });
 
+  it('should tolerate a failure to start the effect', async () => {
+    const api = createCardAPI();
+    vi.mocked(api.getEffectsManager().startEffect).mockRejectedValue(new Error());
+    const actionConfig = createEffectAction('snow', 'start');
+    const action = new EffectAction({}, actionConfig);
+
+    await expect(action.execute(api)).resolves.toBeUndefined();
+  });
+
+  it('should tolerate a failure to toggle the effect', async () => {
+    const api = createCardAPI();
+    vi.mocked(api.getEffectsManager().toggleEffect).mockRejectedValue(new Error());
+    const actionConfig = createEffectAction('snow', 'toggle');
+    const action = new EffectAction({}, actionConfig);
+
+    await expect(action.execute(api)).resolves.toBeUndefined();
+  });
+
   it('should have a no-op stop method', async () => {
     const actionConfig = createEffectAction('snow', 'stop');
     const action = new EffectAction({}, actionConfig);

tests/card-controller/actions/actions/ptz-digital.test.ts

@@ -1,14 +1,25 @@
+import type { ViewContext } from 'view';
 import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest';
 
 import { PTZDigitalAction } from '../../../../src/card-controller/actions/actions/ptz-digital';
+import type { CardController } from '../../../../src/card-controller/controller';
 import type {
   PartialZoomSettings,
   ZoomSettingsObserved,
 } from '../../../../src/components-lib/zoom/types';
 import type { PTZAction } from '../../../../src/config/schema/actions/custom/ptz';
+import { applySetViewModifiers } from '../../../card-controller/view/test-utils';
 import { createCardAPI } from '../../../test-utils';
 import { createView } from '../../../view/test-utils';
 
+const getRequestedZoom = (
+  api: CardController,
+  context?: ViewContext,
+  n = 0,
+): PartialZoomSettings | null | undefined =>
+  applySetViewModifiers(api.getViewManager(), createView({ context }), n).context?.zoom
+    ?.camera?.requested;
+
 describe('should handle ptz digital action', () => {
   const defaultSettings = {
     pan: {
@@ -48,19 +59,12 @@ describe('should handle ptz digital action', () => {
 
     await action.execute(api);
 
-    expect(api.getViewManager().setViewWithMergedContext).toHaveBeenCalledWith({
+    expect(getRequestedZoom(api)).toEqual({
-      zoom: {
+      pan: {
-        camera: {
+        x: 3,
-          observed: undefined,
+        y: 4,
-          requested: {
-            pan: {
-              x: 3,
-              y: 4,
-            },
-            zoom: 2,
-          },
-        },
       },
+      zoom: 2,
     });
   });
 
@@ -78,14 +82,40 @@ describe('should handle ptz digital action', () => {
 
     await action.execute(api);
 
-    expect(api.getViewManager().setViewWithMergedContext).toHaveBeenCalledWith({
+    expect(getRequestedZoom(api)).toEqual({});
-      zoom: {
+  });
-        camera: {
+
-          observed: undefined,
+  // See: https://github.com/dermotduffy/advanced-camera-card/issues/2632
-          requested: {},
+  it('should return to default when chained after an absolute request', async () => {
+    const api = createCardAPI();
+    vi.mocked(api.getViewManager().getView).mockReturnValue(createView());
+
+    await new PTZDigitalAction(
+      {},
+      {
+        action: 'fire-dom-event',
+        advanced_camera_card_action: 'ptz_digital',
+        absolute: {
+          zoom: 3,
         },
       },
-    });
+    ).execute(api);
+
+    await new PTZDigitalAction(
+      {},
+      {
+        action: 'fire-dom-event',
+        advanced_camera_card_action: 'ptz_digital',
+      },
+    ).execute(api);
+
+    // Both actions land on a single view, as they do when an automation runs
+    // them back to back.
+    const view = createView();
+    applySetViewModifiers(api.getViewManager(), view, 0);
+    applySetViewModifiers(api.getViewManager(), view, 1);
+
+    expect(view.context?.zoom?.camera?.requested).toEqual({});
   });
 
   it('should do nothing without a view', async () => {
@@ -102,7 +132,7 @@ describe('should handle ptz digital action', () => {
 
     await action.execute(api);
 
-    expect(api.getViewManager().setViewWithMergedContext).not.toHaveBeenCalledWith();
+    expect(api.getViewManager().setViewWithModifiers).not.toHaveBeenCalled();
   });
 
   it('should do nothing without a camera', async () => {
@@ -126,7 +156,7 @@ describe('should handle ptz digital action', () => {
 
     await action.execute(api);
 
-    expect(api.getViewManager().setViewWithMergedContext).not.toHaveBeenCalled();
+    expect(api.getViewManager().setViewWithModifiers).not.toHaveBeenCalled();
   });
 
   describe('should honor ptz_action', () => {
@@ -339,16 +369,11 @@ describe('should handle ptz digital action', () => {
 
         await action.execute(api);
 
-        expect(api.getViewManager().setViewWithMergedContext).toHaveBeenCalledWith({
+        expect(
-          zoom: {
+          getRequestedZoom(api, { zoom: { camera: { observed: current } } }),
-            camera: {
+        ).toEqual({
-              observed: undefined,
+          ...defaultSettings,
-              requested: {
+          ...expectedSettings,
-                ...defaultSettings,
-                ...expectedSettings,
-              },
-            },
-          },
         });
       },
     );
@@ -380,61 +405,40 @@ describe('should handle ptz digital action', () => {
 
       await action.execute(api);
 
-      expect(api.getViewManager().setViewWithMergedContext).toHaveBeenLastCalledWith({
+      expect(getRequestedZoom(api)).toEqual({
-        zoom: {
+        ...defaultSettings,
-          camera: {
+        pan: {
-            observed: undefined,
+          x: 55,
-            requested: {
+          y: 50,
-              ...defaultSettings,
-              pan: {
-                x: 55,
-                y: 50,
-              },
-            },
-          },
         },
       });
 
       // Update the context to reflect the first step.
+      const observed = createObserved({
+        pan: {
+          x: 55,
+          y: 50,
+        },
+      });
       vi.mocked(api.getViewManager().getView).mockReturnValue(
-        createView({
+        createView({ context: { zoom: { camera: { observed } } } }),
-          context: {
-            zoom: {
-              camera: {
-                observed: createObserved({
-                  pan: {
-                    x: 55,
-                    y: 50,
-                  },
-                }),
-              },
-            },
-          },
-        }),
       );
 
       vi.runOnlyPendingTimers();
 
-      expect(api.getViewManager().setViewWithMergedContext).toHaveBeenLastCalledWith({
+      expect(getRequestedZoom(api, { zoom: { camera: { observed } } }, 1)).toEqual({
-        zoom: {
+        ...defaultSettings,
-          camera: {
+        pan: {
-            observed: undefined,
+          x: 60,
-            requested: {
+          y: 50,
-              ...defaultSettings,
-              pan: {
-                x: 60,
-                y: 50,
-              },
-            },
-          },
         },
       });
-      expect(api.getViewManager().setViewWithMergedContext).toHaveBeenCalledTimes(2);
+      expect(api.getViewManager().setViewWithModifiers).toHaveBeenCalledTimes(2);
 
       action.stop();
       vi.runOnlyPendingTimers();
 
-      expect(api.getViewManager().setViewWithMergedContext).toHaveBeenCalledTimes(2);
+      expect(api.getViewManager().setViewWithModifiers).toHaveBeenCalledTimes(2);
     });
 
     it('stop', async () => {
@@ -450,21 +454,14 @@ describe('should handle ptz digital action', () => {
       });
       await startAction.execute(api);
 
-      expect(api.getViewManager().setViewWithMergedContext).toHaveBeenLastCalledWith({
+      expect(getRequestedZoom(api)).toEqual({
-        zoom: {
+        ...defaultSettings,
-          camera: {
+        pan: {
-            observed: undefined,
+          x: 55,
-            requested: {
+          y: 50,
-              ...defaultSettings,
-              pan: {
-                x: 55,
-                y: 50,
-              },
-            },
-          },
         },
       });
-      expect(api.getViewManager().setViewWithMergedContext).toHaveBeenCalledTimes(1);
+      expect(api.getViewManager().setViewWithModifiers).toHaveBeenCalledTimes(1);
 
       const stopAction = new PTZDigitalAction(context, {
         action: 'fire-dom-event',
@@ -475,7 +472,7 @@ describe('should handle ptz digital action', () => {
 
       vi.runOnlyPendingTimers();
 
-      expect(api.getViewManager().setViewWithMergedContext).toHaveBeenCalledTimes(1);
+      expect(api.getViewManager().setViewWithModifiers).toHaveBeenCalledTimes(1);
     });
   });
 });

tests/card-controller/actions/actions/ptz-multi.test.ts

@@ -4,6 +4,7 @@ import { Capabilities } from '../../../../src/camera-manager/capabilities';
 import { PTZMultiAction } from '../../../../src/card-controller/actions/actions/ptz-multi';
 import { PTZMovementType } from '../../../../src/types';
 import { createCameraManager, createStore } from '../../../camera-manager/test-utils';
+import { applySetViewModifiers } from '../../../card-controller/view/test-utils';
 import { createCardAPI } from '../../../test-utils';
 import { createView } from '../../../view/test-utils';
 
@@ -47,7 +48,7 @@ describe('should handle ptz multi action', () => {
           preset: undefined,
         },
       );
-      expect(api.getViewManager().setViewWithMergedContext).not.toHaveBeenCalled();
+      expect(api.getViewManager().setViewWithModifiers).not.toHaveBeenCalled();
     });
 
     it('should use digital ptz when camera does not have ptz support', async () => {
@@ -77,19 +78,13 @@ describe('should handle ptz multi action', () => {
       await action.execute(api);
 
       expect(api.getCameraManager().executePTZAction).not.toHaveBeenCalled();
-      expect(api.getViewManager().setViewWithMergedContext).toHaveBeenLastCalledWith({
+      const modifiedView = applySetViewModifiers(api.getViewManager(), createView());
-        zoom: {
+      expect(modifiedView.context?.zoom?.['camera.office'].requested).toEqual({
-          'camera.office': {
+        pan: {
-            observed: undefined,
+          x: 55,
-            requested: expect.objectContaining({
+          y: 50,
-              pan: {
-                x: 55,
-                y: 50,
-              },
-              zoom: 1,
-            }),
-          },
         },
+        zoom: 1,
       });
     });
   });
@@ -115,7 +110,7 @@ describe('should handle ptz multi action', () => {
     await action.execute(api);
 
     expect(api.getCameraManager().executePTZAction).not.toHaveBeenCalled();
-    expect(api.getViewManager().setViewWithMergedContext).not.toHaveBeenCalled();
+    expect(api.getViewManager().setViewWithModifiers).not.toHaveBeenCalled();
   });
 
   it('should do nothing with a media-less view without an explicit target_id', async () => {
@@ -144,6 +139,6 @@ describe('should handle ptz multi action', () => {
     await action.execute(api);
 
     expect(api.getCameraManager().executePTZAction).not.toHaveBeenCalled();
-    expect(api.getViewManager().setViewWithMergedContext).not.toHaveBeenCalled();
+    expect(api.getViewManager().setViewWithModifiers).not.toHaveBeenCalled();
   });
 });

tests/card-controller/view/modifiers/zoom-request.test.ts

@@ -0,0 +1,46 @@
+import { expect, it } from 'vitest';
+
+import { ZoomRequestViewModifier } from '../../../../src/card-controller/view/modifiers/zoom-request';
+import { createView } from '../../../view/test-utils';
+
+it('should write a request when the target has no zoom context', () => {
+  const view = createView();
+
+  new ZoomRequestViewModifier('target', { zoom: 3 }).modify(view);
+
+  expect(view.context?.zoom?.target.requested).toEqual({ zoom: 3 });
+});
+
+it('should preserve the observed settings of the target', () => {
+  const observed = {
+    pan: { x: 1, y: 2 },
+    zoom: 3,
+    isDefault: false,
+    unzoomed: false,
+  };
+  const view = createView({ context: { zoom: { target: { observed } } } });
+
+  new ZoomRequestViewModifier('target', { zoom: 4 }).modify(view);
+
+  expect(view.context?.zoom?.target.observed).toEqual(observed);
+});
+
+it('should replace an earlier request rather than merge into it', () => {
+  const view = createView({
+    context: { zoom: { target: { requested: { zoom: 3, pan: { x: 1, y: 2 } } } } },
+  });
+
+  new ZoomRequestViewModifier('target', { pan: { x: 5, y: 6 } }).modify(view);
+
+  expect(view.context?.zoom?.target.requested).toEqual({ pan: { x: 5, y: 6 } });
+});
+
+it('should allow an empty request to erase an earlier request', () => {
+  const view = createView({
+    context: { zoom: { target: { requested: { zoom: 3 } } } },
+  });
+
+  new ZoomRequestViewModifier('target', {}).modify(view);
+
+  expect(view.context?.zoom?.target.requested).toEqual({});
+});

tests/card-controller/view/test-utils.ts

@@ -1,7 +1,10 @@
-import { vi } from 'vitest';
+import { expect, vi } from 'vitest';
 
 import type { CardController } from '../../../src/card-controller/controller';
+import { applyViewModifiers } from '../../../src/card-controller/view/modifiers';
+import type { ViewManagerInterface } from '../../../src/card-controller/view/types';
 import type { RawAdvancedCameraCardConfig } from '../../../src/config/types';
+import type { View } from '../../../src/view/view';
 import {
   createCameraManager,
   createCapabilities,
@@ -42,3 +45,15 @@ export const createPopulatedAPI = (
   vi.mocked(api.getConfigManager().getConfig).mockReturnValue(createConfig(config));
   return api;
 };
+
+// Apply the modifiers from the nth `setViewWithModifiers` call to the given
+// view, so that a caller can assert on the resulting view.
+export const applySetViewModifiers = (
+  viewManager: ViewManagerInterface,
+  view: View,
+  n = 0,
+): View => {
+  const mock = vi.mocked(viewManager.setViewWithModifiers).mock;
+  expect(mock.calls.length).greaterThan(n);
+  return applyViewModifiers(view, mock.calls[n][0]);
+};

tests/card-controller/view/view-manager.test.ts

@@ -4,6 +4,7 @@ import { mock } from 'vitest-mock-extended';
 
 import type { CardController } from '../../../src/card-controller/controller';
 import type { ViewFactory } from '../../../src/card-controller/view/factory';
+import { MergeContextViewModifier } from '../../../src/card-controller/view/modifiers/merge-context';
 import { SetQueryViewModifier } from '../../../src/card-controller/view/modifiers/set-query';
 import type {
   QueryExecutorOptions,
@@ -131,6 +132,26 @@ it('should set view with merged context', () => {
   expect(manager.getView()?.context).toEqual(context);
 });
 
+it('should set view with modifiers', () => {
+  const api = createInitializedCardAPI();
+  const factory = mock<ViewFactory>();
+
+  const manager = new ViewManager(api, { viewFactory: factory });
+  const modifier = new MergeContextViewModifier({ timeline: {} });
+
+  // Applying modifiers with no existing view does nothing.
+  manager.setViewWithModifiers([modifier]);
+  expect(manager.getView()).toBeNull();
+
+  factory.getViewDefault.mockReturnValue(createView({ view: 'live', camera: 'camera' }));
+  manager.setViewDefault();
+  manager.setViewWithModifiers([modifier]);
+
+  expect(manager.getView()?.camera).toBe('camera');
+  expect(manager.getView()?.view).toBe('live');
+  expect(manager.getView()?.context).toEqual({ timeline: {} });
+});
+
 it('should return epoch', () => {
   const factory = mock<ViewFactory>();
   const manager = new ViewManager(createCardAPI(), { viewFactory: factory });