fix(site-builder): final whole-branch review fixes

C1: HtmlBlock's PURIFY_CONFIG omitted 'style' from ALLOWED_ATTR, so the
toolbar colour picker added in this branch was silently deleted by
DOMPurify -- issue #2 was regressed, not fixed. Adds style/id plus table
tags, with tests pinning the markup path in both render and toHtml.

I3: PagesPanel's three confirmation states were not mutually exclusive;
cancelling delete revealed an unbidden reset prompt on a destructive action.

I5: orphan repair logged at console.warn, which the new console buffer
cannot see -- the reporter would never capture the most diagnostic signal
for the still-unreproduced drop bug. Also aligns useWhpApi's initial-load
failure handling with loadState's fallback.

I7: corrects comments (and the design spec) that asserted an orphan
"renders somewhere on the canvas", which a mid-plan audit disproved.

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

craft/src/components/basic/HtmlBlock.test.ts

@@ -22,6 +22,41 @@ describe('purifyHtml', () => {
   });
 });
 
+describe('purifyHtml markup path (C1 review finding)', () => {
+  test('a style attribute survives sanitization (colour picker output must not be silently dropped)', () => {
+    const out = purifyHtml('<p style="color: #ff0000">red text</p>');
+    expect(out).toBe('<p style="color: #ff0000">red text</p>');
+  });
+
+  test('an id attribute survives sanitization (anchor targets)', () => {
+    const out = purifyHtml('<a href="#section" id="section">link</a>');
+    expect(out).toContain('id="section"');
+  });
+
+  test('a pasted table survives sanitization', () => {
+    const input = '<table><thead><tr><th>Head</th></tr></thead><tbody><tr><td>Cell</td></tr></tbody></table>';
+    expect(purifyHtml(input)).toBe(input);
+  });
+
+  test('script tags still do not survive alongside a style attribute', () => {
+    const out = purifyHtml('<p style="color:#ff0000">ok</p><script>alert(1)</script>');
+    expect(out).not.toContain('<script');
+    expect(out).toContain('style="color:#ff0000"');
+  });
+
+  test('on* handlers still do not survive on an element that also carries style', () => {
+    const out = purifyHtml('<p style="color:#ff0000" onclick="bad()">x</p>');
+    expect(out).not.toContain('onclick');
+    expect(out).toContain('style="color:#ff0000"');
+  });
+
+  test('javascript: URLs still do not survive on an element that also carries style', () => {
+    const out = purifyHtml('<a style="color:#ff0000" href="javascript:void(0)">x</a>');
+    expect(out).not.toContain('javascript:');
+    expect(out).toContain('style="color:#ff0000"');
+  });
+});
+
 describe('purifyHtml iframe sandboxing (M-6)', () => {
   test('forces a restrictive sandbox attribute onto every iframe', () => {
     const out = purifyHtml('<iframe src="https://example.com/"></iframe>');

craft/src/components/basic/HtmlBlock.toHtml.test.ts

@@ -33,3 +33,16 @@ test('toHtml never emits the style prop (the other half of the render/export con
   expect(out.html).not.toContain('background');
   expect(out.html).not.toContain('40px');
 });
+
+describe('HtmlBlock.toHtml markup path (C1 review finding)', () => {
+  test('a style attribute inside `code` (e.g. from the toolbar colour picker) reaches exported output', () => {
+    const { html } = toHtml({ code: '<p style="color: #ff0000">red text</p>' }, '');
+    expect(html).toBe('<p style="color: #ff0000">red text</p>');
+  });
+
+  test('a table inside `code` reaches exported output', () => {
+    const code = '<table><tbody><tr><td>Cell</td></tr></tbody></table>';
+    const { html } = toHtml({ code }, '');
+    expect(html).toBe(code);
+  });
+});

craft/src/components/basic/HtmlBlock.tsx

@@ -19,10 +19,17 @@ const PURIFY_CONFIG = {
     'blockquote','code','pre',
     'img','figure','figcaption',
     'iframe',
+    // Tables: pasted content commonly includes these; dropping them
+    // silently ate customer-pasted tables (see C1 review finding).
+    'table','thead','tbody','tfoot','tr','td','th','caption','colgroup','col',
   ],
+  // NOTE: supplying ALLOWED_ATTR replaces DOMPurify's own default attribute
+  // allowlist rather than extending it, so anything the product needs
+  // (style, id, ...) must be listed explicitly here even though DOMPurify
+  // would allow it by default.
   ALLOWED_ATTR: [
     'href','src','alt','title','target','rel',
-    'width','height','class',
+    'width','height','class','id','style',
     'allowfullscreen','allow','frameborder',
     'sandbox','referrerpolicy',
   ],

craft/src/hooks/useWhpApi.orphan-repair-wiring.test.tsx

@@ -3,7 +3,7 @@ import React from 'react';
 import { createRoot, Root } from 'react-dom/client';
 import { act } from 'react-dom/test-utils';
 import { EditorConfigProvider } from '../state/EditorConfigContext';
-import { PageProvider, usePages } from '../state/PageContext';
+import { EMPTY_CANVAS, PageProvider, usePages } from '../state/PageContext';
 import { SiteDesignProvider } from '../state/SiteDesignContext';
 import { useWhpApi } from './useWhpApi';
 import { WhpConfig } from '../types';
@@ -22,8 +22,10 @@ import { WhpConfig } from '../types';
  * This mocks `@craftjs/core` the same way `useWhpApi.load.test.tsx` and
  * `PageContext.orphan-repair-wiring.test.tsx` do, and asserts on the exact
  * string handed to the mocked `actions.deserialize` -- the orphan must be
- * reattached to ROOT and a `console.warn` must fire, exactly like the
+ * reattached to ROOT and a `console.error` must fire, exactly like the
- * page-switch path.
+ * page-switch path (I5 review: this was `console.warn`, which
+ * console-buffer.ts -- feeding the in-builder issue reporter -- does not
+ * capture).
  */
 const deserializeMock = vi.fn();
 vi.mock('@craftjs/core', () => ({
@@ -133,7 +135,7 @@ describe('useWhpApi load() repairs an orphaned node on the FIRST page before des
     });
     vi.stubGlobal('fetch', fetchMock);
 
-    const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {});
+    const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
     const harness = render();
 
     await act(async () => {
@@ -148,11 +150,69 @@ describe('useWhpApi load() repairs an orphaned node on the FIRST page before des
     expect(parsed.stray.parent).toBe('ROOT');
 
     // The observable signal that repair actually ran, not just that the
-    // orphan happened to be absent for some unrelated reason.
+    // orphan happened to be absent for some unrelated reason. (React's own
-    expect(warnSpy).toHaveBeenCalled();
+    // act()-environment warnings also go through console.error in this
-    expect(warnSpy.mock.calls[0][0]).toContain('reattached');
+    // harness, so search all calls rather than assuming index 0.)
+    expect(errorSpy.mock.calls.some((call) => String(call[0]).includes('reattached'))).toBe(true);
 
-    warnSpy.mockRestore();
+    errorSpy.mockRestore();
+    unmount();
+  });
+});
+
+/**
+ * I5 (review): `PageContext.loadState`'s deserialize failure path logs via
+ * `console.error` AND falls back to `EMPTY_CANVAS` so the user always ends
+ * up with a working (if blank) editor. `useWhpApi.load()`'s equivalent path
+ * used to be `console.warn` with NO fallback deserialize -- the initial
+ * load, which decides whether the user sees a working editor at all, both
+ * failed harder (silently leaving the Frame undeserialized) and reported
+ * quieter than every subsequent page switch. This pins the aligned
+ * behaviour.
+ */
+describe('useWhpApi load() falls back to EMPTY_CANVAS when the first page state cannot be deserialized', () => {
+  beforeEach(() => {
+    deserializeMock.mockClear();
+  });
+
+  afterEach(() => {
+    vi.unstubAllGlobals();
+  });
+
+  test('a deserialize failure on the first page logs console.error and retries with EMPTY_CANVAS', async () => {
+    const fetchMock = vi.fn().mockResolvedValue({
+      json: async () => ({
+        success: true,
+        project: {
+          design: null,
+          header_craft_state: null,
+          footer_craft_state: null,
+          pages_craft_state: [
+            { id: 'home', name: 'Home', slug: 'index', craftState: '{"ROOT":{"broken":true}}' },
+          ],
+        },
+      }),
+    });
+    vi.stubGlobal('fetch', fetchMock);
+
+    deserializeMock.mockImplementationOnce(() => {
+      throw new Error('malformed state');
+    });
+
+    const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
+    const harness = render();
+
+    await act(async () => {
+      await harness.get().load();
+    });
+
+    // First call (the broken state) threw; the second call is the fallback.
+    expect(deserializeMock).toHaveBeenCalledTimes(2);
+    expect(deserializeMock.mock.calls[1][0]).toBe(EMPTY_CANVAS);
+
+    expect(errorSpy).toHaveBeenCalledWith('Failed to load page state:', expect.any(Error));
+
+    errorSpy.mockRestore();
     unmount();
   });
 });

craft/src/hooks/useWhpApi.ts

@@ -1,7 +1,7 @@
 import { useCallback } from 'react';
 import { useEditor } from '@craftjs/core';
 import { useEditorConfig } from '../state/EditorConfigContext';
-import { usePages } from '../state/PageContext';
+import { usePages, EMPTY_CANVAS } from '../state/PageContext';
 import { useSiteDesign, SiteDesign } from '../state/SiteDesignContext';
 import { exportBodyHtml } from '../utils/html-export';
 import { repairOrphanNodes } from '../utils/orphan-repair';
@@ -321,14 +321,27 @@ export function useWhpApi() {
               ? firstPage.craftState : JSON.stringify(firstPage.craftState);
             const { state, repaired } = repairOrphanNodes(rawState);
             if (repaired.length > 0) {
-              console.warn(
+              // I5: console-buffer.ts only patches console.error, and this
+              // reattach signal is the single most diagnostic clue for the
+              // still-unreproduced "elements drop off the canvas" report --
+              // it must reach the in-builder issue reporter's console buffer.
+              console.error(
                 `[site-builder] reattached ${repaired.length} unreachable node(s) to the page root:`,
                 repaired.join(', '),
               );
             }
             actions.deserialize(state);
           } catch (e) {
-            console.warn('Failed to load page state:', e);
+            // I5: this is the initial load that decides whether the user
+            // sees a working editor at all -- align with `loadState`'s
+            // behaviour (console.error + a known-safe fallback) instead of
+            // warning quietly and leaving the Frame on whatever it last had.
+            console.error('Failed to load page state:', e);
+            try {
+              actions.deserialize(EMPTY_CANVAS);
+            } catch (_e2) {
+              // give up
+            }
           }
         }
 

craft/src/panels/left/PagesPanel.confirmStates.test.tsx

@@ -0,0 +1,119 @@
+import { describe, test, expect } from 'vitest';
+import React from 'react';
+import { renderEditorHarness } from '../../test-utils/editorHarness';
+import { PageProvider, usePages } from '../../state/PageContext';
+import { PagesPanel } from './PagesPanel';
+
+function clickByLabel(container: HTMLElement, label: string): HTMLButtonElement {
+  const btn = container.querySelector(`[aria-label="${label}"]`) as HTMLButtonElement | null;
+  if (!btn) throw new Error(`No button found with aria-label "${label}"`);
+  return btn;
+}
+
+function clickByText(container: HTMLElement, text: string): HTMLButtonElement {
+  const btn = Array.from(container.querySelectorAll('button')).find(
+    (b) => b.textContent === text,
+  ) as HTMLButtonElement | undefined;
+  if (!btn) throw new Error(`No button found with text "${text}"`);
+  return btn;
+}
+
+/**
+ * I3 (review): `deleteConfirmId`, `resetConfirmId` and `editingId` are three
+ * independent bits of local state, but the row rendering treats them as
+ * mutually exclusive (edit -> delete -> reset -> normal, first match wins).
+ * Before this fix, none of the three setters cleared the other two, so if
+ * both `resetConfirmId` and `deleteConfirmId` were ever set to the same page
+ * id, the delete-confirm view would win (it's checked first) and cancelling
+ * it would fall through to reveal the reset-confirm view "unbidden" -- a
+ * destructive prompt the user never asked for.
+ *
+ * This is reachable any time two arm-clicks land in the same render pass
+ * (e.g. a fast double click / synthetic dual dispatch before React commits
+ * the first click's re-render) -- reproduced below by issuing both clicks
+ * inside a single `act()` batch, which is exactly what removes the re-render
+ * that would otherwise make the second button disappear before it can be
+ * clicked.
+ */
+describe('PagesPanel confirmation-state isolation (I3 review finding)', () => {
+  async function setupTwoPages() {
+    let ctx: ReturnType<typeof usePages> | null = null;
+    const Probe: React.FC = () => { ctx = usePages(); return null; };
+    const harness = renderEditorHarness();
+    harness.mountChild(
+      <PageProvider>
+        <Probe />
+        <PagesPanel />
+      </PageProvider>,
+    );
+    harness.act(() => { ctx!.addPage('About', 'about'); });
+    await harness.act(async () => { await new Promise((resolve) => setTimeout(resolve, 10)); });
+    return harness;
+  }
+
+  test('arming Reset then Delete for the same page in one batch shows Delete (render precedence), and cancelling Delete does NOT reveal a leftover Reset prompt', async () => {
+    const harness = await setupTwoPages();
+
+    // Both arm-clicks land before any re-render commits.
+    harness.act(() => {
+      clickByLabel(harness.container, 'Reset About to blank').click();
+      clickByLabel(harness.container, 'Delete About').click();
+    });
+    expect(harness.container.textContent).toContain('Delete "About"?');
+    expect(harness.container.textContent).not.toContain('Clear every element from "About"?');
+
+    harness.act(() => {
+      clickByText(harness.container, 'Cancel').click();
+    });
+
+    // The fix: arming Delete clears any pending Reset confirmation for the
+    // same page, so cancelling Delete returns to the normal row, not a
+    // surprise Reset prompt.
+    expect(harness.container.textContent).not.toContain('Clear every element from "About"?');
+    expect(harness.container.textContent).not.toContain('Delete "About"?');
+
+    harness.unmount();
+  });
+
+  test('arming Delete then Reset for the same page in one batch shows Reset, and cancelling Reset does NOT reveal a leftover Delete prompt', async () => {
+    const harness = await setupTwoPages();
+
+    harness.act(() => {
+      clickByLabel(harness.container, 'Delete About').click();
+      clickByLabel(harness.container, 'Reset About to blank').click();
+    });
+    expect(harness.container.textContent).toContain('Clear every element from "About"?');
+
+    harness.act(() => {
+      clickByText(harness.container, 'Cancel').click();
+    });
+
+    expect(harness.container.textContent).not.toContain('Delete "About"?');
+    expect(harness.container.textContent).not.toContain('Clear every element from "About"?');
+
+    harness.unmount();
+  });
+
+  test('starting a Rename while a Reset confirmation is pending for the same page clears the pending Reset', async () => {
+    const harness = await setupTwoPages();
+
+    // Arm Reset, then (same batch) start editing the same row -- edit wins
+    // in render precedence, but the fix also clears resetConfirmId so
+    // cancelling the rename doesn't fall through to a stale Reset prompt.
+    harness.act(() => {
+      clickByLabel(harness.container, 'Reset About to blank').click();
+      clickByLabel(harness.container, 'Rename About').click();
+    });
+    const nameInput = harness.container.querySelector('input.control-input') as HTMLInputElement | null;
+    expect(nameInput).toBeTruthy();
+    expect(nameInput!.value).toBe('About');
+
+    harness.act(() => {
+      clickByText(harness.container, 'Cancel').click();
+    });
+
+    expect(harness.container.textContent).not.toContain('Clear every element from "About"?');
+
+    harness.unmount();
+  });
+});

craft/src/panels/left/PagesPanel.tsx

@@ -77,6 +77,7 @@ export const PagesPanel: React.FC = () => {
     setEditName(page.name);
     setEditSlug(page.slug);
     setDeleteConfirmId(null);
+    setResetConfirmId(null);
   };
 
   const autoSlug = (name: string): string => {
@@ -515,7 +516,10 @@ export const PagesPanel: React.FC = () => {
                   <i className="fa fa-pencil" aria-hidden="true" />
                 </button>
                 <button
-                  onClick={() => setResetConfirmId(page.id)}
+                  onClick={() => {
+                    setResetConfirmId(page.id);
+                    setDeleteConfirmId(null);
+                  }}
                   data-tooltip="Reset to blank"
                   aria-label={`Reset ${page.name} to blank`}
                   style={pageActionBtnStyle({ danger: true })}
@@ -524,7 +528,10 @@ export const PagesPanel: React.FC = () => {
                 </button>
                 {pages.length > 1 && !isLanding && (
                   <button
-                    onClick={() => setDeleteConfirmId(page.id)}
+                    onClick={() => {
+                      setDeleteConfirmId(page.id);
+                      setResetConfirmId(null);
+                    }}
                     data-tooltip="Delete"
                     aria-label={`Delete ${page.name}`}
                     style={pageActionBtnStyle({ danger: true })}

craft/src/panels/right/GuidedStyles.tsx

@@ -165,7 +165,7 @@ export const GuidedStyles: React.FC = () => {
       {/* HTML -- code editor only */}
       {isHtml && <HtmlStylePanel selectedId={selected} nodeProps={nodeProps} />}
 
-      {/* UTILITY (Divider, Spacer, HTML) -- use generic but it works well for these */}
+      {/* UTILITY (Divider, Spacer) -- use generic but it works well for these */}
       {isUtility && <GenericPropsEditor selectedId={selected} nodeProps={nodeProps} typeName={typeName} />}
 
       {/* FALLBACK: Anything not matched above */}

craft/src/state/PageContext.orphan-repair-wiring.test.tsx

@@ -122,7 +122,12 @@ describe('loadState (via switchPage) repairs an orphaned node before handing it
       ),
     );
 
-    const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {});
+    // I5 (review): the orphan-repair log must be console.error, not
+    // console.warn -- console-buffer.ts (feeding the in-builder issue
+    // reporter) only patches console.error, and this reattach signal is the
+    // single most diagnostic clue for the still-unreproduced "elements drop
+    // off the canvas" report.
+    const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
     deserializeMock.mockClear();
 
     // The real switch-into-a-stored-page path.
@@ -137,11 +142,12 @@ describe('loadState (via switchPage) repairs an orphaned node before handing it
     expect(parsed.stray.parent).toBe('ROOT');
 
     // The observable signal that repair actually ran, not just that the
-    // orphan happened to be absent for some unrelated reason.
+    // orphan happened to be absent for some unrelated reason. (React's own
-    expect(warnSpy).toHaveBeenCalled();
+    // act()-environment warnings also go through console.error in this
-    expect(warnSpy.mock.calls[0][0]).toContain('reattached');
+    // harness, so search all calls rather than assuming index 0.)
+    expect(errorSpy.mock.calls.some((call) => String(call[0]).includes('reattached'))).toBe(true);
 
-    warnSpy.mockRestore();
+    errorSpy.mockRestore();
     unmount();
   });
 });

craft/src/state/PageContext.tsx

@@ -371,19 +371,30 @@ export const PageProvider: React.FC<{ children: ReactNode }> = ({ children }) =>
 
   /** Load a craft state into the Frame.
    *
-   *  Every state goes through `repairOrphanNodes` first: a node that no
+   *  Every state goes through `repairOrphanNodes` first: a node present in
-   *  parent lists is invisible to Layers and unselectable on the canvas, so
+   *  the serialized state but not reachable from ROOT via `nodes`/
-   *  it can neither be moved nor deleted. Reattaching it to the end of ROOT
+   *  `linkedNodes` is never instantiated by Craft.js's `<Frame>` at all --
-   *  makes it an ordinary child the user can select and delete. Cheap
+   *  it doesn't render, so it isn't merely unselectable, it's invisible and
-   *  (single JSON round-trip) and a no-op -- returning the identical string
+   *  otherwise unrecoverable. Reattaching it to the end of ROOT makes it an
-   *  -- for the overwhelmingly common healthy case. */
+   *  ordinary child the user can see, select and delete. Cheap (single JSON
+   *  round-trip) and a no-op -- returning the identical string -- for the
+   *  overwhelmingly common healthy case.
+   *
+   *  Note this orphan-node repair is a different mechanism from the
+   *  originally-reported symptom (a *visible* element on the canvas that
+   *  can't be selected or deleted) -- that report is still unreproduced;
+   *  see `orphan-repair.ts` for detail. */
   const loadState = useCallback(
     (craftState: string | null, fallback: string) => {
       setTimeout(() => {
         const source = craftState || fallback;
         const { state, repaired } = repairOrphanNodes(source);
         if (repaired.length > 0) {
-          console.warn(
+          // I5: console-buffer.ts only patches console.error, and this
+          // reattach signal is the single most diagnostic clue for the
+          // still-unreproduced "elements drop off the canvas" report -- it
+          // must reach the in-builder issue reporter's console buffer.
+          console.error(
             `[site-builder] reattached ${repaired.length} unreachable node(s) to the page root:`,
             repaired.join(', '),
           );

craft/src/utils/orphan-repair.ts

@@ -3,11 +3,23 @@
  *
  * A node that exists in `SerializedNodes` but appears in no parent's `nodes`
  * or `linkedNodes` list is invisible to the Layers tree AND to Craft's own
- * selection machinery -- it renders somewhere on the canvas but can't be
+ * selection machinery -- but not because it "renders but can't be selected".
- * selected or deleted, which is exactly the "dropped outside the page"
+ * Craft.js's `<Frame>` only instantiates nodes it can actually walk to via
- * report. Reachability is computed from the PARENT'S child lists, not from
+ * `data.nodes`/`linkedNodes` starting from ROOT, so an unreachable node is
- * each node's own `parent` pointer: a stale `parent: 'ROOT'` on a node ROOT
+ * never rendered at all: it doesn't appear on the canvas, it's just inert
- * never lists is precisely the broken case we're looking for.
+ * data sitting in the serialized state. Reachability is computed from the
+ * PARENT'S child lists, not from each node's own `parent` pointer: a stale
+ * `parent: 'ROOT'` on a node ROOT never lists is precisely the broken case
+ * we're looking for.
+ *
+ * This is a DIFFERENT mechanism from the originally-reported symptom -- a
+ * *visible* element on the canvas that can't be selected or deleted. That
+ * symptom requires the node to be both rendered AND excluded from Craft's
+ * selection/interaction machinery, which is not what an unreachable-from-
+ * ROOT node produces (it isn't rendered at all). This repair fixes the
+ * "node is present in state but invisible/unrecoverable" case; the
+ * originally-reported "visible but unselectable" case is still
+ * unreproduced and is presumed to be a different bug.
  *
  * Pure functions over serialized state -- no React, no Craft instance.
  */

docs/superpowers/specs/2026-08-08-site-builder-user-reported-issues-design.md

@@ -46,6 +46,8 @@ The mechanism is deliberately left open pending a live reproduction. Two structu
 
 **The plan's first task is reproduction, and its acceptance criterion is a written description of the actual mechanism.** No fix is written before that.
 
+**Status (2026-08-08, final review pass): NOT implemented.** The reproduction task did not land during this branch — the drop-time mechanism described above (stale drop indicator vs. wrapper-`<div>` drop target, or something else) was never confirmed against a live editor session, so no prevention fix was written, per the acceptance criterion above. What shipped instead is 1b (repair) and 1c (recovery), plus the in-builder issue reporter from Item 5, whose plan was to capture a real repro of this specific bug from customers going forward. A code-review pass on this branch found that `useWhpApi.ts` and `PageContext.tsx`'s orphan-repair log lines used `console.warn`, which the reporter's console buffer does not capture (it patches `console.error` only) — fixed to `console.error` so a future in-the-wild repro of this exact symptom is actually captured. Reproducing the drop-time mechanism and writing the 1a prevention fix remains open work.
+
 ### 1b. Repair (load time)
 
 `PageContext`'s deserialization path runs an orphan sweep before handing state to Craft: