docs(14): add code review report
This commit is contained in:
parent
d56db023be
commit
635141080b
1 changed files with 213 additions and 0 deletions
213
.planning/phases/14-pax8-ui-surface/14-REVIEW.md
Normal file
213
.planning/phases/14-pax8-ui-surface/14-REVIEW.md
Normal file
|
|
@ -0,0 +1,213 @@
|
|||
---
|
||||
phase: 14-pax8-ui-surface
|
||||
reviewed: 2026-07-11T00:00:00Z
|
||||
depth: standard
|
||||
files_reviewed: 10
|
||||
files_reviewed_list:
|
||||
- app/api/data/companies-list/route.ts
|
||||
- app/api/pax8/companies/[id]/route.ts
|
||||
- app/api/pax8/companies/route.ts
|
||||
- app/api/pax8/company-matches/[id]/resolve/route.ts
|
||||
- app/api/pax8/company-matches/route.ts
|
||||
- app/pax8/page.tsx
|
||||
- components/admin/DetailModal.tsx
|
||||
- components/navigation/app-navigation.tsx
|
||||
- lib/services/pax8-company-match-resolver.test.ts
|
||||
- lib/services/pax8-company-match-resolver.ts
|
||||
findings:
|
||||
critical: 1
|
||||
warning: 5
|
||||
info: 5
|
||||
total: 11
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase 14: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-07-11
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 10
|
||||
**Status:** issues_found
|
||||
|
||||
## Summary
|
||||
|
||||
Reviewed the PAX8 UI surface: the companies list/detail API routes, the company-match
|
||||
review queue and its resolve mutation (including the resolver service and its unit
|
||||
tests), the `/pax8` page, and the two shared components touched by this phase
|
||||
(`DetailModal.tsx`, `app-navigation.tsx`).
|
||||
|
||||
SQL injection surface is well-handled throughout (whitelisted sort columns, all
|
||||
values parameterized, zod validation on the mutation). The transactional resolver
|
||||
is careful about locking and re-scoring eligibility. However, there is one
|
||||
functional blocker: the "manual search" resolve path is broken end-to-end because
|
||||
`companies.id` (a `BIGINT`) is returned as a JSON string by `/api/data/companies-list`
|
||||
but the resolve endpoint's Zod schema requires a strict `number`, so every manual
|
||||
link attempt via the search popover will 400. There are also several missing-error-
|
||||
handling and completeness gaps in the new `/pax8` page and `DetailModal.tsx` pax8
|
||||
groups that should be addressed before this ships.
|
||||
|
||||
## Critical Issues
|
||||
|
||||
### CR-01: Manual-search "Link to selected company" flow is broken by a bigint/string type mismatch
|
||||
|
||||
**File:** `app/api/data/companies-list/route.ts:9-18`, `app/pax8/page.tsx:54-57,172-181,466-484`, `app/api/pax8/company-matches/[id]/resolve/route.ts:21-24`
|
||||
|
||||
**Issue:** `companies.id` is declared `BIGINT PRIMARY KEY` (`migrations/001_initial_schema.sql:13`). `node-postgres` returns `BIGINT`/`int8` columns as JavaScript **strings** by default (no `pg.types.setTypeParser` override exists anywhere in the codebase — confirmed via `postgres-client.ts`). `GET /api/data/companies-list` selects `id` directly and returns `result.rows` unmodified:
|
||||
|
||||
```ts
|
||||
const result = await postgresClient.query(
|
||||
`SELECT id, company_name FROM companies WHERE is_active = true AND is_deleted = false ORDER BY company_name ASC`
|
||||
);
|
||||
return NextResponse.json(result.rows);
|
||||
```
|
||||
|
||||
So the wire payload actually contains `"id": "12345"` (a JSON string), even though the frontend types it as `ManualCompanyOption { id: number; ... }` (`app/pax8/page.tsx:54-57`). Every *other* route in this same phase (`/api/pax8/companies`, `/api/pax8/companies/[id]`, `/api/pax8/company-matches`) is careful to `::text`-cast bigint columns in SQL and then `Number(...)` them in JS before serializing — this route is the one place that skips that conversion.
|
||||
|
||||
When a user picks a company from the manual-search popover, `selected.id` (actually a string at runtime) is sent as `companyId` in the resolve POST body:
|
||||
|
||||
```ts
|
||||
if (selected) void resolve(review.id, selected.id, selected.company_name);
|
||||
...
|
||||
body: JSON.stringify({ companyId }),
|
||||
```
|
||||
|
||||
The resolve route's schema requires a real number and does not coerce:
|
||||
|
||||
```ts
|
||||
const ResolveBody = z.object({
|
||||
companyId: z.number().int().positive(),
|
||||
...
|
||||
});
|
||||
```
|
||||
|
||||
`z.number()` rejects a JSON string outright, so `parsed.success` is `false` and the API returns `400 Invalid payload` for every manual-search resolution attempt — the D-05 manual-search fallback (explicitly called out as a requirement in the resolver's own doc comment) is non-functional as shipped. This is not caught by `pax8-company-match-resolver.test.ts` because those tests call `resolvePax8CompanyMatch` directly with JS numbers, bypassing the HTTP/JSON layer entirely where the bug lives.
|
||||
|
||||
**Fix:** Cast to text and convert in `companies-list/route.ts`, matching the pattern used elsewhere in this phase:
|
||||
|
||||
```ts
|
||||
const result = await postgresClient.query<{ id: string; company_name: string }>(
|
||||
`SELECT id::text AS id, company_name FROM companies WHERE is_active = true AND is_deleted = false ORDER BY company_name ASC`
|
||||
);
|
||||
return NextResponse.json(result.rows.map(r => ({ id: Number(r.id), company_name: r.company_name })));
|
||||
```
|
||||
Alternatively, switch `ResolveBody.companyId` to `z.coerce.number().int().positive()` as defense in depth, but the root cause is the untransformed bigint — fix at the source.
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: Currency-unaware cost aggregation produces misleading totals
|
||||
|
||||
**File:** `app/api/pax8/companies/[id]/route.ts:185`, `components/admin/DetailModal.tsx:463-483`
|
||||
|
||||
**Issue:** `SubscriptionBreakdownItem` carries a `currency` per subscription (correctly anticipating that subscriptions for one company could be billed in different currencies), but both the API's `costTotal` and the modal's own re-derived total sum `latestBilledAmount` across all subscriptions with a single `+` regardless of each item's currency:
|
||||
|
||||
```ts
|
||||
const costTotal = subscriptions.reduce((sum, s) => sum + s.latestBilledAmount, 0);
|
||||
```
|
||||
```tsx
|
||||
{(data.subscriptions[0]?.currency ?? 'USD')} {data.subscriptions.reduce((s: number, sub: any) => s + Number(sub.latestBilledAmount ?? 0), 0).toFixed(2)}
|
||||
```
|
||||
|
||||
If a company has subscriptions billed in more than one currency, the displayed total silently mixes amounts and labels the sum with whichever currency happens to belong to the first subscription. This is a real dollar-amount correctness bug on a cost-breakdown page.
|
||||
|
||||
**Fix:** Group by currency before summing, and either show one total per currency or explicitly flag "mixed currency" instead of collapsing to a single misleading number:
|
||||
```ts
|
||||
const totalsByCurrency = subscriptions.reduce<Record<string, number>>((acc, s) => {
|
||||
const cur = s.currency ?? 'USD';
|
||||
acc[cur] = (acc[cur] ?? 0) + s.latestBilledAmount;
|
||||
return acc;
|
||||
}, {});
|
||||
```
|
||||
|
||||
### WR-02: `/pax8` page silently mishandles non-2xx API responses
|
||||
|
||||
**File:** `app/pax8/page.tsx:103-126,220-236`
|
||||
|
||||
**Issue:** `fetchCompanies` and `handleRowClick` never check `res.ok` before consuming the body:
|
||||
|
||||
```ts
|
||||
const res = await fetch(`/api/pax8/companies?${params}`);
|
||||
const data = await res.json();
|
||||
setCompanies(data.items ?? []);
|
||||
```
|
||||
```ts
|
||||
const res = await fetch(`/api/pax8/companies/${row.id}`);
|
||||
const data = await res.json();
|
||||
setSelectedCompany({ ...data.company, subscriptions: data.subscriptions, costTotal: data.costTotal });
|
||||
setModalOpen(true);
|
||||
```
|
||||
|
||||
On a 404/400/500 the response body is `{ error: '...' }` with no `items`/`company` key. `fetchCompanies` will silently clear the table to empty (no toast, no error state) — indistinguishable from "zero results." `handleRowClick` is worse: it spreads `undefined` into `selectedCompany`, sets `modalOpen(true)`, and shows a broken modal titled `"PAX8: undefined"` with no groups rendered, instead of surfacing the failure. Contrast with `loadReviews()` in the same file, which does check `res.ok` and shows a proper `<Alert variant="destructive">`.
|
||||
|
||||
**Fix:** Check `res.ok` in both handlers and route failures to `toast.error(...)` / `reviewError`-style state, and don't open the modal on a failed fetch.
|
||||
|
||||
### WR-03: PAX8 company detail modal never surfaces match status/confidence/matched-company
|
||||
|
||||
**File:** `components/admin/DetailModal.tsx:138-160`
|
||||
|
||||
**Issue:** `PAX8_COMPANY_GROUPS` only surfaces `name, status, city, stateOrProvince, country, website` (Identity) and `id, syncedAt, isDeleted` (System). The API response for a single company also includes `autotaskCompanyId`, `matchConfidence`, `matchMethod`, and `matchedCompanyName` (`app/api/pax8/companies/[id]/route.ts:188-202`), all of which are spread into `selectedCompany` (`app/pax8/page.tsx:225-229`) — but none of them appear anywhere in the Formatted tab. A user drilling into a company from the list has no way to see whether/how it's matched to Autotask, or its confidence score, without switching to the Raw tab and reading unlabeled JSON keys. This directly undercuts the review/matching workflow that is the other half of this page.
|
||||
|
||||
**Fix:** Add a "Matching" field group (or fold into Identity) surfacing `matchedCompanyName`, `matchMethod`, and a formatted `matchConfidence` percentage.
|
||||
|
||||
### WR-04: Loosely-validated UUID regex allows malformed input through to a 500 instead of a clean 400
|
||||
|
||||
**File:** `app/api/pax8/companies/[id]/route.ts:13,79`, `app/api/pax8/company-matches/[id]/resolve/route.ts:34`
|
||||
|
||||
**Issue:** `const UUID_RE = /^[0-9a-f-]{36}$/i;` only checks character class and total length — it does not enforce the canonical `8-4-4-4-12` hyphen positions. Strings like 36 hyphens, or 36 hex characters with no hyphens at all, pass this check and are then bound as a query parameter compared against a `uuid` column. Postgres will throw `invalid input syntax for type uuid` for such strings, which is caught by the route's generic `catch` block and returned as a `500`, defeating the purpose of having an early validation guard that's supposed to produce a clean `400`.
|
||||
|
||||
**Fix:** Use a real UUID regex, e.g. `/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i`.
|
||||
|
||||
### WR-05: `pax8_company_match_review` queue join doesn't exclude soft-deleted PAX8 companies
|
||||
|
||||
**File:** `app/api/pax8/company-matches/route.ts:41-50`
|
||||
|
||||
**Issue:** The review query joins `pax8_company_match_review r` to `pax8_companies pc` with no `pc.is_deleted = false` filter, unlike the companies-list route which does filter (`app/api/pax8/companies/route.ts:74`). If a PAX8 company is soft-deleted after being flagged for review (e.g. it churned before anyone resolved the match), it will keep showing up indefinitely in the Needs Review queue and count badge, with no way to dismiss it short of manually resolving a match for a company that no longer matters.
|
||||
|
||||
**Fix:** Add `AND pc.is_deleted = false` to the `WHERE` clause (and to the paired `total` count query).
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: `DetailModal` recomputes the subscription total instead of using the API's `costTotal`
|
||||
|
||||
**File:** `components/admin/DetailModal.tsx:481`, `app/pax8/page.tsx:228`
|
||||
|
||||
**Issue:** `data.costTotal` is fetched from the API and spread into `selectedCompany`, but `DetailModal` never reads it — it independently re-derives the total via `data.subscriptions.reduce(...)`. The two computations happen to currently agree, but this is duplicated business logic that can silently drift (e.g. if the API's rounding/fallback logic changes) and `costTotal` becomes dead data.
|
||||
|
||||
**Fix:** Pass and render `data.costTotal` directly rather than recomputing it in the component.
|
||||
|
||||
### IN-02: No UI affordance for the `note` field the resolve API supports
|
||||
|
||||
**File:** `app/api/pax8/company-matches/[id]/resolve/route.ts:23`, `app/pax8/page.tsx:192-216`
|
||||
|
||||
**Issue:** `ResolveBody` accepts an optional `note` (max 500 chars) intended to capture why a reviewer chose a match, but `resolve()` in the page never collects or sends one (`body: JSON.stringify({ companyId })`). The backend affordance is effectively unreachable from the shipped UI.
|
||||
|
||||
**Fix:** Either add a small text input for an optional note before resolving, or remove the unused parameter/plan to add it in a follow-up and note that explicitly.
|
||||
|
||||
### IN-03: Concurrent double-submit possible between "Link company" and "Link to selected company" for the same review
|
||||
|
||||
**File:** `app/pax8/page.tsx:192-216,379-398,466-484`
|
||||
|
||||
**Issue:** The `resolving` guard is keyed per `${reviewId}:${companyId}`, so clicking a suggested-candidate "Link company" button and the manual-search "Link to selected company" button for the *same review* but *different* target companies are not mutually exclusive — both requests can be in flight simultaneously. The resolver correctly returns `already_resolved` (409) for the loser, so no data corruption occurs, but the user sees a confusing "Resolve failed: Already resolved" toast with no indication which action actually won.
|
||||
|
||||
**Fix:** Disable all resolve controls for a review (not just the specific key) while any resolve request for that `reviewId` is in flight.
|
||||
|
||||
### IN-04: Redundant duplicate fetch when sorting/searching while not on page 1
|
||||
|
||||
**File:** `app/pax8/page.tsx:128-144`
|
||||
|
||||
**Issue:** `handleSort`/`handleSearch` call `fetchCompanies(1, ...)` directly and also `setPage(1)`, which re-triggers the `useEffect` keyed on `[page]` when the previous page wasn't already 1 — firing a second, redundant `fetchCompanies` call with identical arguments. Not a correctness bug (last response wins) but causes an unnecessary duplicate network request and a double loading-state flicker.
|
||||
|
||||
**Fix:** Either drop the direct `fetchCompanies` call in `handleSort`/`handleSearch` and rely solely on the `page`-effect (also depending on `sortColumn`/`sortOrder`/`searchQuery`), or skip the effect-driven fetch when the direct call already covers it.
|
||||
|
||||
### IN-05: `resolveLabel`'s date-parsing `try/catch` is effectively dead code
|
||||
|
||||
**File:** `components/admin/DetailModal.tsx:182-195`
|
||||
|
||||
**Issue:** `new Date(value)` never throws for malformed strings — it returns an `Invalid Date` object instead — so the `catch { break; }` fallback is unreachable in practice. A malformed `syncedAt` (now also reachable via the new `pax8_company` kind) would render as the literal text "Invalid Date" rather than falling through to generic string rendering. Pre-existing pattern, but now also exercised by the new PAX8 fields.
|
||||
|
||||
**Fix:** Check `Number.isNaN(d.getTime())` explicitly and fall through to generic rendering when true, instead of relying on a try/catch that can't fire.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-07-11_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
Loading…
Add table
Add a link
Reference in a new issue