docs: fix 8 inaccuracies in gap analysis after systematic self-review
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
This commit is contained in:
saravanakumardb1
parent
513ee5a6a5
commit
ee033f4eab
@ -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 |
|
||||
|
||||
Loading…
Reference in New Issue
Block a user