error controller navigation by cehan-Chloe · Pull Request #775 · player-ui/player

cehan-Chloe · 2025-12-22T16:20:11Z

When an error occurs in a state, ErrorController tries in order:

Node-level errorState transition - Most specific, handles errors for this state only
Flow-level errorState - General fallback, handles errors from any state in flow
Reject flow promise - No error handling defined
Error with no node/flow errorState → flowResultDeferred.reject(error)

detail doc: 4481c71

Change Type (required)

Indicate the type of change your pull request is:

patch
minor
major
N/A

Does your PR have any documentation updates?

Updated docs
No Update needed
Unable to update docs

codecov · 2025-12-22T16:34:44Z

Bundle Report

Changes will increase total bundle size by 55.04kB (0.95%) ⬆️. This is within the configured threshold ✅

Detailed changes

Bundle name	Size	Change
plugins/async-node/core	473.2kB	4.33kB (0.92%) ⬆️
plugins/beacon/core	420.43kB	4.33kB (1.04%) ⬆️
core/player	1.0MB	16.09kB (1.63%) ⬆️
plugins/check-path/core	438.83kB	4.33kB (1.0%) ⬆️
plugins/common-expressions/core	424.69kB	4.33kB (1.03%) ⬆️
plugins/common-types/core	499.01kB	4.33kB (0.87%) ⬆️
plugins/markdown/core	679.5kB	4.33kB (0.64%) ⬆️
plugins/metrics/core	456.99kB	4.33kB (0.96%) ⬆️
plugins/stage-revert-data/core	402.45kB	4.33kB (1.09%) ⬆️
plugins/reference-assets/core	480.15kB	4.33kB (0.91%) ⬆️

Affected Assets, Files, and Routes:

view changes for bundle: core/player

Assets Changed:

Asset Name	Size Change	Total Size	Change (%)
`Player.native.js`	4.33kB	421.67kB	1.04%
`cjs/index.cjs`	3.92kB	198.79kB	2.01%
`index.legacy-esm.js`	3.92kB	192.19kB	2.08%
`index.mjs`	3.92kB	192.19kB	2.08%

view changes for bundle: plugins/markdown/core

Assets Changed:

Asset Name	Size Change	Total Size	Change (%)
`MarkdownPlugin.native.js`	4.33kB	654.35kB	0.67%

view changes for bundle: plugins/common-expressions/core

Assets Changed:

Asset Name	Size Change	Total Size	Change (%)
`CommonExpressionsPlugin.native.js`	4.33kB	403.44kB	1.08%

view changes for bundle: plugins/check-path/core

Assets Changed:

Asset Name	Size Change	Total Size	Change (%)
`CheckPathPlugin.native.js`	4.33kB	409.35kB	1.07%

view changes for bundle: plugins/async-node/core

Assets Changed:

Asset Name	Size Change	Total Size	Change (%)
`AsyncNodePlugin.native.js`	4.33kB	427.12kB	1.02%

view changes for bundle: plugins/stage-revert-data/core

Assets Changed:

Asset Name	Size Change	Total Size	Change (%)
`StageRevertDataPlugin.native.js`	4.33kB	395.63kB	1.11%

view changes for bundle: plugins/common-types/core

Assets Changed:

Asset Name	Size Change	Total Size	Change (%)
`CommonTypesPlugin.native.js`	4.33kB	427.84kB	1.02%

view changes for bundle: plugins/metrics/core

Assets Changed:

Asset Name	Size Change	Total Size	Change (%)
`MetricsPlugin.native.js`	4.33kB	424.65kB	1.03%

view changes for bundle: plugins/reference-assets/core

Assets Changed:

Asset Name	Size Change	Total Size	Change (%)
`ReferenceAssetsPlugin.native.js`	4.33kB	450.25kB	0.97%

view changes for bundle: plugins/beacon/core

Assets Changed:

Asset Name	Size Change	Total Size	Change (%)
`BeaconPlugin.native.js`	4.33kB	405.96kB	1.08%

codecov · 2025-12-22T16:48:17Z

Codecov Report

❌ Patch coverage is 69.42149% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.75%. Comparing base (4481c71) to head (beca189).
⚠️ Report is 3 commits behind head on error-controller.

Files with missing lines	Patch %	Lines
core/player/src/controllers/flow/flow.ts	57.83%	35 Missing ⚠️
core/player/src/controllers/error/controller.ts	93.93%	2 Missing ⚠️

Additional details and impacted files

@@                 Coverage Diff                  @@
##           error-controller     #775      +/-   ##
====================================================
- Coverage             85.84%   85.75%   -0.10%     
====================================================
  Files                   508      508              
  Lines                 22892    22992     +100     
  Branches               2673     2697      +24     
====================================================
+ Hits                  19652    19717      +65     
- Misses                 2911     2946      +35     
  Partials                329      329

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tmarmer · 2026-01-15T17:53:23Z

core/player/src/controllers/error/controller.ts

