SCAL-333657: report how a host event resolved and how long it took (PR 2)

[sastaachar]

SCAL-333657 · epic SCAL-325536

PR 2, stacked on #635. Base is SCAL-333657, not main — review #635 first, and this diff shows only what PR 2 adds.

Did the host event work, and how long did it take.

PR 1 answers which host event, with which parameters. It says nothing about whether the trigger succeeded, so a host event that times out for 30 seconds looks exactly like one that works.

What this adds

Two properties on the same upload:

status	success · error · timed-out · render-not-called · host-event-undefined · no-iframe
durationMs	how long the trigger took to settle

The three non-dispatch statuses matter on their own: render-not-called and no-iframe are integration mistakes in the host application that are currently invisible, and no-iframe in particular happens after an auth failure.

timed-out needs saying out loud

processTrigger resolves — it does not reject — with Error(TRIGGER_TIMED_OUT) after 30 seconds. So without a check for that sentinel, a trigger the embedded app never answered reports as a success. That is the single most misleading thing telemetry could say here, which is why the check is in this PR rather than a later one.

isTriggerTimeout is a new export in processTrigger; it changes no behaviour.

An error's message is never uploaded — only the status — because it can echo customer data back. There is a test asserting a GUID from an error message does not appear in the properties.

One behaviour change, on purpose

The upload moves from call time to settle time, because neither the outcome nor the duration exists until the trigger finishes. Same event name, same properties as PR 1, just later by the round trip.

That is the trade this PR asks for: outcome data in exchange for a delayed event. If you would rather keep the call-time event and add a second one at settle, say so — it is a small change, but it doubles the upload count per trigger.

Testing

Four new cases in src/embed/host-event-telemetry.spec.ts: a successful trigger carrying status and durationMs, a trigger the app never answered reported as timed-out, a failed trigger with its error message asserted absent, and a trigger before render().

Full suite 1707 passed (47 files), tsc clean, lint 0 errors.

Still to come

• PR 3 — the response story, both directions: whether the trigger was answered and with what shape, and the same for an embed event handed a responder via on(ApiIntercept, responder).
• PR 4 — embed event tracking: which embed events arrive, their payload shape, how many handlers each dispatch ran.
• PR 5 — the bug fixes: the same 30s timeout is lost entirely for Pin/SaveAnswer/UpdateFilters/DrillDown, where a custom handler turns it into a generic error; and the pre-init Mixpanel queue is unbounded when disableSDKTracking is set.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request enhances host event telemetry by tracking the status (such as success, error, timeout, and validation failures) and duration of host event triggers, supported by new unit tests. The review feedback suggests wrapping the telemetry upload in a try-catch block to prevent telemetry failures from disrupting the main promise chain, and notes a design improvement opportunity regarding promise rejection on timeouts for future versions.

[gemini-code-assist]

Wrapping the telemetry upload in a try-catch block is highly recommended here. Since reportHostEvent is executed inside the .then handler of triggerHostEvent, any unexpected synchronous error thrown during telemetry generation (e.g., if describeParams or uploadMixpanelEvent fails) will cause the entire promise chain to reject. This would incorrectly report a successful host event trigger as a failure to the host application, disrupting core functionality due to a non-critical telemetry side-effect.

        const reportHostEvent = (status: HostEventStatus) => {
            try {
                uploadMixpanelEvent(
                    MIXPANEL_EVENT.VISUAL_SDK_TRIGGER + '-' + messageType,
                    getHostEventTelemetryProps({
                        hostEvent: messageType,
                        payload: data,
                        context,
                        embedComponentType: this.viewConfig?.embedComponentType,
                        status,
                        durationMs: Date.now() - triggerStartedAt,
                    }),
                );
            } catch (e) {
                logger.debug('Failed to upload host event telemetry', e);
            }
        };

[gemini-code-assist]

Resolving a promise with an Error object on timeout instead of rejecting it is generally considered an anti-pattern in Promise design. It forces callers to perform manual type checks (like instanceof Error or using isTriggerTimeout) and can easily lead to unhandled runtime exceptions (e.g., TypeError when destructuring properties like session or currentContext from the resolved value). While this behavior might need to be preserved for backward compatibility in processTrigger, consider rejecting the promise on timeout in the next major version of the SDK to align with standard asynchronous error-handling practices.

[sastaachar]

Applied in 9cdfc435. The promise is observed on the side rather than chained onto:

const triggered = this.hostEventClient.triggerHostEvent(messageType, data, context)
    .catch(/* unchanged from main */);

// Observed, not chained.
triggered.then(
    (response) => reportHostEventOutcome(id, isTriggerTimeout(response) ? 'timed-out' : 'success'),
    () => reportHostEventOutcome(id, 'error'),
).catch((e) => logger.debug('Could not report host event outcome', e));

return triggered;

The trigger's own chain is now exactly what it is on main — the outcome reporting came out of the .catch entirely — and the promise handed back is the one the host application would have received without telemetry. That is a structural guarantee rather than a try/catch one: whatever telemetry does, it is on a derived promise nobody returns.

One non-obvious part, which a test caught rather than review: the observer needs its own terminal .catch. Without it a throwing observer produces an unhandled rejection on the derived promise — and the test written to prove telemetry cannot reach the caller killed the Node process until that catch went in:

[Error: telemetry exploded]
Node.js v22.13.1

So the pattern is right, but incomplete without the trailing catch. That test now passes and pins it.

Full suite 1716 passed (47 files), lint 0 errors.