homaroma/.agents/reviewer_m2_2_v2/review.md

Review Report — Milestone 2: Backend & DB (Reviewer 2 - v2)

Review Summary

Verdict: FAIL (Requires Changes)

The implementation contains multiple critical and major issues regarding transaction atomicity, type safety, unmapped fields, missing validation schemas, and logical data structure corruption in the translation handling of the MCP tool.

---

Findings

1. [Critical] Transaction Integrity Violation in PUT Route

• What: In PUT /menus/:name, the parent Menu record is created or updated using the standard prisma client before and outside the prisma.$transaction block where MenuItem and MenuItemTranslation records are modified.
• Where: apps/backend/src/routes/menus.ts (Lines 97-101, 103-153)
• Why: If a database error, unique constraint violation, or connection drop occurs inside the transaction block, the transaction will roll back all modifications to menu items. However, the changes to the Menu record (such as update to payload, isActive, or rootImage) will not roll back. This violates the ACID properties of the database and can lead to corrupt menu states.
• Suggestion: Perform the prisma.menu.findUnique, prisma.menu.create, and prisma.menu.update calls inside the transaction callback, executing them on the tx transaction client.

2. [Major] Unmapped Fields in POST Route

• What: POST /menus ignores the payload and isActive fields, preventing menus from being fully configured on creation.
• Where: apps/backend/src/routes/menus.ts (Lines 32-73)
• Why: The Prisma schema defines payload (Json) and isActive (Boolean) on the Menu model. However, the POST endpoint's TypeBox schema and database mapping omit these fields, preventing users from initializing menu structures or changing active status via POST.
• Suggestion: Add payload and isActive to the TypeBox body schema and pass them to the prisma.menu.create data parameter.

3. [Major] Missing TypeBox Validation Schema on PATCH Endpoint

• What: The .patch("/items/:id/translations") route completely lacks an Elysia TypeBox validation schema.
• Where: apps/backend/src/routes/menus.ts (Lines 183-228)
• Why: Bypassing schema validation leaves the endpoint open to arbitrary payloads. The request body is manually parsed after casting to any (const payload = body as any), violating the project standard of strict TypeBox validation.
• Suggestion: Pass a TypeBox schema option defining the expected structure of params (validating id as a number) and body.

4. [Major] Relative Import Path Compilation Errors under NodeNext Resolution

• What: Relative import paths are missing explicit .js file extensions.
• Where: apps/backend/src/routes/menus.ts (Lines 2-3)
• Why: Under ECMAScript Module Resolution (NodeNext/Node16 configured in tsconfig.json), TypeScript strictly requires file extensions on relative imports. Without them, the project fails to compile, throwing TS2835: Relative import paths need explicit file extensions....
• Suggestion: Append .js extensions:

  import prisma from "../lib/prisma.js";
  import { adminOnly } from "../middleware/auth.js";
  

5. [Major] Return Type and Status Mismatch for Non-Existent Menu

• What: GET /menus/:name returns a mock object without an id field and a 200 OK status when a menu is not found.
• Where: apps/backend/src/routes/menus.ts (Lines 16-30)
• Why: The shared Menu interface in packages/types/index.ts requires id: number. Returning a mock object like { name, items: [], payload: null, rootImage: null, isActive: false } lacks an id and violates type conformance. Returning 200 OK instead of 404 Not Found also hides resource non-existence from the client.
• Suggestion: Set set.status = 404 and return a structured error message like { error: "Menu not found" } when a menu is missing.

6. [Minor] Shared Types Menu Interface is Missing isActive

• What: The shared Menu interface does not include isActive.
• Where: packages/types/index.ts (Lines 205-211)
• Why: The database model defines isActive Boolean @default(true). Missing it in the TypeScript declaration causes compilation or type-safety issues for consumers.
• Suggestion: Add isActive: boolean to the Menu interface.

7. [Major] Translation Array Spreading Bug in MCP Tools

• What: The applyMenuAddNode helper spreads nextNode.translations directly into an object, even if it is an array.
• Where: apps/mcp-server/menu-tools.ts (Lines 272-300)
• Why: validateMenuNodeData permits translations to be an array or object. If a list of translations is passed as an array (e.g. [{ langCode: "th", label: "..." }]), spreading it creates index-based keys (like { "0": { langCode: "th", label: "..." } }) instead of language code keys. This corrupts the translatable object structure in the database.
• Suggestion: Convert the array to a key-value dictionary (e.g., { th: "..." }) before merging.

8. [Minor] Bypassing Type Safety with tx: any

• What: The database transaction client is cast to any.
• Where: apps/backend/src/routes/menus.ts (Line 103)
• Why: Disabling type safety within the transaction block makes it easy to introduce undetected runtime errors during schema changes.
• Suggestion: Remove the any cast and type the transaction client properly.

---

Verified Claims

• Claim: Menu API supports rootImage, columnImage, and hoverImage storage and retrieval → Verified via running apps/backend/test_menu_api_challenger.ts → PASS (retrieved values match input payloads exactly).
• Claim: Elysia routes perform validation checks on rootImage, columnImage, and order fields → Verified via running test_menu_api_challenger.ts → PASS (correctly throws 422 Unprocessable Entity for invalid types).
• Claim: Database schema correctly defines relation cascades → Verified via inspecting schema.prisma → PASS (cascades on delete of menus and menu items are present).

---

Coverage Gaps

• PATCH Endpoint Validation: The PATCH /menus/items/:id/translations route has no validation schema. (Risk level: Medium; Recommendation: Add TypeBox validation).
• POST Endpoint Mapping: Missing payload and isActive fields on POST /menus. (Risk level: Medium; Recommendation: Map missing fields).

---

Unverified Items

• Frontend Integration: Layout rendering and responsive constraints on frontend components. (Reason: Outside the scope of M2 Backend & DB review).

---

Challenge Summary

Overall risk assessment: HIGH

The combination of split transaction blocks (non-atomic updates) and logical bugs in translation array handling risks database state corruption that could crash storefront rendering of the Mega Menu.

---

Challenges

1. Non-Atomic Menu Updates

• Assumption challenged: PUT operations are atomic because they use prisma.$transaction.
• Attack scenario: A client PUTs an update to a menu payload. The Menu record is updated successfully, but a database error occurs during the subsequent MenuItem updates inside the transaction. The transaction rolls back, but the Menu changes remain committed.
• Blast radius: The Menu model metadata changes but its associated items do not, leaving the menu structure broken.
• Mitigation: Move the Menu creation/update query inside the transaction callback.

2. Translation Array Spreading Corruption

• Assumption challenged: Node translations are always plain objects.
• Attack scenario: A client calls add_menu_node with translations as an array. The tool spreads it, creating index keys.
• Blast radius: Corrupted database payload crashes storefront translation lookup.
• Mitigation: Detect array types and convert to dictionary.

3. Duplicate Menu 500 Leak

• Assumption challenged: Database uniqueness constraints are handled gracefully.
• Attack scenario: Creating a menu with an existing name throws P2002.
• Blast radius: The API crashes with 500 Internal Server Error instead of 409 Conflict, leaking DB constraints in the log.
• Mitigation: Intercept P2002 and return 409.

---

Stress Test Results

• Duplicate Name POST → Expect 400/409 → Got 500 → FAIL
• PUT Transaction Failure → Expect Rollback of all changes → Got partial update of Menu → FAIL
• Array Translation on Node Add → Expect { th: "..." } → Got { "0": { langCode: "th", ... } } → FAIL

---

Unchallenged Areas

• Menu Snapshot Storage: Snapshot storage uses in-memory map which works fine but does not persist across server restarts (implied constraint for local tool execution).