test: verify multi-handler ordering with reverted ExternalActionPlugin

plugins/external-action/core/src/__tests__/multi-handler-ordering.test.ts

@@ -0,0 +1,132 @@
+import { describe, expect, test, vi } from "vitest";
+import type { Flow } from "@player-ui/player";
+import { Player } from "@player-ui/player";
+import { ExternalActionPlugin } from "..";
+
+/**
+ * Tests for ExternalActionPlugin behavior when multiple plugin instances
+ * are registered on the same Player.
+ *
+ * In plugin architectures, a host application may register its own
+ * ExternalActionPlugin alongside one from an embedded component. The
+ * interaction between these handlers matters: if the first handler
+ * resolves synchronously, the second handler should not execute its
+ * side effects, because the external state has already been resolved.
+ */
+const externalFlow: Flow = {
+  id: "test-external-action-ordering",
+  data: {
+    transitionValue: "Next",
+  },
+  navigation: {
+    BEGIN: "FLOW_1",
+    FLOW_1: {
+      startState: "EXT_1",
+      EXT_1: {
+        state_type: "EXTERNAL",
+        ref: "test-action",
+        transitions: {
+          first: "END_FIRST",
+          second: "END_SECOND",
+        },
+      },
+      END_FIRST: {
+        state_type: "END",
+        outcome: "first-handled",
+      },
+      END_SECOND: {
+        state_type: "END",
+        outcome: "second-handled",
+      },
+    },
+  },
+};
+
+describe("ExternalActionPlugin with multiple instances", () => {
+  test("when first handler resolves synchronously, second handler should not be invoked", async () => {
+    const firstHandler = vi.fn().mockReturnValue("first");
+    const secondHandler = vi.fn().mockReturnValue("second");
+
+    const player = new Player({
+      plugins: [
+        new ExternalActionPlugin(firstHandler),
+        new ExternalActionPlugin(secondHandler),
+      ],
+    });
+
+    const result = await player.start(externalFlow);
+
+    // The first handler should win
+    expect(result.endState.outcome).toBe("first-handled");
+    expect(firstHandler).toHaveBeenCalledTimes(1);
+
+    // The second handler should NOT be called — the first handler already
+    // resolved the external state, so invoking the second handler would
+    // cause unexpected side effects in the second plugin.
+    expect(secondHandler).not.toHaveBeenCalled();
+  });
+
+  test("when first handler resolves async, second handler starts but the loser does not transition", async () => {
+    const callOrder: string[] = [];
+
+    const firstHandler = vi.fn().mockImplementation(async () => {
+      callOrder.push("first-start");
+      await new Promise((r) => setTimeout(r, 10));
+      callOrder.push("first-resolve");
+      return "first";
+    });
+
+    const secondHandler = vi.fn().mockImplementation(async () => {
+      callOrder.push("second-start");
+      await new Promise((r) => setTimeout(r, 50));
+      callOrder.push("second-resolve");
+      return "second";
+    });
+
+    const player = new Player({
+      plugins: [
+        new ExternalActionPlugin(firstHandler),
+        new ExternalActionPlugin(secondHandler),
+      ],
+    });
+
+    const result = await player.start(externalFlow);
+
+    // The faster handler should win the transition
+    expect(result.endState.outcome).toBe("first-handled");
+
+    // Both handlers started (since both are async, they race)
+    expect(callOrder).toContain("first-start");
+    expect(callOrder).toContain("second-start");
+
+    // First should have resolved
+    expect(callOrder).toContain("first-resolve");
+
+    // If second also resolved, first must have resolved before it
+    if (callOrder.includes("second-resolve")) {
+      expect(callOrder.indexOf("first-resolve")).toBeLessThan(
+        callOrder.indexOf("second-resolve"),
+      );
+    }
+  });
+
+  test("when first handler returns undefined (delegates), second handler should resolve the state", async () => {
+    const firstHandler = vi.fn().mockReturnValue(undefined);
+    const secondHandler = vi.fn().mockReturnValue("second");
+
+    const player = new Player({
+      plugins: [
+        new ExternalActionPlugin(firstHandler),
+        new ExternalActionPlugin(secondHandler),
+      ],
+    });
+
+    const result = await player.start(externalFlow);
+
+    // First handler delegated by returning undefined
+    expect(firstHandler).toHaveBeenCalledTimes(1);
+    // Second handler should pick it up
+    expect(secondHandler).toHaveBeenCalledTimes(1);
+    expect(result.endState.outcome).toBe("second-handled");
+  });
+});

