Skip to content

fix(#125): DOM mutations not applied after action - #125

Open
lekt9 wants to merge 3 commits into
justrach:mainfrom
lekt9:fix/125-dom-mutations
Open

fix(#125): DOM mutations not applied after action#125
lekt9 wants to merge 3 commits into
justrach:mainfrom
lekt9:fix/125-dom-mutations

Conversation

@lekt9

@lekt9 lekt9 commented Mar 31, 2026

Copy link
Copy Markdown

Summary

  • After Runtime.callFunctionOn dispatches a DOM-mutating action (click, dblclick, check, uncheck, fill, type, select, press), the browser queues microtasks/macrotasks for framework state updates (React/Vue reconciliation, MutationObserver callbacks). The handler was returning immediately, so a subsequent /snapshot call read stale DOM.
  • Fix: send Runtime.evaluate with awaitPromise:true and a setTimeout(r, 0)-based Promise after every mutating action. CDP blocks until the promise resolves — which only happens after the current macrotask queue turn drains — ensuring all pending DOM mutations are committed before we respond.
  • Applies the flush at three call sites: the main action path, the realistic fill path, and the press key-event path.

Test plan

  • New regression test action microtask flush params are well-formed JSON (#125) in src/test/integration.zig verifies the flush JSON payload contains awaitPromise:true and the setTimeout-based Promise expression
  • All existing tests pass: zig build test exits 0
  • Manual: click a button on a React/Vue SPA, then immediately call /snapshot — updated DOM should reflect the mutation

🤖 Generated with Claude Code

lekt9 and others added 3 commits March 31, 2026 14:40
Request-scoped arenas passed by callers are freed when the HTTP request
completes. Buffering raw arena pointers in event_buf.owner caused dangling
references when a later har/stop request tried to drain and free those
events. push() now dupes the event data into the long-lived event_buf
allocator and frees the caller's copy immediately.

Adds regression test that simulates the exact failure scenario: events
buffered under arena A remain readable and correctly freed after arena A
is torn down and arena B reads the buffer.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… field fixed

- Press action resolves ref to objectId and calls focus() before dispatching
  Input.dispatchKeyEvent, so key events reach the intended element rather
  than whatever is currently focused (or nothing)
- Key text field is now empty for non-printable keys like Enter/Escape/Tab;
  only single printable characters (0x20-0x7e) carry text, preventing
  literal key-name strings from being inserted into focused inputs
- Fill action already used nativeInputValueSetter (Object.getOwnPropertyDescriptor)
  for React/Vue compat; adding tests to cover route param parsing for both actions

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
After Runtime.callFunctionOn executes a DOM-mutating action (click,
dblclick, check, uncheck, fill, type, select, press), the browser
queues microtasks and macrotasks for framework state updates
(React/Vue reconciliation, MutationObserver callbacks). Returning
immediately lets a subsequent /snapshot call read stale DOM.

Fix: send Runtime.evaluate with awaitPromise:true and a
setTimeout(r, 0)-based Promise after every mutating action. CDP
blocks until the promise resolves, which only happens after the
current macrotask queue turn drains — ensuring all pending DOM
mutations are committed before we respond.

Adds regression test verifying the flush JSON payload contains
awaitPromise:true and the setTimeout-based Promise expression.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@justrach justrach left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

This is a solid and needed fix — the microtask flush pattern and native key events are real improvements for SPA compatibility. A few suggestions:

✅ What's good

  • Input.dispatchKeyEvent (keyDown+keyUp) instead of JS KeyboardEvent dispatch — Enter/Escape/Tab now work natively
  • nativeInputValueSetter bypasses React/Vue controlled input overrides — this was a real pain point
  • select now dispatches both input + change events for framework compat
  • Microtask flush with awaitPromise:true ensures DOM is settled before returning

⚠️ Suggestions

  1. Microtask flush depth — The setTimeout(r, 0) flush drains one macrotask turn. For heavy SPAs (Angular with zone.js, Svelte with tick()), a single turn may not be enough. Consider either:

    • Two consecutive flushes: new Promise(r => setTimeout(() => setTimeout(r, 0), 0))
    • Or a configurable delay param: &flush_ms=50 for callers who know they need more time
  2. HAR flush after every action — The PR adds flushEventsToHar() after click/fill/press/select/evaluate. This is defensive but adds latency. Consider making it opt-in (&flush_har=true) or only flushing when HAR recording is active (which you partly do with the isRecording() check — good).

  3. Rebase neededclient.zig has diverged significantly on main (EventBuffer is now ArrayListUnmanaged with OwnedEvent struct). The push() signature changed. You'll need to rebase against current main.

🔧 Needs rebase

src/cdp/client.zig and src/server/router.zig both have conflicts with main. The router changes (action handling) should rebase cleanly since they're in different regions than our SSRF/bot-detect work.

@justrach

Copy link
Copy Markdown
Owner

Can you update this PR to build and test against Zig 0.16.0? The active release/CI path is now targeting 0.16.0, so it would help to rebase and rerun on that toolchain before review.

lekt9 added a commit to lekt9/kuri that referenced this pull request May 19, 2026
Merges the unbrowse-fork Zig-0.16 lineage + the x86_64-windows compat
port into the fork's main. cdp/client.zig + server/router.zig conflicts
resolved to the fork lineage (theirs): main's lineage still uses removed
std.net / std.Thread.Mutex and does not compile on Zig 0.16, while the
fork lineage is the 0.16-ported production codebase and already carries
the equivalent EventBuffer dupe-safety (justrach#124) and refactored value
setter (value_action_fn, supersedes the justrach#125 inline patch).

# Conflicts:
#	src/cdp/client.zig
#	src/server/router.zig
@justrach

Copy link
Copy Markdown
Owner

Triage from a fresh look at this against current main:

  1. Conflicts: branch is DIRTY against main — needs a rebase.
  2. Some of this is now obsolete: cmdAction on main no longer interpolates user values into JS source. The fill/select paths bind two JS function constants (value_action_fn, select_action_fn) and pass the value as a CDP argument to Runtime.callFunctionOn. So the React-controlled-input + microtask-flush parts of this PR are still wanted, but the parts that substitute {s} into a JS template are stale.
  3. Still valuable from this PR after rebase:
    • Input.dispatchKeyEvent for press (real native key handling).
    • nativeInputValueSetter to bypass React/Vue value setters.
    • setTimeout(0)-await microtask flush after mutations.
    • Focus-before-keypress via DOM.resolveNode.
  4. EventBuffer changes here look identical to fix(#113): 120s timeout hang in /auth/extract due to unread stderr pipe #126 and fix(#124): HAR recording returns 0 entries - add response_body via getResponseBody #127 — those should probably be a single PR. If you rebase, can you split this into a feat/dom-mutation-flush-after-action PR that only touches server/router.zig, and let fix(#124): HAR recording returns 0 entries - add response_body via getResponseBody #127 own the EventBuffer + HAR-bodies changes?

If you rebase + split, ping me and I'll re-review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants