refactor(visual-editor): route every DOM change through the SDK plugin

[joalves]

Summary

• Unify the visual editor's DOM change pipeline on the SDK plugin's PreviewManager path. New helper src/visual-editor/core/sdk-applier.ts postMessages every commit through the in-page orchestrator under a dedicated __visual_editor__ experiment name with updateMode: "replace".
• VE.start takes over an active production preview (clear → re-apply under __visual_editor__); VE.stop tears down the VE-owned preview and restores production preview if it was active before launch.
• Drop the auto onPreviewToggle(true) that fired on VE launch — it was a pre-refactor mechanism that now double-applies and desyncs sidebar.previewEnabled from the page after VE.stop.
• Squash maps in undo-redo-manager and useVisualEditorCoordination key 'create' changes by index so multiple inserts against the same anchor don't collapse onto each other.
• PreviewManager tracks createdElementsMap so 'create' changes round-trip cleanly across preview off/on.
• element-actions sheds ~580 lines of legacy direct-DOM handling now that the SDK owns the apply path.

Test updates

• ve-discard switches its target to #main-title — saved changes now include a real delete on #test-paragraph (SDK semantics), so the paragraph is gone during VE editing.
• ve-verification.verifyChangesAfterVEExit waits for the banner to detach before sampling DOM state. saveChanges schedules VE.stop behind setTimeout(500) and debugWait is a no-op in headless, so without an explicit wait the verify step read mid-VE state on fast runs.
• ve-actions / ve-undo-redo updated for hard-delete semantics; innerHTML reads after async SDK apply are polled instead of sampled once.
• ve-preview adds a round-trip guard that asserts inserted blocks survive a preview-off / preview-on cycle.

Test plan

• 3034/3034 unit tests pass (bun run test:unit)
• tests/e2e/visual-editor-complete.spec.ts passes 5/5 sequential headless runs (~10–12 s each)
• CI typecheck green
• CI unit-tests green
• CI build green
• CI e2e-tests green

Summary by CodeRabbit

• New Features

  • Unified "Delete" action and new "Create" insertion flow with explicit placement; Visual Editor now routes commits through the preview/apply pipeline.

• Bug Fixes

  • More reliable undo/redo and squashing so distinct inserts don't collapse.
  • Improved preview lifecycle: safer handling and cleanup of newly created elements, and restored DOM baseline capture for accurate reverts.

• Tests

  • Unit and e2e tests updated for the new change types, SDK-driven preview flow, and timing-aware DOM assertions.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

The PR standardises DOM-change actions by replacing "insert"/"remove" with "create"/"delete" across types, validation schemas, hooks, UI, tests and mocks. The Visual Editor no longer mutates the DOM directly: commits, undo and redo are squashed and routed through a new SDK postMessage bridge (sdk-applier) to PreviewManager, which tracks and cleans up created elements. ElementState now captures parent/nextSibling for reliable restores. Multiple VE modules, undo/redo squashing keys, and end-to-end tests were updated to align with the new change vocabulary and SDK-driven preview/apply flow.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• ci: verify full E2E suite after fixture fix #20: Updates DOMChangeList rendering and change-type handling; directly overlaps the UI/type rename from remove/insert → delete/create.

Poem

🐰 I tiptoe through the DOM at dawn,
I swap "remove" for "delete" with a yawn,
I plant tiny "create" shoots where new blocks play,
Messages carry my hops across the way,
A nibble, a patch — the page wakes up today 🥕✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 61.54% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the main objective of the changeset—routing every DOM change through the SDK plugin instead of direct DOM manipulation.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/visual-editor-via-sdk-applier

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/visual-editor/core/__tests__/editor-coordinator-html.test.ts (1)

74-78: ⚠️ Potential issue | 🔴 Critical

Undefined variable toolbar will cause test failures.

The variable toolbar is not defined in this test file. Based on the PR changes, the toolbar was removed in favour of UIComponents banner, so this parameter should be null.

🐛 Proposed fix

     coordinator = new EditorCoordinator(
       stateManager,
       eventHandlers,
       contextMenu,
       undoRedoManager,
       uiComponents,
       editModes,
       cleanup,
-      toolbar,
+      null, // toolbar removed - using UIComponents banner
       notifications,
       mockCallbacks
     )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/visual-editor/core/__tests__/editor-coordinator-html.test.ts` around
lines 74 - 78, The test passes an undefined variable toolbar into the
coordinator helper, causing failures; update the call that currently includes
the undefined symbol toolbar (in the test in editor-coordinator-html.test.ts
where cleanup, toolbar, notifications, mockCallbacks are passed) to pass null
instead (since toolbar was removed in favor of UIComponents/banner), i.e.,
replace the toolbar parameter with null so the helper receives cleanup, null,
notifications, mockCallbacks.

src/lib/validation-schemas.ts (1)

154-165: ⚠️ Potential issue | 🟡 Minor

Keep the e2e applier on the same action contract.

DOMChangeSchema now rejects the old "remove" vocabulary, but tests/e2e/utils/dom-changes-applier.ts still accepts it and does not model the new "create" branch. That lets e2e pass with payloads production would refuse to parse. Please update that helper alongside this schema change.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/validation-schemas.ts` around lines 154 - 165, The test helper that
applies DOM change payloads still accepts the old "remove" action and lacks
handling for the new "create" action, causing tests to accept inputs production
would reject; update the e2e DOM changes applier helper (the dom-changes-applier
function that switches on action values) so it no longer recognizes "remove",
adds a branch for "create" that models node creation (mirror the production
logic used by DOMChangeSchema/DOMChangeActionSchema for "create"), and ensure
all switch/case or if/else comparisons align with DOMChangeActionSchema values
so tests only accept the same action contract as the schema.

🧹 Nitpick comments (4)

src/utils/__tests__/dom-change-operations.test.ts (1)

33-36: Consider renaming the helper to match delete terminology.

