From 06345ca01d94e911c48cef057bac6d9f49232f93 Mon Sep 17 00:00:00 2001 From: Dermot Duffy Date: Tue, 24 Sep 2024 17:56:43 -0700 Subject: [PATCH] fix: Don't run the query string actions until after initialization (#1579) --- src/card-controller/card-element-manager.ts | 20 ++- src/card-controller/query-string-manager.ts | 34 ++--- src/card-controller/view/view-manager.ts | 17 +-- src/card.ts | 7 +- .../initialization-manager.test.ts | 4 +- .../query-string-manager.test.ts | 122 ++++++------------ .../card-controller/view/view-manager.test.ts | 6 +- 7 files changed, 84 insertions(+), 126 deletions(-) diff --git a/src/card-controller/card-element-manager.ts b/src/card-controller/card-element-manager.ts index b08bff51..fb3fa3fb 100644 --- a/src/card-controller/card-element-manager.ts +++ b/src/card-controller/card-element-manager.ts @@ -2,9 +2,9 @@ import { LitElement, ReactiveControllerHost } from 'lit'; import { ActionEventTarget } from '../action-handler-directive'; import { setOrRemoveAttribute } from '../utils/basic'; import { isCardInPanel } from '../utils/ha'; +import { ActionExecutionRequestEventTarget } from './actions/utils/execution-request'; import { InitializationAspect } from './initialization-manager'; import { CardElementAPI } from './types'; -import { ActionExecutionRequestEventTarget } from './actions/utils/execution-request'; export type ScrollCallback = () => void; export type MenuToggleCallback = () => void; @@ -120,16 +120,19 @@ export class CardElementManager { // See: https://github.com/home-assistant/frontend/blob/273992c8e9c3062c6e49481b6d7d688a07067232/src/common/navigate.ts#L43 window.addEventListener( 'location-changed', - this._api.getQueryStringManager().executeAll, + this._api.getQueryStringManager().requestExecution, ); // Listen for history state changes (i.e. user using the browser // back/forward controls). - window.addEventListener('popstate', this._api.getQueryStringManager().executeAll); + window.addEventListener( + 'popstate', + this._api.getQueryStringManager().requestExecution, + ); - // Manually call the location change handler as the card will be + // Manually request query string execute as the card will be // disconnected/reconnected when dashboard 'tab' changes happen within HA. - this._api.getQueryStringManager().executeAll(); + this._api.getQueryStringManager().requestExecution(); // Make sure reconnections call the initialization code. this._element.requestUpdate(); @@ -180,8 +183,11 @@ export class CardElementManager { window.removeEventListener( 'location-changed', - this._api.getQueryStringManager().executeAll, + this._api.getQueryStringManager().requestExecution, + ); + window.removeEventListener( + 'popstate', + this._api.getQueryStringManager().requestExecution, ); - window.removeEventListener('popstate', this._api.getQueryStringManager().executeAll); } } diff --git a/src/card-controller/query-string-manager.ts b/src/card-controller/query-string-manager.ts index ee5c9569..ce105c8a 100644 --- a/src/card-controller/query-string-manager.ts +++ b/src/card-controller/query-string-manager.ts @@ -14,27 +14,27 @@ interface QueryStringViewIntent { export class QueryStringManager { protected _api: CardQueryStringAPI; + protected _shouldRun = true; constructor(api: CardQueryStringAPI) { this._api = api; } - public hasViewRelatedActions(): boolean { - return !!this._calculateIntent().view; + public hasViewRelatedActionsToRun(): boolean { + return !!this._calculateIntent().view && this._shouldRun; } - public executeNonViewRelated = async (): Promise => { - await this._executeNonViewRelated(this._calculateIntent()); + public requestExecution = (): void => { + this._shouldRun = true; + this._api.getCardElementManager().update(); }; - public executeViewRelated = async (): Promise => { - await this._executeViewRelated(this._calculateIntent()); - }; - - public executeAll = async (): Promise => { - const intent = this._calculateIntent(); - await this._executeViewRelated(intent); - await this._executeNonViewRelated(intent); + public executeIfNecessary = async (): Promise => { + if (this._shouldRun) { + this._shouldRun = false; + await this._executeViewRelated(this._calculateIntent()); + await this._executeNonViewRelated(this._calculateIntent()); + } }; protected async _executeViewRelated(intent: QueryStringViewIntent): Promise { @@ -63,15 +63,9 @@ export class QueryStringManager { } protected async _executeNonViewRelated(intent: QueryStringViewIntent): Promise { - if ( - // Only execute non-view actions when the card has rendered at least once. - !this._api.getCardElementManager().hasUpdated() || - !intent.other?.length - ) { - return; + if (intent.other) { + await this._api.getActionsManager().executeActions(intent.other); } - - await this._api.getActionsManager().executeActions(intent.other); } protected _calculateIntent(): QueryStringViewIntent { diff --git a/src/card-controller/view/view-manager.ts b/src/card-controller/view/view-manager.ts index 6fcff9a1..7b40de0e 100644 --- a/src/card-controller/view/view-manager.ts +++ b/src/card-controller/view/view-manager.ts @@ -134,18 +134,11 @@ export class ViewManager implements ViewManagerInterface { } public initialize = async (): Promise => { - // Set a view on initial load. However, if the query string contains a view - // related action, we don't set any view here and allow that content to be - // triggered by the firstUpdated() call that runs query string actions. To - // do otherwise may cause a race condition between the default view and the - // querystring view, see: - // https://github.com/dermotduffy/frigate-hass-card/issues/1200 - const hasViewRelatedActions = this._api - .getQueryStringManager() - .hasViewRelatedActions(); - if (hasViewRelatedActions) { - await this._api.getQueryStringManager().executeViewRelated(); - } else { + // If the query string contains a view related action, we don't set any view + // here and allow that action to be triggered by the next call of to execute + // query actions (called at least once per render cycle). + // Related: https://github.com/dermotduffy/frigate-hass-card/issues/1200 + if (!this._api.getQueryStringManager().hasViewRelatedActionsToRun()) { await this.setViewDefaultWithNewQuery({ failSafe: true }); } return true; diff --git a/src/card.ts b/src/card.ts index 0ac95d60..33a535af 100644 --- a/src/card.ts +++ b/src/card.ts @@ -302,9 +302,10 @@ class FrigateCard extends LitElement { `; } - protected firstUpdated(): void { - // Execute query string actions after first render is complete. - this._controller.getQueryStringManager().executeNonViewRelated(); + protected updated(): void { + if (this._controller.getInitializationManager().isInitializedMandatory()) { + this._controller.getQueryStringManager().executeIfNecessary(); + } } protected _renderInDialogIfNecessary(contents: TemplateResult): TemplateResult | void { diff --git a/tests/card-controller/initialization-manager.test.ts b/tests/card-controller/initialization-manager.test.ts index ba1eb29c..62f3ed51 100644 --- a/tests/card-controller/initialization-manager.test.ts +++ b/tests/card-controller/initialization-manager.test.ts @@ -71,7 +71,7 @@ describe('InitializationManager', () => { vi.mocked(api.getHASSManager().getHASS).mockReturnValue(createHASS()); vi.mocked(api.getConfigManager().getConfig).mockReturnValue(createConfig()); vi.mocked(api.getMessageManager().hasMessage).mockReturnValue(false); - vi.mocked(api.getQueryStringManager().hasViewRelatedActions).mockReturnValue( + vi.mocked(api.getQueryStringManager().hasViewRelatedActionsToRun).mockReturnValue( false, ); vi.mocked(loadLanguages).mockResolvedValue(true); @@ -120,7 +120,7 @@ describe('InitializationManager', () => { vi.mocked(api.getHASSManager().getHASS).mockReturnValue(createHASS()); vi.mocked(api.getConfigManager().getConfig).mockReturnValue(createConfig()); vi.mocked(api.getMessageManager().hasMessage).mockReturnValue(true); - vi.mocked(api.getQueryStringManager().hasViewRelatedActions).mockReturnValue( + vi.mocked(api.getQueryStringManager().hasViewRelatedActionsToRun).mockReturnValue( false, ); vi.mocked(loadLanguages).mockResolvedValue(true); diff --git a/tests/card-controller/query-string-manager.test.ts b/tests/card-controller/query-string-manager.test.ts index 5ebf67dd..5b7dcfb0 100644 --- a/tests/card-controller/query-string-manager.test.ts +++ b/tests/card-controller/query-string-manager.test.ts @@ -22,9 +22,9 @@ describe('QueryStringManager', () => { vi.mocked(api.getMessageManager().hasMessage).mockReturnValue(true); const manager = new QueryStringManager(api); - await manager.executeAll(); + expect(manager.hasViewRelatedActionsToRun()).toBeFalsy(); + await manager.executeIfNecessary(); - expect(manager.hasViewRelatedActions()).toBeFalsy(); expect(api.getActionsManager().executeActions).not.toBeCalled(); expect(api.getViewManager().setViewByParameters).not.toBeCalled(); }); @@ -49,9 +49,10 @@ describe('QueryStringManager', () => { vi.mocked(api.getCardElementManager().hasUpdated).mockReturnValue(false); const manager = new QueryStringManager(api); - await manager.executeAll(); + expect(manager.hasViewRelatedActionsToRun()).toBeTruthy(); + await manager.executeIfNecessary(); + expect(manager.hasViewRelatedActionsToRun()).toBeFalsy(); - expect(manager.hasViewRelatedActions()).toBeTruthy(); expect(api.getViewManager().setViewByParametersWithNewQuery).toBeCalledWith({ params: { view: viewName, @@ -72,9 +73,9 @@ describe('QueryStringManager', () => { vi.mocked(api.getCardElementManager().hasUpdated).mockReturnValue(true); const manager = new QueryStringManager(api); - await manager.executeAll(); + expect(manager.hasViewRelatedActionsToRun()).toBeFalsy(); + await manager.executeIfNecessary(); - expect(manager.hasViewRelatedActions()).toBeFalsy(); expect(api.getActionsManager().executeActions).toBeCalledWith([ { action: 'fire-dom-event', @@ -93,11 +94,11 @@ describe('QueryStringManager', () => { const manager = new QueryStringManager(api); - await manager.executeAll(); + expect(manager.hasViewRelatedActionsToRun()).toBeTruthy(); + await manager.executeIfNecessary(); + expect(manager.hasViewRelatedActionsToRun()).toBeFalsy(); expect(api.getViewManager().setViewDefaultWithNewQuery).toBeCalled(); - - expect(manager.hasViewRelatedActions()).toBeTruthy(); expect(api.getActionsManager().executeActions).not.toBeCalled(); expect(api.getViewManager().setViewByParameters).not.toBeCalled(); }); @@ -108,15 +109,15 @@ describe('QueryStringManager', () => { vi.mocked(api.getCardElementManager().hasUpdated).mockReturnValue(true); const manager = new QueryStringManager(api); - await manager.executeAll(); + expect(manager.hasViewRelatedActionsToRun()).toBeTruthy(); + await manager.executeIfNecessary(); + expect(manager.hasViewRelatedActionsToRun()).toBeFalsy(); expect(api.getViewManager().setViewByParametersWithNewQuery).toBeCalledWith({ params: { camera: 'camera.office', }, }); - - expect(manager.hasViewRelatedActions()).toBeTruthy(); expect(api.getActionsManager().executeActions).not.toBeCalled(); expect(api.getViewManager().setViewDefault).not.toBeCalled(); }); @@ -127,14 +128,15 @@ describe('QueryStringManager', () => { vi.mocked(api.getCardElementManager().hasUpdated).mockReturnValue(true); const manager = new QueryStringManager(api); - await manager.executeAll(); + expect(manager.hasViewRelatedActionsToRun()).toBeTruthy(); + await manager.executeIfNecessary(); + expect(manager.hasViewRelatedActionsToRun()).toBeFalsy(); expect(api.getViewManager().setViewByParametersWithNewQuery).toBeCalledWith({ modifiers: [expect.any(SubstreamSelectViewModifier)], params: {}, }); - expect(manager.hasViewRelatedActions()).toBeTruthy(); expect(api.getActionsManager().executeActions).not.toBeCalled(); expect(api.getViewManager().setViewDefault).not.toBeCalled(); }); @@ -148,9 +150,9 @@ describe('QueryStringManager', () => { vi.mocked(api.getCardElementManager().hasUpdated).mockReturnValue(true); const manager = new QueryStringManager(api); - await manager.executeAll(); + expect(manager.hasViewRelatedActionsToRun()).toBeFalsy(); + await manager.executeIfNecessary(); - expect(manager.hasViewRelatedActions()).toBeFalsy(); expect(api.getActionsManager().executeActions).not.toBeCalled(); expect(api.getViewManager().setViewDefault).not.toBeCalled(); expect(api.getViewManager().setViewByParameters).not.toBeCalled(); @@ -166,9 +168,9 @@ describe('QueryStringManager', () => { vi.mocked(api.getCardElementManager().hasUpdated).mockReturnValue(true); const manager = new QueryStringManager(api); - await manager.executeAll(); + expect(manager.hasViewRelatedActionsToRun()).toBeFalsy(); + await manager.executeIfNecessary(); - expect(manager.hasViewRelatedActions()).toBeFalsy(); expect(api.getActionsManager().executeActions).not.toBeCalled(); expect(api.getViewManager().setViewDefault).not.toBeCalled(); expect(api.getViewManager().setViewByParameters).not.toBeCalled(); @@ -193,9 +195,10 @@ describe('QueryStringManager', () => { vi.mocked(api.getCardElementManager().hasUpdated).mockReturnValue(true); const manager = new QueryStringManager(api); - await manager.executeAll(); + expect(manager.hasViewRelatedActionsToRun()).toBeTruthy(); + await manager.executeIfNecessary(); + expect(manager.hasViewRelatedActionsToRun()).toBeFalsy(); - expect(manager.hasViewRelatedActions()).toBeTruthy(); expect(api.getViewManager().setViewByParametersWithNewQuery).toBeCalledWith({ params: { view: viewName, @@ -204,26 +207,6 @@ describe('QueryStringManager', () => { }); }); - describe('should not execute non-view actions without an initial update', () => { - it.each([ - ['camera_ui' as const], - ['download' as const], - ['expand' as const], - ['menu_toggle' as const], - ])('%s', async (action: string) => { - setQueryString(`?frigate-card-action.id.${action}=value`); - const api = createCardAPI(); - vi.mocked(api.getCardElementManager().hasUpdated).mockReturnValue(false); - const manager = new QueryStringManager(api); - - await manager.executeAll(); - - expect(api.getActionsManager().executeActions).not.toBeCalled(); - expect(api.getViewManager().setViewDefault).not.toBeCalled(); - expect(api.getViewManager().setViewByParameters).not.toBeCalled(); - }); - }); - describe('should handle conflicting but valid actions', () => { it('view and default with camera and substream specified', async () => { setQueryString( @@ -236,7 +219,7 @@ describe('QueryStringManager', () => { vi.mocked(api.getCardElementManager().hasUpdated).mockReturnValue(true); const manager = new QueryStringManager(api); - await manager.executeAll(); + await manager.executeIfNecessary(); expect(api.getViewManager().setViewDefaultWithNewQuery).toBeCalledWith({ params: { @@ -256,7 +239,7 @@ describe('QueryStringManager', () => { vi.mocked(api.getCardElementManager().hasUpdated).mockReturnValue(true); const manager = new QueryStringManager(api); - await manager.executeAll(); + await manager.executeIfNecessary(); expect(api.getViewManager().setViewByParametersWithNewQuery).toBeCalledWith({ params: { @@ -266,47 +249,26 @@ describe('QueryStringManager', () => { }); }); - describe('should not execute view related actions', () => { - it.each([ - ['clip' as const], - ['clips' as const], - ['default' as const], - ['diagnostics' as const], - ['image' as const], - ['live' as const], - ['recording' as const], - ['recordings' as const], - ['snapshot' as const], - ['snapshots' as const], - ['timeline' as const], - ])('%s', async (viewName: string) => { - setQueryString(`?frigate-card-action.id.${viewName}=`); - const api = createCardAPI(); - vi.mocked(api.getCardElementManager().hasUpdated).mockReturnValue(true); - const manager = new QueryStringManager(api); + it('should only execute when needed', async () => { + setQueryString('?frigate-card-action.id.live_substream_select=camera.office_hd'); + const api = createCardAPI(); + vi.mocked(api.getCardElementManager().hasUpdated).mockReturnValue(true); + const manager = new QueryStringManager(api); - await manager.executeNonViewRelated(); + expect(manager.hasViewRelatedActionsToRun()).toBeTruthy(); + await manager.executeIfNecessary(); + expect(manager.hasViewRelatedActionsToRun()).toBeFalsy(); + expect(api.getViewManager().setViewByParametersWithNewQuery).toBeCalledTimes(1); - expect(api.getViewManager().setViewByParameters).not.toBeCalled(); - expect(api.getViewManager().setViewDefault).not.toBeCalled(); - }); - }); + await manager.executeIfNecessary(); + expect(manager.hasViewRelatedActionsToRun()).toBeFalsy(); + expect(api.getViewManager().setViewByParametersWithNewQuery).toBeCalledTimes(1); - describe('should not execute non-view related actions', () => { - it.each([ - ['camera_ui' as const], - ['download' as const], - ['expand' as const], - ['menu_toggle' as const], - ])('%s', async (viewName: string) => { - setQueryString(`?frigate-card-action.id.${viewName}=`); - const api = createCardAPI(); - vi.mocked(api.getCardElementManager().hasUpdated).mockReturnValue(true); - const manager = new QueryStringManager(api); + manager.requestExecution(); - await manager.executeViewRelated(); - - expect(api.getActionsManager().executeActions).not.toBeCalled(); - }); + expect(manager.hasViewRelatedActionsToRun()).toBeTruthy(); + await manager.executeIfNecessary(); + expect(manager.hasViewRelatedActionsToRun()).toBeFalsy(); + expect(api.getViewManager().setViewByParametersWithNewQuery).toBeCalledTimes(2); }); }); diff --git a/tests/card-controller/view/view-manager.test.ts b/tests/card-controller/view/view-manager.test.ts index de65c1d4..809e0d6a 100644 --- a/tests/card-controller/view/view-manager.test.ts +++ b/tests/card-controller/view/view-manager.test.ts @@ -382,11 +382,13 @@ describe('hasMajorMediaChange', () => { const api = createCardAPI(); const factory = mock(); const manager = new ViewManager(api, factory); - vi.mocked(api.getQueryStringManager().hasViewRelatedActions).mockReturnValue(true); + vi.mocked(api.getQueryStringManager().hasViewRelatedActionsToRun).mockReturnValue( + true, + ); expect(await manager.initialize()).toBeTruthy(); - expect(api.getQueryStringManager().executeViewRelated).toBeCalled(); + expect(manager.hasView()).toBeFalsy(); }); }); });