feat(admin): sidebar menu tree with collection grouping, plugin subgroups, and public menu sync#1024
feat(admin): sidebar menu tree with collection grouping, plugin subgroups, and public menu sync#1024ahliweb wants to merge 20 commits into
Conversation
…mic icon resolution for collections and plugins
…nents and API endpoint
…icon support to collection configuration
…e handling, and add unit test coverage
🦋 Changeset detectedLatest commit: 5fd85ba The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 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 |
Scope checkThis PR changes 1,743 lines across 23 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/blocks
@emdash-cms/cloudflare
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
…s, and public menu synchronization
ahliweb
force-pushed
the
sidebar-menu-tree
branch
from
2834b10 to
92306a5
Compare
Fixes typecheck errors in menu-sync.ts and registry.ts where sort_order was not recognized on _emdash_collections.
…pping - Add sortOrder and group columns to Collection interface - Add sortOrder and group to UpdateCollectionInput - Map sort_order and group in mapCollectionRow - Include sortOrder/group in createCollection and updateCollection - Order listCollections by sort_order ASC, slug ASC - Register migration 039_collection_grouping
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
|
Tracking issue: #1038 |
|
/review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid feature scope with good test coverage. Found a handful of correctness issues worth addressing before merge, the most important being a duplicate "Media" link in every named collection group, and several i18n / locale-correctness gaps in the menu-sync engine.
Doc nit: the PR body refers to "Migration 038" but the actual file is 039_collection_grouping.ts.
One note that doesn't fit on a diff line: listCollectionsWithFields was not updated to order by sort_order the way listCollections was. listCollectionsWithFields is the method the manifest build uses (emdash-runtime.ts:1359). The admin sidebar happens to be unaffected because the manifest stores collections as a Record<slug, …> and the client re-sorts by sortOrder, but any other caller still sees alphabetical order, and the divergence between the two list methods is a maintenance trap. Add orderBy("sort_order", "asc") before the slug ordering there too. Related: the collection-ordering.test.ts > listCollectionsWithFields ordering test passes only by coincidence — its inputs sort the same alphabetically and by sort_order. Pick inputs where the two disagree (e.g. zebra(0), art(1)) to actually catch the missing orderBy.
| .map( | ||
| ([group, items]): NavGroup => ({ | ||
| label: group, | ||
| items: [...items, { to: "/media", label: t`Media`, icon: Image }], |
There was a problem hiding this comment.
Bug: Media is duplicated in every named collection group.
The ungrouped collections branch (line 393) already adds Media once into the default "Content" group, and this block appends Media again to every named collection group. A site with named groups "Blog" and "Shop" will render three Media links (one under Content, one under Blog, one under Shop).
| items: [...items, { to: "/media", label: t`Media`, icon: Image }], | |
| items, |
Then drop the spread + extra item entirely so Media only lives in the ungrouped "Content" group.
| const menu = await db | ||
| .selectFrom("_emdash_menus") | ||
| .select(["id", "locale"]) | ||
| .where("name", "=", diff.toAdd[0]?.menuName ?? "") |
There was a problem hiding this comment.
applyMenuSyncDiff derives the target menu from diff.toAdd[0]?.menuName — when toAdd is empty (a sync that only needs removals/reorders), this falls back to "", the menu lookup returns nothing, and the rest of the function operates without ever knowing which menu it is mutating. toRemove / toReorder happen to work because they look items up by ID, but the contract is broken: the function silently does the right thing only by coincidence, and a sync-by-name where the menu doesn't exist returns MENU_NOT_FOUND only when there are additions to apply.
Consider taking menuName as a parameter to applyMenuSyncDiff (it already comes from the caller in syncSidebarToMenu), validating the menu up front, and scoping toReorder / toRemove to that menu's items so a malformed diff can't reorder/delete items from other menus.
| .selectFrom("_emdash_menu_items") | ||
| .select(["_emdash_menu_items.id", "reference_collection", "sort_order"]) | ||
| .innerJoin("_emdash_menus", "_emdash_menu_items.menu_id", "_emdash_menus.id") | ||
| .where("_emdash_menus.name", "=", menuName) |
There was a problem hiding this comment.
Locale-unaware menu lookup. Migration 036_i18n_menus_and_taxonomies made _emdash_menus keyed by UNIQUE(name, locale), so on multi-locale installs there can be multiple menus named e.g. "primary". This query (and the corresponding insert/delete in syncCollectionToMenu) matches all of them indiscriminately — computeMenuSyncDiff will mix items from every locale's menu into one diff, applyMenuSyncDiff will only add items to the first match, and syncCollectionToMenu will pick whichever menu executeTakeFirst() happens to return.
At minimum filter by locale (default to the site's default locale) or accept a locale parameter; otherwise document explicitly that menu-sync is only safe on single-locale installs.
| .where("id", "=", item.id) | ||
| .execute(); | ||
| reordered++; | ||
| } |
There was a problem hiding this comment.
applyMenuSyncDiff runs add / remove / reorder as separate statements with no transaction. If any one fails midway (e.g. a unique-constraint violation on insert, or a D1 hiccup on the third update), the menu is left in a half-applied state and the caller gets a generic SYNC_APPLY_ERROR with no indication of what landed. Wrap the three loops in withTransaction (already used elsewhere in this codebase) so the apply is all-or-nothing.
| success: true, | ||
| data: { toAdd, toRemove, toReorder }, | ||
| }; | ||
| } catch { |
There was a problem hiding this comment.
Bare catch {} swallows all errors and returns a generic message. Per AGENTS.md ("API Routes: Use Shared Utilities"), catch the error binding and log internally so the cause isn't lost — same for the second catch {} at line 173 and the silent catch {} blocks added in schema.ts (syncCollectionToMenu, removeCollectionFromMenu, handleSchemaCollectionMenuSync).
| const rows = await this.db | ||
| .selectFrom("_emdash_collections") | ||
| .selectAll() | ||
| .orderBy("sort_order", "asc") |
There was a problem hiding this comment.
Note: listCollections is now ordered by sort_order, but its sibling listCollectionsWithFields (line 168, not shown in this diff) was not updated. That second method is the one the manifest build uses. See review summary for details.
| for (const { slug, sortOrder } of collections) { | ||
| await this.db | ||
| .updateTable("_emdash_collections") | ||
| .set({ sort_order: sortOrder }) |
There was a problem hiding this comment.
reorderCollections runs an existence check per slug (N queries) followed by an update per slug (N more queries) — 2N round-trips on a route that's already paying a D1 primary write per UPDATE. On a 50-collection reorder that's 100 round-trips when one SELECT slug WHERE slug IN (...) for validation, plus the N updates, would do the same correctness check in N+1.
Also: the reorder isn't wrapped in withTransaction, so a failure mid-batch leaves collections in an inconsistent order.
| * Expandable nav item with children (submenu). | ||
| * The parent item is not navigable — clicking toggles the children. | ||
| */ | ||
| function NavSubMenu({ item, currentPath }: { item: NavItem; currentPath: string }) { |
There was a problem hiding this comment.
NavSubMenu (and the children field on NavItem) is wired up to render but nothing in this PR ever populates children — the "Menus → Header / Footer" nested example in the PR description doesn't actually exist in code. No surface allows children to be set either: ManifestCollection has no children field and PluginAdminPage only gained group/sortOrder.
Not a blocker, but worth either wiring up the Menus -> menu-instances case the PR description promises, or trimming the dead code/docs so reviewers and future contributors don't have to figure out how to populate something that has no entry point.
| .where("name", "=", menuName) | ||
| .executeTakeFirst(); | ||
|
|
||
| if (!menu) return; // Menu doesn't exist |
There was a problem hiding this comment.
syncCollectionToMenu silently returns when the named menu doesn't exist. Combined with addToMenu being a free-form string on CreateCollectionInput, a user who typos addToMenu: "primry" will get a successfully-created collection and no menu item, no warning, no error. At minimum log the miss, and ideally validate the menu name up front in handleSchemaCollectionCreate so the API call fails fast with a useful error.
| collections: z.array( | ||
| z.object({ | ||
| slug: z.string().min(1), | ||
| sortOrder: z.number().int(), |
There was a problem hiding this comment.
The Zod schema doesn't bound sortOrder (negatives are accepted) or limit the collections array length. A client can POST 100k entries and force 100k+ DB round-trips before the handler errors out. Consider .min(0) on sortOrder and a reasonable .max(N) on the array (the number of collections a site has is small).
|
Review posted successfully (review ID 4289278513). The review covers:
|
- Fix Media duplication in every named collection group (Sidebar.tsx)
- Fix applyMenuSyncDiff deriving menu from toAdd[0]; pass menuName explicitly
- Add locale filtering to menu sync queries (menu-sync.ts, schema.ts)
- Wrap applyMenuSyncDiff in withTransaction for atomicity
- Replace bare catch {} with proper error logging
- Optimize reorderCollections: batch validation + transaction
- Fix reorder endpoint Zod validation: bound sortOrder >= 0, array max 200
- Fix addToMenu warning on missing menu; validate menu in handleSchemaCollectionMenuSync
- Fix listCollectionsWithFields ordering test with disagreeing inputs
- Add locale field to test menu creation
|
/review |
There was a problem hiding this comment.
Most of the issues from the prior review have been addressed — nicely done. The big-ticket items (Media duplication, applyMenuSyncDiff deriving its target from diff.toAdd[0], transaction wrapping, locale filtering in menu-sync.ts, listCollectionsWithFields ordering, transactional/batched reorderCollections, the test fixture fix, and the reorder endpoint's input bounds) are all resolved.
A few residual concerns worth a follow-up before merge:
NavSubMenu/NavItem.childrenis still dead code — nothing in the PR populateschildren. The PR description still promises aMenus → Header / Footernested case that has no entry point.addToMenuon collection creation still silently no-ops on typos.syncCollectionToMenulogsconsole.warnbut the API caller gets a successful response.- The sync route handlers (
sync.ts,sync-menu.ts) hardcodelocale = "en"— on multi-locale installs you can only sync the English menu through the API. - The admin client's
fetchMenuSyncDiff/applyMenuSyncare exported but unused — no UI surface calls them in this PR. Worth either wiring them up or holding them back to a follow-up to keep this PR focused. - PR description nit: still says "Migration 038" but the file is
039_collection_grouping.ts.
None are merge blockers in my view; the locale and silent-no-op ones are the most worth tightening before this lands.
| /** Optional badge count (e.g., pending comments) */ | ||
| badge?: number; | ||
| /** Child items for nested submenu (max 1 level) */ | ||
| children?: NavItem[]; |
There was a problem hiding this comment.
NavItem.children is declared and the NavSubMenu renderer (lines 233-302) is wired up to handle it, but nothing in this PR ever populates children — collection groups and plugin groups are flattened into NavGroup.items (lines 381-389, 484-487), and ManifestCollection / PluginAdminPage have no children field. The "Menus → Header / Footer" nested example in the PR description has no path to existing.
Either wire up the Menus → menu-instances case the PR description shows, or drop NavSubMenu + the children field for now so future contributors aren't trying to populate something with no entry point.
|
|
||
| // Sync to public menu if requested | ||
| if (input.addToMenu) { | ||
| await syncCollectionToMenu(db, item.slug, item.label, input.addToMenu); |
There was a problem hiding this comment.
syncCollectionToMenu now logs a warning when the named menu doesn't exist (good), but handleSchemaCollectionCreate still doesn't surface that miss to the API caller — a user who typos addToMenu: "primry" gets a 200 with a created collection and no menu item, no error, no warning visible from the client side.
Either validate the menu up front (mirror what handleSchemaCollectionMenuSync already does at lines 698-711) and 400 on miss, or have syncCollectionToMenu return a result that the create handler can attach to the response so the client knows. Logging server-side isn't enough for a feature whose whole point is auto-wiring sidebar items into the user's menu.
| const denied = requirePerm(user, "menus:manage"); | ||
| if (denied) return denied; | ||
|
|
||
| const result = await computeMenuSyncDiff(emdash!.db, name); |
There was a problem hiding this comment.
locale is hardcoded to the function's default ("en") in both GET (here) and POST (line 54). On a multi-locale install — which is the whole reason migration 036_i18n_menus_and_taxonomies made _emdash_menus keyed by (name, locale) — there's no way to sync the Arabic or French version of primary through this API.
Accept a locale query parameter and forward it. Same applies to sync-menu.ts (line 41) and schema.ts's handleSchemaCollectionCreate calling syncCollectionToMenu without a locale.
If this is intentionally en-only for now, document it explicitly in the route comment and have it 400 on multi-locale sites instead of silently picking the en menu.
| /** | ||
| * Apply sync changes (align menu with sidebar structure). | ||
| */ | ||
| export async function applyMenuSync(name: string): Promise<SyncResult> { |
There was a problem hiding this comment.
fetchMenuSyncDiff and applyMenuSync are exported from lib/api/menus.ts and re-exported from lib/api/index.ts but no UI in this PR consumes them — the only callers are the exports themselves. Either wire up the diff-preview/apply UI promised in the PR description, or drop these helpers and fetchMenuSyncDiff / applyMenuSync server routes until the UI is ready. Shipping an API surface with no consumer makes it harder to evolve the contract later, because anyone could write to it.
Also, neither helper accepts a LocaleOptions parameter the way every other menu function in this file does — same multi-locale gap as the server route.
|
Review posted successfully (review ID 4289894663) with 4 inline comments and a summary. The author has addressed most of the critical issues from the prior review (Media dup, transaction wrapping, locale filtering, listCollectionsWithFields ordering, reorder batching/validation, input bounds, test fixture). Remaining concerns are smaller follow-ups:
|
- Accept locale from query params in menu sync endpoints (sync.ts) - Validate menu exists before collection creation when addToMenu is specified (schema.ts) - Change syncCollectionToMenu to return boolean for success/failure - Remove dead exports fetchMenuSyncDiff and applyMenuSync from menus.ts and index.ts
2 participants
What does this PR do?
Implements a comprehensive admin sidebar redesign that adds collection grouping, plugin subgroups, nested submenus, icon rendering, sidebar configuration, and bidirectional sync with public menus.
Discussion: #1027
Data Layer
sort_order(INTEGER, default 0) andgroup(TEXT, nullable) columns to_emdash_collectionssortOrder,group, andiconadded toCollection,CreateCollectionInput,UpdateCollectionInput,ManifestCollectiongroupandsortOrderadded toPluginAdminPageandManifestPlugin.adminPagessort_order ASC, slug ASCinstead of justslug ASCAdmin Sidebar Rendering
groupfield render in separate collapsible sidebar groups; ungrouped collections remain in the default "Content" groupgroupfield render in separate collapsible sidebar groups; ungrouped pages remain in "Plugins"resolveIcon()helper maps 30+ icon names to Phosphor components with fallback to defaultsNavItem.childrensupport withNavSubMenuexpandable component (max 1 level of nesting)hideCoreFeaturesandhideCollectionsoptions inEmDashConfig.sidebarto hide items from the navms-*,ps-*,start-*); chevrons flip in RTLMenu Sync
addToMenu: New field onCreateCollectionInput— auto-adds atype: "collection"menu item to the specified public menu on collection creationPOST /_emdash/api/schema/collections/:slug/sync-menuto add a collection to a menu after creationGET /_emdash/api/menus/:name/sync-diff— preview changes (toAdd, toRemove, toReorder)POST /_emdash/api/menus/:name/sync— apply changes to align menu with sidebar structureReordering UI
CollectionReorderDialog: Drag-and-drop dialog using@dnd-kit/core+@dnd-kit/sortablefor reordering collectionsPOST /_emdash/api/schema/collections/reorder— batch updatessort_orderfor multiple collectionsWhat changed
packages/core/src/database/migrations/038_collection_grouping.ts(new)packages/core/src/schema/types.ts—sortOrder,group,icon,addToMenufieldspackages/core/src/schema/registry.ts—reorderCollections, ordering inlistCollectionspackages/core/src/astro/types.ts—ManifestCollectiongainssortOrder,group,iconpackages/core/src/astro/integration/runtime.ts—sidebarconfig option,PluginAdminPagefieldspackages/core/src/emdash-runtime.ts— passessortOrder,group,iconto manifestpackages/core/src/api/handlers/schema.ts—syncCollectionToMenu,removeCollectionFromMenu,handleSchemaCollectionMenuSyncpackages/core/src/api/handlers/menu-sync.ts(new) —computeMenuSyncDiff,applyMenuSyncDiff,syncSidebarToMenupackages/core/src/astro/routes/api/schema/collections/reorder.ts(new)packages/core/src/astro/routes/api/schema/collections/[slug]/sync-menu.ts(new)packages/core/src/astro/routes/api/menus/[name]/sync.ts(new)packages/admin/src/components/Sidebar.tsx— rewritten for grouping, icons, submenus, hidingpackages/admin/src/components/CollectionReorderDialog.tsx(new)packages/admin/src/components/ContentTypeList.tsx— reorder button + dialog integrationpackages/admin/src/lib/api/schema.ts—reorderCollections,addToMenufieldpackages/admin/src/lib/api/menus.ts—fetchMenuSyncDiff,applyMenuSyncpackages/core/tests/unit/schema/collection-ordering.test.ts(new) — 11 testspackages/core/tests/unit/menus/menu-sync.test.ts(new) — 12 testspackages/core/tests/unit/menus/menu-sync-exports.test.ts(new) — 5 testsType of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (3285 tests, 208 test files)pnpm formathas been runAI-generated code disclosure
Sidebar Rendering Structure