createMockRemoveChange now produces type: "delete". Renaming it (for example, createMockDeleteChange) would keep test intent clearer.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/__tests__/dom-change-operations.test.ts` around lines 33 - 36,
Rename the helper function createMockRemoveChange to createMockDeleteChange to
match the produced DOMChange.type "delete"; update the function declaration and
all its usages inside the test file (and any other tests) to the new name so
callers reference createMockDeleteChange, and keep the returned object shape
(selector and type: "delete") unchanged.

src/hooks/useVisualEditorCoordination.ts (1)

62-82: Extract duplicated keyFor function to avoid DRY violation.

The keyFor function is defined identically at lines 64-67 and lines 126-129. Extract it to module scope or a shared utility to avoid duplication and ensure consistent behaviour if the logic needs to change.

♻️ Suggested refactor

+const keyFor = (change: DOMChange, index: number): string =>
+  change.type === "create"
+    ? `create-${change.selector}-${index}`
+    : `${change.selector}-${change.type}`
+
 export function useVisualEditorCoordination({
   // ... props
 }: UseVisualEditorCoordinationProps) {
   useEffect(() => {
     const handleVisualEditorChanges = (
       // ...
     ) => {
       // ...
-            const keyFor = (change: DOMChange, index: number): string =>
-              change.type === "create"
-                ? `create-${change.selector}-${index}`
-                : `${change.selector}-${change.type}`
+            // Use module-level keyFor

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/hooks/useVisualEditorCoordination.ts` around lines 62 - 82, The
duplicated keyFor implementation (used to build keys for DOMChange objects)
should be extracted to a single shared function to avoid DRY; create a
standalone helper (e.g., function keyFor(change: DOMChange, index: number):
string) at module scope or shared utility and replace both in-file occurrences
so currentChanges.forEach and the message.changes.forEach both call that single
keyFor; keep the same logic and types (DOMChange) and update any imports/exports
if you move it to a utility module.

src/sdk-bridge/dom/preview-manager.ts (1)

115-127: Type cast (change as any).targetSelector suggests incomplete type definition.

The use of as any to access targetSelector on a create change indicates the DOMChange type may not fully define this property for the create variant. Consider updating the type definitions in ~src/types/dom-changes to include targetSelector as a required property on the create change type, which would provide compile-time safety.

♻️ Suggested type improvement

In src/types/dom-changes.ts, ensure the create change variant includes:

interface CreateDOMChange extends BaseDOMChange {
  type: "create"
  element: string
  targetSelector: string
  position: "before" | "after" | "prepend" | "append"
}

