Repository navigation
feat(post-editor): composed template preview (#618) - #698
Merged
Merged
Conversation
Introduces the "composed view" feature that lets authors preview and edit content inside its resolved template without leaving the editor: - #619 backend: adds GET {resource}/{id}/applied-template. Resolves the content model's `template` attribute through the shared H5 TemplateResolver, returns the referenced template-parts inline, and signals empty/unknown-slug misses as 404 with a discriminated body so the client can trigger the fallback flow. - #620 top bar: adds the Content ⇄ With Template toggle to top-bar.tsx as an aria switch, wires ephemeral view-mode state on editor-app.tsx, and disables the toggle while the applied-template fetch is in flight. Every editor mount starts in `content` mode. - #621 composed view module: new resources/js/visual-editor/editor/composed-view/ containing the applied-template REST client, a pure `composeBlocks` function that expands template-part refs and locks chrome blocks, an inverse `extractContentBlocks` for round-tripping persistence, and a `useAppliedTemplate` hook that caches per (resource, id) so toggling is instant after the first fetch. Wired into editor-app.tsx: swaps the BlockEditorProvider tree without remounting the iframe canvas and extracts the content region back before every save. - #622 bindings: documents that the existing setBindingsResourceContext call already covers template-chrome bindings consumers (post-title/author/date/featured-image resolve against the current content), adds an unmount-clear effect for cross-mount hygiene, and pins the behavior with vitest coverage showing bindings are preserved on chrome blocks under composition + template-part expansion. Server: 6 new Pest tests (happy path, empty template, unknown slug, whitespace-only template, missing part, unauthenticated). Client: 6 new top-bar toggle tests, 8 compose tests, 4 useAppliedTemplate tests, 4 bindings-preservation tests. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two related changes to unblock the composed-view MVP after dev-app testing
surfaced problems with the initial cut:
1. Endpoint switched from 404 → 200 for the "missing template" signal.
The 404 was semantically correct but showed up as a scary error every
time the toggle was clicked on a resource without a template. The
response body still carries the same {status, reason} discriminator;
moving to 200 keeps devtools quiet on the routine case. Client fetch +
hook + tests updated to read `status` from the body.
2. Reverted the BlockEditorProvider value-swap composition. Feeding the
provider a composed tree (chrome + slot) fought Gutenberg's stateful
sync in ways every fix uncovered a new failure:
- hand-shaped blocks tripped block-validation warnings on every chrome
block (fixed by adding a `hydrateAppliedTemplate` helper that runs
each block through `wp.blocks.createBlock`);
- swapping `value` triggered onChange echoes that looped through
extractContentBlocks → onBlocksChange → new composed tree → new value
until the editor froze (even with a reference-shallow guard);
- `core/post-title` was in the slot-candidate set but is a leaf block
that can't accept innerBlocks, deepening the freeze.
Pivoted to render the resolved template as inert chrome preview
panels above / below the block canvas (`ChromePreviewPanel.tsx` +
`split.ts`). The block canvas is unchanged — one BlockEditorProvider,
one tree, existing persistence loop untouched — so selection, undo
history, and unsaved-changes survive by construction. The panels show
the block names in the header/footer as a first-cut "shape" preview.
This is knowingly a placeholder for the "real" in-canvas chrome
rendering the umbrella issue #618 promised. Filed #655 to track the
dual-tree rewrite that will replace it.
Foundation kept:
- `hydrateAppliedTemplate` (composed-view/hydrate.ts) — still useful for
the eventual real-rendering path; not currently called from editor-app.
- `composeBlocks` + `extractContentBlocks` (compose.ts + extract.ts) —
same. All 16 composed-view vitest cases still pass.
Follows: #655
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Rebased onto main (1.5.5) and cleared the blocking CodeRabbit findings.
- Cycle protection in both template-part expanders. `expandParts`
(split.ts) and `expandTemplateParts` (compose.ts) recursed without
tracking what they had already inlined, so a self- or
mutually-referencing part blew the stack and took the composed view
with it. Template parts are theme/DB content and can't be assumed
acyclic — the backend already guards its own walk but still ships the
circular parts in the flat map it returns. The visited set is passed
by value per branch so sibling refs to a shared part still expand.
- Validate the applied-template payload before trusting it. A 2xx that
wasn't a recognized `missing` body was cast straight to
`AppliedTemplate`, so an empty or truncated response surfaced as a
crash inside `splitTemplateAroundContentSlot` rather than at the
fetch. Now throws `ApiError`. Also corrects `AppliedTemplateResult`,
which described an `{ status, template }` wrapper the endpoint never
returned.
- Key the applied-template cache on the selected template. It was keyed
on `(apiBase, resource, id)` only, so switching templates in the
document panel kept serving the previous composed preview until the
editor remounted. Refetching alone would have raced the debounced
save and re-cached the *old* template under the new key, so the slug
now rides along as `?template=` and the endpoint resolves what the
editor asked for instead of what happens to be persisted. A blank
value is meaningful (selection cleared) and is distinct from omitting
the parameter (use the stored value).
- Banner the missing/error states. Composed mode with an unresolvable
template rendered bare content under a toggle still reading "With
template", which read as the toggle doing nothing. This is the
review-fix treatment only; the designed fallback (compose against a
minimal default template + toast) remains #624.
Tests: +6 split (new file, the critical path had no coverage), +3
compose, +5 use-applied-template, +5 Pest for the override contract.
Full suites green — 2492 vitest, 1610 pest.
Refs #619 #622 #654
Replaces the inert "list of block names" panel with the actual template header and footer, rendered as real blocks inside the canvas iframe. ## Approach #655 described this as a dual-tree persistence rewrite. It isn't one. `__experimentalUseBlockPreview` mounts its own block-editor provider around a foreign tree, so the chrome never touches the content provider's `value` — which is what the earlier composed-tree attempt did, and why it froze (value swap → onChange echo → extract → state → new value, forever). The hook also wraps the subtree in `useDisabled()`, so chrome is genuinely non-selectable rather than merely locked, and sets `isPreviewMode` while inheriting the parent's editor settings. It deliberately clears `styles` because it expects to render inside a canvas whose EditorStyles already ran — exactly our case. So: content canvas untouched, one provider, one tree, existing persistence loop. Selection, undo history, and unsaved-changes survive a toggle by construction rather than by guard. ## The reason chrome couldn't render before Not the tree wiring — the namespace. The editor has registered only `artisanpack/*` since the I7 cutover (#415), while theme templates on disk are authored in core markup (`wp:group`, `wp:heading`, `wp:template-part`). The endpoint returned those names untouched, so every chrome block resolved to an unregistered type. The controller now rewrites `core/x` → `artisanpack/x` across the tree, mirroring `BlockMarkupHydrator::aliasFor()`. Blanket rather than registry-gated: `BlockTypeRegistry` only knows server-rendered PHP blocks, and a core name with no fork is unregistered under either namespace anyway. Rewriting `core/template-part` also hands part resolution to the fork from #674/#675, which fetches its own part through the core-data shim. That makes the client-side expanders redundant, so they're gone. ## Splitting `splitTemplateAroundContentSlot` only scanned top level, which meant the conventional shape — header part, `group[main]` wrapping the slot, footer part — found no slot and dumped the entire template into the header. It now descends, and clones the wrapper onto both sides when the slot is nested inside one. An approximation by construction; #618 scopes this to "close enough to see the shape". Templates with no slot at all now say so in the banner instead of silently misplacing the content. ## Cycle protection Moving resolution into the block moves the recursion risk with it: a part referencing itself (or two referencing each other) would mount the component inside itself until the stack blows. Guarded with a context carrying the ancestor part ids. Per-branch, so sibling refs to a shared part still both resolve. ## Removed `ChromePreviewPanel.tsx`, `compose.ts`, `extract.ts` and their tests — the last of the abandoned composed-tree approach, now unreachable. `hydrate.ts` stops being dead code; it's what keeps chrome from tripping block validation. `bindings.test.ts` went with `compose.ts`, but the #622 guarantee it pinned now rides on hydration, so `hydrate.test.ts` re-pins it there. Tests: +8 split, +9 hydrate, +5 template-part cycles, +3 Pest for the namespace rewrite. Full suites green — 2494 vitest, 1612 pest, and the Vite build compiles. Closes #655 Refs #618 #621 #622 #675
Verifying #655 against the dev app's `dev-sample` theme turned up a hole the whole test suite missed: `TemplateResolver` returns `ResolvedTemplate::$blocks` empty for every theme-file source. Those templates carry their content as raw block markup in `$rawContent`, so the applied-template endpoint was reading an empty array and the composed view rendered no chrome at all — for exactly the case it exists to serve, a template that ships with the theme. The tests didn't catch it because they register templates through the `ap.visual-editor.templates` filter with a populated `blocks` key, which bypasses the raw path entirely. Resolving `page` from `dev-sample` returned `blocks: []`; it now returns the template-part ref and the `main` group with its three children, correctly forked. #675 hit the same wall in the site editor and fixed it inside `TemplateAdapter`. Rather than duplicate ~60 lines, the parse and the tree conversion move to `Support\ThemeBlockMarkup` alongside the core→fork rewrite, and `TemplateAdapter` delegates to it. Its #675 tests pass unchanged, which is the point of routing them through the same code. Coverage: `parseToEditorBlocks` needs cms-framework 2.5+ (PHP 8.3+), so the end-to-end controller test carries the same skip guard the adapter tests use. The parser-independent halves — tree conversion, the fork rewrite, blank input — are covered unconditionally in a new unit suite, including the namespace-boundary case (`corefoo/bar` must not rewrite). Tests: +13 unit, +2 feature. 1625 pest, 2494 vitest, build clean. Refs #655 #675
The site-editor template/template-part endpoints rewrote only `core/template-part` to its `artisanpack/*` fork. Since the I7 cutover (#415) registered the artisanpack namespace exclusively, every *other* core name in a template or part reached the editor unregistered and rendered nothing — so the composed view's header/footer chrome resolved correctly and then mounted blank. #674 patched the same hole for `core/template-part` alone. Widen it to the whole namespace via the existing `ThemeBlockMarkup::rewriteCoreToFork` so theme files can keep authoring core markup (the WP convention) without each consuming app pre-forking its own block names. The rewrite runs after NavigationBlockRefResolver on purpose: that resolver matches `core/navigation` by name, so forking first would hide every nav block from it and regress the Keystone #48 `ref` stamping. `content.raw` keeps the narrow template-part-only rewrite — it exists for reserialization, and forking every name there would write artisanpack namespaces back into theme files on save. Also surface an info notice while the composed view is active, naming the applied template and stating that only the content is editable. The chrome renders as real but inert blocks, and without a word for it an author reading a header they cannot click assumes the editor is broken. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The blanket `core/` → `artisanpack/` rewrite means templates and parts now serve `artisanpack/block` for a synced-pattern reference, but the usage counter matched the core name only — so every count read zero. That number backs the delete-confirmation dialog, where "used nowhere" is the most dangerous possible wrong answer. Match both names: the raw fallback path still runs `parse()`, which emits core names regardless of what the adapter did to the parsed tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
When the applied-template endpoint cannot resolve a template for the current content, the composed view previously rendered bare content behind a "no template chrome is shown" banner — the toggle read as having silently done nothing. All three unresolvable states now compose against a minimal built-in default template (title, featured image, content slot) and announce why: - `missing` / `empty` — content has no template selected - `missing` / `unknown-slug` — the selected slug resolves to nothing - `error` — the request failed outright The first two read as warnings, the third as an error, but all land on the same canvas so the toggle stays on and functional. A toast fires once per toggle-on event; the latch keys on the message rather than a boolean so picking a second broken template without leaving composed mode still announces the new failure. The standing alert repeats the copy for after the toast auto-dismisses. The composed chrome now carries `templateSlug` (null on the fallback) and `isFallback` so the #623 ribbon can show the fallback name and hide its Edit template CTA — there is nothing to deep-link to. Closes #624
…-missing-template-fallback Composed view: missing/invalid template fallback + toast
Adds the locked-canvas ribbon to the post editor's composed view and the query-string deep-link surface it targets in the site editor. The ribbon renders inside the canvas iframe (not the editor chrome) as a sticky bar naming the resolved template, with a single Edit template button that opens the site editor in a new tab. It is the only editing affordance for template chrome in v1 — individual template parts get no per-part badge. Composing against the built-in fallback template shows the name but hides the CTA, since that tree is a client-side constant with no site-editor record behind it. The site editor now parses `?entity=template&slug=…` on mount, resolves the slug to an entity id through the templates list endpoint, and navigates to that template's editor view. Path routing addresses entities by id and the applied-template endpoint returns only a slug, so the lookup keeps the router's single id-shaped contract instead of teaching it a second addressing mode. Handling is additive: no query string behaves exactly as before, and an unknown entity is a no-op rather than a throw. An unresolvable slug or a failed lookup lands on the Templates index with a toast naming the slug. Closes #623 Closes #625 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`site-editor-app.test.tsx` failed all 18 of its tests on `window.localStorage.clear()` with "Cannot read properties of undefined". jsdom implements Web Storage correctly; the global was being shadowed before the suite ever ran. vitest's `getWindowKeys()` filters jsdom's window properties with `if (k in global) return KEYS.includes(k)` — a key already present on Node's `globalThis` is only copied when it appears in vitest's own allow-list, and `localStorage` does not. Node 22+ ships an experimental Web Storage global whose accessor returns `undefined` unless the process was started with `--localstorage-file`, so on those versions the key is present, vitest skips it, and Node's inert stub shadows jsdom's working implementation: `'localStorage' in window` is true while `window.localStorage` is undefined. That is why this surfaced without a code change — on older Node the key is absent, vitest copies jsdom's version, and the suite passes. Install an in-memory `Storage` for `localStorage` and `sessionStorage` in the shared test setup, but only when the global is genuinely unusable, so a working jsdom implementation is never replaced. Defined via `defineProperty` rather than assignment because the property Node leaves behind is an accessor whose setter would swallow a plain write. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…on-and-site-editor-deep-link feat: composed-view ribbon and site-editor slug deep links
Rounds out coverage for the composed template preview and adds the user-facing docs page. Two regression tripwires carried over from #621's rewrite: - Chrome is genuinely inert. `chrome-blocks.test.tsx` asserts the chrome renders through `__experimentalUseBlockPreview` — with `BlockEditorProvider` / `BlockList` stubbed as tripwires — that no `lock` attribute is injected, and that the hook's inert markers reach the DOM. A lock-based reimplementation fails it. - Toggling preserves editor state. `toggle-preserves-state.test.tsx` mounts the real `EditorApp` and asserts the content provider mounts exactly once across toggles, its `value` is reference-identical on every render, and the toggle records no undo entry and leaves the save status idle. Reintroducing the abandoned composed-tree swap fails it. Also closes the #622 gap left when the rewrite deleted the old bindings tests: `bindings.test.ts` pins that title/author bindings survive split + hydrate (including when the slot splits a bound wrapper), and the app-level suite pins that the bindings resource context is set to the edited record and cleared on unmount, and that chrome shares the content's entity block context. Docs: new docs/post-editor/Composed-View.md covering what the view is, toggling, editable vs read-only chrome (and why it is a preview rather than a locked editable), the fallback template, and the Edit template deep-link contract. Cross-linked from post-editor.md, site-editor.md and the wiki index. Coverage for #623/#624/#625 already landed and is not duplicated. Closes #626 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…view-tests-and-docs test(composed-view): chrome inertness + toggle-state tripwires, bindings coverage, and Composed-View docs
H1 — useAppliedTemplate raced its own responses: a slower fetch for a previously-selected template could overwrite the current one, and a response arriving after unmount still called setState. Adds an AbortController per run, a captured cache key, staleness bail-outs after the await, and an unmount abort. fetchAppliedTemplate now accepts an optional AbortSignal. M2 (folded in — same catch block): the error state is no longer written to the cache, so a transient failure stops locking the author into the fallback for the rest of the mount. H2 — extractPartSlugs() only matched core/template-part, so templates saved from the site editor (which store artisanpack/template-part since the I7 cutover) returned an empty template_parts map. Now matches both namespaces, mirroring the JS side. H3 — raw-only theme template-parts were served with empty blocks and their nested refs never discovered. Adds editorBlocksForPart(), mirroring editorBlocksFor(), and routes both the envelope and the nested-ref walk through it.
M1 — a malformed `ok` payload white-screened the editor: the hydrate walk dereferenced unvalidated shapes inside a useMemo with no boundary above it. hydrateBlocks now skips nameless elements and treats an absent innerBlocks as empty, and isAppliedTemplatePayload validates block elements recursively so a bad payload lands in the hook's error path. M3 — the template override was sent unconditionally, but only pages surface the control, so posts and custom resources always sent the '' "selection cleared" sentinel and could never resolve their persisted template. Sends the override for pages only. M4 — the deep-link lookup was cancelled on unmount only, so a late resolution yanked a user who had navigated within the SPA. Bails when the pathname changed since the effect started. M5 — the deep-link landing pushed a history entry, leaving Back pointing at a query-string URL that handlePopState reads pathname-only. navigate() takes a `replace` option, which also skips the same-pathname early return so a stranded query string still gets stripped. M6 — a failed per-row detail fetch contributed 0 to the pattern usage count, which can report an in-use pattern as unused and greenlight deleting it. Rethrows into the dialog's existing error state. M8 — adds the missing backend coverage: 403, unknown resource, nonexistent id, array-shaped ?template=, whitespace-only ?template=. M2 landed with H1 (shared catch block). M7 (NavigationBlockRefResolver parity) is left for a maintainer design decision.
L1 — the applied-template route comment claimed 404 for the "missing" state; the controller deliberately returns 200 with a discriminated body and the client branches on it. Comment only. L2 — ThemeBlockMarkup::parseToEditorBlocks() trusted the cross-package parser's return type; guards with is_array so a future signature change there can't 500 every template read. L3 — renamed the loop variable in show() that shadowed the requested slug. L4 — normalized the controller's @SInCE tags to 1.6.0. L5 — a hung applied-template request permanently disabled the view-mode toggle. With H1's abort in place the toggle stays enabled; flipping back to content mode is always safe. L6 — the bindings resource context was set during render and cleared in an effect cleanup, leaving (null, null) after StrictMode's synthetic remount. Both halves now live in the effect. L7 — role="presentation" with aria-label is prohibited ARIA, so the chrome regions announced as nothing. Uses role="group". L8 — TemplatePartEdit called useBlockProps() only in the empty branch, so a block whose slug was filled in crashed React's hook ordering. Extracted to TemplatePartPlaceholder. L9 — buildTemplateDeepLink / buildSiteEditorPath now reject absolute, protocol-relative and javascript: route bases rather than emitting them into an <a href>. L10 — siteEditorRouteBase was a dead prop on ComposedViewRibbon; threaded from the mount dataset through EditorApp and EditorCanvas so custom-mount hosts get a working CTA. L11 — the entity_id deep-link shortcut skipped all slug verification; gated on an id-shaped value, and corrected the docblock that claimed nothing read it. L12 — deduped the not-found toast per slug. L13 — adds malformed-markup coverage at the parseToEditorBlocks boundary plus non-string blockName cases for convertParseBlocksTree.
- useAppliedTemplate: the cache-hit path settled a key without going through run(), leaving an older request in flight and unaborted — it could then land on top of the cached state. Aborts on that path and adds a latest-key comparison alongside the aborted check. - use-deep-link: the toast latch was held for the page's lifetime, so a later remount with the same slug landed on the templates index with no explanation (and the state leaked between tests). Released on the next macrotask, which still covers the StrictMode double-invoke window. - Docs/docblock: Composed-View.md and the TopBar `viewModeDisabledReason` docblock still described the toggle as disabled while the request is in flight, which L5 changed.
|
Warning Review limit reached
Next review available in: 20 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughAdds a composed post-editor view with applied-template resolution, read-only template chrome, fallback handling, caching, and site-editor deep links. It also adds template-part cycle protection, block namespace normalization, test fixtures, extensive coverage, and documentation. ChangesComposed template flow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant EditorUser
participant EditorApp
participant AppliedTemplateAPI
participant SiteEditor
EditorUser->>EditorApp: Select With Template
EditorApp->>AppliedTemplateAPI: Fetch applied template
AppliedTemplateAPI-->>EditorApp: Return template and parts
EditorApp->>EditorApp: Hydrate and split content slot
EditorApp-->>EditorUser: Render read-only chrome and live content
EditorUser->>SiteEditor: Open template edit link
SiteEditor-->>EditorUser: Resolve slug and open template editor
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
resources/js/visual-editor/site-editor/use-site-editor-routing.ts (1)
115-122: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRoute
parseSiteEditorPaththroughnormalizeRouteBase.
parseSiteEditorPathonly strips trailing slashes and leaves non-canonical bases such asvisual-editor/siteintact, whilebuildSiteEditorPathconverts those inputs to the canonical base. If the application stamps a non-canonicalrouteBase, parsed initial locations fall through to the default section while navigation writes URLs under the canonical base. CallnormalizeRouteBase(routeBase)before usingnormalizedBaseinparseSiteEditorPath.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/visual-editor/site-editor/use-site-editor-routing.ts` around lines 115 - 122, Update parseSiteEditorPath to call normalizeRouteBase(routeBase) before using normalizedBase, matching the normalization already performed by buildSiteEditorPath. Preserve the existing parsing behavior while ensuring non-canonical bases such as visual-editor/site resolve to the canonical route base.
🧹 Nitpick comments (12)
resources/js/visual-editor/site-editor/__tests__/use-site-editor-routing.test.tsx (1)
200-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the history spies from a teardown hook.
Lines 216-217 restore the spies only after every assertion passes. If the assertion at line 212, 213, or 214 fails, the test aborts and both spies stay installed on
window.historyfor the remaining tests in this file.setPath()at line 13 callswindow.history.replaceState, so a leaked spy accumulates calls and can turn one failure into several confusing ones.Move the restore into the existing
afterEachso it always runs.♻️ Proposed change
it('navigate({replace}) rewrites the current entry instead of pushing', () => { const pushState = vi.spyOn(window.history, 'pushState'); const replaceState = vi.spyOn(window.history, 'replaceState'); @@ expect(replaceState).toHaveBeenCalled(); expect(pushState).not.toHaveBeenCalled(); expect(window.location.pathname).toBe(`${ROUTE_BASE}/navigation`); - - pushState.mockRestore(); - replaceState.mockRestore(); });Then add the restore to the file's teardown, next to the existing
afterEach:afterEach(() => { vi.restoreAllMocks(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/visual-editor/site-editor/__tests__/use-site-editor-routing.test.tsx` around lines 200 - 218, Move history-spy cleanup out of the individual navigate({replace}) test and into the file’s existing afterEach teardown. Add vi.restoreAllMocks() there so pushState and replaceState spies are restored even when assertions fail, and remove the per-test mockRestore calls.docs/home.md (1)
53-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the new endpoint to the REST API Surface list.
This PR adds
GET /{resource}/{id}/applied-templateinroutes/api.phpat lines 68-70. The Content list at lines 118-121 does not mention it. Add the entry so the documented surface matches the routes.📝 Proposed addition
### Content - `GET/PUT /{resource}/{id}/content` — Block content CRUD for any registered resource +- `GET /{resource}/{id}/applied-template` — Resolved template chrome for the composed post-editor view - `POST /blocks/preview` — Dynamic block editor preview rendering🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/home.md` at line 53, Update the Content REST API Surface list in docs/home.md to include GET /{resource}/{id}/applied-template, alongside the existing content endpoints, so the documentation matches the route defined in routes/api.php.tests/Feature/SiteEditor/TemplatePartControllerTest.php (1)
215-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider mirroring the storage assertion from
TemplateControllerTest.This test pins the read-time fork to
artisanpack/navigation. It does not pin what the write path persisted.tests/Feature/SiteEditor/TemplateControllerTest.phplines 194-197 add that second assertion for templates. Without the equivalent here, a regression that rewrites part names before persisting would still pass.Add a
TemplatePartquery assertion that the storedblock_contentkeepscore/navigation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/SiteEditor/TemplatePartControllerTest.php` around lines 215 - 216, Extend the test around the existing `assertJsonPath` in the TemplatePart controller test with a TemplatePart database assertion verifying the persisted block_content retains `core/navigation`. Mirror the storage assertion pattern used by `TemplateControllerTest`, while keeping the read-time `artisanpack/navigation` assertion unchanged.src/Support/ThemeBlockMarkup.php (1)
59-70: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the
parse()method, not only the class.
class_exists()proves the parser class is loadable. It does not prove a public staticparse()method still exists with that name. The docblock states the goal is to survive a future signature change in cms-framework, but a renamed or non-staticparse()raises a fatalErroron every template read. Add amethod_exists()check next to the class check.♻️ Proposed guard
- if ( ! class_exists( $parserFqcn ) ) { + if ( ! class_exists( $parserFqcn ) || ! method_exists( $parserFqcn, 'parse' ) ) { return []; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Support/ThemeBlockMarkup.php` around lines 59 - 70, Update the parser guard in the relevant parsing method to require both class_exists($parserFqcn) and method_exists($parserFqcn, 'parse') before invoking the static parse method. Return the existing empty-array fallback when either check fails, while preserving the current parsed-array conversion behavior.resources/views/editor/mount.blade.php (1)
17-20: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDo not hardcode the site-editor route base in the editor mount.
editor/mount.blade.phpandsite-editor/index.blade.phpboth use the literal/visual-editor/site, while the SPA reads backdata-route-base. Passing this value instead of hardcoding it keeps the "Edit template ↗" CTA aligned when a host overrides the site-editor mount path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/views/editor/mount.blade.php` around lines 17 - 20, Replace the hardcoded "/visual-editor/site" value in the editor mount’s data-site-editor-route-base attribute with the configured or passed route-base value already used by the site editor mount, so both editor/mount.blade.php and site-editor/index.blade.php remain aligned when the host overrides the mount path.resources/js/visual-editor/editor/__tests__/top-bar.test.tsx (1)
494-528: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the accessible name, not only the ARIA state.
Both state cases check
roleandaria-checked. Neither checks the accessible name. That is the gap that let thearia-labeloverride the visible text (see the comment ontop-bar.tsxlines 568-585). Query the control by role and name so the label contract is covered.💚 Proposed test addition
expect(toggle).toHaveAttribute('role', 'switch'); expect(toggle).toHaveAttribute('aria-checked', 'false'); expect(toggle).not.toBeDisabled(); + // The accessible name must contain the visible label so + // speech-input users can target the control (WCAG 2.5.3). + expect( + screen.getByRole('switch', { name: /view with template/i }) + ).toBe(toggle);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/visual-editor/editor/__tests__/top-bar.test.tsx` around lines 494 - 528, Update the view-mode toggle queries in both tests within the top-bar test cases to locate the control by role “switch” and its expected accessible name, rather than only using getByTestId. Preserve the existing aria-checked and enabled-state assertions while covering the visible-label accessibility contract for both content and with-template states.resources/js/visual-editor/editor/composed-view/__tests__/split.test.ts (1)
116-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a malformed-payload case.
The suite covers every well-formed shape. It does not cover a payload element that is
nullor that has noname. That is the exact input that throws insplitBlockstoday (see the comment onsplit.tslines 100-113). Add a case so the guard stays in place after the fix.💚 Proposed test
+ it('skips malformed payload elements instead of throwing', () => { + const blocks = [ + null, + { attributes: {}, innerBlocks: [] }, + block('artisanpack/template-part', { slug: 'header' }), + block('artisanpack/post-content'), + ] as unknown as BlockInstance[]; + + const result = splitTemplateAroundContentSlot(blocks, 'T'); + + expect(result.slotFound).toBe(true); + expect(names(result.header)).toEqual(['artisanpack/template-part']); + }); +🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/visual-editor/editor/composed-view/__tests__/split.test.ts` around lines 116 - 127, Add a malformed-payload test alongside the existing splitTemplateAroundContentSlot cases, passing a blocks array containing a null element or an element without name to splitTemplateAroundContentSlot. Assert the function does not throw and preserves the expected no-slot result, including slotFound=false and the appropriate header/footer contents.resources/js/visual-editor/editor/composed-view/__tests__/toggle-preserves-state.test.tsx (1)
368-390: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a malformed-payload case to this suite.
This file already mounts the whole editor and controls
fetchAppliedTemplate, so it is the right place to prove that a bad payload does not white-screen the editor.splitTemplateAroundContentSlotruns beforehydrateBlocksand dereferencesblock.namewithout a guard (see the comment onsplit.tslines 100-113). A resolved payload containing anullblock currently throws inside thecomposedChromeuseMemo.💚 Proposed test
+ it('survives a malformed applied-template payload', async () => { + fetchAppliedTemplate.mockResolvedValue({ + ...APPLIED_TEMPLATE, + blocks: [null, ...APPLIED_TEMPLATE.blocks], + }); + + const user = userEvent.setup(); + await renderEditor(); + + await user.click(viewModeToggle()); + + // The editor must stay mounted and take the designed fallback + // path rather than throwing out of the composed-chrome memo. + await waitFor(() => { + expect(fetchAppliedTemplate).toHaveBeenCalled(); + }); + expect(screen.getByTestId('ap-stub-provider')).toBeInTheDocument(); + }); +🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/visual-editor/editor/composed-view/__tests__/toggle-preserves-state.test.tsx` around lines 368 - 390, Add a test in the “view-mode toggle availability” suite that makes fetchAppliedTemplate resolve with a payload containing a null block, then verifies the editor remains mounted and does not white-screen when composedChrome processes it. Use the existing renderEditor, fetchAppliedTemplate, and viewModeToggle helpers, and assert the malformed response is handled without an uncaught render error.resources/js/visual-editor/site-editor/__tests__/use-deep-link.test.tsx (1)
44-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset the module-level toast latch between tests.
use-deep-link.tskeepsannouncedForat module scope, and this suite does not clear it. The latch only clears on asetTimeout(…, 0)callback that the hook schedules. Each test therefore depends on that macrotask running before the next test starts.Today the slugs differ across tests, so no collision occurs. A future test that reuses a slug from an earlier test would fail intermittently, and the failure would point at the hook rather than at the ordering.
Flush a macrotask in
beforeEachso the latch is always clear at the start of a test.♻️ Proposed change to remove the ordering coupling
describe('useSiteEditorDeepLink', () => { - beforeEach(() => { + beforeEach(async () => { listEntities.mockReset(); // The landing compares against the pathname captured at mount, and // one test pushes a new one — reset so ordering can't leak. window.history.replaceState({}, '', '/visual-editor/site'); + // `use-deep-link.ts` holds its not-found latch at module scope and + // releases it on the next macrotask. Drain that macrotask so a + // suite that reuses a slug cannot inherit a set latch. + await new Promise((resolve) => setTimeout(resolve, 0)); });A cleaner alternative is a test-only reset exported from the hook module. That removes the timing dependency entirely.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/visual-editor/site-editor/__tests__/use-deep-link.test.tsx` around lines 44 - 50, Update the beforeEach setup in the useSiteEditorDeepLink test suite to flush the hook’s scheduled zero-delay macrotask before each test, ensuring the module-level announcedFor latch in use-deep-link.ts is cleared independently of prior test ordering. Preserve the existing mock reset and history reset behavior.resources/js/visual-editor/site-editor/use-deep-link.ts (1)
82-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the four ref writes out of the render body.
Lines 83, 85, 87, and 89 assign
ref.currentduring render. React Doctor reportsno-ref-current-in-renderas an error for each one. React can replay or discard render work, so a write made during render can leak from a tree that never commits.The mount effect reads these refs only asynchronously, after commit. A dedicated update effect therefore preserves the current behavior and satisfies the rule. Declare it before the mount effect so the refs hold the latest values on every commit.
♻️ Proposed refactor to keep render pure
const navigateRef = useRef(navigate); - navigateRef.current = navigate; const toastRef = useRef(toast); - toastRef.current = toast; const apiConfigRef = useRef(apiConfig); - apiConfigRef.current = apiConfig; const searchRef = useRef(search); - searchRef.current = search; + + // Refreshed after every commit rather than during render: React may + // replay or discard render work, and a write from a discarded render + // would leak. The mount effect below only reads these refs + // asynchronously, so a post-commit refresh is equivalent. + useEffect(() => { + navigateRef.current = navigate; + toastRef.current = toast; + apiConfigRef.current = apiConfig; + searchRef.current = search; + });Confirm that the project's React Doctor configuration treats
no-ref-current-in-renderas a blocking rule:#!/bin/bash # Description: Locate the React Doctor configuration and any existing suppressions of this rule. set -euo pipefail fd -t f -i 'react-doctor*' -x cat -n fd -t f -i 'eslint.config.*' -o -t f -i '.eslintrc*' -x rg -n 'react-doctor|no-ref-current-in-render' echo '--- other in-render ref writes already in the tree ---' ast-grep run --lang tsx --pattern '$REF.current = $VALUE' resources/js/visual-editor || true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/visual-editor/site-editor/use-deep-link.ts` around lines 82 - 89, Move the ref assignments for navigateRef, toastRef, apiConfigRef, and searchRef out of the render body into a dedicated update effect declared before the existing mount effect. Have that effect update all four refs after commit so asynchronous mount-effect reads always use the latest values, while preserving the current ref initialization and behavior.Source: Linters/SAST tools
resources/js/visual-editor/editor/editor-canvas.tsx (1)
148-180: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winKeep
BlockListat a stable tree position across the toggle.The two branches place
BlockListat different positions. In the bare branch it is the direct child ofObserveTyping. In the composed branch it is a child of a Fragment, preceded by the ribbon and the header chrome. React reconciles children by position, so togglingchromebetweennulland a value unmounts and remountsBlockList.The block-editor store keeps blocks and selection, so content is not lost. Transient DOM state inside the list is lost, including caret position and scroll offset.
Render one Fragment in both branches and make only the chrome conditional. Then
BlockListkeeps its position and its instance across the toggle.The comment on Line 148 states that the canvas never remounts across the toggle. That is true for the content provider
value, but not for theBlockListinstance. Update the comment together with the fix.♻️ Proposed refactor to stabilize the
BlockListposition- const blockList = - chrome === null || chrome === undefined ? ( - <BlockList layout={ROOT_CANVAS_LAYOUT} /> - ) : ( - <> - <ComposedViewRibbon - templateName={chrome.templateName} - templateSlug={chrome.templateSlug} - siteEditorRouteBase={siteEditorRouteBase} - /> - <ChromeBlocks - blocks={chrome.header} - region="header" - label={__('Template header (read-only)', TEXT_DOMAIN)} - /> - <BlockList layout={ROOT_CANVAS_LAYOUT} /> - <ChromeBlocks - blocks={chrome.footer} - region="footer" - label={__('Template footer (read-only)', TEXT_DOMAIN)} - /> - </> - ); + const hasChrome = chrome !== null && chrome !== undefined; + const blockList = ( + <> + {hasChrome ? ( + <> + <ComposedViewRibbon + templateName={chrome.templateName} + templateSlug={chrome.templateSlug} + siteEditorRouteBase={siteEditorRouteBase} + /> + <ChromeBlocks + blocks={chrome.header} + region="header" + label={__('Template header (read-only)', TEXT_DOMAIN)} + /> + </> + ) : null} + <BlockList layout={ROOT_CANVAS_LAYOUT} /> + {hasChrome ? ( + <ChromeBlocks + blocks={chrome.footer} + region="footer" + label={__('Template footer (read-only)', TEXT_DOMAIN)} + /> + ) : null} + </> + );Note: the leading conditional Fragment still shifts
BlockList's index between branches. To hold the index fixed, render three fixed slots instead:+ const blockList = ( + <> + {hasChrome ? ( + <ComposedViewRibbon + templateName={chrome.templateName} + templateSlug={chrome.templateSlug} + siteEditorRouteBase={siteEditorRouteBase} + /> + ) : null} + {hasChrome ? ( + <ChromeBlocks + blocks={chrome.header} + region="header" + label={__('Template header (read-only)', TEXT_DOMAIN)} + /> + ) : null} + <BlockList layout={ROOT_CANVAS_LAYOUT} /> + {hasChrome ? ( + <ChromeBlocks + blocks={chrome.footer} + region="footer" + label={__('Template footer (read-only)', TEXT_DOMAIN)} + /> + ) : null} + </> + );Each conditional slot holds a stable index because
nullstill occupies its position in a static children list.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/visual-editor/editor/editor-canvas.tsx` around lines 148 - 180, Update the blockList composition around BlockList so both bare and composed modes render the same static Fragment child structure with three fixed slots, using null for absent chrome; keep BlockList in the middle slot and conditionally render the ribbon/header/footer in the surrounding slots. Revise the nearby comment to accurately state that BlockList and the canvas retain their instances across composed-view toggles.resources/js/visual-editor/site-editor/deep-link.ts (1)
108-116: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winOpen Redirect (CWE-601): URL Redirection to Untrusted Site ('Open Redirect')
Reachability: Internal
Reachability path
● Entry resources/js/visual-editor/editor/editor-app.tsx:408 EditorAppShell: * Stages a metadata edit on the core-data entity record (when the * editor is mounted against a cms-framework Post/Page) and schedules * … │ ▼ ● Hop resources/js/visual-editor/editor/editor-canvas.tsx │ ▼ ● Hop resources/js/visual-editor/editor/canvas-styles.ts │ ▼ ● Hop resources/js/visual-editor/editor/composed-view/composed-view-ribbon.tsx:60 ComposedViewRibbon: translators: %s: template name. │ ▼ ● Sink resources/js/visual-editor/site-editor/deep-link.tsHarden
normalizeRouteBaseagainst scheme-relative backslashes and a base that carries its own query or fragment.
/\\hostis treated as scheme-relative for special schemes, and a base containing?or#can leave the appendedentity=...&slug=...parameters stranded or swallowed.Reject a leading
/followed by/or\, and reject any query/fragment characters before the builder appends the deep-link query.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/visual-editor/site-editor/deep-link.ts` around lines 108 - 116, Update normalizeRouteBase to reject route bases beginning with “/” followed by either “/” or “\”, and reject any routeBase containing “?” or “#” before normalization succeeds. Preserve the existing SITE_EDITOR_ROUTE_BASE fallback and valid single-slash base behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/site-editor.md`:
- Around line 158-172: Update the site editor routing documentation around the
path-route examples and the query-string deep-link description to reconcile
slug-based examples with the database-id claim. Explicitly state whether the
router produces slug routes or accepts both slug and ID forms; if both are
supported, clarify that the deep-link pushState rewrite uses the resolved
database ID, keeping the canonical route example consistent.
In `@resources/js/visual-editor/editor/composed-view/api.ts`:
- Around line 177-186: Update isAppliedTemplatePayload to validate every entry
in record.template_parts before accepting the payload. Require each part to be
an object with a blocks array whose block entries include a valid name, while
still allowing an empty template_parts map. Reject missing, null, or malformed
part blocks so hydration does not receive invalid data.
In `@resources/js/visual-editor/editor/composed-view/split.ts`:
- Around line 100-113: The splitBlocks loop must ignore malformed blocks before
dereferencing their fields: require block.name to be a string, and treat
non-array block.innerBlocks as absent, matching hydrateBlocks’ existing guard.
Apply the fix in
resources/js/visual-editor/editor/composed-view/split.ts:100-113;
resources/js/visual-editor/editor/editor-app.tsx:921-932 requires no direct
change because the split guard resolves the ordering gap.
In `@resources/js/visual-editor/editor/composed-view/use-applied-template.ts`:
- Around line 71-73: Move the latestKeyRef.current assignment out of the render
body and into the effect responsible for the cacheKey transition, before
invoking the request logic. Keep run’s closure-captured key and existing
staleness guard unchanged so discarded renders cannot invalidate the committed
request.
In `@resources/js/visual-editor/editor/top-bar.css`:
- Around line 162-172: Add a dedicated :focus-visible rule for
.ap-visual-editor-top-bar__view-mode-toggle, applying a visible background and
outline consistent with the existing top-bar focus styling while preserving the
toggle’s current default appearance.
In `@resources/js/visual-editor/editor/top-bar.tsx`:
- Around line 568-585: Update the view-mode toggle button in the
viewModeAvailable branch to remove its aria-label and let the visible “With
template”/“Content only” text provide the accessible name, while retaining
aria-checked for state.
In `@tests/Unit/VisualEditor/SiteEditor/Adapters/TemplatePartAdapterTest.php`:
- Around line 104-117: Move the templatePartParserAvailable() helper from
TemplateAdapterTest.php into the shared test bootstrap, such as tests/Pest.php,
so TemplatePartAdapterTest.php can use it when run independently. Remove the
original file-local declaration and preserve the existing availability check and
skip behavior.
---
Outside diff comments:
In `@resources/js/visual-editor/site-editor/use-site-editor-routing.ts`:
- Around line 115-122: Update parseSiteEditorPath to call
normalizeRouteBase(routeBase) before using normalizedBase, matching the
normalization already performed by buildSiteEditorPath. Preserve the existing
parsing behavior while ensuring non-canonical bases such as visual-editor/site
resolve to the canonical route base.
---
Nitpick comments:
In `@docs/home.md`:
- Line 53: Update the Content REST API Surface list in docs/home.md to include
GET /{resource}/{id}/applied-template, alongside the existing content endpoints,
so the documentation matches the route defined in routes/api.php.
In `@resources/js/visual-editor/editor/__tests__/top-bar.test.tsx`:
- Around line 494-528: Update the view-mode toggle queries in both tests within
the top-bar test cases to locate the control by role “switch” and its expected
accessible name, rather than only using getByTestId. Preserve the existing
aria-checked and enabled-state assertions while covering the visible-label
accessibility contract for both content and with-template states.
In `@resources/js/visual-editor/editor/composed-view/__tests__/split.test.ts`:
- Around line 116-127: Add a malformed-payload test alongside the existing
splitTemplateAroundContentSlot cases, passing a blocks array containing a null
element or an element without name to splitTemplateAroundContentSlot. Assert the
function does not throw and preserves the expected no-slot result, including
slotFound=false and the appropriate header/footer contents.
In
`@resources/js/visual-editor/editor/composed-view/__tests__/toggle-preserves-state.test.tsx`:
- Around line 368-390: Add a test in the “view-mode toggle availability” suite
that makes fetchAppliedTemplate resolve with a payload containing a null block,
then verifies the editor remains mounted and does not white-screen when
composedChrome processes it. Use the existing renderEditor,
fetchAppliedTemplate, and viewModeToggle helpers, and assert the malformed
response is handled without an uncaught render error.
In `@resources/js/visual-editor/editor/editor-canvas.tsx`:
- Around line 148-180: Update the blockList composition around BlockList so both
bare and composed modes render the same static Fragment child structure with
three fixed slots, using null for absent chrome; keep BlockList in the middle
slot and conditionally render the ribbon/header/footer in the surrounding slots.
Revise the nearby comment to accurately state that BlockList and the canvas
retain their instances across composed-view toggles.
In `@resources/js/visual-editor/site-editor/__tests__/use-deep-link.test.tsx`:
- Around line 44-50: Update the beforeEach setup in the useSiteEditorDeepLink
test suite to flush the hook’s scheduled zero-delay macrotask before each test,
ensuring the module-level announcedFor latch in use-deep-link.ts is cleared
independently of prior test ordering. Preserve the existing mock reset and
history reset behavior.
In
`@resources/js/visual-editor/site-editor/__tests__/use-site-editor-routing.test.tsx`:
- Around line 200-218: Move history-spy cleanup out of the individual
navigate({replace}) test and into the file’s existing afterEach teardown. Add
vi.restoreAllMocks() there so pushState and replaceState spies are restored even
when assertions fail, and remove the per-test mockRestore calls.
In `@resources/js/visual-editor/site-editor/deep-link.ts`:
- Around line 108-116: Update normalizeRouteBase to reject route bases beginning
with “/” followed by either “/” or “\”, and reject any routeBase containing “?”
or “#” before normalization succeeds. Preserve the existing
SITE_EDITOR_ROUTE_BASE fallback and valid single-slash base behavior.
In `@resources/js/visual-editor/site-editor/use-deep-link.ts`:
- Around line 82-89: Move the ref assignments for navigateRef, toastRef,
apiConfigRef, and searchRef out of the render body into a dedicated update
effect declared before the existing mount effect. Have that effect update all
four refs after commit so asynchronous mount-effect reads always use the latest
values, while preserving the current ref initialization and behavior.
In `@resources/views/editor/mount.blade.php`:
- Around line 17-20: Replace the hardcoded "/visual-editor/site" value in the
editor mount’s data-site-editor-route-base attribute with the configured or
passed route-base value already used by the site editor mount, so both
editor/mount.blade.php and site-editor/index.blade.php remain aligned when the
host overrides the mount path.
In `@src/Support/ThemeBlockMarkup.php`:
- Around line 59-70: Update the parser guard in the relevant parsing method to
require both class_exists($parserFqcn) and method_exists($parserFqcn, 'parse')
before invoking the static parse method. Return the existing empty-array
fallback when either check fails, while preserving the current parsed-array
conversion behavior.
In `@tests/Feature/SiteEditor/TemplatePartControllerTest.php`:
- Around line 215-216: Extend the test around the existing `assertJsonPath` in
the TemplatePart controller test with a TemplatePart database assertion
verifying the persisted block_content retains `core/navigation`. Mirror the
storage assertion pattern used by `TemplateControllerTest`, while keeping the
read-time `artisanpack/navigation` assertion unchanged.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f1f5624-0bdb-406a-8b22-96f13385aa60
📒 Files selected for processing (60)
database/migrations/testing/2026_07_14_000000_create_test_applied_template_table.phpdocs/home.mddocs/post-editor.mddocs/post-editor/Composed-View.mddocs/site-editor.mdresources/js/visual-editor/blocks/template-part/__tests__/edit-cycle.test.tsxresources/js/visual-editor/blocks/template-part/edit.tsxresources/js/visual-editor/editor-settings.tsresources/js/visual-editor/editor/__tests__/canvas-styles.test.tsresources/js/visual-editor/editor/__tests__/editor-canvas.test.tsxresources/js/visual-editor/editor/__tests__/top-bar.test.tsxresources/js/visual-editor/editor/canvas-styles.tsresources/js/visual-editor/editor/composed-view/ChromeBlocks.tsxresources/js/visual-editor/editor/composed-view/__tests__/bindings.test.tsresources/js/visual-editor/editor/composed-view/__tests__/chrome-blocks.test.tsxresources/js/visual-editor/editor/composed-view/__tests__/composed-view-ribbon.test.tsxresources/js/visual-editor/editor/composed-view/__tests__/fallback.test.tsresources/js/visual-editor/editor/composed-view/__tests__/hydrate.test.tsresources/js/visual-editor/editor/composed-view/__tests__/split.test.tsresources/js/visual-editor/editor/composed-view/__tests__/toggle-preserves-state.test.tsxresources/js/visual-editor/editor/composed-view/__tests__/use-applied-template.test.tsxresources/js/visual-editor/editor/composed-view/__tests__/use-fallback-toast.test.tsxresources/js/visual-editor/editor/composed-view/api.tsresources/js/visual-editor/editor/composed-view/composed-view-ribbon.cssresources/js/visual-editor/editor/composed-view/composed-view-ribbon.tsxresources/js/visual-editor/editor/composed-view/default-template.tsresources/js/visual-editor/editor/composed-view/fallback.tsresources/js/visual-editor/editor/composed-view/hydrate.tsresources/js/visual-editor/editor/composed-view/index.tsresources/js/visual-editor/editor/composed-view/split.tsresources/js/visual-editor/editor/composed-view/use-applied-template.tsresources/js/visual-editor/editor/composed-view/use-fallback-toast.tsresources/js/visual-editor/editor/editor-app.cssresources/js/visual-editor/editor/editor-app.tsxresources/js/visual-editor/editor/editor-canvas.tsxresources/js/visual-editor/editor/main.tsxresources/js/visual-editor/editor/top-bar.cssresources/js/visual-editor/editor/top-bar.tsxresources/js/visual-editor/site-editor/__tests__/deep-link.test.tsresources/js/visual-editor/site-editor/__tests__/use-deep-link.test.tsxresources/js/visual-editor/site-editor/__tests__/use-site-editor-routing.test.tsxresources/js/visual-editor/site-editor/deep-link.tsresources/js/visual-editor/site-editor/patterns/__tests__/use-pattern-usage.test.tsxresources/js/visual-editor/site-editor/patterns/use-pattern-usage.tsresources/js/visual-editor/site-editor/site-editor-app.tsxresources/js/visual-editor/site-editor/use-deep-link.tsresources/js/visual-editor/site-editor/use-site-editor-routing.tsresources/js/visual-editor/test-setup.tsresources/views/editor/mount.blade.phproutes/api.phpsrc/Http/Controllers/ResourceAppliedTemplateController.phpsrc/Http/Resources/Adapters/CmsFramework/SiteEditor/TemplateAdapter.phpsrc/Support/ThemeBlockMarkup.phptests/Feature/SiteEditor/TemplateControllerTest.phptests/Feature/SiteEditor/TemplatePartControllerTest.phptests/Feature/VisualEditor/ResourceAppliedTemplateControllerTest.phptests/Fixtures/TestAppliedTemplateModel.phptests/Unit/VisualEditor/SiteEditor/Adapters/TemplateAdapterTest.phptests/Unit/VisualEditor/SiteEditor/Adapters/TemplatePartAdapterTest.phptests/Unit/VisualEditor/Support/ThemeBlockMarkupTest.php
Comment on lines
+162
to
+172
| .ap-visual-editor-top-bar__view-mode-toggle { | ||
| display: inline-flex; | ||
| align-items: center; | ||
| padding: 6px 10px; | ||
| border: 1px solid var(--ap-visual-editor-top-bar-border); | ||
| border-radius: var(--ap-visual-editor-top-bar-radius); | ||
| background: transparent; | ||
| font-size: 13px; | ||
| color: var(--ap-visual-editor-top-bar-foreground); | ||
| cursor: pointer; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify a focus-visible style covers the new view-mode toggle.
set -euo pipefail
fd -t f 'top-bar.css' resources/js/visual-editor --exec rg -n 'focus|outline|view-mode-toggle|icon-button' {}Repository: ArtisanPack-UI/visual-editor
Length of output: 960
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- top-bar.css focus/selector context ---\n'
sed -n '70,90p;158,190p;216,230p' resources/js/visual-editor/editor/top-bar.css
printf '\n--- all top-bar CSS files ---\n'
fd -t f 'top-bar.*css$' resources/js/visual-editor -x sh -c 'printf "%s\n" "$1"; rg -n "ap-visual-editor-top-bar--*?:focus-visible|outline:|top-bar.*toggle|view-mode-toggle" "$1" || true' sh {}Repository: ArtisanPack-UI/visual-editor
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- top-bar.css focus/selector context ---'
sed -n '70,90p;158,190p;216,230p' resources/js/visual-editor/editor/top-bar.css
printf '%s\n' ''
printf '%s\n' '--- all top-bar CSS files ---'
fd -t f 'top-bar.*css$' resources/js/visual-editor -x sh -c '
printf "%s\n" "$1"
rg -n "ap-visual-editor-top-bar--*?:focus-visible|outline:|top-bar.*toggle|view-mode-toggle" "$1" || true
' sh {}Repository: ArtisanPack-UI/visual-editor
Length of output: 2498
Add :focus-visible styling for the view-mode toggle.
.ap-visual-editor-top-bar__view-mode-toggle is not covered by the existing top-bar :focus-visible rule, so keyboard focus falls back to the user-agent default. Add a visible background/outline focus style for this toggle.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@resources/js/visual-editor/editor/top-bar.css` around lines 162 - 172, Add a
dedicated :focus-visible rule for .ap-visual-editor-top-bar__view-mode-toggle,
applying a visible background and outline consistent with the existing top-bar
focus styling while preserving the toggle’s current default appearance.
- split.ts: the split runs before hydration, so `hydrateBlocks`' guard never covered it — a null or name-less element threw inside the chrome useMemo, the exact white-screen M1 was meant to close. Also treats a null `innerBlocks` as absent, which the response validator accepts. - api.ts: `template_parts` entries are now validated like the top-level block list. `hydrateParts` walks them the same way, and a null part or one missing `blocks` threw there instead of surfacing as an ApiError. `hydrateParts` itself also tolerates them, since `hydrateAppliedTemplate` is exported from the package index. - use-applied-template.ts: `latestKeyRef` was written during render. React can discard an uncommitted render, which would leave it pointing at a key that never took effect and drop the committed request as stale, stranding the hook in `loading`. Moved into the effect. - use-site-editor-routing.ts: `parseSiteEditorPath` now normalizes its base the same way `buildSiteEditorPath` does. The L9 hardening made them disagree, so a non-canonical base wrote URLs the parser read back as the default section. - top-bar: the view-mode switch keeps a constant visible label with `aria-checked` carrying state, so the accessible name matches the visible text (WCAG 2.5.3 Label in Name). Adds the missing `:focus-visible` outline. Docs updated for the new label. - tests/Pest.php: `templatePartParserAvailable()` moved out of TemplateAdapterTest, which made TemplatePartAdapterTest fatal when run on its own. Restores the history spies from `afterEach`. - docs: applied-template endpoint added to the REST surface list; reconciled the site-editor id/slug routing wording and corrected the deep-link landing from pushState to replace.
ViewFromTheBox
marked this pull request as ready for review
August 5, 2026 20:38
github-project-automation
Bot
moved this from In Review
to Done
in ArtisanPack UI Overview
14 of 28 tasks
14 of 28 tasks
ViewFromTheBox
added a commit
that referenced
this pull request
Aug 8, 2026
Applies the fixes from VISUAL_EDITOR_1.6.0_RELEASE_REVIEW.md. Release prep: - Bumped composer.json and package.json to 1.6.0, and both renderer sub-packages (react/vue) 1.1.0 -> 1.2.0 (both changed behaviour on this branch via #702 and the group flex fix below) - Converted the CHANGELOG [Unreleased] block to [1.6.0], added the four missing entries (#698 composed view, #679, #691, #692), the #690 low-trust-author advisory, and entries for the fixes below - Restamped the docs version marker in docs/home.md (1.5.5 -> 1.6.0) Renderer parity (#704): - React and Vue core/group now mirror group.blade.php's #595 flex-panel override, so a group using the Flex Layout panel with no stored layout no longer renders is-layout-flow there (#711) - All three renderers now accept the same three saved post-template layout shapes QueryInliner::postTemplateLayoutIsGrid() accepts, so the inliner can no longer stamp grid spans onto a flow wrapper - Blade clamps post-template columns to [1, 12] like the JS renderers - Declared the Blade-only photo-grid wrapper as a known divergence (#714) - Nine new fixtures and goldens covering all of the above plus - Both canonicalizers now spell out the same ASCII whitespace class (JS \s matches U+00A0, PCRE's does not), and a dropClassTokensMatching pattern that fails to compile throws instead of reading as "no match" - vitest no longer sweeps up the stale vendored copy of this repo under packages/visual-editor-renderer-blade/vendor/ Test hardening and editor-JS fixes: - AssetRouteTest now reaches the realpath() containment guard, which had no coverage on the path where it is the only defense - Composed-view fallback toast re-announces after a successful resolution; useAppliedTemplate caches per key in a Map, as documented - Site-editor warns once on a rejected routeBase instead of silently rewriting SPA navigation; the deep-link guard compares the query string so navigating away and back is not read as "nothing happened" - Corrected @SInCE on the applied-template testing migration Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Integration branch for the composed template preview in the post editor. Authors edit their content in place while the resolved template's header/footer render around it as inert, read-only chrome inside the same canvas iframe — so the preview picks up the theme's compiled CSS without a server-side render round-trip.
Collects the already-reviewed sub-PRs (#654, #694, #696, #697) covering #619–#626 and #655, plus a fix pass over a deep review of the whole branch.
Closes: #618
Type of Change
Related Issue
Issue: #618
Motivation and Context
Editing a page's content with no sense of the template around it means authors guess at how the header, footer and template chrome will frame what they write. Composed view puts the real resolved chrome in the canvas while keeping the content provider untouched — selection, undo history and unsaved changes survive a toggle structurally, which the earlier composed-tree approach could not manage without freezing the editor.
Changes Made
GET {resource}/{id}/applied-templateendpoint resolving a content item's template and its referenced template-parts, gated on the sameviewauthorization as the content endpoints. Returns 200 with a discriminated{status: 'missing', reason}body when nothing resolves, so the routine "no template chosen" case does not litter devtools with 404s.?entity=template&slug=…), resolved to an entity id on mount and rewritten to the canonical URL.useAppliedTemplate(abort + latest-key guard);extractPartSlugs()missedartisanpack/template-part, so templates saved from the site editor returned an empty parts map; raw-only theme template-parts were served with emptyblocksand their nested refs never discovered.role="presentation"+aria-labelcombination, a conditional hook inTemplatePartEdit, route-base hardening in the link builders, the deadsiteEditorRouteBaseprop threaded through, docblock/@sincecorrections.Deliberate, please confirm on review: the applied-template endpoint returns 200 with
{status: 'missing'}rather than 404. The client branches on that payload and the feature tests assertassertOk().How Has This Been Tested?
Testing Environment:
Tests Performed:
./vendor/bin/pest— 1,666 passed, 1 skipped (parser guard), 0 failures.npm test -- --run— 3,050 passed across 329 files, 0 failures.release/1.6, then the PR review pass; findings applied (see below).Not yet done — needs manual browser verification before merge:
Accessibility Tests Run
Details:
ARIA was reviewed at code level and one prohibited combination fixed: the chrome regions carried
role="presentation"together witharia-label, which drops the label — so "Template header (read-only)" announced as nothing. They now userole="group", covered by a test.No keyboard, screen-reader or contrast testing has been run on this branch yet; the composed chrome is inert and non-focusable, but that has not been verified against an actual screen reader.
Resolved since the first push (CodeRabbit PR review): the view-mode switch failed WCAG 2.5.3 (Label in Name) — visible text Content only / With template against an
aria-labelof View with template. It now carries a constant visible View with template label witharia-checkedholding the on/off state, so the accessible name matches the visible text and the switch's name describes what turning it on does. Docs updated to match. The switch also gained the:focus-visibleoutline the other topbar controls already had.Tests Added
Test details:
Every fix in the review pass ships with coverage. Backend additions in
ResourceAppliedTemplateControllerTest(fork-namespace part refs, raw-only parts, nested refs inside a raw-only part, 403, unknown resource, nonexistent id, array-shaped and whitespace-only?template=), plus malformed-markup cases at theparseToEditorBlocksboundary. Frontend additions cover the stale-response race, the cache-hit race, no error caching, malformed payload rejection, the template-override gate, deep-link navigation races and replace semantics, pattern-usage error propagation, the hook-ordering regression inTemplatePartEdit, and route-base hardening.The regression guards were each verified to fail with the fix reverted, rather than only to pass with it. Two candidate tests were discarded during that check because they passed either way and were replaced with ones that genuinely reproduce the bug.
Documentation
Documentation details:
docs/post-editor/Composed-View.mdanddocs/site-editor.mddocument the feature. Corrected the claim that the view-mode switch is disabled while the applied-template request is in flight — it now stays enabled, so a request that never settles cannot strand the author in composed view.Screenshots
None — pending the manual browser verification above.
Pre-Submission Checklist
Linting is unchecked because this package has no lint tooling configured (no Pint or php-cs-fixer in
require-dev, nolintscript) — PHP style was matched by hand: real tabs, spaces inside parens, Yoda conditions, aligned=>. Accessibility is unchecked per the section above.Opened as a draft pending the two manual browser checks under "How Has This Been Tested?".
Summary by CodeRabbit
New Features
Bug Fixes
Documentation