From 3fe5e4004a7a854f87d1e11db426dab8d358388f Mon Sep 17 00:00:00 2001 From: Dermot Duffy Date: Sat, 27 Jun 2026 16:37:48 -0700 Subject: [PATCH] fix: prevent infinite re-render loop from issue retry callbacks (#2550) --- src/card-controller/issues/issue-manager.ts | 62 +++++----- src/card-controller/issues/types.ts | 7 +- src/condition-trigger/conditions/types.ts | 12 +- src/utils/basic.ts | 10 ++ .../actions/actions-manager.test.ts | 9 +- .../issues/issue-manager.test.ts | 115 ++++++++---------- tests/utils/basic.test.ts | 21 ++++ 7 files changed, 137 insertions(+), 99 deletions(-) diff --git a/src/card-controller/issues/issue-manager.ts b/src/card-controller/issues/issue-manager.ts index 8bad3826..b5208c6f 100644 --- a/src/card-controller/issues/issue-manager.ts +++ b/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, so its - // deep equality check naturally catches both presence-set churn (issues + // IssuePresence is a Map, 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- diff --git a/src/card-controller/issues/types.ts b/src/card-controller/issues/types.ts index b87cb1e4..cfc04e49 100644 --- a/src/card-controller/issues/types.ts +++ b/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; export interface IssueReadOnlyState { hasFullCardIssue(): boolean; diff --git a/src/condition-trigger/conditions/types.ts b/src/condition-trigger/conditions/types.ts index 408e02d5..a20c1be4 100644 --- a/src/condition-trigger/conditions/types.ts +++ b/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. diff --git a/src/utils/basic.ts b/src/utils/basic.ts index 2c63ceb7..fa8a0b6e 100644 --- a/src/utils/basic.ts +++ b/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'); }; diff --git a/tests/card-controller/actions/actions-manager.test.ts b/tests/card-controller/actions/actions-manager.test.ts index b25801ff..aae1f985 100644 --- a/tests/card-controller/actions/actions-manager.test.ts +++ b/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(); }); diff --git a/tests/card-controller/issues/issue-manager.test.ts b/tests/card-controller/issues/issue-manager.test.ts index 33331086..aa2d0f20 100644 --- a/tests/card-controller/issues/issue-manager.test.ts +++ b/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, the condition state - // diff sees the value-level change and fires listeners -- the - // IssueManager's own listener calls update(). + // Because IssuePresence is a Map, 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. diff --git a/tests/utils/basic.test.ts b/tests/utils/basic.test.ts index 139e2d68..8fca3668 100644 --- a/tests/utils/basic.test.ts +++ b/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');