Files
site-builder/docs/superpowers/specs/2026-08-08-site-builder-user-reported-issues-design.md
shadowdaoandClaude Opus 5 69e61ab4b2 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>
2026-08-09 12:47:23 -07:00

297 lines
21 KiB
Markdown
Raw Permalink Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Site Builder — Five User-Reported Issues
**Date:** 2026-08-08
**Status:** Approved design, ready for implementation planning
**Repos touched:** `cloud-hosting-platform/site-builder` (`craft/`) and `cloud-hosting-platform/whp` (`web-files/`, `sql/`, `scripts/`)
## Background
Five issues came in from people using the WHP site builder:
1. Elements can be dropped outside the main page; once there they cannot be selected or deleted.
2. The HTML element exposes colour options that show in the builder but do not reach the live page.
3. Users want a way to reset a page to blank.
4. The Layers tab sometimes does not show everything.
5. There is no way to report a builder problem from inside the builder.
Two of these are already explained by the source:
- **#2 is a render/export mismatch, not just a stray control.** `HtmlBlock`'s React render applies its `style` prop, but `HtmlBlock.toHtml()` returns only the DOMPurify-cleaned `code` and ignores `style` entirely. Every style control the panel offers is therefore dead on export. The controls themselves come from `GenericPropsEditor`, the catch-all panel that `GuidedStyles` routes HTML to via its `isUtility` branch (shared with Divider and Spacer).
- **#4 is a data-model consequence.** `FeaturesGrid`, `Tabs`, `Accordion`, `PricingTable`, `Testimonials`, `Gallery`, `ContentSlider`, `NumberCounter`, `Menu`, `SocialLinks`, `Navbar` and `ContactForm` all hold their content in **array props**, not Craft child nodes, so they are genuinely leaf nodes in the Craft tree and `LayersPanel` has nothing to nest under them. `ColumnLayout` is the exception — it uses real `<Element canvas>` children, which is why columns nest correctly and nothing else does.
#1 is **not** diagnosed. Reproducing it is the first task of implementation, and the prevention fix follows the evidence rather than this document.
## Scope
In scope: the five items above, across the Craft.js editor, the WHP site-builder API, one new database table, and one new root-only admin page.
Out of scope: converting composite components from array props to real Craft children; any change to publish/export output beyond `HtmlBlock` no longer applying `style` in the editor; CSRF enforcement anywhere other than the one new endpoint.
---
## Item 1 — Elements dropped outside the page
### Invariant
**Every node in editor state is reachable from `ROOT`.** Enforced at three independent points so no single failure strands a block.
### 1a. Prevention (drop time)
A drop whose resolved target is not a `ROOT` descendant is rejected: a new block from `BlocksPanel` is never created, and a moved block returns to its origin.
The mechanism is deliberately left open pending a live reproduction. Two structural candidates, both to be tested against a real editor session before anything is written:
- Craft.js's `DefaultEventHandlers` retains the **last valid drop indicator** when the pointer leaves every droppable region. The gutter beside `.canvas-device-frame` (visible at tablet/mobile device widths) and the `pointer-events: none` header/footer `ZonePreview` bands are both places where the pointer is over the canvas area but over no Craft node.
- `Canvas.tsx` wraps `<Frame>` in a plain positioned `<div>`; drops landing on that wrapper resolve to no node.
**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:
- Parse the serialized craft state, walk every node's parent chain.
- Any node whose chain does not terminate at `ROOT` is reattached to the end of `ROOT`'s children.
- The repair is logged to the console with node id and type.
This is a **pure function** over serialized state — `repairOrphanNodes(serialized: string): { state: string; repaired: string[] }` — unit-testable with no browser and no Craft instance, following the precedent of `PageContext.treeToState.test.ts`.
### 1c. Recovery (always available)
`LayersPanel` gains an **Unplaced** group, rendered after the `ROOT` tree, listing any node in `state.nodes` not reachable from `ROOT`. Rows behave like normal layer rows: click to select, and the existing delete paths (context menu, Delete key) then work on them.
The group renders only when the set is non-empty, so ordinary sessions see no new UI.
### Error handling
If `repairOrphanNodes` throws on malformed state, it returns the input unchanged and logs — a page must never fail to load because the repair pass could not parse it.
---
## Item 2 — HTML block
### 2a. Only the Edit HTML control
- `GuidedStyles` gains an `isHtml` branch (`/^html$/i`) evaluated **before** `isUtility`, and `html` is removed from the `isUtility` regex so the two cannot both match.
- New `panels/right/styles/HtmlStylePanel.tsx` renders the Edit HTML button and nothing else.
- `HtmlCodeField` moves out of `GenericPropsEditor.tsx` into `panels/right/styles/HtmlCodeField.tsx` and is imported by `HtmlStylePanel`. `GenericPropsEditor` keeps `code` in `SKIP_PROPS` (harmless, and guards any future component carrying a `code` prop).
### 2b. Render must match export
`HtmlBlock`'s render stops applying the `style` prop. It keeps the selection outline and `minHeight: 40px` — both are editor affordances, not content.
This is the fix for the actual complaint. Removing the controls alone would leave already-styled blocks still rendering their dead styling in the editor and still not on the live page.
Existing stored `style` props are left in place: they are already ignored by `toHtml()`, so they have no output effect, and stripping them would mean touching every saved site.
### 2c. Smarter HTML editor
`ui/CodeEditor.tsx` already provides CodeMirror 6 with autocompletion, auto-closing tags and brackets, bracket matching, a fold gutter, `indentWithTab` and the one-dark theme. Nothing new is needed for tab-complete — but the component **lazy-loads CodeMirror and silently falls back to a plain `<textarea>`** when its chunks fail to load, which is a failure mode this project has shipped before. Verification is therefore a required task, not an assumption.
Additions:
- **Imperative handle on `CodeEditor`.** `insertAtCursor(text: string)` and `getValue()`, exposed via `forwardRef` + `useImperativeHandle`, dispatching a CodeMirror transaction at the current selection. In textarea-fallback mode the same handle operates on the textarea's `selectionStart`/`selectionEnd`, so the toolbar keeps working when CodeMirror is unavailable.
- **Toolbar** above the editor in the Edit HTML modal: insert `div`, `section`, heading, paragraph, link, unordered list, image. Each inserts a snippet at the cursor and places the caret inside it.
- **Colour control** in the toolbar: a swatch picker that inserts `style="color: #rrggbb"` at the cursor, so colour becomes part of the user's own markup — which does survive to the live page — instead of a dead panel prop.
- **Format button** running a small built-in prettifier: `formatHtml(src: string): string`, a pure indent-only formatter (block-level tags on their own lines, two-space indent, inline tags untouched, contents of `<pre>` preserved verbatim). No `js-beautify` dependency; unit-tested directly.
The Head Code modal is **not** changed by this work. It uses the same `CodeEditor`, so it inherits the imperative handle harmlessly, but no toolbar is added there.
---
## Item 3 — Reset
Both actions affect the editor/staging state only. The live site changes on **Publish**, and both confirmation dialogs say so.
### 3a. Reset Page
- Entry point: the per-page row in `PagesPanel`, alongside the existing rename/delete controls. The TopBar is already crowded and collapses to an overflow menu at ≤768px.
- Confirmation dialog naming the page.
- Implementation: `actions.deserialize(EMPTY_PAGE_STATE)` where `EMPTY_PAGE_STATE` is a serialized single empty `Container` matching what `Canvas.tsx` mounts. Going through `deserialize` puts the reset in Craft's history, so **Ctrl+Z restores the page**. The dialog says so.
- Only the active page's canvas is cleared. Header, footer, other pages, per-page SEO and design tokens are untouched.
- The control is offered on **page rows only**. The Header and Footer entries in `PagesPanel` do not get it — a blank header or footer is what the existing zone editing already allows, and a reset there would silently change every page on the site.
### 3b. Reset Entire Site
- Entry point: a danger zone at the bottom of `SiteDesignPanel`.
- Guard: the user types the site domain to enable the button, matching the pattern used elsewhere in WHP for destructive actions.
- Effect: all pages replaced by a single empty page named Home (slug `index`), header and footer cleared, design tokens restored to `SiteDesignContext` defaults, `headCode` cleared.
- **Not undoable.** The dialog states this explicitly and states that the live site remains as-published until the user publishes again.
- Per-page SEO is cleared with its page. Uploaded assets are **not** deleted — they are shared, referenced by URL, and deleting them is a separate destructive action with its own blast radius.
Auto-save runs every 30 seconds, so a reset persists to staging shortly after it happens. This is stated in the Reset Entire Site dialog.
---
## Item 4 — Layers tab completeness
### 4a. Virtual rows for array-prop content
A registry maps a component `displayName` to the prop holding its items and the per-item field to use as a label:
```ts
// panels/left/layers-virtual-rows.ts
export const VIRTUAL_CHILD_PROPS: Record<string, { prop: string; label: string; fallback: string }> = {
'Features Grid': { prop: 'features', label: 'title', fallback: 'Feature' },
Tabs: { prop: 'tabs', label: 'title', fallback: 'Tab' },
Accordion: { prop: 'items', label: 'title', fallback: 'Item' },
'Pricing Table': { prop: 'plans', label: 'name', fallback: 'Plan' },
Testimonials: { prop: 'testimonials', label: 'name', fallback: 'Testimonial' },
Gallery: { prop: 'images', label: 'alt', fallback: 'Image' },
'Content Slider': { prop: 'slides', label: 'title', fallback: 'Slide' },
'Number Counter': { prop: 'counters', label: 'label', fallback: 'Counter' },
Menu: { prop: 'links', label: 'text', fallback: 'Link' },
'Social Links': { prop: 'links', label: 'platform', fallback: 'Link' },
Navbar: { prop: 'links', label: 'text', fallback: 'Link' },
'Contact Form': { prop: 'fields', label: 'label', fallback: 'Field' },
};
```
Exact prop and label keys are **verified against each component's `craft.props` defaults during implementation**; the table above is the starting point, not the contract. A registry entry whose prop is absent or not an array yields no rows rather than throwing.
`deriveVirtualRows(displayName, props)` is a pure function returning `{ index, label }[]`, unit-tested independently of React.
Rendering: virtual rows appear as children of their node, visually distinguished from real nodes (dimmer text, an item glyph rather than a component icon, no disclosure triangle) so they do not read as separately draggable or deletable.
### 4b. Selecting a virtual row
Clicking one selects the parent node (so the right panel opens the correct array editor) **and** requests that item's card be scrolled into view. The request travels through a small React context (`{ nodeId, prop, index }`), which the array editors (`ArrayItemFields`, `FeaturesEditor`, `SectionTypePanel`) consume to `scrollIntoView` the matching card. If an editor does not consume it, the click still selects the parent — the scroll is an enhancement, never a dependency.
### 4c. Unplaced group
As described in Item 1c.
### 4d. Panel scrolling
`LayersPanel`'s tree gets an explicit scroll container so deep or long trees remain fully reachable. This was a secondary hypothesis for the report; it is cheap and correct regardless.
### Explicitly not doing
Converting composites to real Craft children. That is a stored-state migration across every existing customer site, for cosmetic tree parity, with real risk to published output.
---
## Item 5 — In-builder issue reporting
### 5a. Editor UI
- A bug icon in the TopBar (and in `TopBarOverflowMenu` for ≤768px) opens a Report an Issue modal.
- Fields: category (`bug` / `confusing` / `feature`), a required description, and a default-on checkbox **"Include this page's contents to help debugging"** with a one-line plain-language note about what that sends. Unchecking it omits the canvas state; everything else in the payload still goes.
- On success the modal shows **`Report #SB-1234 submitted`** and closes on acknowledgement.
- On failure it keeps the user's text and offers retry — a report must never be silently lost.
### 5b. Captured context
| Field | Source |
|---|---|
| `username` | server-side from `AUTH_USER`; never accepted from the client |
| `site_id`, `site_domain` | `WHP_CONFIG` |
| `page_id`, `page_slug` | `PageContext` |
| `editor_version` | build stamp (below) |
| `user_agent`, `viewport` | `navigator` / `window` |
| `device_mode` | current `DeviceMode` |
| `selected_type` | `displayName` of the selected node, or null |
| `console_errors` | last 20 entries from a ring buffer |
| `canvas_state` | `query.serialize()` of the current page, omitted if the checkbox is unchecked |
**Build stamp.** `vite.config.ts` gains a `define` for `__EDITOR_BUILD__`, set from the short git SHA plus build date at build time, so a report identifies exactly which bundle produced it. `craft/package.json`'s `2.0.0` is not build-specific and is not sufficient.
**Console error ring buffer.** Installed in `main.tsx` at boot: patches `console.error` and adds `window.onerror` / `unhandledrejection` listeners, retaining the last 20 entries as `{ ts, message }` with each message truncated to 500 characters. It always chains to the original handler. Message text only — no stack traces, which are minified and would leak file paths for little benefit.
**Cap.** The whole JSON payload is capped at 512 KB. When `canvas_state` would breach it, the canvas state is dropped and a `canvas_state_omitted: "size"` marker is sent so the report is not silently truncated into something misleading.
### 5c. Endpoint
`action=report_issue` added to `web-files/api/site-builder.php`, handled by `handleReportIssue($pdo, $isRoot)`:
- POST only.
- Session auth via the existing `AUTH_USER` / `HOME_DIR` gate at the top of the file.
- **CSRF validated** with `validate_csrf_token()` from `auto-prepend.php`. `WHP_CONFIG` already ships `csrfToken` to the editor. This is a new state-changing endpoint, so it validates now rather than waiting on the fleet-wide CSRF rollout.
- Ownership: a non-root user may only file against a site they own, checked the same way the other handlers in this file check site ownership.
- **Rate limit:** at most 5 reports per user per hour, enforced by counting recent rows in the reports table itself (the pattern `Sitesmith.php` uses). Over the limit returns HTTP 429 with a plain message.
- Validation: description 15000 characters after trimming; category must be one of the three; oversized `canvas_state` rejected rather than truncated.
- Response: `{ success: true, reference: "SB-1234", id: 1234 }`.
### 5d. Schema
New table, idempotent migration in `sql/migrations/staging/`, canonical schema regenerated per the `whp-add-migration` skill:
```sql
CREATE TABLE IF NOT EXISTS whp.site_builder_reports (
id INT AUTO_INCREMENT PRIMARY KEY,
created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP,
username VARCHAR(64) NOT NULL,
site_id INT NULL,
site_domain VARCHAR(255) NULL,
page_slug VARCHAR(255) NULL,
category ENUM('bug','confusing','feature') NOT NULL DEFAULT 'bug',
description TEXT NOT NULL,
editor_version VARCHAR(64) NULL,
user_agent VARCHAR(512) NULL,
viewport VARCHAR(32) NULL,
device_mode VARCHAR(16) NULL,
selected_type VARCHAR(64) NULL,
console_errors JSON NULL,
canvas_state LONGTEXT NULL,
status ENUM('new','triaged','fixed','wontfix') NOT NULL DEFAULT 'new',
admin_notes TEXT NULL,
INDEX idx_status_created (status, created_at),
INDEX idx_user_created (username, created_at)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;
```
`AUTO_INCREMENT` and the `PRIMARY KEY` are stated explicitly and must survive any future migration of this table.
### 5e. Admin page
`web-files/pages/site-builder-reports.php`, registered per the `whp-admin-page-registration` skill: `$allowed_pages`, `$page_permissions` (root only), a sidebar entry under the existing site-builder grouping, and a documentation entry.
- List view: reference, date, user, site, category, status, first line of the description. Filter by status and category.
- Detail view: full description and all captured context.
- Status and notes are editable inline.
- **"Copy for Claude"** button emitting one compact JSON object (context plus description, canvas state included only when present) for pasting straight into a session. This is what makes the feature pay off operationally.
---
## Testing
**Vitest (`craft/`), pure functions first:**
- `repairOrphanNodes` — orphan reattached, healthy state unchanged, malformed input returns input.
- `deriveVirtualRows` — each registry entry, missing prop, non-array prop, missing label field falls back.
- `formatHtml` — nesting, inline tags untouched, `<pre>` preserved, idempotent on already-formatted input.
- `buildReportPayload` — checkbox off omits canvas state, oversize drops canvas state and sets the marker, all context fields present.
- Console ring buffer — retains 20, truncates long messages, chains to the original handler.
**React component tests:** `HtmlStylePanel` renders only the Edit HTML control; `GuidedStyles` routes HTML to it and not to `GenericPropsEditor`; Reset Page dialog wiring; Unplaced group appears only when orphans exist; virtual rows select their parent.
**Export test:** an `HtmlBlock.toHtml` case asserting `style` never reaches output, and a render test asserting the editor no longer applies it — the two sides of the reported mismatch, locked down together.
**PHP:** a validation test script for `handleReportIssue` in the style of `scripts/test-site-builder-asset-urls.php` — category validation, description bounds, size cap, rate-limit counting. Follows the project's "verify through the web path, not bare `php -r`" rule for anything touching `open_basedir`-sensitive paths.
**Manual, on the canary (192.168.1.148) before any prod host:**
- `curl -s -o /dev/null -w '%{http_code}' <host>/site-builder/js/index2.js` returns 200 — CodeMirror chunks reachable, editor not degraded to the textarea fallback.
- File a report end to end and confirm it lands in `whp.site_builder_reports` and renders on the admin page.
- Reproduce the original off-canvas drop and confirm it is now impossible.
## Deployment
Standard path, owned by the `whp-deploy` skill:
1. `cd /workspace/site-builder/craft && npm run build`
2. Copy `dist/index.html`, `dist/css/editor.css` and **all** `dist/js/*.js` into `web-files/site-builder/` — copying only `editor.js` breaks the code editor at runtime.
3. Commit both repos (site-builder source, whp bundle).
4. `build-release.sh` on the build server.
5. Roll out 192.168.1.148 → whp02 → sdbees → whp01, verifying each before the next.
Published sites are static snapshots. None of these changes alter published output, so no site needs re-publishing as a result of this work.
## Risks
- **Item 1 root cause is unknown at design time.** If reproduction fails, prevention cannot be written responsibly. Repair (1b) and recovery (1c) still ship and still solve the user-visible problem — a stranded element becomes deletable — so the item is not blocked, but the plan must not pretend prevention landed if it did not.
- **`canvas_state` is customer content.** It is stored in the WHP database and readable by root only. The checkbox and its note exist so this is never a surprise.
- **Ring buffer patches `console.error` globally.** It must always chain to the original, or it silently swallows diagnostics for everyone.