From ae9038a5198e3cd33f5382395d8801504e1c6ef0 Mon Sep 17 00:00:00 2001 From: Shahab Deljoo Date: Fri, 3 Apr 2026 22:18:39 -0400 Subject: [PATCH 1/2] Revert "Refactor external action plugin (#832)" This reverts commit 9f3439aff5c9456ef2866e2fd772dad6da4ac7c1. --- plugins/external-action/core/src/index.ts | 80 ++++++++++------------- 1 file changed, 35 insertions(+), 45 deletions(-) diff --git a/plugins/external-action/core/src/index.ts b/plugins/external-action/core/src/index.ts index 129669418..d93088e09 100644 --- a/plugins/external-action/core/src/index.ts +++ b/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; -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); } }); }); From ed71a410da163d2dd3ed17bbce0e7dc8f4c4be15 Mon Sep 17 00:00:00 2001 From: Shahab Deljoo Date: Fri, 3 Apr 2026 22:18:53 -0400 Subject: [PATCH 2/2] test: add multi-handler ordering tests for ExternalActionPlugin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When multiple ExternalActionPlugin instances are registered (e.g. in a plugin architecture where a host and embedded component each register their own handler), the interaction between handlers matters. With the pre-refactor implementation (transition hook + setTimeout), if the first handler resolves synchronously, the second handler is never invoked because the player has already transitioned away from the external state. This is the correct behavior — a resolved external state should not trigger additional handler side effects. These tests document and verify this behavior. All 3 tests pass with the reverted implementation. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../__tests__/multi-handler-ordering.test.ts | 132 ++++++++++++++++++ 1 file changed, 132 insertions(+) create mode 100644 plugins/external-action/core/src/__tests__/multi-handler-ordering.test.ts diff --git a/plugins/external-action/core/src/__tests__/multi-handler-ordering.test.ts b/plugins/external-action/core/src/__tests__/multi-handler-ordering.test.ts new file mode 100644 index 000000000..030875aa5 --- /dev/null +++ b/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"); + }); +});