[Fix/208 menus parenting] fix(admin): support parent selection and parentId for menu items (fix #208)#232
Conversation
🦋 Changeset detectedLatest commit: 49d41d4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
Adds parent selection support to the Admin Menu Editor so menu items can be nested, and ensures parentId is sent through create/update flows to persist nesting correctly (addressing issue #208).
Changes:
- Introduces a “Parent” selector in the Menu Editor UI for creating menu items (custom links and content items).
- Propagates
parentIdincreateMenuItemandupdateMenuItemmutation payloads. - Adds a changeset for an
@emdash-cms/adminpatch release describing the behavior change.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| packages/admin/src/components/MenuEditor.tsx | Adds parent selection UI and passes parentId through menu item create/update flows. |
| .changeset/fix-menus-parenting.md | Documents the admin patch release for menu parenting support. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <label className="text-sm text-kumo-subtle mr-2">Parent</label> | ||
| <select | ||
| value={newParentId} | ||
| onChange={(e) => setNewParentId(e.target.value)} | ||
| className="h-10 rounded-md border border-kumo-line bg-kumo-base px-3 py-2 text-sm" |
There was a problem hiding this comment.
The "Parent" label isn’t programmatically associated with the , so screen readers won’t announce it. Add an id to the select and htmlFor on the label (or add an aria-label) so the control is accessible.
| <select | ||
| value={newParentId} | ||
| onChange={(e) => setNewParentId(e.target.value)} |
There was a problem hiding this comment.
newParentId can become stale if the selected parent item is deleted or the menu items refresh; the state may still hold an ID that no longer exists in localItems, and subsequent creates (e.g. Add Content) could send an invalid parentId. Consider resetting newParentId to "" when it’s not found in the latest items.
| <option value="">Top level</option> | ||
| {localItems.map((it) => ( | ||
| <option key={it.id} value={it.id}> | ||
| {it.label[0] + '\u200B' + it.label.slice(1)} |
There was a problem hiding this comment.
Rendering option text as it.label[0] + '\u200B' + it.label.slice(1) will produce incorrect output (or "undefined" prefix) when it.label is an empty string. Prefer rendering the full label directly, or guard against empty labels before indexing.
| {it.label[0] + '\u200B' + it.label.slice(1)} | |
| {it.label.length > 0 | |
| ? it.label[0] + "\u200B" + it.label.slice(1) | |
| : it.label} |
| <select name="parentId" defaultValue={newParentId} className="h-10 rounded-md border border-kumo-line bg-kumo-base px-3 py-2 text-sm"> | ||
| <option value="">Top level</option> | ||
| {localItems.map((it) => ( | ||
| <option key={it.id} value={it.id}> | ||
| {it.label[0] + '\u200B' + it.label.slice(1)} | ||
| </option> | ||
| ))} | ||
| </select> |
There was a problem hiding this comment.
This parent in the add dialog has no accessible name (no , no aria-label), which makes it hard to use with assistive tech and hard to target in tests. Add a visible label tied via htmlFor/id or at least an aria-label. <div className="space-y-2"> <label htmlFor="add-custom-link-parent" className="text-sm font-medium"> Parent </label> <select id="add-custom-link-parent" name="parentId" defaultValue={newParentId} className="h-10 rounded-md border border-kumo-line bg-kumo-base px-3 py-2 text-sm" > <option value="">Top level</option> {localItems.map((it) => ( <option key={it.id} value={it.id}> {it.label[0] + '\u200B' + it.label.slice(1)} </option> ))} </select> </div>
| {localItems.map((it) => ( | ||
| <option key={it.id} value={it.id}> | ||
| {it.label[0] + '\u200B' + it.label.slice(1)} | ||
| </option> |
There was a problem hiding this comment.
Rendering option text as it.label[0] + '\u200B' + it.label.slice(1) will produce incorrect output (or "undefined" prefix) when it.label is an empty string. Prefer rendering the full label directly, or guard against empty labels before indexing.
| <select name="parentId" defaultValue={editingItem.parent_id ?? ""} className="h-10 rounded-md border border-kumo-line bg-kumo-base px-3 py-2 text-sm"> | ||
| <option value="">Top level</option> | ||
| {localItems | ||
| .filter((it) => it.id !== editingItem.id) | ||
| .map((it) => ( | ||
| <option key={it.id} value={it.id}> | ||
| {it.label[0] + '\u200B' + it.label.slice(1)} | ||
| </option> | ||
| ))} | ||
| </select> |
There was a problem hiding this comment.
This parent in the edit dialog has no accessible name (no , no aria-label), which makes it difficult for screen readers and also prevents getByLabelText-style tests. Add a visible label tied via htmlFor/id or at least an aria-label. <div className="space-y-2"> <label htmlFor="edit-menu-item-parent" className="text-sm font-medium"> Parent </label> <select id="edit-menu-item-parent" name="parentId" defaultValue={editingItem.parent_id ?? ""} className="h-10 w-full rounded-md border border-kumo-line bg-kumo-base px-3 py-2 text-sm" > <option value="">Top level</option> {localItems .filter((it) => it.id !== editingItem.id) .map((it) => ( <option key={it.id} value={it.id}> {it.label[0] + '\u200B' + it.label.slice(1)} </option> ))} </select> </div>
| {localItems | ||
| .filter((it) => it.id !== editingItem.id) | ||
| .map((it) => ( | ||
| <option key={it.id} value={it.id}> | ||
| {it.label[0] + '\u200B' + it.label.slice(1)} | ||
| </option> | ||
| ))} |
There was a problem hiding this comment.
Parent selection only filters out the item itself. This still allows creating parent/child cycles (e.g. setting an item’s parent to one of its own descendants), and there’s no server-side validation in handleMenuItemUpdate to prevent that. Consider filtering out descendants here (and/or validating on the API) to avoid corrupt menu trees.
| .filter((it) => it.id !== editingItem.id) | ||
| .map((it) => ( | ||
| <option key={it.id} value={it.id}> | ||
| {it.label[0] + '\u200B' + it.label.slice(1)} |
There was a problem hiding this comment.
Rendering option text as it.label[0] + '\u200B' + it.label.slice(1) will produce incorrect output (or "undefined" prefix) when it.label is an empty string. Prefer rendering the full label directly, or guard against empty labels before indexing.
| {it.label[0] + '\u200B' + it.label.slice(1)} | |
| {it.label ? it.label[0] + '\u200B' + it.label.slice(1) : ""} |
| const parentVal = formData.get("parentId"); | ||
| createMutation.mutate({ | ||
| type: "custom", | ||
| label: typeof labelVal === "string" ? labelVal : "", | ||
| customUrl: typeof urlVal === "string" ? urlVal : "", | ||
| target: (typeof targetVal === "string" ? targetVal : "") || undefined, | ||
| parentId: typeof parentVal === "string" && parentVal !== "" ? parentVal : undefined, | ||
| }); |
There was a problem hiding this comment.
MenuEditor has existing component tests, but the new parent selection + parentId propagation path isn’t covered. Add tests that select a parent and assert createMenuItem / updateMenuItem are called with the expected parentId (including clearing it when "Top level" is selected).
What does this PR do?
Adds parent selection UI to the Menu Editor and ensures parentId is propagated on create/update/reorder so menu nesting and order persist correctly.
Closes #208
Type of change
Checklist
pnpm typecheckpassespnpm --silent lint:json | jq '.diagnostics | length'returns 0pnpm testpasses (or targeted tests for my change)pnpm formathas been run