From 69e61ab4b29f9bb4859dbf71f3744f4616935325 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 9 Aug 2026 12:47:23 -0700 Subject: [PATCH] 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) --- craft/src/components/basic/HtmlBlock.test.ts | 35 ++++++ .../components/basic/HtmlBlock.toHtml.test.ts | 13 ++ craft/src/components/basic/HtmlBlock.tsx | 9 +- .../useWhpApi.orphan-repair-wiring.test.tsx | 76 +++++++++-- craft/src/hooks/useWhpApi.ts | 19 ++- .../left/PagesPanel.confirmStates.test.tsx | 119 ++++++++++++++++++ craft/src/panels/left/PagesPanel.tsx | 11 +- craft/src/panels/right/GuidedStyles.tsx | 2 +- .../PageContext.orphan-repair-wiring.test.tsx | 16 ++- craft/src/state/PageContext.tsx | 25 ++-- craft/src/utils/orphan-repair.ts | 22 +++- ...ite-builder-user-reported-issues-design.md | 2 + 12 files changed, 317 insertions(+), 32 deletions(-) create mode 100644 craft/src/panels/left/PagesPanel.confirmStates.test.tsx diff --git a/craft/src/components/basic/HtmlBlock.test.ts b/craft/src/components/basic/HtmlBlock.test.ts index 4b9ebed..3aa46c3 100644 --- a/craft/src/components/basic/HtmlBlock.test.ts +++ b/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('

red text

'); + expect(out).toBe('

red text

'); + }); + + test('an id attribute survives sanitization (anchor targets)', () => { + const out = purifyHtml('link'); + expect(out).toContain('id="section"'); + }); + + test('a pasted table survives sanitization', () => { + const input = '
Head
Cell
'; + expect(purifyHtml(input)).toBe(input); + }); + + test('script tags still do not survive alongside a style attribute', () => { + const out = purifyHtml('

ok

'); + expect(out).not.toContain(' { + const out = purifyHtml('

x

'); + 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('x'); + 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(''); diff --git a/craft/src/components/basic/HtmlBlock.toHtml.test.ts b/craft/src/components/basic/HtmlBlock.toHtml.test.ts index dafc090..48e0e18 100644 --- a/craft/src/components/basic/HtmlBlock.toHtml.test.ts +++ b/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: '

red text

' }, ''); + expect(html).toBe('

red text

'); + }); + + test('a table inside `code` reaches exported output', () => { + const code = '
Cell
'; + const { html } = toHtml({ code }, ''); + expect(html).toBe(code); + }); +}); diff --git a/craft/src/components/basic/HtmlBlock.tsx b/craft/src/components/basic/HtmlBlock.tsx index 367d7f2..befbeca 100644 --- a/craft/src/components/basic/HtmlBlock.tsx +++ b/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', ], diff --git a/craft/src/hooks/useWhpApi.orphan-repair-wiring.test.tsx b/craft/src/hooks/useWhpApi.orphan-repair-wiring.test.tsx index e89c1a7..180b238 100644 --- a/craft/src/hooks/useWhpApi.orphan-repair-wiring.test.tsx +++ b/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 - * page-switch path. + * reattached to ROOT and a `console.error` must fire, exactly like the + * 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. - expect(warnSpy).toHaveBeenCalled(); - expect(warnSpy.mock.calls[0][0]).toContain('reattached'); + // orphan happened to be absent for some unrelated reason. (React's own + // act()-environment warnings also go through console.error in this + // 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(); }); }); diff --git a/craft/src/hooks/useWhpApi.ts b/craft/src/hooks/useWhpApi.ts index de87ba0..28f321c 100644 --- a/craft/src/hooks/useWhpApi.ts +++ b/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 + } } } diff --git a/craft/src/panels/left/PagesPanel.confirmStates.test.tsx b/craft/src/panels/left/PagesPanel.confirmStates.test.tsx new file mode 100644 index 0000000..871eed3 --- /dev/null +++ b/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 | null = null; + const Probe: React.FC = () => { ctx = usePages(); return null; }; + const harness = renderEditorHarness(); + harness.mountChild( + + + + , + ); + 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(); + }); +}); diff --git a/craft/src/panels/left/PagesPanel.tsx b/craft/src/panels/left/PagesPanel.tsx index f0ecd46..76ece38 100644 --- a/craft/src/panels/left/PagesPanel.tsx +++ b/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 = () => {