✨ Code Quality & Standards

📋 Summary

The code is disciplined and consistent, and its measured quality is above what is typical for a product of this age and size. The type checker runs in strict mode with zero errors, the linter reports zero warnings, duplicated code is low (2.3%), and the escape hatches developers use to silence the tools are rare and almost all justified. Unusually, the team has turned its own coding rules into automated tests. One example is a test that fails the build if a database write's result is never checked. The main weaknesses are a handful of very large, very complex functions and screens that will be costly to change safely. Request validation is done by hand rather than with a schema library. Error reporting to production is close to non-existent. Some unreachable code is still in the tree. Comments are exceptionally plentiful and written as a narrative history, which is a real asset for understanding why the code is as it is, but it also adds reading load and goes stale.

🎯 Rating

🟢 ●●●●○ 4 / 5 — Green. Strict typing, clean lint, low duplication and self-enforcing quality gates are strong signals. The score is held back by concentrated complexity (52 functions with cyclomatic complexity over 20), hand-rolled validation, missing observability and leftover dead code.

🔍 Findings

Q1. Measured baselines (from the shared brief, not re-run)

Q2. Compiler and linter configuration

• [Observed] tsconfig.json: "strict": true (:7), skipLibCheck, isolatedModules, allowJs, target ES2017. It does not enable noUncheckedIndexedAccess, exactOptionalPropertyTypes, noImplicitReturns, noUnusedLocals or noUnusedParameters, so array and record indexing is typed as always present.
• [Observed] eslint.config.mjs uses only the stock eslint-config-next core-web-vitals and typescript presets. There are no project-specific rules, no import-ordering or complexity rules, and design/, scripts/ and e2e/ are excluded on purpose. A clean lint therefore means "meets Next's defaults", not a bespoke standard.
• [Observed] Typed database access: src/types/database.ts (1,150 lines) types the Supabase client (SupabaseClient<Database>). [Inferred] It is hand-maintained or generated outside CI, since no generation script runs in the pipeline. src/lib/schema-columns.test.ts compensates by checking queried and written columns against supabase/baseline.sql.

Q3. Escape hatches

[Observed] The product code is close to free of type escapes. The double-casts are concentrated in the harness that fakes the Supabase client.

Q4. Error-handling discipline

• [Observed] src/lib/write-results.test.ts (135 lines) walks every non-test .ts/.tsx under src/app (excluding src/app/qa) and fails if:

  1. an awaited write (insert/update/upsert/delete) captures nothing;
  2. a DELETE route or its harness-store mirror does not count affected rows;
  3. a user-client update ends in .single().

  A deliberate exception must be declared on the line above as // unread-write: <reason>. There are 13 such declarations. Limitation: it scans src/app only, so helpers in src/lib that write (e.g. notify.ts, rounds-server.ts, review-notify.ts) are not covered.

• [Observed] src/lib/schema-columns.test.ts (142 lines) refuses any selected, filtered or written column missing from supabase/baseline.sql, and asserts how many write literals it read so the scan cannot pass vacuously.

• [Observed] In src/app/api: 178 destructures read error and 151 take { data } alone. Most of the latter are authorisation or lookup reads where a null fails closed, e.g. api-auth.ts. There are 14 try blocks in the API tree. Errors go back to the client verbatim (error: x.message) about 121 times. That is honest for users but leaks DB detail (see 09-security F9). 19 handlers call await request.json() without a .catch, so a malformed body becomes an unhandled 500 rather than a 400.

• [Observed] Write sequences are rolled back by hand where a multi-row change must be all-or-nothing (e.g. the Writes helper in frames/card-writes.ts). There is no DB transaction or RPC for these, so atomicity rests on compensating writes.

• [Observed] There are no error.tsx or global-error.tsx boundaries anywhere in src/app (find src/app -name error.tsx returns nothing), so a server-component exception shows Next's default error page.

Q5. Logging and observability

• [Observed] 11 console.* calls in non-test code (9 error, 2 warn). Only 2 are in API routes (admin/invite, feedback). The rest are in lib/activity.ts, lib/review-notify.ts, the dev auto-sign-in and two pages. Most failures are therefore either returned to the client or swallowed (.catch(() => {}) on emails, e.g. forgot-password/route.ts:58).
• [Observed] There is no structured logger, error tracker, APM or log drain in package.json (no Sentry, pino, winston, OpenTelemetry or Datadog). The app_errors table exists in the schema but has no writer (only types/database.ts and the QA store mention it). activity_log (via logActivity / track) is a product-usage audit trail, not operational telemetry. The repo says so itself in design/COMPLIANCE.md §8.
• [Inferred] Production incidents can be diagnosed only from Vercel's default function logs and the Supabase dashboard.

Q6. Input validation

• [Observed] There is no schema-validation library (no zod, valibot, yup or joi in package.json). Validation is hand-rolled per route: 122 typeof checks in src/app/api, length clamps (.trim().slice(0, 240)), allow-lists (ALLOWED_FIELDS, EDITABLE_FIELDS in ad-units/[unitId]/fields.ts, VALID_ROLES, isDiscipline) and a shared UUID_RE (src/app/api/track/actions.ts).
• [Inferred] The checks are generally careful, but there is no single contract per endpoint, so request shapes are documented only in prose (docs/API.md) and nothing guarantees that a new field is validated. A concrete miss is in 09-security F4: brief_text is sanitised on one route and accepted raw on another.

