fix(fork): design-mode requests name untagged elements and stop stacking stale ones

[NoahHendrickson]

A review pass over design mode's accuracy, token cost and responsiveness turned up two ways a request could misrepresent what the user drafted, and three places the host and guest talked more than they needed to.

Accuracy

Untagged elements shipped an address they never got. An element with no source tag rendered as (no source tag — locate by selector/text) — and then never got a selector. Every ElementChange already carries a sanitized cssPath (request.ts's elementContext); it just stopped at the guest. On a page with no source mapping at all (protocol.ts's selector-only mode) that is every element, so the whole request arrived addressed by nothing but text and classes. It is printed now, and only where there is no file:line:col to beat it.

Send appended a pill per press. buildSend builds from all of a tab's live drafts rather than from the selection, so a second Send was always a strict superset of the first, and a Send → Discard → Send left the earlier pill asking for changes the user had thrown away. Both rode the outgoing message, so the agent got overlapping — sometimes contradictory — asks. A Send now replaces that tab's pill. The key is the tab, not the thread, so two preview tabs still contribute one each.

Responsiveness

emitSelection had no change gate, though it fires on every selection change, every draft-sync flush (so every 300ms through a scrub) and every late source resolution — usually with a byte-identical snapshot of ~45 computed properties per element. Each one crossed the console bridge into the host's one ungated store setter and re-rendered all seven panel sections. It now carries the same lastJson gate LayersSession already uses, reset on deactivate so it cannot desync from the host's own selection reset.

The bridge re-queried the DOM per command. findPreviewWebview ran a document-wide querySelectorAll on every call, which during a scrub is per frame. Cached per tab id, revalidated on isConnected.

The transcript extractor ran its regex unconditionally over every user message's full text, on every render of that row. A literal includes bail answers for the ~all of them that carry no block.

Notes

• No visual change, so no before/after images. The one user-visible behavior change is the pill replacing rather than stacking.
• Deliberately not touched: the scrub path still sends one executeJavaScript per pointermove. Chrome already coalesces pointermove to frame rate, so the mechanism may cost nothing real — that is worth a profile before adding a coalescer, rather than machinery on suspicion.
• .fork/customizations.yaml's fork-design-mode intent gains the replace-not-stack clause, since a sync that reverted it would bring the bug back silently. All touched files were already registered; no new files, shadows or watches.

Verification

