Skip to content

fix(admin): support nested menu items in the menu editor#1739

Merged
ascorbic merged 5 commits into
emdash-cms:mainfrom
Emdash-Bug-Testing:fix/208-menu-nesting-ui
Jul 6, 2026
Merged

fix(admin): support nested menu items in the menu editor#1739
ascorbic merged 5 commits into
emdash-cms:mainfrom
Emdash-Bug-Testing:fix/208-menu-nesting-ui

Conversation

@marcusbellamyshaw-cell

Copy link
Copy Markdown
Contributor

What does this PR do?

MenuItem.parentId was already supported end-to-end by the API and admin API client, but the MenuEditor UI never exposed it, so there was no way to build a nested menu from the admin. Adds a Parent select to the edit dialog (excluding the item itself and its descendants, to prevent cycles), renders items indented by nesting depth, and makes the up/down reorder buttons operate on siblings only instead of the whole flat list.

Closes #208

Type of change

  • Bug fix

Checklist

  • I have read CONTRIBUTING.md
  • pnpm typecheck passes
  • pnpm lint passes
  • pnpm test passes (targeted: packages/admin/tests/components/MenuEditor.test.tsx, 14/14)
  • pnpm format has been run
  • I have added/updated tests for my changes (if applicable)
  • I have added a changeset (if this PR changes a published package)

AI-generated code disclosure

  • This PR includes AI-generated code — model/tool: Claude (Claude Code CLI)

MenuItem.parentId was already supported end-to-end by the API and admin
API client, but the MenuEditor UI never exposed it, so there was no way
to build a nested menu from the admin.

Adds a Parent select to the edit dialog (excluding the item itself and
its descendants to prevent cycles, which the server doesn't validate
against either), renders items indented by nesting depth, and makes the
up/down reorder buttons operate on siblings only instead of the whole
flat list.

Fixes emdash-cms#208
Three of the new tests added for emdash-cms#208 were broken: two used
getByText("Services") which ambiguously matched both the item label
and its "/services" URL text (case-insensitive substring match), and
the Parent-select test used a Playwright locator .click() that this
browser-test env's missing Tailwind CSS causes to fail Base UI's
pointer-event hit test on a Dialog-nested trigger. Add exact:true to
the ambiguous text queries, wait for the edit dialog to render before
interacting with it, and use a native DOM .click() for the Select
trigger (matching the pattern already used for Dialog buttons
elsewhere in this test suite).
@changeset-bot

changeset-bot Bot commented Jul 3, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 1e68c2e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@emdash-cms/admin Patch
emdash Patch
@emdash-cms/cloudflare Patch
@emdash-cms/sandbox-workerd Patch
@emdash-cms/fixture-perf-site Patch
@emdash-cms/perf-demo-site Patch
@emdash-cms/cache-demo-site Patch
@emdash-cms/do-demo-site Patch
@emdash-cms/do-solo-demo-site Patch
@emdash-cms/auth Patch
@emdash-cms/blocks Patch
@emdash-cms/gutenberg-to-portable-text Patch
@emdash-cms/x402 Patch
create-emdash Patch
@emdash-cms/auth-atproto Patch
@emdash-cms/plugin-embeds Patch

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

@github-actions github-actions Bot added review/needs-review No maintainer or bot review yet area/admin size/L labels Jul 3, 2026
@pkg-pr-new

pkg-pr-new Bot commented Jul 3, 2026

Copy link
Copy Markdown

Open in StackBlitz

@emdash-cms/admin

npm i https://pkg.pr.new/@emdash-cms/admin@1739

@emdash-cms/auth

npm i https://pkg.pr.new/@emdash-cms/auth@1739

@emdash-cms/auth-atproto

npm i https://pkg.pr.new/@emdash-cms/auth-atproto@1739

@emdash-cms/blocks

npm i https://pkg.pr.new/@emdash-cms/blocks@1739

@emdash-cms/cloudflare

npm i https://pkg.pr.new/@emdash-cms/cloudflare@1739

@emdash-cms/contentful-to-portable-text

npm i https://pkg.pr.new/@emdash-cms/contentful-to-portable-text@1739

emdash

npm i https://pkg.pr.new/emdash@1739

create-emdash

npm i https://pkg.pr.new/create-emdash@1739

@emdash-cms/gutenberg-to-portable-text

npm i https://pkg.pr.new/@emdash-cms/gutenberg-to-portable-text@1739

@emdash-cms/plugin-cli

npm i https://pkg.pr.new/@emdash-cms/plugin-cli@1739

@emdash-cms/plugin-types

npm i https://pkg.pr.new/@emdash-cms/plugin-types@1739

@emdash-cms/registry-client

npm i https://pkg.pr.new/@emdash-cms/registry-client@1739

@emdash-cms/registry-lexicons

npm i https://pkg.pr.new/@emdash-cms/registry-lexicons@1739

@emdash-cms/sandbox-workerd

npm i https://pkg.pr.new/@emdash-cms/sandbox-workerd@1739

@emdash-cms/x402

