ExternalActionPlugin & ExternalActionViewModifierPlugin use array of match-handler pairs

[KVSRoyal]

What

This is the ios/swiftui part of #817. This does not include a docs update. I plan to update the multi-platform docs in a separate PR once the APIs are solidified.

This PR changes the ExternalActionPlugin & ExternalActionViewModifierPlugin to accept an array of match-handler pairs. These pairs are passed through to the core.

ExternalActionPlugin Before / After

// Before: Single handler - requires manual routing logic
let plugin = ExternalActionPlugin { state, options, transition in
    // Must manually check which action to handle
    guard state.ref == "custom-action" else {
        return transition("default")
    }
    transition("next")
}

// After: Array of keyed handlers - automatic routing with specificity matching
let plugin = ExternalActionPlugin(handlers: [
    ExternalStateHandler(
        match: ["ref": "custom-action"],
        handler: { state, options, transition in
            transition("next")
        }
    ),
    ExternalStateHandler(
        match: ["ref": "other-action"],
        handler: { state, options, transition in
            transition("prev")
        }
    )
])

ExternalActionViewModifierPlugin Before / After

(For those unfamiliar, this version of the plugin allows the user to also provide the External Action view through the same API.)

// Before: Single handler - requires manual routing logic
let plugin = ExternalActionViewModifierPlugin<ExternalStateSheetModifier> { state, options, transition in
    // Must manually check which action to handle
    guard state.ref == "custom-action" else {
        return AnyView(Text("Unknown Action"))
    } 
    return AnyView(
        CustomActionView(onComplete: { transition("next") })
    )
}

// After: Array of keyed handlers - automatic routing with specificity matching
let plugin = ExternalActionViewModifierPlugin<ExternalStateSheetModifier>(handlers: [
    ExternalStateViewModifierHandler(
        match: ["ref": "custom-action"],
        handler: { state, options, transition in
            AnyView(CustomActionView(onComplete: { transition("next") }))
        }
    ),
    ExternalStateViewModifierHandler(
        match: ["ref": "other-action"],
        handler: { state, options, transition in
            AnyView(OtherActionView(onComplete: { transition("prev") }))
        }
    )
])

Additionally:

1. We were randomly using ExternalAction vs ExternalState. Everything is now aligned to use ExternalAction.
2. I realized that the Swift JavaScriptCore bypasses TypeScripty safety. It was allowing [:] as a valid match. Valid matches should require at least ["ref" : something]. I added a check in the core layer to throw out match-handler pairs without a ref in them.

Why

Deterministic Overrides: Users want predictable behaviour when multiple plugins or handlers are registered. Registerying handlers by match ensures that:

• More specific matchers always take precedence (e.g., { ref: "x", type: "y" } beats { ref: "x" })
• Later registrations replace earlier ones with the same specificity
• Behaviour is consistent regardless of registration order

Change Type (required)

Indicate the type of change your pull request is:

• patch
• minor
• major
• N/A (part of 1.0)

Does your PR have any documentation updates?

• Updated docs
• No Update needed
• Unable to update docs

[KVSRoyal]

@claude You are a principal engineer specialized in https://player-ui.github.io/next/ who follows best practices. Review this PR. Please follow these additional guidelines:

1. Use simple tenses where possible. Simple verb tenses (past, present, and future) keep things concise and people process simpler tenses faster than complex ones.
2. Keep words shorter than 9 characters, where possible. Technical terms are okay. Studies show that people are more likely to skip longer words.
3. Keep sentences shorter than 20 words. Longer sentences are harder to follow.
4. Use mermaid diagrams, tables, etc. where it will help illustrate a point.”

[claude]

Claude finished @KVSRoyal's task in 1m 59s —— View job

---

PR Review

• Read changed files
• Review core TypeScript changes
• Review iOS Swift changes
• Review SwiftUI changes
• Review tests
• Post final review

---

Overall, this is a solid PR. The registry-based dispatch is clean. The specificity logic is well-tested. Below are findings across the layers.

---

How It Works

sequenceDiagram
    participant SW as Swift/SwiftUI
    participant JS as JS Core (ExternalActionPlugin)
    participant R as Registry

    SW->>JS: apply() with [match, handler] pairs
    JS->>JS: validate each match has ref
    JS->>R: set(match, handler) for each valid pair
    Note over R: More specific matches win
    JS->>JS: tap flow.transition hook
    JS->>R: get(externalState)
    R-->>JS: best matching handler
    JS->>SW: invoke handler callback
    SW->>JS: transition(value)

Loading

---

Findings

Bug — isExternalState not reset on error

File: swiftui/Sources/ExternalActionViewModifierPlugin.swift:91-105

When a handler throws, isExternalState stays true and state stays set. The catch block only calls reject. This leaves the plugin in a bad state.

// isExternalState = true  ← set here
// self?.state = state     ← set here
do {
    self?.content = try handler.handler(...)
} catch {
    reject(...)  // ← isExternalState and state never cleared
}