Then replace (change as any).targetSelector with direct property access after narrowing the type.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/sdk-bridge/dom/preview-manager.ts` around lines 115 - 127, The
create-branch uses (change as any).targetSelector which hides a missing property
on the DOMChange create variant; update src/types/dom-changes to add a
CreateDOMChange interface (e.g., type: "create", element, targetSelector:
string, position) and make the union include this variant, then in
src/sdk-bridge/dom/preview-manager.ts narrow change to the create type
(change.type === "create") and access change.targetSelector directly (remove the
as any cast) so the compiler enforces the property exists.

src/visual-editor/core/__tests__/element-actions.test.ts (1)

356-362: Assert the SDK replay count as well as the payload.

toHaveBeenCalledWith still passes if one edit emits the same PREVIEW_CHANGES twice. Adding toHaveBeenCalledTimes(1) here would catch duplicate replay regressions in the new SDK path.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/visual-editor/core/__tests__/element-actions.test.ts` around lines 356 -
362, The test currently only asserts the payload for postSpy
(expect(postSpy).toHaveBeenCalledWith(...)) which won't catch duplicate
emissions; update the test to also assert the invocation count by adding
expect(postSpy).toHaveBeenCalledTimes(1) (near the existing assertion that
references postSpy and the "PREVIEW_CHANGES" type) so the test verifies both the
SDK replay payload and that it was emitted exactly once.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/components/dom-editor/DOMChangeList.tsx`:
- Around line 260-267: The "create" switch case in DOMChangeList.tsx declares
createChange directly in the case which can leak into other cases; fix by
wrapping the case body in a block (e.g., case "create": { ... break } ) so
createChange's scope is limited to that block. Locate the switch inside the
DOMChangeList component (the case "create" branch) and enclose the const
createChange = change as any and its returned JSX inside a { } block and ensure
you include a break/return as appropriate.

In `@src/hooks/useDOMChangesEditor.ts`:
- Around line 245-255: In the "create" branch of the switch inside
useDOMChangesEditor (the case handling creation of domChange), add the missing
property triggerOnView: changeToSave.triggerOnView to the domChange object to
match other change types, or if omission was intentional, add an inline comment
above the case explaining why triggerOnView is excluded; ensure the property
name exactly matches other cases so serialization remains consistent.

In `@src/sdk-bridge/dom/element-state.ts`:
- Around line 75-87: The restore block should reposition elements even when the
parent is the same and should accept shadow DOM parents; replace the current
condition that uses document.contains(originalState.parent) and only checks
parent inequality with a combined check: compute a needsReposition boolean that
is true if originalState.parent !== element.parentNode OR
originalState.nextSibling !== element.nextSibling, and check that
originalState.parent is connected using originalState.parent.isConnected (not
document.contains). If needsReposition is true, compute sibling =
originalState.nextSibling and siblingStillThere = sibling && sibling.parentNode
=== originalState.parent, then call originalState.parent.insertBefore(element,
siblingStillThere ? sibling : null).

In `@src/visual-editor/core/element-actions.ts`:
- Around line 130-133: The action methods currently call
applyCurrentChangesViaSDK (which calls undoRedoManager.squashChanges() and
applyChangesViaSDK) while addChange/setOnChangeAdded already replays those same
changes, causing duplicate PREVIEW_CHANGES cycles; remove the extra SDK replay
by deleting calls to applyCurrentChangesViaSDK from action methods that invoke
addChange (or alternatively gate applyCurrentChangesViaSDK to no-op when replay
is already owned by setOnChangeAdded), and ensure only the setOnChangeAdded
callback performs applyChangesViaSDK using undoRedoManager.squashChanges() and
stateManager.getConfig().variantName so the SDK replay has a single owner.
- Around line 318-336: The current selection logic reads
referenceElement.previousElementSibling/nextElementSibling too early and can
pick the old sibling; instead, wait for the preview apply to complete or locate
the newly-created node by its deterministic marker and then call
this.selectElement. Concretely: after calling applyPreviewChange/PreviewManager
create branch, either (a) subscribe to the preview apply completion or
postMessage response and then compute insertedElement, or (b) query the DOM for
the marker attribute that PreviewManager sets (data-absmartly-experiment or the
specific marker used in create) to find the new element, and then invoke
this.selectElement(foundElement) (replace the current
previousElementSibling/nextElementSibling logic in the insert path that uses
referenceElement and options.position).

In `@src/visual-editor/core/sdk-applier.ts`:
- Around line 33-45: The postMessage calls in
src/visual-editor/core/sdk-applier.ts currently always use
window.location.origin as the targetOrigin which fails for file: and opaque
("null") origins; update the postMessage invocations (the two blocks that send
PREVIEW_CHANGES and CLEAR_CHANGES) to reuse the same fallback logic used in
visual-editor.ts: compute a safeTargetOrigin that is window.location.origin
unless window.location.origin === "null" or
window.location.origin.startsWith("file:"), in which case use "*" and pass that
safeTargetOrigin as the second argument to window.postMessage; ensure both
occurrences (around the PREVIEW_CHANGES send and the CLEAR_CHANGES send) are
changed consistently.

---

Outside diff comments:
In `@src/lib/validation-schemas.ts`:
- Around line 154-165: The test helper that applies DOM change payloads still
accepts the old "remove" action and lacks handling for the new "create" action,
causing tests to accept inputs production would reject; update the e2e DOM
changes applier helper (the dom-changes-applier function that switches on action
values) so it no longer recognizes "remove", adds a branch for "create" that
models node creation (mirror the production logic used by
DOMChangeSchema/DOMChangeActionSchema for "create"), and ensure all switch/case
or if/else comparisons align with DOMChangeActionSchema values so tests only
accept the same action contract as the schema.

In `@src/visual-editor/core/__tests__/editor-coordinator-html.test.ts`:
- Around line 74-78: The test passes an undefined variable toolbar into the
coordinator helper, causing failures; update the call that currently includes
the undefined symbol toolbar (in the test in editor-coordinator-html.test.ts
where cleanup, toolbar, notifications, mockCallbacks are passed) to pass null
instead (since toolbar was removed in favor of UIComponents/banner), i.e.,
replace the toolbar parameter with null so the helper receives cleanup, null,
notifications, mockCallbacks.

---

Nitpick comments:
In `@src/hooks/useVisualEditorCoordination.ts`:
- Around line 62-82: The duplicated keyFor implementation (used to build keys
for DOMChange objects) should be extracted to a single shared function to avoid
DRY; create a standalone helper (e.g., function keyFor(change: DOMChange, index:
number): string) at module scope or shared utility and replace both in-file
occurrences so currentChanges.forEach and the message.changes.forEach both call
that single keyFor; keep the same logic and types (DOMChange) and update any
imports/exports if you move it to a utility module.

In `@src/sdk-bridge/dom/preview-manager.ts`:
- Around line 115-127: The create-branch uses (change as any).targetSelector
which hides a missing property on the DOMChange create variant; update
src/types/dom-changes to add a CreateDOMChange interface (e.g., type: "create",
element, targetSelector: string, position) and make the union include this
variant, then in src/sdk-bridge/dom/preview-manager.ts narrow change to the
create type (change.type === "create") and access change.targetSelector directly
(remove the as any cast) so the compiler enforces the property exists.

In `@src/utils/__tests__/dom-change-operations.test.ts`:
- Around line 33-36: Rename the helper function createMockRemoveChange to
createMockDeleteChange to match the produced DOMChange.type "delete"; update the
function declaration and all its usages inside the test file (and any other
tests) to the new name so callers reference createMockDeleteChange, and keep the
returned object shape (selector and type: "delete") unchanged.

In `@src/visual-editor/core/__tests__/element-actions.test.ts`:
- Around line 356-362: The test currently only asserts the payload for postSpy
(expect(postSpy).toHaveBeenCalledWith(...)) which won't catch duplicate
emissions; update the test to also assert the invocation count by adding
expect(postSpy).toHaveBeenCalledTimes(1) (near the existing assertion that
references postSpy and the "PREVIEW_CHANGES" type) so the test verifies both the
SDK replay payload and that it was emitted exactly once.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: abe51817-3c42-4be8-9355-218580dbe413

📥 Commits

Reviewing files that changed from the base of the PR and between 57ae018 and 7413952.

📒 Files selected for processing (30)

• background/main.ts
• src/components/dom-editor/DOMChangeList.tsx
• src/hooks/useDOMChangesEditor.ts
• src/hooks/useVisualEditorCoordination.ts
• src/lib/__tests__/ai-dom-generator.integration.test.ts
• src/lib/validation-schemas.ts
• src/sdk-bridge/dom/element-state.ts
• src/sdk-bridge/dom/preview-manager.ts
• src/types/dom-changes.ts
• src/utils/__tests__/dom-change-operations.test.ts
• src/visual-editor/core/__tests__/editor-coordinator-html.test.ts
• src/visual-editor/core/__tests__/editor-coordinator.test.ts
• src/visual-editor/core/__tests__/element-actions.test.ts
• src/visual-editor/core/edit-modes.ts
• src/visual-editor/core/editor-coordinator.ts
• src/visual-editor/core/element-actions.ts
• src/visual-editor/core/sdk-applier.ts
• src/visual-editor/core/undo-redo-manager.ts
• src/visual-editor/core/visual-editor.ts
• tests/e2e/ai-chat-diagnostic.spec.ts
• tests/e2e/ai-dom-generation-complete.spec.ts
• tests/e2e/ai-dom-generation.spec.ts
• tests/e2e/ai-session-image.spec.ts
• tests/e2e/ai-storage-quota.spec.ts
• tests/e2e/helpers/ve-actions.ts
• tests/e2e/helpers/ve-discard.ts
• tests/e2e/helpers/ve-preview.ts
• tests/e2e/helpers/ve-undo-redo.ts
• tests/e2e/helpers/ve-verification.ts
• tests/e2e/message-bridge.spec.ts

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/visual-editor/core/editor-coordinator.ts (1)

250-265: ⚠️ Potential issue | 🟠 Major

Delete-key handler still bypasses the SDK apply pipeline with direct DOM removal.

At Line 254, selectedElement.remove() mutates the page immediately, while other flows now queue changes for SDK/PreviewManager application. This risks state-capture drift in undo/redo.

🔧 Proposed fix

       if (e.key === "Delete") {
         const selectedElement = this.stateManager.getState().selectedElement
         if (selectedElement && !this.eventHandlers.isEditingMode) {
           e.preventDefault()
-          selectedElement.remove()
           const selector = this.callbacks.getSelector(
             selectedElement as HTMLElement
           )
-          const oldValue = selectedElement.outerHTML
           this.undoRedoManager.addChange(
             {
               selector,
               type: "delete"
             },
-            oldValue
+            null
           )
         }
       }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/visual-editor/core/editor-coordinator.ts` around lines 250 - 265, The