+    if (typeof errorState === "object" && errorState !== null) {
+      const dict = errorState as Record<string, string>;


Pulling the check into a type assertion function removes the need to cast after checking here. As well, for real type safety you also need to check !Array.isArray(errorState) since arrays are also object types.

export const isRecord = ( obj: unknown ): obj is Record<PropertyKey, unknown> => typeof obj === "object" && obj !== null && !Array.isArray(obj);

This example has the values of the record as unknown which would require you to type check those too. You could add a generic to pre-determine the record value type to skip that step but it is technically more error-prone:

export const isRecord = <T>( obj: unknown ): obj is Record<PropertyKey, T> => ...

tmarmer · 2026-01-15T18:01:35Z

core/player/src/controllers/error/controller.ts

+    const nodeErrorStateConfig = currentState?.value.errorState as
+      | ErrorStateTransition
+      | undefined;


Is there a reason we need to type cast here? Do not all navigation states support an errorState?

The errorState is optional for both flow and node level. Also for some states like end state, errorState may not be needed

core/player/src/player.ts

tmarmer · 2026-01-15T18:12:53Z

core/player/src/controllers/flow/flow.ts

+    let errorStateName: string | undefined;
+
+    if (typeof errorStateConfig === "string") {
+      errorStateName = errorStateConfig;
+    } else {
+      errorStateName =
+        (errorType && errorStateConfig[errorType]) || errorStateConfig["*"];
+    }


This seems pretty similar to the error controller's resolveErrorState. Could we pull that out into a shared util?

KetanReddy · 2026-01-16T19:23:31Z

core/player/src/controllers/flow/flow.ts

+   * @param errorType Optional error type for dictionary-based error transition
+   * @returns true if navigation succeeded, false if errorState not defined or navigation failed
+   */
+  public transitionToErrorState(errorType?: string): boolean {


Why do we need this function? Couldn't we just use the existing transition function?

For node level, I still use the existing transition function. This is the flow-level error we don't have transitions map, so I use error type as the key. Do you think it's better to have consistency between flow and node? I can add transitions to flow level.
samples:
flow:

"errorState": { "network": "NETWORK_ERROR_VIEW", }

node:

"SHIPPING_VIEW": { "state_type": "VIEW", "ref": "shipping-form", "errorState": "node-error", "transitions": { "node-error": "NODE_ERROR_VIEW" } },

I think so. Also if we can centralize some of the logic here and in navigateToErrorState and make them more utility oriented I think it might be cleaner long term as that logic needs to evolve

core/player/src/controllers/error/controller.ts

…er-ui/player into error-controller-navigation

KetanReddy · 2026-01-29T01:18:57Z

core/player/src/controllers/error/utils.ts

+ * Type guard to check if a value is a non-null object record.
+ * This excludes arrays, null, and other non-object types.
+ */
+export function isRecord(obj: unknown): obj is Record<PropertyKey, unknown> {


Huh, surprised this isn't a standard function from ts

core/types/src/index.ts

KetanReddy · 2026-01-29T01:20:33Z

Looks good in principle, just a couple minor questions we might want to resolve before fully merging it

KetanReddy · 2026-01-30T18:23:08Z

core/types/src/index.ts

-    | NavigationFlowState;
+    | NavigationFlowState
+    | NavigationFlowTransition
+    | Record<string, string>;


I think NavigationFlowTransition already covers this case?

* error controller navigation

error controller navigation

4bc2b87

cehan-Chloe added 3 commits January 5, 2026 13:13

Merge branch 'error-controller' into error-controller-navigation

1c6357c

Merge branch 'error-controller' into error-controller-navigation

e73b8a8

fix failure after merge

3f2db3b

cehan-Chloe marked this pull request as ready for review January 8, 2026 15:36

cehan-Chloe requested a review from a team as a code owner January 8, 2026 15:36

cehan-Chloe requested review from KetanReddy, sugarmanz and tmarmer January 14, 2026 15:35

Merge branch 'error-controller' into error-controller-navigation

6cde2d4

tmarmer requested changes Jan 15, 2026

View reviewed changes

KetanReddy reviewed Jan 16, 2026

View reviewed changes

core/player/src/controllers/error/controller.ts Outdated Show resolved Hide resolved

cehan-Chloe added 2 commits January 20, 2026 10:54

address review comments-util and type check

1899eaa

Merge branch 'error-controller-navigation' of https://github.com/play…

4f44988

…er-ui/player into error-controller-navigation

cehan-Chloe requested review from KetanReddy and tmarmer January 20, 2026 15:56

cehan-Chloe added 4 commits January 20, 2026 11:26

fix linting error

d274059

update type

86769c7

fix lint...

40f173b

add flow transitions

31869a6

KetanReddy reviewed Jan 29, 2026

View reviewed changes

core/types/src/index.ts Show resolved Hide resolved

separate errorTransitions map

beca189

cehan-Chloe requested a review from KetanReddy January 30, 2026 17:11

KetanReddy reviewed Jan 30, 2026

View reviewed changes

remove redundant type

ce1dae7

KetanReddy approved these changes Jan 30, 2026

View reviewed changes

cehan-Chloe merged commit 03898f1 into error-controller Jan 30, 2026
9 of 11 checks passed

cehan-Chloe deleted the error-controller-navigation branch January 30, 2026 18:52

cehan-Chloe added a commit that referenced this pull request Feb 9, 2026

error controller navigation (#775)

61a85e7

* error controller navigation

		if (typeof errorState === "object" && errorState !== null) {
		const dict = errorState as Record<string, string>;

Conversation

cehan-Chloe commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Type (required)

Does your PR have any documentation updates?

Uh oh!

codecov bot commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle Report

Affected Assets, Files, and Routes:

Assets Changed:

Assets Changed:

Assets Changed:

Assets Changed:

Assets Changed:

Assets Changed:

Assets Changed:

Assets Changed:

Assets Changed:

Assets Changed:

Uh oh!

codecov bot commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KetanReddy commented Jan 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cehan-Chloe commented Dec 22, 2025 •

edited

Loading

codecov bot commented Dec 22, 2025 •

edited

Loading

codecov bot commented Dec 22, 2025 •

edited

Loading