The skipped test skiptestExternalStateHandlingThrowsError at ViewInspector/ExternalActionViewModifierPluginTests.swift:238 likely fails because of this. This bug exists in the old code, but this PR is a good time to fix it. Fix this →

---

Medium — No Swift-level ref validation

File: ios/Sources/ExternalActionPlugin.swift:29, swiftui/Sources/ExternalActionViewModifierPlugin.swift:24

The Match type is [String: Any]. Swift does not require ref. The runtime check in TS logs a warning but only at apply() time. Users won't know about the bad handler until the player starts.

Consider validating in init:

public init(match: Match, handler: @escaping Handler) throws {
    guard match["ref"] is String else {
        throw ExternalStateHandlerError.missingRef
    }
    self.match = match
    self.handler = handler
}

Or, at minimum, add a precondition or assertionFailure so bugs surface in debug builds.

---

Medium — Duplicate Match type alias

Type	File
ExternalStateHandler.Match	ios/Sources/ExternalActionPlugin.swift:14
ExternalStateViewModifierHandler.Match	swiftui/Sources/ExternalActionViewModifierPlugin.swift:9

Both are [String: Any]. They live in separate modules, so sharing is not trivial. But the names diverge from the TS ExternalActionMatch type. A shared typealias ExternalActionMatch = [String: Any] in a common module would help.

---

Minor — ExternalActionMatch type comment is slightly off

File: core/src/index.ts:18

The comment says the new form "overrides any index signature leniency." This is correct. If NavigationFlowExternalState has a [key: string]: any index signature, then the old Pick<..., "ref"> form could allow {} through. The new { ref: string } literal form is stricter and the intent is clearer. No change needed, but the comment could be more direct:

// { ref: string } is a plain object type (no index signature).
// Prevents TypeScript from accepting {} when NavigationFlowExternalState has [key: string]: any.

---

Minor — Missing space in demo code

File: ios/demo/Sources/FlowManagerView.swift:40

match: ["ref":"test-1"],  // ← missing space after colon

Should be ["ref": "test-1"].

---

Minor — Test assertions could be tighter

File: core/src/__tests__/index.test.ts:527-530

expect(warnSpy).toHaveBeenCalledWith(
    expect.stringContaining("An external action match is missing the 'ref' property")
);
expect(warnSpy).toHaveBeenCalledWith(
    expect.stringContaining("This handler will be ignored")
);

Both strings appear in the same warn message. The two toHaveBeenCalledWith checks don't prove they came from the same call. A single expect.stringContaining with the full message (or using toHaveBeenNthCalledWith) would be more precise.

---

Summary

Area	Status
Core TS logic	✅ Clean
TS type safety	✅ Improved
Swift API shape	✅ Good
SwiftUI error cleanup	⚠️ Bug — isExternalState not reset on throw
Swift ref validation	⚠️ Late failure, no init-time guard
Test coverage	✅ Good, one test still skipped
Demo code	✅ Works, minor style nit

The core change is well-designed. Fix the error-state cleanup in the SwiftUI plugin (and unskip the test) before merging.

[KVSRoyal]

@claude You are a principal engineer specialized in https://player-ui.github.io/next/ who follows best practices. Review this PR. Please follow these additional guidelines:

1. Use simple tenses where possible. Simple verb tenses (past, present, and future) keep things concise and people process simpler tenses faster than complex ones.
2. Keep words shorter than 9 characters, where possible. Technical terms are okay. Studies show that people are more likely to skip longer words.
3. Keep sentences shorter than 20 words. Longer sentences are harder to follow.
4. Use mermaid diagrams, tables, etc. where it will help illustrate a point.”

[claude]

Claude finished @KVSRoyal's task in 2m 10s —— View job

---

PR Review

All four items from the prior review are fixed. Here is what changed and what is new.

---

How It Works

sequenceDiagram
    participant Swift
    participant JS as JS Core
    participant R as Registry

    Swift->>Swift: init() — throws if match has no ref
    Swift->>JS: apply() with [match, handler] pairs
    JS->>JS: skip any match without ref (+ warn)
    JS->>R: set(match, handler) per valid pair
    Note over R: More specific matches win
    JS->>JS: tap flow.transition hook
    JS->>R: get(externalState)
    R-->>JS: best matching handler
    JS->>Swift: invoke callback
    Swift->>JS: transition(value)

Loading

---

Fixed from Prior Review

Finding	Status
isExternalAction not reset on throw	✅ Fixed — catch block clears all state
No Swift ref check at init	✅ Fixed — both inits throw on missing ref
Skipped throw test	✅ Unskipped and passing
Demo spacing nit	✅ Fixed

---

New Findings

Medium — @Published updates off main thread

File: swiftui/Sources/ExternalActionViewModifierPlugin.swift:107-109, 120-124

isExternalAction, state, and content are @Published. They update inside the JSCore callback. That runs off the main thread. SwiftUI needs main thread updates for @Published. The error path has the same issue.