Delete-key handler currently removes the element directly
(selectedElement.remove()) which bypasses the SDK/PreviewManager pipeline;
instead, construct the same change payload used elsewhere (use the selector from
callbacks.getSelector(selectedElement), type: "delete", and include oldValue =
selectedElement.outerHTML) and submit it through the SDK/PreviewManager apply
method (or the existing callbacks/queue API used for edits) rather than calling
selectedElement.remove(); then call undoRedoManager.addChange with that
selector/type and oldValue after queuing the change and keep the early-return
guard using eventHandlers.isEditingMode and e.preventDefault() as before.

src/visual-editor/core/visual-editor.ts (1)

510-521: ⚠️ Potential issue | 🟠 Major

Do not mark changes as saved when the save callback fails

Line 510-521 still sets hasUnsavedChanges = false and updates lastSavedChanges even if onChangesUpdate throws. That can falsely report success and restore unsent state on stop().

Suggested fix

-    try {
-      // Send squashed changes to sidebar
-      this.options.onChangesUpdate(squashedChanges)
-    } catch (error) {
-      console.error("Error in onChangesUpdate callback:", error)
-    }
-
-    // Mark changes as saved
-    this.hasUnsavedChanges = false
-    this.lastSavedChangesCount = squashedChanges.length
-    this.lastSavedChanges = squashedChanges
+    try {
+      // Send squashed changes to sidebar
+      this.options.onChangesUpdate(squashedChanges)
+    } catch (error) {
+      console.error("Error in onChangesUpdate callback:", error)
+      this.notifications.show(
+        "Couldn’t save changes",
+        "Please try again.",
+        "error"
+      )
+      return
+    }
+
+    // Mark changes as saved only after successful callback
+    this.hasUnsavedChanges = false
+    this.lastSavedChangesCount = squashedChanges.length
+    this.lastSavedChanges = squashedChanges

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/visual-editor/core/visual-editor.ts` around lines 510 - 521, The code
currently marks changes as saved regardless of whether
this.options.onChangesUpdate succeeded; update the logic so that
hasUnsavedChanges, lastSavedChangesCount and lastSavedChanges are only updated
after onChangesUpdate completes successfully (i.e., move those assignments
inside the try block immediately after
this.options.onChangesUpdate(squashedChanges)), and leave the state unchanged
when the catch path executes (so stop() and other flows will still see unsaved
changes); keep the existing catch logging for errors thrown by onChangesUpdate.

♻️ Duplicate comments (4)

src/visual-editor/core/element-actions.ts (1)

318-336: ⚠️ Potential issue | 🟠 Major

Inserted-node selection is still racing async create application.

Line 316 triggers SDK apply asynchronously, but Line 323-Line 327 immediately reads sibling pointers. That can reselect an old sibling instead of the newly created element.

💡 Suggested fix

-      this.undoRedoManager.addChange(change, null)
-      this.applyCurrentChangesViaSDK()
+      this.undoRedoManager.addChange(change, null)
+      this.applyCurrentChangesViaSDK()

-      const insertedElement =
-        options.position === "before"
-          ? referenceElement.previousElementSibling
-          : referenceElement.nextElementSibling
+      const insertedElement = await this.waitForInsertedSibling(
+        referenceElement,
+        options.position
+      )

       this.notifications.show(
         `HTML block inserted ${options.position} selected element`,
         "",
         "success"
       )

       if (insertedElement) {
         this.selectElement(insertedElement as HTMLElement)
       }

private async waitForInsertedSibling(
  referenceElement: HTMLElement,
  position: "before" | "after",
  timeoutMs = 1500
): Promise<Element | null> {
  const start = Date.now()
  while (Date.now() - start < timeoutMs) {
    const candidate =
      position === "before"
        ? referenceElement.previousElementSibling
        : referenceElement.nextElementSibling
    if (candidate && !candidate.className.includes("absmartly")) {
      return candidate
    }
    await new Promise((r) => setTimeout(r, 50))
  }
  return null
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/visual-editor/core/element-actions.ts` around lines 318 - 336, The
selection of the newly-inserted node can race with the async preview apply:
after calling the SDK apply (the create branch in applyPreviewChange) the
current code reads referenceElement.previousElementSibling/nextElementSibling
immediately and may pick an old sibling; fix by polling/waiting for the actual
inserted sibling before calling selectElement — implement a helper like
waitForInsertedSibling(referenceElement, position, timeoutMs) that repeatedly
checks previousElementSibling/nextElementSibling until a new element appears (or
timeout) and then use its result instead of the immediate insertedElement;
update the insertion flow to await waitForInsertedSibling and only call
this.selectElement(found) if non-null.

src/hooks/useDOMChangesEditor.ts (1)

245-254: ⚠️ Potential issue | 🟡 Minor

create serialisation drops triggerOnView, causing flag loss on save.

triggerOnView is propagated for other change types but omitted for type: "create" at Line 245-254.

🔧 Proposed fix

         case "create":
           domChange = {
             selector: changeToSave.selector,
             type: "create",
             element: changeToSave.htmlValue || "",
             targetSelector: changeToSave.selector,
             position: changeToSave.position || "after",
             waitForElement: changeToSave.waitForElement,
+            triggerOnView: changeToSave.triggerOnView,
             observerRoot: changeToSave.observerRoot
           }
           break

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/hooks/useDOMChangesEditor.ts` around lines 245 - 254, The "create" branch
builds domChange but omits the triggerOnView flag, so add triggerOnView:
changeToSave.triggerOnView (or default false if required) to the domChange
object created in the case "create" block so it matches the other change types;
locate the case "create" branch that constructs domChange and add the
triggerOnView property there (reference: the domChange object in the "create"
case within useDOMChangesEditor).

src/components/dom-editor/DOMChangeList.tsx (1)

258-267: ⚠️ Potential issue | 🟠 Major

Wrap case "create" in a block to avoid switch-scope declaration leakage.

const createChange at Line 261 is declared directly in a switch clause, which fails Biome correctness checks.

🔧 Proposed fix

       case "delete":
         return <span className="text-red-600">Delete element</span>
-      case "create":
-        const createChange = change as any
+      case "create": {
+        const createChange = change as any
         return (
           <span className="text-green-600">
             Insert new element {createChange.position || "after"} the selected
             element
           </span>
         )
+      }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/components/dom-editor/DOMChangeList.tsx` around lines 258 - 267, The
switch case for "create" declares const createChange directly in switch scope
which leaks and fails Biome; fix by wrapping the case body in a block (e.g.,
case "create": { ... }) and move the declaration const createChange = change as
any inside that block, keeping the existing return JSX (and closing the block
before the next case) so the variable is scoped correctly to the case.

src/sdk-bridge/dom/element-state.ts (1)

