|
| 1 | +# Code Review Report: PRD View - Document Creation & Discovery |
| 2 | + |
| 3 | +**Date:** 2026-02-05 |
| 4 | +**Reviewer:** Code Review Agent |
| 5 | +**Component:** PRD View (PR #337, Issue #330) |
| 6 | +**Files Reviewed:** 34 files (20 source, 8 test, 1 mock, 1 docs, 4 config) |
| 7 | +**Ready for Production:** Yes, with 2 major issues recommended for near-term fix |
| 8 | + |
| 9 | +## Executive Summary |
| 10 | + |
| 11 | +This PR implements the full PRD View for Phase 3 UI — a well-structured, component-driven implementation across 9 incremental commits. The code follows established project patterns (Shadcn/UI Nova template, Hugeicons, SWR, axios namespace pattern) and includes 64 new tests. Two major reliability issues were found (missing error handling in page.tsx handlers and a misused React hook in DiscoveryPanel), plus a few minor improvements. No critical security vulnerabilities. |
| 12 | + |
| 13 | +**Critical Issues:** 0 |
| 14 | +**Major Issues:** 2 |
| 15 | +**Minor Issues:** 4 |
| 16 | +**Positive Findings:** 7 |
| 17 | + |
| 18 | +--- |
| 19 | + |
| 20 | +## Review Context |
| 21 | + |
| 22 | +**Code Type:** Frontend (Next.js React components, hooks, API client) |
| 23 | +**Risk Level:** Medium (user input handling, file upload, SSE connections, AI chat rendering) |
| 24 | +**Business Constraints:** Phase 3 UI rebuild — first user-facing view beyond workspace |
| 25 | + |
| 26 | +### Review Focus Areas |
| 27 | + |
| 28 | +- ✅ A03 - Injection/XSS — User markdown rendered via react-markdown, file upload content |
| 29 | +- ✅ Reliability — Error handling in async handlers, resource cleanup in SSE hooks |
| 30 | +- ✅ Resource Management — EventSource lifecycle, FileReader cleanup, SWR cache management |
| 31 | +- ✅ A06 - Vulnerable Components — Dependency audit |
| 32 | +- ❌ OWASP LLM Top 10 — Skipped (frontend doesn't interact with LLM directly) |
| 33 | +- ❌ Zero Trust / Auth — Skipped (auth is backend concern; API client already has `withCredentials: true`) |
| 34 | +- ❌ Performance — Skipped (not performance-critical UI code) |
| 35 | + |
| 36 | +--- |
| 37 | + |
| 38 | +## Priority 1 Issues - Critical ⛔ |
| 39 | + |
| 40 | +None found. |
| 41 | + |
| 42 | +--- |
| 43 | + |
| 44 | +## Priority 2 Issues - Major ⚠️ |
| 45 | + |
| 46 | +### 1. Missing error handling in `handleSavePrd` and `handleGenerateTasks` |
| 47 | + |
| 48 | +**Location:** `web-ui/src/app/prd/page.tsx:89-103` and `web-ui/src/app/prd/page.tsx:118-127` |
| 49 | +**Severity:** Major |
| 50 | +**Category:** Reliability |
| 51 | + |
| 52 | +**Problem:** |
| 53 | +Both `handleSavePrd` and `handleGenerateTasks` use `try...finally` without a `catch` block. If the API call fails, the error propagates as an unhandled rejection. Unlike `DiscoveryPanel` and `UploadPRDModal` (which properly catch and display errors), these handlers silently fail — the user sees the spinner stop but gets no feedback about what went wrong. |
| 54 | + |
| 55 | +**Current Code:** |
| 56 | +```typescript |
| 57 | +const handleSavePrd = async (content: string, changeSummary: string) => { |
| 58 | + if (!prd || !workspacePath) return; |
| 59 | + setIsSaving(true); |
| 60 | + try { |
| 61 | + const updated = await prdApi.createVersion(...); |
| 62 | + mutatePrd(updated, false); |
| 63 | + } finally { |
| 64 | + setIsSaving(false); |
| 65 | + } |
| 66 | +}; |
| 67 | +``` |
| 68 | + |
| 69 | +**Recommended Fix:** |
| 70 | +```typescript |
| 71 | +const handleSavePrd = async (content: string, changeSummary: string) => { |
| 72 | + if (!prd || !workspacePath) return; |
| 73 | + setIsSaving(true); |
| 74 | + try { |
| 75 | + const updated = await prdApi.createVersion(...); |
| 76 | + mutatePrd(updated, false); |
| 77 | + } catch (err) { |
| 78 | + const apiError = err as ApiError; |
| 79 | + console.error('[PRD] Save failed:', apiError.detail); |
| 80 | + // TODO: Show error toast/banner to user |
| 81 | + } finally { |
| 82 | + setIsSaving(false); |
| 83 | + } |
| 84 | +}; |
| 85 | +``` |
| 86 | + |
| 87 | +**Why This Fix Works:** |
| 88 | +Prevents unhandled promise rejections and gives the user feedback. A toast/notification system would be the ideal UX, but at minimum logging prevents silent failures. |
| 89 | + |
| 90 | +--- |
| 91 | + |
| 92 | +### 2. Misuse of `useState` as initializer in DiscoveryPanel |
| 93 | + |
| 94 | +**Location:** `web-ui/src/components/prd/DiscoveryPanel.tsx:68-70` |
| 95 | +**Severity:** Major |
| 96 | +**Category:** Reliability / Correctness |
| 97 | + |
| 98 | +**Problem:** |
| 99 | +`useState` is being used with a callback to auto-start the discovery session on mount. This is an unconventional pattern — `useState`'s initializer runs during the first render (synchronously), but here it triggers an async side effect (`startSession()`). This works coincidentally because React state initializers run once, but: |
| 100 | +1. It violates React's rules — side effects should use `useEffect` |
| 101 | +2. The async call fires during render, not after mount |
| 102 | +3. React StrictMode in development will call it twice |
| 103 | + |
| 104 | +**Current Code:** |
| 105 | +```typescript |
| 106 | +useState(() => { |
| 107 | + if (!sessionId) startSession(); |
| 108 | +}); |
| 109 | +``` |
| 110 | + |
| 111 | +**Recommended Fix:** |
| 112 | +```typescript |
| 113 | +useEffect(() => { |
| 114 | + if (!sessionId) startSession(); |
| 115 | + // eslint-disable-next-line react-hooks/exhaustive-deps |
| 116 | +}, []); |
| 117 | +``` |
| 118 | + |
| 119 | +**Why This Fix Works:** |
| 120 | +`useEffect` with `[]` runs after the component mounts, which is the correct lifecycle for firing API calls. The eslint-disable is needed because `startSession` and `sessionId` are intentionally excluded (we only want to run on mount). |
| 121 | + |
| 122 | +--- |
| 123 | + |
| 124 | +## Priority 3 Issues - Minor 📝 |
| 125 | + |
| 126 | +### 1. `sessionId` interpolated directly into URL path |
| 127 | + |
| 128 | +**Location:** `web-ui/src/lib/api.ts:258`, `270` |
| 129 | +**Severity:** Minor |
| 130 | +**Category:** A03 - Injection (Defense in depth) |
| 131 | + |
| 132 | +**Recommendation:** |
| 133 | +`sessionId` is interpolated into the URL path via template literal: `` `/api/v2/discovery/${sessionId}/answer` ``. While `sessionId` comes from the server (not user input), encoding it would add defense-in-depth against future misuse. |
| 134 | + |
| 135 | +**Suggested Approach:** |
| 136 | +```typescript |
| 137 | +`/api/v2/discovery/${encodeURIComponent(sessionId)}/answer` |
| 138 | +``` |
| 139 | + |
| 140 | +This is a nitpick — the backend validates the session ID format, and the value originates from the server. No immediate risk. |
| 141 | + |
| 142 | +--- |
| 143 | + |
| 144 | +### 2. No file size limit on upload |
| 145 | + |
| 146 | +**Location:** `web-ui/src/components/prd/UploadPRDModal.tsx:45-67` |
| 147 | +**Severity:** Minor |
| 148 | +**Category:** Reliability |
| 149 | + |
| 150 | +**Recommendation:** |
| 151 | +The file upload handler reads the entire file into memory via `FileReader.readAsText()` without checking file size. A user could accidentally select a very large file (e.g., a binary misnamed `.md`), causing browser memory issues. |
| 152 | + |
| 153 | +**Suggested Approach:** |
| 154 | +Add a size check before reading: |
| 155 | +```typescript |
| 156 | +const MAX_FILE_SIZE = 5 * 1024 * 1024; // 5 MB |
| 157 | +if (file.size > MAX_FILE_SIZE) { |
| 158 | + setError('File too large (max 5 MB)'); |
| 159 | + return; |
| 160 | +} |
| 161 | +``` |
| 162 | + |
| 163 | +--- |
| 164 | + |
| 165 | +### 3. `genResp` variable unused in `handleGeneratePrd` |
| 166 | + |
| 167 | +**Location:** `web-ui/src/components/prd/DiscoveryPanel.tsx:124` |
| 168 | +**Severity:** Minor |
| 169 | +**Category:** Code Quality |
| 170 | + |
| 171 | +**Recommendation:** |
| 172 | +The response from `discoveryApi.generatePrd()` is assigned to `genResp` but never used — the function immediately fetches the full PRD via `prdApi.getLatest()`. This is correct behavior (the generate endpoint returns a preview, not the full PRD), but the unused variable should be removed for clarity. |
| 173 | + |
| 174 | +**Suggested Approach:** |
| 175 | +```typescript |
| 176 | +await discoveryApi.generatePrd(sessionId, workspacePath); |
| 177 | +const fullPrd = await prdApi.getLatest(workspacePath); |
| 178 | +``` |
| 179 | + |
| 180 | +--- |
| 181 | + |
| 182 | +### 4. DiscoveryPanel and PRDView lack test coverage |
| 183 | + |
| 184 | +**Location:** `web-ui/src/components/prd/DiscoveryPanel.tsx`, `PRDView.tsx` |
| 185 | +**Severity:** Minor |
| 186 | +**Category:** Test Coverage |
| 187 | + |
| 188 | +**Recommendation:** |
| 189 | +DiscoveryPanel (0% coverage) and PRDView (0% coverage) are the two orchestrator components that tie everything together. While their child components are well-tested (93-100%), the orchestrators contain the async API flow logic (session start, answer submission, PRD generation) that is most likely to break in production. |
| 190 | + |
| 191 | +**Suggested Approach:** |
| 192 | +Add tests that mock `@/lib/api` and SWR, then verify: |
| 193 | +- DiscoveryPanel: session auto-start, answer submission flow, error display |
| 194 | +- PRDView: loading/empty/content state rendering, discovery toggle |
| 195 | + |
| 196 | +--- |
| 197 | + |
| 198 | +## Positive Findings ✨ |
| 199 | + |
| 200 | +### Excellent Practices |
| 201 | +- **Consistent error pattern:** All API-facing components (`DiscoveryPanel`, `UploadPRDModal`, `DiscoveryInput`) use `try/catch/finally` with typed `ApiError` extraction — follows the project's established `normalizeErrorDetail` pattern. |
| 202 | +- **SWR optimistic updates:** `mutatePrd(newPrd, false)` correctly uses `false` for `revalidate` to avoid redundant refetches after mutations. |
| 203 | +- **Proper React patterns:** Stable callback refs in `useEventSource` prevent unnecessary effect re-runs. `useCallback` used consistently for handlers passed to children. |
| 204 | + |
| 205 | +### Good Architectural Decisions |
| 206 | +- **Incremental commits:** Each of the 9 commits is independently reviewable and represents a testable increment — excellent for bisecting bugs. |
| 207 | +- **Component separation:** Clear responsibility split (PRDView = layout, PRDHeader = actions, MarkdownEditor = content, DiscoveryPanel = chat lifecycle). Each component is independently testable. |
| 208 | +- **Generic + specific hook pattern:** `useEventSource` (generic SSE) wrapping into `useTaskStream` (typed for task events) is a clean, reusable pattern. |
| 209 | + |
| 210 | +### Security Wins |
| 211 | +- **react-markdown 10.1.0:** Uses `micromark` parser which does not support raw HTML by default — safe against XSS in markdown content without needing `rehype-raw` or `rehype-sanitize`. |
| 212 | +- **`accept` attribute on file input:** Limits file picker to `.md,.markdown,.txt` — client-side defense against wrong file types. |
| 213 | +- **API client `withCredentials: true`:** Already configured in the existing axios instance — cookies sent with cross-origin requests, matching backend auth pattern. |
| 214 | +- **Zero npm audit vulnerabilities:** `npm audit --production` shows 0 vulnerabilities. |
| 215 | + |
| 216 | +--- |
| 217 | + |
| 218 | +## Team Collaboration Needed |
| 219 | + |
| 220 | +### Handoffs to Other Agents |
| 221 | + |
| 222 | +**Architecture Agent:** |
| 223 | +- The `AppSidebar` reads workspace state from `localStorage` independently of the workspace page's own state management. If the workspace is deselected on the home page, the sidebar relies on a `storage` event listener to update. Consider a shared React context for workspace state to ensure consistency. |
| 224 | + |
| 225 | +**UX Designer Agent:** |
| 226 | +- Error feedback for `handleSavePrd` and `handleGenerateTasks` failures currently has no visual indicator — user sees spinner stop but no message. A toast/notification system should be prioritized. |
| 227 | +- Disabled nav items in the sidebar show as dimmed text with no tooltip on mobile (icon-only mode). Consider adding `title` tooltips on the icon-only view. |
| 228 | + |
| 229 | +--- |
| 230 | + |
| 231 | +## Testing Recommendations |
| 232 | + |
| 233 | +### Unit Tests Needed |
| 234 | +- [x] PRDHeader (10 tests) ✅ |
| 235 | +- [x] AssociatedTasksSummary (4 tests) ✅ |
| 236 | +- [x] MarkdownEditor (8 tests) ✅ |
| 237 | +- [x] DiscoveryTranscript (6 tests) ✅ |
| 238 | +- [x] DiscoveryInput (9 tests) ✅ |
| 239 | +- [x] AppSidebar (7 tests) ✅ |
| 240 | +- [x] useEventSource (9 tests) ✅ |
| 241 | +- [x] useTaskStream (9 tests) ✅ |
| 242 | +- [ ] DiscoveryPanel (mock API, test session lifecycle) |
| 243 | +- [ ] PRDView (test state-driven rendering) |
| 244 | +- [ ] UploadPRDModal (mock prdApi.create, test submit flow) |
| 245 | + |
| 246 | +### Integration Tests |
| 247 | +- [ ] Full discovery flow: mount → auto-start → answer questions → generate PRD |
| 248 | +- [ ] Upload PRD via paste → verify editor populated |
| 249 | +- [ ] Task generation → verify AssociatedTasksSummary updates |
| 250 | + |
| 251 | +--- |
| 252 | + |
| 253 | +## Future Considerations |
| 254 | + |
| 255 | +### Patterns for Project Evolution |
| 256 | +- **Toast/notification system:** Multiple components need user-facing error feedback. Consider adding a lightweight toast (e.g., Sonner or Shadcn Toast) before building more views. |
| 257 | +- **Workspace context:** As more pages are added (Tasks, Execution, Blockers, Review), workspace state should move from localStorage + per-page hooks to a shared React context. |
| 258 | + |
| 259 | +### Technical Debt Items |
| 260 | +- `useState` misuse in DiscoveryPanel (issue #2 above) — should be fixed before more components copy this pattern |
| 261 | +- Unused `genResp` variable (issue #3 above) |
| 262 | + |
| 263 | +--- |
| 264 | + |
| 265 | +## Compliance & Best Practices |
| 266 | + |
| 267 | +### Security Standards Met |
| 268 | +- ✅ No raw HTML rendering in markdown (react-markdown default config) |
| 269 | +- ✅ File upload restricted by `accept` attribute |
| 270 | +- ✅ API client uses `withCredentials` for cookie-based auth |
| 271 | +- ✅ No secrets or credentials in frontend code |
| 272 | +- ✅ Zero npm audit vulnerabilities |
| 273 | +- ✅ User input sent to API via POST body (not URL path), except workspace_path which is from localStorage |
| 274 | + |
| 275 | +### Enterprise Best Practices |
| 276 | +- ✅ TypeScript strict types for all API responses |
| 277 | +- ✅ Consistent error handling pattern across components |
| 278 | +- ✅ 64 tests with 93-100% coverage on tested components |
| 279 | +- ⚠️ Two orchestrator components (DiscoveryPanel, PRDView) at 0% coverage |
| 280 | + |
| 281 | +--- |
| 282 | + |
| 283 | +## Action Items Summary |
| 284 | + |
| 285 | +### Immediate (Before Merge - Recommended) |
| 286 | +1. Add `catch` blocks to `handleSavePrd` and `handleGenerateTasks` in `page.tsx` |
| 287 | +2. Replace `useState()` with `useEffect()` for auto-start in `DiscoveryPanel.tsx` |
| 288 | + |
| 289 | +### Short-term (Next Sprint) |
| 290 | +1. Add toast/notification system for error feedback |
| 291 | +2. Add tests for DiscoveryPanel and PRDView orchestrator components |
| 292 | +3. Add file size validation to UploadPRDModal |
| 293 | + |
| 294 | +### Long-term (Backlog) |
| 295 | +1. Workspace state context (replace localStorage reads per-component) |
| 296 | +2. `encodeURIComponent` for path-interpolated IDs in API client |
| 297 | +3. Remove unused `genResp` variable |
| 298 | + |
| 299 | +--- |
| 300 | + |
| 301 | +## Conclusion |
| 302 | + |
| 303 | +This is a well-executed PR that delivers a complete PRD View with strong component architecture, comprehensive tests for leaf components, and proper security defaults. The two major issues (missing error handling and misused `useState`) are straightforward fixes that don't require architectural changes. The codebase follows established patterns consistently across all 20 source files. |
| 304 | + |
| 305 | +**Recommendation:** Fix the 2 major issues, then merge. Short-term items can be addressed in follow-up. |
| 306 | + |
| 307 | +--- |
| 308 | + |
| 309 | +## Appendix |
| 310 | + |
| 311 | +### Tools Used for Review |
| 312 | +- Manual code review of all 34 changed files |
| 313 | +- `npm audit --production` — 0 vulnerabilities |
| 314 | +- `npx tsc --noEmit` — 0 new type errors |
| 315 | +- `npx jest` — 152/152 tests passing |
| 316 | +- react-markdown version check (v10.1.0 — safe defaults) |
| 317 | + |
| 318 | +### References |
| 319 | +- OWASP Top 10 Web Application Security (A03, A06, A07) |
| 320 | +- React Rules of Hooks documentation |
| 321 | +- react-markdown security model (micromark parser, no raw HTML by default) |
| 322 | + |
| 323 | +### Metrics |
| 324 | +- **Lines of Code Reviewed:** ~2,400 (source), ~880 (tests) |
| 325 | +- **Components Reviewed:** 10 new components, 2 hooks, 1 API client extension |
| 326 | +- **Security Patterns Checked:** 6 (XSS, injection, file upload, auth headers, dependency audit, resource cleanup) |
0 commit comments