fix(trace): settle in-flight captures before writing a per-test slice

packages/app/src/components/browser/vnode-transform.ts

@@ -12,13 +12,39 @@ interface SerializedVNode {
 
 type TransformInput = SerializedVNode | string | number | null
 
+/**
+ * Inline event attributes (`<button onclick="…">`) are captured as STRINGS, and
+ * Preact reads any `on*` prop as a listener: it stamps its own bookkeeping
+ * property onto the value, which throws `Cannot create property … on string`
+ * and aborts the whole render. One such attribute anywhere in the document was
+ * enough to leave the replay iframe blank with nothing logged — the render
+ * never reached an `<html>` element, so the caller's own guard returned early.
+ *
+ * Dropping them costs nothing: the replay is a static reconstruction and
+ * `#renderVdom` strips `<script>` tags for the same reason, so a handler has
+ * nothing left to fire. A function value (never produced by capture) is left
+ * alone so a real listener still binds.
+ */
+function withoutInlineHandlers(
+  props: Record<string, unknown>
+): Record<string, unknown> {
+  const kept: Record<string, unknown> = {}
+  for (const [key, value] of Object.entries(props)) {
+    if (!(/^on[A-Za-z]/.test(key) && typeof value === 'string')) {
+      kept[key] = value
+    }
+  }
+  return kept
+}
+
 export function transform(node: TransformInput): VNode<{}> {
   if (typeof node !== 'object' || node === null) {
     // Plain string/number text node — return as-is for Preact to render as text.
     return node as unknown as VNode<{}>
   }
 
-  const { children, ...props } = node.props ?? {}
+  const { children, ...rawProps } = node.props ?? {}
+  const props = withoutInlineHandlers(rawProps)
   /**
    * ToDo(Christian): fix way we collect data on added nodes in script
    */

packages/app/tests/vnode-transform.test.ts

@@ -2,7 +2,7 @@
  * @vitest-environment happy-dom
  */
 import { describe, expect, it } from 'vitest'
-import type { VNode } from 'preact'
+import { render, type VNode } from 'preact'
 
 import { transform } from '../src/components/browser/vnode-transform.js'
 // The producers on the other side of the wire. `transform` only ever sees what
@@ -320,6 +320,45 @@ describe('transform', () => {
       expect(propsOf(username)['data-wdio-ref']).toBe(ref)
     })
 
+    it('renders without throwing when the page carries an inline handler', () => {
+      // The crux: Preact reads any `on*` prop as a listener and stamps its own
+      // bookkeeping onto the value. A captured handler is a STRING, so that
+      // throws `Cannot create property … on string` and aborts the render
+      // mid-tree — #renderVdom then finds no <html> and returns early, leaving
+      // the replay iframe blank with nothing logged. One attribute anywhere in
+      // the document was enough (measured on a real mobile-web trace).
+      const root = transform(
+        captureDocument(
+          '<head></head><body><button onclick="submitIt()">Go</button></body>'
+        )
+      )
+
+      const container = document.createDocumentFragment()
+      expect(() => render(root, container)).not.toThrow()
+      expect(container.firstElementChild?.tagName).toBe('HTML')
+      expect(container.firstElementChild?.querySelector('button')).toBeTruthy()
+    })
+
+    it('drops the inline handler rather than rendering it', () => {
+      // Nothing is lost: the replay is a static reconstruction and
+      // #renderVdom strips <script> tags for the same reason, so a handler has
+      // nothing left to fire.
+      const captured = captureFragment(
+        '<button onclick="submitIt()">Go</button>'
+      )
+
+      expect(propsOf(transform(captured)).onclick).toBeUndefined()
+    })
+
+    it('keeps a function-valued handler, which capture never produces', () => {
+      // Narrow the guard to the string case so a real listener still binds if
+      // one is ever passed in by hand.
+      const handler = () => {}
+      const node = transform({ type: 'button', props: { onClick: handler } })
+
+      expect(propsOf(node).onClick).toBe(handler)
+    })
+
     it('gives a one-child head a bare child rather than a list', () => {
       // snapshot.ts's #renderNewDocument prepends its <base> by spreading
       // head.props.children, so this shape — a real page with a single <title>

packages/core/src/spec-trace-helpers.ts

@@ -36,6 +36,14 @@ export interface SpecRange {
   networkStartIdx: number
   mutationStartIdx: number
   traceLogStartIdx: number
+  /** How many in-flight captures the adapter had recorded when this slice
+   *  opened — the same shape as the index fields above, over the capture list
+   *  rather than a data array. A flush waits for the captures AFTER this point
+   *  and no others: those are its own, while an earlier one belongs to a slice
+   *  that has already had its wait. Absent for an adapter that records no
+   *  boundaries or awaits its captures inline (the WDIO service), where the
+   *  wait is a no-op either way. */
+  captureStartIdx?: number
 }
 
 /**
@@ -262,6 +270,9 @@ export interface SpecBoundaryContext {
     mutations: ArrayLike<unknown>
     traceLogs: ArrayLike<unknown>
   }
+  /** The adapter's live in-flight capture list, stamped onto each range as
+   *  `captureStartIdx`. Live by reference: it grows as the run does. */
+  pendingCaptures?: ArrayLike<unknown>
 }
 
 /** Push a new slice range and return the previous (unflushed) range to flush.
@@ -290,7 +301,8 @@ function pushSliceRange(
     consoleStartIdx: ctx.capturer.consoleLogs.length,
     networkStartIdx: ctx.capturer.networkRequests.length,
     mutationStartIdx: ctx.capturer.mutations.length,
-    traceLogStartIdx: ctx.capturer.traceLogs.length
+    traceLogStartIdx: ctx.capturer.traceLogs.length,
+    captureStartIdx: ctx.pendingCaptures?.length ?? 0
   })
 
   return prevRange

packages/core/src/trace-finalizer.ts

@@ -81,8 +81,20 @@ export interface TraceExportContext {
   flushed: Set<string>
   /** Adapters keep their differing dir logic; range is set for spec writes. */
   resolveOutputDir: (range?: SpecRange) => string
-  /** Pending snapshot captures to settle before writing (selenium/nightwatch). */
+  /** Everything finalize must settle before writing: the adapters' in-flight
+   *  captures AND their eager slice-write promises (selenium/nightwatch). */
   awaitPending?: Promise<unknown>[]
+  /** The captures alone, live by reference and in record order, so an eager
+   *  flush can settle the ones from `SpecRange.captureStartIdx` on — its own —
+   *  without waiting on another slice's. */
+  pendingCaptures?: readonly Promise<unknown>[]
+  /** Session-wide work every slice depends on whatever its window: the metadata
+   *  read (viewport, device, capabilities), captured once at session start and
+   *  stamped on every trace written after it. Scoping cannot cover this — it
+   *  belongs to no slice — and a slice written before it resolves carries the
+   *  fallback viewport, which finalize cannot correct once the artifact exists.
+   *  Only the WDIO service defers it; the others await theirs inline. */
+  metadataCapture?: Promise<unknown>
   log?: (level: 'info' | 'warn', msg: string) => void
   onArtifact?: (a: TraceArtifact) => void
   /** When true, finalize writes a `devtools-artifacts-<sessionId>.json` manifest
@@ -140,26 +152,62 @@ async function settlePending(
   return result
 }
 
-/** Settle in-flight snapshot captures under the timeout cap, warning if the
- *  bound elapses. Never throws; never hangs. */
-async function awaitPendingCaptures(ctx: TraceExportContext): Promise<void> {
-  if (!ctx.awaitPending?.length) {
+/**
+ * Settle in-flight work under the timeout cap, warning if the bound elapses.
+ * Never throws; never hangs.
+ *
+ * `sliceFrom` is an eager per-slice flush, and it waits on exactly its OWN
+ * captures: the ones the adapter recorded after this slice opened. A capture
+ * from an earlier slice is that slice's business and has already had its wait,
+ * so a single wedged driver read cannot tax every later slice — and nothing of
+ * this slice's own is skipped, which guessing by "have we awaited this promise
+ * before" could not promise.
+ *
+ * Finalize passes no index and waits on everything, deliberately: the full list
+ * also carries the adapters' eager slice-WRITE promises, and the artifacts
+ * manifest is written from what they produce, so skipping one there would let
+ * the manifest be written while a slice still is.
+ */
+async function awaitPendingCaptures(
+  ctx: TraceExportContext,
+  sliceFrom?: number
+): Promise<void> {
+  // Session-wide prerequisites ride along with both: they belong to no slice,
+  // and re-awaiting a settled one is free.
+  const sessionWide = ctx.metadataCapture ? [ctx.metadataCapture] : []
+  const pending =
+    sliceFrom === undefined
+      ? [...sessionWide, ...(ctx.awaitPending ?? [])]
+      : [
+          ...sessionWide,
+          ...Array.from(ctx.pendingCaptures ?? []).slice(sliceFrom)
+        ]
+  if (!pending.length) {
     return
   }
-  const timedOut = await settlePending(
-    ctx.awaitPending,
-    PENDING_SETTLE_TIMEOUT_MS
-  )
+  const timedOut = await settlePending(pending, PENDING_SETTLE_TIMEOUT_MS)
   if (timedOut) {
     ctx.log?.(
       'warn',
-      `One or more of ${ctx.awaitPending.length} snapshot capture(s) did not ` +
+      `One or more of ${pending.length} snapshot capture(s) did not ` +
         `settle within ${PENDING_SETTLE_TIMEOUT_MS}ms; writing trace with the ` +
         'snapshots captured so far.'
     )
   }
 }
 
+/** The range that bounds `range`, read from the adapter's LIVE list — the next
+ *  test's boundary is appended to it as that test starts, so a flush that waited
+ *  sees a bound that did not exist when it began. Identity lookup: every caller
+ *  passes a range it took from this same array. */
+function successorOf(
+  ctx: TraceExportContext,
+  range: SpecRange
+): SpecRange | undefined {
+  const index = ctx.ranges.indexOf(range)
+  return index === -1 ? undefined : ctx.ranges[index + 1]
+}
+
 function sliceCountWarning(count: number): string {
   return (
     `traceGranularity produced ${count} trace slices. Consider pairing it ` +
@@ -240,6 +288,18 @@ export async function flushRangeTrace(
   }
   ctx.flushed.add(range.key)
 
+  // A slice is written from whatever the capturer holds RIGHT NOW, and the
+  // adapters' DOM captures are fire-and-forget, so an eager flush has to settle
+  // them or it writes a slice describing a page nobody has read yet. Cheap
+  // where those captures already resolved (a desktop drain is milliseconds),
+  // and load-bearing where they cannot: on Appium every page-side call is a
+  // round trip of hundreds of ms to seconds, so the per-test slice reliably
+  // beat the collector and carried no `trace.mutations` at all. Bounded by the
+  // same timeout the finalizer uses, so a stuck capture delays a test hook
+  // rather than hanging it. Scoped to this slice's own captures; finalize
+  // waits on everything.
+  await awaitPendingCaptures(ctx, range.captureStartIdx ?? 0)
+
   const isTestSlice = range.testUid !== undefined
   const sliceMetadata = isTestSlice
     ? filterTestMetadataByUid(ctx.testMetadata, range.testUid!)
@@ -261,7 +321,14 @@ export async function flushRangeTrace(
   const writeSlice = isTestSlice ? writeTestSliceTrace : writeSpecTrace
   artifact.path = await writeSlice({
     range,
-    nextRange,
+    // Re-resolved AFTER the wait, which can span the start of the next test:
+    // Nightwatch — and Selenium's cucumber hook — start the flush without
+    // awaiting it, and an eager flush is open-ended, so that test's commands
+    // and mutations would land in this test's trace. Its boundary appears in
+    // `ctx.ranges` as it starts, which closes this slice on exactly the bound
+    // that now exists. Freezing indexes before the wait cannot serve: the
+    // captures being waited for append to those same arrays.
+    nextRange: nextRange ?? successorOf(ctx, range),
     capturer: ctx.capturer,
     actionSnapshots: ctx.actionSnapshots ?? [],
     screencastFrames: ctx.screencastFrames,