75-87: ⚠️ Potential issue | 🟠 Major

Reattach condition still skips same-parent reordering and can miss connected shadow-DOM parents.

The current check only runs when parent changes and relies on document.contains(...). Use parent.isConnected plus a needsReposition check so restore also fixes sibling order.

🔧 Proposed fix

-      if (
-        originalState.parent &&
-        element.parentNode !== originalState.parent &&
-        document.contains(originalState.parent)
-      ) {
-        const sibling = originalState.nextSibling
-        const siblingStillThere =
-          sibling && sibling.parentNode === originalState.parent
-        originalState.parent.insertBefore(
-          element,
-          siblingStillThere ? sibling : null
-        )
-      }
+      const parent = originalState.parent
+      if (parent && parent.isConnected) {
+        const sibling = originalState.nextSibling
+        const siblingStillThere =
+          sibling && sibling !== element && sibling.parentNode === parent
+        const targetSibling = siblingStillThere ? sibling : null
+        const needsReposition =
+          element.parentNode !== parent || element.nextSibling !== targetSibling
+
+        if (needsReposition) {
+          parent.insertBefore(element, targetSibling)
+        }
+      }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/sdk-bridge/dom/element-state.ts` around lines 75 - 87, The reattach logic
around originalState currently only runs when originalState.parent differs from
element.parentNode and uses document.contains, which skips same-parent
reordering and misses connected shadow DOM parents; change the condition to
check parent.isConnected and introduce a needsReposition boolean that is true
when either parent changed (originalState.parent !== element.parentNode) or the
originalState.nextSibling is not the same as element.nextSibling (sibling order
differs), then if originalState.parent?.isConnected && needsReposition perform
the insertBefore using originalState.nextSibling to restore sibling order;
update references in this block (originalState, parent, nextSibling, element)
accordingly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/types/dom-changes.ts`:
- Around line 10-11: Update any downstream validators and hard-coded
allowed-type lists that still expect the old values "remove" and "insert" to
instead accept the new "delete" and "create" values; search for symbols like
allowedChangeTypes, validTypes, isValidChangeType, validateDomChange, or any
tests that build a whitelist of change types and replace "remove"→"delete" and
"insert"→"create" (or accept both where backward-compatibility is needed),
ensuring assertions and persistence E2E snippets use the updated type names so
legitimate delete/create changes are not rejected.

In `@src/visual-editor/core/visual-editor.ts`:
- Around line 92-96: Calls to applyChangesViaSDK (used in methods like the
squash/apply flow, undo/redo flows, and other update handlers) must be wrapped
in try-catch blocks so thrown errors don’t leave editor/preview in a
half-applied state; update each site that calls applyChangesViaSDK to catch
exceptions, show a user-visible error (e.g., via the editor’s toast/notification
API or showError method) with a friendly message and the error details, and
restore or keep the undoRedoManager/state consistent (rollback or avoid
committing the squash result when applyChangesViaSDK fails). Ensure you wrap
every direct call to applyChangesViaSDK (including the calls referenced around
applyChangesViaSDK(...) and any similar invocations) and centralize the
error-display logic so messages are consistent.

---

Outside diff comments:
In `@src/visual-editor/core/editor-coordinator.ts`:
- Around line 250-265: The Delete-key handler currently removes the element
directly (selectedElement.remove()) which bypasses the SDK/PreviewManager
pipeline; instead, construct the same change payload used elsewhere (use the
selector from callbacks.getSelector(selectedElement), type: "delete", and
include oldValue = selectedElement.outerHTML) and submit it through the
SDK/PreviewManager apply method (or the existing callbacks/queue API used for
edits) rather than calling selectedElement.remove(); then call
undoRedoManager.addChange with that selector/type and oldValue after queuing the
change and keep the early-return guard using eventHandlers.isEditingMode and
e.preventDefault() as before.

In `@src/visual-editor/core/visual-editor.ts`:
- Around line 510-521: The code currently marks changes as saved regardless of
whether this.options.onChangesUpdate succeeded; update the logic so that
hasUnsavedChanges, lastSavedChangesCount and lastSavedChanges are only updated
after onChangesUpdate completes successfully (i.e., move those assignments
inside the try block immediately after
this.options.onChangesUpdate(squashedChanges)), and leave the state unchanged
when the catch path executes (so stop() and other flows will still see unsaved
changes); keep the existing catch logging for errors thrown by onChangesUpdate.

---

Duplicate comments:
In `@src/components/dom-editor/DOMChangeList.tsx`:
- Around line 258-267: The switch case for "create" declares const createChange
directly in switch scope which leaks and fails Biome; fix by wrapping the case
body in a block (e.g., case "create": { ... }) and move the declaration const
createChange = change as any inside that block, keeping the existing return JSX
(and closing the block before the next case) so the variable is scoped correctly
to the case.

In `@src/hooks/useDOMChangesEditor.ts`:
- Around line 245-254: The "create" branch builds domChange but omits the
triggerOnView flag, so add triggerOnView: changeToSave.triggerOnView (or default
false if required) to the domChange object created in the case "create" block so
it matches the other change types; locate the case "create" branch that
constructs domChange and add the triggerOnView property there (reference: the
domChange object in the "create" case within useDOMChangesEditor).

In `@src/sdk-bridge/dom/element-state.ts`:
- Around line 75-87: The reattach logic around originalState currently only runs
when originalState.parent differs from element.parentNode and uses
document.contains, which skips same-parent reordering and misses connected
shadow DOM parents; change the condition to check parent.isConnected and
introduce a needsReposition boolean that is true when either parent changed
(originalState.parent !== element.parentNode) or the originalState.nextSibling
is not the same as element.nextSibling (sibling order differs), then if
originalState.parent?.isConnected && needsReposition perform the insertBefore
using originalState.nextSibling to restore sibling order; update references in
this block (originalState, parent, nextSibling, element) accordingly.

In `@src/visual-editor/core/element-actions.ts`:
- Around line 318-336: The selection of the newly-inserted node can race with
the async preview apply: after calling the SDK apply (the create branch in
applyPreviewChange) the current code reads
referenceElement.previousElementSibling/nextElementSibling immediately and may
pick an old sibling; fix by polling/waiting for the actual inserted sibling
before calling selectElement — implement a helper like
waitForInsertedSibling(referenceElement, position, timeoutMs) that repeatedly
checks previousElementSibling/nextElementSibling until a new element appears (or
timeout) and then use its result instead of the immediate insertedElement;
update the insertion flow to await waitForInsertedSibling and only call
this.selectElement(found) if non-null.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2f7e05a4-ba9d-4965-8ece-2dcde62a64c0

