From 8512df1720de42b605c1b2e08f3d65392f0d5108 Mon Sep 17 00:00:00 2001 From: Dermot Duffy Date: Sat, 22 Mar 2025 18:39:36 -0700 Subject: [PATCH] fix: Prevent "action loops" (#1978) - Closes: #1967 - Closes: #1969 --- .../actions/actions-manager.ts | 16 +++++++++++-- .../actions/actions/ptz-digital.ts | 4 ++-- src/card-controller/actions/actions/ptz.ts | 21 +++++++++++++---- .../actions/utils/action-state.ts | 8 +++---- .../actions/actions-manager.test.ts | 23 ++++++++++++++++++- .../actions/actions/ptz.test.ts | 18 +++++++++++++-- 6 files changed, 75 insertions(+), 15 deletions(-) diff --git a/src/card-controller/actions/actions-manager.ts b/src/card-controller/actions/actions-manager.ts index 9d3b8000..ec8b4864 100644 --- a/src/card-controller/actions/actions-manager.ts +++ b/src/card-controller/actions/actions-manager.ts @@ -8,7 +8,10 @@ import { AuxillaryActionConfig, } from '../../config/schema/actions/types.js'; import { forwardHaptic } from '../../ha/haptic.js'; -import { getActionConfigGivenAction } from '../../utils/action.js'; +import { + getActionConfigGivenAction, + isAdvancedCameraCardCustomAction, +} from '../../utils/action.js'; import { allPromises } from '../../utils/basic.js'; import { TemplateRenderer } from '../templates/index.js'; import { CardActionsManagerAPI } from '../types.js'; @@ -101,7 +104,16 @@ export class ActionsManager { // https://github.com/custom-cards/custom-card-helpers/blob/master/src/fire-event.ts#L70 return; } - await this.executeActions(ev.detail as ActionConfig); + const action: ActionConfig = ev.detail; + + // If the received action is not a custom action specifically for this card + // to handle, ignore it. Otherwise, we can get action "loops". See: + // https://github.com/dermotduffy/advanced-camera-card/issues/1969 + if (!isAdvancedCameraCardCustomAction(action)) { + return; + } + + await this.executeActions(action); }; /** diff --git a/src/card-controller/actions/actions/ptz-digital.ts b/src/card-controller/actions/actions/ptz-digital.ts index e15b8b38..9de03da0 100644 --- a/src/card-controller/actions/actions/ptz-digital.ts +++ b/src/card-controller/actions/actions/ptz-digital.ts @@ -67,7 +67,7 @@ export class PTZDigitalAction extends AdvancedCameraCardAction { protected _timer = new Timer(); + protected _stopped = false; public async stop(): Promise { + this._stopped = true; this._timer.stop(); } @@ -79,7 +81,7 @@ export class PTZAction extends AdvancedCameraCardAction { if (this._action.ptz_phase === 'start') { // Scenario: Asked to start a continuous move, camera only supports relative moves natively. - stopInProgressForThisTarget(ptzCameraID, this._context.ptz); + await stopInProgressForThisTarget(ptzCameraID, this._context.ptz); setInProgressForThisTarget(ptzCameraID, this._context, 'ptz', this); const singleStep = async (): Promise => { @@ -89,15 +91,26 @@ export class PTZAction extends AdvancedCameraCardAction { .executePTZAction(ptzCameraID, this._action.ptz_action, { preset: this._action.ptz_preset, })); - // Only start the timer for the next step after this step returns. - this._timer.start(ptzConfiguration.r2c_delay_between_calls_seconds, singleStep); + + if (!this._stopped) { + // Only start the timer for the next step after this step returns, and + // only if this action has not been stopped. + // See: https://github.com/dermotduffy/advanced-camera-card/issues/1967 + this._timer.start( + ptzConfiguration.r2c_delay_between_calls_seconds, + singleStep, + ); + } }; + this._stopped = false; await singleStep(); } else if (this._action.ptz_phase === 'stop') { // Scenario: Asked to stop continuous move, camera only supports relative moves natively. - stopInProgressForThisTarget(ptzCameraID, this._context.ptz); + await stopInProgressForThisTarget(ptzCameraID, this._context.ptz); } else { + this._stopped = false; + // Relative move (but camera only supports continuous). await api .getCameraManager() diff --git a/src/card-controller/actions/utils/action-state.ts b/src/card-controller/actions/utils/action-state.ts index 3884800f..39a6bbe0 100644 --- a/src/card-controller/actions/utils/action-state.ts +++ b/src/card-controller/actions/utils/action-state.ts @@ -2,11 +2,11 @@ import merge from 'lodash-es/merge'; import { Action, TargetedActionContext } from '../types'; import { ActionContext } from 'action'; -export const stopInProgressForThisTarget = ( +export const stopInProgressForThisTarget = async ( targetID: string, context?: TargetedActionContext, -): void => { - context?.[targetID]?.inProgressAction?.stop(); +): Promise => { + await context?.[targetID]?.inProgressAction?.stop(); }; export const setInProgressForThisTarget = ( @@ -14,7 +14,7 @@ export const setInProgressForThisTarget = ( context: ActionContext, contextKey: keyof ActionContext, action: Action, -) => { +): void => { merge(context, { [contextKey]: { [targetID]: { diff --git a/tests/card-controller/actions/actions-manager.test.ts b/tests/card-controller/actions/actions-manager.test.ts index 3d227565..a9313ee4 100644 --- a/tests/card-controller/actions/actions-manager.test.ts +++ b/tests/card-controller/actions/actions-manager.test.ts @@ -206,7 +206,7 @@ describe('ActionsManager', () => { vi.restoreAllMocks(); }); - it('should handle event', async () => { + it('should handle advanced camera card event', async () => { const action = createLogAction('Hello, world!'); const event = new CustomEvent('ll-custom', { detail: action, @@ -220,6 +220,27 @@ describe('ActionsManager', () => { expect(consoleSpy).toBeCalled(); }); + it('should not handle generic event', async () => { + const event = new CustomEvent('ll-custom', { + detail: { + type: 'fire-dom-event', + foo: 'bar', + }, + }); + + const card = document.createElement('div'); + const handler = vi.fn(); + card.addEventListener('ll-custom', handler); + + const api = createCardAPI(); + vi.mocked(api.getCardElementManager().getElement).mockReturnValue(card); + const manager = new ActionsManager(api); + + await manager.handleCustomActionEvent(event); + + expect(handler).not.toBeCalled(); + }); + it('should not handle event without detail', async () => { const manager = new ActionsManager(createCardAPI()); diff --git a/tests/card-controller/actions/actions/ptz.test.ts b/tests/card-controller/actions/actions/ptz.test.ts index 36a6f279..bfe98082 100644 --- a/tests/card-controller/actions/actions/ptz.test.ts +++ b/tests/card-controller/actions/actions/ptz.test.ts @@ -478,7 +478,8 @@ describe('should handle ptz action', () => { }), }, ]); - vi.mocked(api.getCameraManager).mockReturnValue(createCameraManager(store)); + const cameraManager = createCameraManager(store); + vi.mocked(api.getCameraManager).mockReturnValue(cameraManager); vi.mocked(api.getViewManager().getView).mockReturnValue( createView({ camera: 'camera.office' }), ); @@ -497,11 +498,24 @@ describe('should handle ptz action', () => { await vi.runOnlyPendingTimersAsync(); expect(api.getCameraManager().executePTZAction).toBeCalledTimes(2); + // Emulate the stop being called while the action is running, but before + // the *next* timer is scheduled. + let resolve: () => void; + const promise: Promise = new Promise((_resolve) => { + resolve = _resolve; + }); + vi.mocked(cameraManager.executePTZAction).mockReturnValueOnce(promise); + + await vi.runOnlyPendingTimersAsync(); + expect(api.getCameraManager().executePTZAction).toBeCalledTimes(3); + action.stop(); + + resolve!(); await vi.runOnlyPendingTimersAsync(); // There should be no additional calls. - expect(api.getCameraManager().executePTZAction).toBeCalledTimes(2); + expect(api.getCameraManager().executePTZAction).toBeCalledTimes(3); }); }); });