plugins/external-action/core/src/index.ts

@@ -3,7 +3,6 @@ import type {
   PlayerPlugin,
   InProgressState,
   PlayerFlowState,
-  NavigationFlowState,
   NavigationFlowExternalState,
 } from "@player-ui/player";
 
@@ -12,16 +11,6 @@ export type ExternalStateHandler = (
   options: InProgressState["controllers"],
 ) => string | undefined | Promise<string | undefined>;
 
-function isExternal(
-  state: NavigationFlowState,
-): state is NavigationFlowExternalState {
-  return state.state_type === "EXTERNAL";
-}
-
-function isInProgress(state: PlayerFlowState): state is InProgressState {
-  return state.status === "in-progress";
-}
-
 /**
  * A plugin to handle external actions states
  */
@@ -33,46 +22,47 @@ export class ExternalActionPlugin implements PlayerPlugin {
     this.handler = handler;
   }
 
-  apply(player: Player): void {
+  apply(player: Player) {
     player.hooks.flowController.tap(this.name, (flowController) => {
       flowController.hooks.flow.tap(this.name, (flow) => {
-        flow.hooks.afterTransition.tap(this.name, async (flowInstance) => {
-          const state = flowInstance.currentState;
-          const currentState = player.getState();
+        flow.hooks.transition.tap(this.name, (fromState, toState) => {
+          const { value: state } = toState;
+          if (state.state_type === "EXTERNAL") {
+            setTimeout(async () => {
+              /** Helper for ensuring state is still current relative to external state this is handling */
+              const shouldTransition = (
+                currentState: PlayerFlowState,
+              ): currentState is InProgressState =>
+                currentState.status === "in-progress" &&
+                currentState.controllers.flow.current?.currentState?.value ===
+                  state;
 
-          if (
-            state &&
-            state.value &&
-            isExternal(state.value) &&
-            isInProgress(currentState)
-          ) {
-            try {
-              const transitionValue = await this.handler(
-                state.value,
-                currentState.controllers,
-              );
-
-              if (transitionValue !== undefined) {
-                const latestState = player.getState();
-
-                // Ensure the Player is still in the same state after waiting for transitionValue
-                if (
-                  isInProgress(latestState) &&
-                  latestState.controllers.flow.current?.currentState?.name ===
-                    state.name
-                ) {
-                  latestState.controllers.flow.transition(transitionValue);
-                } else {
-                  player.logger.warn(
-                    `External state resolved with [${transitionValue}], but Player already navigated away from [${state.name}]`,
+              const currentState = player.getState();
+              if (shouldTransition(currentState)) {
+                try {
+                  const transitionValue = await this.handler(
+                    state,
+                    currentState.controllers,
                   );
+
+                  if (transitionValue !== undefined) {
+                    // Ensure the Player is still in the same state after waiting for transitionValue
+                    const latestState = player.getState();
+                    if (shouldTransition(latestState)) {
+                      latestState.controllers.flow.transition(transitionValue);
+                    } else {
+                      player.logger.warn(
+                        `External state resolved with [${transitionValue}], but Player already navigated away from [${toState.name}]`,
+                      );
+                    }
+                  }
+                } catch (error) {
+                  if (error instanceof Error) {
+                    currentState.fail(error);
+                  }
                 }
               }
-            } catch (error) {
-              if (error instanceof Error) {
-                currentState.fail(error);
-              }
-            }
+            }, 0);
           }
         });
       });