perf(fork): a design-mode scrub crosses into the guest once per frame

[NoahHendrickson]

Every pointermove tick of a design-panel label scrub (and every repeat of a held arrow key) became its own JSON-serialize + executeJavaScript crossing into the preview guest — the one per-frame process boundary left in the design-mode edit loop, flagged in the Cursor design-mode research (DESIGN-MODE-CURSOR-RESEARCH.md, Speed §1). The guest already coalesces repaints to rAF, so within one host frame only the newest value of a gesture can ever be observed.

applyDraft and setInset now coalesce host-side in designModeBridge.ts: at most one pending call per (tab, member, target), flushed on the next animation frame carrying the newest value. Every other bridge command flushes the queue synchronously ahead of itself — as does buildSend — so a revert or send issued right after a scrub tick still lands after it and observable order is unchanged. A frame stranded by a hidden window flushes on whatever command comes next.

Verification

New colocated designModeBridge.test.ts (5 cases: same-target coalescing keeps the newest value, distinct targets flush in first-seen order, discrete commands flush ahead of themselves with no replay after the canceled frame, buildSend observes pending writes, multi-select scrubs stay one crossing) with rAF and document stubbed — no DOM. Fork guard suite (227 tests), apps/web typecheck, fork lint — all clean. No visual change.

Model: Claude Fable 5, harness: Claude Code.

🤖 Generated with Claude Code

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[cursor]

Thermo-nuclear code quality review — REQUEST CHANGES

Host-side scrub coalescing at the bridge is the right layer, and the flush-ahead ordering story is sound. What is not acceptable is that the coalesce key embeds literal NUL bytes, which turns designModeBridge.ts into a git/GitHub binary blob — this PR’s own main change has patch: null / Bin 6183 → 8369, so reviewers cannot even see a text hunk. Fix that (and collapse the pending-queue shape while you’re there) before this is approvable.

1. Structural regression — NUL key makes the .ts file binary

apps/web/src/custom/designMode/designModeBridge.ts ~L90:

const key = `${runtimeTabId}\0${member}\0${target}`;

Those literal \0 separators make git/file(1) classify the source as data. Every future blame, review diff, and agent pass loses the text hunk. Collision-free keys do not require NULs in a .ts file — use JSON.stringify([runtimeTabId, member, target]), a printable delimiter the alphabet cannot contain, or stop string-keying entirely (Map). Make this file text again.

2. Missed code-judo — array + linear find + mutable args

Same file ~L64–98: PendingCall carries a denormalized key, pending.find scans on every scrub tick, and upsert mutates args in place — three concepts for “latest args per identity, flush in first-seen order.” A Map<string, { runtimeTabId; member; args }> already preserves insertion order on first insert and upserts with .set, so you can delete the key field, the linear scan, and the mutable slot. Same behavior, fewer moving pieces; pairs cleanly with dropping the NUL separator.

Non-findings

• Coalescing belongs at the bridge (not callers): every scrub path already funnels through applyDraft / setInset; caller-side rAF would scatter flush duties and miss buildSend / revert ordering.
• Module-level pending / pendingFrame matches the existing WeakRef cache pattern; tests drain via discardAll.
• fire / fireNow / fireCoalesced / flushPending earns its keep (buildSend must flush without going through fire).
• File size 142 → 195 is fine; tests cover the contract.

Approval bar not met until the file is text again and the pending queue is reshaped to the simpler Map model.

  

_{Sent by Cursor Automation: Thermo-nuclear PR review}

[NoahHendrickson]

Both review points addressed in b510b64: the coalesce key joins with | instead of raw NUL bytes — the file diffs as text again (this push's own diff is the proof) — and the pending queue is now a Map keyed by that string, so insertion order is the flush order it already promised and replacing a target's args is O(1) instead of a linear scan. Bridge tests (5), web typecheck, and the fork guard suite (227) all pass.

[NoahHendrickson]

Code review — at b510b647

Reviewed against origin/custom, at the current head rather than the SHA that opened the PR. The two items from the earlier bot review are genuinely resolved by b510b647: the coalesce key is tab|member|target and the file is text again (I confirmed the blob has zero NUL bytes and the hunk renders as a normal diff), and the queue is a Map whose insertion order is the flush order. No need to revisit either.

The core design holds up, and I checked the things that would actually break it:

• Newest-wins is sound for these two verbs specifically. Both are absolute, not delta-accumulating — headlessMode.applyDraft does drafts.apply(el, property, value) and setInset does write(el, px, drafts), and the fields scrub from an absolute scrubBase rather than += delta. Dropping intermediate ticks therefore loses nothing. This is the assumption the whole PR rests on and it's correct.
• Ordering. fire and buildSend both flushPending() first; flushPending cancels the frame before draining; the rAF callback nulls pendingFrame before calling flushPending, so there's no double-cancel and no replay after a discrete command. destroy routes through fire, so toggle-off flushes before the guest is torn down.
• Tests cover the contract well — coalescing, distinct targets in first-seen order, flush-ahead-of-revert including the canceled-frame-no-replay case, flush-ahead-of-buildSend, and the multi-select key. I could not execute them (node_modules isn't installed in this environment), so that's a static read.

One finding:

Engine re-injection is the one guest-mutating path that doesn't touch the queue

ForkPreviewDesignMode.tsx:35 injects via webview.executeJavaScript(engineCode) directly, bypassing the bridge, so onDomReady (:90) never flushes or drops pending work. Before this PR that didn't matter — applyDraft fired synchronously and there was no queue to go stale. The deferral opens the window.

At dom-ready the queued calls are stale by definition: they name ids from the previous document's registry, and boot() constructs a fresh ElementIdRegistry with nextId = 1. Two outcomes:

• rAF fires before injection completes → the call hits wiped globals, ?. no-ops. Harmless.
• rAF fires after boot() → resolve() can hand back a different, still-connected element on the new page. The isConnected guard doesn't help here; new-page elements pass it. A draft then lands on an element the user never touched, persists to the guest's sessionStorage, and rides into the next Send.

Realistically the window is tight — re-injection is an IPC round-trip and usually outruns a 16ms frame — so I'd call this narrow rather than urgent. But the correct behavior doesn't depend on how tight the race is: those calls should be discarded, not raced or flushed, since flushing them into the old document is equally pointless. A dropPending(runtimeTabId) (clear the tab's keys, cancel the frame if the map empties) called at the top of onDomReady covers it and is a truer statement of intent than a flush would be.

Worth noting the same reasoning applies to boot.ts:46, where a protocol-version mismatch tears down the engine and mints a fresh registry within the same document.

---

Nothing else blocking from me. The | key separator is fine in practice — member is a fixed internal set and the id list is numeric, so a collision would need a | inside runtimeTabId. And the hidden-window stranded-frame case the header comment mentions self-heals on visibility restore, since browsers resume rAF rather than dropping the callback.

---

Generated by Claude Code