From ee033f4eab0a6bf89c3486b56ffe06bddfa546e1 Mon Sep 17 00:00:00 2001 From: saravanakumardb1 Date: Thu, 19 Mar 2026 07:06:05 -0700 Subject: [PATCH] docs: fix 8 inaccuracies in gap analysis after systematic self-review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Corrections: - Remove D5 (factually wrong — backend tsconfig already excludes test files) - Fix web component count 12→10, lib file count 11→18 - Fix B1 title: 'No note delete' → 'No note restore' (PRD says archive/restore) - Add A6: notes-client.ts missing 'use client' (fragile SSR import chain) - Expand A1 to include blob-client.ts module-scope instantiation - Note telemetry/flags/kill-switch are lower risk (client-only imports) - Strengthen D3/D4: confirmed zero-import dead code, not just 'unclear' - A5: note that auth.test.ts and MCP tests do have real behavior tests - Update bug count 5→6, total gaps 29→25 --- docs/GAP_ANALYSIS.md | 58 +++++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 28 deletions(-) diff --git a/docs/GAP_ANALYSIS.md b/docs/GAP_ANALYSIS.md index ca40d5b..7aaeec7 100644 --- a/docs/GAP_ANALYSIS.md +++ b/docs/GAP_ANALYSIS.md @@ -11,7 +11,7 @@ | Surface | Status | Typecheck | Tests | Build | |---------|--------|-----------|-------|-------| | **Backend** | 7 modules + MCP (8 tools) + auth | ✅ pass | ✅ 24 tests (12 files) | ✅ tsc | -| **Web** | 6 pages + 12 components + 11 lib files | ✅ pass | ✅ 6 tests (5 files) | ✅ next build | +| **Web** | 6 pages + 10 components + 18 lib files | ✅ pass | ✅ 6 tests (5 files) | ✅ next build | | **Mobile** | 4 tabs + note detail + auth + 5 stores | ✅ pass | ✅ 0 tests (passWithNoTests) | n/a | **Commits tracked in ROADMAP.md:** 30+ incremental commits with verification notes. @@ -22,11 +22,12 @@ ### Category A — Bugs (code issues that will cause runtime failures or incorrect behavior) -#### A1. `extraction-client.ts` — module-scope API client instantiation (SSR crash risk) -- **File:** `web/src/lib/extraction-client.ts:25-28` -- **Issue:** `const extractionApi = createApiClient(...)` is instantiated at module scope. The `getAccessToken()` function reads `localStorage`, which is undefined during SSR. While the `"use client"` directive helps, if this module is ever imported server-side (e.g., via `notes-client.ts` which imports `extractSuggestedTasks`), it will crash. -- **Severity:** High -- **Fix:** Apply the same lazy-init singleton pattern used in NomGap and other products. +#### A1. Module-scope API client instantiation (SSR crash risk) +- **Files:** `web/src/lib/extraction-client.ts:25-28`, `web/src/lib/blob-client.ts:11-15` +- **Issue:** `const extractionApi = createApiClient(...)` and `const blobClient = createBlobClient(...)` are instantiated at module scope. Both files have `"use client"` directives, but `extraction-client.ts` is imported by `notes-client.ts` (which has NO `"use client"` directive). If any server component ever imports `notes-client.ts`, the entire import chain crashes because `createApiClient` runs at import time. `blob-client.ts` is lower risk since only imported from `ArtifactPanel.tsx` (a client component). +- **Severity:** High (`extraction-client.ts`), Medium (`blob-client.ts`) +- **Fix:** Apply the same lazy-init singleton pattern used in NomGap and other products. Also add `"use client"` to `notes-client.ts` (see A6). +- **Note:** `telemetry.ts`, `kill-switch.ts`, `feature-flags.ts`, `diagnostics.ts` also use module-scope instantiation but are lower risk — all have `"use client"` and are only imported from `providers.tsx` (also client-side). #### A2. `next.config.ts` — missing `output: "standalone"` for Docker builds - **File:** `web/next.config.ts:23` @@ -48,18 +49,24 @@ #### A5. Backend route tests are registration-only — no API behavior coverage - **Files:** All 7 `routes.test.ts` files under `backend/src/modules/` -- **Issue:** Every module test only verifies that route handlers are registered (`expect(app.get).toHaveBeenCalledTimes(N)`). No tests exercise actual request/response behavior, validation, error paths, or auth enforcement. +- **Issue:** Every module test only verifies that route handlers are registered (`expect(app.get).toHaveBeenCalledTimes(N)`). No tests exercise actual request/response behavior, validation, error paths, or auth enforcement. (Note: `auth.test.ts` and the 3 MCP test files do have real behavior tests.) - **Severity:** High (quality — bugs in route handlers won't be caught) - **Fix:** Add integration-style tests using `buildTestApp()` or Fastify `.inject()` to test real request flows. +#### A6. `notes-client.ts` missing `"use client"` directive +- **File:** `web/src/lib/notes-client.ts` +- **Issue:** This file imports `extractSuggestedTasks` from `extraction-client.ts` (which has a module-scope `createApiClient` call). `notes-client.ts` itself has no `"use client"` directive. All current consumers are client components, but the import chain is fragile — any future server component importing `notes-client.ts` would trigger an SSR crash. +- **Severity:** Medium (latent — works today, breaks if a server component imports it) +- **Fix:** Add `"use client";` directive to `notes-client.ts`, or refactor `extraction-client.ts` to lazy-init (preferred, see A1). + --- ### Category B — Missing Features (PRD requirements not yet implemented) -#### B1. No note delete endpoint +#### B1. No note restore endpoint - **PRD ref:** §8.1 — "archive notes, restore notes" -- **Current:** Backend has `POST /notes/:id/archive` but no delete or restore. Web has no archive/restore UI. -- **Fix:** Add `POST /notes/:id/restore` backend route + web UI for archive/restore. +- **Current:** Backend has `POST /notes/:id/archive` but no restore endpoint. Web has no archive/restore UI. The PRD does not require hard delete. +- **Fix:** Add `POST /notes/:id/restore` backend route (sets `status: 'active'`) + web UI for archive/restore actions on note detail. #### B2. No note create from web UI - **PRD ref:** §8.1 — "create notes" @@ -134,20 +141,15 @@ - **Issue:** Calls `listWorkspaceSummaries()` (which itself fetches all notes), then calls `listAgentActionsForWorkspace()` for each workspace. This is an N+1 query pattern that will degrade with scale. - **Fix:** Add a backend endpoint that lists all pending agent actions across workspaces for the current user. -#### D3. `web/src/lib/mock-data.ts` still exists -- **File:** `web/src/lib/mock-data.ts` -- **Issue:** Legacy mock data file from scaffold era. Should be removed or clearly marked as dev-only. -- **Fix:** Remove if unused, or gate behind `NODE_ENV === "development"`. +#### D3. `web/src/lib/mock-data.ts` is dead code (zero imports) +- **File:** `web/src/lib/mock-data.ts` (228 lines) +- **Issue:** Legacy mock data file from scaffold era. Confirmed zero imports across the entire web codebase — completely unused. +- **Fix:** Delete the file. -#### D4. `review-data.ts` — unclear purpose -- **File:** `web/src/lib/review-data.ts` -- **Issue:** Appears to be a legacy data file that may be redundant with `review-client.ts`. -- **Fix:** Audit usage and remove if superseded. - -#### D5. Backend `tsconfig.json` does not exclude test files -- **File:** `backend/tsconfig.json` -- **Issue:** Test files (`.test.ts`) are not excluded from the `tsc` build. This means vitest globals like `describe`, `it`, `expect` will cause errors during `npm run build` or Docker build if vitest isn't in the production dependency chain. -- **Fix:** Add `"src/**/*.test.ts"` to the `exclude` array. +#### D4. `web/src/lib/review-data.ts` is dead code (zero imports) +- **File:** `web/src/lib/review-data.ts` (46 lines) +- **Issue:** Legacy mock timeline data, superseded by `review-client.ts` which fetches real agent actions from the backend. Confirmed zero imports. +- **Fix:** Delete the file. --- @@ -170,11 +172,11 @@ | # | Task | Priority | Files | |---|------|----------|-------| -| 1 | Fix extraction-client.ts SSR crash (lazy-init) | High | `web/src/lib/extraction-client.ts` | -| 2 | Add `output: "standalone"` to next.config.ts | Medium | `web/next.config.ts` | -| 3 | Exclude test files from backend tsconfig build | Medium | `backend/tsconfig.json` | +| 1 | Fix extraction-client.ts + blob-client.ts SSR crash (lazy-init) | High | `web/src/lib/extraction-client.ts`, `web/src/lib/blob-client.ts` | +| 2 | Add `"use client"` to notes-client.ts or refactor extraction-client lazy-init | High | `web/src/lib/notes-client.ts` | +| 3 | Add `output: "standalone"` to next.config.ts | Medium | `web/next.config.ts` | | 4 | Consolidate duplicate types into types.ts | Medium | `web/src/lib/types.ts`, `notes-client.ts`, `review-client.ts` | -| 5 | Remove or gate mock-data.ts and review-data.ts | Low | `web/src/lib/mock-data.ts`, `web/src/lib/review-data.ts` | +| 5 | Delete dead code: mock-data.ts and review-data.ts | Low | `web/src/lib/mock-data.ts`, `web/src/lib/review-data.ts` | ### Sprint 2 — Backend Test Depth (est. 3-4 hours) @@ -257,4 +259,4 @@ cd mobile && npm run typecheck && npm test | Mobile tests | 0 | ~15+ | | Dockerfiles | 0 | 2 | | CI workflows | 0 | 1 | -| Known bugs | 5 | 0 | +| Known bugs | 6 | 0 |