npm i https://pkg.pr.new/@emdash-cms/x402@1739

@emdash-cms/plugin-ai-moderation

npm i https://pkg.pr.new/@emdash-cms/plugin-ai-moderation@1739

@emdash-cms/plugin-atproto

npm i https://pkg.pr.new/@emdash-cms/plugin-atproto@1739

@emdash-cms/plugin-audit-log

npm i https://pkg.pr.new/@emdash-cms/plugin-audit-log@1739

@emdash-cms/plugin-color

npm i https://pkg.pr.new/@emdash-cms/plugin-color@1739

@emdash-cms/plugin-embeds

npm i https://pkg.pr.new/@emdash-cms/plugin-embeds@1739

@emdash-cms/plugin-field-kit

npm i https://pkg.pr.new/@emdash-cms/plugin-field-kit@1739

@emdash-cms/plugin-forms

npm i https://pkg.pr.new/@emdash-cms/plugin-forms@1739

@emdash-cms/plugin-webhook-notifier

npm i https://pkg.pr.new/@emdash-cms/plugin-webhook-notifier@1739

commit: 1e68c2e

@emdashbot emdashbot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the right change for #208: the API and repository already support parentId, and the gap was purely in the admin UI. The implementation is mostly idiomatic — a hierarchical display list, cycle exclusion in the parent picker, sibling-scoped reorder buttons, RTL-safe indentation via marginInlineStart, and Lingui-wrapped strings. The targeted tests pass and cover the headline behaviors.

What I checked

  • Changeset — found one, but it names the wrong package (only packages/admin changed, not emdash core).
  • Localization / RTL — new user-facing strings go through Lingui; indentation uses logical marginInlineStart. The option-label prefix is worth an RTL review.
  • Cycle safety — the UI excludes the item itself and its descendants from parent options, so the picker cannot create cycles. getSelfAndDescendantIds uses its ids set to avoid infinite recursion on cyclic data, and buildDisplayList has a visited guard.
  • Reorder logicmoveItem swaps only same-parent siblings for the buttons, and the resulting flat sortOrder indices preserve the sibling ordering after the next fetch/display.
  • Tests — three new tests cover the parent select, indentation, and sibling-only disables. Coverage of the descendant-exclusion rule is missing.

Headline conclusion: fix the changeset package name before merge; otherwise the code looks solid with two minor suggestions.

Comment thread .changeset/menu-editor-nesting.md Outdated
@@ -0,0 +1,5 @@
---
"emdash": patch

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[needs fixing] The changeset names emdash, but this PR only changes packages/admin/src/components/MenuEditor.tsx and the @emdash-cms/admin tests. Because the workspace uses fixed versioning, the version bump will still propagate, but the changelog entry will be attributed to the wrong package (and may be missed from the @emdash-cms/admin release notes).

Suggested change
"emdash": patch
"@emdash-cms/admin": patch

expect(inputEl.validity.valid).toBe(false);
});

it("edit dialog shows a Parent select that excludes the item itself", async () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] The title says the Parent select excludes "the item itself", but the implemented logic also excludes all descendants to prevent cycles. The current fixture (Home) has no descendants, so that branch is untested. Consider adding a nested fixture (e.g. Home → Services) and editing Home to assert that Services is also absent from the options.

.filter(({ item }) => !excluded.has(item.id))
.map(({ item, depth }) => ({
value: item.id,
label: `${"— ".repeat(depth)}${item.label}`,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] Using a leading "— " text prefix for indentation may not align correctly under RTL document direction, because neutral prefixes can move with the bidi context. If Kumo’s Select.Option supports custom rendering or per-option padding, prefer an RTL-safe padding-inline-start / ps-* style instead of a text prefix.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a valid comment. Can you use a RTL-safe alternative

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7168473 — swapped the text prefix for marginInlineStart, matching the item-list row indentation a few lines up.

@github-actions github-actions Bot added review/awaiting-author Reviewed; waiting on the author to respond and removed review/needs-review No maintainer or bot review yet labels Jul 3, 2026
@marcusbellamyshaw-cell

Copy link
Copy Markdown
Contributor Author

recheck

Only packages/admin changed in this PR, but the changeset named
"emdash" (core). Fixed per review on emdash-cms#1739.
@github-actions github-actions Bot added review/needs-rereview Author pushed changes since the last review and removed review/awaiting-author Reviewed; waiting on the author to respond labels Jul 3, 2026
Text-based "— " repeat prefix could misalign under RTL bidi reordering.
Match the same-file precedent (item list rows) and indent via
marginInlineStart instead.
Existing test only exercised self-exclusion (flat fixture had no
descendants to check against). Adds a nested fixture and asserts a
child is also absent from its ancestor's Parent options.

@ascorbic ascorbic left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@ascorbic ascorbic merged commit b6ba0d7 into emdash-cms:main Jul 6, 2026
45 checks passed
@emdashbot emdashbot Bot mentioned this pull request Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/admin cla: signed review/needs-rereview Author pushed changes since the last review size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to create child menus in menu interface

2 participants