homaroma/.agents/reviewer_m2_2_v3/review.md

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:

  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:

  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:

  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:

  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:

  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:

  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:

  {
    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:

  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:

  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:

  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).