From 0b231b40dec721c99378897f3e2a63c78876d7b8 Mon Sep 17 00:00:00 2001 From: Chloe Han Date: Tue, 9 Dec 2025 12:35:07 -0500 Subject: [PATCH 1/9] error controller --- .../error/__tests__/controller.test.ts | 259 ++++++++++++++++++ .../src/controllers/error/controller.ts | 163 +++++++++++ core/player/src/controllers/error/index.ts | 2 + core/player/src/controllers/error/types.ts | 51 ++++ core/player/src/controllers/index.ts | 1 + core/player/src/player.ts | 20 ++ core/player/src/types.ts | 6 + 7 files changed, 502 insertions(+) create mode 100644 core/player/src/controllers/error/__tests__/controller.test.ts create mode 100644 core/player/src/controllers/error/controller.ts create mode 100644 core/player/src/controllers/error/index.ts create mode 100644 core/player/src/controllers/error/types.ts diff --git a/core/player/src/controllers/error/__tests__/controller.test.ts b/core/player/src/controllers/error/__tests__/controller.test.ts new file mode 100644 index 000000000..b0eae0872 --- /dev/null +++ b/core/player/src/controllers/error/__tests__/controller.test.ts @@ -0,0 +1,259 @@ +import { describe, it, beforeEach, expect, vitest } from "vitest"; +import { ErrorController } from "../controller"; +import { ErrorSeverity, ErrorTypes } from "../types"; +import type { DataController } from "../../data/controller"; +import type { Logger } from "../../../logger"; + +describe("ErrorController", () => { + let errorController: ErrorController; + let mockDataController: DataController; + let mockLogger: Logger; + + beforeEach(() => { + mockDataController = { + set: vitest.fn(), + get: vitest.fn(), + delete: vitest.fn(), + } as any; + + mockLogger = { + trace: vitest.fn(), + debug: vitest.fn(), + info: vitest.fn(), + warn: vitest.fn(), + error: vitest.fn(), + }; + + errorController = new ErrorController({ + logger: mockLogger, + dataController: mockDataController, + }); + }); + + describe("captureError", () => { + it("should capture error with metadata", () => { + const error = new Error("Test error"); + const metadata = { + errorType: ErrorTypes.EXPRESSION, + severity: ErrorSeverity.ERROR, + state: "VIEW_Test", + }; + + const playerError = errorController.captureError(error, metadata); + + expect(playerError.error).toBe(error); + expect(playerError.metadata.errorType).toBe(ErrorTypes.EXPRESSION); + expect(playerError.metadata.severity).toBe(ErrorSeverity.ERROR); + expect(playerError.metadata.state).toBe("VIEW_Test"); + expect(playerError.metadata.timestamp).toBeDefined(); + }); + + it("should add error to history", () => { + const error1 = new Error("Error 1"); + const error2 = new Error("Error 2"); + + errorController.captureError(error1); + errorController.captureError(error2); + + const history = errorController.getErrors(); + expect(history).toHaveLength(2); + expect(history[0]?.error).toBe(error1); + expect(history[1]?.error).toBe(error2); + }); + + it("should set as current error", () => { + const error = new Error("Test error"); + errorController.captureError(error); + + const currentError = errorController.getCurrentError(); + expect(currentError?.error).toBe(error); + }); + + it("should write to data model", () => { + const error = new Error("Test error"); + errorController.captureError(error, { + errorType: ErrorTypes.EXPRESSION, + }); + + expect(mockDataController.set).toHaveBeenCalledWith([ + [ + "errorState", + expect.objectContaining({ + message: "Test error", + name: "Error", + errorType: ErrorTypes.EXPRESSION, + }), + ], + ]); + }); + }); + + describe("formatErrorForData", () => { + it("should format error with default formatter", () => { + const error = new Error("Test error"); + const playerError = errorController.captureError(error, { + errorType: ErrorTypes.VIEW, + severity: ErrorSeverity.ERROR, + state: "VIEW_Test", + context: { viewId: "test-view" }, + }); + + const formatted = errorController.formatErrorForData(playerError); + + expect(formatted).toEqual({ + message: "Test error", + name: "Error", + errorType: ErrorTypes.VIEW, + severity: ErrorSeverity.ERROR, + state: "VIEW_Test", + timestamp: expect.any(Number), + assetId: undefined, + assetType: undefined, + bindingPath: undefined, + context: { viewId: "test-view" }, + }); + }); + + it("should include asset context", () => { + const error = new Error("Test error"); + const playerError = errorController.captureError(error, { + assetContext: { + asset: { id: "test-asset", type: "form" } as any, + bindingPath: "user.email", + }, + }); + + const formatted = errorController.formatErrorForData(playerError); + + expect(formatted.assetId).toBe("test-asset"); + expect(formatted.assetType).toBe("form"); + expect(formatted.bindingPath).toBe("user.email"); + }); + }); + + describe("getCurrentError", () => { + it("should return undefined when no errors", () => { + expect(errorController.getCurrentError()).toBeUndefined(); + }); + + it("should return current error", () => { + const error = new Error("Test error"); + errorController.captureError(error); + + expect(errorController.getCurrentError()?.error).toBe(error); + }); + }); + + describe("getErrors", () => { + it("should return empty array when no errors", () => { + expect(errorController.getErrors()).toEqual([]); + }); + + it("should return error history", () => { + const error1 = new Error("Error 1"); + const error2 = new Error("Error 2"); + + errorController.captureError(error1); + errorController.captureError(error2); + + const errors = errorController.getErrors(); + expect(errors).toHaveLength(2); + expect(errors[0]?.error).toBe(error1); + expect(errors[1]?.error).toBe(error2); + }); + }); + + describe("clearErrors", () => { + it("should clear all errors and history", () => { + errorController.captureError(new Error("Error 1")); + errorController.captureError(new Error("Error 2")); + + errorController.clearErrors(); + + expect(errorController.getCurrentError()).toBeUndefined(); + expect(errorController.getErrors()).toEqual([]); + }); + }); + + describe("clearCurrentError", () => { + it("should clear current error but preserve history", () => { + const error1 = new Error("Error 1"); + const error2 = new Error("Error 2"); + + errorController.captureError(error1); + errorController.captureError(error2); + + errorController.clearCurrentError(); + + expect(errorController.getCurrentError()).toBeUndefined(); + expect(errorController.getErrors()).toHaveLength(2); + }); + }); + + describe("onError hook", () => { + it("should allow plugins to observe errors", () => { + const onErrorSpy = vitest.fn(); + errorController.hooks.onError.tap("test", onErrorSpy); + + const error = new Error("Test error"); + errorController.captureError(error); + + expect(onErrorSpy).toHaveBeenCalledTimes(1); + expect(onErrorSpy).toHaveBeenCalledWith( + expect.objectContaining({ + error, + }), + ); + }); + + it("should allow plugins to bail by returning true, stopping execution and preventing data model update", () => { + const observer1 = vitest.fn(() => undefined); + const skipPlugin = vitest.fn(() => true); + const observer2 = vitest.fn(() => undefined); + + errorController.hooks.onError.tap("observer1", observer1); + errorController.hooks.onError.tap("skip-plugin", skipPlugin); + errorController.hooks.onError.tap("observer2", observer2); + + const error = new Error("Test error"); + const playerError = errorController.captureError(error); + + expect(observer1).toHaveBeenCalledTimes(1); + expect(skipPlugin).toHaveBeenCalledWith(playerError); + expect(observer2).not.toHaveBeenCalled(); // Execution stops after bail + // Data model should not be updated when skipped + expect(mockDataController.set).not.toHaveBeenCalled(); + }); + + it("should continue to next plugin when undefined is returned", () => { + const observer1 = vitest.fn(() => undefined); + const observer2 = vitest.fn(() => undefined); + errorController.hooks.onError.tap("observer1", observer1); + errorController.hooks.onError.tap("observer2", observer2); + + const error = new Error("Test error"); + errorController.captureError(error); + + expect(observer1).toHaveBeenCalledTimes(1); + expect(observer2).toHaveBeenCalledTimes(1); + // Data model should be updated when not skipped + expect(mockDataController.set).toHaveBeenCalled(); + }); + }); + + describe("custom error types", () => { + it("should allow custom plugin error types", () => { + const error = new Error("Custom plugin error"); + const playerError = errorController.captureError(error, { + errorType: "my-custom-plugin", + severity: ErrorSeverity.WARNING, + }); + + expect(playerError.metadata.errorType).toBe("my-custom-plugin"); + + const formatted = errorController.formatErrorForData(playerError); + expect(formatted.errorType).toBe("my-custom-plugin"); + }); + }); +}); + diff --git a/core/player/src/controllers/error/controller.ts b/core/player/src/controllers/error/controller.ts new file mode 100644 index 000000000..207be4488 --- /dev/null +++ b/core/player/src/controllers/error/controller.ts @@ -0,0 +1,163 @@ +import { SyncBailHook } from "tapable-ts"; +import type { Logger } from "../../logger"; +import type { DataController } from "../data/controller"; +import type { PlayerError, ErrorMetadata, ErrorSeverity } from "./types"; + +export interface ErrorControllerHooks { + /** + * Fired when any error is captured + * - Called in order for each tapped plugin + * - Return true to bail and prevent error state navigation + * - Return undefined/false to continue to next handler + * - Once true is returned, no further plugins are called + */ + onError: SyncBailHook<[PlayerError], boolean | undefined>; +} + +export interface ErrorControllerOptions { + /** Optional logger for error operations */ + logger?: Logger; + /** Required for setting data.errorState */ + dataController?: DataController; +} + +/** Default error format for data.errorState */ +interface FormattedError { + message: string; + name: string; + state?: string; + timestamp?: number; + errorType?: string; + severity?: ErrorSeverity; + assetId?: string; + assetType?: string; + bindingPath?: string; + context?: Record; +} + +/** The orchestrator for player error handling */ +export class ErrorController { + public hooks: ErrorControllerHooks = { + onError: new SyncBailHook<[PlayerError], boolean | undefined>(), + }; + + private readonly log?: Logger; + private readonly dataController?: DataController; + private errorHistory: PlayerError[] = []; + private currentError?: PlayerError; + + constructor(options: ErrorControllerOptions = {}) { + this.log = options.logger; + this.dataController = options.dataController; + } + + /** + * Capture error with metadata, add to history, fire hooks, update data model + */ + public captureError(error: Error, metadata: ErrorMetadata = {}): PlayerError { + const playerError: PlayerError = { + error, + metadata: { + ...metadata, + timestamp: metadata.timestamp ?? Date.now(), + }, + }; + + // Add to history + this.errorHistory.push(playerError); + + // Set as current error + this.currentError = playerError; + + this.log?.debug( + `[ErrorController] Captured error: ${error.message}`, + playerError.metadata, + ); + + // Notify listeners and check if navigation should be skipped + // Plugins can observe the error and optionally return true to bail + const shouldSkip = this.hooks.onError.call(playerError); + + if (shouldSkip) { + this.log?.debug( + "[ErrorController] Error state navigation skipped by plugin", + ); + return playerError; + } + + this.setErrorInDataModel(playerError); + + return playerError; + } + + /** + * Format error for content binding + */ + public formatErrorForData(playerError: PlayerError): FormattedError { + const { error, metadata } = playerError; + + return { + message: error.message, + name: error.name, + state: metadata.state, + timestamp: metadata.timestamp, + errorType: metadata.errorType, + severity: metadata.severity, + assetId: metadata.assetContext?.asset?.id, + assetType: metadata.assetContext?.asset?.type, + bindingPath: metadata.assetContext?.bindingPath, + context: metadata.context, + }; + } + + /** + * Get most recent error + */ + public getCurrentError(): PlayerError | undefined { + return this.currentError; + } + + /** + * Get error history (read-only) + */ + public getErrors(): ReadonlyArray { + return this.errorHistory; + } + + /** + * Clear all errors (history + current) + */ + public clearErrors(): void { + this.errorHistory = []; + this.currentError = undefined; + this.log?.debug("[ErrorController] All errors cleared"); + } + + /** + * Clear only current error, preserve history + */ + public clearCurrentError(): void { + this.currentError = undefined; + this.log?.debug("[ErrorController] Current error cleared"); + } + + /** + * Write error to data model errorState + */ + private setErrorInDataModel(playerError: PlayerError): void { + if (!this.dataController) { + this.log?.warn("[ErrorController] No DataController available"); + return; + } + + try { + const formattedError = this.formatErrorForData(playerError); + this.dataController.set([["errorState", formattedError]]); + this.log?.debug( + "[ErrorController] Error set in data model at 'data.errorState'", + ); + } catch (e) { + this.log?.error("[ErrorController] Failed to set error in data model", e); + } + } +} diff --git a/core/player/src/controllers/error/index.ts b/core/player/src/controllers/error/index.ts new file mode 100644 index 000000000..000fae134 --- /dev/null +++ b/core/player/src/controllers/error/index.ts @@ -0,0 +1,2 @@ +export * from "./controller"; +export * from "./types"; diff --git a/core/player/src/controllers/error/types.ts b/core/player/src/controllers/error/types.ts new file mode 100644 index 000000000..aaf635be9 --- /dev/null +++ b/core/player/src/controllers/error/types.ts @@ -0,0 +1,51 @@ +import type { Asset } from "@player-ui/types"; + +/** Severity levels */ +export enum ErrorSeverity { + FATAL = "fatal", // Cannot continue, flow must end + ERROR = "error", // Standard error, may allow recovery + WARNING = "warning", // Non-blocking, logged for telemetry +} + +/** Known error types for Player */ +export const ErrorTypes = { + EXPRESSION: "expression", + BINDING: "binding", + VIEW: "view", + ASSET: "asset", + NAVIGATION: "navigation", + VALIDATION: "validation", + DATA: "data", + SCHEMA: "schema", + NETWORK: "network", + PLUGIN: "plugin", +} as const; + +export interface AssetErrorContext { + asset?: Asset; + bindingPath?: string; +} + +export interface ErrorMetadata { + /** Flow state name when error occurred */ + state?: string; + /** Additional context (transition name, view ref, etc) */ + context?: Record; + /** timestamp */ + timestamp?: number; + /** Error category (use ErrorTypes constants or custom plugin types) */ + errorType?: string; + /** Impact level */ + severity?: ErrorSeverity; + /** Asset-specific details */ + assetContext?: AssetErrorContext; + /** Allow custom fields for domain-specific information */ + [key: string]: unknown; +} + +export interface PlayerError { + /** Native Error object */ + error: Error; + /** Error metadata */ + metadata: ErrorMetadata; +} diff --git a/core/player/src/controllers/index.ts b/core/player/src/controllers/index.ts index 9064b28e3..66d008329 100644 --- a/core/player/src/controllers/index.ts +++ b/core/player/src/controllers/index.ts @@ -3,3 +3,4 @@ export * from "./validation"; export * from "./view"; export * from "./data/controller"; export * from "./constants"; +export * from "./error"; diff --git a/core/player/src/player.ts b/core/player/src/player.ts index b6b6ef6d2..a97f3e018 100644 --- a/core/player/src/player.ts +++ b/core/player/src/player.ts @@ -19,6 +19,7 @@ import { DataController, ValidationController, FlowController, + ErrorController, } from "./controllers"; import { FlowExpPlugin } from "./plugins/flow-exp-plugin"; import { DefaultExpPlugin } from "./plugins/default-exp-plugin"; @@ -115,6 +116,7 @@ export class Player { schema: new SyncHook<[SchemaController]>(), validationController: new SyncHook<[ValidationController]>(), bindingParser: new SyncHook<[BindingParser]>(), + errorController: new SyncHook<[ErrorController]>(), state: new SyncHook<[PlayerFlowState]>(), onStart: new SyncHook<[Flow]>(), onEnd: new SyncHook<[]>(), @@ -217,6 +219,8 @@ export class Player { let expressionEvaluator: ExpressionEvaluator; // eslint-disable-next-line prefer-const let dataController: DataController; + // eslint-disable-next-line prefer-const + let errorController: ErrorController; const pathResolver = new BindingParser({ get: (binding) => { @@ -264,6 +268,21 @@ export class Player { (binding) => schema.getApparentType(binding)?.default, ); + // Initialize ErrorController with DataController reference + errorController = new ErrorController({ + logger: this.logger, + dataController, + }); + + this.hooks.errorController.call(errorController); + + flowController.hooks.flow.tap("error-controller-cleanup", (flowInstance) => { + flowInstance.hooks.transition.tap("error-controller-cleanup", () => { + errorController.clearCurrentError(); + dataController.delete("errorState"); + }); + }); + // eslint-disable-next-line prefer-const let viewController: ViewController; @@ -482,6 +501,7 @@ export class Player { expression: expressionEvaluator, binding: pathResolver, validation: validationController, + error: errorController, }, fail: flowResultDeferred.reject, flow: userFlow, diff --git a/core/player/src/types.ts b/core/player/src/types.ts index 44eb6e25b..9598fa73e 100644 --- a/core/player/src/types.ts +++ b/core/player/src/types.ts @@ -8,6 +8,7 @@ import type { DataController, ValidationController, FlowController, + ErrorController, } from "./controllers"; import type { ReadOnlyDataController } from "./controllers/data/utils"; import { SyncHook, SyncWaterfallHook } from "tapable-ts"; @@ -33,6 +34,8 @@ export interface PlayerHooks { validationController: SyncHook<[ValidationController], Record>; /** Manages parsing binding */ bindingParser: SyncHook<[BindingParser], Record>; + /** Manages error handling and captures errors from all subsystems */ + errorController: SyncHook<[ErrorController], Record>; /** A that's called for state changes in the flow execution */ state: SyncHook<[PlayerFlowState], Record>; /** A hook to access the current flow */ @@ -97,6 +100,9 @@ export interface ControllerState { /** the manager for the flow state machine */ flow: FlowController; + + /** The manager for error handling */ + error: ErrorController; } /** A flow is currently executing */ From 259038c238f7e9a5e7fa561c06c4f8f3e01b30da Mon Sep 17 00:00:00 2001 From: Chloe Han Date: Tue, 9 Dec 2025 14:10:39 -0500 Subject: [PATCH 2/9] fix test failure --- .../error/__tests__/controller.test.ts | 3 --- .../src/controllers/error/controller.ts | 4 ---- core/player/src/controllers/error/types.ts | 20 +++++++++---------- core/player/src/player.ts | 10 +--------- 4 files changed, 11 insertions(+), 26 deletions(-) diff --git a/core/player/src/controllers/error/__tests__/controller.test.ts b/core/player/src/controllers/error/__tests__/controller.test.ts index b0eae0872..57cbb7207 100644 --- a/core/player/src/controllers/error/__tests__/controller.test.ts +++ b/core/player/src/controllers/error/__tests__/controller.test.ts @@ -105,8 +105,6 @@ describe("ErrorController", () => { name: "Error", errorType: ErrorTypes.VIEW, severity: ErrorSeverity.ERROR, - state: "VIEW_Test", - timestamp: expect.any(Number), assetId: undefined, assetType: undefined, bindingPath: undefined, @@ -256,4 +254,3 @@ describe("ErrorController", () => { }); }); }); - diff --git a/core/player/src/controllers/error/controller.ts b/core/player/src/controllers/error/controller.ts index 207be4488..a4786ba42 100644 --- a/core/player/src/controllers/error/controller.ts +++ b/core/player/src/controllers/error/controller.ts @@ -25,8 +25,6 @@ export interface ErrorControllerOptions { interface FormattedError { message: string; name: string; - state?: string; - timestamp?: number; errorType?: string; severity?: ErrorSeverity; assetId?: string; @@ -99,8 +97,6 @@ export class ErrorController { return { message: error.message, name: error.name, - state: metadata.state, - timestamp: metadata.timestamp, errorType: metadata.errorType, severity: metadata.severity, assetId: metadata.assetContext?.asset?.id, diff --git a/core/player/src/controllers/error/types.ts b/core/player/src/controllers/error/types.ts index aaf635be9..61e4e75bb 100644 --- a/core/player/src/controllers/error/types.ts +++ b/core/player/src/controllers/error/types.ts @@ -9,16 +9,16 @@ export enum ErrorSeverity { /** Known error types for Player */ export const ErrorTypes = { - EXPRESSION: "expression", - BINDING: "binding", - VIEW: "view", - ASSET: "asset", - NAVIGATION: "navigation", - VALIDATION: "validation", - DATA: "data", - SCHEMA: "schema", - NETWORK: "network", - PLUGIN: "plugin", + EXPRESSION: "expression", + BINDING: "binding", + VIEW: "view", + ASSET: "asset", + NAVIGATION: "navigation", + VALIDATION: "validation", + DATA: "data", + SCHEMA: "schema", + NETWORK: "network", + PLUGIN: "plugin", } as const; export interface AssetErrorContext { diff --git a/core/player/src/player.ts b/core/player/src/player.ts index a97f3e018..33133167a 100644 --- a/core/player/src/player.ts +++ b/core/player/src/player.ts @@ -219,7 +219,6 @@ export class Player { let expressionEvaluator: ExpressionEvaluator; // eslint-disable-next-line prefer-const let dataController: DataController; - // eslint-disable-next-line prefer-const let errorController: ErrorController; const pathResolver = new BindingParser({ @@ -268,7 +267,7 @@ export class Player { (binding) => schema.getApparentType(binding)?.default, ); - // Initialize ErrorController with DataController reference + // eslint-disable-next-line prefer-const errorController = new ErrorController({ logger: this.logger, dataController, @@ -276,13 +275,6 @@ export class Player { this.hooks.errorController.call(errorController); - flowController.hooks.flow.tap("error-controller-cleanup", (flowInstance) => { - flowInstance.hooks.transition.tap("error-controller-cleanup", () => { - errorController.clearCurrentError(); - dataController.delete("errorState"); - }); - }); - // eslint-disable-next-line prefer-const let viewController: ViewController; From d3e25ff3a0a795da6a546475da1c3704bd41185d Mon Sep 17 00:00:00 2001 From: Chloe Han Date: Thu, 11 Dec 2025 15:06:16 -0500 Subject: [PATCH 3/9] add middleware and update controller clear functions --- .../error/__tests__/controller.test.ts | 56 +++++- .../error/__tests__/middleware.test.ts | 166 ++++++++++++++++++ .../src/controllers/error/controller.ts | 106 +++++++++-- core/player/src/controllers/error/index.ts | 1 + .../src/controllers/error/middleware.ts | 111 ++++++++++++ core/player/src/player.ts | 17 +- 6 files changed, 431 insertions(+), 26 deletions(-) create mode 100644 core/player/src/controllers/error/__tests__/middleware.test.ts create mode 100644 core/player/src/controllers/error/middleware.ts diff --git a/core/player/src/controllers/error/__tests__/controller.test.ts b/core/player/src/controllers/error/__tests__/controller.test.ts index 57cbb7207..28d75ab88 100644 --- a/core/player/src/controllers/error/__tests__/controller.test.ts +++ b/core/player/src/controllers/error/__tests__/controller.test.ts @@ -26,7 +26,7 @@ describe("ErrorController", () => { errorController = new ErrorController({ logger: mockLogger, - dataController: mockDataController, + model: mockDataController, }); }); @@ -171,6 +171,24 @@ describe("ErrorController", () => { expect(errorController.getCurrentError()).toBeUndefined(); expect(errorController.getErrors()).toEqual([]); }); + + it("should delete errorState from data model", () => { + errorController.captureError(new Error("Test error")); + + // Reset mock to track only clearErrors call + vitest.clearAllMocks(); + + errorController.clearErrors(); + + expect(mockDataController.delete).toHaveBeenCalledWith("errorState"); + }); + + it("should handle missing data controller gracefully", () => { + const controller = new ErrorController({ logger: mockLogger }); + controller.captureError(new Error("Test error")); + + expect(() => controller.clearErrors()).not.toThrow(); + }); }); describe("clearCurrentError", () => { @@ -186,6 +204,24 @@ describe("ErrorController", () => { expect(errorController.getCurrentError()).toBeUndefined(); expect(errorController.getErrors()).toHaveLength(2); }); + + it("should delete errorState from data model", () => { + errorController.captureError(new Error("Test error")); + + // Reset mock to track only clearCurrentError call + vitest.clearAllMocks(); + + errorController.clearCurrentError(); + + expect(mockDataController.delete).toHaveBeenCalledWith("errorState"); + }); + + it("should handle missing data controller gracefully", () => { + const controller = new ErrorController({ logger: mockLogger }); + controller.captureError(new Error("Test error")); + + expect(() => controller.clearCurrentError()).not.toThrow(); + }); }); describe("onError hook", () => { @@ -253,4 +289,22 @@ describe("ErrorController", () => { expect(formatted.errorType).toBe("my-custom-plugin"); }); }); + + describe("integration with middleware", () => { + it("should use middleware to protect errorState from external deletes", () => { + const middleware = errorController.getDataMiddleware(); + + // Middleware should block deletes by default + expect(middleware.name).toBe("error-state-middleware"); + + // Capture an error (sets errorState) + errorController.captureError(new Error("Test error")); + expect(mockDataController.set).toHaveBeenCalled(); + + // Clear error should delete via middleware + vitest.clearAllMocks(); + errorController.clearCurrentError(); + expect(mockDataController.delete).toHaveBeenCalledWith("errorState"); + }); + }); }); diff --git a/core/player/src/controllers/error/__tests__/middleware.test.ts b/core/player/src/controllers/error/__tests__/middleware.test.ts new file mode 100644 index 000000000..b65787d59 --- /dev/null +++ b/core/player/src/controllers/error/__tests__/middleware.test.ts @@ -0,0 +1,166 @@ +import { describe, it, beforeEach, expect, vitest } from "vitest"; +import { ErrorStateMiddleware } from "../middleware"; +import { BindingParser } from "../../../binding"; +import type { DataModelImpl } from "../../../data"; +import { LocalModel } from "../../../data"; +import type { Logger } from "../../../logger"; + +describe("ErrorStateMiddleware", () => { + let middleware: ErrorStateMiddleware; + let baseDataModel: DataModelImpl; + let mockLogger: Logger; + let parser: BindingParser; + + beforeEach(() => { + mockLogger = { + trace: vitest.fn(), + debug: vitest.fn(), + info: vitest.fn(), + warn: vitest.fn(), + error: vitest.fn(), + }; + + middleware = new ErrorStateMiddleware({ logger: mockLogger }); + baseDataModel = new LocalModel({ foo: "bar" }); + + parser = new BindingParser({ + get: () => undefined, + set: () => undefined, + evaluate: () => undefined, + }); + }); + + describe("set", () => { + it("should block writes to errorState by default", () => { + const binding = parser.parse("errorState"); + const updates = middleware.set( + [[binding, { message: "test" }]], + undefined, + baseDataModel, + ); + + // Should not write to base model + expect(baseDataModel.get(binding)).toBeUndefined(); + + // Should log warning + expect(mockLogger.warn).toHaveBeenCalledWith( + expect.stringContaining("Blocked write to protected path: errorState"), + ); + + // Should return no-op update + expect(updates.length).toBe(1); + expect(updates[0].binding).toBe(binding); + }); + + it("should block writes to nested errorState paths", () => { + const binding = parser.parse("errorState.message"); + middleware.set([[binding, "test message"]], undefined, baseDataModel); + + expect(baseDataModel.get(binding)).toBeUndefined(); + expect(mockLogger.warn).toHaveBeenCalledWith( + expect.stringContaining( + "Blocked write to protected path: errorState.message", + ), + ); + }); + + it("should allow writes to other paths", () => { + const binding = parser.parse("foo"); + const updates = middleware.set( + [[binding, "newValue"]], + undefined, + baseDataModel, + ); + + expect(baseDataModel.get(binding)).toBe("newValue"); + expect(mockLogger.warn).not.toHaveBeenCalled(); + expect(updates.length).toBe(1); + expect(updates[0].newValue).toBe("newValue"); + }); + + it("should allow writes when enabled", () => { + const binding = parser.parse("errorState"); + + middleware.enableWrites(); + const updates = middleware.set( + [[binding, { message: "test" }]], + undefined, + baseDataModel, + ); + middleware.disableWrites(); + + expect(baseDataModel.get(binding)).toEqual({ message: "test" }); + expect(mockLogger.warn).not.toHaveBeenCalled(); + expect(updates.length).toBe(1); + expect(updates[0].newValue).toEqual({ message: "test" }); + }); + + it("should block writes after disabling", () => { + const binding = parser.parse("errorState"); + + middleware.enableWrites(); + middleware.disableWrites(); + + middleware.set( + [[binding, { message: "test" }]], + undefined, + baseDataModel, + ); + + expect(baseDataModel.get(binding)).toBeUndefined(); + expect(mockLogger.warn).toHaveBeenCalled(); + }); + }); + + describe("get", () => { + it("should always allow reads", () => { + const binding = parser.parse("errorState"); + + // Set value directly on base model + baseDataModel.set([[binding, { message: "test" }]]); + + const value = middleware.get(binding, undefined, baseDataModel); + expect(value).toEqual({ message: "test" }); + }); + }); + + describe("delete", () => { + it("should block deletes to errorState by default", () => { + const binding = parser.parse("errorState"); + + // Set value first + baseDataModel.set([[binding, { message: "test" }]]); + + middleware.delete(binding, undefined, baseDataModel); + + // Should still exist + expect(baseDataModel.get(binding)).toEqual({ message: "test" }); + expect(mockLogger.warn).toHaveBeenCalledWith( + expect.stringContaining("Blocked delete of protected path: errorState"), + ); + }); + + it("should allow deletes when enabled", () => { + const binding = parser.parse("errorState"); + + // Set value first + baseDataModel.set([[binding, { message: "test" }]]); + + middleware.enableWrites(); + middleware.delete(binding, undefined, baseDataModel); + middleware.disableWrites(); + + expect(baseDataModel.get(binding)).toBeUndefined(); + expect(mockLogger.warn).not.toHaveBeenCalled(); + }); + + it("should allow deletes to other paths", () => { + const binding = parser.parse("foo"); + + middleware.delete(binding, undefined, baseDataModel); + + expect(baseDataModel.get(binding)).toBeUndefined(); + expect(mockLogger.warn).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/core/player/src/controllers/error/controller.ts b/core/player/src/controllers/error/controller.ts index a4786ba42..776493d38 100644 --- a/core/player/src/controllers/error/controller.ts +++ b/core/player/src/controllers/error/controller.ts @@ -2,6 +2,7 @@ import { SyncBailHook } from "tapable-ts"; import type { Logger } from "../../logger"; import type { DataController } from "../data/controller"; import type { PlayerError, ErrorMetadata, ErrorSeverity } from "./types"; +import { ErrorStateMiddleware } from "./middleware"; export interface ErrorControllerHooks { /** @@ -17,8 +18,18 @@ export interface ErrorControllerHooks { export interface ErrorControllerOptions { /** Optional logger for error operations */ logger?: Logger; - /** Required for setting data.errorState */ - dataController?: DataController; + /** Data model for setting errorState */ + model?: DataController; +} + +/** + * Get the middleware for protecting errorState from external writes + * Should be added to DataController's middleware array + */ +export function getErrorStateMiddleware(options?: { + logger?: Logger; +}): ErrorStateMiddleware { + return new ErrorStateMiddleware(options); } /** Default error format for data.errorState */ @@ -39,14 +50,34 @@ export class ErrorController { onError: new SyncBailHook<[PlayerError], boolean | undefined>(), }; - private readonly log?: Logger; - private readonly dataController?: DataController; + private options?: ErrorControllerOptions; + private readonly middleware: ErrorStateMiddleware; + /** + * Complete history of all captured errors in chronological order + * Newest errors are APPENDED to the end of the array + */ private errorHistory: PlayerError[] = []; private currentError?: PlayerError; constructor(options: ErrorControllerOptions = {}) { - this.log = options.logger; - this.dataController = options.dataController; + this.options = options; + + this.middleware = getErrorStateMiddleware({ logger: options.logger }); + } + + /** + * Get the middleware for protecting errorState + * This should be added to DataController's middleware array + */ + public getDataMiddleware(): ErrorStateMiddleware { + return this.middleware; + } + + /** + * Set options after initialization (e.g., to inject DataController and logger) + */ + public setOptions(options: ErrorControllerOptions): void { + this.options = options; } /** @@ -67,17 +98,17 @@ export class ErrorController { // Set as current error this.currentError = playerError; - this.log?.debug( + this.options?.logger?.debug( `[ErrorController] Captured error: ${error.message}`, playerError.metadata, ); // Notify listeners and check if navigation should be skipped // Plugins can observe the error and optionally return true to bail - const shouldSkip = this.hooks.onError.call(playerError); + const shouldSkip = this.hooks.onError.call(playerError) ?? false; if (shouldSkip) { - this.log?.debug( + this.options?.logger?.debug( "[ErrorController] Error state navigation skipped by plugin", ); return playerError; @@ -121,39 +152,78 @@ export class ErrorController { } /** - * Clear all errors (history + current) + * Clear all errors (history + current + data model) */ public clearErrors(): void { this.errorHistory = []; this.currentError = undefined; - this.log?.debug("[ErrorController] All errors cleared"); + this.deleteErrorFromDataModel(); + this.options?.logger?.debug("[ErrorController] All errors cleared"); } /** - * Clear only current error, preserve history + * Clear only current error and remove from data model, preserve history */ public clearCurrentError(): void { this.currentError = undefined; - this.log?.debug("[ErrorController] Current error cleared"); + this.deleteErrorFromDataModel(); + this.options?.logger?.debug("[ErrorController] Current error cleared"); } /** * Write error to data model errorState */ private setErrorInDataModel(playerError: PlayerError): void { - if (!this.dataController) { - this.log?.warn("[ErrorController] No DataController available"); + if (!this.options?.model) { + this.options?.logger?.warn( + "[ErrorController] No DataController available", + ); return; } try { const formattedError = this.formatErrorForData(playerError); - this.dataController.set([["errorState", formattedError]]); - this.log?.debug( + + // Temporarily allow writes to errorState + this.middleware?.enableWrites(); + this.options.model.set([["errorState", formattedError]]); + this.middleware?.disableWrites(); + + this.options?.logger?.debug( "[ErrorController] Error set in data model at 'data.errorState'", ); } catch (e) { - this.log?.error("[ErrorController] Failed to set error in data model", e); + this.middleware?.disableWrites(); + this.options?.logger?.error( + "[ErrorController] Failed to set error in data model", + e, + ); + } + } + + /** + * Remove errorState from data model + */ + private deleteErrorFromDataModel(): void { + if (!this.options?.model) { + return; + } + + try { + // Temporarily allow deletes to errorState + this.middleware?.enableWrites(); + this.options.model.delete("errorState"); + this.middleware?.disableWrites(); + + this.options?.logger?.debug( + "[ErrorController] errorState deleted from data model", + ); + } catch (e) { + this.middleware?.disableWrites(); + this.options?.logger?.error( + "[ErrorController] Failed to delete errorState from data model", + e, + ); } } } diff --git a/core/player/src/controllers/error/index.ts b/core/player/src/controllers/error/index.ts index 000fae134..67a5ccbd0 100644 --- a/core/player/src/controllers/error/index.ts +++ b/core/player/src/controllers/error/index.ts @@ -1,2 +1,3 @@ export * from "./controller"; export * from "./types"; +export * from "./middleware"; diff --git a/core/player/src/controllers/error/middleware.ts b/core/player/src/controllers/error/middleware.ts new file mode 100644 index 000000000..968211278 --- /dev/null +++ b/core/player/src/controllers/error/middleware.ts @@ -0,0 +1,111 @@ +import type { BindingInstance } from "../../binding"; +import type { + BatchSetTransaction, + DataModelImpl, + DataModelMiddleware, + DataModelOptions, + Updates, +} from "../../data"; +import type { Logger } from "../../logger"; + +/** + * Middleware that prevents external writes to errorState + * Only ErrorController should write to this path + */ +export class ErrorStateMiddleware implements DataModelMiddleware { + name = "error-state-middleware"; + + private logger?: Logger; + private allowWrites = false; + + constructor(options?: { logger?: Logger }) { + this.logger = options?.logger; + } + + /** + * Allow ErrorController to temporarily bypass protection + */ + public enableWrites(): void { + this.allowWrites = true; + } + + /** + * Re-enable protection after ErrorController write + */ + public disableWrites(): void { + this.allowWrites = false; + } + + public set( + transaction: BatchSetTransaction, + options?: DataModelOptions, + next?: DataModelImpl, + ): Updates { + // If writes are allowed (from ErrorController), pass through + if (this.allowWrites) { + return next?.set(transaction, options) ?? []; + } + + // Filter out any writes to errorState + const filteredTransaction: BatchSetTransaction = []; + const blockedBindings: BindingInstance[] = []; + + transaction.forEach(([binding, value]) => { + const path = binding.asString(); + + // Block writes to errorState namespace + if (path === "errorState" || path.startsWith("errorState.")) { + blockedBindings.push(binding); + this.logger?.warn( + `[ErrorStateMiddleware] Blocked write to protected path: ${path}`, + ); + } else { + filteredTransaction.push([binding, value]); + } + }); + + // Process allowed writes + const validResults = next?.set(filteredTransaction, options) ?? []; + + // Return no-op updates for blocked paths + const blockedResults: Updates = blockedBindings.map((binding) => ({ + binding, + oldValue: next?.get(binding, options), + newValue: next?.get(binding, options), // Keep old value + force: false, + })); + + return [...validResults, ...blockedResults]; + } + + public get( + binding: BindingInstance, + options?: DataModelOptions, + next?: DataModelImpl, + ) { + return next?.get(binding, options); + } + + public delete( + binding: BindingInstance, + options?: DataModelOptions, + next?: DataModelImpl, + ) { + // If writes are allowed (from ErrorController), pass through + if (this.allowWrites) { + return next?.delete(binding, options); + } + + const path = binding.asString(); + + // Block deletes to errorState namespace + if (path === "errorState" || path.startsWith("errorState.")) { + this.logger?.warn( + `[ErrorStateMiddleware] Blocked delete of protected path: ${path}`, + ); + return; + } + + return next?.delete(binding, options); + } +} diff --git a/core/player/src/player.ts b/core/player/src/player.ts index 33133167a..fada62754 100644 --- a/core/player/src/player.ts +++ b/core/player/src/player.ts @@ -219,7 +219,6 @@ export class Player { let expressionEvaluator: ExpressionEvaluator; // eslint-disable-next-line prefer-const let dataController: DataController; - let errorController: ErrorController; const pathResolver = new BindingParser({ get: (binding) => { @@ -244,9 +243,16 @@ export class Player { this.hooks.validationController.call(validationController); + const errorController = new ErrorController(); + + this.hooks.errorController.call(errorController); + dataController = new DataController(userFlow.data, { pathResolver, - middleware: validationController.getDataMiddleware(), + middleware: [ + ...validationController.getDataMiddleware(), + errorController.getDataMiddleware(), + ], logger: this.logger, }); @@ -267,14 +273,11 @@ export class Player { (binding) => schema.getApparentType(binding)?.default, ); - // eslint-disable-next-line prefer-const - errorController = new ErrorController({ + errorController.setOptions({ + model: dataController, logger: this.logger, - dataController, }); - this.hooks.errorController.call(errorController); - // eslint-disable-next-line prefer-const let viewController: ViewController; From 6ce901fa597e4a4105d254a927b9507af6817edd Mon Sep 17 00:00:00 2001 From: Chloe Han Date: Tue, 23 Dec 2025 11:54:16 -0500 Subject: [PATCH 4/9] update metadata --- .../src/controllers/error/controller.ts | 61 +++++++------------ .../src/controllers/error/middleware.ts | 10 +-- core/player/src/controllers/error/types.ts | 30 +++------ 3 files changed, 37 insertions(+), 64 deletions(-) diff --git a/core/player/src/controllers/error/controller.ts b/core/player/src/controllers/error/controller.ts index 776493d38..bb3f7125e 100644 --- a/core/player/src/controllers/error/controller.ts +++ b/core/player/src/controllers/error/controller.ts @@ -32,18 +32,6 @@ export function getErrorStateMiddleware(options?: { return new ErrorStateMiddleware(options); } -/** Default error format for data.errorState */ -interface FormattedError { - message: string; - name: string; - errorType?: string; - severity?: ErrorSeverity; - assetId?: string; - assetType?: string; - bindingPath?: string; - context?: Record; -} - /** The orchestrator for player error handling */ export class ErrorController { public hooks: ErrorControllerHooks = { @@ -83,13 +71,17 @@ export class ErrorController { /** * Capture error with metadata, add to history, fire hooks, update data model */ - public captureError(error: Error, metadata: ErrorMetadata = {}): PlayerError { + public captureError( + error: Error, + errorType: string, + severity?: ErrorSeverity, + metadata?: ErrorMetadata, + ): PlayerError { const playerError: PlayerError = { error, - metadata: { - ...metadata, - timestamp: metadata.timestamp ?? Date.now(), - }, + errorType, + severity, + metadata, }; // Add to history @@ -100,7 +92,7 @@ export class ErrorController { this.options?.logger?.debug( `[ErrorController] Captured error: ${error.message}`, - playerError.metadata, + { errorType, severity, metadata }, ); // Notify listeners and check if navigation should be skipped @@ -119,24 +111,6 @@ export class ErrorController { return playerError; } - /** - * Format error for content binding - */ - public formatErrorForData(playerError: PlayerError): FormattedError { - const { error, metadata } = playerError; - - return { - message: error.message, - name: error.name, - errorType: metadata.errorType, - severity: metadata.severity, - assetId: metadata.assetContext?.asset?.id, - assetType: metadata.assetContext?.asset?.type, - bindingPath: metadata.assetContext?.bindingPath, - context: metadata.context, - }; - } - /** * Get most recent error */ @@ -182,11 +156,22 @@ export class ErrorController { } try { - const formattedError = this.formatErrorForData(playerError); + const { error, errorType, severity, metadata } = playerError; // Temporarily allow writes to errorState this.middleware?.enableWrites(); - this.options.model.set([["errorState", formattedError]]); + this.options.model.set([ + [ + "errorState", + { + message: error.message, + name: error.name, + errorType, + severity, + ...metadata, + }, + ], + ]); this.middleware?.disableWrites(); this.options?.logger?.debug( diff --git a/core/player/src/controllers/error/middleware.ts b/core/player/src/controllers/error/middleware.ts index 968211278..43b387648 100644 --- a/core/player/src/controllers/error/middleware.ts +++ b/core/player/src/controllers/error/middleware.ts @@ -82,7 +82,7 @@ export class ErrorStateMiddleware implements DataModelMiddleware { binding: BindingInstance, options?: DataModelOptions, next?: DataModelImpl, - ) { + ): unknown { return next?.get(binding, options); } @@ -90,10 +90,10 @@ export class ErrorStateMiddleware implements DataModelMiddleware { binding: BindingInstance, options?: DataModelOptions, next?: DataModelImpl, - ) { + ): boolean { // If writes are allowed (from ErrorController), pass through if (this.allowWrites) { - return next?.delete(binding, options); + return next?.delete(binding, options) ?? false; } const path = binding.asString(); @@ -103,9 +103,9 @@ export class ErrorStateMiddleware implements DataModelMiddleware { this.logger?.warn( `[ErrorStateMiddleware] Blocked delete of protected path: ${path}`, ); - return; + return false; } - return next?.delete(binding, options); + return next?.delete(binding, options) ?? false; } } diff --git a/core/player/src/controllers/error/types.ts b/core/player/src/controllers/error/types.ts index 61e4e75bb..f44e236d2 100644 --- a/core/player/src/controllers/error/types.ts +++ b/core/player/src/controllers/error/types.ts @@ -1,5 +1,3 @@ -import type { Asset } from "@player-ui/types"; - /** Severity levels */ export enum ErrorSeverity { FATAL = "fatal", // Cannot continue, flow must end @@ -21,24 +19,10 @@ export const ErrorTypes = { PLUGIN: "plugin", } as const; -export interface AssetErrorContext { - asset?: Asset; - bindingPath?: string; -} - +/** + * Error metadata + */ export interface ErrorMetadata { - /** Flow state name when error occurred */ - state?: string; - /** Additional context (transition name, view ref, etc) */ - context?: Record; - /** timestamp */ - timestamp?: number; - /** Error category (use ErrorTypes constants or custom plugin types) */ - errorType?: string; - /** Impact level */ - severity?: ErrorSeverity; - /** Asset-specific details */ - assetContext?: AssetErrorContext; /** Allow custom fields for domain-specific information */ [key: string]: unknown; } @@ -46,6 +30,10 @@ export interface ErrorMetadata { export interface PlayerError { /** Native Error object */ error: Error; - /** Error metadata */ - metadata: ErrorMetadata; + /** Error category (use ErrorTypes constants or custom plugin types) */ + errorType: string; + /** Impact level */ + severity?: ErrorSeverity; + /** Additional metadata */ + metadata?: ErrorMetadata; } From 6e7d6dd94c1a1eea7d74aa3e469f0fb2d0943911 Mon Sep 17 00:00:00 2001 From: Chloe Han Date: Thu, 8 Jan 2026 09:45:38 -0500 Subject: [PATCH 5/9] update middleware with auth symbol --- .../error/__tests__/controller.test.ts | 157 ++++++++---------- .../error/__tests__/middleware.test.ts | 94 ++++++++--- .../src/controllers/error/controller.ts | 60 +++---- .../src/controllers/error/middleware.ts | 40 ++--- core/player/src/data/model.ts | 6 + 5 files changed, 191 insertions(+), 166 deletions(-) diff --git a/core/player/src/controllers/error/__tests__/controller.test.ts b/core/player/src/controllers/error/__tests__/controller.test.ts index 28d75ab88..0ffc6504b 100644 --- a/core/player/src/controllers/error/__tests__/controller.test.ts +++ b/core/player/src/controllers/error/__tests__/controller.test.ts @@ -33,27 +33,26 @@ describe("ErrorController", () => { describe("captureError", () => { it("should capture error with metadata", () => { const error = new Error("Test error"); - const metadata = { - errorType: ErrorTypes.EXPRESSION, - severity: ErrorSeverity.ERROR, - state: "VIEW_Test", - }; - const playerError = errorController.captureError(error, metadata); + const playerError = errorController.captureError( + error, + ErrorTypes.EXPRESSION, + ErrorSeverity.ERROR, + { state: "VIEW_Test" }, + ); expect(playerError.error).toBe(error); - expect(playerError.metadata.errorType).toBe(ErrorTypes.EXPRESSION); - expect(playerError.metadata.severity).toBe(ErrorSeverity.ERROR); - expect(playerError.metadata.state).toBe("VIEW_Test"); - expect(playerError.metadata.timestamp).toBeDefined(); + expect(playerError.errorType).toBe(ErrorTypes.EXPRESSION); + expect(playerError.severity).toBe(ErrorSeverity.ERROR); + expect(playerError.metadata?.state).toBe("VIEW_Test"); }); it("should add error to history", () => { const error1 = new Error("Error 1"); const error2 = new Error("Error 2"); - errorController.captureError(error1); - errorController.captureError(error2); + errorController.captureError(error1, "test-error-1"); + errorController.captureError(error2, "test-error-2"); const history = errorController.getErrors(); expect(history).toHaveLength(2); @@ -63,7 +62,7 @@ describe("ErrorController", () => { it("should set as current error", () => { const error = new Error("Test error"); - errorController.captureError(error); + errorController.captureError(error, "test-error"); const currentError = errorController.getCurrentError(); expect(currentError?.error).toBe(error); @@ -71,61 +70,23 @@ describe("ErrorController", () => { it("should write to data model", () => { const error = new Error("Test error"); - errorController.captureError(error, { - errorType: ErrorTypes.EXPRESSION, - }); + errorController.captureError(error, ErrorTypes.EXPRESSION); - expect(mockDataController.set).toHaveBeenCalledWith([ + expect(mockDataController.set).toHaveBeenCalledWith( [ - "errorState", - expect.objectContaining({ - message: "Test error", - name: "Error", - errorType: ErrorTypes.EXPRESSION, - }), + [ + "errorState", + expect.objectContaining({ + message: "Test error", + name: "Error", + errorType: ErrorTypes.EXPRESSION, + }), + ], ], - ]); - }); - }); - - describe("formatErrorForData", () => { - it("should format error with default formatter", () => { - const error = new Error("Test error"); - const playerError = errorController.captureError(error, { - errorType: ErrorTypes.VIEW, - severity: ErrorSeverity.ERROR, - state: "VIEW_Test", - context: { viewId: "test-view" }, - }); - - const formatted = errorController.formatErrorForData(playerError); - - expect(formatted).toEqual({ - message: "Test error", - name: "Error", - errorType: ErrorTypes.VIEW, - severity: ErrorSeverity.ERROR, - assetId: undefined, - assetType: undefined, - bindingPath: undefined, - context: { viewId: "test-view" }, - }); - }); - - it("should include asset context", () => { - const error = new Error("Test error"); - const playerError = errorController.captureError(error, { - assetContext: { - asset: { id: "test-asset", type: "form" } as any, - bindingPath: "user.email", - }, - }); - - const formatted = errorController.formatErrorForData(playerError); - - expect(formatted.assetId).toBe("test-asset"); - expect(formatted.assetType).toBe("form"); - expect(formatted.bindingPath).toBe("user.email"); + expect.objectContaining({ + authToken: expect.any(Symbol), + }), + ); }); }); @@ -136,7 +97,7 @@ describe("ErrorController", () => { it("should return current error", () => { const error = new Error("Test error"); - errorController.captureError(error); + errorController.captureError(error, "test-error"); expect(errorController.getCurrentError()?.error).toBe(error); }); @@ -151,8 +112,8 @@ describe("ErrorController", () => { const error1 = new Error("Error 1"); const error2 = new Error("Error 2"); - errorController.captureError(error1); - errorController.captureError(error2); + errorController.captureError(error1, "error-1"); + errorController.captureError(error2, "error-2"); const errors = errorController.getErrors(); expect(errors).toHaveLength(2); @@ -163,8 +124,8 @@ describe("ErrorController", () => { describe("clearErrors", () => { it("should clear all errors and history", () => { - errorController.captureError(new Error("Error 1")); - errorController.captureError(new Error("Error 2")); + errorController.captureError(new Error("Error 1"), "error-1"); + errorController.captureError(new Error("Error 2"), "error-2"); errorController.clearErrors(); @@ -173,19 +134,24 @@ describe("ErrorController", () => { }); it("should delete errorState from data model", () => { - errorController.captureError(new Error("Test error")); + errorController.captureError(new Error("Test error"), "test-error"); // Reset mock to track only clearErrors call vitest.clearAllMocks(); errorController.clearErrors(); - expect(mockDataController.delete).toHaveBeenCalledWith("errorState"); + expect(mockDataController.delete).toHaveBeenCalledWith( + "errorState", + expect.objectContaining({ + authToken: expect.any(Symbol), + }), + ); }); it("should handle missing data controller gracefully", () => { const controller = new ErrorController({ logger: mockLogger }); - controller.captureError(new Error("Test error")); + controller.captureError(new Error("Test error"), "test-error"); expect(() => controller.clearErrors()).not.toThrow(); }); @@ -196,8 +162,8 @@ describe("ErrorController", () => { const error1 = new Error("Error 1"); const error2 = new Error("Error 2"); - errorController.captureError(error1); - errorController.captureError(error2); + errorController.captureError(error1, "error-1"); + errorController.captureError(error2, "error-2"); errorController.clearCurrentError(); @@ -206,19 +172,24 @@ describe("ErrorController", () => { }); it("should delete errorState from data model", () => { - errorController.captureError(new Error("Test error")); + errorController.captureError(new Error("Test error"), "test-error"); // Reset mock to track only clearCurrentError call vitest.clearAllMocks(); errorController.clearCurrentError(); - expect(mockDataController.delete).toHaveBeenCalledWith("errorState"); + expect(mockDataController.delete).toHaveBeenCalledWith( + "errorState", + expect.objectContaining({ + authToken: expect.any(Symbol), + }), + ); }); it("should handle missing data controller gracefully", () => { const controller = new ErrorController({ logger: mockLogger }); - controller.captureError(new Error("Test error")); + controller.captureError(new Error("Test error"), "test-error"); expect(() => controller.clearCurrentError()).not.toThrow(); }); @@ -230,7 +201,7 @@ describe("ErrorController", () => { errorController.hooks.onError.tap("test", onErrorSpy); const error = new Error("Test error"); - errorController.captureError(error); + errorController.captureError(error, "test-error"); expect(onErrorSpy).toHaveBeenCalledTimes(1); expect(onErrorSpy).toHaveBeenCalledWith( @@ -250,7 +221,7 @@ describe("ErrorController", () => { errorController.hooks.onError.tap("observer2", observer2); const error = new Error("Test error"); - const playerError = errorController.captureError(error); + const playerError = errorController.captureError(error, "test-error"); expect(observer1).toHaveBeenCalledTimes(1); expect(skipPlugin).toHaveBeenCalledWith(playerError); @@ -266,7 +237,7 @@ describe("ErrorController", () => { errorController.hooks.onError.tap("observer2", observer2); const error = new Error("Test error"); - errorController.captureError(error); + errorController.captureError(error, "test-error"); expect(observer1).toHaveBeenCalledTimes(1); expect(observer2).toHaveBeenCalledTimes(1); @@ -278,15 +249,14 @@ describe("ErrorController", () => { describe("custom error types", () => { it("should allow custom plugin error types", () => { const error = new Error("Custom plugin error"); - const playerError = errorController.captureError(error, { - errorType: "my-custom-plugin", - severity: ErrorSeverity.WARNING, - }); - - expect(playerError.metadata.errorType).toBe("my-custom-plugin"); + const playerError = errorController.captureError( + error, + "my-custom-plugin", + ErrorSeverity.WARNING, + ); - const formatted = errorController.formatErrorForData(playerError); - expect(formatted.errorType).toBe("my-custom-plugin"); + expect(playerError.errorType).toBe("my-custom-plugin"); + expect(playerError.severity).toBe(ErrorSeverity.WARNING); }); }); @@ -301,10 +271,15 @@ describe("ErrorController", () => { errorController.captureError(new Error("Test error")); expect(mockDataController.set).toHaveBeenCalled(); - // Clear error should delete via middleware + // Clear error should delete via middleware with authToken vitest.clearAllMocks(); errorController.clearCurrentError(); - expect(mockDataController.delete).toHaveBeenCalledWith("errorState"); + expect(mockDataController.delete).toHaveBeenCalledWith( + "errorState", + expect.objectContaining({ + authToken: expect.any(Symbol), + }), + ); }); }); }); diff --git a/core/player/src/controllers/error/__tests__/middleware.test.ts b/core/player/src/controllers/error/__tests__/middleware.test.ts index b65787d59..db6d433e6 100644 --- a/core/player/src/controllers/error/__tests__/middleware.test.ts +++ b/core/player/src/controllers/error/__tests__/middleware.test.ts @@ -10,6 +10,7 @@ describe("ErrorStateMiddleware", () => { let baseDataModel: DataModelImpl; let mockLogger: Logger; let parser: BindingParser; + let authSymbol: symbol; beforeEach(() => { mockLogger = { @@ -20,7 +21,11 @@ describe("ErrorStateMiddleware", () => { error: vitest.fn(), }; - middleware = new ErrorStateMiddleware({ logger: mockLogger }); + authSymbol = Symbol("test-auth"); + middleware = new ErrorStateMiddleware({ + logger: mockLogger, + authSymbol, + }); baseDataModel = new LocalModel({ foo: "bar" }); parser = new BindingParser({ @@ -31,7 +36,7 @@ describe("ErrorStateMiddleware", () => { }); describe("set", () => { - it("should block writes to errorState by default", () => { + it("should block writes to errorState without authToken", () => { const binding = parser.parse("errorState"); const updates = middleware.set( [[binding, { message: "test" }]], @@ -49,7 +54,9 @@ describe("ErrorStateMiddleware", () => { // Should return no-op update expect(updates.length).toBe(1); - expect(updates[0].binding).toBe(binding); + expect(updates[0]!.binding).toBe(binding); + expect(updates[0]!.newValue).toBeUndefined(); + expect(updates[0]!.oldValue).toBeUndefined(); }); it("should block writes to nested errorState paths", () => { @@ -75,41 +82,65 @@ describe("ErrorStateMiddleware", () => { expect(baseDataModel.get(binding)).toBe("newValue"); expect(mockLogger.warn).not.toHaveBeenCalled(); expect(updates.length).toBe(1); - expect(updates[0].newValue).toBe("newValue"); + expect(updates[0]!.newValue).toBe("newValue"); }); - it("should allow writes when enabled", () => { + it("should allow writes when authorized with authToken", () => { const binding = parser.parse("errorState"); - middleware.enableWrites(); const updates = middleware.set( [[binding, { message: "test" }]], - undefined, + { authToken: authSymbol }, baseDataModel, ); - middleware.disableWrites(); expect(baseDataModel.get(binding)).toEqual({ message: "test" }); expect(mockLogger.warn).not.toHaveBeenCalled(); expect(updates.length).toBe(1); - expect(updates[0].newValue).toEqual({ message: "test" }); + expect(updates[0]!.newValue).toEqual({ message: "test" }); }); - it("should block writes after disabling", () => { + it("should block writes with wrong authToken", () => { const binding = parser.parse("errorState"); - - middleware.enableWrites(); - middleware.disableWrites(); + const wrongSymbol = Symbol("wrong-auth"); middleware.set( [[binding, { message: "test" }]], - undefined, + { authToken: wrongSymbol }, baseDataModel, ); expect(baseDataModel.get(binding)).toBeUndefined(); expect(mockLogger.warn).toHaveBeenCalled(); }); + + it("should handle mixed transactions with blocked and allowed paths", () => { + const errorBinding = parser.parse("errorState"); + const fooBinding = parser.parse("foo"); + + const updates = middleware.set( + [ + [errorBinding, { message: "blocked" }], + [fooBinding, "allowed"], + ], + undefined, + baseDataModel, + ); + + // foo should be updated + expect(baseDataModel.get(fooBinding)).toBe("allowed"); + + // errorState should not be updated + expect(baseDataModel.get(errorBinding)).toBeUndefined(); + + // Should have logged warning for errorState + expect(mockLogger.warn).toHaveBeenCalledWith( + expect.stringContaining("Blocked write to protected path: errorState"), + ); + + // Should have updates for both paths + expect(updates.length).toBe(2); + }); }); describe("get", () => { @@ -125,7 +156,7 @@ describe("ErrorStateMiddleware", () => { }); describe("delete", () => { - it("should block deletes to errorState by default", () => { + it("should block deletes to errorState without authToken", () => { const binding = parser.parse("errorState"); // Set value first @@ -140,20 +171,33 @@ describe("ErrorStateMiddleware", () => { ); }); - it("should allow deletes when enabled", () => { + it("should allow deletes when authorized with authToken", () => { const binding = parser.parse("errorState"); // Set value first baseDataModel.set([[binding, { message: "test" }]]); - middleware.enableWrites(); - middleware.delete(binding, undefined, baseDataModel); - middleware.disableWrites(); + middleware.delete(binding, { authToken: authSymbol }, baseDataModel); expect(baseDataModel.get(binding)).toBeUndefined(); expect(mockLogger.warn).not.toHaveBeenCalled(); }); + it("should block deletes with wrong authToken", () => { + const binding = parser.parse("errorState"); + const wrongSymbol = Symbol("wrong-auth"); + + // Set value first + baseDataModel.set([[binding, { message: "test" }]]); + + middleware.delete(binding, { authToken: wrongSymbol }, baseDataModel); + + expect(baseDataModel.get(binding)).toEqual({ message: "test" }); + expect(mockLogger.warn).toHaveBeenCalledWith( + expect.stringContaining("Blocked delete of protected path: errorState"), + ); + }); + it("should allow deletes to other paths", () => { const binding = parser.parse("foo"); @@ -162,5 +206,17 @@ describe("ErrorStateMiddleware", () => { expect(baseDataModel.get(binding)).toBeUndefined(); expect(mockLogger.warn).not.toHaveBeenCalled(); }); + + it("should allow deletes to nested errorState paths when authorized", () => { + const binding = parser.parse("errorState.nested.path"); + + // Set value first + baseDataModel.set([[binding, "test"]]); + + middleware.delete(binding, { authToken: authSymbol }, baseDataModel); + + expect(baseDataModel.get(binding)).toBeUndefined(); + expect(mockLogger.warn).not.toHaveBeenCalled(); + }); }); }); diff --git a/core/player/src/controllers/error/controller.ts b/core/player/src/controllers/error/controller.ts index bb3f7125e..a4391a4f7 100644 --- a/core/player/src/controllers/error/controller.ts +++ b/core/player/src/controllers/error/controller.ts @@ -4,6 +4,14 @@ import type { DataController } from "../data/controller"; import type { PlayerError, ErrorMetadata, ErrorSeverity } from "./types"; import { ErrorStateMiddleware } from "./middleware"; +/** + * Private symbol used to authorize ErrorController's writes to errorState + * Only ErrorController has access to this symbol + */ +const ERROR_CONTROLLER_AUTH_SYMBOL: unique symbol = Symbol( + "ERROR_CONTROLLER_AUTH", +); + export interface ErrorControllerHooks { /** * Fired when any error is captured @@ -22,16 +30,6 @@ export interface ErrorControllerOptions { model?: DataController; } -/** - * Get the middleware for protecting errorState from external writes - * Should be added to DataController's middleware array - */ -export function getErrorStateMiddleware(options?: { - logger?: Logger; -}): ErrorStateMiddleware { - return new ErrorStateMiddleware(options); -} - /** The orchestrator for player error handling */ export class ErrorController { public hooks: ErrorControllerHooks = { @@ -50,7 +48,10 @@ export class ErrorController { constructor(options: ErrorControllerOptions = {}) { this.options = options; - this.middleware = getErrorStateMiddleware({ logger: options.logger }); + this.middleware = new ErrorStateMiddleware({ + logger: options.logger, + authSymbol: ERROR_CONTROLLER_AUTH_SYMBOL, + }); } /** @@ -158,27 +159,27 @@ export class ErrorController { try { const { error, errorType, severity, metadata } = playerError; - // Temporarily allow writes to errorState - this.middleware?.enableWrites(); - this.options.model.set([ + // Pass auth token to authorize write through middleware + this.options.model.set( [ - "errorState", - { - message: error.message, - name: error.name, - errorType, - severity, - ...metadata, - }, + [ + "errorState", + { + message: error.message, + name: error.name, + errorType, + severity, + ...metadata, + }, + ], ], - ]); - this.middleware?.disableWrites(); + { authToken: ERROR_CONTROLLER_AUTH_SYMBOL }, + ); this.options?.logger?.debug( "[ErrorController] Error set in data model at 'data.errorState'", ); } catch (e) { - this.middleware?.disableWrites(); this.options?.logger?.error( "[ErrorController] Failed to set error in data model", e, @@ -195,16 +196,15 @@ export class ErrorController { } try { - // Temporarily allow deletes to errorState - this.middleware?.enableWrites(); - this.options.model.delete("errorState"); - this.middleware?.disableWrites(); + // Pass auth token to authorize delete through middleware + this.options.model.delete("errorState", { + authToken: ERROR_CONTROLLER_AUTH_SYMBOL, + }); this.options?.logger?.debug( "[ErrorController] errorState deleted from data model", ); } catch (e) { - this.middleware?.disableWrites(); this.options?.logger?.error( "[ErrorController] Failed to delete errorState from data model", e, diff --git a/core/player/src/controllers/error/middleware.ts b/core/player/src/controllers/error/middleware.ts index 43b387648..f41c423db 100644 --- a/core/player/src/controllers/error/middleware.ts +++ b/core/player/src/controllers/error/middleware.ts @@ -10,30 +10,17 @@ import type { Logger } from "../../logger"; /** * Middleware that prevents external writes to errorState - * Only ErrorController should write to this path + * Only authorized callers (with the auth symbol) can write to this path */ export class ErrorStateMiddleware implements DataModelMiddleware { name = "error-state-middleware"; private logger?: Logger; - private allowWrites = false; + private authSymbol: symbol; - constructor(options?: { logger?: Logger }) { - this.logger = options?.logger; - } - - /** - * Allow ErrorController to temporarily bypass protection - */ - public enableWrites(): void { - this.allowWrites = true; - } - - /** - * Re-enable protection after ErrorController write - */ - public disableWrites(): void { - this.allowWrites = false; + constructor(options: { logger?: Logger; authSymbol: symbol }) { + this.logger = options.logger; + this.authSymbol = options.authSymbol; } public set( @@ -41,8 +28,8 @@ export class ErrorStateMiddleware implements DataModelMiddleware { options?: DataModelOptions, next?: DataModelImpl, ): Updates { - // If writes are allowed (from ErrorController), pass through - if (this.allowWrites) { + // Check if this write is authorized by comparing the auth tokens + if (options?.authToken === this.authSymbol) { return next?.set(transaction, options) ?? []; } @@ -90,10 +77,11 @@ export class ErrorStateMiddleware implements DataModelMiddleware { binding: BindingInstance, options?: DataModelOptions, next?: DataModelImpl, - ): boolean { - // If writes are allowed (from ErrorController), pass through - if (this.allowWrites) { - return next?.delete(binding, options) ?? false; + ): void { + // Check if this delete is authorized by comparing the auth tokens + if (options?.authToken === this.authSymbol) { + next?.delete(binding, options); + return; } const path = binding.asString(); @@ -103,9 +91,9 @@ export class ErrorStateMiddleware implements DataModelMiddleware { this.logger?.warn( `[ErrorStateMiddleware] Blocked delete of protected path: ${path}`, ); - return false; + return; } - return next?.delete(binding, options) ?? false; + next?.delete(binding, options); } } diff --git a/core/player/src/data/model.ts b/core/player/src/data/model.ts index efb2d4d2d..be527d1ee 100644 --- a/core/player/src/data/model.ts +++ b/core/player/src/data/model.ts @@ -46,6 +46,12 @@ export interface DataModelOptions { */ silent?: boolean; + /** + * Authorization token for internal middleware operations + * Middleware can use this to verify the caller has permission for protected operations + */ + authToken?: symbol; + /** Other context associated with this request */ context?: { /** The data model to use when getting other data from the context of this request */ From c57ce6cd19943443a8c8530f110f90b65e882f53 Mon Sep 17 00:00:00 2001 From: Chloe Han Date: Wed, 14 Jan 2026 11:33:35 -0500 Subject: [PATCH 6/9] doc update --- .../content/docs/content/error-handling.mdx | 326 ++++++++++++++++++ docs/site/src/content/docs/content/index.mdx | 2 + 2 files changed, 328 insertions(+) create mode 100644 docs/site/src/content/docs/content/error-handling.mdx diff --git a/docs/site/src/content/docs/content/error-handling.mdx b/docs/site/src/content/docs/content/error-handling.mdx new file mode 100644 index 000000000..39f01bcd9 --- /dev/null +++ b/docs/site/src/content/docs/content/error-handling.mdx @@ -0,0 +1,326 @@ +--- +title: Error Handling +--- + +## Overview + +The `ErrorController` manages error handling throughout the flow lifecycle. It captures errors with metadata, maintains error history, and exposes errors to views via the protected `errorState` binding. The ErrorController is automatically instantiated by Player when a flow starts and is accessible through the `error` property in the controller state. + +### Key Features + +- **Error Capture**: Capture errors with type, severity, and custom metadata +- **Error History**: Maintain a complete history of all captured errors in chronological order +- **Protected State**: Automatically manages `errorState` in the data model with middleware protection +- **Hook System**: Allows plugins to observe errors and optionally prevent error state navigation +- **Cross-Platform**: Available on TypeScript/React, iOS, and JVM platforms + +## Error Types & Severity + +Player provides standard error types: `expression`, `binding`, `view`, `asset`, `navigation`, `validation`, `data`, `schema`, `network`, `plugin`. Plugins can define custom types. + +**Severity Levels:** +- `fatal` - Cannot continue, flow must end +- `error` - Standard error, may allow recovery +- `warning` - Non-blocking, logged for telemetry + +## API Reference + +### Capturing Errors + +```typescript +errorController.captureError( + new Error("Failed to load view"), + ErrorTypes.VIEW, + ErrorSeverity.ERROR, + { viewId: "my-view" } +); +``` + +The same API is available on Kotlin (`ErrorTypes.VIEW`, `ErrorSeverity.ERROR`) and Swift (`ErrorTypes.view`, `.error`). + +### Retrieving Errors + +```typescript +// Get the most recent error +const currentError = errorController.getCurrentError(); + +// Get complete error history +const allErrors = errorController.getErrors(); +``` + +### Clearing Errors + +```typescript +// Clear all errors (history + current + data model) +errorController.clearErrors(); + +// Clear only current error (preserve history) +errorController.clearCurrentError(); +``` + +## Accessing Error State in Views + +When an error is captured, the ErrorController automatically sets `errorState` in the data model. This makes error information accessible to views using bindings: + +```json +{ + "id": "error-view", + "type": "text", + "value": "Error: {{errorState.message}}" +} +``` + +### Error State Structure + +```json +{ + "errorState": { + "message": "Failed to load view", + "name": "Error", + "errorType": "view", + "severity": "error", + "viewId": "my-view" + } +} +``` + +## Protected ErrorState + +The `errorState` binding in the data model is **protected** by middleware: + +- ✅ Views can **read** `errorState` using bindings like `{{errorState.message}}` +- ❌ Only the ErrorController can **write** to `errorState` - views, expressions, and plugins cannot modify it directly + +This protection ensures error state integrity and prevents accidental overwrites. Expressions like `{{errorState}} = null` will be blocked. To clear errors, use `clearCurrentError()` or `clearErrors()` methods. + +## Hooks + +### onError Hook + +The `onError` hook fires whenever an error is captured, allowing plugins to observe errors and optionally prevent the error from being exposed to views. + +**Hook Behavior:** +- Called in order for each tapped plugin +- Return `true` to bail and prevent the error from being set in `errorState` (for custom error handling) +- Return `undefined` or `false` to continue - error will be set in `errorState` and trigger navigation if `errorState` property is defined +- Once `true` is returned, no further plugins are called + +### Example: Error Logging + +```typescript +export class ErrorLoggingPlugin implements Plugin { + name = "error-logging"; + + apply(player) { + player.hooks.errorController.tap(this.name, (errorController) => { + errorController.hooks.onError.tap(this.name, (playerError) => { + // Log to external service + logToService({ + message: playerError.error.message, + type: playerError.errorType, + severity: playerError.severity + }); + + // Return undefined to allow error state navigation + return undefined; + }); + }); + } +} +``` + +### Example: Custom Error Handler + +```typescript +errorController.hooks.onError.tap("custom-handler", (error) => { + // Handle specific error types with custom logic + if (error.errorType === "network" && error.severity === "warning") { + console.warn("Network warning:", error.error.message); + return true; // Prevent errorState from being set + } + + // Allow other errors to proceed normally + return undefined; +}); +``` + +### Cross-Platform Hook Examples + +```typescript +// TypeScript +errorController.hooks.onError.tap("logger", (error) => { + console.log(error.error.message); + return undefined; +}); +``` + +```kotlin +// Kotlin +player.errorController?.hooks?.onError?.tap { errorInfo -> + println("Error: ${errorInfo.message}") + null // Return null to continue +} +``` + +```swift +// Swift +errorController.hooks.onError.tap(name: "logger") { errorInfo in + print("Error: \(errorInfo.message)") + return nil // Return nil to continue +} +``` + +## Error Navigation Pattern + +Errors can trigger automatic navigation using the `errorState` property at node or flow level. + +### ErrorState Property Formats + +The `errorState` property supports two formats: + +1. **String** - Navigate to a single error state for all error types + ```json + "errorState": "ERROR_VIEW" + ``` + +2. **Object (Mapping)** - Navigate to different states based on error type + ```json + "errorState": { + "binding": "BINDING_ERROR_VIEW", + "validation": "VALIDATION_ERROR_VIEW", + "*": "GENERIC_ERROR_VIEW" + } + ``` + +Both formats can be used at node-level or flow-level. + +### Navigation Fallback Pattern + +The navigation follows a hierarchical fallback pattern: + +1. **Node-level `errorState`** - If defined, transition to the specified state +2. **Flow-level `errorState`** - If node-level not defined or no match found, use flow-level error state +3. **Flow rejection** - If neither defined, reject/fail the flow + +### String Format (Single Error State) + +Use a string to navigate to the same error state for all error types: + +```json +{ + "navigation": { + "BEGIN": "FLOW_1", + "FLOW_1": { + "startState": "VIEW_1", + "VIEW_1": { + "state_type": "VIEW", + "ref": "main-view", + "errorState": "ERROR_VIEW", + "transitions": { + "*": "END_Done" + } + }, + "ERROR_VIEW": { + "state_type": "VIEW", + "ref": "error-view", + "transitions": { + "*": "END_Error" + } + }, + "END_Done": { + "state_type": "END", + "outcome": "done" + }, + "END_Error": { + "state_type": "END", + "outcome": "error" + } + } + } +} +``` + +When an error is captured while on `VIEW_1`, Player will automatically call `transition("ERROR_VIEW")` for any error type. + +### Object Format (Error Type Mapping) + +Use an object to navigate to different states based on the error type: + +```json +{ + "navigation": { + "BEGIN": "FLOW_1", + "FLOW_1": { + "startState": "VIEW_1", + "VIEW_1": { + "state_type": "VIEW", + "ref": "main-view", + "errorState": { + "binding": "BINDING_ERROR_VIEW", + "validation": "VALIDATION_ERROR_VIEW", + "*": "GENERIC_ERROR_VIEW" + }, + "transitions": { + "*": "END_Done" + } + }, + "BINDING_ERROR_VIEW": { + "state_type": "VIEW", + "ref": "binding-error-view", + "transitions": { "*": "END_Error" } + }, + "VALIDATION_ERROR_VIEW": { + "state_type": "VIEW", + "ref": "validation-error-view", + "transitions": { "*": "END_Error" } + }, + "GENERIC_ERROR_VIEW": { + "state_type": "VIEW", + "ref": "generic-error-view", + "transitions": { "*": "END_Error" } + } + } + } +} +``` + +The `"*"` wildcard matches any error type not explicitly defined. If an error type doesn't match and no wildcard is defined, it falls through to the flow-level `errorState`. + +### Flow-Level Error Navigation + +Define `errorState` at the flow level as a fallback for states without their own `errorState`: + +```json +{ + "navigation": { + "BEGIN": "FLOW_1", + "FLOW_1": { + "startState": "VIEW_1", + "errorState": "ERROR_VIEW", + "VIEW_1": { + "state_type": "VIEW", + "ref": "main-view", + "transitions": { + "*": "VIEW_2" + } + }, + "VIEW_2": { + "state_type": "VIEW", + "ref": "second-view", + "transitions": { + "*": "END_Done" + } + }, + "ERROR_VIEW": { + "state_type": "VIEW", + "ref": "error-view", + "transitions": { + "*": "END_Error" + } + } + } + } +} +``` + +Flow-level `errorState` can also use the object format for error type mapping. diff --git a/docs/site/src/content/docs/content/index.mdx b/docs/site/src/content/docs/content/index.mdx index 8d219f196..511274aa3 100644 --- a/docs/site/src/content/docs/content/index.mdx +++ b/docs/site/src/content/docs/content/index.mdx @@ -20,6 +20,8 @@ The high level JSON payload for Player to render consists of: `id`, `views`, `da [**navigation**](./navigation) - (required) - The `navigation` section describes a finite state machine that is responsible driving the core Player experience. The nodes can either be: `VIEW`, `ACTION`, `EXTERNAL`, or `FLOW` +For information about error handling in Player, see [Error Handling](./error-handling). + ## Example Below is a minimal example of Player content that loads a view. From 61a85e7dcb75aaf4690f9b5211876f1efbef34ad Mon Sep 17 00:00:00 2001 From: Chloeeeeeee Date: Fri, 30 Jan 2026 13:52:53 -0500 Subject: [PATCH 7/9] error controller navigation (#775) * error controller navigation --- .../error/__tests__/controller.test.ts | 23 +- .../error/__tests__/navigation.test.ts | 154 ++++++++ .../src/controllers/error/controller.ts | 78 ++-- .../controllers/flow/__tests__/flow.test.ts | 345 ++++++++++++++++++ core/player/src/controllers/flow/flow.ts | 131 ++++++- core/player/src/player.ts | 7 +- core/types/src/index.ts | 21 +- 7 files changed, 728 insertions(+), 31 deletions(-) create mode 100644 core/player/src/controllers/error/__tests__/navigation.test.ts diff --git a/core/player/src/controllers/error/__tests__/controller.test.ts b/core/player/src/controllers/error/__tests__/controller.test.ts index 0ffc6504b..af47991a7 100644 --- a/core/player/src/controllers/error/__tests__/controller.test.ts +++ b/core/player/src/controllers/error/__tests__/controller.test.ts @@ -2,12 +2,15 @@ import { describe, it, beforeEach, expect, vitest } from "vitest"; import { ErrorController } from "../controller"; import { ErrorSeverity, ErrorTypes } from "../types"; import type { DataController } from "../../data/controller"; +import type { FlowController } from "../../flow/controller"; import type { Logger } from "../../../logger"; describe("ErrorController", () => { let errorController: ErrorController; let mockDataController: DataController; + let mockFlowController: FlowController; let mockLogger: Logger; + let mockFail: ReturnType; beforeEach(() => { mockDataController = { @@ -16,6 +19,10 @@ describe("ErrorController", () => { delete: vitest.fn(), } as any; + mockFlowController = { + current: undefined, + } as any; + mockLogger = { trace: vitest.fn(), debug: vitest.fn(), @@ -24,8 +31,12 @@ describe("ErrorController", () => { error: vitest.fn(), }; + mockFail = vitest.fn(); + errorController = new ErrorController({ logger: mockLogger, + flow: mockFlowController, + fail: mockFail, model: mockDataController, }); }); @@ -150,7 +161,11 @@ describe("ErrorController", () => { }); it("should handle missing data controller gracefully", () => { - const controller = new ErrorController({ logger: mockLogger }); + const controller = new ErrorController({ + logger: mockLogger, + flow: mockFlowController, + fail: mockFail, + }); controller.captureError(new Error("Test error"), "test-error"); expect(() => controller.clearErrors()).not.toThrow(); @@ -188,7 +203,11 @@ describe("ErrorController", () => { }); it("should handle missing data controller gracefully", () => { - const controller = new ErrorController({ logger: mockLogger }); + const controller = new ErrorController({ + logger: mockLogger, + flow: mockFlowController, + fail: mockFail, + }); controller.captureError(new Error("Test error"), "test-error"); expect(() => controller.clearCurrentError()).not.toThrow(); diff --git a/core/player/src/controllers/error/__tests__/navigation.test.ts b/core/player/src/controllers/error/__tests__/navigation.test.ts new file mode 100644 index 000000000..977bfee90 --- /dev/null +++ b/core/player/src/controllers/error/__tests__/navigation.test.ts @@ -0,0 +1,154 @@ +import { describe, it, beforeEach, expect, vitest } from "vitest"; +import { ErrorController } from "../controller"; +import { FlowController } from "../../flow/controller"; +import { FlowInstance } from "../../flow/flow"; +import { ErrorTypes } from "../types"; +import type { Logger } from "../../../logger"; +import type { DataController } from "../../data/controller"; + +describe("ErrorController Navigation", () => { + let errorController: ErrorController; + let mockFlowController: FlowController; + let mockFlowInstance: FlowInstance; + let mockDataController: DataController; + let mockLogger: Logger; + let mockFail: ReturnType; + + beforeEach(() => { + mockLogger = { + trace: vitest.fn(), + debug: vitest.fn(), + info: vitest.fn(), + warn: vitest.fn(), + error: vitest.fn(), + }; + + mockDataController = { + set: vitest.fn(), + get: vitest.fn(), + delete: vitest.fn(), + } as any; + + mockFail = vitest.fn(); + + // Mock FlowInstance + mockFlowInstance = { + currentState: { + name: "VIEW_Start", + value: { + state_type: "VIEW", + ref: "start-view", + transitions: { + next: "VIEW_Next", + }, + }, + }, + errorTransition: vitest.fn(), + } as any; + + // Mock FlowController + mockFlowController = { + current: mockFlowInstance, + reject: vitest.fn(), + } as any; + + errorController = new ErrorController({ + logger: mockLogger, + model: mockDataController, + flow: mockFlowController, + fail: mockFail, + }); + }); + + describe("errorTransitions navigation", () => { + it("should navigate using errorTransition method", () => { + const error = new Error("Test error"); + errorController.captureError(error, ErrorTypes.VIEW); + + // Should call errorTransition with errorType + expect(mockFlowInstance.errorTransition).toHaveBeenCalledWith( + ErrorTypes.VIEW, + ); + expect(mockFail).not.toHaveBeenCalled(); + }); + + it("should reject flow when errorTransition throws", () => { + mockFlowInstance.errorTransition = vitest.fn().mockImplementation(() => { + throw new Error("No errorTransitions defined"); + }); + + const error = new Error("Test error"); + errorController.captureError(error, ErrorTypes.NAVIGATION); + + expect(mockFlowInstance.errorTransition).toHaveBeenCalledWith( + ErrorTypes.NAVIGATION, + ); + expect(mockFail).toHaveBeenCalledWith(error); + }); + + it("should pass correct errorType to errorTransition", () => { + const error = new Error("Binding failed"); + errorController.captureError(error, "binding"); + + expect(mockFlowInstance.errorTransition).toHaveBeenCalledWith("binding"); + }); + + it("should pass custom errorType to errorTransition", () => { + const error = new Error("Custom error"); + errorController.captureError(error, "custom_type"); + + expect(mockFlowInstance.errorTransition).toHaveBeenCalledWith( + "custom_type", + ); + }); + }); + + describe("Hook integration", () => { + it("should skip navigation when plugin bails", () => { + errorController.hooks.onError.tap("test", () => true); + + const error = new Error("Test error"); + errorController.captureError(error, ErrorTypes.NAVIGATION); + + // Should not navigate when bailed + expect(mockFlowInstance.errorTransition).not.toHaveBeenCalled(); + expect(mockFail).not.toHaveBeenCalled(); + }); + + it("should navigate when plugin does not bail", () => { + errorController.hooks.onError.tap("test", () => undefined); + + const error = new Error("Test error"); + errorController.captureError(error, ErrorTypes.NAVIGATION); + + expect(mockFlowInstance.errorTransition).toHaveBeenCalledWith( + ErrorTypes.NAVIGATION, + ); + }); + + it("should navigate when plugin returns false", () => { + errorController.hooks.onError.tap("test", () => false); + + const error = new Error("Test error"); + errorController.captureError(error, ErrorTypes.VIEW); + + expect(mockFlowInstance.errorTransition).toHaveBeenCalledWith( + ErrorTypes.VIEW, + ); + }); + }); + + describe("No active flow", () => { + it("should warn and not navigate when no active flow", () => { + mockFlowController.current = undefined; + + const error = new Error("Test error"); + errorController.captureError(error, ErrorTypes.VIEW); + + expect(mockLogger.warn).toHaveBeenCalledWith( + "[ErrorController] No active flow instance for error navigation", + ); + expect(mockFail).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/core/player/src/controllers/error/controller.ts b/core/player/src/controllers/error/controller.ts index a4391a4f7..3247b82a8 100644 --- a/core/player/src/controllers/error/controller.ts +++ b/core/player/src/controllers/error/controller.ts @@ -1,6 +1,7 @@ import { SyncBailHook } from "tapable-ts"; import type { Logger } from "../../logger"; import type { DataController } from "../data/controller"; +import type { FlowController } from "../flow/controller"; import type { PlayerError, ErrorMetadata, ErrorSeverity } from "./types"; import { ErrorStateMiddleware } from "./middleware"; @@ -24,9 +25,13 @@ export interface ErrorControllerHooks { } export interface ErrorControllerOptions { - /** Optional logger for error operations */ - logger?: Logger; - /** Data model for setting errorState */ + /** Logger for error operations */ + logger: Logger; + /** Flow controller for error navigation */ + flow: FlowController; + /** Callback to fail/reject the flow */ + fail: (error: Error) => void; + /** Data model for setting errorState (can be set later via setOptions) */ model?: DataController; } @@ -36,7 +41,7 @@ export class ErrorController { onError: new SyncBailHook<[PlayerError], boolean | undefined>(), }; - private options?: ErrorControllerOptions; + private options: ErrorControllerOptions; private readonly middleware: ErrorStateMiddleware; /** * Complete history of all captured errors in chronological order @@ -45,7 +50,7 @@ export class ErrorController { private errorHistory: PlayerError[] = []; private currentError?: PlayerError; - constructor(options: ErrorControllerOptions = {}) { + constructor(options: ErrorControllerOptions) { this.options = options; this.middleware = new ErrorStateMiddleware({ @@ -63,14 +68,14 @@ export class ErrorController { } /** - * Set options after initialization (e.g., to inject DataController and logger) + * Set the DataController after initialization */ - public setOptions(options: ErrorControllerOptions): void { - this.options = options; + public setOptions(options: Pick): void { + this.options.model = options.model; } /** - * Capture error with metadata, add to history, fire hooks, update data model + * Capture error with metadata, add to history, fire hooks, update data model, and navigate */ public captureError( error: Error, @@ -91,7 +96,7 @@ export class ErrorController { // Set as current error this.currentError = playerError; - this.options?.logger?.debug( + this.options.logger.debug( `[ErrorController] Captured error: ${error.message}`, { errorType, severity, metadata }, ); @@ -101,17 +106,48 @@ export class ErrorController { const shouldSkip = this.hooks.onError.call(playerError) ?? false; if (shouldSkip) { - this.options?.logger?.debug( + this.options.logger.debug( "[ErrorController] Error state navigation skipped by plugin", ); return playerError; } + // Set error in data model this.setErrorInDataModel(playerError); + // Navigate to error state + this.navigateToErrorState(playerError); + return playerError; } + /** + * Navigate to error state using errorTransitions. + * Uses errorTransition() which handles node-level and flow-level fallback internally. + */ + private navigateToErrorState(playerError: PlayerError): void { + const flowInstance = this.options.flow.current; + + if (!flowInstance) { + this.options.logger.warn( + "[ErrorController] No active flow instance for error navigation", + ); + return; + } + + try { + flowInstance.errorTransition(playerError.errorType); + } catch (e) { + this.options.logger.error( + `[ErrorController] Error transition failed with unexpected error: ${e}`, + ); + + // Fallback: Reject flow + this.options.logger.debug("[ErrorController] Rejecting flow with error"); + this.options.fail(playerError.error); + } + } + /** * Get most recent error */ @@ -133,7 +169,7 @@ export class ErrorController { this.errorHistory = []; this.currentError = undefined; this.deleteErrorFromDataModel(); - this.options?.logger?.debug("[ErrorController] All errors cleared"); + this.options.logger.debug("[ErrorController] All errors cleared"); } /** @@ -142,17 +178,15 @@ export class ErrorController { public clearCurrentError(): void { this.currentError = undefined; this.deleteErrorFromDataModel(); - this.options?.logger?.debug("[ErrorController] Current error cleared"); + this.options.logger.debug("[ErrorController] Current error cleared"); } /** * Write error to data model errorState */ private setErrorInDataModel(playerError: PlayerError): void { - if (!this.options?.model) { - this.options?.logger?.warn( - "[ErrorController] No DataController available", - ); + if (!this.options.model) { + this.options.logger.warn("[ErrorController] No DataController available"); return; } @@ -176,11 +210,11 @@ export class ErrorController { { authToken: ERROR_CONTROLLER_AUTH_SYMBOL }, ); - this.options?.logger?.debug( + this.options.logger.debug( "[ErrorController] Error set in data model at 'data.errorState'", ); } catch (e) { - this.options?.logger?.error( + this.options.logger.error( "[ErrorController] Failed to set error in data model", e, ); @@ -191,7 +225,7 @@ export class ErrorController { * Remove errorState from data model */ private deleteErrorFromDataModel(): void { - if (!this.options?.model) { + if (!this.options.model) { return; } @@ -201,11 +235,11 @@ export class ErrorController { authToken: ERROR_CONTROLLER_AUTH_SYMBOL, }); - this.options?.logger?.debug( + this.options.logger.debug( "[ErrorController] errorState deleted from data model", ); } catch (e) { - this.options?.logger?.error( + this.options.logger.error( "[ErrorController] Failed to delete errorState from data model", e, ); diff --git a/core/player/src/controllers/flow/__tests__/flow.test.ts b/core/player/src/controllers/flow/__tests__/flow.test.ts index d2c0109dd..5afee8fdd 100644 --- a/core/player/src/controllers/flow/__tests__/flow.test.ts +++ b/core/player/src/controllers/flow/__tests__/flow.test.ts @@ -379,3 +379,348 @@ test("fails if transitioning to unknown state", () => { flow.transition("Next"); expect(flow.currentState?.name).toBe("View1"); }); + +describe("errorTransition", () => { + test("navigates using node-level errorTransitions", () => { + const flow = new FlowInstance("flow", { + startState: "View1", + View1: { + state_type: "VIEW", + ref: "view-1", + transitions: { + next: "End", + }, + errorTransitions: { + network: "NetworkError", + validation: "ValidationError", + }, + }, + NetworkError: { + state_type: "VIEW", + ref: "network-error", + transitions: {}, + }, + ValidationError: { + state_type: "VIEW", + ref: "validation-error", + transitions: {}, + }, + End: { + state_type: "END", + outcome: "done", + }, + }); + + flow.start(); + flow.errorTransition("network"); + + expect(flow.currentState?.name).toBe("NetworkError"); + }); + + test("uses wildcard in node-level errorTransitions", () => { + const flow = new FlowInstance("flow", { + startState: "View1", + View1: { + state_type: "VIEW", + ref: "view-1", + transitions: {}, + errorTransitions: { + network: "NetworkError", + "*": "GenericError", + }, + }, + NetworkError: { + state_type: "VIEW", + ref: "network-error", + transitions: {}, + }, + GenericError: { + state_type: "VIEW", + ref: "generic-error", + transitions: {}, + }, + }); + + flow.start(); + flow.errorTransition("unknown"); + + expect(flow.currentState?.name).toBe("GenericError"); + }); + + test("falls back to flow-level errorTransitions", () => { + const flow = new FlowInstance("flow", { + startState: "View1", + errorTransitions: { + network: "NetworkError", + "*": "GenericError", + }, + View1: { + state_type: "VIEW", + ref: "view-1", + transitions: {}, + }, + NetworkError: { + state_type: "VIEW", + ref: "network-error", + transitions: {}, + }, + GenericError: { + state_type: "VIEW", + ref: "generic-error", + transitions: {}, + }, + }); + + flow.start(); + flow.errorTransition("network"); + + expect(flow.currentState?.name).toBe("NetworkError"); + }); + + test("node-level errorTransitions takes priority over flow-level", () => { + const flow = new FlowInstance("flow", { + startState: "View1", + errorTransitions: { + network: "FlowNetworkError", + }, + View1: { + state_type: "VIEW", + ref: "view-1", + transitions: {}, + errorTransitions: { + network: "NodeNetworkError", + }, + }, + NodeNetworkError: { + state_type: "VIEW", + ref: "node-network-error", + transitions: {}, + }, + FlowNetworkError: { + state_type: "VIEW", + ref: "flow-network-error", + transitions: {}, + }, + }); + + flow.start(); + flow.errorTransition("network"); + + expect(flow.currentState?.name).toBe("NodeNetworkError"); + }); + + test("warns when no errorTransitions match", () => { + const logger = { + trace: vitest.fn(), + debug: vitest.fn(), + info: vitest.fn(), + warn: vitest.fn(), + error: vitest.fn(), + }; + + const flow = new FlowInstance( + "flow", + { + startState: "View1", + View1: { + state_type: "VIEW", + ref: "view-1", + transitions: {}, + }, + }, + { logger }, + ); + + flow.start(); + flow.errorTransition("network"); + + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining("No errorTransition found"), + ); + expect(flow.currentState?.name).toBe("View1"); + }); + + test("cannot transition from END state", () => { + const logger = { + trace: vitest.fn(), + debug: vitest.fn(), + info: vitest.fn(), + warn: vitest.fn(), + error: vitest.fn(), + }; + + const flow = new FlowInstance( + "flow", + { + startState: "End", + errorTransitions: { + network: "ErrorView", + }, + End: { + state_type: "END", + outcome: "done", + }, + ErrorView: { + state_type: "VIEW", + ref: "error-view", + transitions: {}, + }, + }, + { logger }, + ); + + flow.start(); + flow.errorTransition("network"); + + expect(logger.warn).toHaveBeenCalledWith( + "Cannot error transition from END state", + ); + expect(flow.currentState?.name).toBe("End"); + }); + + test("uses flow-level errorTransitions when no currentState", () => { + const flow = new FlowInstance("flow", { + startState: "View1", + errorTransitions: { + init: "ErrorView", + }, + View1: { + state_type: "VIEW", + ref: "view-1", + transitions: {}, + }, + ErrorView: { + state_type: "VIEW", + ref: "error-view", + transitions: {}, + }, + }); + + // Don't call flow.start() - no currentState + flow.errorTransition("init"); + + // Should navigate to ErrorView via flow-level errorTransitions + expect(flow.currentState?.name).toBe("ErrorView"); + }); +}); + +describe("Flow-level transitions", () => { + test("flowTransition falls back to flow-level transitions", () => { + const flow = new FlowInstance("flow", { + startState: "View1", + transitions: { + error: "ErrorView", + "*": "FallbackView", + }, + View1: { + state_type: "VIEW", + ref: "view-1", + transitions: { + next: "End", + }, + }, + ErrorView: { + state_type: "VIEW", + ref: "error-view", + transitions: { + retry: "View1", + }, + }, + FallbackView: { + state_type: "VIEW", + ref: "fallback-view", + transitions: { + back: "View1", + }, + }, + End: { + state_type: "END", + outcome: "done", + }, + }); + + flow.start(); + expect(flow.currentState?.name).toBe("View1"); + + // flowTransition should use flow-level transitions since "error" is not in View1.transitions + flow.flowTransition("error"); + expect(flow.currentState?.name).toBe("ErrorView"); + }); + + test("flowTransition uses flow-level transitions only", () => { + const flow = new FlowInstance("flow", { + startState: "View1", + transitions: { + error: "FlowErrorView", + }, + View1: { + state_type: "VIEW", + ref: "view-1", + transitions: { + next: "End", + error: "NodeErrorView", + }, + }, + NodeErrorView: { + state_type: "VIEW", + ref: "node-error-view", + transitions: { + retry: "View1", + }, + }, + FlowErrorView: { + state_type: "VIEW", + ref: "flow-error-view", + transitions: { + retry: "View1", + }, + }, + End: { + state_type: "END", + outcome: "done", + }, + }); + + flow.start(); + expect(flow.currentState?.name).toBe("View1"); + + // flowTransition should ONLY use flow-level, not node-level + flow.flowTransition("error"); + expect(flow.currentState?.name).toBe("FlowErrorView"); + }); + + test("regular transition does NOT use flow-level transitions", () => { + const flow = new FlowInstance("flow", { + startState: "View1", + transitions: { + error: "ErrorView", + }, + View1: { + state_type: "VIEW", + ref: "view-1", + transitions: { + next: "End", + }, + }, + ErrorView: { + state_type: "VIEW", + ref: "error-view", + transitions: { + retry: "View1", + }, + }, + End: { + state_type: "END", + outcome: "done", + }, + }); + + flow.start(); + expect(flow.currentState?.name).toBe("View1"); + + // Regular transition should NOT fall back to flow-level + flow.transition("error"); + // Should still be on View1 because transition just warns and returns + expect(flow.currentState?.name).toBe("View1"); + }); +}); diff --git a/core/player/src/controllers/flow/flow.ts b/core/player/src/controllers/flow/flow.ts index e49220e43..9924a9b0a 100644 --- a/core/player/src/controllers/flow/flow.ts +++ b/core/player/src/controllers/flow/flow.ts @@ -150,10 +150,129 @@ export class FlowInstance { return this.flowPromise.promise; } + /** + * Get the flow-level error transitions map + */ + public getFlowErrorTransitions(): Record | undefined { + return this.flow.errorTransitions; + } + + /** + * Helper to lookup a key in a map with wildcard fallback + */ + private lookupInMap( + map: Record | undefined, + key: string, + ): string | undefined { + if (!map) return undefined; + return map[key] || map["*"]; + } + + /** + * Navigate using errorTransitions map. + * Tries node-level first, then falls back to flow-level. + * Bypasses validation hooks and expression resolution. + * @throws Error if errorTransitions references a non-existent state + */ + public errorTransition(errorType: string): void { + // Can't navigate from END state + if (this.currentState?.value.state_type === "END") { + this.log?.warn("Cannot error transition from END state"); + return; + } + + // Try node-level errorTransitions (only if we have a current state) + if (this.currentState) { + const nodeState = this.lookupInMap( + this.currentState.value.errorTransitions, + errorType, + ); + + if (nodeState) { + if (!Object.prototype.hasOwnProperty.call(this.flow, nodeState)) { + this.log?.debug( + `Node-level errorTransition references non-existent state "${nodeState}", trying flow-level fallback`, + ); + // Fall through to try flow-level + } else { + this.log?.debug( + `Error transition (node-level) from ${this.currentState.name} to ${nodeState} using ${errorType}`, + ); + return this.pushHistory(nodeState); + } + } + } + + // Try flow-level errorTransitions + const flowState = this.lookupInMap(this.flow.errorTransitions, errorType); + + if (flowState) { + // Validate state exists before navigating + if (!Object.prototype.hasOwnProperty.call(this.flow, flowState)) { + this.log?.debug( + `Flow-level errorTransition references non-existent state "${flowState}"`, + ); + // No valid transition found, will warn below + } else { + this.log?.debug( + `Error transition (flow-level) to ${flowState} using ${errorType}${this.currentState ? ` from ${this.currentState.name}` : ""}`, + ); + return this.pushHistory(flowState); + } + } + + // No match found + this.log?.warn( + `No errorTransition found for ${errorType} (checked node and flow level)`, + ); + } + + /** + * Transition using flow-level transitions map only. + * This ONLY looks at flow.transitions (not node-level transitions). + */ + public flowTransition(transitionValue: string): void { + // Can't transition while another transition is in progress + if (this.isTransitioning) { + throw new Error( + `Transitioning while ongoing transition is in progress is not supported`, + ); + } + + // Can't transition from END state + if (this.currentState?.value.state_type === "END") { + this.log?.warn( + `Skipping flow transition using ${transitionValue}. Already at END state`, + ); + return; + } + + // Look up in flow-level transitions + if (!this.flow.transitions) { + this.log?.warn("No flow-level transitions defined"); + return; + } + + const nextState = + this.flow.transitions[transitionValue] || this.flow.transitions["*"]; + + if (nextState === undefined) { + this.log?.warn(`No flow-level transition for ${transitionValue} or *`); + return; + } + + this.log?.debug( + `Flow-level transition to ${nextState} using ${transitionValue}${this.currentState ? ` from ${this.currentState.name}` : ""}`, + ); + + return this.pushHistory(nextState); + } + public transition( transitionValue: string, options?: TransitionOptions, ): void { + // Check if we can transition if (this.isTransitioning) { throw new Error( `Transitioning while ongoing transition from ${this.currentState?.name} is in progress is not supported`, @@ -162,9 +281,8 @@ export class FlowInstance { if (this.currentState?.value.state_type === "END") { this.log?.warn( - `Skipping transition using ${transitionValue}. Already at and END state`, + `Skipping transition using ${transitionValue}. Already at END state`, ); - return; } @@ -172,6 +290,9 @@ export class FlowInstance { throw new Error("Cannot transition when there's no current state"); } + const currentState = this.currentState.value; + + // For normal transitions: use hooks if (options?.force) { this.log?.debug(`Forced transition. Skipping validation checks`); } else { @@ -186,7 +307,7 @@ export class FlowInstance { } const state = this.hooks.beforeTransition.call( - this.currentState.value, + currentState as Exclude, transitionValue, ); @@ -232,7 +353,9 @@ export class FlowInstance { const prevState = this.currentState; this.isTransitioning = true; - nextState = this.hooks.resolveTransitionNode.call(nextState); + nextState = this.hooks.resolveTransitionNode.call( + nextState as NavigationFlowState, + ); const newCurrentState = { name: stateName, diff --git a/core/player/src/player.ts b/core/player/src/player.ts index fada62754..73a2776f7 100644 --- a/core/player/src/player.ts +++ b/core/player/src/player.ts @@ -243,7 +243,11 @@ export class Player { this.hooks.validationController.call(validationController); - const errorController = new ErrorController(); + const errorController = new ErrorController({ + logger: this.logger, + flow: flowController, + fail: flowResultDeferred.reject, + }); this.hooks.errorController.call(errorController); @@ -275,7 +279,6 @@ export class Player { errorController.setOptions({ model: dataController, - logger: this.logger, }); // eslint-disable-next-line prefer-const diff --git a/core/types/src/index.ts b/core/types/src/index.ts index 777366da7..7e93af6f7 100644 --- a/core/types/src/index.ts +++ b/core/types/src/index.ts @@ -116,12 +116,25 @@ export interface NavigationFlow { /** An optional expression to run when this Flow ends */ onEnd?: Expression | ExpressionObject; + /** + * An optional flow-level transitions map (fallback when node-level is not defined). + * Used as a fallback when the current state doesn't have a transition defined. + */ + transitions?: NavigationFlowTransition; + + /** + * Optional flow-level error transitions map. + * Maps error types directly to state names (no indirection through transitions map). + */ + errorTransitions?: Record; + [key: string]: | undefined | string | Expression | ExpressionObject - | NavigationFlowState; + | NavigationFlowState + | NavigationFlowTransition; } export type NavigationFlowTransition = Record; @@ -154,6 +167,12 @@ export interface NavigationFlowTransitionableState extends NavigationBaseState { /** A mapping of transition-name to FlowState name */ transitions: NavigationFlowTransition; + + /** + * Optional error transitions map. + * Maps error types directly to state names (no indirection through transitions map). + */ + errorTransitions?: Record; } /** A state representing a view */ From 8bcf7d4f2cb8b0b7acf528e1a418a69f00efb2da Mon Sep 17 00:00:00 2001 From: Chloe Han Date: Fri, 30 Jan 2026 11:36:01 -0800 Subject: [PATCH 8/9] address comments and update doc --- .../error/__tests__/controller.test.ts | 10 +- .../error/__tests__/middleware.test.ts | 28 +-- .../src/controllers/error/controller.ts | 14 +- .../src/controllers/error/middleware.ts | 16 +- .../controllers/flow/__tests__/flow.test.ts | 121 ------------ core/player/src/controllers/flow/flow.ts | 41 ----- core/player/src/data/model.ts | 6 +- .../content/docs/content/error-handling.mdx | 174 +++++++++++++----- 8 files changed, 161 insertions(+), 249 deletions(-) diff --git a/core/player/src/controllers/error/__tests__/controller.test.ts b/core/player/src/controllers/error/__tests__/controller.test.ts index af47991a7..97f4351a7 100644 --- a/core/player/src/controllers/error/__tests__/controller.test.ts +++ b/core/player/src/controllers/error/__tests__/controller.test.ts @@ -95,7 +95,7 @@ describe("ErrorController", () => { ], ], expect.objectContaining({ - authToken: expect.any(Symbol), + writeSymbol: expect.any(Symbol), }), ); }); @@ -155,7 +155,7 @@ describe("ErrorController", () => { expect(mockDataController.delete).toHaveBeenCalledWith( "errorState", expect.objectContaining({ - authToken: expect.any(Symbol), + writeSymbol: expect.any(Symbol), }), ); }); @@ -197,7 +197,7 @@ describe("ErrorController", () => { expect(mockDataController.delete).toHaveBeenCalledWith( "errorState", expect.objectContaining({ - authToken: expect.any(Symbol), + writeSymbol: expect.any(Symbol), }), ); }); @@ -290,13 +290,13 @@ describe("ErrorController", () => { errorController.captureError(new Error("Test error")); expect(mockDataController.set).toHaveBeenCalled(); - // Clear error should delete via middleware with authToken + // Clear error should delete via middleware with writeSymbol vitest.clearAllMocks(); errorController.clearCurrentError(); expect(mockDataController.delete).toHaveBeenCalledWith( "errorState", expect.objectContaining({ - authToken: expect.any(Symbol), + writeSymbol: expect.any(Symbol), }), ); }); diff --git a/core/player/src/controllers/error/__tests__/middleware.test.ts b/core/player/src/controllers/error/__tests__/middleware.test.ts index db6d433e6..22a94deb9 100644 --- a/core/player/src/controllers/error/__tests__/middleware.test.ts +++ b/core/player/src/controllers/error/__tests__/middleware.test.ts @@ -10,7 +10,7 @@ describe("ErrorStateMiddleware", () => { let baseDataModel: DataModelImpl; let mockLogger: Logger; let parser: BindingParser; - let authSymbol: symbol; + let writeSymbol: symbol; beforeEach(() => { mockLogger = { @@ -21,10 +21,10 @@ describe("ErrorStateMiddleware", () => { error: vitest.fn(), }; - authSymbol = Symbol("test-auth"); + writeSymbol = Symbol("test-write"); middleware = new ErrorStateMiddleware({ logger: mockLogger, - authSymbol, + writeSymbol, }); baseDataModel = new LocalModel({ foo: "bar" }); @@ -36,7 +36,7 @@ describe("ErrorStateMiddleware", () => { }); describe("set", () => { - it("should block writes to errorState without authToken", () => { + it("should block writes to errorState without writeSymbol", () => { const binding = parser.parse("errorState"); const updates = middleware.set( [[binding, { message: "test" }]], @@ -85,12 +85,12 @@ describe("ErrorStateMiddleware", () => { expect(updates[0]!.newValue).toBe("newValue"); }); - it("should allow writes when authorized with authToken", () => { + it("should allow writes when authorized with writeSymbol", () => { const binding = parser.parse("errorState"); const updates = middleware.set( [[binding, { message: "test" }]], - { authToken: authSymbol }, + { writeSymbol: writeSymbol }, baseDataModel, ); @@ -100,13 +100,13 @@ describe("ErrorStateMiddleware", () => { expect(updates[0]!.newValue).toEqual({ message: "test" }); }); - it("should block writes with wrong authToken", () => { + it("should block writes with wrong writeSymbol", () => { const binding = parser.parse("errorState"); const wrongSymbol = Symbol("wrong-auth"); middleware.set( [[binding, { message: "test" }]], - { authToken: wrongSymbol }, + { writeSymbol: wrongSymbol }, baseDataModel, ); @@ -156,7 +156,7 @@ describe("ErrorStateMiddleware", () => { }); describe("delete", () => { - it("should block deletes to errorState without authToken", () => { + it("should block deletes to errorState without writeSymbol", () => { const binding = parser.parse("errorState"); // Set value first @@ -171,26 +171,26 @@ describe("ErrorStateMiddleware", () => { ); }); - it("should allow deletes when authorized with authToken", () => { + it("should allow deletes when authorized with writeSymbol", () => { const binding = parser.parse("errorState"); // Set value first baseDataModel.set([[binding, { message: "test" }]]); - middleware.delete(binding, { authToken: authSymbol }, baseDataModel); + middleware.delete(binding, { writeSymbol: writeSymbol }, baseDataModel); expect(baseDataModel.get(binding)).toBeUndefined(); expect(mockLogger.warn).not.toHaveBeenCalled(); }); - it("should block deletes with wrong authToken", () => { + it("should block deletes with wrong writeSymbol", () => { const binding = parser.parse("errorState"); const wrongSymbol = Symbol("wrong-auth"); // Set value first baseDataModel.set([[binding, { message: "test" }]]); - middleware.delete(binding, { authToken: wrongSymbol }, baseDataModel); + middleware.delete(binding, { writeSymbol: wrongSymbol }, baseDataModel); expect(baseDataModel.get(binding)).toEqual({ message: "test" }); expect(mockLogger.warn).toHaveBeenCalledWith( @@ -213,7 +213,7 @@ describe("ErrorStateMiddleware", () => { // Set value first baseDataModel.set([[binding, "test"]]); - middleware.delete(binding, { authToken: authSymbol }, baseDataModel); + middleware.delete(binding, { writeSymbol: writeSymbol }, baseDataModel); expect(baseDataModel.get(binding)).toBeUndefined(); expect(mockLogger.warn).not.toHaveBeenCalled(); diff --git a/core/player/src/controllers/error/controller.ts b/core/player/src/controllers/error/controller.ts index 3247b82a8..06e2ffb3d 100644 --- a/core/player/src/controllers/error/controller.ts +++ b/core/player/src/controllers/error/controller.ts @@ -9,8 +9,8 @@ import { ErrorStateMiddleware } from "./middleware"; * Private symbol used to authorize ErrorController's writes to errorState * Only ErrorController has access to this symbol */ -const ERROR_CONTROLLER_AUTH_SYMBOL: unique symbol = Symbol( - "ERROR_CONTROLLER_AUTH", +const errorControllerWriteSymbol: unique symbol = Symbol( + "errorControllerWrite", ); export interface ErrorControllerHooks { @@ -55,7 +55,7 @@ export class ErrorController { this.middleware = new ErrorStateMiddleware({ logger: options.logger, - authSymbol: ERROR_CONTROLLER_AUTH_SYMBOL, + writeSymbol: errorControllerWriteSymbol, }); } @@ -193,7 +193,7 @@ export class ErrorController { try { const { error, errorType, severity, metadata } = playerError; - // Pass auth token to authorize write through middleware + // Pass write symbol to authorize write through middleware this.options.model.set( [ [ @@ -207,7 +207,7 @@ export class ErrorController { }, ], ], - { authToken: ERROR_CONTROLLER_AUTH_SYMBOL }, + { writeSymbol: errorControllerWriteSymbol }, ); this.options.logger.debug( @@ -230,9 +230,9 @@ export class ErrorController { } try { - // Pass auth token to authorize delete through middleware + // Pass write symbol to authorize delete through middleware this.options.model.delete("errorState", { - authToken: ERROR_CONTROLLER_AUTH_SYMBOL, + writeSymbol: errorControllerWriteSymbol, }); this.options.logger.debug( diff --git a/core/player/src/controllers/error/middleware.ts b/core/player/src/controllers/error/middleware.ts index f41c423db..2eb51d6e0 100644 --- a/core/player/src/controllers/error/middleware.ts +++ b/core/player/src/controllers/error/middleware.ts @@ -10,17 +10,17 @@ import type { Logger } from "../../logger"; /** * Middleware that prevents external writes to errorState - * Only authorized callers (with the auth symbol) can write to this path + * Only authorized callers (with the write symbol) can write to this path */ export class ErrorStateMiddleware implements DataModelMiddleware { name = "error-state-middleware"; private logger?: Logger; - private authSymbol: symbol; + private writeSymbol: symbol; - constructor(options: { logger?: Logger; authSymbol: symbol }) { + constructor(options: { logger?: Logger; writeSymbol: symbol }) { this.logger = options.logger; - this.authSymbol = options.authSymbol; + this.writeSymbol = options.writeSymbol; } public set( @@ -28,8 +28,8 @@ export class ErrorStateMiddleware implements DataModelMiddleware { options?: DataModelOptions, next?: DataModelImpl, ): Updates { - // Check if this write is authorized by comparing the auth tokens - if (options?.authToken === this.authSymbol) { + // Check if this write is authorized by comparing the write symbols + if (options?.writeSymbol === this.writeSymbol) { return next?.set(transaction, options) ?? []; } @@ -78,8 +78,8 @@ export class ErrorStateMiddleware implements DataModelMiddleware { options?: DataModelOptions, next?: DataModelImpl, ): void { - // Check if this delete is authorized by comparing the auth tokens - if (options?.authToken === this.authSymbol) { + // Check if this delete is authorized by comparing the write symbols + if (options?.writeSymbol === this.writeSymbol) { next?.delete(binding, options); return; } diff --git a/core/player/src/controllers/flow/__tests__/flow.test.ts b/core/player/src/controllers/flow/__tests__/flow.test.ts index 5afee8fdd..79268fb75 100644 --- a/core/player/src/controllers/flow/__tests__/flow.test.ts +++ b/core/player/src/controllers/flow/__tests__/flow.test.ts @@ -603,124 +603,3 @@ describe("errorTransition", () => { expect(flow.currentState?.name).toBe("ErrorView"); }); }); - -describe("Flow-level transitions", () => { - test("flowTransition falls back to flow-level transitions", () => { - const flow = new FlowInstance("flow", { - startState: "View1", - transitions: { - error: "ErrorView", - "*": "FallbackView", - }, - View1: { - state_type: "VIEW", - ref: "view-1", - transitions: { - next: "End", - }, - }, - ErrorView: { - state_type: "VIEW", - ref: "error-view", - transitions: { - retry: "View1", - }, - }, - FallbackView: { - state_type: "VIEW", - ref: "fallback-view", - transitions: { - back: "View1", - }, - }, - End: { - state_type: "END", - outcome: "done", - }, - }); - - flow.start(); - expect(flow.currentState?.name).toBe("View1"); - - // flowTransition should use flow-level transitions since "error" is not in View1.transitions - flow.flowTransition("error"); - expect(flow.currentState?.name).toBe("ErrorView"); - }); - - test("flowTransition uses flow-level transitions only", () => { - const flow = new FlowInstance("flow", { - startState: "View1", - transitions: { - error: "FlowErrorView", - }, - View1: { - state_type: "VIEW", - ref: "view-1", - transitions: { - next: "End", - error: "NodeErrorView", - }, - }, - NodeErrorView: { - state_type: "VIEW", - ref: "node-error-view", - transitions: { - retry: "View1", - }, - }, - FlowErrorView: { - state_type: "VIEW", - ref: "flow-error-view", - transitions: { - retry: "View1", - }, - }, - End: { - state_type: "END", - outcome: "done", - }, - }); - - flow.start(); - expect(flow.currentState?.name).toBe("View1"); - - // flowTransition should ONLY use flow-level, not node-level - flow.flowTransition("error"); - expect(flow.currentState?.name).toBe("FlowErrorView"); - }); - - test("regular transition does NOT use flow-level transitions", () => { - const flow = new FlowInstance("flow", { - startState: "View1", - transitions: { - error: "ErrorView", - }, - View1: { - state_type: "VIEW", - ref: "view-1", - transitions: { - next: "End", - }, - }, - ErrorView: { - state_type: "VIEW", - ref: "error-view", - transitions: { - retry: "View1", - }, - }, - End: { - state_type: "END", - outcome: "done", - }, - }); - - flow.start(); - expect(flow.currentState?.name).toBe("View1"); - - // Regular transition should NOT fall back to flow-level - flow.transition("error"); - // Should still be on View1 because transition just warns and returns - expect(flow.currentState?.name).toBe("View1"); - }); -}); diff --git a/core/player/src/controllers/flow/flow.ts b/core/player/src/controllers/flow/flow.ts index 9924a9b0a..26ab827ba 100644 --- a/core/player/src/controllers/flow/flow.ts +++ b/core/player/src/controllers/flow/flow.ts @@ -227,47 +227,6 @@ export class FlowInstance { ); } - /** - * Transition using flow-level transitions map only. - * This ONLY looks at flow.transitions (not node-level transitions). - */ - public flowTransition(transitionValue: string): void { - // Can't transition while another transition is in progress - if (this.isTransitioning) { - throw new Error( - `Transitioning while ongoing transition is in progress is not supported`, - ); - } - - // Can't transition from END state - if (this.currentState?.value.state_type === "END") { - this.log?.warn( - `Skipping flow transition using ${transitionValue}. Already at END state`, - ); - return; - } - - // Look up in flow-level transitions - if (!this.flow.transitions) { - this.log?.warn("No flow-level transitions defined"); - return; - } - - const nextState = - this.flow.transitions[transitionValue] || this.flow.transitions["*"]; - - if (nextState === undefined) { - this.log?.warn(`No flow-level transition for ${transitionValue} or *`); - return; - } - - this.log?.debug( - `Flow-level transition to ${nextState} using ${transitionValue}${this.currentState ? ` from ${this.currentState.name}` : ""}`, - ); - - return this.pushHistory(nextState); - } - public transition( transitionValue: string, options?: TransitionOptions, diff --git a/core/player/src/data/model.ts b/core/player/src/data/model.ts index be527d1ee..685762a2e 100644 --- a/core/player/src/data/model.ts +++ b/core/player/src/data/model.ts @@ -47,10 +47,10 @@ export interface DataModelOptions { silent?: boolean; /** - * Authorization token for internal middleware operations - * Middleware can use this to verify the caller has permission for protected operations + * Write authorization symbol for internal middleware operations + * Middleware can use this to verify the caller has permission for write operations */ - authToken?: symbol; + writeSymbol?: symbol; /** Other context associated with this request */ context?: { diff --git a/docs/site/src/content/docs/content/error-handling.mdx b/docs/site/src/content/docs/content/error-handling.mdx index 39f01bcd9..46a21fd2c 100644 --- a/docs/site/src/content/docs/content/error-handling.mdx +++ b/docs/site/src/content/docs/content/error-handling.mdx @@ -172,39 +172,35 @@ errorController.hooks.onError.tap(name: "logger") { errorInfo in ## Error Navigation Pattern -Errors can trigger automatic navigation using the `errorState` property at node or flow level. +Errors can trigger automatic navigation using the `errorTransitions` map at node or flow level. This provides a dedicated error routing mechanism separate from regular transitions. -### ErrorState Property Formats +### errorTransitions Map -The `errorState` property supports two formats: +The `errorTransitions` property maps error types directly to state names: -1. **String** - Navigate to a single error state for all error types - ```json - "errorState": "ERROR_VIEW" - ``` - -2. **Object (Mapping)** - Navigate to different states based on error type - ```json - "errorState": { - "binding": "BINDING_ERROR_VIEW", - "validation": "VALIDATION_ERROR_VIEW", - "*": "GENERIC_ERROR_VIEW" - } - ``` +```json +{ + "errorTransitions": { + "binding": "BINDING_ERROR_VIEW", + "validation": "VALIDATION_ERROR_VIEW", + "*": "GENERIC_ERROR_VIEW" + } +} +``` -Both formats can be used at node-level or flow-level. +The `"*"` wildcard matches any error type not explicitly defined. ### Navigation Fallback Pattern -The navigation follows a hierarchical fallback pattern: +When an error is captured, the ErrorController uses `errorTransition()` to navigate following this hierarchical fallback: -1. **Node-level `errorState`** - If defined, transition to the specified state -2. **Flow-level `errorState`** - If node-level not defined or no match found, use flow-level error state -3. **Flow rejection** - If neither defined, reject/fail the flow +1. **Node-level `errorTransitions`** - If defined on the current state, navigate to the mapped state for the error type +2. **Flow-level `errorTransitions`** - If node-level not found or current state doesn't have a match, use flow-level mapping +3. **No Navigation** - If neither level has a match, log a warning and stay on the current state (or reject flow if critical) -### String Format (Single Error State) +### Node-Level Error Transitions -Use a string to navigate to the same error state for all error types: +Define `errorTransitions` on individual states to handle errors specific to that state: ```json { @@ -215,17 +211,29 @@ Use a string to navigate to the same error state for all error types: "VIEW_1": { "state_type": "VIEW", "ref": "main-view", - "errorState": "ERROR_VIEW", + "errorTransitions": { + "binding": "BINDING_ERROR_VIEW", + "validation": "VALIDATION_ERROR_VIEW", + "*": "GENERIC_ERROR_VIEW" + }, "transitions": { "*": "END_Done" } }, - "ERROR_VIEW": { + "BINDING_ERROR_VIEW": { "state_type": "VIEW", - "ref": "error-view", - "transitions": { - "*": "END_Error" - } + "ref": "binding-error-view", + "transitions": { "*": "END_Error" } + }, + "VALIDATION_ERROR_VIEW": { + "state_type": "VIEW", + "ref": "validation-error-view", + "transitions": { "*": "END_Error" } + }, + "GENERIC_ERROR_VIEW": { + "state_type": "VIEW", + "ref": "generic-error-view", + "transitions": { "*": "END_Error" } }, "END_Done": { "state_type": "END", @@ -240,11 +248,11 @@ Use a string to navigate to the same error state for all error types: } ``` -When an error is captured while on `VIEW_1`, Player will automatically call `transition("ERROR_VIEW")` for any error type. +When a `binding` error is captured on `VIEW_1`, Player automatically navigates to `BINDING_ERROR_VIEW`. -### Object Format (Error Type Mapping) +### Flow-Level Error Transitions -Use an object to navigate to different states based on the error type: +Define `errorTransitions` at the flow level as a fallback for states without their own error handling: ```json { @@ -252,14 +260,21 @@ Use an object to navigate to different states based on the error type: "BEGIN": "FLOW_1", "FLOW_1": { "startState": "VIEW_1", + "errorTransitions": { + "binding": "BINDING_ERROR_VIEW", + "validation": "VALIDATION_ERROR_VIEW", + "*": "GENERIC_ERROR_VIEW" + }, "VIEW_1": { "state_type": "VIEW", "ref": "main-view", - "errorState": { - "binding": "BINDING_ERROR_VIEW", - "validation": "VALIDATION_ERROR_VIEW", - "*": "GENERIC_ERROR_VIEW" - }, + "transitions": { + "*": "VIEW_2" + } + }, + "VIEW_2": { + "state_type": "VIEW", + "ref": "second-view", "transitions": { "*": "END_Done" } @@ -267,28 +282,58 @@ Use an object to navigate to different states based on the error type: "BINDING_ERROR_VIEW": { "state_type": "VIEW", "ref": "binding-error-view", - "transitions": { "*": "END_Error" } + "transitions": { + "*": "END_Error" + } }, "VALIDATION_ERROR_VIEW": { "state_type": "VIEW", "ref": "validation-error-view", - "transitions": { "*": "END_Error" } + "transitions": { + "*": "END_Error" + } }, "GENERIC_ERROR_VIEW": { "state_type": "VIEW", "ref": "generic-error-view", - "transitions": { "*": "END_Error" } + "transitions": { + "*": "END_Error" + } + }, + "END_Done": { + "state_type": "END", + "outcome": "done" + }, + "END_Error": { + "state_type": "END", + "outcome": "error" } } } } ``` -The `"*"` wildcard matches any error type not explicitly defined. If an error type doesn't match and no wildcard is defined, it falls through to the flow-level `errorState`. +Any error captured on `VIEW_1` or `VIEW_2` will use the flow-level `errorTransitions` since they don't define their own. + +### Wildcard Support + +The `"*"` wildcard matches any error type not explicitly mapped: + +```json +{ + "errorTransitions": { + "network": "NETWORK_ERROR_VIEW", + "validation": "VALIDATION_ERROR_VIEW", + "*": "GENERIC_ERROR_VIEW" + } +} +``` + +A `binding` error would navigate to `GENERIC_ERROR_VIEW` via the wildcard. -### Flow-Level Error Navigation +### Combining Node and Flow Level -Define `errorState` at the flow level as a fallback for states without their own `errorState`: +Node-level takes precedence, with flow-level as fallback: ```json { @@ -296,10 +341,15 @@ Define `errorState` at the flow level as a fallback for states without their own "BEGIN": "FLOW_1", "FLOW_1": { "startState": "VIEW_1", - "errorState": "ERROR_VIEW", + "errorTransitions": { + "*": "GENERIC_ERROR_VIEW" + }, "VIEW_1": { "state_type": "VIEW", "ref": "main-view", + "errorTransitions": { + "validation": "CUSTOM_VALIDATION_ERROR" + }, "transitions": { "*": "VIEW_2" } @@ -311,16 +361,40 @@ Define `errorState` at the flow level as a fallback for states without their own "*": "END_Done" } }, - "ERROR_VIEW": { + "CUSTOM_VALIDATION_ERROR": { "state_type": "VIEW", - "ref": "error-view", - "transitions": { - "*": "END_Error" - } + "ref": "custom-validation-error", + "transitions": { "*": "END_Error" } + }, + "GENERIC_ERROR_VIEW": { + "state_type": "VIEW", + "ref": "generic-error-view", + "transitions": { "*": "END_Error" } + }, + "END_Done": { + "state_type": "END", + "outcome": "done" + }, + "END_Error": { + "state_type": "END", + "outcome": "error" } } } } ``` -Flow-level `errorState` can also use the object format for error type mapping. +**Behavior:** +- On `VIEW_1`: `validation` error → `CUSTOM_VALIDATION_ERROR` (node-level) +- On `VIEW_1`: `binding` error → `GENERIC_ERROR_VIEW` (flow-level fallback) +- On `VIEW_2`: any error → `GENERIC_ERROR_VIEW` (flow-level fallback) + +### Key Differences from Regular Transitions + +`errorTransitions` differs from regular `transitions`: + +1. **Direct Navigation**: Maps error types directly to state names (no intermediate transition values) +2. **Bypasses Hooks**: Skips `skipTransition` and `beforeTransition` hooks for immediate error handling +3. **No Expression Resolution**: State names are used as-is without expression evaluation +4. **Two-Level Fallback**: Supports both node-level and flow-level with automatic fallback +5. **Protected Writes**: ErrorController writes to `errorState` in the data model using protected middleware From 07bcf2a252599e28ec2d08c6415645c3b24403b1 Mon Sep 17 00:00:00 2001 From: Chloeeeeeee Date: Mon, 9 Feb 2026 11:33:04 -0500 Subject: [PATCH 9/9] Error controller mobile exposure (#783) * Error controller expose to iOS/Android --- ios/core/Sources/Player/HeadlessPlayer.swift | 3 + .../Sources/Types/Core/CompletedState.swift | 4 + .../Sources/Types/Core/ErrorController.swift | 199 +++++++++ .../Types/Hooks/ErrorControllerHooks.swift | 24 ++ ios/core/Sources/Types/Hooks/Hook.swift | 33 ++ ios/core/Tests/ErrorControllerTests.swift | 395 ++++++++++++++++++ ios/core/Tests/Types/HooksTests.swift | 170 ++++++++ ios/swiftui/Sources/SwiftUIPlayer.swift | 4 + .../utilities/HeadlessPlayerImpl.swift | 3 + .../Sources/utilities/TestPlayer.swift | 3 + .../playerui/core/error/ErrorController.kt | 163 ++++++++ .../com/intuit/playerui/core/player/Player.kt | 6 + .../core/player/state/PlayerFlowState.kt | 4 + .../core/error/ErrorControllerTest.kt | 250 +++++++++++ 14 files changed, 1261 insertions(+) create mode 100644 ios/core/Sources/Types/Core/ErrorController.swift create mode 100644 ios/core/Sources/Types/Hooks/ErrorControllerHooks.swift create mode 100644 ios/core/Tests/ErrorControllerTests.swift create mode 100644 ios/core/Tests/Types/HooksTests.swift create mode 100644 jvm/core/src/main/kotlin/com/intuit/playerui/core/error/ErrorController.kt create mode 100644 jvm/core/src/test/kotlin/com/intuit/playerui/core/error/ErrorControllerTest.kt diff --git a/ios/core/Sources/Player/HeadlessPlayer.swift b/ios/core/Sources/Player/HeadlessPlayer.swift index 313b80e4b..c3c3db8ff 100644 --- a/ios/core/Sources/Player/HeadlessPlayer.swift +++ b/ios/core/Sources/Player/HeadlessPlayer.swift @@ -56,6 +56,9 @@ public protocol CoreHooks { /// Fired when the DataController changes var dataController: Hook { get } + /// Fired when the ErrorController changes + var errorController: Hook { get } + /// Fired when the state changes var state: Hook { get } diff --git a/ios/core/Sources/Types/Core/CompletedState.swift b/ios/core/Sources/Types/Core/CompletedState.swift index b1b628585..8373a95a3 100644 --- a/ios/core/Sources/Types/Core/CompletedState.swift +++ b/ios/core/Sources/Types/Core/CompletedState.swift @@ -26,6 +26,9 @@ public class PlayerControllers { /// The ExpressionEvaluator for the current flow public let expression: ExpressionEvaluator + /// The ErrorController for the current flow + public let error: ErrorController + public init?(from value: JSValue?) { guard let controllers = value else { return nil } rawValue = controllers @@ -33,6 +36,7 @@ public class PlayerControllers { flow = FlowController.createInstance(value: rawValue.objectForKeyedSubscript("flow")) view = ViewController.createInstance(value: rawValue.objectForKeyedSubscript("view")) expression = ExpressionEvaluator.createInstance(value: rawValue.objectForKeyedSubscript("expression")) + error = ErrorController.createInstance(value: rawValue.objectForKeyedSubscript("error")) } } diff --git a/ios/core/Sources/Types/Core/ErrorController.swift b/ios/core/Sources/Types/Core/ErrorController.swift new file mode 100644 index 000000000..78a7ab5b9 --- /dev/null +++ b/ios/core/Sources/Types/Core/ErrorController.swift @@ -0,0 +1,199 @@ +// +// ErrorController.swift +// PlayerUI +// +// Created by Player Team +// + +import Foundation +import JavaScriptCore + +/** + Severity levels for errors + */ +public enum ErrorSeverity: String { + /// Cannot continue, flow must end + case fatal + /// Standard error, may allow recovery + case error + /// Non-blocking, logged for telemetry + case warning +} + +/** + Known error types for Player + */ +public struct ErrorTypes { + public static let expression = "expression" + public static let binding = "binding" + public static let view = "view" + public static let asset = "asset" + public static let navigation = "navigation" + public static let validation = "validation" + public static let data = "data" + public static let schema = "schema" + public static let network = "network" + public static let plugin = "plugin" +} + +/** + Represents a Player error with metadata + */ +public class PlayerErrorInfo: CreatedFromJSValue { + /// Typealias for associated type + public typealias T = PlayerErrorInfo + + /// The JSValue that backs this wrapper + private let value: JSValue + + /// The error message + public var message: String { + value.objectForKeyedSubscript("error")?.objectForKeyedSubscript("message")?.toString() ?? "" + } + + /// The error name + public var name: String { + value.objectForKeyedSubscript("error")?.objectForKeyedSubscript("name")?.toString() ?? "" + } + + /// Error category + public var errorType: String { + value.objectForKeyedSubscript("errorType")?.toString() ?? "" + } + + /// Impact level + public var severity: ErrorSeverity? { + guard let severityString = value.objectForKeyedSubscript("severity")?.toString() else { + return nil + } + return ErrorSeverity(rawValue: severityString) + } + + /// Additional metadata + public var metadata: [String: Any]? { + guard let metadataValue = value.objectForKeyedSubscript("metadata"), + !metadataValue.isUndefined, + !metadataValue.isNull else { + return nil + } + return metadataValue.toObject() as? [String: Any] + } + + /** + Creates an instance from a JSValue, used for generic construction + - parameters: + - value: The JSValue to construct from + */ + public static func createInstance(value: JSValue) -> PlayerErrorInfo { + return PlayerErrorInfo(value) + } + + /** + Construct a PlayerErrorInfo from a JSValue + - parameters: + - value: The JSValue that is the error object + */ + public init(_ value: JSValue) { + self.value = value + } +} + +/** + A wrapper around the JS ErrorController in the core player + */ +public class ErrorController: CreatedFromJSValue { + /// Typealias for associated type + public typealias T = ErrorController + + /** + Creates an instance from a JSValue, used for generic construction + - parameters: + - value: The JSValue to construct from + */ + public static func createInstance(value: JSValue) -> ErrorController { ErrorController(value) } + + /// The JSValue that backs this wrapper + private let value: JSValue + + /// The hooks that can be tapped into + public let hooks: ErrorControllerHooks + + /** + Construct an ErrorController from a JSValue + - parameters: + - value: The JSValue that is the ErrorController + */ + public init(_ value: JSValue) { + self.value = value + hooks = ErrorControllerHooks( + onError: BailHook(baseValue: value, name: "onError") + ) + } + + /** + Capture an error with metadata + - parameters: + - error: The native Error object + - errorType: Error category (use ErrorTypes constants) + - severity: Impact level + - metadata: Additional metadata dictionary + - returns: The captured error as a JSValue + */ + @discardableResult + public func captureError( + error: Error, + errorType: String, + severity: ErrorSeverity? = nil, + metadata: [String: Any]? = nil + ) -> JSValue? { + var args: [Any] = [ + [ + "message": error.localizedDescription, + "name": String(describing: type(of: error)) + ] as [String: Any], + errorType + ] + + if let severity = severity { + args.append(severity.rawValue) + } else { + args.append(JSValue(undefinedIn: value.context) as Any) + } + + if let metadata = metadata { + args.append(metadata) + } + + return value.invokeMethod("captureError", withArguments: args) + } + + /** + Get the most recent error + - returns: The current error as a JSValue if one exists + */ + public func getCurrentError() -> JSValue? { + return value.invokeMethod("getCurrentError", withArguments: []) + } + + /** + Get the complete error history + - returns: JSValue representing the array of errors + */ + public func getErrors() -> JSValue? { + return value.invokeMethod("getErrors", withArguments: []) + } + + /** + Clear all errors (history + current + data model) + */ + public func clearErrors() { + value.invokeMethod("clearErrors", withArguments: []) + } + + /** + Clear only current error and remove from data model, preserve history + */ + public func clearCurrentError() { + value.invokeMethod("clearCurrentError", withArguments: []) + } +} diff --git a/ios/core/Sources/Types/Hooks/ErrorControllerHooks.swift b/ios/core/Sources/Types/Hooks/ErrorControllerHooks.swift new file mode 100644 index 000000000..484a696a5 --- /dev/null +++ b/ios/core/Sources/Types/Hooks/ErrorControllerHooks.swift @@ -0,0 +1,24 @@ +// +// ErrorControllerHooks.swift +// PlayerUI +// +// Created by Player Team +// + +import Foundation +import JavaScriptCore + +/** + Hooks that can be tapped into for the ErrorController + This lets users tap into error events in the JS environment + */ +public struct ErrorControllerHooks { + /** + Fired when any error is captured + - The callback receives a PlayerErrorInfo object + - Return true from the callback to bail and prevent error state navigation + - Return nil/false to allow automatic navigation to continue + */ + public var onError: BailHook +} + diff --git a/ios/core/Sources/Types/Hooks/Hook.swift b/ios/core/Sources/Types/Hooks/Hook.swift index 07e3e57b7..e924e46e6 100644 --- a/ios/core/Sources/Types/Hooks/Hook.swift +++ b/ios/core/Sources/Types/Hooks/Hook.swift @@ -81,6 +81,39 @@ public class Hook: BaseJSHook where T: CreatedFromJSValue { } } +/** + This class represents a SyncBailHook in the JS runtime that can be tapped into + to receive JS events and optionally return a value to bail/stop further execution + */ +public class BailHook: BaseJSHook where T: CreatedFromJSValue { + /** + Attach a closure to the hook, so when the hook is fired in the JS runtime + we receive the event in the native runtime and can return a value to bail + + - parameters: + - hook: A function to run when the JS hook is fired, return Bool? to bail (return nil to continue) + */ + public func tap(_ hook: @escaping (T) -> Bool?) { + let tapMethod: @convention(block) (JSValue?) -> JSValue? = { [weak self] value in + guard + let self = self, + let val = value, + let hookValue = T.createInstance(value: val) as? T + else { return nil } + + let result = hook(hookValue) + if let boolResult = result { + // Return JS true/false to bail + return JSValue(bool: boolResult, in: self.context) + } + // Return JS undefined to continue + return JSValue(undefinedIn: self.context) + } + + self.hook.invokeMethod("tap", withArguments: [name, JSValue(object: tapMethod, in: context) as Any]) + } +} + /** This class represents an object in the JS runtime that can be tapped into to receive JS events that has 2 parameters diff --git a/ios/core/Tests/ErrorControllerTests.swift b/ios/core/Tests/ErrorControllerTests.swift new file mode 100644 index 000000000..2d9079694 --- /dev/null +++ b/ios/core/Tests/ErrorControllerTests.swift @@ -0,0 +1,395 @@ +// +// ErrorControllerTests.swift +// PlayerUI_Tests +// +// Created by Player Team +// + +import Foundation +import XCTest +import JavaScriptCore +@testable import PlayerUI +@testable import PlayerUIInternalTestUtilities +@testable import PlayerUITestUtilitiesCore + +class ErrorControllerTests: XCTestCase { + var player: HeadlessPlayerImpl! + + override func setUp() { + super.setUp() + player = HeadlessPlayerImpl(plugins: []) + } + + override func tearDown() { + player = nil + super.tearDown() + } + + // MARK: - Hook Tests + + func testErrorControllerHookIsCalled() { + let errorHookCalled = expectation(description: "Error hook called") + + player.hooks?.errorController.tap { errorController in + XCTAssertNotNil(errorController) + XCTAssertNotNil(errorController.hooks.onError) + errorHookCalled.fulfill() + } + + player.start(flow: FlowData.COUNTER) { _ in } + + wait(for: [errorHookCalled], timeout: 2) + } + + func testOnErrorHookIsTapped() { + let onErrorCalled = expectation(description: "onError hook called") + + player.hooks?.errorController.tap { errorController in + errorController.hooks.onError.tap { errorInfo in + XCTAssertNotNil(errorInfo) + XCTAssertEqual(errorInfo.errorType, ErrorTypes.validation) + XCTAssertEqual(errorInfo.severity, .error) + XCTAssertFalse(errorInfo.message.isEmpty) + onErrorCalled.fulfill() + return nil + } + + // Capture a test error + errorController.captureError( + error: NSError(domain: "test", code: 123, userInfo: [NSLocalizedDescriptionKey: "Test error"]), + errorType: ErrorTypes.validation, + severity: .error, + metadata: ["testKey": "testValue"] + ) + } + + player.start(flow: FlowData.COUNTER) { _ in } + + wait(for: [onErrorCalled], timeout: 2) + } + + // MARK: - Error Capture Tests + + func testCaptureErrorWithAllParameters() { + player.start(flow: FlowData.COUNTER) { _ in } + + guard let state = player.state as? InProgressState else { + return XCTFail("Player not in progress") + } + + let errorController = state.controllers?.error + XCTAssertNotNil(errorController) + + let testError = NSError( + domain: "com.test", + code: 404, + userInfo: [NSLocalizedDescriptionKey: "Not found"] + ) + + let capturedErrorValue = errorController?.captureError( + error: testError, + errorType: ErrorTypes.network, + severity: .error, + metadata: ["url": "https://example.com", "statusCode": 404] + ) + + XCTAssertNotNil(capturedErrorValue) + + // Convert JSValue to PlayerErrorInfo + if let jsValue = capturedErrorValue { + let capturedError = PlayerErrorInfo(jsValue) + XCTAssertEqual(capturedError.message, "Not found") + XCTAssertEqual(capturedError.errorType, ErrorTypes.network) + XCTAssertEqual(capturedError.severity, .error) + XCTAssertNotNil(capturedError.metadata) + } + } + + func testCaptureErrorWithMinimalParameters() { + player.start(flow: FlowData.COUNTER) { _ in } + + guard let state = player.state as? InProgressState else { + return XCTFail("Player not in progress") + } + + let errorController = state.controllers?.error + XCTAssertNotNil(errorController) + + let testError = NSError( + domain: "com.test", + code: 500, + userInfo: [NSLocalizedDescriptionKey: "Internal error"] + ) + + let capturedErrorValue = errorController?.captureError( + error: testError, + errorType: ErrorTypes.plugin + ) + + XCTAssertNotNil(capturedErrorValue) + + // Convert JSValue to PlayerErrorInfo + if let jsValue = capturedErrorValue { + let capturedError = PlayerErrorInfo(jsValue) + XCTAssertEqual(capturedError.message, "Internal error") + XCTAssertEqual(capturedError.errorType, ErrorTypes.plugin) + XCTAssertNil(capturedError.severity) + XCTAssertNil(capturedError.metadata) + } + } + + func testCaptureMultipleErrorsAndCurrentErrorUpdates() { + player.start(flow: FlowData.COUNTER) { _ in } + + guard let state = player.state as? InProgressState else { + return XCTFail("Player not in progress") + } + + let errorController = state.controllers?.error + XCTAssertNotNil(errorController) + + // Capture first error + errorController?.captureError( + error: NSError(domain: "test", code: 1, userInfo: [NSLocalizedDescriptionKey: "First error"]), + errorType: ErrorTypes.validation, + severity: .warning + ) + + // Verify current error is the first one + if let firstErrorValue = errorController?.getCurrentError(), !firstErrorValue.isUndefined { + XCTAssertEqual(PlayerErrorInfo(firstErrorValue).message, "First error") + } + + // Capture second error + errorController?.captureError( + error: NSError(domain: "test", code: 2, userInfo: [NSLocalizedDescriptionKey: "Second error"]), + errorType: ErrorTypes.binding, + severity: .error + ) + + // Current error should be updated to the second one + if let secondErrorValue = errorController?.getCurrentError(), !secondErrorValue.isUndefined { + XCTAssertEqual(PlayerErrorInfo(secondErrorValue).message, "Second error") + } + + // Capture third error + errorController?.captureError( + error: NSError(domain: "test", code: 3, userInfo: [NSLocalizedDescriptionKey: "Third error"]), + errorType: ErrorTypes.view, + severity: .fatal + ) + + // Current error should be updated to the third one + if let thirdErrorValue = errorController?.getCurrentError(), !thirdErrorValue.isUndefined { + XCTAssertEqual(PlayerErrorInfo(thirdErrorValue).message, "Third error") + } + + // Get all errors and verify history + guard let errorsValue = errorController?.getErrors(), + let errorsArray = errorsValue.toArray() else { + return XCTFail("Could not get errors array") + } + + // Verify we have 3 errors + XCTAssertEqual(errorsArray.count, 3, "Expected 3 errors in history") + + // Verify the errors by accessing them as JSValues directly from the errorsValue + let firstError = PlayerErrorInfo(errorsValue.atIndex(0)) + let secondError = PlayerErrorInfo(errorsValue.atIndex(1)) + let thirdError = PlayerErrorInfo(errorsValue.atIndex(2)) + + XCTAssertEqual(firstError.message, "First error") + XCTAssertEqual(secondError.message, "Second error") + XCTAssertEqual(thirdError.message, "Third error") + } + + // MARK: - Get Current Error Tests + + func testGetCurrentError() { + player.start(flow: FlowData.COUNTER) { _ in } + + guard let state = player.state as? InProgressState else { + return XCTFail("Player not in progress") + } + + let errorController = state.controllers?.error + XCTAssertNotNil(errorController) + + // Initially no current error + let initialError = errorController?.getCurrentError() + XCTAssertTrue(initialError?.isUndefined ?? false) + + // Capture an error + let testError = NSError( + domain: "test", + code: 100, + userInfo: [NSLocalizedDescriptionKey: "Current error"] + ) + + errorController?.captureError( + error: testError, + errorType: ErrorTypes.data, + severity: .error + ) + + // Now should have a current error + guard let currentErrorValue = errorController?.getCurrentError(), + !currentErrorValue.isUndefined else { + return XCTFail("Current error should exist") + } + + let currentError = PlayerErrorInfo(currentErrorValue) + XCTAssertEqual(currentError.message, "Current error") + XCTAssertEqual(currentError.errorType, ErrorTypes.data) + } + + // MARK: - Clear Errors Tests + + func testClearAllErrors() { + player.start(flow: FlowData.COUNTER) { _ in } + + guard let state = player.state as? InProgressState else { + return XCTFail("Player not in progress") + } + + let errorController = state.controllers?.error + + // Capture multiple errors + errorController?.captureError( + error: NSError(domain: "test", code: 1, userInfo: [NSLocalizedDescriptionKey: "Error 1"]), + errorType: ErrorTypes.validation + ) + errorController?.captureError( + error: NSError(domain: "test", code: 2, userInfo: [NSLocalizedDescriptionKey: "Error 2"]), + errorType: ErrorTypes.binding + ) + + let errorsBeforeCount = errorController?.getErrors()?.toArray()?.count ?? 0 + XCTAssertEqual(errorsBeforeCount, 2) + + let currentErrorBefore = errorController?.getCurrentError() + XCTAssertFalse(currentErrorBefore?.isUndefined ?? true) + + // Clear all errors + errorController?.clearErrors() + + let errorsAfterCount = errorController?.getErrors()?.toArray()?.count ?? 0 + XCTAssertEqual(errorsAfterCount, 0) + + let currentErrorAfter = errorController?.getCurrentError() + XCTAssertTrue(currentErrorAfter?.isUndefined ?? false) + } + + func testClearCurrentError() { + player.start(flow: FlowData.COUNTER) { _ in } + + guard let state = player.state as? InProgressState else { + return XCTFail("Player not in progress") + } + + let errorController = state.controllers?.error + + // Capture multiple errors + errorController?.captureError( + error: NSError(domain: "test", code: 1, userInfo: [NSLocalizedDescriptionKey: "Error 1"]), + errorType: ErrorTypes.validation + ) + errorController?.captureError( + error: NSError(domain: "test", code: 2, userInfo: [NSLocalizedDescriptionKey: "Error 2"]), + errorType: ErrorTypes.binding + ) + + let errorsBeforeCount = errorController?.getErrors()?.toArray()?.count ?? 0 + XCTAssertEqual(errorsBeforeCount, 2) + + let currentErrorBefore = errorController?.getCurrentError() + XCTAssertFalse(currentErrorBefore?.isUndefined ?? true) + + // Clear only current error + errorController?.clearCurrentError() + + // History should be preserved + let errorsAfterCount = errorController?.getErrors()?.toArray()?.count ?? 0 + XCTAssertEqual(errorsAfterCount, 2) + + // Current error should be cleared + let currentErrorAfter = errorController?.getCurrentError() + XCTAssertTrue(currentErrorAfter?.isUndefined ?? false) + } + + // MARK: - PlayerControllers Integration Test + + func testPlayerControllersIncludesErrorController() { + player.start(flow: FlowData.COUNTER) { _ in } + + guard let state = player.state as? InProgressState else { + return XCTFail("Player not in progress") + } + + let controllers = state.controllers + XCTAssertNotNil(controllers) + XCTAssertNotNil(controllers?.data) + XCTAssertNotNil(controllers?.flow) + XCTAssertNotNil(controllers?.view) + XCTAssertNotNil(controllers?.expression) + XCTAssertNotNil(controllers?.error) + } + + // MARK: - Error Metadata Tests + + func testErrorMetadataCapture() { + player.start(flow: FlowData.COUNTER) { _ in } + + guard let state = player.state as? InProgressState else { + return XCTFail("Player not in progress") + } + + let errorController = state.controllers?.error + + let metadata: [String: Any] = [ + "binding": "data.user.name", + "attemptedValue": "invalid", + "validationRule": "minLength", + "component": "TextInput", + "timestamp": Date().timeIntervalSince1970 + ] + + let capturedErrorValue = errorController?.captureError( + error: NSError(domain: "test", code: 1, userInfo: [NSLocalizedDescriptionKey: "Validation failed"]), + errorType: ErrorTypes.validation, + severity: .error, + metadata: metadata + ) + + guard let jsValue = capturedErrorValue else { + return XCTFail("Error should be captured") + } + + let capturedError = PlayerErrorInfo(jsValue) + XCTAssertNotNil(capturedError.metadata) + XCTAssertEqual(capturedError.metadata?["binding"] as? String, "data.user.name") + XCTAssertEqual(capturedError.metadata?["attemptedValue"] as? String, "invalid") + XCTAssertEqual(capturedError.metadata?["validationRule"] as? String, "minLength") + XCTAssertEqual(capturedError.metadata?["component"] as? String, "TextInput") + } + + // MARK: - Error Controller Accessibility from Controllers + + func testErrorControllerAccessibleViaControllers() { + let errorControllerAccessed = expectation(description: "Error controller accessed") + + player.hooks?.state.tap { state in + guard let inProgress = state as? InProgressState else { return } + + if let errorController = inProgress.controllers?.error { + XCTAssertNotNil(errorController) + errorControllerAccessed.fulfill() + } + } + + player.start(flow: FlowData.COUNTER) { _ in } + + wait(for: [errorControllerAccessed], timeout: 2) + } +} + diff --git a/ios/core/Tests/Types/HooksTests.swift b/ios/core/Tests/Types/HooksTests.swift new file mode 100644 index 000000000..c57b4884b --- /dev/null +++ b/ios/core/Tests/Types/HooksTests.swift @@ -0,0 +1,170 @@ +// +// HooksTests.swift +// PlayerUI_Tests +// +// Created by Player Team +// + +import Foundation +import XCTest +import JavaScriptCore +@testable import PlayerUI +@testable import PlayerUIInternalTestUtilities +@testable import PlayerUITestUtilitiesCore + +class HooksTests: XCTestCase { + var player: HeadlessPlayerImpl! + + override func setUp() { + super.setUp() + player = HeadlessPlayerImpl(plugins: []) + } + + override func tearDown() { + player = nil + super.tearDown() + } + + // MARK: - Hook2 Return Value Behavior + + func testHook2ReturnValueIsIgnoredForNonBailHooks() { + // Track whether each handler is called (proving JS ignores the first handler's return value) + var firstHandlerCallCount = 0 + var secondHandlerCallCount = 0 + + let bothHandlersCalled = expectation(description: "Both handlers called at least once") + + player.hooks?.flowController.tap { flowController in + flowController.hooks.flow.tap { flow in + // First handler - returns true (attempting to bail) + flow.hooks.transition.tap { oldState, newState -> Bool in + firstHandlerCallCount += 1 + + // Fulfill once both handlers have been called at least once + if secondHandlerCallCount > 0 { + bothHandlersCalled.fulfill() + } + + return true // Returning true, but JS IGNORES it (not a bail hook!) + } + + // Second handler - SHOULD be called despite first returning true + flow.hooks.transition.tap { oldState, newState -> Bool in + secondHandlerCallCount += 1 + + // Fulfill once both handlers have been called at least once + if firstHandlerCallCount > 0 { + bothHandlersCalled.fulfill() + } + + return false + } + } + } + + // Allow expectation to be fulfilled multiple times (transitions happen during start AND manual transition) + bothHandlersCalled.assertForOverFulfill = false + + player.start(flow: FlowData.COUNTER) { _ in } + + // Trigger a transition + if let inProgressState = player.state as? InProgressState { + do { + try inProgressState.controllers?.flow.transition(with: "NEXT") + } catch { + XCTFail("Transition failed: \(error)") + } + } + + // Verify: BOTH handlers were called (proves JS ignored the true return value) + wait(for: [bothHandlersCalled], timeout: 2) + + // Verify both handlers were actually called + XCTAssertGreaterThan(firstHandlerCallCount, 0, "First handler should be called at least once") + XCTAssertGreaterThan(secondHandlerCallCount, 0, "Second handler should be called despite first returning true") + + // Verify: Despite returning true, the transition completed normally + if let inProgressState = player.state as? InProgressState { + XCTAssertNotNil(inProgressState.controllers?.flow.current?.currentState, + "Flow should have transitioned successfully - return value was ignored by JS") + } + } + + // MARK: - BailHook Return Value Behavior + + func testBailHookReturnsTrueStopsSubsequentHandlers() { + let firstHandlerCalled = expectation(description: "First handler called") + let secondHandlerCalled = expectation(description: "Second handler called") + secondHandlerCalled.isInverted = true // Should NOT be called + + player.hooks?.errorController.tap { errorController in + // First handler - returns true to bail + errorController.hooks.onError.tap { errorInfo -> Bool? in + XCTAssertEqual(errorInfo.errorType, ErrorTypes.network) + firstHandlerCalled.fulfill() + return true // BAIL - should prevent second handler from being called + } + + // Second handler - should NOT be called due to bail + errorController.hooks.onError.tap { errorInfo -> Bool? in + secondHandlerCalled.fulfill() + return nil + } + + // Capture error to trigger the hooks + errorController.captureError( + error: NSError(domain: "test", code: 500, userInfo: [NSLocalizedDescriptionKey: "Server error"]), + errorType: ErrorTypes.network, + severity: .fatal + ) + + // Verify error was captured + let currentError = errorController.getCurrentError() + XCTAssertNotNil(currentError) + + // Check that errorState was NOT set in data model (bail prevented it) + if let inProgressState = self.player.state as? InProgressState, + let dataController = inProgressState.controllers?.data { + let errorState = dataController.get(binding: "errorState") + // errorState should be nil because bail prevented it from being set + XCTAssertTrue(errorState == nil || (errorState as? NSNull) != nil) + } + } + + player.start(flow: FlowData.COUNTER) { _ in } + + wait(for: [firstHandlerCalled, secondHandlerCalled], timeout: 2) + } + + func testBailHookReturnsNilContinuesToNextHandler() { + let firstHandlerCalled = expectation(description: "First handler called") + let secondHandlerCalled = expectation(description: "Second handler called") + + player.hooks?.errorController.tap { errorController in + // First handler - returns nil to continue + errorController.hooks.onError.tap { errorInfo -> Bool? in + XCTAssertEqual(errorInfo.errorType, ErrorTypes.data) + firstHandlerCalled.fulfill() + return nil // Continue to next handler + } + + // Second handler - should be called + errorController.hooks.onError.tap { errorInfo -> Bool? in + XCTAssertEqual(errorInfo.errorType, ErrorTypes.data) + secondHandlerCalled.fulfill() + return nil + } + + // Capture error to trigger the hooks + errorController.captureError( + error: NSError(domain: "test", code: 400, userInfo: [NSLocalizedDescriptionKey: "Data error"]), + errorType: ErrorTypes.data, + severity: .error + ) + } + + player.start(flow: FlowData.COUNTER) { _ in } + + wait(for: [firstHandlerCalled, secondHandlerCalled], timeout: 2) + } +} diff --git a/ios/swiftui/Sources/SwiftUIPlayer.swift b/ios/swiftui/Sources/SwiftUIPlayer.swift index cc7e587d1..9a55a76eb 100644 --- a/ios/swiftui/Sources/SwiftUIPlayer.swift +++ b/ios/swiftui/Sources/SwiftUIPlayer.swift @@ -281,6 +281,9 @@ public struct SwiftUIPlayerHooks: CoreHooks { /// Fired when the DataController changes public var dataController: Hook + /// Fired when the ErrorController changes + public var errorController: Hook + /// Fired when the state changes public var state: Hook @@ -298,6 +301,7 @@ public struct SwiftUIPlayerHooks: CoreHooks { flowController = Hook(baseValue: player, name: "flowController") viewController = Hook(baseValue: player, name: "viewController") dataController = Hook(baseValue: player, name: "dataController") + errorController = Hook(baseValue: player, name: "errorController") state = Hook(baseValue: player, name: "state") view = SyncWaterfallHook() transition = SyncBailHook() diff --git a/ios/test-utils-core/Sources/utilities/HeadlessPlayerImpl.swift b/ios/test-utils-core/Sources/utilities/HeadlessPlayerImpl.swift index fc2e25a68..69cf9340b 100644 --- a/ios/test-utils-core/Sources/utilities/HeadlessPlayerImpl.swift +++ b/ios/test-utils-core/Sources/utilities/HeadlessPlayerImpl.swift @@ -31,6 +31,8 @@ public class HeadlessHooks: CoreHooks { public var dataController: Hook + public var errorController: Hook + public var state: Hook public var onStart: Hook @@ -39,6 +41,7 @@ public class HeadlessHooks: CoreHooks { flowController = Hook(baseValue: value, name: "flowController") viewController = Hook(baseValue: value, name: "viewController") dataController = Hook(baseValue: value, name: "dataController") + errorController = Hook(baseValue: value, name: "errorController") state = Hook(baseValue: value, name: "state") onStart = Hook(baseValue: value, name: "onStart") } diff --git a/ios/test-utils-core/Sources/utilities/TestPlayer.swift b/ios/test-utils-core/Sources/utilities/TestPlayer.swift index 8bf34c205..130d4636b 100644 --- a/ios/test-utils-core/Sources/utilities/TestPlayer.swift +++ b/ios/test-utils-core/Sources/utilities/TestPlayer.swift @@ -45,6 +45,8 @@ public class TestHooks: CoreHooks { public var viewController: Hook public var dataController: Hook + + public var errorController: Hook public var state: Hook @@ -54,6 +56,7 @@ public class TestHooks: CoreHooks { flowController = Hook(baseValue: player, name: "flowController") viewController = Hook(baseValue: player, name: "viewController") dataController = Hook(baseValue: player, name: "dataController") + errorController = Hook(baseValue: player, name: "errorController") state = Hook(baseValue: player, name: "state") onStart = Hook(baseValue: player, name: "onStart") } diff --git a/jvm/core/src/main/kotlin/com/intuit/playerui/core/error/ErrorController.kt b/jvm/core/src/main/kotlin/com/intuit/playerui/core/error/ErrorController.kt new file mode 100644 index 000000000..989e654bb --- /dev/null +++ b/jvm/core/src/main/kotlin/com/intuit/playerui/core/error/ErrorController.kt @@ -0,0 +1,163 @@ +package com.intuit.playerui.core.error + +import com.intuit.playerui.core.bridge.Invokable +import com.intuit.playerui.core.bridge.Node +import com.intuit.playerui.core.bridge.NodeWrapper +import com.intuit.playerui.core.bridge.hooks.NodeSyncBailHook1 +import com.intuit.playerui.core.bridge.serialization.serializers.NodeSerializableField +import com.intuit.playerui.core.bridge.serialization.serializers.NodeSerializableFunction +import com.intuit.playerui.core.bridge.serialization.serializers.NodeWrapperSerializer +import kotlinx.serialization.Serializable +import kotlinx.serialization.builtins.nullable +import kotlinx.serialization.builtins.serializer + +/** Severity levels for errors */ +public enum class ErrorSeverity( + public val value: String, +) { + /** Cannot continue, flow must end */ + FATAL("fatal"), + + /** Standard error, may allow recovery */ + ERROR("error"), + + /** Non-blocking, logged for telemetry */ + WARNING("warning"), +} + +/** Known error types for Player */ +public object ErrorTypes { + public const val EXPRESSION: String = "expression" + public const val BINDING: String = "binding" + public const val VIEW: String = "view" + public const val ASSET: String = "asset" + public const val NAVIGATION: String = "navigation" + public const val VALIDATION: String = "validation" + public const val DATA: String = "data" + public const val SCHEMA: String = "schema" + public const val NETWORK: String = "network" + public const val PLUGIN: String = "plugin" +} + +/** + * Represents a Player error with metadata + */ +@Serializable(with = PlayerErrorInfo.Serializer::class) +public class PlayerErrorInfo internal constructor( + override val node: Node, +) : NodeWrapper { + /** Nested error object containing message and name */ + private val error: Node? by NodeSerializableField(Node.serializer().nullable) + + /** The error message */ + public val message: String + get() = error?.getString("message") ?: "" + + /** The error name */ + public val name: String + get() = error?.getString("name") ?: "" + + /** Error category */ + public val errorType: String by NodeSerializableField(String.serializer()) { "" } + + /** Impact level */ + public val severity: ErrorSeverity? + get() = node.getString("severity")?.let { ErrorSeverity.valueOf(it.uppercase()) } + + /** Additional metadata */ + public val metadata: Map? + get() = node.getObject("metadata") as? Map + + internal object Serializer : NodeWrapperSerializer(::PlayerErrorInfo) +} + +/** + * Limited definition of the player error controller to enable error capture and management + */ +@Serializable(with = ErrorController.Serializer::class) +public class ErrorController internal constructor( + override val node: Node, +) : NodeWrapper { + private val captureError: Invokable? by NodeSerializableFunction() + private val getCurrentError: Invokable? by NodeSerializableFunction() + private val getErrors: Invokable?>? by NodeSerializableFunction() + private val clearErrors: Invokable? by NodeSerializableFunction() + private val clearCurrentError: Invokable? by NodeSerializableFunction() + + public val hooks: Hooks by NodeSerializableField(Hooks.serializer()) + + /** + * Capture an error with metadata + * @param error The error/exception object + * @param errorType Error category (use ErrorTypes constants) + * @param severity Impact level + * @param metadata Additional metadata map + * @return The captured error as a Node + */ + public fun captureError( + error: Throwable, + errorType: String, + severity: ErrorSeverity? = null, + metadata: Map? = null, + ): Node? { + val errorObj = mapOf( + "message" to error.message, + "name" to error::class.simpleName, + ) + + return when { + severity != null && metadata != null -> + captureError?.invoke(errorObj, errorType, severity.value, metadata) + severity != null -> + captureError?.invoke(errorObj, errorType, severity.value) + metadata != null -> + captureError?.invoke(errorObj, errorType, null, metadata) + else -> + captureError?.invoke(errorObj, errorType) + } + } + + /** + * Get the most recent error + * @return The current error as a Node if one exists + */ + public fun getCurrentError(): Node? = getCurrentError?.invoke() + + /** + * Get the complete error history + * @return List of all captured errors in chronological order + */ + public fun getErrors(): List? = getErrors?.invoke() + + /** + * Clear all errors (history + current + data model) + */ + public fun clearErrors() { + clearErrors?.invoke() + } + + /** + * Clear only current error and remove from data model, preserve history + */ + public fun clearCurrentError() { + clearCurrentError?.invoke() + } + + @Serializable(Hooks.Serializer::class) + public class Hooks internal constructor( + override val node: Node, + ) : NodeWrapper { + /** + * Fired when any error is captured + * - The callback receives a PlayerErrorInfo object + * - Return true from the callback to bail and prevent error state navigation + * - Return false/null to continue to next handler + */ + public val onError: NodeSyncBailHook1 + by NodeSerializableField(NodeSyncBailHook1.serializer(PlayerErrorInfo.serializer(), Boolean.serializer())) + + internal object Serializer : NodeWrapperSerializer(::Hooks) + } + + internal object Serializer : NodeWrapperSerializer(::ErrorController) +} diff --git a/jvm/core/src/main/kotlin/com/intuit/playerui/core/player/Player.kt b/jvm/core/src/main/kotlin/com/intuit/playerui/core/player/Player.kt index 4ad8a4ee6..733867be4 100644 --- a/jvm/core/src/main/kotlin/com/intuit/playerui/core/player/Player.kt +++ b/jvm/core/src/main/kotlin/com/intuit/playerui/core/player/Player.kt @@ -8,6 +8,7 @@ import com.intuit.playerui.core.bridge.serialization.serializers.NodeSerializabl import com.intuit.playerui.core.bridge.serialization.serializers.NodeWrapperSerializer import com.intuit.playerui.core.constants.ConstantsController import com.intuit.playerui.core.data.DataController +import com.intuit.playerui.core.error.ErrorController import com.intuit.playerui.core.experimental.ExperimentalPlayerApi import com.intuit.playerui.core.expressions.ExpressionController import com.intuit.playerui.core.flow.Flow @@ -63,6 +64,9 @@ public abstract class Player : Pluggable { /** Manages validations (schema and x-field ) */ public val validationController: NodeSyncHook1 + /** Manages error handling and captures errors from all subsystems */ + public val errorController: NodeSyncHook1 + /** A that's called for state changes in the flow execution */ public val state: NodeSyncHook1 @@ -87,6 +91,8 @@ public abstract class Player : Pluggable { by NodeSerializableField(NodeSyncHook1.serializer(DataController.serializer())) override val validationController: NodeSyncHook1 by NodeSerializableField(NodeSyncHook1.serializer(ValidationController.serializer())) + override val errorController: NodeSyncHook1 + by NodeSerializableField(NodeSyncHook1.serializer(ErrorController.serializer())) override val state: NodeSyncHook1 by NodeSerializableField(NodeSyncHook1.serializer(PlayerFlowState.serializer())) override val onStart: NodeSyncHook1 by NodeSerializableField(NodeSyncHook1.serializer(Flow.serializer())) diff --git a/jvm/core/src/main/kotlin/com/intuit/playerui/core/player/state/PlayerFlowState.kt b/jvm/core/src/main/kotlin/com/intuit/playerui/core/player/state/PlayerFlowState.kt index 2d5fa1e02..7612a33d3 100644 --- a/jvm/core/src/main/kotlin/com/intuit/playerui/core/player/state/PlayerFlowState.kt +++ b/jvm/core/src/main/kotlin/com/intuit/playerui/core/player/state/PlayerFlowState.kt @@ -15,6 +15,7 @@ import com.intuit.playerui.core.bridge.serialization.serializers.NodeSerializabl import com.intuit.playerui.core.bridge.serialization.serializers.NodeWrapperSerializer import com.intuit.playerui.core.data.DataController import com.intuit.playerui.core.data.DataModelWithParser +import com.intuit.playerui.core.error.ErrorController import com.intuit.playerui.core.experimental.RuntimeClassDiscriminator import com.intuit.playerui.core.expressions.ExpressionController import com.intuit.playerui.core.expressions.ExpressionEvaluator @@ -175,6 +176,9 @@ public class ControllerState internal constructor( /** the manager for the flow state machine */ public val flow: FlowController by NodeSerializableField(FlowController.serializer()) + /** The manager for error handling */ + public val error: ErrorController by NodeSerializableField(ErrorController.serializer()) + internal object Serializer : NodeWrapperSerializer(::ControllerState) } diff --git a/jvm/core/src/test/kotlin/com/intuit/playerui/core/error/ErrorControllerTest.kt b/jvm/core/src/test/kotlin/com/intuit/playerui/core/error/ErrorControllerTest.kt new file mode 100644 index 000000000..20858d720 --- /dev/null +++ b/jvm/core/src/test/kotlin/com/intuit/playerui/core/error/ErrorControllerTest.kt @@ -0,0 +1,250 @@ +package com.intuit.playerui.core.error + +import com.intuit.playerui.core.player.state.InProgressState +import com.intuit.playerui.core.plugins.Plugin +import com.intuit.playerui.plugins.assets.ReferenceAssetsPlugin +import com.intuit.playerui.plugins.types.CommonTypesPlugin +import com.intuit.playerui.utils.test.PlayerTest +import com.intuit.playerui.utils.test.runBlockingTest +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertFalse +import org.junit.jupiter.api.Assertions.assertNotNull +import org.junit.jupiter.api.Assertions.assertNull +import org.junit.jupiter.api.Assertions.assertTrue +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.TestTemplate + +internal class ErrorControllerTest : PlayerTest() { + override val plugins: List = listOf(ReferenceAssetsPlugin(), CommonTypesPlugin()) + + private lateinit var errorController: ErrorController + + private val simpleFlow = + """ + { + "id": "test-flow", + "views": [ + { + "id": "action", + "type": "action", + "value": "next", + "label": { + "asset": { + "id": "action-label", + "type": "text", + "value": "Next" + } + } + } + ], + "data": {}, + "navigation": { + "BEGIN": "FLOW_1", + "FLOW_1": { + "startState": "VIEW_1", + "VIEW_1": { + "state_type": "VIEW", + "ref": "action", + "transitions": { + "*": "END_Done" + } + }, + "END_Done": { + "state_type": "END", + "outcome": "done" + } + } + } + } + """.trimIndent() + + @BeforeEach + fun setup() = runBlockingTest { + player.start(simpleFlow) + val state = player.state as? InProgressState + assertNotNull(state) + errorController = state!!.controllers.error + } + + @TestTemplate + fun `error controller hook is called and onError hook exists`() { + assertNotNull(errorController.hooks.onError) + } + + @TestTemplate + fun `capture error with all parameters`() { + val testError = Exception("Not found") + val capturedError = errorController.captureError( + testError, + ErrorTypes.NETWORK, + ErrorSeverity.ERROR, + mapOf("url" to "https://example.com", "statusCode" to 404), + ) + + assertNotNull(capturedError) + + val errorInfo = capturedError?.let { PlayerErrorInfo(it) } + assertEquals("Not found", errorInfo?.message) + assertEquals(ErrorTypes.NETWORK, errorInfo?.errorType) + assertEquals(ErrorSeverity.ERROR, errorInfo?.severity) + assertNotNull(errorInfo?.metadata) + } + + @TestTemplate + fun `capture error with minimal parameters`() { + val testError = Exception("Internal error") + val capturedError = errorController.captureError( + testError, + ErrorTypes.PLUGIN, + ) + + assertNotNull(capturedError) + + val errorInfo = capturedError?.let { PlayerErrorInfo(it) } + assertEquals("Internal error", errorInfo?.message) + assertEquals(ErrorTypes.PLUGIN, errorInfo?.errorType) + assertNull(errorInfo?.severity) + } + + @TestTemplate + fun `capture multiple errors with chronological history and current error updates`() { + // Capture first error + errorController.captureError( + Exception("First error"), + ErrorTypes.VALIDATION, + ErrorSeverity.WARNING, + ) + + // Verify current error is the first one + var currentError = errorController.getCurrentError() + assertEquals("First error", currentError?.let { PlayerErrorInfo(it).message }) + + // Capture second error + errorController.captureError( + Exception("Second error"), + ErrorTypes.BINDING, + ErrorSeverity.ERROR, + ) + + // Current error should be updated to the second one + currentError = errorController.getCurrentError() + assertEquals("Second error", currentError?.let { PlayerErrorInfo(it).message }) + + // Capture third error + errorController.captureError( + Exception("Third error"), + ErrorTypes.VIEW, + ErrorSeverity.FATAL, + ) + + // Current error should be updated to the third one + currentError = errorController.getCurrentError() + assertEquals("Third error", currentError?.let { PlayerErrorInfo(it).message }) + + // Verify all errors are in chronological order + val errors = errorController.getErrors() + assertNotNull(errors) + assertEquals(3, errors?.size) + + val errorInfos = errors?.map { PlayerErrorInfo(it) } + assertEquals("First error", errorInfos?.get(0)?.message) + assertEquals("Second error", errorInfos?.get(1)?.message) + assertEquals("Third error", errorInfos?.get(2)?.message) + } + + @TestTemplate + fun `get current error`() { + // Initially no current error + val initialError = errorController.getCurrentError() + assertNull(initialError) + + // Capture an error + errorController.captureError( + Exception("Current error"), + ErrorTypes.DATA, + ErrorSeverity.ERROR, + ) + + // Now should have a current error + val currentError = errorController.getCurrentError() + assertNotNull(currentError) + assertFalse(currentError!!.isUndefined()) + + val errorInfo = PlayerErrorInfo(currentError) + assertEquals("Current error", errorInfo.message) + assertEquals(ErrorTypes.DATA, errorInfo.errorType) + } + + @TestTemplate + fun `clear all errors`() { + // Capture multiple errors + errorController.captureError( + Exception("Error 1"), + ErrorTypes.VALIDATION, + ) + errorController.captureError( + Exception("Error 2"), + ErrorTypes.BINDING, + ) + + val errorsBeforeClear = errorController.getErrors() + assertEquals(2, errorsBeforeClear?.size) + + val currentErrorBeforeClear = errorController.getCurrentError() + assertNotNull(currentErrorBeforeClear) + assertFalse(currentErrorBeforeClear!!.isUndefined()) + + // Clear all errors + errorController.clearErrors() + + val errorsAfterClear = errorController.getErrors() + assertNotNull(errorsAfterClear) + assertTrue(errorsAfterClear!!.isEmpty()) + + val currentErrorAfterClear = errorController.getCurrentError() + assertNull(currentErrorAfterClear) + } + + @TestTemplate + fun `clear current error preserves history`() { + // Capture multiple errors + errorController.captureError( + Exception("Error 1"), + ErrorTypes.VALIDATION, + ) + errorController.captureError( + Exception("Error 2"), + ErrorTypes.BINDING, + ) + + val errorsBeforeClear = errorController.getErrors() + assertEquals(2, errorsBeforeClear?.size) + + val currentErrorBeforeClear = errorController.getCurrentError() + assertNotNull(currentErrorBeforeClear) + assertFalse(currentErrorBeforeClear!!.isUndefined()) + + // Clear only current error + errorController.clearCurrentError() + + // History should be preserved + val errorsAfterClear = errorController.getErrors() + assertEquals(2, errorsAfterClear?.size) + + // Current error should be cleared + val currentErrorAfterClear = errorController.getCurrentError() + assertNull(currentErrorAfterClear) + } + + @TestTemplate + fun `error controller accessible via controllers`() { + val state = player.state as InProgressState + val controllers = state.controllers + + assertNotNull(controllers.data) + assertNotNull(controllers.flow) + assertNotNull(controllers.view) + assertNotNull(controllers.expression) + assertNotNull(controllers.error) + } +}