fix(live): land valid TSX through wrap → preview → accept → carbonize#118
Merged
fix(live): land valid TSX through wrap → preview → accept → carbonize#118
Conversation
Deploying impeccable with
|
| 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 |
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>
3f3c977 to
54d9f05
Compare
…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>
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>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ 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.
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

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.
[comment, <div>, comment]yields three adjacent siblings, which oxc rejects. A Fragment<></>solves adjacency but breakscloneElement-using parents (RadixasChild, 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.<style>—extractCsscaptured{`` 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.findElementreturned first substring match, so picking the second of three identical<aside className="card">siblings rewrote the first. Fix:live-wrap.mjsaccepts--text TEXT(the picked element'stextContent), collects all candidates, narrows by tag-stripped substring match, returnselement_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 forwardsevent.element.textContentautomatically andlive.mdtells real agents to do the same.Test plan
bun run test— 186/186 unit + static fixture checks passbun run test:live-e2e— 21/21 fixtures pass (20 prior + newvite8-react-tsx-repeated-aside)<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).live-wrap.test.mjsandlive-accept.test.mjscover the wrapper layout, leading/trailing template-literal placements,--textdisambiguation, dynamic-content fallback, and theelement_ambiguouserror shape.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.mjsto emit a JSX-valid single-child wrapper (marker comments moved inside the wrapper) and updatinglive-accept.mjsto replace/discard the entire wrapper via an expanded replacement range that correctly matches multi-line JSX<div>tags.Adds
--textdisambiguation to wrapping: the CLI can now narrow repeated class/tag matches using the picked element’stextContent, and returns a structuredelement_ambiguouserror 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.