Repository navigation
feat: D1 site-editor shell (routing, navigator, canvas frame, tabs) - #374
Merged
Merged
Conversation
Mounts the site-editor SPA at /visual-editor/site/{path?} with a four-
region shell: top bar, navigator (5 sections w/ persisted collapse),
iframe canvas (BlockCanvas + BlockEditorProvider, empty state for D1),
and inspector outlet. URL-per-scope routing uses history.pushState so
the section in the URL is the source of truth and browser back/forward
stay in sync.
Reuses the post-editor TopBar via two new optional aria-label props
(inserter/inspector toggle labels) — no fork. Adds the editing-mode
shell decision (Pattern B: persistent icon-only navigator + secondary
inserter rail + list-view in inspector) to the macro design brief as
§3.2.1 so D2–D5 inherit a fixed chrome.
Closes #368
📝 WalkthroughWalkthroughAdds a D1 site-editor shell: SPA boot/mount and Vite entry, route-backed section navigation, navigator sidebar with ARIA vertical tabs, iframe-based CanvasFrame with BlockEditor providers and empty state, inspector outlet, persisted sidebar toggles, TopBar aria-label overrides and tests, design doc editing-mode IA spec, styles, Blade route/view, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as User/Browser
participant Boot as bootSiteEditor()
participant Main as main.tsx
participant App as SiteEditorApp
participant Router as useSiteEditorRouting
participant Nav as NavigatorSidebar
participant Canvas as CanvasFrame
participant Block as BlockEditorProvider
Browser->>Boot: DOMContentLoaded or manual boot
Boot->>Main: find `[data-ap-site-editor]` and mount
Main->>App: dynamic import & render (routeBase, postEditorUrl)
App->>Router: init from current pathname
Router->>App: return {section, entityId}
App->>Nav: render tabs with active section
App->>Canvas: mount BlockEditorProvider / BlockCanvas (empty-state if no entity)
Nav->>Router: navigate(section) on tab activation
Router->>window.history: pushState(newPath)
Router->>App: update state -> rerender shell
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (5)
resources/js/visual-editor/site-editor/__tests__/main.test.tsx (1)
16-34: Guard console spy cleanup withfinally.This avoids cross-test contamination if the assertion path fails before
mockRestore()is reached.Proposed refactor
it('skips elements that are missing required data attributes', async () => { const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => { // suppress test-log noise — the warning is the assertion target. }); @@ - await bootSiteEditor(root); - - expect(errorSpy).toHaveBeenCalledWith( - expect.stringContaining('mount point is missing'), - incomplete - ); - - errorSpy.mockRestore(); + try { + await bootSiteEditor(root); + expect(errorSpy).toHaveBeenCalledWith( + expect.stringContaining('mount point is missing'), + incomplete + ); + } finally { + errorSpy.mockRestore(); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/site-editor/__tests__/main.test.tsx` around lines 16 - 34, The test creates a console.error spy (errorSpy) but calls errorSpy.mockRestore() only at the end, which can leak into other tests if an assertion fails; wrap the spy lifecycle in a try/finally: create errorSpy = vi.spyOn(console, 'error').mockImplementation(...), run the test logic including await bootSiteEditor(root) and the expect(...) inside try, and always call errorSpy.mockRestore() in the finally block so console.error is restored even on failures.resources/views/site-editor/index.blade.php (1)
18-18: Avoid hard-coding the route base in markup.Using a literal path here creates route drift risk. Derive it from the named route instead so Blade and route config stay in sync.
Proposed refactor
- data-route-base="/visual-editor/site" + data-route-base="{{ route('visual-editor.site-editor') }}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/site-editor/index.blade.php` at line 18, Replace the hard-coded data-route-base="/visual-editor/site" attribute with a value derived from the named route using Laravel's route() helper so the Blade view and route definitions stay in sync; locate the data-route-base attribute in resources/views/site-editor/index.blade.php and set its value using route('your.named.route') (or route('visual-editor.site.index') if that is the route name) ensuring proper Blade echoing/escaping.resources/js/visual-editor/site-editor/sections.tsx (1)
29-33:slugtyping and its comment are currently out of sync.Line 33 types
slugasSiteEditorSectionId, so it cannot diverge fromiddespite the comment saying alias slugs are supported in the future.🛠️ Optional alignment patch
- slug: SiteEditorSectionId; + slug: string;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/site-editor/sections.tsx` around lines 29 - 33, The comment says slug may diverge from id but the property slug is currently typed as SiteEditorSectionId which prevents aliases; change slug's type to a more permissive type (e.g., string or a new alias like SectionSlug = string | SiteEditorSectionId) and update the comment to reflect the chosen design so slug can be an alias while id remains the canonical SiteEditorSectionId; adjust references to slug in the SiteEditor section interface and any usages to satisfy the new type.resources/js/visual-editor/site-editor/__tests__/site-editor-app.test.tsx (1)
52-203: Add a keyboard-navigation test for the navigator tablist.The suite is strong, but it currently doesn’t assert ArrowUp/ArrowDown/Home/End behavior for section tabs. Adding that test will lock in the accessibility contract.
✅ Suggested test addition
+ it('supports ArrowDown keyboard navigation between section tabs', async () => { + const user = userEvent.setup(); + renderApp(); + + const templatesTab = screen.getByTestId('ap-site-editor-navigator-templates'); + templatesTab.focus(); + + await user.keyboard('{ArrowDown}'); + + expect(window.location.pathname).toBe(`${ROUTE_BASE}/template-parts`); + expect( + screen.getByTestId('ap-site-editor-navigator-template-parts') + ).toHaveAttribute('aria-selected', 'true'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/site-editor/__tests__/site-editor-app.test.tsx` around lines 52 - 203, Add a new test that verifies keyboard navigation for the navigator tablist by rendering the app with renderApp(), focusing the navigator tablist (getByTestId('ap-site-editor-navigator')) or a specific tab (e.g., getByTestId('ap-site-editor-navigator-templates')), then simulate ArrowDown/ArrowUp/Home/End keystrokes with userEvent.keyboard or fireEvent.keyDown and assert the focused/active tab changes accordingly (check aria-selected on 'ap-site-editor-navigator-templates', 'ap-site-editor-navigator-patterns', 'ap-site-editor-navigator-styles', etc. and that document.activeElement is the expected tab); use userEvent.setup() and the existing ROUTE_BASE expectations to ensure navigation updates the mode indicator ('ap-site-editor-mode-indicator') as needed.resources/js/visual-editor/site-editor/use-site-editor-routing.ts (1)
101-107: Recompute location whenrouteBasechanges.Line 101 initializes from
routeBaseonce, but in Line 109–123 the effect only rebinds listeners. IfrouteBasechanges, state can stay stale until the next navigation event.Suggested fix
useEffect(() => { if (typeof window === 'undefined') { return; } + setLocation(parseSiteEditorPath(window.location.pathname, routeBase)); + function handlePopState(): void { setLocation(parseSiteEditorPath(window.location.pathname, routeBase)); }Also applies to: 109-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/site-editor/use-site-editor-routing.ts` around lines 101 - 107, The initial state for location (useState with location and setLocation) only parses routeBase once; update the code so whenever routeBase changes you recompute and set location by calling parseSiteEditorPath(window.location.pathname, routeBase) (e.g., inside the existing effect that currently rebinds listeners), ensuring the effect depends on routeBase and calls setLocation accordingly so state never remains stale when routeBase updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/site-editor-ux.md`:
- Around line 525-552: The fenced code blocks for the "Edit mode (entity
selected)" and "Browse mode (no entity selected)" diagrams lack a language tag
and trigger MD040; update both opening triple-backtick fences (the blocks that
start the ASCII layout diagrams) to include a language such as "text" (e.g.,
```text) so the linter is satisfied, leaving the content and closing triple
backticks unchanged; look for the diagram blocks in site-editor-ux.md around the
ASCII diagrams shown under "Edit mode (entity selected)" and "Browse mode (no
entity selected)" and add the language token to each opening fence.
In `@resources/js/visual-editor/site-editor/__tests__/navigator-sidebar.test.tsx`:
- Around line 29-44: NavigatorSidebar currently leaves inactive tabs
non-tabbable and lacks Arrow/Home/End keyboard handling; update the component to
implement a roving tabindex pattern (set tabIndex=0 on the active item and
tabIndex=-1 on all other items) and add a keyDown handler to NavigatorSidebar
that handles ArrowLeft/ArrowRight (or ArrowUp/ArrowDown as appropriate), Home
and End to move focus and call onSelectSection; ensure you reference the
data-testids used in tests ('ap-site-editor-navigator-patterns' and
'ap-site-editor-navigator-templates') and the onSelectSection prop when wiring
selection changes. Then update/add tests in navigator-sidebar.test.tsx to assert
inactive items have tabIndex=-1, active has tabIndex=0, and to simulate
Arrow/Home/End key events to verify focus moves and onSelectSection is invoked.
In `@resources/js/visual-editor/site-editor/main.tsx`:
- Around line 21-23: The ready promise can resolve prematurely because the code
returns Promise.resolve() whenever a root exists even if the dynamic import is
still loading; update the mount/ready logic to track an in-flight readiness
promise on the host element (the MountableElement / its [ROOT_SYMBOL] entry)
instead of unconditionally returning Promise.resolve(). Specifically, in the
function that creates/attaches the root (referenced by ROOT_SYMBOL and
MountableElement), set a readyPromise property on that root/host when you start
the dynamic import, return that same promise for subsequent ready() callers, and
clear/replace it only after the import and initialization complete (and on
unmount). Ensure all paths that currently return Promise.resolve() check and
return the tracked readyPromise first.
In `@resources/js/visual-editor/site-editor/navigator-sidebar.tsx`:
- Around line 49-73: The tablist only sets tabIndex=0 for the active tab, making
other tabs unreachable by keyboard; add roving-tabindex/keyboard support by
making tab buttons all focusable (set tabIndex=0 for each), add an onKeyDown
handler on the tab list or each button to handle ArrowUp/ArrowDown (or
ArrowLeft/ArrowRight for horizontal), Home and End keys to move focus to the
previous/next/first/last section, and call onSelectSection(section.id) when
Enter/Space or navigation selects a tab; update the sections.map rendered
buttons (the element with role="tab" and data-section) to use this handler and
ensure aria-selected is updated via activeSection so screen readers stay in
sync.
In `@resources/js/visual-editor/site-editor/site-editor-app.css`:
- Line 12: Replace the `@import` that currently uses url(...) with the configured
string notation so stylelint passes: change the `@import` rule that references
'../editor/top-bar.css' (the `@import` in site-editor-app.css) to use quoted
string notation instead of url(...), keeping the same path and semantics.
In `@resources/js/visual-editor/site-editor/site-editor-app.tsx`:
- Around line 191-194: Change the plain div with id
"ap-site-editor-section-outlet" into a proper tabpanel by adding role="tabpanel"
and setting aria-labelledby to the currently active tab's id (the same id used
by tabs in navigator-sidebar.tsx via aria-controls); ensure the tabpanel
reflects the active tab state (use the component's active tab identifier—e.g.,
activeTabId/activeSectionId—from site-editor-app.tsx props/state) and include
tabIndex="0" (and hidden or aria-hidden when not active) so screen readers
correctly associate the panel with its controlling tab.
In `@resources/js/visual-editor/site-editor/use-site-editor-routing.ts`:
- Around line 47-79: The parser currently returns entityId as raw path text
which breaks round-trips for reserved chars; update parseSiteEditorPath so when
rest.length > 0 you set entityId = decodeURIComponent(rest.join('/')) (wrap
decode in try/catch and fall back to the raw join on decode errors) and ensure
any code that builds paths for the editor (the callers that compose pathname
from section.id and entityId) uses encodeURIComponent(entityId) when
constructing the URL; reference parseSiteEditorPath, the entityId variable,
routeBase and findSectionBySlug to locate the logic to change.
---
Nitpick comments:
In `@resources/js/visual-editor/site-editor/__tests__/main.test.tsx`:
- Around line 16-34: The test creates a console.error spy (errorSpy) but calls
errorSpy.mockRestore() only at the end, which can leak into other tests if an
assertion fails; wrap the spy lifecycle in a try/finally: create errorSpy =
vi.spyOn(console, 'error').mockImplementation(...), run the test logic including
await bootSiteEditor(root) and the expect(...) inside try, and always call
errorSpy.mockRestore() in the finally block so console.error is restored even on
failures.
In `@resources/js/visual-editor/site-editor/__tests__/site-editor-app.test.tsx`:
- Around line 52-203: Add a new test that verifies keyboard navigation for the
navigator tablist by rendering the app with renderApp(), focusing the navigator
tablist (getByTestId('ap-site-editor-navigator')) or a specific tab (e.g.,
getByTestId('ap-site-editor-navigator-templates')), then simulate
ArrowDown/ArrowUp/Home/End keystrokes with userEvent.keyboard or
fireEvent.keyDown and assert the focused/active tab changes accordingly (check
aria-selected on 'ap-site-editor-navigator-templates',
'ap-site-editor-navigator-patterns', 'ap-site-editor-navigator-styles', etc. and
that document.activeElement is the expected tab); use userEvent.setup() and the
existing ROUTE_BASE expectations to ensure navigation updates the mode indicator
('ap-site-editor-mode-indicator') as needed.
In `@resources/js/visual-editor/site-editor/sections.tsx`:
- Around line 29-33: The comment says slug may diverge from id but the property
slug is currently typed as SiteEditorSectionId which prevents aliases; change
slug's type to a more permissive type (e.g., string or a new alias like
SectionSlug = string | SiteEditorSectionId) and update the comment to reflect
the chosen design so slug can be an alias while id remains the canonical
SiteEditorSectionId; adjust references to slug in the SiteEditor section
interface and any usages to satisfy the new type.
In `@resources/js/visual-editor/site-editor/use-site-editor-routing.ts`:
- Around line 101-107: The initial state for location (useState with location
and setLocation) only parses routeBase once; update the code so whenever
routeBase changes you recompute and set location by calling
parseSiteEditorPath(window.location.pathname, routeBase) (e.g., inside the
existing effect that currently rebinds listeners), ensuring the effect depends
on routeBase and calls setLocation accordingly so state never remains stale when
routeBase updates.
In `@resources/views/site-editor/index.blade.php`:
- Line 18: Replace the hard-coded data-route-base="/visual-editor/site"
attribute with a value derived from the named route using Laravel's route()
helper so the Blade view and route definitions stay in sync; locate the
data-route-base attribute in resources/views/site-editor/index.blade.php and set
its value using route('your.named.route') (or route('visual-editor.site.index')
if that is the route name) ensuring proper Blade echoing/escaping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b0aaa11-442b-42f8-b778-615b72c37b87
📒 Files selected for processing (25)
docs/design/site-editor-ux.mdresources/js/visual-editor/editor/__tests__/top-bar.test.tsxresources/js/visual-editor/editor/top-bar.tsxresources/js/visual-editor/site-editor/__tests__/canvas-frame.test.tsxresources/js/visual-editor/site-editor/__tests__/main.test.tsxresources/js/visual-editor/site-editor/__tests__/navigator-sidebar.test.tsxresources/js/visual-editor/site-editor/__tests__/site-editor-app.test.tsxresources/js/visual-editor/site-editor/__tests__/use-site-editor-routing.test.tsxresources/js/visual-editor/site-editor/canvas-frame.cssresources/js/visual-editor/site-editor/canvas-frame.tsxresources/js/visual-editor/site-editor/inspector-outlet.cssresources/js/visual-editor/site-editor/inspector-outlet.tsxresources/js/visual-editor/site-editor/main.tsxresources/js/visual-editor/site-editor/navigator-sidebar.cssresources/js/visual-editor/site-editor/navigator-sidebar.tsxresources/js/visual-editor/site-editor/section-outlet.tsxresources/js/visual-editor/site-editor/sections.tsxresources/js/visual-editor/site-editor/site-editor-app.cssresources/js/visual-editor/site-editor/site-editor-app.tsxresources/js/visual-editor/site-editor/use-persisted-toggle.tsresources/js/visual-editor/site-editor/use-site-editor-routing.tsresources/views/site-editor/index.blade.phproutes/web.phptests/Feature/SiteEditorShellTest.phpvite.config.ts
- Roving-tabindex navigator gains ArrowUp/Down + Home/End keyboard handlers (WAI-ARIA APG `tabs` pattern, vertical orientation, automatic activation). Tabs now expose stable ids the tabpanel can reference. - Body region is marked role="tabpanel" with aria-labelledby pointing at the active tab's id, satisfying the tab/panel ARIA contract that aria-controls already implied. - mountSiteEditor caches the in-flight ready promise on the host so a second mount call on the same element waits for the same render commit instead of resolving immediately and racing the dynamic import. - parseSiteEditorPath decodes percent-encoded entity ids (with a raw fallback for malformed escapes); buildSiteEditorPath encodes them, so reserved characters round-trip cleanly. - CSS @import switched to string notation (stylelint default). - Two new ASCII diagrams in docs/design/site-editor-ux.md §3.2.1 are language-tagged (`text`) to satisfy MD040. Tests: - navigator-sidebar.test.tsx — roving-tabindex assertion + ArrowDown/Up with wrap, Home/End, and stable tab id wiring (10 tests, +5). - use-site-editor-routing.test.tsx — decode round-trip, malformed-input fallback, encode + parse symmetry (15 tests, +4). - site-editor-app.test.tsx — tabpanel role + aria-labelledby swap on section change (12 tests, +1).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/design/site-editor-ux.md (1)
531-535: Align inspector tab naming in the diagram with the rest of the brief.The ASCII mock uses
Docwhile the document standardizes inspector scopes asBlock / Entity / Section(see §3.9). Consider changingDoctoEntity(or explicitly definingDoc) to avoid ambiguity in downstream D-phase PRs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/site-editor-ux.md` around lines 531 - 535, The diagram uses the inspector tab label "Doc" which conflicts with the document's standardized inspector scopes (Block / Entity / Section in §3.9); update the ASCII mock in the table so the inspector tab shows "Entity" instead of "Doc" (or add a brief parenthetical defining "Doc" as "Entity") to make the naming consistent with the rest of the brief and avoid downstream ambiguity—locate the ASCII block containing "Block │ Doc" and replace "Doc" with "Entity" or append "(Entity)" as appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/design/site-editor-ux.md`:
- Around line 531-535: The diagram uses the inspector tab label "Doc" which
conflicts with the document's standardized inspector scopes (Block / Entity /
Section in §3.9); update the ASCII mock in the table so the inspector tab shows
"Entity" instead of "Doc" (or add a brief parenthetical defining "Doc" as
"Entity") to make the naming consistent with the rest of the brief and avoid
downstream ambiguity—locate the ASCII block containing "Block │ Doc" and replace
"Doc" with "Entity" or append "(Entity)" as appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1809de28-8fed-4de2-bc3b-d500919c35ca
📒 Files selected for processing (9)
docs/design/site-editor-ux.mdresources/js/visual-editor/site-editor/__tests__/navigator-sidebar.test.tsxresources/js/visual-editor/site-editor/__tests__/site-editor-app.test.tsxresources/js/visual-editor/site-editor/__tests__/use-site-editor-routing.test.tsxresources/js/visual-editor/site-editor/main.tsxresources/js/visual-editor/site-editor/navigator-sidebar.tsxresources/js/visual-editor/site-editor/site-editor-app.cssresources/js/visual-editor/site-editor/site-editor-app.tsxresources/js/visual-editor/site-editor/use-site-editor-routing.ts
✅ Files skipped from review due to trivial changes (1)
- resources/js/visual-editor/site-editor/site-editor-app.css
🚧 Files skipped from review as they are similar to previous changes (4)
- resources/js/visual-editor/site-editor/tests/navigator-sidebar.test.tsx
- resources/js/visual-editor/site-editor/navigator-sidebar.tsx
- resources/js/visual-editor/site-editor/site-editor-app.tsx
- resources/js/visual-editor/site-editor/tests/site-editor-app.test.tsx
…SiteEditor The success path of the dynamic-import promise already checks host[ROOT_SYMBOL] === root before rendering, so a slow first import that resolves after the host has been re-mounted leaves the newer root alone. The failure path was missing the same guard: a rejected import from an old mount would call unmountSiteEditor(host) unconditionally and tear down the current (newer) root. Mirror the guard so failure handling respects the same staleness rule.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@resources/js/visual-editor/site-editor/main.tsx`:
- Around line 77-80: The returned unmount closure currently closes over the
mutable host and can unmount a newer root when an old MountedSiteEditor handle
is used; fix by capturing the specific mount instance/root when you mount and
close over that immutable reference instead of the shared host (e.g. capture the
mount's root or the MountedSiteEditor instance and call unmountSiteEditor on
that captured value). Update both places that return { ready: ..., unmount: ()
=> unmountSiteEditor(host) } so unmount uses the mount-bound reference rather
than the outer host variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1782f5a9-805d-44cb-b5a2-1ea6e16bc277
📒 Files selected for processing (1)
resources/js/visual-editor/site-editor/main.tsx
Both unmount closures returned by mountSiteEditor previously called unmountSiteEditor(host), which reads host[ROOT_SYMBOL] every time. A caller holding a stale MountedSiteEditor handle (already-unmounted, or from a mount that has since been replaced via HMR / rapid re-boot) would therefore tear down whichever root happened to be on the host — including a newer one — instead of being a no-op. Capture the specific root at mount time and only unmount when it's still the live root on the host, mirroring the staleness guards the import success/failure paths already use. Adds a regression test that mounts → unmounts → re-mounts → re-calls the stale handle and asserts the live root survives.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
resources/js/visual-editor/site-editor/__tests__/main.test.tsx (1)
16-34: Harden console spy cleanup to avoid cross-test leakageIf this test fails before
mockRestore(), theconsole.errorspy can leak into subsequent tests.Proposed change
it('skips elements that are missing required data attributes', async () => { const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => { // suppress test-log noise — the warning is the assertion target. }); - - const root = document.createElement('div'); - const incomplete = document.createElement('div'); - incomplete.setAttribute('data-ap-site-editor', ''); - // route-base + post-editor-url intentionally absent. - root.appendChild(incomplete); - - await bootSiteEditor(root); - - expect(errorSpy).toHaveBeenCalledWith( - expect.stringContaining('mount point is missing'), - incomplete - ); - - errorSpy.mockRestore(); + try { + const root = document.createElement('div'); + const incomplete = document.createElement('div'); + incomplete.setAttribute('data-ap-site-editor', ''); + // route-base + post-editor-url intentionally absent. + root.appendChild(incomplete); + + await bootSiteEditor(root); + + expect(errorSpy).toHaveBeenCalledWith( + expect.stringContaining('mount point is missing'), + incomplete + ); + } finally { + errorSpy.mockRestore(); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/site-editor/__tests__/main.test.tsx` around lines 16 - 34, The console.error spy in the test can leak if the test throws before errorSpy.mockRestore(); wrap the spy usage so it is always restored—e.g., obtain the spy via vi.spyOn(console, 'error') before invoking bootSiteEditor and ensure errorSpy.mockRestore() runs in a finally block (or move spy setup/restore to a test-level afterEach) so that the spy is always cleaned up; reference the existing errorSpy, console.error and bootSiteEditor symbols when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@resources/js/visual-editor/site-editor/__tests__/main.test.tsx`:
- Around line 16-34: The console.error spy in the test can leak if the test
throws before errorSpy.mockRestore(); wrap the spy usage so it is always
restored—e.g., obtain the spy via vi.spyOn(console, 'error') before invoking
bootSiteEditor and ensure errorSpy.mockRestore() runs in a finally block (or
move spy setup/restore to a test-level afterEach) so that the spy is always
cleaned up; reference the existing errorSpy, console.error and bootSiteEditor
symbols when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a294c91c-322b-44a8-b557-61039f6ef310
📒 Files selected for processing (2)
resources/js/visual-editor/site-editor/__tests__/main.test.tsxresources/js/visual-editor/site-editor/main.tsx
github-project-automation
Bot
moved this from In Review
to Done
in ArtisanPack UI Overview
This was referenced Apr 24, 2026
12 of 13 tasks
18 of 28 tasks
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
D1 of the V1 site-editor expansion (#309). Mounts a dedicated site-editor SPA at
/visual-editor/site/{path?}with the four-region shell defined in the macro design brief (docs/design/site-editor-ux.md§3.2): top bar, navigator (5 sections), iframe canvas, and inspector outlet. D2–D5 plug per-section UI into this shell — no section content lands in this PR.Closes: #368
Type of Change
Related Issue
Issue: #368
Motivation and Context
Per the V1 plan §2.2, the site-editor shell is built in-house rather than depending on
@wordpress/edit-site, to keep admin-chrome ownership inside the package and avoid coupling our upgrade cadence to WP-admin's REST/CPT assumptions. D1 is the chrome scaffolding that makes D2 (templates + parts), D3 (styles), D4 (navigation), and D5 (patterns) implementable in parallel.Mid-review, we agreed on the editing-mode region model ("Pattern B" — persistent icon-only navigator + secondary inserter rail + list-view as inspector tab) so the chrome doesn't drift section-by-section as D2–D5 land. That decision is recorded in the brief as a new §3.2.1.
Changes Made
Route::get('/visual-editor/site/{path?}')(catch-all; SPA owns sub-paths).resources/views/site-editor/index.blade.phpmounts a[data-ap-site-editor]element withdata-route-base+data-post-editor-url.site-editor→resources/js/visual-editor/site-editor/main.tsx(auto-boots on the mount element; lazy-imports the shell so@wordpress/*lands in the existinggutenbergchunk).resources/js/visual-editor/site-editor/:site-editor-app.tsx— root shell composition.sections.tsx— single registry of the five sections (id, label, mode label, save label, icon). Save labels honor design brief §4.3 ("Save template", "Save template part", "Save pattern", "Save global styles", "Save menu").navigator-sidebar.tsx— left-rail tablist, ARIA-labelled navigation landmark,role="tab"items, vertical orientation.canvas-frame.tsx—BlockEditorProvider+BlockCanvas(iframe-isolated) with empty state when no entity is selected.inspector-outlet.tsx— right-rail placeholder with section-scoped header.section-outlet.tsx— small "Templates UI lands in D2" placeholder shown inside the navigator.use-site-editor-routing.ts— pure path parser +history.pushStatenavigation +popstatelistener for browser back/forward.use-persisted-toggle.ts— localStorage-backed boolean for the navigator/inspector collapse state.editor/top-bar.tsxreused directly (no fork). Added two backward-compatible optional props —inserterToggleAriaLabelandinspectorToggleAriaLabel— so the same component can host the site editor with semantically correct toggle labels ("Open navigator" / "Open inspector").docs/design/site-editor-ux.md§3.2.1 records the editing-mode region decision (Pattern B) so D2–D5 inherit a fixed chrome.How Has This Been Tested?
Tests Performed:
npm test— 580 tests pass (33 new for the shell across 5 files)../vendor/bin/pest— 341 tests pass (8 new inSiteEditorShellTest.phpcovering route registration, blade contents, vite entry wiring, shell composition, and the section registry).https://artisanpack-ui.test/visual-editor/site: all five sections resolve via URL + navigator clicks, mode indicator + save label swap, navigator/inspector collapse persists across reloads, browser back/forward syncs, back-to-post-editor link routes correctly.Accessibility Tests Run
Details: Navigator is a
<nav>landmark with an accessible name; section list is arole="tablist"witharia-orientation="vertical", each entryrole="tab"witharia-selected+ rovingtabIndex. Top-bar toggles usearia-pressed/aria-expanded/aria-labelwith site-editor-specific wording. Mode indicator is arole="status"aria-live="polite"region. Color contrast and screen-reader passes deferred to the per-section A11y reviews in D2–D5.Tests Added
Test details:
use-site-editor-routing.test.tsx— pure parser, hook init from URL, navigate/popstate, dedup of duplicate navigate.navigator-sidebar.test.tsx— section rendering, active selection, click handler, navigation landmark + tablist semantics.canvas-frame.test.tsx— empty state, BlockCanvas mount (mocked),hasEntityswap.site-editor-app.test.tsx— four-region render, default Templates landing, section switching updates URL + mode + save label, popstate sync, navigator/inspector toggles + persistence, back-link, default-disabled save, deep-link initialization, document.title sync.main.test.tsx— boot logs missing-attribute errors, mounts on valid element, idempotent re-mount.top-bar.test.tsx— new test for the optional aria-label override props.tests/Feature/SiteEditorShellTest.php— static wiring assertions (route, blade, vite entry, shell composition, sections registry, navigator landmarks, canvas BlockCanvas).Documentation
docs/design/site-editor-ux.md§3.2.1)Documentation details: Every new file carries a header docblock describing its role and pointing back to #368 / the design brief. The brief's new §3.2.1 records the Pattern B editing-mode decision and notes that D2–D5 PRs must reference it when landing editing UI.
Screenshots
(Manual review captured in browser — empty-state canvas + navigator + mode indicator; will attach if requested.)
Pre-Submission Checklist
Summary by CodeRabbit