fix(api): menu REST API with camelCase responses#1052
Conversation
…schema validation, and repository-pattern implementation.
…a transaction-scoped existence check and throwing MenuGoneError
🦋 Changeset detectedLatest commit: 11cfdc7 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 2,345 lines across 14 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: |
… remove legacy query string support
|
/review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid fix and a clean refactor. The bugs being addressed are real (silent snake_case key drop, query-string id route falling through to the SSR 404 HTML, response shape mismatch with the rest of the REST surface), and the new MenuRepository aligns menus with the existing repository pattern. Test coverage on the repository (mapping, item counts, translation cloning, atomic setItems, cross-menu guards, listTranslations) and on the MenuGoneError race is thorough.
A couple of small observations, no blockers:
-
MCP tool responses also became camelCase. The MCP server delegates to the same
handleMenu*functions, somenu_get/menu_set_itemscallers now see camelCase too (the MCP test file in this PR confirms it). The PR's heads-up only mentions REST consumers / theme authors — worth widening the note to MCP clients as well, since that's a separately-consumed surface. -
The path-style route test is structural-only. It asserts the file exports
PUT/DELETE/prerender = falsebut doesn't exercise theinjectCoreRouteswiring or hit the route end-to-end. The route injection is the bug being fixed (without it, Astro fell through to the SSR 404), so a smoke test that the registered route actually accepts:idwould close the loop. Not a blocker — the schema/handler tests cover the logic, and the route file itself is trivial.
LGTM otherwise.
| const mod = await import("../../../src/astro/routes/api/menus/[name]/items/[id].js"); | ||
| expect(typeof mod.PUT).toBe("function"); | ||
| expect(typeof mod.DELETE).toBe("function"); | ||
| expect(mod.prerender).toBe(false); |
There was a problem hiding this comment.
Minor: this verifies the module exports but not that injectCoreRoutes actually registers the path-style pattern (which is the bug being fixed — without registration Astro returns the SSR 404 HTML page, regardless of what the module exports). A small additional assertion that injectCoreRoutes calls injectRoute with pattern: "/_emdash/api/menus/[name]/items/[id]" would close the loop and guard against a future refactor that drops that line from routes.ts.
| ).error, | ||
| }; | ||
| } | ||
| return { success: true, menu: matches[0] }; |
There was a problem hiding this comment.
Style/clarity: matches[0] here is typed as Menu only because packages/core/tsconfig.json overrides noUncheckedIndexedAccess to false (the base config has it true). After the length === 0 and length > 1 guards above, the array is known non-empty but TS won't narrow indexed access without that override. An explicit matches[0]! (or destructuring const [menu] = matches) would make the intent obvious to a reader used to the stricter base setting.
| .executeTakeFirst(); | ||
| // eslint-disable-next-line typescript-eslint(no-unsafe-type-assertion) -- Kysely fn.max returns unknown; always a number for sort_order column | ||
| sortOrder = ((maxOrder?.max as number) ?? -1) + 1; | ||
| } |
There was a problem hiding this comment.
Pre-existing behavior carried over from handleMenuItemCreate, but worth flagging now that there's a dedicated repository layer: this reads max(sort_order) and inserts without a surrounding transaction. Two concurrent createItem calls with sortOrder omitted under the same (menu_id, parent_id) can both compute the same next index. Out of scope for this PR (the original handler had the same race), but the move to a repository makes it a natural place to wrap the read+insert in a single transaction in a follow-up.
| locale: row.locale, | ||
| translation_group: row.translation_group, | ||
| }), | ||
| })); |
There was a problem hiding this comment.
Nit: this manually re-spells every column just to feed rowToMenu. If MenuTable ever gains a column, this object literal has to be updated in lockstep with the select([...]) and groupBy([...]) above, or rowToMenu will start producing stale entities for findMany while findById is correct. Two cheaper options: (a) destructure the row and spread (const { itemCount, ...menuRow } = row; return { itemCount: ..., ...rowToMenu(menuRow) };), or (b) select m.* and group by m.id only on dialects that allow it. Not a correctness bug today.
|
Review submitted successfully. The review includes a summary highlighting the strengths of the PR (clean refactor, real bugs fixed, thorough test coverage) plus four line-anchored comments covering:
All comments are nits/observations, not blockers. The PR is approved in spirit (LGTM otherwise). |
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
Thanks! Feel free to follow up with the suggested fixes, as they're not blockers
2 participants
What does this PR do?
Addresses the menu API problems reported in #932.
What detected while tracing the flow:
POST /menus/:name/itemswithcustom_url(snake_case) returned201withcustom_url: null. The Zod schema only knows the documented camelCase keys (customUrl,sortOrder, …), and Zod's default.object()silently strips unknown keys.DELETE /menus/:name/items/:idreturned the SSR 404 HTML page because menus were the only resource using a query-string id (/items?id=); every other resource (content, taxonomies, redirects, sections, widget-areas) uses path-style[id].ts.custom_url,menu_id, …) — every other resource (content, media, taxonomies, comments, redirects) goes through a*Repository.rowToX()mapper that produces camelCase. Menus had no repository at all; all SQL lived inhandlers/menus.ts.What changed:
MenuRepositorynext to the other repositories. All SQL and snake_case ↔ camelCase mapping lives there.handlers/menus.tsis now a thin orchestrator.createdAt,menuId,parentId,sortOrder,customUrl,referenceCollection, …).createMenuItemBody/updateMenuItemBodyare.strict()— unknown keys (custom_url,url,sort_order) now fail with400 VALIDATION_ERRORinstead of being silently dropped.typeis validated against the canonical enum (custom | page | post | taxonomy | collection).PUTandDELETE /_emdash/api/menus/:name/items/:id(path-style).Heads-up for theme authors: if your theme renders a custom menu block on the frontend by reading the REST API directly (rather than going through
getMenu()from the runtime), the response keys have changed from snake_case to camelCase.getMenu()already returned camelCase — themes using it are unaffected.Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
Repository test:
tests/integration/database/menu-repository.test.ts(12 cases — row mapping, item counts, translation cloning, atomic setItems, scoped delete/reorder, listTranslations).Handler/schema/route test:
tests/integration/api/menus-handlers.test.ts(9 cases —.strict()rejects snake_case, enum rejectstype: "link", customUrl persists and is returned in camelCase, path-style route module exists with PUT/DELETE).