fix: prevent infinite re-render loop from issue retry callbacks (#2550)

src/card-controller/issues/issue-manager.ts

@@ -1,6 +1,7 @@
 import type { IssueTriggerContext } from 'issue';
 
 import type { ConditionStateChange } from '../../condition-trigger/conditions/types';
+import { contentsChanged, ignoreFunctionIdentity } from '../../utils/basic';
 import { isActionAllowedBasedOnInteractionState } from '../../utils/interaction-mode';
 import { RetryTimer } from '../../utils/retry-timer';
 import type { CardIssueManagerAPI } from '../types';
@@ -8,6 +9,7 @@ import { IssueStateManager } from './state-manager';
 import type {
   Issue,
   IssueKey,
+  IssuePresence,
   IssueReadOnlyState,
   IssueTriggerContextKey,
 } from './types';
@@ -34,11 +36,9 @@ export class IssueManager {
   });
   private _suspended = false;
 
-  // Reentrancy guard: evaluate() calls setState() on the condition state
-  // manager, which fires listeners synchronously -- including the one
-  // registered in this constructor. Without this guard, detectDynamic()
-  // and presence computation would run twice per evaluation.
-  private _evaluating = false;
+  // The issue presence from the last evaluation, compared against on the next
+  // one to decide whether a re-render is needed.
+  private _lastPresence: IssuePresence = new Map();
 
   constructor(api: CardIssueManagerAPI) {
     this._api = api;
@@ -71,40 +71,42 @@ export class IssueManager {
     this.evaluate();
   }
 
-  // Evaluate all dynamic issues against current state, then react to any
-  // changes: notify, update condition state, and schedule retries.
+  // Evaluate all dynamic issues against current state, re-render the card if
+  // the issue presence changed, and schedule retries.
   //
-  // Detection of "anything changed" is delegated to the condition state
-  // manager: IssuePresence is a Map<IssueKey, IssueDescription>, so its
-  // deep equality check naturally catches both presence-set churn (issues
+  // IssuePresence is a Map<IssueKey, IssueDescription>, so comparing the new
+  // presence against the previous one catches both presence-set churn (issues
   // appearing/disappearing) and content-level churn (an issue swapping
-  // sub-states without changing its key, e.g. ConnectionIssue going from
-  // 'lost' to 'starting').
+  // sub-states without changing its key, e.g. ConnectionIssue going from 'lost'
+  // to 'starting').
   public evaluate(): void {
-    if (this._suspended || this._evaluating) {
+    if (this._suspended) {
       return;
     }
 
-    this._evaluating = true;
-    try {
-      const state = this._api.getConditionStateManager().getState();
-      this._stateManager.detectDynamic(state);
+    const state = this._api.getConditionStateManager().getState();
+    this._stateManager.detectDynamic(state);
 
-      if (
-        this._api.getConditionStateManager().setState({
-          issues: this._stateManager.getIssuePresence(),
-        })
-      ) {
-        // Re-render to show the change. The re-render also re-attempts
-        // initialization, which matters when a blocking notice like "Home
-        // Assistant is starting" clears and the card can finally initialize.
-        this._api.getCardElementManager().update();
-      }
+    // getIssuePresence() rebuilds notifications fresh each call, so a retry
+    // control embeds a new callback closure every time; ignore that identity
+    // churn so only an observable change in the issue set or its content
+    // triggers a re-render.
+    const presence = this._stateManager.getIssuePresence();
+    const changed = contentsChanged(
+      presence,
+      this._lastPresence,
+      ignoreFunctionIdentity,
+    );
+    this._lastPresence = presence;
 
-      this._scheduleRetryIfNeeded();
-    } finally {
-      this._evaluating = false;
+    if (changed) {
+      // Re-render to show the change. The re-render also re-attempts
+      // initialization, which matters when a blocking notice like "Home
+      // Assistant is starting" clears and the card can finally initialize.
+      this._api.getCardElementManager().update();
     }
+
+    this._scheduleRetryIfNeeded();
   }
 
   // Attempts a retry for the given issue. Pass `force = true` for user-

src/card-controller/issues/types.ts

@@ -30,10 +30,9 @@ export interface KeyedIssueDescription {
 
 // Map of currently active issues keyed by IssueKey, with each entry's value
 // being the issue's current rendered description. Stored as a Map (not just a
-// Set of keys) so that sub-state changes within an issue -- e.g.
-// ConnectionIssue swapping between 'lost' and 'starting' -- are reflected as
-// real value-level diffs to the condition state, triggering re-renders and any
-// user-defined conditions that depend on issue state.
+// Set of keys) so that sub-state changes within an issue -- e.g. ConnectionIssue
+// swapping between 'lost' and 'starting' -- surface as value-level diffs when
+// IssueManager compares presence between evaluations, triggering a re-render.
 export type IssuePresence = Map<IssueKey, IssueDescription>;
 export interface IssueReadOnlyState {
   hasFullCardIssue(): boolean;

src/condition-trigger/conditions/types.ts

@@ -1,4 +1,3 @@
-import type { IssuePresence } from '../../card-controller/issues/types';
 import type { KeysState, MicrophoneState } from '../../card-controller/types';
 import type { AdvancedCameraCardView } from '../../config/schema/common/const';
 import type { ViewDisplayMode } from '../../config/schema/common/display';
@@ -6,6 +5,16 @@ import type { AdvancedCameraCardConfig } from '../../config/schema/types';
 import type { HomeAssistant } from '../../ha/types';
 import type { MediaLoadedInfo } from '../../types';
 
+// ConditionStateManager checks each written field with lodash isEqual. Prefer
+// plain immutable data: functions (callbacks) compare by identity, and opaque
+// objects get deep-walked through their enumerable state, which is rarely a
+// meaningful equality contract. Store such a value only when its reference is
+// the intended state (`mediaLoadedInfo`'s MediaPlayerController, which
+// consumers also reference-compare) or its identity is stable across writes
+// (`hass`'s methods).
+//
+// Counterexample: Rebuilding an equivalent function callback each write making
+// the field look changed when nothing observable did.
 export interface ConditionState {
   call?: boolean;
   camera?: string;
@@ -22,7 +31,6 @@ export interface ConditionState {
   mediaLoadedInfo?: MediaLoadedInfo | null;
   microphone?: MicrophoneState;
   panel?: boolean;
-  issues?: IssuePresence;
   hass?: HomeAssistant;
 
   // Generic media target identifier. See @view/target-id for details.

src/utils/basic.ts

@@ -329,6 +329,16 @@ export const generateFloatApproximatelyEqualsCustomizer = (
   };
 };
 
+// For change-detection equality: a function's identity churns and is not
+// observable state, so two functions compare equal; a function appearing or
+// disappearing is still a real change. Only apply where a data field beside the
+// callback carries the meaningful change, not where a function's identity is
+// itself the state (e.g. a controller keyed by which element it wraps).
+export const ignoreFunctionIdentity = (a: unknown, b: unknown): boolean | undefined =>
+  typeof a === 'function' || typeof b === 'function'
+    ? typeof a === 'function' && typeof b === 'function'
+    : undefined;
+
 export const convertHTTPAdressToWebsocket = (url: string): string => {
   return url.replace(/^http/i, 'ws');
 };

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

@@ -28,6 +28,7 @@ import {
   createHASS,
   createMockTemplateRenderer,
   createView,
+  stubConnectedHomeAssistant,
 } from '../../test-utils';
 
 const createAPI = (): CardController => {
@@ -38,6 +39,7 @@ const createAPI = (): CardController => {
   return api;
 };
 
+// @vitest-environment jsdom
 describe('ActionsManager', () => {
   describe('getMergedActions', () => {
     const config = {
@@ -176,8 +178,13 @@ describe('ActionsManager', () => {
     });
   });
 
-  // @vitest-environment jsdom
   describe('handleInteractionEvent', () => {
+    // Templated actions render through ha-nunjucks, which polls (via setTimeout)
+    // for a connected `home-assistant` element until ready. Stubbing one makes
+    // it resolve synchronously, so no retry timer leaks past test teardown.
+    beforeAll(() => {
+      stubConnectedHomeAssistant();
+    });
     beforeEach(() => {
       vi.restoreAllMocks();
     });

tests/card-controller/issues/issue-manager.test.ts

@@ -8,6 +8,7 @@ import {
   RETRY_EXPONENTIAL_BASE_SECONDS,
   RETRY_EXPONENTIAL_MAX_SECONDS,
 } from '../../../src/card-controller/issues/issue-manager';
+import { createRetryControl } from '../../../src/card-controller/issues/retry-control';
 import type {
   Issue,
   IssueDescription,
@@ -52,7 +53,6 @@ const createRetriableSetup = (options?: {
   issue: Issue;
 } => {
   const api = createCardAPI();
-  vi.mocked(api.getConditionStateManager().getState).mockReturnValue({});
 
   if (options?.hasInteraction !== undefined) {
     vi.mocked(api.getInteractionManager().hasInteraction).mockReturnValue(
@@ -178,7 +178,6 @@ describe('IssueManager', () => {
   describe('trigger', () => {
     it('should trigger the issue and call evaluate', () => {
       const api = createCardAPI();
-      vi.mocked(api.getConditionStateManager().getState).mockReturnValue({});
 
       const manager = new IssueManager(api);
 
@@ -192,17 +191,15 @@ describe('IssueManager', () => {
       expect(issue.trigger).toBeCalledWith({ error: expect.any(Error) });
     });
 
-    it('should update presence even when state was mutated before detectDynamic', () => {
+    it('should update the card even when state was mutated before detectDynamic', () => {
       const api = createCardAPI();
-      vi.mocked(api.getConditionStateManager().getState).mockReturnValue({});
-      vi.mocked(api.getConditionStateManager().setState).mockReturnValue(true);
 
       const manager = new IssueManager(api);
 
       // hasIssue returns true from the start -- simulates trigger() having
       // already mutated state before detectDynamic snapshots. The before/after
       // check inside detectDynamic sees true→true (no transition), but the
-      // presence comparison against ConditionState must still detect the
+      // presence comparison against the last evaluation must still detect the
       // change.
       const description = createIssueDescription();
       const issue = createIssue('config_error', {
@@ -214,15 +211,11 @@ describe('IssueManager', () => {
 
       manager.trigger('config_error', { error: new Error('cfg') });
 
-      expect(api.getConditionStateManager().setState).toBeCalledWith({
-        issues: new Map([['config_error', description]]),
-      });
       expect(api.getCardElementManager().update).toBeCalled();
     });
 
     it('should never auto-popup on trigger -- non-full-card issues surface via the status-bar icon; user clicks to open', () => {
       const api = createCardAPI();
-      vi.mocked(api.getConditionStateManager().getState).mockReturnValue({});
 
       const manager = new IssueManager(api);
 
@@ -261,7 +254,6 @@ describe('IssueManager', () => {
 
     it('should force retry even when needsRetry is false', () => {
       const api = createCardAPI();
-      vi.mocked(api.getConditionStateManager().getState).mockReturnValue({});
       const manager = new IssueManager(api);
       const issue = createIssue('media_load', {
         retry: vi.fn().mockReturnValue(false),
@@ -275,10 +267,8 @@ describe('IssueManager', () => {
   });
 
   describe('evaluate', () => {
-    it('should update condition state and card when presence differs from state', () => {
+    it('should update the card when presence differs from the last evaluation', () => {
       const api = createCardAPI();
-      vi.mocked(api.getConditionStateManager().getState).mockReturnValue({});
-      vi.mocked(api.getConditionStateManager().setState).mockReturnValue(true);
 
       const manager = new IssueManager(api);
       const description = createIssueDescription();
@@ -290,15 +280,28 @@ describe('IssueManager', () => {
 
       manager.evaluate();
 
-      expect(api.getConditionStateManager().setState).toBeCalledWith({
-        issues: new Map([['config_error', description]]),
-      });
       expect(api.getCardElementManager().update).toBeCalled();
     });
 
-    it('should sync presence to condition state without update when unchanged', () => {
+    it('should not write issue presence back into the condition state', () => {
+      // Locks in the fix: routing issue presence through ConditionState (which
+      // embeds churning callback closures) is what caused the re-render loop.
+      const api = createCardAPI();
+
+      const manager = new IssueManager(api);
+      const issue = createIssue('config_error', {
+        hasIssue: vi.fn().mockReturnValue(true),
+        getIssue: vi.fn().mockReturnValue(createIssueDescription()),
+      });
+      manager.addIssue(issue);
+
+      manager.evaluate();
+
+      expect(api.getConditionStateManager().setState).not.toBeCalled();
+    });
+
+    it('should not update the card when there are no issues', () => {
       const api = createCardAPI();
-      vi.mocked(api.getConditionStateManager().getState).mockReturnValue({});
 
       const manager = new IssueManager(api);
       const issue = createIssue('config_error');
@@ -306,24 +309,16 @@ describe('IssueManager', () => {
 
       manager.evaluate();
 
-      expect(api.getConditionStateManager().setState).toBeCalledWith({
-        issues: new Map(),
-      });
       expect(api.getCardElementManager().update).not.toBeCalled();
     });
 
     it('should call update when an active issue swaps sub-states without changing the key set', () => {
       // Simulates ConnectionIssue going from 'lost' to 'starting': the presence
       // key set ({connection}) is identical, but the description value differs.
-      // Because IssuePresence is a Map<key, description>, the condition state
-      // diff sees the value-level change and fires listeners -- the
-      // IssueManager's own listener calls update().
+      // Because IssuePresence is a Map<key, description>, the presence diff sees
+      // the value-level change and requests a re-render.
       const api = createCardAPI();
 
-      // Real ConditionStateManager so its isEqual-based diff actually runs.
-      const stateManager = new ConditionStateManager();
-      vi.mocked(api.getConditionStateManager).mockReturnValue(stateManager);
-
       const manager = new IssueManager(api);
       const getIssue = vi
         .fn()
@@ -350,8 +345,6 @@ describe('IssueManager', () => {
 
     it('should not call update when content is identical across evaluations', () => {
       const api = createCardAPI();
-      const stateManager = new ConditionStateManager();
-      vi.mocked(api.getConditionStateManager).mockReturnValue(stateManager);
 
       const manager = new IssueManager(api);
       const issue = createIssue('connection', {
@@ -369,6 +362,34 @@ describe('IssueManager', () => {
       expect(api.getCardElementManager().update).not.toBeCalled();
     });
 
+    it('should not request repeated updates as retry callback closures churn', () => {
+      // Regression: getIssuePresence() rebuilds notifications fresh each call,
+      // so a retry control embeds a new callback closure every evaluation. The
+      // presence diff must ignore that function-identity churn, otherwise every
+      // evaluation looks changed and the card re-renders endlessly.
+      const api = createCardAPI();
+
+      const manager = new IssueManager(api);
+      const issue = createIssue('media_load', {
+        hasIssue: vi.fn().mockReturnValue(true),
+        getIssue: vi.fn().mockImplementation(() =>
+          createIssueDescription({
+            notification: {
+              controls: [createRetryControl('media_load')],
+            },
+          }),
+        ),
+      });
+      manager.addIssue(issue);
+
+      manager.evaluate();
+      vi.mocked(api.getCardElementManager().update).mockClear();
+
+      manager.evaluate();
+
+      expect(api.getCardElementManager().update).not.toBeCalled();
+    });
+
     it('should trigger evaluate from listener on condition state manager', () => {
       const api = createCardAPI();
       const stateManager = new ConditionStateManager();
@@ -385,32 +406,11 @@ describe('IssueManager', () => {
 
       expect(issue.detectDynamic).toBeCalled();
     });
-
-    it('should not re-enter evaluate when setState triggers listener', () => {
-      const api = createCardAPI();
-      const stateManager = new ConditionStateManager();
-      vi.mocked(api.getConditionStateManager).mockReturnValue(stateManager);
-
-      const manager = new IssueManager(api);
-      const issue = createIssue('config_error', {
-        hasIssue: vi.fn().mockReturnValue(true),
-        detectDynamic: vi.fn(),
-      });
-      manager.addIssue(issue);
-
-      // Calling evaluate() will call setState() on the real
-      // ConditionStateManager, which fires listeners synchronously. The
-      // reentrancy guard must prevent detectDynamic from running twice.
-      manager.evaluate();
-
-      expect(issue.detectDynamic).toBeCalledTimes(1);
-    });
   });
 
   describe('showNotification', () => {
     it('should call setNotification when a notification is available', () => {
       const api = createCardAPI();
-      vi.mocked(api.getConditionStateManager().getState).mockReturnValue({});
       const manager = new IssueManager(api);
 
       const notification = { body: { text: 'test notification' } };
@@ -436,7 +436,6 @@ describe('IssueManager', () => {
   describe('scheduled retries', () => {
     it('should not schedule a retry when no issue wants retry', () => {
       const api = createCardAPI();
-      vi.mocked(api.getConditionStateManager().getState).mockReturnValue({});
       vi.mocked(api.getConfigManager().getConfig).mockReturnValue(createConfig());
 
       const manager = new IssueManager(api);
@@ -451,7 +450,6 @@ describe('IssueManager', () => {
 
     it('should not schedule a retry when config is null', () => {
       const api = createCardAPI();
-      vi.mocked(api.getConditionStateManager().getState).mockReturnValue({});
       vi.mocked(api.getConfigManager().getConfig).mockReturnValue(null);
 
       const manager = new IssueManager(api);
@@ -735,7 +733,6 @@ describe('IssueManager', () => {
   describe('reset', () => {
     it('should reset a specific issue and re-evaluate', () => {
       const api = createCardAPI();
-      vi.mocked(api.getConditionStateManager().getState).mockReturnValue({});
 
       const manager = new IssueManager(api);
 
@@ -753,7 +750,6 @@ describe('IssueManager', () => {
 
     it('should skip reset when targeted key has no active issue', () => {
       const api = createCardAPI();
-      vi.mocked(api.getConditionStateManager().getState).mockReturnValue({});
 
       const manager = new IssueManager(api);
 
@@ -785,7 +781,6 @@ describe('IssueManager', () => {
 
     it('should gate evaluate while suspended', () => {
       const api = createCardAPI();
-      vi.mocked(api.getConditionStateManager().getState).mockReturnValue({});
 
       const manager = new IssueManager(api);
       const issue = createIssue('config_error', {
@@ -802,7 +797,6 @@ describe('IssueManager', () => {
 
     it('should preserve issue state across suspend', () => {
       const api = createCardAPI();
-      vi.mocked(api.getConditionStateManager().getState).mockReturnValue({});
 
       const manager = new IssueManager(api);
       const issue = createIssue('config_error', {
@@ -820,12 +814,11 @@ describe('IssueManager', () => {
 
     it('should resume evaluation on resume', () => {
       const api = createCardAPI();
-      vi.mocked(api.getConditionStateManager().getState).mockReturnValue({});
-      vi.mocked(api.getConditionStateManager().setState).mockReturnValue(true);
 
       const manager = new IssueManager(api);
       const issue = createIssue('config_error', {
         hasIssue: vi.fn().mockReturnValue(true),
+        getIssue: vi.fn().mockReturnValue(createIssueDescription()),
         detectDynamic: vi.fn(),
       });
       manager.addIssue(issue);
@@ -839,7 +832,6 @@ describe('IssueManager', () => {
 
     it('should invoke Issue.suspend on timer-backed issues when suspended', () => {
       const api = createCardAPI();
-      vi.mocked(api.getConditionStateManager().getState).mockReturnValue({});
 
       const manager = new IssueManager(api);
       const issue = createIssue('media_load', { suspend: vi.fn() });
@@ -852,7 +844,6 @@ describe('IssueManager', () => {
 
     it('should tolerate issues without a suspend hook', () => {
       const api = createCardAPI();
-      vi.mocked(api.getConditionStateManager().getState).mockReturnValue({});
 
       const manager = new IssueManager(api);
       // Plain Issue implementation -- no optional methods installed.

tests/utils/basic.test.ts

@@ -18,6 +18,7 @@ import {
   generateFloatApproximatelyEqualsCustomizer,
   getChildrenFromElement,
   getDurationString,
+  ignoreFunctionIdentity,
   isHoverableDevice,
   isHTMLElement,
   isSuperset,
@@ -523,6 +524,26 @@ describe('generateFloatApproximatelyEqualsCustomizer', () => {
   });
 });
 
+describe('ignoreFunctionIdentity', () => {
+  it('should treat two functions as equal regardless of identity', () => {
+    expect(
+      ignoreFunctionIdentity(
+        () => 1,
+        () => 2,
+      ),
+    ).toBe(true);
+  });
+  it('should treat a function appearing as a real change', () => {
+    expect(ignoreFunctionIdentity(() => 1, undefined)).toBe(false);
+  });
+  it('should treat a function disappearing as a real change', () => {
+    expect(ignoreFunctionIdentity(undefined, () => 1)).toBe(false);
+  });
+  it('should defer to default equality for non-functions', () => {
+    expect(ignoreFunctionIdentity(1, 2)).toBeUndefined();
+  });
+});
+
 describe('convertHTTPAdressToWebsocket', () => {
   it('should convert http to ws', () => {
     expect(convertHTTPAdressToWebsocket('http://example.com')).toBe('ws://example.com');