131 lines
7.3 KiB
Markdown
131 lines
7.3 KiB
Markdown
# 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).
|