Repository navigation
feat: D4 · Navigation editor screen + core/navigation enablement - #377
Merged
Merged
Conversation
Builds the Navigation section inside the D1 site-editor shell per design brief §3.8 and enables the `core/navigation` block in the template canvas. Backend additions: a `location` column on navigations with single-occupant assignment semantics, a `MenuLocationsController` (read-only locations + effective-menu resolution), and an `EntitySearchController` backing the link-control picker. Frontend: a native tree editor (drag-sort with nested re-parenting via the dnd-kit "tree" recipe), entity + item inspector panels, locations panel, and create-menu dialog — all sharing the existing `.ap-site-editor__dialog-*` form-field styles. Closes #371 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a full Navigation (D4) editor: backend APIs for menu locations and entity search, DB-backed per-location assignments, controller/validation updates, and a complete frontend navigation section including browser, canvas tree editor, inspector, API client, utilities, styles, tests, and site-editor integration. Also removes Changes
Sequence DiagramsequenceDiagram
actor User
participant Canvas as NavigationCanvas (UI)
participant Editor as useNavigationEditor (Hook)
participant API as api-client.ts
participant Backend as Laravel Controllers
participant DB as Database
User->>Canvas: Open menu / Edit tree / Click Save
Canvas->>Editor: save()
Editor->>API: updateNavigation(id, payload)
API->>Backend: PUT /visual-editor/api/navigation/{id}
Backend->>DB: START TRANSACTION\n(ensure unique location: clear other records)
Backend->>DB: UPDATE visual_editor_navigations SET ...
Backend->>DB: COMMIT
Backend->>API: 200 JSON NavigationRecord
API->>Editor: return NavigationRecord
Editor->>Canvas: update state (lastSavedAt, saveStatus)
User->>Canvas: Drag item / Project drop
Canvas->>Canvas: flattenTree() -> projectDrop -> commitProjection
Canvas->>Editor: setTree(newTree)
Editor->>Editor: mark dirty / prepare serialized blocks
User->>Canvas: Open LinkPicker & type query
Canvas->>API: searchEntities({type,q})
API->>Backend: GET /visual-editor/api/search?type=...&q=...
Backend->>DB: SELECT ... LIMIT 20
Backend->>API: return results
API->>Canvas: filtered SearchResult[]
Canvas->>Editor: set selected target on choose
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 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: 5
🧹 Nitpick comments (11)
tests/Feature/VisualEditor/MenuLocationResolverTest.php (1)
240-262:effectiveAssignments()test should assert all configured locations.The test title promises per-location reporting, but it only verifies
primary. Addfooter/count assertions so regressions aren’t missed.Suggested assertion tightening
expect( $assignments )->toHaveKey( 'primary' ) + ->toHaveKey( 'footer' ) + ->toHaveCount( 2 ) ->and( $assignments['primary']->id )->toBe( $primary->id );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Feature/VisualEditor/MenuLocationResolverTest.php` around lines 240 - 262, The test for MenuLocationResolver::effectiveAssignments currently only checks the 'primary' location; update the spec in MenuLocationResolverTest.php to assert all configured locations are reported by asserting the $assignments array has count 2, hasKey('footer') in addition to hasKey('primary'), and that $assignments['footer'] is null (or otherwise reflects an unassigned location) so the test verifies both presence and expected value for the footer location as well as the primary assignment.resources/js/visual-editor/site-editor/site-editor-app.tsx (1)
264-275: Prefer a single shared “open entity” callback.Lines 270-275 duplicate the logic later in Lines 330-335. Hoisting one shared callback above the hook calls would reduce drift risk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/site-editor/site-editor-app.tsx` around lines 264 - 275, There are two duplicated callbacks—handleNavigationOpen and handleOpenEntity—that both call routing.navigate(activeSection.id, entityId); remove the duplicate by hoisting a single shared callback (e.g., declare handleOpenEntity above the hook calls) and use that same function where handleNavigationOpen is currently passed to the navigation views hook; update references to call handleOpenEntity (or rename consistently) and delete the unused handleNavigationOpen to avoid TDZ issues and drift.src/Http/Requests/UpdateNavigationRequest.php (1)
94-119: Consider extractinglocationSlugRule()into a shared rule class.This closure is mirrored in another request class; duplicating slug-validation logic in multiple places increases drift risk over time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/Requests/UpdateNavigationRequest.php` around lines 94 - 119, The duplicate closure in locationSlugRule() should be extracted into a reusable validation rule class (e.g., MenuLocationRule) that encapsulates the logic of resolving MenuLocationResolver and checking the value against array_keys($resolver->locations()); implement the Laravel validation Rule interface (or make it invokable) so both StoreNavigationRequest and UpdateNavigationRequest can use new MenuLocationRule() instead of duplicating the closure, and update the requests to replace calls to locationSlugRule() with the new rule class.resources/js/visual-editor/site-editor/navigation/navigation-canvas.tsx (1)
439-453: Consider removingaria-pressedfrom the row selection button.The parent
<li role="treeitem">already hasaria-selected(line 338), which is the semantically correct attribute for tree item selection. Thearia-pressedon this inner button (line 443) is typically for toggle buttons that stay pressed, which could confuse screen readers. Since the treeitem selection is already announced viaaria-selected, this inner button could usearia-currentor simply omit the pressed state.This is a minor accessibility refinement, not a blocker.
♿ Consider removing aria-pressed
<button type="button" className="ap-nav-canvas__row-main" onClick={handleSelect} - aria-pressed={isSelected} data-testid={`ap-nav-canvas-row-${item.localId}`} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/site-editor/navigation/navigation-canvas.tsx` around lines 439 - 453, Remove the incorrect aria-pressed on the inner button used for row selection: in the button element that uses onClick={handleSelect} and data-testid={`ap-nav-canvas-row-${item.localId}`}, delete aria-pressed={isSelected} (or replace it with aria-current if you want to indicate the currently focused/active row) because the parent tree item already exposes selection via aria-selected; keep handleSelect and other attributes unchanged.resources/js/visual-editor/site-editor/navigation/navigation-browser.tsx (1)
124-127:itemCountForRowrecalculates on each render.This helper function is defined inside the component and recalculates for each row on every render. For typical menu counts (under 50 rows with shallow trees), this is fine. If performance becomes a concern, it could be memoized per row.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/site-editor/navigation/navigation-browser.tsx` around lines 124 - 127, The helper itemCountForRow currently defined inside the component recalculates on every render; change it to a memoized implementation so counts are computed only when a row's blocks change—e.g., replace the inline const itemCountForRow = (row) => countNavigationLeaves(row.content.blocks) with a memoized function using useCallback or compute a memo Map via useRef/useMemo keyed by row id (or row.content.blocks) and call countNavigationLeaves only when that key changes; reference the existing itemCountForRow and countNavigationLeaves symbols and ensure dependencies include the specific row content so repeated renders reuse cached counts.src/Http/Controllers/NavigationController.php (1)
127-132: Consider atomicity:releaseLocationFromOtherRecordsruns beforesave().If two concurrent requests try to assign the same location to different navigations, both could pass the release step before either commits their save. This could result in duplicate location assignments until the next assignment/release cycle.
For a V1 admin UI this is likely low-risk, but for robustness you could either:
- Wrap the release + save in a transaction, or
- Move the release call after
save()(though this briefly leaves duplicates).Given the resolver's
forLocationalready handles the "multiple records" case by picking the first match, this is acceptable as-is for V1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/Controllers/NavigationController.php` around lines 127 - 132, The current flow in NavigationController calls releaseLocationFromOtherRecords($navigation) before $navigation->save(), which can allow a race where two concurrent requests both release and then save the same location; to fix, wrap the release + save in a single database transaction (use DB::transaction or the app's transaction helper) so releaseLocationFromOtherRecords($navigation) and $navigation->save() are atomic, or alternatively move the call to releaseLocationFromOtherRecords($navigation) to after $navigation->save() if you prefer a simpler approach; update the code around the releaseLocationFromOtherRecords and save calls to use the transaction API or reorder them accordingly.resources/js/visual-editor/site-editor/navigation/create-menu-dialog.tsx (1)
136-139: UnnecessaryuseMemofor static translation.The
titleLabelis just a call to__()with no dependencies. The empty dependency array means it only computes once, butuseMemoadds overhead here without benefit — the translation string is stable.♻️ Simplify by removing useMemo
- const titleLabel = useMemo( - () => __('Menu name', TEXT_DOMAIN), - [] - ); + const titleLabel = __('Menu name', TEXT_DOMAIN);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/site-editor/navigation/create-menu-dialog.tsx` around lines 136 - 139, The titleLabel variable is wrapped in an unnecessary useMemo; replace the useMemo call with a direct assignment like const titleLabel = __('Menu name', TEXT_DOMAIN) in create-menu-dialog.tsx (remove the useMemo import usage if no longer used) so the static translation is computed directly without memoization overhead.resources/js/visual-editor/site-editor/navigation/navigation-inspector.tsx (1)
342-355: Redundantidon checkbox wrapped by label.The
<input>hasid={newTabId}but it's already wrapped by the<label>, which provides implicit association. Theidis unused since there's no externalhtmlForreference.♻️ Remove unused id
<div className="ap-site-editor__dialog-field"> <label className="ap-nav-inspector__field-checkbox"> <input - id={newTabId} type="checkbox" checked={item.opensInNewTab} onChange={(event) => onPatch({ opensInNewTab: event.target.checked }) } data-testid="ap-nav-inspector-new-tab" /> {__('Open in new tab', TEXT_DOMAIN)} </label> </div>And remove the unused
useIdcall:const labelId = useId(); const classId = useId(); const relId = useId(); - const newTabId = useId();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/site-editor/navigation/navigation-inspector.tsx` around lines 342 - 355, Remove the redundant id on the checkbox and the unused useId call: in navigation-inspector.tsx remove id={newTabId} from the input element (the checkbox is already implicitly associated by being wrapped in the <label> within the Nav inspector component) and delete the corresponding useId import and the newTabId variable (the useId() call) so there are no unused identifiers remaining.resources/js/visual-editor/site-editor/navigation/navigation-section.tsx (1)
186-232: Consider error handling granularity for location assignment.The
handleAssignLocationcatches errors and setslocationsError, but this error persists until the next locations fetch. If the user tries another assignment, the error from the previous attempt remains visible. Consider clearinglocationsErrorat the start of the handler.♻️ Clear error before assignment attempt
const handleAssignLocation = useCallback( async ( locationSlug: string, navigationId: number | null ): Promise<void> => { + setLocationsError(null); + // Two-step write semantics: the chosen menu gets the new // slug; whoever already had the slug is released by the // backend. If the user picked "None" we still need to find // the current owner and clear it. try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/site-editor/navigation/navigation-section.tsx` around lines 186 - 232, The handler handleAssignLocation leaves previous errors visible because setLocationsError is only called on catch; clear the prior error at the start of the function (before the try) by calling setLocationsError with the "no error" value used in this component (e.g., null or empty string), so the UI resets when a new assignment begins; update handleAssignLocation (and reference setLocationsError) accordingly.src/Http/Controllers/EntitySearchController.php (1)
61-73: Unused closure parameters flagged by static analysis.The
$templateand$partparameters in the URL resolver closures are unused. PHP 8+ allows omitting unused parameters with$_or using no parameter when not needed.♻️ Use shorter closure syntax
protected function builtInSources(): array { return [ 'template' => [ 'model' => VisualEditorTemplate::class, - 'url' => fn ( Model $template ): ?string => null, + 'url' => fn (): ?string => null, ], 'template-part' => [ 'model' => VisualEditorTemplatePart::class, - 'url' => fn ( Model $part ): ?string => null, + 'url' => fn (): ?string => null, ], ]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/Controllers/EntitySearchController.php` around lines 61 - 73, The builtInSources() method defines URL resolver arrow functions with unused parameters ($template and $part) which static analysis flags; update the 'url' entries for VisualEditorTemplate and VisualEditorTemplatePart to use no-argument arrow functions (e.g., fn(): ?string => null) or a single intentionally unused parameter like fn($_): ?string => null so the closures no longer declare unused variables, keeping the surrounding array keys and model references (builtInSources, VisualEditorTemplate, VisualEditorTemplatePart) intact.resources/js/visual-editor/site-editor/navigation/link-picker.tsx (1)
195-238: Consider keyboard navigation for listbox results.The results use
role="listbox"withrole="option"children, but keyboard navigation (arrow keys, Enter to select) is not implemented. Currently users must click. This is acceptable for V1 but could be a follow-up accessibility improvement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/site-editor/navigation/link-picker.tsx` around lines 195 - 238, The listbox results currently render an ul with role="listbox" and li role="option" but have no keyboard navigation; add keyboard handling so arrow keys move focus among results and Enter/Space selects the focused result. Implement a focusedIndex state in the LinkPicker component, set appropriate aria-activedescendant on the ul (or manage focus on the result buttons), add an onKeyDown handler (e.g. handleResultsKeyDown) to capture ArrowUp/ArrowDown/Home/End to update focusedIndex and preventDefault, and trigger the same onChange call used in the existing onClick when Enter/Space is pressed for the active result (use result.id, result.title, result.url and item.url as in the current onClick). Ensure the result buttons reflect focus/aria-selected based on focusedIndex and maintain existing click behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@database/migrations/2026_04_24_000000_add_location_to_visual_editor_navigations_table.php`:
- Line 36: The migration currently creates a non-unique index on the location
column; change it to a unique index so only one row can claim a given location.
Replace the $table->string('location')->nullable()->index() call with
$table->string('location')->nullable()->unique() (and update the other
occurrence at the second $table->string('location') call too) so the migration
creates a unique constraint enforcing single-occupant semantics.
In `@resources/js/visual-editor/site-editor/navigation/api-client.ts`:
- Around line 327-338: The runtime guards are too permissive: isMenuLocation and
isSearchResult currently don't validate their nested fields (isMenuLocation
never checks the menu object, and isSearchResult never validates the url
object), so malformed API payloads slip through; update isMenuLocation to assert
that row.menu exists and matches the Menu shape (e.g., required string/id
fields, items array or expected properties) and update isSearchResult to assert
that row.url exists and matches the URL shape (e.g., required href string and
any other required properties), using the existing pattern (typeof checks and
nested type predicates) and the same Record<string,unknown> casting used in
isMenuLocation to reject rows missing or mis-typed menu/url fields so only
payloads that fully satisfy the exported contracts are accepted.
In `@resources/js/visual-editor/site-editor/navigation/menu-tree.ts`:
- Around line 152-165: blocksToMenuTree currently skips any block whose name
isn't NAV_LINK or NAV_SUBMENU, causing unsupported navigation child blocks to be
dropped; instead of continuing when name !== NAV_LINK && name !== NAV_SUBMENU,
push a passthrough MenuItem that preserves the original block data (e.g., tag it
as type 'unsupported' or 'raw' and include the original UnknownBlock under a
property like originalBlock) so the in-memory tree can round-trip unknown
children without loss; update any downstream code that expects MenuItem variants
to handle this new passthrough case.
- Around line 29-30: The MenuItemType mapping loses original post-type and
taxonomy identifiers; update the types and conversion helpers so
non-'page'/'post' post types and taxonomies round-trip: change MenuItemType and
related discriminants used by blockAttributesToType() to carry the original post
type or taxonomy slug (e.g., include a postType or taxonomy field in the union
instead of collapsing to 'custom'/'taxonomy'), update blockAttributesToType() to
return that specific identifier when encountering non-standard post types or
taxonomy links, and update menuItemTypeToAttributes() to serialize those
identifiers back into attributes (emit a type/postType or taxonomy property
rather than only { kind: 'taxonomy' }) so templates/template-parts and custom
post types keep their identity across edit/save cycles.
In `@src/Services/MenuLocationResolver.php`:
- Around line 146-160: The docblock for effectiveAssignments() is inaccurate: it
claims a location with no DB assignment and no config `primary_id` resolves to
null, but the method calls forLocation($slug) which can return a published
fallback VisualEditorNavigation; update the comment to reflect the actual
behavior (that effectiveAssignments() maps each location slug to whatever
forLocation() returns, which may be a VisualEditorNavigation fallback or null),
and ensure the description references the forLocation() behavior and keeps the
declared return type array<string, ?VisualEditorNavigation>; no code changes
required unless you prefer to change forLocation() behavior instead—if so, make
forLocation() return null in that specific case and update its docblock
accordingly.
---
Nitpick comments:
In `@resources/js/visual-editor/site-editor/navigation/create-menu-dialog.tsx`:
- Around line 136-139: The titleLabel variable is wrapped in an unnecessary
useMemo; replace the useMemo call with a direct assignment like const titleLabel
= __('Menu name', TEXT_DOMAIN) in create-menu-dialog.tsx (remove the useMemo
import usage if no longer used) so the static translation is computed directly
without memoization overhead.
In `@resources/js/visual-editor/site-editor/navigation/link-picker.tsx`:
- Around line 195-238: The listbox results currently render an ul with
role="listbox" and li role="option" but have no keyboard navigation; add
keyboard handling so arrow keys move focus among results and Enter/Space selects
the focused result. Implement a focusedIndex state in the LinkPicker component,
set appropriate aria-activedescendant on the ul (or manage focus on the result
buttons), add an onKeyDown handler (e.g. handleResultsKeyDown) to capture
ArrowUp/ArrowDown/Home/End to update focusedIndex and preventDefault, and
trigger the same onChange call used in the existing onClick when Enter/Space is
pressed for the active result (use result.id, result.title, result.url and
item.url as in the current onClick). Ensure the result buttons reflect
focus/aria-selected based on focusedIndex and maintain existing click behavior.
In `@resources/js/visual-editor/site-editor/navigation/navigation-browser.tsx`:
- Around line 124-127: The helper itemCountForRow currently defined inside the
component recalculates on every render; change it to a memoized implementation
so counts are computed only when a row's blocks change—e.g., replace the inline
const itemCountForRow = (row) => countNavigationLeaves(row.content.blocks) with
a memoized function using useCallback or compute a memo Map via useRef/useMemo
keyed by row id (or row.content.blocks) and call countNavigationLeaves only when
that key changes; reference the existing itemCountForRow and
countNavigationLeaves symbols and ensure dependencies include the specific row
content so repeated renders reuse cached counts.
In `@resources/js/visual-editor/site-editor/navigation/navigation-canvas.tsx`:
- Around line 439-453: Remove the incorrect aria-pressed on the inner button
used for row selection: in the button element that uses onClick={handleSelect}
and data-testid={`ap-nav-canvas-row-${item.localId}`}, delete
aria-pressed={isSelected} (or replace it with aria-current if you want to
indicate the currently focused/active row) because the parent tree item already
exposes selection via aria-selected; keep handleSelect and other attributes
unchanged.
In `@resources/js/visual-editor/site-editor/navigation/navigation-inspector.tsx`:
- Around line 342-355: Remove the redundant id on the checkbox and the unused
useId call: in navigation-inspector.tsx remove id={newTabId} from the input
element (the checkbox is already implicitly associated by being wrapped in the
<label> within the Nav inspector component) and delete the corresponding useId
import and the newTabId variable (the useId() call) so there are no unused
identifiers remaining.
In `@resources/js/visual-editor/site-editor/navigation/navigation-section.tsx`:
- Around line 186-232: The handler handleAssignLocation leaves previous errors
visible because setLocationsError is only called on catch; clear the prior error
at the start of the function (before the try) by calling setLocationsError with
the "no error" value used in this component (e.g., null or empty string), so the
UI resets when a new assignment begins; update handleAssignLocation (and
reference setLocationsError) accordingly.
In `@resources/js/visual-editor/site-editor/site-editor-app.tsx`:
- Around line 264-275: There are two duplicated callbacks—handleNavigationOpen
and handleOpenEntity—that both call routing.navigate(activeSection.id,
entityId); remove the duplicate by hoisting a single shared callback (e.g.,
declare handleOpenEntity above the hook calls) and use that same function where
handleNavigationOpen is currently passed to the navigation views hook; update
references to call handleOpenEntity (or rename consistently) and delete the
unused handleNavigationOpen to avoid TDZ issues and drift.
In `@src/Http/Controllers/EntitySearchController.php`:
- Around line 61-73: The builtInSources() method defines URL resolver arrow
functions with unused parameters ($template and $part) which static analysis
flags; update the 'url' entries for VisualEditorTemplate and
VisualEditorTemplatePart to use no-argument arrow functions (e.g., fn(): ?string
=> null) or a single intentionally unused parameter like fn($_): ?string => null
so the closures no longer declare unused variables, keeping the surrounding
array keys and model references (builtInSources, VisualEditorTemplate,
VisualEditorTemplatePart) intact.
In `@src/Http/Controllers/NavigationController.php`:
- Around line 127-132: The current flow in NavigationController calls
releaseLocationFromOtherRecords($navigation) before $navigation->save(), which
can allow a race where two concurrent requests both release and then save the
same location; to fix, wrap the release + save in a single database transaction
(use DB::transaction or the app's transaction helper) so
releaseLocationFromOtherRecords($navigation) and $navigation->save() are atomic,
or alternatively move the call to releaseLocationFromOtherRecords($navigation)
to after $navigation->save() if you prefer a simpler approach; update the code
around the releaseLocationFromOtherRecords and save calls to use the transaction
API or reorder them accordingly.
In `@src/Http/Requests/UpdateNavigationRequest.php`:
- Around line 94-119: The duplicate closure in locationSlugRule() should be
extracted into a reusable validation rule class (e.g., MenuLocationRule) that
encapsulates the logic of resolving MenuLocationResolver and checking the value
against array_keys($resolver->locations()); implement the Laravel validation
Rule interface (or make it invokable) so both StoreNavigationRequest and
UpdateNavigationRequest can use new MenuLocationRule() instead of duplicating
the closure, and update the requests to replace calls to locationSlugRule() with
the new rule class.
In `@tests/Feature/VisualEditor/MenuLocationResolverTest.php`:
- Around line 240-262: The test for MenuLocationResolver::effectiveAssignments
currently only checks the 'primary' location; update the spec in
MenuLocationResolverTest.php to assert all configured locations are reported by
asserting the $assignments array has count 2, hasKey('footer') in addition to
hasKey('primary'), and that $assignments['footer'] is null (or otherwise
reflects an unassigned location) so the test verifies both presence and expected
value for the footer location as well as the primary assignment.
🪄 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: ac395f98-62c5-4d66-8b58-1ddb0c15e40c
📒 Files selected for processing (34)
config/visual-editor.phpdatabase/migrations/2026_04_24_000000_add_location_to_visual_editor_navigations_table.phpresources/js/visual-editor/site-editor/navigation/__tests__/api-client.test.tsresources/js/visual-editor/site-editor/navigation/__tests__/link-picker.test.tsxresources/js/visual-editor/site-editor/navigation/__tests__/menu-tree.test.tsresources/js/visual-editor/site-editor/navigation/__tests__/navigation-canvas.test.tsxresources/js/visual-editor/site-editor/navigation/__tests__/navigation-section.test.tsxresources/js/visual-editor/site-editor/navigation/api-client.tsresources/js/visual-editor/site-editor/navigation/create-menu-dialog.tsxresources/js/visual-editor/site-editor/navigation/link-picker.tsxresources/js/visual-editor/site-editor/navigation/locations-panel.tsxresources/js/visual-editor/site-editor/navigation/menu-tree.tsresources/js/visual-editor/site-editor/navigation/navigation-browser.cssresources/js/visual-editor/site-editor/navigation/navigation-browser.tsxresources/js/visual-editor/site-editor/navigation/navigation-canvas.cssresources/js/visual-editor/site-editor/navigation/navigation-canvas.tsxresources/js/visual-editor/site-editor/navigation/navigation-inspector.cssresources/js/visual-editor/site-editor/navigation/navigation-inspector.tsxresources/js/visual-editor/site-editor/navigation/navigation-section.tsxresources/js/visual-editor/site-editor/navigation/use-navigation-editor.tsresources/js/visual-editor/site-editor/site-editor-app.tsxroutes/api.phpsrc/Http/Controllers/EntitySearchController.phpsrc/Http/Controllers/MenuLocationsController.phpsrc/Http/Controllers/NavigationController.phpsrc/Http/Requests/StoreNavigationRequest.phpsrc/Http/Requests/UpdateNavigationRequest.phpsrc/Http/Resources/NavigationResource.phpsrc/Models/VisualEditorNavigation.phpsrc/Services/MenuLocationResolver.phptests/Feature/VisualEditor/EntitySearchControllerTest.phptests/Feature/VisualEditor/MenuLocationResolverTest.phptests/Feature/VisualEditor/MenuLocationsControllerTest.phptests/Feature/VisualEditor/NavigationControllerTest.php
- Promote the `location` column index to UNIQUE so single-occupant semantics are enforced at the database layer; wrap the release + save in a transaction so the constraint can't trap us in a half-applied state. - Tighten `isMenuLocation` and `isSearchResult` to validate their nested `menu` and `url` fields (allow null / undefined for the lenient "no value" cases). - Preserve the source `kind` / `type` block attributes through the editor's read → write cycle so custom post types (`kind: post-type, type: book`) and specific taxonomies round-trip cleanly. The IA-derived attributes still drive serialization whenever the user explicitly retypes a menu item via the link picker. - Correct the `effectiveAssignments()` docblock — the method returns whatever `forLocation()` resolves (which may be a published-fallback nav, not always null). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
resources/js/visual-editor/site-editor/navigation/__tests__/menu-tree.test.ts (1)
225-229: Optional: deduplicate the repeatedstripIdshelper.The same helper appears twice; extracting it once at file scope will reduce duplication and keep future updates consistent.
♻️ Suggested refactor
+function stripLocalIds(items: MenuItem[]): unknown[] { + return items.map(({ localId, children, ...rest }) => { + void localId; + return { ...rest, children: stripLocalIds(children) }; + }); +} @@ - const stripIds = (items: MenuItem[]): unknown[] => - items.map(({ localId, children, ...rest }) => { - void localId; - return { ...rest, children: stripIds(children) }; - }); - - expect(stripIds(reparsed)).toEqual(stripIds(original)); + expect(stripLocalIds(reparsed)).toEqual(stripLocalIds(original)); @@ - const stripIds = (items: MenuItem[]): unknown[] => - items.map(({ localId, children, ...rest }) => { - void localId; - return { ...rest, children: stripIds(children) }; - }); - - expect(stripIds(reparsed)).toEqual(stripIds(tree)); + expect(stripLocalIds(reparsed)).toEqual(stripLocalIds(tree));Also applies to: 332-336
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/site-editor/navigation/__tests__/menu-tree.test.ts` around lines 225 - 229, The helper stripIds used in menu-tree.test.ts is duplicated; extract a single shared stripIds function at the top/file scope and have both test locations reference it to avoid repetition. Locate the existing inline stripIds implementations (the function handling MenuItem, destructuring localId and children) and remove the duplicates, keeping one exported/hoisted function name stripIds(items: MenuItem[]) that returns the same shape, then update the tests that previously declared their own versions to call the single shared stripIds function.
🤖 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/navigation/__tests__/menu-tree.test.ts`:
- Around line 225-229: The helper stripIds used in menu-tree.test.ts is
duplicated; extract a single shared stripIds function at the top/file scope and
have both test locations reference it to avoid repetition. Locate the existing
inline stripIds implementations (the function handling MenuItem, destructuring
localId and children) and remove the duplicates, keeping one exported/hoisted
function name stripIds(items: MenuItem[]) that returns the same shape, then
update the tests that previously declared their own versions to call the single
shared stripIds function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 89952e14-0ebe-4ca1-afe2-bec18e6337e4
📒 Files selected for processing (8)
database/migrations/2026_04_24_000000_add_location_to_visual_editor_navigations_table.phpresources/js/visual-editor/site-editor/navigation/__tests__/link-picker.test.tsxresources/js/visual-editor/site-editor/navigation/__tests__/menu-tree.test.tsresources/js/visual-editor/site-editor/navigation/api-client.tsresources/js/visual-editor/site-editor/navigation/link-picker.tsxresources/js/visual-editor/site-editor/navigation/menu-tree.tssrc/Http/Controllers/NavigationController.phpsrc/Services/MenuLocationResolver.php
✅ Files skipped from review due to trivial changes (1)
- resources/js/visual-editor/site-editor/navigation/menu-tree.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- database/migrations/2026_04_24_000000_add_location_to_visual_editor_navigations_table.php
github-project-automation
Bot
moved this from In Review
to Done
in ArtisanPack UI Overview
17 of 21 tasks
12 of 13 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
Builds the Navigation editor screen inside the D1 site-editor shell per design brief §3.8 and enables the
core/navigationblock in the template canvas. The navigation section is the one site-editor surface that deliberately ISN'T a block-editor iframe — nav trees are structural data, not block trees, so this ships a native tree editor with drag-sort + nested re-parenting + a typed-target picker.Closes: #371
Type of Change
Related Issue
Issue: #371
Motivation and Context
Navigation is called out in V1 plan §4.3 as "the hardest single piece" of Phase D —
core/navigation's editor experience depends on a substantialcore-datashim surface (thewp_navigationentity, the link-control menu picker, fallback resolution). C4 (#360) landed the backend; B1 (#353) locked the shim. D4 is the UI surface that depends on both, and the section that actually lets users build menus.Changes Made
Backend
locationcolumn onvisual_editor_navigationsso the editor can write menu-location assignments (single-occupant: assigning a location to one menu releases it from any prior owner)MenuLocationsController— GET/menu-locations, returning the configured slugs paired with the menu currently resolved for each + anis_fallbackflag so the UI can render the "falling back to X" hintEntitySearchController— GET/search?type=…&q=…, the link-picker's source for pages / posts / templates / template-parts. Searches use a portableLIKE … ESCAPE '|'pattern and cap at 20 results per requestMenuLocationResolvernow prefers DB assignments over the configprimary_idchain, with a neweffectiveAssignments()helper for cross-location readsFrontend (
resources/js/visual-editor/site-editor/navigation/)menu-tree.ts— pure functions for the Gutenberg-block ↔ typed-MenuItem-tree bridge and the flat-list / projection helpers powering DnD (flattenTree,buildTreeFromFlat,getDescendantIds,projectDrop,commitProjection)navigation-canvas.tsx— the native tree editor. Single sortable list with depth-by-padding; drag X-offset projects target depth (clamped toprevious-sibling-depth + 1andnext-sibling-depth) so users drag right to nest into a submenu and left to outdent back to root. Subtrees move with their parentnavigation-inspector.tsx+link-picker.tsx— entity panel (name, slug, location dropdown) and item panel (typed link picker, label override, advanced rel/class/new-tab). All form fields use the shared.ap-site-editor__dialog-*classes so they match the templates / parts inspectornavigation-browser.tsx+locations-panel.tsx— left-rail menu list + locations assignment panelcreate-menu-dialog.tsx— slug + name + optional location, also using the shared dialog chromenavigation-section.tsx— orchestrator that returns{ navigator, canvas, inspector, overlay }to the shellcore/navigationblock enabled (removed fromdisabled_blocksconfig and the JS shell mirror)How Has This Been Tested?
Testing Environment:
Tests Performed:
PUT /navigation/{id}locationcleared (single-occupant)core/navigationis now in the inserterAccessibility Tests Run
Details: All interactive elements (drag handle, row main, action buttons, picker results) are buttons with descriptive
aria-labels; the tree usesrole="tree"/role="treeitem"witharia-levelandaria-selected; focus-visible outlines are explicit on the row main + action buttons.Tests Added
Test details:
navigation-canvas,navigation-section,link-picker, andapi-clientMenuLocationsControllerTest,EntitySearchControllerTest) plus extensions toMenuLocationResolverTest(DB-priority +effectiveAssignments) andNavigationControllerTest(location persistence, slug validation, single-occupant reassignment, null-clears)Documentation
Documentation details: Each new file carries a header docblock with rationale; the menu-tree → block bridge and the DnD projection helpers carry inline comments naming the standard dnd-kit "tree" recipe they implement.
Out of scope (called out in plan / #371 but deferred)
core/navigationblock link picker — when the block is placed in a template, its own LinkControl uses WordPress's/wp/v2/searchendpoint, which the B1 shim doesn't expose. Block placement + render works; richer in-block link picking is a B1-shim follow-upPre-Submission Checklist
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
API
Database
Tests