Both TS projects clean (web plus the engine island's own tsconfig), 223 fork guards green across 29 files, lint clean on the changed scope.

Model: Claude Opus 5 (1M context). Harness: Claude Code, driven from T3 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

Verdict: approval bar not met.

The five fixes are the right size for the problems they name, and most of the diff is direct. Two design issues keep it under the bar: the replacement key is named like the other tab id on this surface, and “replace” still delete+creates a new attachment identity, which fights the composer chip model.

What is fine

• Selector print in request.ts: correct layer, one branch.
• emitSelection stringify gate matching LayersSession, reset on deactivate.
• findPreviewWebview cache with isConnected + attribute revalidation.
• Transcript includes early-bail.
• No spaghetti bolted into shared upstream paths.
• headlessMode.ts at 995 — under the 1k line, watch only.

Not approved until the two blockers below land. After rename to runtimeTabId and stable-id upsert, this clears the bar.

  

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

[cursor]

Boundary / type-contract (blocker). The store adds a field/param named tabId, but the only caller passes runtimeTabId.

ForkDesignPanel already has a distinct tabId prop (preview-session / viewport identity). PreviewPanel wires both: runtimeTabId={runtimeTabId} and tabId={activeTabId} — they are not the same concept. designModeStore correctly keys by runtimeTabId.

Naming the replacement key tabId invites the next edit to pass the wrong id and silently break replace semantics (or merge two tabs’ pills).

Remedy: Rename the field and add parameter to runtimeTabId. Keep the “one pill per preview webview” comment, but use the name the rest of design-mode already uses.

[cursor]

Missed model simplification (blocker). The new invariant is “at most one pending request per (thread, runtimeTab).” This still filters then appends a freshly minted id — delete+create, not replace.

ForkComposerDesignChanges keys chips with key={entry.id} and colors via chipFill(entry.id) (intentionally stable per id so neighbor removal never recolors). Minting a new id on every Send remounts the chip and recolors it. That fights the composer’s own stable-identity model.

Remedy: Upsert with stable identity — reuse existing?.id when the same runtimeTabId already has a pill. Nested Record<thread, Record<runtimeTabId, Pending>> is optional; the move that deletes the remount/recolor class of bugs is stable upsert. Do not keep array+filter while minting new ids.

[cursor]

Call-site evidence for the naming blocker. This passes runtimeTabId into a parameter named tabId. The panel also receives a separate tabId prop (used for canvas/viewport). Once the store renames to runtimeTabId, this call site becomes self-documenting and the wrong-prop footgun goes away.

[NoahHendrickson]

Code review

Reviewed the five fixes against the code they touch. Four of them are clean and I'd take them as-is. The fifth — replace-not-stack — is right for the case it names but loses work in a case it doesn't.

What holds up

• Selector print (request.ts). Correct layer, one branch, and gated to untagged elements so tagged ones pay nothing. Safe against the injection threat model the surrounding comments care about: selector goes through sanitizeInline at elementContext, so backticks are stripped and whitespace is collapsed before it reaches the code span. The selector-only claim checks out — cssPath is populated unconditionally, so on those pages this is the difference between an addressable request and an unaddressable one.
• Transcript fast path. includes("<design_change_request>") matches exactly the literal the regex requires — no case-insensitivity, no optional whitespace, no attributes — so the bail can't skip a real block. Free win on every user row.
• emitSelection gate. Traced the reset for a host/guest desync and found none: every setEnabled wipe on the host side is driven by a guest setActive flip that also resets the field, and the injection ordering doesn't leave a restored selection stranded behind a primed gate. The gate also can't stick — ids are WeakMap-stable and a scrubbed-back draft keeps its record, so values still differ. (One comment inaccuracy inline.)
• Webview cache. Revalidation is right; it just never evicts. Inline.

The one I'd change before merging

buildSend's "strict superset" guarantee is a same-document guarantee. Drafts are re-located per document and dropped when they don't resolve, so after a preview navigation a Send carries only the new page's asks — and still replaces the old pill. Tweak the header on /, Send; navigate to /settings, tweak, Send, and the header asks are gone with one pill quietly becoming another. Detail and three options inline on designChangeDraftStore.ts.

Agreeing with the bot

Cursor's two blockers both check out independently:

• Naming. runtimeTabId is the right value — previewRuntimeTabId folds in serverEpoch, so a bare tabId (unique only within one server process) could collide across a restart and replace a different tab's pill. That makes the fix a rename, not a value change.
• Stable id. Confirmed at the consumer: ForkComposerDesignChanges uses key={entry.id} and chipFill(entry.id), with chipFill's own comment saying the id keying exists so neighbor removal never recolors. Minting a fresh id per Send remounts and recolors the chip on every re-send. Reusing existing?.id on the replace fixes both, and stacks fine with whatever you decide on the navigation case.

Notes

• Deliberately-not-touched scrub coalescer: agreed, and worth adding that the host-side pointermove path is the one that did deserve the cache in this PR, which is the cheaper half of the same concern.
• Test coverage for the new behavior — inline on the manifest.
• I reviewed statically; this container has no installed deps, so I did not re-run the suites the PR body reports. Check is green on the head SHA; Test was still running.

Reviewed by Claude Code.

---

Generated by Claude Code

[NoahHendrickson]

The "strict superset" premise doesn't survive a preview navigation — and this is the one case where replacing loses work.

The justification for replacing is that buildSend builds from all of the tab's live drafts, so a re-send always covers the earlier one. That holds while the guest stays on the same document. It stops holding the moment the preview navigates:

• Drafts are persisted by dcSource/selector and re-located against the current document (lifecycle-store.ts locateBySource).
• drainPendingRestore keeps unresolvable entries pending and scheduleRestoreRetry drops them after ~12s (headlessMode.ts:527-539). They never enter this.drafts.
• Cross-origin is even more clear-cut: sessionStorage doesn't follow, so the restore has nothing to drain.

So buildSend after a navigation returns only the new page's asks, and this filter still throws away the old pill. Concrete flow:

1. Design mode on /, tweak the header, Send → pill A (header changes).
2. Navigate the preview to /settings, tweak a card, Send → pill A is replaced by pill B.
3. Send the message. The header asks are gone, and the user's only signal was one pill quietly becoming another.

Before this PR both pills rode the message. That was noisy in the same-page case the PR is fixing, but correct here.

Options, cheapest first:

• Replace only when the new payload's element set covers the pending one (compare elements by sourceLabel/tag+delta), append otherwise. Keeps the fix, drops the loss.
• Key the replacement on tab + page, so a navigation starts a new pill.
• Or accept it and make it visible — say "replaced" in the toast rather than "attached", and record the navigation caveat in the manifest intent next to the replace-not-stack clause.

I'd take the first. Whichever way you go, the invariant as written in the comment above ("never from the selection, so a re-send is a strict superset") is stated more strongly than the code can guarantee, and the next reader will trust it.

---

Generated by Claude Code

[NoahHendrickson]

The cache itself is right — revalidating on isConnected plus the attribute recheck covers the remount and id-reuse cases, and the per-pointermove applyDraft → fire → findPreviewWebview path is real (DesignPanelFields.tsx:124 scrubs straight through it), so the win is genuine.

One gap: entries are only evicted on a lookup miss, and nothing ever looks up a closed tab. Close a preview tab and its detached <webview> (whole subtree) stays pinned in this module-global Map for the life of the renderer. Grows one entry per preview tab the session ever opened; a desktop session runs for days.

designModeStore.remove looks like the natural hook but it has no call sites today, so the cheapest real one is ForkPreviewDesignMode's effect cleanup — it already owns the tab id and runs on unmount:

return () => {
  if (retryTimer !== null) window.clearTimeout(retryTimer);
  webviewByTabId.delete(runtimeTabId);   // ← re-found on the next lookup if the tab is still live
  ...
};

Harmless on a plain tab switch (the next findPreviewWebview just re-queries once), and it bounds the map to live tabs.

---

Generated by Claude Code

[NoahHendrickson]

The gate is correct — I traced the reset for desync and couldn't find a hole. designModeStore.setEnabled wipes selection on every enabled flip, but every one of those flips is driven by a guest setActive change that also resets this field, and the injection path's store.setEnabled(true) early-returns once the guest's own state message has already flipped it, so a restored selection isn't wiped behind a primed gate. The gate also can't stick: registry ids are WeakMap-stable, and a css draft scrubbed back to its original keeps its draft record (drafts.ts:152-156), so values still differ frame to frame.

Small correction to the comment, though: "on every draft-sync flush (so every RIPPLE_DEBOUNCE_MS through a scrub)" isn't what happens. drafts.onChange clears and re-arms draftSyncTimer on every change (line 211-212), so it's a pure debounce — during a continuous scrub it fires zero times, only once ~300ms after the scrub stops. flushDraftSync's own comment two screens down says exactly that ("Riding the debounce keeps scrub bursts at zero extra bridge traffic"), so the two now contradict.

Doesn't change the code — the real wins are repeated selection emits and late source resolutions, which is plenty. But comments are load-bearing documentation in this module, and the PR body carries the same claim.

---

Generated by Claude Code

[NoahHendrickson]

The stated reason for adding this clause is that "a sync that reverted it would bring the bug back silently" — but a manifest intent is prose. Nothing fails if the behavior goes away.

designChangeDraftStore.ts has no test file, and forkDesignMode.test.ts only greps for the forkDesignChanges import fence, so a future merge could restore [...pending, entry] with all 223 guards green. Three pure-node tests in the add reducer would actually hold it (the store is plain zustand, no DOM):

• two adds with the same tab id → one entry;
• two adds with different tab ids → two entries, both preserved;
• add → remove → add → one entry.

Same argument for extractTrailingDesignChanges: the includes fast path is correct today (the literal matches exactly what the regex requires — no case-insensitivity, no optional whitespace), but one test asserting the fast path and the regex agree on a block-carrying prompt would keep it that way if either side is edited.

Given #13's precedent — a guard that couldn't fail is decoration, and the mutation test is the one that matters — this feels like the same gap.

---

Generated by Claude Code

[NoahHendrickson]

All six findings verified against the source and addressed in a6d7741 — with one deliberate divergence on the navigation case, explained below.

Taken as written

• Naming (tabId → runtimeTabId). Confirmed the footgun: ForkDesignPanel receives both runtimeTabId and tabId (PreviewPanel passes tabId={activeTabId}), so the store's param was named after the wrong one of two ids in scope. Renamed, with the "server tab ids are unique only per server process" reasoning recorded on the field.
• Stable id. Confirmed at the consumer — chipFill's own comment says the id keying exists so removal never recolors neighbors, and minting per Send defeated exactly that. Replacement now reuses existing?.id, and holds the entry's position too, so an in-place update doesn't jump the chip to the end of the row.
• Cache eviction. Real: a miss was the only eviction and nothing looks up a closed tab. Added forgetPreviewWebview, called from ForkPreviewDesignMode's effect cleanup as suggested.
• Comment inaccuracy. You're right and my PR body carried the same error. drafts.onChange clears and re-arms draftSyncTimer on every change, so it's a trailing debounce — zero flushes during a scrub, one ~300ms after. The comment now names the real wins (re-selecting the same element, a size-mode pick landing on the current value, a discard with nothing to discard, late source resolutions) and explicitly says it is not a scrub-rate saving.
• Tests. Nine of them, pure node. The reducer's rules, plus the fast-path/regex agreement including a present-but-not-trailing block. Also extended the payload-decode guard, since pageUrl is now required.

The navigation case — same diagnosis, different fix

Your analysis is right and it's the most valuable finding here: the "strict superset" premise is a same-document guarantee, drafts are re-located per document and dropped when they don't resolve, and replacing on tab alone silently drops the previous page's pill.

I went with your second option (key on tab + page) rather than the first (coverage check), because coverage reopens the other bug this PR exists for:

Send on /, Discard all, make different edits, Send.

The new payload is disjoint from the pending pill, so a coverage test appends — leaving pill A asking for changes the user explicitly threw away. That was half of the original finding. Page identity discriminates both cases: same page → supersedes (superset or post-discard correction), different page → append.

pageUrl rides the payload from the guest's location.href rather than anything host-side, because SPA route changes are pushState — the webview's src never changes, but the drafted elements unmount and drop out of the request just the same. It is compared, never rendered, and never reaches the agent.

Made it required, so DESIGN_MODE_PROTOCOL_VERSION goes 3 → 4. An older engine would otherwise return a payload that parses fine and silently replaces across navigations — the exact bug — so this is precisely the case the version stamp and boot()'s rebuild exist for.

Where it's imperfect, it errs toward appending: a URL that changes without unmounting anything (a hash anchor) gets one duplicate pill, which is the pre-PR behavior. A duplicate ask is visible and recoverable; lost work isn't.

Manifest

The intent clause now records both halves of the key and why, since a sync that dropped the page half would reintroduce silent work loss. The nine tests are what actually hold it — agreed that prose alone was decoration.

Verification: both TS projects clean, 249 tests green across 34 files, lint clean on the changed scope.