📥 Commits

Reviewing files that changed from the base of the PR and between 7413952 and 6bead18.

📒 Files selected for processing (30)

• background/main.ts
• src/components/dom-editor/DOMChangeList.tsx
• src/hooks/useDOMChangesEditor.ts
• src/hooks/useVisualEditorCoordination.ts
• src/lib/__tests__/ai-dom-generator.integration.test.ts
• src/lib/validation-schemas.ts
• src/sdk-bridge/dom/element-state.ts
• src/sdk-bridge/dom/preview-manager.ts
• src/types/dom-changes.ts
• src/utils/__tests__/dom-change-operations.test.ts
• src/visual-editor/core/__tests__/editor-coordinator-html.test.ts
• src/visual-editor/core/__tests__/editor-coordinator.test.ts
• src/visual-editor/core/__tests__/element-actions.test.ts
• src/visual-editor/core/edit-modes.ts
• src/visual-editor/core/editor-coordinator.ts
• src/visual-editor/core/element-actions.ts
• src/visual-editor/core/sdk-applier.ts
• src/visual-editor/core/undo-redo-manager.ts
• src/visual-editor/core/visual-editor.ts
• tests/e2e/ai-chat-diagnostic.spec.ts
• tests/e2e/ai-dom-generation-complete.spec.ts
• tests/e2e/ai-dom-generation.spec.ts
• tests/e2e/ai-session-image.spec.ts
• tests/e2e/ai-storage-quota.spec.ts
• tests/e2e/helpers/ve-actions.ts
• tests/e2e/helpers/ve-discard.ts
• tests/e2e/helpers/ve-preview.ts
• tests/e2e/helpers/ve-undo-redo.ts
• tests/e2e/helpers/ve-verification.ts
• tests/e2e/message-bridge.spec.ts

✅ Files skipped from review due to trivial changes (3)

• src/visual-editor/core/tests/editor-coordinator.test.ts
• background/main.ts
• tests/e2e/ai-session-image.spec.ts

🚧 Files skipped from review as they are similar to previous changes (10)

• src/visual-editor/core/undo-redo-manager.ts
• tests/e2e/helpers/ve-preview.ts
• tests/e2e/ai-storage-quota.spec.ts
• tests/e2e/ai-chat-diagnostic.spec.ts
• tests/e2e/helpers/ve-actions.ts
• src/sdk-bridge/dom/preview-manager.ts
• src/lib/validation-schemas.ts
• src/visual-editor/core/sdk-applier.ts
• tests/e2e/helpers/ve-verification.ts
• src/visual-editor/core/tests/element-actions.test.ts

[coderabbitai]

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/visual-editor/core/editor-coordinator.ts (1)

250-265: ⚠️ Potential issue | 🟠 Major

Delete shortcut bypasses the SDK pipeline with a direct DOM mutation.

selectedElement.remove() runs before the "delete" change is applied via the SDK path. That can break PreviewManager’s pre-change capture and make undo/restore unreliable.

Suggested fix

       if (e.key === "Delete") {
         const selectedElement = this.stateManager.getState().selectedElement
         if (selectedElement && !this.eventHandlers.isEditingMode) {
           e.preventDefault()
-          selectedElement.remove()
           const selector = this.callbacks.getSelector(
             selectedElement as HTMLElement
           )
-          const oldValue = selectedElement.outerHTML
           this.undoRedoManager.addChange(
             {
               selector,
               type: "delete"
             },
-            oldValue
+            null
           )
         }
       }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/visual-editor/core/editor-coordinator.ts` around lines 250 - 265, The
code currently calls selectedElement.remove() directly which bypasses the SDK
pipeline and prevents PreviewManager pre-change capture; instead, compute the
selector using this.callbacks.getSelector(selectedElement) and invoke the SDK
delete/change API through the existing callbacks (e.g., call a method on
this.callbacks such as a delete or applyChange handler) so the SDK pipeline runs
first, then only remove the DOM node after the SDK/change callback confirms the
change (or let the SDK perform the DOM mutation); update the flow around
this.undoRedoManager.addChange to pass the same selector and oldValue to the SDK
call and remove the direct selectedElement.remove() invocation so PreviewManager
and undo/redo remain consistent.

♻️ Duplicate comments (3)

src/components/dom-editor/DOMChangeList.tsx (1)

260-267: ⚠️ Potential issue | 🟡 Minor

Wrap the create switch case in braces.

Line 261 still declares createChange directly in the case clause, which violates noSwitchDeclarations and risks scope bleed.

🔧 Proposed fix

-      case "create":
-        const createChange = change as any
-        return (
-          <span className="text-green-600">
-            Insert new element {createChange.position || "after"} the selected
-            element
-          </span>
-        )
+      case "create": {
+        const createChange = change as any
+        return (
+          <span className="text-green-600">
+            Insert new element {createChange.position || "after"} the selected
+            element
+          </span>
+        )
+      }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/components/dom-editor/DOMChangeList.tsx` around lines 260 - 267, The
"create" branch in the switch inside DOMChangeList.tsx declares createChange
directly in the case clause (violates noSwitchDeclarations); to fix, wrap the
entire case "create" block in braces and move the const createChange = change as
any declaration inside that block so its scope is limited to the case body
(adjust the return JSX inside the new block accordingly); reference the "create"
case in the switch within the DOMChangeList component and the createChange
identifier when updating the code.

src/visual-editor/core/element-actions.ts (1)

318-336: ⚠️ Potential issue | 🟠 Major

Selection after insert is still racy and can target the wrong node.

Lines 323-327 read siblings immediately after applyCurrentChangesViaSDK(). Because creation is async, this can pick a pre-existing sibling instead of the inserted element.

🔧 Suggested direction