// Off main thread — can cause runtime warnings or data races
self?.isExternalAction = true   // line 107
self?.state = state             // line 109
self?.content = try handler...  // line 111

Note: withAnimation does not dispatch to main. It only wraps animations.

Fix: wrap all @Published writes in DispatchQueue.main.async. Fix this →

---

Minor — try? in demo silently drops plugin

File: ios/demo/Sources/FlowManagerView.swift:75

// Silent failure — a bad match drops the plugin with no warning
private let externalActionPlugin = try? ExternalActionViewModifierPlugin<...>

try? turns an init error into nil. The plugin is just gone. Use try! in demo code so bad input is obvious. Fix this →

---

Minor — Test warn assertions could pin call order

File: core/src/__tests__/index.test.ts:535-541

The test checks toHaveBeenCalledTimes(2) and two toHaveBeenCalledWith calls. This is good. But the order is not pinned. Use toHaveBeenNthCalledWith to lock in the exact call sequence. Fix this →

---

Summary

Area	Status
Core TS logic	✅ Clean
TS type safety	✅ Improved
Swift API shape	✅ Good
SwiftUI error cleanup	✅ Fixed
Swift ref validation at init	✅ Fixed
@Published thread safety	⚠️ Updates off main thread
Demo error handling	⚠️ try? drops plugin silently
Test coverage	✅ Good — all tests enabled

Fix the @Published threading issue before merging. The rest are minor.

[KVSRoyal]

@claude Any remaining feedback? Avoid nit feedback.

[claude]

Claude finished @KVSRoyal's task in 1m 17s —— View job

---

• Read current file state
• Check for remaining non-nit issues
• Post findings

---

Remaining Finding

All prior items are fixed. One thread-safety gap remains.

Bug — @Published writes in apply() are off main thread

File: swiftui/Sources/ExternalActionViewModifierPlugin.swift:84-85

The getArguments() fix correctly uses Task { @MainActor in }. But the transition hook in apply() still writes @Published properties without main thread dispatch:

flow.hooks.transition.tap {[weak self] old, newState in
    guard
        old?.value?.stateType == "EXTERNAL",
        newState.value?.stateType != "EXTERNAL"
    else { return }
    self?.isExternalAction = false  // ← off main thread
    self?.state = nil               // ← off main thread
}

This fires when the player leaves an external state (e.g. via core navigation). That hook runs on the JS thread, not the main thread. SwiftUI requires @Published writes on the main thread.

Fix: wrap lines 84-85 in Task { @MainActor in }. Fix this →

[JunDangIntuit]

Consider removing the swiftlint disable and wrapping this in a do/catch with fatalError
instead of try!. A bare try! crash gives no context about what went wrong — a fatalError with
the error message makes debugging much easier.

[JunDangIntuit]

should self?.content = nil also in withAnimation block?

Also, Task { @mainactor ... } here might cause a race condition for unit tests (which could fail). Moreover, there is one outer Task and then another nested Task, might more likely have delays.

[KVSRoyal]

It wasn't in the block before, so I wasn't planning on moving it. I didn't want to mess with that logic too much.

I think the content might not be part of the animation because the view itself (content is a view) might have an animation applied though?

Also, Task { @mainactor ... } here might cause a race condition for unit tests (which could fail). Moreover, there is one outer Task and then another nested Task, might more likely have delays.

Thought I got rid of that second task. Will remove.

I'm not sure what to do about the race condition for unit tests. It's important that the stuff happens async (so it's not blocking) on the main thread (because the published changes might affect the UI). Do you have any recommendations?

[JunDangIntuit]

Consider removing the swiftlint disable and wrapping this in a do/catch with fatalError
instead of try!. A bare try! crash gives no context about what went wrong — a fatalError with
the error message makes debugging much easier

[JunDangIntuit]

Is this intentional? looks like the app might hang here since no transition?

[KVSRoyal]

Yeah. This one isn't actually used in the demo app as far as I can tell. I've just left it the way I found it.

[JunDangIntuit]

having a question regarding this: if any single handler in the array has a nil ref, the entire init fails (no ExternalActionPlugin is created at all), even if the other handlers are valid. Is this expected?

[JunDangIntuit]

same question: if any single handler in the array has a nil ref, then there will be no ExternalActionViewModifierPlugin created, is this expected?

[KVSRoyal]

That's the current behaviour on ios/android, yeah. We can revise it. Let's sync off of Github

[JunDangIntuit]

self?.content should be in withAnimation block? as content could be nil before the animation finished. then you might see a blank content.

[JunDangIntuit]

now updateUI is protected. however, the transition part is still not protected. Those published var (self?.isExternalAction = false, self?.state = nil, self?.content = nil) could be updated on a wrong thread
so you would apply the same pattern to it (like you previously has two Task {@mainactor in }, you would need two here as well).
if Thread.isMainThread {
//do reset
} else {
DispatchQueue.main.sync(execute: // do reset)
}