fix: Fix conditions re-triggering incorrectly in certain cases (#1950)

- Closes #1948


[skip ci]

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<void> {
     const hass = this._api.getHASSManager().getHASS();

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;
     };

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;

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 = {
+      triggerData = {
-        ...data,
+        ...triggerData,
-        ...evaluation.data,
+        ...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,44 +145,44 @@ 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 && toState !== fromState) ||
-            ((!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 &&
+              !!toState &&
               (!condition.state ||
                 (Array.isArray(condition.state)
-                    ? condition.state.includes(newState.state[condition.entity].state)
+                  ? condition.state.includes(toState)
-                    : condition.state === newState.state[condition.entity].state)) &&
+                  : condition.state === toState)) &&
               (!condition.state_not ||
                 (Array.isArray(condition.state_not)
-                    ? !condition.state_not.includes(
+                  ? !condition.state_not.includes(toState)
-                        newState.state[condition.entity].state,
+                  : condition.state_not !== toState))),
-                      )
+          ...(fromState !== toState && {
-                    : condition.state_not !== newState.state[condition.entity].state)))),
+            triggerData: {
-          data: {
               state: {
                 entity: condition.entity,
-              ...(oldState?.state?.[condition.entity]?.state && {
+                ...(fromState && { from: fromState }),
-                from: oldState?.state?.[condition.entity]?.state,
+                ...(toState && { to: toState }),
-              }),
-              ...(newState?.state?.[condition.entity]?.state && {
-                to: newState?.state?.[condition.entity]?.state,
-              }),
               },
             },
+          }),
         };
-      case 'view':
+      }
+      case 'view': {
+        const oldView = oldState?.view;
+        const newView = newState?.view;
+
         return {
           result:
-            (!!newState?.view && condition.views?.includes(newState.view)) ||
+            (!!newView && condition.views?.includes(newView)) ||
-            (newState?.view !== oldState?.view && !condition.views?.length),
+            (newView !== oldView && !condition.views?.length),
-          data: {
+          ...(oldView !== newView && {
+            triggerData: {
               ...((oldState?.view || newState?.view) && {
                 view: {
                   ...(oldState?.view && { from: oldState.view }),
@@ -188,7 +190,9 @@ export class ConditionsManager implements ConditionsManagerReadonlyInterface {
                 },
               }),
             },
+          }),
         };
+      }
       case 'fullscreen':
         return {
           result:
@@ -199,12 +203,16 @@ 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)) ||
+            (!!newCamera && !!condition.cameras?.includes(newCamera)) ||
-            (newState?.camera !== oldState?.camera && !condition.cameras?.length),
+            (newCamera !== oldCamera && !condition.cameras?.length),
-          data: {
+          ...(newCamera !== oldCamera && {
+            triggerData: {
               ...((oldState?.camera || newState?.camera) && {
                 camera: {
                   ...(oldState?.camera && { from: oldState?.camera }),
@@ -212,7 +220,9 @@ export class ConditionsManager implements ConditionsManagerReadonlyInterface {
                 },
               }),
             },
+          }),
         };
+      }
       case 'numeric_state':
         return {
           result:
@@ -287,17 +297,21 @@ 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 &&
+            !!newConfig &&
-            newState.config !== oldState?.config &&
+            newConfig !== oldConfig &&
             (!condition.paths?.length ||
               condition.paths.some(
                 (key) =>
-                  getConfigValue(newState.config!, key) !==
+                  getConfigValue(newConfig, key) !==
-                  (oldState?.config ? getConfigValue(oldState?.config, key) : undefined),
+                  (oldConfig ? getConfigValue(oldConfig, key) : undefined),
               )),
-          data: {
+          ...(newConfig !== oldConfig && {
+            triggerData: {
               config: {
                 ...((oldState?.config || newState?.config) && {
                   ...(oldState?.config && { from: oldState?.config }),
@@ -305,6 +319,7 @@ export class ConditionsManager implements ConditionsManagerReadonlyInterface {
                 }),
               },
             },
+          }),
         };
       }
       case 'initialized':

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 {
+export interface ConditionsTriggerData {
-  camera?: ConditionsEvaluationDataFromTo;
+  camera?: ConditionsTriggerDataFromTo;
-  view?: ConditionsEvaluationDataFromTo;
+  view?: ConditionsTriggerDataFromTo;
-  state?: ConditionsEvaluationDataState;
+  state?: ConditionsTriggerDataState;
-  config?: ConditionsEvaluationDataConfig;
+  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;

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);
     });
 

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',