# Review Report — M2 Backend & DB Review (v3)

**Verdict**: REQUEST_CHANGES

---

## Findings

### 1. [Critical] Label Corruption Bug in `applyMenuUpdateTranslation`
- **What**: Existing menu node labels are corrupted when updating translation fields if the label was originally a primitive string.
- **Where**: `apps/mcp-server/menu-tools.ts` (Line 302)
- **Why**: 
  The implementation attempts to merge translation updates into the existing label object via:
  ```typescript
  found.node[labelKey] = { ...(found.node[labelKey] || {}), ...translations };
  ```
  If `found.node[labelKey]` is a primitive string (e.g. `"Shop"`), spreading the string in JavaScript converts it into an index-mapped object (i.e. `{ "0": "S", "1": "h", "2": "o", "3": "p" }`). The resulting label object becomes `{ "0": "S", "1": "h", "2": "o", "3": "p", "th": "..." }`, which breaks frontend label rendering.
- **Suggestion**: 
  Check the type of the existing label before spreading it:
  ```typescript
  const existingLabel = found.node[labelKey];
  const baseObj = typeof existingLabel === "string" ? { en: existingLabel } : (existingLabel || {});
  found.node[labelKey] = { ...baseObj, ...translations };
  ```

### 2. [Major] Duplicate Link Auditing Logic Defect in `audit_missing_pages`
- **What**: The duplicate menu link checker in the `audit_missing_pages` tool never reports duplicates.
- **Where**: `apps/mcp-server/menu-toolkit.ts` (Line 304-323)
- **Why**: 
  The tool maps and converts the list of URLs using a Map to deduplicate them *before* performing the audit:
  ```typescript
  const uniqueUrls = [...new Map(urls.map((entry) => [entry.url, entry])).values()];
  ```
  Because the entries in `uniqueUrls` are already distinct, the subsequent duplication check `seenUrls.has(url)` inside the `uniqueUrls.forEach` loop is guaranteed to always evaluate to `false`.
- **Suggestion**: 
  Iterate over the raw `urls` list directly to perform the auditing and duplicate reporting:
  ```typescript
  urls.forEach(({ url, label, source }) => {
    if (seenUrls.has(url)) {
      duplicates.push({ menuName: menu.name, url, label: label || url, source });
      return;
    }
    seenUrls.add(url);
    // ... rest of the audit checks
  });
  ```

### 3. [Major] Validation Bypass in `validateMenuNodeData` (Leaf Link under Root)
- **What**: Leaf link nodes can be added directly under the root menu, bypassing the Mega Menu structure constraint.
- **Where**: `apps/mcp-server/menu-tools.ts` (Line 128)
- **Why**: 
  The function `validateMenuNodeData` restricts leaf link insertion under the root using the check:
  ```typescript
  if (parentId === "root" && hasUrl && structuralKeys.length === 0)
  ```
  However, in `add_menu_node` or `bulk_menu_operations`, the `parentId` is optional and can be `undefined` or `null` (which defaults to root insertion in `applyMenuAddNode`). When `parentId` is omitted, the check `parentId === "root"` is bypassed, allowing the node to be created directly at the root level.
- **Suggestion**: 
  Update the condition to match root defaulting:
  ```typescript
  if ((!parentId || parentId === "root") && hasUrl && structuralKeys.length === 0)
  ```

### 4. [Major] Missing Schema Validation & Potential Crash in PATCH `/items/:id/translations`
- **What**: The route has no TypeBox validation schema, making it susceptible to runtime crash errors on malformed payloads.
- **Where**: `apps/backend/src/routes/menus.ts` (Line 183)
- **Why**: 
  No schema constraints are supplied to the Elysia `.patch` definition. The request body is directly cast via `body as any`. If a request is sent with an empty or non-JSON body, `payload` is `null`, and evaluating `payload.label` throws `TypeError: Cannot read properties of null (reading 'label')` which crashes the request handler with a 500 status.
- **Suggestion**: 
  Define a proper Elysia TypeBox schema validator for the parameters and the body:
  ```typescript
  {
    params: t.Object({ id: t.Numeric() }),
    body: t.Object({
      label: t.Union([t.String(), t.Record(t.String(), t.String())])
    })
  }
  ```

### 5. [Major] TS Compiler Errors in Backend Routes (`src/routes/menus.ts`)
- **What**: The file contains 7 TypeScript compiler errors under strict checking mode.
- **Where**: `apps/backend/src/routes/menus.ts` (Lines 2, 3, 32, 74)
- **Why**: 
  - Lines 2 & 3: Mismatched ES imports under the `nodenext` module resolution profile (`Relative import paths need explicit file extensions...`).
  - Lines 32 & 74: Implicit `any` type on destructured parameters (`body`, `set`, and `name`) due to generic type inference limitations.
- **Suggestion**: 
  - Add explicit `.js` extensions to local relative imports.
  - Add type annotations to destructured parameter arguments in POST and PUT route handlers.

### 6. [Major] Missing `id` in GET `/menus/:name` Fallback Payload
- **What**: The fallback object returned when a menu is not found violates the TypeScript `Menu` interface.
- **Where**: `apps/backend/src/routes/menus.ts` (Line 27)
- **Why**: 
  If the requested menu doesn't exist, the route returns:
  ```typescript
  return { name, items: [], payload: null, rootImage: null, isActive: false };
  ```
  This object lacks the `id: number` property, which is required by the `Menu` interface in `@homaroma/types` (`packages/types/index.ts`). A frontend consumer accessing `menu.id` will receive `undefined`, which may lead to React key errors or TypeScript violations.
- **Suggestion**: 
  Return a default fallback numeric ID (e.g. `id: 0`) in the returned object:
  ```typescript
  return { id: 0, name, items: [], payload: null, rootImage: null, isActive: false };
  ```

### 7. [Minor] Schema Validation Bypass in `upsert_menu` parameter mapping
- **What**: Malformed `items` parameter types are silently ignored instead of throwing schema errors.
- **Where**: `apps/mcp-server/menu-toolkit.ts` (Line 217)
- **Why**: 
  When the `items` parameter is passed, the tool does:
  ```typescript
  const items = params.items !== undefined ? (Array.isArray(params.items) ? params.items : []) : ...
  ```
  If `params.items` is not an array (e.g., a string or object), the tool silently replaces it with `[]` rather than throwing a validation error, bypassing the schema constraints of the tool definition.
- **Suggestion**: 
  Throw a validation error if `params.items` is provided but is not an array.

### 8. [Minor] Snapshots Volatility
- **What**: Captured menu snapshots are volatile.
- **Where**: `apps/mcp-server/menu-tools.ts` (Line 223)
- **Why**: 
  Snapshots are stored in an in-memory `Map` inside the MCP server process. If the server process restarts (which happens on file saves, container restarts, or scale-downs), all captured snapshots are permanently lost.
- **Suggestion**: 
  Document this process-local limitation, or migrate snapshot storage to the database schema.

---

## Verified Claims

- **Prisma Schema columns successfully migrated** → Verified via `bunx prisma migrate status` → **PASS** (Database schema is up-to-date and PostgreSQL has `rootImage`, `columnImage`, and `hoverImage` columns).
- **TypeScript matches DB columns** → Verified via `packages/types/index.ts` file audit → **PASS** (Shared TypeScript interfaces have matching columns).
- **Integration Tests compile and run** → Verified via `bun test_menu_comprehensive.ts` → **PASS** (Integration checks completed successfully, but highlighted the schema validation bypass for `items` parameters).