From dbd47c93c1437f934ed5d36374a18e2c047e58e5 Mon Sep 17 00:00:00 2001 From: Dermot Duffy Date: Fri, 7 Mar 2025 20:28:45 -0800 Subject: [PATCH] fix: Fix conditions re-triggering incorrectly in certain cases (#1950) - Closes #1948 [skip ci] --- .../actions/actions-manager.ts | 4 +- src/card-controller/automations-manager.ts | 2 +- src/card-controller/templates/index.ts | 8 +- src/conditions/conditions-manager.ts | 157 ++++++++++-------- src/conditions/types.ts | 25 ++- tests/conditions/conditions-manager.test.ts | 112 +++++++++++-- tests/templates/index.test.ts | 4 +- 7 files changed, 210 insertions(+), 102 deletions(-) diff --git a/src/card-controller/actions/actions-manager.ts b/src/card-controller/actions/actions-manager.ts index db052e93..9c7b9cd1 100644 --- a/src/card-controller/actions/actions-manager.ts +++ b/src/card-controller/actions/actions-manager.ts @@ -1,6 +1,6 @@ import { ActionContext } from 'action'; import { z } from 'zod'; -import { ConditionsEvaluationData } from '../../conditions/types.js'; +import { ConditionsTriggerData } from '../../conditions/types.js'; import { Actions, ActionsConfig, ActionType } from '../../config/types.js'; import { getActionConfigGivenAction } from '../../utils/action.js'; import { TemplateRenderer } from '../templates/index.js'; @@ -115,7 +115,7 @@ export class ActionsManager { action: ActionType | ActionType[], options?: { config?: AuxillaryActionConfig; - triggerData?: ConditionsEvaluationData; + triggerData?: ConditionsTriggerData; }, ): Promise { const hass = this._api.getHASSManager().getHASS(); diff --git a/src/card-controller/automations-manager.ts b/src/card-controller/automations-manager.ts index c051c3fb..67635cd6 100644 --- a/src/card-controller/automations-manager.ts +++ b/src/card-controller/automations-manager.ts @@ -75,7 +75,7 @@ export class AutomationsManager { await this._api .getActionsManager() - .executeActions(actions, { triggerData: result.data }); + .executeActions(actions, { triggerData: result.triggerData }); --this._nestedAutomationExecutions; }; diff --git a/src/card-controller/templates/index.ts b/src/card-controller/templates/index.ts index 94e18297..8f17990a 100644 --- a/src/card-controller/templates/index.ts +++ b/src/card-controller/templates/index.ts @@ -1,12 +1,12 @@ import { HomeAssistant } from '@dermotduffy/custom-card-helpers'; import { HASS, renderTemplate } from 'ha-nunjucks/dist'; -import { ConditionsEvaluationData, ConditionState } from '../../conditions/types'; +import { ConditionsTriggerData, ConditionState } from '../../conditions/types'; import { ActionType } from '../../config/types'; interface TemplateContextInternal { camera?: string; view?: string; - trigger?: ConditionsEvaluationData; + trigger?: ConditionsTriggerData; } interface TemplateContext { @@ -22,7 +22,7 @@ export class TemplateRenderer { data: unknown, options?: { conditionState?: ConditionState; - triggerData?: ConditionsEvaluationData; + triggerData?: ConditionsTriggerData; }, ): ActionType => { return this._renderTemplateRecursively( @@ -37,7 +37,7 @@ export class TemplateRenderer { protected _conditionStateToTemplateContext( conditionState?: ConditionState, - triggerData?: ConditionsEvaluationData, + triggerData?: ConditionsTriggerData, ): TemplateContext | undefined { if (!conditionState?.camera && !conditionState?.view && !triggerData) { return; diff --git a/src/conditions/conditions-manager.ts b/src/conditions/conditions-manager.ts index 085aff63..487debee 100644 --- a/src/conditions/conditions-manager.ts +++ b/src/conditions/conditions-manager.ts @@ -1,15 +1,14 @@ -import { isEqual } from 'lodash-es'; import { getConfigValue } from '../config/management'; import { AdvancedCameraCardCondition } from '../config/types'; import { isCompanionApp } from '../utils/companion'; import { - ConditionsEvaluationData, ConditionsEvaluationResult, ConditionsListener, ConditionsManagerReadonlyInterface, ConditionState, ConditionStateChange, ConditionStateManagerReadonlyInterface, + ConditionsTriggerData, } from './types'; /** @@ -104,7 +103,7 @@ export class ConditionsManager implements ConditionsManagerReadonlyInterface { const state = options?.stateChange?.new ?? this._stateManager?.getState(); let result = true; - let data: ConditionsEvaluationData = {}; + let triggerData: ConditionsTriggerData = {}; for (const condition of this._conditions) { const evaluation = this._evaluateCondition( @@ -116,17 +115,20 @@ export class ConditionsManager implements ConditionsManagerReadonlyInterface { result = false; break; } - data = { - ...data, - ...evaluation.data, + triggerData = { + ...triggerData, + ...evaluation.triggerData, }; } const evaluation: ConditionsEvaluationResult = result - ? { result, data } + ? { result, triggerData } : { result }; - if (!isEqual(evaluation, this._evaluation)) { + if ( + evaluation.result !== this._evaluation.result || + (evaluation.triggerData && Object.keys(evaluation.triggerData).length) + ) { this._evaluation = evaluation; if (options?.callListeners ?? true) { this._listeners.forEach( @@ -143,52 +145,54 @@ export class ConditionsManager implements ConditionsManagerReadonlyInterface { ): ConditionsEvaluationResult { switch (condition.condition) { case undefined: - case 'state': + case 'state': { + const fromState = oldState?.state?.[condition.entity]?.state; + const toState = newState?.state?.[condition.entity]?.state; + return { result: - !!newState?.state && - ((!condition.state && - !condition.state_not && - newState.state[condition.entity]?.state !== - oldState?.state?.[condition.entity]?.state) || - ((!!condition.state || !!condition.state_not) && - condition.entity in newState.state && - (!condition.state || - (Array.isArray(condition.state) - ? condition.state.includes(newState.state[condition.entity].state) - : condition.state === newState.state[condition.entity].state)) && - (!condition.state_not || - (Array.isArray(condition.state_not) - ? !condition.state_not.includes( - newState.state[condition.entity].state, - ) - : condition.state_not !== newState.state[condition.entity].state)))), - data: { - state: { - entity: condition.entity, - ...(oldState?.state?.[condition.entity]?.state && { - from: oldState?.state?.[condition.entity]?.state, - }), - ...(newState?.state?.[condition.entity]?.state && { - to: newState?.state?.[condition.entity]?.state, + (!condition.state && !condition.state_not && toState !== fromState) || + ((!!condition.state || !!condition.state_not) && + !!toState && + (!condition.state || + (Array.isArray(condition.state) + ? condition.state.includes(toState) + : condition.state === toState)) && + (!condition.state_not || + (Array.isArray(condition.state_not) + ? !condition.state_not.includes(toState) + : condition.state_not !== toState))), + ...(fromState !== toState && { + triggerData: { + state: { + entity: condition.entity, + ...(fromState && { from: fromState }), + ...(toState && { to: toState }), + }, + }, + }), + }; + } + case 'view': { + const oldView = oldState?.view; + const newView = newState?.view; + + return { + result: + (!!newView && condition.views?.includes(newView)) || + (newView !== oldView && !condition.views?.length), + ...(oldView !== newView && { + triggerData: { + ...((oldState?.view || newState?.view) && { + view: { + ...(oldState?.view && { from: oldState.view }), + ...(newState?.view && { to: newState.view }), + }, }), }, - }, - }; - case 'view': - return { - result: - (!!newState?.view && condition.views?.includes(newState.view)) || - (newState?.view !== oldState?.view && !condition.views?.length), - data: { - ...((oldState?.view || newState?.view) && { - view: { - ...(oldState?.view && { from: oldState.view }), - ...(newState?.view && { to: newState.view }), - }, - }), - }, + }), }; + } case 'fullscreen': return { result: @@ -199,20 +203,26 @@ export class ConditionsManager implements ConditionsManagerReadonlyInterface { return { result: newState?.expand !== undefined && condition.expand === newState.expand, }; - case 'camera': + case 'camera': { + const oldCamera = oldState?.camera; + const newCamera = newState?.camera; + return { result: - (!!newState?.camera && !!condition.cameras?.includes(newState.camera)) || - (newState?.camera !== oldState?.camera && !condition.cameras?.length), - data: { - ...((oldState?.camera || newState?.camera) && { - camera: { - ...(oldState?.camera && { from: oldState?.camera }), - ...(newState?.camera && { to: newState?.camera }), - }, - }), - }, + (!!newCamera && !!condition.cameras?.includes(newCamera)) || + (newCamera !== oldCamera && !condition.cameras?.length), + ...(newCamera !== oldCamera && { + triggerData: { + ...((oldState?.camera || newState?.camera) && { + camera: { + ...(oldState?.camera && { from: oldState?.camera }), + ...(newState?.camera && { to: newState?.camera }), + }, + }), + }, + }), }; + } case 'numeric_state': return { result: @@ -287,24 +297,29 @@ export class ConditionsManager implements ConditionsManagerReadonlyInterface { new RegExp(condition.user_agent_re).test(newState.userAgent)), }; case 'config': { + const newConfig = newState?.config; + const oldConfig = oldState?.config; + return { result: - !!newState?.config && - newState.config !== oldState?.config && + !!newConfig && + newConfig !== oldConfig && (!condition.paths?.length || condition.paths.some( (key) => - getConfigValue(newState.config!, key) !== - (oldState?.config ? getConfigValue(oldState?.config, key) : undefined), + getConfigValue(newConfig, key) !== + (oldConfig ? getConfigValue(oldConfig, key) : undefined), )), - data: { - config: { - ...((oldState?.config || newState?.config) && { - ...(oldState?.config && { from: oldState?.config }), - ...(newState?.config && { to: newState?.config }), - }), + ...(newConfig !== oldConfig && { + triggerData: { + config: { + ...((oldState?.config || newState?.config) && { + ...(oldState?.config && { from: oldState?.config }), + ...(newState?.config && { to: newState?.config }), + }), + }, }, - }, + }), }; } case 'initialized': diff --git a/src/conditions/types.ts b/src/conditions/types.ts index b3f7f182..84cc3995 100644 --- a/src/conditions/types.ts +++ b/src/conditions/types.ts @@ -36,29 +36,36 @@ export interface ConditionStateManagerReadonlyInterface { getState(): ConditionState; } -interface ConditionsEvaluationDataFromTo { +interface ConditionsTriggerDataFromTo { from?: string; to?: string; } -interface ConditionsEvaluationDataState extends ConditionsEvaluationDataFromTo { +interface ConditionsTriggerDataState extends ConditionsTriggerDataFromTo { entity: string; } -interface ConditionsEvaluationDataConfig { +interface ConditionsTriggerDataConfig { from?: AdvancedCameraCardConfig; to?: AdvancedCameraCardConfig; } -export interface ConditionsEvaluationData { - camera?: ConditionsEvaluationDataFromTo; - view?: ConditionsEvaluationDataFromTo; - state?: ConditionsEvaluationDataState; - config?: ConditionsEvaluationDataConfig; +export interface ConditionsTriggerData { + camera?: ConditionsTriggerDataFromTo; + view?: ConditionsTriggerDataFromTo; + state?: ConditionsTriggerDataState; + config?: ConditionsTriggerDataConfig; } export interface ConditionsEvaluationResult { result: boolean; - data?: ConditionsEvaluationData; + + // Trigger data is only provided if there was a real change of state (For + // example: if a state condition that matches 'on' previously evaluated to + // true, and a call from the state manager arrives for an unrelated hass state + // update, the condition will still evaluate true, but there won't be any + // trigger data provided since the state relevant to the condition did not + // change). + triggerData?: ConditionsTriggerData; } export type ConditionsListener = (result: ConditionsEvaluationResult) => void; diff --git a/tests/conditions/conditions-manager.test.ts b/tests/conditions/conditions-manager.test.ts index 9207aae8..8d7bf99b 100644 --- a/tests/conditions/conditions-manager.test.ts +++ b/tests/conditions/conditions-manager.test.ts @@ -42,7 +42,7 @@ describe('ConditionsManager', () => { stateManager.setState({ view: 'clips' }); expect(listener).toHaveBeenLastCalledWith({ result: true, - data: { + triggerData: { view: { to: 'clips', }, @@ -52,7 +52,7 @@ describe('ConditionsManager', () => { stateManager.setState({ view: 'timeline' }); expect(listener).toHaveBeenLastCalledWith({ result: true, - data: { + triggerData: { view: { from: 'clips', to: 'timeline', @@ -62,6 +62,23 @@ describe('ConditionsManager', () => { expect(listener).toBeCalledTimes(2); }); + + it('should not re-trigger without a real change', () => { + const stateManager = new ConditionStateManager(); + const manager = new ConditionsManager( + [{ condition: 'view' as const }], + stateManager, + ); + + const listener = vi.fn(); + manager.addListener(listener); + + stateManager.setState({ view: 'clips' }); + expect(listener).toHaveBeenCalledTimes(1); + + stateManager.setState({ view: 'clips' }); + expect(listener).toHaveBeenCalledTimes(1); + }); }); it('with fullscreen condition', () => { @@ -120,7 +137,7 @@ describe('ConditionsManager', () => { stateManager.setState({ camera: 'bar' }); expect(listener).toHaveBeenLastCalledWith({ result: true, - data: { + triggerData: { camera: { to: 'bar', }, @@ -130,7 +147,7 @@ describe('ConditionsManager', () => { stateManager.setState({ camera: 'foo' }); expect(listener).toHaveBeenLastCalledWith({ result: true, - data: { + triggerData: { camera: { from: 'bar', to: 'foo', @@ -140,6 +157,23 @@ describe('ConditionsManager', () => { expect(listener).toBeCalledTimes(2); }); + + it('should not re-trigger without a real change', () => { + const stateManager = new ConditionStateManager(); + const manager = new ConditionsManager( + [{ condition: 'camera' as const }], + stateManager, + ); + + const listener = vi.fn(); + manager.addListener(listener); + + stateManager.setState({ camera: 'bar' }); + expect(listener).toHaveBeenCalledTimes(1); + + stateManager.setState({ camera: 'bar' }); + expect(listener).toHaveBeenCalledTimes(1); + }); }); describe('with stock HA conditions', () => { @@ -163,7 +197,7 @@ describe('ConditionsManager', () => { }); expect(listener).toBeCalledWith({ result: true, - data: { + triggerData: { state: { entity: 'binary_sensor.foo', to: 'on', @@ -177,7 +211,7 @@ describe('ConditionsManager', () => { }); expect(listener).toBeCalledWith({ result: true, - data: { + triggerData: { state: { entity: 'binary_sensor.foo', from: 'on', @@ -336,7 +370,7 @@ describe('ConditionsManager', () => { }); expect(listener).toHaveBeenLastCalledWith({ result: true, - data: { + triggerData: { // Only the last matching state will be included in the data. state: { entity: 'switch.two', @@ -354,7 +388,7 @@ describe('ConditionsManager', () => { expect(listener).toHaveBeenLastCalledWith({ result: true, - data: { + triggerData: { // Only the last matching state will be included in the data. state: { entity: 'switch.two', @@ -366,6 +400,41 @@ describe('ConditionsManager', () => { expect(listener).toBeCalledTimes(2); }); + + it('should not re-trigger without a real change', () => { + const stateManager = new ConditionStateManager(); + const manager = new ConditionsManager( + [{ condition: 'state' as const, entity: 'switch.one' }], + stateManager, + ); + + const listener = vi.fn(); + manager.addListener(listener); + + stateManager.setState({ + state: { + 'switch.one': createStateEntity({ state: 'on' }), + }, + }); + expect(listener).toBeCalledTimes(1); + expect(manager.getEvaluation()).toEqual({ + result: true, + triggerData: { + // Only the last matching state will be included in the data. + state: { + entity: 'switch.one', + to: 'on', + }, + }, + }); + + stateManager.setState({ + state: { + 'switch.one': createStateEntity({ state: 'on' }), + }, + }); + expect(listener).toBeCalledTimes(1); + }); }); describe('with numeric state condition', () => { @@ -526,7 +595,7 @@ describe('ConditionsManager', () => { addEventListener.mock.calls[0][1](); // This should result in a callback to our state listener. - expect(callback).toBeCalledWith({ result: true, data: {} }); + expect(callback).toBeCalledWith({ result: true, triggerData: {} }); // Destroy the manager and ensure the event listener is removed. manager.destroy(); @@ -887,7 +956,7 @@ describe('ConditionsManager', () => { stateManager.setState({ config: config_1 }); expect(listener).toHaveBeenLastCalledWith({ result: true, - data: { + triggerData: { config: { to: config_1, }, @@ -897,7 +966,7 @@ describe('ConditionsManager', () => { stateManager.setState({ config: config_2 }); expect(listener).toHaveBeenLastCalledWith({ result: true, - data: { + triggerData: { config: { from: config_1, to: config_2, @@ -971,6 +1040,23 @@ describe('ConditionsManager', () => { stateManager.setState({ fullscreen: true }); expect(listener).toBeCalledTimes(2); }); + + it('should not re-trigger without a real change', () => { + const stateManager = new ConditionStateManager(); + const manager = new ConditionsManager( + [{ condition: 'config' as const }], + stateManager, + ); + + const listener = vi.fn(); + manager.addListener(listener); + + stateManager.setState({ config: config_1 }); + expect(listener).toHaveBeenCalledTimes(1); + + stateManager.setState({ config: config_1 }); + expect(listener).toHaveBeenCalledTimes(1); + }); }); it('with initialized condition', () => { @@ -1001,7 +1087,7 @@ describe('ConditionsManager', () => { stateManager.setState({ fullscreen: true }); - expect(listener).toBeCalledWith({ result: true, data: {} }); + expect(listener).toBeCalledWith({ result: true, triggerData: {} }); expect(listener).toBeCalledTimes(1); stateManager.setState({ fullscreen: false }); @@ -1013,7 +1099,7 @@ describe('ConditionsManager', () => { stateManager.setState({ fullscreen: true }); - expect(listener).toBeCalledWith({ result: true, data: {} }); + expect(listener).toBeCalledWith({ result: true, triggerData: {} }); expect(listener).toBeCalledTimes(3); }); diff --git a/tests/templates/index.test.ts b/tests/templates/index.test.ts index 29c9800d..226fd84a 100644 --- a/tests/templates/index.test.ts +++ b/tests/templates/index.test.ts @@ -1,7 +1,7 @@ import { renderTemplate } from 'ha-nunjucks'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { TemplateRenderer } from '../../src/card-controller/templates'; -import { ConditionsEvaluationData, ConditionState } from '../../src/conditions/types'; +import { ConditionsTriggerData, ConditionState } from '../../src/conditions/types'; import { createHASS } from '../test-utils'; // ha-nunjucks attempts to make websocket calls initially so mock it out. @@ -46,7 +46,7 @@ describe('TemplateRenderer', () => { camera: 'camera', view: 'view', }; - const triggerData: ConditionsEvaluationData = { + const triggerData: ConditionsTriggerData = { camera: { to: 'camera', from: 'previous-camera',