Skip to content

fix(live): land valid TSX through wrap → preview → accept → carbonize#118

Merged
pbakaus merged 8 commits intomainfrom
feat/live-jsx-wrap-and-carbonize
Apr 29, 2026
Merged

fix(live): land valid TSX through wrap → preview → accept → carbonize#118
pbakaus merged 8 commits intomainfrom
feat/live-jsx-wrap-and-carbonize

Conversation

@pbakaus
Copy link
Copy Markdown
Owner

@pbakaus pbakaus commented Apr 28, 2026

Closes #114.

Summary

Three orthogonal bugs in live mode for Vite React/TSX, all triggered when the picked element lives inside a component with sibling branches.

  • JSX wrapper insertion produced invalid TSX — replacing one picked JSX child with [comment, <div>, comment] yields three adjacent siblings, which oxc rejects. A Fragment <></> solves adjacency but breaks cloneElement-using parents (Radix asChild, Headless UI) with "Invalid prop supplied to React.Fragment." Fix: keep the wrapper <div data-impeccable-variants> as the single JSX-slot child and tuck both marker comments inside it. accept/discard now expands its replacement range to include the wrapper's <div> open/close via div-depth tracking.
  • Carbonize produced nested template literals in TSX <style>extractCss captured {`` and `` } lines from the agent's `<style>{\`…\`}</style>`, then `handleAccept` re-wrapped, producing `<style>{\`{\`@scope…\`}\`}</style>` which oxc rejects. Fix: strip leading `{ and trailing `} from extracted content (own line or attached to first/last CSS line) so re-wrapping always yields exactly one pair.
  • Ambiguous source matching for repeated JSX branchesfindElement returned first substring match, so picking the second of three identical <aside className="card"> siblings rewrote the first. Fix: live-wrap.mjs accepts --text TEXT (the picked element's textContent), collects all candidates, narrows by tag-stripped substring match, returns element_ambiguous + candidates[] when multiple branches match equally. Falls back to first-match when source uses dynamic content (<h1>{title}</h1>) so existing flows aren't broken. The fake e2e agent now forwards event.element.textContent automatically and live.md tells real agents to do the same.

Test plan

  • bun run test — 186/186 unit + static fixture checks pass
  • bun run test:live-e2e — 21/21 fixtures pass (20 prior + new vite8-react-tsx-repeated-aside)
  • New e2e fixture exercises the user's exact repro shape: three identical <aside> branches, picks the second card, runs wrap → Go → cycle → accept → carbonize on real Vite + TSX dev server, asserts Hero One and Hero Three survive untouched (proves wrap disambiguated).
  • Six new unit tests across live-wrap.test.mjs and live-accept.test.mjs cover the wrapper layout, leading/trailing template-literal placements, --text disambiguation, dynamic-content fallback, and the element_ambiguous error shape.
  • Existing Radix Dialog regression (bug: live picker is not clickable inside Radix portal dialogs #113) still passes — Fragment-free wrapping plays nicely with DialogPrimitive.Title asChild.

🤖 Generated with Claude Code


Note

Medium Risk
Touches live-mode source rewriting and parsing logic (live-wrap/live-accept), so mistakes could corrupt edited files or fail accepts/discards, but changes are localized and add explicit ambiguity errors and safer parsing.

Overview
Improves live-mode reliability for React/TSX sessions by changing live-wrap.mjs to emit a JSX-valid single-child wrapper (marker comments moved inside the wrapper) and updating live-accept.mjs to replace/discard the entire wrapper via an expanded replacement range that correctly matches multi-line JSX <div> tags.

Adds --text disambiguation to wrapping: the CLI can now narrow repeated class/tag matches using the picked element’s textContent, and returns a structured element_ambiguous error with candidate line ranges when it still can’t choose safely.

Hardens accept/carbonize parsing for TSX by stripping JSX template-literal wrappers from extracted <style> CSS to avoid nested {} wraps on repeated accepts, and fixes minor live tooling issues (CSP meta patching preserves self-closing tag whitespace; screenshot canvas background now defaults to white when page background is transparent).

Reviewed by Cursor Bugbot for commit 1f760af. Bugbot is set up for automated code reviews on this repo. Configure here.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Apr 28, 2026

Deploying impeccable with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1f760af
Status: ✅  Deploy successful!
Preview URL: https://ba78dfac.impeccable-2rv.pages.dev
Branch Preview URL: https://feat-live-jsx-wrap-and-carbo.impeccable-2rv.pages.dev

View logs

Comment thread .agents/skills/impeccable/scripts/live-accept.mjs
Closes #114.

Three orthogonal bugs that surfaced together when live mode picked an
element inside a Vite React/TSX component with sibling branches:

1. JSX wrapper insertion produced invalid TSX
   - Replacing a single picked JSX child with [comment, <div>, comment]
     yields three adjacent siblings, which oxc rejects with "Adjacent
     JSX elements must be wrapped in an enclosing tag."
   - A Fragment `<></>` solves the adjacency case but breaks
     `cloneElement`-using parents (Radix `asChild`, Headless UI, etc.)
     with "Invalid prop supplied to React.Fragment."
   - Fix: keep the wrapper `<div data-impeccable-variants="ID">` as the
     single JSX-slot child and tuck both marker comments INSIDE it.
     accept/discard now expands its replacement range to include the
     wrapper's `<div>` open/close lines via div-depth tracking.

2. carbonize produced nested template literals in TSX `<style>`
   - extractCss captured `{` / `` `} `` lines from the agent's existing
     `<style>{`…`}</style>` template, then handleAccept re-wrapped with
     another pair, producing `<style>{`{`@scope…`}`}</style>` which oxc
     rejects with "Expected `}` but found `@`".
   - Fix: extractCss now strips a leading `{` and trailing `` `} ``
     wherever they appear in the captured content (own line OR attached
     to the first/last CSS line), so re-wrapping always yields exactly
     one `{` ` … ` `}` pair.

3. Ambiguous source matching for repeated JSX branches
   - `findElement` returned the first substring match. Multiple
     `<aside className="card">` siblings all matched the same query, so
     wrap silently landed on the first regardless of which one the user
     picked.
   - Fix: live-wrap accepts `--text TEXT` (the picked element's
     textContent), collects ALL candidates via `findAllElements`, and
     narrows by a tag-stripped, JSX-expression-stripped substring match.
     Returns `element_ambiguous + candidates[]` when multiple branches
     match equally; falls back to first-match when source uses dynamic
     content (`<h1>{title}</h1>`) so existing flows aren't broken.
   - The fake e2e agent now forwards `event.element.textContent` to
     wrap, and live.md tells the agent to do the same.

Test coverage:
- New `vite8-react-tsx-repeated-aside` e2e fixture: three identical
  `<aside>` branches, picks the second card's <h1>, runs the full
  wrap → Go → cycle → accept → carbonize cycle on a real Vite + TSX
  dev server, asserts that Hero One and Hero Three survive untouched
  (proving wrap landed on the correct branch).
- Six new unit tests across live-wrap.test.mjs and live-accept.test.mjs
  covering the Fragment-replacement design, both leading/trailing
  template-literal placements, --text disambiguation, the dynamic-
  content fallback, and the element_ambiguous error shape.
- New `runtime.assertSourceContains` fixture hook so other regression
  fixtures can assert sibling-branch survivability cheaply.

All 186 unit + static-fixture tests pass; all 21 live e2e fixtures
(20 prior + new TSX) pass with no console errors.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@pbakaus pbakaus force-pushed the feat/live-jsx-wrap-and-carbonize branch from 3f3c977 to 54d9f05 Compare April 28, 2026 20:23
Paul Bakaus and others added 5 commits April 28, 2026 15:03
…hitespace

While driving the new live loop end-to-end against the repeated-aside
fixture, --text disambiguation silently fell back to first-match instead
of landing on the picked card.

Root cause: `el.textContent` concatenates child text nodes without
inserting whitespace, so `<h1>Hero Two</h1><p>Second card body copy.</p>`
reads as "Hero TwoSecond card body copy." — but the source has whitespace
between </h1> and <p>. The single-space normalization on both sides
missed the join boundary; substring comparison failed; filterByText
returned [] and the caller fell through to first-match.

Fix: filterByText now compares both single-space AND no-whitespace
normalizations on each side, accepting the candidate if EITHER matches.
Bumped the minimum-target-length threshold from 6 to 8 to compensate
for the slightly looser comparison.

Plus two doc clarifications surfaced during the same session:

- live.md now warns that variant CSS using bare `:scope { ... }` styles
  the variant wrapper div, not the picked element. Always use a
  descendant combinator (`:scope > .card`, `:scope .hero-title`, etc.) —
  the fake test agent's CSS is the canonical template.
- live.md documents the agent-side abort path. Aborting an in-flight
  generate via `live-accept --discard` only mutates source — the browser
  bar stays in GENERATING forever. Use `live-poll --reply EVENT_ID error
  "msg"` instead so the browser receives the error SSE and resets.

Test coverage:
- New unit test in live-wrap.test.mjs covering the textContent-without-
  inter-element-whitespace shape (three identical <aside> branches each
  with <h1> + <p>, picks the second by --text).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ading

Same alpha-string trap pattern as the recent detectPageTheme fix, on a
different code path. resolveCanvasBackground walks parents looking for
an opaque background; on a page that doesn't set its own bg the loop
runs out and fell through to:

  return getComputedStyle(document.body).backgroundColor
    || getComputedStyle(document.documentElement).backgroundColor
    || '#ffffff';

`getComputedStyle(body).backgroundColor` for a default-bg page returns
the literal string "rgba(0, 0, 0, 0)" — non-empty, truthy — so the `||`
chain short-circuits to transparent-black instead of falling through to
'#ffffff'. modern-screenshot then composites the capture onto a black
canvas; the WebGL shader overlay flashes solid black until the shader
finishes loading.

Fix: drop the buggy fallback. The while-loop already covered <body> and
<html>; if neither is opaque the only sensible answer is the browser's
default canvas color (white).

Test coverage:
- New tests/live-browser-regression.test.mjs pins the anti-pattern
  with a static-source check (live-browser.js is an IIFE with no module
  exports, so this is the cheapest reliable regression guard). Also
  pins the equivalent guard for detectPageTheme's readOpaque helper
  added in the prior commit.
- Wired the new test file into `bun run test`'s explicit list.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…iew)

Cursor Bugbot caught this on PR #118 review:

> JSX discard/accept restores content with wrong indentation. In the JSX
> path, `indent` is captured from `lines[block.start]` — the marker comment
> line inside the wrapper div, which is indented 2 extra spaces relative
> to the original element. But `expandReplaceRange` expands the replacement
> to include the outer `<div data-impeccable-variants>` wrapper, which sits
> at the original element's indent level. `deindentContent(original, indent)`
> restores content to the marker's deeper indent, so all restored lines end
> up 2 spaces deeper than the original element was.

I'd actually noticed the symptom during the live testing session ("some
odd indentation in card-2 after discard") and dismissed it as cosmetic.
Bugbot's analysis matches exactly.

Fix: anchor the deindent base on `replaceRange.start` instead of
`block.start`. For HTML the two are identical (markers sit outside the
wrapper), so HTML is unchanged. For JSX `replaceRange.start` is the
outer `<div>` at the original element's indent — correct base.

Also dropped a duplicate `expandReplaceRange` call in handleAccept that
the earlier edit left orphaned.

Test coverage:
- Two new regression tests in live-accept.test.mjs:
  - `discard restores JSX content at the original indent` runs the
    real wrap CLI and asserts the restored <aside> opener lands at
    its original 6-space indent (was 8 before the fix).
  - `accept (no carbonize, raw HTML) restores at the original indent
    on JSX` exercises the same anchor on the accept path.
- Inner-element indent loss inside the wrapped content (`<h1>` ending
  up at the same indent as its parent `<aside>`) is a separate,
  pre-existing wrap behavior — left for a follow-up; explicitly
  noted in the test comments.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Companion to the prior outer-indent fix. live-wrap.mjs's
`originalLines.map(l => indent + '    ' + l.trimStart())` calls
`trimStart()` on every line, which strips ALL leading whitespace and
collapses multi-line picked elements to a uniform indent. So a 6/8/6
shape like

    <aside className="card">
      <h1 className="hero-title">Hero</h1>
    </aside>

was being reindented to 10/10/10 inside the wrapper, and on
accept/discard the round-trip restored 6/6/6 — the <h1> ended up at
its parent's depth instead of nested inside it.

Fix: extract `minLeadingSpaces(lines)` and strip only the COMMON
minimum across the picked lines before reindenting under the wrapper.
That mirrors how `deindentContent` on the accept side already works,
so wrap+accept now form a clean round-trip.

Test coverage:
- Expanded the indent regression test in live-accept.test.mjs to
  also assert the inner `<h1>` at 8-space indent and the closing
  `</aside>` at 6 — proving the relative depth survives wrap and
  discard end-to-end.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sanity-check on the live-inject unwrap path turned up a real round-trip
bug on HTML files that ship a `<meta http-equiv="Content-Security-Policy"
content="..." />` tag (the leading space before `/>` is the canonical
self-closing form).

Trace:
- The tag-finder regex (`<meta\s+([^>]*?)\/?>`) captures any whitespace
  between the last attribute and the closing `/>` as part of `attrs`.
- patchCspMeta did `attrs.replace(content, newContent) + ' ' + marker`,
  appending the marker AFTER that captured trailing whitespace. Result:
  `...content="..."  data-...="..."` — a double space inside attrs and
  the original space-before-slash gone.
- revertCspMeta then strips the marker via `\s*${origAttr.full}`, which
  greedily eats both spaces — so the round trip leaves `"/>` with no
  space, even though the original was `" />`.

Fix: split off the trailing whitespace from `attrs` before patching,
splice the marker into the attribute body with a single leading space,
and re-append the original trailing whitespace. The marker-removal
regex then consumes exactly one space and the trailing space rides
through unchanged.

Test coverage:
- New `round-trips through CSP-meta patch and revert` test in
  live-inject.test.mjs covers the canonical Vite shape (CSP meta with
  ` />`).
- Plus a `round-trips with insertAfter` test for symmetry — the existing
  suite only covered insertBefore.
- Existing 4 round-trip tests (HTML, JSX layout, multi-file, column-0)
  all still pass byte-for-byte.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread .agents/skills/impeccable/scripts/live-wrap.mjs Outdated
Comment thread .agents/skills/impeccable/scripts/live-wrap.mjs
Two more Cursor Bugbot findings on commit 11dfad8:

1. `filterByText` short-text returned the wrong sentinel value.
   When the trimmed snippet was shorter than 8 chars, the function
   returned `candidates.slice()` (all candidates). The caller then
   sees `filtered.length > 1` and fires `element_ambiguous` — exactly
   the opposite of the documented short-text fallback ("caller falls
   back to first-match," which corresponds to `filtered.length === 0`).
   So any picker event with a short textContent on a page with multiple
   matching siblings spuriously errored.
   Fix: return `[]` for short text, matching the JSDoc.

2. `endLine` in the wrap output was wrong for multi-line picked elements.
   `wrapperLines.length` counts ARRAY elements, but one element is a
   `\n`-joined multi-line string (originalIndented). The actual
   wrapper-region row count is `wrapperLines.length + (originalLines.length
   - 1)`. Reporting `endLine = startLine + wrapperLines.length` placed
   the boundary inside the wrapper for any multi-line pick, giving
   downstream agents an incorrect range.
   Fix: add the originalLines offset (matching what `insertLine` already
   does after the prior commit).

Test coverage:
- `short --text falls back to first-match instead of erroneously firing
  element_ambiguous` covers fix #1.
- `returns endLine that includes the multi-line original content offset`
  covers fix #2 by wrapping a 5-line <section> in a real HTML file and
  asserting the reported endLine points at the variants-end marker (and
  the next line is </main>, proving no rows were missed).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 8660d3a. Configure here.

Comment thread .agents/skills/impeccable/scripts/live-accept.mjs
…v />

Cursor Bugbot review on 8660d3a flagged a real corruption bug:

> Multi-line self-closing div breaks depth tracking in expandReplaceRange.
> The forward div-depth walk applies openRe / selfCloseRe / closeRe
> per-line. A multi-line `<div\n  className="spacer"\n/>` causes openRe
> to match the opener line but selfCloseRe fails on both lines because
> `/<div\b[^>]*\/\s*>/` requires the full tag on one line. Depth is
> permanently over-counted by 1, so the walk overshoots.

Trace on the JSX-marker-inside-wrapper layout:
- Inside the wrapped element, a multi-line `<div … />` increments depth
  at the `<div` line and never decrements.
- Forward walk's depth never returns to 0 → end stays at block.end (the
  inner marker comment) → replace range stops there.
- Wrapper's outer `</div>` is left orphaned in the file after
  accept/discard, breaking the JSX. Worse: an unrelated subsequent
  `<div className="next-card">…</div>` sibling gets its `</div>`
  mis-counted as the wrapper close, and the depth walk corrupts further.

Fix: rewrite the forward walk on JOINED text instead of per-line. A
single regex `/<div\b[^>]*?(\/?)>|<\/div\s*>/g` spans newlines (because
`[^>]` matches `\n`), so it correctly identifies multi-line opens,
closes, AND self-closes. Convert the match offset back to a file line
index to set `end`. Walk-back logic for the wrapper opener is
unchanged.

Test coverage:
- New `expandReplaceRange handles multi-line self-closing <div />` test
  in live-accept.test.mjs constructs the exact Bugbot scenario: a
  multi-line `<div\n  className="spacer"\n/>` inside the picked
  element AND an unrelated `<div className="next-card">After</div>`
  sibling right after. Asserts the discard removes ALL impeccable
  markers / wrapper attrs, preserves the next-card sibling intact, and
  the multi-line `<div />` survives inside the restored content.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@pbakaus pbakaus merged commit c1e1104 into main Apr 29, 2026
3 checks passed
@pbakaus pbakaus deleted the feat/live-jsx-wrap-and-carbonize branch April 29, 2026 00:41
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.

bug: live mode writes invalid TSX when wrapping and accepting React variants

1 participant