-      const insertedElement =
-        options.position === "before"
-          ? referenceElement.previousElementSibling
-          : referenceElement.nextElementSibling
+      // Wait for SDK apply to land, then resolve inserted node deterministically.
+      const insertedElement = await this.waitForInsertedSibling(
+        referenceElement,
+        options.position
+      )
@@
-      if (insertedElement) {
+      if (insertedElement) {
         this.selectElement(insertedElement as HTMLElement)
       }

// helper sketch (same file)
private async waitForInsertedSibling(
  referenceElement: HTMLElement,
  position: "before" | "after"
): Promise<Element | null> {
  const deadline = Date.now() + 1000
  while (Date.now() < deadline) {
    const candidate =
      position === "before"
        ? referenceElement.previousElementSibling
        : referenceElement.nextElementSibling
    if (candidate && candidate !== referenceElement) return candidate
    await new Promise((r) => requestAnimationFrame(() => r(null)))
  }
  return null
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/visual-editor/core/element-actions.ts` around lines 318 - 336, The
sibling lookup after applyCurrentChangesViaSDK() is racy and can pick an
existing sibling; add a helper like waitForInsertedSibling(referenceElement,
position) that polls via requestAnimationFrame (with a short timeout, e.g. 1s)
for referenceElement.previousElementSibling / nextElementSibling to become a new
node, then replace the immediate lookup in the insert flow with await
waitForInsertedSibling(...) and call selectElement(foundNode) only when a
non-null node is returned; keep existing options.position ("before"/"after") and
selectElement(...) semantics and fall back gracefully if the helper returns
null.

src/sdk-bridge/dom/element-state.ts (1)

75-87: ⚠️ Potential issue | 🟠 Major

Restore needs to handle same-parent reorders and connected shadow roots.

The current guard still skips cases where the element stayed in the same parent but changed sibling order, and document.contains(originalState.parent) misses shadow-root / fragment parents. That means restore can silently fail in exactly the cases this metadata was added to cover.

🛠️ Suggested fix

-      if (
-        originalState.parent &&
-        element.parentNode !== originalState.parent &&
-        document.contains(originalState.parent)
-      ) {
-        const sibling = originalState.nextSibling
-        const siblingStillThere =
-          sibling && sibling.parentNode === originalState.parent
-        originalState.parent.insertBefore(
-          element,
-          siblingStillThere ? sibling : null
-        )
-      }
+      const parent = originalState.parent
+      if (parent && parent.isConnected) {
+        const sibling = originalState.nextSibling
+        const siblingStillThere =
+          sibling && sibling !== element && sibling.parentNode === parent
+        const targetSibling = siblingStillThere ? sibling : null
+        const needsReposition =
+          element.parentNode !== parent || element.nextSibling !== targetSibling
+
+        if (needsReposition) {
+          parent.insertBefore(element, targetSibling)
+        }
+      }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/sdk-bridge/dom/element-state.ts` around lines 75 - 87, The restore block
that currently only runs when element.parentNode !== originalState.parent and
document.contains(originalState.parent) misses same-parent reorders and parents
that are shadow roots or fragments; change the guard to also run when
element.parentNode === originalState.parent but the current sibling order
differs (compare element.nextSibling or originalState.nextSibling), and replace
document.contains(originalState.parent) with a more robust check (e.g.,
originalState.parent.isConnected OR parent is a
ShadowRoot/DocumentFragment/nodeType 11) so shadow-root/fragment parents are
considered; keep use of originalState.nextSibling and
originalState.parent.insertBefore(element, siblingStillThere ? sibling : null)
to perform the actual reinsertion.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/visual-editor/core/edit-modes.ts`:
- Around line 656-674: The revert logic currently uses
originalParent/originalNextSibling which get cleared once an element already has
a move entry, so subsequent drags lose the rollback anchor; preserve the
first-move anchor separately (e.g., store initialParent/initialNextSibling on
the move metadata or a dedicated property on the element) and use those
preserved anchors in the revert block before adding this.addChange(moveChange)
so repeated drags still revert to the original pre-first-drop DOM; update any
code that clears originalParent/originalNextSibling to only clear the transient
live anchors, not the preserved initial anchors.

In `@src/visual-editor/core/editor-coordinator.ts`:
- Around line 454-460: The HTML payloads (element.innerHTML / newHtml) are being
enqueued raw into this.undoRedoManager.addChange with type: "html"; sanitize the
HTML before adding to the change queue to prevent scriptable payloads from being
replayed. Locate the code paths that call this.undoRedoManager.addChange
(references: addChange, element.innerHTML, newHtml, type: "html") and run the
HTML through the project's sanitizer utility (or a trusted sanitizer like
DOMPurify/DOMPurify.sanitize or sanitizedHtml function) and then pass the
sanitized string as value to addChange; ensure you import/use the existing
sanitation helper rather than storing raw innerHTML.

In `@tests/e2e/helpers/ve-actions.ts`:
- Around line 223-237: The two assertions that read DOM once via page.evaluate
(the beforeBlockExists and beforeBlockContent checks) can race the async SDK
apply path; instead of sampling immediately, wait until the concrete DOM
condition appears before asserting: use Playwright's waiting/polling helpers
(e.g., page.waitForSelector or a small polling loop) to wait for
'.inserted-block-before' to exist and have expected text, then read its
textContent and assert it equals 'This is a BEFORE block!'; update the code
around beforeBlockExists, beforeBlockContent, and debugWait to first wait for
the selector/text to stabilise to avoid flakes.
- Around line 190-195: The test currently uses brittle class-based locators
(beforePositionBtn -> locator('.position-btn[data-position="before"]') and the
selected check '.position-btn[data-position="before"].position-btn-selected');
instead add a stable id to the position toggle element (e.g.
id="position-before") and update the test to use ID selectors (replace
page.locator('.position-btn[data-position="before"]') with
page.locator('#position-before') and the selected-state check with
page.locator('#position-before.position-btn-selected') or
'#position-before.selected' as appropriate), keeping the same waitFor/click
flow.

---

Outside diff comments:
In `@src/visual-editor/core/editor-coordinator.ts`:
- Around line 250-265: The code currently calls selectedElement.remove()
directly which bypasses the SDK pipeline and prevents PreviewManager pre-change
capture; instead, compute the selector using
this.callbacks.getSelector(selectedElement) and invoke the SDK delete/change API
through the existing callbacks (e.g., call a method on this.callbacks such as a
delete or applyChange handler) so the SDK pipeline runs first, then only remove
the DOM node after the SDK/change callback confirms the change (or let the SDK
perform the DOM mutation); update the flow around this.undoRedoManager.addChange
to pass the same selector and oldValue to the SDK call and remove the direct
selectedElement.remove() invocation so PreviewManager and undo/redo remain
consistent.

---

Duplicate comments:
In `@src/components/dom-editor/DOMChangeList.tsx`:
- Around line 260-267: The "create" branch in the switch inside
DOMChangeList.tsx declares createChange directly in the case clause (violates
noSwitchDeclarations); to fix, wrap the entire case "create" block in braces and
move the const createChange = change as any declaration inside that block so its
scope is limited to the case body (adjust the return JSX inside the new block
accordingly); reference the "create" case in the switch within the DOMChangeList
component and the createChange identifier when updating the code.

In `@src/sdk-bridge/dom/element-state.ts`:
- Around line 75-87: The restore block that currently only runs when
element.parentNode !== originalState.parent and
document.contains(originalState.parent) misses same-parent reorders and parents
that are shadow roots or fragments; change the guard to also run when
element.parentNode === originalState.parent but the current sibling order
differs (compare element.nextSibling or originalState.nextSibling), and replace
document.contains(originalState.parent) with a more robust check (e.g.,
originalState.parent.isConnected OR parent is a
ShadowRoot/DocumentFragment/nodeType 11) so shadow-root/fragment parents are
considered; keep use of originalState.nextSibling and
originalState.parent.insertBefore(element, siblingStillThere ? sibling : null)
to perform the actual reinsertion.

In `@src/visual-editor/core/element-actions.ts`:
- Around line 318-336: The sibling lookup after applyCurrentChangesViaSDK() is
racy and can pick an existing sibling; add a helper like
waitForInsertedSibling(referenceElement, position) that polls via
requestAnimationFrame (with a short timeout, e.g. 1s) for
referenceElement.previousElementSibling / nextElementSibling to become a new
node, then replace the immediate lookup in the insert flow with await
waitForInsertedSibling(...) and call selectElement(foundNode) only when a
non-null node is returned; keep existing options.position ("before"/"after") and
selectElement(...) semantics and fall back gracefully if the helper returns
null.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 86dcb139-ee20-48df-b971-a75325b9f947

📥 Commits

Reviewing files that changed from the base of the PR and between 6bead18 and 4681eae.

📒 Files selected for processing (30)

• background/main.ts
• src/components/dom-editor/DOMChangeList.tsx
• src/hooks/useDOMChangesEditor.ts
• src/hooks/useVisualEditorCoordination.ts
• src/lib/__tests__/ai-dom-generator.integration.test.ts
• src/lib/validation-schemas.ts
• src/sdk-bridge/dom/element-state.ts
• src/sdk-bridge/dom/preview-manager.ts
• src/types/dom-changes.ts
• src/utils/__tests__/dom-change-operations.test.ts
• src/visual-editor/core/__tests__/editor-coordinator-html.test.ts
• src/visual-editor/core/__tests__/editor-coordinator.test.ts
• src/visual-editor/core/__tests__/element-actions.test.ts
• src/visual-editor/core/edit-modes.ts
• src/visual-editor/core/editor-coordinator.ts
• src/visual-editor/core/element-actions.ts
• src/visual-editor/core/sdk-applier.ts
• src/visual-editor/core/undo-redo-manager.ts
• src/visual-editor/core/visual-editor.ts
• tests/e2e/ai-chat-diagnostic.spec.ts
• tests/e2e/ai-dom-generation-complete.spec.ts
• tests/e2e/ai-dom-generation.spec.ts
• tests/e2e/ai-session-image.spec.ts
• tests/e2e/ai-storage-quota.spec.ts
• tests/e2e/helpers/ve-actions.ts
• tests/e2e/helpers/ve-discard.ts
• tests/e2e/helpers/ve-preview.ts
• tests/e2e/helpers/ve-undo-redo.ts
• tests/e2e/helpers/ve-verification.ts
• tests/e2e/message-bridge.spec.ts

✅ Files skipped from review due to trivial changes (5)

• background/main.ts
• src/visual-editor/core/tests/editor-coordinator.test.ts
• tests/e2e/ai-chat-diagnostic.spec.ts
• src/lib/validation-schemas.ts
• tests/e2e/helpers/ve-discard.ts

🚧 Files skipped from review as they are similar to previous changes (11)

• src/utils/tests/dom-change-operations.test.ts
• tests/e2e/ai-dom-generation-complete.spec.ts
• tests/e2e/ai-session-image.spec.ts
• src/lib/tests/ai-dom-generator.integration.test.ts
• tests/e2e/message-bridge.spec.ts
• tests/e2e/ai-storage-quota.spec.ts
• src/visual-editor/core/undo-redo-manager.ts
• tests/e2e/helpers/ve-preview.ts
• src/sdk-bridge/dom/preview-manager.ts
• src/visual-editor/core/sdk-applier.ts
• src/visual-editor/core/tests/element-actions.test.ts

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

tests/global-setup.ts (1)

233-236: Backoff comment and formula are inconsistent (“full jitter” vs equal-jitter-ish).

At Line 233, the comment says “full jitter”, but Math.random() * base + base / 2 does not implement full jitter. Either rename the strategy in the comment or switch to true full jitter.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/global-setup.ts` around lines 233 - 236, The comment says "full jitter"
but the code computes delay as Math.floor(Math.random() * base) + base / 2 (an
equal-jitter style); fix by either updating the comment to "equal jitter" or
implementing true full jitter: compute base = 250 * 2 ** (attempt - 1) and set
delay = Math.floor(Math.random() * base) (or Math.random() * base rounded) so
the delay is uniformly random in [0, base); adjust the comment accordingly and
keep references to the variables used (base, delay, attempt) consistent.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/global-setup.ts`:
- Around line 211-237: The fetch loop in fetchOne lacks a per-attempt timeout
and uses an untyped Promise<unknown[]]; add an AbortController per attempt with
an 8-second timer and pass its signal to fetch (clear the timer after fetch
completes or on error) so stalled sockets abort and the retry loop can proceed;
change the fetchOne signature from Promise<unknown[]> to a typed return (define
interfaces for the expected resource shapes — e.g., ApplicationsResponse,
UnitTypesResponse, MetricsResponse — or a discriminated union) and use that type
in parsing/return logic; finally update the backoff comment near the exponential
jitter calculation to reflect that it is a "jittered exponential backoff with
offset" rather than "full jitter."

---

Nitpick comments:
In `@tests/global-setup.ts`:
- Around line 233-236: The comment says "full jitter" but the code computes
delay as Math.floor(Math.random() * base) + base / 2 (an equal-jitter style);
fix by either updating the comment to "equal jitter" or implementing true full
jitter: compute base = 250 * 2 ** (attempt - 1) and set delay =
Math.floor(Math.random() * base) (or Math.random() * base rounded) so the delay
is uniformly random in [0, base); adjust the comment accordingly and keep
references to the variables used (base, delay, attempt) consistent.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d2d67d3c-1233-473d-b876-17c202cc33c1

📥 Commits

Reviewing files that changed from the base of the PR and between 4681eae and 9cb44e7.

📒 Files selected for processing (1)

• tests/global-setup.ts