Q7. Complexity and very large files

Top 12 non-test source files by line count (wc -l, src/):

High-complexity hot spots from the brief: for-you.ts forYouItems CCN 109; ad-unit PATCH 66; brief PATCH 64; review/files/[fileId] PATCH 59; render-push POST 53; jobs POST 52; cron purge-review GET 44. [Inferred] These are the business-critical write paths. Combined with 0.9% line coverage of src/app/api (brief), they are the riskiest code to modify. Several UI components exceed 800–1,300 lines and mix data fetching, state and rendering.

Q8. Duplication: worst offenders (jscpd JSON report)

Files with the most duplicated lines overall: admin/users/[id]/route.ts (120), qa/store.ts (115), workspace/campaign/[id]/layout.tsx (108), ad-units/[id]/route.ts (100), plan/route.ts (100).

[Observed] Two systemic patterns drive this. First, the QA harness re-implements route behaviour in store.ts, a parallel implementation that must be kept in sync by hand. scripts/harness-contract.js checks only that routes are matched, not that behaviour is. Second, authorisation boilerplate: 25 inline role === 'admin' checks and 7 local requireStaff/requireAdmin functions exist alongside isStaff in api-auth.ts.

Q9. Dead and commented-out code

• [Observed] CLAUDE.md says the checklist and build files are unreachable and "to be deleted". They are still present:

  • src/app/(app)/campaign/[id]/checklist/ (checklist-view.tsx 432 + page.tsx 35 lines)
  • src/app/(app)/campaign/[id]/build/ (build-view.tsx 328 + page.tsx 26)
  • redirect stubs assets/page.tsx (12) and members/page.tsx (12)
  • e2e/checklist.spec.ts (80)

  That is about 925 lines. They are reachable only as source, because next.config.ts redirects their URLs. (members/members-page.tsx is not dead: it is imported by campaign-header.tsx and workspace/.../header-tile.tsx.)

• [Observed] Further deliberately orphaned code is kept as "groundwork": the brief, review and sign-off pipeline (/timeline, /debrief) and the Studio schema left behind by migration 059 (tables with no readers). See CLAUDE.md "Parked decisions".

• [Observed] Commented-out code is nearly absent: 5 lines match // const|let|await|return|import|if (|export|function. There are 0 TODO/FIXME/HACK/XXX markers.

• [Observed] One declared dependency is unused (google-auth-library, never imported), plus date-fns per module 08. The feature flag SHOW_ASK = false (src/lib/flags.ts:10) keeps the whole Ask sheet compiled but inert.

Q10. Naming, readability and comment style

• [Observed] Naming is consistent and domain-driven (canEditArea, carriesLink, purgableAfterHandover, copyPatchFromParsed). Single sources of truth are named and referenced, e.g. "ONE list" in track/actions.ts and "the one rule" in placement.ts.
• [Observed] Comments are unusually dense (about 28 per 100 code lines) and narrative. They record decisions, past bugs and their dates in prose. Examples: the header of ad-units/[unitId]/route.ts, 30-line essays at the top of preview/[token]/custody/route.ts and write-results.test.ts, and migration numbers cited inline about 250 times ((0NN)). CLAUDE.md (832 lines) continues the same voice.
• [Inferred] Strength: the "why" behind non-obvious rules is preserved next to the code, which mitigates the squashed git history (single commit). Weaknesses: (a) the reading cost is high; (b) prose decays. 09-security found CLAUDE.md claiming email names are escaped and that harness routes 404 outside QA, and both are untrue in code. (c) Some comments describe history rather than current behaviour, which a new maintainer must filter.

🧾 Evidence

• Facts reused from the shared brief: tsc, lint, jscpd totals, lizard, cloc and coverage figures.
• cat tsconfig.json eslint.config.mjs
• grep -rn "eslint-disable" src | wc -l → 27, with breakdown by rule. @ts-ignore/@ts-expect-error/@ts-nocheck → 0. as unknown as → 82 (6 outside app/qa and tests). as any → 1.
• grep -rnE "console\.(log|warn|error|info|debug)" src → 11.
• In src/app/api: const { data } = await → 151; { … error … } = await → 178; try { → 14; error: x.message → 121; await request.json() without catch → 19; typeof → 122; role (!==|===) 'admin' → 25; local require(Staff|Admin) definitions → 7.
• python3 over /tmp/claude-0/dd/jscpd/jscpd-report.json, aggregating duplicated lines by file pair and by file.
• find src -name '*.ts*' ! -name '*.test.*' | xargs wc -l | sort -rn for the top files.
• ls and wc -l of src/app/(app)/campaign/[id]/{checklist,build,assets,members} and e2e/checklist.spec.ts; grep for importers of members-page.
• Read: src/lib/write-results.test.ts, src/lib/api-auth.ts, src/lib/supabase/*.ts, src/app/api/campaigns/[id]/ad-units/[unitId]/route.ts (header), frames/card-writes.ts, package.json.

⚠️ Risks

✅ Recommendations