From ddb0bd25c289d33a723c491d26497af7ab063158 Mon Sep 17 00:00:00 2001 From: Richard Herman <1429781+GeekyEggo@users.noreply.github.com> Date: Mon, 10 Jun 2024 23:59:59 +0100 Subject: [PATCH] fix: race condition tracking current ui (#42) * fix: race condition tracking current ui * refactor: update ui tracking to use a debounce counter * refactor: remove inline if --------- Co-authored-by: Richard Herman --- src/plugin/ui/__tests__/router.test.ts | 130 ++++++++++++++++++++++++- src/plugin/ui/router.ts | 35 ++++++- 2 files changed, 158 insertions(+), 7 deletions(-) diff --git a/src/plugin/ui/__tests__/router.test.ts b/src/plugin/ui/__tests__/router.test.ts index 4913a99f..e620486c 100644 --- a/src/plugin/ui/__tests__/router.test.ts +++ b/src/plugin/ui/__tests__/router.test.ts @@ -12,6 +12,18 @@ jest.mock("../../logging"); jest.mock("../../manifest"); describe("current UI", () => { + beforeEach(() => { + // Resets the debounce counter. + const context = { + action: "__reset__", + context: "__reset__", + device: "__reset__" + }; + + connection.emit("propertyInspectorDidAppear", { event: "propertyInspectorDidAppear", ...context }); + connection.emit("propertyInspectorDidDisappear", { event: "propertyInspectorDidDisappear", ...context }); + }); + /** * Asserts {@link getCurrentUI} is set when the connection emits `propertyInspectorDidAppear`. */ @@ -36,10 +48,17 @@ describe("current UI", () => { }); /** - * Asserts {@link getCurrentUI} is unset when the connection emits `propertyInspectorDidDisappear`. + * Asserts {@link getCurrentUI} is overwritten when the connection emits `propertyInspectorDidAppear`. */ - it("unset on propertyInspectorDidDisappear", () => { + it("overwrites on propertyInspectorDidAppear", () => { // Arrange. + connection.emit("propertyInspectorDidAppear", { + action: "com.elgato.test.one", + context: "__first__", + device: "dev123", + event: "propertyInspectorDidAppear" + }); + connection.emit("propertyInspectorDidAppear", { action: "com.elgato.test.one", context: "abc123", @@ -47,11 +66,103 @@ describe("current UI", () => { event: "propertyInspectorDidAppear" }); + // Act. + const current = getCurrentUI(); + + // Assert. + expect(current).toBeInstanceOf(PropertyInspector); + expect(current).not.toBeUndefined(); + expect(current?.deviceId).toBe("dev123"); + expect(current?.id).toBe("abc123"); + expect(current?.manifestId).toBe("com.elgato.test.one"); + }); + + /** + * Asserts {@link getCurrentUI} is unset when the connection emits `propertyInspectorDidDisappear` for the current UI. + */ + it("clears matching PI", () => { + // Arrange. + const context = { + action: "com.elgato.test.one", + context: "abc123", + device: "dev123" + }; + + connection.emit("propertyInspectorDidAppear", { + ...context, + event: "propertyInspectorDidAppear" + }); + + expect(getCurrentUI()).not.toBeUndefined(); + connection.emit("propertyInspectorDidDisappear", { + ...context, + event: "propertyInspectorDidDisappear" + }); + + // Act. + const current = getCurrentUI(); + + // Assert. + expect(current).toBeUndefined(); + }); + + /** + * Asserts {@link getCurrentUI} is not cleared when the connection emits `propertyInspectorDidDisappear` when the debounce count is greater than zero. + */ + it("does not clear matching PI with debounce", () => { + // Arrange. + const context = { + action: "com.elgato.test.one", + context: "abc123", + device: "dev123" + }; + + connection.emit("propertyInspectorDidAppear", { + ...context, + event: "propertyInspectorDidAppear" + }); + + // Show twice (mock event race) + connection.emit("propertyInspectorDidAppear", { + ...context, + event: "propertyInspectorDidAppear" + }); + expect(getCurrentUI()).not.toBeUndefined(); connection.emit("propertyInspectorDidDisappear", { + ...context, + event: "propertyInspectorDidDisappear" + }); + + // Act, assert. + const current = getCurrentUI(); + expect(current).not.toBeUndefined(); + + // Act, assert. + connection.emit("propertyInspectorDidDisappear", { + ...context, + event: "propertyInspectorDidDisappear" + }); + expect(getCurrentUI()).toBeUndefined(); + }); + + /** + * Asserts {@link getCurrentUI} is not cleared when the connection emits `propertyInspectorDidDisappear` for a UI that is not the current. + */ + it("does not clear non-matching PI", () => { + // Arrange. + connection.emit("propertyInspectorDidAppear", { action: "com.elgato.test.one", context: "abc123", device: "dev123", + event: "propertyInspectorDidAppear" + }); + + expect(getCurrentUI()).not.toBeUndefined(); + connection.emit("propertyInspectorDidDisappear", { + action: "com.elgato.test.one", + context: "__other__", + device: "dev123", event: "propertyInspectorDidDisappear" }); @@ -59,7 +170,7 @@ describe("current UI", () => { const current = getCurrentUI(); // Assert. - expect(current).toBeUndefined(); + expect(current).not.toBeUndefined(); }); /** @@ -232,10 +343,19 @@ describe("router", () => { */ test("without ui", async () => { // Arrange. - connection.emit("propertyInspectorDidDisappear", { + const ev = { action: "com.elgato.test.one", context: "proxy-outbound-message-without-ui", - device: "dev123", + device: "dev123" + }; + + connection.emit("propertyInspectorDidAppear", { + ...ev, + event: "propertyInspectorDidAppear" + }); + + connection.emit("propertyInspectorDidDisappear", { + ...ev, event: "propertyInspectorDidDisappear" }); diff --git a/src/plugin/ui/router.ts b/src/plugin/ui/router.ts index eb3b94a1..256dc2e0 100644 --- a/src/plugin/ui/router.ts +++ b/src/plugin/ui/router.ts @@ -1,3 +1,4 @@ +import type { PropertyInspectorDidAppear, PropertyInspectorDidDisappear } from "../../api"; import type { JsonValue } from "../../common/json"; import { MessageGateway } from "../../common/messaging"; import { Action } from "../actions/action"; @@ -5,6 +6,7 @@ import { connection } from "../connection"; import { PropertyInspector } from "./property-inspector"; let current: PropertyInspector | undefined; +let debounceCount = 0; /** * Gets the current property inspector. @@ -35,8 +37,37 @@ const router = new MessageGateway( (source) => new Action(source) ); -connection.on("propertyInspectorDidAppear", (ev) => (current = new PropertyInspector(router, ev))); -connection.on("propertyInspectorDidDisappear", () => (current = undefined)); +/** + * Determines whether the specified event is related to the current tracked property inspector. + * @param ev The event. + * @returns `true` when the event is related to the current property inspector. + */ +function isCurrent(ev: PropertyInspectorDidAppear | PropertyInspectorDidDisappear): boolean { + return current?.id === ev.context && current.manifestId === ev.action && current.deviceId === ev.device; +} + +/* + * To overcome event races, the debounce counter keeps track of appear vs disappear events, ensuring we only + * clear the current ui when an equal number of matching disappear events occur. + */ +connection.on("propertyInspectorDidAppear", (ev) => { + if (isCurrent(ev)) { + debounceCount++; + } else { + debounceCount = 1; + current = new PropertyInspector(router, ev); + } +}); + +connection.on("propertyInspectorDidDisappear", (ev) => { + if (isCurrent(ev)) { + debounceCount--; + if (debounceCount <= 0) { + current = undefined; + } + } +}); + connection.on("sendToPlugin", (ev) => router.process(ev)); export { router };