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 <[email protected]>
elgatosf · Jun 10, 2024 · ddb0bd2 · ddb0bd2
1 parent 45c54af
commit ddb0bd2
Show file tree

Hide file tree

Showing 2 changed files with 158 additions and 7 deletions.
diff --git 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,30 +48,129 @@ 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",
 			device: "dev123",
 			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"
 		});
 
 		// Act.
 		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
@@ -1,10 +1,12 @@
+import type { PropertyInspectorDidAppear, PropertyInspectorDidDisappear } from "../../api";
 import type { JsonValue } from "../../common/json";
 import { MessageGateway } from "../../common/messaging";
 import { Action } from "../actions/action";
 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<Action>(
 	(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 };